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

[Jan Beulich <jbeulich@suse.com>]

On 29.11.2022 17:44, Roger Pau Monné wrote:
> 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@suse.com>
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

>> ---
>> 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,

Hence my question, rather than making a patch right away. I was
hoping that maybe someone might see or recall why such a check would
have been put there.

I'm not certain enough to put ASSERT_UNREACHABLE() there, though. I
might make it a one-time warning instead.

> I have to admit I
> sometimes find the p2m code difficult to follow.

You're not the only one.

Jan