[PATCH] x86/vpmu: fix race-condition in vpmu_load

[PATCH] x86/vpmu: fix race-condition in vpmu_load

[Tamas K Lengyel]

While experimenting with the vPMU subsystem an ASSERT failure was
observed in vmx_find_msr because the vcpu_runnable state was true.

The root cause of the bug appears to be the fact that the vPMU subsystem
doesn't save its state on context_switch. The vpmu_load function will attempt
to gather the PMU state if its still loaded two different ways:
    1. if the current pcpu is not where the vcpu ran before doing a remote save
    2. if the current pcpu had another vcpu active before doing a local save

However, in case the prev vcpu is being rescheduled on another pcpu its state
has already changed and vcpu_runnable is returning true, thus #2 will trip the
ASSERT. The only way to avoid this race condition is to make sure the
prev vcpu is paused while being checked and its context saved. Once the prev
vcpu is resumed and does #1 it will find its state already saved.

Signed-off-by: Tamas K Lengyel <tamas.lengyel@intel.com>
---
 xen/arch/x86/cpu/vpmu.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/arch/x86/cpu/vpmu.c b/xen/arch/x86/cpu/vpmu.c
index cacc24a30f..076c2e5a8d 100644
--- a/xen/arch/x86/cpu/vpmu.c
+++ b/xen/arch/x86/cpu/vpmu.c
@@ -419,8 +419,10 @@ int vpmu_load(struct vcpu *v, bool_t from_guest)
         vpmu = vcpu_vpmu(prev);
 
         /* Someone ran here before us */
+        vcpu_pause(prev);
         vpmu_save_force(prev);
         vpmu_reset(vpmu, VPMU_CONTEXT_LOADED);
+        vcpu_unpause(prev);
 
         vpmu = vcpu_vpmu(v);
     }
-- 
2.34.1

Re: [PATCH] x86/vpmu: fix race-condition in vpmu_load

[Jan Beulich]

On 15.09.2022 16:01, Tamas K Lengyel wrote:
> While experimenting with the vPMU subsystem an ASSERT failure was
> observed in vmx_find_msr because the vcpu_runnable state was true.
> 
> The root cause of the bug appears to be the fact that the vPMU subsystem
> doesn't save its state on context_switch. The vpmu_load function will attempt
> to gather the PMU state if its still loaded two different ways:
>     1. if the current pcpu is not where the vcpu ran before doing a remote save
>     2. if the current pcpu had another vcpu active before doing a local save
> 
> However, in case the prev vcpu is being rescheduled on another pcpu its state
> has already changed and vcpu_runnable is returning true, thus #2 will trip the
> ASSERT. The only way to avoid this race condition is to make sure the
> prev vcpu is paused while being checked and its context saved. Once the prev
> vcpu is resumed and does #1 it will find its state already saved.

While I consider this explanation plausible, I'm worried:

> --- a/xen/arch/x86/cpu/vpmu.c
> +++ b/xen/arch/x86/cpu/vpmu.c
> @@ -419,8 +419,10 @@ int vpmu_load(struct vcpu *v, bool_t from_guest)
>          vpmu = vcpu_vpmu(prev);
>  
>          /* Someone ran here before us */
> +        vcpu_pause(prev);
>          vpmu_save_force(prev);
>          vpmu_reset(vpmu, VPMU_CONTEXT_LOADED);
> +        vcpu_unpause(prev);
>  
>          vpmu = vcpu_vpmu(v);
>      }

We're running with IRQs off here, yet vcpu_pause() waits for the vcpu
to actually be de-scheduled. Even with IRQs on this is already a
relatively heavy operation (also including its impact on the remote
side). Additionally the function is called from context_switch(), and
I'm unsure of the usability of vcpu_pause() on such a path. In
particular: Is there a risk of two CPUs doing this mutually to one
another? If so, is deadlocking excluded?

Hence at the very least I think the description wants extending, to
discuss the safety of the change.

Boris - any chance you could comment here? Iirc that's code you did
introduce.

Jan

Re: [PATCH] x86/vpmu: fix race-condition in vpmu_load

[Boris Ostrovsky]

On 9/16/22 8:52 AM, Jan Beulich wrote:
> On 15.09.2022 16:01, Tamas K Lengyel wrote:
>> While experimenting with the vPMU subsystem an ASSERT failure was
>> observed in vmx_find_msr because the vcpu_runnable state was true.
>>
>> The root cause of the bug appears to be the fact that the vPMU subsystem
>> doesn't save its state on context_switch. The vpmu_load function will attempt
>> to gather the PMU state if its still loaded two different ways:
>>      1. if the current pcpu is not where the vcpu ran before doing a remote save
>>      2. if the current pcpu had another vcpu active before doing a local save
>>
>> However, in case the prev vcpu is being rescheduled on another pcpu its state
>> has already changed and vcpu_runnable is returning true, thus #2 will trip the
>> ASSERT. The only way to avoid this race condition is to make sure the
>> prev vcpu is paused while being checked and its context saved. Once the prev
>> vcpu is resumed and does #1 it will find its state already saved.
> While I consider this explanation plausible, I'm worried:
>
>> --- a/xen/arch/x86/cpu/vpmu.c
>> +++ b/xen/arch/x86/cpu/vpmu.c
>> @@ -419,8 +419,10 @@ int vpmu_load(struct vcpu *v, bool_t from_guest)
>>           vpmu = vcpu_vpmu(prev);
>>   
>>           /* Someone ran here before us */
>> +        vcpu_pause(prev);
>>           vpmu_save_force(prev);
>>           vpmu_reset(vpmu, VPMU_CONTEXT_LOADED);
>> +        vcpu_unpause(prev);
>>   
>>           vpmu = vcpu_vpmu(v);
>>       }
> We're running with IRQs off here, yet vcpu_pause() waits for the vcpu
> to actually be de-scheduled. Even with IRQs on this is already a
> relatively heavy operation (also including its impact on the remote
> side). Additionally the function is called from context_switch(), and
> I'm unsure of the usability of vcpu_pause() on such a path. In
> particular: Is there a risk of two CPUs doing this mutually to one
> another? If so, is deadlocking excluded?
>
> Hence at the very least I think the description wants extending, to
> discuss the safety of the change.
>
> Boris - any chance you could comment here? Iirc that's code you did
> introduce.


Is the assertion in vmx_find_msr() really needs to be for runnable vcpu or can it be a check on whether vcpu is actually running (e.g. RUNSTATE_running)? I think call chain vpmu_load()->..->*_vpmu_save()->...->vmx_find_msr() is the only one where we are doing it for non-current vcpu. If we can guarantee that run state is set after vpmu_load() call (maybe it is already, I haven't checked) then testing the state may avoid the assertion.


-boris

Re: [PATCH] x86/vpmu: fix race-condition in vpmu_load

[Jan Beulich]

On 16.09.2022 23:35, Boris Ostrovsky wrote:
> 
> On 9/16/22 8:52 AM, Jan Beulich wrote:
>> On 15.09.2022 16:01, Tamas K Lengyel wrote:
>>> While experimenting with the vPMU subsystem an ASSERT failure was
>>> observed in vmx_find_msr because the vcpu_runnable state was true.
>>>
>>> The root cause of the bug appears to be the fact that the vPMU subsystem
>>> doesn't save its state on context_switch. The vpmu_load function will attempt
>>> to gather the PMU state if its still loaded two different ways:
>>>      1. if the current pcpu is not where the vcpu ran before doing a remote save
>>>      2. if the current pcpu had another vcpu active before doing a local save
>>>
>>> However, in case the prev vcpu is being rescheduled on another pcpu its state
>>> has already changed and vcpu_runnable is returning true, thus #2 will trip the
>>> ASSERT. The only way to avoid this race condition is to make sure the
>>> prev vcpu is paused while being checked and its context saved. Once the prev
>>> vcpu is resumed and does #1 it will find its state already saved.
>> While I consider this explanation plausible, I'm worried:
>>
>>> --- a/xen/arch/x86/cpu/vpmu.c
>>> +++ b/xen/arch/x86/cpu/vpmu.c
>>> @@ -419,8 +419,10 @@ int vpmu_load(struct vcpu *v, bool_t from_guest)
>>>           vpmu = vcpu_vpmu(prev);
>>>   
>>>           /* Someone ran here before us */
>>> +        vcpu_pause(prev);
>>>           vpmu_save_force(prev);
>>>           vpmu_reset(vpmu, VPMU_CONTEXT_LOADED);
>>> +        vcpu_unpause(prev);
>>>   
>>>           vpmu = vcpu_vpmu(v);
>>>       }
>> We're running with IRQs off here, yet vcpu_pause() waits for the vcpu
>> to actually be de-scheduled. Even with IRQs on this is already a
>> relatively heavy operation (also including its impact on the remote
>> side). Additionally the function is called from context_switch(), and
>> I'm unsure of the usability of vcpu_pause() on such a path. In
>> particular: Is there a risk of two CPUs doing this mutually to one
>> another? If so, is deadlocking excluded?
>>
>> Hence at the very least I think the description wants extending, to
>> discuss the safety of the change.
>>
>> Boris - any chance you could comment here? Iirc that's code you did
>> introduce.
> 
> 
> Is the assertion in vmx_find_msr() really needs to be for runnable vcpu or can it be a check on whether vcpu is actually running (e.g. RUNSTATE_running)?

