Re: [Qemu-arm] [Qemu-devel] [PATCH for-4.1] hw/arm/sbsa-ref: Remove unnecessary check for secure_sysmem == NULL

[Radoslaw Biernacki]

On Fri, 5 Jul 2019 at 10:51, Philippe Mathieu-Daudé <address@hidden> wrote:

On 7/4/19 4:20 PM, Peter Maydell wrote:
> In the virt machine, we support TrustZone being either present or
> absent, and so the code must deal with the secure_sysmem pointer
> possibly being NULL. In the sbsa-ref machine, TrustZone is always
> present, but some code and comments copied from virt still treat
> it as possibly not being present.
>
> This causes Coverity to complain (CID 1407287) that we check
> secure_sysmem for being NULL after an unconditional dereference.
> Simplify the code so that instead of initializing the variable
> to NULL, unconditionally assigning it, and then testing it for NULL,
> we just initialize it correctly in the variable declaration and
> then assume it to be non-NULL. We also delete a comment which
> only applied to the non-TrustZone config.
>
> Signed-off-by: Peter Maydell <address@hidden>
> ---
> Not a bug as such, but we should put it in for 4.1 to
> keep Coverity happy.
> ---
>  hw/arm/sbsa-ref.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
> index ee53f0ff60d..6f315b79445 100644
> --- a/hw/arm/sbsa-ref.c
> +++ b/hw/arm/sbsa-ref.c
> @@ -254,8 +254,6 @@ static void sbsa_flash_map(SBSAMachineState *sms,
>       * sysmem is the system memory space. secure_sysmem is the secure view
>       * of the system, and the first flash device should be made visible only
>       * there. The second flash device is visible to both secure and nonsecure.
> -     * If sysmem == secure_sysmem this means there is no separate Secure
> -     * address space and both flash devices are generally visible.
>       */
>      hwaddr flashsize = sbsa_ref_memmap[SBSA_FLASH].size / 2;
>      hwaddr flashbase = sbsa_ref_memmap[SBSA_FLASH].base;
> @@ -588,7 +586,7 @@ static void sbsa_ref_init(MachineState *machine)
>      SBSAMachineState *sms = SBSA_MACHINE(machine);
>      MachineClass *mc = MACHINE_GET_CLASS(machine);
>      MemoryRegion *sysmem = get_system_memory();
> -    MemoryRegion *secure_sysmem = NULL;
> +    MemoryRegion *secure_sysmem = g_new(MemoryRegion, 1);
>      MemoryRegion *ram = g_new(MemoryRegion, 1);
>      bool firmware_loaded;
>      const CPUArchIdList *possible_cpus;
> @@ -612,13 +610,11 @@ static void sbsa_ref_init(MachineState *machine)
>       * containing the system memory at low priority; any secure-only
>       * devices go in at higher priority and take precedence.
>       */
> -    secure_sysmem = g_new(MemoryRegion, 1);
>      memory_region_init(secure_sysmem, OBJECT(machine), "secure-memory",
>                         UINT64_MAX);
>      memory_region_add_subregion_overlap(secure_sysmem, 0, sysmem, -1);
> 
> -    firmware_loaded = sbsa_firmware_init(sms, sysmem,
> -                                         secure_sysmem ?: sysmem);
> +    firmware_loaded = sbsa_firmware_init(sms, sysmem, secure_sysmem);
> 
>      if (machine->kernel_filename && firmware_loaded) {
>          error_report("sbsa-ref: No fw_cfg device on this machine, "
>

Reviewed-by: Philippe Mathieu-Daudé <address@hidden>

Thank you Peter.

Tested-by: Radosław Biernacki <address@hidden>

Reviewed-by: Radosław Biernacki <address@hidden>