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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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?

Regards, Roger.