[PATCH 0/2] KVM: arm: fix KVM_CAP_ARM_INJECT_EXT_DABT for aarch32 guests

[PATCH 0/2] KVM: arm: fix KVM_CAP_ARM_INJECT_EXT_DABT for aarch32 guests

[James Morse]

Beata reports that KVM_CAP_ARM_INJECT_EXT_DABT doesn't do the expected
thing for aarch32 guests. We get the wrong register layout, and weren't
even trying to set a 'external abort' in the first place!

Both patches are intended as fixes, but patch 2 is somewhat in the eye
of the beholder. I don't know why an imp-def exception was picked...

Thanks,

James Morse (2):
  KVM: arm: fix DFSR setting for non-LPAE aarch32 guests
  KVM: arm: Make inject_abt32() inject an external abort instead

 virt/kvm/arm/aarch32.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

-- 
2.24.1

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 1/2] KVM: arm: fix DFSR setting for non-LPAE aarch32 guests

[James Morse]

Beata reports that KVM_SET_VCPU_EVENTS doesn't inject the expected
exception to a non-LPAE aarch32 guest.

The host intends to inject DFSR.FS=0x14 "IMPLEMENTATION DEFINED fault
(Lockdown fault)", but the guest receives DFSR.FS=0x04 "Fault on
instruction cache maintenance". This fault is hooked by
do_translation_fault() since ARMv6, which goes on to silently 'handle'
the exception, and restart the faulting instruction.

It turns out, when TTBCR.EAE is clear DFSR is split, and FS[4] has
to shuffle up to DFSR[10].

As KVM only does this in one place, fix up the static values. We
now get the expected:
| Unhandled fault: lock abort (0x404) at 0x9c800f00

Reported-by: Beata Michalska <beata.michalska@linaro.org>
Fixes: 74a64a981662a ("KVM: arm/arm64: Unify 32bit fault injection")
Signed-off-by: James Morse <james.morse@arm.com>
---
 virt/kvm/arm/aarch32.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/virt/kvm/arm/aarch32.c b/virt/kvm/arm/aarch32.c
index c4c57ba99e90..942108b62cd3 100644
--- a/virt/kvm/arm/aarch32.c
+++ b/virt/kvm/arm/aarch32.c
@@ -90,10 +90,12 @@ static void inject_abt32(struct kvm_vcpu *vcpu, bool is_pabt,
 
 	/* Give the guest an IMPLEMENTATION DEFINED exception */
 	is_lpae = (vcpu_cp15(vcpu, c2_TTBCR) >> 31);
-	if (is_lpae)
+	if (is_lpae) {
 		*fsr = 1 << 9 | 0x34;
-	else
-		*fsr = 0x14;
+	} else {
+		/* Surprise! DFSR's FS[4] lives in bit 10 */
+		*fsr = BIT(10) | 0x4; /* 0x14 */
+	}
 }
 
 void kvm_inject_dabt32(struct kvm_vcpu *vcpu, unsigned long addr)
-- 
2.24.1

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH 2/2] KVM: arm: Make inject_abt32() inject an external abort instead

[James Morse]

KVM's inject_abt64() injects an external-abort into an aarch64 guest.
The KVM_CAP_ARM_INJECT_EXT_DABT is intended to do exactly this, but
for an aarch32 guest inject_abt32() injects an implementation-defined
exception, 'Lockdown fault'.

Change this to external abort. For non-LPAE we now get the documented:
| Unhandled fault: external abort on non-linefetch (0x008) at 0x9c800f00
and for LPAE:
| Unhandled fault: synchronous external abort (0x210) at 0x9c800f00

Reported-by: Beata Michalska <beata.michalska@linaro.org>
Signed-off-by: James Morse <james.morse@arm.com>
---
 virt/kvm/arm/aarch32.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/virt/kvm/arm/aarch32.c b/virt/kvm/arm/aarch32.c
index 942108b62cd3..90433bc486d5 100644
--- a/virt/kvm/arm/aarch32.c
+++ b/virt/kvm/arm/aarch32.c
@@ -14,6 +14,10 @@
 #include <asm/kvm_emulate.h>
 #include <asm/kvm_hyp.h>
 
+#define DFSR_FSC_EXTABT_LPAE	0x10
+#define DFSR_FSC_EXTABT_nLPAE	0x08
+#define DFSR_LPAE		BIT(9)
+
 /*
  * Table taken from ARMv8 ARM DDI0487B-B, table G1-10.
  */
@@ -91,10 +95,10 @@ static void inject_abt32(struct kvm_vcpu *vcpu, bool is_pabt,
 	/* Give the guest an IMPLEMENTATION DEFINED exception */
 	is_lpae = (vcpu_cp15(vcpu, c2_TTBCR) >> 31);
 	if (is_lpae) {
-		*fsr = 1 << 9 | 0x34;
+		*fsr = DFSR_LPAE | DFSR_FSC_EXTABT_LPAE;
 	} else {
-		/* Surprise! DFSR's FS[4] lives in bit 10 */
-		*fsr = BIT(10) | 0x4; /* 0x14 */
+		/* no need to shuffle FS[4] into DFSR[10] as its 0 */
+		*fsr = DFSR_FSC_EXTABT_nLPAE;
 	}
 }
 
-- 
2.24.1

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 0/2] KVM: arm: fix KVM_CAP_ARM_INJECT_EXT_DABT for aarch32 guests

[Marc Zyngier]

Hi James,

Thanks for this.

On 2020-01-21 12:33, James Morse wrote:
> Beata reports that KVM_CAP_ARM_INJECT_EXT_DABT doesn't do the expected
> thing for aarch32 guests. We get the wrong register layout, and weren't
> even trying to set a 'external abort' in the first place!
> 
> Both patches are intended as fixes, but patch 2 is somewhat in the eye
> of the beholder. I don't know why an imp-def exception was picked...

Because we had no idea what we were doing (this is all 2013 vintage).
This code comes from e82e030556e42e8 (the initial arm64 port), which
didn't do the same thing as 5b3e5e5bf230f56 (the 32bit version).

Both were wrong. BTW.

> 
> Thanks,
> 
> James Morse (2):
>   KVM: arm: fix DFSR setting for non-LPAE aarch32 guests
>   KVM: arm: Make inject_abt32() inject an external abort instead
> 
>  virt/kvm/arm/aarch32.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)

It all looks good to me. I'm minded to add a

Fixes: 74a64a981662a ("KVM: arm/arm64: Unify 32bit fault injection")

to the second patch as well, so that KVM_CAP_ARM_INJECT_EXT_DABT does
what it says on the tin.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 0/2] KVM: arm: fix KVM_CAP_ARM_INJECT_EXT_DABT for aarch32 guests

[Beata Michalska]

Hi James,

Thanks for the fixes - they work like a charm.

On Tue, 21 Jan 2020 at 12:34, James Morse <james.morse@arm.com> wrote:
>
> Beata reports that KVM_CAP_ARM_INJECT_EXT_DABT doesn't do the expected
> thing for aarch32 guests. We get the wrong register layout, and weren't
> even trying to set a 'external abort' in the first place!
>
> Both patches are intended as fixes, but patch 2 is somewhat in the eye
> of the beholder. I don't know why an imp-def exception was picked...
>
On a side note - currently KVM exposes capability that is not fully supported
(till the fix gets applied) and there is no easy way for the user space to
determine whether the injection will work as expected and whether it is safe to
use it or not. Although this is addressing a problem that is not that common
(I suppose) but still it might be worth to add a way for the kernel to inform
the user-space that it is all good to go? There has been a 'similar' case in the
past with KVM_SET_USER_MEMORY_REGION, where fixes where needed
and those were announced through new caps. Now, I'm not sure if adding new
capability would be the best approach here though it seems that there is not
much of a choice?

Best Regards
Beata

> Thanks,
>
> James Morse (2):
>   KVM: arm: fix DFSR setting for non-LPAE aarch32 guests
>   KVM: arm: Make inject_abt32() inject an external abort instead
>
>  virt/kvm/arm/aarch32.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
>
> --
> 2.24.1
>
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 0/2] KVM: arm: fix KVM_CAP_ARM_INJECT_EXT_DABT for aarch32 guests

[Marc Zyngier]

On Fri, 24 Jan 2020 15:39:29 +0000
Beata Michalska <beata.michalska@linaro.org> wrote:

Hi Beata,

