hw/arm/sbsa-ref: Remove unnecessary check for secure_sysmem == NULL

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

Posted by Peter Maydell 4 years, 10 months ago

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 <peter.maydell@linaro.org>
---
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, "
-- 
2.20.1

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

Posted by Philippe Mathieu-Daudé 4 years, 10 months ago

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 <peter.maydell@linaro.org>
> ---
> 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é <philmd@redhat.com>

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

Posted by Radoslaw Biernacki 4 years, 10 months ago

On Fri, 5 Jul 2019 at 10:51, Philippe Mathieu-Daudé <philmd@redhat.com>
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 <peter.maydell@linaro.org>
> > ---
> > 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é <philmd@redhat.com>
>

Thank you Peter.

Tested-by: Radosław Biernacki <radoslaw.biernacki@linaro.org>
Reviewed-by: Radosław Biernacki <radoslaw.biernacki@linaro.org>