xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
From: Julien Grall <julien@xen.org>
To: Elliott Mitchell <ehem+xen@m5p.com>, xen-devel@lists.xenproject.org
Cc: Stefano Stabellini <sstabellini@kernel.org>,
	Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Subject: Re: [PATCH] xen/arm: domain_build: Ignore device nodes with invalid addresses
Date: Tue, 2 Feb 2021 19:21:14 +0000	[thread overview]
Message-ID: <b6d342f8-c833-db88-9808-cdc946999300@xen.org> (raw)
In-Reply-To: <a422c04c-f908-6fb6-f2de-fea7b18a6e7d@xen.org>



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@m5p.com>
>>
>> ---
>> 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


  reply	other threads:[~2021-02-02 19:21 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-02 17:47 [PATCH] xen/arm: domain_build: Ignore device nodes with invalid addresses Elliott Mitchell
2021-02-02 18:12 ` Julien Grall
2021-02-02 19:21   ` Julien Grall [this message]
2021-02-03  0:18     ` Stefano Stabellini
2021-02-03 17:44       ` Julien Grall
2021-02-03 22:18         ` Stefano Stabellini
2021-02-03 22:52           ` Julien Grall
2021-02-04  0:13             ` Stefano Stabellini
2021-02-04 16:29               ` Julien Grall
2021-02-04 17:56                 ` Question on PCIe Device Tree bindings, Was: " Stefano Stabellini
2021-02-04 18:31                   ` Rob Herring
2021-02-04 20:36                     ` Stefano Stabellini
2021-02-04 21:08                       ` Rob Herring
2021-02-04 21:33                         ` Stefano Stabellini
2021-02-04 21:52                           ` Rob Herring
2021-02-04 22:02                             ` Stefano Stabellini
2021-02-05  3:32                             ` Elliott Mitchell
2021-02-05 13:56                               ` Rob Herring
2021-02-04 22:39                 ` Stefano Stabellini
2021-02-06  8:47                   ` Julien Grall

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=b6d342f8-c833-db88-9808-cdc946999300@xen.org \
    --to=julien@xen.org \
    --cc=Volodymyr_Babchuk@epam.com \
    --cc=ehem+xen@m5p.com \
    --cc=sstabellini@kernel.org \
    --cc=xen-devel@lists.xenproject.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).