| Message ID | 20211027083730.1406947-1-andr2000@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/arm: do not try to add pci-domain for disabled devices | expand |
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
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.
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,
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"
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,
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, >
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!
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
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
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) ) {