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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.11.2021 07:56, Oleksandr Andrushchenko wrote:
> --- a/xen/arch/arm/vpci.c
> +++ b/xen/arch/arm/vpci.c
> @@ -41,6 +41,15 @@ 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;
>  
> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
> +    /*
> +     * 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) )
> +            return 1;

Nit: Indentation.

> @@ -59,6 +68,15 @@ static int vpci_mmio_write(struct vcpu *v, mmio_info_t 
> *info,
>      struct pci_host_bridge *bridge = p;
>      pci_sbdf_t sbdf = vpci_sbdf_from_gpa(bridge, info->gpa);
>  
> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
> +    /*
> +     * 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) )
> +            return 1;

Again.

> @@ -172,10 +175,37 @@ REGISTER_VPCI_INIT(vpci_add_virtual_device, 
> VPCI_PRIORITY_MIDDLE);
>  static void vpci_remove_virtual_device(struct domain *d,
>                                         const struct pci_dev *pdev)
>  {
> +    ASSERT(pcidevs_locked());
> +
>      clear_bit(pdev->vpci->guest_sbdf.dev, &d->vpci_dev_assigned_map);
>      pdev->vpci->guest_sbdf.sbdf = ~0;
>  }
>  
> +/*
> + * 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)

const struct domain *d ?

> +{
> +    const struct pci_dev *pdev;
> +    bool found = false;
> +
> +    pcidevs_lock();
> +    for_each_pdev( d, pdev )
> +    {
> +        if ( pdev->vpci->guest_sbdf.sbdf == sbdf->sbdf )
> +        {
> +            /* Replace virtual SBDF with the physical one. */
> +            *sbdf = pdev->sbdf;
> +            found = true;
> +            break;
> +        }
> +    }
> +    pcidevs_unlock();

I think the description wants to at least mention that in principle
this is too coarse grained a lock, providing justification for why
it is deemed good enough nevertheless. (Personally, as expressed
before, I don't think the lock should be used here, but as long as
Roger agrees with you, you're fine.)

Jan