On Fri, May 16, 2025 at 09:03:33PM +0800, Huaitong Han wrote:
Stefano Garzarella <sgarz...@redhat.com> 于2025年5月16日周五 16:19写道：On Tue, May 13, 2025 at 07:28:25PM +0800, oen...@gmail.com wrote: >From: Huaitong Han <han...@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 <yuanzhiy...@chinatelecom.cn> >Signed-off-by: Jidong Xia <xi...@chinatelecom.cn> >Signed-off-by: Huaitong Han <han...@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
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 >
