From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH 01/10] VMX: Enable EPT A/D bit support Date: Mon, 30 Mar 2015 10:36:30 +0100 Message-ID: <5519191E.3050002@citrix.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> <5518E909.6050303@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <5518E909.6050303@linux.intel.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: Kai Huang , 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 30/03/15 07:11, Kai Huang wrote: > > > 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. Ok - so the middle doesn't have a D. What about the superpage having D set? Surely that needs propagated down to the new shattered leaves? >> 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) const ept_entry_t *old_entry (for consistency with other similar functions, or even just 'new' and 'old' as you are already changing the names) This looks fine. Being a static function with only two callsites, it is very likely to be inlined by the compiler. ~Andrew