qemu-arm
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-arm] [PATCH v14 5/9] target-arm: kvm64: inject synchronous Ext


From: gengdongjiu
Subject: Re: [Qemu-arm] [PATCH v14 5/9] target-arm: kvm64: inject synchronous External Abort
Date: Thu, 11 Jan 2018 13:59:47 +0800
User-agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0

Hi Peter.

On 2018/1/10 1:30, Peter Maydell wrote:
> On 28 December 2017 at 05:54, Dongjiu Geng <address@hidden> wrote:
>> Add synchronous external abort injection logic, setup
>> exception type and syndrome value. When switch to guest,
>> guest will jump to the synchronous external abort vector
>> table entry.
>>
>> The ESR_ELx.DFSC is set to synchronous external abort(0x10),
>> and ESR_ELx.FnV is set to not valid(0x1), which will tell
>> guest that FAR is not valid and holds an UNKNOWN value.
>> These value will be set to KVM register structures through
>> KVM_SET_ONE_REG IOCTL.
>>
>> Signed-off-by: Dongjiu Geng <address@hidden>
>> ---
>> Marc is against that KVM inject the synchronous external abort(SEA) in [1],
>> so user space how to inject it. The test result that injection SEA to guest 
>> by Qemu
>> is shown in [2].
>>
>> [1]: https://lkml.org/lkml/2017/3/2/110
>> [2]:
>> Taking exception 4 [Data Abort]
>> ...from EL0 to EL1
>> ...with ESR 0x24/0x92000410
>> ...with FAR 0x0
>> ...with ELR 0x40cf04
>> ...to EL1 PC 0xffffffc000084c00 PSTATE 0x3c5
>> after kvm_inject_arm_sea
>> Unhandled fault: synchronous external abort (0x92000410) at 
>> 0x0000007fa234c12c
>> CPU: 0 PID: 536 Comm: devmem Not tainted 4.1.0+ #20
>> Hardware name: linux,dummy-virt (DT)
>> task: ffffffc019ab2b00 ti: ffffffc008134000 task.ti: ffffffc008134000
>> PC is at 0x40cf04
>> LR is at 0x40cdec
>> pc : [<000000000040cf04>] lr : [<000000000040cdec>] pstate: 60000000
>> sp : 0000007ff7b24130
>> x29: 0000007ff7b24260 x28: 0000000000000000
>> x27: 00000000000000ad x26: 000000000049c000
>> x25: 000000000048904b x24: 000000000049c000
>> x23: 0000000040600000 x22: 0000007ff7b243a0
>> x21: 0000000000000002 x20: 0000000000000000
>> x19: 0000000000000020 x18: 0000000000000000
>> x17: 000000000049c6d0 x16: 0000007fa22c85c0
>> x15: 0000000000005798 x14: 0000007fa2205f1c
>> x13: 0000007fa241ccb0 x12: 0000000000000137
>> x11: 0000000000000000 x10: 0000000000000000
>> x9 : 0000000000000000 x8 : 00000000000000de
>> x7 : 0000000000000000 x6 : 0000000000002000
>> x5 : 0000000040600000 x4 : 0000000000000003
>> x3 : 0000000000000001 x2 : 0000000000000000
>> x1 : 0000000000000000 x0 : 0000007fa2418000
>> ---
>>  target/arm/kvm64.c | 65 
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 65 insertions(+)
>>
>> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
>> index a16abc8..c00450d 100644
>> --- a/target/arm/kvm64.c
>> +++ b/target/arm/kvm64.c
>> @@ -582,6 +582,71 @@ int kvm_arm_cpreg_level(uint64_t regidx)
>>      return KVM_PUT_RUNTIME_STATE;
>>  }
>>
>> +static int kvm_arm_cpreg_value(ARMCPU *cpu, ptrdiff_t fieldoffset)
>> +{
>> +    int i;
>> +
>> +    for (i = 0; i < cpu->cpreg_array_len; i++) {
> 
> This is still absolutely the wrong thing to do. Nothing should
> need to scan this array like this.
I will confirm that whether KVM mode will need this code, if not, I will remove 
this function.

> 
>> +        uint32_t regidx = kvm_to_cpreg_id(cpu->cpreg_indexes[i]);
>> +        const ARMCPRegInfo *ri;
>> +        ri = get_arm_cp_reginfo(cpu->cp_regs, regidx);
>> +        if (!ri) {
>> +            continue;
>> +        }
>> +
>> +        if (ri->type & ARM_CP_NO_RAW) {
>> +            continue;
>> +        }
>> +
>> +        if (ri->fieldoffset == fieldoffset) {
>> +            cpu->cpreg_values[i] = read_raw_cp_reg(&cpu->env, ri);
>> +            return 0;
>> +        }
>> +    }
>> +    return -EINVAL;
>> +}
>> +
>> +/* Inject synchronous external abort */
>> +static void kvm_inject_arm_sea(CPUState *c)
>> +{
>> +    ARMCPU *cpu = ARM_CPU(c);
>> +    CPUARMState *env = &cpu->env;
>> +    unsigned long cpsr = pstate_read(env);
>> +    uint32_t esr, ret;
>> +
>> +    /* This exception is synchronous data abort*/
> 
> Missing space before */
will fix it.

> 
>> +    c->exception_index = EXCP_DATA_ABORT;
>> +    /* Inject the exception to guest El1 */
> 
> "EL1", all caps.
will fix it.

> 
>> +    env->exception.target_el = 1;
>> +    CPUClass *cc = CPU_GET_CLASS(c);
> 
> Don't declare variables in the middle of the code -- check QEMU's
> CODING_STYLE doc for more info.
will fix it.

> 
>> +
>> +    /* Set the DFSC to synchronous external abort and set FnV to not valid,
>> +     * this will tell guest the FAR_ELx is UNKNOWN for this abort.
>> +     */
>> +    esr = (0x10 | (1 << 10));
>> +
>> +    /* This exception comes from lower or current exception level. */
>> +    if ((cpsr & 0xf) == PSTATE_MODE_EL0t) {
> 
> This looks like it'll be wrong for AArch32 guests (which you can
> still have with KVM with a 64-bit host), and even for AArch32
> userspace in a 64-bit guest. The correct way to find out what the
> current EL is is to use arm_current_el().
 sorry, in the OS(include guest OS), for software error recovery, we only 
support AArch64 kernel, not support AArch32
 kernel or AArch32 user space.

> 
>> +        esr |= (EC_DATAABORT << ARM_EL_EC_SHIFT);
>> +    } else {
>> +        esr |= (EC_DATAABORT_SAME_EL << ARM_EL_EC_SHIFT);
>> +    }
> 
> I'm pretty sure in a previous round of review I said you shouldn't
> be manually constructing ESR values. We have helper functions for
> those (syn_data_abort_*).
sorry, it is my mistake. I will use syn_data_abort_*.

> 
>> +
>> +    /* For the AArch64, instruction length is 32-bit */
>> +    esr |= ARM_EL_IL;
>> +    env->exception.syndrome = esr;
>> +
>> +    cc->do_interrupt(c);
>> +
>> +    /* set ESR_EL1 */
>> +    ret = kvm_arm_cpreg_value(cpu, offsetof(CPUARMState, cp15.esr_el[1]));
> 
> Breakpoint injection doesn't need to do this. Neither should this code.
Good point.
After I confirmed, in the TCG mode, it does not need this code. I am not sure 
whether KVM mode will need it.
I will test it in the KVM mode. If KVM mode also does not need it, I will 
remove this code.


> 
>> +    if (ret) {
>> +        fprintf(stderr, "<%s> failed to set esr_el1\n", __func__);
>> +        abort();
>> +    }
>> +}
>> +
>>  #define AARCH64_CORE_REG(x)   (KVM_REG_ARM64 | KVM_REG_SIZE_U64 | \
>>                   KVM_REG_ARM_CORE | KVM_REG_ARM_CORE_REG(x))
>>
>> --
>> 1.8.3.1
> 
> thanks
> -- PMM
> 
> .
> 




reply via email to

[Prev in Thread] Current Thread [Next in Thread]