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

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 10/31/23 06:56, Jan Beulich wrote:
> 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?

Hm. Allowing/enabling vPCI for dom0 on ARM should follow or be part of the PCI 
passthrough SMMU series [1]. I'll move this change to the next patch ("xen/arm: 
enable vPCI for dom0") and add a note over there.

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

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

OK, I'll change both to unsigned int.

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

OK, I'll fix.

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

Yes, I will move it to xen/common/domain.c:sanitise_domain_config().

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

I'll change it.

> 
> Jan