domain: add barrier in vcpu_create()

[PATCH] domain: add barrier in vcpu_create()

Posted by Jan Beulich 2 weeks, 3 days ago

The lock-less list updating isn't safe against racing for_each_vcpu(),
unless done (by hardware) in exactly the order written.

Fixes: 3037c5a2cb82 ("arm: domain")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
The Fixes: tag is pretty arbitrary; the issue was becoming non-latent when
Arm support was added. (Strictly speaking IA-64 and PPC would have been
affected too afaict, just that now that doesn't matter anymore [or, for
PPC, not yet, seeing that its support is being re-built from scratch].)

I'm not quite happy about prev_id being plain int, but changing it to
unsigned (with suitable other adjustments) actually makes gcc15 generate
worse code on x86.

--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -464,6 +464,7 @@ struct vcpu *vcpu_create(struct domain *
             prev_id--;
         BUG_ON(prev_id < 0);
         v->next_in_list = d->vcpu[prev_id]->next_in_list;
+        smp_wmb();
         d->vcpu[prev_id]->next_in_list = v;
     }

Re: [PATCH] domain: add barrier in vcpu_create()

Posted by Andrew Cooper 2 weeks, 3 days ago

On 26/11/2025 10:13 am, Jan Beulich wrote:
> The lock-less list updating isn't safe against racing for_each_vcpu(),
> unless done (by hardware) in exactly the order written.
>
> Fixes: 3037c5a2cb82 ("arm: domain")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> The Fixes: tag is pretty arbitrary; the issue was becoming non-latent when
> Arm support was added. (Strictly speaking IA-64 and PPC would have been
> affected too afaict, just that now that doesn't matter anymore [or, for
> PPC, not yet, seeing that its support is being re-built from scratch].)
>
> I'm not quite happy about prev_id being plain int, but changing it to
> unsigned (with suitable other adjustments) actually makes gcc15 generate
> worse code on x86.
>
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -464,6 +464,7 @@ struct vcpu *vcpu_create(struct domain *
>              prev_id--;
>          BUG_ON(prev_id < 0);
>          v->next_in_list = d->vcpu[prev_id]->next_in_list;
> +        smp_wmb();
>          d->vcpu[prev_id]->next_in_list = v;
>      }
>  

Where's the matching smp_rmb()?  There needs to be one for this
smp_wmb() to be correct.

It's rather rhetorical, because clearly the smp_rmb() needs to be in
for_each_vcpu() given your commit message, but we obviously don't want
to do that.

This list can only be changed once during a VM's lifecycle, and even
then it only gets appended to.  i.e. this particular piece of logic to
splice a vCPU into the middle of a single linked list can be simplified
to the second assignment, as the first is always copying NULL.

~Andrew

Re: [PATCH] domain: add barrier in vcpu_create()

Posted by Jan Beulich 2 weeks, 3 days ago

On 26.11.2025 12:09, Andrew Cooper wrote:
> On 26/11/2025 10:13 am, Jan Beulich wrote:
>> The lock-less list updating isn't safe against racing for_each_vcpu(),
>> unless done (by hardware) in exactly the order written.
>>
>> Fixes: 3037c5a2cb82 ("arm: domain")
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> The Fixes: tag is pretty arbitrary; the issue was becoming non-latent when
>> Arm support was added. (Strictly speaking IA-64 and PPC would have been
>> affected too afaict, just that now that doesn't matter anymore [or, for
>> PPC, not yet, seeing that its support is being re-built from scratch].)
>>
>> I'm not quite happy about prev_id being plain int, but changing it to
>> unsigned (with suitable other adjustments) actually makes gcc15 generate
>> worse code on x86.
>>
>> --- a/xen/common/domain.c
>> +++ b/xen/common/domain.c
>> @@ -464,6 +464,7 @@ struct vcpu *vcpu_create(struct domain *
>>              prev_id--;
>>          BUG_ON(prev_id < 0);
>>          v->next_in_list = d->vcpu[prev_id]->next_in_list;
>> +        smp_wmb();
>>          d->vcpu[prev_id]->next_in_list = v;
>>      }
>>  
> 
> Where's the matching smp_rmb()?  There needs to be one for this
> smp_wmb() to be correct.
> 
> It's rather rhetorical, because clearly the smp_rmb() needs to be in
> for_each_vcpu() given your commit message, but we obviously don't want
> to do that.

It's not rhetorical at all, I think. It's not needed there because of the
data dependency due to reading the loop iteration variable from the link
fields. I.e. there are no two reads there which would need ordering against
one another, and hence there's simply no-where to place a read barrier.

> This list can only be changed once during a VM's lifecycle, and even
> then it only gets appended to.  i.e. this particular piece of logic to
> splice a vCPU into the middle of a single linked list can be simplified
> to the second assignment, as the first is always copying NULL.

Except for the idle domain, as Jürgen also mentioned.

Jan

Re: [PATCH] domain: add barrier in vcpu_create()

Posted by Juergen Gross 2 weeks, 3 days ago

