Re: [PATCH v5 2/5] xen/vpci: move xen_domctl_createdomain vPCI flag to common

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.11.2023 20:59, Stewart Hildebrand wrote:
> --- a/tools/libs/light/libxl_x86.c
> +++ b/tools/libs/light/libxl_x86.c
> @@ -8,13 +8,16 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>  {
>      switch(d_config->c_info.type) {
>      case LIBXL_DOMAIN_TYPE_HVM:
> -        config->arch.emulation_flags = (XEN_X86_EMU_ALL & ~XEN_X86_EMU_VPCI);
> +        config->arch.emulation_flags = XEN_X86_EMU_ALL;
> +        config->flags &= ~XEN_DOMCTL_CDF_vpci;
>          break;
>      case LIBXL_DOMAIN_TYPE_PVH:
>          config->arch.emulation_flags = XEN_X86_EMU_LAPIC;
> +        config->flags &= ~XEN_DOMCTL_CDF_vpci;
>          break;
>      case LIBXL_DOMAIN_TYPE_PV:
>          config->arch.emulation_flags = 0;
> +        config->flags &= ~XEN_DOMCTL_CDF_vpci;
>          break;

Why all this explicit clearing of XEN_DOMCTL_CDF_vpci? I can't even spot
where the bit would be set.

> --- a/tools/python/xen/lowlevel/xc/xc.c
> +++ b/tools/python/xen/lowlevel/xc/xc.c
> @@ -159,7 +159,10 @@ static PyObject *pyxc_domain_create(XcObject *self,
>  
>  #if defined (__i386) || defined(__x86_64__)
>      if ( config.flags & XEN_DOMCTL_CDF_hvm )
> -        config.arch.emulation_flags = (XEN_X86_EMU_ALL & ~XEN_X86_EMU_VPCI);
> +    {
> +        config.arch.emulation_flags = XEN_X86_EMU_ALL;
> +        config.flags &= ~XEN_DOMCTL_CDF_vpci;
> +    }

Same question here then.

> @@ -575,6 +577,18 @@ static int sanitise_domain_config(struct 
> xen_domctl_createdomain *config)
>          return -EINVAL;
>      }
>  
> +    if ( vpci && !hvm )
> +    {
> +        dprintk(XENLOG_INFO, "vPCI requested for non-HVM guest\n");
> +        return -EINVAL;
> +    }
> +
> +    if ( vpci && !IS_ENABLED(CONFIG_HAS_VPCI) )
> +    {
> +        dprintk(XENLOG_INFO, "vPCI requested but not enabled\n");
> +        return -EINVAL;
> +    }

Maybe flip the order of these checks? But I'm uncertain about the first
one anyway: Isn't this something that needs deciding per-arch?

> --- a/xen/include/public/arch-x86/xen.h
> +++ b/xen/include/public/arch-x86/xen.h
> @@ -283,15 +283,12 @@ struct xen_arch_domainconfig {
>  #define XEN_X86_EMU_PIT             (1U<<_XEN_X86_EMU_PIT)
>  #define _XEN_X86_EMU_USE_PIRQ       9
>  #define XEN_X86_EMU_USE_PIRQ        (1U<<_XEN_X86_EMU_USE_PIRQ)
> -#define _XEN_X86_EMU_VPCI           10
> -#define XEN_X86_EMU_VPCI            (1U<<_XEN_X86_EMU_VPCI)

This, aiui, being the reason for ...

> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -21,7 +21,7 @@
>  #include "hvm/save.h"
>  #include "memory.h"
>  
> -#define XEN_DOMCTL_INTERFACE_VERSION 0x00000016
> +#define XEN_DOMCTL_INTERFACE_VERSION 0x00000017

... this bump, I wonder whether nevertheless we wouldn't better leave a
comment there to indicate that bit 10 better wouldn't be used again any
time soon.

> @@ -55,9 +55,12 @@ struct xen_domctl_createdomain {
>  #define XEN_DOMCTL_CDF_nested_virt    (1U << _XEN_DOMCTL_CDF_nested_virt)
>  /* Should we expose the vPMU to the guest? */
>  #define XEN_DOMCTL_CDF_vpmu           (1U << 7)
> +/* Should vPCI be enabled for the guest? */
> +#define _XEN_DOMCTL_CDF_vpci          8

What is this needed for besides ...

> +#define XEN_DOMCTL_CDF_vpci           (1U<<_XEN_DOMCTL_CDF_vpci)

... its use here? Imo like was done for vpmu, there should be only one
new identifier. As an aside, there are blanks missing around <<.

> --- a/xen/include/xen/domain.h
> +++ b/xen/include/xen/domain.h
> @@ -51,6 +51,9 @@ void arch_get_domain_info(const struct domain *d,
>  
>  #define is_domain_using_staticmem(d) ((d)->cdf & CDF_staticmem)
>  
> +#define has_vpci(d) (((d)->options & XEN_DOMCTL_CDF_vpci) && \
> +                     IS_ENABLED(CONFIG_HAS_VPCI))

Aiui the IS_ENABLED() is wanted so where suitable code conditional upon
this predicate can be eliminated by the compiler. Question is whether
we can't achieve this by, say, overriding XEN_DOMCTL_CDF_vpci to 0 in
such cases (without touching what you add to the public header, i.e.
not as easy as what we do in xen/arch/x86/include/asm/domain.h for
X86_EMU_*).

Jan