xen/arm: do not try to add pci-domain for disabled devices

[PATCH] xen/arm: do not try to add pci-domain for disabled devices

Posted by Oleksandr Andrushchenko 2 years, 6 months ago

From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

If a PCI host bridge device is present in the device tree, but is
disabled, then its PCI host bridge driver was not instantiated.
This results in the following panic during Xen start:

(XEN) Device tree generation failed (-22).
(XEN)
(XEN) ****************************************
(XEN) Panic on CPU 0:
(XEN) Could not set up DOM0 guest OS
(XEN) ****************************************

Fix this by not adding linux,pci-domain property for hwdom if it is
neither available nor device enabled.

Fixes: 4cfab4425d39 ("xen/arm: Add linux,pci-domain property for hwdom if not available.")

Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
---
 xen/arch/arm/domain_build.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 124ade09123a..beeecf84a209 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -747,7 +747,8 @@ static int __init write_properties(struct domain *d, struct kernel_info *kinfo,
             return res;
     }
 
-    if ( is_pci_passthrough_enabled() && dt_device_type_is_equal(node, "pci") )
+    if ( is_pci_passthrough_enabled() && dt_device_type_is_equal(node, "pci") &&
+         dt_device_is_available(node) )
     {
         if ( !dt_find_property(node, "linux,pci-domain", NULL) )
         {
-- 
2.25.1

Re: [PATCH] xen/arm: do not try to add pci-domain for disabled devices

Posted by Julien Grall 2 years, 5 months ago

Hi Oleksandr,

On 27/10/2021 09:37, Oleksandr Andrushchenko wrote:
> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> 
> If a PCI host bridge device is present in the device tree, but is
> disabled, then its PCI host bridge driver was not instantiated.
> This results in the following panic during Xen start:
> 
> (XEN) Device tree generation failed (-22).

It would good to clarify in the commit message where the error is coming 
from. I think this is from pci_get_host_bridge_segment().

> (XEN)
> (XEN) ****************************************
> (XEN) Panic on CPU 0:
> (XEN) Could not set up DOM0 guest OS
> (XEN) ****************************************
> 
> Fix this by not adding linux,pci-domain property for hwdom if it is
> neither available nor device enabled.
 From my reading of the binding [1], the property should be present in 
all the hostbridges if one specify it. IOW, I think the property should 
also be added for hostbridges that are not available.

AFAICT, Linux will ignore hostbridge that are not available. But it 
feels to me we are twisting the rule. So, could we consider to allocate 
an unused number?

> 
> Fixes: 4cfab4425d39 ("xen/arm: Add linux,pci-domain property for hwdom if not available.")
> 
> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> ---
>   xen/arch/arm/domain_build.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 124ade09123a..beeecf84a209 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -747,7 +747,8 @@ static int __init write_properties(struct domain *d, struct kernel_info *kinfo,
>               return res;
>       }
>   
> -    if ( is_pci_passthrough_enabled() && dt_device_type_is_equal(node, "pci") )
> +    if ( is_pci_passthrough_enabled() && dt_device_type_is_equal(node, "pci") &&
 From my understanding, PCI device described in the DT would also have 
'type="pci"'. So we would also return an error because we don't have a 
corresponding hostbridge.

I think this check should be replaced with device_get_class(node) == 
DEVICE_PCI (confusingly DEVICE_PCI indicates that this is an 
hostbridge...). Although, I would consider to pass the device class as a 
parameter of write_properties() to avoid calling device_class() multiple 
time (it is already used twice in handle_node()).

I am aware this is not a bug introduced by your patch, but I think this 
should be dealt in this patch as well.

> +         dt_device_is_available(node) )

Shouldn't you also check that the hostbridge wasn't passthrough-ed?

>       {
>           if ( !dt_find_property(node, "linux,pci-domain", NULL) )
>           {
> 

Cheers,

[1] Documentation/devicetree/bindings/pci/pci.txt

-- 
Julien Grall

Re: [PATCH] xen/arm: do not try to add pci-domain for disabled devices

Posted by Stefano Stabellini 2 years, 5 months ago

On Wed, 27 Oct 2021, Julien Grall wrote:
> Hi Oleksandr,
> 
> On 27/10/2021 09:37, Oleksandr Andrushchenko wrote:
> > From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> > 
> > If a PCI host bridge device is present in the device tree, but is
> > disabled, then its PCI host bridge driver was not instantiated.
> > This results in the following panic during Xen start:
> > 
> > (XEN) Device tree generation failed (-22).
> 
> It would good to clarify in the commit message where the error is coming from.
> I think this is from pci_get_host_bridge_segment().
> 
> > (XEN)
> > (XEN) ****************************************
> > (XEN) Panic on CPU 0:
> > (XEN) Could not set up DOM0 guest OS
> > (XEN) ****************************************
> > 
> > Fix this by not adding linux,pci-domain property for hwdom if it is
> > neither available nor device enabled.
> From my reading of the binding [1], the property should be present in all the
> hostbridges if one specify it. IOW, I think the property should also be added
> for hostbridges that are not available.

Just wanted to say that I think you are right:

"""
It is required to either not set this property at all or set it for all
host bridges in the system, otherwise potentially conflicting domain numbers
may be assigned to root buses behind different host bridges.  The domain
number for each host bridge in the system must be unique.
"""

and I am ready to believe device trees with disabled bridges might have
(incorrectly) ignored the rule.

> AFAICT, Linux will ignore hostbridge that are not available. But it feels to
> me we are twisting the rule. So, could we consider to allocate an unused
> number?

I think that would be fine but it doesn't look easy from the current Xen
code point of view because the allocation depends on the Xen driver,
which we don't have. But I'll let others comment on it. Otherwise
skipping the disabled host bridge node for Dom0 sounds OK.

Re: [PATCH] xen/arm: do not try to add pci-domain for disabled devices

Posted by Julien Grall 2 years, 5 months ago

Hi Stefano,

On 28/10/2021 00:57, Stefano Stabellini wrote:
> On Wed, 27 Oct 2021, Julien Grall wrote:
>> Hi Oleksandr,
>>
>> On 27/10/2021 09:37, Oleksandr Andrushchenko wrote:
>>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>>
>>> If a PCI host bridge device is present in the device tree, but is
>>> disabled, then its PCI host bridge driver was not instantiated.
>>> This results in the following panic during Xen start:
>>>
>>> (XEN) Device tree generation failed (-22).
>>
>> It would good to clarify in the commit message where the error is coming from.
>> I think this is from pci_get_host_bridge_segment().
>>
>>> (XEN)
>>> (XEN) ****************************************
>>> (XEN) Panic on CPU 0:
>>> (XEN) Could not set up DOM0 guest OS
>>> (XEN) ****************************************
>>>
>>> Fix this by not adding linux,pci-domain property for hwdom if it is
>>> neither available nor device enabled.
>>  From my reading of the binding [1], the property should be present in all the
>> hostbridges if one specify it. IOW, I think the property should also be added
>> for hostbridges that are not available.
> 
> Just wanted to say that I think you are right:
> 
> """
> It is required to either not set this property at all or set it for all
> host bridges in the system, otherwise potentially conflicting domain numbers
> may be assigned to root buses behind different host bridges.  The domain
> number for each host bridge in the system must be unique.
> """
> 
> and I am ready to believe device trees with disabled bridges might have
> (incorrectly) ignored the rule.

Looking at linux/arch/arm64/boot/dts/, there are a few Device-Tree that 
contain the property "linux,pci-domain". All of them seems to also add 
it for disabled hostbridges.

However, I am under the impression that it is more common to have a 
Devide-Tree without any property "linux,pci-domain". When PCI support is 
enabled, Xen will generate the domain ID for the hostbridge and write it 
to the DT.

This doesn't work for disabled hostbridge and I think this is 
Oleksandr's problem. @Oleksandr can you confirm it?

> 
> 
>> AFAICT, Linux will ignore hostbridge that are not available. But it feels to
>> me we are twisting the rule. So, could we consider to allocate an unused
>> number?
> 
> I think that would be fine but it doesn't look easy from the current Xen
> code point of view because the allocation depends on the Xen driver,
> which we don't have. But I'll let others comment on it.
So what matters is Xen doesn't make things worse. We have two cases to care:
   1) Xen only has drivers for a part of the hostbriges
   2) Some hostbriges are disabled

Case 1) will definitely generate a DT that will make Linux unhappy if we 
have don't add the property consistently.

I believe that in case 2), current Linux will not check for the 
consistency. But that something, we probably should avoid to rely on.

I think in the two cases we can generate the domain ID by calling 
pci_get_new_domain_nr().

Now if we have to support inconsistent device-tree. Then we could 
collect the "linux,pci-domain" and find the maximum one. We would 
allocate a number above for any hostbridges with no property.

> Otherwise
> skipping the disabled host bridge node for Dom0 sounds OK.

At the moment, I haven't found any example of Device-Tree where 
"linux,pci-domain" will be only on part of the hostbridges (see above).

So I think we should avoid breaking the rule here at least until we have 
a "real" DT that break it.

Cheers,

-- 
Julien Grall

Re: [PATCH] xen/arm: do not try to add pci-domain for disabled devices

Posted by Oleksandr Andrushchenko 2 years, 5 months ago

Hi, Julien!

[snip]
On 28.10.21 13:48, Julien Grall wrote:
> Hi Stefano,
>
> Looking at linux/arch/arm64/boot/dts/, there are a few Device-Tree that contain the property "linux,pci-domain". All of them seems to also add it for disabled hostbridges.
>
> However, I am under the impression that it is more common to have a Devide-Tree without any property "linux,pci-domain". When PCI support is enabled, Xen will generate the domain ID for the hostbridge and write it to the DT.
>
> This doesn't work for disabled hostbridge and I think this is Oleksandr's problem. @Oleksandr can you confirm it?
Yes, what I have is a disabled node without "linux,pci-domain"

