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

[dmkhn@xxxxxxxxx]

On Tue, Apr 08, 2025 at 05:34:27PM +0200, Jan Beulich wrote:
> On 01.04.2025 02:52, dmkhn@xxxxxxxxx wrote:
> > From: Denis Mukhin <dmukhin@xxxxxxxx>
> >
> > Rewrite emulation_flags_ok() using XEN_X86_EMU_{OPTIONAL,BASELINE}
> > to simplify future modifications.
> >
> > Signed-off-by: Denis Mukhin <dmukhin@xxxxxxxx>
> > ---
> > Came in the context of NS16550 emulator v3 series:
> >   
> > https://lore.kernel.org/xen-devel/20250103-vuart-ns8250-v3-v1-0-c5d36b31d66c@xxxxxxxx/
> >
> > 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