RPi4's memreserve use in fdt not handled by aarch64 fdt_get_reserved_mem (called via its initarm)

[Kyle Evans]

(Removing Jeff from CC- it's becoming clear that this isn't his problem)

On Mon, Feb 10, 2020 at 12:00 AM Mark Millard <marklmi at yahoo.com> wrote:
> On 2020-Feb-9, at 21:12, Mark Millard <marklmi at yahoo.com> wrote:
>
> > aarch64 seems to be ignoring the RPi4B's memreserve use.
> >
> > My hypothesis is that head -r356776 and later are
> > allocating RAM areas that overlap with the
> > memreserve area.
> >
> > 1st see separate submittals about the live dts text reported
> > by fdt print in u-boot on the example RPi4B 4 GiByte machine:
> >
> > https://lists.freebsd.org/pipermail/freebsd-arm/2020-February/021207.html
> > https://lists.freebsd.org/pipermail/freebsd-arm/2020-February/021205.html
> >
> > Then:
> >
> > aarch64's initarm uses fdt_get_reserved_mem and not
> > fdt_get_reserved_regions. So, looking at what that
> > implies:
> >
> > int
> > fdt_get_reserved_mem(struct mem_region *reserved, int *mreserved)
> > {
> >        pcell_t reg[FDT_REG_CELLS];
> >        phandle_t child, root;
> >        int addr_cells, size_cells;
> >        int i, rv;
> >
> >        root = OF_finddevice("/reserved-memory");
> >        if (root == -1) {
> >                return (ENXIO);
> >        }
> > . . . (I'll not list it all to show the lack of
> > memreserve handling) . . .
> >
> > This does not check for and handle memreserve.
> >
> > By contrast armv7 and armv6 each have an initarm
> > that uses fdt_get_reserved_regions and that in
> > turn has:
> >
> > int
> > fdt_get_reserved_regions(struct mem_region *mr, int *mrcnt)
> > {
> >        pcell_t reserve[FDT_REG_CELLS * FDT_MEM_REGIONS];
> >        pcell_t *reservep;
> >        phandle_t memory, root;
> >        int addr_cells, size_cells;
> >        int i, res_len, rv, tuple_size, tuples;
> >
> >        root = OF_finddevice("/");
> >        memory = OF_finddevice("/memory");
> >        if (memory == -1) {
> >                rv = ENXIO;
> >                goto out;
> >        }
> >
> >        if ((rv = fdt_addrsize_cells(OF_parent(memory), &addr_cells,
> >            &size_cells)) != 0)
> >                goto out;
> >
> >        if (addr_cells > 2) {
> >                rv = ERANGE;
> >                goto out;
> >        }
> >
> >        tuple_size = sizeof(pcell_t) * (addr_cells + size_cells);
> >
> >        res_len = OF_getproplen(root, "memreserve");
> >        if (res_len <= 0 || res_len > sizeof(reserve)) {
> >                rv = ERANGE;
> >                goto out;
> >        }
> >
> >        if (OF_getprop(root, "memreserve", reserve, res_len) <= 0) {
> >                rv = ENXIO;
> >                goto out;
> >        }
> > . . .
> >
> > So this handles memreserve but not /reserved-memory .
> >
> > It appears that for the RPi4B's the 32-bit "normal use"
> > has lead to aarch64 having memreserve instead of
> > /reserved-memory .
> >
>
> I managed to make a quick test patch for head -r356529
> but it did not make the RPi4B boot attempt behave
> differently. So, either I messed up or handling memreserve
> is not sufficient. (Some alternate information might be
> covering the address range already?)
>
> I'm not familiar with the subject matter in the code, so
> I may have messed up the test. I just used:
>
> # svnlite diff /usr/src/sys/dev/fdt/fdt_common.c
> Index: /usr/src/sys/dev/fdt/fdt_common.c
> ===================================================================
> --- /usr/src/sys/dev/fdt/fdt_common.c   (revision 357529)
> +++ /usr/src/sys/dev/fdt/fdt_common.c   (working copy)
> @@ -512,6 +512,11 @@
>
>         root = OF_finddevice("/reserved-memory");
>         if (root == -1) {
> +               // Fail over to checking for and handling memreserve,
> +               // such as for a RPi4B.
> +               if (0 == fdt_get_reserved_regions(reserved,mreserved))
> +                       return (0);
> +
>                 return (ENXIO);
>         }
>

This seems reasonable; specifically CC'ing andrew@ and manu@ to get
their opinion on the patch, as having authored and reviewed the
relevant section respectively.