On 27.09.2021 14:08, Oleksandr Andrushchenko wrote:
> On 27.09.21 14:31, Jan Beulich wrote:
>> On 23.09.2021 14:55, Oleksandr Andrushchenko wrote:
>>> --- a/xen/drivers/passthrough/pci.c
>>> +++ b/xen/drivers/passthrough/pci.c
>>> @@ -890,6 +890,31 @@ int pci_remove_virtual_device(struct domain *d, const
>>> struct pci_dev *pdev)
>>> return 0;
>>> }
>>>
>>> +/*
>>> + * Find the physical device which is mapped to the virtual device
>>> + * and translate virtual SBDF to the physical one.
>>> + */
>>> +bool pci_translate_virtual_device(struct vcpu *v, pci_sbdf_t *sbdf)
>> Why struct vcpu, when you only need ...
>>
>>> +{
>>> + struct domain *d = v->domain;
>> ... this? It's also not really logical for this function to take a
>> struct vcpu, as the translation should be uniform within a domain.
> Agree, struct domain is just enough
>>
>> Also - const please (as said elsewhere before, ideally wherever possible
>> and sensible).
> Ok
>>
>>> + struct vpci_dev *vdev;
>>> + bool found = false;
>>> +
>>> + pcidevs_lock();
>>> + list_for_each_entry ( vdev, &d->vdev_list, list )
>>> + {
>>> + if ( vdev->sbdf.sbdf == sbdf->sbdf )
>>> + {
>>> + /* Replace virtual SBDF with the physical one. */
>>> + *sbdf = vdev->pdev->sbdf;
>>> + found = true;
>>> + break;
>>> + }
>>> + }
>> For a DomU with just one or at most a couple of devices, such a brute
>> force lookup may be fine. What about Dom0 though? The physical topology
>> gets split at the segment level, so maybe this would by a reasonable
>> granularity here as well?
>
> Not sure I am following why topology matters here. We are just trying to
> match one SBDF (as seen by the guest) to other SBDF (physical,
> as seen by Dom0), so we can proxy DomU's configuration space access
> to the proper device in Dom0.
Topology here matters only in so far as I've suggested to have separate
lists per segment, to reduce look times. Other methods of avoiding a
fully linear search are of course possible as well.
>>> + pcidevs_unlock();
>>> + return found;
>> Nit: Blank line please ahead of the main "return" of a function.
> Sure
>>
>>> +}
>>> +
>>> /* Caller should hold the pcidevs_lock */
>>> static int deassign_device(struct domain *d, uint16_t seg, uint8_t bus,
>>> uint8_t devfn)
>> Seeing this function in context (which patch 2 adds without any #ifdef
>> around it afaics),
>
> I believe you are talking about vpci_deassign_device here
> vpci_{assign|deassign}_device seem to be not called on x86 PVH as of now,
> this is true.
>
>> will this new function needlessly be built on x86 as
>> well?
>
> It will at the moment. But in order to avoid ifdefery I would like
> to still implement it as an empty function for x86.
>
>> (I didn't look at other intermediate patches yet, so please
>> forgive if I've missed the addition of an #ifdef.)
>
> So, I can gate this with HAS_VPCI_GUEST_SUPPORT in patch 2
> (HAS_VPCI_GUEST_SUPPORT is introduced in patch 4, so I'll move it to 2)
> Does this sound good?
Yes.
Jan
