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

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

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(...);
   }

>> --- a/xen/arch/x86/mm/p2m-ept.c
>> +++ b/xen/arch/x86/mm/p2m-ept.c
>> @@ -1089,9 +1089,10 @@ static void ept_memory_type_changed(struct p2m_domain
>> *p2m)
>>
>>  static void __ept_sync_domain(void *info)
>>  {
>> -    struct ept_data *ept = &((struct p2m_domain *)info)->ept;
>> -
>> -    __invept(INVEPT_SINGLE_CONTEXT, ept_get_eptp(ept), 0);
>> +    /*
>> +     * The invalidate will be done before VMENTER (see
> 
> invalidate -> invalidation?

That would be the correct English grammer, yes.

David