On Sat, Oct 19, 2024 at 10:10:04AM +0100, Marc Zyngier wrote:
On Fri, 18 Oct 2024 20:47:56 +0100, Oliver Upton oliver.upton@linux.dev wrote:
Returning an abort to the guest for an unsupported MMIO access is a documented feature of the KVM UAPI. Nevertheless, it's clear that this plumbing has seen limited testing, since userspace can trivially cause a WARN in the MMIO return:
WARNING: CPU: 0 PID: 30558 at arch/arm64/include/asm/kvm_emulate.h:536 kvm_handle_mmio_return+0x46c/0x5c4 arch/arm64/include/asm/kvm_emulate.h:536 Call trace: kvm_handle_mmio_return+0x46c/0x5c4 arch/arm64/include/asm/kvm_emulate.h:536 kvm_arch_vcpu_ioctl_run+0x98/0x15b4 arch/arm64/kvm/arm.c:1133 kvm_vcpu_ioctl+0x75c/0xa78 virt/kvm/kvm_main.c:4487 __do_sys_ioctl fs/ioctl.c:51 [inline] __se_sys_ioctl fs/ioctl.c:893 [inline] __arm64_sys_ioctl+0x14c/0x1c8 fs/ioctl.c:893 __invoke_syscall arch/arm64/kernel/syscall.c:35 [inline] invoke_syscall+0x98/0x2b8 arch/arm64/kernel/syscall.c:49 el0_svc_common+0x1e0/0x23c arch/arm64/kernel/syscall.c:132 do_el0_svc+0x48/0x58 arch/arm64/kernel/syscall.c:151 el0_svc+0x38/0x68 arch/arm64/kernel/entry-common.c:712 el0t_64_sync_handler+0x90/0xfc arch/arm64/kernel/entry-common.c:730 el0t_64_sync+0x190/0x194 arch/arm64/kernel/entry.S:598
The splat is complaining that KVM is advancing PC while an exception is pending, i.e. that KVM is retiring the MMIO instruction despite a pending external abort. Womp womp.
nit: *synchronous* external abort.
Doh!
+static inline bool kvm_pending_sync_exception(struct kvm_vcpu *vcpu) +{
- if (!vcpu_get_flag(vcpu, PENDING_EXCEPTION))
return false;- if (vcpu_el1_is_32bit(vcpu)) {
switch (vcpu_get_flag(vcpu, EXCEPT_MASK)) {case unpack_vcpu_flag(EXCEPT_AA32_UND):case unpack_vcpu_flag(EXCEPT_AA32_IABT):case unpack_vcpu_flag(EXCEPT_AA32_DABT):return true;default:return false;}- } else {
switch (vcpu_get_flag(vcpu, EXCEPT_MASK)) {case unpack_vcpu_flag(EXCEPT_AA64_EL1_SYNC):case unpack_vcpu_flag(EXCEPT_AA64_EL2_SYNC):return true;default:return false;}- }
+}
Is there any advantage in adding this to an otherwise unsuspecting include file, given that this is only used in a single spot?
v0 of this was a bit more involved, which is why I had this in a header. I'll move it.
Otherwise looks good to me!
Thanks!