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

[Kai Huang <kaih.linux@xxxxxxxxx>]

On Mon, Mar 30, 2015 at 5:36 PM, Andrew Cooper
<andrew.cooper3@xxxxxxxxxx> wrote:
> 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@xxxxxxxxxxxxxxx>
>>>> ---
>>>>   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, &neThanks,
-Kaiw_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?

Yes shattered leaves will inherit both A and D bits from the original
superpage entry in " *epte = *ept_entry; " statement in below code in
ept_split_super_page.

    for ( int i = 0; i < EPT_PAGETABLE_ENTRIES; i++ )
    {
        ept_entry_t *epte = table + i;

        *epte = *ept_entry;
        ......

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

Sure I will  do in this way.

>
> ~Andrew
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel



-- 
Thanks,
-Kai

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel