[PATCH 0/4] Improve reporting of ROM blob overlap errors

[PATCH 0/4] Improve reporting of ROM blob overlap errors

[Peter Maydell]

(This was inspired by a conversation on IRC with a user.)

We report an error if we detect that there's an overlap in guest
memory between two ROM blobs, but the warning is pretty opaque.
Currently it looks like this:

rom: requested regions overlap (rom dtb. free=0x0000000000008000, addr=0x0000000000000000)
qemu-system-aarch64: rom check and register reset failed

which is pretty cryptic and also is missing information that we
could fairly easily tell the user (like the name of both the ROMs
involved in the overlap rather than just one of them...)


After this patchset it looks like:

qemu-system-aarch64: Some ROM regions are overlapping
These ROM regions might have been loaded by direct user request or by default.
They could be BIOS/firmware images, a guest kernel, initrd or some other file loaded into guest memory.
Check whether you intended to load all this guest code, and whether it has been built to load to the correct addresses.

The following two regions overlap (in the cpu-memory-0 address space):
  /home/petmay01/linaro/qemu-misc-tests/ldmia-fault.axf ELF program header segment 0 (addresses 0x0000000000000000 - 0x0000000000008000)
  dtb (addresses 0x0000000000000000 - 0x0000000000100000)

The following two regions overlap (in the cpu-memory-0 address space):
  /home/petmay01/linaro/qemu-misc-tests/bad-psci-call.axf ELF program header segment 1 (addresses 0x0000000040000000 - 0x0000000040000010)
  /home/petmay01/linaro/qemu-misc-tests/bp-test.elf ELF program header segment 0 (addresses 0x0000000040000000 - 0x0000000040000020)



We're still somewhat at the mercy of QEMU code that creates ROM blobs
to give them usefully diagnostic names (in this example, for example
"dtb" is a bit unhelpfully brief -- it's the virt board's "let me put
the autogenerated DTB at the base of RAM" rather than a DTB passed by
the user). I tweaked the names that the ELF loader uses in the last
patch of the series because that's a pretty common source of one of
the ROMs in a conflict.

thanks
-- PMM

Peter Maydell (4):
  hw/core/loader.c: Track last-seen ROM in
    rom_check_and_register_reset()
  hw/core/loader.c: Improve reporting of ROM overlap errors
  elf_ops.h: Don't truncate name of the ROM blobs we create
  elf_ops.h: Be more verbose with ROM blob names

 include/hw/elf_ops.h |  5 ++--
 hw/core/loader.c     | 67 ++++++++++++++++++++++++++++++++++++--------
 softmmu/vl.c         |  1 -
 3 files changed, 58 insertions(+), 15 deletions(-)

-- 
2.20.1

[PATCH 1/4] hw/core/loader.c: Track last-seen ROM in rom_check_and_register_reset()

[Peter Maydell]

In rom_check_and_register_reset() we detect overlaps by looking at
whether the ROM blob we're currently examining is in the same address
space and starts before the previous ROM blob ends.  (This works
because the ROM list is kept sorted in order by AddressSpace and then
by address.)

Instead of keeping the AddressSpace and last address of the previous ROM
blob in local variables, just keep a pointer to it.

This will allow us to print more useful information when we do detect
an overlap.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/core/loader.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/hw/core/loader.c b/hw/core/loader.c
index 8bbb1797a4c..05052ee797e 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -1165,28 +1165,35 @@ static void rom_reset(void *unused)
     }
 }
 
