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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.08.2021 07:44, Oleksandr Andrushchenko wrote:
> Hi, Jan!
> 
> On 24.08.21 19:09, Jan Beulich wrote:
>> On 19.08.2021 14:02, Rahul Singh wrote:
>>> --- a/xen/drivers/passthrough/pci.c
>>> +++ b/xen/drivers/passthrough/pci.c
>>> @@ -767,6 +767,13 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
>>>       else
>>>           iommu_enable_device(pdev);
>>>   
>>> +#ifdef CONFIG_ARM
>>> +    ret = vpci_add_handlers(pdev);
>>> +    if ( ret ) {
>>> +        printk(XENLOG_ERR "setup of vPCI for failed: %d\n",ret);
>>> +        goto out;
>>> +    }
>>> +#endif
>>>       pci_enable_acs(pdev);
>> I'm afraid I can't see why this is to be Arm-specific. The present
>> placement of the existing call to vpci_add_handlers() looks to be
>> sub-optimal anyway - did you look into whether it could be moved
>> to a place (potentially right here) fitting everyone? If you did,
>> would mind justifying the Arm-specific code in a non-Arm-specific
>> file in at least a post-commit-message remark?
>>
>> If it were to remain like this, please add a blank line after the #endif.
>>
>>> --- a/xen/drivers/vpci/Makefile
>>> +++ b/xen/drivers/vpci/Makefile
>>> @@ -1 +1,2 @@
>>> -obj-y += vpci.o header.o msi.o msix.o
>>> +obj-y += vpci.o header.o
>>> +obj-$(CONFIG_HAS_PCI_MSI) += msi.o msix.o
>> I continue to consider this a wrong connection - HAS_PCI_MSI expresses
>> (quoting text from patch 1 of this series) "code that implements MSI
>> functionality to support MSI within XEN", while here we're talking of
>> vPCI (i.e. guest support). I can accept that this is transiently the
>> best you can do, but then please add a comment to this effect (if
>> need be in multiple places), such that future readers or people
>> wanting to further adjust this understand why it is the way it is.
>>
>> But perhaps you instead want to introduce a HAS_VPCI_MSI (or, less
>> desirable because of possible ambiguity, HAS_VMSI) Kconfig option?
> 
> Eventually we'll have the code like that:
> 
> obj-y += vpci.o header.o msi.o msix.o
> obj-$(CONFIG_X86) += x86/
> obj-$(CONFIG_ARM) += arm/
> 
> So, this is indeed a transitional change. Will you be ok with a comment
> about that then?

Well, yes, as said - if this is a transitional state, then a comment
will do. I was suggesting the further Kconfig option merely because
all I've been hearing so far was that MSI works entirely differently
on Arm, and hence the MSI code we have is not going to be used there
at all. If that was a correct understanding of mine (including its
extending to vPCI's MSI code), then adding just a comment would
continue to be misleading imo.

Jan