Re: [PATCH v2 03/26] xen/x86: consolidate vram tracking support

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.09.2025 09:38, 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.
> We release both from PG_log_dirty, and also move paging_log_dirty_range(),
> remamed with p2m_log_dirty_range(), into p2m.c, where it logically belongs.

Aren't these two independent changes? One to deal with struct sh_dirty_vram,
the other to move and rename paging_log_dirty_range()? Irrespective, in the
interest of making progress:
Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
with ...

> --- a/xen/arch/x86/include/asm/p2m.h
> +++ b/xen/arch/x86/include/asm/p2m.h
> @@ -1110,6 +1110,10 @@ static inline int p2m_entry_modify(struct p2m_domain 
> *p2m, p2m_type_t nt,
>  
>  #endif /* CONFIG_HVM */
>  
> +/* get the dirty bitmap for a specific range of pfns */

... comment style corrected here (happy to do so while committing).

Aiui the patch is independent of the earlier two, and hence could go in ahead
of them. Sadly once again nothing like this is stated anywhere, so please
confirm.

> --- a/xen/arch/x86/include/asm/paging.h
> +++ b/xen/arch/x86/include/asm/paging.h
> @@ -133,13 +133,20 @@ struct paging_mode {
>      (DIV_ROUND_UP(PADDR_BITS - PAGE_SHIFT - (PAGE_SHIFT + 3), \
>                    PAGE_SHIFT - ilog2(sizeof(mfn_t))) + 1)
>  
> -#if PG_log_dirty
> +#ifdef CONFIG_HVM
> +/* VRAM dirty tracking support */
> +struct sh_dirty_vram {
> +    unsigned long begin_pfn;
> +    unsigned long end_pfn;
> +#ifdef CONFIG_SHADOW_PAGING
> +    paddr_t *sl1ma;
> +    uint8_t *dirty_bitmap;
> +    s_time_t last_dirty;
> +#endif
> +};
> +#endif

Subsequently I think we will want to do more cleanup here. Us using a shadow
mode struct also in HAP code is bogus and, afaics, wasteful. The three latter
members are used only by shadow code, so HAP could have its own, smaller
variant of the type. And each type could be private to the hap/ and shadow/
subtrees respectively.

Jan