| Message ID | 20240830173452.2086140-2-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | m68k: Fix a couple of Coverity nits | expand |
Am Fri, 30 Aug 2024 18:34:50 +0100 schrieb Peter Maydell <peter.maydell@linaro.org>: > In m5208_sys_read(), we have a loop of n from 0 to 31, and we > calculate (2u << n). For the n == 31 iteration this will shift off > the top of the unsigned 32 bit integer. > > This is harmless, because we're going to stop the loop with n == 31 > anyway, but we can avoid the error by using 64-bit arithmetic here. > > (The SDCS0 register is documented at > https://www.nxp.com/docs/en/reference-manual/MCF5208RM.pdf > section 18.4.5; we want the lower 5 bits to indicate the > RAM size, where 31 == 4GB, 30 == 2GB, and so on down. > As it happens, the layout of the mcf5208evb board memory map > means it doesn't make sense to have more than 1GB of RAM > in any case.) > > Resolves: Coverity CID 1547727 > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > hw/m68k/mcf5208.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/m68k/mcf5208.c b/hw/m68k/mcf5208.c > index ec14096aa43..0ad347dfa81 100644 > --- a/hw/m68k/mcf5208.c > +++ b/hw/m68k/mcf5208.c > @@ -158,7 +158,7 @@ static uint64_t m5208_sys_read(void *opaque, hwaddr addr, > { > int n; > for (n = 0; n < 32; n++) { > - if (current_machine->ram_size < (2u << n)) { > + if (current_machine->ram_size < (2ULL << n)) { > break; > } > } Reviewed-by: Thomas Huth <huth@tuxfamily.org>
On 8/31/24 03:34, Peter Maydell wrote: > In m5208_sys_read(), we have a loop of n from 0 to 31, and we > calculate (2u << n). For the n == 31 iteration this will shift off > the top of the unsigned 32 bit integer. > > This is harmless, because we're going to stop the loop with n == 31 > anyway, but we can avoid the error by using 64-bit arithmetic here. > > (The SDCS0 register is documented at > https://www.nxp.com/docs/en/reference-manual/MCF5208RM.pdf > section 18.4.5; we want the lower 5 bits to indicate the > RAM size, where 31 == 4GB, 30 == 2GB, and so on down. > As it happens, the layout of the mcf5208evb board memory map > means it doesn't make sense to have more than 1GB of RAM > in any case.) > > Resolves: Coverity CID 1547727 > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > hw/m68k/mcf5208.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/m68k/mcf5208.c b/hw/m68k/mcf5208.c > index ec14096aa43..0ad347dfa81 100644 > --- a/hw/m68k/mcf5208.c > +++ b/hw/m68k/mcf5208.c > @@ -158,7 +158,7 @@ static uint64_t m5208_sys_read(void *opaque, hwaddr addr, > { > int n; > for (n = 0; n < 32; n++) { > - if (current_machine->ram_size < (2u << n)) { > + if (current_machine->ram_size < (2ULL << n)) { > break; > } > } 31 - clz32(ram_size) ? r~
On 9/2/24 07:34, Richard Henderson wrote: >> @@ -158,7 +158,7 @@ static uint64_t m5208_sys_read(void *opaque, hwaddr addr, >> { >> int n; >> for (n = 0; n < 32; n++) { >> - if (current_machine->ram_size < (2u << n)) { >> + if (current_machine->ram_size < (2ULL << n)) { >> break; >> } >> } > > 31 - clz32(ram_size) We might could do with lg2_pow2ceil() in host-utils.h, if that's clearer. r~
diff --git a/hw/m68k/mcf5208.c b/hw/m68k/mcf5208.c index ec14096aa43..0ad347dfa81 100644 --- a/hw/m68k/mcf5208.c +++ b/hw/m68k/mcf5208.c @@ -158,7 +158,7 @@ static uint64_t m5208_sys_read(void *opaque, hwaddr addr, { int n; for (n = 0; n < 32; n++) { - if (current_machine->ram_size < (2u << n)) { + if (current_machine->ram_size < (2ULL << n)) { break; } }
In m5208_sys_read(), we have a loop of n from 0 to 31, and we calculate (2u << n). For the n == 31 iteration this will shift off the top of the unsigned 32 bit integer. This is harmless, because we're going to stop the loop with n == 31 anyway, but we can avoid the error by using 64-bit arithmetic here. (The SDCS0 register is documented at https://www.nxp.com/docs/en/reference-manual/MCF5208RM.pdf section 18.4.5; we want the lower 5 bits to indicate the RAM size, where 31 == 4GB, 30 == 2GB, and so on down. As it happens, the layout of the mcf5208evb board memory map means it doesn't make sense to have more than 1GB of RAM in any case.) Resolves: Coverity CID 1547727 Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/m68k/mcf5208.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)