Re: [RFC PATCH] x86/p2m-pt: do type recalculations with p2m read lock

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Apr 03, 2023 at 02:39:08PM +0200, Jan Beulich wrote:
> On 03.04.2023 12:14, Roger Pau Monne wrote:
> > --- a/xen/arch/x86/mm/p2m-pt.c
> > +++ b/xen/arch/x86/mm/p2m-pt.c
> > @@ -486,9 +486,6 @@ static int cf_check do_recalc(struct p2m_domain *p2m, 
> > unsigned long gfn)
> >          p2m_type_t ot, nt;
> >          unsigned long mask = ~0UL << (level * PAGETABLE_ORDER);
> >  
> > -        if ( !valid_recalc(l1, e) )
> > -            P2M_DEBUG("bogus recalc leaf at d%d:%lx:%u\n",
> > -                      p2m->domain->domain_id, gfn, level);
> >          ot = p2m_flags_to_type(l1e_get_flags(e));
> >          nt = p2m_recalc_type_range(true, ot, p2m, gfn & mask, gfn | ~mask);
> >          if ( nt != ot )
> 
> I'm afraid I neither understand why you make this change, nor why you
> then leave the other use of valid_recalc() in place.

The message can be bogus if we allow concurrent do_recalc(), and I
did miss the previous one.

I missed the one at the top.  Originally I wanted to send the RFC with
just changing the lock to read mode, but then I though I might as
well fix that (now bogus) print message.

> > @@ -538,9 +535,9 @@ int p2m_pt_handle_deferred_changes(uint64_t gpa)
> >       */
> >      ASSERT(!altp2m_active(current->domain));
> >  
> > -    p2m_lock(p2m);
> > +    p2m_read_lock(p2m);
> >      rc = do_recalc(p2m, PFN_DOWN(gpa));
> > -    p2m_unlock(p2m);
> > +    p2m_read_unlock(p2m);
> >  
> >      return rc;
> >  }
> 
> How can this be safe, when do_recalc() involves p2m_next_level(), which
> may install new (intermediate) page tables?

Oh, great, didn't realize it was capable of doing so, it's more hidden
than in the EPT case.  Seems like this will only happen if a superpage
needs to be split because a lower order frame is being used as an
ioreq server page.

Do you think it would be safe to try to attempt to perform the recalc
with the read lock only and fallback to the write lock if there's a
need to call p2m_next_level()?

Do you agree it might be possible to do the recalc with just the read
lock if it's updating of PTE type / recalc flags only?

Thanks, Roger.