Re: [PATCH v3 1/2] xen/domain: introduce common hardware emulation flags

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 28 May 2025, dmkhn@xxxxxxxxx wrote:
> From: Denis Mukhin <dmukhin@xxxxxxxx>
> 
> Add common emulation_flags for configuring domain emulation features.
> 
> Print d->emulation_flags from 'q' keyhandler for better traceability while
> debugging.
> 
> Signed-off-by: Denis Mukhin <dmukhin@xxxxxxxx>
> ---
> Changes since v2:
> - move emulation_flags to common domain struct
> ---
>  xen/arch/x86/domain.c             |  2 +-
>  xen/arch/x86/domctl.c             |  2 +-
>  xen/arch/x86/include/asm/domain.h | 25 +++++++++++--------------
>  xen/common/keyhandler.c           |  1 +
>  xen/include/xen/sched.h           |  2 ++
>  5 files changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index 7536b6c871..0363ccb384 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -831,7 +831,7 @@ int arch_domain_create(struct domain *d,
>                 emflags);
>          return -EOPNOTSUPP;
>      }
> -    d->arch.emulation_flags = emflags;
> +    d->emulation_flags = emflags;
>  
>  #ifdef CONFIG_PV32
>      HYPERVISOR_COMPAT_VIRT_START(d) =
> diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
> index 3044f706de..37d848f683 100644
> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -144,7 +144,7 @@ void arch_get_domain_info(const struct domain *d,
>      if ( paging_mode_hap(d) )
>          info->flags |= XEN_DOMINF_hap;
>  
> -    info->arch_config.emulation_flags = d->arch.emulation_flags;
> +    info->arch_config.emulation_flags = d->emulation_flags;
>      info->gpaddr_bits = hap_paddr_bits;
>  }
>  
> diff --git a/xen/arch/x86/include/asm/domain.h 
> b/xen/arch/x86/include/asm/domain.h
> index 8c0dea12a5..eafd5cfc90 100644
> --- a/xen/arch/x86/include/asm/domain.h
> +++ b/xen/arch/x86/include/asm/domain.h
> @@ -455,9 +455,6 @@ struct arch_domain
>  
>      /* Don't unconditionally inject #GP for unhandled MSRs. */
>      bool msr_relaxed;
> -
> -    /* Emulated devices enabled bitmap. */
> -    uint32_t emulation_flags;
>  } __cacheline_aligned;
>  
>  #ifdef CONFIG_HVM
> @@ -494,17 +491,17 @@ struct arch_domain
>                                   X86_EMU_PIT | X86_EMU_USE_PIRQ |       \
>                                   X86_EMU_VPCI)
>  
> -#define has_vlapic(d)      (!!((d)->arch.emulation_flags & X86_EMU_LAPIC))
> -#define has_vhpet(d)       (!!((d)->arch.emulation_flags & X86_EMU_HPET))
> -#define has_vpm(d)         (!!((d)->arch.emulation_flags & X86_EMU_PM))
> -#define has_vrtc(d)        (!!((d)->arch.emulation_flags & X86_EMU_RTC))
> -#define has_vioapic(d)     (!!((d)->arch.emulation_flags & X86_EMU_IOAPIC))
> -#define has_vpic(d)        (!!((d)->arch.emulation_flags & X86_EMU_PIC))
> -#define has_vvga(d)        (!!((d)->arch.emulation_flags & X86_EMU_VGA))
> -#define has_viommu(d)      (!!((d)->arch.emulation_flags & X86_EMU_IOMMU))
> -#define has_vpit(d)        (!!((d)->arch.emulation_flags & X86_EMU_PIT))
> -#define has_pirq(d)        (!!((d)->arch.emulation_flags & X86_EMU_USE_PIRQ))
> -#define has_vpci(d)        (!!((d)->arch.emulation_flags & X86_EMU_VPCI))
> +#define has_vlapic(d)      (!!((d)->emulation_flags & X86_EMU_LAPIC))
> +#define has_vhpet(d)       (!!((d)->emulation_flags & X86_EMU_HPET))
> +#define has_vpm(d)         (!!((d)->emulation_flags & X86_EMU_PM))
> +#define has_vrtc(d)        (!!((d)->emulation_flags & X86_EMU_RTC))
> +#define has_vioapic(d)     (!!((d)->emulation_flags & X86_EMU_IOAPIC))
> +#define has_vpic(d)        (!!((d)->emulation_flags & X86_EMU_PIC))
> +#define has_vvga(d)        (!!((d)->emulation_flags & X86_EMU_VGA))
> +#define has_viommu(d)      (!!((d)->emulation_flags & X86_EMU_IOMMU))
> +#define has_vpit(d)        (!!((d)->emulation_flags & X86_EMU_PIT))
> +#define has_pirq(d)        (!!((d)->emulation_flags & X86_EMU_USE_PIRQ))
> +#define has_vpci(d)        (!!((d)->emulation_flags & X86_EMU_VPCI))
>  
>  #define gdt_ldt_pt_idx(v) \
>        ((v)->vcpu_id >> (PAGETABLE_ORDER - GDT_LDT_VCPU_SHIFT))
> diff --git a/xen/common/keyhandler.c b/xen/common/keyhandler.c
> index 0bb842ec00..cd731452ba 100644
> --- a/xen/common/keyhandler.c
> +++ b/xen/common/keyhandler.c
> @@ -306,6 +306,7 @@ static void cf_check dump_domains(unsigned char key)
>              if ( test_bit(i, &d->watchdog_inuse_map) )
>                  printk("    watchdog %d expires in %d seconds\n",
>                         i, (u32)((d->watchdog_timer[i].expires - NOW()) >> 
> 30));
> +        printk("    emulation_flags %#x\n", d->emulation_flags);
>  
>          arch_dump_domain_info(d);
>  
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> index 559d201e0c..dc4f917664 100644
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -651,6 +651,8 @@ struct domain
>      unsigned int num_llc_colors;
>      const unsigned int *llc_colors;
>  #endif
> +
> +    uint32_t emulation_flags;

I think it is a good idea to move emulation_flags to common and I think
we can make use of them on ARM too because we have a choice of emulators
on ARM as well (pl011, gicv2, gicv3, etc.). I think it makes sense to
move at least kinfo->arch.vpl011 to be an emulation_flags instead.

I was going to ask to add an #ifdef CONFIG_X86 around the
emulation_flags field as it is currently still unused on other
architectures, but then it would break the compilation of keyhandler.c.

So maybe it is OK this way.

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>


>  } __aligned(PAGE_SIZE);
>  
>  static inline struct page_list_head *page_to_list(
> -- 
> 2.34.1
> 
>