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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 31.10.2023 00:52, Stewart Hildebrand wrote:
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -607,7 +607,8 @@ int arch_sanitise_domain_config(struct 
> xen_domctl_createdomain *config)
>  {
>      unsigned int max_vcpus;
>      unsigned int flags_required = (XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap);
> -    unsigned int flags_optional = (XEN_DOMCTL_CDF_iommu | 
> XEN_DOMCTL_CDF_vpmu);
> +    unsigned int flags_optional = (XEN_DOMCTL_CDF_iommu | 
> XEN_DOMCTL_CDF_vpmu |
> +                                   XEN_DOMCTL_CDF_vpci);

Is the flag (going to be, with the initial work) okay to have for Dom0
on Arm?

> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -712,7 +712,8 @@ int arch_sanitise_domain_config(struct 
> xen_domctl_createdomain *config)
>      return 0;
>  }
>  
> -static bool emulation_flags_ok(const struct domain *d, uint32_t emflags)
> +static bool emulation_flags_ok(const struct domain *d, uint32_t emflags,
> +                               uint32_t cdf)

While apparently views differ, ./CODING_STYLE wants "unsigned int" to be
used for the latter two arguments.

> @@ -722,14 +723,17 @@ static bool emulation_flags_ok(const struct domain *d, 
> uint32_t emflags)
>      if ( is_hvm_domain(d) )
>      {
>          if ( is_hardware_domain(d) &&
> -             emflags != (X86_EMU_VPCI | X86_EMU_LAPIC | X86_EMU_IOAPIC) )
> +             (!( cdf & XEN_DOMCTL_CDF_vpci ) ||

Nit: Stray blanks inside the inner parentheses.

> +              emflags != (X86_EMU_LAPIC | X86_EMU_IOAPIC)) )
>              return false;
>          if ( !is_hardware_domain(d) &&
> -             emflags != (X86_EMU_ALL & ~X86_EMU_VPCI) &&
> -             emflags != X86_EMU_LAPIC )
> +             ((cdf & XEN_DOMCTL_CDF_vpci) ||
> +              (emflags != X86_EMU_ALL &&
> +               emflags != X86_EMU_LAPIC)) )
>              return false;
>      }
> -    else if ( emflags != 0 && emflags != X86_EMU_PIT )
> +    else if ( (cdf & XEN_DOMCTL_CDF_vpci) ||

Wouldn't this better be enforced in common code?

> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -892,10 +892,11 @@ static struct domain *__init create_dom0(const module_t 
> *image,
>      {
>          dom0_cfg.flags |= (XEN_DOMCTL_CDF_hvm |
>                             ((hvm_hap_supported() && !opt_dom0_shadow) ?
> -                            XEN_DOMCTL_CDF_hap : 0));
> +                            XEN_DOMCTL_CDF_hap : 0) |
> +                           XEN_DOMCTL_CDF_vpci);

Less of a change and imo slightly neater as a result would be to simply
put the addition on the same line where CDF_hvm already is. But as with
many style aspects, views may differ here of course ...

Jan