vmd(8): ignore masks on asserts, use synchronous deasserts.

The i8259 was considering the state of the mask register when a
device requested raising the bit in the interrupt request register.
This caused a race condition where if the virtio device asserted
the irq while it was masked in the i8259 by the vm, we'd miss the
interrupt request. The device and the pic would become out of sync
and users reported virtio block device stalls as the vioblk(4)
driver would starve, waiting for an interrupt that will never arrive.

The mask is now considered only at ack time, when finding possible
interrupts to inject. This bug was never a problem previously as
virtio devices were emulated synchronously.

Deasserts related to the vcpu reading the virtio isr register are
also made now in response to the read request instead of issued
asynchronously. This removes a subsequent race condition.

Testing from mbuhl@, stsp@, and Florian Riehm.

ok mlarkin@

diff --git a/usr.sbin/vmd/i8259.c b/usr.sbin/vmd/i8259.c
index f7862f5..1895989 100644 (file)
--- a/usr.sbin/vmd/i8259.c
+++ b/usr.sbin/vmd/i8259.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: i8259.c,v 1.21 2022/11/10 18:58:02 mbuhl Exp $ */
+/* $OpenBSD: i8259.c,v 1.22 2023/09/01 19:42:26 dv Exp $ */
 /*
  * Copyright (c) 2016 Mike Larkin <mlarkin@openbsd.org>
  *
@@ -222,20 +222,16 @@ i8259_assert_irq(uint8_t irq)
 {
        mutex_lock(&pic_mtx);
        if (irq <= 7) {
-               if (!ISSET(pics[MASTER].imr, 1 << irq)) {
-                       SET(pics[MASTER].irr, 1 << irq);
-                       pics[MASTER].asserted = 1;
-               }
+               SET(pics[MASTER].irr, 1 << irq);
+               pics[MASTER].asserted = 1;
        } else {
                irq -= 8;
-               if (!ISSET(pics[SLAVE].imr, 1 << irq)) {
-                       SET(pics[SLAVE].irr, 1 << irq);
-                       pics[SLAVE].asserted = 1;
+               SET(pics[SLAVE].irr, 1 << irq);
+               pics[SLAVE].asserted = 1;
 
-                       /* Assert cascade IRQ on master PIC */
-                       SET(pics[MASTER].irr, 1 << 2);
-                       pics[MASTER].asserted = 1;
-               }
+               /* Assert cascade IRQ on master PIC */
+               SET(pics[MASTER].irr, 1 << 2);
+               pics[MASTER].asserted = 1;
        }
        mutex_unlock(&pic_mtx);
 }

diff --git a/usr.sbin/vmd/vioblk.c b/usr.sbin/vmd/vioblk.c
index 7bc76c4..ef38e9e 100644 (file)
--- a/usr.sbin/vmd/vioblk.c
+++ b/usr.sbin/vmd/vioblk.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: vioblk.c,v 1.4 2023/05/28 05:28:50 asou Exp $ */
+/*     $OpenBSD: vioblk.c,v 1.5 2023/09/01 19:42:26 dv Exp $   */
 
 /*
  * Copyright (c) 2023 Dave Voutila <dv@openbsd.org>
@@ -39,7 +39,8 @@ extern char *__progname;
 extern struct vmd_vm *current_vm;
 
 static const char *disk_type(int);
-static uint32_t handle_io_read(struct viodev_msg *, struct virtio_dev *);
+static uint32_t handle_io_read(struct viodev_msg *, struct virtio_dev *,
+    int8_t *);
 static int handle_io_write(struct viodev_msg *, struct virtio_dev *);
 void vioblk_notify_rx(struct vioblk_dev *);
 int vioblk_notifyq(struct vioblk_dev *);
@@ -723,6 +724,7 @@ handle_sync_io(int fd, short event, void *arg)
        struct viodev_msg msg;
        struct imsg imsg;
        ssize_t n;
+       int8_t intr = INTR_STATE_NOOP;
 
        if (event & EV_READ) {
                if ((n = imsg_read(ibuf)) == -1 && errno != EAGAIN)
@@ -770,8 +772,9 @@ handle_sync_io(int fd, short event, void *arg)
                        }
                case VIODEV_MSG_IO_READ:
                        /* Read IO: make sure to send a reply */
