Hi Peter, On 9/22/23 17:29, Peter Maydell wrote: > The STE_CTXPTR() and STE_S2TTB() macros both extract two halves > of an address from fields in the STE and combine them into a > single value to return. The current code for this uses a GCC > statement expression. There are two problems with this: > > (1) The type chosen for the variable in the statement expr > is 'unsigned long', which might not be 64 bits > > (2) the name chosen for the variable causes -Wshadow warnings > because it's the same as a variable in use at the callsite: > > In file included from ../../hw/arm/smmuv3.c:34: > ../../hw/arm/smmuv3.c: In function ‘smmu_get_cd’: > ../../hw/arm/smmuv3-internal.h:538:23: warning: declaration of ‘addr’ shadows > a previous local [-Wshadow=compatible-local] > 538 | unsigned long addr; \ > | ^~~~ > ../../hw/arm/smmuv3.c:339:23: note: in expansion of macro ‘STE_CTXPTR’ > 339 | dma_addr_t addr = STE_CTXPTR(ste); > | ^~~~~~~~~~ > ../../hw/arm/smmuv3.c:339:16: note: shadowed declaration is here > 339 | dma_addr_t addr = STE_CTXPTR(ste); > | ^~~~ > > Sidestep both of these problems by just using a single > expression rather than a statement expr. > > For CMD_ADDR, we got the type of the variable right but still > run into -Wshadow problems: > > In file included from ../../hw/arm/smmuv3.c:34: > ../../hw/arm/smmuv3.c: In function ‘smmuv3_range_inval’: > ../../hw/arm/smmuv3-internal.h:334:22: warning: declaration of ‘addr’ shadows > a previous local [-Wshadow=compatible-local] > 334 | uint64_t addr = high << 32 | (low << 12); \ > | ^~~~ > ../../hw/arm/smmuv3.c:1104:28: note: in expansion of macro ‘CMD_ADDR’ > 1104 | dma_addr_t end, addr = CMD_ADDR(cmd); > | ^~~~~~~~ > ../../hw/arm/smmuv3.c:1104:21: note: shadowed declaration is here > 1104 | dma_addr_t end, addr = CMD_ADDR(cmd); > | ^~~~ > > so convert it too. > > CD_TTB has neither problem, but it is the only other macro in > the file that uses this pattern, so we convert it also for > consistency's sake. > > We use extract64() rather than extract32() to avoid having > to explicitly cast the result to uint64_t. > > Signed-off-by: Peter Maydell <peter.mayd...@linaro.org> > --- > hw/arm/smmuv3-internal.h | 41 +++++++++++++--------------------------- > 1 file changed, 13 insertions(+), 28 deletions(-) > > diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h > index 6d1c1edab7b..648c2e37a27 100644 > --- a/hw/arm/smmuv3-internal.h > +++ b/hw/arm/smmuv3-internal.h > @@ -328,12 +328,9 @@ enum { /* Command completion notification */ > #define CMD_TTL(x) extract32((x)->word[2], 8 , 2) > #define CMD_TG(x) extract32((x)->word[2], 10, 2) > #define CMD_STE_RANGE(x) extract32((x)->word[2], 0 , 5) > -#define CMD_ADDR(x) ({ \ > - uint64_t high = (uint64_t)(x)->word[3]; \ > - uint64_t low = extract32((x)->word[2], 12, 20); \ > - uint64_t addr = high << 32 | (low << 12); \ > - addr; \ > - }) > +#define CMD_ADDR(x) \ > + (((uint64_t)((x)->word[3]) << 32) | \ > + ((extract64((x)->word[2], 12, 20)) << 12)) > > #define SMMU_FEATURE_2LVL_STE (1 << 0) > > @@ -533,21 +530,13 @@ typedef struct CD { > #define STE_S2S(x) extract32((x)->word[5], 25, 1) > #define STE_S2R(x) extract32((x)->word[5], 26, 1) > > -#define STE_CTXPTR(x) \ > - ({ \ > - unsigned long addr; \ > - addr = (uint64_t)extract32((x)->word[1], 0, 16) << 32; \ > - addr |= (uint64_t)((x)->word[0] & 0xffffffc0); \ > - addr; \ > - }) > +#define STE_CTXPTR(x) \ > + ((extract64((x)->word[1], 0, 16) << 32) | \ > + ((x)->word[0] & 0xffffffc0)) > > -#define STE_S2TTB(x) \ > - ({ \ > - unsigned long addr; \ > - addr = (uint64_t)extract32((x)->word[7], 0, 16) << 32; \ > - addr |= (uint64_t)((x)->word[6] & 0xfffffff0); \ > - addr; \ > - }) > +#define STE_S2TTB(x) \ > + ((extract64((x)->word[7], 0, 16) << 32) | \ > + ((x)->word[6] & 0xfffffff0)) > > static inline int oas2bits(int oas_field) > { > @@ -585,14 +574,10 @@ static inline int pa_range(STE *ste) > > #define CD_VALID(x) extract32((x)->word[0], 31, 1) > #define CD_ASID(x) extract32((x)->word[1], 16, 16) > -#define CD_TTB(x, sel) \ > - ({ \ > - uint64_t hi, lo; \ > - hi = extract32((x)->word[(sel) * 2 + 3], 0, 19); \ > - hi <<= 32; \ > - lo = (x)->word[(sel) * 2 + 2] & ~0xfULL; \ > - hi | lo; \ > - }) > +#define CD_TTB(x, sel) \ > + ((extract64((x)->word[(sel) * 2 + 3], 0, 19) << 32) | \ > + ((x)->word[(sel) * 2 + 2] & ~0xfULL)) > + > #define CD_HAD(x, sel) extract32((x)->word[(sel) * 2 + 2], 1, 1) > > #define CD_TSZ(x, sel) extract32((x)->word[0], (16 * (sel)) + 0, 6) Reviewed-by: Eric Auger <eric.au...@redhat.com>
Eric