On 8 November 2018 at 10:29, Dongjiu Geng <gengdongjiu@xxxxxxxxxx> wrote: > Add synchronous external abort injection logic, setup > exception type and syndrome value. When switch to guest, > guest will jump to the synchronous external abort vector > table entry. > > The ESR_ELx.DFSC is set to synchronous external abort(0x10), > and ESR_ELx.FnV is set to not valid(0x1), which will tell > guest that FAR is not valid and holds an UNKNOWN value. > These value will be set to KVM register structures through > KVM_SET_ONE_REG IOCTL. > > Signed-off-by: Dongjiu Geng <gengdongjiu@xxxxxxxxxx> > --- > Marc is against that KVM inject the synchronous external abort(SEA) in [1], > so user space how to inject it. The test result that injection SEA to guest by Qemu > is shown in [2]. > > [1]: https://lkml.org/lkml/2017/3/2/110 > [2]: > Taking exception 4 [Data Abort] > ...from EL0 to EL1 > ...with ESR 0x24/0x92000410 > ...with FAR 0x0 > ...with ELR 0x40cf04 > ...to EL1 PC 0xffffffc000084c00 PSTATE 0x3c5 > after kvm_inject_arm_sea > Unhandled fault: synchronous external abort (0x92000410) at 0x0000007fa234c12c > CPU: 0 PID: 536 Comm: devmem Not tainted 4.1.0+ #20 > Hardware name: linux,dummy-virt (DT) > task: ffffffc019ab2b00 ti: ffffffc008134000 task.ti: ffffffc008134000 > PC is at 0x40cf04 > LR is at 0x40cdec > pc : [<000000000040cf04>] lr : [<000000000040cdec>] pstate: 60000000 > sp : 0000007ff7b24130 > x29: 0000007ff7b24260 x28: 0000000000000000 > x27: 00000000000000ad x26: 000000000049c000 > x25: 000000000048904b x24: 000000000049c000 > x23: 0000000040600000 x22: 0000007ff7b243a0 > x21: 0000000000000002 x20: 0000000000000000 > x19: 0000000000000020 x18: 0000000000000000 > x17: 000000000049c6d0 x16: 0000007fa22c85c0 > x15: 0000000000005798 x14: 0000007fa2205f1c > x13: 0000007fa241ccb0 x12: 0000000000000137 > x11: 0000000000000000 x10: 0000000000000000 > x9 : 0000000000000000 x8 : 00000000000000de > x7 : 0000000000000000 x6 : 0000000000002000 > x5 : 0000000040600000 x4 : 0000000000000003 > x3 : 0000000000000001 x2 : 0000000000000000 > x1 : 0000000000000000 x0 : 0000007fa2418000 > --- > target/arm/cpu.h | 2 ++ > target/arm/helper.c | 23 +++++++++++++++++++++++ > target/arm/internals.h | 5 +++-- > target/arm/kvm64.c | 39 +++++++++++++++++++++++++++++++++++++++ > target/arm/op_helper.c | 2 +- > 5 files changed, 68 insertions(+), 3 deletions(-) > > diff --git a/target/arm/cpu.h b/target/arm/cpu.h > index b5eff79..502507d 100644 > --- a/target/arm/cpu.h > +++ b/target/arm/cpu.h > @@ -2331,6 +2331,8 @@ bool write_list_to_cpustate(ARMCPU *cpu); > */ > bool write_cpustate_to_list(ARMCPU *cpu); > > +bool write_part_cpustate_to_list(ARMCPU *cpu, ptrdiff_t fieldoffset); > + > #define ARM_CPUID_TI915T 0x54029152 > #define ARM_CPUID_TI925T 0x54029252 > > diff --git a/target/arm/helper.c b/target/arm/helper.c > index 9630193..df078ff 100644 > --- a/target/arm/helper.c > +++ b/target/arm/helper.c > @@ -263,6 +263,29 @@ static bool raw_accessors_invalid(const ARMCPRegInfo *ri) > return true; > } > > +bool write_part_cpustate_to_list(ARMCPU *cpu, ptrdiff_t fieldoffset) > +{ > + const ARMCPRegInfo *ri; > + uint32_t regidx, i; > + > + for (i = 0; i < cpu->cpreg_array_len; i++) { > + regidx = kvm_to_cpreg_id(cpu->cpreg_indexes[i]); > + ri = get_arm_cp_reginfo(cpu->cp_regs, regidx); > + if (!ri) { > + continue; > + } > + > + if (ri->type & ARM_CP_NO_RAW) { > + continue; > + } > + if (ri->fieldoffset == fieldoffset) { > + cpu->cpreg_values[i] = read_raw_cp_reg(&cpu->env, ri); > + return true; > + } > + } > + return false; > +} What is this about? Nothing else in QEMU needs to mess with the cpustate synchronization. My first assumption is that you should not need to do so either. > + > bool write_cpustate_to_list(ARMCPU *cpu) > { > /* Write the coprocessor state from cpu->env to the (index,value) list. */ > diff --git a/target/arm/internals.h b/target/arm/internals.h > index 6c2bb2d..04ea074 100644 > --- a/target/arm/internals.h > +++ b/target/arm/internals.h > @@ -415,13 +415,14 @@ static inline uint32_t syn_insn_abort(int same_el, int ea, int s1ptw, int fsc) > | ARM_EL_IL | (ea << 9) | (s1ptw << 7) | fsc; > } > > -static inline uint32_t syn_data_abort_no_iss(int same_el, > +static inline uint32_t syn_data_abort_no_iss(int same_el, int fnv, > int ea, int cm, int s1ptw, > int wnr, int fsc) > { > return (EC_DATAABORT << ARM_EL_EC_SHIFT) | (same_el << ARM_EL_EC_SHIFT) > | ARM_EL_IL > - | (ea << 9) | (cm << 8) | (s1ptw << 7) | (wnr << 6) | fsc; > + | (fnv << 10) | (ea << 9) | (cm << 8) | (s1ptw << 7) > + | (wnr << 6) | fsc; > } > > static inline uint32_t syn_data_abort_with_iss(int same_el, > diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c > index 5de8ff0..0ca2b29 100644 > --- a/target/arm/kvm64.c > +++ b/target/arm/kvm64.c > @@ -594,6 +594,45 @@ int kvm_arm_cpreg_level(uint64_t regidx) > return KVM_PUT_RUNTIME_STATE; > } > > +/* Inject synchronous external abort */ > +static void kvm_inject_arm_sea(CPUState *c) > +{ > + ARMCPU *cpu = ARM_CPU(c); > + CPUARMState *env = &cpu->env; > + CPUClass *cc = CPU_GET_CLASS(c); > + uint32_t esr; > + int ret; > + > + /* This exception is synchronous data abort */ > + c->exception_index = EXCP_DATA_ABORT; > + /* Inject the exception to guest EL1 */ > + env->exception.target_el = 1; These comments don't tell us anything that the code does not. > + > + /* Set the DFSC to synchronous external abort and set FnV to not valid, > + * this will tell guest the FAR_ELx is UNKNOWN for this abort. > + */ > + > + /* This exception comes from lower or current exception level. */ > + if (arm_current_el(env) == PSTATE_MODE_EL0t) { arm_current_el() returns an integer; you should be comparing it to 0,1,2,3, not to PSTATE_MODE_* constants. > + esr = syn_data_abort_no_iss(0, 1, 0, 0, 0, 0, 0x10); > + } else { > + esr = syn_data_abort_no_iss(1, 1, 0, 0, 0, 0, 0x10); > + } Better written as bool same_el; [...] same_el = arm_current_el(env) == env->exception.target_el; env->exception.syndrome = syn_data_abort_no_iss(same_el, 1, 0, 0, 0, 0, 0x10); > + > + env->exception.syndrome = esr; > + > + cc->do_interrupt(c); You need to take the iothread lock before calling do_interrupt. Compare the code in kvm_arm_handle_debug(). > + if (kvm_enabled()) { How can we get here if KVM is not enabled ? > + /* write ESR_EL1 from cpustate to list*/ > + ret = write_part_cpustate_to_list(cpu, > + offsetof(CPUARMState, cp15.esr_el[1])); > + if (!ret) { > + fprintf(stderr, "<%s> failed to set esr_el1\n", __func__); > + abort(); > + } kvm_arm_handle_debug() doesn't need to do this, I don't understand why this would be different. > + } > +} > + > #define AARCH64_CORE_REG(x) (KVM_REG_ARM64 | KVM_REG_SIZE_U64 | \ > KVM_REG_ARM_CORE | KVM_REG_ARM_CORE_REG(x)) > > diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c > index 90741f6..3f1d656 100644 > --- a/target/arm/op_helper.c > +++ b/target/arm/op_helper.c > @@ -109,7 +109,7 @@ static inline uint32_t merge_syn_data_abort(uint32_t template_syn, > * ISV field. > */ > if (!(template_syn & ARM_EL_ISV) || target_el != 2 || s1ptw) { > - syn = syn_data_abort_no_iss(same_el, > + syn = syn_data_abort_no_iss(same_el, 0, > ea, 0, s1ptw, is_write, fsc); > } else { > /* Fields: IL, ISV, SAS, SSE, SRT, SF and AR come from the template > -- > 1.8.3.1 > thanks -- PMM
