On 9 September 2014 19:45, Peter Maydell <peter.mayd...@linaro.org> wrote:
> On 5 September 2014 13:24, Ard Biesheuvel <ard.biesheu...@linaro.org> wrote:
>> From: Rob Herring <rob.herr...@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.
>
> 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)
