Re: [PATCH v3 00/16] hw/arm/raspi: Add thermal/timer, improve address space, run U-boot

[Philippe Mathieu-Daudé]

On 10/24/19 5:31 PM, Peter Maydell wrote:
> On Thu, 24 Oct 2019 at 14:42, Peter Maydell <address@hidden> wrote:
> > On Sun, 20 Oct 2019 at 00:47, Philippe Mathieu-Daudé <address@hidden> wrote:
> > > Since v2:
> > > - fixed issue in videocore address space
> > > - allow to start with some cores OFF (to boot firmwares)
> > > - add proof-of-concept test for '-smp cores=1' and U-boot
> > > - fixed my email setup
> > >
> > > Previous cover:
> > >
> > > Hi,
> > >
> > > Some patches from v1 are already merged. This v2 addresses the
> > > review comment from v1, and add patches to clean the memory
> > > space when using multiple cores.
> > >
> > > Laurent, if you test U-Boot with this patchset again, do you mind
> > > replying with a "Tested-by:" tag?
> > >
> > > The next patchset is probably about the interrupt controller blocks,
> > > then will come another one about the MBox/Properties.
> > >
> > > The last patch is unrelated to the series, but since I cleaned this
> > > for the raspi and the highbank is the only board with the same issue,
> > > I included the patch in this series.
> >
> > I'm going to apply 1-10 and 14 to target-arm.next.
> > (I've reviewed 10, and the rest have been reviewed.)
>
> ...but that causes tests/boot-serial-test to throw
> a clang sanitizer error and then hang:
>
> e104462:bionic:clang$ QTEST_QEMU_BINARY=arm-softmmu/qemu-system-arm
> ./tests/boot-serial-test
> /arm/boot-serial/raspi2:
> /home/petmay01/linaro/qemu-from-laptop/qemu/memory.c:2517:27: runtime
> error: null pointer passed as argument 2, which is declared to never
> be null
> /usr/include/stdlib.h:819:6: note: nonnull attribute specified here
>
> The offending patch is "hw/arm/bcm2836: Use per CPU address spaces"
> (patch 7). So I'm dropping 7/8/9.

With -bios, raspi.c::setup_boot() we call
 -> load_image_targphys[_as]
    -> rom_add_file_fixed_as
       -> rom_add_file with mr=NULL, as=set

vl.c::main() call
 -> rom_check_and_register_reset

        if (!rom->mr) {
            as = rom->as;
        }
        section = memory_region_find(rom->mr
                                     ? rom->mr
                                     : get_system_memory(),
                                     rom->addr, 1);

In my patches I stop using system_memory, each CPU use its
own AS view on the GPU AXI bus.

Apparently we can not (yet) live without a system_memory bus.

At this point I can use the RAM memory region because
setup_boot() is called from raspi_init().

What is odd is load_image_targphys[_as]() get a 'addr' argument
(as an offset within the address space) but load_image_mr() don't
take offset, only loads at base (offset=0). Neither it takes a
'max_sz' argument.

This snippet fixed the issue:

-- >8 --
diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c
index 569d85c11a..eb84f74dc7 100644
--- a/hw/arm/raspi.c
+++ b/hw/arm/raspi.c
@@ -111,7 +111,8 @@ static void reset_secondary(ARMCPU *cpu, const 
struct arm_boot_info *info)
     cpu_set_pc(cs, info->smp_loader_start);
 }

-static void setup_boot(MachineState *machine, int version, size_t ram_size)
+static void setup_boot(MachineState *machine, int version,
+                       MemoryRegion *ram, size_t ram_size)
 {
     static struct arm_boot_info binfo;
     int r;
@@ -149,9 +150,9 @@ static void setup_boot(MachineState *machine, int 
version, size_t ram_size)
      */
     if (machine->firmware) {
         hwaddr firmware_addr = version == 3 ? FIRMWARE_ADDR_3 : 
FIRMWARE_ADDR_2;
+
         /* load the firmware image (typically kernel.img) */
-        r = load_image_targphys(machine->firmware, firmware_addr,
-                                ram_size - firmware_addr);
+        r = load_image_mr(machine->firmware, firmware_addr, ram);
         if (r < 0) {
             error_report("Failed to load firmware from %s", 
machine->firmware);
             exit(1);
@@ -211,7 +212,7 @@ static void raspi_init(MachineState *machine, int 
version)

     vcram_size = object_property_get_uint(OBJECT(&s->soc), "vcram-size",
                                           &error_abort);
-    setup_boot(machine, version, machine->ram_size - vcram_size);
+    setup_boot(machine, version, &s->ram, machine->ram_size - vcram_size);
 }

 static void raspi2_init(MachineState *machine)
diff --git a/hw/core/loader.c b/hw/core/loader.c
index a3f5333258..0f11d1104a 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -137,7 +137,7 @@ int load_image_targphys_as(const char *filename,
     return size;
 }

-int load_image_mr(const char *filename, MemoryRegion *mr)
+int load_image_mr(const char *filename, hwaddr addr, MemoryRegion *mr)
 {
     int size;

@@ -152,7 +152,7 @@ int load_image_mr(const char *filename, MemoryRegion 
*mr)
         return -1;
     }
     if (size > 0) {
-        if (rom_add_file_mr(filename, mr, -1) < 0) {
+        if (rom_add_file_mr(filename, addr, mr, -1) < 0) {
             return -1;
         }
     }
diff --git a/include/hw/loader.h b/include/hw/loader.h
index 48a96cd559..9cb47707de 100644
--- a/include/hw/loader.h
+++ b/include/hw/loader.h
@@ -65,6 +65,7 @@ int load_image_targphys(const char *filename, hwaddr,
 /**
  * load_image_mr: load an image into a memory region
  * @filename: Path to the image file
+ * @addr: Address to load the image to (relative to the memory region)
  * @mr: Memory Region to load into
  *
  * Load the specified file into the memory region.
@@ -73,7 +74,7 @@ int load_image_targphys(const char *filename, hwaddr,
  * If the file is larger than the memory region's size the call will fail.
  * Returns -1 on failure, or the size of the file.
  */
-int load_image_mr(const char *filename, MemoryRegion *mr);
+int load_image_mr(const char *filename, hwaddr addr, MemoryRegion *mr);

 /* This is the limit on the maximum uncompressed image size that
  * load_image_gzipped_buffer() and load_image_gzipped() will read. It 
prevents
@@ -293,8 +294,8 @@ void hmp_info_roms(Monitor *mon, const QDict *qdict);
     rom_add_file(_f, NULL, _a, _i, false, NULL, NULL)
 #define rom_add_blob_fixed(_f, _b, _l, _a)      \
     rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, NULL, NULL, true)
-#define rom_add_file_mr(_f, _mr, _i)            \
-    rom_add_file(_f, NULL, 0, _i, false, _mr, NULL)
+#define rom_add_file_mr(_f, _a, _mr, _i)            \
+    rom_add_file(_f, NULL, _a, _i, false, _mr, NULL)
 #define rom_add_file_as(_f, _as, _i)            \
     rom_add_file(_f, NULL, 0, _i, false, NULL, _as)
 #define rom_add_file_fixed_as(_f, _a, _i, _as)          \
---

rom_add_file_mr() is mostly used by ARM, so it'll be easy to
update the other calls.

Regards,

Phil.