Re: [PATCH 01/10] VMX: Enable EPT A/D bit support

[Kai Huang <kai.huang@linux.intel.com>]

On 03/28/2015 04:38 AM, Andrew Cooper wrote:
> On 27/03/15 02:35, Kai Huang wrote:
>> PML requires A/D bit support so enable it for further use.
>>
>> Signed-off-by: Kai Huang <kai.huang@linux.intel.com>
>> ---
>>   xen/arch/x86/hvm/vmx/vmcs.c        | 1 +
>>   xen/arch/x86/mm/p2m-ept.c          | 8 +++++++-
>>   xen/include/asm-x86/hvm/vmx/vmcs.h | 4 +++-
>>   xen/include/asm-x86/hvm/vmx/vmx.h  | 5 ++++-
>>   4 files changed, 15 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
>> index d614638..2f645fe 100644
>> --- a/xen/arch/x86/hvm/vmx/vmcs.c
>> +++ b/xen/arch/x86/hvm/vmx/vmcs.c
>> @@ -103,6 +103,7 @@ static void __init vmx_display_features(void)
>>       P(cpu_has_vmx_tpr_shadow, "APIC TPR shadow");
>>       P(cpu_has_vmx_ept, "Extended Page Tables (EPT)");
>>       P(cpu_has_vmx_vpid, "Virtual-Processor Identifiers (VPID)");
>> +    P(cpu_has_vmx_ept_ad_bit, "EPT A/D bit");
>>       P(cpu_has_vmx_vnmi, "Virtual NMI");
>>       P(cpu_has_vmx_msr_bitmap, "MSR direct-access bitmap");
>>       P(cpu_has_vmx_unrestricted_guest, "Unrestricted Guest");
>> diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
>> index c2d7720..8650092 100644
>> --- a/xen/arch/x86/mm/p2m-ept.c
>> +++ b/xen/arch/x86/mm/p2m-ept.c
>> @@ -233,6 +233,9 @@ static int ept_split_super_page(struct p2m_domain *p2m, ept_entry_t *ept_entry,
>>       if ( !ept_set_middle_entry(p2m, &new_ept) )
>>           return 0;
>>   
>> +    /* It's better to copy A bit of Middle entry from original entry */
>> +    new_ept.a = ept_entry->a;
> Surely d needs to be propagated as well?
No it's not necessary. D-bit is not defined in middle level EPT table. 
Only leaf table entry has D-bit definition.
> Would it make sense to extend
> ept_set_middle_entry() to do all of new_ept setup in one location?
Yes it certainly makes sense to move A-bit propagation into 
ept_set_middle_entry, but this also requires adding additional original 
EPT entry pointer to ept_set_middle_entry as parameter. And 
ept_set_middle_entry is also called by ept_next_level, therefore 
changing it requires more code change, something like below. While I am 
fine with both, which solution do you prefer?

+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -208,7 +208,8 @@ static void ept_p2m_type_to_flags(struct p2m_domain 
*p2m, ept_entry_t *entry,
  #define GUEST_TABLE_POD_PAGE    3

  /* Fill in middle levels of ept table */
-static int ept_set_middle_entry(struct p2m_domain *p2m, ept_entry_t 
*ept_entry)
+static int ept_set_middle_entry(struct p2m_domain *p2m, ept_entry_t 
*new_entry,
+        ept_entry_t *ori_entry)
  {
      struct page_info *pg;

@@ -216,11 +217,13 @@ static int ept_set_middle_entry(struct p2m_domain 
*p2m, ept_entry_t *ept_entry)
      if ( pg == NULL )
          return 0;

-    ept_entry->epte = 0;
-    ept_entry->mfn = page_to_mfn(pg);
-    ept_entry->access = p2m->default_access;
+    new_entry->epte = 0;
+    new_entry->mfn = page_to_mfn(pg);
+    new_entry->access = p2m->default_access;

-    ept_entry->r = ept_entry->w = ept_entry->x = 1;
+    new_entry->r = new_entry->w = new_entry->x = 1;
+
+    new_entry->a = ori_entry->a;

      return 1;
  }
@@ -257,7 +260,7 @@ static int ept_split_super_page(struct p2m_domain 
*p2m, ept_entry_t *ept_entry,

      ASSERT(is_epte_superpage(ept_entry));

-    if ( !ept_set_middle_entry(p2m, &new_ept) )
+    if ( !ept_set_middle_entry(p2m, &new_ept, ept_entry) )
          return 0;

      table = map_domain_page(new_ept.mfn);
@@ -337,7 +340,7 @@ static int ept_next_level(struct p2m_domain *p2m, 
bool_t read_only,
          if ( read_only )
              return GUEST_TABLE_MAP_FAILED;

-        if ( !ept_set_middle_entry(p2m, ept_entry) )
+        if ( !ept_set_middle_entry(p2m, ept_entry, &e) )
              return GUEST_TABLE_MAP_FAILED;
          else
              e = atomic_read_ept_entry(ept_entry); /* Refresh */
>
>> +
>>       table = map_domain_page(new_ept.mfn);
>>       trunk = 1UL << ((level - 1) * EPT_TABLE_ORDER);
>>   
>> @@ -244,7 +247,7 @@ static int ept_split_super_page(struct p2m_domain *p2m, ept_entry_t *ept_entry,
>>           epte->sp = (level > 1);
>>           epte->mfn += i * trunk;
>>           epte->snp = (iommu_enabled && iommu_snoop);
>> -        ASSERT(!epte->rsvd1);
>> +        /* A/D bits are inherited from superpage */
>>           ASSERT(!epte->avail3);
>>   
>>           ept_p2m_type_to_flags(epte, epte->sa_p2mt, epte->access);
>> @@ -1071,6 +1074,9 @@ int ept_p2m_init(struct p2m_domain *p2m)
>>       /* set EPT page-walk length, now it's actual walk length - 1, i.e. 3 */
>>       ept->ept_wl = 3;
>>   
>> +    /* Enable EPT A/D bit if it's supported by hardware */
>> +    ept->ept_ad = cpu_has_vmx_ept_ad_bit ? 1 : 0;
> This will incur overhead on all EPT operations.  It should only be
> enabled if pml is going to be in use.  (I think you need reverse patches
> 1 and 2 in the series, and gate on pml_enable here)
Sure. Will do.

>
>> +
>>       if ( !zalloc_cpumask_var(&ept->synced_mask) )
>>           return -ENOMEM;
>>   
>> diff --git a/xen/include/asm-x86/hvm/vmx/vmcs.h b/xen/include/asm-x86/hvm/vmx/vmcs.h
>> index 6fce6aa..4528346 100644
>> --- a/xen/include/asm-x86/hvm/vmx/vmcs.h
>> +++ b/xen/include/asm-x86/hvm/vmx/vmcs.h
>> @@ -62,7 +62,8 @@ struct ept_data {
>>       struct {
>>               u64 ept_mt :3,
>>                   ept_wl :3,
>> -                rsvd   :6,
>> +                ept_ad :1,
>> +                rsvd   :5,
>>                   asr    :52;
> While you are making this change, can you add comments similar to
> ept_entry_t describing the bits?
Sure. I can add a comment for 'ept_ad' bit here. I didn't do it as 
there's no comments for other bits neither.

>
>>           };
>>           u64 eptp;
>> @@ -226,6 +227,7 @@ extern u32 vmx_secondary_exec_control;
>>   #define VMX_EPT_INVEPT_INSTRUCTION              0x00100000
>>   #define VMX_EPT_INVEPT_SINGLE_CONTEXT           0x02000000
>>   #define VMX_EPT_INVEPT_ALL_CONTEXT              0x04000000
>> +#define VMX_EPT_AD_BIT_SUPPORT                  0x00200000
>>   
>>   #define VMX_MISC_VMWRITE_ALL                    0x20000000
>>   
>> diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h b/xen/include/asm-x86/hvm/vmx/vmx.h
>> index 91c5e18..9afd351 100644
>> --- a/xen/include/asm-x86/hvm/vmx/vmx.h
>> +++ b/xen/include/asm-x86/hvm/vmx/vmx.h
>> @@ -37,7 +37,8 @@ typedef union {
>>           emt         :   3,  /* bits 5:3 - EPT Memory type */
>>           ipat        :   1,  /* bit 6 - Ignore PAT memory type */
>>           sp          :   1,  /* bit 7 - Is this a superpage? */
>> -        rsvd1       :   2,  /* bits 9:8 - Reserved for future use */
>> +        a           :   1,  /* bit 8 - Access bit */
>> +        d           :   1,  /* bit 9 - Dirty bit */
>>           recalc      :   1,  /* bit 10 - Software available 1 */
>>           snp         :   1,  /* bit 11 - VT-d snoop control in shared
>>                                  EPT/VT-d usage */
>> @@ -261,6 +262,8 @@ extern uint8_t posted_intr_vector;
>>       (vmx_ept_vpid_cap & VMX_EPT_SUPERPAGE_2MB)
>>   #define cpu_has_vmx_ept_invept_single_context   \
>>       (vmx_ept_vpid_cap & VMX_EPT_INVEPT_SINGLE_CONTEXT)
>> +#define cpu_has_vmx_ept_ad_bit                  \
>> +    (vmx_ept_vpid_cap & VMX_EPT_AD_BIT_SUPPORT)
> I think cpu_has_vmx_ept_ad is sufficient, without the _bit suffix making
> it longer.
Sure. Will do.

Thanks,
-Kai
>
> ~Andrew
>
>>   
>>   #define EPT_2MB_SHIFT     16
>>   #define EPT_1GB_SHIFT     17
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel