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.

Please review.

Regards,

Phil.

$ git backport-diff -u v2
001/16:[----] [--] 'hw/misc/bcm2835_thermal: Add a dummy BCM2835 thermal sensor'
002/16:[----] [--] 'hw/arm/bcm2835_peripherals: Use the thermal sensor block'
003/16:[----] [--] 'hw/timer/bcm2835: Add the BCM2835 SYS_timer'
004/16:[----] [--] 'hw/arm/bcm2835_peripherals: Use the SYS_timer'
005/16:[----] [--] 'hw/arm/bcm2836: Make the SoC code modular'
006/16:[down] 'hw/arm/bcm2836: Rename cpus[] as cpu[].core'
007/16:[0053] [FC] 'hw/arm/bcm2836: Use per CPU address spaces'
008/16:[down] 'hw/arm/bcm2835_peripherals: Add const link property in realize()'
009/16:[0105] [FC] 'hw/arm/bcm2836: Create VideoCore address space in the SoC'
010/16:[----] [--] 'hw/arm/raspi: Use AddressSpace when using arm_boot::write_secondary_boot'
011/16:[down] 'hw/arm/raspi: Use -smp cores=<N> option to restrict enabled cores'
012/16:[down] 'hw/arm/bcm2836: Rename enabled_cpus -> enabled_cores'
013/16:[----] [-C] 'hw/arm/raspi: Make the board code modular'
014/16:[----] [--] 'hw/arm/highbank: Use AddressSpace when using write_secondary_boot()'
015/16:[down] 'python/qemu/machine: Allow to use other serial consoles than default'
016/16:[down] 'NOTFORMERGE tests/acceptance: Test U-boot on the Raspberry Pi 3'

v2: https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg04474.html

Philippe Mathieu-Daudé (16):
  hw/misc/bcm2835_thermal: Add a dummy BCM2835 thermal sensor
  hw/arm/bcm2835_peripherals: Use the thermal sensor block
  hw/timer/bcm2835: Add the BCM2835 SYS_timer
  hw/arm/bcm2835_peripherals: Use the SYS_timer
  hw/arm/bcm2836: Make the SoC code modular
  hw/arm/bcm2836: Rename cpus[] as cpu[].core
  hw/arm/bcm2836: Use per CPU address spaces
  hw/arm/bcm2835_peripherals: Add const link property in realize()
  hw/arm/bcm2836: Create VideoCore address space in the SoC
  hw/arm/raspi: Use AddressSpace when using
    arm_boot::write_secondary_boot
  hw/arm/raspi: Use -smp cores=<N> option to restrict enabled cores
  hw/arm/bcm2836: Rename enabled_cpus -> enabled_cores
  hw/arm/raspi: Make the board code modular
  hw/arm/highbank: Use AddressSpace when using write_secondary_boot()
  python/qemu/machine: Allow to use other serial consoles than default
  NOTFORMERGE tests/acceptance: Test U-boot on the Raspberry Pi 3

 hw/arm/bcm2835_peripherals.c           |  76 +++++++----
 hw/arm/bcm2836.c                       | 119 ++++++++++++++----
 hw/arm/highbank.c                      |   3 +-
 hw/arm/raspi.c                         | 127 +++++++++++++++----
 hw/misc/Makefile.objs                  |   1 +
 hw/misc/bcm2835_thermal.c              | 135 ++++++++++++++++++++
 hw/timer/Makefile.objs                 |   1 +
 hw/timer/bcm2835_systmr.c              | 166 +++++++++++++++++++++++++
 hw/timer/trace-events                  |   5 +
 include/hw/arm/bcm2835_peripherals.h   |   9 +-
 include/hw/arm/bcm2836.h               |  17 ++-
 include/hw/arm/raspi_platform.h        |   1 +
 include/hw/misc/bcm2835_thermal.h      |  27 ++++
 include/hw/timer/bcm2835_systmr.h      |  33 +++++
 python/qemu/machine.py                 |   9 +-
 tests/acceptance/boot_linux_console.py |  23 ++++
 16 files changed, 671 insertions(+), 81 deletions(-)
 create mode 100644 hw/misc/bcm2835_thermal.c
 create mode 100644 hw/timer/bcm2835_systmr.c
 create mode 100644 include/hw/misc/bcm2835_thermal.h
 create mode 100644 include/hw/timer/bcm2835_systmr.h

-- 
2.21.0

On Sun, 20 Oct 2019 at 00:47, Philippe Mathieu-Daudé <f4bug@amsat.org> 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.)

thanks
-- PMM

