On 05/16/2018 01:23 PM, Peter Maydell wrote: > On 16 May 2018 at 16:16, Philippe Mathieu-Daudé <f4...@amsat.org> wrote: >> Hi Eric, >> >> On 05/16/2018 03:03 PM, Eric Auger wrote: >>> Coverity points out that this can overflow if n > 31, >>> because it's only doing 32-bit arithmetic. Let's use 1ULL instead >>> of 1. Also the formulae used to compute n can be replaced by >>> the level_shift() macro. >> >> This level_shift() replacement doesn't seems that obvious to me, can you >> split it in another patch? >> >>> >>> Reported-by: Peter Maydell <peter.mayd...@linaro.org> >>> Signed-off-by: Eric Auger <eric.au...@redhat.com> >>> --- >>> hw/arm/smmu-common.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c >>> index 01c7be8..3c5f724 100644 >>> --- a/hw/arm/smmu-common.c >>> +++ b/hw/arm/smmu-common.c >>> @@ -83,9 +83,9 @@ static inline hwaddr get_table_pte_address(uint64_t pte, >>> int granule_sz) >>> static inline hwaddr get_block_pte_address(uint64_t pte, int level, >>> int granule_sz, uint64_t *bsz) >>> { >>> - int n = (granule_sz - 3) * (4 - level) + 3; >>> + int n = level_shift(level, granule_sz); >> >> Shouldn't this be level_shift(level + 1, granule_sz)? > > No. The two expressions are equivalent, they're > just arranged differently: > > level_shift(lvl, gsz) > == gsz + (3 - lvl) * (gsz - 3) > == gsz + (4 - lvl) * (gsz - 3) - (gsz - 3) > == gsz - gsz + (4 - lvl) * (gsz - 3) + 3 > == (gsz - 3) * (4 - lvl) + 3
Argh I failed this middle school demonstrations... Thanks Peter :) So for the much cleaner level_shift() use: Reviewed-by: Philippe Mathieu-Daudé <f4...@amsat.org>