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

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

On Fri, 2019-12-13 at 14:58 +0000, Andrew Cooper wrote:
> On 13/12/2019 14:36, Jan Beulich wrote:
> > On 13.12.2019 15:19, Andrew Cooper wrote:
> > > On 12/12/2019 12:46, Hongyan Xia wrote:
> > > > diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> > > > index 7d4dd80a85..8def4fb8d8 100644
> > > > --- a/xen/arch/x86/mm.c
> > > > +++ b/xen/arch/x86/mm.c
> > > > @@ -5151,6 +5151,52 @@ 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 bool shatter_l3e(l3_pgentry_t *pl3e, unsigned long
> > > > virt, bool locking)
> > > > +{
> > > > +    unsigned int i;
> > > > +    l3_pgentry_t ol3e = *pl3e;
> > > > +    l2_pgentry_t l2e = l2e_from_intpte(l3e_get_intpte(ol3e));
> > > > +    l2_pgentry_t *l2t = alloc_xen_pagetable();
> > > > +
> > > > +    if ( !l2t )
> > > > +        return false;
> > > > +
> > > > +    for ( i = 0; i < L2_PAGETABLE_ENTRIES; i++ )
> > > > +    {
> > > > +        l2e_write(l2t + i, l2e);
> > > > +        l2e = l2e_from_intpte(
> > > > +                  l2e_get_intpte(l2e) + (PAGE_SIZE <<
> > > > PAGETABLE_ORDER));
> > > > +    }
> > > > +
> > > > +    if ( locking )
> > > > +        spin_lock(&map_pgdir_lock);
> > > > +    if ( (l3e_get_flags(*pl3e) & _PAGE_PRESENT) &&
> > > > +         (l3e_get_flags(*pl3e) & _PAGE_PSE) )
> > > > +    {
> > > > +        l3e_write_atomic(pl3e,
> > > > +            l3e_from_paddr(virt_to_maddr(l2t),
> > > > __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;
> > > 
> > > Another problematic use of ol3e which is racy on conflict.  You
> > > need to
> > > strictly use the content of *pl3e from within the locked region.
> > 
> > But this isn't a problem introduced here, i.e. fixing of it doesn't
> > strictly fall under "re-factor". (I'm certainly not opposed to
> > getting this right at the same time.)
> 
> It is brand new code which is racy.  Its either not necessary, or an
> XSA-in-waiting.  (And not necessary, AFAICT).
> 

I am really confused. The original code already does sub-region
flushes, and it uses a flush flag from ol3e that is even more outdated
than the refactored version, so I am not quite getting your point. I
hope I am not missing something obvious.

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