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.
> >> --- a/xen/arch/x86/include/asm/paging.h
> >> +++ b/xen/arch/x86/include/asm/paging.h
> >> @@ -165,8 +165,9 @@ void paging_log_dirty_init(struct domain
> >>
> >> /* mark a page as dirty */
> >> void paging_mark_dirty(struct domain *d, mfn_t gmfn);
> >> -/* mark a page as dirty with taking guest pfn as parameter */
> >> +/* mark a page as dirty/clean with taking guest pfn as parameter */
> >
> > I think it would be clearer to use gfn here rather than "guest pfn",
> > and the function parameter should be "gfn_t gfn".
>
> For HVM I'd agree, but please see the one use for PV guests. As per
> xen/mm.h gfn == mfn for them, i.e. we particularly mean PFN there.
Sorry, I now realize. paging_mark_dirty() takes a plain MFN, while
paging_mark_pfn_dirty() takes a PFN.
Thanks, Roger.
