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 <jbeul...@suse.com>
>
> Acked-by: Roger Pau Monné <roger....@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