-                       msg.data = handle_io_read(&msg, dev);
+                       msg.data = handle_io_read(&msg, dev, &intr);
                        msg.data_valid = 1;
+                       msg.state = intr;
                        imsg_compose_event(iev, IMSG_DEVOP_MSG, 0, 0, -1, &msg,
                            sizeof(msg));
                        break;
@@ -844,7 +847,7 @@ handle_io_write(struct viodev_msg *msg, struct virtio_dev *dev)
 }
 
 static uint32_t
-handle_io_read(struct viodev_msg *msg, struct virtio_dev *dev)
+handle_io_read(struct viodev_msg *msg, struct virtio_dev *dev, int8_t *intr)
 {
        struct vioblk_dev *vioblk = &dev->vioblk;
        uint8_t sz = msg->io_sz;
@@ -1001,7 +1004,8 @@ handle_io_read(struct viodev_msg *msg, struct virtio_dev *dev)
        case VIRTIO_CONFIG_ISR_STATUS:
                data = vioblk->cfg.isr_status;
                vioblk->cfg.isr_status = 0;
-               virtio_deassert_pic_irq(dev, 0);
+               if (intr != NULL)
+                       *intr = INTR_STATE_DEASSERT;
                break;
        default:
                return (0xFFFFFFFF);

diff --git a/usr.sbin/vmd/vionet.c b/usr.sbin/vmd/vionet.c
index c16ad26..5123ad4 100644 (file)
--- a/usr.sbin/vmd/vionet.c
+++ b/usr.sbin/vmd/vionet.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: vionet.c,v 1.3 2023/05/13 23:15:28 dv Exp $   */
+/*     $OpenBSD: vionet.c,v 1.4 2023/09/01 19:42:26 dv Exp $   */
 
 /*
  * Copyright (c) 2023 Dave Voutila <dv@openbsd.org>
@@ -52,7 +52,8 @@ struct event ev_tap;
 
 static int vionet_rx(struct vionet_dev *);
 static void vionet_rx_event(int, short, void *);
-static uint32_t handle_io_read(struct viodev_msg *, struct virtio_dev *);
+static uint32_t handle_io_read(struct viodev_msg *, struct virtio_dev *,
+    int8_t *);
 static int handle_io_write(struct viodev_msg *, struct virtio_dev *);
 void vionet_notify_rx(struct virtio_dev *);
 int vionet_notifyq(struct virtio_dev *);
@@ -757,6 +758,7 @@ handle_sync_io(int fd, short event, void *arg)
        struct viodev_msg msg;
        struct imsg imsg;
        ssize_t n;
+       int8_t intr = INTR_STATE_NOOP;
 
        if (event & EV_READ) {
                if ((n = imsg_read(ibuf)) == -1 && errno != EAGAIN)
@@ -804,8 +806,9 @@ handle_sync_io(int fd, short event, void *arg)
                        }
                case VIODEV_MSG_IO_READ:
                        /* Read IO: make sure to send a reply */
-                       msg.data = handle_io_read(&msg, dev);
+                       msg.data = handle_io_read(&msg, dev, &intr);
                        msg.data_valid = 1;
+                       msg.state = intr;
                        imsg_compose_event(iev, IMSG_DEVOP_MSG, 0, 0, -1, &msg,
                            sizeof(msg));
                        break;
@@ -891,7 +894,7 @@ handle_io_write(struct viodev_msg *msg, struct virtio_dev *dev)
 }
 
 static uint32_t
-handle_io_read(struct viodev_msg *msg, struct virtio_dev *dev)
+handle_io_read(struct viodev_msg *msg, struct virtio_dev *dev, int8_t *intr)
 {
        struct vionet_dev *vionet = &dev->vionet;
        uint32_t data;
@@ -930,7 +933,8 @@ handle_io_read(struct viodev_msg *msg, struct virtio_dev *dev)
        case VIRTIO_CONFIG_ISR_STATUS:
                data = vionet->cfg.isr_status;
                vionet->cfg.isr_status = 0;
-               virtio_deassert_pic_irq(dev, 0);
+               if (intr != NULL)
+                       *intr = INTR_STATE_DEASSERT;
                break;
        default:
                return (0xFFFFFFFF);