Re: [PATCH v2 28/39] xen/riscv: handle the case when no vCPU migration is needed

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.09.2026 07:12, Oleksii Kurochko wrote:
> 
> 
> On 9/16/26 3:02 PM, Jan Beulich wrote:
>> On 16.09.2026 07:55, Oleksii Kurochko wrote:
>>> On 9/14/26 2:12 PM, Jan Beulich wrote:
>>>> On 27.08.2026 17:21, Oleksii Kurochko wrote:
>>>>> --- a/xen/arch/riscv/imsic.c
>>>>> +++ b/xen/arch/riscv/imsic.c
>>>>> @@ -689,5 +689,15 @@ int __init vimsic_make_domu_dt_node(struct 
>>>>> kernel_info *kinfo,
>>>>>    
>>>>>    void imsic_migrate_vcpu(struct vcpu *v)
>>>>>    {
>>>>> +    /*
>>>>> +     * The scheduler can mark a freshly created vCPU's unit as migrated 
>>>>> and
>>>>> +     * invoke this before the vCPU has ever run (see the migrated branch 
>>>>> in
>>>>> +     * schedule()). No need to do migration for such vCPUs as they 
>>>>> aren't fully
>>>>> +     * initialized (for example, context_switch() will be called after
>>>>> +     * imsic_migrate_vcpu()).
>>>>> +     */
>>>>> +    if ( v->arch.last_cpu == NR_CPUS )
>>>>
>>>> May I suggest to use >= ? I'm still somewhat unconvinced of NR_CPUS being a
>>>> good sentinel. If we/you decided to switch to ~0, >= here would continue to
>>>> be correct.
>>>
>>> I agree with >= but I am not quite sure that I fully understand what is
>>> wrong with NR_CPUS. We have for example the following:
>>>
>>> static inline unsigned int smp_processor_id(void)
>>> {
>>>       unsigned int id = tp->processor_id;
>>>
>>>       BUG_ON(id >= NR_CPUS);
>>>
>>>       return id;
>>> }
>>>
>>> So it is guaranteed that NR_CPUS what be used as cpu id and so it still
>>> could be considered as a good sentinel.
>>
>> Arbitrary numbers can be problematic when used as a sentinel. If you look
>> at disassembly, you may not recognize that number as a sentinel. Further
>> there's also a code-gen concern: ~0, aiui, will always generate the same
>> code (to e.g. load into a register). NR_CPUS, depending on .config, may
>> not. The value may not be loadable by a single insn.
> 
> Witch such explanation it started to be more sense in it.
> 
> I will introduce then
> 
> /* Value of arch_vcpu.last_cpu for a vCPU which hasn't run yet. */
> #define VCPU_NEVER_RAN (~0U)
> 
> and use it to work with v->arch.last_cpu.
> 
> Just to be sure that I understand correctly your suggestion with ~0U is 
> only for the case of ->last_cpu and check if vcpu was ran or not.
> 
> For
> 
> struct pcpu_info pcpu_info[NR_CPUS] = { [0 ... NR_CPUS - 1] = {
>      .processor_id = NR_CPUS,
> }};
> 
> and
> 
> struct vimsic_state {
> ...
>      /*
>       * s/w IMSIC VS-file -> vsfile_cpu == NR_CPUS
>       * h/w IMSIC VS-file -> vsfile_cpu < NR_CPUS
>       */
>      unsigned int vsfile_cpu;
> };
> 
> I can continue to use NR_CPUS, right?

You _can_ everywhere. It may merely be beneficial to use ~0 instead, at
least in some cases. The "how to load value into a register" aspect of
course doesn't affect static initializers. The "easy to recognize" one,
otoh, may apply there as well. You get to judge...

Jan