Replace arm_hcr_el2_{fmo,imo,amo} with a more general routine
that also takes SCR_EL3.NS (aka arm_is_secure_below_el3) into
account, as documented for the plethora of bits in HCR_EL2.
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
target/arm/cpu.h | 67 +++++++++------------------------------
hw/intc/arm_gicv3_cpuif.c | 21 ++++++------
target/arm/helper.c | 65 +++++++++++++++++++++++++++++++------
3 files changed, 82 insertions(+), 71 deletions(-)
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 20d97b66de..e871b946c8 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -1722,6 +1722,14 @@ static inline bool arm_is_secure(CPUARMState *env)
}
#endif
+/**
+ * arm_hcr_el2_eff(): Return the effective value of HCR_EL2.
+ * E.g. when in secure state, fields in HCR_EL2 are suppressed,
+ * "for all purposes other than a direct read or write access of HCR_EL2."
+ * Not included here are RW, VI, VF.
+ */
+uint64_t arm_hcr_el2_eff(CPUARMState *env);
+
/* Return true if the specified exception level is running in AArch64 state. */
static inline bool arm_el_is_aa64(CPUARMState *env, int el)
{
@@ -2407,54 +2415,6 @@ bool write_cpustate_to_list(ARMCPU *cpu);
# define TARGET_VIRT_ADDR_SPACE_BITS 32
#endif
-/**
- * arm_hcr_el2_imo(): Return the effective value of HCR_EL2.IMO.
- * Depending on the values of HCR_EL2.E2H and TGE, this may be
- * "behaves as 1 for all purposes other than direct read/write" or
- * "behaves as 0 for all purposes other than direct read/write"
- */
-static inline bool arm_hcr_el2_imo(CPUARMState *env)
-{
- switch (env->cp15.hcr_el2 & (HCR_TGE | HCR_E2H)) {
- case HCR_TGE:
- return true;
- case HCR_TGE | HCR_E2H:
- return false;
- default:
- return env->cp15.hcr_el2 & HCR_IMO;
- }
-}
-
-/**
- * arm_hcr_el2_fmo(): Return the effective value of HCR_EL2.FMO.
- */
-static inline bool arm_hcr_el2_fmo(CPUARMState *env)
-{
- switch (env->cp15.hcr_el2 & (HCR_TGE | HCR_E2H)) {
- case HCR_TGE:
- return true;
- case HCR_TGE | HCR_E2H:
- return false;
- default:
- return env->cp15.hcr_el2 & HCR_FMO;
- }
-}
-
-/**
- * arm_hcr_el2_amo(): Return the effective value of HCR_EL2.AMO.
- */
-static inline bool arm_hcr_el2_amo(CPUARMState *env)
-{
- switch (env->cp15.hcr_el2 & (HCR_TGE | HCR_E2H)) {
- case HCR_TGE:
- return true;
- case HCR_TGE | HCR_E2H:
- return false;
- default:
- return env->cp15.hcr_el2 & HCR_AMO;
- }
-}
-
static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx,
unsigned int target_el)
{
@@ -2463,6 +2423,7 @@ static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx,
bool secure = arm_is_secure(env);
bool pstate_unmasked;
int8_t unmasked = 0;
+ uint64_t hcr_el2;
/* Don't take exceptions if they target a lower EL.
* This check should catch any exceptions that would not be taken but left
@@ -2472,6 +2433,8 @@ static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx,
return false;
}
+ hcr_el2 = arm_hcr_el2_eff(env);
+
switch (excp_idx) {
case EXCP_FIQ:
pstate_unmasked = !(env->daif & PSTATE_F);
@@ -2482,13 +2445,13 @@ static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx,
break;
case EXCP_VFIQ:
- if (secure || !arm_hcr_el2_fmo(env) || (env->cp15.hcr_el2 & HCR_TGE)) {
+ if (secure || !(hcr_el2 & HCR_FMO) || (hcr_el2 & HCR_TGE)) {
/* VFIQs are only taken when hypervized and non-secure. */
return false;
}
return !(env->daif & PSTATE_F);
case EXCP_VIRQ:
- if (secure || !arm_hcr_el2_imo(env) || (env->cp15.hcr_el2 & HCR_TGE)) {
+ if (secure || !(hcr_el2 & HCR_IMO) || (hcr_el2 & HCR_TGE)) {
/* VIRQs are only taken when hypervized and non-secure. */
return false;
}
@@ -2527,7 +2490,7 @@ static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx,
* to the CPSR.F setting otherwise we further assess the state
* below.
*/
- hcr = arm_hcr_el2_fmo(env);
+ hcr = hcr_el2 & HCR_FMO;
scr = (env->cp15.scr_el3 & SCR_FIQ);
/* When EL3 is 32-bit, the SCR.FW bit controls whether the
@@ -2544,7 +2507,7 @@ static inline bool arm_excp_unmasked(CPUState *cs, unsigned int excp_idx,
* when setting the target EL, so it does not have a further
* affect here.
*/
- hcr = arm_hcr_el2_imo(env);
+ hcr = hcr_el2 & HCR_IMO;
scr = false;
break;
default:
diff --git a/hw/intc/arm_gicv3_cpuif.c b/hw/intc/arm_gicv3_cpuif.c
index 068a8e8e9b..cbad6037f1 100644
--- a/hw/intc/arm_gicv3_cpuif.c
+++ b/hw/intc/arm_gicv3_cpuif.c
@@ -85,8 +85,8 @@ static bool icv_access(CPUARMState *env, int hcr_flags)
* * access if NS EL1 and either IMO or FMO == 1:
* CTLR, DIR, PMR, RPR
*/
- bool flagmatch = ((hcr_flags & HCR_IMO) && arm_hcr_el2_imo(env)) ||
- ((hcr_flags & HCR_FMO) && arm_hcr_el2_fmo(env));
+ uint64_t hcr_el2 = arm_hcr_el2_eff(env);
+ bool flagmatch = hcr_el2 & hcr_flags & (HCR_IMO | HCR_FMO);
return flagmatch && arm_current_el(env) == 1
&& !arm_is_secure_below_el3(env);
@@ -1552,8 +1552,9 @@ static void icc_dir_write(CPUARMState *env, const ARMCPRegInfo *ri,
/* No need to include !IsSecure in route_*_to_el2 as it's only
* tested in cases where we know !IsSecure is true.
*/
- route_fiq_to_el2 = arm_hcr_el2_fmo(env);
- route_irq_to_el2 = arm_hcr_el2_imo(env);
+ uint64_t hcr_el2 = arm_hcr_el2_eff(env);
+ route_fiq_to_el2 = hcr_el2 & HCR_FMO;
+ route_irq_to_el2 = hcr_el2 & HCR_IMO;
switch (arm_current_el(env)) {
case 3:
@@ -1895,8 +1896,8 @@ static CPAccessResult gicv3_irqfiq_access(CPUARMState *env,
if ((env->cp15.scr_el3 & (SCR_FIQ | SCR_IRQ)) == (SCR_FIQ | SCR_IRQ)) {
switch (el) {
case 1:
- if (arm_is_secure_below_el3(env) ||
- (arm_hcr_el2_imo(env) == 0 && arm_hcr_el2_fmo(env) == 0)) {
+ /* Note that arm_hcr_el2_eff takes secure state into account. */
+ if ((arm_hcr_el2_eff(env) & (HCR_IMO | HCR_FMO)) == 0) {
r = CP_ACCESS_TRAP_EL3;
}
break;
@@ -1936,8 +1937,8 @@ static CPAccessResult gicv3_dir_access(CPUARMState *env,
static CPAccessResult gicv3_sgi_access(CPUARMState *env,
const ARMCPRegInfo *ri, bool isread)
{
- if ((arm_hcr_el2_imo(env) || arm_hcr_el2_fmo(env)) &&
- arm_current_el(env) == 1 && !arm_is_secure_below_el3(env)) {
+ if (arm_current_el(env) == 1 &&
+ (arm_hcr_el2_eff(env) & (HCR_IMO | HCR_FMO)) != 0) {
/* Takes priority over a possible EL3 trap */
return CP_ACCESS_TRAP_EL2;
}
@@ -1961,7 +1962,7 @@ static CPAccessResult gicv3_fiq_access(CPUARMState *env,
if (env->cp15.scr_el3 & SCR_FIQ) {
switch (el) {
case 1:
- if (arm_is_secure_below_el3(env) || !arm_hcr_el2_fmo(env)) {
+ if ((arm_hcr_el2_eff(env) & HCR_FMO) == 0) {
r = CP_ACCESS_TRAP_EL3;
}
break;
@@ -2000,7 +2001,7 @@ static CPAccessResult gicv3_irq_access(CPUARMState *env,
if (env->cp15.scr_el3 & SCR_IRQ) {
switch (el) {
case 1:
- if (arm_is_secure_below_el3(env) || !arm_hcr_el2_imo(env)) {
+ if ((arm_hcr_el2_eff(env) & HCR_IMO) == 0) {
r = CP_ACCESS_TRAP_EL3;
}
break;
diff --git a/target/arm/helper.c b/target/arm/helper.c
index bf020364e1..5874c5a73f 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -1327,9 +1327,10 @@ static void csselr_write(CPUARMState *env, const ARMCPRegInfo *ri,
static uint64_t isr_read(CPUARMState *env, const ARMCPRegInfo *ri)
{
CPUState *cs = ENV_GET_CPU(env);
+ uint64_t hcr_el2 = arm_hcr_el2_eff(env);
uint64_t ret = 0;
- if (arm_hcr_el2_imo(env)) {
+ if (hcr_el2 & HCR_IMO) {
if (cs->interrupt_request & CPU_INTERRUPT_VIRQ) {
ret |= CPSR_I;
}
@@ -1339,7 +1340,7 @@ static uint64_t isr_read(CPUARMState *env, const ARMCPRegInfo *ri)
}
}
- if (arm_hcr_el2_fmo(env)) {
+ if (hcr_el2 & HCR_FMO) {
if (cs->interrupt_request & CPU_INTERRUPT_VFIQ) {
ret |= CPSR_F;
}
@@ -3991,6 +3992,50 @@ static void hcr_writelow(CPUARMState *env, const ARMCPRegInfo *ri,
hcr_write(env, NULL, value);
}
+/*
+ * Return the effective value of HCR_EL2.
+ * Bits that are not included here:
+ * RW (read from SCR_EL3.RW as needed)
+ */
+uint64_t arm_hcr_el2_eff(CPUARMState *env)
+{
+ uint64_t ret = env->cp15.hcr_el2;
+
+ if (arm_is_secure_below_el3(env)) {
+ /*
+ * "This register has no effect if EL2 is not enabled in the
+ * current Security state". This is ARMv8.4-SecEL2 speak for
+ * !(SCR_EL3.NS==1 || SCR_EL3.EEL2==1).
+ *
+ * Prior to that, the language was "In an implementation that
+ * includes EL3, when the value of SCR_EL3.NS is 0 the PE behaves
+ * as if this field is 0 for all purposes other than a direct
+ * read or write access of HCR_EL2". With lots of enumeration
+ * on a per-field basis. In current QEMU, this is condition
+ * is arm_is_secure_below_el3.
+ *
+ * Since the v8.4 language applies to the entire register, and
+ * appears to be backward compatible, use that.
+ */
+ ret = 0;
+ } else if (ret & HCR_TGE) {
+ if (ret & HCR_E2H) {
+ ret &= ~(HCR_TTLBOS | HCR_TTLBIS | HCR_TOCU | HCR_TICAB |
+ HCR_TID4 | HCR_ID | HCR_CD | HCR_TDZ | HCR_TPU |
+ HCR_TPCP | HCR_TID2 | HCR_TID1 | HCR_TID0 | HCR_TWE |
+ HCR_TWI | HCR_DC | HCR_BSU_MASK | HCR_FB | HCR_AMO |
+ HCR_IMO | HCR_FMO | HCR_VM);
+ } else {
+ ret &= ~(HCR_PTW | HCR_FB | HCR_TID1 | HCR_TID3 | HCR_TSC |
+ HCR_TACR | HCR_TSW | HCR_TTLB | HCR_TVM | HCR_HCD |
+ HCR_TRVM | HCR_TLOR);
+ ret |= HCR_FMO | HCR_IMO | HCR_AMO;
+ }
+ }
+
+ return ret;
+}
+
static const ARMCPRegInfo el2_cp_reginfo[] = {
{ .name = "HCR_EL2", .state = ARM_CP_STATE_AA64,
.type = ARM_CP_IO,
@@ -6505,12 +6550,13 @@ uint32_t arm_phys_excp_target_el(CPUState *cs, uint32_t excp_idx,
uint32_t cur_el, bool secure)
{
CPUARMState *env = cs->env_ptr;
- int rw;
- int scr;
- int hcr;
+ bool rw;
+ bool scr;
+ bool hcr;
int target_el;
/* Is the highest EL AArch64? */
- int is64 = arm_feature(env, ARM_FEATURE_AARCH64);
+ bool is64 = arm_feature(env, ARM_FEATURE_AARCH64);
+ uint64_t hcr_el2;
if (arm_feature(env, ARM_FEATURE_EL3)) {
rw = ((env->cp15.scr_el3 & SCR_RW) == SCR_RW);
@@ -6522,18 +6568,19 @@ uint32_t arm_phys_excp_target_el(CPUState *cs, uint32_t excp_idx,
rw = is64;
}
+ hcr_el2 = arm_hcr_el2_eff(env);
switch (excp_idx) {
case EXCP_IRQ:
scr = ((env->cp15.scr_el3 & SCR_IRQ) == SCR_IRQ);
- hcr = arm_hcr_el2_imo(env);
+ hcr = hcr_el2 & HCR_IMO;
break;
case EXCP_FIQ:
scr = ((env->cp15.scr_el3 & SCR_FIQ) == SCR_FIQ);
- hcr = arm_hcr_el2_fmo(env);
+ hcr = hcr_el2 & HCR_FMO;
break;
default:
scr = ((env->cp15.scr_el3 & SCR_EA) == SCR_EA);
- hcr = arm_hcr_el2_amo(env);
+ hcr = hcr_el2 & HCR_AMO;
break;
};
--
2.17.2
On Mon, 3 Dec 2018 at 20:38, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Replace arm_hcr_el2_{fmo,imo,amo} with a more general routine
> that also takes SCR_EL3.NS (aka arm_is_secure_below_el3) into
> account, as documented for the plethora of bits in HCR_EL2.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> target/arm/cpu.h | 67 +++++++++------------------------------
> hw/intc/arm_gicv3_cpuif.c | 21 ++++++------
> target/arm/helper.c | 65 +++++++++++++++++++++++++++++++------
> 3 files changed, 82 insertions(+), 71 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 20d97b66de..e871b946c8 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -1722,6 +1722,14 @@ static inline bool arm_is_secure(CPUARMState *env)
> }
> #endif
>
> +/**
> + * arm_hcr_el2_eff(): Return the effective value of HCR_EL2.
> + * E.g. when in secure state, fields in HCR_EL2 are suppressed,
> + * "for all purposes other than a direct read or write access of HCR_EL2."
> + * Not included here are RW, VI, VF.
> + */
> +uint64_t arm_hcr_el2_eff(CPUARMState *env);
This comment says the not-included bits are RW, VI, VF...
> +/*
> + * Return the effective value of HCR_EL2.
> + * Bits that are not included here:
> + * RW (read from SCR_EL3.RW as needed)
> + */
...but this comment only lists RW. Which is correct ?
Also, if VI, VF are in the list shouldn't VSE be too ?
> +uint64_t arm_hcr_el2_eff(CPUARMState *env)
> +{
> + uint64_t ret = env->cp15.hcr_el2;
> +
> + if (arm_is_secure_below_el3(env)) {
> + /*
> + * "This register has no effect if EL2 is not enabled in the
> + * current Security state". This is ARMv8.4-SecEL2 speak for
> + * !(SCR_EL3.NS==1 || SCR_EL3.EEL2==1).
> + *
> + * Prior to that, the language was "In an implementation that
> + * includes EL3, when the value of SCR_EL3.NS is 0 the PE behaves
> + * as if this field is 0 for all purposes other than a direct
> + * read or write access of HCR_EL2". With lots of enumeration
> + * on a per-field basis. In current QEMU, this is condition
> + * is arm_is_secure_below_el3.
> + *
> + * Since the v8.4 language applies to the entire register, and
> + * appears to be backward compatible, use that.
> + */
> + ret = 0;
> + } else if (ret & HCR_TGE) {
> + if (ret & HCR_E2H) {
> + ret &= ~(HCR_TTLBOS | HCR_TTLBIS | HCR_TOCU | HCR_TICAB |
> + HCR_TID4 | HCR_ID | HCR_CD | HCR_TDZ | HCR_TPU |
> + HCR_TPCP | HCR_TID2 | HCR_TID1 | HCR_TID0 | HCR_TWE |
> + HCR_TWI | HCR_DC | HCR_BSU_MASK | HCR_FB | HCR_AMO |
> + HCR_IMO | HCR_FMO | HCR_VM);
This list should also in theory include HCR_MIOCNCE, but the
QEMU implementation of that bit is going to be RAZ/WI anyway.
HCR_TID1 is in the "ignore if TGE==1" group, not the "ignore if
{E2H, TGE} == {1,1}" group.
Maybe we should be also setting HCR_ATA here ? We could perhaps
leave that til we implement ARMv8.5-MemTag and understand it better.
> + } else {
> + ret &= ~(HCR_PTW | HCR_FB | HCR_TID1 | HCR_TID3 | HCR_TSC |
> + HCR_TACR | HCR_TSW | HCR_TTLB | HCR_TVM | HCR_HCD |
> + HCR_TRVM | HCR_TLOR);
Shouldn't these bits be being cleared regardless of E2H, rather
than only in the TGE==1 E2H==0 case ?
Missing HCR_SWIO ?
The list of bits cleared if TGE && E2H is highest-first, but this
list is lowest-first...
> + ret |= HCR_FMO | HCR_IMO | HCR_AMO;
> + }
> + }
> +
> + return ret;
> +}
Patch looks good otherwise.
thanks
-- PMM
On 12/6/18 6:09 AM, Peter Maydell wrote:
> On Mon, 3 Dec 2018 at 20:38, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>>
>> Replace arm_hcr_el2_{fmo,imo,amo} with a more general routine
>> that also takes SCR_EL3.NS (aka arm_is_secure_below_el3) into
>> account, as documented for the plethora of bits in HCR_EL2.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>> target/arm/cpu.h | 67 +++++++++------------------------------
>> hw/intc/arm_gicv3_cpuif.c | 21 ++++++------
>> target/arm/helper.c | 65 +++++++++++++++++++++++++++++++------
>> 3 files changed, 82 insertions(+), 71 deletions(-)
>>
>> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
>> index 20d97b66de..e871b946c8 100644
>> --- a/target/arm/cpu.h
>> +++ b/target/arm/cpu.h
>> @@ -1722,6 +1722,14 @@ static inline bool arm_is_secure(CPUARMState *env)
>> }
>> #endif
>>
>> +/**
>> + * arm_hcr_el2_eff(): Return the effective value of HCR_EL2.
>> + * E.g. when in secure state, fields in HCR_EL2 are suppressed,
>> + * "for all purposes other than a direct read or write access of HCR_EL2."
>> + * Not included here are RW, VI, VF.
>> + */
>> +uint64_t arm_hcr_el2_eff(CPUARMState *env);
>
> This comment says the not-included bits are RW, VI, VF...
>
>> +/*
>> + * Return the effective value of HCR_EL2.
>> + * Bits that are not included here:
>> + * RW (read from SCR_EL3.RW as needed)
>> + */
>
> ...but this comment only lists RW. Which is correct ?
>
> Also, if VI, VF are in the list shouldn't VSE be too ?
VI and VF were on the list when I first wrote the patch, then one of the
target-arm.next patches rearranged how those bits are handled.
So correct is that they are *not* excluded.
>> + } else if (ret & HCR_TGE) {
>> + if (ret & HCR_E2H) {
>> + ret &= ~(HCR_TTLBOS | HCR_TTLBIS | HCR_TOCU | HCR_TICAB |
>> + HCR_TID4 | HCR_ID | HCR_CD | HCR_TDZ | HCR_TPU |
>> + HCR_TPCP | HCR_TID2 | HCR_TID1 | HCR_TID0 | HCR_TWE |
>> + HCR_TWI | HCR_DC | HCR_BSU_MASK | HCR_FB | HCR_AMO |
>> + HCR_IMO | HCR_FMO | HCR_VM);
>
>
> This list should also in theory include HCR_MIOCNCE, but the
> QEMU implementation of that bit is going to be RAZ/WI anyway.
Even if we do RAZ/WI, it's probably better to match the docs and exclude it
here. Fixed.
>
> HCR_TID1 is in the "ignore if TGE==1" group, not the "ignore if
> {E2H, TGE} == {1,1}" group.
Fixed.
>
> Maybe we should be also setting HCR_ATA here ? We could perhaps
> leave that til we implement ARMv8.5-MemTag and understand it better.
This list was created with the ARMv8.4 document.
Perhaps a comment to that effect while v8.5 is still in flight?
>
>> + } else {
>> + ret &= ~(HCR_PTW | HCR_FB | HCR_TID1 | HCR_TID3 | HCR_TSC |
>> + HCR_TACR | HCR_TSW | HCR_TTLB | HCR_TVM | HCR_HCD |
>> + HCR_TRVM | HCR_TLOR);
>
> Shouldn't these bits be being cleared regardless of E2H, rather
> than only in the TGE==1 E2H==0 case ?
Fixed.
> Missing HCR_SWIO ?
Fixed.
> The list of bits cleared if TGE && E2H is highest-first, but this
> list is lowest-first...
Both lists now lowest first.
r~
© 2016 - 2025 Red Hat, Inc.