On Sun, Jun 28, 2020 at 04:04:59PM +0100, Beata Michalska wrote:
> Injecting external data abort through KVM might trigger
> an issue on kernels that do not get updated to include the KVM fix.
> For those and aarch32 guests, the injected abort gets misconfigured
> to be an implementation defined exception. This leads to the guest
> repeatedly re-running the faulting instruction.
>
> Add support for handling that case.
>
> [
> Fixed-by: 018f22f95e8a
> ('KVM: arm: Fix DFSR setting for non-LPAE aarch32 guests')
> Fixed-by: 21aecdbd7f3a
> ('KVM: arm: Make inject_abt32() inject an external abort instead')
> ]
>
> Signed-off-by: Beata Michalska <beata.michalska@xxxxxxxxxx>
Not sure why you didn't pick up my a-b tag on this patch, as I had no
comments at all on it from the previous review. Actually, the last patch
could have picked up my r-b tag too, despite some comments needing
rework. Anyway, here's this tag again.
Acked-by: Andrew Jones <drjones@xxxxxxxxxx>
> ---
> target/arm/cpu.h | 2 ++
> target/arm/kvm.c | 30 +++++++++++++++++++++++++++++-
> target/arm/kvm32.c | 34 ++++++++++++++++++++++++++++++++++
> target/arm/kvm64.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
> target/arm/kvm_arm.h | 10 ++++++++++
> 5 files changed, 124 insertions(+), 1 deletion(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 677584e..ed0ff09 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -570,6 +570,8 @@ typedef struct CPUARMState {
> uint64_t esr;
> } serror;
>
> + uint8_t ext_dabt_raised; /* Tracking/verifying injection of ext DABT */
> +
> /* State of our input IRQ/FIQ/VIRQ/VFIQ lines */
> uint32_t irq_line_state;
>
> diff --git a/target/arm/kvm.c b/target/arm/kvm.c
> index 2dd8a9a..e7a596e 100644
> --- a/target/arm/kvm.c
> +++ b/target/arm/kvm.c
> @@ -749,6 +749,29 @@ int kvm_get_vcpu_events(ARMCPU *cpu)
>
> void kvm_arch_pre_run(CPUState *cs, struct kvm_run *run)
> {
> + ARMCPU *cpu = ARM_CPU(cs);
> + CPUARMState *env = &cpu->env;
> +
> + if (unlikely(env->ext_dabt_raised)) {
> + /*
> + * Verifying that the ext DABT has been properly injected,
> + * otherwise risking indefinitely re-running the faulting instruction
> + * Covering a very narrow case for kernels 5.5..5.5.4
> + * when injected abort was misconfigured to be
> + * an IMPLEMENTATION DEFINED exception (for 32-bit EL1)
> + */
> + if (!arm_feature(env, ARM_FEATURE_AARCH64) &&
> + unlikely(!kvm_arm_verify_ext_dabt_pending(cs))) {
> +
> + error_report("Data abort exception with no valid ISS generated by "
> + "guest memory access. KVM unable to emulate faulting "
> + "instruction. Failed to inject an external data abort "
> + "into the guest.");
> + abort();
> + }
> + /* Clear the status */
> + env->ext_dabt_raised = 0;
> + }
> }
>
> MemTxAttrs kvm_arch_post_run(CPUState *cs, struct kvm_run *run)
> @@ -833,6 +856,8 @@ void kvm_arm_vm_state_change(void *opaque, int running, RunState state)
> static int kvm_arm_handle_dabt_nisv(CPUState *cs, uint64_t esr_iss,
> uint64_t fault_ipa)
> {
> + ARMCPU *cpu = ARM_CPU(cs);
> + CPUARMState *env = &cpu->env;
> /*
> * Request KVM to inject the external data abort into the guest
> */
> @@ -849,7 +874,10 @@ static int kvm_arm_handle_dabt_nisv(CPUState *cs, uint64_t esr_iss,
> /*
> * KVM_CAP_ARM_INJECT_EXT_DABT implies KVM_CAP_VCPU_EVENTS
> */
> - return kvm_vcpu_ioctl(cs, KVM_SET_VCPU_EVENTS, &events);
> + if (!kvm_vcpu_ioctl(cs, KVM_SET_VCPU_EVENTS, &events)) {
> + env->ext_dabt_raised = 1;
> + return 0;
> + }
>
> } else {
> error_report("Data abort exception triggered by guest memory access "
> diff --git a/target/arm/kvm32.c b/target/arm/kvm32.c
> index 7b3a19e..0af46b4 100644
> --- a/target/arm/kvm32.c
> +++ b/target/arm/kvm32.c
> @@ -559,3 +559,37 @@ void kvm_arm_pmu_init(CPUState *cs)
> {
> qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
> }
> +
> +#define ARM_REG_DFSR ARM_CP15_REG32(0, 5, 0, 0)
> +#define ARM_REG_TTBCR ARM_CP15_REG32(0, 2, 0, 2)
> +/*
> + *DFSR:
> + * TTBCR.EAE == 0
> + * FS[4] - DFSR[10]
> + * FS[3:0] - DFSR[3:0]
> + * TTBCR.EAE == 1
> + * FS, bits [5:0]
> + */
> +#define DFSR_FSC(lpae, v) \
> + ((lpae) ? ((v) & 0x3F) : (((v) >> 6) | ((v) & 0x1F)))
> +
> +#define DFSC_EXTABT(lpae) ((lpae) ? 0x10 : 0x08)
> +
> +bool kvm_arm_verify_ext_dabt_pending(CPUState *cs)
> +{
> + uint32_t dfsr_val;
> +
> + if (!kvm_get_one_reg(cs, ARM_REG_DFSR, &dfsr_val)) {
> + ARMCPU *cpu = ARM_CPU(cs);
> + CPUARMState *env = &cpu->env;
> + uint32_t ttbcr;
> + int lpae = 0;
> +
> + if (!kvm_get_one_reg(cs, ARM_REG_TTBCR, &ttbcr)) {
> + lpae = arm_feature(env, ARM_FEATURE_LPAE) && (ttbcr & TTBCR_EAE);
> + }
> + /* The verification is based on FS filed of the DFSR reg only*/
> + return (DFSR_FSC(lpae, dfsr_val) == DFSC_EXTABT(lpae));
> + }
> + return false;
> +}
> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> index f09ed9f..88cf10c 100644
> --- a/target/arm/kvm64.c
> +++ b/target/arm/kvm64.c
> @@ -1497,3 +1497,52 @@ bool kvm_arm_handle_debug(CPUState *cs, struct kvm_debug_exit_arch *debug_exit)
>
> return false;
> }
> +
> +#define ARM64_REG_ESR_EL1 ARM64_SYS_REG(3, 0, 5, 2, 0)
> +#define ARM64_REG_TCR_EL1 ARM64_SYS_REG(3, 0, 2, 0, 2)
> +
> +/*
> + * ESR_EL1
> + * ISS encoding
> + * AARCH64: DFSC, bits [5:0]
> + * AARCH32:
> + * TTBCR.EAE == 0
> + * FS[4] - DFSR[10]
> + * FS[3:0] - DFSR[3:0]
> + * TTBCR.EAE == 1
> + * FS, bits [5:0]
> + */
> +#define ESR_DFSC(aarch64, lpae, v) \
> + ((aarch64 || (lpae)) ? ((v) & 0x3F) \
> + : (((v) >> 6) | ((v) & 0x1F)))
> +
> +#define ESR_DFSC_EXTABT(aarch64, lpae) \
> + ((aarch64) ? 0x10 : (lpae) ? 0x10 : 0x8)
> +
> +bool kvm_arm_verify_ext_dabt_pending(CPUState *cs)
> +{
> + uint64_t dfsr_val;
> +
> + if (!kvm_get_one_reg(cs, ARM64_REG_ESR_EL1, &dfsr_val)) {
> + ARMCPU *cpu = ARM_CPU(cs);
> + CPUARMState *env = &cpu->env;
> + int aarch64_mode = arm_feature(env, ARM_FEATURE_AARCH64);
> + int lpae = 0;
> +
> + if (!aarch64_mode) {
> + uint64_t ttbcr;
> +
> + if (!kvm_get_one_reg(cs, ARM64_REG_TCR_EL1, &ttbcr)) {
> + lpae = arm_feature(env, ARM_FEATURE_LPAE)
> + && (ttbcr & TTBCR_EAE);
> + }
> + }
> + /*
> + * The verification here is based on the DFSC bits
> + * of the ESR_EL1 reg only
> + */
> + return (ESR_DFSC(aarch64_mode, lpae, dfsr_val) ==
> + ESR_DFSC_EXTABT(aarch64_mode, lpae));
> + }
> + return false;
> +}
> diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
> index 48bf5e1..471ddd1 100644
> --- a/target/arm/kvm_arm.h
> +++ b/target/arm/kvm_arm.h
> @@ -453,6 +453,16 @@ struct kvm_guest_debug_arch;
> void kvm_arm_copy_hw_debug_data(struct kvm_guest_debug_arch *ptr);
>
> /**
> + * kvm_arm_verify_ext_dabt_pending:
> + * @cs: CPUState
> + *
> + * Verify the fault status code wrt the Ext DABT injection
> + *
> + * Returns: true if the fault status code is as expected, false otherwise
> + */
> +bool kvm_arm_verify_ext_dabt_pending(CPUState *cs);
> +
> +/**
> * its_class_name:
> *
> * Return the ITS class name to use depending on whether KVM acceleration
> --
> 2.7.4
>
>
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
