Re: [PATCH] x86/p2m: don't calculate page owner twice in p2m_add_page()

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

On Tue, Nov 29, 2022 at 03:47:53PM +0100, Jan Beulich wrote:
> Neither page_get_owner() nor mfn_to_page() are entirely trivial
> operations - don't do the same thing twice in close succession. Instead
> help CSE (when MEM_SHARING=y) by introducing a local variable holding
> the page owner.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

> ---
> According to my observations gcc12 manages to CSE mfn_to_page() but not
> (all of) page_get_owner(). The overall savings there are, partly due to
> knock-on effects, 64 bytes of code.
> 
> While looking there, "mfn_eq(omfn, mfn_add(mfn, i))" near the end of the
> same loop caught my eye: Is that really correct? Shouldn't we fail the
> operation if the MFN which "ogfn" was derived from doesn't match the MFN
> "ogfn" maps to?

Getting into that state possibly means something has gone wrong if we
have rules out grants and foreign maps?

So it should be:

if ( !mfn_eq(omfn, mfn_add(mfn, i)) )
{
    /* Something has gone wrong, ASSERT_UNREACHABLE()? */
    goto out;
}
rc = p2m_remove_entry(p2m, ogfn, omfn, 0)
if ( rc )
    goto out;

but maybe I'm missing the point of the check there, I have to admit I
sometimes find the p2m code difficult to follow.

Thanks, Roger.