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

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

[Public]

> -----Original Message-----
> From: Penny, Zheng
> Sent: Thursday, September 11, 2025 3:16 PM
> To: Jan Beulich <jbeulich@xxxxxxxx>
> 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
>
>
>
> > -----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.

Reading relative codes, found that we have a "struct sh_dirty_vram *dirty_vram" 
in "struct hvm_domain",
If we defined different type "struct hap_dirty_vram" and "struct sh_dirty_vram" 
private to the hap/ and shadow/ subtrees respectively, either we add different 
type in "struct hvm_domain", or we change it to the "void *" there and do the 
type casting on referring... maybe the former is safer or any better suggestion?

>
> > Jan