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

[Jan Beulich <jbeulich@suse.com>]

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.

Furthermore p2m_entry_modify() also isn't really suitable: We don't
know the GFN there.

>> --- 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.

Jan