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

["Penny, Zheng" <penny.zheng@xxxxxxx>]

[Public]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Wednesday, September 10, 2025 10:09 PM
> To: Penny, Zheng <penny.zheng@xxxxxxx>
> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>; Roger Pau Monné <roger.pau@xxxxxxxxxx>; xen-
> devel@xxxxxxxxxxxxxxxxxxxx
> 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 <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.
>

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