Re: [PATCH 4/4] hw/arm/smmuv3-internal.h: Don't use locals in statement macros

[Eric Auger]

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.maydell@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.auger@redhat.com>

Eric