On Tue, Aug 29, 2023 at 11:19:46PM +0000, Volodymyr Babchuk 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.
>
> Please note, that at the moment only function 0 of a multifunction
> device can be passed through.
>
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushche...@epam.com>
> ---
> Since v9:
> - Lock in add_virtual_device() replaced with ASSERT (thanks, Stewart)
> Since v8:
> - Added write lock in add_virtual_device
> Since v6:
> - re-work wrt new locking scheme
> - OT: add ASSERT(pcidevs_write_locked()); to add_virtual_device()
> Since v5:
> - s/vpci_add_virtual_device/add_virtual_device and make it static
> - call add_virtual_device from vpci_assign_device and do not use
> REGISTER_VPCI_INIT machinery
> - add pcidevs_locked ASSERT
> - use DECLARE_BITMAP for vpci_dev_assigned_map
> Since v4:
> - moved and re-worked guest sbdf initializers
> - s/set_bit/__set_bit
> - s/clear_bit/__clear_bit
> - minor comment fix s/Virtual/Guest/
> - added VPCI_MAX_VIRT_DEV constant (PCI_SLOT(~0) + 1) which will be used
> later for counting the number of MMIO handlers required for a guest
> (Julien)
> Since v3:
> - make use of VPCI_INIT
> - moved all new code to vpci.c which belongs to it
> - changed open-coded 31 to PCI_SLOT(~0)
> - added comments and code to reject multifunction devices with
> functions other than 0
> - updated comment about vpci_dev_next and made it unsigned int
> - implement roll back in case of error while assigning/deassigning devices
> - s/dom%pd/%pd
> 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/drivers/vpci/vpci.c | 69 +++++++++++++++++++++++++++++++++++++++++
> xen/include/xen/sched.h | 8 +++++
> xen/include/xen/vpci.h | 11 +++++++
> 3 files changed, 88 insertions(+)
>
> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
> index 412685f41d..b284f95e05 100644
> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -36,6 +36,54 @@ extern vpci_register_init_t *const __start_vpci_array[];
> extern vpci_register_init_t *const __end_vpci_array[];
> #define NUM_VPCI_INIT (__end_vpci_array - __start_vpci_array)
>
> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
> +static int add_virtual_device(struct pci_dev *pdev)
> +{
> + struct domain *d = pdev->domain;
> + pci_sbdf_t sbdf = { 0 };
> + unsigned long new_dev_number;
> +
> + if ( is_hardware_domain(d) )
> + return 0;
> +
> + ASSERT(pcidevs_locked() && rw_is_write_locked(&pdev->domain->pci_lock));
Do you need to check for pcidevs here? I would think d->pci_lock
would be enough to protect the virtual allocation device bitmap.
> +
> + /*
> + * Each PCI bus supports 32 devices/slots at max or up to 256 when
> + * there are multi-function ones which are not yet supported.
> + */
> + if ( pdev->info.is_extfn )
I think you are missing a !pdev->info.is_virtfn, as is_extfn &&
is_virtfn mean the PF it's an extended function, but not the VF we are
trying to passthrough.
> + {
> + gdprintk(XENLOG_ERR, "%pp: only function 0 passthrough supported\n",
> + &pdev->sbdf);
> + return -EOPNOTSUPP;
> + }
> + new_dev_number = find_first_zero_bit(d->vpci_dev_assigned_map,
> + VPCI_MAX_VIRT_DEV);
> + if ( new_dev_number >= VPCI_MAX_VIRT_DEV )
The > is not required, as find_first_zero_bit() will return
VPCI_MAX_VIRT_DEV if the bitmap is all set.
> + {
> + write_unlock(&pdev->domain->pci_lock);
> + return -ENOSPC;
> + }
> +
> + __set_bit(new_dev_number, &d->vpci_dev_assigned_map);
> +
> + /*
> + * Both segment and bus number are 0:
> + * - we emulate a single host bridge for the guest, e.g. segment 0
> + * - with bus 0 the virtual devices are seen as embedded
> + * endpoints behind the root complex
> + *
> + * TODO: add support for multi-function devices.
> + */
> + sbdf.devfn = PCI_DEVFN(new_dev_number, 0);
> + pdev->vpci->guest_sbdf = sbdf;
You could avoid the local sbdf variable and just use PCI_SBDF(0, 0,
new_dev_number, 0);
> +
> + return 0;
> +}
> +
> +#endif /* CONFIG_HAS_VPCI_GUEST_SUPPORT */
> +
> void vpci_deassign_device(struct pci_dev *pdev)
> {
> unsigned int i;
> @@ -46,6 +94,16 @@ void vpci_deassign_device(struct pci_dev *pdev)
> return;
>
> spin_lock(&pdev->vpci->lock);
> +
> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
> + if ( pdev->vpci->guest_sbdf.sbdf != ~0 )
> + {
> + __clear_bit(pdev->vpci->guest_sbdf.dev,
> + &pdev->domain->vpci_dev_assigned_map);
> + pdev->vpci->guest_sbdf.sbdf = ~0;
> + }
> +#endif
There's no need to set sbdf = ~0 as vpci is just about to be freed.
> +
> while ( !list_empty(&pdev->vpci->handlers) )
> {
> struct vpci_register *r = list_first_entry(&pdev->vpci->handlers,
> @@ -101,6 +159,13 @@ int vpci_assign_device(struct pci_dev *pdev)
> INIT_LIST_HEAD(&pdev->vpci->handlers);
> spin_lock_init(&pdev->vpci->lock);
>
> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
> + pdev->vpci->guest_sbdf.sbdf = ~0;
> + rc = add_virtual_device(pdev);
> + if ( rc )
> + goto out;
> +#endif
> +
> for ( i = 0; i < NUM_VPCI_INIT; i++ )
> {
> rc = __start_vpci_array[i](pdev);
> @@ -108,11 +173,15 @@ int vpci_assign_device(struct pci_dev *pdev)
> break;
> }
>
> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
> + out:
> +#endif
That's ugly, can you use the __maybe_unused attribute with a label?
> if ( rc )
> vpci_deassign_device(pdev);
>
> return rc;
> }
> +
Spurious change.
Thanks, Roger.
