Re: [PATCH v2 14/17] xen/arm: Enable the existing x86 virtual PCI support for ARM.

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hi Jan

> On 24 Sep 2021, at 8:44 am, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> On 22.09.2021 13:35, Rahul Singh wrote:
>> @@ -623,7 +624,7 @@ int arch_sanitise_domain_config(struct 
>> xen_domctl_createdomain *config)
>>     unsigned int max_vcpus;
>> 
>>     /* HVM and HAP must be set. IOMMU may or may not be */
>> -    if ( (config->flags & ~XEN_DOMCTL_CDF_iommu) !=
>> +    if ( (config->flags & ~XEN_DOMCTL_CDF_iommu & ~XEN_DOMCTL_CDF_vpci) !=
>>          (XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap) )
>>     {
>>         dprintk(XENLOG_INFO, "Unsupported configuration %#x\n",
> 
> While you accept the new flag here and ...
> 
>> --- a/xen/common/domain.c
>> +++ b/xen/common/domain.c
>> @@ -483,7 +483,7 @@ static int sanitise_domain_config(struct 
>> xen_domctl_createdomain *config)
>>          ~(XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap |
>>            XEN_DOMCTL_CDF_s3_integrity | XEN_DOMCTL_CDF_oos_off |
>>            XEN_DOMCTL_CDF_xs_domain | XEN_DOMCTL_CDF_iommu |
>> -           XEN_DOMCTL_CDF_nested_virt) )
>> +           XEN_DOMCTL_CDF_nested_virt | XEN_DOMCTL_CDF_vpci) )
>>     {
>>         dprintk(XENLOG_INFO, "Unknown CDF flags %#x\n", config->flags);
>>         return -EINVAL;
> 
> ... here, you need to somehow reject it on x86, until DomU support
> there gets added (unless I have misunderstood things and you're
> aiming at enabing that support for x86 here at the same time). I
> cannot spot existing code which would take care of such a newly
> added flag.

Ok. I will reject the flag in x86 arch_sanitise_domain_config().
> 
> 
>> --- a/xen/include/asm-x86/pci.h
>> +++ b/xen/include/asm-x86/pci.h
>> @@ -6,8 +6,6 @@
>> #define CF8_ADDR_HI(cf8) (  ((cf8) & 0x0f000000) >> 16)
>> #define CF8_ENABLED(cf8) (!!((cf8) & 0x80000000))
>> 
>> -#define MMCFG_BDF(addr)  ( ((addr) & 0x0ffff000) >> 12)
> 
> While there was a reason for the padding blank after the first
> opening parentheses here, ...
> 
>> --- a/xen/include/xen/pci.h
>> +++ b/xen/include/xen/pci.h
>> @@ -41,6 +41,8 @@
>> #define PCI_SBDF3(s,b,df) \
>>     ((pci_sbdf_t){ .sbdf = (((s) & 0xffff) << 16) | PCI_BDF2(b, df) })
>> 
>> +#define MMCFG_BDF(addr)  ( ((addr) & 0x0ffff000) >> 12)
> 
> ... that blank ends up bogus here.
Ack . I will remove the extra blank in next version.

Regards,
Rahul
> 
> Jan
>