[PATCH v2] xen: workaround missing device_type property in pci/pcie nodes

[PATCH v2] xen: workaround missing device_type property in pci/pcie nodes

[Stefano Stabellini]

PCI buses differ from default buses in a few important ways, so it is
important to detect them properly. Normally, PCI buses are expected to
have the following property:

    device_type = "pci"

In reality, it is not always the case. To handle PCI bus nodes that
don't have the device_type property, also consider the node name: if the
node name is "pcie" or "pci" then consider the bus as a PCI bus.

This commit is based on the Linux kernel commit
d1ac0002dd29 "of: address: Work around missing device_type property in
pcie nodes".

This fixes Xen boot on RPi4. Some RPi4 kernels have the following node
on their device trees:

&pcie0 {
	pci@1,0 {
		#address-cells = <3>;
		#size-cells = <2>;
		ranges;

		reg = <0 0 0 0 0>;

		usb@1,0 {
				reg = <0x10000 0 0 0 0>;
				resets = <&reset RASPBERRYPI_FIRMWARE_RESET_ID_USB>;
		};
	};
};

The pci@1,0 node is a PCI bus. If we parse the node and its children as
a default bus, the reg property under usb@1,0 would have to be
interpreted as an address range mappable by the CPU, which is not the
case and would break.

Link: https://lore.kernel.org/xen-devel/YBmQQ3Tzu++AadKx@mattapan.m5p.com/
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
---
Changes in v2:
- improve commit message

diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
index 18825e333e..f1a96a3b90 100644
--- a/xen/common/device_tree.c
+++ b/xen/common/device_tree.c
@@ -563,14 +563,28 @@ static unsigned int dt_bus_default_get_flags(const __be32 *addr)
  * PCI bus specific translator
  */
 
+static bool_t dt_node_is_pci(const struct dt_device_node *np)
+{
+    bool is_pci = !strcmp(np->name, "pcie") || !strcmp(np->name, "pci");
+
+    if (is_pci)
+        printk(XENLOG_WARNING "%s: Missing device_type\n", np->full_name);
+
+    return is_pci;
+}
+
 static bool_t dt_bus_pci_match(const struct dt_device_node *np)
 {
     /*
      * "pciex" is PCI Express "vci" is for the /chaos bridge on 1st-gen PCI
      * powermacs "ht" is hypertransport
+     *
+     * If none of the device_type match, and that the node name is
+     * "pcie" or "pci", accept the device as PCI (with a warning).
      */
     return !strcmp(np->type, "pci") || !strcmp(np->type, "pciex") ||
-        !strcmp(np->type, "vci") || !strcmp(np->type, "ht");
+        !strcmp(np->type, "vci") || !strcmp(np->type, "ht") ||
+        dt_node_is_pci(np);
 }
 
 static void dt_bus_pci_count_cells(const struct dt_device_node *np,

Re: [PATCH v2] xen: workaround missing device_type property in pci/pcie nodes

[Stefano Stabellini]

On Tue, 9 Feb 2021, Stefano Stabellini wrote:
> PCI buses differ from default buses in a few important ways, so it is
> important to detect them properly. Normally, PCI buses are expected to
> have the following property:
> 
>     device_type = "pci"
> 
> In reality, it is not always the case. To handle PCI bus nodes that
> don't have the device_type property, also consider the node name: if the
> node name is "pcie" or "pci" then consider the bus as a PCI bus.
> 
> This commit is based on the Linux kernel commit
> d1ac0002dd29 "of: address: Work around missing device_type property in
> pcie nodes".
> 
> This fixes Xen boot on RPi4. Some RPi4 kernels have the following node
> on their device trees:
> 
> &pcie0 {
> 	pci@1,0 {
> 		#address-cells = <3>;
> 		#size-cells = <2>;
> 		ranges;
> 
> 		reg = <0 0 0 0 0>;
> 
> 		usb@1,0 {
> 				reg = <0x10000 0 0 0 0>;
> 				resets = <&reset RASPBERRYPI_FIRMWARE_RESET_ID_USB>;

there is one more tab than needed here, sorry


> 		};
> 	};
> };
> 
> The pci@1,0 node is a PCI bus. If we parse the node and its children as
> a default bus, the reg property under usb@1,0 would have to be
> interpreted as an address range mappable by the CPU, which is not the
> case and would break.
> 
> Link: https://lore.kernel.org/xen-devel/YBmQQ3Tzu++AadKx@mattapan.m5p.com/
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> ---
> Changes in v2:
> - improve commit message
> 
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 18825e333e..f1a96a3b90 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -563,14 +563,28 @@ static unsigned int dt_bus_default_get_flags(const __be32 *addr)
>   * PCI bus specific translator
>   */
>  
> +static bool_t dt_node_is_pci(const struct dt_device_node *np)
> +{
> +    bool is_pci = !strcmp(np->name, "pcie") || !strcmp(np->name, "pci");
> +
> +    if (is_pci)
> +        printk(XENLOG_WARNING "%s: Missing device_type\n", np->full_name);
> +
> +    return is_pci;
> +}
> +
>  static bool_t dt_bus_pci_match(const struct dt_device_node *np)
>  {
>      /*
>       * "pciex" is PCI Express "vci" is for the /chaos bridge on 1st-gen PCI
>       * powermacs "ht" is hypertransport
> +     *
> +     * If none of the device_type match, and that the node name is
> +     * "pcie" or "pci", accept the device as PCI (with a warning).
>       */
>      return !strcmp(np->type, "pci") || !strcmp(np->type, "pciex") ||
> -        !strcmp(np->type, "vci") || !strcmp(np->type, "ht");
> +        !strcmp(np->type, "vci") || !strcmp(np->type, "ht") ||
> +        dt_node_is_pci(np);
>  }
>  
>  static void dt_bus_pci_count_cells(const struct dt_device_node *np,
> 

Re: [PATCH v2] xen: workaround missing device_type property in pci/pcie nodes

[Andrew Cooper]

On 09/02/2021 19:53, Stefano Stabellini wrote:
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 18825e333e..f1a96a3b90 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -563,14 +563,28 @@ static unsigned int dt_bus_default_get_flags(const __be32 *addr)
>   * PCI bus specific translator
>   */
>  
> +static bool_t dt_node_is_pci(const struct dt_device_node *np)
> +{
> +    bool is_pci = !strcmp(np->name, "pcie") || !strcmp(np->name, "pci");
> +
> +    if (is_pci)

bool on the function, and spaces here, which I'm sure you can fix while
committing :)

~Andrew

Re: [PATCH v2] xen: workaround missing device_type property in pci/pcie nodes

[Stefano Stabellini]

On Tue, 9 Feb 2021, Andrew Cooper wrote:
> On 09/02/2021 19:53, Stefano Stabellini wrote:
> > diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> > index 18825e333e..f1a96a3b90 100644
> > --- a/xen/common/device_tree.c
> > +++ b/xen/common/device_tree.c
> > @@ -563,14 +563,28 @@ static unsigned int dt_bus_default_get_flags(const __be32 *addr)
> >   * PCI bus specific translator
> >   */
> >  
> > +static bool_t dt_node_is_pci(const struct dt_device_node *np)
> > +{
> > +    bool is_pci = !strcmp(np->name, "pcie") || !strcmp(np->name, "pci");
> > +
> > +    if (is_pci)
> 
> bool on the function, and spaces here, which I'm sure you can fix while
> committing :)

Oops, thanks Andrew

Re: [PATCH v2] xen: workaround missing device_type property in pci/pcie nodes

[Elliott Mitchell]

On Tue, Feb 09, 2021 at 11:53:34AM -0800, Stefano Stabellini wrote:
> This fixes Xen boot on RPi4. Some RPi4 kernels have the following node
> on their device trees:

IMO I like keeping the reference to Linux kernel d1ac0002dd29 in the
commit message.  The commit is distinctly informative and takes some
searching to find in the thread archive.  This though is merely /my/
opinion.

Two builds later to ensure I've got a build which doesn't work due to the
problematic device-tree and one with the patch to ensure it does fix the
issue and:

Tested-by: Elliott Mitchell <ehem+xen@m5p.com>


Note to Jukka Kaartinen, people who merely build from source are useful
for confirming proposed fixes work.  The above line gets added to commit
messages to note people have tried it and it works for them.


-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         ehem+sigmsg@m5p.com  PGP 87145445         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445

Re: [PATCH v2] xen: workaround missing device_type property in pci/pcie nodes

[Jukka Kaartinen]

On 10.2.2021 3.59, Elliott Mitchell wrote:
> On Tue, Feb 09, 2021 at 11:53:34AM -0800, Stefano Stabellini wrote:
>> This fixes Xen boot on RPi4. Some RPi4 kernels have the following node
>> on their device trees:
> 
> IMO I like keeping the reference to Linux kernel d1ac0002dd29 in the
> commit message.  The commit is distinctly informative and takes some
> searching to find in the thread archive.  This though is merely /my/
> opinion.
> 
> Two builds later to ensure I've got a build which doesn't work due to the
> problematic device-tree and one with the patch to ensure it does fix the
> issue and:
> 
> Tested-by: Elliott Mitchell <ehem+xen@m5p.com>
> 
> 
> Note to Jukka Kaartinen, people who merely build from source are useful
> for confirming proposed fixes work.  The above line gets added to commit
> messages to note people have tried it and it works for them.
> 
> 

Thanks for the info!
I tested the fix and it works fine.

Tested-by: Jukka Kaartinen <jukka.kaartinen@unikie.com>


-Jukka

Re: [PATCH v2] xen: workaround missing device_type property in pci/pcie nodes

[Bertrand Marquis]

Hi Stefano,

> On 9 Feb 2021, at 19:53, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> PCI buses differ from default buses in a few important ways, so it is
> important to detect them properly. Normally, PCI buses are expected to
> have the following property:
> 
>    device_type = "pci"
> 
> In reality, it is not always the case. To handle PCI bus nodes that
> don't have the device_type property, also consider the node name: if the
> node name is "pcie" or "pci" then consider the bus as a PCI bus.
> 
> This commit is based on the Linux kernel commit
> d1ac0002dd29 "of: address: Work around missing device_type property in
> pcie nodes".
> 
> This fixes Xen boot on RPi4. Some RPi4 kernels have the following node
> on their device trees:
> 
> &pcie0 {
> 	pci@1,0 {
> 		#address-cells = <3>;
> 		#size-cells = <2>;
> 		ranges;
> 
> 		reg = <0 0 0 0 0>;
> 
> 		usb@1,0 {
> 				reg = <0x10000 0 0 0 0>;
> 				resets = <&reset RASPBERRYPI_FIRMWARE_RESET_ID_USB>;
> 		};
> 	};
> };
> 
> The pci@1,0 node is a PCI bus. If we parse the node and its children as
> a default bus, the reg property under usb@1,0 would have to be
> interpreted as an address range mappable by the CPU, which is not the
> case and would break.
> 
> Link: https://lore.kernel.org/xen-devel/YBmQQ3Tzu++AadKx@mattapan.m5p.com/
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
With the type, spaces and tab fixes already found:

Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Thanks the commit message is more clear :-)

Cheers
Bertrand

> ---
> Changes in v2:
> - improve commit message
> 
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 18825e333e..f1a96a3b90 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -563,14 +563,28 @@ static unsigned int dt_bus_default_get_flags(const __be32 *addr)
>  * PCI bus specific translator
>  */
> 
> +static bool_t dt_node_is_pci(const struct dt_device_node *np)
> +{
> +    bool is_pci = !strcmp(np->name, "pcie") || !strcmp(np->name, "pci");
> +
> +    if (is_pci)
> +        printk(XENLOG_WARNING "%s: Missing device_type\n", np->full_name);
> +
> +    return is_pci;
> +}
> +
> static bool_t dt_bus_pci_match(const struct dt_device_node *np)
> {
>     /*
>      * "pciex" is PCI Express "vci" is for the /chaos bridge on 1st-gen PCI
>      * powermacs "ht" is hypertransport
> +     *
> +     * If none of the device_type match, and that the node name is
> +     * "pcie" or "pci", accept the device as PCI (with a warning).
>      */
>     return !strcmp(np->type, "pci") || !strcmp(np->type, "pciex") ||
> -        !strcmp(np->type, "vci") || !strcmp(np->type, "ht");
> +        !strcmp(np->type, "vci") || !strcmp(np->type, "ht") ||
> +        dt_node_is_pci(np);
> }
> 
> static void dt_bus_pci_count_cells(const struct dt_device_node *np,
> 

Re: [PATCH v2] xen: workaround missing device_type property in pci/pcie nodes

[Julien Grall]

On 09/02/2021 19:53, Stefano Stabellini wrote:
> PCI buses differ from default buses in a few important ways, so it is
> important to detect them properly. Normally, PCI buses are expected to
> have the following property:
> 
>      device_type = "pci"
> 
> In reality, it is not always the case. To handle PCI bus nodes that
> don't have the device_type property, also consider the node name: if the
> node name is "pcie" or "pci" then consider the bus as a PCI bus.
> 
> This commit is based on the Linux kernel commit
> d1ac0002dd29 "of: address: Work around missing device_type property in
> pcie nodes".
> 
> This fixes Xen boot on RPi4. Some RPi4 kernels have the following node
> on their device trees:
> 
> &pcie0 {
> 	pci@1,0 {
> 		#address-cells = <3>;
> 		#size-cells = <2>;
> 		ranges;
> 
> 		reg = <0 0 0 0 0>;
> 
> 		usb@1,0 {
> 				reg = <0x10000 0 0 0 0>;
> 				resets = <&reset RASPBERRYPI_FIRMWARE_RESET_ID_USB>;
> 		};
> 	};
> };
> 
> The pci@1,0 node is a PCI bus. If we parse the node and its children as
> a default bus, the reg property under usb@1,0 would have to be
> interpreted as an address range mappable by the CPU, which is not the
> case and would break.
> 
> Link: https://lore.kernel.org/xen-devel/YBmQQ3Tzu++AadKx@mattapan.m5p.com/
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall