Re: [PATCH v3 2/2] target/arm: kvm: Handle DABT with no valid ISS
On Thu, 12 Mar 2020 at 10:25, Andrew Jones wrote:
>
> On Thu, Mar 12, 2020 at 12:34:01AM +, Beata Michalska wrote:
> > On ARMv7 & ARMv8 some load/store instructions might trigger a data abort
> > exception with no valid ISS info to be decoded. The lack of decode info
> > makes it at least tricky to emulate those instruction which is one of the
> > (many) reasons why KVM will not even try to do so.
> >
> > Add support for handling those by requesting KVM to inject external
> > dabt into the quest.
> >
> > Signed-off-by: Beata Michalska
> > ---
> > target/arm/cpu.h | 3 ++
> > target/arm/kvm.c | 81
> >
> > target/arm/kvm32.c | 26 +
> > target/arm/kvm64.c | 36 +++
> > target/arm/kvm_arm.h | 22 ++
> > 5 files changed, 168 insertions(+)
> >
> > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> > index 4ffd991..45fdd2e 100644
> > --- a/target/arm/cpu.h
> > +++ b/target/arm/cpu.h
> > @@ -560,6 +560,9 @@ typedef struct CPUARMState {
> > uint64_t esr;
> > } serror;
> >
> > +uint8_t ext_dabt_pending:1; /* Request for injecting ext DABT */
> > +uint8_t ext_dabt_raised:1; /* Tracking/verifying injection of ext DABT
> > */
> > +
>
> Why the bit-fields? We don't use them anywhere else in cpu.h, and that's
> probably because they're not portable. We should just use bools.
>
Old habit of optimizations.
I can drop the bit fields but I'd rather stay with the original type
to be consistent with the kvm ones. I am not sure though why in this case
that would not be portable - bit fields can get tricky but that should not
be the case here (?)
> > /* 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 85860e6..8b7b708 100644
> > --- a/target/arm/kvm.c
> > +++ b/target/arm/kvm.c
> > @@ -39,6 +39,7 @@ const KVMCapabilityInfo kvm_arch_required_capabilities[]
> > = {
> >
> > static bool cap_has_mp_state;
> > static bool cap_has_inject_serror_esr;
> > +static bool cap_has_inject_ext_dabt;
> >
> > static ARMHostCPUFeatures arm_host_cpu_features;
> >
> > @@ -244,6 +245,16 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
> > ret = -EINVAL;
> > }
> >
> > +if (kvm_check_extension(s, KVM_CAP_ARM_NISV_TO_USER)) {
> > +if (kvm_vm_enable_cap(s, KVM_CAP_ARM_NISV_TO_USER, 0)) {
> > +warn_report("Failed to enable DABT NISV cap");
> > +} else {
> > +/* Set status for supporting the external dabt injection */
> > +cap_has_inject_ext_dabt = kvm_check_extension(s,
> > +KVM_CAP_ARM_INJECT_EXT_DABT);
> > +}
> > +}
> > +
> > return ret;
> > }
> >
> > @@ -703,9 +714,20 @@ int kvm_put_vcpu_events(ARMCPU *cpu)
> > events.exception.serror_esr = env->serror.esr;
> > }
> >
> > +if (cap_has_inject_ext_dabt) {
> > +events.exception.ext_dabt_pending = env->ext_dabt_pending;
> > +}
> > +
> > ret = kvm_vcpu_ioctl(CPU(cpu), KVM_SET_VCPU_EVENTS, &events);
> > if (ret) {
> > error_report("failed to put vcpu events");
> > +} else if (env->ext_dabt_pending) {
> > +/*
> > + * Mark that the external DABT has been injected,
> > + * if one has been requested
> > + */
> > +env->ext_dabt_raised = env->ext_dabt_pending;
> > +env->ext_dabt_pending = 0;
> > }
> >
> > return ret;
> > @@ -737,6 +759,30 @@ 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
>
> I'm still not convinced that QEMU needs to add workarounds for broken KVM,
> when KVM can be fixed, and even is already fixed. If you really want to
> keep it, then can you break this patch into two, splitting the dabt
> injection out from the workaround?
>
I can definitely do that.
Not a big fan of adding features that expose issues, even if those
are a rare case. Rather have those handled, on the safe side, so
I would prefer to keep it. Although I must admit this is bit unfortunate.
> > + * 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 faulti
Re: [PATCH v3 2/2] target/arm: kvm: Handle DABT with no valid ISS
On Thu, Mar 12, 2020 at 12:34:01AM +, Beata Michalska wrote:
> On ARMv7 & ARMv8 some load/store instructions might trigger a data abort
> exception with no valid ISS info to be decoded. The lack of decode info
> makes it at least tricky to emulate those instruction which is one of the
> (many) reasons why KVM will not even try to do so.
>
> Add support for handling those by requesting KVM to inject external
> dabt into the quest.
>
> Signed-off-by: Beata Michalska
> ---
> target/arm/cpu.h | 3 ++
> target/arm/kvm.c | 81
>
> target/arm/kvm32.c | 26 +
> target/arm/kvm64.c | 36 +++
> target/arm/kvm_arm.h | 22 ++
> 5 files changed, 168 insertions(+)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 4ffd991..45fdd2e 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -560,6 +560,9 @@ typedef struct CPUARMState {
> uint64_t esr;
> } serror;
>
> +uint8_t ext_dabt_pending:1; /* Request for injecting ext DABT */
> +uint8_t ext_dabt_raised:1; /* Tracking/verifying injection of ext DABT */
> +
Why the bit-fields? We don't use them anywhere else in cpu.h, and that's
probably because they're not portable. We should just use bools.
> /* 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 85860e6..8b7b708 100644
> --- a/target/arm/kvm.c
> +++ b/target/arm/kvm.c
> @@ -39,6 +39,7 @@ const KVMCapabilityInfo kvm_arch_required_capabilities[] = {
>
> static bool cap_has_mp_state;
> static bool cap_has_inject_serror_esr;
> +static bool cap_has_inject_ext_dabt;
>
> static ARMHostCPUFeatures arm_host_cpu_features;
>
> @@ -244,6 +245,16 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
> ret = -EINVAL;
> }
>
> +if (kvm_check_extension(s, KVM_CAP_ARM_NISV_TO_USER)) {
> +if (kvm_vm_enable_cap(s, KVM_CAP_ARM_NISV_TO_USER, 0)) {
> +warn_report("Failed to enable DABT NISV cap");
> +} else {
> +/* Set status for supporting the external dabt injection */
> +cap_has_inject_ext_dabt = kvm_check_extension(s,
> +KVM_CAP_ARM_INJECT_EXT_DABT);
> +}
> +}
> +
> return ret;
> }
>
> @@ -703,9 +714,20 @@ int kvm_put_vcpu_events(ARMCPU *cpu)
> events.exception.serror_esr = env->serror.esr;
> }
>
> +if (cap_has_inject_ext_dabt) {
> +events.exception.ext_dabt_pending = env->ext_dabt_pending;
> +}
> +
> ret = kvm_vcpu_ioctl(CPU(cpu), KVM_SET_VCPU_EVENTS, &events);
> if (ret) {
> error_report("failed to put vcpu events");
> +} else if (env->ext_dabt_pending) {
> +/*
> + * Mark that the external DABT has been injected,
> + * if one has been requested
> + */
> +env->ext_dabt_raised = env->ext_dabt_pending;
> +env->ext_dabt_pending = 0;
> }
>
> return ret;
> @@ -737,6 +759,30 @@ 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
I'm still not convinced that QEMU needs to add workarounds for broken KVM,
when KVM can be fixed, and even is already fixed. If you really want to
keep it, then can you break this patch into two, splitting the dabt
injection out from the workaround?
> + * 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)
> @@ -819,6 +865,11 @@ int kvm_arch_handle_exit(CPUState *cs, struct kvm_run
> *run)
> ret = EXCP_DEBUG;
> } /* otherwise return to guest */
> break;
> +case KVM_EXIT_ARM_NISV:
> +/* External DABT with no valid iss to decode */
> +ret = kvm_arm_handle_dabt_nisv(cs, run->arm_nisv.esr_iss,
> + run->arm_nisv.fault_ipa);
> +break;
> default:
>
[PATCH v3 2/2] target/arm: kvm: Handle DABT with no valid ISS
On ARMv7 & ARMv8 some load/store instructions might trigger a data abort
exception with no valid ISS info to be decoded. The lack of decode info
makes it at least tricky to emulate those instruction which is one of the
(many) reasons why KVM will not even try to do so.
Add support for handling those by requesting KVM to inject external
dabt into the quest.
Signed-off-by: Beata Michalska
---
target/arm/cpu.h | 3 ++
target/arm/kvm.c | 81
target/arm/kvm32.c | 26 +
target/arm/kvm64.c | 36 +++
target/arm/kvm_arm.h | 22 ++
5 files changed, 168 insertions(+)
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 4ffd991..45fdd2e 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -560,6 +560,9 @@ typedef struct CPUARMState {
uint64_t esr;
} serror;
+uint8_t ext_dabt_pending:1; /* Request for injecting ext DABT */
+uint8_t ext_dabt_raised:1; /* 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 85860e6..8b7b708 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -39,6 +39,7 @@ const KVMCapabilityInfo kvm_arch_required_capabilities[] = {
static bool cap_has_mp_state;
static bool cap_has_inject_serror_esr;
+static bool cap_has_inject_ext_dabt;
static ARMHostCPUFeatures arm_host_cpu_features;
@@ -244,6 +245,16 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
ret = -EINVAL;
}
+if (kvm_check_extension(s, KVM_CAP_ARM_NISV_TO_USER)) {
+if (kvm_vm_enable_cap(s, KVM_CAP_ARM_NISV_TO_USER, 0)) {
+warn_report("Failed to enable DABT NISV cap");
+} else {
+/* Set status for supporting the external dabt injection */
+cap_has_inject_ext_dabt = kvm_check_extension(s,
+KVM_CAP_ARM_INJECT_EXT_DABT);
+}
+}
+
return ret;
}
@@ -703,9 +714,20 @@ int kvm_put_vcpu_events(ARMCPU *cpu)
events.exception.serror_esr = env->serror.esr;
}
+if (cap_has_inject_ext_dabt) {
+events.exception.ext_dabt_pending = env->ext_dabt_pending;
+}
+
ret = kvm_vcpu_ioctl(CPU(cpu), KVM_SET_VCPU_EVENTS, &events);
if (ret) {
error_report("failed to put vcpu events");
+} else if (env->ext_dabt_pending) {
+/*
+ * Mark that the external DABT has been injected,
+ * if one has been requested
+ */
+env->ext_dabt_raised = env->ext_dabt_pending;
+env->ext_dabt_pending = 0;
}
return ret;
@@ -737,6 +759,30 @@ 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)
@@ -819,6 +865,11 @@ int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
ret = EXCP_DEBUG;
} /* otherwise return to guest */
break;
+case KVM_EXIT_ARM_NISV:
+/* External DABT with no valid iss to decode */
+ret = kvm_arm_handle_dabt_nisv(cs, run->arm_nisv.esr_iss,
+ run->arm_nisv.fault_ipa);
+break;
default:
qemu_log_mask(LOG_UNIMP, "%s: un-handled exit reason %d\n",
__func__, run->exit_reason);
@@ -953,3 +1004,33 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
{
return (data - 32) & 0x;
}
+
+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;
+
+ /*
+* ISS [23:14] is invalid so there is a limited info
+* on what has just happened so the only *useful* thing that can
+* be retrieved from ISS is WnR & DFSC (though in some cases WnR
+* might be less of a value as well)
+*/
+
+/*
+ * Set pending ext dabt and trigger SET
