[Qemu-devel] [PATCH] target/mips: Add initrd support for Boston board

[Qemu-devel] [PATCH] target/mips: Add initrd support for Boston board

[Aleksandar Markovic]

From: Aleksandar Rikalo <arikalo@wavecomp.com>

Add support for initial ramdisk loading for the Mips Boston board.

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: Aleksandar Rikalo <arikalo@wavecomp.com>
Signed-off-by: Aleksandar Markovic <amarkovic@wavecomp.com>
---
v2->v3:
  - a comment was reformatted
  - rebased to the latest QEMU code
v1->v2:
  - 'long inird_size' is changed to 'target_ulong initrd_size',
    as it should be
  - error_report() is used instead of fprintf()
---
 hw/mips/boston.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 46 insertions(+), 9 deletions(-)

diff --git a/hw/mips/boston.c b/hw/mips/boston.c
index 6c9c20a..788bf69 100644
--- a/hw/mips/boston.c
+++ b/hw/mips/boston.c
@@ -31,6 +31,7 @@
 #include "hw/loader-fit.h"
 #include "hw/mips/cps.h"
 #include "hw/mips/cpudevs.h"
+#include "hw/mips/mips.h"
 #include "hw/pci-host/xilinx-pcie.h"
 #include "qapi/error.h"
 #include "qemu/error-report.h"
