RE: [PATCH v2 1/2] aspeed/soc: Fix possible divide by zero
Hi Cedric,
> -Original Message-
> From: Cédric Le Goater
> Sent: Tuesday, June 25, 2024 2:38 PM
> To: Jamin Lin ; cmd
> ; Peter Maydell
> ; Steven Lee ; Troy
> Lee ; Andrew Jeffery ;
> Joel Stanley ; open list:ASPEED BMCs
> ; open list:All patches CC here
>
> Cc: Cédric Le Goater
> 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
> >> Sent: Tuesday, June 25, 2024 2:07 PM
> >> To: Cédric Le Goater ; Jamin Lin
> >> ; Peter Maydell ;
> >> Steven Lee ; Troy Lee
> ;
> >> Andrew Jeffery ; Joel Stanley
> >> ; open list:ASPEED BMCs ; open
> >> list:All patches CC here
> >> Cc: Cédric Le Goater
> >> 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
> >>>>> Reviewed-by: Cédric Le Goater [ clg: Rewrote
> >>>>> commit log ]
> >>>>> Signed-off-by: Cédric Le Goater
> >>>>> ---
> >>>>> 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(>sdmc),
> >>>>> "ram-size",
> >>>>>
> 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,
> >>>
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
Sent: Tuesday, June 25, 2024 2:07 PM
To: Cédric Le Goater ; Jamin Lin ;
Peter Maydell ; Steven Lee
; Troy Lee ; Andrew
Jeffery ; Joel Stanley ; open
list:ASPEED BMCs ; open list:All patches CC here
Cc: Cédric Le Goater
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
Reviewed-by: Cédric Le Goater [ clg: Rewrote commit
log ]
Signed-off-by: Cédric Le Goater
---
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(>sdmc),
"ram-size",
_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
Hi cmd, Cedric and Peter,
> -Original Message-
> From: cmd
> Sent: Tuesday, June 25, 2024 2:07 PM
> To: Cédric Le Goater ; Jamin Lin ;
> Peter Maydell ; Steven Lee
> ; Troy Lee ; Andrew
> Jeffery ; Joel Stanley ; open
> list:ASPEED BMCs ; open list:All patches CC here
>
> Cc: Cédric Le Goater
> 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
> >>> Reviewed-by: Cédric Le Goater [ clg: Rewrote commit
> >>> log ]
> >>> Signed-off-by: Cédric Le Goater
> >>> ---
> >>> 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(>sdmc),
> >>> "ram-size",
> >>> _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
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
Reviewed-by: Cédric Le Goater
[ clg: Rewrote commit log ]
Signed-off-by: Cédric Le Goater
---
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(>sdmc), "ram-size",
_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
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
Reviewed-by: Cédric Le Goater
[ clg: Rewrote commit log ]
Signed-off-by: Cédric Le Goater
---
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(>sdmc), "ram-size",
_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
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
Reviewed-by: Cédric Le Goater
[ clg: Rewrote commit log ]
Signed-off-by: Cédric Le Goater
---
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(>sdmc), "ram-size",
_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,
[PATCH v2 1/2] aspeed/soc: Fix possible divide by zero
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
Reviewed-by: Cédric Le Goater
[ clg: Rewrote commit log ]
Signed-off-by: Cédric Le Goater
---
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(>sdmc), "ram-size",
_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
