|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: Question on PCIe Device Tree bindings, Was: [PATCH] xen/arm: domain_build: Ignore device nodes with invalid addresses
On Thu, 4 Feb 2021, Rob Herring wrote:
> On Thu, Feb 4, 2021 at 11:56 AM Stefano Stabellini
> <sstabellini@xxxxxxxxxx> wrote:
> >
> > Hi Rob,
> >
> > We have a question on the PCIe device tree bindings. In summary, we have
> > come across the Raspberry Pi 4 PCIe description below:
> >
> >
> > pcie0: pcie@7d500000 {
> > compatible = "brcm,bcm2711-pcie";
> > reg = <0x0 0x7d500000 0x0 0x9310>;
> > device_type = "pci";
> > #address-cells = <3>;
> > #interrupt-cells = <1>;
> > #size-cells = <2>;
> > interrupts = <GIC_SPI 148 IRQ_TYPE_LEVEL_HIGH>,
> > <GIC_SPI 148 IRQ_TYPE_LEVEL_HIGH>;
> > interrupt-names = "pcie", "msi";
> > interrupt-map-mask = <0x0 0x0 0x0 0x7>;
> > interrupt-map = <0 0 0 1 &gicv2 GIC_SPI 143
> > IRQ_TYPE_LEVEL_HIGH>;
> > msi-controller;
> > msi-parent = <&pcie0>;
> >
> > ranges = <0x02000000 0x0 0xc0000000 0x6 0x00000000
> > 0x0 0x40000000>;
> > /*
> > * The wrapper around the PCIe block has a bug
> > * preventing it from accessing beyond the first 3GB of
> > * memory.
> > */
> > dma-ranges = <0x02000000 0x0 0x00000000 0x0 0x00000000
> > 0x0 0xc0000000>;
> > brcm,enable-ssc;
> >
> > 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>;
> > };
> > };
> > };
> >
> >
> > Xen fails to parse it with an error because it tries to remap reg =
> > <0x10000 0 0 0 0> as if it was a CPU address and of course it fails.
> >
> > Reading the device tree description in details, I cannot tell if Xen has
> > a bug: the ranges property under pci@1,0 means that pci@1,0 is treated
> > like a default bus (not a PCI bus), hence, the children regs are
> > translated using the ranges property of the parent (pcie@7d500000).
> >
> > Is it possible that the device tree is missing device_type =
> > "pci" under pci@1,0? Or is it just implied because pci@1,0 is a child of
> > pcie@7d500000?
>
> Indeed, it should have device_type. Linux (only recently due to
> another missing device_type case) will also look at node name, but
> only 'pcie'.
>
> We should be able to create (or extend pci-bus.yaml) a schema to catch
> this case.
Ah, that is what I needed to know, thank you! Is Linux considering a
node named "pcie" as if it has device_type = "pci"?
In Xen, also to cover the RPi4 case, maybe I could add a check for the
node name to be "pci" or "pcie" and if so Xen could assume device_type =
"pci".
> > I'd like to make Xen able to parse this device tree without errors but I
> > am not sure what is the best way to fix it.
> >
> > Thanks for any help you can provide!
> >
> > Cheers,
> >
> > Stefano
> >
> >
> >
> > On Thu, 4 Feb 2021, Julien Grall wrote:
> > > On 04/02/2021 00:13, Stefano Stabellini wrote:
> > > > On Wed, 3 Feb 2021, Julien Grall wrote:
> > > > > On Wed, 3 Feb 2021 at 22:18, Stefano Stabellini
> > > > > <sstabellini@xxxxxxxxxx>
> > > > > wrote:
> > > > > > > > But aside from PCIe, let's say that we know of a few nodes for
> > > > > > > > which
> > > > > > > > "reg" needs a special treatment. I am not sure it makes sense to
> > > > > > > > proceed
> > > > > > > > with parsing those nodes without knowing how to deal with that.
> > > > > > >
> > > > > > > I believe that most of the time the "special" treatment would be
> > > > > > > to
> > > > > > > ignore the
> > > > > > > property "regs" as it will not be an CPU memory address.
> > > > > > >
> > > > > > > > So maybe
> > > > > > > > we should add those nodes to skip_matches until we know what to
> > > > > > > > do
> > > > > > > > with
> > > > > > > > them. At that point, I would imagine we would introduce a
> > > > > > > > special
> > > > > > > > handle_device function that knows what to do. In the case of
> > > > > > > > PCIe,
> > > > > > > > something like "handle_device_pcie".
> > > > > > > Could you outline how "handle_device_pcie()" will differ with
> > > > > > > handle_node()?
> > > > > > >
> > > > > > > In fact, the problem is not the PCIe node directly. Instead, it
> > > > > > > is the
> > > > > > > second
> > > > > > > level of nodes below it (i.e usb@...).
> > > > > > >
> > > > > > > The current implementation of dt_number_of_address() only look at
> > > > > > > the
> > > > > > > bus type
> > > > > > > of the parent. As the parent has no bus type and "ranges" then it
> > > > > > > thinks this
> > > > > > > is something we can translate to a CPU address.
> > > > > > >
> > > > > > > However, this is below a PCI bus so the meaning of "reg" is
> > > > > > > completely
> > > > > > > different. In this case, we only need to ignore "reg".
> > > > > >
> > > > > > I see what you are saying and I agree: if we had to introduce a
> > > > > > special
> > > > > > case for PCI, then dt_number_of_address() seems to be a good
> > > > > > place. In
> > > > > > fact, we already have special PCI handling, see our
> > > > > > __dt_translate_address function and
> > > > > > xen/common/device_tree.c:dt_busses.
> > > > > >
> > > > > > Which brings the question: why is this actually failing?
> > > > >
> > > > > I already hinted at the reason in my previous e-mail :). Let me expand
> > > > > a bit more.
> > > > >
> > > > > >
> > > > > > pcie0 {
> > > > > > ranges = <0x02000000 0x0 0xc0000000 0x6 0x00000000 0x0
> > > > > > 0x40000000>;
> > > > > >
> > > > > > Which means that PCI addresses 0xc0000000-0x100000000 become
> > > > > > 0x600000000-0x700000000.
> > > > > >
> > > > > > The offending DT is:
> > > > > >
> > > > > > &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>;
> > > > > > };
> > > > > > };
> > > > > > };
> > > > > >
> > > > > >
> > > > > > reg = <0x10000 0 0 0 0> means that usb@1,0 is PCI device 01:00.0.
> > > > > > However, the rest of the regs cells are left as zero. It shouldn't
> > > > > > be an
> > > > > > issue because usb@1,0 is a child of pci@1,0 but pci@1,0 is not a
> > > > > > bus.
> > > > >
> > > > > The property "ranges" is used to define a mapping or translation
> > > > > between the address space of the "bus" (here pci@1,0) and the address
> > > > > space of the bus node's parent (&pcie0).
> > > > > IOW, it means "reg" in usb@1,0 is an address on the PCI bus (i.e.
> > > > > BDF).
> > > > >
> > > > > The problem is dt_number_of_address() will only look at the "bus" type
> > > > > of the parent using dt_match_bus(). This will return the default bus
> > > > > (see dt_bus_default_match()), because this is a property "ranges" in
> > > > > the parent node (i.e. pci@1,0). Therefore...
> > > > >
> > > > > > So
> > > > > > in theory dt_number_of_address() should already return 0 for it.
> > > > >
> > > > > ... dt_number_of_address() will return 1 even if the address is not a
> > > > > CPU address. So when Xen will try to translate it, it will fail.
> > > > >
> > > > > >
> > > > > > Maybe reg = <0 0 0 0 0> is the problem. In that case, we could
> > > > > > simply
> > > > > > add a check to skip 0 size ranges. Just a hack to explain what I
> > > > > > mean:
> > > > >
> > > > > The parent of pci@1,0 is a PCI bridge (see the property type), so the
> > > > > CPU addresses are found not via "regs" but "assigned-addresses".
> > > > >
> > > > > In this situation, "regs" will have a different meaning and therefore
> > > > > there is no promise that the size will be 0.
> > > >
> > > > I copy/pasted the following:
> > > >
> > > > 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>;
> > > > };
> > > > };
> > > >
> > > > under pcie0 in my DTS to see what happens (the node is not there in the
> > > > device tree for the rpi-5.9.y kernel.) It results in the expected error:
> > > >
> > > > (XEN) Unable to retrieve address 0 for
> > > > /scb/pcie@7d500000/pci@1,0/usb@1,0
> > > > (XEN) Device tree generation failed (-22).
> > > >
> > > > I could verify that pci@1,0 is seen as "default" bus due to the range
> > > > property, thus dt_number_of_address() returns 1.
> > > >
> > > >
> > > > I can see that reg = <0 0 0 0 0> is not a problem because it is ignored
> > > > given that the parent is a PCI bus. assigned-addresses is the one that
> > > > is read.
> > > >
> > > >
> > > > But from a device tree perspective I am actually confused by the
> > > > presence of the "ranges" property under pci@1,0. Is that correct? It is
> > > > stating that addresses of children devices will be translated to the
> > > > address space of the parent (pcie0) using the parent translation rules.
> > > > I mean -- it looks like Xen is right in trying to translate reg =
> > > > <0x10000 0 0 0 0> using ranges = <0x02000000 0x0 0xc0000000 0x6
> > > > 0x00000000 0x0 0x40000000>.
> > > >
> > > > Or maybe since pcie0 is a PCI bus all the children addresses, even
> > > > grand-children, are expected to be specified using "assigned-addresses"?
> > > >
> > > >
> > > > Looking at other examples [1][2] maybe the mistake is that pci@1,0 is
> > > > missing device_type = "pci"? Of course, if I add that, the error
> > > > disappear.
> > >
> > > I am afraid, I don't know the answer. I think it would be best to ask the
> > > Linux DT folks about it.
> > >
> > > >
> > > > [1] Documentation/devicetree/bindings/pci/mvebu-pci.txt
> > > > [2] Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt
> > > >
> > > > For the sake of making Xen more resilient to possible DTSes, maybe we
> > > > should try to extend the dt_bus_pci_match check? See for instance the
> > > > change below, but we might be able to come up with better ideas.
> > > >
> > > >
> > > > diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> > > > index 18825e333e..24d998f725 100644
> > > > --- a/xen/common/device_tree.c
> > > > +++ b/xen/common/device_tree.c
> > > > @@ -565,12 +565,21 @@ static unsigned int dt_bus_default_get_flags(const
> > > > __be32 *addr)
> > > > static bool_t dt_bus_pci_match(const struct dt_device_node *np)
> > > > {
> > > > + bool ret = false;
> > > > +
> > > > /*
> > > > * "pciex" is PCI Express "vci" is for the /chaos bridge on
> > > > 1st-gen
> > > > PCI
> > > > * powermacs "ht" is hypertransport
> > > > */
> > > > - return !strcmp(np->type, "pci") || !strcmp(np->type, "pciex") ||
> > > > + ret = !strcmp(np->type, "pci") || !strcmp(np->type, "pciex") ||
> > > > !strcmp(np->type, "vci") || !strcmp(np->type, "ht");
> > > > +
> > > > + if ( ret ) return ret;
> > > > +
> > > > + if ( !strcmp(np->name, "pci") )
> > > > + ret = dt_bus_pci_match(dt_get_parent(np));
> > >
> > > It is probably safe to assume that a PCI device (not hostbridge) will
> > > start
> > > with "pci". Although, I don't much like the idea because the name is not
> > > meant
> > > to be stable.
> > >
> > > AFAICT, we can only rely on "compatible" and "type".
> > >
> > > Cheers,
> > >
> > > --
> > > Julien Grall
> > >
>
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |