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

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

Posted by Stefano Stabellini 3 years, 2 months ago

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.

Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>

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] xen: workaround missing device_type property in pci/pcie nodes

Posted by Bertrand Marquis 3 years, 2 months ago

Hi Stefano,

> On 8 Feb 2021, at 23:56, 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.

We are really handling here a wrong device-tree bug that could easily be fixed
by the user.
We should at least mention in the commit message that this is a workaround
to solve RPi4 buggy device tree.

> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> 
> 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");

The Linux commit is a bit more restrictive and only does that for “pcie”.
Any reason why you also want to have this workaround done also for “pci” ?

Cheers
Bertrand

> +
> +    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] xen: workaround missing device_type property in pci/pcie nodes

Posted by Stefano Stabellini 3 years, 2 months ago

On Tue, 9 Feb 2021, Bertrand Marquis wrote:
> Hi Stefano,
> 
> > On 8 Feb 2021, at 23:56, 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.
> 
> We are really handling here a wrong device-tree bug that could easily be fixed
> by the user.
> We should at least mention in the commit message that this is a workaround
> to solve RPi4 buggy device tree.

Not sure if it can be "easily" fixed by the user -- it took me a few
hours to figure out what the problem was, and I know Xen and device tree
pretty well :-)

Yes it would be good to have a link to the discussion in the commit
message, using the Link tag. It could be done on commit, or I can add it
to the next version.

Link: https://lore.kernel.org/xen-devel/YBmQQ3Tzu++AadKx@mattapan.m5p.com/


> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> > 
> > 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");
> 
> The Linux commit is a bit more restrictive and only does that for “pcie”.
> Any reason why you also want to have this workaround done also for “pci” ?

Yes, that's because in our case the offending node is named "pci" not
"pcie" so the original Linux commit wouldn't cover it.


> > +
> > +    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] xen: workaround missing device_type property in pci/pcie nodes

Posted by Julien Grall 3 years, 2 months ago

Hi Stefano,

On 09/02/2021 17:47, Stefano Stabellini wrote:
> On Tue, 9 Feb 2021, Bertrand Marquis wrote:
>> Hi Stefano,
>>
>>> On 8 Feb 2021, at 23:56, 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.
I am a bit confused with this sentence... How did you manage to boot Xen 
on RPi4 before hand?

>>
>> We are really handling here a wrong device-tree bug that could easily be fixed
>> by the user.
>> We should at least mention in the commit message that this is a workaround
>> to solve RPi4 buggy device tree.
> 
> Not sure if it can be "easily" fixed by the user -- it took me a few
> hours to figure out what the problem was, and I know Xen and device tree
> pretty well :-)
> 
> Yes it would be good to have a link to the discussion in the commit
> message, using the Link tag. It could be done on commit, or I can add it
> to the next version.

A summary of the discussion would be useful in the commit message so a 
reader can easily make the connection between the Linux commit and the 
Xen one.

> 
> Link: https://lore.kernel.org/xen-devel/YBmQQ3Tzu++AadKx@mattapan.m5p.com/
> 
> 
>>>
>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
>>>
>>> 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");
>>
>> The Linux commit is a bit more restrictive and only does that for “pcie”.
>> Any reason why you also want to have this workaround done also for “pci” ?
> 
> Yes, that's because in our case the offending node is named "pci" not
> "pcie" so the original Linux commit wouldn't cover it.
> 
> 
>>> +
>>> +    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,
>>>
>>

-- 
Julien Grall

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

Posted by Stefano Stabellini 3 years, 2 months ago

On Tue, 9 Feb 2021, Julien Grall wrote:
> Hi Stefano,
> 
> On 09/02/2021 17:47, Stefano Stabellini wrote:
> > On Tue, 9 Feb 2021, Bertrand Marquis wrote:
> > > Hi Stefano,
> > > 
> > > > On 8 Feb 2021, at 23:56, 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.
> I am a bit confused with this sentence... How did you manage to boot Xen on
> RPi4 before hand?

The older rpi kernel that I was using didn't have the problematic pci
node in device tree at all.


> > > We are really handling here a wrong device-tree bug that could easily be
> > > fixed
> > > by the user.
> > > We should at least mention in the commit message that this is a workaround
> > > to solve RPi4 buggy device tree.
> > 
> > Not sure if it can be "easily" fixed by the user -- it took me a few
> > hours to figure out what the problem was, and I know Xen and device tree
> > pretty well :-)
> > 
> > Yes it would be good to have a link to the discussion in the commit
> > message, using the Link tag. It could be done on commit, or I can add it
> > to the next version.
> 
> A summary of the discussion would be useful in the commit message so a reader
> can easily make the connection between the Linux commit and the Xen one.

OK, good idea


> > 
> > Link: https://lore.kernel.org/xen-devel/YBmQQ3Tzu++AadKx@mattapan.m5p.com/
> > 
> > 
> > > > 
> > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> > > > 
> > > > 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");
> > > 
> > > The Linux commit is a bit more restrictive and only does that for “pcie”.
> > > Any reason why you also want to have this workaround done also for “pci” ?
> > 
> > Yes, that's because in our case the offending node is named "pci" not
> > "pcie" so the original Linux commit wouldn't cover it.
> > 
> > 
> > > > +
> > > > +    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,