Re: [PATCHv5 1/2] x86/ept: invalidate guest physical mappings on VMENTER

[David Vrabel <david.vrabel@citrix.com>]

On 18/12/15 10:32, Jan Beulich wrote:
>>>> On 18.12.15 at 11:18, <david.vrabel@citrix.com> wrote:
>> On 18/12/15 07:53, Tian, Kevin wrote:
>>>> From: David Vrabel [mailto:david.vrabel@citrix.com]
>>>> Sent: Thursday, December 17, 2015 11:17 PM
>>>
>>> [...]
>>>
>>>> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
>>>> index f7c5e4f..cca35f2 100644
>>>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>>>
>>> [...]
>>>
>>>> @@ -3507,6 +3495,16 @@ void vmx_vmenter_helper(const struct cpu_user_regs 
>> *regs)
>>>>      if ( unlikely(need_flush) )
>>>>          vpid_sync_all();
>>>>
>>>> +    if ( paging_mode_hap(curr->domain) )
>>>> +    {
>>>> +        struct ept_data *ept = &p2m_get_hostp2m(curr->domain)->ept;
>>>> +        unsigned int cpu = smp_processor_id();
>>>> +
>>>> +        if ( cpumask_test_cpu(cpu, ept->invalidate)
>>>> +             && cpumask_test_and_clear_cpu(cpu, ept->invalidate) )
>>>
>>> Just test_and_clear should be enough.
>>
>> The first test is to avoid the locked test and clear in the common case.
>>  But this is probably better written as
>>
>>   if ( cpumask_test_cpu(cpu, ept->invalidate) )
>>   {
>>       cpumask_clear_cpu(cpu, ept->invalidate);
>>       __invept(...);
>>    }
> 
> The question is what you care for more: Avoiding the invalidation if
> you lose the race here, or avoiding the extra conditional. (Either is
> correct in this context afaict.)

There is no race since each pcpu clears its own bit.

David