Re: port-arm/60655: vmt(4) probes the VMware backdoor unconditionally on aarch64 and panics the kernel on hypervisors that do not implement it

[zakinko%snowrabbit.org@localhost]

You were right that the version is the variable, and it narrows further than
that.  It is one commit, and it can be shown without rebuilding anything:

	qemu 11.1.0, -M virt (= virt-11.1)		panics
	qemu 11.1.0, -M virt-11.0			boots
	qemu 11.1.0, -M virt,kernel-irqchip=off		boots
	qemu 10.2.0, -M virt				boots

Same host, same NetBSD 11.0 GENERIC64 arm64.img, same command line otherwise.
The commit is 37863fff59 ("hvf: arm: enable vGIC by default for virt-11.1 and
later"), which makes kernel-irqchip default to on for the new machine type.

With the vGIC in play the MDCCSR_EL0 read is trapped out to QEMU, and QEMU's
hvf path has no case for that register, so it injects an undefined
instruction.  Its own trace point says so, at the pc from the panic:

	hvf_unhandled_sysreg_read unhandled sysreg read at
	  pc=0xffffc000003fa318: 0x0020c002 (op0=2 op1=3 crn=0 crm=1 op2=0)

The TCG path does not do this: debug_helper.c defines MDCCSR_EL0 as
ARM_CP_CONST with resetvalue 0, and the comment there says it is RAZ
specifically to avoid spurious SIGILLs "as the support cannot be probed for".
I have sent a patch to qemu-devel making hvf do the same, under the subject
"target/arm/hvf: implement MDCCSR_EL0 as RAZ"; it has not appeared in the
archive yet.  It is not fixed upstream either: master is 1168 commits past
v11.1.0 and none of them touch accel/hvf or target/arm/hvf, and 11.1.1 has
none.

That said, I do not think this is only QEMU's problem, and I would like your
opinion on the NetBSD side.

A hypervisor is free to trap MDCCSR_EL0 and refuse it, and QEMU's own comment
is the point: the guest cannot ask beforehand.  The register is
architecturally mandatory, so testing for its presence answers "yes" and the
probe dies all the same.  On x86 the same driver is safe by construction --
the backdoor there is an inl from an unused I/O port, which cannot fault -- so
the exposure is specific to aarch64, where the probe is an instruction a
hypervisor can decline.

The machinery to survive it is already in the tree.  trap_el1h_error() does
exactly this for SErrors, with a comment saying it would otherwise panic
unconditionally but bus_space_peek(9) has to probe.  trap_el1h_sync() has no
such escape, so ESR_EC_UNKNOWN falls to the default arm and panics.

Built from trunk, cross-compiled for evbarm-aarch64, -Werror -Wall clean.
With that kernel, a stock unpatched QEMU 11.1.0 on -M virt -accel hvf reaches
login, while the trace point above still fires once -- so the kernel is
surviving the injected undefined instruction, rather than the register having
become readable.

One thing I am unsure of, and would rather raise myself: this reuses the
onfault machinery, which copyin/copyout also use.  A genuine kernel bug that
executed an undefined instruction while an onfault was set would now return
EFAULT instead of panicking.  A dedicated flag would be narrower.  I went this
way because trap_el1h_error() already took the broad approach for the same
kind of probe, but if you would rather have something narrower I will redo it.

--- sys/arch/aarch64/aarch64/trap.c.orig
+++ sys/arch/aarch64/aarch64/trap.c
@@ -244,6 +244,25 @@
 		panic("No debugger in kernel");
 #endif
 		break;
+
+	case ESR_EC_UNKNOWN:
+		/*
+		 * An undefined instruction in the kernel is normally fatal,
+		 * but a driver may have to execute one on purpose: an
+		 * instruction that some hypervisors answer and others refuse,
+		 * with no way to ask beforehand.  vmt(4) does this to look
+		 * for the VMware backdoor.  Let such a probe recover the way
+		 * bus_space_peek(9) recovers from an SError, see
+		 * trap_el1h_error() below.
+		 */
+		if (curcpu()->ci_intr_depth == 0) {
+			struct faultbuf * const fb = cpu_disable_onfault();
+			if (fb != NULL) {
+				cpu_jump_onfault(tf, fb, EFAULT);
+				break;
+			}
+		}
+		goto fatal;
 
 	case ESR_EC_FP_ACCESS:
 		if ((curlwp->l_flag & (LW_SYSTEM|LW_SYSTEM_FPU)) ==
@@ -258,6 +277,7 @@
 	case ESR_EC_ILL_STATE:
 	case ESR_EC_BTE_A64:
 	default:
+ fatal:
 		panic("Trap: fatal %s: pc=%016" PRIx64 " sp=%016" PRIx64
 		    " esr=%08x", eclass_trapname(eclass), tf->tf_pc, tf->tf_sp,
 		    esr);
--- sys/arch/aarch64/aarch64/vmt.c.orig
+++ sys/arch/aarch64/aarch64/vmt.c
@@ -33,6 +33,8 @@
 #include <sys/device.h>
 #include <sys/module.h>
 
+#include <aarch64/machdep.h>
+
 #include <dev/vmt/vmtreg.h>
 #include <dev/vmt/vmtvar.h>
 
@@ -48,11 +50,27 @@
 static int
 vmt_match(device_t parent, cfdata_t match, void *aux)
 {
+	struct faultbuf fb;
+	int rv;
+
 	/* vmt should not attach to more than a single CPU. */
 	if (vmt_attached)
 		return 0;
 
-	return vmt_probe();
+	/*
+	 * On aarch64 the backdoor is a read of MDCCSR_EL0 with a magic
+	 * value in x7.  The register is mandatory, but a hypervisor is
+	 * free to trap it and answer with an undefined instruction, and
+	 * the guest cannot ask which one it is on beforehand.  QEMU 11.1
+	 * with the Apple vGIC does exactly that, and the probe took the
+	 * whole kernel down with it.  Catch the trap and report no vmt.
+	 */
+	if (cpu_set_onfault(&fb) != 0)
+		return 0;
+	rv = vmt_probe();
+	cpu_unset_onfault();
+
+	return rv;
 }
 
 static void