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: Mon, 30 Mar 2015 14:11:21 +0800 Message-ID: <5518E909.6050303@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? 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