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@xxxxxxxxx> 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!
--
Best,
Oliver