You cannot safely check for "running", as "runnable" may transition
to/from "running" behind your back.

Jan

> I think call chain vpmu_load()->..->*_vpmu_save()->...->vmx_find_msr() is the only one where we are doing it for non-current vcpu. If we can guarantee that run state is set after vpmu_load() call (maybe it is already, I haven't checked) then testing the state may avoid the assertion.
> 
> 
> -boris
> 

Re: [PATCH] x86/vpmu: fix race-condition in vpmu_load

[Tamas K Lengyel]

On Mon, Sep 19, 2022 at 5:28 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 16.09.2022 23:35, Boris Ostrovsky wrote:
> >
> > On 9/16/22 8:52 AM, Jan Beulich wrote:
> >> On 15.09.2022 16:01, Tamas K Lengyel wrote:
> >>> While experimenting with the vPMU subsystem an ASSERT failure was
> >>> observed in vmx_find_msr because the vcpu_runnable state was true.
> >>>
> >>> The root cause of the bug appears to be the fact that the vPMU subsystem
> >>> doesn't save its state on context_switch. The vpmu_load function will attempt
> >>> to gather the PMU state if its still loaded two different ways:
> >>>      1. if the current pcpu is not where the vcpu ran before doing a remote save
> >>>      2. if the current pcpu had another vcpu active before doing a local save
> >>>
> >>> However, in case the prev vcpu is being rescheduled on another pcpu its state
> >>> has already changed and vcpu_runnable is returning true, thus #2 will trip the
> >>> ASSERT. The only way to avoid this race condition is to make sure the
> >>> prev vcpu is paused while being checked and its context saved. Once the prev
> >>> vcpu is resumed and does #1 it will find its state already saved.
> >> While I consider this explanation plausible, I'm worried:
> >>
> >>> --- a/xen/arch/x86/cpu/vpmu.c
> >>> +++ b/xen/arch/x86/cpu/vpmu.c
> >>> @@ -419,8 +419,10 @@ int vpmu_load(struct vcpu *v, bool_t from_guest)
> >>>           vpmu = vcpu_vpmu(prev);
> >>>
> >>>           /* Someone ran here before us */
> >>> +        vcpu_pause(prev);
> >>>           vpmu_save_force(prev);
> >>>           vpmu_reset(vpmu, VPMU_CONTEXT_LOADED);
> >>> +        vcpu_unpause(prev);
> >>>
> >>>           vpmu = vcpu_vpmu(v);
> >>>       }
> >> We're running with IRQs off here, yet vcpu_pause() waits for the vcpu
> >> to actually be de-scheduled. Even with IRQs on this is already a
> >> relatively heavy operation (also including its impact on the remote
> >> side). Additionally the function is called from context_switch(), and
> >> I'm unsure of the usability of vcpu_pause() on such a path. In
> >> particular: Is there a risk of two CPUs doing this mutually to one
> >> another? If so, is deadlocking excluded?
> >>
> >> Hence at the very least I think the description wants extending, to
> >> discuss the safety of the change.
> >>
> >> Boris - any chance you could comment here? Iirc that's code you did
> >> introduce.
> >
> >
> > Is the assertion in vmx_find_msr() really needs to be for runnable vcpu or can it be a check on whether vcpu is actually running (e.g. RUNSTATE_running)?
>
> You cannot safely check for "running", as "runnable" may transition
> to/from "running" behind your back.

The more I look at this the more I think the only sensible solution is
to have the vPMU state be saved on vmexit for all vCPUs. That way all
this having to figure out where and when a context needs saving during
scheduling goes away. Yes, it adds a bit of overhead for cases where
the vCPU will resume on the same pCPU and that context saved could
have been skipped, but it makes it so that the vCPU can be resumed on
any pCPU without having to have evidently fragile checks that may
potentially lead to deadlocks (TBH I don't know if that's a real
concern at the moment because the current setup is very hard to reason
about). We can still keep track if the context needs reloading from
the saved context and skip that if we know the state is still active.
Any objection to that change in light of these issues?

Tamas

Re: [PATCH] x86/vpmu: fix race-condition in vpmu_load

[Jan Beulich]

On 19.09.2022 14:25, Tamas K Lengyel wrote:
> On Mon, Sep 19, 2022 at 5:28 AM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 16.09.2022 23:35, Boris Ostrovsky wrote:
>>>
>>> On 9/16/22 8:52 AM, Jan Beulich wrote:
>>>> On 15.09.2022 16:01, Tamas K Lengyel wrote:
>>>>> While experimenting with the vPMU subsystem an ASSERT failure was
>>>>> observed in vmx_find_msr because the vcpu_runnable state was true.
>>>>>
>>>>> The root cause of the bug appears to be the fact that the vPMU subsystem
>>>>> doesn't save its state on context_switch.

For the further reply below - is this actually true? What is the
vpmu_switch_from() (resolving to vpmu_save()) doing then early
in context_switch()?

>>>>> The vpmu_load function will attempt
>>>>> to gather the PMU state if its still loaded two different ways:
>>>>>      1. if the current pcpu is not where the vcpu ran before doing a remote save
>>>>>      2. if the current pcpu had another vcpu active before doing a local save
>>>>>
>>>>> However, in case the prev vcpu is being rescheduled on another pcpu its state
>>>>> has already changed and vcpu_runnable is returning true, thus #2 will trip the
>>>>> ASSERT. The only way to avoid this race condition is to make sure the
>>>>> prev vcpu is paused while being checked and its context saved. Once the prev
>>>>> vcpu is resumed and does #1 it will find its state already saved.
>>>> While I consider this explanation plausible, I'm worried:
>>>>
>>>>> --- a/xen/arch/x86/cpu/vpmu.c
>>>>> +++ b/xen/arch/x86/cpu/vpmu.c
>>>>> @@ -419,8 +419,10 @@ int vpmu_load(struct vcpu *v, bool_t from_guest)
>>>>>           vpmu = vcpu_vpmu(prev);
>>>>>
>>>>>           /* Someone ran here before us */
>>>>> +        vcpu_pause(prev);
>>>>>           vpmu_save_force(prev);
>>>>>           vpmu_reset(vpmu, VPMU_CONTEXT_LOADED);
>>>>> +        vcpu_unpause(prev);
>>>>>
>>>>>           vpmu = vcpu_vpmu(v);
>>>>>       }
>>>> We're running with IRQs off here, yet vcpu_pause() waits for the vcpu
>>>> to actually be de-scheduled. Even with IRQs on this is already a
>>>> relatively heavy operation (also including its impact on the remote
>>>> side). Additionally the function is called from context_switch(), and
>>>> I'm unsure of the usability of vcpu_pause() on such a path. In
>>>> particular: Is there a risk of two CPUs doing this mutually to one
>>>> another? If so, is deadlocking excluded?
>>>>
>>>> Hence at the very least I think the description wants extending, to
>>>> discuss the safety of the change.
>>>>
>>>> Boris - any chance you could comment here? Iirc that's code you did
>>>> introduce.
>>>
>>>
>>> Is the assertion in vmx_find_msr() really needs to be for runnable vcpu or can it be a check on whether vcpu is actually running (e.g. RUNSTATE_running)?
>>
>> You cannot safely check for "running", as "runnable" may transition
>> to/from "running" behind your back.
> 
> The more I look at this the more I think the only sensible solution is
> to have the vPMU state be saved on vmexit for all vCPUs.

Do you really mean vmexit? It would suffice if state was reliably
saved during context-switch-out, wouldn't it? At that point the
vCPU can't be resumed on another pCPU, yet.

> That way all
> this having to figure out where and when a context needs saving during
> scheduling goes away. Yes, it adds a bit of overhead for cases where
> the vCPU will resume on the same pCPU and that context saved could
> have been skipped,

If you really mean vmexit, then I'm inclined to say that's more
than just "a bit of overhead". I'd agree if you really meant
context-switch-out, but as said further up it looks to me as if
that was already occurring. Apparently I'm overlooking something
crucial ...

Jan

> but it makes it so that the vCPU can be resumed on
> any pCPU without having to have evidently fragile checks that may
> potentially lead to deadlocks (TBH I don't know if that's a real
> concern at the moment because the current setup is very hard to reason
> about). We can still keep track if the context needs reloading from
> the saved context and skip that if we know the state is still active.
> Any objection to that change in light of these issues?
> 
> Tamas

Re: [PATCH] x86/vpmu: fix race-condition in vpmu_load

[Tamas K Lengyel]

On Mon, Sep 19, 2022 at 9:21 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 19.09.2022 14:25, Tamas K Lengyel wrote:
> > On Mon, Sep 19, 2022 at 5:28 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 16.09.2022 23:35, Boris Ostrovsky wrote:
> >>>
> >>> On 9/16/22 8:52 AM, Jan Beulich wrote:
> >>>> On 15.09.2022 16:01, Tamas K Lengyel wrote:
> >>>>> While experimenting with the vPMU subsystem an ASSERT failure was
> >>>>> observed in vmx_find_msr because the vcpu_runnable state was true.
> >>>>>
> >>>>> The root cause of the bug appears to be the fact that the vPMU subsystem
> >>>>> doesn't save its state on context_switch.
>
> For the further reply below - is this actually true? What is the
> vpmu_switch_from() (resolving to vpmu_save()) doing then early
> in context_switch()?
>
> >>>>> The vpmu_load function will attempt
> >>>>> to gather the PMU state if its still loaded two different ways:
> >>>>>      1. if the current pcpu is not where the vcpu ran before doing a remote save
> >>>>>      2. if the current pcpu had another vcpu active before doing a local save
> >>>>>
> >>>>> However, in case the prev vcpu is being rescheduled on another pcpu its state
> >>>>> has already changed and vcpu_runnable is returning true, thus #2 will trip the
> >>>>> ASSERT. The only way to avoid this race condition is to make sure the
> >>>>> prev vcpu is paused while being checked and its context saved. Once the prev
> >>>>> vcpu is resumed and does #1 it will find its state already saved.
> >>>> While I consider this explanation plausible, I'm worried:
> >>>>
> >>>>> --- a/xen/arch/x86/cpu/vpmu.c
> >>>>> +++ b/xen/arch/x86/cpu/vpmu.c
> >>>>> @@ -419,8 +419,10 @@ int vpmu_load(struct vcpu *v, bool_t from_guest)
> >>>>>           vpmu = vcpu_vpmu(prev);
> >>>>>
> >>>>>           /* Someone ran here before us */
> >>>>> +        vcpu_pause(prev);
> >>>>>           vpmu_save_force(prev);
> >>>>>           vpmu_reset(vpmu, VPMU_CONTEXT_LOADED);
> >>>>> +        vcpu_unpause(prev);
> >>>>>
> >>>>>           vpmu = vcpu_vpmu(v);
> >>>>>       }
> >>>> We're running with IRQs off here, yet vcpu_pause() waits for the vcpu
> >>>> to actually be de-scheduled. Even with IRQs on this is already a
> >>>> relatively heavy operation (also including its impact on the remote
> >>>> side). Additionally the function is called from context_switch(), and
> >>>> I'm unsure of the usability of vcpu_pause() on such a path. In
> >>>> particular: Is there a risk of two CPUs doing this mutually to one
> >>>> another? If so, is deadlocking excluded?
> >>>>
> >>>> Hence at the very least I think the description wants extending, to
> >>>> discuss the safety of the change.
> >>>>
> >>>> Boris - any chance you could comment here? Iirc that's code you did
> >>>> introduce.
> >>>
> >>>
> >>> Is the assertion in vmx_find_msr() really needs to be for runnable vcpu or can it be a check on whether vcpu is actually running (e.g. RUNSTATE_running)?
> >>
> >> You cannot safely check for "running", as "runnable" may transition
> >> to/from "running" behind your back.
> >
> > The more I look at this the more I think the only sensible solution is
> > to have the vPMU state be saved on vmexit for all vCPUs.
>
> Do you really mean vmexit? It would suffice if state was reliably
> saved during context-switch-out, wouldn't it? At that point the
> vCPU can't be resumed on another pCPU, yet.
>
> > That way all
> > this having to figure out where and when a context needs saving during
> > scheduling goes away. Yes, it adds a bit of overhead for cases where
> > the vCPU will resume on the same pCPU and that context saved could
> > have been skipped,
>
> If you really mean vmexit, then I'm inclined to say that's more
> than just "a bit of overhead". I'd agree if you really meant
> context-switch-out, but as said further up it looks to me as if
> that was already occurring. Apparently I'm overlooking something
> crucial ...

Yes, the current setup is doing exactly that, saving the vPMU context
on context-switch-out, and that's where the ASSERT failure occurs
because the vCPU it needs to save the context for is already runnable
on another pCPU.

Tamas

Re: [PATCH] x86/vpmu: fix race-condition in vpmu_load

[Jan Beulich]

On 19.09.2022 15:24, Tamas K Lengyel wrote:
> On Mon, Sep 19, 2022 at 9:21 AM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 19.09.2022 14:25, Tamas K Lengyel wrote:
>>> On Mon, Sep 19, 2022 at 5:28 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>
>>>> On 16.09.2022 23:35, Boris Ostrovsky wrote:
>>>>>
>>>>> On 9/16/22 8:52 AM, Jan Beulich wrote:
>>>>>> On 15.09.2022 16:01, Tamas K Lengyel wrote:
>>>>>>> While experimenting with the vPMU subsystem an ASSERT failure was
>>>>>>> observed in vmx_find_msr because the vcpu_runnable state was true.
>>>>>>>
>>>>>>> The root cause of the bug appears to be the fact that the vPMU subsystem
>>>>>>> doesn't save its state on context_switch.
>>
>> For the further reply below - is this actually true? What is the
>> vpmu_switch_from() (resolving to vpmu_save()) doing then early
>> in context_switch()?
>>
>>>>>>> The vpmu_load function will attempt
>>>>>>> to gather the PMU state if its still loaded two different ways:
>>>>>>>      1. if the current pcpu is not where the vcpu ran before doing a remote save
>>>>>>>      2. if the current pcpu had another vcpu active before doing a local save
>>>>>>>
>>>>>>> However, in case the prev vcpu is being rescheduled on another pcpu its state
>>>>>>> has already changed and vcpu_runnable is returning true, thus #2 will trip the
>>>>>>> ASSERT. The only way to avoid this race condition is to make sure the
>>>>>>> prev vcpu is paused while being checked and its context saved. Once the prev
>>>>>>> vcpu is resumed and does #1 it will find its state already saved.
>>>>>> While I consider this explanation plausible, I'm worried:
>>>>>>
>>>>>>> --- a/xen/arch/x86/cpu/vpmu.c
>>>>>>> +++ b/xen/arch/x86/cpu/vpmu.c
>>>>>>> @@ -419,8 +419,10 @@ int vpmu_load(struct vcpu *v, bool_t from_guest)
>>>>>>>           vpmu = vcpu_vpmu(prev);
>>>>>>>
>>>>>>>           /* Someone ran here before us */
>>>>>>> +        vcpu_pause(prev);
>>>>>>>           vpmu_save_force(prev);
>>>>>>>           vpmu_reset(vpmu, VPMU_CONTEXT_LOADED);
>>>>>>> +        vcpu_unpause(prev);
>>>>>>>
>>>>>>>           vpmu = vcpu_vpmu(v);
>>>>>>>       }
>>>>>> We're running with IRQs off here, yet vcpu_pause() waits for the vcpu
>>>>>> to actually be de-scheduled. Even with IRQs on this is already a
>>>>>> relatively heavy operation (also including its impact on the remote
>>>>>> side). Additionally the function is called from context_switch(), and
>>>>>> I'm unsure of the usability of vcpu_pause() on such a path. In
>>>>>> particular: Is there a risk of two CPUs doing this mutually to one
>>>>>> another? If so, is deadlocking excluded?
>>>>>>
>>>>>> Hence at the very least I think the description wants extending, to
>>>>>> discuss the safety of the change.
>>>>>>
>>>>>> Boris - any chance you could comment here? Iirc that's code you did
>>>>>> introduce.
>>>>>
>>>>>
>>>>> Is the assertion in vmx_find_msr() really needs to be for runnable vcpu or can it be a check on whether vcpu is actually running (e.g. RUNSTATE_running)?
>>>>
>>>> You cannot safely check for "running", as "runnable" may transition
>>>> to/from "running" behind your back.
>>>
>>> The more I look at this the more I think the only sensible solution is
>>> to have the vPMU state be saved on vmexit for all vCPUs.
>>
>> Do you really mean vmexit? It would suffice if state was reliably
>> saved during context-switch-out, wouldn't it? At that point the
>> vCPU can't be resumed on another pCPU, yet.
>>
>>> That way all
>>> this having to figure out where and when a context needs saving during
>>> scheduling goes away. Yes, it adds a bit of overhead for cases where
>>> the vCPU will resume on the same pCPU and that context saved could
>>> have been skipped,
>>
>> If you really mean vmexit, then I'm inclined to say that's more
>> than just "a bit of overhead". I'd agree if you really meant
>> context-switch-out, but as said further up it looks to me as if
>> that was already occurring. Apparently I'm overlooking something
>> crucial ...
> 
> Yes, the current setup is doing exactly that, saving the vPMU context
> on context-switch-out, and that's where the ASSERT failure occurs
> because the vCPU it needs to save the context for is already runnable
> on another pCPU.

