[PATCH v2 0/2] Fix coverity issues for AST2700

[PATCH v2 0/2] Fix coverity issues for AST2700

[Jamin Lin via]

change from v1:
aspeed/soc: coverity defect: DIVIDE_BY_ZERO
aspeed/sdmc: coverity defect: Control flow issues (DEADCODE)

change from v2:
add more commit log from reviewer, Cédric.

Jamin Lin (2):
  aspeed/soc: Fix possible divide by zero
  aspeed/sdmc: Remove extra R_MAIN_STATUS case

 hw/arm/aspeed_ast27x0.c | 6 ++++++
 hw/misc/aspeed_sdmc.c   | 1 -
 2 files changed, 6 insertions(+), 1 deletion(-)

-- 
2.25.1

[PATCH v2 0/2] Fix coverity issues for AST2700

[Jamin Lin via]

change from v1:
aspeed/soc: coverity defect: DIVIDE_BY_ZERO
aspeed/sdmc: coverity defect: Control flow issues (DEADCODE)

change from v2:
add more commit log from reviewer, Cédric.

Jamin Lin (2):
  aspeed/soc: Fix possible divide by zero
  aspeed/sdmc: Remove extra R_MAIN_STATUS case

 hw/arm/aspeed_ast27x0.c | 6 ++++++
 hw/misc/aspeed_sdmc.c   | 1 -
 2 files changed, 6 insertions(+), 1 deletion(-)

-- 
2.25.1

[PATCH v2 1/2] aspeed/soc: Fix possible divide by zero

[Jamin Lin via]

Coverity reports a possible DIVIDE_BY_ZERO issue regarding the
"ram_size" object property. This can not happen because RAM has
predefined valid sizes per SoC. Nevertheless, add a test to
close the issue.

