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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.02.17 at 15:29, <JBeulich@xxxxxxxx> wrote:
>>>> 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.

Which then also indicates a likely reason for the endless loop you
saw with my patch - the spin loop (which I'm no longer convinced is
needed at all) would need to be skipped if the current CPU is the
active one:

void vmx_vmcs_reload(struct vcpu *v)
{
    /*
     * As we're running with interrupts disabled, we can't acquire
     * v->arch.hvm_vmx.vmcs_lock here. However, with interrupts disabled
     * the VMCS can't be taken away from us anymore if we still own it.
     */
    ASSERT(!local_irq_is_enabled());
    if ( v->arch.hvm_vmx.vmcs_pa == this_cpu(current_vmcs) )
        return;
    ASSERT(!this_cpu(current_vmcs));

    if ( v->arch.hvm_vmx.active_cpu != smp_processor_id() )
    {
        /*
         * Wait for the remote side to be done with the VMCS before
         * loading it here.
         */
        while ( v->arch.hvm_vmx.active_cpu != -1 )
            cpu_relax();
    }
    vmx_load_vmcs(v);
}

As to the spinning not being needed in the first place - the remote
CPU using the VMCS won't unpause us before being done with the
VMCS anyway. Thoughts?

Jan

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