Re: [Xen-devel] [PATCH 1/3] x86: nuke PV superpage option and code

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Mon, Jul 24, 2017 at 05:18:41PM +0100, Andrew Cooper wrote:
> On 24/07/17 15:02, Wei Liu wrote:
> > @@ -1317,8 +1285,13 @@ static int put_page_from_l2e(l2_pgentry_t l2e, 
> > unsigned long pfn)
> >          return 1;
> >  
> >      if ( l2e_get_flags(l2e) & _PAGE_PSE )
> > -        put_superpage(l2e_get_pfn(l2e));
> > -    else
> > +    {
> > +        struct page_info *page = mfn_to_page(l2e_get_pfn(l2e));
> > +        unsigned int i;
> > +
> > +        for ( i = 0; i < (1u << PAGETABLE_ORDER); i++, page++ )
> > +            put_page_and_type(page);
> 
> With the removal of PV superpages, can this _PAGE_PSE check ever be hit?
> 

Yes. It is hit when dom0 boots. My first version of this patch had an
ASSERT(!(l2e_get_flags(l2e) & _PAGE_PSE)) and it was triggered.

> > +    } else
> >          put_page_and_type(l2e_get_page(l2e));
> >  
> >      return 0;
> > diff --git a/xen/include/asm-x86/guest_pt.h b/xen/include/asm-x86/guest_pt.h
> > index 72126d58d5..af18c90efa 100644
> > --- a/xen/include/asm-x86/guest_pt.h
> > +++ b/xen/include/asm-x86/guest_pt.h
> > @@ -210,7 +210,7 @@ static inline bool guest_can_use_l2_superpages(const 
> > struct vcpu *v)
> >       * It's also used in the dummy PT for vcpus with CR0.PG cleared.
> >       */
> >      return (is_pv_vcpu(v)
> > -            ? opt_allow_superpage
> > +            ? false
> 
> This raises an interesting question.  A PV guest likely has 2M
> superpages in the M2P mapping, irrespective of whether the PV guest is
> permitted to create its own 2M superpages or not.
> 
> In this context, I think it is a layering violation.  The purpose of
> guest_walk() is to match what hardware does, whereas this check is
> superimposing PV policy.
> 
> I will submit a patch changing this in isolation, along with suitable
> justification.
> 

OK.

> > diff --git a/xen/include/asm-x86/paging.h b/xen/include/asm-x86/paging.h
> > index 44e86d6a1f..6dcf5e9ca8 100644
> > --- a/xen/include/asm-x86/paging.h
> > +++ b/xen/include/asm-x86/paging.h
> > @@ -372,7 +372,7 @@ static inline unsigned int paging_max_paddr_bits(const 
> > struct domain *d)
> >      unsigned int bits = paging_mode_hap(d) ? hap_paddr_bits : paddr_bits;
> >  
> >      if ( !IS_ENABLED(BIGMEM) && paging_mode_shadow(d) &&
> > -         (!is_pv_domain(d) || opt_allow_superpage) )
> > +         !is_pv_domain(d) )
> 
> Reflow onto the previous line?
> 

NP.

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