On Wed, May 21, 2025 at 05:07:12PM +0200, Roger Pau Monné wrote:
> On Fri, May 16, 2025 at 02:29:16AM +0000, dm...@proton.me wrote:
> > From: Denis Mukhin <dmuk...@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 <dmuk...@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.
