From: Niek Linnenbank <nieklinnenbank@gmail.com>

A real Allwinner H3 SoC contains a Boot ROM which is the
first code that runs right after the SoC is powered on.
The Boot ROM is responsible for loading user code (e.g. a bootloader)
from any of the supported external devices and writing the downloaded
code to internal SRAM. After loading the SoC begins executing the code
written to SRAM.

This commits adds emulation of the Boot ROM firmware setup functionality
by loading user code from SD card in the A1 SRAM. While the A1 SRAM is
64KiB, we limit the size to 32KiB because the real H3 Boot ROM also rejects
sizes larger than 32KiB. For reference, this behaviour is documented
by the Linux Sunxi project wiki at:

  https://linux-sunxi.org/BROM#U-Boot_SPL_limitations

Signed-off-by: Niek Linnenbank <nieklinnenbank@gmail.com>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Message-id: 20200311221854.30370-11-nieklinnenbank@gmail.com
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/hw/arm/allwinner-h3.h | 21 +++++++++++++++++++++
 hw/arm/allwinner-h3.c         | 17 +++++++++++++++++
 hw/arm/orangepi.c             |  5 +++++
 3 files changed, 43 insertions(+)

diff --git a/include/hw/arm/allwinner-h3.h b/include/hw/arm/allwinner-h3.h
index f9b9a023734..d338003724e 100644
--- a/include/hw/arm/allwinner-h3.h
+++ b/include/hw/arm/allwinner-h3.h
@@ -46,6 +46,7 @@
 #include "hw/sd/allwinner-sdhost.h"
 #include "hw/net/allwinner-sun8i-emac.h"
 #include "target/arm/cpu.h"
+#include "sysemu/block-backend.h"
 
 /**
  * Allwinner H3 device list
@@ -129,4 +130,24 @@ typedef struct AwH3State {
     MemoryRegion sram_c;
 } AwH3State;
 
+/**
+ * Emulate Boot ROM firmware setup functionality.
+ *
+ * A real Allwinner H3 SoC contains a Boot ROM
+ * which is the first code that runs right after
+ * the SoC is powered on. The Boot ROM is responsible
+ * for loading user code (e.g. a bootloader) from any
+ * of the supported external devices and writing the
+ * downloaded code to internal SRAM. After loading the SoC
+ * begins executing the code written to SRAM.
+ *
+ * This function emulates the Boot ROM by copying 32 KiB
+ * of data from the given block device and writes it to
+ * the start of the first internal SRAM memory.
+ *
+ * @s: Allwinner H3 state object pointer
+ * @blk: Block backend device object pointer
+ */
+void allwinner_h3_bootrom_setup(AwH3State *s, BlockBackend *blk);
+
 #endif /* HW_ARM_ALLWINNER_H3_H */
diff --git a/hw/arm/allwinner-h3.c b/hw/arm/allwinner-h3.c
index d1245d2b016..a9767c70c08 100644
--- a/hw/arm/allwinner-h3.c
+++ b/hw/arm/allwinner-h3.c
@@ -29,6 +29,7 @@
 #include "hw/char/serial.h"
 #include "hw/misc/unimp.h"
 #include "hw/usb/hcd-ehci.h"
+#include "hw/loader.h"
 #include "sysemu/sysemu.h"
 #include "hw/arm/allwinner-h3.h"
 
@@ -170,6 +171,22 @@ enum {
     AW_H3_GIC_NUM_SPI       = 128
 };
 
