On Fri, 5 Jul 2019 at 10:51, Philippe Mathieu-Daudé <[email protected]> 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 <[email protected]> > > --- > > 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é <[email protected]> > Thank you Peter. Tested-by: Radosław Biernacki <[email protected]> Reviewed-by: Radosław Biernacki <[email protected]>
