Re: [Qemu-devel] [PATCH 3/7] target-arm: add hvc and smc exception emulation handling infrastructure

[Peter Maydell]

On 5 May 2014 17:00, Rob Herring <address@hidden> wrote:
> From: Rob Herring <address@hidden>
>
> 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 <address@hidden>
> ---
>  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