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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.02.2026 18:35, Roger Pau Monné wrote:
> 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>

Thanks.

>> --- 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.

As I don't mind much, I've changed it. It's odd either way - either, as you say,
it looks odd, or it's not in line with the other field decls which all align
their names (except for the in-place struct decls).

>> --- 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);

Indeed, changed.

>> @@ -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.

Changed as well. I can't tell why I didn't do it this way right away.

Jan