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]

Thanks for catching that -- I had missed it.  I read the pointer as:
HypervisorId only exists from FADT revision 6, so the revision (and the
length) should be checked before the field is read.  Is that the right
understanding?

Two things I went on to do.  I am not confident about either, so I would
be grateful for your view:

- The FADT reading moved into a small helper in dev/acpi,
  acpi_fadt_hypervisor_id(), so that the next user of the field need not
  copy it.

- vmt_match() now probes only when the firmware names VMware, rather
  than skipping only when it names something else.  The difference shows
  on a hypervisor that traps MDCCSR_EL0 but leaves the field zero: the
  earlier form still panics there (I checked with a QEMU 11.1.0 patched
  to write zeros).  The ACPI 6.0 note for the field reads to me as
  "probe only on a vendor you recognise", but I may be reading too much
  into it -- if you would rather keep the old behaviour on a zero field,
  it is a single condition to flip.

Rebuilt from trunk, -Werror -Wall clean.  Boots on stock QEMU 11.1 (hvf;
"QEMU", vmt does not attach), on the zero-field QEMU, and on VMware
Fusion 26 ("VMware", vmt0 attaches and answers the host).

--- sys/dev/acpi/acpi_util.h.orig
+++ sys/dev/acpi/acpi_util.h
@@ -114,4 +114,6 @@
 ACPI_STATUS	 acpi_claim_childdevs(device_t, struct acpi_devnode *,
 			const char *);
 
+bool		 acpi_fadt_hypervisor_id(char *, size_t);
+
 #endif	/* !_SYS_DEV_ACPI_ACPI_UTIL_H */
--- sys/dev/acpi/acpi_util.c.orig
+++ sys/dev/acpi/acpi_util.c
@@ -1422,3 +1422,42 @@
 
 	return AE_OK;
 }
+
+/*
+ * Return the Hypervisor Vendor Identity from the FADT as a NUL-terminated
+ * string, or false if there is none to report.
+ *
+ * ACPI 6.0 added the field (5.2.9, FADT Format, offset 268): eight bytes
+ * that name the hypervisor vendor, "usually following the name of the
+ * hypervisor product", with no version information in it.  QEMU writes
+ * "QEMU" and VMware writes "VMware", zero padded.  Firmware places zero
+ * bytes in it when no hypervisor is present, so zero is reported as
+ * absence.  The specification's own note is that a guest "can consult it
+ * and act on the result, based on whether it recognized the vendor",
+ * which is what a caller does with the string.
+ *
+ * The field only exists from FADT revision 6.  ACPICA zeroes its copy of
+ * the FADT before filling it from the firmware's table, so on an older,
+ * shorter table the field reads as zero rather than as garbage.  That is
+ * a property of the copy and not something to lean on, so both the
+ * revision and the length are checked before the field is read, as
+ * open-vm-tools does.  The bytes are copied in table order rather than
+ * through the UINT64, so that this reads the same on big-endian machines.
+ */
+bool
+acpi_fadt_hypervisor_id(char *buf, size_t buflen)
+{
+	const size_t len = sizeof(AcpiGbl_FADT.HypervisorId);
+
+	KASSERT(buflen > len);
+
+	if (AcpiGbl_FADT.Header.Revision < 6 ||
+	    AcpiGbl_FADT.Header.Length < ACPI_FADT_V6_SIZE)
+		return false;
+	if (AcpiGbl_FADT.HypervisorId == 0)
+		return false;
+
+	memcpy(buf, &AcpiGbl_FADT.HypervisorId, len);
+	buf[len] = '\0';
+	return true;
+}
--- sys/arch/aarch64/aarch64/vmt.c.orig
+++ sys/arch/aarch64/aarch64/vmt.c
@@ -32,7 +32,16 @@
 
 #include <sys/device.h>
 #include <sys/module.h>
+#include <sys/systm.h>
 
+#ifdef _KERNEL_OPT
+#include "acpica.h"
+#endif
+
+#if NACPICA > 0
+#include <dev/acpi/acpivar.h>
+#endif
+
 #include <dev/vmt/vmtreg.h>
 #include <dev/vmt/vmtvar.h>
 
@@ -48,10 +57,29 @@
 static int
 vmt_match(device_t parent, cfdata_t match, void *aux)
 {
+#if NACPICA > 0
+	char hv[sizeof(AcpiGbl_FADT.HypervisorId) + 1];
+#endif
+
 	/* vmt should not attach to more than a single CPU. */
 	if (vmt_attached)
 		return 0;
 
+#if NACPICA > 0
+	/*
+	 * The backdoor is a read of MDCCSR_EL0 with a magic value in x7.  A
+	 * hypervisor other than VMware is free to answer that read with an
+	 * undefined instruction trap, which is fatal in kernel mode; QEMU
+	 * does so once the Apple vGIC is in use.  Probe only when the
+	 * firmware names VMware; a hypervisor that names something else,
+	 * or nothing, is not one whose API this driver knows.  See
+	 * port-arm/60655.
+	 */
+	if (!acpi_fadt_hypervisor_id(hv, sizeof(hv)) ||
+	    strncmp(hv, "VMware", 6) != 0)
+		return 0;
+#endif
+
 	return vmt_probe();
 }