RE: [PATCH v1 1/2] aspeed/soc: fix coverity issue

[Jamin Lin]

Hi Cedric,
> -----Original Message-----
> From: Cédric Le Goater <clg@kaod.org>
> Sent: Monday, June 24, 2024 9:58 PM
> To: Peter Maydell <peter.maydell@linaro.org>; Jamin Lin
> <jamin_lin@aspeedtech.com>
> Cc: 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>; Troy Lee
> <troy_lee@aspeedtech.com>; Yunlin Tang <yunlin.tang@aspeedtech.com>
> Subject: Re: [PATCH v1 1/2] aspeed/soc: fix coverity issue
> 
> On 6/24/24 2:18 PM, Peter Maydell wrote:
> > On Wed, 19 Jun 2024 at 10:35, Jamin Lin <jamin_lin@aspeedtech.com>
> wrote:
> >>
> >> Fix coverity defect: DIVIDE_BY_ZERO.
> >>
> >> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.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;
> >> +    }
> >> +
> >
> > Isn't this a QEMU bug rather than a guest error? The RAM size
> > presumably should never be zero unless the board set the ram-size
> > property on the SDMC incorrectly. So the SDMC device should check (and
> > return an error from its realize
> > method) that the ram-size property is valid,
> 
> That's the case in aspeed_sdmc_set_ram_size() which is called from the
> aspeed machine init routine when the ram size is set.
> 
> Setting the machine ram size to zero on the command line doesn't report an
> error though and the size is the default.
> 
> > and then here we can just assert(ram_size != 0).
> 
> Yes.
> 
> Jamin, could you please send a v2 with the commit logs update I proposed ?
> See the patches on my aspeed-9.1 branch.
I resend v2 patch with your commit log, 
https://www.mail-archive.com/qemu-devel@nongnu.org/msg1050302.html
Do we need to drop this patch, 
https://www.mail-archive.com/qemu-devel@nongnu.org/msg1050301.html? 

Thanks-Jamin
> 
> Thanks,
> 
> C.