Re: [PATCH v3 7/8] x86/mm: update log-dirty bitmap when manipulating P2M

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Dec 10, 2025 at 10:55:50AM +0100, Jan Beulich wrote:
> On 10.12.2025 10:42, Roger Pau Monné wrote:
> > On Tue, Apr 26, 2022 at 12:26:10PM +0200, Jan Beulich wrote:
> >> Just like for PV guests MMU_MACHPHYS_UPDATE implies marking of the
> >> respective page as dirty, additions to a HVM guest's P2M should do so.
> >>
> >> For HVM the opposite is also true: Pages being removed from the P2M are
> >> no longer dirty at their prior GFN; there's no point in telling the tool
> >> stack to try and copy that page, when this will fail anyway (until
> >> perhaps a new page gets placed there). Introduce paging_mark_pfn_clean()
> >> (intentionally without a paging_mark_clean() counterpart) to handle
> >> this. Note that while there is an earlier call to set_gpfn_from_mfn() in
> >> guest_physmap_add_entry(), but there's little reason to mark the page
> >> clean there when later in the function it'll be marked dirty. This is
> >> even more so given that at this point it's only the M2P that gets
> >> updated, with the P2M still left unchanged.
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> > 
> > Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> 
> Thanks.
> 
> >> --- a/xen/arch/x86/mm/paging.c
> >> +++ b/xen/arch/x86/mm/paging.c
> >> @@ -260,7 +260,7 @@ static int paging_log_dirty_disable(stru
> >>  }
> >>  
> >>  /* Mark a page as dirty, with taking guest pfn as parameter */
> >> -void paging_mark_pfn_dirty(struct domain *d, pfn_t pfn)
> >> +static void mark_pfn_dirty(struct domain *d, pfn_t pfn, bool dirty)
> > 
> > Nit: set_pfn_logdirty() or similar?  The function name makes it look like
> > it's marking the pfn as dirty (when it's also used to clear it).
> > 
> > No strong opinion, it just seems to me the name is slightly confusing.
> 
> Changed. While making the change I noticed that I had one other change in
> there for a possible v4. This is the extra hunk:
> 
> @@ -260,7 +260,7 @@ void paging_mark_pfn_dirty(struct domain
>          return;
>  
>      /* Shared MFNs should NEVER be marked dirty */
> -    BUG_ON(paging_mode_translate(d) && SHARED_M2P(pfn_x(pfn)));
> +    BUG_ON(dirty && paging_mode_translate(d) && SHARED_M2P(pfn_x(pfn)));
>  
>      /*
>       * Values with the MSB set denote MFNs that aren't really part of the
> 
> I hope that won't invalidate your R-b.

No, that's fine, please keep the RB.

Thanks, Roger.