[PATCH 1/4] lib: sbi_scratch: Apply bounds check to platform hart_count

[Samuel Holland]

Hi Rahul,

On 2025-03-03 8:41 AM, Rahul Pathak wrote:
> On Fri, Feb 21, 2025 at 12:13 AM Samuel Holland
> <samuel.holland at sifive.com> wrote:
>>
>> The internal limit on the number of harts is SBI_HARTMASK_MAX_BITS, as
>> this value determines the size of various bitmaps and arrays (including
>> hartindex_to_hartid_table and hartindex_to_scratch_table). Clamp the
>> value provided by the platform, and drop the extra array element.
>>
>> Update the documentation to indicate that hart_index2id must be sized
>> based on hart_count, and that hart indexes must be contiguous. As of
>> commit 5e90e54a1a53 ("lib: utils:Check that hartid is valid"), there is
>> no restriction on the valid hart ID values.
>>
>> Signed-off-by: Samuel Holland <samuel.holland at sifive.com>
>> ---
>>
>>  include/sbi/sbi_platform.h | 11 +++--------
>>  lib/sbi/sbi_scratch.c      | 14 +++++++++-----
>>  2 files changed, 12 insertions(+), 13 deletions(-)
>>
>> diff --git a/include/sbi/sbi_platform.h b/include/sbi/sbi_platform.h
>> index 6d5fbc7d..5e50c997 100644
>> --- a/include/sbi/sbi_platform.h
>> +++ b/include/sbi/sbi_platform.h
>> @@ -169,7 +169,7 @@ struct sbi_platform {
>>         char name[64];
>>         /** Supported features */
>>         u64 features;
>> -       /** Total number of HARTs */
>> +       /** Total number of HARTs (at most SBI_HARTMASK_MAX_BITS) */
>>         u32 hart_count;
>>         /** Per-HART stack size for exception/interrupt handling */
>>         u32 hart_stack_size;
>> @@ -184,17 +184,12 @@ struct sbi_platform {
>>         /**
>>          * HART index to HART id table
>>          *
>> -        * For used HART index <abc>:
>> +        * If hart_index2id != NULL then the table must contain a mapping
>> +        * for each HART index 0 <= <abc> < hart_count:
>>          *     hart_index2id[<abc>] = some HART id
>> -        * For unused HART index <abc>:
>> -        *     hart_index2id[<abc>] = -1U
>>          *
>>          * If hart_index2id == NULL then we assume identity mapping
>>          *     hart_index2id[<abc>] = <abc>
>> -        *
>> -        * We have only two restrictions:
>> -        * 1. HART index < sbi_platform hart_count
>> -        * 2. HART id < SBI_HARTMASK_MAX_BITS
>>          */
>>         const u32 *hart_index2id;
>>  };
>> diff --git a/lib/sbi/sbi_scratch.c b/lib/sbi/sbi_scratch.c
>> index ccbbc68e..5a9e935b 100644
>> --- a/lib/sbi/sbi_scratch.c
>> +++ b/lib/sbi/sbi_scratch.c
>> @@ -15,8 +15,8 @@
>>  #include <sbi/sbi_string.h>
>>
>>  u32 last_hartindex_having_scratch = 0;
>> -u32 hartindex_to_hartid_table[SBI_HARTMASK_MAX_BITS + 1] = { -1U };
>> -struct sbi_scratch *hartindex_to_scratch_table[SBI_HARTMASK_MAX_BITS + 1] = { 0 };
>> +u32 hartindex_to_hartid_table[SBI_HARTMASK_MAX_BITS] = { -1U };
>> +struct sbi_scratch *hartindex_to_scratch_table[SBI_HARTMASK_MAX_BITS] = { 0 };
>>
>>  static spinlock_t extra_lock = SPIN_LOCK_INITIALIZER;
>>  static unsigned long extra_offset = SBI_SCRATCH_EXTRA_SPACE_OFFSET;
>> @@ -36,17 +36,21 @@ typedef struct sbi_scratch *(*hartid2scratch)(ulong hartid, ulong hartindex);
>>
>>  int sbi_scratch_init(struct sbi_scratch *scratch)
>>  {
>> -       u32 i, h;
>> +       u32 i, h, hart_count;
>>         const struct sbi_platform *plat = sbi_platform_ptr(scratch);
>>
>> -       for (i = 0; i < plat->hart_count; i++) {
>> +       hart_count = plat->hart_count;
>> +       if (hart_count > SBI_HARTMASK_MAX_BITS)
>> +               hart_count = SBI_HARTMASK_MAX_BITS;
>>
> From a platform perspective case where hart_count > SBI_HARTMASK_MAX_BITS
> Since its a limitation of SBI to not be able to represent all what
> platform expects. Is it a
> point of failure and it should not continue from here or it should
> silently move forward with
> what SBI can accommodate?

Only OpenSBI the implementation has this limitation, not SBI more generally.

I would expect a good quality implementation to crash or hang only as a last
resort. In this case, the system can continue running normally, with a reduced
number of harts available to lower privilege modes. The remaining harts remain
safely off or parked in M-mode. S-mode software will be aware of the issue when
sbi_hsm_hart_start() fails to start the excess harts, but it should already
expect to handle errors from that function, so it would be just another instance
of the interface working as intended.

Regards,
Samuel

>>
>> +       for (i = 0; i < hart_count; i++) {
>>                 h = (plat->hart_index2id) ? plat->hart_index2id[i] : i;
>>                 hartindex_to_hartid_table[i] = h;
>>                 hartindex_to_scratch_table[i] =
>>                         ((hartid2scratch)scratch->hartid_to_scratch)(h, i);
>>         }
>>
>> -       last_hartindex_having_scratch = plat->hart_count - 1;
>> +       last_hartindex_having_scratch = hart_count - 1;
>>
>>         return 0;
>>  }
>> --
>> 2.47.2
>>
>>
>> --
>> opensbi mailing list
>> opensbi at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/opensbi
>