[PATCH v5 0/2] xen/domain: updates to hardware emulation flags

[PATCH v5 0/2] xen/domain: updates to hardware emulation flags

[dmkhn]

Patch 1 introduces emulation_flags in common domain struct for enabling domain
emulation features on non-x86 platforms.

Patch 2 rewrites emulation_flags_ok() on x86 with a goal of improving
readability and maintainability of the code.

Originally, the code was part of [1], part of NS16550 emulator v3 series.

[1] https://lore.kernel.org/xen-devel/20250103-vuart-ns8250-v3-v1-6-c5d36b31d66c@ford.com/
[2] Link to v4: https://lore.kernel.org/xen-devel/20250530220242.63175-1-dmukhin@ford.com/
[3] Link to CI: https://gitlab.com/xen-project/people/dmukhin/xen/-/pipelines/1849512714

Denis Mukhin (2):
  xen/domain: introduce common hardware emulation flags
  xen/domain: rewrite emulation_flags_ok()

 xen/arch/x86/domain.c             | 93 ++++++++++++++++++++++++-------
 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, 89 insertions(+), 34 deletions(-)

-- 
2.34.1

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

[dmkhn]

From: Denis Mukhin <dmukhin@ford.com>

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@ford.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
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);
 
         arch_dump_domain_info(d);
 
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index b17aada5f5..1393923986 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;
 } __aligned(PAGE_SIZE);
 
 static inline struct page_list_head *page_to_list(
-- 
2.34.1

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

[Jan Beulich]

On 02.06.2025 21:17, dmkhn@proton.me wrote:
> From: Denis Mukhin <dmukhin@ford.com>
> 
> 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@ford.com>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

It's not becoming clear why this would want doing, nor in how far some of
the bits there may gain "common" meaning, too. Imo this kind of change is
meaningful only in a series where later the common-ness is also used.

Jan

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

[dmkhn]

On Wed, Jun 04, 2025 at 12:36:17PM +0200, Jan Beulich wrote:
> On 02.06.2025 21:17, dmkhn@proton.me wrote:
> > From: Denis Mukhin <dmukhin@ford.com>
> >
> > 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@ford.com>
> > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> It's not becoming clear why this would want doing, nor in how far some of
> the bits there may gain "common" meaning, too. Imo this kind of change is
> meaningful only in a series where later the common-ness is also used.

I have a set of upcoming changes here:
  https://gitlab.com/xen-project/people/dmukhin/xen/-/tree/vuart-framework?ref_type=heads

Specifically,
 https://gitlab.com/xen-project/people/dmukhin/xen/-/commit/17f44d23c1904374963c4ec839bc8219041b5d26

enables the use of emulation_flags on Arm.

I can move this patch to the upcoming series, if needed, but looks like it is
OK to have it for Arm even now.

> 
> Jan

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

[Jan Beulich]

On 05.06.2025 03:17, dmkhn@proton.me wrote:
> On Wed, Jun 04, 2025 at 12:36:17PM +0200, Jan Beulich wrote:
>> On 02.06.2025 21:17, dmkhn@proton.me wrote:
>>> From: Denis Mukhin <dmukhin@ford.com>
>>>
>>> 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@ford.com>
>>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>>
>> It's not becoming clear why this would want doing, nor in how far some of
>> the bits there may gain "common" meaning, too. Imo this kind of change is
>> meaningful only in a series where later the common-ness is also used.
> 
> I have a set of upcoming changes here:
>   https://gitlab.com/xen-project/people/dmukhin/xen/-/tree/vuart-framework?ref_type=heads
> 
> Specifically,
>  https://gitlab.com/xen-project/people/dmukhin/xen/-/commit/17f44d23c1904374963c4ec839bc8219041b5d26
> 
> enables the use of emulation_flags on Arm.
> 
> I can move this patch to the upcoming series, if needed, but looks like it is
> OK to have it for Arm even now.

Prior to that series making it to a ready-to-be-committed state, it'll be
(effectively) dead code on Arm. Which strictly speaking Misra objects to.
I wonder if you, Stefano, considered that when giving your R-b.

Further - what about PPC and RISC-V?

Jan

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

[Stefano Stabellini]

On Thu, 5 Jun 2025, Jan Beulich wrote:
> On 05.06.2025 03:17, dmkhn@proton.me wrote:
> > On Wed, Jun 04, 2025 at 12:36:17PM +0200, Jan Beulich wrote:
> >> On 02.06.2025 21:17, dmkhn@proton.me wrote:
> >>> From: Denis Mukhin <dmukhin@ford.com>
> >>>
> >>> 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@ford.com>
> >>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> >>
> >> It's not becoming clear why this would want doing, nor in how far some of
> >> the bits there may gain "common" meaning, too. Imo this kind of change is
> >> meaningful only in a series where later the common-ness is also used.
> > 
> > I have a set of upcoming changes here:
> >   https://gitlab.com/xen-project/people/dmukhin/xen/-/tree/vuart-framework?ref_type=heads
> > 
> > Specifically,
> >  https://gitlab.com/xen-project/people/dmukhin/xen/-/commit/17f44d23c1904374963c4ec839bc8219041b5d26
> > 
> > enables the use of emulation_flags on Arm.
> > 
> > I can move this patch to the upcoming series, if needed, but looks like it is
> > OK to have it for Arm even now.
> 
> Prior to that series making it to a ready-to-be-committed state, it'll be
> (effectively) dead code on Arm. Which strictly speaking Misra objects to.
> I wonder if you, Stefano, considered that when giving your R-b.

We have not been enforcing the "no dead code" rule (R2.2) especially on
a per patch basis. In general in my view, it is better to reduce dead
code rather than increase dead code, but it is OK to do so temporarily.

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

[dmkhn]

On Thu, Jun 05, 2025 at 08:44:30AM +0200, Jan Beulich wrote:
> On 05.06.2025 03:17, dmkhn@proton.me wrote:
> > On Wed, Jun 04, 2025 at 12:36:17PM +0200, Jan Beulich wrote:
> >> On 02.06.2025 21:17, dmkhn@proton.me wrote:
> >>> From: Denis Mukhin <dmukhin@ford.com>
> >>>
> >>> 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@ford.com>
> >>> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> >>
> >> It's not becoming clear why this would want doing, nor in how far some of
> >> the bits there may gain "common" meaning, too. Imo this kind of change is
> >> meaningful only in a series where later the common-ness is also used.
> >
> > I have a set of upcoming changes here:
> >   https://gitlab.com/xen-project/people/dmukhin/xen/-/tree/vuart-framework?ref_type=heads
> >
> > Specifically,
> >  https://gitlab.com/xen-project/people/dmukhin/xen/-/commit/17f44d23c1904374963c4ec839bc8219041b5d26
> >
> > enables the use of emulation_flags on Arm.
> >
> > I can move this patch to the upcoming series, if needed, but looks like it is
> > OK to have it for Arm even now.
> 
> Prior to that series making it to a ready-to-be-committed state, it'll be
> (effectively) dead code on Arm. Which strictly speaking Misra objects to.
> I wonder if you, Stefano, considered that when giving your R-b.
> 
> Further - what about PPC and RISC-V?

I have no problem plumbing this patch to the follow on series.
Will re-adjust.

> 
> Jan
> 

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

[Roger Pau Monné]

On Mon, Jun 02, 2025 at 07:17:30PM +0000, dmkhn@proton.me wrote:
> From: Denis Mukhin <dmukhin@ford.com>
> 
> 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@ford.com>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
> 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, Roger.

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

[dmkhn]

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@proton.me wrote:
> > From: Denis Mukhin <dmukhin@ford.com>
> >
> > 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@ford.com>
> > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> > ---
> > 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.

[PATCH v5 2/2] xen/domain: rewrite emulation_flags_ok()

[dmkhn]

From: Denis Mukhin <dmukhin@ford.com>

Rewrite emulation_flags_ok() to simplify future modifications.

No functional change intended.

Signed-off-by: Denis Mukhin <dmukhin@ford.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Reviewed-by: Teddy Astie <teddy.astie@vates.tech>
---
Changes since v4:
- updated commentaries
- added Teddy's R-b, kept Stefano's R-b
---
 xen/arch/x86/domain.c | 91 ++++++++++++++++++++++++++++++++++---------
 1 file changed, 73 insertions(+), 18 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 0363ccb384..4f6670ce37 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -743,32 +743,87 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
     return 0;
 }
 
+/*
+ * Verify that the domain's emulation flags resolve to a supported configuration.
+ *
+ * This ensures we only allow a known, safe subset of emulation combinations
+ * (for both functionality and security). Arbitrary mixes are likely to cause
+ * errors (e.g., null pointer dereferences).
+ *
+ * NB: use the internal X86_EMU_XXX symbols, not the public XEN_X86_EMU_XXX
+ * symbols.
+ */
 static bool emulation_flags_ok(const struct domain *d, uint32_t emflags)
 {
+    enum {
+        CAP_PV          = BIT(0, U),
+        CAP_HVM         = BIT(1, U),
+        CAP_HWDOM       = BIT(2, U),
+        CAP_DOMU        = BIT(3, U),
+    };
+    static const struct {
+        unsigned int caps;
+        uint32_t min;
+        uint32_t opt;
+    } configs[] = {
+#ifdef CONFIG_PV
+        /* PV */
+        {
+            .caps   = CAP_PV | CAP_DOMU,
+            .min    = 0,
+            .opt    = 0,
+        },
+
+        /* PV dom0 */
+        {
+            .caps   = CAP_PV | CAP_HWDOM,
+            .min    = X86_EMU_PIT,
+            .opt    = 0,
+        },
+#endif /* #ifdef CONFIG_PV */
+
+#ifdef CONFIG_HVM
+        /* PVH dom0 */
+        {
+            .caps   = CAP_HVM | CAP_HWDOM,
+            .min    = X86_EMU_LAPIC | X86_EMU_IOAPIC | X86_EMU_VPCI,
+            .opt    = 0,
+        },
+
+        /* HVM domU */
+        {
+            .caps   = CAP_HVM | CAP_DOMU,
+            .min    = X86_EMU_ALL & ~(X86_EMU_VPCI | X86_EMU_USE_PIRQ),
+            /* HVM PIRQ feature is user-selectable. */
+            .opt    = X86_EMU_USE_PIRQ,
+        },
+
+        /* PVH domU */
+        {
+            .caps   = CAP_HVM | CAP_DOMU,
+            .min    = X86_EMU_LAPIC,
+            .opt    = 0,
+        },
+#endif /* #ifdef CONFIG_HVM */
+    };
+    unsigned int i, caps = is_hardware_domain(d) ? CAP_HWDOM : CAP_DOMU;
+
+    if ( is_pv_domain(d) )
+        caps |= CAP_PV;
+    else if ( is_hvm_domain(d) )
+        caps |= CAP_HVM;
+
 #ifdef CONFIG_HVM
     /* This doesn't catch !CONFIG_HVM case but it is better than nothing */
     BUILD_BUG_ON(X86_EMU_ALL != XEN_X86_EMU_ALL);
 #endif
 
-    if ( is_hvm_domain(d) )
-    {
-        if ( is_hardware_domain(d) &&
-             emflags != (X86_EMU_VPCI | X86_EMU_LAPIC | X86_EMU_IOAPIC) )
-            return false;
-        if ( !is_hardware_domain(d) &&
-             /* HVM PIRQ feature is user-selectable. */
-             (emflags & ~X86_EMU_USE_PIRQ) !=
-             (X86_EMU_ALL & ~(X86_EMU_VPCI | X86_EMU_USE_PIRQ)) &&
-             emflags != X86_EMU_LAPIC )
-            return false;
-    }
-    else if ( emflags != 0 && emflags != X86_EMU_PIT )
-    {
-        /* PV or classic PVH. */
-        return false;
-    }
+    for ( i = 0; i < ARRAY_SIZE(configs); i++ )
+        if ( caps == configs[i].caps &&
+             (emflags & ~configs[i].opt) == configs[i].min )
+            return true;
 
-    return true;
+    return false;
 }
 
 void __init arch_init_idle_domain(struct domain *d)
-- 
2.34.1

Re: [PATCH v5 2/2] xen/domain: rewrite emulation_flags_ok()

[Jan Beulich]

On 02.06.2025 21:17, dmkhn@proton.me wrote:
> From: Denis Mukhin <dmukhin@ford.com>
> 
> Rewrite emulation_flags_ok() to simplify future modifications.
> 
> No functional change intended.
> 
> Signed-off-by: Denis Mukhin <dmukhin@ford.com>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> Reviewed-by: Teddy Astie <teddy.astie@vates.tech>
> ---
> Changes since v4:
> - updated commentaries
> - added Teddy's R-b, kept Stefano's R-b
> ---
>  xen/arch/x86/domain.c | 91 ++++++++++++++++++++++++++++++++++---------
>  1 file changed, 73 insertions(+), 18 deletions(-)

Given this diffstat, I wonder what the other x86 maintainers think about
this.

> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -743,32 +743,87 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
>      return 0;
>  }
>  
> +/*
> + * Verify that the domain's emulation flags resolve to a supported configuration.
> + *
> + * This ensures we only allow a known, safe subset of emulation combinations
> + * (for both functionality and security). Arbitrary mixes are likely to cause
> + * errors (e.g., null pointer dereferences).
> + *
> + * NB: use the internal X86_EMU_XXX symbols, not the public XEN_X86_EMU_XXX
> + * symbols.
> + */
>  static bool emulation_flags_ok(const struct domain *d, uint32_t emflags)
>  {
> +    enum {
> +        CAP_PV          = BIT(0, U),
> +        CAP_HVM         = BIT(1, U),
> +        CAP_HWDOM       = BIT(2, U),
> +        CAP_DOMU        = BIT(3, U),
> +    };
> +    static const struct {
> +        unsigned int caps;
> +        uint32_t min;
> +        uint32_t opt;
> +    } configs[] = {
> +#ifdef CONFIG_PV
> +        /* PV */
> +        {
> +            .caps   = CAP_PV | CAP_DOMU,
> +            .min    = 0,
> +            .opt    = 0,

Why the latter two initializers? Imo adding ones which say nothing else than
what's the default is only enlarging code without much real benefit.

> +        },
> +
> +        /* PV dom0 */
> +        {
> +            .caps   = CAP_PV | CAP_HWDOM,
> +            .min    = X86_EMU_PIT,
> +            .opt    = 0,
> +        },
> +#endif /* #ifdef CONFIG_PV */
> +
> +#ifdef CONFIG_HVM
> +        /* PVH dom0 */
> +        {
> +            .caps   = CAP_HVM | CAP_HWDOM,
> +            .min    = X86_EMU_LAPIC | X86_EMU_IOAPIC | X86_EMU_VPCI,
> +            .opt    = 0,
> +        },
> +
> +        /* HVM domU */
> +        {
> +            .caps   = CAP_HVM | CAP_DOMU,
> +            .min    = X86_EMU_ALL & ~(X86_EMU_VPCI | X86_EMU_USE_PIRQ),
> +            /* HVM PIRQ feature is user-selectable. */
> +            .opt    = X86_EMU_USE_PIRQ,
> +        },
> +
> +        /* PVH domU */
> +        {
> +            .caps   = CAP_HVM | CAP_DOMU,
> +            .min    = X86_EMU_LAPIC,
> +            .opt    = 0,
> +        },
> +#endif /* #ifdef CONFIG_HVM */
> +    };
> +    unsigned int i, caps = is_hardware_domain(d) ? CAP_HWDOM : CAP_DOMU;
> +
> +    if ( is_pv_domain(d) )
> +        caps |= CAP_PV;
> +    else if ( is_hvm_domain(d) )
> +        caps |= CAP_HVM;

There's no 3rd case, so this could be expressed with plain "else", and hence
also with a conditional operator, and hence could also be right in the
initializer. How far to go with those transformations I'm not sure; personally
I'd go all the way, but I'd be okay-ish with just the first of the steps.

Jan

Re: [PATCH v5 2/2] xen/domain: rewrite emulation_flags_ok()

[dmkhn]

On Wed, Jun 04, 2025 at 12:43:22PM +0200, Jan Beulich wrote:
> On 02.06.2025 21:17, dmkhn@proton.me wrote:
> > From: Denis Mukhin <dmukhin@ford.com>
> >
> > Rewrite emulation_flags_ok() to simplify future modifications.
> >
> > No functional change intended.
> >
> > Signed-off-by: Denis Mukhin <dmukhin@ford.com>
> > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> > Reviewed-by: Teddy Astie <teddy.astie@vates.tech>
> > ---
> > Changes since v4:
> > - updated commentaries
> > - added Teddy's R-b, kept Stefano's R-b
> > ---
> >  xen/arch/x86/domain.c | 91 ++++++++++++++++++++++++++++++++++---------
> >  1 file changed, 73 insertions(+), 18 deletions(-)
> 
> Given this diffstat, I wonder what the other x86 maintainers think about
> this.
> 
> > --- a/xen/arch/x86/domain.c
> > +++ b/xen/arch/x86/domain.c
> > @@ -743,32 +743,87 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
> >      return 0;
> >  }
> >
> > +/*
> > + * Verify that the domain's emulation flags resolve to a supported configuration.
> > + *
> > + * This ensures we only allow a known, safe subset of emulation combinations
> > + * (for both functionality and security). Arbitrary mixes are likely to cause
> > + * errors (e.g., null pointer dereferences).
> > + *
> > + * NB: use the internal X86_EMU_XXX symbols, not the public XEN_X86_EMU_XXX
> > + * symbols.
> > + */
> >  static bool emulation_flags_ok(const struct domain *d, uint32_t emflags)
> >  {
> > +    enum {
> > +        CAP_PV          = BIT(0, U),
> > +        CAP_HVM         = BIT(1, U),
> > +        CAP_HWDOM       = BIT(2, U),
> > +        CAP_DOMU        = BIT(3, U),
> > +    };
> > +    static const struct {
> > +        unsigned int caps;
> > +        uint32_t min;
> > +        uint32_t opt;
> > +    } configs[] = {
> > +#ifdef CONFIG_PV
> > +        /* PV */
> > +        {
> > +            .caps   = CAP_PV | CAP_DOMU,
> > +            .min    = 0,
> > +            .opt    = 0,
> 
> Why the latter two initializers? Imo adding ones which say nothing else than
> what's the default is only enlarging code without much real benefit.

Sure, no problem, I can address that.
Thanks!

> 
> > +        },
> > +
> > +        /* PV dom0 */
> > +        {
> > +            .caps   = CAP_PV | CAP_HWDOM,
> > +            .min    = X86_EMU_PIT,
> > +            .opt    = 0,
> > +        },
> > +#endif /* #ifdef CONFIG_PV */
> > +
> > +#ifdef CONFIG_HVM
> > +        /* PVH dom0 */
> > +        {
> > +            .caps   = CAP_HVM | CAP_HWDOM,
> > +            .min    = X86_EMU_LAPIC | X86_EMU_IOAPIC | X86_EMU_VPCI,
> > +            .opt    = 0,
> > +        },
> > +
> > +        /* HVM domU */
> > +        {
> > +            .caps   = CAP_HVM | CAP_DOMU,
> > +            .min    = X86_EMU_ALL & ~(X86_EMU_VPCI | X86_EMU_USE_PIRQ),
> > +            /* HVM PIRQ feature is user-selectable. */
> > +            .opt    = X86_EMU_USE_PIRQ,
> > +        },
> > +
> > +        /* PVH domU */
> > +        {
> > +            .caps   = CAP_HVM | CAP_DOMU,
> > +            .min    = X86_EMU_LAPIC,
> > +            .opt    = 0,
> > +        },
> > +#endif /* #ifdef CONFIG_HVM */
> > +    };
> > +    unsigned int i, caps = is_hardware_domain(d) ? CAP_HWDOM : CAP_DOMU;
> > +
> > +    if ( is_pv_domain(d) )
> > +        caps |= CAP_PV;
> > +    else if ( is_hvm_domain(d) )
> > +        caps |= CAP_HVM;
> 
> There's no 3rd case, so this could be expressed with plain "else", and hence
> also with a conditional operator, and hence could also be right in the
> initializer. How far to go with those transformations I'm not sure; personally
> I'd go all the way, but I'd be okay-ish with just the first of the steps.

Ack.

> 
> Jan

Re: [PATCH v5 2/2] xen/domain: rewrite emulation_flags_ok()

[Roger Pau Monné]

On Wed, Jun 04, 2025 at 12:43:22PM +0200, Jan Beulich wrote:
> On 02.06.2025 21:17, dmkhn@proton.me wrote:
> > From: Denis Mukhin <dmukhin@ford.com>
> > 
> > Rewrite emulation_flags_ok() to simplify future modifications.
> > 
> > No functional change intended.
> > 
> > Signed-off-by: Denis Mukhin <dmukhin@ford.com>
> > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> > Reviewed-by: Teddy Astie <teddy.astie@vates.tech>
> > ---
> > Changes since v4:
> > - updated commentaries
> > - added Teddy's R-b, kept Stefano's R-b
> > ---
> >  xen/arch/x86/domain.c | 91 ++++++++++++++++++++++++++++++++++---------
> >  1 file changed, 73 insertions(+), 18 deletions(-)
> 
> Given this diffstat, I wonder what the other x86 maintainers think about
> this.

I think the array is a cleaner way of expressing the possible domain
configurations.

See below, I got some suggestions that would likely make the diffstat
better.

> > --- a/xen/arch/x86/domain.c
> > +++ b/xen/arch/x86/domain.c
> > @@ -743,32 +743,87 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
> >      return 0;
> >  }
> >  
> > +/*
> > + * Verify that the domain's emulation flags resolve to a supported configuration.
> > + *
> > + * This ensures we only allow a known, safe subset of emulation combinations
> > + * (for both functionality and security). Arbitrary mixes are likely to cause
> > + * errors (e.g., null pointer dereferences).
> > + *
> > + * NB: use the internal X86_EMU_XXX symbols, not the public XEN_X86_EMU_XXX
> > + * symbols.
> > + */
> >  static bool emulation_flags_ok(const struct domain *d, uint32_t emflags)
> >  {
> > +    enum {
> > +        CAP_PV          = BIT(0, U),
> > +        CAP_HVM         = BIT(1, U),
> > +        CAP_HWDOM       = BIT(2, U),
> > +        CAP_DOMU        = BIT(3, U),
> > +    };
> > +    static const struct {
> > +        unsigned int caps;
> > +        uint32_t min;
> > +        uint32_t opt;
> > +    } configs[] = {
> > +#ifdef CONFIG_PV
> > +        /* PV */
> > +        {
> > +            .caps   = CAP_PV | CAP_DOMU,
> > +            .min    = 0,
> > +            .opt    = 0,
> 
> Why the latter two initializers? Imo adding ones which say nothing else than
> what's the default is only enlarging code without much real benefit.

I'm fine with skipping explicit initialization of 0 fields, I don't
think there's much benefit here.

> > +        },
> > +
> > +        /* PV dom0 */
> > +        {
> > +            .caps   = CAP_PV | CAP_HWDOM,
> > +            .min    = X86_EMU_PIT,
> > +            .opt    = 0,
> > +        },
> > +#endif /* #ifdef CONFIG_PV */

I think the above two elements could be folded into a single one,
iow:

	/* PV domU and dom0 */
        {
            .caps   = CAP_PV
            .min    = X86_EMU_PIT,
        },

As given the current code we do allow PV domUs with X86_EMU_PIT, so
otherwise the change here is not non-functional.

> > +
> > +#ifdef CONFIG_HVM
> > +        /* PVH dom0 */
> > +        {
> > +            .caps   = CAP_HVM | CAP_HWDOM,
> > +            .min    = X86_EMU_LAPIC | X86_EMU_IOAPIC | X86_EMU_VPCI,
> > +            .opt    = 0,
> > +        },
> > +
> > +        /* HVM domU */
> > +        {
> > +            .caps   = CAP_HVM | CAP_DOMU,
> > +            .min    = X86_EMU_ALL & ~(X86_EMU_VPCI | X86_EMU_USE_PIRQ),
> > +            /* HVM PIRQ feature is user-selectable. */
> > +            .opt    = X86_EMU_USE_PIRQ,
> > +        },
> > +
> > +        /* PVH domU */
> > +        {
> > +            .caps   = CAP_HVM | CAP_DOMU,
> > +            .min    = X86_EMU_LAPIC,
> > +            .opt    = 0,
> > +        },
> > +#endif /* #ifdef CONFIG_HVM */
> > +    };
> > +    unsigned int i, caps = is_hardware_domain(d) ? CAP_HWDOM : CAP_DOMU;
> > +
> > +    if ( is_pv_domain(d) )
> > +        caps |= CAP_PV;
> > +    else if ( is_hvm_domain(d) )
> > +        caps |= CAP_HVM;
> 
> There's no 3rd case, so this could be expressed with plain "else", and hence
> also with a conditional operator, and hence could also be right in the
> initializer. How far to go with those transformations I'm not sure; personally
> I'd go all the way, but I'd be okay-ish with just the first of the steps.

I agree, I would place them all in the definition:

    unsigned int caps = (is_hardware_domain(d) ? CAP_HWDOM : CAP_DOMU) |
                        (is_pv_domain(d) ? CAP_PV : CAP_HVM);

Thanks, Roger.