Re: [PATCH] xen/arm: do not try to add pci-domain for disabled devices

Posted by Julien Grall 2 years, 5 months ago


On 28/10/2021 12:01, Oleksandr Andrushchenko wrote:
> Hi, Julien!
> 
> [snip]
> On 28.10.21 13:48, Julien Grall wrote:
>> Hi Stefano,
>>
>> Looking at linux/arch/arm64/boot/dts/, there are a few Device-Tree that contain the property "linux,pci-domain". All of them seems to also add it for disabled hostbridges.
>>
>> However, I am under the impression that it is more common to have a Devide-Tree without any property "linux,pci-domain". When PCI support is enabled, Xen will generate the domain ID for the hostbridge and write it to the DT.
>>
>> This doesn't work for disabled hostbridge and I think this is Oleksandr's problem. @Oleksandr can you confirm it?
> Yes, what I have is a disabled node without "linux,pci-domain"

Just to confirm, is it the same for enabled hostbridges?

Cheers,

-- 
Julien Grall

Re: [PATCH] xen/arm: do not try to add pci-domain for disabled devices

Posted by Oleksandr Andrushchenko 2 years, 5 months ago


On 28.10.21 14:03, Julien Grall wrote:
>
>
> On 28/10/2021 12:01, Oleksandr Andrushchenko wrote:
>> Hi, Julien!
>>
>> [snip]
>> On 28.10.21 13:48, Julien Grall wrote:
>>> Hi Stefano,
>>>
>>> Looking at linux/arch/arm64/boot/dts/, there are a few Device-Tree that contain the property "linux,pci-domain". All of them seems to also add it for disabled hostbridges.
>>>
>>> However, I am under the impression that it is more common to have a Devide-Tree without any property "linux,pci-domain". When PCI support is enabled, Xen will generate the domain ID for the hostbridge and write it to the DT.
>>>
>>> This doesn't work for disabled hostbridge and I think this is Oleksandr's problem. @Oleksandr can you confirm it?
>> Yes, what I have is a disabled node without "linux,pci-domain"
>
> Just to confirm, is it the same for enabled hostbridges?
None of the bridges have "linux,pci-domain" in my device trees
>
> Cheers,
>

Re: [PATCH] xen/arm: do not try to add pci-domain for disabled devices

Posted by Stefano Stabellini 2 years, 5 months ago

