Re: [PATCH] xen/arm: domain_build: Ignore device nodes with invalid addresses

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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.

[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));
+
+    return ret;
 }
 
 static void dt_bus_pci_count_cells(const struct dt_device_node *np,