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
