LGTM Reviewed-by: Cameron Esfahani <[email protected] <mailto:[email protected]>>
Cameron > On Feb 9, 2022, at 4:41 AM, Alexander Graf <[email protected]> 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 <[email protected]> > Signed-off-by: Alexander Graf <[email protected]> > --- > 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) >
