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

[Peter Maydell]

On 14 May 2014 21:55, Rob Herring <address@hidden> wrote:
> On Wed, May 14, 2014 at 12:44 PM, Peter Maydell
> <address@hidden> 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