Re: [PATCH 1/3] x86/P2M: synchronize fast and slow paths of p2m_get_page_from_gfn()

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

On Tue, Mar 11, 2025 at 09:44:18AM +0100, Jan Beulich wrote:
> On 10.03.2025 15:53, Roger Pau Monné wrote:
> > On Wed, Feb 26, 2025 at 12:52:27PM +0100, Jan Beulich wrote:
> >> Handling of both grants and foreign pages was different between the two
> >> paths.
> >>
> >> While permitting access to grants would be desirable, doing so would
> >> require more involved handling; undo that for the time being. In
> >> particular the page reference obtained would prevent the owning domain
> >> from changing e.g. the page's type (after the grantee has released the
> >> last reference of the grant). Instead perhaps another reference on the
> >> grant would need obtaining. Which in turn would require determining
> >> which grant that was.
> >>
> >> Foreign pages in any event need permitting on both paths.
> >>
> >> Introduce a helper function to be used on both paths, such that
> >> respective checking differs in just the extra "to be unshared" condition
> >> on the fast path.
> >>
> >> While there adjust the sanity check for foreign pages: Don't leak the
> >> reference on release builds when on a debug build the assertion would
> >> have triggered. (Thanks to Roger for the suggestion.)
> >>
> >> Fixes: 80ea7af17269 ("x86/mm: Introduce get_page_from_gfn()")
> >> Fixes: 50fe6e737059 ("pvh dom0: add and remove foreign pages")
> >> Fixes: cbbca7be4aaa ("x86/p2m: make p2m_get_page_from_gfn() handle grant 
> >> case correctly")
> >> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> > 
> > Just a couple of nits below (with a reply to your RFC).
> > 
> >> ---
> >> RFC: While the helper could take const struct domain * as first
> >>      parameter, for a P2M function it seemed more natural to have it
> >>      take const struct p2m_domain *.
> >>
> >> --- a/xen/arch/x86/mm/p2m.c
> >> +++ b/xen/arch/x86/mm/p2m.c
> >> @@ -328,12 +328,45 @@ void p2m_put_gfn(struct p2m_domain *p2m,
> >>      gfn_unlock(p2m, gfn_x(gfn), 0);
> >>  }
> >>  
> >> +static struct page_info *get_page_from_mfn_and_type(
> >> +    const struct p2m_domain *p2m, mfn_t mfn, p2m_type_t t)
> > 
> > Re your RFC: since it's a static function, just used for
> > p2m_get_page_from_gfn(), I would consider passing a domain instead of
> > a p2m_domain, as the solely usage of p2m is to obtain the domain.
> 
> Okay, will do.
> 
> >> +{
> >> +    struct page_info *page;
> >> +
> >> +    if ( !mfn_valid(mfn) )
> >> +        return NULL;
> >> +
> >> +    page = mfn_to_page(mfn);
> >> +
> >> +    if ( p2m_is_ram(t) )
> > 
> > Should this be a likely() to speed up the common successful path?
> 
> Well. Andrew's general advice looks to be to avoid likely() / unlikely()
> in almost all situations. Therefore unless he positively indicates that
> in a case like this using likely() is acceptable, I'd rather stay away
> from adding that.

Oh, OK.  My suggestion was based on the use of unlikely below.  I
assume the later unlikely() in the patch is just inheritance from the
previous code.

> docs/process/coding-best-practices.pandoc could certainly do with some
> rough guidelines on when adding these two is acceptable (or even
> desirable). Right now to me it is largely unclear when their use is
> deemed okay; it certainly doesn't match anymore what I was told some
> 20 years ago when I started working on Linux.

I would be happy to have some guidance there, as I think I've been a
bit erratic with suggestions about how to use them.

Thanks, Roger.