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
