Re: DT with memory bank of size 0 (WAS: Re: AW: AW: Colibri imx8qxp: Missing kernel boot module)

[Julien Grall <julien@xxxxxxx>]

Hi Andre,

On 17/09/2020 10:46, André Przywara wrote:
> On 17/09/2020 01:31, Stefano Stabellini wrote:
>
> Hi,
>
> > On Wed, 16 Sep 2020, Julien Grall wrote:
> > > On 14/09/2020 15:26, Daniel Wagner2 wrote:
> > > > Hi Julien,
> > > Hi Daniel,
> > >
> > > I am moving the thread to xen-devel and adding a couple of more folks.
> > >
> > > > > > this is the full version of the fdt that threw the error:
> > > > > > https://pastebin.com/63TZ9z3k
> > > > > > The problematic memory node appears in line 126
> > > > > Thanks! The output looks corrupted as some of the lines are not valid DTB:
> > > > >
> > > > > fsl,pins = * 0x000000009300184c [0x00000048];
> > > > >
> > > > > Although, I am not sure if it is just U-boot dumping the DTB differently.
> > > > >
> > > > > Anyway, after removing the "corrupted" line, I managed to get a compile
> > > > the
> > > > > DTB. I don't have a Colibri IMX8QXP. However, given this is an early
> > > > parsing
> > > > > error, I have just embed the DTB in Xen binary via CONFIG_DTB_FILE.
> > > > >
> > > > > Unfortunately I couldn't reproduce your error. This either suggests the
> > > > DTB gets
> > > > > corrupted or Xen doesn't access the DTB with the correct memory attribute.
> > > > >
> > > > > Do you have the DTB in hand?
> > > > Sorry for the corrupted version, I've uploaded the DTB
> > > > (fsl-imx8qxp-colibri-eval-v3.dtb) to
> > > > https://drive.google.com/drive/folders/1jbpnz35sC0NbCyEjrkLqelBsKBztW1S6?usp
> > > > =sharing
> > > >
> > > > I have also uploaded my modified xen source files.
> > > > 1. arch/arm/bootfdt.c
> > > > where I have added the additional printk's seen in the log and
> > > > 2. arch/arm/setup.c
> > > > where I rerun the devicetree parser in line 935 to get the logs, since the
> > > > console is not yet initialised when the function is called for the first
> > > > time and I
> > > > didn't manage to enable earlyprintk.
> > > >
> > > > I think the breaking point is the second memory bank which appears in the
> > > > logs (see the output line marked with "!!")  with start=0x8 8000 0000 and
> > > > size=0.
> > > > It isn't specified in the DTB, so I am not sure where this comes from.
> > > > It has size=0 so
> > > > if ( !size )
> > > >       {
> > > >           printk("invalid size, bank %d\n",i);
> > > >           return -EINVAL;
> > > >       }
> > > > In bootfdt.c makes the function stop.
> > > >
> > > > Log:
> > > > (XEN) arch/arm/bootfdt.c: early_scan_node
> > > > (XEN) -> fdt: node `memory@80000000': parsing
> > > > (XEN) -> process_memory_node
> > > > (XEN)
> > > > (XEN) arch/arm/bootfdt.c: process_memory_node
> > > > (XEN) ->found memory:reg
> > > > (XEN) ->cell=
> > > > (XEN) ->banks=2
> > > > (XEN) ->mem->nr_banks=1
> > > > (XEN) ->NR_MEM_BANKS=128
> > > > (XEN) ->start=0x80200000 size=0x7fe00000
> > > > !! (XEN) ->start=0x880000000 size=0
> > > > (XEN) invalid size, bank 1
> > > > (XEN) END of arch/arm/bootfdt.c: process_memory_node
> > > When I tried to run it on the model I get:
> > >
> > > (XEN) device_tree_for_each_node: memory@80000000
> > > (XEN)
> > > (XEN) arch/arm/bootfdt.c: early_scan_node
> > > (XEN) -> fdt: node `memory@80000000': parsing
> > > (XEN) -> process_memory_node
> > > (XEN)
> > > (XEN) arch/arm/bootfdt.c: process_memory_node
> > > (XEN) ->found memory:reg
> > > (XEN) ->cell=
> > > (XEN) ->banks=1
> > > (XEN) ->mem->nr_banks=0
> > > (XEN) ->NR_MEM_BANKS=128
> > > (XEN) ->start=0x80000000 size=0x40000000
> > > (XEN) END of arch/arm/bootfdt.c: process_memory_node
> > >
> > > > Btw 8_8000_0000 is the start address of this systems DDR Main memory,
> > > > according to the Reference Manual of the i.MX8QXP.
> > > I couldn't find this value in the DT. It is possible that U-boot is modifying
> > > the memory node before jumping to Xen (or Linux).
> U-Boot almost always rewrites the memory node, filling in its own view
> of DRAM. It's actually not that easy to avoid that.
> This is in U-Boot's colibri-imx8x.h:
> #define CONFIG_SYS_SDRAM_BASE           0x80000000
> #define PHYS_SDRAM_1                    0x80000000
> #define PHYS_SDRAM_2                    0x880000000
> #define PHYS_SDRAM_1_SIZE               SZ_2G           /* 2 GB */
> #define PHYS_SDRAM_2_SIZE               0x00000000      /* 0 GB */
>
> The Colibri iMX8X SoM seems to support "Up to 2GB LPDDR4", so not sure
> why we have the upper memory here at all. It could just set
> CONFIG_NR_DRAM_BANKS to 1 and be done. But anyway ...
>
> > > Looking at Linux, they seem to ignore any bank with size == 0. I am starting
> > > to wonder whether your DT is (ab)using it.
> > >
> > > Do you have Linux running on baremetal on this board? If so would you mind to
> > > dump the DT from the userspace (via /proc/device-tree) this time?
> > >
> > > In any case, we may want to relax the check in Xen. Any opinions?
> > Yeah, ignoring a bank with size=0 is fine. I checked the epapr and it
> Not sure you meant this, but the official DT spec replaced ePAPR for a
> while now: https://www.devicetree.org/specifications/
> It has been heavily updated and even got some bug fixes.
>
> > doesn't specify that size=0 is invalid, so I think it is actually better
> > to ignore it and continue even from a spec perspective.
> Yeah, I don't see any harm as well with ignoring 0 sized banks.
Great! I have sent a patch that should solve this.

> Just not sure if that has any implication with the number of memory
> banks that Xen wants to deal with (it if counts two, but there is only
> one valid one).
It should not have any impact as we store in memory our representation 
of the memory.
Cheers,

--
Julien Grall