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

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 11/13/23 08:26, Jan Beulich wrote:
> 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.

You're right, it's not being set currently, so no need to explicitly clear the 
bit here. I'll remove.

> 
>> --- 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.

Same answer here.

> 
>> @@ -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?

In v4, the equivalent of the ( vpci && !hvm ) check was indeed in 
xen/arch/x86/domain.c:emulation_flags_ok(), but it seemed there was a 
suggestion that it be moved to common code... See discussion at [1]. How about 
putting it back into xen/arch/x86/domain.c, in arch_sanitise_domain_config(), 
not emulation_flags_ok()?

[1] https://lists.xenproject.org/archives/html/xen-devel/2023-10/msg02345.html

> 
>> --- 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.

I'll add a comment (above, in arch-x86/xen.h)

> 
>> @@ -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 <<.

OK, I'll fix this

> 
>> --- 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_*).

I had only added the IS_ENABLED() here due to some (unnecessary) related 
changes in the later patch ("xen/arm: enable vPCI for domUs"). I hadn't 
considered the compiler optimization aspect. I can understand the potential 
benefit, but I'm not ready to introduce such complexity at this time. I'm in 
favor of "simpler is better" in this case, so I'll change it back to how it was 
in v4:

#define has_vpci(d) (!!((d)->options & XEN_DOMCTL_CDF_vpci))

> 
> Jan