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

[Jan Beulich <jbeulich@suse.com>]

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?

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