[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 4/4] hw/arm/smmuv3-internal.h: Don't use locals in statement
From: |
Eric Auger |
Subject: |
Re: [PATCH 4/4] hw/arm/smmuv3-internal.h: Don't use locals in statement macros |
Date: |
Tue, 26 Sep 2023 17:00:51 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.13.0 |
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
- [PATCH 1/4] hw/intc/arm_gicv3_its: Avoid shadowing variable in do_process_its_cmd(), (continued)
- [PATCH 1/4] hw/intc/arm_gicv3_its: Avoid shadowing variable in do_process_its_cmd(), Peter Maydell, 2023/09/22
- [PATCH 2/4] hw/misc/arm_sysctl.c: Avoid shadowing local variable, Peter Maydell, 2023/09/22
- [PATCH 3/4] hw/arm/smmuv3.c: Avoid shadowing variable, Peter Maydell, 2023/09/22
- [PATCH 4/4] hw/arm/smmuv3-internal.h: Don't use locals in statement macros, Peter Maydell, 2023/09/22
- Re: [PATCH 4/4] hw/arm/smmuv3-internal.h: Don't use locals in statement macros,
Eric Auger <=
- Re: [PATCH 0/4] arm: fix some -Wshadow warnings, Markus Armbruster, 2023/09/29