| Message ID | 20220409000742.293691-15-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | target/arm: Implement features Debugv8p4, RAS, IESB | expand |
On Sat, 9 Apr 2022 at 01:18, Richard Henderson <richard.henderson@linaro.org> wrote: > > Check for and defer any pending virtual SError. > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> > --- > target/arm/helper.h | 1 + > target/arm/a32.decode | 16 +++++++++----- > target/arm/t32.decode | 18 +++++++-------- > target/arm/op_helper.c | 45 ++++++++++++++++++++++++++++++++++++++ > target/arm/translate-a64.c | 7 ++++++ > target/arm/translate.c | 10 +++++++++ > 6 files changed, 82 insertions(+), 15 deletions(-) > > diff --git a/target/arm/helper.h b/target/arm/helper.h > index b463d9343b..bb7f901668 100644 > --- a/target/arm/helper.h > +++ b/target/arm/helper.h > @@ -54,6 +54,7 @@ DEF_HELPER_1(wfe, void, env) > DEF_HELPER_1(yield, void, env) > DEF_HELPER_1(pre_hvc, void, env) > DEF_HELPER_2(pre_smc, void, env, i32) > +DEF_HELPER_1(esb, void, env) > > DEF_HELPER_3(cpsr_write, void, env, i32, i32) > DEF_HELPER_2(cpsr_write_eret, void, env, i32) > diff --git a/target/arm/a32.decode b/target/arm/a32.decode > index fcd8cd4f7d..f2ca480949 100644 > --- a/target/arm/a32.decode > +++ b/target/arm/a32.decode > @@ -187,13 +187,17 @@ SMULTT .... 0001 0110 .... 0000 .... 1110 .... @rd0mn > > { > { > - YIELD ---- 0011 0010 0000 1111 ---- 0000 0001 > - WFE ---- 0011 0010 0000 1111 ---- 0000 0010 > - WFI ---- 0011 0010 0000 1111 ---- 0000 0011 > + [ > + YIELD ---- 0011 0010 0000 1111 ---- 0000 0001 > + WFE ---- 0011 0010 0000 1111 ---- 0000 0010 > + WFI ---- 0011 0010 0000 1111 ---- 0000 0011 > > - # TODO: Implement SEV, SEVL; may help SMP performance. > - # SEV ---- 0011 0010 0000 1111 ---- 0000 0100 > - # SEVL ---- 0011 0010 0000 1111 ---- 0000 0101 > + # TODO: Implement SEV, SEVL; may help SMP performance. > + # SEV ---- 0011 0010 0000 1111 ---- 0000 0100 > + # SEVL ---- 0011 0010 0000 1111 ---- 0000 0101 > + > + ESB ---- 0011 0010 0000 1111 ---- 0001 0000 > + ] Why don't we decode bits [11:8] here? I see it's the same as YIELD/WFE/WFI, but I'm not sure why we're not decoding those bits in those insns either... > > # The canonical nop ends in 00000000, but the whole of the > # rest of the space executes as nop if otherwise unsupported. > diff --git a/target/arm/t32.decode b/target/arm/t32.decode > index 78fadef9d6..f21ad0167a 100644 > --- a/target/arm/t32.decode > +++ b/target/arm/t32.decode > @@ -364,17 +364,17 @@ CLZ 1111 1010 1011 ---- 1111 .... 1000 .... @rdm > [ > # Hints, and CPS > { > - YIELD 1111 0011 1010 1111 1000 0000 0000 0001 > - WFE 1111 0011 1010 1111 1000 0000 0000 0010 > - WFI 1111 0011 1010 1111 1000 0000 0000 0011 > + [ > + YIELD 1111 0011 1010 1111 1000 0000 0000 0001 > + WFE 1111 0011 1010 1111 1000 0000 0000 0010 > + WFI 1111 0011 1010 1111 1000 0000 0000 0011 > > - # TODO: Implement SEV, SEVL; may help SMP performance. > - # SEV 1111 0011 1010 1111 1000 0000 0000 0100 > - # SEVL 1111 0011 1010 1111 1000 0000 0000 0101 > + # TODO: Implement SEV, SEVL; may help SMP performance. > + # SEV 1111 0011 1010 1111 1000 0000 0000 0100 > + # SEVL 1111 0011 1010 1111 1000 0000 0000 0101 > > - # For M-profile minimal-RAS ESB can be a NOP, which is the > - # default behaviour since it is in the hint space. > - # ESB 1111 0011 1010 1111 1000 0000 0001 0000 > + ESB 1111 0011 1010 1111 1000 0000 0001 0000 > + ] > > # The canonical nop ends in 0000 0000, but the whole rest > # of the space is "reserved hint, behaves as nop". > diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c > index 70b42b55fd..f50424b301 100644 > --- a/target/arm/op_helper.c > +++ b/target/arm/op_helper.c > @@ -972,3 +972,48 @@ void HELPER(probe_access)(CPUARMState *env, target_ulong ptr, > access_type, mmu_idx, ra); > } > } > + > +void HELPER(esb)(CPUARMState *env) > +{ > + /* > + * QEMU does not have a source of physical SErrors, so we are > + * only concerned with virtual SErrors. > + * > + * During translation, we have already checked: RAS enabled, > + * EL2 present (enabled check done in arm_hcr_el2_eff), and > + * PSTATE.EL in {EL0, EL1}. This function corresponds to > + * AArch64.vESBOperation(), noting that the AArch32 version > + * is not functionally different. > + */ > + uint64_t hcr = arm_hcr_el2_eff(env); > + bool enabled = !(hcr & HCR_TGE) && (hcr & HCR_AMO); > + bool pending = enabled && (hcr & HCR_VSE); > + bool masked = (env->daif & PSTATE_A); > + > + /* If VSE pending and masked, defer the exception. */ > + if (pending && masked) { > + uint32_t syndrome; > + > + if (arm_el_is_aa64(env, 1)) { > + /* Copy across IDS and ISS from VSESR. */ > + syndrome = env->cp15.vsesr_el2 & 0x1ffffff; > + } else { > + ARMMMUFaultInfo fi = { .type = ARMFault_AsyncExternal }; > + > + if (extended_addresses_enabled(env)) { > + syndrome = arm_fi_to_lfsc(&fi); > + } else { > + syndrome = arm_fi_to_sfsc(&fi); > + } > + /* Copy across AET and ExT from VSESR. */ > + syndrome |= env->cp15.vsesr_el2 & 0xd000; > + } > + > + /* Set VDISR_EL2.A along with the syndrome. */ > + env->cp15.vdisr_el2 = syndrome | (1u << 31); > + > + /* Clear pending virtual SError */ > + env->cp15.hcr_el2 &= ~HCR_VSE; > + cpu_reset_interrupt(env_cpu(env), CPU_INTERRUPT_VSERR); > + } > +} > diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c > index 9333d7be41..cc54dff83c 100644 > --- a/target/arm/translate-a64.c > +++ b/target/arm/translate-a64.c > @@ -1469,6 +1469,13 @@ static void handle_hint(DisasContext *s, uint32_t insn, > gen_helper_autib(cpu_X[17], cpu_env, cpu_X[17], cpu_X[16]); > } > break; > + case 0b10000: /* ESB */ > + if (dc_isar_feature(aa64_ras, s) && > + arm_dc_feature(s, ARM_FEATURE_EL2) && > + s->current_el <= 1) { > + gen_helper_esb(cpu_env); > + } > + break; > case 0b11000: /* PACIAZ */ > if (s->pauth_active) { > gen_helper_pacia(cpu_X[30], cpu_env, cpu_X[30], > diff --git a/target/arm/translate.c b/target/arm/translate.c > index bf2196b9e2..b42ca53d99 100644 > --- a/target/arm/translate.c > +++ b/target/arm/translate.c > @@ -6275,6 +6275,16 @@ static bool trans_WFI(DisasContext *s, arg_WFI *a) > return true; > } > > +static bool trans_ESB(DisasContext *s, arg_ESB *a) > +{ > + if (dc_isar_feature(aa32_ras, s) && > + arm_dc_feature(s, ARM_FEATURE_EL2) && > + s->current_el <= 1) { This is doing the right thing for M-profile but only rather indirectly because it happens to get caught by the FEATURE_EL2 check. I think it would be safer to explicitly check for not-M-profile (which then gives you a place to put the "For M-profile minimal-RAS ESB can be a NOP" comment that got removed above). > + gen_helper_esb(cpu_env); > + } > + return true; I think a comment noting that without RAS we must NOP would be useful here. > +} Otherwise Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM
On 4/11/22 09:18, Peter Maydell wrote: >> + ESB ---- 0011 0010 0000 1111 ---- 0001 0000 >> + ] > > Why don't we decode bits [11:8] here? I see it's the same > as YIELD/WFE/WFI, but I'm not sure why we're not decoding > those bits in those insns either... See page F4-7074 in H.a, where bits [11:8] of the imm12 field are described with 'xxxx'. >> +static bool trans_ESB(DisasContext *s, arg_ESB *a) >> +{ >> + if (dc_isar_feature(aa32_ras, s) && >> + arm_dc_feature(s, ARM_FEATURE_EL2) && >> + s->current_el <= 1) { > > This is doing the right thing for M-profile but only rather > indirectly because it happens to get caught by the FEATURE_EL2 > check. Yes, I had though that a feature, reducing the number of checks, but... > I think it would be safer to explicitly check for > not-M-profile (which then gives you a place to put the > "For M-profile minimal-RAS ESB can be a NOP" comment that got > removed above). ... fair enough. > I think a comment noting that without RAS we must NOP would > be useful here. Ok. r~
On Mon, 11 Apr 2022 at 23:14, Richard Henderson <richard.henderson@linaro.org> wrote: > > On 4/11/22 09:18, Peter Maydell wrote: > >> + ESB ---- 0011 0010 0000 1111 ---- 0001 0000 > >> + ] > > > > Why don't we decode bits [11:8] here? I see it's the same > > as YIELD/WFE/WFI, but I'm not sure why we're not decoding > > those bits in those insns either... > > See page F4-7074 in H.a, where bits [11:8] of the imm12 field are described with 'xxxx'. Hmm. That just means "decodes to the NOP/WFI/ESB/whatever instruction-description whatever the value of those bits", but when the specific instruction-description then marks those bits as "(0)" or "(1)", that has the usual CONSTRAINED UNPREDICTABLE meaning described in section F1.7.2, where we get a free choice of UNDEF, NOP, ignore the bit, or any-dest-regs-are-UNKNOWN. So we're within the spec to not decode [11:8] but I think it would be more consistent with how we try to handle those (0) and (1) bits generally if we insist that [11:8] is all zeroes here. For this series, I guess go along with the current way we handle hint instructions, and maybe fix this as a separate cleanup later. -- PMM
On 4/12/22 02:56, Peter Maydell wrote: > On Mon, 11 Apr 2022 at 23:14, Richard Henderson > <richard.henderson@linaro.org> wrote: >> >> On 4/11/22 09:18, Peter Maydell wrote: >>>> + ESB ---- 0011 0010 0000 1111 ---- 0001 0000 >>>> + ] >>> >>> Why don't we decode bits [11:8] here? I see it's the same >>> as YIELD/WFE/WFI, but I'm not sure why we're not decoding >>> those bits in those insns either... >> >> See page F4-7074 in H.a, where bits [11:8] of the imm12 field are described with 'xxxx'. > > Hmm. That just means "decodes to the NOP/WFI/ESB/whatever > instruction-description whatever the value of those bits", > but when the specific instruction-description then marks > those bits as "(0)" or "(1)", that has the usual CONSTRAINED > UNPREDICTABLE meaning described in section F1.7.2, where > we get a free choice of UNDEF, NOP, ignore the bit, or > any-dest-regs-are-UNKNOWN. So we're within the spec to > not decode [11:8] but I think it would be more consistent > with how we try to handle those (0) and (1) bits generally > if we insist that [11:8] is all zeroes here. > > For this series, I guess go along with the current way we > handle hint instructions, and maybe fix this as a separate > cleanup later. Ok. r~
diff --git a/target/arm/helper.h b/target/arm/helper.h index b463d9343b..bb7f901668 100644 --- a/target/arm/helper.h +++ b/target/arm/helper.h @@ -54,6 +54,7 @@ DEF_HELPER_1(wfe, void, env) DEF_HELPER_1(yield, void, env) DEF_HELPER_1(pre_hvc, void, env) DEF_HELPER_2(pre_smc, void, env, i32) +DEF_HELPER_1(esb, void, env) DEF_HELPER_3(cpsr_write, void, env, i32, i32) DEF_HELPER_2(cpsr_write_eret, void, env, i32) diff --git a/target/arm/a32.decode b/target/arm/a32.decode index fcd8cd4f7d..f2ca480949 100644 --- a/target/arm/a32.decode +++ b/target/arm/a32.decode @@ -187,13 +187,17 @@ SMULTT .... 0001 0110 .... 0000 .... 1110 .... @rd0mn { { - YIELD ---- 0011 0010 0000 1111 ---- 0000 0001 - WFE ---- 0011 0010 0000 1111 ---- 0000 0010 - WFI ---- 0011 0010 0000 1111 ---- 0000 0011 + [ + YIELD ---- 0011 0010 0000 1111 ---- 0000 0001 + WFE ---- 0011 0010 0000 1111 ---- 0000 0010 + WFI ---- 0011 0010 0000 1111 ---- 0000 0011 - # TODO: Implement SEV, SEVL; may help SMP performance. - # SEV ---- 0011 0010 0000 1111 ---- 0000 0100 - # SEVL ---- 0011 0010 0000 1111 ---- 0000 0101 + # TODO: Implement SEV, SEVL; may help SMP performance. + # SEV ---- 0011 0010 0000 1111 ---- 0000 0100 + # SEVL ---- 0011 0010 0000 1111 ---- 0000 0101 + + ESB ---- 0011 0010 0000 1111 ---- 0001 0000 + ] # The canonical nop ends in 00000000, but the whole of the # rest of the space executes as nop if otherwise unsupported. diff --git a/target/arm/t32.decode b/target/arm/t32.decode index 78fadef9d6..f21ad0167a 100644 --- a/target/arm/t32.decode +++ b/target/arm/t32.decode @@ -364,17 +364,17 @@ CLZ 1111 1010 1011 ---- 1111 .... 1000 .... @rdm [ # Hints, and CPS { - YIELD 1111 0011 1010 1111 1000 0000 0000 0001 - WFE 1111 0011 1010 1111 1000 0000 0000 0010 - WFI 1111 0011 1010 1111 1000 0000 0000 0011 + [ + YIELD 1111 0011 1010 1111 1000 0000 0000 0001 + WFE 1111 0011 1010 1111 1000 0000 0000 0010 + WFI 1111 0011 1010 1111 1000 0000 0000 0011 - # TODO: Implement SEV, SEVL; may help SMP performance. - # SEV 1111 0011 1010 1111 1000 0000 0000 0100 - # SEVL 1111 0011 1010 1111 1000 0000 0000 0101 + # TODO: Implement SEV, SEVL; may help SMP performance. + # SEV 1111 0011 1010 1111 1000 0000 0000 0100 + # SEVL 1111 0011 1010 1111 1000 0000 0000 0101 - # For M-profile minimal-RAS ESB can be a NOP, which is the - # default behaviour since it is in the hint space. - # ESB 1111 0011 1010 1111 1000 0000 0001 0000 + ESB 1111 0011 1010 1111 1000 0000 0001 0000 + ] # The canonical nop ends in 0000 0000, but the whole rest # of the space is "reserved hint, behaves as nop". diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c index 70b42b55fd..f50424b301 100644 --- a/target/arm/op_helper.c +++ b/target/arm/op_helper.c @@ -972,3 +972,48 @@ void HELPER(probe_access)(CPUARMState *env, target_ulong ptr, access_type, mmu_idx, ra); } } + +void HELPER(esb)(CPUARMState *env) +{ + /* + * QEMU does not have a source of physical SErrors, so we are + * only concerned with virtual SErrors. + * + * During translation, we have already checked: RAS enabled, + * EL2 present (enabled check done in arm_hcr_el2_eff), and + * PSTATE.EL in {EL0, EL1}. This function corresponds to + * AArch64.vESBOperation(), noting that the AArch32 version + * is not functionally different. + */ + uint64_t hcr = arm_hcr_el2_eff(env); + bool enabled = !(hcr & HCR_TGE) && (hcr & HCR_AMO); + bool pending = enabled && (hcr & HCR_VSE); + bool masked = (env->daif & PSTATE_A); + + /* If VSE pending and masked, defer the exception. */ + if (pending && masked) { + uint32_t syndrome; + + if (arm_el_is_aa64(env, 1)) { + /* Copy across IDS and ISS from VSESR. */ + syndrome = env->cp15.vsesr_el2 & 0x1ffffff; + } else { + ARMMMUFaultInfo fi = { .type = ARMFault_AsyncExternal }; + + if (extended_addresses_enabled(env)) { + syndrome = arm_fi_to_lfsc(&fi); + } else { + syndrome = arm_fi_to_sfsc(&fi); + } + /* Copy across AET and ExT from VSESR. */ + syndrome |= env->cp15.vsesr_el2 & 0xd000; + } + + /* Set VDISR_EL2.A along with the syndrome. */ + env->cp15.vdisr_el2 = syndrome | (1u << 31); + + /* Clear pending virtual SError */ + env->cp15.hcr_el2 &= ~HCR_VSE; + cpu_reset_interrupt(env_cpu(env), CPU_INTERRUPT_VSERR); + } +} diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c index 9333d7be41..cc54dff83c 100644 --- a/target/arm/translate-a64.c +++ b/target/arm/translate-a64.c @@ -1469,6 +1469,13 @@ static void handle_hint(DisasContext *s, uint32_t insn, gen_helper_autib(cpu_X[17], cpu_env, cpu_X[17], cpu_X[16]); } break; + case 0b10000: /* ESB */ + if (dc_isar_feature(aa64_ras, s) && + arm_dc_feature(s, ARM_FEATURE_EL2) && + s->current_el <= 1) { + gen_helper_esb(cpu_env); + } + break; case 0b11000: /* PACIAZ */ if (s->pauth_active) { gen_helper_pacia(cpu_X[30], cpu_env, cpu_X[30], diff --git a/target/arm/translate.c b/target/arm/translate.c index bf2196b9e2..b42ca53d99 100644 --- a/target/arm/translate.c +++ b/target/arm/translate.c @@ -6275,6 +6275,16 @@ static bool trans_WFI(DisasContext *s, arg_WFI *a) return true; } +static bool trans_ESB(DisasContext *s, arg_ESB *a) +{ + if (dc_isar_feature(aa32_ras, s) && + arm_dc_feature(s, ARM_FEATURE_EL2) && + s->current_el <= 1) { + gen_helper_esb(cpu_env); + } + return true; +} + static bool trans_NOP(DisasContext *s, arg_NOP *a) { return true;
Check for and defer any pending virtual SError. Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- target/arm/helper.h | 1 + target/arm/a32.decode | 16 +++++++++----- target/arm/t32.decode | 18 +++++++-------- target/arm/op_helper.c | 45 ++++++++++++++++++++++++++++++++++++++ target/arm/translate-a64.c | 7 ++++++ target/arm/translate.c | 10 +++++++++ 6 files changed, 82 insertions(+), 15 deletions(-)