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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@epam.com>]

Hi, Jan!

On 18.11.21 18:45, Jan Beulich wrote:
> On 05.11.2021 07:56, Oleksandr Andrushchenko wrote:
>> 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)
>>   - revisited locking: add dedicated vdev list's lock
> What is this about? I can't spot any locking in the patch. In particular ...
I will update
>
>> @@ -125,6 +128,54 @@ 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;
>> +    unsigned long new_dev_number;
>> +
>> +    /*
>> +     * 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,
>> +                                         PCI_SLOT(~0) + 1);
>> +    if ( new_dev_number > PCI_SLOT(~0) )
>> +        return -ENOSPC;
>> +
>> +    set_bit(new_dev_number, &d->vpci_dev_assigned_map);
> ... I wonder whether this isn't racy without any locking around it,
Locking is going to be implemented by moving vpci->lock to the
outside, so this code will be protected
> and without looping over test_and_set_bit(). Whereas with locking I
> think you could just use __set_bit().
Although __set_bit == set_bit on Arm I see there is  a difference on x86
I wil use __set_bit
>
>> +    /*
>> +     * 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.sbdf = 0;
> I think this would be better expressed as an initializer,
Ok,
-    pci_sbdf_t sbdf;
+    pci_sbdf_t sbdf = { 0 };

>   making it
> clear to the reader that the whole object gets initialized with out
> them needing to go check the type (and find that .sbdf covers the
> entire object).
>
>> --- a/xen/include/xen/vpci.h
>> +++ b/xen/include/xen/vpci.h
>> @@ -145,6 +145,10 @@ struct vpci {
>>               struct vpci_arch_msix_entry arch;
>>           } entries[];
>>       } *msix;
>> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
>> +    /* Virtual SBDF of the device. */
>> +    pci_sbdf_t guest_sbdf;
> Would vsbdf perhaps be better in line with things like vpci or vcpu
> (as well as with the comment here)?
This is the same as guest_addr...
@Roger what is your preference here?
>
> Jan
>
Thank you,
Oleksandr