On 26.11.25 12:09, Andrew Cooper wrote:
> On 26/11/2025 10:13 am, Jan Beulich wrote:
>> The lock-less list updating isn't safe against racing for_each_vcpu(),
>> unless done (by hardware) in exactly the order written.
>>
>> Fixes: 3037c5a2cb82 ("arm: domain")
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> The Fixes: tag is pretty arbitrary; the issue was becoming non-latent when
>> Arm support was added. (Strictly speaking IA-64 and PPC would have been
>> affected too afaict, just that now that doesn't matter anymore [or, for
>> PPC, not yet, seeing that its support is being re-built from scratch].)
>>
>> I'm not quite happy about prev_id being plain int, but changing it to
>> unsigned (with suitable other adjustments) actually makes gcc15 generate
>> worse code on x86.
>>
>> --- a/xen/common/domain.c
>> +++ b/xen/common/domain.c
>> @@ -464,6 +464,7 @@ struct vcpu *vcpu_create(struct domain *
>>               prev_id--;
>>           BUG_ON(prev_id < 0);
>>           v->next_in_list = d->vcpu[prev_id]->next_in_list;
>> +        smp_wmb();
>>           d->vcpu[prev_id]->next_in_list = v;
>>       }
>>   
> 
> Where's the matching smp_rmb()?  There needs to be one for this
> smp_wmb() to be correct.
> 
> It's rather rhetorical, because clearly the smp_rmb() needs to be in
> for_each_vcpu() given your commit message, but we obviously don't want
> to do that.
> 
> This list can only be changed once during a VM's lifecycle, and even
> then it only gets appended to.  i.e. this particular piece of logic to
> splice a vCPU into the middle of a single linked list can be simplified
> to the second assignment, as the first is always copying NULL.

This is not true for cpu hotplug: the associated idle vcpu will be inserted
at the appropriate place in the list.

The question is whether we call for_each_vcpu() for the idle domain at all.


Juergen

Re: [PATCH] domain: add barrier in vcpu_create()

Posted by Juergen Gross 2 weeks, 3 days ago

On 26.11.25 11:13, Jan Beulich wrote:
> The lock-less list updating isn't safe against racing for_each_vcpu(),
> unless done (by hardware) in exactly the order written.
> 
> Fixes: 3037c5a2cb82 ("arm: domain")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> The Fixes: tag is pretty arbitrary; the issue was becoming non-latent when
> Arm support was added. (Strictly speaking IA-64 and PPC would have been
> affected too afaict, just that now that doesn't matter anymore [or, for
> PPC, not yet, seeing that its support is being re-built from scratch].)
> 
> I'm not quite happy about prev_id being plain int, but changing it to
> unsigned (with suitable other adjustments) actually makes gcc15 generate
> worse code on x86.
> 
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -464,6 +464,7 @@ struct vcpu *vcpu_create(struct domain *
>               prev_id--;
>           BUG_ON(prev_id < 0);
>           v->next_in_list = d->vcpu[prev_id]->next_in_list;
> +        smp_wmb();
>           d->vcpu[prev_id]->next_in_list = v;
>       }
>   
> 

It should be noted that this is an issue only in case for_each_vcpu() is
running against the idle domain during cpu hotplug.

All other domain get the vcpus populated form vcpu 0 upwards, so
v->next_in_list will always be NULL for a new vcpu of a "normal" domain.

That said I believe the fix is fine and should be done, but there is no
latent issue right now, as I believe cpu hotplug is supported for x86 today.


Juergen

Re: [PATCH] domain: add barrier in vcpu_create()

Posted by Jan Beulich 2 weeks, 3 days ago

On 26.11.2025 12:05, Juergen Gross wrote:
> On 26.11.25 11:13, Jan Beulich wrote:
>> The lock-less list updating isn't safe against racing for_each_vcpu(),
>> unless done (by hardware) in exactly the order written.
>>
>> Fixes: 3037c5a2cb82 ("arm: domain")
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> The Fixes: tag is pretty arbitrary; the issue was becoming non-latent when
>> Arm support was added. (Strictly speaking IA-64 and PPC would have been
>> affected too afaict, just that now that doesn't matter anymore [or, for
>> PPC, not yet, seeing that its support is being re-built from scratch].)
>>
>> I'm not quite happy about prev_id being plain int, but changing it to
>> unsigned (with suitable other adjustments) actually makes gcc15 generate
>> worse code on x86.
>>
>> --- a/xen/common/domain.c
>> +++ b/xen/common/domain.c
>> @@ -464,6 +464,7 @@ struct vcpu *vcpu_create(struct domain *
>>               prev_id--;
>>           BUG_ON(prev_id < 0);
>>           v->next_in_list = d->vcpu[prev_id]->next_in_list;
>> +        smp_wmb();
>>           d->vcpu[prev_id]->next_in_list = v;
>>       }
>>   
>>
> 
> It should be noted that this is an issue only in case for_each_vcpu() is
> running against the idle domain during cpu hotplug.
> 
> All other domain get the vcpus populated form vcpu 0 upwards, so
> v->next_in_list will always be NULL for a new vcpu of a "normal" domain.

Hmm, right. And I don't think we ever do for_each_vcpu() on the idle domain.

Jan