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

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

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.

> +{
> +    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?

Thanks, Roger.