Hidden devices (e.g. an add-in PCI serial card used for Xen's serial
console) are associated with DomXEN, not Dom0. This means that while
looking for overlapping BARs such devices cannot be found on Dom0's
list of devices; DomXEN's list also needs to be scanned.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
RFC: Patch intentionally mis-formatted, as the necessary re-indentation
     would make the diff difficult to read. At this point I'd merely
     like to gather input towards possible better approaches to solve
     the issue (not the least because quite possibly there are further
     places needing changing).

--- a/xen/drivers/vpci/header.c
+++ b/xen/drivers/vpci/header.c
@@ -206,6 +206,7 @@ static int modify_bars(const struct pci_
     struct vpci_header *header = &pdev->vpci->header;
     struct rangeset *mem = rangeset_new(NULL, NULL, 0);
     struct pci_dev *tmp, *dev = NULL;
+    const struct domain *d;
     const struct vpci_msix *msix = pdev->vpci->msix;
     unsigned int i;
     int rc;
@@ -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
+    for_each_pdev ( d, tmp )
     {
         if ( tmp == pdev )
         {
@@ -282,6 +284,7 @@ static int modify_bars(const struct pci_
                  */
                 continue;
         }
+if ( !tmp->vpci ) { ASSERT(d == dom_xen && system_state < SYS_STATE_active); continue; }//todo
 
         for ( i = 0; i < ARRAY_SIZE(tmp->vpci->header.bars); i++ )
         {
@@ -308,6 +311,7 @@ static int modify_bars(const struct pci_
             }
         }
     }
+if ( !is_hardware_domain(d) ) break; }//todo
 
     ASSERT(dev);
 

Hello, Jan!

On 30.08.21 16:04, Jan Beulich wrote:
> Hidden devices (e.g. an add-in PCI serial card used for Xen's serial
> console) are associated with DomXEN, not Dom0. This means that while
> looking for overlapping BARs such devices cannot be found on Dom0's
> list of devices; DomXEN's list also needs to be scanned.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> RFC: Patch intentionally mis-formatted, as the necessary re-indentation
>       would make the diff difficult to read. At this point I'd merely
>       like to gather input towards possible better approaches to solve
>       the issue (not the least because quite possibly there are further
>       places needing changing).
>
> --- a/xen/drivers/vpci/header.c
> +++ b/xen/drivers/vpci/header.c
> @@ -206,6 +206,7 @@ static int modify_bars(const struct pci_
>       struct vpci_header *header = &pdev->vpci->header;
>       struct rangeset *mem = rangeset_new(NULL, NULL, 0);
>       struct pci_dev *tmp, *dev = NULL;
> +    const struct domain *d;
>       const struct vpci_msix *msix = pdev->vpci->msix;
>       unsigned int i;
>       int rc;
> @@ -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.

Another question is why such a hidden device has its pdev->domain not set correctly,

so we need to work this around?

Thank you,

Oleksandr

> +    for_each_pdev ( d, tmp )
>       {
>           if ( tmp == pdev )
>           {
> @@ -282,6 +284,7 @@ static int modify_bars(const struct pci_
>                    */
>                   continue;
>           }
> +if ( !tmp->vpci ) { ASSERT(d == dom_xen && system_state < SYS_STATE_active); continue; }//todo
>   
>           for ( i = 0; i < ARRAY_SIZE(tmp->vpci->header.bars); i++ )
>           {
> @@ -308,6 +311,7 @@ static int modify_bars(const struct pci_
>               }
>           }
>       }
> +if ( !is_hardware_domain(d) ) break; }//todo
>   
>       ASSERT(dev);
>   
>
>

On 31.08.2021 07:35, Oleksandr Andrushchenko wrote:
> Hello, Jan!
> 
> On 30.08.21 16:04, Jan Beulich wrote:
>> Hidden devices (e.g. an add-in PCI serial card used for Xen's serial
>> console) are associated with DomXEN, not Dom0. This means that while
>> looking for overlapping BARs such devices cannot be found on Dom0's
>> list of devices; DomXEN's list also needs to be scanned.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> RFC: Patch intentionally mis-formatted, as the necessary re-indentation
>>       would make the diff difficult to read. At this point I'd merely
>>       like to gather input towards possible better approaches to solve
>>       the issue (not the least because quite possibly there are further
>>       places needing changing).
>>
>> --- a/xen/drivers/vpci/header.c
>> +++ b/xen/drivers/vpci/header.c
>> @@ -206,6 +206,7 @@ static int modify_bars(const struct pci_
>>       struct vpci_header *header = &pdev->vpci->header;
>>       struct rangeset *mem = rangeset_new(NULL, NULL, 0);
>>       struct pci_dev *tmp, *dev = NULL;
>> +    const struct domain *d;
>>       const struct vpci_msix *msix = pdev->vpci->msix;
>>       unsigned int i;
>>       int rc;
>> @@ -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).

> 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).

Jan

>> @@ -308,6 +311,7 @@ static int modify_bars(const struct pci_
>>               }
>>           }
>>       }
>> +if ( !is_hardware_domain(d) ) break; }//todo
>>   
>>       ASSERT(dev);
>>   
>>
>>
> 

