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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.02.17 at 16:16, <andrew.cooper3@xxxxxxxxxx> 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.

Jan


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