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