> Hi James,
> 
> Thanks for the fixes - they work like a charm.
> 
> On Tue, 21 Jan 2020 at 12:34, James Morse <james.morse@arm.com> wrote:
> >
> > Beata reports that KVM_CAP_ARM_INJECT_EXT_DABT doesn't do the expected
> > thing for aarch32 guests. We get the wrong register layout, and weren't
> > even trying to set a 'external abort' in the first place!
> >
> > Both patches are intended as fixes, but patch 2 is somewhat in the eye
> > of the beholder. I don't know why an imp-def exception was picked...
> >  
> On a side note - currently KVM exposes capability that is not fully supported
> (till the fix gets applied) and there is no easy way for the user space to
> determine whether the injection will work as expected and whether it is safe to
> use it or not. Although this is addressing a problem that is not that common
> (I suppose) but still it might be worth to add a way for the kernel to inform
> the user-space that it is all good to go? There has been a 'similar' case in the
> past with KVM_SET_USER_MEMORY_REGION, where fixes where needed
> and those were announced through new caps. Now, I'm not sure if adding new
> capability would be the best approach here though it seems that there is not
> much of a choice?

My take on this particular issue is that although the functionality is
not perfectly working (far from it), it isn't completely broken (the
guest does get some form of abort). Furthermore, we tend to add this
kind of discovery mechanism when the userspace interface is broken, not
when we have an implementation defect in the CPU emulation.

The real question is whether there anything out there that would depend
on such broken behaviour?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH 0/2] KVM: arm: fix KVM_CAP_ARM_INJECT_EXT_DABT for aarch32 guests

[Beata Michalska]

Hi Marc,

On Sun, 26 Jan 2020 at 11:56, Marc Zyngier <maz@kernel.org> wrote:
>
> On Fri, 24 Jan 2020 15:39:29 +0000
> Beata Michalska <beata.michalska@linaro.org> wrote:
>
> Hi Beata,
>
> > Hi James,
> >
> > Thanks for the fixes - they work like a charm.
> >
> > On Tue, 21 Jan 2020 at 12:34, James Morse <james.morse@arm.com> wrote:
> > >
> > > Beata reports that KVM_CAP_ARM_INJECT_EXT_DABT doesn't do the expected
> > > thing for aarch32 guests. We get the wrong register layout, and weren't
> > > even trying to set a 'external abort' in the first place!
> > >
> > > Both patches are intended as fixes, but patch 2 is somewhat in the eye
> > > of the beholder. I don't know why an imp-def exception was picked...
> > >
> > On a side note - currently KVM exposes capability that is not fully supported
> > (till the fix gets applied) and there is no easy way for the user space to
> > determine whether the injection will work as expected and whether it is safe to
> > use it or not. Although this is addressing a problem that is not that common
> > (I suppose) but still it might be worth to add a way for the kernel to inform
> > the user-space that it is all good to go? There has been a 'similar' case in the
> > past with KVM_SET_USER_MEMORY_REGION, where fixes where needed
> > and those were announced through new caps. Now, I'm not sure if adding new
> > capability would be the best approach here though it seems that there is not
> > much of a choice?
>
> My take on this particular issue is that although the functionality is
> not perfectly working (far from it), it isn't completely broken (the
> guest does get some form of abort). Furthermore, we tend to add this
> kind of discovery mechanism when the userspace interface is broken, not
> when we have an implementation defect in the CPU emulation.
>
Indeed, the guest will get 'the' abort with a small catch though:
the fault handler will actually manage to handle it leading to the faulting
instruction being restarted, trapping thereby the guest in a vicious
circle. This is trading in abnormal termination of the guest (aka 'no idea what
has just happened' ) for rather meaningless back-and-forth with the host kernel.

> The real question is whether there anything out there that would depend
> on such broken behaviour?
>
AFAICT, that would be Qemu trying to handle 'nicely' all the unexpected cases,
where guest triggers the DABT with no valid instruction syndrome (changes on
the way). Now, I agree this is not the most common case ever, but still.
Currently the guest will keep on repeating the same mistake ... until
it gets told
it's high time to stop trying. And that becomes arbitrary without implementing
some additional logic behind it - a bit of an overkill.

BR


Beata
> Thanks,
>
>         M.
> --
> Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm