[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 3/7] target-arm: add hvc and smc exception emula
From: |
Rob Herring |
Subject: |
Re: [Qemu-devel] [PATCH 3/7] target-arm: add hvc and smc exception emulation handling infrastructure |
Date: |
Wed, 14 May 2014 15:55:11 -0500 |
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