diff mbox series

[v3,1/2] hvf: arm: Use macros for sysreg shift/masking

Message ID 20220209124135.69183-1-agraf@csgraf.de (mailing list archive)
State New, archived
Headers show
Series [v3,1/2] hvf: arm: Use macros for sysreg shift/masking | expand

Commit Message

Alexander Graf Feb. 9, 2022, 12:41 p.m. UTC
We are parsing the syndrome field for sysregs in multiple places across
the hvf code, but repeat shift/mask operations with hard coded constants
every time. This is an error prone approach and makes it harder to reason
about the correctness of these operations.

Let's introduce macros that allow us to unify the constants used as well
as create new helpers to extract fields from the sysreg value.

Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Alexander Graf <agraf@csgraf.de>
---
 target/arm/hvf/hvf.c | 69 ++++++++++++++++++++++++++++++--------------
 1 file changed, 47 insertions(+), 22 deletions(-)

Comments

Cameron Esfahani Feb. 11, 2022, 8:13 a.m. UTC | #1
LGTM

Reviewed-by: Cameron Esfahani <dirty@apple.com <mailto:dirty@apple.com>>

Cameron

> On Feb 9, 2022, at 4:41 AM, Alexander Graf <agraf@csgraf.de> wrote:
> 
> We are parsing the syndrome field for sysregs in multiple places across
> the hvf code, but repeat shift/mask operations with hard coded constants
> every time. This is an error prone approach and makes it harder to reason
> about the correctness of these operations.
> 
> Let's introduce macros that allow us to unify the constants used as well
> as create new helpers to extract fields from the sysreg value.
> 
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Alexander Graf <agraf@csgraf.de>
> ---
> target/arm/hvf/hvf.c | 69 ++++++++++++++++++++++++++++++--------------
> 1 file changed, 47 insertions(+), 22 deletions(-)
> 
> diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
> index 92ad0d29c4..8d0447ab01 100644
> --- a/target/arm/hvf/hvf.c
> +++ b/target/arm/hvf/hvf.c
> @@ -35,9 +35,34 @@
>         ENCODE_AA64_CP_REG(CP_REG_ARM64_SYSREG_CP, crn, crm, op0, op1, op2)
> #define PL1_WRITE_MASK 0x4
> 
> +#define SYSREG_OP0_SHIFT      20
> +#define SYSREG_OP0_MASK       0x3
> +#define SYSREG_OP0(sysreg)    ((sysreg >> SYSREG_OP0_SHIFT) & SYSREG_OP0_MASK)
> +#define SYSREG_OP1_SHIFT      14
> +#define SYSREG_OP1_MASK       0x7
> +#define SYSREG_OP1(sysreg)    ((sysreg >> SYSREG_OP1_SHIFT) & SYSREG_OP1_MASK)
> +#define SYSREG_CRN_SHIFT      10
> +#define SYSREG_CRN_MASK       0xf
> +#define SYSREG_CRN(sysreg)    ((sysreg >> SYSREG_CRN_SHIFT) & SYSREG_CRN_MASK)
> +#define SYSREG_CRM_SHIFT      1
> +#define SYSREG_CRM_MASK       0xf
> +#define SYSREG_CRM(sysreg)    ((sysreg >> SYSREG_CRM_SHIFT) & SYSREG_CRM_MASK)
> +#define SYSREG_OP2_SHIFT      17
> +#define SYSREG_OP2_MASK       0x7
> +#define SYSREG_OP2(sysreg)    ((sysreg >> SYSREG_OP2_SHIFT) & SYSREG_OP2_MASK)
> +
> #define SYSREG(op0, op1, crn, crm, op2) \
> -    ((op0 << 20) | (op2 << 17) | (op1 << 14) | (crn << 10) | (crm << 1))
> -#define SYSREG_MASK           SYSREG(0x3, 0x7, 0xf, 0xf, 0x7)
> +    ((op0 << SYSREG_OP0_SHIFT) | \
> +     (op1 << SYSREG_OP1_SHIFT) | \
> +     (crn << SYSREG_CRN_SHIFT) | \
> +     (crm << SYSREG_CRM_SHIFT) | \
> +     (op2 << SYSREG_OP2_SHIFT))
> +#define SYSREG_MASK \
> +    SYSREG(SYSREG_OP0_MASK, \
> +           SYSREG_OP1_MASK, \
> +           SYSREG_CRN_MASK, \
> +           SYSREG_CRM_MASK, \
> +           SYSREG_OP2_MASK)
> #define SYSREG_OSLAR_EL1      SYSREG(2, 0, 1, 0, 4)
> #define SYSREG_OSLSR_EL1      SYSREG(2, 0, 1, 1, 4)
> #define SYSREG_OSDLR_EL1      SYSREG(2, 0, 1, 3, 4)
> @@ -783,21 +808,21 @@ static int hvf_sysreg_read(CPUState *cpu, uint32_t reg, uint32_t rt)
>     default:
>         cpu_synchronize_state(cpu);
>         trace_hvf_unhandled_sysreg_read(env->pc, reg,
> -                                        (reg >> 20) & 0x3,
> -                                        (reg >> 14) & 0x7,
> -                                        (reg >> 10) & 0xf,
> -                                        (reg >> 1) & 0xf,
> -                                        (reg >> 17) & 0x7);
> +                                        SYSREG_OP0(reg),
> +                                        SYSREG_OP1(reg),
> +                                        SYSREG_CRN(reg),
> +                                        SYSREG_CRM(reg),
> +                                        SYSREG_OP2(reg));
>         hvf_raise_exception(cpu, EXCP_UDEF, syn_uncategorized());
>         return 1;
>     }
> 
>     trace_hvf_sysreg_read(reg,
> -                          (reg >> 20) & 0x3,
> -                          (reg >> 14) & 0x7,
> -                          (reg >> 10) & 0xf,
> -                          (reg >> 1) & 0xf,
> -                          (reg >> 17) & 0x7,
> +                          SYSREG_OP0(reg),
> +                          SYSREG_OP1(reg),
> +                          SYSREG_CRN(reg),
> +                          SYSREG_CRM(reg),
> +                          SYSREG_OP2(reg),
>                           val);
>     hvf_set_reg(cpu, rt, val);
> 
> @@ -886,11 +911,11 @@ static int hvf_sysreg_write(CPUState *cpu, uint32_t reg, uint64_t val)
>     CPUARMState *env = &arm_cpu->env;
> 
>     trace_hvf_sysreg_write(reg,
> -                           (reg >> 20) & 0x3,
> -                           (reg >> 14) & 0x7,
> -                           (reg >> 10) & 0xf,
> -                           (reg >> 1) & 0xf,
> -                           (reg >> 17) & 0x7,
> +                           SYSREG_OP0(reg),
> +                           SYSREG_OP1(reg),
> +                           SYSREG_CRN(reg),
> +                           SYSREG_CRM(reg),
> +                           SYSREG_OP2(reg),
>                            val);
> 
>     switch (reg) {
> @@ -960,11 +985,11 @@ static int hvf_sysreg_write(CPUState *cpu, uint32_t reg, uint64_t val)
>     default:
>         cpu_synchronize_state(cpu);
>         trace_hvf_unhandled_sysreg_write(env->pc, reg,
> -                                         (reg >> 20) & 0x3,
> -                                         (reg >> 14) & 0x7,
> -                                         (reg >> 10) & 0xf,
> -                                         (reg >> 1) & 0xf,
> -                                         (reg >> 17) & 0x7);
> +                                         SYSREG_OP0(reg),
> +                                         SYSREG_OP1(reg),
> +                                         SYSREG_CRN(reg),
> +                                         SYSREG_CRM(reg),
> +                                         SYSREG_OP2(reg));
>         hvf_raise_exception(cpu, EXCP_UDEF, syn_uncategorized());
>         return 1;
>     }
> -- 
> 2.32.0 (Apple Git-132)
>
Philippe Mathieu-Daudé Feb. 11, 2022, 9:40 a.m. UTC | #2
On 9/2/22 13:41, Alexander Graf wrote:
> We are parsing the syndrome field for sysregs in multiple places across
> the hvf code, but repeat shift/mask operations with hard coded constants
> every time. This is an error prone approach and makes it harder to reason
> about the correctness of these operations.
> 
> Let's introduce macros that allow us to unify the constants used as well
> as create new helpers to extract fields from the sysreg value.
> 
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Alexander Graf <agraf@csgraf.de>
> ---
>   target/arm/hvf/hvf.c | 69 ++++++++++++++++++++++++++++++--------------
>   1 file changed, 47 insertions(+), 22 deletions(-)
> 
> diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
> index 92ad0d29c4..8d0447ab01 100644
> --- a/target/arm/hvf/hvf.c
> +++ b/target/arm/hvf/hvf.c
> @@ -35,9 +35,34 @@
>           ENCODE_AA64_CP_REG(CP_REG_ARM64_SYSREG_CP, crn, crm, op0, op1, op2)
>   #define PL1_WRITE_MASK 0x4
>   
> +#define SYSREG_OP0_SHIFT      20
> +#define SYSREG_OP0_MASK       0x3
> +#define SYSREG_OP0(sysreg)    ((sysreg >> SYSREG_OP0_SHIFT) & SYSREG_OP0_MASK)
> +#define SYSREG_OP1_SHIFT      14
> +#define SYSREG_OP1_MASK       0x7
> +#define SYSREG_OP1(sysreg)    ((sysreg >> SYSREG_OP1_SHIFT) & SYSREG_OP1_MASK)
> +#define SYSREG_CRN_SHIFT      10
> +#define SYSREG_CRN_MASK       0xf
> +#define SYSREG_CRN(sysreg)    ((sysreg >> SYSREG_CRN_SHIFT) & SYSREG_CRN_MASK)
> +#define SYSREG_CRM_SHIFT      1
> +#define SYSREG_CRM_MASK       0xf
> +#define SYSREG_CRM(sysreg)    ((sysreg >> SYSREG_CRM_SHIFT) & SYSREG_CRM_MASK)
> +#define SYSREG_OP2_SHIFT      17
> +#define SYSREG_OP2_MASK       0x7
> +#define SYSREG_OP2(sysreg)    ((sysreg >> SYSREG_OP2_SHIFT) & SYSREG_OP2_MASK)

Alternatively easier to read using the "hw/registerfields.h" macros:

FIELD(SYSREG, OP0, 20, 2)
FIELD(SYSREG, OP2, 17, 3)
FIELD(SYSREG, OP1, 14, 3)
FIELD(SYSREG, CRN, 10, 4)
FIELD(SYSREG, CRM,  1, 4)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Peter Maydell Feb. 16, 2022, 5:35 p.m. UTC | #3
On Wed, 9 Feb 2022 at 12:41, Alexander Graf <agraf@csgraf.de> wrote:
>
> We are parsing the syndrome field for sysregs in multiple places across
> the hvf code, but repeat shift/mask operations with hard coded constants
> every time. This is an error prone approach and makes it harder to reason
> about the correctness of these operations.
>
> Let's introduce macros that allow us to unify the constants used as well
> as create new helpers to extract fields from the sysreg value.
>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Alexander Graf <agraf@csgraf.de>

Applied this and patch 2/2 to target-arm.next, thanks.

(If you can send multi-patch sets with a cover letter that helps the
automated tools, by the way.)

-- PMM
diff mbox series

Patch

diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
index 92ad0d29c4..8d0447ab01 100644
--- a/target/arm/hvf/hvf.c
+++ b/target/arm/hvf/hvf.c
@@ -35,9 +35,34 @@ 
         ENCODE_AA64_CP_REG(CP_REG_ARM64_SYSREG_CP, crn, crm, op0, op1, op2)
 #define PL1_WRITE_MASK 0x4
 
+#define SYSREG_OP0_SHIFT      20
+#define SYSREG_OP0_MASK       0x3
+#define SYSREG_OP0(sysreg)    ((sysreg >> SYSREG_OP0_SHIFT) & SYSREG_OP0_MASK)
+#define SYSREG_OP1_SHIFT      14
+#define SYSREG_OP1_MASK       0x7
+#define SYSREG_OP1(sysreg)    ((sysreg >> SYSREG_OP1_SHIFT) & SYSREG_OP1_MASK)
+#define SYSREG_CRN_SHIFT      10
+#define SYSREG_CRN_MASK       0xf
+#define SYSREG_CRN(sysreg)    ((sysreg >> SYSREG_CRN_SHIFT) & SYSREG_CRN_MASK)
+#define SYSREG_CRM_SHIFT      1
+#define SYSREG_CRM_MASK       0xf
+#define SYSREG_CRM(sysreg)    ((sysreg >> SYSREG_CRM_SHIFT) & SYSREG_CRM_MASK)
+#define SYSREG_OP2_SHIFT      17
+#define SYSREG_OP2_MASK       0x7
+#define SYSREG_OP2(sysreg)    ((sysreg >> SYSREG_OP2_SHIFT) & SYSREG_OP2_MASK)
+
 #define SYSREG(op0, op1, crn, crm, op2) \
