Re: [PATCH V7 10/11] xen/arm: translate virtual PCI bus topology for guests

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.07.2022 19:42, Oleksandr Tyshchenko wrote:
> --- a/xen/arch/arm/vpci.c
> +++ b/xen/arch/arm/vpci.c
> @@ -41,6 +41,16 @@ static int vpci_mmio_read(struct vcpu *v, mmio_info_t 
> *info,
>      /* data is needed to prevent a pointer cast on 32bit */
>      unsigned long data;
>  
> +    /*
> +     * For the passed through devices we need to map their virtual SBDF
> +     * to the physical PCI device being passed through.
> +     */
> +    if ( !bridge && !vpci_translate_virtual_device(v->domain, &sbdf) )
> +    {
> +        *r = ~0ul;
> +        return 1;
> +    }

I'm probably simply lacking specific Arm-side knowledge, but it strikes
me as odd that the need for translation would be dependent upon "bridge".

> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -158,6 +158,32 @@ static void vpci_remove_virtual_device(const struct 
> pci_dev *pdev)
>      }
>  }
>  
> +/*
> + * Find the physical device which is mapped to the virtual device
> + * and translate virtual SBDF to the physical one.
> + */
> +bool vpci_translate_virtual_device(struct domain *d, pci_sbdf_t *sbdf)
> +{
> +    struct pci_dev *pdev;

const wherever possible please (i.e. likely also for the first function
parameter).

> --- a/xen/include/xen/vpci.h
> +++ b/xen/include/xen/vpci.h
> @@ -276,6 +276,7 @@ static inline bool __must_check 
> vpci_process_pending(struct vcpu *v)
>  /* Notify vPCI that device is assigned/de-assigned to/from guest. */
>  int vpci_assign_device(struct pci_dev *pdev);
>  void vpci_deassign_device(struct pci_dev *pdev);
> +bool vpci_translate_virtual_device(struct domain *d, pci_sbdf_t *sbdf);
>  #else
>  static inline int vpci_assign_device(struct pci_dev *pdev)
>  {
> @@ -285,6 +286,12 @@ static inline int vpci_assign_device(struct pci_dev 
> *pdev)
>  static inline void vpci_deassign_device(struct pci_dev *pdev)
>  {
>  };
> +
> +static inline bool vpci_translate_virtual_device(struct domain *d,
> +                                                 pci_sbdf_t *sbdf)
> +{
> +    return false;
> +}

Please don't add stubs which aren't really needed (which, afaict, is the
case for this one).

Jan