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

[Rob Herring]

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

[...]

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

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.

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

Ah yes, I had it like that and forgot to go back and split it up in
the process of rebasing.


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


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

You're right. ARM and Thumb are reversed.

Rob