On Thu, 28 Oct 2021, Julien Grall wrote:
> Hi Stefano,
> 
> On 28/10/2021 00:57, Stefano Stabellini wrote:
> > On Wed, 27 Oct 2021, Julien Grall wrote:
> > > Hi Oleksandr,
> > > 
> > > On 27/10/2021 09:37, Oleksandr Andrushchenko wrote:
> > > > From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> > > > 
> > > > If a PCI host bridge device is present in the device tree, but is
> > > > disabled, then its PCI host bridge driver was not instantiated.
> > > > This results in the following panic during Xen start:
> > > > 
> > > > (XEN) Device tree generation failed (-22).
> > > 
> > > It would good to clarify in the commit message where the error is coming
> > > from.
> > > I think this is from pci_get_host_bridge_segment().
> > > 
> > > > (XEN)
> > > > (XEN) ****************************************
> > > > (XEN) Panic on CPU 0:
> > > > (XEN) Could not set up DOM0 guest OS
> > > > (XEN) ****************************************
> > > > 
> > > > Fix this by not adding linux,pci-domain property for hwdom if it is
> > > > neither available nor device enabled.
> > >  From my reading of the binding [1], the property should be present in all
> > > the
> > > hostbridges if one specify it. IOW, I think the property should also be
> > > added
> > > for hostbridges that are not available.
> > 
> > Just wanted to say that I think you are right:
> > 
> > """
> > It is required to either not set this property at all or set it for all
> > host bridges in the system, otherwise potentially conflicting domain numbers
> > may be assigned to root buses behind different host bridges.  The domain
> > number for each host bridge in the system must be unique.
> > """
> > 
> > and I am ready to believe device trees with disabled bridges might have
> > (incorrectly) ignored the rule.
> 
> Looking at linux/arch/arm64/boot/dts/, there are a few Device-Tree that
> contain the property "linux,pci-domain". All of them seems to also add it for
> disabled hostbridges.
> 
> However, I am under the impression that it is more common to have a
> Devide-Tree without any property "linux,pci-domain". When PCI support is
> enabled, Xen will generate the domain ID for the hostbridge and write it to
> the DT.
> 
> This doesn't work for disabled hostbridge and I think this is Oleksandr's
> problem. @Oleksandr can you confirm it?
> 
> > 
> > 
> > > AFAICT, Linux will ignore hostbridge that are not available. But it feels
> > > to
> > > me we are twisting the rule. So, could we consider to allocate an unused
> > > number?
> > 
> > I think that would be fine but it doesn't look easy from the current Xen
> > code point of view because the allocation depends on the Xen driver,
> > which we don't have. But I'll let others comment on it.
> So what matters is Xen doesn't make things worse. We have two cases to care:
>   1) Xen only has drivers for a part of the hostbriges
>   2) Some hostbriges are disabled
> 
> Case 1) will definitely generate a DT that will make Linux unhappy if we have
> don't add the property consistently.

Good point!

Re: [PATCH] xen/arm: do not try to add pci-domain for disabled devices

Posted by Oleksandr Andrushchenko 2 years, 5 months ago


On 28.10.21 19:50, Stefano Stabellini wrote:
> On Thu, 28 Oct 2021, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 28/10/2021 00:57, Stefano Stabellini wrote:
>>> On Wed, 27 Oct 2021, Julien Grall wrote:
>>>> Hi Oleksandr,
>>>>
>>>> On 27/10/2021 09:37, Oleksandr Andrushchenko wrote:
>>>>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>>>>
>>>>> If a PCI host bridge device is present in the device tree, but is
>>>>> disabled, then its PCI host bridge driver was not instantiated.
>>>>> This results in the following panic during Xen start:
>>>>>
>>>>> (XEN) Device tree generation failed (-22).
>>>> It would good to clarify in the commit message where the error is coming
>>>> from.
>>>> I think this is from pci_get_host_bridge_segment().
>>>>
>>>>> (XEN)
>>>>> (XEN) ****************************************
>>>>> (XEN) Panic on CPU 0:
>>>>> (XEN) Could not set up DOM0 guest OS
>>>>> (XEN) ****************************************
>>>>>
>>>>> Fix this by not adding linux,pci-domain property for hwdom if it is
>>>>> neither available nor device enabled.
>>>>   From my reading of the binding [1], the property should be present in all
>>>> the
>>>> hostbridges if one specify it. IOW, I think the property should also be
>>>> added
>>>> for hostbridges that are not available.
>>> Just wanted to say that I think you are right:
>>>
>>> """
>>> It is required to either not set this property at all or set it for all
>>> host bridges in the system, otherwise potentially conflicting domain numbers
>>> may be assigned to root buses behind different host bridges.  The domain
>>> number for each host bridge in the system must be unique.
>>> """
>>>
>>> and I am ready to believe device trees with disabled bridges might have
>>> (incorrectly) ignored the rule.
>> Looking at linux/arch/arm64/boot/dts/, there are a few Device-Tree that
>> contain the property "linux,pci-domain". All of them seems to also add it for
>> disabled hostbridges.
>>
>> However, I am under the impression that it is more common to have a
>> Devide-Tree without any property "linux,pci-domain". When PCI support is
>> enabled, Xen will generate the domain ID for the hostbridge and write it to
>> the DT.
>>
>> This doesn't work for disabled hostbridge and I think this is Oleksandr's
>> problem. @Oleksandr can you confirm it?
>>
>>>
>>>> AFAICT, Linux will ignore hostbridge that are not available. But it feels
>>>> to
>>>> me we are twisting the rule. So, could we consider to allocate an unused
>>>> number?
>>> I think that would be fine but it doesn't look easy from the current Xen
>>> code point of view because the allocation depends on the Xen driver,
>>> which we don't have. But I'll let others comment on it.
>> So what matters is Xen doesn't make things worse. We have two cases to care:
>>    1) Xen only has drivers for a part of the hostbriges
>>    2) Some hostbriges are disabled
>>
>> Case 1) will definitely generate a DT that will make Linux unhappy if we have
>> don't add the property consistently.
> Good point!
So, the bottom line: we add the property regardless of the status?
And the segment we assign for the disabled ones is pci_get_new_domain_nr()?
I guess nothing bad happens if we have say 3 bridges:
okay - > seg 0
disabled - > seg 1
okay - > seg 2

