Re: [PATCH RFC] vPCI: account for hidden devices in modify_bars()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.09.2021 06:50, Oleksandr Andrushchenko wrote:
> On 31.08.21 11:35, Jan Beulich wrote:
>> On 31.08.2021 10:14, Oleksandr Andrushchenko wrote:
>>> On 31.08.21 11:05, Jan Beulich wrote:
>>>> On 31.08.2021 09:56, Oleksandr Andrushchenko wrote:
>>>>> On 31.08.21 10:47, Jan Beulich wrote:
>>>>>> On 31.08.2021 09:06, Oleksandr Andrushchenko wrote:
>>>>>>> On 31.08.21 09:51, Jan Beulich wrote:
>>>>>>>> On 31.08.2021 07:35, Oleksandr Andrushchenko wrote:
>>>>>>>>> On 30.08.21 16:04, Jan Beulich wrote:
>>>>>>>>>> @@ -265,7 +266,8 @@ static int modify_bars(const struct pci_
>>>>>>>>>>            * Check for overlaps with other BARs. Note that only BARs 
>>>>>>>>>> that are
>>>>>>>>>>            * currently mapped (enabled) are checked for overlaps.
>>>>>>>>>>            */
>>>>>>>>>> -    for_each_pdev ( pdev->domain, tmp )
>>>>>>>>>> +for ( d = pdev->domain; ; d = dom_xen ) {//todo
>>>>>>>>> I am not quite sure this will be correct for the cases where 
>>>>>>>>> pdev->domain != dom0,
>>>>>>>>> e.g. in the series for PCI passthrough for Arm this can be any guest. 
>>>>>>>>> For such cases
>>>>>>>>> we'll force running the loop for dom_xen which I am not sure is 
>>>>>>>>> desirable.
>>>>>>>> It is surely not desirable, but it also doesn't happen - see the
>>>>>>>> is_hardware_domain() check further down (keeping context below).
>>>>>>> Right
>>>>>>>>> Another question is why such a hidden device has its pdev->domain not 
>>>>>>>>> set correctly,
>>>>>>>>> so we need to work this around?
>>>>>>>> Please see _setup_hwdom_pci_devices() and commit e46ea4d44dc0
>>>>>>>> ("PCI: don't allow guest assignment of devices used by Xen")
>>>>>>>> introducing that temporary override. To permit limited
>>>>>>>> visibility to Dom0, these devices still need setting up in the
>>>>>>>> IOMMU for Dom0. Consequently BAR overlap detection also needs
>>>>>>>> to take these into account (i.e. the goal here is not just to
>>>>>>>> prevent triggering the ASSERT() in question).
>>>>>>> So, why don't we set pdev->domain = dom_xen for such devices and call
>>>>>>> modify_bars or something from pci_hide_device for instance (I didn't 
>>>>>>> get too
>>>>>>> much into implementation details though)? If pci_hide_device already 
>>>>>>> handles
>>>>>>> such exceptions, so it should also take care of the correct BAR 
>>>>>>> overlaps etc.
>>>>>> How would it? It runs long before Dom0 gets created, let alone when
>>>>>> Dom0 may make adjustments to the BAR arrangement.
>>>>> So, why don't we call "yet another hide function" while creating Dom0 for 
>>>>> that
>>>>> exactly reason, e.g. BAR overlap handling? E.g. make it 2-stage hide for 
>>>>> special
>>>>> devices such as console etc.
>>>> This might be an option, but is imo going to result not only in more
>>>> code churn, but also in redundant code. After all what modify_bars()
>>>> needs is the union of BARs from Dom0's and DomXEN's devices.
>>> To me DomXEN here is yet another workaround as strictly speaking
>>> vpci code didn't need and doesn't(?) need it at the moment. Yes, at least 
>>> on Arm.
>>> So, I do understand why you want it there, but this then does need a very
>>> good description of what is happening and why...
>>>
>>>>>> The temporary overriding of pdev->domain is because other IOMMU code
>>>>>> takes the domain to act upon from that field.
>>>>> So, you mean pdev->domain in that case is pointing to what?
>>>> Did you look at the function I've pointed you at? DomXEN there gets
>>>> temporarily overridden to Dom0.
>>> This looks like yet another workaround to me which is not cute.
>>> So, the overall solution is spread over multiple subsystems, each
>>> introducing something which is hard to follow
>> If you have any better suggestions, I'm all ears. Or feel free to send
>> patches.
> 
> Unfortunately I don't have any. But, could you please at least document a bit
> more what is happening here with DomXEN: either in the commit message or
> in the code itself, so it is easier to understand why vpci has this code at 
> all...

Well, the commit message imo already says what needs saying. I'll extend
the pre-existing code comment, though. But of course the primary purpose
of this RFC is to potentially have a better approach pointed out in the
first place, which I guess will mean to wait with v1 until Roger's return.

Jan