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 15:33, <andrew.cooper3@xxxxxxxxxx> 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.

> +        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?

Jan


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