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

["Roger Pau Monné" <roger.pau@citrix.com>]

On Mon, Apr 03, 2023 at 05:30:37PM +0200, Jan Beulich wrote:
> On 03.04.2023 16:27, Roger Pau Monné wrote:
> > 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()?
> 
> Yes, that ought to be okay.
> 
> > 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?
> 
> Technically this looks to be possible, yes. Question is whether we do
> ourselves much good by introducing such a special case of permitting
> a certain kind of writes with the lock only held in read mode. The
> latest when we find a second (more or less similar) use case thing
> are likely to become difficult.

Yes, it's not very nice.  I'm open to suggestions of other ways to
remove some of the contention.  If we go down this route it would need
to be clearly documented.

Thanks, Roger.