On 31.08.21 09:51, Jan Beulich wrote:
> On 31.08.2021 07:35, Oleksandr Andrushchenko wrote:
>> Hello, Jan!
>>
>> On 30.08.21 16:04, Jan Beulich wrote:
>>> Hidden devices (e.g. an add-in PCI serial card used for Xen's serial
>>> console) are associated with DomXEN, not Dom0. This means that while
>>> looking for overlapping BARs such devices cannot be found on Dom0's
>>> list of devices; DomXEN's list also needs to be scanned.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> ---
>>> RFC: Patch intentionally mis-formatted, as the necessary re-indentation
>>>        would make the diff difficult to read. At this point I'd merely
>>>        like to gather input towards possible better approaches to solve
>>>        the issue (not the least because quite possibly there are further
>>>        places needing changing).
>>>
>>> --- a/xen/drivers/vpci/header.c
>>> +++ b/xen/drivers/vpci/header.c
>>> @@ -206,6 +206,7 @@ static int modify_bars(const struct pci_
>>>        struct vpci_header *header = &pdev->vpci->header;
>>>        struct rangeset *mem = rangeset_new(NULL, NULL, 0);
>>>        struct pci_dev *tmp, *dev = NULL;
>>> +    const struct domain *d;
>>>        const struct vpci_msix *msix = pdev->vpci->msix;
>>>        unsigned int i;
>>>        int rc;
>>> @@ -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.

Otherwise it looks like we put some unrelated logic into vpci which is for hiding

the devices (on x86).

Thank you,

Oleksandr

>
> Jan
>
>>> @@ -308,6 +311,7 @@ static int modify_bars(const struct pci_
>>>                }
>>>            }
>>>        }
>>> +if ( !is_hardware_domain(d) ) break; }//todo
>>>    
>>>        ASSERT(dev);
>>>    
>>>
>>>

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:
>>> Hello, Jan!
>>>
>>> On 30.08.21 16:04, Jan Beulich wrote:
>>>> Hidden devices (e.g. an add-in PCI serial card used for Xen's serial
>>>> console) are associated with DomXEN, not Dom0. This means that while
>>>> looking for overlapping BARs such devices cannot be found on Dom0's
>>>> list of devices; DomXEN's list also needs to be scanned.
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>> ---
>>>> RFC: Patch intentionally mis-formatted, as the necessary re-indentation
>>>>        would make the diff difficult to read. At this point I'd merely
>>>>        like to gather input towards possible better approaches to solve
>>>>        the issue (not the least because quite possibly there are further
>>>>        places needing changing).
>>>>
>>>> --- a/xen/drivers/vpci/header.c
>>>> +++ b/xen/drivers/vpci/header.c
>>>> @@ -206,6 +206,7 @@ static int modify_bars(const struct pci_
>>>>        struct vpci_header *header = &pdev->vpci->header;
>>>>        struct rangeset *mem = rangeset_new(NULL, NULL, 0);
>>>>        struct pci_dev *tmp, *dev = NULL;
>>>> +    const struct domain *d;
>>>>        const struct vpci_msix *msix = pdev->vpci->msix;
>>>>        unsigned int i;
>>>>        int rc;
>>>> @@ -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.

The temporary overriding of pdev->domain is because other IOMMU code
takes the domain to act upon from that field. This could have been
solved without override, but then much heavier code churn would have
resulted.

> Otherwise it looks like we put some unrelated logic into vpci which is for hiding
> the devices (on x86).

Hiding devices is in no way x86-specific.

Jan

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:
>>>> Hello, Jan!
>>>>
>>>> On 30.08.21 16:04, Jan Beulich wrote:
>>>>> Hidden devices (e.g. an add-in PCI serial card used for Xen's serial
>>>>> console) are associated with DomXEN, not Dom0. This means that while
>>>>> looking for overlapping BARs such devices cannot be found on Dom0's
>>>>> list of devices; DomXEN's list also needs to be scanned.
>>>>>
>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>> ---
>>>>> RFC: Patch intentionally mis-formatted, as the necessary re-indentation
>>>>>         would make the diff difficult to read. At this point I'd merely
>>>>>         like to gather input towards possible better approaches to solve
>>>>>         the issue (not the least because quite possibly there are further
>>>>>         places needing changing).
>>>>>
>>>>> --- a/xen/drivers/vpci/header.c
>>>>> +++ b/xen/drivers/vpci/header.c
>>>>> @@ -206,6 +206,7 @@ static int modify_bars(const struct pci_
>>>>>         struct vpci_header *header = &pdev->vpci->header;
>>>>>         struct rangeset *mem = rangeset_new(NULL, NULL, 0);
>>>>>         struct pci_dev *tmp, *dev = NULL;
>>>>> +    const struct domain *d;
>>>>>         const struct vpci_msix *msix = pdev->vpci->msix;
>>>>>         unsigned int i;
>>>>>         int rc;
>>>>> @@ -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.

>
> 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?


>   This could have been
> solved without override, but then much heavier code churn would have
> resulted.
>
>> Otherwise it looks like we put some unrelated logic into vpci which is for hiding
>> the devices (on x86).
> Hiding devices is in no way x86-specific.

I mean that the use-case you have, e.g. a *PCI* console you want to hide,

is definitely not something used on Arm at least.

>
> Jan
>

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.

>> 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 could have been
>> solved without override, but then much heavier code churn would have
>> resulted.
>>
>>> Otherwise it looks like we put some unrelated logic into vpci which is for hiding
>>> the devices (on x86).
>> Hiding devices is in no way x86-specific.
> 
> I mean that the use-case you have, e.g. a *PCI* console you want to hide,
> is definitely not something used on Arm at least.

Not yet, that is? Why would - in the long run - somebody not want to
put in a PCI serial card in a system that supports PCI and has no
(available) other serial port? And if you have looked at the commit
I did point you at, you will also have found that it's more than just
the serial device that we hide.

Jan

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

>
>>>    This could have been
>>> solved without override, but then much heavier code churn would have
>>> resulted.
>>>
>>>> Otherwise it looks like we put some unrelated logic into vpci which is for hiding
>>>> the devices (on x86).
>>> Hiding devices is in no way x86-specific.
>> I mean that the use-case you have, e.g. a *PCI* console you want to hide,
>> is definitely not something used on Arm at least.
> Not yet, that is? Why would - in the long run - somebody not want to
> put in a PCI serial card in a system that supports PCI and has no
> (available) other serial port? And if you have looked at the commit
> I did point you at
I did
> , you will also have found that it's more than just
> the serial device that we hide.
Serial was just an example from the list
>
> Jan
>

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

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...

Thank you,

Oleksandr

>
> Jan
>

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

On Mon, Aug 30, 2021 at 03:04:55PM +0200, Jan Beulich wrote:
> Hidden devices (e.g. an add-in PCI serial card used for Xen's serial
> console) are associated with DomXEN, not Dom0. This means that while
> looking for overlapping BARs such devices cannot be found on Dom0's
> list of devices; DomXEN's list also needs to be scanned.

Thanks for looking into this, I certainly didn't take hidden devices
into account for vPCI dom0.

> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> RFC: Patch intentionally mis-formatted, as the necessary re-indentation
>      would make the diff difficult to read. At this point I'd merely
>      like to gather input towards possible better approaches to solve
>      the issue (not the least because quite possibly there are further
>      places needing changing).

I have a couple of questions, AFAICT we currently hide the serial
console and/or the VGA adapter if it's in use by Xen.

I wonder whether we need to add vPCI handlers for those:
setup_one_hwdom_device will attempt to add vPCI handlers to the hidden
device because of the temporary override of pdev->domain done in
_setup_hwdom_pci_devices, but I think that for hidden devices we
shouldn't add vPCI handlers. We should instead block PCI config space
access from dom0 to the device so that it doesn't mess with Xen usage.

It's also not clear why does Xen want to have those hidden devices
partly controlled by dom0, as it would seem to me that dom0 interfering
with hidden devices in use by Xen can only lead to trouble.

Thanks, Roger.

On 14.09.2021 12:00, Roger Pau Monné wrote:
> On Mon, Aug 30, 2021 at 03:04:55PM +0200, Jan Beulich wrote:
>> Hidden devices (e.g. an add-in PCI serial card used for Xen's serial
>> console) are associated with DomXEN, not Dom0. This means that while
>> looking for overlapping BARs such devices cannot be found on Dom0's
>> list of devices; DomXEN's list also needs to be scanned.
> 
> Thanks for looking into this, I certainly didn't take hidden devices
> into account for vPCI dom0.
> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> RFC: Patch intentionally mis-formatted, as the necessary re-indentation
>>      would make the diff difficult to read. At this point I'd merely
>>      like to gather input towards possible better approaches to solve
>>      the issue (not the least because quite possibly there are further
>>      places needing changing).
> 
> I have a couple of questions, AFAICT we currently hide the serial
> console and/or the VGA adapter if it's in use by Xen.
> 
> I wonder whether we need to add vPCI handlers for those:
> setup_one_hwdom_device will attempt to add vPCI handlers to the hidden
> device because of the temporary override of pdev->domain done in
> _setup_hwdom_pci_devices, but I think that for hidden devices we
> shouldn't add vPCI handlers. We should instead block PCI config space
> access from dom0 to the device so that it doesn't mess with Xen usage.

The answer to this follows (I think) from the one below.

> It's also not clear why does Xen want to have those hidden devices
> partly controlled by dom0, as it would seem to me that dom0 interfering
> with hidden devices in use by Xen can only lead to trouble.

Dom0 can't interfere as long as it can only read from the device.
Restricting accesses to reads is one of the purposes of "hiding"
(the other is to make it impossible to assign these to a DomU). Not
allowing Dom0 to read from such devices would lead to wrong PCI
device discovery - some devices would be missing (which may or may
not be merely a cosmetic issue). If the missing device is a multi-
function one at function 0, other devices in the same slot may also
not be found by Dom0 (depending on whether it looks at non-zero
function numbers in this case).

Jan

On Tue, Sep 14, 2021 at 12:12:12PM +0200, Jan Beulich wrote:
> On 14.09.2021 12:00, Roger Pau Monné wrote:
> > On Mon, Aug 30, 2021 at 03:04:55PM +0200, Jan Beulich wrote:
> >> Hidden devices (e.g. an add-in PCI serial card used for Xen's serial
> >> console) are associated with DomXEN, not Dom0. This means that while
> >> looking for overlapping BARs such devices cannot be found on Dom0's
> >> list of devices; DomXEN's list also needs to be scanned.
> > 
> > Thanks for looking into this, I certainly didn't take hidden devices
> > into account for vPCI dom0.
> > 
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >> ---
> >> RFC: Patch intentionally mis-formatted, as the necessary re-indentation
> >>      would make the diff difficult to read. At this point I'd merely
> >>      like to gather input towards possible better approaches to solve
> >>      the issue (not the least because quite possibly there are further
> >>      places needing changing).
> > 
> > I have a couple of questions, AFAICT we currently hide the serial
> > console and/or the VGA adapter if it's in use by Xen.
> > 
> > I wonder whether we need to add vPCI handlers for those:
> > setup_one_hwdom_device will attempt to add vPCI handlers to the hidden
> > device because of the temporary override of pdev->domain done in
> > _setup_hwdom_pci_devices, but I think that for hidden devices we
> > shouldn't add vPCI handlers. We should instead block PCI config space
> > access from dom0 to the device so that it doesn't mess with Xen usage.
> 
> The answer to this follows (I think) from the one below.
> 
> > It's also not clear why does Xen want to have those hidden devices
> > partly controlled by dom0, as it would seem to me that dom0 interfering
> > with hidden devices in use by Xen can only lead to trouble.
> 
> Dom0 can't interfere as long as it can only read from the device.
> Restricting accesses to reads is one of the purposes of "hiding"
> (the other is to make it impossible to assign these to a DomU). Not
> allowing Dom0 to read from such devices would lead to wrong PCI
> device discovery - some devices would be missing (which may or may
> not be merely a cosmetic issue). If the missing device is a multi-
> function one at function 0, other devices in the same slot may also
> not be found by Dom0 (depending on whether it looks at non-zero
> function numbers in this case).

Hm, indeed seems possible that missing function 0 the whole device is
skipped.

Maybe we need a special vPCI handling for those devices that just
allows reads but not writes, and that doesn't maps the BARs into dom0
p2m? Or could the BARs potentially be shared between multiple
functions on the same device?

This also makes me wonder why they have to be added to the IOMMU, is
that related to device groups and the fact that Xen is not clever
enough to figure whether devices belong to the same IOMMU group and
thus must be assigned together?

Thanks Roger.

On 14.09.2021 13:21, Roger Pau Monné wrote:
> On Tue, Sep 14, 2021 at 12:12:12PM +0200, Jan Beulich wrote:
>> On 14.09.2021 12:00, Roger Pau Monné wrote:
>>> On Mon, Aug 30, 2021 at 03:04:55PM +0200, Jan Beulich wrote:
>>>> Hidden devices (e.g. an add-in PCI serial card used for Xen's serial
>>>> console) are associated with DomXEN, not Dom0. This means that while
>>>> looking for overlapping BARs such devices cannot be found on Dom0's
>>>> list of devices; DomXEN's list also needs to be scanned.
>>>
>>> Thanks for looking into this, I certainly didn't take hidden devices
>>> into account for vPCI dom0.
>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>> ---
>>>> RFC: Patch intentionally mis-formatted, as the necessary re-indentation
>>>>      would make the diff difficult to read. At this point I'd merely
>>>>      like to gather input towards possible better approaches to solve
>>>>      the issue (not the least because quite possibly there are further
>>>>      places needing changing).
>>>
>>> I have a couple of questions, AFAICT we currently hide the serial
>>> console and/or the VGA adapter if it's in use by Xen.
>>>
>>> I wonder whether we need to add vPCI handlers for those:
>>> setup_one_hwdom_device will attempt to add vPCI handlers to the hidden
>>> device because of the temporary override of pdev->domain done in
>>> _setup_hwdom_pci_devices, but I think that for hidden devices we
>>> shouldn't add vPCI handlers. We should instead block PCI config space
>>> access from dom0 to the device so that it doesn't mess with Xen usage.
>>
>> The answer to this follows (I think) from the one below.
>>
>>> It's also not clear why does Xen want to have those hidden devices
>>> partly controlled by dom0, as it would seem to me that dom0 interfering
>>> with hidden devices in use by Xen can only lead to trouble.
>>
>> Dom0 can't interfere as long as it can only read from the device.
>> Restricting accesses to reads is one of the purposes of "hiding"
>> (the other is to make it impossible to assign these to a DomU). Not
>> allowing Dom0 to read from such devices would lead to wrong PCI
>> device discovery - some devices would be missing (which may or may
>> not be merely a cosmetic issue). If the missing device is a multi-
>> function one at function 0, other devices in the same slot may also
>> not be found by Dom0 (depending on whether it looks at non-zero
>> function numbers in this case).
> 
> Hm, indeed seems possible that missing function 0 the whole device is
> skipped.
> 
> Maybe we need a special vPCI handling for those devices that just
> allows reads but not writes, and that doesn't maps the BARs into dom0
> p2m?

Not sure about mapping. They could be mapped r/o. And they may
actually need mapping for multi-function devices, but I guess for
such devices to actually function properly then there would be
more work required.

> Or could the BARs potentially be shared between multiple
> functions on the same device?

I don't think that's allowed (or would even work) - if anything
(like in general) small BARs may share a page, but without overlaps.

> This also makes me wonder why they have to be added to the IOMMU, is
> that related to device groups and the fact that Xen is not clever
> enough to figure whether devices belong to the same IOMMU group and
> thus must be assigned together?

I'm actually not sure whether they need to be added to the IOMMU.
With multi-function devices in mind, back at the time I though it
would make more sense that way. But I may have been wrong, and
hence we may want to revisit this. Problem with such changes is
that they may cause rare issues on very specific systems, which we
may have no way noticing even during a full release cycle.

Jan

On Tue, Sep 14, 2021 at 02:05:01PM +0200, Jan Beulich wrote:
> On 14.09.2021 13:21, Roger Pau Monné wrote:
> > On Tue, Sep 14, 2021 at 12:12:12PM +0200, Jan Beulich wrote:
> >> On 14.09.2021 12:00, Roger Pau Monné wrote:
> >>> On Mon, Aug 30, 2021 at 03:04:55PM +0200, Jan Beulich wrote:
> >>>> Hidden devices (e.g. an add-in PCI serial card used for Xen's serial
> >>>> console) are associated with DomXEN, not Dom0. This means that while
> >>>> looking for overlapping BARs such devices cannot be found on Dom0's
> >>>> list of devices; DomXEN's list also needs to be scanned.
> >>>
> >>> Thanks for looking into this, I certainly didn't take hidden devices
> >>> into account for vPCI dom0.
> >>>
> >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >>>> ---
> >>>> RFC: Patch intentionally mis-formatted, as the necessary re-indentation
> >>>>      would make the diff difficult to read. At this point I'd merely
> >>>>      like to gather input towards possible better approaches to solve
> >>>>      the issue (not the least because quite possibly there are further
> >>>>      places needing changing).
> >>>
> >>> I have a couple of questions, AFAICT we currently hide the serial
> >>> console and/or the VGA adapter if it's in use by Xen.
> >>>
> >>> I wonder whether we need to add vPCI handlers for those:
> >>> setup_one_hwdom_device will attempt to add vPCI handlers to the hidden
> >>> device because of the temporary override of pdev->domain done in
> >>> _setup_hwdom_pci_devices, but I think that for hidden devices we
> >>> shouldn't add vPCI handlers. We should instead block PCI config space
> >>> access from dom0 to the device so that it doesn't mess with Xen usage.
> >>
> >> The answer to this follows (I think) from the one below.
> >>
> >>> It's also not clear why does Xen want to have those hidden devices
> >>> partly controlled by dom0, as it would seem to me that dom0 interfering
> >>> with hidden devices in use by Xen can only lead to trouble.
> >>
> >> Dom0 can't interfere as long as it can only read from the device.
> >> Restricting accesses to reads is one of the purposes of "hiding"
> >> (the other is to make it impossible to assign these to a DomU). Not
> >> allowing Dom0 to read from such devices would lead to wrong PCI
> >> device discovery - some devices would be missing (which may or may
> >> not be merely a cosmetic issue). If the missing device is a multi-
> >> function one at function 0, other devices in the same slot may also
> >> not be found by Dom0 (depending on whether it looks at non-zero
> >> function numbers in this case).
> > 
> > Hm, indeed seems possible that missing function 0 the whole device is
> > skipped.
> > 
> > Maybe we need a special vPCI handling for those devices that just
> > allows reads but not writes, and that doesn't maps the BARs into dom0
> > p2m?
> 
> Not sure about mapping. They could be mapped r/o. And they may
> actually need mapping for multi-function devices, but I guess for
> such devices to actually function properly then there would be
> more work required.

I'm also slightly puzzled as to why ehci-dbgp uses pci_hide_device
while ns16550 uses pci_ro_device instead.

Is this because the PCI device used by ehci-dbgp must be shared with
dom0 for other USB ports to be usable, and hence dom0 will need read
and write access to the device PCI config space and BARs?

Note that pci_hide_device is less restrictive than pci_ro_device, as
it doesn't mark the device as RO.

That would seem quite risky, as it's likely that dom0 will perform
some kind of reset of the USB device and thus the console will be
lost.

> > Or could the BARs potentially be shared between multiple
> > functions on the same device?
> 
> I don't think that's allowed (or would even work) - if anything
> (like in general) small BARs may share a page, but without overlaps.

BARs sharing a page should already be handled correctly by vPCI.

> > This also makes me wonder why they have to be added to the IOMMU, is
> > that related to device groups and the fact that Xen is not clever
> > enough to figure whether devices belong to the same IOMMU group and
> > thus must be assigned together?
> 
> I'm actually not sure whether they need to be added to the IOMMU.
> With multi-function devices in mind, back at the time I though it
> would make more sense that way. But I may have been wrong, and
> hence we may want to revisit this. Problem with such changes is
> that they may cause rare issues on very specific systems, which we
> may have no way noticing even during a full release cycle.

I guess it depends on whether the device is to be used by dom0, see
my question above about ehci-dbgp for example.

Thanks, Roger.

On 14.09.2021 16:22, Roger Pau Monné wrote:
> On Tue, Sep 14, 2021 at 02:05:01PM +0200, Jan Beulich wrote:
>> On 14.09.2021 13:21, Roger Pau Monné wrote:
>>> On Tue, Sep 14, 2021 at 12:12:12PM +0200, Jan Beulich wrote:
>>>> On 14.09.2021 12:00, Roger Pau Monné wrote:
>>>>> On Mon, Aug 30, 2021 at 03:04:55PM +0200, Jan Beulich wrote:
>>>>>> Hidden devices (e.g. an add-in PCI serial card used for Xen's serial
>>>>>> console) are associated with DomXEN, not Dom0. This means that while
>>>>>> looking for overlapping BARs such devices cannot be found on Dom0's
>>>>>> list of devices; DomXEN's list also needs to be scanned.
>>>>>
>>>>> Thanks for looking into this, I certainly didn't take hidden devices
>>>>> into account for vPCI dom0.
>>>>>
>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>>> ---
>>>>>> RFC: Patch intentionally mis-formatted, as the necessary re-indentation
>>>>>>      would make the diff difficult to read. At this point I'd merely
>>>>>>      like to gather input towards possible better approaches to solve
>>>>>>      the issue (not the least because quite possibly there are further
>>>>>>      places needing changing).
>>>>>
>>>>> I have a couple of questions, AFAICT we currently hide the serial
>>>>> console and/or the VGA adapter if it's in use by Xen.
>>>>>
>>>>> I wonder whether we need to add vPCI handlers for those:
>>>>> setup_one_hwdom_device will attempt to add vPCI handlers to the hidden
>>>>> device because of the temporary override of pdev->domain done in
>>>>> _setup_hwdom_pci_devices, but I think that for hidden devices we
>>>>> shouldn't add vPCI handlers. We should instead block PCI config space
>>>>> access from dom0 to the device so that it doesn't mess with Xen usage.
>>>>
>>>> The answer to this follows (I think) from the one below.
>>>>
>>>>> It's also not clear why does Xen want to have those hidden devices
>>>>> partly controlled by dom0, as it would seem to me that dom0 interfering
>>>>> with hidden devices in use by Xen can only lead to trouble.
>>>>
>>>> Dom0 can't interfere as long as it can only read from the device.
>>>> Restricting accesses to reads is one of the purposes of "hiding"
>>>> (the other is to make it impossible to assign these to a DomU). Not
>>>> allowing Dom0 to read from such devices would lead to wrong PCI
>>>> device discovery - some devices would be missing (which may or may
>>>> not be merely a cosmetic issue). If the missing device is a multi-
>>>> function one at function 0, other devices in the same slot may also
>>>> not be found by Dom0 (depending on whether it looks at non-zero
>>>> function numbers in this case).
>>>
>>> Hm, indeed seems possible that missing function 0 the whole device is
>>> skipped.
>>>
>>> Maybe we need a special vPCI handling for those devices that just
>>> allows reads but not writes, and that doesn't maps the BARs into dom0
>>> p2m?
>>
>> Not sure about mapping. They could be mapped r/o. And they may
>> actually need mapping for multi-function devices, but I guess for
>> such devices to actually function properly then there would be
>> more work required.
> 
> I'm also slightly puzzled as to why ehci-dbgp uses pci_hide_device
> while ns16550 uses pci_ro_device instead.
> 
> Is this because the PCI device used by ehci-dbgp must be shared with
> dom0 for other USB ports to be usable, and hence dom0 will need read
> and write access to the device PCI config space and BARs?
> 
> Note that pci_hide_device is less restrictive than pci_ro_device, as
> it doesn't mark the device as RO.
> 
> That would seem quite risky, as it's likely that dom0 will perform
> some kind of reset of the USB device and thus the console will be
> lost.

There's actually a protocol to prevent exactly that: See PHYSDEVOP_dbgp_op.

Jan

On Tue, Sep 14, 2021 at 05:16:17PM +0200, Jan Beulich wrote:
> On 14.09.2021 16:22, Roger Pau Monné wrote:
> > On Tue, Sep 14, 2021 at 02:05:01PM +0200, Jan Beulich wrote:
> >> On 14.09.2021 13:21, Roger Pau Monné wrote:
> >>> On Tue, Sep 14, 2021 at 12:12:12PM +0200, Jan Beulich wrote:
> >>>> On 14.09.2021 12:00, Roger Pau Monné wrote:
> >>>>> On Mon, Aug 30, 2021 at 03:04:55PM +0200, Jan Beulich wrote:
> >>>>>> Hidden devices (e.g. an add-in PCI serial card used for Xen's serial
> >>>>>> console) are associated with DomXEN, not Dom0. This means that while
> >>>>>> looking for overlapping BARs such devices cannot be found on Dom0's
> >>>>>> list of devices; DomXEN's list also needs to be scanned.
> >>>>>
> >>>>> Thanks for looking into this, I certainly didn't take hidden devices
> >>>>> into account for vPCI dom0.
> >>>>>
> >>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >>>>>> ---
> >>>>>> RFC: Patch intentionally mis-formatted, as the necessary re-indentation
> >>>>>>      would make the diff difficult to read. At this point I'd merely
> >>>>>>      like to gather input towards possible better approaches to solve
> >>>>>>      the issue (not the least because quite possibly there are further
> >>>>>>      places needing changing).
> >>>>>
> >>>>> I have a couple of questions, AFAICT we currently hide the serial
> >>>>> console and/or the VGA adapter if it's in use by Xen.
> >>>>>
> >>>>> I wonder whether we need to add vPCI handlers for those:
> >>>>> setup_one_hwdom_device will attempt to add vPCI handlers to the hidden
> >>>>> device because of the temporary override of pdev->domain done in
> >>>>> _setup_hwdom_pci_devices, but I think that for hidden devices we
> >>>>> shouldn't add vPCI handlers. We should instead block PCI config space
> >>>>> access from dom0 to the device so that it doesn't mess with Xen usage.
> >>>>
> >>>> The answer to this follows (I think) from the one below.
> >>>>
> >>>>> It's also not clear why does Xen want to have those hidden devices
> >>>>> partly controlled by dom0, as it would seem to me that dom0 interfering
> >>>>> with hidden devices in use by Xen can only lead to trouble.
> >>>>
> >>>> Dom0 can't interfere as long as it can only read from the device.
> >>>> Restricting accesses to reads is one of the purposes of "hiding"
> >>>> (the other is to make it impossible to assign these to a DomU). Not
> >>>> allowing Dom0 to read from such devices would lead to wrong PCI
> >>>> device discovery - some devices would be missing (which may or may
> >>>> not be merely a cosmetic issue). If the missing device is a multi-
> >>>> function one at function 0, other devices in the same slot may also
> >>>> not be found by Dom0 (depending on whether it looks at non-zero
> >>>> function numbers in this case).
> >>>
> >>> Hm, indeed seems possible that missing function 0 the whole device is
> >>> skipped.
> >>>
> >>> Maybe we need a special vPCI handling for those devices that just
> >>> allows reads but not writes, and that doesn't maps the BARs into dom0
> >>> p2m?
> >>
> >> Not sure about mapping. They could be mapped r/o. And they may
> >> actually need mapping for multi-function devices, but I guess for
> >> such devices to actually function properly then there would be
> >> more work required.
> > 
> > I'm also slightly puzzled as to why ehci-dbgp uses pci_hide_device
> > while ns16550 uses pci_ro_device instead.
> > 
> > Is this because the PCI device used by ehci-dbgp must be shared with
> > dom0 for other USB ports to be usable, and hence dom0 will need read
> > and write access to the device PCI config space and BARs?
> > 
> > Note that pci_hide_device is less restrictive than pci_ro_device, as
> > it doesn't mark the device as RO.
> > 
> > That would seem quite risky, as it's likely that dom0 will perform
> > some kind of reset of the USB device and thus the console will be
> > lost.
> 
> There's actually a protocol to prevent exactly that: See PHYSDEVOP_dbgp_op.

OK, so dom0 should be allowed write access to hidden devices config
space and BARs, while RO devices don't. That means that for hidden devices we
need to intercept PCI config space accesses and use vPCI for PVH.

For RO devices we already allow read-only access to it's config space
when using vPCI, albeit it might be nice to also add BARs as RO to the
guest physmap if they are enabled.

Will look into sorting this out.

Thanks, Roger.