| Message ID | 20190906083152.25716-6-zhengxiang9@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add ARMv8 RAS virtualization support in QEMU | expand |
On Fri, 6 Sep 2019 at 09:33, Xiang Zheng <zhengxiang9@huawei.com> wrote: > > From: Dongjiu Geng <gengdongjiu@huawei.com> > > Introduce kvm_inject_arm_sea() function in which we will setup the type > of exception and the syndrome information in order to inject a virtual > synchronous external abort. When switching to guest, it 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 hold an UNKNOWN value. These values will be set to KVM > register structures through KVM_SET_ONE_REG IOCTL. > > Signed-off-by: Dongjiu Geng <gengdongjiu@huawei.com> > Signed-off-by: Xiang Zheng <zhengxiang9@huawei.com> > +/* Inject synchronous external abort */ > +static void kvm_inject_arm_sea(CPUState *c) This will cause a compilation failure at this point in the patch series, because the compiler will complain about a static function which is defined but never used. To avoid breaking bisection, we need to put the definition of the function in the same patch where it's used. > +{ > + ARMCPU *cpu = ARM_CPU(c); > + CPUARMState *env = &cpu->env; > + CPUClass *cc = CPU_GET_CLASS(c); > + uint32_t esr; > + bool same_el; > + > + /** > + * Set the exception type to synchronous data abort > + * and the target exception Level to EL1. > + */ This comment doesn't really tell us anything that's not obvious from the two lines of code that it's commenting on: > + c->exception_index = EXCP_DATA_ABORT; > + env->exception.target_el = 1; > + > + /* > + * 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. */ This comment too is stating the obvious I think. > + same_el = arm_current_el(env) == env->exception.target_el; > + esr = syn_data_abort_no_iss(same_el, 1, 0, 0, 0, 0, 0x10); > + > + env->exception.syndrome = esr; > + > + /** There's a stray second '*' in this comment-start. > + * The vcpu thread already hold BQL, so no need hold again when > + * calling do_interrupt I think this requirement would be better placed as a comment at the top of the function noting that callers must hold the iothread lock. > + */ > + cc->do_interrupt(c); > +} > + > #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/tlb_helper.c b/target/arm/tlb_helper.c > index 5feb312941..499672ebbc 100644 > --- a/target/arm/tlb_helper.c > +++ b/target/arm/tlb_helper.c > @@ -33,7 +33,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 { > /* > -- > 2.19.1 thanks -- PMM
On 2019/9/27 21:33, Peter Maydell wrote: > On Fri, 6 Sep 2019 at 09:33, Xiang Zheng <zhengxiang9@huawei.com> wrote: >> >> From: Dongjiu Geng <gengdongjiu@huawei.com> >> >> Introduce kvm_inject_arm_sea() function in which we will setup the type >> of exception and the syndrome information in order to inject a virtual >> synchronous external abort. When switching to guest, it 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 hold an UNKNOWN value. These values will be set to KVM >> register structures through KVM_SET_ONE_REG IOCTL. >> >> Signed-off-by: Dongjiu Geng <gengdongjiu@huawei.com> >> Signed-off-by: Xiang Zheng <zhengxiang9@huawei.com> > >> +/* Inject synchronous external abort */ >> +static void kvm_inject_arm_sea(CPUState *c) > > This will cause a compilation failure at this point in > the patch series, because the compiler will complain about > a static function which is defined but never used. > To avoid breaking bisection, we need to put the definition > of the function in the same patch where it's used. Thanks, I will merge this patch with the next patch. > >> +{ >> + ARMCPU *cpu = ARM_CPU(c); >> + CPUARMState *env = &cpu->env; >> + CPUClass *cc = CPU_GET_CLASS(c); >> + uint32_t esr; >> + bool same_el; >> + >> + /** >> + * Set the exception type to synchronous data abort >> + * and the target exception Level to EL1. >> + */ > > This comment doesn't really tell us anything that's not obvious > from the two lines of code that it's commenting on: Yes, I will remove this comment. > >> + c->exception_index = EXCP_DATA_ABORT; >> + env->exception.target_el = 1; >> + >> + /* >> + * 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. */ > > This comment too is stating the obvious I think. I will remove it too. > >> + same_el = arm_current_el(env) == env->exception.target_el; >> + esr = syn_data_abort_no_iss(same_el, 1, 0, 0, 0, 0, 0x10); >> + >> + env->exception.syndrome = esr; >> + >> + /** > > There's a stray second '*' in this comment-start. OK, I will remove this stray '*'. > > >> + * The vcpu thread already hold BQL, so no need hold again when >> + * calling do_interrupt > > I think this requirement would be better placed as a > comment at the top of the function noting that callers > must hold the iothread lock. OK, I will add the comment at the top of the function. > >> + */ >> + cc->do_interrupt(c); >> +} >> + >> #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/tlb_helper.c b/target/arm/tlb_helper.c >> index 5feb312941..499672ebbc 100644 >> --- a/target/arm/tlb_helper.c >> +++ b/target/arm/tlb_helper.c >> @@ -33,7 +33,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 { >> /* >> -- >> 2.19.1 > > thanks > -- PMM > > . >
diff --git a/target/arm/helper.c b/target/arm/helper.c index 507026c915..a13baeb085 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -3005,7 +3005,7 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t value, * Report exception with ESR indicating a fault due to a * translation table walk for a cache maintenance instruction. */ - syn = syn_data_abort_no_iss(current_el == target_el, + syn = syn_data_abort_no_iss(current_el == target_el, 0, fi.ea, 1, fi.s1ptw, 1, fsc); env->exception.vaddress = value; env->exception.fsr = fsr; diff --git a/target/arm/internals.h b/target/arm/internals.h index 232d963875..98cde702ad 100644 --- a/target/arm/internals.h +++ b/target/arm/internals.h @@ -451,13 +451,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 28f6db57d5..bf6edaa3f6 100644 --- a/target/arm/kvm64.c +++ b/target/arm/kvm64.c @@ -710,6 +710,40 @@ 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; + bool same_el; + + /** + * Set the exception type to synchronous data abort + * and the target exception Level to EL1. + */ + c->exception_index = EXCP_DATA_ABORT; + env->exception.target_el = 1; + + /* + * 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. */ + same_el = arm_current_el(env) == env->exception.target_el; + esr = syn_data_abort_no_iss(same_el, 1, 0, 0, 0, 0, 0x10); + + env->exception.syndrome = esr; + + /** + * The vcpu thread already hold BQL, so no need hold again when + * calling do_interrupt + */ + cc->do_interrupt(c); +} + #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/tlb_helper.c b/target/arm/tlb_helper.c index 5feb312941..499672ebbc 100644 --- a/target/arm/tlb_helper.c +++ b/target/arm/tlb_helper.c @@ -33,7 +33,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 { /*