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

[Oleksandr Andrushchenko <andr2000@xxxxxxxxx>]

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?

> > --- a/xen/drivers/vpci/header.c
> > +++ b/xen/drivers/vpci/header.c
> > @@ -96,8 +96,10 @@ static void modify_decoding(const struct pci_dev *pdev, 
> > uint16_t cmd,
> >        * FIXME: punching holes after the p2m has been set up might be racy for
> >        * DomU usage, needs to be revisited.
> >        */
> > +#ifdef CONFIG_HAS_PCI_MSI
> >       if ( map && !rom_only && vpci_make_msix_hole(pdev) )
> >           return;
> > +#endif
> (This would be another such place.)
>
> > --- a/xen/include/asm-arm/domain.h
> > +++ b/xen/include/asm-arm/domain.h
> > @@ -262,7 +262,10 @@ static inline void arch_vcpu_block(struct vcpu *v) {}
> >   
> >   #define arch_vm_assist_valid_mask(d) (1UL << VMASST_TYPE_runstate_update_flag)
> >   
> > -#define has_vpci(d)    ({ (void)(d); false; })
> > +/* For X86 VPCI is enabled and tested for PVH DOM0 only but
> > + * for ARM we enable support VPCI for guest domain also.
> > + */
> Comment style (/* goes on its own line).
>
> > +#define has_vpci(d) ({ (void)(d); IS_ENABLED(CONFIG_HAS_VPCI); })
> Personally I'd recommend to get away without using extensions whenever
> possible, i.e. use
>
> #define has_vpci(d) ((void)(d), IS_ENABLED(CONFIG_HAS_VPCI))
>
> here.
>
> > --- a/xen/include/asm-arm/pci.h
> > +++ b/xen/include/asm-arm/pci.h
> > @@ -27,6 +27,14 @@ struct arch_pci_dev {
> >       struct device dev;
> >   };
> >   
> > +/* Arch-specific MSI data for vPCI. */
> > +struct vpci_arch_msi {
> > +};
> > +
> > +/* Arch-specific MSI-X entry data for vPCI. */
> > +struct vpci_arch_msix_entry {
> > +};
> But isn't it that you don't support vPCI's MSI in the first place?
> Perhaps the need for these would go away with CONFIG_HAS_VCPI_MSI?
>
> Jan