+/* Return true if two consecutive ROMs in the ROM list overlap */
+static bool roms_overlap(Rom *last_rom, Rom *this_rom)
+{
+    if (!last_rom) {
+        return false;
+    }
+    return last_rom->as == this_rom->as &&
+        last_rom->addr + last_rom->romsize > this_rom->addr;
+}
+
 int rom_check_and_register_reset(void)
 {
-    hwaddr addr = 0;
     MemoryRegionSection section;
-    Rom *rom;
-    AddressSpace *as = NULL;
+    Rom *rom, *last_rom = NULL;
 
     QTAILQ_FOREACH(rom, &roms, next) {
         if (rom->fw_file) {
             continue;
         }
         if (!rom->mr) {
-            if ((addr > rom->addr) && (as == rom->as)) {
+            if (roms_overlap(last_rom, rom)) {
                 fprintf(stderr, "rom: requested regions overlap "
                         "(rom %s. free=0x" TARGET_FMT_plx
                         ", addr=0x" TARGET_FMT_plx ")\n",
-                        rom->name, addr, rom->addr);
+                        rom->name, last_rom->addr + last_rom->romsize,
+                        rom->addr);
                 return -1;
             }
-            addr  = rom->addr;
-            addr += rom->romsize;
-            as = rom->as;
+            last_rom = rom;
         }
         section = memory_region_find(rom->mr ? rom->mr : get_system_memory(),
                                      rom->addr, 1);
-- 
2.20.1

Re: [PATCH 1/4] hw/core/loader.c: Track last-seen ROM in rom_check_and_register_reset()

[Richard Henderson]

On 11/29/20 2:39 PM, Peter Maydell wrote:
> In rom_check_and_register_reset() we detect overlaps by looking at
> whether the ROM blob we're currently examining is in the same address
> space and starts before the previous ROM blob ends.  (This works
> because the ROM list is kept sorted in order by AddressSpace and then
> by address.)
> 
> Instead of keeping the AddressSpace and last address of the previous ROM
> blob in local variables, just keep a pointer to it.
> 
> This will allow us to print more useful information when we do detect
> an overlap.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/core/loader.c | 23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 2/4] hw/core/loader.c: Improve reporting of ROM overlap errors

[Peter Maydell]

In rom_check_and_register_reset() we report to the user if there is
a "ROM region overlap". This has a couple of problems:
 * the reported information is not very easy to intepret
 * the function just prints the overlap to stderr (and relies on
   its single callsite in vl.c to do an error_report() and exit)
 * only the first overlap encountered is diagnosed

Make this function use error_report() and error_printf() and
report a more user-friendly report with all the overlaps
diagnosed.

Sample old output:

rom: requested regions overlap (rom dtb. free=0x0000000000008000, addr=0x0000000000000000)
qemu-system-aarch64: rom check and register reset failed

Sample new output:

qemu-system-aarch64: Some ROM regions are overlapping
These ROM regions might have been loaded by direct user request or by default.
They could be BIOS/firmware images, a guest kernel, initrd or some other file loaded into guest memory.
Check whether you intended to load all this guest code, and whether it has been built to load to the correct addresses.

The following two regions overlap (in the cpu-memory-0 address space):
  phdr #0: /home/petmay01/linaro/qemu-misc-tests/ldmia-fault.axf (addresses 0x0000000000000000 - 0x0000000000008000)
  dtb (addresses 0x0000000000000000 - 0x0000000000100000)

The following two regions overlap (in the cpu-memory-0 address space):
  phdr #1: /home/petmay01/linaro/qemu-misc-tests/bad-psci-call.axf (addresses 0x0000000040000000 - 0x0000000040000010)
  phdr #0: /home/petmay01/linaro/qemu-misc-tests/bp-test.elf (addresses 0x0000000040000000 - 0x0000000040000020)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
The sample output is from a completely bogus commandline where I just
loaded multiple clashing ELF files I had to hand using -device loader.
---
 hw/core/loader.c | 48 ++++++++++++++++++++++++++++++++++++++++++------
 softmmu/vl.c     |  1 -
 2 files changed, 42 insertions(+), 7 deletions(-)

diff --git a/hw/core/loader.c b/hw/core/loader.c
index 05052ee797e..de3c319e34f 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -1175,10 +1175,42 @@ static bool roms_overlap(Rom *last_rom, Rom *this_rom)
         last_rom->addr + last_rom->romsize > this_rom->addr;
 }
 
