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

[dmkhn@xxxxxxxxx]

On Thu, Jun 05, 2025 at 11:08:07AM +0200, Roger Pau Monné wrote:
> On Mon, Jun 02, 2025 at 07:17:30PM +0000, 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>
> > Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > ---
> > Changes since v4:
> > - kept Stefano's R-b
> > ---
> >  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);
> 
> No strong opinion, but my preference would be to print those as
> strings if possible, ie:
> 
> printk("    emulation_flags: %s%s%s...(%#x)\n",
>        !d->emulation_flags ? "none " : "",
>        has_vlapic(d) ? "vlapic " : "",
>        has_vhpet(d) ? "hpet " : "",
>        ...,
>        d->emulation_flags);

Thanks for suggestion.

There was the same feedback already:
  https://lore.kernel.org/xen-devel/aDd9Z3eY3RQgTTdy@kraken/

Is it OK to address it in the follow on change for both Arm and x86?

> 
> Thanks, Roger.