Re: [PATCH v5 4/7] xen/riscv: introduce functionality to work with CPU info

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.08.2024 12:56, oleksii.kurochko@xxxxxxxxx wrote:
> On Tue, 2024-08-27 at 15:44 +0200, Jan Beulich wrote:
>> On 21.08.2024 18:06, Oleksii Kurochko wrote:
>>> --- a/xen/arch/riscv/include/asm/smp.h
>>> +++ b/xen/arch/riscv/include/asm/smp.h
>>> @@ -5,6 +5,10 @@
>>>  #include <xen/cpumask.h>
>>>  #include <xen/percpu.h>
>>>  
>>> +#include <asm/processor.h>
>>> +
>>> +#define INVALID_HARTID ULONG_MAX
>>
>> So what if the firmware report this value for one of the harts?
> It could be an issue, but in my opinion, there is a small chance that
> the firmware will use such a high number. I can add a BUG_ON() in
> start_xen() to check that bootcpu_id is not equal to INVALID_HARTID to
> ensure that the firmware does not report this value. Otherwise, we
> would need to add a 'bool valid;' to struct pcpu_info and use it
> instead of INVALID_HARTID.

Which route to go largely depends on expectations to actual hardware
we're intending Xen to be usable on.

>>> --- a/xen/arch/riscv/setup.c
>>> +++ b/xen/arch/riscv/setup.c
>>> @@ -8,6 +8,7 @@
>>>  #include <public/version.h>
>>>  
>>>  #include <asm/early_printk.h>
>>> +#include <asm/smp.h>
>>>  #include <asm/traps.h>
>>>  
>>>  void arch_get_xen_caps(xen_capabilities_info_t *info)
>>> @@ -40,6 +41,10 @@ void __init noreturn start_xen(unsigned long
>>> bootcpu_id,
>>>  {
>>>      remove_identity_mapping();
>>>  
>>> +    set_processor_id(0);
>>
>> This isn't really needed, is it? The pcpu_info[] initializer already
>> installs the necessary 0. Another thing would be if the initializer
>> set the field to, say, NR_CPUS.

As suggested here, ...

>>> --- /dev/null
>>> +++ b/xen/arch/riscv/smp.c
>>> @@ -0,0 +1,21 @@
>>> +#include <xen/smp.h>
>>> +
>>> +/*
>>> + * FIXME: make pcpu_info[] dynamically allocated when necessary
>>> + *        functionality will be ready
>>> + */
>>> +/* tp points to one of these per cpu */
>>> +struct pcpu_info pcpu_info[NR_CPUS] = { { 0, INVALID_HARTID } };
>>
>> As to the initializer - what about CPUs other than CPU0? Would they
>> better all have hart_id set to invalid?
> I thought about that, but I decided that if we have INVALID_HARTID as
> hart_id and the hart_id is checked in the appropriate places, then it
> doesn't really matter what the processor_id member of struct pcpu_info
> is. For clarity, it might be better to set it to an invalid value, but
> it doesn't clear which value we should choose as invalid. I assume that
> NR_CPUS is a good candidate for that?

... yes. With that you'd also avoid the need for a "valid" flag: An
entry's hart ID would be valid (no matter which value) if its
processor_id field is valid (less than NR_CPUS).

>> Also, as a pretty strong suggestion to avoid excessive churn going
>> forward: Please consider using dedicated initializers here. IOW
>> perhaps
>>
>> struct pcpu_info pcpu_info[NR_CPUS] = { [0 ... NR_CPUS - 1] = {
>>     .hart_id = INVALID_HARTID,
>> }};
>>
>> Yet as said earlier - in addition you likely want to make sure no
>> two CPUs have (part of) their struct instance in the same cache line.
>> That won't matter right now, as you have no fields you alter at
>> runtime, but I expect such fields will appear.
> Is my understanding correct that adding __cacheline_aligned will be
> sufficient:
>    struct pcpu_info {
>    ...
>    } __cacheline_aligned;

Yes, that's what we do elsewhere.

Jan