Re: [PATCH v10 13/17] vpci: add initial support for virtual PCI bus topology

[Julien Grall <julien@xxxxxxx>]

Hi Volodymyr,

This patch was mentioned in another context about allocating the BDF. So 
I thought I would comment here as well.
On 12/10/2023 23:09, Volodymyr Babchuk wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
>
> 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_andrushchenko@xxxxxxxx>
Your signed-off-by should be added even if you are only sending the 
patch on behalf of Oleksandr. This is part of the DCO [1]
> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
> index 5e34d0092a..7c46a2d3f4 100644
> --- a/xen/drivers/vpci/vpci.c
> +++ b/xen/drivers/vpci/vpci.c
> @@ -36,6 +36,52 @@ 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;
> +    unsigned long new_dev_number;
> +
> +    if ( is_hardware_domain(d) )
> +        return 0;
> +
> +    ASSERT(rw_is_write_locked(&pdev->domain->pci_lock));
> +
> +    /*
> +     * 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 && !pdev->info.is_virtfn )
> +    {
> +        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);
IIUC, this means that Xen will allocate the BDF. I think this will 
become a problem quite quickly as some of the PCI may need to be 
assigned at a specific vBDF (I have the intel graphic card in mind).
Also, xl allows you to specificy the slot (e.g. <bdf>@<vslot>) which 
would not work with this approach.
For dom0less passthrough, I feel the virtual BDF should always be 
specified in device-tree. When a domain is created after boot, then I 
think you want to support <bdf>@<vslot> where <vslot> is optional.
[1] https://cert-manager.io/docs/contributing/sign-off/

--
Julien Grall