Re: [PATCH] xen/arm: do not try to add pci-domain for disabled devices

Posted by Stefano Stabellini 2 years, 5 months ago

On Thu, 28 Oct 2021, Oleksandr Andrushchenko wrote:
> On 28.10.21 19:50, Stefano Stabellini wrote:
> > On Thu, 28 Oct 2021, Julien Grall wrote:
> >> Hi Stefano,
> >>
> >> On 28/10/2021 00:57, Stefano Stabellini wrote:
> >>> On Wed, 27 Oct 2021, Julien Grall wrote:
> >>>> Hi Oleksandr,
> >>>>
> >>>> On 27/10/2021 09:37, Oleksandr Andrushchenko wrote:
> >>>>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> >>>>>
> >>>>> If a PCI host bridge device is present in the device tree, but is
> >>>>> disabled, then its PCI host bridge driver was not instantiated.
> >>>>> This results in the following panic during Xen start:
> >>>>>
> >>>>> (XEN) Device tree generation failed (-22).
> >>>> It would good to clarify in the commit message where the error is coming
> >>>> from.
> >>>> I think this is from pci_get_host_bridge_segment().
> >>>>
> >>>>> (XEN)
> >>>>> (XEN) ****************************************
> >>>>> (XEN) Panic on CPU 0:
> >>>>> (XEN) Could not set up DOM0 guest OS
> >>>>> (XEN) ****************************************
> >>>>>
> >>>>> Fix this by not adding linux,pci-domain property for hwdom if it is
> >>>>> neither available nor device enabled.
> >>>>   From my reading of the binding [1], the property should be present in all
> >>>> the
> >>>> hostbridges if one specify it. IOW, I think the property should also be
> >>>> added
> >>>> for hostbridges that are not available.
> >>> Just wanted to say that I think you are right:
> >>>
> >>> """
> >>> It is required to either not set this property at all or set it for all
> >>> host bridges in the system, otherwise potentially conflicting domain numbers
> >>> may be assigned to root buses behind different host bridges.  The domain
> >>> number for each host bridge in the system must be unique.
> >>> """
> >>>
> >>> and I am ready to believe device trees with disabled bridges might have
> >>> (incorrectly) ignored the rule.
> >> Looking at linux/arch/arm64/boot/dts/, there are a few Device-Tree that
> >> contain the property "linux,pci-domain". All of them seems to also add it for
> >> disabled hostbridges.
> >>
> >> However, I am under the impression that it is more common to have a
> >> Devide-Tree without any property "linux,pci-domain". When PCI support is
> >> enabled, Xen will generate the domain ID for the hostbridge and write it to
> >> the DT.
> >>
> >> This doesn't work for disabled hostbridge and I think this is Oleksandr's
> >> problem. @Oleksandr can you confirm it?
> >>
> >>>
> >>>> AFAICT, Linux will ignore hostbridge that are not available. But it feels
> >>>> to
> >>>> me we are twisting the rule. So, could we consider to allocate an unused
> >>>> number?
> >>> I think that would be fine but it doesn't look easy from the current Xen
> >>> code point of view because the allocation depends on the Xen driver,
> >>> which we don't have. But I'll let others comment on it.
> >> So what matters is Xen doesn't make things worse. We have two cases to care:
> >>    1) Xen only has drivers for a part of the hostbriges
> >>    2) Some hostbriges are disabled
> >>
> >> Case 1) will definitely generate a DT that will make Linux unhappy if we have
> >> don't add the property consistently.
> > Good point!
> So, the bottom line: we add the property regardless of the status?
> And the segment we assign for the disabled ones is pci_get_new_domain_nr()?

Yeah I think so


> I guess nothing bad happens if we have say 3 bridges:
> okay - > seg 0
> disabled - > seg 1
> okay - > seg 2

It should be fine I think