Re: [PATCH V2] vhost: Don't set vring call if guest notifier is unused

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, May 20, 2025 at 01:04:10PM +0200, Stefano Garzarella wrote:
> On Fri, May 16, 2025 at 09:03:33PM +0800, Huaitong Han wrote:
> > Stefano Garzarella <sgarzare@redhat.com> 于2025年5月16日周五 16:19写道：
> > > 
> > > On Tue, May 13, 2025 at 07:28:25PM +0800, oenhan@gmail.com wrote:
> > > >From: Huaitong Han <hanht2@chinatelecom.cn>
> > > >
> > > >The vring call fd is set even when the guest does not use MSI-X (e.g., in the
> > > >case of virtio PMD), leading to unnecessary CPU overhead for processing
> > > >interrupts.
> > > >
> > > >The commit 96a3d98d2c("vhost: don't set vring call if no vector") optimized the
> > > >case where MSI-X is enabled but the queue vector is unset. However, there's an
> > > >additional case where the guest uses INTx and the INTx_DISABLED bit in the PCI
> > > >config is set, meaning that no interrupt notifier will actually be used.
> > > >
> > > >In such cases, the vring call fd should also be cleared to avoid redundant
> > > >interrupt handling.
> > > >
> > > >Fixes: 96a3d98d2c("vhost: don't set vring call if no vector")
> > > >Reported-by: Zhiyuan Yuan <yuanzhiyuan@chinatelecom.cn>
> > > >Signed-off-by: Jidong Xia <xiajd@chinatelecom.cn>
> > > >Signed-off-by: Huaitong Han <hanht2@chinatelecom.cn>
> > > >---
> > > >V2:
> > > >- Retain the name `query_guest_notifiers`
> > > >- All qtest/unit test cases pass
> > > >- Fix V1 patch style problems
> > > >
> > > > hw/pci/pci.c                   |  2 +-
> > > > hw/s390x/virtio-ccw.c          |  7 +++++--
> > > > hw/virtio/vhost.c              |  3 +--
> > > > hw/virtio/virtio-pci.c         | 10 ++++++++--
> > > > include/hw/pci/pci.h           |  1 +
> > > > include/hw/virtio/virtio-bus.h |  2 +-
> > > > 6 files changed, 17 insertions(+), 8 deletions(-)
> > > >
> > > >diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > >index 352b3d12c8..45b491412a 100644
> > > >--- a/hw/pci/pci.c
> > > >+++ b/hw/pci/pci.c
> > > >@@ -1712,7 +1712,7 @@ static void pci_update_mappings(PCIDevice *d)
> > > >     pci_update_vga(d);
> > > > }
> > > >
> > > >-static inline int pci_irq_disabled(PCIDevice *d)
> > > >+int pci_irq_disabled(PCIDevice *d)
> > > 
> > > Since it was inline, will it be better to move the whole function to
> > > include/hw/pci/pci.h and keep it inline?
> > > 
> > I did try moving the function to include/hw/pci/pci.h and marking it
> > inline, but ran into compilation issues due to the use of the incomplete
> > PCIDevice type.
> > Specifically, accessing d->config triggers the following error:
> > include/hw/pci/pci.h:674:26: error: invalid use of incomplete typedef
> > ‘PCIDevice’
> > return pci_get_word(d->config + PCI_COMMAND) & PCI_COMMAND_INTX_DISABLE;
> > Including hw/pci/pci_device.h in pci.h to resolve this introduces
> > further issues, so I suggest to keep the function as a non-inline
> > helper in the .c file.
> 
> I see. If Michael is happy with that, it's fine by me!
> 
> Thanks,
> Stefano
> 

I think it's fine.

> > > Thanks,
> > > Stefano
> > > 
> > > > {
> > > >     return pci_get_word(d->config + PCI_COMMAND) & PCI_COMMAND_INTX_DISABLE;
> > > > }
> > > >diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
> > > >index d2f85b39f3..632708ba4d 100644
> > > >--- a/hw/s390x/virtio-ccw.c
> > > >+++ b/hw/s390x/virtio-ccw.c
> > > >@@ -936,11 +936,14 @@ static void virtio_ccw_vmstate_change(DeviceState *d, bool running)
> > > >     }
> > > > }
> > > >
> > > >-static bool virtio_ccw_query_guest_notifiers(DeviceState *d)
> > > >+static bool virtio_ccw_query_guest_notifiers(DeviceState *d, int n)
> > > > {
> > > >     CcwDevice *dev = CCW_DEVICE(d);
> > > >+    VirtioCcwDevice *vdev = VIRTIO_CCW_DEVICE(d);
> > > >+    VirtIODevice *virtio_dev = virtio_bus_get_device(&vdev->bus);
> > > >
> > > >-    return !!(dev->sch->curr_status.pmcw.flags & PMCW_FLAGS_MASK_ENA);
> > > >+    return !!(dev->sch->curr_status.pmcw.flags & PMCW_FLAGS_MASK_ENA)
> > > >+            && virtio_queue_vector(virtio_dev, n) != VIRTIO_NO_VECTOR;
> > > > }
> > > >
> > > > static int virtio_ccw_get_mappings(VirtioCcwDevice *dev)
> > > >diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > > >index 4cae7c1664..2a9a839763 100644
> > > >--- a/hw/virtio/vhost.c
> > > >+++ b/hw/virtio/vhost.c
> > > >@@ -1341,8 +1341,7 @@ int vhost_virtqueue_start(struct vhost_dev *dev,
> > > >     }
> > > >
> > > >     if (k->query_guest_notifiers &&
> > > >-        k->query_guest_notifiers(qbus->parent) &&
> > > >-        virtio_queue_vector(vdev, idx) == VIRTIO_NO_VECTOR) {
> > > >+        !k->query_guest_notifiers(qbus->parent, idx)) {
> > > >         file.fd = -1;
> > > >         r = dev->vhost_ops->vhost_set_vring_call(dev, &file);
> > > >         if (r) {
> > > >diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> > > >index 0fa8fe4955..d62e199489 100644
> > > >--- a/hw/virtio/virtio-pci.c
> > > >+++ b/hw/virtio/virtio-pci.c
> > > >@@ -1212,10 +1212,16 @@ static int virtio_pci_set_guest_notifier(DeviceState *d, int n, bool assign,
> > > >     return 0;
> > > > }
> > > >
> > > >-static bool virtio_pci_query_guest_notifiers(DeviceState *d)
> > > >+static bool virtio_pci_query_guest_notifiers(DeviceState *d, int n)
> > > > {
> > > >     VirtIOPCIProxy *proxy = to_virtio_pci_proxy(d);
> > > >-    return msix_enabled(&proxy->pci_dev);
> > > >+    VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
> > > >+
> > > >+    if (msix_enabled(&proxy->pci_dev)) {
> > > >+        return virtio_queue_vector(vdev, n) != VIRTIO_NO_VECTOR;
> > > >+    } else {
> > > >+        return !pci_irq_disabled(&proxy->pci_dev);
> > > >+    }
> > > > }
> > > >
> > > > static int virtio_pci_set_guest_notifiers(DeviceState *d, int nvqs, bool assign)
> > > >diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> > > >index c2fe6caa2c..8c24bd97db 100644
> > > >--- a/include/hw/pci/pci.h
> > > >+++ b/include/hw/pci/pci.h
> > > >@@ -668,6 +668,7 @@ void lsi53c8xx_handle_legacy_cmdline(DeviceState *lsi_dev);
> > > >
> > > > qemu_irq pci_allocate_irq(PCIDevice *pci_dev);
> > > > void pci_set_irq(PCIDevice *pci_dev, int level);
> > > >+int pci_irq_disabled(PCIDevice *d);
> > > >
> > > > static inline void pci_irq_assert(PCIDevice *pci_dev)
> > > > {
> > > >diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h
> > > >index 7ab8c9dab0..75d43b508a 100644
> > > >--- a/include/hw/virtio/virtio-bus.h
> > > >+++ b/include/hw/virtio/virtio-bus.h
> > > >@@ -48,7 +48,7 @@ struct VirtioBusClass {
> > > >     int (*load_done)(DeviceState *d, QEMUFile *f);
> > > >     int (*load_extra_state)(DeviceState *d, QEMUFile *f);
> > > >     bool (*has_extra_state)(DeviceState *d);
> > > >-    bool (*query_guest_notifiers)(DeviceState *d);
> > > >+    bool (*query_guest_notifiers)(DeviceState *d, int n);
> > > >     int (*set_guest_notifiers)(DeviceState *d, int nvqs, bool assign);
> > > >     int (*set_host_notifier_mr)(DeviceState *d, int n,
> > > >                                 MemoryRegion *mr, bool assign);
> > > >--
> > > >2.43.5
> > > >
> > > 
> >