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

[Peter Maydell]

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.

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

thanks
-- PMM