Re: [PATCH v6 1/7] xen/riscv: imsic_init() implementation

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2165 bytes --]


On 7/8/25 3:52 PM, Jan Beulich wrote:
> On 07.07.2025 11:01, Oleksii Kurochko wrote:
>> imsic_init() is introduced to parse device tree node, which has the following
>> bindings [2], and based on the parsed information update IMSIC configuration
>> which is stored in imsic_cfg.
>>
>> The following helpers are introduces for imsic_init() usage:
>>    - imsic_parse_node() parses IMSIC node from DTS
>>    - imsic_get_parent_hartid() returns the hart ( CPU ) ID of the given device
>>      tree node.
>>
>> This patch is based on the code from [1].
>>
>> Since Microchip originally developed imsic.{c,h}, an internal discussion with
>> them led to the decision to use the MIT license.
>>
>> [1]https://gitlab.com/xen-project/people/olkur/xen/-/commit/0b1a94f2bc3bb1a81cd26bb75f0bf578f84cb4d4
>> [2]https://elixir.bootlin.com/linux/v6.12/source/Documentation/devicetree/bindings/interrupt-controller/riscv,imsics.yaml
>>
>> Co-developed-by: Romain Caritey<Romain.Caritey@microchip.com>
>> Signed-off-by: Oleksii Kurochko<oleksii.kurochko@gmail.com>
> Acked-by: Jan Beulich <jbeulich@suse.com
>
> I'm curious though:
>
>> --- a/xen/arch/riscv/include/asm/smp.h
>> +++ b/xen/arch/riscv/include/asm/smp.h
>> @@ -3,6 +3,7 @@
>>   #define ASM__RISCV__SMP_H
>>   
>>   #include <xen/cpumask.h>
>> +#include <xen/macros.h>
>>   #include <xen/percpu.h>
>>   
>>   #include <asm/current.h>
>> @@ -18,6 +19,18 @@ static inline unsigned long cpuid_to_hartid(unsigned long cpuid)
>>       return pcpu_info[cpuid].hart_id;
>>   }
>>   
>> +static inline unsigned int hartid_to_cpuid(unsigned long hartid)
>> +{
>> +    for ( unsigned int cpu = 0; cpu < ARRAY_SIZE(pcpu_info); cpu++ )
>> +    {
>> +        if ( hartid == cpuid_to_hartid(cpu) )
>> +            return cpu;
>> +    }
>> +
>> +    /* hartid isn't valid for some reason */
>> +    return NR_CPUS;
>> +}
> Since there's no FIXME or alike here, is this really intended to remain this
> way? With many CPUs this form of lookup can be pretty inefficient.

In the case with a big amount of CPUs it will require to update it. I will create
TODO task in my repo to not forget to update it in a future.

Thanks.

~ Oleksii

[-- Attachment #2: Type: text/html, Size: 3386 bytes --]