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

[Ard Biesheuvel]

On 9 September 2014 19:45, Peter Maydell <address@hidden> wrote:
> 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.
>

OK, so does that mean I need to add fields to DisasContext for these
functions to inspect at the translation stage, and copy the
PSCI_METHOD_[SVC|HVC|NONE] values in it?
Or is there another way to get at that information at that stage?

> 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.
>

ok

>> --- 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...)
>

Yeah, makes sense. I will also add ARCH(6K) and ARCH(7) checks, for
SMC and HVC respectively.
(I don't suppose there is any point in adding TZ and VIRT feature bits
for this atm)