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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan