| Message ID | 1399305623-22016-4-git-send-email-robherring2@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 5 May 2014 17:00, Rob Herring <robherring2@gmail.com> wrote: > From: Rob Herring <rob.herring@linaro.org> > > Add the infrastructure to handle and emulate hvc and smc exceptions. > This will enable emulation of things such as PSCI calls. This commit > does not change the behavior and will exit with unknown exception. > > Signed-off-by: Rob Herring <rob.herring@linaro.org> > --- > target-arm/cpu-qom.h | 3 +++ > target-arm/cpu.h | 2 ++ > target-arm/helper-a64.c | 11 +++++++++++ > target-arm/helper.c | 33 +++++++++++++++++++++++++++++++++ > target-arm/internals.h | 15 +++++++++++++++ > target-arm/translate-a64.c | 13 ++++++++++--- > target-arm/translate.c | 24 +++++++++++++++++------- > 7 files changed, 91 insertions(+), 10 deletions(-) > > diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h > index 8ccb227..88aaf6a 100644 > --- a/target-arm/cpu-qom.h > +++ b/target-arm/cpu-qom.h > @@ -185,6 +185,9 @@ extern const struct VMStateDescription vmstate_arm_cpu; > void register_cp_regs_for_features(ARMCPU *cpu); > void init_cpreg_list(ARMCPU *cpu); > > +bool arm_cpu_do_hvc(CPUState *cs); > +bool arm_cpu_do_smc(CPUState *cs); > + > void arm_cpu_do_interrupt(CPUState *cpu); > void arm_v7m_cpu_do_interrupt(CPUState *cpu); > > diff --git a/target-arm/cpu.h b/target-arm/cpu.h > index c83f249..905ba02 100644 > --- a/target-arm/cpu.h > +++ b/target-arm/cpu.h > @@ -51,6 +51,8 @@ > #define EXCP_EXCEPTION_EXIT 8 /* Return from v7M exception. */ > #define EXCP_KERNEL_TRAP 9 /* Jumped to kernel code page. */ > #define EXCP_STREX 10 > +#define EXCP_HVC 11 > +#define EXCP_SMC 12 > > #define ARMV7M_EXCP_RESET 1 > #define ARMV7M_EXCP_NMI 2 > diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c > index 84411b4..d2c1097 100644 > --- a/target-arm/helper-a64.c > +++ b/target-arm/helper-a64.c > @@ -485,6 +485,17 @@ void aarch64_cpu_do_interrupt(CPUState *cs) > case EXCP_FIQ: > addr += 0x100; > break; > + case EXCP_HVC: > + if (arm_cpu_do_hvc(cs)) { > + return; > + } > + cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index); > + return; > + case EXCP_SMC: > + if (arm_cpu_do_smc(cs)) { > + return; > + } > + /* Fall-though */ We can't cpu_abort() just because the guest hands us an HVC or SMC that we don't happen to handle in QEMU. We should just qemu_log_mask(LOG_GUEST_ERROR, ...) and then do the architecturally mandated behaviour for SMC or HVC instructions on a CPU which doesn't implement EL2/EL3, ie treat as UnallocatedEncoding(). (Don't forget to fix up exception.syndrome in this case, since it should be syn_uncategorized(), not the SMC/HVC value.) > default: > cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index); (this cpu_abort() is OK because it's really a "can never reach here" assertion.) > } > diff --git a/target-arm/helper.c b/target-arm/helper.c > index 3be917c..b5b4a17 100644 > --- a/target-arm/helper.c > +++ b/target-arm/helper.c > @@ -3253,6 +3253,28 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs) > env->thumb = addr & 1; > } > > +bool arm_cpu_do_hvc(CPUState *cs) > +{ > + bool ret; > + > + ret = arm_handle_psci(cs); > + if (ret) { > + return ret; > + } > + return false; > +} > + > +bool arm_cpu_do_smc(CPUState *cs) > +{ > + bool ret; > + > + ret = arm_handle_psci(cs); > + if (ret) { > + return ret; > + } > + return false; > +} This hunk means the series doesn't compile after this patch is applied. I think in this patch these two functions should both just "return false;" indicating that no HVC or SMC calls have any special handling by QEMU. Then the patch which adds psci.c can also add the calls. > + > /* Handle a CPU exception. */ > void arm_cpu_do_interrupt(CPUState *cs) > { > @@ -3355,6 +3377,17 @@ void arm_cpu_do_interrupt(CPUState *cs) > mask = CPSR_A | CPSR_I | CPSR_F; > offset = 4; > break; > + case EXCP_HVC: > + if (arm_cpu_do_hvc(cs)) { > + return; > + } > + cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index); > + return; > + case EXCP_SMC: > + if (arm_cpu_do_smc(cs)) { > + return; > + } > + /* Fall-though */ Same remarks about cpu_abort() apply here. > default: > cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index); > return; /* Never happens. Keep compiler happy. */ > diff --git a/target-arm/internals.h b/target-arm/internals.h > index d63a975..c71eabb 100644 > --- a/target-arm/internals.h > +++ b/target-arm/internals.h > @@ -184,6 +184,21 @@ static inline uint32_t syn_aa32_svc(uint32_t imm16, bool is_thumb) > | (is_thumb ? 0 : ARM_EL_IL); > } > > +static inline uint32_t syn_aa64_hvc(uint32_t imm16) > +{ > + return (EC_AA64_HVC << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff); > +} > + > +static inline uint32_t syn_aa32_hvc(uint32_t imm16) > +{ > + return (EC_AA32_HVC << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff); > +} > + > +static inline uint32_t syn_aa64_smc(uint32_t imm16) > +{ > + return (EC_AA64_SMC << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff); > +} > + You want a syn_aa32_smc() as well [note that it doesn't take an immediate]. > static inline uint32_t syn_aa64_bkpt(uint32_t imm16) > { > return (EC_AA64_BKPT << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff); > diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c > index b62db4d..fa49ed8 100644 > --- a/target-arm/translate-a64.c > +++ b/target-arm/translate-a64.c > @@ -1449,11 +1449,18 @@ static void disas_exc(DisasContext *s, uint32_t insn) > /* SVC, HVC, SMC; since we don't support the Virtualization > * or TrustZone extensions these all UNDEF except SVC. > */ > - if (op2_ll != 1) { > - unallocated_encoding(s); > + switch (op2_ll) { > + case 1: > + gen_exception_insn(s, 0, EXCP_SWI, syn_aa64_svc(imm16)); > + break; > + case 2: > + gen_exception_insn(s, 0, EXCP_HVC, syn_aa64_smc(imm16)); This should be syn_aa64_hvc()... > + break; > + case 3: > + gen_exception_insn(s, 0, EXCP_SMC, syn_aa64_hvc(imm16)); ...and this should be syn_aa64_smc(). > break; > } > - gen_exception_insn(s, 0, EXCP_SWI, syn_aa64_svc(imm16)); > + unallocated_encoding(s); > break; > case 1: > if (op2_ll != 0) { > diff --git a/target-arm/translate.c b/target-arm/translate.c > index a4d920b..13ece7f 100644 > --- a/target-arm/translate.c > +++ b/target-arm/translate.c > @@ -7727,9 +7727,14 @@ static void disas_arm_insn(CPUARMState * env, DisasContext *s) > case 7: > { > int imm16 = extract32(insn, 0, 4) | (extract32(insn, 8, 12) << 4); > - /* SMC instruction (op1 == 3) > - and undefined instructions (op1 == 0 || op1 == 2) > - will trap */ > + /* HVC and SMC instructions */ > + if (op1 == 2) { > + gen_exception_insn(s, 0, EXCP_HVC, imm16); > + break; > + } else if (op1 == 3) { > + gen_exception_insn(s, 0, EXCP_SMC, 0); > + break; The fourth argument to gen_exception_insn() should be the syndrome register value, so there should be calls to syn_aa32_hvc()/syn_aa32_smc() here. > + } > if (op1 != 1) { > goto illegal_op; > } > @@ -9555,10 +9560,15 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw > goto illegal_op; > > if (insn & (1 << 26)) { > - /* Secure monitor call (v6Z) */ > - qemu_log_mask(LOG_UNIMP, > - "arm: unimplemented secure monitor call\n"); > - goto illegal_op; /* not implemented. */ > + if (!(insn & (1 << 20))) { > + /* Hypervisor call (v7) */ > + uint32_t imm16 = extract32(insn, 16, 4); > + imm16 |= extract32(insn, 0, 12) << 4; This is the wrong way round, I think: imm16 is imm4:imm12, not imm12:imm4. > + gen_exception_insn(s, 0, EXCP_HVC, imm16); > + } else { > + /* Secure monitor call (v6+) */ > + gen_exception_insn(s, 0, EXCP_SMC, 0); Again, missing calls to syn_aa32_hvc()/syn_aa32_smc(). > + } > } else { > op = (insn >> 20) & 7; > switch (op) { > -- > 1.9.1 > thanks -- PMM
On Wed, May 14, 2014 at 12:44 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 5 May 2014 17:00, Rob Herring <robherring2@gmail.com> wrote: >> From: Rob Herring <rob.herring@linaro.org> >> >> Add the infrastructure to handle and emulate hvc and smc exceptions. >> This will enable emulation of things such as PSCI calls. This commit >> does not change the behavior and will exit with unknown exception. [...] >> + case EXCP_HVC: >> + if (arm_cpu_do_hvc(cs)) { >> + return; >> + } >> + cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index); >> + return; >> + case EXCP_SMC: >> + if (arm_cpu_do_smc(cs)) { >> + return; >> + } >> + /* Fall-though */ > > We can't cpu_abort() just because the guest hands us an HVC > or SMC that we don't happen to handle in QEMU. We should > just qemu_log_mask(LOG_GUEST_ERROR, ...) and then do the > architecturally mandated behaviour for SMC or HVC instructions > on a CPU which doesn't implement EL2/EL3, ie treat as > UnallocatedEncoding(). (Don't forget to fix up exception.syndrome > in this case, since it should be syn_uncategorized(), not > the SMC/HVC value.) So I think this and shifting setting ESR/FAR after the switch should be sufficient: case EXCP_SMC: if (arm_cpu_do_smc(cs)) { return; } env->exception.syndrome = syn_uncategorized(); if (is_a64(env)) { env->pc -= 4; } else { env->regs[15] -= env->thumb ? 2 : 4; } break; I'm not completely sure the PC adjustment is correct. >> diff --git a/target-arm/helper.c b/target-arm/helper.c >> index 3be917c..b5b4a17 100644 >> --- a/target-arm/helper.c >> +++ b/target-arm/helper.c >> @@ -3253,6 +3253,28 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs) >> env->thumb = addr & 1; >> } >> >> +bool arm_cpu_do_hvc(CPUState *cs) >> +{ >> + bool ret; >> + >> + ret = arm_handle_psci(cs); >> + if (ret) { >> + return ret; >> + } >> + return false; >> +} >> + >> +bool arm_cpu_do_smc(CPUState *cs) >> +{ >> + bool ret; >> + >> + ret = arm_handle_psci(cs); >> + if (ret) { >> + return ret; >> + } >> + return false; >> +} > > This hunk means the series doesn't compile after this > patch is applied. I think in this patch these two functions > should both just "return false;" indicating that no HVC > or SMC calls have any special handling by QEMU. Then the > patch which adds psci.c can also add the calls. Ah yes, I had it like that and forgot to go back and split it up in the process of rebasing. >> +static inline uint32_t syn_aa64_hvc(uint32_t imm16) >> +{ >> + return (EC_AA64_HVC << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff); >> +} >> + >> +static inline uint32_t syn_aa32_hvc(uint32_t imm16) >> +{ >> + return (EC_AA32_HVC << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff); >> +} >> + >> +static inline uint32_t syn_aa64_smc(uint32_t imm16) >> +{ >> + return (EC_AA64_SMC << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff); >> +} >> + > > You want a syn_aa32_smc() as well [note that it doesn't > take an immediate]. I also noticed I need to set ARM_EL_IL based on thumb or not on the aa32 variants. >> + if (!(insn & (1 << 20))) { >> + /* Hypervisor call (v7) */ >> + uint32_t imm16 = extract32(insn, 16, 4); >> + imm16 |= extract32(insn, 0, 12) << 4; > > This is the wrong way round, I think: imm16 is imm4:imm12, not > imm12:imm4. You're right. ARM and Thumb are reversed. Rob
On 14 May 2014 21:55, Rob Herring <rob.herring@linaro.org> wrote: > On Wed, May 14, 2014 at 12:44 PM, Peter Maydell > <peter.maydell@linaro.org> wrote: >> We can't cpu_abort() just because the guest hands us an HVC >> or SMC that we don't happen to handle in QEMU. We should >> just qemu_log_mask(LOG_GUEST_ERROR, ...) and then do the >> architecturally mandated behaviour for SMC or HVC instructions >> on a CPU which doesn't implement EL2/EL3, ie treat as >> UnallocatedEncoding(). (Don't forget to fix up exception.syndrome >> in this case, since it should be syn_uncategorized(), not >> the SMC/HVC value.) > > So I think this and shifting setting ESR/FAR after the switch should > be sufficient: > > case EXCP_SMC: > if (arm_cpu_do_smc(cs)) { > return; > } > env->exception.syndrome = syn_uncategorized(); > if (is_a64(env)) { > env->pc -= 4; > } else { > env->regs[15] -= env->thumb ? 2 : 4; > } > break; > > I'm not completely sure the PC adjustment is correct. I don't think you want that env->thumb conditional: the SMC instruction is 4 bytes in both the A32 and T32 encodings, so we always need to rewind by 4 bytes. >>> +static inline uint32_t syn_aa64_hvc(uint32_t imm16) >>> +{ >>> + return (EC_AA64_HVC << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff); >>> +} >>> + >>> +static inline uint32_t syn_aa32_hvc(uint32_t imm16) >>> +{ >>> + return (EC_AA32_HVC << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff); >>> +} >>> + >>> +static inline uint32_t syn_aa64_smc(uint32_t imm16) >>> +{ >>> + return (EC_AA64_SMC << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff); >>> +} >>> + >> >> You want a syn_aa32_smc() as well [note that it doesn't >> take an immediate]. > > I also noticed I need to set ARM_EL_IL based on thumb or not on the > aa32 variants. No, for HVC and SMC the Thumb instructions are 32 bit so ARM_EL_IL should be set. (Breakpoint and SVC are different because the T32 BKPT and SVC insn encodings are only 16 bits long. In retrospect calling the syn_aa32_* parameter "is_thumb" rather than "is_16bit" was perhaps misleading.) thanks -- PMM
diff --git a/target-arm/cpu-qom.h b/target-arm/cpu-qom.h index 8ccb227..88aaf6a 100644 --- a/target-arm/cpu-qom.h +++ b/target-arm/cpu-qom.h @@ -185,6 +185,9 @@ extern const struct VMStateDescription vmstate_arm_cpu; void register_cp_regs_for_features(ARMCPU *cpu); void init_cpreg_list(ARMCPU *cpu); +bool arm_cpu_do_hvc(CPUState *cs); +bool arm_cpu_do_smc(CPUState *cs); + void arm_cpu_do_interrupt(CPUState *cpu); void arm_v7m_cpu_do_interrupt(CPUState *cpu); diff --git a/target-arm/cpu.h b/target-arm/cpu.h index c83f249..905ba02 100644 --- a/target-arm/cpu.h +++ b/target-arm/cpu.h @@ -51,6 +51,8 @@ #define EXCP_EXCEPTION_EXIT 8 /* Return from v7M exception. */ #define EXCP_KERNEL_TRAP 9 /* Jumped to kernel code page. */ #define EXCP_STREX 10 +#define EXCP_HVC 11 +#define EXCP_SMC 12 #define ARMV7M_EXCP_RESET 1 #define ARMV7M_EXCP_NMI 2 diff --git a/target-arm/helper-a64.c b/target-arm/helper-a64.c index 84411b4..d2c1097 100644 --- a/target-arm/helper-a64.c +++ b/target-arm/helper-a64.c @@ -485,6 +485,17 @@ void aarch64_cpu_do_interrupt(CPUState *cs) case EXCP_FIQ: addr += 0x100; break; + case EXCP_HVC: + if (arm_cpu_do_hvc(cs)) { + return; + } + cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index); + return; + case EXCP_SMC: + if (arm_cpu_do_smc(cs)) { + return; + } + /* Fall-though */ default: cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index); } diff --git a/target-arm/helper.c b/target-arm/helper.c index 3be917c..b5b4a17 100644 --- a/target-arm/helper.c +++ b/target-arm/helper.c @@ -3253,6 +3253,28 @@ void arm_v7m_cpu_do_interrupt(CPUState *cs) env->thumb = addr & 1; } +bool arm_cpu_do_hvc(CPUState *cs) +{ + bool ret; + + ret = arm_handle_psci(cs); + if (ret) { + return ret; + } + return false; +} + +bool arm_cpu_do_smc(CPUState *cs) +{ + bool ret; + + ret = arm_handle_psci(cs); + if (ret) { + return ret; + } + return false; +} + /* Handle a CPU exception. */ void arm_cpu_do_interrupt(CPUState *cs) { @@ -3355,6 +3377,17 @@ void arm_cpu_do_interrupt(CPUState *cs) mask = CPSR_A | CPSR_I | CPSR_F; offset = 4; break; + case EXCP_HVC: + if (arm_cpu_do_hvc(cs)) { + return; + } + cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index); + return; + case EXCP_SMC: + if (arm_cpu_do_smc(cs)) { + return; + } + /* Fall-though */ default: cpu_abort(cs, "Unhandled exception 0x%x\n", cs->exception_index); return; /* Never happens. Keep compiler happy. */ diff --git a/target-arm/internals.h b/target-arm/internals.h index d63a975..c71eabb 100644 --- a/target-arm/internals.h +++ b/target-arm/internals.h @@ -184,6 +184,21 @@ static inline uint32_t syn_aa32_svc(uint32_t imm16, bool is_thumb) | (is_thumb ? 0 : ARM_EL_IL); } +static inline uint32_t syn_aa64_hvc(uint32_t imm16) +{ + return (EC_AA64_HVC << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff); +} + +static inline uint32_t syn_aa32_hvc(uint32_t imm16) +{ + return (EC_AA32_HVC << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff); +} + +static inline uint32_t syn_aa64_smc(uint32_t imm16) +{ + return (EC_AA64_SMC << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff); +} + static inline uint32_t syn_aa64_bkpt(uint32_t imm16) { return (EC_AA64_BKPT << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff); diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c index b62db4d..fa49ed8 100644 --- a/target-arm/translate-a64.c +++ b/target-arm/translate-a64.c @@ -1449,11 +1449,18 @@ static void disas_exc(DisasContext *s, uint32_t insn) /* SVC, HVC, SMC; since we don't support the Virtualization * or TrustZone extensions these all UNDEF except SVC. */ - if (op2_ll != 1) { - unallocated_encoding(s); + switch (op2_ll) { + case 1: + gen_exception_insn(s, 0, EXCP_SWI, syn_aa64_svc(imm16)); + break; + case 2: + gen_exception_insn(s, 0, EXCP_HVC, syn_aa64_smc(imm16)); + break; + case 3: + gen_exception_insn(s, 0, EXCP_SMC, syn_aa64_hvc(imm16)); break; } - gen_exception_insn(s, 0, EXCP_SWI, syn_aa64_svc(imm16)); + unallocated_encoding(s); break; case 1: if (op2_ll != 0) { diff --git a/target-arm/translate.c b/target-arm/translate.c index a4d920b..13ece7f 100644 --- a/target-arm/translate.c +++ b/target-arm/translate.c @@ -7727,9 +7727,14 @@ static void disas_arm_insn(CPUARMState * env, DisasContext *s) case 7: { int imm16 = extract32(insn, 0, 4) | (extract32(insn, 8, 12) << 4); - /* SMC instruction (op1 == 3) - and undefined instructions (op1 == 0 || op1 == 2) - will trap */ + /* HVC and SMC instructions */ + if (op1 == 2) { + gen_exception_insn(s, 0, EXCP_HVC, imm16); + break; + } else if (op1 == 3) { + gen_exception_insn(s, 0, EXCP_SMC, 0); + break; + } if (op1 != 1) { goto illegal_op; } @@ -9555,10 +9560,15 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw goto illegal_op; if (insn & (1 << 26)) { - /* Secure monitor call (v6Z) */ - qemu_log_mask(LOG_UNIMP, - "arm: unimplemented secure monitor call\n"); - goto illegal_op; /* not implemented. */ + if (!(insn & (1 << 20))) { + /* Hypervisor call (v7) */ + uint32_t imm16 = extract32(insn, 16, 4); + imm16 |= extract32(insn, 0, 12) << 4; + gen_exception_insn(s, 0, EXCP_HVC, imm16); + } else { + /* Secure monitor call (v6+) */ + gen_exception_insn(s, 0, EXCP_SMC, 0); + } } else { op = (insn >> 20) & 7; switch (op) {