On Tue, Dec 09, 2025 at 12:49:59PM +0100, Jan Beulich wrote:
> On 09.12.2025 12:34, Roger Pau Monné wrote:
> > On Mon, Dec 08, 2025 at 11:48:00AM +0100, Jan Beulich wrote:
> >> On 05.12.2025 14:53, Roger Pau Monné wrote:
> >>> On Tue, Apr 26, 2022 at 12:26:10PM +0200, Jan Beulich wrote:
> >>>> --- a/xen/arch/x86/mm/p2m.c
> >>>> +++ b/xen/arch/x86/mm/p2m.c
> >>>> @@ -549,7 +549,10 @@ p2m_remove_entry(struct p2m_domain *p2m,
> >>>> {
> >>>> p2m->get_entry(p2m, gfn_add(gfn, i), &t, &a, 0, NULL, NULL);
> >>>> if ( !p2m_is_special(t) && !p2m_is_shared(t) )
> >>>> + {
> >>>> set_gpfn_from_mfn(mfn_x(mfn) + i, INVALID_M2P_ENTRY);
> >>>> + paging_mark_pfn_clean(p2m->domain, _pfn(gfn_x(gfn) +
> >>>> i));
> >>>> + }
> >>>> }
> >>>> }
> >>>>
> >>>> @@ -737,8 +740,11 @@ p2m_add_page(struct domain *d, gfn_t gfn
> >>>> if ( !p2m_is_grant(t) )
> >>>> {
> >>>> for ( i = 0; i < (1UL << page_order); i++ )
> >>>> + {
> >>>> set_gpfn_from_mfn(mfn_x(mfn_add(mfn, i)),
> >>>> gfn_x(gfn_add(gfn, i)));
> >>>> + paging_mark_pfn_dirty(d, _pfn(gfn_x(gfn) + i));
> >>>
> >>> Have you considered placing the respective
> >>> paging_mark_pfn_{clean,dirty}() calls in p2m_entry_modify()?
> >>
> >> I didn't, but since you ask - I also don't think that's layering-wise
> >> an appropriate place for them to live. Whether a page has to be
> >> considered dirty needs determining elsewhere. No matter that ...
> >>
> >>> There's a lot of repetition here with regard to handling the side
> >>> effects of p2m changes that are forced into the callers, that could
> >>> likely be contained inside of p2m_entry_modify() at first sight.
> >>
> >> ... this way there is some redundancy.
> >
> > Redundancy is one of the aspects, the other being IMO code more prone
> > to errors. Having to do all this non-trivial extra work after a call
> > to set a p2m entry, both in the success and failure cases, seems
> > likely that it will be forgotten or incorrectly implemented by some
> > of the callers.
> >
> > It's you doing the work to fix this, so I'm not going to insist. It
> > seems a lot of extra complexity duplicated across multiple callers.
> >
> > FWIW, it would be easier to understand if we had the logic to mark
> > pages as dirty in a single place, rather than scattered around
> > different callers that do p2m modifications. For the time being I'm
> > fine with doing as you propose, but long term we should see about
> > cleaning this code IMO.
> >
> >> Furthermore p2m_entry_modify() also isn't really suitable: We don't
> >> know the GFN there.
> >
> > For one of the callers there's the GFN in context. For the EPT caller
> > it will likely require some plumbing.
>
> From a more abstract perspective, passing a GFN into that function would
> be wrong imo: Constructing a PTE may be unrelated to any particular GFN.
> The association with a GFN comes into play only when placing the PTE in
> a particular (live) page table.
But the usage of p2m_entry_modify() is not about constructing a PTE,
but getting notified on p2m changes, and any p2m entry change has an
associated GFN.
IMO the point of p2m_entry_modify() is to put all the side-effects of
p2m modifications into a single place, where it's easier to manage
them. I think the log-dirty stuff is suitable for being one of those
p2m modification side-effects, but I'm not going to insist.
I might take a look at this myself if I get some free time.
> Furthermore it's not quite clear what the conditions would be for
> p2m_entry_modify() to actually mark a page dirty. Only new type? Might
> old type also matter (so we wouldn't mark a page needlessly as dirty)?
> Yet other criteria? That's what callers will "know" (better?).
I've just taken a quick look, but callers from the p2m tree seem to
mark pages as dirty based on the new type.
Thanks, Roger.