+static const char *rom_as_name(Rom *rom)
+{
+    const char *name = rom->as ? rom->as->name : NULL;
+    return name ?: "anonymous";
+}
+
+static void rom_print_overlap_error_header(void)
+{
+    error_report("Some ROM regions are overlapping");
+    error_printf(
+        "These ROM regions might have been loaded by "
+        "direct user request or by default.\n"
+        "They could be BIOS/firmware images, a guest kernel, "
+        "initrd or some other file loaded into guest memory.\n"
+        "Check whether you intended to load all this guest code, and "
+        "whether it has been built to load to the correct addresses.\n");
+}
+
+static void rom_print_one_overlap_error(Rom *last_rom, Rom *rom)
+{
+    error_printf(
+        "\nThe following two regions overlap (in the %s address space):\n",
+        rom_as_name(rom));
+    error_printf(
+        "  %s (addresses 0x" TARGET_FMT_plx " - 0x" TARGET_FMT_plx ")\n",
+        last_rom->name, last_rom->addr, last_rom->addr + last_rom->romsize);
+    error_printf(
+        "  %s (addresses 0x" TARGET_FMT_plx " - 0x" TARGET_FMT_plx ")\n",
+        rom->name, rom->addr, rom->addr + rom->romsize);
+}
+
 int rom_check_and_register_reset(void)
 {
     MemoryRegionSection section;
     Rom *rom, *last_rom = NULL;
+    bool found_overlap = false;
 
     QTAILQ_FOREACH(rom, &roms, next) {
         if (rom->fw_file) {
@@ -1186,12 +1218,12 @@ int rom_check_and_register_reset(void)
         }
         if (!rom->mr) {
             if (roms_overlap(last_rom, rom)) {
-                fprintf(stderr, "rom: requested regions overlap "
-                        "(rom %s. free=0x" TARGET_FMT_plx
-                        ", addr=0x" TARGET_FMT_plx ")\n",
-                        rom->name, last_rom->addr + last_rom->romsize,
-                        rom->addr);
-                return -1;
+                if (!found_overlap) {
+                    found_overlap = true;
+                    rom_print_overlap_error_header();
+                }
+                rom_print_one_overlap_error(last_rom, rom);
+                /* Keep going through the list so we report all overlaps */
             }
             last_rom = rom;
         }
@@ -1200,6 +1232,10 @@ int rom_check_and_register_reset(void)
         rom->isrom = int128_nz(section.size) && memory_region_is_rom(section.mr);
         memory_region_unref(section.mr);
     }
+    if (found_overlap) {
+        return -1;
+    }
+
     qemu_register_reset(rom_reset, NULL);
     roms_loaded = 1;
     return 0;
diff --git a/softmmu/vl.c b/softmmu/vl.c
index e6e0ad5a925..aa6fd3243d5 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -4459,7 +4459,6 @@ void qemu_init(int argc, char **argv, char **envp)
     qemu_run_machine_init_done_notifiers();
 
     if (rom_check_and_register_reset() != 0) {
-        error_report("rom check and register reset failed");
         exit(1);
     }
 
-- 
2.20.1

Re: [PATCH 2/4] hw/core/loader.c: Improve reporting of ROM overlap errors

[Richard Henderson]

On 11/29/20 2:39 PM, Peter Maydell wrote:
> In rom_check_and_register_reset() we report to the user if there is
> a "ROM region overlap". This has a couple of problems:
>  * the reported information is not very easy to intepret
>  * the function just prints the overlap to stderr (and relies on
>    its single callsite in vl.c to do an error_report() and exit)
>  * only the first overlap encountered is diagnosed
> 
> Make this function use error_report() and error_printf() and
> report a more user-friendly report with all the overlaps
> diagnosed.
> 
> Sample old output:
> 
> rom: requested regions overlap (rom dtb. free=0x0000000000008000, addr=0x0000000000000000)
> qemu-system-aarch64: rom check and register reset failed
> 
> Sample new output:
> 
> qemu-system-aarch64: Some ROM regions are overlapping
> These ROM regions might have been loaded by direct user request or by default.
> They could be BIOS/firmware images, a guest kernel, initrd or some other file loaded into guest memory.
> Check whether you intended to load all this guest code, and whether it has been built to load to the correct addresses.
> 
> The following two regions overlap (in the cpu-memory-0 address space):
>   phdr #0: /home/petmay01/linaro/qemu-misc-tests/ldmia-fault.axf (addresses 0x0000000000000000 - 0x0000000000008000)
>   dtb (addresses 0x0000000000000000 - 0x0000000000100000)
> 
> The following two regions overlap (in the cpu-memory-0 address space):
>   phdr #1: /home/petmay01/linaro/qemu-misc-tests/bad-psci-call.axf (addresses 0x0000000040000000 - 0x0000000040000010)
>   phdr #0: /home/petmay01/linaro/qemu-misc-tests/bp-test.elf (addresses 0x0000000040000000 - 0x0000000040000020)
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 3/4] elf_ops.h: Don't truncate name of the ROM blobs we create

[Peter Maydell]