Well, if that's the scenario (sorry for not understanding it that
way earlier on), then the assertion is too strict: While in context
switch, the vCPU may be runnable, but certainly won't actually run
anywhere. Therefore I'd be inclined to suggest to relax the
condition just enough to cover this case, without actually going to
checking for "running".

Jan

Re: [PATCH] x86/vpmu: fix race-condition in vpmu_load

[Tamas K Lengyel]

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

On Mon, Sep 19, 2022 at 9:58 AM Jan Beulich <jbeulich@suse.com> wrote:

> On 19.09.2022 15:24, Tamas K Lengyel wrote:
> > On Mon, Sep 19, 2022 at 9:21 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 19.09.2022 14:25, Tamas K Lengyel wrote:
> >>> On Mon, Sep 19, 2022 at 5:28 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>>>
> >>>> On 16.09.2022 23:35, Boris Ostrovsky wrote:
> >>>>>
> >>>>> On 9/16/22 8:52 AM, Jan Beulich wrote:
> >>>>>> On 15.09.2022 16:01, Tamas K Lengyel wrote:
> >>>>>>> While experimenting with the vPMU subsystem an ASSERT failure was
> >>>>>>> observed in vmx_find_msr because the vcpu_runnable state was true.
> >>>>>>>
> >>>>>>> The root cause of the bug appears to be the fact that the vPMU
> subsystem
> >>>>>>> doesn't save its state on context_switch.
> >>
> >> For the further reply below - is this actually true? What is the
> >> vpmu_switch_from() (resolving to vpmu_save()) doing then early
> >> in context_switch()?
> >>
> >>>>>>> The vpmu_load function will attempt
> >>>>>>> to gather the PMU state if its still loaded two different ways:
> >>>>>>>      1. if the current pcpu is not where the vcpu ran before doing
> a remote save
> >>>>>>>      2. if the current pcpu had another vcpu active before doing a
> local save
> >>>>>>>
> >>>>>>> However, in case the prev vcpu is being rescheduled on another
> pcpu its state
> >>>>>>> has already changed and vcpu_runnable is returning true, thus #2
> will trip the
> >>>>>>> ASSERT. The only way to avoid this race condition is to make sure
> the
> >>>>>>> prev vcpu is paused while being checked and its context saved.
> Once the prev
> >>>>>>> vcpu is resumed and does #1 it will find its state already saved.
> >>>>>> While I consider this explanation plausible, I'm worried:
> >>>>>>
> >>>>>>> --- a/xen/arch/x86/cpu/vpmu.c
> >>>>>>> +++ b/xen/arch/x86/cpu/vpmu.c
> >>>>>>> @@ -419,8 +419,10 @@ int vpmu_load(struct vcpu *v, bool_t
> from_guest)
> >>>>>>>           vpmu = vcpu_vpmu(prev);
> >>>>>>>
> >>>>>>>           /* Someone ran here before us */
> >>>>>>> +        vcpu_pause(prev);
> >>>>>>>           vpmu_save_force(prev);
> >>>>>>>           vpmu_reset(vpmu, VPMU_CONTEXT_LOADED);
> >>>>>>> +        vcpu_unpause(prev);
> >>>>>>>
> >>>>>>>           vpmu = vcpu_vpmu(v);
> >>>>>>>       }
> >>>>>> We're running with IRQs off here, yet vcpu_pause() waits for the
> vcpu
> >>>>>> to actually be de-scheduled. Even with IRQs on this is already a
> >>>>>> relatively heavy operation (also including its impact on the remote
> >>>>>> side). Additionally the function is called from context_switch(),
> and
> >>>>>> I'm unsure of the usability of vcpu_pause() on such a path. In
> >>>>>> particular: Is there a risk of two CPUs doing this mutually to one
> >>>>>> another? If so, is deadlocking excluded?
> >>>>>>
> >>>>>> Hence at the very least I think the description wants extending, to
> >>>>>> discuss the safety of the change.
> >>>>>>
> >>>>>> Boris - any chance you could comment here? Iirc that's code you did
> >>>>>> introduce.
> >>>>>
> >>>>>
> >>>>> Is the assertion in vmx_find_msr() really needs to be for runnable
> vcpu or can it be a check on whether vcpu is actually running (e.g.
> RUNSTATE_running)?
> >>>>
> >>>> You cannot safely check for "running", as "runnable" may transition
> >>>> to/from "running" behind your back.
> >>>
> >>> The more I look at this the more I think the only sensible solution is
> >>> to have the vPMU state be saved on vmexit for all vCPUs.
> >>
> >> Do you really mean vmexit? It would suffice if state was reliably
> >> saved during context-switch-out, wouldn't it? At that point the
> >> vCPU can't be resumed on another pCPU, yet.
> >>
> >>> That way all
> >>> this having to figure out where and when a context needs saving during
> >>> scheduling goes away. Yes, it adds a bit of overhead for cases where
> >>> the vCPU will resume on the same pCPU and that context saved could
> >>> have been skipped,
> >>
> >> If you really mean vmexit, then I'm inclined to say that's more
> >> than just "a bit of overhead". I'd agree if you really meant
> >> context-switch-out, but as said further up it looks to me as if
> >> that was already occurring. Apparently I'm overlooking something
> >> crucial ...
> >
> > Yes, the current setup is doing exactly that, saving the vPMU context
> > on context-switch-out, and that's where the ASSERT failure occurs
> > because the vCPU it needs to save the context for is already runnable
> > on another pCPU.
>
> Well, if that's the scenario (sorry for not understanding it that
> way earlier on), then the assertion is too strict: While in context
> switch, the vCPU may be runnable, but certainly won't actually run
> anywhere. Therefore I'd be inclined to suggest to relax the
> condition just enough to cover this case, without actually going to
> checking for "running".
>

What ensures the vCPU won't actually run anywhere if it's in the runnable
state? And how do I relax the condition just enough to cover just this case?

Tamas

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

Re: [PATCH] x86/vpmu: fix race-condition in vpmu_load

[Jan Beulich]

On 19.09.2022 16:11, Tamas K Lengyel wrote:
> On Mon, Sep 19, 2022 at 9:58 AM Jan Beulich <jbeulich@suse.com> wrote:
> 
>> On 19.09.2022 15:24, Tamas K Lengyel wrote:
>>> On Mon, Sep 19, 2022 at 9:21 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>
>>>> On 19.09.2022 14:25, Tamas K Lengyel wrote:
>>>>> On Mon, Sep 19, 2022 at 5:28 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>
>>>>>> On 16.09.2022 23:35, Boris Ostrovsky wrote:
>>>>>>>
>>>>>>> On 9/16/22 8:52 AM, Jan Beulich wrote:
>>>>>>>> On 15.09.2022 16:01, Tamas K Lengyel wrote:
>>>>>>>>> While experimenting with the vPMU subsystem an ASSERT failure was
>>>>>>>>> observed in vmx_find_msr because the vcpu_runnable state was true.
>>>>>>>>>
>>>>>>>>> The root cause of the bug appears to be the fact that the vPMU
>> subsystem
>>>>>>>>> doesn't save its state on context_switch.
>>>>
>>>> For the further reply below - is this actually true? What is the
>>>> vpmu_switch_from() (resolving to vpmu_save()) doing then early
>>>> in context_switch()?
>>>>
>>>>>>>>> The vpmu_load function will attempt
>>>>>>>>> to gather the PMU state if its still loaded two different ways:
>>>>>>>>>      1. if the current pcpu is not where the vcpu ran before doing
>> a remote save
>>>>>>>>>      2. if the current pcpu had another vcpu active before doing a
>> local save
>>>>>>>>>
>>>>>>>>> However, in case the prev vcpu is being rescheduled on another
>> pcpu its state
>>>>>>>>> has already changed and vcpu_runnable is returning true, thus #2
>> will trip the
>>>>>>>>> ASSERT. The only way to avoid this race condition is to make sure
>> the
>>>>>>>>> prev vcpu is paused while being checked and its context saved.
>> Once the prev
>>>>>>>>> vcpu is resumed and does #1 it will find its state already saved.
>>>>>>>> While I consider this explanation plausible, I'm worried:
>>>>>>>>
>>>>>>>>> --- a/xen/arch/x86/cpu/vpmu.c
>>>>>>>>> +++ b/xen/arch/x86/cpu/vpmu.c
>>>>>>>>> @@ -419,8 +419,10 @@ int vpmu_load(struct vcpu *v, bool_t
>> from_guest)
>>>>>>>>>           vpmu = vcpu_vpmu(prev);
>>>>>>>>>
>>>>>>>>>           /* Someone ran here before us */
>>>>>>>>> +        vcpu_pause(prev);
>>>>>>>>>           vpmu_save_force(prev);
>>>>>>>>>           vpmu_reset(vpmu, VPMU_CONTEXT_LOADED);
>>>>>>>>> +        vcpu_unpause(prev);
>>>>>>>>>
>>>>>>>>>           vpmu = vcpu_vpmu(v);
>>>>>>>>>       }
>>>>>>>> We're running with IRQs off here, yet vcpu_pause() waits for the
>> vcpu
>>>>>>>> to actually be de-scheduled. Even with IRQs on this is already a
>>>>>>>> relatively heavy operation (also including its impact on the remote
>>>>>>>> side). Additionally the function is called from context_switch(),
>> and
>>>>>>>> I'm unsure of the usability of vcpu_pause() on such a path. In
>>>>>>>> particular: Is there a risk of two CPUs doing this mutually to one
>>>>>>>> another? If so, is deadlocking excluded?
>>>>>>>>
>>>>>>>> Hence at the very least I think the description wants extending, to
>>>>>>>> discuss the safety of the change.
>>>>>>>>
>>>>>>>> Boris - any chance you could comment here? Iirc that's code you did
>>>>>>>> introduce.
>>>>>>>
>>>>>>>
>>>>>>> Is the assertion in vmx_find_msr() really needs to be for runnable
>> vcpu or can it be a check on whether vcpu is actually running (e.g.
>> RUNSTATE_running)?
>>>>>>
>>>>>> You cannot safely check for "running", as "runnable" may transition
>>>>>> to/from "running" behind your back.
>>>>>
>>>>> The more I look at this the more I think the only sensible solution is
>>>>> to have the vPMU state be saved on vmexit for all vCPUs.
>>>>
>>>> Do you really mean vmexit? It would suffice if state was reliably
>>>> saved during context-switch-out, wouldn't it? At that point the
>>>> vCPU can't be resumed on another pCPU, yet.
>>>>
>>>>> That way all
>>>>> this having to figure out where and when a context needs saving during
>>>>> scheduling goes away. Yes, it adds a bit of overhead for cases where
>>>>> the vCPU will resume on the same pCPU and that context saved could
>>>>> have been skipped,
>>>>
>>>> If you really mean vmexit, then I'm inclined to say that's more
>>>> than just "a bit of overhead". I'd agree if you really meant
>>>> context-switch-out, but as said further up it looks to me as if
>>>> that was already occurring. Apparently I'm overlooking something
>>>> crucial ...
>>>
>>> Yes, the current setup is doing exactly that, saving the vPMU context
>>> on context-switch-out, and that's where the ASSERT failure occurs
>>> because the vCPU it needs to save the context for is already runnable
>>> on another pCPU.
>>
>> Well, if that's the scenario (sorry for not understanding it that
>> way earlier on), then the assertion is too strict: While in context
>> switch, the vCPU may be runnable, but certainly won't actually run
>> anywhere. Therefore I'd be inclined to suggest to relax the
>> condition just enough to cover this case, without actually going to
>> checking for "running".
>>
> 
> What ensures the vCPU won't actually run anywhere if it's in the runnable
> state?

The fact that the vCPU is the subject of context_switch().

> And how do I relax the condition just enough to cover just this case?

That's the more difficult question. The immediate solution, passing a
boolean or flag to vpmu_switch_from(), may not be practical, as it
would likely mean passing this through many layers.

Utilizing that I have Jürgen sitting next to me, I've discussed this
with him, and he suggested to simply check for v == current. And
indeed - set_current() in context_switch() happens a few lines after
vpmu_switch_from().

However, going back to vmx_find_msr() I find that the v == current
case is already included there. Which makes me wonder again - what
exactly is the scenario that you're observing the assertion
triggering? Would you mind spelling out the call chain, perhaps by
way of the call stack from the assertion?

Jan

Re: [PATCH] x86/vpmu: fix race-condition in vpmu_load

[Boris Ostrovsky]

On 9/19/22 10:56 AM, Jan Beulich wrote:
> On 19.09.2022 16:11, Tamas K Lengyel wrote:
>> On Mon, Sep 19, 2022 at 9:58 AM Jan Beulich <jbeulich@suse.com> wrote:
>>
>>> On 19.09.2022 15:24, Tamas K Lengyel wrote:
>>>> On Mon, Sep 19, 2022 at 9:21 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>
>>>>> On 19.09.2022 14:25, Tamas K Lengyel wrote:
>>>>>> On Mon, Sep 19, 2022 at 5:28 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>>
>>>>>>> On 16.09.2022 23:35, Boris Ostrovsky wrote:
>>>>>>>>
>>>>>>>> On 9/16/22 8:52 AM, Jan Beulich wrote:
>>>>>>>>> On 15.09.2022 16:01, Tamas K Lengyel wrote:
>>>>>>>>>> While experimenting with the vPMU subsystem an ASSERT failure was
>>>>>>>>>> observed in vmx_find_msr because the vcpu_runnable state was true.
>>>>>>>>>>
>>>>>>>>>> The root cause of the bug appears to be the fact that the vPMU
>>> subsystem
>>>>>>>>>> doesn't save its state on context_switch.
>>>>>
>>>>> For the further reply below - is this actually true? What is the
>>>>> vpmu_switch_from() (resolving to vpmu_save()) doing then early
>>>>> in context_switch()?
>>>>>
>>>>>>>>>> The vpmu_load function will attempt
>>>>>>>>>> to gather the PMU state if its still loaded two different ways:
>>>>>>>>>>       1. if the current pcpu is not where the vcpu ran before doing
>>> a remote save
>>>>>>>>>>       2. if the current pcpu had another vcpu active before doing a
>>> local save
>>>>>>>>>>
>>>>>>>>>> However, in case the prev vcpu is being rescheduled on another
>>> pcpu its state
>>>>>>>>>> has already changed and vcpu_runnable is returning true, thus #2
>>> will trip the
>>>>>>>>>> ASSERT. The only way to avoid this race condition is to make sure
>>> the
>>>>>>>>>> prev vcpu is paused while being checked and its context saved.
>>> Once the prev
>>>>>>>>>> vcpu is resumed and does #1 it will find its state already saved.
>>>>>>>>> While I consider this explanation plausible, I'm worried:
>>>>>>>>>
>>>>>>>>>> --- a/xen/arch/x86/cpu/vpmu.c
>>>>>>>>>> +++ b/xen/arch/x86/cpu/vpmu.c
>>>>>>>>>> @@ -419,8 +419,10 @@ int vpmu_load(struct vcpu *v, bool_t
>>> from_guest)
>>>>>>>>>>            vpmu = vcpu_vpmu(prev);
>>>>>>>>>>
>>>>>>>>>>            /* Someone ran here before us */
>>>>>>>>>> +        vcpu_pause(prev);
>>>>>>>>>>            vpmu_save_force(prev);
>>>>>>>>>>            vpmu_reset(vpmu, VPMU_CONTEXT_LOADED);
>>>>>>>>>> +        vcpu_unpause(prev);
>>>>>>>>>>
>>>>>>>>>>            vpmu = vcpu_vpmu(v);
>>>>>>>>>>        }
>>>>>>>>> We're running with IRQs off here, yet vcpu_pause() waits for the
>>> vcpu
>>>>>>>>> to actually be de-scheduled. Even with IRQs on this is already a
>>>>>>>>> relatively heavy operation (also including its impact on the remote
>>>>>>>>> side). Additionally the function is called from context_switch(),
>>> and
>>>>>>>>> I'm unsure of the usability of vcpu_pause() on such a path. In
>>>>>>>>> particular: Is there a risk of two CPUs doing this mutually to one
>>>>>>>>> another? If so, is deadlocking excluded?
>>>>>>>>>
>>>>>>>>> Hence at the very least I think the description wants extending, to
>>>>>>>>> discuss the safety of the change.
>>>>>>>>>
>>>>>>>>> Boris - any chance you could comment here? Iirc that's code you did
>>>>>>>>> introduce.
>>>>>>>>
>>>>>>>>
>>>>>>>> Is the assertion in vmx_find_msr() really needs to be for runnable
>>> vcpu or can it be a check on whether vcpu is actually running (e.g.
>>> RUNSTATE_running)?
>>>>>>>
>>>>>>> You cannot safely check for "running", as "runnable" may transition
>>>>>>> to/from "running" behind your back.
>>>>>>
>>>>>> The more I look at this the more I think the only sensible solution is
>>>>>> to have the vPMU state be saved on vmexit for all vCPUs.
>>>>>
>>>>> Do you really mean vmexit? It would suffice if state was reliably
>>>>> saved during context-switch-out, wouldn't it? At that point the
>>>>> vCPU can't be resumed on another pCPU, yet.
>>>>>
>>>>>> That way all
>>>>>> this having to figure out where and when a context needs saving during
>>>>>> scheduling goes away. Yes, it adds a bit of overhead for cases where
>>>>>> the vCPU will resume on the same pCPU and that context saved could
>>>>>> have been skipped,
>>>>>
>>>>> If you really mean vmexit, then I'm inclined to say that's more
>>>>> than just "a bit of overhead". I'd agree if you really meant
>>>>> context-switch-out, but as said further up it looks to me as if
>>>>> that was already occurring. Apparently I'm overlooking something
>>>>> crucial ...
>>>>
>>>> Yes, the current setup is doing exactly that, saving the vPMU context
>>>> on context-switch-out, and that's where the ASSERT failure occurs
>>>> because the vCPU it needs to save the context for is already runnable
>>>> on another pCPU.
>>>
>>> Well, if that's the scenario (sorry for not understanding it that
>>> way earlier on), then the assertion is too strict: While in context
>>> switch, the vCPU may be runnable, but certainly won't actually run
>>> anywhere. Therefore I'd be inclined to suggest to relax the
>>> condition just enough to cover this case, without actually going to
>>> checking for "running".
>>>
>>
>> What ensures the vCPU won't actually run anywhere if it's in the runnable
>> state?
> 
> The fact that the vCPU is the subject of context_switch().
> 
>> And how do I relax the condition just enough to cover just this case?
> 
> That's the more difficult question. The immediate solution, passing a
> boolean or flag to vpmu_switch_from(), may not be practical, as it
> would likely mean passing this through many layers.
> 
> Utilizing that I have Jürgen sitting next to me, I've discussed this
> with him, and he suggested to simply check for v == current. And
> indeed - set_current() in context_switch() happens a few lines after
> vpmu_switch_from().



It is saving vpmu data from current pcpu's MSRs for a remote vcpu so @v
in vmx_find_msr() is not @current:

      vpmu_load()
          ...
          prev = per_cpu(last_vcpu, pcpu);
          vpmu_save_force(prev)
              core2_vpmu_save()
                  __core2_vpmu_save()
                      vmx_read_guest_msr()
                          vmx_find_msr()


The call to vmx_find_msr() was introduced by 755087eb9b10c. I wonder though whether
this call is needed when code path above is executed (i.e. when we are saving
remove vcpu)


-boris

> 
> However, going back to vmx_find_msr() I find that the v == current
> case is already included there. Which makes me wonder again - what
> exactly is the scenario that you're observing the assertion
> triggering? Would you mind spelling out the call chain, perhaps by
> way of the call stack from the assertion?
> 
> Jan

Re: [PATCH] x86/vpmu: fix race-condition in vpmu_load

[Jan Beulich]

On 20.09.2022 00:42, Boris Ostrovsky wrote:
> 
> 
> On 9/19/22 10:56 AM, Jan Beulich wrote:
>> On 19.09.2022 16:11, Tamas K Lengyel wrote:
>>> On Mon, Sep 19, 2022 at 9:58 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>
>>>> On 19.09.2022 15:24, Tamas K Lengyel wrote:
>>>>> On Mon, Sep 19, 2022 at 9:21 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>
>>>>>> On 19.09.2022 14:25, Tamas K Lengyel wrote:
>>>>>>> On Mon, Sep 19, 2022 at 5:28 AM Jan Beulich <jbeulich@suse.com> wrote:
>>>>>>>>
>>>>>>>> On 16.09.2022 23:35, Boris Ostrovsky wrote:
>>>>>>>>>
>>>>>>>>> On 9/16/22 8:52 AM, Jan Beulich wrote:
>>>>>>>>>> On 15.09.2022 16:01, Tamas K Lengyel wrote:
>>>>>>>>>>> While experimenting with the vPMU subsystem an ASSERT failure was
>>>>>>>>>>> observed in vmx_find_msr because the vcpu_runnable state was true.
>>>>>>>>>>>
>>>>>>>>>>> The root cause of the bug appears to be the fact that the vPMU
>>>> subsystem
>>>>>>>>>>> doesn't save its state on context_switch.
>>>>>>
>>>>>> For the further reply below - is this actually true? What is the
>>>>>> vpmu_switch_from() (resolving to vpmu_save()) doing then early
>>>>>> in context_switch()?
>>>>>>
>>>>>>>>>>> The vpmu_load function will attempt
>>>>>>>>>>> to gather the PMU state if its still loaded two different ways:
>>>>>>>>>>>       1. if the current pcpu is not where the vcpu ran before doing
>>>> a remote save
>>>>>>>>>>>       2. if the current pcpu had another vcpu active before doing a
>>>> local save
>>>>>>>>>>>
>>>>>>>>>>> However, in case the prev vcpu is being rescheduled on another
>>>> pcpu its state
>>>>>>>>>>> has already changed and vcpu_runnable is returning true, thus #2
>>>> will trip the
>>>>>>>>>>> ASSERT. The only way to avoid this race condition is to make sure
>>>> the
>>>>>>>>>>> prev vcpu is paused while being checked and its context saved.
>>>> Once the prev
>>>>>>>>>>> vcpu is resumed and does #1 it will find its state already saved.
>>>>>>>>>> While I consider this explanation plausible, I'm worried:
>>>>>>>>>>
>>>>>>>>>>> --- a/xen/arch/x86/cpu/vpmu.c
>>>>>>>>>>> +++ b/xen/arch/x86/cpu/vpmu.c
>>>>>>>>>>> @@ -419,8 +419,10 @@ int vpmu_load(struct vcpu *v, bool_t
>>>> from_guest)
>>>>>>>>>>>            vpmu = vcpu_vpmu(prev);
>>>>>>>>>>>
>>>>>>>>>>>            /* Someone ran here before us */
>>>>>>>>>>> +        vcpu_pause(prev);
>>>>>>>>>>>            vpmu_save_force(prev);
>>>>>>>>>>>            vpmu_reset(vpmu, VPMU_CONTEXT_LOADED);
>>>>>>>>>>> +        vcpu_unpause(prev);
>>>>>>>>>>>
>>>>>>>>>>>            vpmu = vcpu_vpmu(v);
>>>>>>>>>>>        }
>>>>>>>>>> We're running with IRQs off here, yet vcpu_pause() waits for the
>>>> vcpu
>>>>>>>>>> to actually be de-scheduled. Even with IRQs on this is already a
>>>>>>>>>> relatively heavy operation (also including its impact on the remote
>>>>>>>>>> side). Additionally the function is called from context_switch(),
>>>> and
>>>>>>>>>> I'm unsure of the usability of vcpu_pause() on such a path. In
>>>>>>>>>> particular: Is there a risk of two CPUs doing this mutually to one
>>>>>>>>>> another? If so, is deadlocking excluded?
>>>>>>>>>>
>>>>>>>>>> Hence at the very least I think the description wants extending, to
>>>>>>>>>> discuss the safety of the change.
>>>>>>>>>>
>>>>>>>>>> Boris - any chance you could comment here? Iirc that's code you did
>>>>>>>>>> introduce.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Is the assertion in vmx_find_msr() really needs to be for runnable
>>>> vcpu or can it be a check on whether vcpu is actually running (e.g.
>>>> RUNSTATE_running)?
>>>>>>>>
>>>>>>>> You cannot safely check for "running", as "runnable" may transition
>>>>>>>> to/from "running" behind your back.
>>>>>>>
>>>>>>> The more I look at this the more I think the only sensible solution is
>>>>>>> to have the vPMU state be saved on vmexit for all vCPUs.
>>>>>>
>>>>>> Do you really mean vmexit? It would suffice if state was reliably
>>>>>> saved during context-switch-out, wouldn't it? At that point the
>>>>>> vCPU can't be resumed on another pCPU, yet.
>>>>>>
>>>>>>> That way all
>>>>>>> this having to figure out where and when a context needs saving during
>>>>>>> scheduling goes away. Yes, it adds a bit of overhead for cases where
>>>>>>> the vCPU will resume on the same pCPU and that context saved could
>>>>>>> have been skipped,
>>>>>>
>>>>>> If you really mean vmexit, then I'm inclined to say that's more
>>>>>> than just "a bit of overhead". I'd agree if you really meant
>>>>>> context-switch-out, but as said further up it looks to me as if
>>>>>> that was already occurring. Apparently I'm overlooking something
>>>>>> crucial ...
>>>>>
>>>>> Yes, the current setup is doing exactly that, saving the vPMU context
>>>>> on context-switch-out, and that's where the ASSERT failure occurs
>>>>> because the vCPU it needs to save the context for is already runnable
>>>>> on another pCPU.
>>>>
>>>> Well, if that's the scenario (sorry for not understanding it that
>>>> way earlier on), then the assertion is too strict: While in context
>>>> switch, the vCPU may be runnable, but certainly won't actually run
>>>> anywhere. Therefore I'd be inclined to suggest to relax the
>>>> condition just enough to cover this case, without actually going to
>>>> checking for "running".
>>>>
>>>
>>> What ensures the vCPU won't actually run anywhere if it's in the runnable
>>> state?
>>
>> The fact that the vCPU is the subject of context_switch().
>>
>>> And how do I relax the condition just enough to cover just this case?
>>
>> That's the more difficult question. The immediate solution, passing a
>> boolean or flag to vpmu_switch_from(), may not be practical, as it
>> would likely mean passing this through many layers.
>>
>> Utilizing that I have Jürgen sitting next to me, I've discussed this
>> with him, and he suggested to simply check for v == current. And
>> indeed - set_current() in context_switch() happens a few lines after
>> vpmu_switch_from().
> 
> 
> 
> It is saving vpmu data from current pcpu's MSRs for a remote vcpu so @v
> in vmx_find_msr() is not @current:
> 
>       vpmu_load()
>           ...
>           prev = per_cpu(last_vcpu, pcpu);
>           vpmu_save_force(prev)
>               core2_vpmu_save()
>                   __core2_vpmu_save()
>                       vmx_read_guest_msr()
>                           vmx_find_msr()
> 
> 
> The call to vmx_find_msr() was introduced by 755087eb9b10c. I wonder though whether
> this call is needed when code path above is executed (i.e. when we are saving
> remove vcpu)

