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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 15/02/17 08:37, Jan Beulich wrote:
>>>> On 14.02.17 at 16:16, <andrew.cooper3@citrix.com> wrote:
>> On 14/02/17 10:28, Jan Beulich wrote:
>>> --- a/xen/arch/x86/hvm/vmx/vmcs.c
>>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
>>> @@ -552,6 +552,27 @@ static void vmx_load_vmcs(struct vcpu *v
>>>      local_irq_restore(flags);
>>>  }
>>>  
>>> +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));
>>> +
>>> +    /*
>>> +     * 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();
>> Doesn't this need a ACCESS_ONCE() read?
>>
>> While the compiled code (using GCC 4.9) isn't an infinite loop, I am not
>> aware of anything which prevents a compiler hoisting the comparison out
>> as being constant.
> That's the (intended) side effect of cpu_relax() having a memory
> clobber.

Ah ok.  In which case that should be fine.

~Andrew

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