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

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 1/25/24 11:00, Jan Beulich wrote:
> On 09.01.2024 22:51, Stewart Hildebrand wrote:
>> --- a/xen/drivers/Kconfig
>> +++ b/xen/drivers/Kconfig
>> @@ -15,4 +15,8 @@ source "drivers/video/Kconfig"
>>  config HAS_VPCI
>>      bool
>>  
>> +config HAS_VPCI_GUEST_SUPPORT
>> +    bool
>> +    depends on HAS_VPCI
> 
> Wouldn't this better be "select", or even just "imply"?

I prefer "select"

> 
>> --- a/xen/drivers/vpci/vpci.c
>> +++ b/xen/drivers/vpci/vpci.c
>> @@ -40,6 +40,49 @@ 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 int 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);
> 
> The message suggests you ought to check pdev->devfn to have the low
> three bits clear. Yet what you check are two booleans.

I'll check pdev->sbdf.fn

> 
> Further doesn't this require the multi-function bit to be emulated
> clear?

I consider this to be future work. The header type register, where the
bit resides, is not yet emulated for domUs. I have a series in the works
for emulating additional registers (including PCI_HEADER_TYPE), but I'm
planning to wait to submit it until after the current series is finished
so as to not delay the current series any further.

> And finally don't you then also need to disallow assignment of
> devices with phantom functions?

No, I don't think so. My understanding is that there is no configuration
space associated with the phantom SBDFs. There's no special handling
required in vPCI per se, because the phantom function RIDs get mapped
in the IOMMU when the device gets assigned. Future work would include
exposing the PCI Express Capability, including device control register
with the phantom function enable bit. I say this having only done
limited testing of phantom functions on ARM, and by faking it using the
pci-phantom= Xen arg because I don't have a real device with phantom
function capability.

> 
>> --- a/xen/include/xen/sched.h
>> +++ b/xen/include/xen/sched.h
>> @@ -484,6 +484,14 @@ struct domain
>>       * 2. pdev->vpci->lock
>>       */
>>      rwlock_t pci_lock;
>> +#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.
>> +     */
>> +    DECLARE_BITMAP(vpci_dev_assigned_map, VPCI_MAX_VIRT_DEV);
>> +#endif
>>  #endif
> 
> With this the 2nd #endif would likely better gain a comment.

I will add it. Actually, I see no harm in adding a comment for both of
these #endifs.

> 
>> --- 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)
> 
> The limit being this means only bus 0 / seg 0 is supported, which I
> think the comment would better also say. (In add_virtual_device(),
> which has a similar comment, there's then at least a 2nd one saying
> so.)

OK, I'll adjust the comment.