1. Patchew
  2. Xen
  3. [PATCH v1 0/3] xen/domain: updates to hardware emulation flags
  4. View patch

[PATCH v1 3/3] xen/domain: rewrite emulation_flags_ok()

dmkhn@proton.me posted 3 patches 7 months ago
There is a newer version of this series
[PATCH v1 3/3] xen/domain: rewrite emulation_flags_ok()
Posted by dmkhn@proton.me 7 months ago 
From: Denis Mukhin <dmukhin@ford.com>

Rewrite emulation_flags_ok() using XEN_X86_EMU_{OPTIONAL,BASELINE}
to simplify future modifications.

Signed-off-by: Denis Mukhin <dmukhin@ford.com>
---
Came in the context of NS16550 emulator v3 series:
  https://lore.kernel.org/xen-devel/20250103-vuart-ns8250-v3-v1-0-c5d36b31d66c@ford.com/

After modifying emulation_flags_ok() with a new NS16550 vUART
configuration switch passed from the toolstack for the HVM
case, I decided to look into how to improve emulation_flags_ok().
---
 xen/arch/x86/domain.c | 29 +++++++++++------------------
 1 file changed, 11 insertions(+), 18 deletions(-)

diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 15c5e2a652..23051bb176 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 or classic PVH */
+    if ( !is_hvm_domain(d) )
+        return emflags == 0 || emflags == XEN_X86_EMU_PIT;
 
-    return true;
+    /* HVM */
+    if ( is_hardware_domain(d) )
+        return emflags == (XEN_X86_EMU_LAPIC |
+                           XEN_X86_EMU_IOAPIC |
+                           XEN_X86_EMU_VPCI);
+
+    return (emflags & ~XEN_X86_EMU_OPTIONAL) == XEN_X86_EMU_BASELINE ||
+            emflags == XEN_X86_EMU_LAPIC;
 }
 
 void __init arch_init_idle_domain(struct domain *d)
-- 
2.34.1
Re: [PATCH v1 3/3] xen/domain: rewrite emulation_flags_ok()
Posted by Jan Beulich 6 months, 3 weeks ago 
On 01.04.2025 02:52, dmkhn@proton.me wrote:
> From: Denis Mukhin <dmukhin@ford.com>
> 
> Rewrite emulation_flags_ok() using XEN_X86_EMU_{OPTIONAL,BASELINE}
> to simplify future modifications.
> 
> Signed-off-by: Denis Mukhin <dmukhin@ford.com>
> ---
> Came in the context of NS16550 emulator v3 series:
>   https://lore.kernel.org/xen-devel/20250103-vuart-ns8250-v3-v1-0-c5d36b31d66c@ford.com/
> 
> After modifying emulation_flags_ok() with a new NS16550 vUART
> configuration switch passed from the toolstack for the HVM
> case, I decided to look into how to improve emulation_flags_ok().
> ---
>  xen/arch/x86/domain.c | 29 +++++++++++------------------
>  1 file changed, 11 insertions(+), 18 deletions(-)

There is a readability win, yes.

> --- 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 or classic PVH */
> +    if ( !is_hvm_domain(d) )
> +        return emflags == 0 || emflags == XEN_X86_EMU_PIT;

What's "classic PVH" here? This got to be PVHv1, which is dead. As you touch /
move such a comment, you want to adjust it so it's at least no longer stale.

> -    return true;
> +    /* HVM */
> +    if ( is_hardware_domain(d) )
> +        return emflags == (XEN_X86_EMU_LAPIC |
> +                           XEN_X86_EMU_IOAPIC |
> +                           XEN_X86_EMU_VPCI);
> +
> +    return (emflags & ~XEN_X86_EMU_OPTIONAL) == XEN_X86_EMU_BASELINE ||
> +            emflags == XEN_X86_EMU_LAPIC;

There was a comment about X86_EMU_USE_PIRQ being optional, which you've lost
together with (only) that (i.e. not at the same time including vPCI) optionality.

Furthermore you move from X86_EMU_* namespace to XEN_X86_EMU_* one without even
mentioning that (and the reason to do so) in the description. Aiui the function
deliberately uses the internal names, not the public interface ones.

Jan
Re: [PATCH v1 3/3] xen/domain: rewrite emulation_flags_ok()
Posted by dmkhn@proton.me 6 months, 1 week ago 
On Tue, Apr 08, 2025 at 05:34:27PM +0200, Jan Beulich wrote:
> On 01.04.2025 02:52, dmkhn@proton.me wrote:
> > From: Denis Mukhin <dmukhin@ford.com>
> >
> > Rewrite emulation_flags_ok() using XEN_X86_EMU_{OPTIONAL,BASELINE}
> > to simplify future modifications.
> >
> > Signed-off-by: Denis Mukhin <dmukhin@ford.com>
> > ---
> > Came in the context of NS16550 emulator v3 series:
> >   https://lore.kernel.org/xen-devel/20250103-vuart-ns8250-v3-v1-0-c5d36b31d66c@ford.com/
> >
> > After modifying emulation_flags_ok() with a new NS16550 vUART
> > configuration switch passed from the toolstack for the HVM
> > case, I decided to look into how to improve emulation_flags_ok().
> > ---
> >  xen/arch/x86/domain.c | 29 +++++++++++------------------
> >  1 file changed, 11 insertions(+), 18 deletions(-)
> 
> There is a readability win, yes.
> 
> > --- 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 or classic PVH */
> > +    if ( !is_hvm_domain(d) )
> > +        return emflags == 0 || emflags == XEN_X86_EMU_PIT;
> 
> What's "classic PVH" here? This got to be PVHv1, which is dead. As you touch /
> move such a comment, you want to adjust it so it's at least no longer stale.

Looks like the comment should say "PV".

> 
> > -    return true;
> > +    /* HVM */
> > +    if ( is_hardware_domain(d) )
> > +        return emflags == (XEN_X86_EMU_LAPIC |
> > +                           XEN_X86_EMU_IOAPIC |
> > +                           XEN_X86_EMU_VPCI);
> > +
> > +    return (emflags & ~XEN_X86_EMU_OPTIONAL) == XEN_X86_EMU_BASELINE ||
> > +            emflags == XEN_X86_EMU_LAPIC;
> 
> There was a comment about X86_EMU_USE_PIRQ being optional, which you've lost
> together with (only) that (i.e. not at the same time including vPCI) optionality.
> 
> Furthermore you move from X86_EMU_* namespace to XEN_X86_EMU_* one without even
> mentioning that (and the reason to do so) in the description. Aiui the function
> deliberately uses the internal names, not the public interface ones.
> 
> Jan

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.