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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.12.2025 10:01, Roger Pau Monné wrote:
> On Tue, Dec 09, 2025 at 12:49:59PM +0100, Jan Beulich wrote:
>> 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.
> 
> But the usage of p2m_entry_modify() is not about constructing a PTE,
> but getting notified on p2m changes, and any p2m entry change has an
> associated GFN.
> 
> IMO the point of p2m_entry_modify() is to put all the side-effects of
> p2m modifications into a single place, where it's easier to manage
> them.  I think the log-dirty stuff is suitable for being one of those
> p2m modification side-effects, but I'm not going to insist.

I can see your point, but I'm still having trouble seeing how exactly this
would work, especially as the function then would have to have enough
knowledge to choose between calling (at most) one of paging_mark_pfn_dirty()
and paging_mark_pfn_clean().

> I might take a look at this myself if I get some free time.

In the meantime I wonder if we couldn't get this in, and then your other
already pending patch on top.

Jan