@@ -333,10 +334,12 @@ static const void *boston_fdt_filter(void *opaque, const void *fdt_orig,
 {
     BostonState *s = BOSTON(opaque);
     MachineState *machine = s->mach;
-    const char *cmdline;
+    GString *cmdline;
     int err;
     void *fdt;
     size_t fdt_sz, ram_low_sz, ram_high_sz;
+    target_ulong initrd_size;
+    ram_addr_t initrd_offset;
 
     fdt_sz = fdt_totalsize(fdt_orig) * 2;
     fdt = g_malloc0(fdt_sz);
@@ -347,20 +350,54 @@ static const void *boston_fdt_filter(void *opaque, const void *fdt_orig,
         return NULL;
     }
 
-    cmdline = (machine->kernel_cmdline && machine->kernel_cmdline[0])
-            ? machine->kernel_cmdline : " ";
-    err = qemu_fdt_setprop_string(fdt, "/chosen", "bootargs", cmdline);
-    if (err < 0) {
-        fprintf(stderr, "couldn't set /chosen/bootargs\n");
-        return NULL;
-    }
-
     ram_low_sz = MIN(256 * MiB, machine->ram_size);
     ram_high_sz = machine->ram_size - ram_low_sz;
     qemu_fdt_setprop_sized_cells(fdt, "/memory@0", "reg",
                                  1, 0x00000000, 1, ram_low_sz,
                                  1, 0x90000000, 1, ram_high_sz);
 
+    cmdline = g_string_new(machine->kernel_cmdline);
+
+    /* load initrd */
+    initrd_offset = 0;
+    if (machine->initrd_filename) {
+        initrd_size = get_image_size(machine->initrd_filename);
+        if (initrd_size != (target_ulong) -1) {
+            /*
+             * The kernel allocates the bootmap memory in the low memory after
+             * the initrd. It takes at most 128kiB for 2GB RAM and 4kiB pages.
+             */
+            initrd_offset = (ram_low_sz - initrd_size - 131072
+                             - ~INITRD_PAGE_MASK) & INITRD_PAGE_MASK;
+
+            if ((int64_t)cpu_mips_kseg0_to_phys(NULL, *load_addr + fdt_sz)
+                >= (int64_t)initrd_offset) {
+                error_report("memory too small for initial ram disk '%s'",
+                             machine->initrd_filename);
+                exit(1);
+            }
+
+            initrd_size = load_image_targphys(machine->initrd_filename,
+                                              initrd_offset,
+                                              initrd_size);
+        }
+        if (initrd_size == (target_ulong) -1) {
+            error_report("could not load initial ram disk '%s'",
+                         machine->initrd_filename);
+            exit(1);
+        }
+        g_string_append_printf(cmdline, " rd_start=0x%" PRIx64 " rd_size=%li",
+                               cpu_mips_phys_to_kseg0(NULL, initrd_offset),
+                               initrd_size);
+    }
+
+    err = qemu_fdt_setprop_string(fdt, "/chosen", "bootargs", cmdline->str);
+    g_string_free(cmdline, true);
+    if (err < 0) {
+        error_report("couldn't set /chosen/bootargs");
+        exit(1);
+    }
+
     fdt = g_realloc(fdt, fdt_totalsize(fdt));
     qemu_fdt_dumpdtb(fdt, fdt_sz);
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH] target/mips: Add initrd support for Boston board

[Paul Burton]

Hi Aleksandar,

On Tue, Oct 23, 2018 at 03:12:14PM +0200, Aleksandar Markovic wrote:
> From: Aleksandar Rikalo <arikalo@wavecomp.com>
> 
> Add support for initial ramdisk loading for the Mips Boston board.
> 
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Signed-off-by: Aleksandar Rikalo <arikalo@wavecomp.com>
> Signed-off-by: Aleksandar Markovic <amarkovic@wavecomp.com>
> ---
> v2->v3:
>   - a comment was reformatted
>   - rebased to the latest QEMU code
> v1->v2:
>   - 'long inird_size' is changed to 'target_ulong initrd_size',
>     as it should be
>   - error_report() is used instead of fprintf()
> ---
>  hw/mips/boston.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 46 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/mips/boston.c b/hw/mips/boston.c
> index 6c9c20a..788bf69 100644
> --- a/hw/mips/boston.c
> +++ b/hw/mips/boston.c
> @@ -31,6 +31,7 @@
>  #include "hw/loader-fit.h"
>  #include "hw/mips/cps.h"
>  #include "hw/mips/cpudevs.h"
> +#include "hw/mips/mips.h"
>  #include "hw/pci-host/xilinx-pcie.h"
>  #include "qapi/error.h"
>  #include "qemu/error-report.h"
> @@ -333,10 +334,12 @@ static const void *boston_fdt_filter(void *opaque, const void *fdt_orig,
>  {
>      BostonState *s = BOSTON(opaque);
>      MachineState *machine = s->mach;
> -    const char *cmdline;
> +    GString *cmdline;
>      int err;
>      void *fdt;
>      size_t fdt_sz, ram_low_sz, ram_high_sz;
> +    target_ulong initrd_size;
> +    ram_addr_t initrd_offset;
>  
>      fdt_sz = fdt_totalsize(fdt_orig) * 2;
>      fdt = g_malloc0(fdt_sz);
> @@ -347,20 +350,54 @@ static const void *boston_fdt_filter(void *opaque, const void *fdt_orig,
>          return NULL;
>      }
>  
> -    cmdline = (machine->kernel_cmdline && machine->kernel_cmdline[0])
> -            ? machine->kernel_cmdline : " ";

Just a bit of background (which I probably ought to have written as a
comment here): the string consisting of a single space character there
is a workaround for a Linux bug wherein if the DT doesn't contain a
bootargs property on its /chosen node, or it does but it's of length
zero, then any kernel command line arguments that are built-in using
CONFIG_CMDLINE get duplicated. That is, the final command line ends up
being CONFIG_CMDLINE concatenated with itself (with a space character in
the middle).

That can be problematic for some arguments, including the earlycon
argument which is commonly part of CONFIG_CMDLINE - when given twice the
kernel tries to register the same early console twice & shows a scary
warning about it.

This problem should be fixed from Linux v4.16 onwards by commit
8ce355cf2e38 ("MIPS: Setup boot_command_line before plat_mem_setup") and
later commit 951d223c6c16 ("MIPS: Fix CONFIG_CMDLINE handling").

> -    err = qemu_fdt_setprop_string(fdt, "/chosen", "bootargs", cmdline);
> -    if (err < 0) {
> -        fprintf(stderr, "couldn't set /chosen/bootargs\n");
> -        return NULL;
> -    }
> -
>      ram_low_sz = MIN(256 * MiB, machine->ram_size);
>      ram_high_sz = machine->ram_size - ram_low_sz;
>      qemu_fdt_setprop_sized_cells(fdt, "/memory@0", "reg",
>                                   1, 0x00000000, 1, ram_low_sz,
>                                   1, 0x90000000, 1, ram_high_sz);
>  
> +    cmdline = g_string_new(machine->kernel_cmdline);

As such, I suspect this may be problematic for Linux earlier than v4.16
since it appears to remove that workaround. If you want to remove the
workaround anyway then fair enough, but it seems a little too soon to
me.

> +
> +    /* load initrd */
> +    initrd_offset = 0;
> +    if (machine->initrd_filename) {
> +        initrd_size = get_image_size(machine->initrd_filename);
> +        if (initrd_size != (target_ulong) -1) {
> +            /*
> +             * The kernel allocates the bootmap memory in the low memory after
> +             * the initrd. It takes at most 128kiB for 2GB RAM and 4kiB pages.
> +             */

Just FYI this may have changed from Linux v4.20 where we switch from the
old bootmem allocator to memblock. But if the kernel tries to use memory
containing the initrd then I'd say that's a kernel bug anyway & we
should fix it there. So catering for the old bootmem case probably makes
most sense, to deal with pre-v4.20 kernels.

There's also conceptually no reason Boston has to be limited to 2GB RAM
- it could in theory have more.

> +            initrd_offset = (ram_low_sz - initrd_size - 131072
> +                             - ~INITRD_PAGE_MASK) & INITRD_PAGE_MASK;

As such I wonder if the magic number 131072 would be best changed to
something like this:

  uint32_t min_page = 4 * KiB;

  machine->ram_size / min_page / BITS_PER_BYTE

Although the result of that for 2GB RAM is 64KB for the bitmap - if it
really should be 128KB then it'd probably be worth commenting why &
changing the arithmetic accordingly.

Or given that we constrain Boston to 2GB RAM at the moment anyway
perhaps it'd be better to just go with the hardcoded number & deal with
it if & when we remove the 2GB limitation.

> +
> +            if ((int64_t)cpu_mips_kseg0_to_phys(NULL, *load_addr + fdt_sz)
> +                >= (int64_t)initrd_offset) {
> +                error_report("memory too small for initial ram disk '%s'",
> +                             machine->initrd_filename);
> +                exit(1);
> +            }
> +
> +            initrd_size = load_image_targphys(machine->initrd_filename,
> +                                              initrd_offset,
> +                                              initrd_size);
> +        }
> +        if (initrd_size == (target_ulong) -1) {
> +            error_report("could not load initial ram disk '%s'",
> +                         machine->initrd_filename);
> +            exit(1);
> +        }
> +        g_string_append_printf(cmdline, " rd_start=0x%" PRIx64 " rd_size=%li",
> +                               cpu_mips_phys_to_kseg0(NULL, initrd_offset),
> +                               initrd_size);
> +    }
> +
> +    err = qemu_fdt_setprop_string(fdt, "/chosen", "bootargs", cmdline->str);
> +    g_string_free(cmdline, true);
> +    if (err < 0) {
> +        error_report("couldn't set /chosen/bootargs");
> +        exit(1);
> +    }
> +
>      fdt = g_realloc(fdt, fdt_totalsize(fdt));
>      qemu_fdt_dumpdtb(fdt, fdt_sz);
>  
> -- 
> 2.7.4

Apart from those comments, this looks good to me.

Thanks,
    Paul