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

[Julien Grall <julien@xen.org>]

On 03/02/2021 00:18, Stefano Stabellini wrote:
> On Tue, 2 Feb 2021, Julien Grall wrote:
>> On 02/02/2021 18:12, Julien Grall wrote:
>>> 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:
> 
> Yes, I think you are right. There are a few instances where "reg" is not
> a address ready to be remapped. It is not just PCI, although that's the
> most common.  Maybe we need a list, like skip_matches in handle_node.

 From my understanding, "reg" can be considered as an MMIO region only 
if all the *parents up to the root have the property "ranges" and they 
are not on a different bus (e.g. pci).

Do you have example where this is not the case?

Whether Xen does it correctly is another question :).

> 
> 
>> 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?
> 
> Would PCIe even work today? Because if it doesn't, we could just add it
> to skip_matches until we get PCI passthrough properly supported.
PCIe (or PCI) definitely works in dom0 today but Xen is not aware of the 
hostbridge. So you would break quite a few uses cases by skipping the nodes.

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

Cheers,

-- 
Julien Grall