Re: [PATCH for-4.7] xen/nested_p2m: Don't walk EPT tables with a regular PT walker

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

On 13/05/16 15:13, Jan Beulich wrote:
>>>> On 13.05.16 at 15:33, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/arch/x86/mm/hap/nested_hap.c
>> +++ b/xen/arch/x86/mm/hap/nested_hap.c
>> @@ -141,7 +141,7 @@ nestedhap_fix_p2m(struct vcpu *v, struct p2m_domain *p2m,
>>   * walk is successful, the translated value is returned in
>>   * L1_gpa. The result value tells what to do next.
>>   */
>> -static int
>> +int
>>  nestedhap_walk_L1_p2m(struct vcpu *v, paddr_t L2_gpa, paddr_t *L1_gpa,
>>                        unsigned int *page_order, uint8_t *p2m_acc,
> The function becoming non-static widens the visibility of the bogus
> uint8_t here (should be p2m_access_t afaics). Granted this isn't an
> issue the patch introduces, but I would prefer this to be adjusted
> prior to dropping the static here...
>
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -2081,20 +2081,29 @@ unsigned long paging_gva_to_gfn(struct vcpu *v,
>>  
>>      if ( is_hvm_vcpu(v) && paging_mode_hap(v->domain) && nestedhvm_is_n2(v) )
>>      {
>> -        unsigned long gfn;
>> +        unsigned long l2_gfn, l1_gfn;
>>          struct p2m_domain *p2m;
>>          const struct paging_mode *mode;
>> -        uint32_t pfec_21 = *pfec;
>>          uint64_t np2m_base = nhvm_vcpu_p2m_base(v);
>> +        uint8_t l1_p2ma;
> ... avoiding proliferation of the quirkiness.

Will do.

>
>> +        unsigned int l1_page_order;
>> +        int rv;
>>  
>>          /* translate l2 guest va into l2 guest gfn */
>>          p2m = p2m_get_nestedp2m(v, np2m_base);
>>          mode = paging_get_nestedmode(v);
>> -        gfn = mode->gva_to_gfn(v, p2m, va, pfec);
>> +        l2_gfn = mode->gva_to_gfn(v, p2m, va, pfec);
>> +
>> +        if ( l2_gfn == INVALID_GFN )
>> +            return INVALID_GFN;
>>  
>>          /* translate l2 guest gfn into l1 guest gfn */
>> -        return hostmode->p2m_ga_to_gfn(v, hostp2m, np2m_base,
>> -                                       gfn << PAGE_SHIFT, &pfec_21, NULL);
>> +        rv = nestedhap_walk_L1_p2m(v, l2_gfn, &l1_gfn, &l1_page_order, &l1_p2ma,
>> +                                   1,
>> +                                   !!(*pfec & PFEC_write_access),
>> +                                   !!(*pfec & PFEC_insn_fetch));
>> +
>> +        return rv == NESTEDHVM_PAGEFAULT_DONE ? l1_gfn : INVALID_GFN;
>>      }
> I'm really happy to see this getting cleaned up - I've stumbled across
> the apparent brokenness here more than once, without ever finding
> the time to help the situation. One question though: Is the return
> value here correct even for l1_page_order > 0?

Not completely sure.  This code is messy, and my current testing is "it
now doesn't crash where it used to crash before".

The specific path being tripped over was __hvm_copy() performing an
instruction fetch for emulation from the L2 guest.

Looking at the code, if your caller only cares about 4K mappings, you
will get the correct translation, due to the adjustment
nept_walk_tables() makes.

~Andrew

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