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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.09.2021 11:34, Oleksandr Andrushchenko wrote:
> On 30.09.21 11:51, Jan Beulich wrote:
>> On 30.09.2021 09:52, Oleksandr Andrushchenko wrote:
>>> --- a/xen/drivers/passthrough/pci.c
>>> +++ b/xen/drivers/passthrough/pci.c
>>> @@ -831,6 +831,66 @@ int pci_remove_device(u16 seg, u8 bus, u8 devfn)
>>>       return ret;
>>>   }
>>>   
>>> +#ifdef CONFIG_HAS_VPCI_GUEST_SUPPORT
>> May I ask why the code enclosed by this conditional has been put here
>> rather than under drivers/vpci/?
> Indeed this can be moved to xen/drivers/vpci/vpci.c.
> I'll move and update function names accordingly.
>>
>>> +static struct vpci_dev *pci_find_virtual_device(const struct domain *d,
>>> +                                                const struct pci_dev *pdev)
>>> +{
>>> +    struct vpci_dev *vdev;
>>> +
>>> +    list_for_each_entry ( vdev, &d->vdev_list, list )
>>> +        if ( vdev->pdev == pdev )
>>> +            return vdev;
>>> +    return NULL;
>>> +}
>> No locking here or ...
>>
>>> +int pci_add_virtual_device(struct domain *d, const struct pci_dev *pdev)
>>> +{
>>> +    struct vpci_dev *vdev;
>>> +
>>> +    ASSERT(!pci_find_virtual_device(d, pdev));
>> ... in this first caller that I've managed to spot? See also below as
>> to up the call tree the pcidevs-lock being held (which at the very
>> least you would then want to ASSERT() for here).
> I will move the code to vpci and make sure proper locking there
>>
>>> +    /* Each PCI bus supports 32 devices/slots at max. */
>>> +    if ( d->vpci_dev_next > 31 )
>>> +        return -ENOSPC;
>> Please avoid open-coding literals when they can be suitably expressed.
> I failed to find a suitable constant for that. Could you please point
> me to the one I can use here?

I wasn't hinting at a constant, but at an expression. If you grep, you
will find e.g. at least one instance of PCI_FUNC(~0); I'd suggest to
use PCI_SLOT(~0) here. (My rule of thumb is: Before I write a literal
number anywhere outside of a #define, and not 0 or 1 or some such
starting a loop, I try to think hard how that number can instead be
expressed. Such expressions then often also serve as documentation for
what the number actually means, helping future readers.)

Jan