From: Julien Grall <julien.grall.oss@gmail.com>
To: Stefano Stabellini <sstabellini@kernel.org>
Cc: Elliott Mitchell <ehem+xen@m5p.com>,
xen-devel <xen-devel@lists.xenproject.org>,
Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Subject: Re: [PATCH] xen/arm: domain_build: Ignore device nodes with invalid addresses
Date: Wed, 3 Feb 2021 22:52:44 +0000 [thread overview]
Message-ID: <CAJ=z9a1LsqOMFXV5GLYEkF7=akMx7fT_vpgVtT6xP6MPfmP9vQ@mail.gmail.com> (raw)
In-Reply-To: <alpine.DEB.2.21.2102031315350.29047@sstabellini-ThinkPad-T480s>
On Wed, 3 Feb 2021 at 22:18, Stefano Stabellini <sstabellini@kernel.org> 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.
Cheers,
next prev parent reply other threads:[~2021-02-03 22:53 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
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 [this message]
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='CAJ=z9a1LsqOMFXV5GLYEkF7=akMx7fT_vpgVtT6xP6MPfmP9vQ@mail.gmail.com' \
--to=julien.grall.oss@gmail.com \
--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).