Re: [PATCH] x86/mm: split struct sh_dirty_vram and make results private

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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 <jbeulich@xxxxxxxx>

Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

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.