From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kai Huang Subject: Re: [PATCH 01/10] VMX: Enable EPT A/D bit support Date: Thu, 02 Apr 2015 14:32:17 +0800 Message-ID: <551CE271.7090807@linux.intel.com> References: <1427423754-11841-1-git-send-email-kai.huang@linux.intel.com> <1427423754-11841-2-git-send-email-kai.huang@linux.intel.com> <5515BFC5.4000109@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5515BFC5.4000109@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Andrew Cooper , jbeulich@suse.com, tim@xen.org, kevin.tian@intel.com, yang.z.zhang@intel.com, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org 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 >> --- >> 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) Hi Andrew, I'd like to also put patch 3 (PML feature detection) before this patch, as it's better to use cpu_has_vmx_pml to gate A/D bit enabling here. Theoretically simple "pml_enable = 1" here doesn't guarantee cpu_has_vmx_pml to be true, as PML may be turned off during vmx_init_vmcs_config. And in this case I also want to delete below code, as if PML is not enabled, below code will print but actually EPT A/D bits is not enabled in hardware. P(cpu_has_vmx_ept_ad, "EPT A/D bit"); Thanks, -Kai >> + >> 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 > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel