[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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





On Tue, Sep 20, 2022 at 2:12 PM Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx> 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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.