Re: [PATCH v2 3/5] x86: don't override INVALID_M2P_ENTRY with SHARED_M2P_ENTRY

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Mon, Aug 24, 2020 at 9:06 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 24.08.2020 15:00, Andrew Cooper wrote:
> > On 24/08/2020 13:34, Jan Beulich wrote:
> >> While in most cases code ahead of the invocation of set_gpfn_from_mfn()
> >> deals with shared pages, at least in set_typed_p2m_entry() I can't spot
> >> such handling (it's entirely possible there's code missing there). Let's
> >> try to play safe and add an extra check.
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> >
> > I agree that this is an improvement.
> >
> > Therefore, tentatively Acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>
> Thanks, but - what do I do with a tentative ack? Take it as a "normal"
> one, or not take it at all?
>
> > However, I don't think it is legitimate for set_gpfn_from_mfn() to be
> > overriding anything.
> >
> > IMO, we should be asserting something like (pfn == SHARED_M2P_ENTRY) ==
> > (d == dom_cow).
> >
> > Any code not passing this properly is almost certainly broken anyway,
> > and fixing up behind its back like this doesn't feel like a clever idea
> > (in debug builds at least).
>
> As said on v1: I agree in principle, but I'd like such a change to be
> made by the mem-sharing maintainer(s), so we wouldn't notice fallout
> only several months or years later. Tamas - would you be up for this?

Please feel free to add that ASSERT, if it does actually catch a
situation where it doesn't hold we'll fix it when it crosses our path.
It might indeed be several months/years before we get there. Currently
no bandwidth to check manually whether it triggers anything. Having
some CI tests would help with this for sure, but currently I only
check stuff like this by hand when we get to rc's.

Tamas