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

[Julien Grall <julien@xxxxxxx>]

On 02/02/2021 18:12, Julien Grall wrote:
> Hi,
>
> On 02/02/2021 17:47, Elliott Mitchell wrote:
> > The handle_device() function has been returning failure upon
> > encountering a device address which was invalid.  A device tree which
> > had such an entry has now been seen in the wild.  As it causes no
> > failures to simply ignore the entries, ignore them. >
> > Signed-off-by: Elliott Mitchell <ehem+xenn@xxxxxxx>
> >
> > ---
> > I'm starting to suspect there are an awful lot of places in the various
> > domain_build.c files which should simply ignore errors.  This is now the
> > second place I've encountered in 2 months where ignoring errors was the
> > correct action.
> Right, as a counterpoint, we run Xen on Arm HW for several years now and 
> this is the first time I heard about issue parsing the DT. So while I 
> appreciate that you are eager to run Xen on the RPI...
> >  I know failing in case of error is an engineer's
> > favorite approach, but there seem an awful lot of harmless failures
> > causing panics.
> >
> > This started as the thread "[RFC PATCH] xen/arm: domain_build: Ignore
> > empty memory bank".  Now it seems clear the correct approach is to simply
> > ignore these entries.
> ... we first need to fully understand the issues. Here a few questions:
>     1) Can you provide more information why you believe the address is 
> invalid?
>     2) How does Linux use the node?
>     3) Is it happening with all the RPI DT? If not, what are the 
> differences?
So I had another look at the device-tree you provided earlier on. The 
node is the following (copied directly from the DTS):
&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>;
                };
        };
};

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;
};

The interpretation of "reg" depends on the context. In this case, we are 
trying to interpret as a memory address from the CPU PoV when it has a 
different meaning (I am not exactly sure what).
In fact, you are lucky that Xen doesn't manage to interpret it. Xen 
should really stop trying to look region to map when it discover a PCI 
bus. I wrote a quick hack patch that should ignore it:
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 374bf655ee34..937fd1e387b7 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1426,7 +1426,7 @@ static int __init handle_device(struct domain *d, 
struct dt_device_node *dev,
 static int __init handle_node(struct domain *d, struct kernel_info *kinfo,
                               struct dt_device_node *node,
-                              p2m_type_t p2mt)
+                              p2m_type_t p2mt, bool pci_bus)
 {
     static const struct dt_device_match skip_matches[] __initconst =
     {
@@ -1532,9 +1532,14 @@ static int __init handle_node(struct domain *d, 
struct kernel_info *kinfo,
                "WARNING: Path %s is reserved, skip the node as we may 
re-use the path.\n",
                path);

-    res = handle_device(d, node, p2mt);
-    if ( res)
-        return res;
+    if ( !pci_bus )
+    {
+        res = handle_device(d, node, p2mt);
+        if ( res)
+           return res;
+
+        pci_bus = dt_device_type_is_equal(node, "pci");
+    }

     /*
      * The property "name" is used to have a different name on older FDT
@@ -1554,7 +1559,7 @@ static int __init handle_node(struct domain *d, 
struct kernel_info *kinfo,
     for ( child = node->child; child != NULL; child = child->sibling )
     {
-        res = handle_node(d, kinfo, child, p2mt);
+        res = handle_node(d, kinfo, child, p2mt, pci_bus);
         if ( res )
             return res;
     }
@@ -2192,7 +2197,7 @@ static int __init prepare_dtb_hwdom(struct domain 
*d, struct kernel_info *kinfo)
     fdt_finish_reservemap(kinfo->fdt);

-    ret = handle_node(d, kinfo, dt_host, default_p2mt);
+    ret = handle_node(d, kinfo, dt_host, default_p2mt, false);
     if ( ret )
         goto err;

A less hackish possibility would be to modify dt_number_of_address() and 
return 0 when the device is a child of a PCI below.
Stefano, do you have any opinions?

Cheers,

--
Julien Grall