[PATCH] lib: sbi: Add sparse hartid support for mtimer/ipi

[yang.zhang]

At 2024-02-20 19:15:35, "Anup Patel" <anup at brainfault.org> wrote:
>On Thu, Feb 1, 2024 at 4:00 PM yang.zhang <gaoshanliukou at 163.com> wrote:
>>
>> If hartid is sparse, such as 0, 7, 16..., mtimer/mswi
>> can't work well.
>>
>> Test:
>> 1.Our platform support SMT, one core contains many harts
>> whoes number is configurable, so the hartids is sparse when
>> enter Opensbi。
>> 2.I just simply simulate a scenario on qemu, like below
>> "qemu-system-riscv64 -M virt,aia=aplic -m 2g -smp 8 -dtb qemu2cpu.dtb
>>  -bios fw_jump.bin ..."
>> There are total 8 cpus, but the dts only suppor 2 cpu nodes, hartid are
>> 0 and 7, so when enter opensbi, just hartid 0 and 7 are running, the others
>> are hanged, like sparse hartid.Then the issue happened.
>> Logs:
>> OpenSBI v1.4-11-g0dcc2e6
>>    ____                    _____ ____ _____
>>   / __ \                  / ____|  _ \_   _|
>>  | |  | |_ __   ___ _ __ | (___ | |_) || |
>>  | |  | | '_ \ / _ \ '_ \ \___ \|  _ < | |
>>  | |__| | |_) |  __/ | | |____) | |_) || |_
>>   \____/| .__/ \___|_| |_|_____/|____/_____|
>>         | |
>>         |_|
>>
>> init_coldboot: timer init failed (error -1000)
>
>The patch description needs to be less verbose.
>
>>
>> Signed-off-by: yang.zhang <gaoshanliukou at 163.com>
>> ---
>
>If you have additional commentary then add it here below the
>"---" line.
>
>Overall, this patch provides a half-baked solution for an
>important problem. This patch will not work for the case
>where we have multiple ACLINT instances.
>
>To improve this patch, I suggest replacing "first_hartid" in
>"struct aclint_mswi_data" and "struct aclint_mtimer_data"
>with "first_hartindex".
>
>Regards,

>Anup

Yeah， i didn't consider the case of multiple ACLINT instances, i 
would try to reproduce it in this case, thanks.

>>  lib/utils/ipi/aclint_mswi.c     | 2 +-
>>  lib/utils/timer/aclint_mtimer.c | 2 +-
>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/utils/ipi/aclint_mswi.c b/lib/utils/ipi/aclint_mswi.c
>> index 4ae6bb1..3bbab8b 100644
>> --- a/lib/utils/ipi/aclint_mswi.c
>> +++ b/lib/utils/ipi/aclint_mswi.c
>> @@ -102,7 +102,7 @@ int aclint_mswi_cold_init(struct aclint_mswi_data *mswi)
>>
>>         /* Update MSWI pointer in scratch space */
>>         for (i = 0; i < mswi->hart_count; i++) {
>> -               scratch = sbi_hartid_to_scratch(mswi->first_hartid + i);
>> +               scratch = sbi_hartindex_to_scratch(i);
>>                 /*
>>                  * We don't need to fail if scratch pointer is not available
>>                  * because we might be dealing with hartid of a HART disabled
>> diff --git a/lib/utils/timer/aclint_mtimer.c b/lib/utils/timer/aclint_mtimer.c
>> index 271e625..77d5522 100644
>> --- a/lib/utils/timer/aclint_mtimer.c
>> +++ b/lib/utils/timer/aclint_mtimer.c
>> @@ -218,7 +218,7 @@ int aclint_mtimer_cold_init(struct aclint_mtimer_data *mt,
>>
>>         /* Update MTIMER pointer in scratch space */
>>         for (i = 0; i < mt->hart_count; i++) {
>> -               scratch = sbi_hartid_to_scratch(mt->first_hartid + i);
>> +               scratch = sbi_hartindex_to_scratch(i);
>>                 /*
>>                  * We don't need to fail if scratch pointer is not available
>>                  * because we might be dealing with hartid of a HART disabled
>> --
>> 2.25.1
>>
>>
>> --
>> opensbi mailing list
>> opensbi at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/opensbi