On Thu, Sep 30, 2021 at 10:52:22AM +0300, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushche...@epam.com>
>
> Assign SBDF to the PCI devices being passed through with bus 0.
> The resulting topology is where PCIe devices reside on the bus 0 of the
> root complex itself (embedded endpoints).
> This implementation is limited to 32 devices which are allowed on
> a single PCI bus.
>
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushche...@epam.com>
>
> ---
> Since v2:
> - remove casts that are (a) malformed and (b) unnecessary
> - add new line for better readability
> - remove CONFIG_HAS_VPCI_GUEST_SUPPORT ifdef's as the relevant vPCI
> functions are now completely gated with this config
> - gate common code with CONFIG_HAS_VPCI_GUEST_SUPPORT
> New in v2
> ---
> xen/common/domain.c | 3 ++
> xen/drivers/passthrough/pci.c | 60 +++++++++++++++++++++++++++++++++++
> xen/drivers/vpci/vpci.c | 14 +++++++-
> xen/include/xen/pci.h | 22 +++++++++++++
> xen/include/xen/sched.h | 8 +++++
> 5 files changed, 106 insertions(+), 1 deletion(-)
>
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 40d67ec34232..e0170087612d 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -601,6 +601,9 @@ struct domain *domain_create(domid_t domid,
>
> #ifdef CONFIG_HAS_PCI
> INIT_LIST_HEAD(&d->pdev_list);
> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
> + INIT_LIST_HEAD(&d->vdev_list);
> +#endif
> #endif
>
> /* All error paths can depend on the above setup. */
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index 805ab86ed555..5b963d75d1ba 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -831,6 +831,66 @@ int pci_remove_device(u16 seg, u8 bus, u8 devfn)
> return ret;
> }
>
> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
> +static struct vpci_dev *pci_find_virtual_device(const struct domain *d,
> + const struct pci_dev *pdev)
> +{
> + struct vpci_dev *vdev;
> +
> + list_for_each_entry ( vdev, &d->vdev_list, list )
> + if ( vdev->pdev == pdev )
> + return vdev;
> + return NULL;
> +}
> +
> +int pci_add_virtual_device(struct domain *d, const struct pci_dev *pdev)
> +{
> + struct vpci_dev *vdev;
> +
> + ASSERT(!pci_find_virtual_device(d, pdev));
> +
> + /* Each PCI bus supports 32 devices/slots at max. */
> + if ( d->vpci_dev_next > 31 )
> + return -ENOSPC;
> +
> + vdev = xzalloc(struct vpci_dev);
> + if ( !vdev )
> + return -ENOMEM;
> +
> + /* We emulate a single host bridge for the guest, so segment is always
> 0. */
> + vdev->seg = 0;
> +
> + /*
> + * The bus number is set to 0, so virtual devices are seen
> + * as embedded endpoints behind the root complex.
> + */
> + vdev->bus = 0;
> + vdev->devfn = PCI_DEVFN(d->vpci_dev_next++, 0);
This would likely be better as a bitmap where you set the bits of
in-use slots. Then you can use find_first_bit or similar to get a free
slot.
Long term you might want to allow the caller to provide a pre-selected
slot, as it's possible for users to request the device to appear at a
specific slot on the emulated bus.
> +
> + vdev->pdev = pdev;
> + vdev->domain = d;
> +
> + pcidevs_lock();
> + list_add_tail(&vdev->list, &d->vdev_list);
> + pcidevs_unlock();
> +
> + return 0;
> +}
> +
> +int pci_remove_virtual_device(struct domain *d, const struct pci_dev *pdev)
> +{
> + struct vpci_dev *vdev;
> +
> + pcidevs_lock();
> + vdev = pci_find_virtual_device(d, pdev);
> + if ( vdev )
> + list_del(&vdev->list);
> + pcidevs_unlock();
> + xfree(vdev);
> + return 0;
> +}
> +#endif /* CONFIG_HAS_VPCI_GUEST_SUPPORT */
> +
> /* Caller should hold the pcidevs_lock */
> static int deassign_device(struct domain *d, uint16_t seg, uint8_t bus,
> uint8_t devfn)
> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
> index 702f7b5d5dda..d787f13e679e 100644
> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -91,20 +91,32 @@ int __hwdom_init vpci_add_handlers(struct pci_dev *pdev)
> /* Notify vPCI that device is assigned to guest. */
> int vpci_assign_device(struct domain *d, const struct pci_dev *dev)
> {
> + int rc;
> +
> /* It only makes sense to assign for hwdom or guest domain. */
> if ( is_system_domain(d) || !has_vpci(d) )
> return 0;
>
> - return vpci_bar_add_handlers(d, dev);
> + rc = vpci_bar_add_handlers(d, dev);
> + if ( rc )
> + return rc;
> +
> + return pci_add_virtual_device(d, dev);
> }
>
> /* Notify vPCI that device is de-assigned from guest. */
> int vpci_deassign_device(struct domain *d, const struct pci_dev *dev)
> {
> + int rc;
> +
> /* It only makes sense to de-assign from hwdom or guest domain. */
> if ( is_system_domain(d) || !has_vpci(d) )
> return 0;
>
> + rc = pci_remove_virtual_device(d, dev);
> + if ( rc )
> + return rc;
> +
> return vpci_bar_remove_handlers(d, dev);
> }
> #endif /* CONFIG_HAS_VPCI_GUEST_SUPPORT */
> diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
> index 43b8a0817076..33033a3a8f8d 100644
> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -137,6 +137,24 @@ struct pci_dev {
> struct vpci *vpci;
> };
>
> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
> +struct vpci_dev {
> + struct list_head list;
> + /* Physical PCI device this virtual device is connected to. */
> + const struct pci_dev *pdev;
> + /* Virtual SBDF of the device. */
> + union {
> + struct {
> + uint8_t devfn;
> + uint8_t bus;
> + uint16_t seg;
> + };
> + pci_sbdf_t sbdf;
> + };
> + struct domain *domain;
> +};
> +#endif
I wonder whether this is strictly needed. Won't it be enough to store
the virtual (ie: guest) sbdf inside the existing vpci struct?
It would avoid the overhead of the translation you do from pdev ->
vdev, and there doesn't seem to be anything relevant stored in
vpci_dev apart from the virtual sbdf.
Thanks, Roger.
