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 <[email protected]>
>
> Acked-by: Roger Pau Monné <[email protected]>
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
