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

[dmkhn@xxxxxxxxx]

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@xxxxxxxxx wrote:
> > From: Denis Mukhin <dmukhin@xxxxxxxx>
> >
> > 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@xxxxxxxx>
> > ---
> > 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.