[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PULL 21/36] hw/arm/allwinner-h3: add Boot ROM support

From: Niek Linnenbank
Subject: Re: [PULL 21/36] hw/arm/allwinner-h3: add Boot ROM support
Date: Sat, 21 Mar 2020 18:17:29 +0100

Hi Peter,

On Fri, Mar 20, 2020 at 1:08 PM Peter Maydell <address@hidden> wrote:
On Thu, 12 Mar 2020 at 16:45, Peter Maydell <address@hidden> wrote:
> From: Niek Linnenbank <address@hidden>
> 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 <address@hidden>
> Reviewed-by: Alex Bennée <address@hidden>
> Message-id: address@hidden
> Signed-off-by: Peter Maydell <address@hidden>

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.



-- PMM

Niek Linnenbank

reply via email to

[Prev in Thread] Current Thread [Next in Thread]