-    ((op0 << 20) | (op2 << 17) | (op1 << 14) | (crn << 10) | (crm << 1))
-#define SYSREG_MASK           SYSREG(0x3, 0x7, 0xf, 0xf, 0x7)
+    ((op0 << SYSREG_OP0_SHIFT) | \
+     (op1 << SYSREG_OP1_SHIFT) | \
+     (crn << SYSREG_CRN_SHIFT) | \
+     (crm << SYSREG_CRM_SHIFT) | \
+     (op2 << SYSREG_OP2_SHIFT))
+#define SYSREG_MASK \
+    SYSREG(SYSREG_OP0_MASK, \
+           SYSREG_OP1_MASK, \
+           SYSREG_CRN_MASK, \
+           SYSREG_CRM_MASK, \
+           SYSREG_OP2_MASK)
 #define SYSREG_OSLAR_EL1      SYSREG(2, 0, 1, 0, 4)
 #define SYSREG_OSLSR_EL1      SYSREG(2, 0, 1, 1, 4)
 #define SYSREG_OSDLR_EL1      SYSREG(2, 0, 1, 3, 4)
@@ -783,21 +808,21 @@  static int hvf_sysreg_read(CPUState *cpu, uint32_t reg, uint32_t rt)
     default:
         cpu_synchronize_state(cpu);
         trace_hvf_unhandled_sysreg_read(env->pc, reg,
-                                        (reg >> 20) & 0x3,
-                                        (reg >> 14) & 0x7,
-                                        (reg >> 10) & 0xf,
-                                        (reg >> 1) & 0xf,
-                                        (reg >> 17) & 0x7);
+                                        SYSREG_OP0(reg),
+                                        SYSREG_OP1(reg),
+                                        SYSREG_CRN(reg),
+                                        SYSREG_CRM(reg),
+                                        SYSREG_OP2(reg));
         hvf_raise_exception(cpu, EXCP_UDEF, syn_uncategorized());
         return 1;
     }
 
     trace_hvf_sysreg_read(reg,
-                          (reg >> 20) & 0x3,
-                          (reg >> 14) & 0x7,
-                          (reg >> 10) & 0xf,
-                          (reg >> 1) & 0xf,
-                          (reg >> 17) & 0x7,
+                          SYSREG_OP0(reg),
+                          SYSREG_OP1(reg),
+                          SYSREG_CRN(reg),
+                          SYSREG_CRM(reg),
+                          SYSREG_OP2(reg),
                           val);
     hvf_set_reg(cpu, rt, val);
 
@@ -886,11 +911,11 @@  static int hvf_sysreg_write(CPUState *cpu, uint32_t reg, uint64_t val)
     CPUARMState *env = &arm_cpu->env;
 
     trace_hvf_sysreg_write(reg,
-                           (reg >> 20) & 0x3,
-                           (reg >> 14) & 0x7,
-                           (reg >> 10) & 0xf,
-                           (reg >> 1) & 0xf,
-                           (reg >> 17) & 0x7,
+                           SYSREG_OP0(reg),
+                           SYSREG_OP1(reg),
+                           SYSREG_CRN(reg),
+                           SYSREG_CRM(reg),
+                           SYSREG_OP2(reg),
                            val);
 
     switch (reg) {
@@ -960,11 +985,11 @@  static int hvf_sysreg_write(CPUState *cpu, uint32_t reg, uint64_t val)
     default:
         cpu_synchronize_state(cpu);
         trace_hvf_unhandled_sysreg_write(env->pc, reg,
-                                         (reg >> 20) & 0x3,
-                                         (reg >> 14) & 0x7,
-                                         (reg >> 10) & 0xf,
-                                         (reg >> 1) & 0xf,
-                                         (reg >> 17) & 0x7);
+                                         SYSREG_OP0(reg),
+                                         SYSREG_OP1(reg),
+                                         SYSREG_CRN(reg),
+                                         SYSREG_CRM(reg),
+                                         SYSREG_OP2(reg));
         hvf_raise_exception(cpu, EXCP_UDEF, syn_uncategorized());
         return 1;
     }