Re: [Xen-devel] [PATCH 1/2] x86/mm: factor out the code for shattering an l3 PTE

["Xia, Hongyan" <hongyxia@xxxxxxxxxx>]

Hi Andrew,

On Fri, 2019-12-06 at 17:50 +0000, Andrew Cooper wrote:
> On 06/12/2019 15:53, Hongyan Xia wrote:
> > map_pages_to_xen and modify_xen_mappings are performing almost
> > exactly
> > the same operations when shattering an l3 PTE, the only difference
> > being whether we want to flush.
> > 
> > Signed-off-by: Hongyan Xia <hongyxia@xxxxxxxxxx>
> 
> Just for reviewing purposes, how does this relate to your other
> posted
> series.  Is it independent, a prerequisite, or does it depend on that
> series?

This is independent. Other series I posted will touch a lot of PTE
functions, and as Jan suggested, it would be nice to refactor some of
the long and complicated ones before changing them, which could also
prepare us for 5-level paging in the future. Although if these
refactoring patches get in, I need to rebase the series I posted
before.

> 
> > ---
> >  xen/arch/x86/mm.c | 85 ++++++++++++++++++++++---------------------
> > ----
> >  1 file changed, 40 insertions(+), 45 deletions(-)
> > 
> > diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> > index 7d4dd80a85..42aaaa1083 100644
> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> > @@ -5151,6 +5151,43 @@ l1_pgentry_t *virt_to_xen_l1e(unsigned long
> > v)
> >                           flush_area_local((const void *)v, f) : \
> >                           flush_area_all((const void *)v, f))
> >  
> > +/* Shatter an l3 entry and populate l2. If virt is passed in, also
> > do flush. */
> > +static void shatter_l3e(l3_pgentry_t *pl3e, l2_pgentry_t *l2t,
> > +        unsigned long virt, bool locking)
> > +{
> > +    unsigned int i;
> > +    l3_pgentry_t ol3e = *pl3e;
> > +
> > +    for ( i = 0; i < L2_PAGETABLE_ENTRIES; i++ )
> > +        l2e_write(l2t + i,
> > +                  l2e_from_pfn(l3e_get_pfn(ol3e) +
> > +                               (i << PAGETABLE_ORDER),
> > +                               l3e_get_flags(ol3e)));
> 
> The PTE macros are especially poor for generated asm, and in cases
> like
> this, I'd like to improve things.
> 
> In particular, I believe the following code has identical behaviour:
> 
> l2_pgentry_t nl2e = l2e_from_intpte(l3e_get_intpte(ol3e));
> 
> for ( i = 0; i < L2_PAGETABLE_ENTRIES; i++, nl2e.l2 +=
> PAGETABLE_ORDER )
>     l2e_write(l2t + i, nl2e);
> 
> (although someone please double check my logic) and rather better asm
> generation.  (I also expect there to be some discussion on whether
> using
> n2le.l2 directly is something we'd want to start doing.)
> 

I believe it should be nl2e.l2 += 1<<(PAGETABLE_ORDER+PAGE_SHIFT) ?
Although the code rarely touches the field (.l2) directly, so maybe use
the macros (get_intpte -> add -> from_intpte) for consistency? They
should produce the same code if the compiler is not too stupid.

> > +
> > +    if ( locking )
> > +        spin_lock(&map_pgdir_lock);
> > +    if ( (l3e_get_flags(ol3e) & _PAGE_PRESENT) &&
> > +         (l3e_get_flags(ol3e) & _PAGE_PSE) )
> 
> There is a subtle difference between the original code, and the
> refactored code, and it depends on the memory barrier from
> spin_lock().
> 
> Previously, it was re-read from memory after the lock, whereas now it
> is
> likely the stale version from before map_pgdir was locked.
> 
> Either you can go back to the old version and use *pl3e, or
> alternatively use:
> 
>     if ( locking )
>         spin_lock(&map_pgdir_lock);
>     ol3e = ACCESS_ONCE(*pl3e);
>     if ( ...
> 
> to make it clear that a reread from memory is required.
> 

Good point. Thanks.

> > +    {
> > +        l3e_write_atomic(pl3e, l3e_from_mfn(virt_to_mfn(l2t),
> 
> This would probably generate better asm by using the maddr variants
> so
> we don't have a double shift.
> 

Right. I can change that.

> > +                                            __PAGE_HYPERVISOR));
> > +        l2t = NULL;
> > +    }
> > +    if ( locking )
> > +        spin_unlock(&map_pgdir_lock);
> > +    if ( virt )
> > +    {
> > +        unsigned int flush_flags =
> > +            FLUSH_TLB | FLUSH_ORDER(2 * PAGETABLE_ORDER);
> > +
> > +        if ( (l3e_get_flags(ol3e) & _PAGE_GLOBAL) )
> > +                flush_flags |= FLUSH_TLB_GLOBAL;
> > +        flush_area(virt, flush_flags);
> > +    }
> > +    if ( l2t )
> > +        free_xen_pagetable(l2t);
> 
> This surely needs to NULL out l2t, just so the caller doesn't get any
> clever ideas and ends up with a use-after-free?
> 
> ~Andrew

hmm... if we want to make the nullification visible to the caller we
might need to pass &. I wonder if it makes sense to simply move the
memory allocation of pl2e into shatter_l3e as well, so that the caller
cannot have any ideas.

Thanks,
Hongyan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel