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

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

[dmkhn]

Patch 1 introduces use of d->arch.emulation_flags for non-x86 platforms and
hooks emulation_flags to 'q' keyhandler for debugging. emulation_flags on
non-x86 systems can be used for enabling domain emulation features.

Patch 2 rewrites emulation_flags_ok() on x86 with a goal of improving
readability 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 v1: https://lore.kernel.org/xen-devel/20250401005224.461325-1-dmukhin@ford.com/
[3] Link to CI: https://gitlab.com/xen-project/people/dmukhin/xen/-/pipelines/1820121879 

Denis Mukhin (2):
  xen/domain: introduce non-x86 hardware emulation flags
  xen/domain: rewrite emulation_flags_ok()

 xen/arch/arm/include/asm/domain.h   |  1 +
 xen/arch/ppc/include/asm/domain.h   |  1 +
 xen/arch/riscv/include/asm/domain.h |  1 +
 xen/arch/x86/domain.c               | 29 +++++++++++------------------
 xen/arch/x86/include/asm/domain.h   |  6 ++++++
 xen/common/keyhandler.c             |  1 +
 6 files changed, 21 insertions(+), 18 deletions(-)

-- 
2.34.1

[PATCH v2 1/2] xen/domain: introduce non-x86 hardware emulation flags

[dmkhn]

From: Denis Mukhin <dmukhin@ford.com>

Define per-architecture emulation_flags for configuring domain emulation
features.

Print d->arch.emulation_flags from 'q' keyhandler for better traceability
while debugging.

Signed-off-by: Denis Mukhin <dmukhin@ford.com>
---
Changes since v1:
- dropped comments
---
 xen/arch/arm/include/asm/domain.h   | 1 +
 xen/arch/ppc/include/asm/domain.h   | 1 +
 xen/arch/riscv/include/asm/domain.h | 1 +
 xen/common/keyhandler.c             | 1 +
 4 files changed, 4 insertions(+)

diff --git a/xen/arch/arm/include/asm/domain.h b/xen/arch/arm/include/asm/domain.h
index a3487ca713..70e6e7d49b 100644
--- a/xen/arch/arm/include/asm/domain.h
+++ b/xen/arch/arm/include/asm/domain.h
@@ -121,6 +121,7 @@ struct arch_domain
     void *tee;
 #endif
 
+    uint32_t emulation_flags;
 }  __cacheline_aligned;
 
 struct arch_vcpu
diff --git a/xen/arch/ppc/include/asm/domain.h b/xen/arch/ppc/include/asm/domain.h
index 3a447272c6..001116a0ab 100644
--- a/xen/arch/ppc/include/asm/domain.h
+++ b/xen/arch/ppc/include/asm/domain.h
@@ -21,6 +21,7 @@ struct arch_vcpu {
 
 struct arch_domain {
     struct hvm_domain hvm;
+    uint32_t emulation_flags;
 };
 
 #include <xen/sched.h>
diff --git a/xen/arch/riscv/include/asm/domain.h b/xen/arch/riscv/include/asm/domain.h
index c3d965a559..7bc242da55 100644
--- a/xen/arch/riscv/include/asm/domain.h
+++ b/xen/arch/riscv/include/asm/domain.h
@@ -18,6 +18,7 @@ struct arch_vcpu {
 
 struct arch_domain {
     struct hvm_domain hvm;
+    uint32_t emulation_flags;
 };
 
 #include <xen/sched.h>
diff --git a/xen/common/keyhandler.c b/xen/common/keyhandler.c
index 0bb842ec00..73f5134b68 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->arch.emulation_flags);
 
         arch_dump_domain_info(d);
 
-- 
2.34.1

Re: [PATCH v2 1/2] xen/domain: introduce non-x86 hardware emulation flags

[Jan Beulich]

On 16.05.2025 04:29, dmkhn@proton.me wrote:
> From: Denis Mukhin <dmukhin@ford.com>
> 
> Define per-architecture emulation_flags for configuring domain emulation
> features.
> 
> Print d->arch.emulation_flags from 'q' keyhandler for better traceability
> while debugging.
> 
> Signed-off-by: Denis Mukhin <dmukhin@ford.com>
> ---
> Changes since v1:
> - dropped comments
> ---
>  xen/arch/arm/include/asm/domain.h   | 1 +
>  xen/arch/ppc/include/asm/domain.h   | 1 +
>  xen/arch/riscv/include/asm/domain.h | 1 +
>  xen/common/keyhandler.c             | 1 +
>  4 files changed, 4 insertions(+)

If every arch gains identical fields, accessed from common code, why would
those need to live in struct arch_domain?

Jan

Re: [PATCH v2 1/2] xen/domain: introduce non-x86 hardware emulation flags

[dmkhn]

On Tue, May 20, 2025 at 05:21:06PM +0200, Jan Beulich wrote:
> On 16.05.2025 04:29, dmkhn@proton.me wrote:
> > From: Denis Mukhin <dmukhin@ford.com>
> >
> > Define per-architecture emulation_flags for configuring domain emulation
> > features.
> >
> > Print d->arch.emulation_flags from 'q' keyhandler for better traceability
> > while debugging.
> >
> > Signed-off-by: Denis Mukhin <dmukhin@ford.com>
> > ---
> > Changes since v1:
> > - dropped comments
> > ---
> >  xen/arch/arm/include/asm/domain.h   | 1 +
> >  xen/arch/ppc/include/asm/domain.h   | 1 +
> >  xen/arch/riscv/include/asm/domain.h | 1 +
> >  xen/common/keyhandler.c             | 1 +
> >  4 files changed, 4 insertions(+)
> 
> If every arch gains identical fields, accessed from common code, why would
> those need to live in struct arch_domain?

I did it this way to keep the diff smaller, but I agree such property
makes sense to put in common domain struct. Will update in v3.

Thanks!

> 
> Jan

Re: [PATCH v2 1/2] xen/domain: introduce non-x86 hardware emulation flags

[Jan Beulich]

On 20.05.2025 23:39, dmkhn@proton.me wrote:
> On Tue, May 20, 2025 at 05:21:06PM +0200, Jan Beulich wrote:
>> On 16.05.2025 04:29, dmkhn@proton.me wrote:
>>> From: Denis Mukhin <dmukhin@ford.com>
>>>
>>> Define per-architecture emulation_flags for configuring domain emulation
>>> features.
>>>
>>> Print d->arch.emulation_flags from 'q' keyhandler for better traceability
>>> while debugging.
>>>
>>> Signed-off-by: Denis Mukhin <dmukhin@ford.com>
>>> ---
>>> Changes since v1:
>>> - dropped comments
>>> ---
>>>  xen/arch/arm/include/asm/domain.h   | 1 +
>>>  xen/arch/ppc/include/asm/domain.h   | 1 +
>>>  xen/arch/riscv/include/asm/domain.h | 1 +
>>>  xen/common/keyhandler.c             | 1 +
>>>  4 files changed, 4 insertions(+)
>>
>> If every arch gains identical fields, accessed from common code, why would
>> those need to live in struct arch_domain?
> 
> I did it this way to keep the diff smaller, but I agree such property
> makes sense to put in common domain struct. Will update in v3.

Provided there's arch maintainer buy-off on generalizing this. There's (at
least) a 3rd option, after all: Have arch-specific and (separate) common
emulation flags.

Jan

Re: [PATCH v2 1/2] xen/domain: introduce non-x86 hardware emulation flags

[Roger Pau Monné]

On Fri, May 16, 2025 at 02:29:09AM +0000, dmkhn@proton.me wrote:
> From: Denis Mukhin <dmukhin@ford.com>
> 
> Define per-architecture emulation_flags for configuring domain emulation
> features.
> 
> Print d->arch.emulation_flags from 'q' keyhandler for better traceability
> while debugging.
> 
> Signed-off-by: Denis Mukhin <dmukhin@ford.com>
> ---
> Changes since v1:
> - dropped comments
> ---
>  xen/arch/arm/include/asm/domain.h   | 1 +
>  xen/arch/ppc/include/asm/domain.h   | 1 +
>  xen/arch/riscv/include/asm/domain.h | 1 +
>  xen/common/keyhandler.c             | 1 +
>  4 files changed, 4 insertions(+)
> 
> diff --git a/xen/arch/arm/include/asm/domain.h b/xen/arch/arm/include/asm/domain.h
> index a3487ca713..70e6e7d49b 100644
> --- a/xen/arch/arm/include/asm/domain.h
> +++ b/xen/arch/arm/include/asm/domain.h
> @@ -121,6 +121,7 @@ struct arch_domain
>      void *tee;
>  #endif
>  
> +    uint32_t emulation_flags;
>  }  __cacheline_aligned;
>  
>  struct arch_vcpu
> diff --git a/xen/arch/ppc/include/asm/domain.h b/xen/arch/ppc/include/asm/domain.h
> index 3a447272c6..001116a0ab 100644
> --- a/xen/arch/ppc/include/asm/domain.h
> +++ b/xen/arch/ppc/include/asm/domain.h
> @@ -21,6 +21,7 @@ struct arch_vcpu {
>  
>  struct arch_domain {
>      struct hvm_domain hvm;
> +    uint32_t emulation_flags;
>  };
>  
>  #include <xen/sched.h>
> diff --git a/xen/arch/riscv/include/asm/domain.h b/xen/arch/riscv/include/asm/domain.h
> index c3d965a559..7bc242da55 100644
> --- a/xen/arch/riscv/include/asm/domain.h
> +++ b/xen/arch/riscv/include/asm/domain.h
> @@ -18,6 +18,7 @@ struct arch_vcpu {
>  
>  struct arch_domain {
>      struct hvm_domain hvm;
> +    uint32_t emulation_flags;
>  };
>  
>  #include <xen/sched.h>
> diff --git a/xen/common/keyhandler.c b/xen/common/keyhandler.c
> index 0bb842ec00..73f5134b68 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->arch.emulation_flags);
>  
>          arch_dump_domain_info(d);

Hello,

I think it might be easier to print emulation_flags in
arch_dump_domain_info(), ideally it would be helpful if this could be
printed in a user friendly way apart from the raw dump:

printk("    emulation_flags:%s%s... (%#x)\n",
       !d->arch.emulation_flags ? " none" : "",
       has_vlapic(d) ? " lapic" : "", ...
       d->arch.emulation_flags);

Regards, Roger.

Re: [PATCH v2 1/2] xen/domain: introduce non-x86 hardware emulation flags

[dmkhn]

On Wed, May 21, 2025 at 04:05:04PM +0200, Roger Pau Monné wrote:
> On Fri, May 16, 2025 at 02:29:09AM +0000, dmkhn@proton.me wrote:
> > From: Denis Mukhin <dmukhin@ford.com>
> >
> > Define per-architecture emulation_flags for configuring domain emulation
> > features.
> >
> > Print d->arch.emulation_flags from 'q' keyhandler for better traceability
> > while debugging.
> >
> > Signed-off-by: Denis Mukhin <dmukhin@ford.com>
> > ---
> > Changes since v1:
> > - dropped comments
> > ---
> >  xen/arch/arm/include/asm/domain.h   | 1 +
> >  xen/arch/ppc/include/asm/domain.h   | 1 +
> >  xen/arch/riscv/include/asm/domain.h | 1 +
> >  xen/common/keyhandler.c             | 1 +
> >  4 files changed, 4 insertions(+)
> >
> > diff --git a/xen/arch/arm/include/asm/domain.h b/xen/arch/arm/include/asm/domain.h
> > index a3487ca713..70e6e7d49b 100644
> > --- a/xen/arch/arm/include/asm/domain.h
> > +++ b/xen/arch/arm/include/asm/domain.h
> > @@ -121,6 +121,7 @@ struct arch_domain
> >      void *tee;
> >  #endif
> >
> > +    uint32_t emulation_flags;
> >  }  __cacheline_aligned;
> >
> >  struct arch_vcpu
> > diff --git a/xen/arch/ppc/include/asm/domain.h b/xen/arch/ppc/include/asm/domain.h
> > index 3a447272c6..001116a0ab 100644
> > --- a/xen/arch/ppc/include/asm/domain.h
> > +++ b/xen/arch/ppc/include/asm/domain.h
> > @@ -21,6 +21,7 @@ struct arch_vcpu {
> >
> >  struct arch_domain {
> >      struct hvm_domain hvm;
> > +    uint32_t emulation_flags;
> >  };
> >
> >  #include <xen/sched.h>
> > diff --git a/xen/arch/riscv/include/asm/domain.h b/xen/arch/riscv/include/asm/domain.h
> > index c3d965a559..7bc242da55 100644
> > --- a/xen/arch/riscv/include/asm/domain.h
> > +++ b/xen/arch/riscv/include/asm/domain.h
> > @@ -18,6 +18,7 @@ struct arch_vcpu {
> >
> >  struct arch_domain {
> >      struct hvm_domain hvm;
> > +    uint32_t emulation_flags;
> >  };
> >
> >  #include <xen/sched.h>
> > diff --git a/xen/common/keyhandler.c b/xen/common/keyhandler.c
> > index 0bb842ec00..73f5134b68 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->arch.emulation_flags);
> >
> >          arch_dump_domain_info(d);
> 
> Hello,
> 
> I think it might be easier to print emulation_flags in
> arch_dump_domain_info(), ideally it would be helpful if this could be
> printed in a user friendly way apart from the raw dump:
> 
> printk("    emulation_flags:%s%s... (%#x)\n",
>        !d->arch.emulation_flags ? " none" : "",
>        has_vlapic(d) ? " lapic" : "", ...
>        d->arch.emulation_flags);

I moved emulation_flags to the common domain struct in v3 and I kept the
emulation_flags flags printout here in common dump_domains().

I will plumb the human-readable printout for x86 flags in the follow on patch.

> 
> Regards, Roger.

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

[dmkhn]

From: Denis Mukhin <dmukhin@ford.com>

Rewrite emulation_flags_ok() to simplify future modifications.

Also, introduce X86_EMU_{BASELINE,OPTIONAL} helper macros.

No functional change intended.

Signed-off-by: Denis Mukhin <dmukhin@ford.com>
---
Changes since v1:
- kept use of non-public X86_EMU_XXX flags
- corrected some comments and macro definitions
---
 xen/arch/x86/domain.c             | 29 +++++++++++------------------
 xen/arch/x86/include/asm/domain.h |  6 ++++++
 2 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index f197dad4c0..c64c2c6fef 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -750,25 +750,18 @@ static bool emulation_flags_ok(const struct domain *d, uint32_t emflags)
     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;
-    }
+    /* PV */
+    if ( !is_hvm_domain(d) )
+        return emflags == 0 || emflags == X86_EMU_PIT;
 
-    return true;
+    /* HVM */
+    if ( is_hardware_domain(d) )
+        return emflags == (X86_EMU_LAPIC |
+                           X86_EMU_IOAPIC |
+                           X86_EMU_VPCI);
+
+    return (emflags & ~X86_EMU_OPTIONAL) == X86_EMU_BASELINE ||
+            emflags == X86_EMU_LAPIC;
 }
 
 void __init arch_init_idle_domain(struct domain *d)
diff --git a/xen/arch/x86/include/asm/domain.h b/xen/arch/x86/include/asm/domain.h
index 8c0dea12a5..3a9a9fd80d 100644
--- a/xen/arch/x86/include/asm/domain.h
+++ b/xen/arch/x86/include/asm/domain.h
@@ -494,6 +494,12 @@ struct arch_domain
                                  X86_EMU_PIT | X86_EMU_USE_PIRQ |       \
                                  X86_EMU_VPCI)
 
+/* User-selectable features. */
+#define X86_EMU_OPTIONAL        (X86_EMU_USE_PIRQ)
+
+#define X86_EMU_BASELINE        (X86_EMU_ALL & ~(X86_EMU_VPCI | \
+                                                 X86_EMU_OPTIONAL))
+
 #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))
-- 
2.34.1

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

[Jan Beulich]

On 16.05.2025 04:29, dmkhn@proton.me wrote:
> --- a/xen/arch/x86/include/asm/domain.h
> +++ b/xen/arch/x86/include/asm/domain.h
> @@ -494,6 +494,12 @@ struct arch_domain
>                                   X86_EMU_PIT | X86_EMU_USE_PIRQ |       \
>                                   X86_EMU_VPCI)
>  
> +/* User-selectable features. */
> +#define X86_EMU_OPTIONAL        (X86_EMU_USE_PIRQ)
> +
> +#define X86_EMU_BASELINE        (X86_EMU_ALL & ~(X86_EMU_VPCI | \
> +                                                 X86_EMU_OPTIONAL))

That is, VPCI is neither baseline nor optional. Certainly at least strange.

Jan

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

[dmkhn]

On Tue, May 20, 2025 at 05:24:33PM +0200, Jan Beulich wrote:
> On 16.05.2025 04:29, dmkhn@proton.me wrote:
> > --- a/xen/arch/x86/include/asm/domain.h
> > +++ b/xen/arch/x86/include/asm/domain.h
> > @@ -494,6 +494,12 @@ struct arch_domain
> >                                   X86_EMU_PIT | X86_EMU_USE_PIRQ |       \
> >                                   X86_EMU_VPCI)
> >
> > +/* User-selectable features. */
> > +#define X86_EMU_OPTIONAL        (X86_EMU_USE_PIRQ)
> > +
> > +#define X86_EMU_BASELINE        (X86_EMU_ALL & ~(X86_EMU_VPCI | \
> > +                                                 X86_EMU_OPTIONAL))
> 
> That is, VPCI is neither baseline nor optional. Certainly at least strange.

IMO, X86_EMU_OPTIONAL should include both VPCI and PIRQ.

But that will be a behavior change: AFAIU, VPCI is injected implicitly for dom0
case only, "default" xl toolstack currently excludes VPCI for HVM domains.

Do I understand it correctly that "BASELINE" in the symbol name is a concern?

> 
> Jan

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

[Stefano Stabellini]

On Tue, 20 May 2025, dmkhn@proton.me wrote:
> On Tue, May 20, 2025 at 05:24:33PM +0200, Jan Beulich wrote:
> > On 16.05.2025 04:29, dmkhn@proton.me wrote:
> > > --- a/xen/arch/x86/include/asm/domain.h
> > > +++ b/xen/arch/x86/include/asm/domain.h
> > > @@ -494,6 +494,12 @@ struct arch_domain
> > >                                   X86_EMU_PIT | X86_EMU_USE_PIRQ |       \
> > >                                   X86_EMU_VPCI)
> > >
> > > +/* User-selectable features. */
> > > +#define X86_EMU_OPTIONAL        (X86_EMU_USE_PIRQ)
> > > +
> > > +#define X86_EMU_BASELINE        (X86_EMU_ALL & ~(X86_EMU_VPCI | \
> > > +                                                 X86_EMU_OPTIONAL))
> > 
> > That is, VPCI is neither baseline nor optional. Certainly at least strange.

I think Denis tried to keep the code more similar to the original. This
way it is easier to review the code change and it seems correct. But at
the same time it is easier to spot inconsistency that were present even
before the patch.


> IMO, X86_EMU_OPTIONAL should include both VPCI and PIRQ.

It looks that way to me too.

However, then we need to be careful as the check would differ from the
original, but maybe that's OK. We want vPCI to be potentially exposed to
DomUs as well.


> But that will be a behavior change: AFAIU, VPCI is injected implicitly for dom0
> case only, "default" xl toolstack currently excludes VPCI for HVM domains.
> 
> Do I understand it correctly that "BASELINE" in the symbol name is a concern?

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

[Jan Beulich]

On 21.05.2025 01:00, Stefano Stabellini wrote:
> On Tue, 20 May 2025, dmkhn@proton.me wrote:
>> On Tue, May 20, 2025 at 05:24:33PM +0200, Jan Beulich wrote:
>>> On 16.05.2025 04:29, dmkhn@proton.me wrote:
>>>> --- a/xen/arch/x86/include/asm/domain.h
>>>> +++ b/xen/arch/x86/include/asm/domain.h
>>>> @@ -494,6 +494,12 @@ struct arch_domain
>>>>                                   X86_EMU_PIT | X86_EMU_USE_PIRQ |       \
>>>>                                   X86_EMU_VPCI)
>>>>
>>>> +/* User-selectable features. */
>>>> +#define X86_EMU_OPTIONAL        (X86_EMU_USE_PIRQ)
>>>> +
>>>> +#define X86_EMU_BASELINE        (X86_EMU_ALL & ~(X86_EMU_VPCI | \
>>>> +                                                 X86_EMU_OPTIONAL))
>>>
>>> That is, VPCI is neither baseline nor optional. Certainly at least strange.
> 
> I think Denis tried to keep the code more similar to the original. This
> way it is easier to review the code change and it seems correct. But at
> the same time it is easier to spot inconsistency that were present even
> before the patch.

Right, and that was in response to me complaining on an earlier version
that the behavior was (silently) changed. Yet doing it like this wasn't
the only way to address my comment there. At the same time (or already
before) I raised the question whether having such constants is helpful /
necessary in the first place. And now that their names don't reflect
their purpose, this question becomes yet more relevant.

Jan

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

[Jan Beulich]

On 21.05.2025 00:38, dmkhn@proton.me wrote:
> On Tue, May 20, 2025 at 05:24:33PM +0200, Jan Beulich wrote:
>> On 16.05.2025 04:29, dmkhn@proton.me wrote:
>>> --- a/xen/arch/x86/include/asm/domain.h
>>> +++ b/xen/arch/x86/include/asm/domain.h
>>> @@ -494,6 +494,12 @@ struct arch_domain
>>>                                   X86_EMU_PIT | X86_EMU_USE_PIRQ |       \
>>>                                   X86_EMU_VPCI)
>>>
>>> +/* User-selectable features. */
>>> +#define X86_EMU_OPTIONAL        (X86_EMU_USE_PIRQ)
>>> +
>>> +#define X86_EMU_BASELINE        (X86_EMU_ALL & ~(X86_EMU_VPCI | \
>>> +                                                 X86_EMU_OPTIONAL))
>>
>> That is, VPCI is neither baseline nor optional. Certainly at least strange.
> 
> IMO, X86_EMU_OPTIONAL should include both VPCI and PIRQ.
> 
> But that will be a behavior change: AFAIU, VPCI is injected implicitly for dom0
> case only, "default" xl toolstack currently excludes VPCI for HVM domains.
> 
> Do I understand it correctly that "BASELINE" in the symbol name is a concern?

It's not the word by itself. _If_ we want to have such symbols (which I had put
under question before), they need to be named to accurately reflect the purpose.
Imo with the names chosen an implication is that the two are non-overlapping,
while when combined yield the full set of flags.

Jan

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

[Roger Pau Monné]

On Fri, May 16, 2025 at 02:29:16AM +0000, dmkhn@proton.me wrote:
> From: Denis Mukhin <dmukhin@ford.com>
> 
> Rewrite emulation_flags_ok() to simplify future modifications.
> 
> Also, introduce X86_EMU_{BASELINE,OPTIONAL} helper macros.
> 
> No functional change intended.
> 
> Signed-off-by: Denis Mukhin <dmukhin@ford.com>
> ---
> Changes since v1:
> - kept use of non-public X86_EMU_XXX flags
> - corrected some comments and macro definitions
> ---
>  xen/arch/x86/domain.c             | 29 +++++++++++------------------
>  xen/arch/x86/include/asm/domain.h |  6 ++++++
>  2 files changed, 17 insertions(+), 18 deletions(-)
> 
> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index f197dad4c0..c64c2c6fef 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -750,25 +750,18 @@ static bool emulation_flags_ok(const struct domain *d, uint32_t emflags)
>      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;
> -    }
> +    /* PV */
> +    if ( !is_hvm_domain(d) )
> +        return emflags == 0 || emflags == X86_EMU_PIT;
>  
> -    return true;
> +    /* HVM */
> +    if ( is_hardware_domain(d) )
> +        return emflags == (X86_EMU_LAPIC |
> +                           X86_EMU_IOAPIC |
> +                           X86_EMU_VPCI);
> +
> +    return (emflags & ~X86_EMU_OPTIONAL) == X86_EMU_BASELINE ||
> +            emflags == X86_EMU_LAPIC;
>  }
>  
>  void __init arch_init_idle_domain(struct domain *d)
> diff --git a/xen/arch/x86/include/asm/domain.h b/xen/arch/x86/include/asm/domain.h
> index 8c0dea12a5..3a9a9fd80d 100644
> --- a/xen/arch/x86/include/asm/domain.h
> +++ b/xen/arch/x86/include/asm/domain.h
> @@ -494,6 +494,12 @@ struct arch_domain
>                                   X86_EMU_PIT | X86_EMU_USE_PIRQ |       \
>                                   X86_EMU_VPCI)
>  
> +/* User-selectable features. */
> +#define X86_EMU_OPTIONAL        (X86_EMU_USE_PIRQ)
> +
> +#define X86_EMU_BASELINE        (X86_EMU_ALL & ~(X86_EMU_VPCI | \
> +
> X86_EMU_OPTIONAL))

If you go this route I think you need to name those
X86_EMU_HVM_{BASELINE,OPTIONAL}, because they are really meaningful
only for HVM domains.

Regarding vPCI and HVM: we might want to enable it in the future for
domUs, but the fact is that right now it will collide badly with
ioreqs.  So for the time being on x86 it would be best if vPCI is
limited to PVH hardware domain exclusively, otherwise the hypervisor
internals might malfunction.  We shouldn't really allow dom0 to create
this kind of malformed domain as much as possible.

static const struct {
   bool pv, hwdom;
   uint32_t base, optional;
} allowed_conf[] = {
    /* PV */
    { true, false, 0, X86_EMU_PIT },
    /* PVH hardware domain */
    { false, true, X86_EMU_LAPIC | X86_EMU_IOAPIC | X86_EMU_VPCI, 0 },
    /* PVH domU */
    { false, false, X86_EMU_LAPIC, 0 },
    /* HVM */
    { false, false, X86_EMU_ALL & ~(X86_EMU_VPCI | X86_EMU_USE_PIRQ),
      X86_EMU_VPCI | X86_EMU_USE_PIRQ },
};
unsigned int i;

for ( i = 0; i < ARRAY_SIZE(allowed_conf); i++ )
{
    if ( is_pv_domain(d) == allowed_conf[i].pv &&
         /*
	  * A hardware domain can also use !hwdom entries, but not the
	  * other way around
	  */
         (is_hardware_domain(d) || !allowed_conf[i].hwdom) &&
	 (emflags & ~allowed_conf[i].optional) == allowed_conf[i].base )
        return true;
}

printk(XENLOG_INFO "%pd: invalid emulation flags: %#x\n", d, emflags);

return false;

I think the above (not even build tested) is slightly clearer, and
allows for easier expansion going forward?

Regards, Roger.

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

[dmkhn]

On Wed, May 21, 2025 at 05:07:12PM +0200, Roger Pau Monné wrote:
> On Fri, May 16, 2025 at 02:29:16AM +0000, dmkhn@proton.me wrote:
> > From: Denis Mukhin <dmukhin@ford.com>
> >
> > Rewrite emulation_flags_ok() to simplify future modifications.
> >
> > Also, introduce X86_EMU_{BASELINE,OPTIONAL} helper macros.
> >
> > No functional change intended.
> >
> > Signed-off-by: Denis Mukhin <dmukhin@ford.com>
> > ---
> > Changes since v1:
> > - kept use of non-public X86_EMU_XXX flags
> > - corrected some comments and macro definitions
> > ---
> >  xen/arch/x86/domain.c             | 29 +++++++++++------------------
> >  xen/arch/x86/include/asm/domain.h |  6 ++++++
> >  2 files changed, 17 insertions(+), 18 deletions(-)
> >
> > diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> > index f197dad4c0..c64c2c6fef 100644
> > --- a/xen/arch/x86/domain.c
> > +++ b/xen/arch/x86/domain.c
> > @@ -750,25 +750,18 @@ static bool emulation_flags_ok(const struct domain *d, uint32_t emflags)
> >      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;
> > -    }
> > +    /* PV */
> > +    if ( !is_hvm_domain(d) )
> > +        return emflags == 0 || emflags == X86_EMU_PIT;
> >
> > -    return true;
> > +    /* HVM */
> > +    if ( is_hardware_domain(d) )
> > +        return emflags == (X86_EMU_LAPIC |
> > +                           X86_EMU_IOAPIC |
> > +                           X86_EMU_VPCI);
> > +
> > +    return (emflags & ~X86_EMU_OPTIONAL) == X86_EMU_BASELINE ||
> > +            emflags == X86_EMU_LAPIC;
> >  }
> >
> >  void __init arch_init_idle_domain(struct domain *d)
> > diff --git a/xen/arch/x86/include/asm/domain.h b/xen/arch/x86/include/asm/domain.h
> > index 8c0dea12a5..3a9a9fd80d 100644
> > --- a/xen/arch/x86/include/asm/domain.h
> > +++ b/xen/arch/x86/include/asm/domain.h
> > @@ -494,6 +494,12 @@ struct arch_domain
> >                                   X86_EMU_PIT | X86_EMU_USE_PIRQ |       \
> >                                   X86_EMU_VPCI)
> >
> > +/* User-selectable features. */
> > +#define X86_EMU_OPTIONAL        (X86_EMU_USE_PIRQ)
> > +
> > +#define X86_EMU_BASELINE        (X86_EMU_ALL & ~(X86_EMU_VPCI | \
> > +
> > X86_EMU_OPTIONAL))
> 
> If you go this route I think you need to name those
> X86_EMU_HVM_{BASELINE,OPTIONAL}, because they are really meaningful
> only for HVM domains.
> 
> Regarding vPCI and HVM: we might want to enable it in the future for
> domUs, but the fact is that right now it will collide badly with
> ioreqs.  So for the time being on x86 it would be best if vPCI is
> limited to PVH hardware domain exclusively, otherwise the hypervisor
> internals might malfunction.  We shouldn't really allow dom0 to create
> this kind of malformed domain as much as possible.
> 
> static const struct {
>    bool pv, hwdom;
>    uint32_t base, optional;
> } allowed_conf[] = {
>     /* PV */
>     { true, false, 0, X86_EMU_PIT },
>     /* PVH hardware domain */
>     { false, true, X86_EMU_LAPIC | X86_EMU_IOAPIC | X86_EMU_VPCI, 0 },
>     /* PVH domU */
>     { false, false, X86_EMU_LAPIC, 0 },
>     /* HVM */
>     { false, false, X86_EMU_ALL & ~(X86_EMU_VPCI | X86_EMU_USE_PIRQ),
>       X86_EMU_VPCI | X86_EMU_USE_PIRQ },
> };
> unsigned int i;
> 
> for ( i = 0; i < ARRAY_SIZE(allowed_conf); i++ )
> {
>     if ( is_pv_domain(d) == allowed_conf[i].pv &&
>          /*
> 	  * A hardware domain can also use !hwdom entries, but not the
> 	  * other way around
> 	  */
>          (is_hardware_domain(d) || !allowed_conf[i].hwdom) &&
> 	 (emflags & ~allowed_conf[i].optional) == allowed_conf[i].base )
>         return true;
> }
> 
> printk(XENLOG_INFO "%pd: invalid emulation flags: %#x\n", d, emflags);
> 
> return false;
> 
> I think the above (not even build tested) is slightly clearer, and
> allows for easier expansion going forward?

I like the idea! Thanks for the suggestion.

I will wait a bit to collect some feedback, if any, before doing coding.

> 
> Regards, Roger.