+void allwinner_h3_bootrom_setup(AwH3State *s, BlockBackend *blk)
+{
+    const int64_t rom_size = 32 * KiB;
+    g_autofree uint8_t *buffer = g_new0(uint8_t, rom_size);
+
+    if (blk_pread(blk, 8 * KiB, buffer, rom_size) < 0) {
+        error_setg(&error_fatal, "%s: failed to read BlockBackend data",
+                   __func__);
+        return;
+    }
+
+    rom_add_blob("allwinner-h3.bootrom", buffer, rom_size,
+                  rom_size, s->memmap[AW_H3_SRAM_A1],
+                  NULL, NULL, NULL, NULL, false);
+}
+
 static void allwinner_h3_init(Object *obj)
 {
     AwH3State *s = AW_H3(obj);
diff --git a/hw/arm/orangepi.c b/hw/arm/orangepi.c
index d65bbf8a2fe..b8ebcb08b76 100644
--- a/hw/arm/orangepi.c
+++ b/hw/arm/orangepi.c
@@ -97,6 +97,11 @@ static void orangepi_init(MachineState *machine)
     memory_region_add_subregion(get_system_memory(), h3->memmap[AW_H3_SDRAM],
                                 machine->ram);
 
+    /* Load target kernel or start using BootROM */
+    if (!machine->kernel_filename && blk_is_available(blk)) {
+        /* Use Boot ROM to copy data from SD card to SRAM */
+        allwinner_h3_bootrom_setup(h3, blk);
+    }
     orangepi_binfo.loader_start = h3->memmap[AW_H3_SDRAM];
     orangepi_binfo.ram_size = machine->ram_size;
     arm_load_kernel(ARM_CPU(first_cpu), machine, &orangepi_binfo);
-- 
2.20.1

On Thu, 12 Mar 2020 at 16:45, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> From: Niek Linnenbank <nieklinnenbank@gmail.com>
>
> A real Allwinner H3 SoC contains a Boot ROM which is the
> first code that runs right after the SoC is powered on.
> The Boot ROM is responsible for loading user code (e.g. a bootloader)
> from any of the supported external devices and writing the downloaded
> code to internal SRAM. After loading the SoC begins executing the code
> written to SRAM.
>
> This commits adds emulation of the Boot ROM firmware setup functionality
> by loading user code from SD card in the A1 SRAM. While the A1 SRAM is
> 64KiB, we limit the size to 32KiB because the real H3 Boot ROM also rejects
> sizes larger than 32KiB. For reference, this behaviour is documented
> by the Linux Sunxi project wiki at:
>
>   https://linux-sunxi.org/BROM#U-Boot_SPL_limitations
>
> Signed-off-by: Niek Linnenbank <nieklinnenbank@gmail.com>
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> Message-id: 20200311221854.30370-11-nieklinnenbank@gmail.com
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Hi; Coverity (CID 1421882) points out a possible NULL
pointer dereference in this change:

> diff --git a/hw/arm/orangepi.c b/hw/arm/orangepi.c
> index d65bbf8a2fe..b8ebcb08b76 100644
> --- a/hw/arm/orangepi.c
> +++ b/hw/arm/orangepi.c
> @@ -97,6 +97,11 @@ static void orangepi_init(MachineState *machine)
>      memory_region_add_subregion(get_system_memory(), h3->memmap[AW_H3_SDRAM],
>                                  machine->ram);
>
> +    /* Load target kernel or start using BootROM */
> +    if (!machine->kernel_filename && blk_is_available(blk)) {
> +        /* Use Boot ROM to copy data from SD card to SRAM */
> +        allwinner_h3_bootrom_setup(h3, blk);
> +    }

blk_is_available() assumes its argument is non-NULL, but
earlier in the function we set up blk with:
   blk = di ? blk_by_legacy_dinfo(di) : NULL;

so it can be NULL here.

Could you send a patch to fix this, please? Probably
just adding '&& blk' in the middle of the condition
is sufficient.

thanks
-- PMM

Hi Peter,


On Fri, Mar 20, 2020 at 1:08 PM Peter Maydell <peter.maydell@linaro.org>
wrote:

> On Thu, 12 Mar 2020 at 16:45, Peter Maydell <peter.maydell@linaro.org>
> wrote:
> >
> > From: Niek Linnenbank <nieklinnenbank@gmail.com>
> >
> > A real Allwinner H3 SoC contains a Boot ROM which is the
> > first code that runs right after the SoC is powered on.
> > The Boot ROM is responsible for loading user code (e.g. a bootloader)
> > from any of the supported external devices and writing the downloaded
> > code to internal SRAM. After loading the SoC begins executing the code
> > written to SRAM.
> >
> > This commits adds emulation of the Boot ROM firmware setup functionality
> > by loading user code from SD card in the A1 SRAM. While the A1 SRAM is
> > 64KiB, we limit the size to 32KiB because the real H3 Boot ROM also
> rejects
> > sizes larger than 32KiB. For reference, this behaviour is documented
> > by the Linux Sunxi project wiki at:
> >
> >   https://linux-sunxi.org/BROM#U-Boot_SPL_limitations
> >
> > Signed-off-by: Niek Linnenbank <nieklinnenbank@gmail.com>
> > Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> > Message-id: 20200311221854.30370-11-nieklinnenbank@gmail.com
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>
> Hi; Coverity (CID 1421882) points out a possible NULL
> pointer dereference in this change:
>
> > diff --git a/hw/arm/orangepi.c b/hw/arm/orangepi.c
> > index d65bbf8a2fe..b8ebcb08b76 100644
> > --- a/hw/arm/orangepi.c
> > +++ b/hw/arm/orangepi.c
> > @@ -97,6 +97,11 @@ static void orangepi_init(MachineState *machine)
> >      memory_region_add_subregion(get_system_memory(),
> h3->memmap[AW_H3_SDRAM],
> >                                  machine->ram);
> >
> > +    /* Load target kernel or start using BootROM */
> > +    if (!machine->kernel_filename && blk_is_available(blk)) {
> > +        /* Use Boot ROM to copy data from SD card to SRAM */
> > +        allwinner_h3_bootrom_setup(h3, blk);
> > +    }
>
> blk_is_available() assumes its argument is non-NULL, but
> earlier in the function we set up blk with:
>    blk = di ? blk_by_legacy_dinfo(di) : NULL;
>
> so it can be NULL here.
>
>
Right, indeed.


> Could you send a patch to fix this, please? Probably
>
just adding '&& blk' in the middle of the condition
> is sufficient.
>

Sure, I'll make a small patch for this, and also the other one you reported
for the SDRAM controller.

By the way, I do not have the coverity tool unfortunately. So I can't
really check myself
if any of the other Allwinner H3 files also have warnings that can be
fixed..
But if coverity finds more, just let me know, and I'll look into it.

Regards,
Niek



>
> thanks
> -- PMM
>


-- 
Niek Linnenbank

On Sat, 21 Mar 2020 at 17:17, Niek Linnenbank <nieklinnenbank@gmail.com> wrote:
> By the way, I do not have the coverity tool unfortunately. So I can't really check myself
> if any of the other Allwinner H3 files also have warnings that can be fixed..
> But if coverity finds more, just let me know, and I'll look into it.

Yeah, we only have Coverity as an online tool, so it
just runs once code gets into master, and then we
fix stuff after the fact. Either I or somebody else
will usually go through the reported new issues when
we get a new run and alert relevant people.

thanks
-- PMM