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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

Jan