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,
> >>>