Re: [PATCH v2 11/11] xen/arm: Translate virtual PCI bus topology for guests

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.09.2021 16:04, Oleksandr Andrushchenko wrote:
> 
> On 27.09.21 16:51, Jan Beulich wrote:
>> On 27.09.2021 15:43, Oleksandr Andrushchenko wrote:
>>> On 27.09.21 16:34, Jan Beulich wrote:
>>>> 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.
>>> Ah, with that that respect then of course. But let's be realistic.
>>> How many PCI devices are normally passed through to a guest?
>>> I can assume this is probably less than 10 most of the time.
>>> By assuming that the number of devices is small I see no profit,
>>> but unneeded complexity in accounting virtual devices per segment
>>> and performing the relevant lookup. So, I would go with a single list
>>> and "brute force lookup" unless it is clearly seen that this needs to be
>>> optimized.
>>
>> Just to repeat my initial reply: "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?" If the code uses the simpler form because it's only
>> going to be used for DomU, then that's fine for now. But such latent
>> issues will want recording - e.g. by TODO comments or at the very
>> least suitable pointing out in the description.
> 
> As we do not emulate virtual bus topology for Dom0 then it is
> 
> clearly seen that the code may only have impact on DomUs.
> 
> But anyways, virtual bus topology for DomUs is emulated with
> 
> a single segment 0. We have a single list of virtual SBDFs,
> 
> again, for virtual segment 0, which maps those virtual SBDFs
> 
> to physical SBDFs. So, we go over the list of virtual devices
> 
> assigned to that guest and match the virtual SBDF in question to
> 
> its counterpart in Dom0. I can't see how this can be optimized or
> 
> needs that optimization because of the fact that Dom0 may have
> 
> multiple segments...
> 
> So, how would that comment look like?

Well - if the plan is that this code would never be used for Dom0,
then all is fine as is, I guess. But as you can see the Dom0 plans
on Arm wrt vPCI weren't clear to me at all.

Jan