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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.05.16 at 17:00, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 13/05/16 15:13, Jan Beulich wrote:
>>>>> On 13.05.16 at 15:33, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> +        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.

Ah, indeed. But very odd for it to be done there, the more that "all
callers" is exactly one afaics. In that case may I suggest adding an
assertion to check that the respective bits in L1 and L2 GFN match?

Jan


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