Re: [PATCH v5 11/14] vpci: add initial support for virtual PCI bus topology

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

Hi, Roger!

On 13.01.22 13:35, Roger Pau Monné wrote:
> On Thu, Nov 25, 2021 at 01:02:48PM +0200, Oleksandr Andrushchenko 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>
>> ---
>> 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 | 51 +++++++++++++++++++++++++++++++++++++++++
>>   xen/include/xen/sched.h |  8 +++++++
>>   xen/include/xen/vpci.h  | 11 +++++++++
>>   3 files changed, 70 insertions(+)
>>
>> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
>> index 98b12a61be6f..c2fb4d4db233 100644
>> --- a/xen/drivers/vpci/vpci.c
>> +++ b/xen/drivers/vpci/vpci.c
>> @@ -114,6 +114,9 @@ int vpci_add_handlers(struct pci_dev *pdev)
>>       spin_lock(&pdev->vpci_lock);
>>       pdev->vpci = vpci;
>>       INIT_LIST_HEAD(&pdev->vpci->handlers);
>> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
>> +    pdev->vpci->guest_sbdf.sbdf = ~0;
>> +#endif
>>   
>>       header = &pdev->vpci->header;
>>       for ( i = 0; i < ARRAY_SIZE(header->bars); i++ )
>> @@ -145,6 +148,53 @@ int vpci_add_handlers(struct pci_dev *pdev)
>>   }
>>   
>>   #ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
>> +int vpci_add_virtual_device(struct pci_dev *pdev)
>> +{
>> +    struct domain *d = pdev->domain;
>> +    pci_sbdf_t sbdf = { 0 };
>> +    unsigned long new_dev_number;
> I think this needs to be limited to non-hardware domains?
>
> Or else you will report failures for the hardware domain even if it's
> not using the virtual topology at all.
Yes, this wants an is_hardware_domain check
>
>> +    /*
>> +     * 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 )
>> +    {
>> +        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 )
>> +        return -ENOSPC;
>> +
>> +    __set_bit(new_dev_number, &d->vpci_dev_assigned_map);
> How is vpci_dev_assigned_map protected from concurrent accesses? Does
> it rely on the pcidevs lock being held while accessing it?
It does rely on pcidevs lock, I'll add an assert here
>
> If so it needs spelling out (and likely an assert added).
>
>> +    /*
>> +     * 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;
>> +
>> +    return 0;
>> +
>> +}
>> +REGISTER_VPCI_INIT(vpci_add_virtual_device, VPCI_PRIORITY_MIDDLE);
> I'm unsure this is the right place to do virtual SBDF assignment, my
> plan was to use REGISTER_VPCI_INIT exclusively with PCI capabilities.
>
> I think it would be better to do the virtual SBDF assignment from
> vpci_assign_device.
Ok, will do
>
>> +
>> +static void vpci_remove_virtual_device(struct domain *d,
>> +                                       const struct pci_dev *pdev)
>> +{
>> +    __clear_bit(pdev->vpci->guest_sbdf.dev, &d->vpci_dev_assigned_map);
>> +    pdev->vpci->guest_sbdf.sbdf = ~0;
>> +}
>> +
>>   /* Notify vPCI that device is assigned to guest. */
>>   int vpci_assign_device(struct domain *d, struct pci_dev *pdev)
>>   {
>> @@ -171,6 +221,7 @@ int vpci_deassign_device(struct domain *d, struct 
>> pci_dev *pdev)
>>           return 0;
>>   
>>       spin_lock(&pdev->vpci_lock);
>> +    vpci_remove_virtual_device(d, pdev);
>>       vpci_remove_device_handlers_locked(pdev);
>>       spin_unlock(&pdev->vpci_lock);
>>   
>> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
>> index 28146ee404e6..10bff103317c 100644
>> --- a/xen/include/xen/sched.h
>> +++ b/xen/include/xen/sched.h
>> @@ -444,6 +444,14 @@ struct domain
>>   
>>   #ifdef CONFIG_HAS_PCI
>>       struct list_head pdev_list;
>> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
>> +    /*
>> +     * The bitmap which shows which device numbers are already used by the
>> +     * virtual PCI bus topology and is used to assign a unique SBDF to the
>> +     * next passed through virtual PCI device.
>> +     */
>> +    unsigned long vpci_dev_assigned_map;
> Please use DECLARE_BITMAP with the maximum number of supported
> devices as parameter.
Will use
>
>> +#endif
>>   #endif
>>   
>>   #ifdef CONFIG_HAS_PASSTHROUGH
>> diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
>> index 18319fc329f9..e5258bd7ce90 100644
>> --- a/xen/include/xen/vpci.h
>> +++ b/xen/include/xen/vpci.h
>> @@ -21,6 +21,13 @@ typedef int vpci_register_init_t(struct pci_dev *dev);
>>   
>>   #define VPCI_ECAM_BDF(addr)     (((addr) & 0x0ffff000) >> 12)
>>   
>> +/*
>> + * Maximum number of devices supported by the virtual bus topology:
>> + * each PCI bus supports 32 devices/slots at max or up to 256 when
>> + * there are multi-function ones which are not yet supported.
>> + */
>> +#define VPCI_MAX_VIRT_DEV       (PCI_SLOT(~0) + 1)
>> +
>>   #define REGISTER_VPCI_INIT(x, p)                \
>>     static vpci_register_init_t *const x##_entry  \
>>                  __used_section(".data.vpci." p) = x
>> @@ -143,6 +150,10 @@ struct vpci {
>>               struct vpci_arch_msix_entry arch;
>>           } entries[];
>>       } *msix;
>> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
>> +    /* Guest SBDF of the device. */
>> +    pci_sbdf_t guest_sbdf;
>> +#endif
>>   #endif
>>   };
>>   
>> -- 
>> 2.25.1
>>
Thank you,
Oleksandr