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

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

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?  Would it make sense to extend
ept_set_middle_entry() to do all of new_ept setup in one location?

> +
>      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)

> +
>      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?

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

~Andrew

>  
>  #define EPT_2MB_SHIFT     16
>  #define EPT_1GB_SHIFT     17