On Mon, Aug 24, 2020 at 9:06 AM Jan Beulich <jbeul...@suse.com> 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 <jbeul...@suse.com> > > > > I agree that this is an improvement. > > > > Therefore, tentatively Acked-by: Andrew Cooper <andrew.coop...@citrix.com> > > 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