Fixes: Coverity CID 1547113
Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
Reviewed-by: Cédric Le Goater <clg@redhat.com>
[ clg: Rewrote commit log ]
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 hw/arm/aspeed_ast27x0.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c
index b6876b4862..d14a46df6f 100644
--- a/hw/arm/aspeed_ast27x0.c
+++ b/hw/arm/aspeed_ast27x0.c
@@ -211,6 +211,12 @@ static void aspeed_ram_capacity_write(void *opaque, hwaddr addr, uint64_t data,
     ram_size = object_property_get_uint(OBJECT(&s->sdmc), "ram-size",
                                         &error_abort);
 
+    if (!ram_size) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: ram_size is zero",  __func__);
+        return;
+    }
+
     /*
      * Emulate ddr capacity hardware behavior.
      * If writes the data to the address which is beyond the ram size,
-- 
2.25.1

[PATCH v2 1/2] aspeed/soc: Fix possible divide by zero

[Jamin Lin via]

Coverity reports a possible DIVIDE_BY_ZERO issue regarding the
"ram_size" object property. This can not happen because RAM has
predefined valid sizes per SoC. Nevertheless, add a test to
close the issue.

Fixes: Coverity CID 1547113
Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
Reviewed-by: Cédric Le Goater <clg@redhat.com>
[ clg: Rewrote commit log ]
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 hw/arm/aspeed_ast27x0.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c
index b6876b4862..d14a46df6f 100644
--- a/hw/arm/aspeed_ast27x0.c
+++ b/hw/arm/aspeed_ast27x0.c
@@ -211,6 +211,12 @@ static void aspeed_ram_capacity_write(void *opaque, hwaddr addr, uint64_t data,
     ram_size = object_property_get_uint(OBJECT(&s->sdmc), "ram-size",
                                         &error_abort);
 
+    if (!ram_size) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: ram_size is zero",  __func__);
+        return;
+    }
+
     /*
      * Emulate ddr capacity hardware behavior.
      * If writes the data to the address which is beyond the ram size,
-- 
2.25.1

[PATCH v2 2/2] aspeed/sdmc: Remove extra R_MAIN_STATUS case

[Jamin Lin via]

Coverity reports that the newly added 'case R_MAIN_STATUS' is DEADCODE
because it can not be reached. This is because R_MAIN_STATUS is handled
before in the "Unprotected registers" switch statement. Remove it.

Fixes: Coverity CID 1547112
Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
Reviewed-by: Cédric Le Goater <clg@redhat.com>
[ clg: Rewrote commit log ]
Signed-off-by: Cédric Le Goater <clg@redhat.com>
---
 hw/misc/aspeed_sdmc.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c
index 93e2e29ead..94eed9264d 100644
--- a/hw/misc/aspeed_sdmc.c
+++ b/hw/misc/aspeed_sdmc.c
@@ -589,7 +589,6 @@ static void aspeed_2700_sdmc_write(AspeedSDMCState *s, uint32_t reg,
     case R_INT_STATUS:
     case R_INT_CLEAR:
     case R_INT_MASK:
-    case R_MAIN_STATUS:
     case R_ERR_STATUS:
     case R_ECC_FAIL_STATUS:
     case R_ECC_FAIL_ADDR:
-- 
2.25.1

Re: [PATCH v2 1/2] aspeed/soc: Fix possible divide by zero

[cmd]

Hi

On 25/06/2024 03:50, Jamin Lin via wrote:
> Coverity reports a possible DIVIDE_BY_ZERO issue regarding the
> "ram_size" object property. This can not happen because RAM has
> predefined valid sizes per SoC. Nevertheless, add a test to
> close the issue.
>
> Fixes: Coverity CID 1547113
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> Reviewed-by: Cédric Le Goater <clg@redhat.com>
> [ clg: Rewrote commit log ]
> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> ---
>   hw/arm/aspeed_ast27x0.c | 6 ++++++
>   1 file changed, 6 insertions(+)
>
> diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c
> index b6876b4862..d14a46df6f 100644
> --- a/hw/arm/aspeed_ast27x0.c
> +++ b/hw/arm/aspeed_ast27x0.c
> @@ -211,6 +211,12 @@ static void aspeed_ram_capacity_write(void *opaque, hwaddr addr, uint64_t data,
>       ram_size = object_property_get_uint(OBJECT(&s->sdmc), "ram-size",
>                                           &error_abort);
>   
> +    if (!ram_size) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: ram_size is zero",  __func__);
> +        return;
> +    }
> +
If we are sure that the error cannot happen, shouldn't we assert instead?
>       /*
>        * Emulate ddr capacity hardware behavior.
>        * If writes the data to the address which is beyond the ram size,

Re: [PATCH v2 1/2] aspeed/soc: Fix possible divide by zero

[Cédric Le Goater]

On 6/25/24 8:00 AM, cmd wrote:
> Hi
> 
> On 25/06/2024 03:50, Jamin Lin via wrote:
>> Coverity reports a possible DIVIDE_BY_ZERO issue regarding the
>> "ram_size" object property. This can not happen because RAM has
>> predefined valid sizes per SoC. Nevertheless, add a test to
>> close the issue.
>>
>> Fixes: Coverity CID 1547113
>> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
>> Reviewed-by: Cédric Le Goater <clg@redhat.com>
>> [ clg: Rewrote commit log ]
>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>> ---
>>   hw/arm/aspeed_ast27x0.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c
>> index b6876b4862..d14a46df6f 100644
>> --- a/hw/arm/aspeed_ast27x0.c
>> +++ b/hw/arm/aspeed_ast27x0.c
>> @@ -211,6 +211,12 @@ static void aspeed_ram_capacity_write(void *opaque, hwaddr addr, uint64_t data,
>>       ram_size = object_property_get_uint(OBJECT(&s->sdmc), "ram-size",
>>                                           &error_abort);
>> +    if (!ram_size) {
>> +        qemu_log_mask(LOG_GUEST_ERROR,
>> +                      "%s: ram_size is zero",  __func__);
>> +        return;
>> +    }
>> +
> If we are sure that the error cannot happen, shouldn't we assert instead?

Yes. That is what Peter suggested. This needs to be changed.


Thanks,

C.



>>       /*
>>        * Emulate ddr capacity hardware behavior.
>>        * If writes the data to the address which is beyond the ram size,

Re: [PATCH v2 1/2] aspeed/soc: Fix possible divide by zero

[cmd]

On 25/06/2024 08:03, Cédric Le Goater wrote:
> On 6/25/24 8:00 AM, cmd wrote:
>> Hi
>>
>> On 25/06/2024 03:50, Jamin Lin via wrote:
>>> Coverity reports a possible DIVIDE_BY_ZERO issue regarding the
>>> "ram_size" object property. This can not happen because RAM has
>>> predefined valid sizes per SoC. Nevertheless, add a test to
>>> close the issue.
>>>
>>> Fixes: Coverity CID 1547113
>>> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
>>> Reviewed-by: Cédric Le Goater <clg@redhat.com>
>>> [ clg: Rewrote commit log ]
>>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>>> ---
>>>   hw/arm/aspeed_ast27x0.c | 6 ++++++
>>>   1 file changed, 6 insertions(+)
>>>
>>> diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c
>>> index b6876b4862..d14a46df6f 100644
>>> --- a/hw/arm/aspeed_ast27x0.c
>>> +++ b/hw/arm/aspeed_ast27x0.c
>>> @@ -211,6 +211,12 @@ static void aspeed_ram_capacity_write(void 
>>> *opaque, hwaddr addr, uint64_t data,
>>>       ram_size = object_property_get_uint(OBJECT(&s->sdmc), "ram-size",
>>>                                           &error_abort);
>>> +    if (!ram_size) {
>>> +        qemu_log_mask(LOG_GUEST_ERROR,
>>> +                      "%s: ram_size is zero",  __func__);
>>> +        return;
>>> +    }
>>> +
>> If we are sure that the error cannot happen, shouldn't we assert 
>> instead?
>
> Yes. That is what Peter suggested. This needs to be changed.
>
>
> Thanks,
>
> C.
>
Ok fine, I didn't see the message, sorry!

Thanks

 >cmd

>
>
>>>       /*
>>>        * Emulate ddr capacity hardware behavior.
>>>        * If writes the data to the address which is beyond the ram 
>>> size,
>

RE: [PATCH v2 1/2] aspeed/soc: Fix possible divide by zero

[Jamin Lin]

Hi cmd, Cedric and Peter,

> -----Original Message-----
> From: cmd <clement.mathieudrif.etu@gmail.com>
> Sent: Tuesday, June 25, 2024 2:07 PM
> To: Cédric Le Goater <clg@kaod.org>; Jamin Lin <jamin_lin@aspeedtech.com>;
> Peter Maydell <peter.maydell@linaro.org>; Steven Lee
> <steven_lee@aspeedtech.com>; Troy Lee <leetroy@gmail.com>; Andrew
> Jeffery <andrew@codeconstruct.com.au>; Joel Stanley <joel@jms.id.au>; open
> list:ASPEED BMCs <qemu-arm@nongnu.org>; open list:All patches CC here
> <qemu-devel@nongnu.org>
> Cc: Cédric Le Goater <clg@redhat.com>
> Subject: Re: [PATCH v2 1/2] aspeed/soc: Fix possible divide by zero
> 
> 
> On 25/06/2024 08:03, Cédric Le Goater wrote:
> > On 6/25/24 8:00 AM, cmd wrote:
> >> Hi
> >>
> >> On 25/06/2024 03:50, Jamin Lin via wrote:
> >>> Coverity reports a possible DIVIDE_BY_ZERO issue regarding the
> >>> "ram_size" object property. This can not happen because RAM has
> >>> predefined valid sizes per SoC. Nevertheless, add a test to close
> >>> the issue.
> >>>
> >>> Fixes: Coverity CID 1547113
> >>> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> >>> Reviewed-by: Cédric Le Goater <clg@redhat.com> [ clg: Rewrote commit
> >>> log ]
> >>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> >>> ---
> >>>   hw/arm/aspeed_ast27x0.c | 6 ++++++
> >>>   1 file changed, 6 insertions(+)
> >>>
> >>> diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c index
> >>> b6876b4862..d14a46df6f 100644
> >>> --- a/hw/arm/aspeed_ast27x0.c
> >>> +++ b/hw/arm/aspeed_ast27x0.c
> >>> @@ -211,6 +211,12 @@ static void aspeed_ram_capacity_write(void
> >>> *opaque, hwaddr addr, uint64_t data,
> >>>       ram_size = object_property_get_uint(OBJECT(&s->sdmc),
> >>> "ram-size",
> >>>                                           &error_a
> bort);
> >>> +    if (!ram_size) {
> >>> +        qemu_log_mask(LOG_GUEST_ERROR,
> >>> +                      "%s: ram_size is zero",  __func__);
> >>> +        return;
> >>> +    }
> >>> +
> >> If we are sure that the error cannot happen, shouldn't we assert
> >> instead?
> >
> > Yes. That is what Peter suggested. This needs to be changed.
> >
Thanks for review and suggestion.
How about this change?

assert(ram_size > 0);

If you agree, I will send v3 patch.
Thanks-Jamin

> >
> > Thanks,
> >
> > C.
> >
> Ok fine, I didn't see the message, sorry!
> 
> Thanks
> 
>  >cmd
> 
> >
> >
> >>>       /*
> >>>        * Emulate ddr capacity hardware behavior.
> >>>        * If writes the data to the address which is beyond the ram
> >>> size,
> >

Re: [PATCH v2 1/2] aspeed/soc: Fix possible divide by zero

[Cédric Le Goater]

On 6/25/24 8:15 AM, Jamin Lin wrote:
> Hi cmd, Cedric and Peter,
> 
>> -----Original Message-----
>> From: cmd <clement.mathieudrif.etu@gmail.com>
>> Sent: Tuesday, June 25, 2024 2:07 PM
>> To: Cédric Le Goater <clg@kaod.org>; Jamin Lin <jamin_lin@aspeedtech.com>;
>> Peter Maydell <peter.maydell@linaro.org>; Steven Lee
>> <steven_lee@aspeedtech.com>; Troy Lee <leetroy@gmail.com>; Andrew
>> Jeffery <andrew@codeconstruct.com.au>; Joel Stanley <joel@jms.id.au>; open
>> list:ASPEED BMCs <qemu-arm@nongnu.org>; open list:All patches CC here
>> <qemu-devel@nongnu.org>
>> Cc: Cédric Le Goater <clg@redhat.com>
>> Subject: Re: [PATCH v2 1/2] aspeed/soc: Fix possible divide by zero
>>
>>
>> On 25/06/2024 08:03, Cédric Le Goater wrote:
>>> On 6/25/24 8:00 AM, cmd wrote:
>>>> Hi
>>>>
>>>> On 25/06/2024 03:50, Jamin Lin via wrote:
>>>>> Coverity reports a possible DIVIDE_BY_ZERO issue regarding the
>>>>> "ram_size" object property. This can not happen because RAM has
>>>>> predefined valid sizes per SoC. Nevertheless, add a test to close
>>>>> the issue.
>>>>>
>>>>> Fixes: Coverity CID 1547113
>>>>> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
>>>>> Reviewed-by: Cédric Le Goater <clg@redhat.com> [ clg: Rewrote commit
>>>>> log ]
>>>>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
>>>>> ---
>>>>>    hw/arm/aspeed_ast27x0.c | 6 ++++++
>>>>>    1 file changed, 6 insertions(+)
>>>>>
>>>>> diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c index
>>>>> b6876b4862..d14a46df6f 100644
>>>>> --- a/hw/arm/aspeed_ast27x0.c
>>>>> +++ b/hw/arm/aspeed_ast27x0.c
>>>>> @@ -211,6 +211,12 @@ static void aspeed_ram_capacity_write(void
>>>>> *opaque, hwaddr addr, uint64_t data,
>>>>>        ram_size = object_property_get_uint(OBJECT(&s->sdmc),
>>>>> "ram-size",
>>>>>                                            &error_a
>> bort);
>>>>> +    if (!ram_size) {
>>>>> +        qemu_log_mask(LOG_GUEST_ERROR,
>>>>> +                      "%s: ram_size is zero",  __func__);
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>> If we are sure that the error cannot happen, shouldn't we assert
>>>> instead?
>>>
>>> Yes. That is what Peter suggested. This needs to be changed.
>>>
> Thanks for review and suggestion.
> How about this change?
> 
> assert(ram_size > 0);

yes.

I will send another patch fixing a long standing issue in the SDMC
model not checking the ram_size value in the realize handler. It
relies on the "ram-size" property being set.

Thanks,

C.


> If you agree, I will send v3 patch.
> Thanks-Jamin
> 
>>>
>>> Thanks,
>>>
>>> C.
>>>
>> Ok fine, I didn't see the message, sorry!
>>
>> Thanks
>>
>>   >cmd
>>
>>>
>>>
>>>>>        /*
>>>>>         * Emulate ddr capacity hardware behavior.
>>>>>         * If writes the data to the address which is beyond the ram
>>>>> size,
>>>

RE: [PATCH v2 1/2] aspeed/soc: Fix possible divide by zero

[Jamin Lin]

Hi Cedric,

> -----Original Message-----
> From: Cédric Le Goater <clg@kaod.org>
> Sent: Tuesday, June 25, 2024 2:38 PM
> To: Jamin Lin <jamin_lin@aspeedtech.com>; cmd
> <clement.mathieudrif.etu@gmail.com>; Peter Maydell
> <peter.maydell@linaro.org>; Steven Lee <steven_lee@aspeedtech.com>; Troy
> Lee <leetroy@gmail.com>; Andrew Jeffery <andrew@codeconstruct.com.au>;
> Joel Stanley <joel@jms.id.au>; open list:ASPEED BMCs
> <qemu-arm@nongnu.org>; open list:All patches CC here
> <qemu-devel@nongnu.org>
> Cc: Cédric Le Goater <clg@redhat.com>
> Subject: Re: [PATCH v2 1/2] aspeed/soc: Fix possible divide by zero
> 
> On 6/25/24 8:15 AM, Jamin Lin wrote:
> > Hi cmd, Cedric and Peter,
> >
> >> -----Original Message-----
> >> From: cmd <clement.mathieudrif.etu@gmail.com>
> >> Sent: Tuesday, June 25, 2024 2:07 PM
> >> To: Cédric Le Goater <clg@kaod.org>; Jamin Lin
> >> <jamin_lin@aspeedtech.com>; Peter Maydell <peter.maydell@linaro.org>;
> >> Steven Lee <steven_lee@aspeedtech.com>; Troy Lee
> <leetroy@gmail.com>;
> >> Andrew Jeffery <andrew@codeconstruct.com.au>; Joel Stanley
> >> <joel@jms.id.au>; open list:ASPEED BMCs <qemu-arm@nongnu.org>; open
> >> list:All patches CC here <qemu-devel@nongnu.org>
> >> Cc: Cédric Le Goater <clg@redhat.com>
> >> Subject: Re: [PATCH v2 1/2] aspeed/soc: Fix possible divide by zero
> >>
> >>
> >> On 25/06/2024 08:03, Cédric Le Goater wrote:
> >>> On 6/25/24 8:00 AM, cmd wrote:
> >>>> Hi
> >>>>
> >>>> On 25/06/2024 03:50, Jamin Lin via wrote:
> >>>>> Coverity reports a possible DIVIDE_BY_ZERO issue regarding the
> >>>>> "ram_size" object property. This can not happen because RAM has
> >>>>> predefined valid sizes per SoC. Nevertheless, add a test to close
> >>>>> the issue.
> >>>>>
> >>>>> Fixes: Coverity CID 1547113
> >>>>> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> >>>>> Reviewed-by: Cédric Le Goater <clg@redhat.com> [ clg: Rewrote
> >>>>> commit log ]
> >>>>> Signed-off-by: Cédric Le Goater <clg@redhat.com>
> >>>>> ---
> >>>>>    hw/arm/aspeed_ast27x0.c | 6 ++++++
> >>>>>    1 file changed, 6 insertions(+)
> >>>>>
> >>>>> diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c
> >>>>> index b6876b4862..d14a46df6f 100644
> >>>>> --- a/hw/arm/aspeed_ast27x0.c
> >>>>> +++ b/hw/arm/aspeed_ast27x0.c
> >>>>> @@ -211,6 +211,12 @@ static void aspeed_ram_capacity_write(void
> >>>>> *opaque, hwaddr addr, uint64_t data,
> >>>>>        ram_size = object_property_get_uint(OBJECT(&s->sdmc),
> >>>>> "ram-size",
> >>>>>                                            &erro
> r_a
> >> bort);
> >>>>> +    if (!ram_size) {
> >>>>> +        qemu_log_mask(LOG_GUEST_ERROR,
> >>>>> +                      "%s: ram_size is zero",  __func__);
> >>>>> +        return;
> >>>>> +    }
> >>>>> +
> >>>> If we are sure that the error cannot happen, shouldn't we assert
> >>>> instead?
> >>>
> >>> Yes. That is what Peter suggested. This needs to be changed.
> >>>
> > Thanks for review and suggestion.
> > How about this change?
> >
> > assert(ram_size > 0);
> 
> yes.
> 
> I will send another patch fixing a long standing issue in the SDMC model not
> checking the ram_size value in the realize handler. It relies on the "ram-size"
> property being set.
> 
> Thanks,
> 
Will send v3 patch and thanks for your review and help.
Jamin

> C.
> 
> 
> > If you agree, I will send v3 patch.
> > Thanks-Jamin
> >
> >>>
> >>> Thanks,
> >>>
> >>> C.
> >>>
> >> Ok fine, I didn't see the message, sorry!
> >>
> >> Thanks
> >>
> >>   >cmd
> >>
> >>>
> >>>
> >>>>>        /*
> >>>>>         * Emulate ddr capacity hardware behavior.
> >>>>>         * If writes the data to the address which is beyond the
> >>>>> ram size,
> >>>