[Public]
> -----Original Message-----
> From: Jan Beulich <jbeul...@suse.com>
> Sent: Wednesday, September 10, 2025 10:09 PM
> To: Penny, Zheng <penny.zh...@amd.com>
> Cc: Huang, Ray <ray.hu...@amd.com>; Andrew Cooper
> <andrew.coop...@citrix.com>; Roger Pau Monné <roger....@citrix.com>; xen-
> de...@lists.xenproject.org
> Subject: Re: [PATCH v2 03/26] xen/x86: consolidate vram tracking support
>
> 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 <jbeul...@suse.com> 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.
>
Yes, it could go in ahead of them. I'll split it into two commits, and I will
do this immediately to send regardless of this patch serie.
> > --- 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.
>
Understood.
> Jan
