Re: [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>