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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 30.09.21 13:23, Jan Beulich wrote:
> 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.
Great, will use this. It is indeed does the job in a clear way.
Thank you!!
>   (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.)
Sounds good
> Jan
>
>
Thank you,
Oleksandr