| Message ID | 20181030170353.7447-1-peter.maydell@linaro.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [for-3.1] hw/ppc/ppc440_uc: Remove dead code in sdram_size() | expand |
On Tue, 30 Oct 2018, Peter Maydell wrote: > Coverity points out in CID 1390588 that the test for sh == 0 > in sdram_size() can never fire, because we calculate sh with > sh = 1024 - ((bcr >> 6) & 0x3ff); > which must result in a value between 1 and 1024 inclusive. > > Without the relevant manual for the SoC, we're not completely > sure of the correct behaviour here, but we can remove the > dead code without changing how QEMU currently behaves. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > We had a discussion about this coverity error a while back: > https://lists.nongnu.org/archive/html/qemu-devel/2018-04/msg05187.html > I'd just like to squash the Coverity warning, I think. As I told back then, feel free to go ahead and avoid the warning if it's in your way. As for the proper fix, I'm still not sure how it works on real SoC but the current formula is likely wrong and this U-Boot commit may provide some hints instead: https://git.denx.de/?p=u-boot.git;a=commitdiff;h=8ac41e3e37c3080c6b1d9461d654161cfe2aa492 Here RAM sizes are encoded in some strange way and I think this function (and the ppc460ex_sdram_bank_sizes array in sam460ex.c) might need to be changed to work according to this but I don't get this enough to be able to write a patch that I'm confident won't break anything else. (Also this is the reason why RAM size is limited to 1GB on the sam460ex emulation currently so fixing this would also allow RAM up to 4 GB as on real hardware.) If this makes more sense to someone or there's someone who knows how it works on the 460EX SoC, help is appreciated as I likely won't be able to spend enough time to try to understand and fix this based on just the U-Boot source without more proper docs anytime soon. Regards, BALATON Zoltan > hw/ppc/ppc440_uc.c | 6 +----- > 1 file changed, 1 insertion(+), 5 deletions(-) > > diff --git a/hw/ppc/ppc440_uc.c b/hw/ppc/ppc440_uc.c > index 09ccda548f3..9360f781cef 100644 > --- a/hw/ppc/ppc440_uc.c > +++ b/hw/ppc/ppc440_uc.c > @@ -559,11 +559,7 @@ static target_ulong sdram_size(uint32_t bcr) > int sh; > > sh = 1024 - ((bcr >> 6) & 0x3ff); > - if (sh == 0) { > - size = -1; > - } else { > - size = 8 * MiB * sh; > - } > + size = 8 * MiB * sh; > > return size; > } >
On 30/10/2018 18:03, Peter Maydell wrote: > Coverity points out in CID 1390588 that the test for sh == 0 > in sdram_size() can never fire, because we calculate sh with > sh = 1024 - ((bcr >> 6) & 0x3ff); > which must result in a value between 1 and 1024 inclusive. > > Without the relevant manual for the SoC, we're not completely > sure of the correct behaviour here, but we can remove the > dead code without changing how QEMU currently behaves. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > We had a discussion about this coverity error a while back: > https://lists.nongnu.org/archive/html/qemu-devel/2018-04/msg05187.html > I'd just like to squash the Coverity warning, I think. > > hw/ppc/ppc440_uc.c | 6 +----- > 1 file changed, 1 insertion(+), 5 deletions(-) > > diff --git a/hw/ppc/ppc440_uc.c b/hw/ppc/ppc440_uc.c > index 09ccda548f3..9360f781cef 100644 > --- a/hw/ppc/ppc440_uc.c > +++ b/hw/ppc/ppc440_uc.c > @@ -559,11 +559,7 @@ static target_ulong sdram_size(uint32_t bcr) > int sh; > > sh = 1024 - ((bcr >> 6) & 0x3ff); > - if (sh == 0) { > - size = -1; > - } else { > - size = 8 * MiB * sh; > - } > + size = 8 * MiB * sh; > > return size; > } > Reviewed-by: Laurent Vivier <lvivier@redhat.com>
On Tue, Oct 30, 2018 at 05:03:53PM +0000, Peter Maydell wrote: > Coverity points out in CID 1390588 that the test for sh == 0 > in sdram_size() can never fire, because we calculate sh with > sh = 1024 - ((bcr >> 6) & 0x3ff); > which must result in a value between 1 and 1024 inclusive. > > Without the relevant manual for the SoC, we're not completely > sure of the correct behaviour here, but we can remove the > dead code without changing how QEMU currently behaves. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Applied to ppc-for-3.1, thanks. > --- > We had a discussion about this coverity error a while back: > https://lists.nongnu.org/archive/html/qemu-devel/2018-04/msg05187.html > I'd just like to squash the Coverity warning, I think. > > hw/ppc/ppc440_uc.c | 6 +----- > 1 file changed, 1 insertion(+), 5 deletions(-) > > diff --git a/hw/ppc/ppc440_uc.c b/hw/ppc/ppc440_uc.c > index 09ccda548f3..9360f781cef 100644 > --- a/hw/ppc/ppc440_uc.c > +++ b/hw/ppc/ppc440_uc.c > @@ -559,11 +559,7 @@ static target_ulong sdram_size(uint32_t bcr) > int sh; > > sh = 1024 - ((bcr >> 6) & 0x3ff); > - if (sh == 0) { > - size = -1; > - } else { > - size = 8 * MiB * sh; > - } > + size = 8 * MiB * sh; > > return size; > } -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson
diff --git a/hw/ppc/ppc440_uc.c b/hw/ppc/ppc440_uc.c index 09ccda548f3..9360f781cef 100644 --- a/hw/ppc/ppc440_uc.c +++ b/hw/ppc/ppc440_uc.c @@ -559,11 +559,7 @@ static target_ulong sdram_size(uint32_t bcr) int sh; sh = 1024 - ((bcr >> 6) & 0x3ff); - if (sh == 0) { - size = -1; - } else { - size = 8 * MiB * sh; - } + size = 8 * MiB * sh; return size; }
Coverity points out in CID 1390588 that the test for sh == 0 in sdram_size() can never fire, because we calculate sh with sh = 1024 - ((bcr >> 6) & 0x3ff); which must result in a value between 1 and 1024 inclusive. Without the relevant manual for the SoC, we're not completely sure of the correct behaviour here, but we can remove the dead code without changing how QEMU currently behaves. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- We had a discussion about this coverity error a while back: https://lists.nongnu.org/archive/html/qemu-devel/2018-04/msg05187.html I'd just like to squash the Coverity warning, I think. hw/ppc/ppc440_uc.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) -- 2.19.1