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

[Julien Grall <julien@xxxxxxx>]

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@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxx/
>
>
> > > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
> > >
> > > 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