xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
From: Julien Grall <julien@xen.org>
To: Stefano Stabellini <sstabellini@kernel.org>,
	Julien Grall <julien.grall.oss@gmail.com>
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: Thu, 4 Feb 2021 16:29:41 +0000	[thread overview]
Message-ID: <9b97789b-5560-0186-642a-0501789830e5@xen.org> (raw)
In-Reply-To: <alpine.DEB.2.21.2102031519540.29047@sstabellini-ThinkPad-T480s>



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


  reply	other threads:[~2021-02-04 16:30 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
2021-02-04  0:13             ` Stefano Stabellini
2021-02-04 16:29               ` Julien Grall [this message]
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=9b97789b-5560-0186-642a-0501789830e5@xen.org \
    --to=julien@xen.org \
    --cc=Volodymyr_Babchuk@epam.com \
    --cc=ehem+xen@m5p.com \
    --cc=julien.grall.oss@gmail.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).