Re: [Xen-devel] [PATCH 1/2] VMX: fix VMCS race on context-switch paths

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.02.17 at 14:40, <sergey.dyasli@xxxxxxxxxx> wrote:
> On Wed, 2017-02-15 at 06:03 -0700, Jan Beulich wrote:
>> > > > On 15.02.17 at 12:55, <JBeulich@xxxxxxxx> wrote:
>> > > > > On 15.02.17 at 12:48, <sergey.dyasli@xxxxxxxxxx> wrote:
>> > > 
>> > > On Wed, 2017-02-15 at 04:39 -0700, Jan Beulich wrote:
>> > > > > > > On 15.02.17 at 11:27, <sergey.dyasli@xxxxxxxxxx> wrote:
>> > > > > 
>> > > > > (XEN) [ 1408.075638] Xen call trace:
>> > > > > (XEN) [ 1408.079322]    [<ffff82d0801ea2a2>] 
>> > > > > vmx_vmcs_reload+0x32/0x50
>> > > > > (XEN) [ 1408.086303]    [<ffff82d08016c58d>] 
>> > > > > context_switch+0x85d/0xeb0
>> > > > > (XEN) [ 1408.093380]    [<ffff82d08012fb8e>] 
> schedule.c#schedule+0x46e/0x7d0
>> > > > > (XEN) [ 1408.100942]    [<ffff82d080164305>] 
>> > > > > reprogram_timer+0x75/0xe0
>> > > > > (XEN) [ 1408.107925]    [<ffff82d080136400>] 
>> > 
>> > timer.c#timer_softirq_action+0x90/0x210
>> > > > > (XEN) [ 1408.116263]    [<ffff82d08013311c>] 
> softirq.c#__do_softirq+0x5c/0x90
>> > > > > (XEN) [ 1408.123921]    [<ffff82d080167d35>] 
>> > > > > domain.c#idle_loop+0x25/0x60
>> > > > 
>> > > > Taking your later reply into account - were you able to figure out
>> > > > what other party held onto the VMCS being waited for here?
>> > > 
>> > > Unfortunately, no. It was unclear from debug logs. But judging from
>> > > the following vmx_do_resume() code:
>> > > 
>> > >     if ( v->arch.hvm_vmx.active_cpu == smp_processor_id() )
>> > >     {
>> > >         if ( v->arch.hvm_vmx.vmcs_pa != this_cpu(current_vmcs) )
>> > >             vmx_load_vmcs(v);
>> > >     }
>> > > 
>> > > If both of the above conditions are true then vmx_vmcs_reload() will
>> > > probably hang.
>> > 
>> > I don't follow (reload should run before this, not after), but I must
>> > be missing something more general anyway, as I'm seeing the code
>> > above being needed despite the reload additions.
>> 
>> I think I've understood part of it over lunch: Surprisingly enough
>> vmx_ctxt_switch_to() doesn't re-establish the VMCS, so it needs
>> to be done here. Which I think means we don't need the new
>> hook at all, as that way the state is no different between going
>> through ->to() or bypassing it.
>> 
>> What I continue to not understand is why vmcs_pa would ever
>> not match current_vmcs when active_cpu is smp_processor_id().
>> So far I thought both are always updated together. Looking
>> further ...
> 
> This is exactly what will happen should the 3.1 occur:
> 
>     1. HVM vCPU#1 --> idle vCPU context_switch
> 
>     2. softirq --> vmx_vmcs_enter() + vmx_vmcs_exit() for a remote vCPU
>        [scenario with PML]
>        This will switch current_vmcs to a remote one.
>        has_hvm_container_vcpu(current) will be false and vmcs will not
>        be reloaded.

Oh, right - this updates v's active_cpu, but doesn't update the
field for the vCPU the VMCS is being taken away from.

>     3.1. idle vCPU --> HVM vCPU#1 (same) context_switch
>          vmx_do_resume
>             v->arch.hvm_vmx.active_cpu == smp_processor_id()
>             v->arch.hvm_vmx.vmcs_pa != this_cpu(current_vmcs)
> 
>     3.2 idle vCPU --> HVM vCPU#2 (different)
>             __context_switch()
>                 vmwrite
>                 BUG()
>        This is the original BUG() scenario which my patch
>        addresses.

Which I'm not convinced of, as indicated in an earlier reply (on the
thread Anshul did start) to my own consideration of using curr_vcpu.
The problem of vcpu_pause() not spinning when v->is_running is
clear does not get addressed by your patch afaict, so to me it looks
as if you only shrink the critical time window without fully eliminating
it.

Nor can I see how the above would have been an issue in the
first place - that's precisely what vmx_do_resume() is taking care
of (unless you've found a vmwrite that happens before this
function runs, which would then get us back to the question why
vmx_ctxt_switch_to() doesn't load the VMCS). Afaics the problem
is on the switch-out and switch-same paths, but not on the
switch-in one (or if there is one there, then it's yet another one,
with a yet to be explained way of the VMCS being taken away
underneath the vCPU's feet).

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel