16393 bhyve should take more care with MSR_AMD_TSC_RATIO

Reviewed by: Dan McDonald <[email protected]>
Reviewed by: Jordan Paige Hendricks <[email protected]>
Approved by: Richard Lowe <[email protected]>

Author: pfmooney

usr/src/test/bhyve-tests/tests/vmm/datarw_constraints.c

@@ -10,7 +10,7 @@
  */
 
 /*
- * Copyright 2023 Oxide Computer Company
+ * Copyright 2024 Oxide Computer Company
  */
 
 #include <stdio.h>
@@ -145,6 +145,7 @@ test_vm_classes(int vmfd)
 			.vdx_version = cases[i].ctc_version,
 			.vdx_len = bufsz,
 			.vdx_data = buf,
+			.vdx_vcpuid = -1,
 		};
 
 		/* First do a read */
@@ -340,6 +341,49 @@ test_vcpuid_combos(int vmfd)
 	free(buf);
 }
 
+static void
+test_vcpuid_time(int vmfd)
+{
+	struct vdi_time_info_v1 data;
+	struct vm_data_xfer vdx = {
+		.vdx_class = VDC_VMM_TIME,
+		.vdx_version = 1,
+		.vdx_len = sizeof (data),
+		.vdx_data = &data,
+	};
+
+	/* This should work with the system-wide vcpuid */
+	vdx.vdx_vcpuid = -1;
+	if (ioctl(vmfd, VM_DATA_READ, &vdx) != 0) {
+		err(EXIT_FAILURE, "VM_DATA_READ failed for valid vcpuid");
+	}
+
+	/* But fail for other vcpuids */
+	vdx.vdx_vcpuid = 0;
+	if (ioctl(vmfd, VM_DATA_READ, &vdx) == 0) {
+		err(EXIT_FAILURE, "VM_DATA_READ should fail for vcpuid %d",
+		    vdx.vdx_vcpuid);
+	}
+
+	/*
+	 * Perform same check for writes
+	 *
+	 * Normally this would require care to handle hosts which lack frequency
+	 * scaling functionality, but since we are writing back the same data,
+	 * the guest frequency should match that of the host, requiring no real
+	 * scaling be done for the instance.
+	 */
+	vdx.vdx_vcpuid = -1;
+	if (ioctl(vmfd, VM_DATA_WRITE, &vdx) != 0) {
+		err(EXIT_FAILURE, "VM_DATA_WRITE failed for valid vcpuid");
+	}
+	vdx.vdx_vcpuid = 0;
+	if (ioctl(vmfd, VM_DATA_WRITE, &vdx) == 0) {
+		errx(EXIT_FAILURE, "VM_DATA_READ should fail for vcpuid %d",
+		    vdx.vdx_vcpuid);
+	}
+}
+
 int
 main(int argc, char *argv[])
 {
@@ -372,6 +416,9 @@ main(int argc, char *argv[])
 	/* Try some weird vdx_vcpuid cases */
 	test_vcpuid_combos(vmfd);
 
+	/* VMM_TIME is picky about vcpuid */
+	test_vcpuid_time(vmfd);
+
 	vm_destroy(ctx);
 	(void) printf("%s\tPASS\n", suite_name);
 	return (EXIT_SUCCESS);

usr/src/uts/intel/io/vmm/amd/svm_msr.c

@@ -118,8 +118,10 @@ svm_msr_guest_exit(struct svm_softc *sc, int vcpu)
 	wrmsr(MSR_STAR, host_msrs[IDX_MSR_STAR]);
 	wrmsr(MSR_SF_MASK, host_msrs[IDX_MSR_SF_MASK]);
 
-	/* Reset frequency multiplier MSR */
-	wrmsr(MSR_AMD_TSC_RATIO, AMD_TSCM_RESET_VAL);
+	/* Reset frequency multiplier MSR if any scaling is configured */
+	if (vm_get_freq_multiplier(sc->vm) != VM_TSCM_NOSCALE) {
+		wrmsr(MSR_AMD_TSC_RATIO, AMD_TSCM_RESET_VAL);
+	}
 
 	/* MSR_KGSBASE will be restored on the way back to userspace */
 }

usr/src/uts/intel/io/vmm/sys/vmm_kernel.h

@@ -37,7 +37,7 @@
  *
  * Copyright 2015 Pluribus Networks Inc.
  * Copyright 2019 Joyent, Inc.
- * Copyright 2023 Oxide Computer Company
+ * Copyright 2024 Oxide Computer Company
  * Copyright 2021 OmniOS Community Edition (OmniOSce) Association.
  */
 
@@ -506,6 +506,7 @@ typedef struct vmm_data_req {
 	uint32_t	vdr_len;
 	void		*vdr_data;
 	uint32_t	*vdr_result_len;
+	int		vdr_vcpuid;
 } vmm_data_req_t;
 
 typedef int (*vmm_data_writef_t)(void *, const vmm_data_req_t *);
@@ -564,8 +565,8 @@ typedef struct vmm_data_version_entry {
 
 #define	VMM_DATA_VERSION(sym)	SET_ENTRY(vmm_data_version_entries, sym)
 
-int vmm_data_read(struct vm *, int, const vmm_data_req_t *);
-int vmm_data_write(struct vm *, int, const vmm_data_req_t *);
+int vmm_data_read(struct vm *, const vmm_data_req_t *);
+int vmm_data_write(struct vm *, const vmm_data_req_t *);
 
 /*
  * TSC Scaling

usr/src/uts/intel/io/vmm/vmm.c

@@ -37,7 +37,7 @@
  *
  * Copyright 2015 Pluribus Networks Inc.
  * Copyright 2018 Joyent, Inc.
- * Copyright 2023 Oxide Computer Company
+ * Copyright 2024 Oxide Computer Company
  * Copyright 2021 OmniOS Community Edition (OmniOSce) Association.
  */
 
@@ -3934,8 +3934,7 @@ vmm_kstat_update_vcpu(struct kstat *ksp, int rw)
 SET_DECLARE(vmm_data_version_entries, const vmm_data_version_entry_t);
 
 static int
-vmm_data_find(const vmm_data_req_t *req, int vcpuid,
-    const vmm_data_version_entry_t **resp)
+vmm_data_find(const vmm_data_req_t *req, const vmm_data_version_entry_t **resp)
 {
 	const vmm_data_version_entry_t **vdpp, *vdp;
 
@@ -3977,7 +3976,8 @@ vmm_data_find(const vmm_data_req_t *req, int vcpuid,
 			 * others expect vcpuid [0, VM_MAXCPU).
 			 */
 			const int llimit = vdp->vdve_vcpu_wildcard ? -1 : 0;
-			if (vcpuid < llimit || vcpuid >= VM_MAXCPU) {
+			if (req->vdr_vcpuid < llimit ||
+			    req->vdr_vcpuid >= VM_MAXCPU) {
 				return (EINVAL);
 			}
 		} else {
@@ -4882,6 +4882,14 @@ vmm_data_read_vmm_time(void *arg, const vmm_data_req_t *req)
 	struct vm *vm = arg;
 	struct vdi_time_info_v1 *out = req->vdr_data;
 
+	/*
+	 * Since write operations on VMM_TIME data are strict about vcpuid
+	 * (see: vmm_data_write_vmm_time()), read operations should be as well.
+	 */
+	if (req->vdr_vcpuid != -1) {
+		return (EINVAL);
+	}
+
 	/* Take a snapshot of this point in time */
 	uint64_t tsc;
 	hrtime_t hrtime;
@@ -4937,6 +4945,17 @@ vmm_data_write_vmm_time(void *arg, const vmm_data_req_t *req)
 	struct vm *vm = arg;
 	const struct vdi_time_info_v1 *src = req->vdr_data;
 
+	/*
+	 * While vcpuid values != -1 are tolerated by the vmm_data machinery for
+	 * VM-wide endpoints, the time-related data is more strict: It relies on
+	 * write-locking the VM (implied by the vcpuid -1) to prevent vCPUs or
+	 * other bits from observing inconsistent values while the state is
+	 * being written.
+	 */
+	if (req->vdr_vcpuid != -1) {
+		return (EINVAL);
+	}
+
 	/*
 	 * Platform-specific checks will verify the requested frequency against
 	 * the supported range further, but a frequency of 0 is never valid.
@@ -5064,12 +5083,12 @@ static const vmm_data_version_entry_t versions_v1 = {
 VMM_DATA_VERSION(versions_v1);
 
 int
-vmm_data_read(struct vm *vm, int vcpuid, const vmm_data_req_t *req)
+vmm_data_read(struct vm *vm, const vmm_data_req_t *req)
 {
 	int err = 0;
 
 	const vmm_data_version_entry_t *entry = NULL;
-	err = vmm_data_find(req, vcpuid, &entry);
+	err = vmm_data_find(req, &entry);
 	if (err != 0) {
 		return (err);
 	}
@@ -5080,7 +5099,7 @@ vmm_data_read(struct vm *vm, int vcpuid, const vmm_data_req_t *req)
 
 		err = entry->vdve_readf(datap, req);
 	} else if (entry->vdve_vcpu_readf != NULL) {
-		err = entry->vdve_vcpu_readf(vm, vcpuid, req);
+		err = entry->vdve_vcpu_readf(vm, req->vdr_vcpuid, req);
 	} else {
 		err = EINVAL;
 	}
@@ -5097,12 +5116,12 @@ vmm_data_read(struct vm *vm, int vcpuid, const vmm_data_req_t *req)
 }
 
 int
-vmm_data_write(struct vm *vm, int vcpuid, const vmm_data_req_t *req)
+vmm_data_write(struct vm *vm, const vmm_data_req_t *req)
 {
 	int err = 0;
 
 	const vmm_data_version_entry_t *entry = NULL;
-	err = vmm_data_find(req, vcpuid, &entry);
+	err = vmm_data_find(req, &entry);
 	if (err != 0) {
 		return (err);
 	}
@@ -5113,7 +5132,7 @@ vmm_data_write(struct vm *vm, int vcpuid, const vmm_data_req_t *req)
 
 		err = entry->vdve_writef(datap, req);
 	} else if (entry->vdve_vcpu_writef != NULL) {
-		err = entry->vdve_vcpu_writef(vm, vcpuid, req);
+		err = entry->vdve_vcpu_writef(vm, req->vdr_vcpuid, req);
 	} else {
 		err = EINVAL;
 	}

usr/src/uts/intel/io/vmm/vmm_sol_dev.c

@@ -1848,8 +1848,9 @@ vmmdev_do_ioctl(vmm_softc_t *sc, int cmd, intptr_t arg, int md,
 			.vdr_len = len,
 			.vdr_data = buf,
 			.vdr_result_len = &vdx.vdx_result_len,
+			.vdr_vcpuid = vdx.vdx_vcpuid,
 		};
-		error = vmm_data_read(sc->vmm_vm, vdx.vdx_vcpuid, &req);
+		error = vmm_data_read(sc->vmm_vm, &req);
 
 		if (error == 0 && buf != NULL) {
 			if (ddi_copyout(buf, vdx.vdx_data, len, md) != 0) {
@@ -1906,10 +1907,10 @@ vmmdev_do_ioctl(vmm_softc_t *sc, int cmd, intptr_t arg, int md,
 			.vdr_len = len,
 			.vdr_data = buf,
 			.vdr_result_len = &vdx.vdx_result_len,
+			.vdr_vcpuid = vdx.vdx_vcpuid,
 		};
 		if (vmm_allow_state_writes != 0) {
-			error = vmm_data_write(sc->vmm_vm, vdx.vdx_vcpuid,
-			    &req);
+			error = vmm_data_write(sc->vmm_vm, &req);
 		} else {
 			/*
 			 * Reject the write if somone has thrown the switch back