On 10/24/19 3:42 PM, Peter Maydell wrote:
> On Sun, 20 Oct 2019 at 00:47, Philippe Mathieu-Daudé <f4bug@amsat.org> 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.)

Thanks!

Do you mind amending this to patch #3
"hw/timer/bcm2835: Add the BCM2835 SYS_timer"
or should I respin (or resend it alone)?

-- >8 --
diff --git a/hw/timer/bcm2835_systmr.c b/hw/timer/bcm2835_systmr.c
index 49b40b55f9..3387a6214a 100644
--- a/hw/timer/bcm2835_systmr.c
+++ b/hw/timer/bcm2835_systmr.c
@@ -115,10 +115,7 @@ static void bcm2835_systmr_reset(DeviceState *dev)
  {
      BCM2835SystemTimerState *s = BCM2835_SYSTIMER(dev);

-    s->reg.status = 0;
-    for (size_t i = 0; i < ARRAY_SIZE(s->reg.compare); i++) {
-        s->reg.compare[i] = 0;
-    }
+    memset(&s->reg, 0, sizeof(s->reg));
  }

---

Thanks,

Phil.

On Thu, 24 Oct 2019 at 14:46, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 10/24/19 3:42 PM, Peter Maydell wrote:
> > On Sun, 20 Oct 2019 at 00:47, Philippe Mathieu-Daudé <f4bug@amsat.org> 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.)
>
> Thanks!
>
> Do you mind amending this to patch #3
> "hw/timer/bcm2835: Add the BCM2835 SYS_timer"
> or should I respin (or resend it alone)?
>
> -- >8 --
> diff --git a/hw/timer/bcm2835_systmr.c b/hw/timer/bcm2835_systmr.c
> index 49b40b55f9..3387a6214a 100644
> --- a/hw/timer/bcm2835_systmr.c
> +++ b/hw/timer/bcm2835_systmr.c
> @@ -115,10 +115,7 @@ static void bcm2835_systmr_reset(DeviceState *dev)
>   {
>       BCM2835SystemTimerState *s = BCM2835_SYSTIMER(dev);
>
> -    s->reg.status = 0;
> -    for (size_t i = 0; i < ARRAY_SIZE(s->reg.compare); i++) {
> -        s->reg.compare[i] = 0;
> -    }
> +    memset(&s->reg, 0, sizeof(s->reg));
>   }
>

Sure, I'll just squash that in.

-- PMM

On 10/24/19 3:49 PM, Peter Maydell wrote:
> On Thu, 24 Oct 2019 at 14:46, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> On 10/24/19 3:42 PM, Peter Maydell wrote:
>>> On Sun, 20 Oct 2019 at 00:47, Philippe Mathieu-Daudé <f4bug@amsat.org> 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.)

I guess you meant you reviewed patch 9.

>>
>> Thanks!
>>
>> Do you mind amending this to patch #3
>> "hw/timer/bcm2835: Add the BCM2835 SYS_timer"
>> or should I respin (or resend it alone)?
>>
>> -- >8 --
>> diff --git a/hw/timer/bcm2835_systmr.c b/hw/timer/bcm2835_systmr.c
>> index 49b40b55f9..3387a6214a 100644
>> --- a/hw/timer/bcm2835_systmr.c
>> +++ b/hw/timer/bcm2835_systmr.c
>> @@ -115,10 +115,7 @@ static void bcm2835_systmr_reset(DeviceState *dev)
>>    {
>>        BCM2835SystemTimerState *s = BCM2835_SYSTIMER(dev);
>>
>> -    s->reg.status = 0;
>> -    for (size_t i = 0; i < ARRAY_SIZE(s->reg.compare); i++) {
>> -        s->reg.compare[i] = 0;
>> -    }
>> +    memset(&s->reg, 0, sizeof(s->reg));
>>    }
>>
> 
> Sure, I'll just squash that in.

Thanks a lot!

Phil.

On Thu, 24 Oct 2019 at 14:42, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Sun, 20 Oct 2019 at 00:47, Philippe Mathieu-Daudé <f4bug@amsat.org> 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.

thanks
-- PMM

On 10/24/19 5:31 PM, Peter Maydell wrote:
> On Thu, 24 Oct 2019 at 14:42, Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> On Sun, 20 Oct 2019 at 00:47, Philippe Mathieu-Daudé <f4bug@amsat.org> 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.

On Thu, 24 Oct 2019 at 20:46, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> In my patches I stop using system_memory, each CPU use its
> own AS view on the GPU AXI bus.

That seems an odd thing to do  -- is there really no
common baseline view of the physical address space
that the CPUs all share ?

thanks
-- PMM