Re: [PATCH v1 02/25] xen/x86: consolidate vram tracking support

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.08.2025 11:47, Penny Zheng wrote:
> Flag PG_log_dirty is for paging log dirty support, not vram tracking support.
> However data structure sh_dirty_vram{} and function paging_log_dirty_range()
> designed for vram tracking support, are guarded with PG_log_dirty.

The struct decl is guarded by both, which is fine, merely a bit redundant.
I'm okay with moving it if that helps elsewhere.

> We release both from PG_log_dirty, and also move paging_log_dirty_range() into
> hap.c, to make it static.

This, I think, wants doing separately. paging.c isn't quite a correct home
for it, yes, but neither is hap/hap.c. This is noticeable first and foremost
from ...

> --- a/xen/arch/x86/mm/hap/hap.c
> +++ b/xen/arch/x86/mm/hap/hap.c
> @@ -36,6 +36,38 @@
>  /*          HAP VRAM TRACKING SUPPORT           */
>  /************************************************/
>  
> +#ifdef CONFIG_HVM
> +static void paging_log_dirty_range(struct domain *d,
> +                                   unsigned long begin_pfn,
> +                                   unsigned long nr,
> +                                   uint8_t *dirty_bitmap)
> +{
> +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
> +    int i;
> +    unsigned long pfn;
> +
> +    /*
> +     * Set l1e entries of P2M table to be read-only.
> +     *
> +     * On first write, it page faults, its entry is changed to read-write,
> +     * and on retry the write succeeds.
> +     *
> +     * We populate dirty_bitmap by looking for entries that have been
> +     * switched to read-write.
> +     */
> +
> +    p2m_lock(p2m);

... the lock it uses. This function really wants to move to p2m.c imo, and
be suitably renamed (s/paging_/p2m_/). While there please also consider
switching i to unsigned int.

> +    for ( i = 0, pfn = begin_pfn; pfn < begin_pfn + nr; i++, pfn++ )
> +        if ( !p2m_change_type_one(d, pfn, p2m_ram_rw, p2m_ram_logdirty) )
> +            dirty_bitmap[i >> 3] |= (1 << (i & 7));
> +
> +    p2m_unlock(p2m);
> +
> +    guest_flush_tlb_mask(d, d->dirty_cpumask);
> +}
> +#endif /* CONFIG_HVM */

This #ifdef / #endif isn't needed. Neither when it lives in hap/hap.c
(the parent dir's Makefile has

obj-$(CONFIG_HVM) += hap/

) nor when in p2m.c. The function then still not being non-static is
acceptable; it living where it logically belongs is more important imo.

Jan