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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan