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

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

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

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