How could it not be needed? We need to obtain the guest value. The
thing I don't understand is why this forced saving is necessary,
when context_switch() unconditionally calls vpmu_switch_from().

Jan

Re: [PATCH] x86/vpmu: fix race-condition in vpmu_load

[Boris Ostrovsky]

On 9/20/22 4:01 AM, Jan Beulich wrote:
> On 20.09.2022 00:42, Boris Ostrovsky wrote:

>>
>> It is saving vpmu data from current pcpu's MSRs for a remote vcpu so @v
>> in vmx_find_msr() is not @current:
>>
>>        vpmu_load()
>>            ...
>>            prev = per_cpu(last_vcpu, pcpu);
>>            vpmu_save_force(prev)
>>                core2_vpmu_save()
>>                    __core2_vpmu_save()
>>                        vmx_read_guest_msr()
>>                            vmx_find_msr()
>>
>>
>> The call to vmx_find_msr() was introduced by 755087eb9b10c. I wonder though whether
>> this call is needed when code path above is executed (i.e. when we are saving
>> remove vcpu)
> 
> How could it not be needed? We need to obtain the guest value. The
> thing I don't understand is why this forced saving is necessary,
> when context_switch() unconditionally calls vpmu_switch_from().


IIRC the logic is:

1. vcpuA runs on pcpu0
2. vcpuA is de-scheduled and is selected to run on pcpu1. It has not yet called vpmu_load() from pcpu1
3. vcpuB is ready to run on pcpu0, calls vpmu_load()
4. vcpuB discovers that pcpu0's MSRs are still holding values from vcpuA
5. vcpuB calls vpmu_force_save(vcpuA) which stashes pcpu0's MSRs into vcpuA's vpmu context.


-boris

Re: [PATCH] x86/vpmu: fix race-condition in vpmu_load

[Jan Beulich]

On 20.09.2022 16:26, Boris Ostrovsky wrote:
> On 9/20/22 4:01 AM, Jan Beulich wrote:
>> On 20.09.2022 00:42, Boris Ostrovsky wrote:
>>> It is saving vpmu data from current pcpu's MSRs for a remote vcpu so @v
>>> in vmx_find_msr() is not @current:
>>>
>>>        vpmu_load()
>>>            ...
>>>            prev = per_cpu(last_vcpu, pcpu);
>>>            vpmu_save_force(prev)
>>>                core2_vpmu_save()
>>>                    __core2_vpmu_save()
>>>                        vmx_read_guest_msr()
>>>                            vmx_find_msr()
>>>
>>>
>>> The call to vmx_find_msr() was introduced by 755087eb9b10c. I wonder though whether
>>> this call is needed when code path above is executed (i.e. when we are saving
>>> remove vcpu)
>>
>> How could it not be needed? We need to obtain the guest value. The
>> thing I don't understand is why this forced saving is necessary,
>> when context_switch() unconditionally calls vpmu_switch_from().
> 
> 
> IIRC the logic is:
> 
> 1. vcpuA runs on pcpu0
> 2. vcpuA is de-scheduled and is selected to run on pcpu1. It has not yet called vpmu_load() from pcpu1

The calling of vpmu_load() shouldn't matter here. What does matter is
that vpmu_save() was necessarily called already. Therefore I'm having
trouble seeing why ...

> 3. vcpuB is ready to run on pcpu0, calls vpmu_load()
> 4. vcpuB discovers that pcpu0's MSRs are still holding values from vcpuA
> 5. vcpuB calls vpmu_force_save(vcpuA) which stashes pcpu0's MSRs into vcpuA's vpmu context.

... forced saving would be necessary here. What's necessary at this
point is only the loading of vcpuB's MSR values.

Jan

Re: [PATCH] x86/vpmu: fix race-condition in vpmu_load

[Boris Ostrovsky]

On 9/20/22 10:54 AM, Jan Beulich wrote:
> On 20.09.2022 16:26, Boris Ostrovsky wrote:
>> On 9/20/22 4:01 AM, Jan Beulich wrote:
>>> On 20.09.2022 00:42, Boris Ostrovsky wrote:
>>>> It is saving vpmu data from current pcpu's MSRs for a remote vcpu so @v
>>>> in vmx_find_msr() is not @current:
>>>>
>>>>         vpmu_load()
>>>>             ...
>>>>             prev = per_cpu(last_vcpu, pcpu);
>>>>             vpmu_save_force(prev)
>>>>                 core2_vpmu_save()
>>>>                     __core2_vpmu_save()
>>>>                         vmx_read_guest_msr()
>>>>                             vmx_find_msr()
>>>>
>>>>
>>>> The call to vmx_find_msr() was introduced by 755087eb9b10c. I wonder though whether
>>>> this call is needed when code path above is executed (i.e. when we are saving
>>>> remove vcpu)
>>> How could it not be needed? We need to obtain the guest value. The
>>> thing I don't understand is why this forced saving is necessary,
>>> when context_switch() unconditionally calls vpmu_switch_from().
>>
>> IIRC the logic is:
>>
>> 1. vcpuA runs on pcpu0
>> 2. vcpuA is de-scheduled and is selected to run on pcpu1. It has not yet called vpmu_load() from pcpu1
> The calling of vpmu_load() shouldn't matter here. What does matter is
> that vpmu_save() was necessarily called already.


I thought we don't always save MSRs on context switch because we optimize for case when the same vcpu gets scheduled again. But I am not sure I see this now that I am looking at the code.


-boris



>   Therefore I'm having
> trouble seeing why ...
>
>> 3. vcpuB is ready to run on pcpu0, calls vpmu_load()
>> 4. vcpuB discovers that pcpu0's MSRs are still holding values from vcpuA
>> 5. vcpuB calls vpmu_force_save(vcpuA) which stashes pcpu0's MSRs into vcpuA's vpmu context.
> ... forced saving would be necessary here. What's necessary at this
> point is only the loading of vcpuB's MSR values.

Re: [PATCH] x86/vpmu: fix race-condition in vpmu_load

[Tamas K Lengyel]

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

On Tue, Sep 20, 2022 at 2:12 PM Boris Ostrovsky <boris.ostrovsky@oracle.com>
wrote:

>
> On 9/20/22 10:54 AM, Jan Beulich wrote:
> > On 20.09.2022 16:26, Boris Ostrovsky wrote:
> >> On 9/20/22 4:01 AM, Jan Beulich wrote:
> >>> On 20.09.2022 00:42, Boris Ostrovsky wrote:
> >>>> It is saving vpmu data from current pcpu's MSRs for a remote vcpu so
> @v
> >>>> in vmx_find_msr() is not @current:
> >>>>
> >>>>         vpmu_load()
> >>>>             ...
> >>>>             prev = per_cpu(last_vcpu, pcpu);
> >>>>             vpmu_save_force(prev)
> >>>>                 core2_vpmu_save()
> >>>>                     __core2_vpmu_save()
> >>>>                         vmx_read_guest_msr()
> >>>>                             vmx_find_msr()
> >>>>
> >>>>
> >>>> The call to vmx_find_msr() was introduced by 755087eb9b10c. I wonder
> though whether
> >>>> this call is needed when code path above is executed (i.e. when we
> are saving
> >>>> remove vcpu)
> >>> How could it not be needed? We need to obtain the guest value. The
> >>> thing I don't understand is why this forced saving is necessary,
> >>> when context_switch() unconditionally calls vpmu_switch_from().
> >>
> >> IIRC the logic is:
> >>
> >> 1. vcpuA runs on pcpu0
> >> 2. vcpuA is de-scheduled and is selected to run on pcpu1. It has not
> yet called vpmu_load() from pcpu1
> > The calling of vpmu_load() shouldn't matter here. What does matter is
> > that vpmu_save() was necessarily called already.
>
>
> I thought we don't always save MSRs on context switch because we optimize
> for case when the same vcpu gets scheduled again. But I am not sure I see
> this now that I am looking at the code.
>

I see context_switch calling vpmu_save_from before __context_switch, but if
that did actually save the vPMU state it would have reset
VPMU_CONTEXT_LOADED, so by the time vpmu_load calls vpmu_save_force it
would have just bailed before hitting the ASSERT failure in vmx_find_msrs.
The context must still be loaded at that point (I'm trying to verify right
now by adding some printks).

Tamas

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

Re: [PATCH] x86/vpmu: fix race-condition in vpmu_load

[Tamas K Lengyel]

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

On Tue, Sep 20, 2022 at 2:27 PM Tamas K Lengyel <tamas.k.lengyel@gmail.com>
wrote:

>
>
> On Tue, Sep 20, 2022 at 2:12 PM Boris Ostrovsky <
> boris.ostrovsky@oracle.com> wrote:
>
>>
>> On 9/20/22 10:54 AM, Jan Beulich wrote:
>> > On 20.09.2022 16:26, Boris Ostrovsky wrote:
>> >> On 9/20/22 4:01 AM, Jan Beulich wrote:
>> >>> On 20.09.2022 00:42, Boris Ostrovsky wrote:
>> >>>> It is saving vpmu data from current pcpu's MSRs for a remote vcpu so
>> @v
>> >>>> in vmx_find_msr() is not @current:
>> >>>>
>> >>>>         vpmu_load()
>> >>>>             ...
>> >>>>             prev = per_cpu(last_vcpu, pcpu);
>> >>>>             vpmu_save_force(prev)
>> >>>>                 core2_vpmu_save()
>> >>>>                     __core2_vpmu_save()
>> >>>>                         vmx_read_guest_msr()
>> >>>>                             vmx_find_msr()
>> >>>>
>> >>>>
>> >>>> The call to vmx_find_msr() was introduced by 755087eb9b10c. I wonder
>> though whether
>> >>>> this call is needed when code path above is executed (i.e. when we
>> are saving
>> >>>> remove vcpu)
>> >>> How could it not be needed? We need to obtain the guest value. The
>> >>> thing I don't understand is why this forced saving is necessary,
>> >>> when context_switch() unconditionally calls vpmu_switch_from().
>> >>
>> >> IIRC the logic is:
>> >>
>> >> 1. vcpuA runs on pcpu0
>> >> 2. vcpuA is de-scheduled and is selected to run on pcpu1. It has not
>> yet called vpmu_load() from pcpu1
>> > The calling of vpmu_load() shouldn't matter here. What does matter is
>> > that vpmu_save() was necessarily called already.
>>
>>
>> I thought we don't always save MSRs on context switch because we optimize
>> for case when the same vcpu gets scheduled again. But I am not sure I see
>> this now that I am looking at the code.
>>
>
> I see context_switch calling vpmu_save_from before __context_switch, but
> if that did actually save the vPMU state it would have reset
> VPMU_CONTEXT_LOADED, so by the time vpmu_load calls vpmu_save_force it
> would have just bailed before hitting the ASSERT failure in vmx_find_msrs.
> The context must still be loaded at that point (I'm trying to verify right
> now by adding some printks).
>

OK, Boris was correct above, MSRs are not saved on context switch
automatically because of that optimization. VPMU_CONTEXT_SAVE isn't set, so
the only thing vpmu_switch_from does is set global control to 0 in case it
was a PV vCPU (see core2_vpmu_save checks for both VPMU_CONTEXT_SAVE and
VPMU_CONTEXT_LOADED) and vpmu_switch_from doesn't set VPMU_CONTEXT_SAVE. So
for HVM vCPUs it does nothing, that's why we still see the context still
loaded when we get to vpmu_load.

It would be a simple enough change to make vpmu_switch_from always save the
context and then we could get rid of vpmu_load trying to do it later and
getting into that ASSERT failure. Thoughts?

Tamas

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

Re: [PATCH] x86/vpmu: fix race-condition in vpmu_load

[Jan Beulich]

On 22.09.2022 15:35, Tamas K Lengyel wrote:
> On Tue, Sep 20, 2022 at 2:27 PM Tamas K Lengyel <tamas.k.lengyel@gmail.com>
> wrote:
> 
>>
>>
>> On Tue, Sep 20, 2022 at 2:12 PM Boris Ostrovsky <
>> boris.ostrovsky@oracle.com> wrote:
>>
>>>
>>> On 9/20/22 10:54 AM, Jan Beulich wrote:
>>>> On 20.09.2022 16:26, Boris Ostrovsky wrote:
>>>>> On 9/20/22 4:01 AM, Jan Beulich wrote:
>>>>>> On 20.09.2022 00:42, Boris Ostrovsky wrote:
>>>>>>> It is saving vpmu data from current pcpu's MSRs for a remote vcpu so
>>> @v
>>>>>>> in vmx_find_msr() is not @current:
>>>>>>>
>>>>>>>         vpmu_load()
>>>>>>>             ...
>>>>>>>             prev = per_cpu(last_vcpu, pcpu);
>>>>>>>             vpmu_save_force(prev)
>>>>>>>                 core2_vpmu_save()
>>>>>>>                     __core2_vpmu_save()
>>>>>>>                         vmx_read_guest_msr()
>>>>>>>                             vmx_find_msr()
>>>>>>>
>>>>>>>
>>>>>>> The call to vmx_find_msr() was introduced by 755087eb9b10c. I wonder
>>> though whether
>>>>>>> this call is needed when code path above is executed (i.e. when we
>>> are saving
>>>>>>> remove vcpu)
>>>>>> How could it not be needed? We need to obtain the guest value. The
>>>>>> thing I don't understand is why this forced saving is necessary,
>>>>>> when context_switch() unconditionally calls vpmu_switch_from().
>>>>>
>>>>> IIRC the logic is:
>>>>>
>>>>> 1. vcpuA runs on pcpu0
>>>>> 2. vcpuA is de-scheduled and is selected to run on pcpu1. It has not
>>> yet called vpmu_load() from pcpu1
>>>> The calling of vpmu_load() shouldn't matter here. What does matter is
>>>> that vpmu_save() was necessarily called already.
>>>
>>>
>>> I thought we don't always save MSRs on context switch because we optimize
>>> for case when the same vcpu gets scheduled again. But I am not sure I see
>>> this now that I am looking at the code.
>>>
>>
>> I see context_switch calling vpmu_save_from before __context_switch, but
>> if that did actually save the vPMU state it would have reset
>> VPMU_CONTEXT_LOADED, so by the time vpmu_load calls vpmu_save_force it
>> would have just bailed before hitting the ASSERT failure in vmx_find_msrs.
>> The context must still be loaded at that point (I'm trying to verify right
>> now by adding some printks).
>>
> 
> OK, Boris was correct above, MSRs are not saved on context switch
> automatically because of that optimization. VPMU_CONTEXT_SAVE isn't set, so
> the only thing vpmu_switch_from does is set global control to 0 in case it
> was a PV vCPU (see core2_vpmu_save checks for both VPMU_CONTEXT_SAVE and
> VPMU_CONTEXT_LOADED) and vpmu_switch_from doesn't set VPMU_CONTEXT_SAVE. So
> for HVM vCPUs it does nothing, that's why we still see the context still
> loaded when we get to vpmu_load.
> 
> It would be a simple enough change to make vpmu_switch_from always save the
> context and then we could get rid of vpmu_load trying to do it later and
> getting into that ASSERT failure. Thoughts?

I'd much prefer that over e.g. the vCPU-pausing approach. I also think
vpmu_save() is misnamed if it might not really save anything.

Jan