Clarify hart_mask_base requirements

[jones-drew]

hart_mask_base doesn't need to be valid unless bit0 of hart_mask is set, since the base isn't used independently. Clarify this to ensure implementations don't return SBI_ERR_INVALID_PARAM for invalid hart_mask_base when hart_mask isn't selecting it.

[avpatel]

Overall looks good to me but there is spec build failure.

Add separate first commit in this PR to fix this build issue. Here's how we fixed it for BRS spec (commit)

[SiFiveHolland]

hart_mask_base doesn't need to be valid unless bit0 of hart_mask is set, since the base isn't used independently.

Where does this requirement come from? Why is bit 0 special? If I want to select hart 2, why can't I set hart_mask_base to 0 and set bit 2?

This isn't how existing software works, for example: https://github.com/riscv-software-src/opensbi/blob/fe11dee7eaeef71c4e03f8244f1b3d16b8cac460/lib/sbi/sbi_ipi.c#L124.

[SiFiveHolland]

This change would break FreeBSD: https://github.com/freebsd/freebsd-src/blob/11db70b6057e41b259dc2245cd893d5b19179fcc/sys/riscv/riscv/sbi.c#L209

[radimkrcmar]

If I want to select hart 2, why can't I set hart_mask_base to 0 and set bit 2?

It's the other way around. The spec should now say that you can, while you couldn't before. SBI now returns an error only if hart_mask bit 0 is set when the hart with id of hart_mask_base is invalid.

[SiFiveHolland]

It's the other way around. The spec should now say that you can, while you couldn't before. SBI now returns an error only if hart_mask bit 0 is set when the hart with id of hart_mask_base is invalid.

Ah, I see now how you're interpreting it. I never thought of hart_mask_base as being itself a hartid, only as helping to define the mask. So I read the change as "hart_mask_base does not need to be an enabled or supervisor available
hartid [because the entire hart mask is ignored] unless the zeroth bit of hart_mask is set."

With a fresh look, the change looks good to me.

[radimkrcmar]

Maybe we could move the note to the changelog (or just remove it). You're right that no-one should think that the note is needed... it only made sense to people who knew how the spec was before.

[jones-drew]

Maybe we could move the note to the changelog (or just remove it). You're right that no-one should think that the note is needed... it only made sense to people who knew how the spec was before.

But above the note we have:

• unsigned long hart_mask is a scalar bit-vector containing hartids
• unsigned long hart_mask_base is the starting hartid from which the
  bit-vector must be computed.

We could try to reword hart_mask_base's definition, but I feel like it would get convoluted, and the current "starting hartid" may imply a valid hartid to some readers, simply because references to hartids aren't generally to potentiality invalid hartids.

[atishp04]

I also had the same interpretation as @SiFiveHolland at first read. I think the commit text is bit confusing. The note in the actual commit makes more sense.

hart_mask_base does not need to be an enabled or supervisor available hartid unless the zeroth bit of hart_mask is set.