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

[Peter Maydell]

On 5 September 2014 13:24, Ard Biesheuvel <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.

This generally looks ok except that it has nasty interactions
with singlestep debug. For SVC, HVC and SMC instructions that
do something, we want to advance the singlestep state machine
using gen_ss_advance(), so that singlestep of one of these
insns works correctly. For UNDEF instruction patterns we don't
want to advance the state machine, so that we don't treat the
insn we didn't execute like we single-stepped it. Unfortunately
this patch means that SMC and HVC are now "sometimes this
does something but sometimes we act like it UNDEFed"...

This is my suggestion for the best compromise between
"theoretical perfect fidelity to the architecture" and
"not too painful to implement":
at translate time, do:

  if (psci enabled via HVC || EL2 implemented) {
      gen_ss_advance(s);
      gen_exception_insn(s, 0, EXCP_HVC, syn_aa64_hvc(imm16));
  } else {
      unallocated_encoding();
  }
and ditto for SMC.

Then in the exception handler have the same code as
now (with fall through to UNDEF if PSCI doesn't
recognise the op). That corresponds to "EL3 firmware
implements PSCI and handles unrecognised ops by
feeding the guest an UNDEF", which nobody would
expect to singlestep cleanly either.

> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -7871,9 +7871,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, syn_aa32_hvc(imm16));
> +                break;
> +            } else if (op1 == 3) {
> +                gen_exception_insn(s, 0, EXCP_SMC, syn_aa32_smc());
> +                break;
> +            }
>              if (op1 != 1) {
>                  goto illegal_op;
>              }
> @@ -9709,10 +9714,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) << 12;
> +                        imm16 |= extract32(insn, 0, 12);
> +                        gen_exception_insn(s, 0, EXCP_HVC, 
> syn_aa32_hvc(imm16));
> +                    } else {
> +                        /* Secure monitor call (v6+) */
> +                        gen_exception_insn(s, 0, EXCP_SMC, syn_aa32_smc());
> +                    }
>                  } else {
>                      op = (insn >> 20) & 7;
>                      switch (op) {

I'm pretty sure you can't do the A32/T32 HVC/SMC
like this, because of oddball cases like SMC in
an IT block. Look at the way we handle SWI by
setting s->is_jmp and then doing the actual
gen_exception() work in the tail end of the
main loop. (It might be possible to do HVC
the way you have it since HVC in an IT block
is UNPREDICTABLE, but let's do it all the same
way...)

thanks
-- PMM