Re: [PATCH v18 1/2] xen/arm: translate virtual PCI bus topology for guests

[Julien Grall <julien@xxxxxxx>]

Hi Stewart,

I realize this series is at v18, but there are a few bits security-wise 
that concerns me. They don't have to be handled now because vPCI is 
still in tech preview. But we probably want to keep track of any issues 
if this hasn't yet been done in the code.
On 25/03/2025 17:27, Stewart Hildebrand wrote:
> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
> index 1e6aa5d799b9..49c9444195d7 100644
> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -81,6 +81,32 @@ static int assign_virtual_sbdf(struct pci_dev *pdev)
>       return 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 pci_dev *pdev;
> +
> +    ASSERT(!is_hardware_domain(d));
> +    read_lock(&d->pci_lock);
> +
> +    for_each_pdev ( d, pdev )
I can't remember whether this was discussed before (there is no 
indication in the commit message). What's the maximum number of PCI 
devices supported? Do we limit it?
Asking because iterating through the list could end up to be quite 
expensive.
Also, not a change for this patch, but it seems vcpi_{read,write} will 
also do another lookup. This is quite innefficient. We should consider 
return a pdev and use it for vcpi_{read,write}.
> +    {
> +        if ( pdev->vpci && (pdev->vpci->guest_sbdf.sbdf == sbdf->sbdf) )
> +        {
> +            /* Replace guest SBDF with the physical one. */
> +            *sbdf = pdev->sbdf;
> +            read_unlock(&d->pci_lock);
> +            return true;
> +        }
> +    }
> +
> +    read_unlock(&d->pci_lock);
At the point this is unlocked, what prevent the sbdf to be detached from 
the domain? If nothing, would this mean the domain can access something 
it should not?
> +    return false;> +}
> +
>   #endif /* CONFIG_HAS_VPCI_GUEST_SUPPORT */
>   
>   void vpci_deassign_device(struct pci_dev *pdev)
> diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
> index 807401b2eaa2..e355329913ef 100644
> --- a/xen/include/xen/vpci.h
> +++ b/xen/include/xen/vpci.h
> @@ -311,6 +311,18 @@ static inline int __must_check vpci_reset_device(struct 
> pci_dev *pdev)
>       return vpci_assign_device(pdev);
>   }
>   
> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
> +bool vpci_translate_virtual_device(struct domain *d, pci_sbdf_t *sbdf);
> +#else
> +static inline bool vpci_translate_virtual_device(struct domain *d,
> +                                                 pci_sbdf_t *sbdf)
> +{
> +    ASSERT_UNREACHABLE();
> +
> +    return false;
> +}
> +#endif
> +
>   #endif
>   
>   /*
Cheers,

--
Julien Grall