Currently the load_elf code assembles the ROM blob name into a
local 128 byte fixed-size array. Use g_strdup_printf() instead so
that we don't truncate the pathname if it happens to be long.
(This matters mostly for monitor 'info roms' output and for the
error messages if ROM blobs overlap.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/elf_ops.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
index 6fdff3dced5..53e0152af53 100644
--- a/include/hw/elf_ops.h
+++ b/include/hw/elf_ops.h
@@ -330,7 +330,6 @@ static int glue(load_elf, SZ)(const char *name, int fd,
     uint64_t addr, low = (uint64_t)-1, high = 0;
     GMappedFile *mapped_file = NULL;
     uint8_t *data = NULL;
-    char label[128];
     int ret = ELF_LOAD_FAILED;
 
     if (read(fd, &ehdr, sizeof(ehdr)) != sizeof(ehdr))
@@ -544,7 +543,8 @@ static int glue(load_elf, SZ)(const char *name, int fd,
              */
             if (mem_size != 0) {
                 if (load_rom) {
-                    snprintf(label, sizeof(label), "phdr #%d: %s", i, name);
+                    g_autofree char *label =
+                        g_strdup_printf("phdr #%d: %s", i, name);
 
                     /*
                      * rom_add_elf_program() takes its own reference to
-- 
2.20.1

Re: [PATCH 3/4] elf_ops.h: Don't truncate name of the ROM blobs we create

[Richard Henderson]

On 11/29/20 2:39 PM, Peter Maydell wrote:
> Currently the load_elf code assembles the ROM blob name into a
> local 128 byte fixed-size array. Use g_strdup_printf() instead so
> that we don't truncate the pathname if it happens to be long.
> (This matters mostly for monitor 'info roms' output and for the
> error messages if ROM blobs overlap.)
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  include/hw/elf_ops.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 4/4] elf_ops.h: Be more verbose with ROM blob names

[Peter Maydell]

Instead of making the ROM blob name something like:
  phdr #0: /home/petmay01/linaro/qemu-misc-tests/ldmia-fault.axf
make it a little more self-explanatory for people who don't know
ELF format details:
  /home/petmay01/linaro/qemu-misc-tests/ldmia-fault.axf ELF program header segment 0

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
This seems nicer to me, but it's a matter of taste, so if people
prefer the current name form we should probably leave it be.
---
 include/hw/elf_ops.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
index 53e0152af53..8e8436831d2 100644
--- a/include/hw/elf_ops.h
+++ b/include/hw/elf_ops.h
@@ -544,7 +544,8 @@ static int glue(load_elf, SZ)(const char *name, int fd,
             if (mem_size != 0) {
                 if (load_rom) {
                     g_autofree char *label =
-                        g_strdup_printf("phdr #%d: %s", i, name);
+                        g_strdup_printf("%s ELF program header segment %d",
+                                        name, i);
 
                     /*
                      * rom_add_elf_program() takes its own reference to
-- 
2.20.1

Re: [PATCH 4/4] elf_ops.h: Be more verbose with ROM blob names

[Richard Henderson]

On 11/29/20 2:39 PM, Peter Maydell wrote:
> Instead of making the ROM blob name something like:
>   phdr #0: /home/petmay01/linaro/qemu-misc-tests/ldmia-fault.axf
> make it a little more self-explanatory for people who don't know
> ELF format details:
>   /home/petmay01/linaro/qemu-misc-tests/ldmia-fault.axf ELF program header segment 0
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 0/4] Improve reporting of ROM blob overlap errors

[Peter Maydell]

On Sun, 29 Nov 2020 at 20:39, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> (This was inspired by a conversation on IRC with a user.)
>
> We report an error if we detect that there's an overlap in guest
> memory between two ROM blobs, but the warning is pretty opaque.
> Currently it looks like this:
>
> rom: requested regions overlap (rom dtb. free=0x0000000000008000, addr=0x0000000000000000)
> qemu-system-aarch64: rom check and register reset failed
>
> which is pretty cryptic and also is missing information that we
> could fairly easily tell the user (like the name of both the ROMs
> involved in the overlap rather than just one of them...)
>
>
> After this patchset it looks like:
>
> qemu-system-aarch64: Some ROM regions are overlapping
> These ROM regions might have been loaded by direct user request or by default.
> They could be BIOS/firmware images, a guest kernel, initrd or some other file loaded into guest memory.
> Check whether you intended to load all this guest code, and whether it has been built to load to the correct addresses.
>
> The following two regions overlap (in the cpu-memory-0 address space):
>   /home/petmay01/linaro/qemu-misc-tests/ldmia-fault.axf ELF program header segment 0 (addresses 0x0000000000000000 - 0x0000000000008000)
>   dtb (addresses 0x0000000000000000 - 0x0000000000100000)
>
> The following two regions overlap (in the cpu-memory-0 address space):
>   /home/petmay01/linaro/qemu-misc-tests/bad-psci-call.axf ELF program header segment 1 (addresses 0x0000000040000000 - 0x0000000040000010)
>   /home/petmay01/linaro/qemu-misc-tests/bp-test.elf ELF program header segment 0 (addresses 0x0000000040000000 - 0x0000000040000020)
>

I'm going to take this series via target-arm.next.


thanks
-- PMM