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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 27.09.21 17:16, Jan Beulich wrote:
> 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.

It is all new to all of us ;) No problem.


> Jan
>
Thank you,

Oleksandr