Re: [PATCH 1/2] KVM: arm64: Don't retire aborted MMIO instruction

[Oliver Upton <oliver.upton@linux.dev>]

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!

-- 
Best,
Oliver