On Wed, Nov 12, 2025 at 04:47:31PM +0100, Jan Beulich wrote: > The types are local to the shadow and HAP subsystems respectively, and > HAP has no need for the shadow-specific fields (i.e. it can get away with > smaller allocations). In struct hvm_domain it therefore suffices to have > a union of two (generally opaque) pointers. > > Signed-off-by: Jan Beulich <[email protected]>
Acked-by: Roger Pau Monné <[email protected]> Just some minor suggestions below. > > --- a/xen/arch/x86/include/asm/hvm/domain.h > +++ b/xen/arch/x86/include/asm/hvm/domain.h > @@ -95,7 +95,10 @@ struct hvm_domain { > struct list_head pinned_cacheattr_ranges; > > /* VRAM dirty support. Protect with the domain paging lock. */ > - struct sh_dirty_vram *dirty_vram; > + union { > + struct sh_dirty_vram *sh; > + struct hap_dirty_vram *hap; > + } dirty_vram; Other in-place declared structures don't use this aligning. I have to admit it looks somewhat odd for structs like this one. > > /* If one of vcpus of this domain is in no_fill_mode or > * mtrr/pat between vcpus is not the same, set is_in_uc_mode > --- a/xen/arch/x86/include/asm/paging.h > +++ b/xen/arch/x86/include/asm/paging.h > @@ -133,19 +133,6 @@ struct paging_mode { > (DIV_ROUND_UP(PADDR_BITS - PAGE_SHIFT - (PAGE_SHIFT + 3), \ > PAGE_SHIFT - ilog2(sizeof(mfn_t))) + 1) > > -#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 > - > #if PG_log_dirty > > /* log dirty initialization */ > --- a/xen/arch/x86/mm/hap/hap.c > +++ b/xen/arch/x86/mm/hap/hap.c > @@ -36,6 +36,11 @@ > /* HAP VRAM TRACKING SUPPORT */ > /************************************************/ > > +struct hap_dirty_vram { > + unsigned long begin_pfn; > + unsigned long end_pfn; > +}; > + > /* > * hap_track_dirty_vram() > * Create the domain's dv_dirty_vram struct on demand. > @@ -52,7 +57,7 @@ int hap_track_dirty_vram(struct domain * > XEN_GUEST_HANDLE(void) guest_dirty_bitmap) > { > long rc = 0; > - struct sh_dirty_vram *dirty_vram; > + struct hap_dirty_vram *dirty_vram; > uint8_t *dirty_bitmap = NULL; > > if ( nr_frames ) > @@ -66,17 +71,17 @@ int hap_track_dirty_vram(struct domain * > > paging_lock(d); > > - dirty_vram = d->arch.hvm.dirty_vram; > + dirty_vram = d->arch.hvm.dirty_vram.hap; > if ( !dirty_vram ) > { > rc = -ENOMEM; > - if ( (dirty_vram = xzalloc(struct sh_dirty_vram)) == NULL ) > + if ( (dirty_vram = xzalloc(struct hap_dirty_vram)) == NULL ) > { > paging_unlock(d); > goto out; > } > > - d->arch.hvm.dirty_vram = dirty_vram; > + d->arch.hvm.dirty_vram.hap = dirty_vram; > } > > if ( begin_pfn != dirty_vram->begin_pfn || > @@ -132,7 +137,7 @@ int hap_track_dirty_vram(struct domain * > { > paging_lock(d); > > - dirty_vram = d->arch.hvm.dirty_vram; > + dirty_vram = d->arch.hvm.dirty_vram.hap; > if ( dirty_vram ) > { > /* > @@ -142,7 +147,7 @@ int hap_track_dirty_vram(struct domain * > begin_pfn = dirty_vram->begin_pfn; > nr_frames = dirty_vram->end_pfn - dirty_vram->begin_pfn; > xfree(dirty_vram); > - d->arch.hvm.dirty_vram = NULL; > + d->arch.hvm.dirty_vram.hap = NULL; > } > > paging_unlock(d); > @@ -630,7 +635,7 @@ void hap_teardown(struct domain *d, bool > > d->arch.paging.mode &= ~PG_log_dirty; > > - XFREE(d->arch.hvm.dirty_vram); > + XFREE(d->arch.hvm.dirty_vram.hap); > > out: > paging_unlock(d); > --- a/xen/arch/x86/mm/shadow/common.c > +++ b/xen/arch/x86/mm/shadow/common.c > @@ -2886,11 +2886,11 @@ void shadow_teardown(struct domain *d, b > d->arch.paging.mode &= ~PG_log_dirty; > > #ifdef CONFIG_HVM > - if ( is_hvm_domain(d) && d->arch.hvm.dirty_vram ) > + if ( is_hvm_domain(d) && d->arch.hvm.dirty_vram.sh ) > { > - xfree(d->arch.hvm.dirty_vram->sl1ma); > - xfree(d->arch.hvm.dirty_vram->dirty_bitmap); > - XFREE(d->arch.hvm.dirty_vram); > + xfree(d->arch.hvm.dirty_vram.sh->sl1ma); > + xfree(d->arch.hvm.dirty_vram.sh->dirty_bitmap); > + XFREE(d->arch.hvm.dirty_vram.sh); > } > #endif > > --- a/xen/arch/x86/mm/shadow/hvm.c > +++ b/xen/arch/x86/mm/shadow/hvm.c > @@ -1033,7 +1033,7 @@ int shadow_track_dirty_vram(struct domai > p2m_lock(p2m_get_hostp2m(d)); > paging_lock(d); > > - dirty_vram = d->arch.hvm.dirty_vram; > + dirty_vram = d->arch.hvm.dirty_vram.sh; > > if ( dirty_vram && (!nr_frames || > ( begin_pfn != dirty_vram->begin_pfn > @@ -1043,8 +1043,8 @@ int shadow_track_dirty_vram(struct domai > gdprintk(XENLOG_INFO, "stopping tracking VRAM %lx - %lx\n", > dirty_vram->begin_pfn, dirty_vram->end_pfn); > xfree(dirty_vram->sl1ma); > xfree(dirty_vram->dirty_bitmap); > - xfree(dirty_vram); > - dirty_vram = d->arch.hvm.dirty_vram = NULL; > + XFREE(dirty_vram); > + d->arch.hvm.dirty_vram.sh = NULL; It would be better if this was done the other way around, first set the reference to NULL, then free the memory? d->arch.hvm.dirty_vram.sh = NULL; XFREE(dirty_vram); > } > > if ( !nr_frames ) > @@ -1075,7 +1075,7 @@ int shadow_track_dirty_vram(struct domai > goto out; > dirty_vram->begin_pfn = begin_pfn; > dirty_vram->end_pfn = end_pfn; > - d->arch.hvm.dirty_vram = dirty_vram; > + d->arch.hvm.dirty_vram.sh = dirty_vram; > > if ( (dirty_vram->sl1ma = xmalloc_array(paddr_t, nr_frames)) == NULL > ) > goto out_dirty_vram; > @@ -1202,8 +1202,8 @@ int shadow_track_dirty_vram(struct domai > out_sl1ma: > xfree(dirty_vram->sl1ma); > out_dirty_vram: > - xfree(dirty_vram); > - dirty_vram = d->arch.hvm.dirty_vram = NULL; > + XFREE(dirty_vram); > + d->arch.hvm.dirty_vram.sh = NULL; Similar here, I would change the order. Thanks, Roger.
