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
