OS-7622 bhyve vioapic writes can deadlock instance

Reviewed by: Hans Rosenfeld <hans.rosenfeld@joyent.com>
Reviewed by: John Baldwin <jhb@FreeBSD.org>
Reviewed by: Jerry Jelinek <jerry.jelinek@joyent.com>
Approved by: Robert Mustacchi <rm@joyent.com>

usr/src/compat/freebsd/sys/cpuset.h

@@ -11,7 +11,7 @@
 
 /*
  * Copyright 2014 Pluribus Networks Inc.
- * Copyright 2017 Joyent, Inc.
+ * Copyright 2019 Joyent, Inc.
  */
 
 #ifndef _COMPAT_FREEBSD_SYS_CPUSET_H_
@@ -27,11 +27,15 @@
 #define	CPU_SETOF(cpu, set)		cpuset_only((set), (cpu))
 #define	CPU_ZERO(set)			cpuset_zero((cpuset_t *)(set))
 #define	CPU_CLR(cpu, set)		cpuset_del((set), (cpu))
+#define	CPU_EMPTY(set)			cpuset_isnull((set))
 #define	CPU_FFS(set)			cpusetobj_ffs(set)
 #define	CPU_ISSET(cpu, set)		cpu_in_set((cpuset_t *)(set), (cpu))
 #define	CPU_AND(dst, src)		cpuset_and(			\
 						(cpuset_t *)(dst),	\
 						(cpuset_t *)(src))
+#define	CPU_OR(dst, src)		cpuset_or(			\
+						(cpuset_t *)(dst),	\
+						(cpuset_t *)(src))
 #define	CPU_CMP(set1, set2)		(cpuset_isequal(		\
 						(cpuset_t *)(set1),	\
 						(cpuset_t *)(set2)) == 0)
@@ -42,7 +46,6 @@
 						(cpuset_t *)(set),	\
 						(cpu))
 
-/* XXXJOY: The _ACQ variants appear to imply a membar too. Is that an issue? */
 #define	CPU_SET_ATOMIC_ACQ(cpu, set)	cpuset_atomic_add((set), (cpu))
 
 

usr/src/uts/i86pc/io/vmm/amd/svm.c

@@ -1629,6 +1629,8 @@ svm_inj_interrupts(struct svm_softc *sc, int vcpu, struct vlapic *vlapic)
 
 	need_intr_window = 0;
 
+	vlapic_tmr_update(vlapic);
+
 	if (vcpustate->nextrip != state->rip) {
 		ctrl->intr_shadow = 0;
 		VCPU_CTR2(sc->vm, vcpu, "Guest interrupt blocking "
@@ -2060,8 +2062,8 @@ svm_vmrun(void *arg, int vcpu, register_t rip, pmap_t pmap,
 		 * XXX
 		 * Setting 'vcpustate->lastcpu' here is bit premature because
 		 * we may return from this function without actually executing
-		 * the VMRUN  instruction. This could happen if a rendezvous
-		 * or an AST is pending on the first time through the loop.
+		 * the VMRUN  instruction. This could happen if an AST or yield
+		 * condition is pending on the first time through the loop.
 		 *
 		 * This works for now but any new side-effects of vcpu
 		 * migration should take this case into account.
@@ -2106,9 +2108,9 @@ svm_vmrun(void *arg, int vcpu, register_t rip, pmap_t pmap,
 			break;
 		}
 
-		if (vcpu_rendezvous_pending(evinfo)) {
+		if (vcpu_runblocked(evinfo)) {
 			enable_gintr();
-			vm_exit_rendezvous(vm, vcpu, state->rip);
+			vm_exit_runblock(vm, vcpu, state->rip);
 			break;
 		}
 

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

@@ -1525,6 +1525,8 @@ vmx_inject_interrupts(struct vmx *vmx, int vcpu, struct vlapic *vlapic,
 	int vector;
 	boolean_t extint_pending = B_FALSE;
 
+	vlapic_tmr_update(vlapic);
+
 	gi = vmcs_read(VMCS_GUEST_INTERRUPTIBILITY);
 	info = vmcs_read(VMCS_ENTRY_INTR_INFO);
 
@@ -1701,6 +1703,8 @@ vmx_inject_interrupts(struct vmx *vmx, int vcpu, struct vlapic *vlapic,
 	uint64_t rflags, entryinfo;
 	uint32_t gi, info;
 
+	vlapic_tmr_update(vlapic);
+
 	if (vmx->state[vcpu].nextrip != guestrip) {
 		gi = vmcs_read(VMCS_GUEST_INTERRUPTIBILITY);
 		if (gi & HWINTR_BLOCKING) {
@@ -3321,9 +3325,9 @@ vmx_run(void *arg, int vcpu, register_t rip, pmap_t pmap,
 			break;
 		}
 
-		if (vcpu_rendezvous_pending(evinfo)) {
+		if (vcpu_runblocked(evinfo)) {
 			enable_intr();
-			vm_exit_rendezvous(vmx->vm, vcpu, rip);
+			vm_exit_runblock(vmx->vm, vcpu, rip);
 			break;
 		}
 
@@ -4046,30 +4050,12 @@ vmx_intr_accepted(struct vlapic *vlapic, int vector)
 }
 
 static void
-vmx_set_tmr(struct vlapic *vlapic, int vector, bool level)
+vmx_set_tmr(struct vlapic *vlapic, const uint32_t *masks)
 {
-	struct vlapic_vtx *vlapic_vtx;
-	struct vmx *vmx;
-	struct vmcs *vmcs;
-	uint64_t mask, val;
-
-	KASSERT(vector >= 0 && vector <= 255, ("invalid vector %d", vector));
-	KASSERT(!vcpu_is_running(vlapic->vm, vlapic->vcpuid, NULL),
-	    ("vmx_set_tmr: vcpu cannot be running"));
-
-	vlapic_vtx = (struct vlapic_vtx *)vlapic;
-	vmx = vlapic_vtx->vmx;
-	vmcs = &vmx->vmcs[vlapic->vcpuid];
-	mask = 1UL << (vector % 64);
-
-	VMPTRLD(vmcs);
-	val = vmcs_read(VMCS_EOI_EXIT(vector));
-	if (level)
-		val |= mask;
-	else
-		val &= ~mask;
-	vmcs_write(VMCS_EOI_EXIT(vector), val);
-	VMCLEAR(vmcs);
+	vmcs_write(VMCS_EOI_EXIT0, ((uint64_t)masks[1] << 32) | masks[0]);
+	vmcs_write(VMCS_EOI_EXIT1, ((uint64_t)masks[3] << 32) | masks[2]);
+	vmcs_write(VMCS_EOI_EXIT2, ((uint64_t)masks[5] << 32) | masks[4]);
+	vmcs_write(VMCS_EOI_EXIT3, ((uint64_t)masks[7] << 32) | masks[6]);
 }
 
 static void

usr/src/uts/i86pc/io/vmm/io/vioapic.c

@@ -52,6 +52,7 @@ __FBSDID("$FreeBSD$");
 #include <sys/systm.h>
 #include <sys/kernel.h>
 #include <sys/malloc.h>
+#include <sys/cpuset.h>
 
 #include <x86/apicreg.h>
 #include <machine/vmm.h>
@@ -236,48 +237,139 @@ vioapic_pulse_irq(struct vm *vm, int irq)
 	return (vioapic_set_irqstate(vm, irq, IRQSTATE_PULSE));
 }
 
+#define	REDIR_IS_PHYS(reg)	(((reg) & IOART_DESTMOD) == IOART_DESTPHY)
+#define	REDIR_IS_LOWPRIO(reg)	(((reg) & IOART_DELMOD) == IOART_DELLOPRI)
+/* Level-triggered interrupts only valid in fixed and low-priority modes */
+#define	REDIR_IS_LVLTRIG(reg)						\
+    (((reg) & IOART_TRGRLVL) != 0 &&					\
+    (((reg) & IOART_DELMOD) == IOART_DELFIXED || REDIR_IS_LOWPRIO(reg)))
+#define	REDIR_DEST(reg)		((reg) >> (32 + APIC_ID_SHIFT))
+#define	REDIR_VECTOR(reg)	((reg) & IOART_INTVEC)
+
 /*
- * Reset the vlapic's trigger-mode register to reflect the ioapic pin
- * configuration.
+ * Given a redirection entry, determine which vCPUs would be targeted.
  */
 static void
-vioapic_update_tmr(struct vm *vm, int vcpuid, void *arg)
+vioapic_calcdest(struct vioapic *vioapic, uint64_t redir_ent, cpuset_t *dmask)
 {
-	struct vioapic *vioapic;
-	struct vlapic *vlapic;
-	uint32_t low, high, dest;
-	int delmode, pin, vector;
-	bool level, phys;
 
-	vlapic = vm_lapic(vm, vcpuid);
-	vioapic = vm_ioapic(vm);
+	/*
+	 * When calculating interrupt destinations with vlapic_calcdest(), the
+	 * legacy xAPIC format is assumed, since the system lacks interrupt
+	 * redirection hardware.
+	 * See vlapic_deliver_intr() for more details.
+	 */
+	vlapic_calcdest(vioapic->vm, dmask, REDIR_DEST(redir_ent),
+	    REDIR_IS_PHYS(redir_ent), REDIR_IS_LOWPRIO(redir_ent), false);
+}
+
+/*
+ * Across all redirection entries utilizing a specified vector, determine the
+ * set of vCPUs which would be targeted by a level-triggered interrupt.
+ */
+static void
+vioapic_tmr_active(struct vioapic *vioapic, uint8_t vec, cpuset_t *result)
+{
+	u_int i;
+
+	CPU_ZERO(result);
+	if (vec == 0) {
+		return;
+	}
+
+	for (i = 0; i < REDIR_ENTRIES; i++) {
+		cpuset_t dest;
+		const uint64_t val = vioapic->rtbl[i].reg;
+
+		if (!REDIR_IS_LVLTRIG(val) || REDIR_VECTOR(val) != vec) {
+			continue;
+		}
+
+		CPU_ZERO(&dest);
+		vioapic_calcdest(vioapic, val, &dest);
+		CPU_OR(result, &dest);
+	}
+}
+
+/*
+ * Update TMR state in vLAPICs after changes to vIOAPIC pin configuration
+ */
+static void
+vioapic_update_tmrs(struct vioapic *vioapic, int vcpuid, uint64_t oldval,
+    uint64_t newval)
+{
+	cpuset_t active, allset, newset, oldset;
+	struct vm *vm;
+	uint8_t newvec, oldvec;
+
+	vm = vioapic->vm;
+	CPU_ZERO(&allset);
+	CPU_ZERO(&newset);
+	CPU_ZERO(&oldset);
+	newvec = oldvec = 0;
+
+	if (REDIR_IS_LVLTRIG(oldval)) {
+		vioapic_calcdest(vioapic, oldval, &oldset);
+		CPU_OR(&allset, &oldset);
+		oldvec = REDIR_VECTOR(oldval);
+	}
+
+	if (REDIR_IS_LVLTRIG(newval)) {
+		vioapic_calcdest(vioapic, newval, &newset);
+		CPU_OR(&allset, &newset);
+		newvec = REDIR_VECTOR(newval);
+	}
+
+	if (CPU_EMPTY(&allset) ||
+	    (CPU_CMP(&oldset, &newset) == 0 && oldvec == newvec)) {
+		return;
+	}
 
-	VIOAPIC_LOCK(vioapic);
 	/*
-	 * Reset all vectors to be edge-triggered.
+	 * Since the write to the redirection table has already occurred, a
+	 * scan of level-triggered entries referencing the old vector will find
+	 * only entries which are now currently valid.
 	 */
-	vlapic_reset_tmr(vlapic);
-	for (pin = 0; pin < REDIR_ENTRIES; pin++) {
-		low = vioapic->rtbl[pin].reg;
-		high = vioapic->rtbl[pin].reg >> 32;
+	vioapic_tmr_active(vioapic, oldvec, &active);
 
-		level = low & IOART_TRGRLVL ? true : false;
-		if (!level)
+	while (!CPU_EMPTY(&allset)) {
+		struct vlapic *vlapic;
+		u_int i;
+
+		i = CPU_FFS(&allset) - 1;
+		CPU_CLR(i, &allset);
+
+		if (oldvec == newvec &&
+		    CPU_ISSET(i, &oldset) && CPU_ISSET(i, &newset)) {
 			continue;
+		}
 
-		/*
-		 * For a level-triggered 'pin' let the vlapic figure out if
-		 * an assertion on this 'pin' would result in an interrupt
-		 * being delivered to it. If yes, then it will modify the
-		 * TMR bit associated with this vector to level-triggered.
-		 */
-		phys = ((low & IOART_DESTMOD) == IOART_DESTPHY);
-		delmode = low & IOART_DELMOD;
-		vector = low & IOART_INTVEC;
-		dest = high >> APIC_ID_SHIFT;
-		vlapic_set_tmr_level(vlapic, dest, phys, delmode, vector);
+		if (i != vcpuid) {
+			vcpu_block_run(vm, i);
+		}
+
+		vlapic = vm_lapic(vm, i);
+		if (CPU_ISSET(i, &oldset)) {
+			/*
+			 * Perform the deassertion if no other level-triggered
+			 * IOAPIC entries target this vCPU with the old vector
+			 *
+			 * Note: Sharing of vectors like that should be
+			 * extremely rare in modern operating systems and was
+			 * previously unsupported by the bhyve vIOAPIC.
+			 */
+			if (!CPU_ISSET(i, &active)) {
+				vlapic_tmr_set(vlapic, oldvec, false);
+			}
+		}
+		if (CPU_ISSET(i, &newset)) {
+			vlapic_tmr_set(vlapic, newvec, true);
+		}
+
+		if (i != vcpuid) {
+			vcpu_unblock_run(vm, i);
+		}
 	}
-	VIOAPIC_UNLOCK(vioapic);
 }
 
 static uint32_t
@@ -321,7 +413,6 @@ vioapic_write(struct vioapic *vioapic, int vcpuid, uint32_t addr, uint32_t data)
 	uint64_t data64, mask64;
 	uint64_t last, changed;
 	int regnum, pin, lshift;
-	cpuset_t allvcpus;
 
 	regnum = addr & 0xff;
 	switch (regnum) {
@@ -357,18 +448,15 @@ vioapic_write(struct vioapic *vioapic, int vcpuid, uint32_t addr, uint32_t data)
 
 		/*
 		 * If any fields in the redirection table entry (except mask
-		 * or polarity) have changed then rendezvous all the vcpus
-		 * to update their vlapic trigger-mode registers.
+		 * or polarity) have changed then update the trigger-mode
+		 * registers on all the vlapics.
 		 */
 		changed = last ^ vioapic->rtbl[pin].reg;
 		if (changed & ~(IOART_INTMASK | IOART_INTPOL)) {
 			VIOAPIC_CTR1(vioapic, "ioapic pin%d: recalculate "
 			    "vlapic trigger-mode register", pin);
-			VIOAPIC_UNLOCK(vioapic);
-			allvcpus = vm_active_cpus(vioapic->vm);
-			vm_smp_rendezvous(vioapic->vm, vcpuid, allvcpus,
-			    vioapic_update_tmr, NULL);
-			VIOAPIC_LOCK(vioapic);
+			vioapic_update_tmrs(vioapic, vcpuid, last,
+			    vioapic->rtbl[pin].reg);
 		}
 
 		/*