On Tue, May 20, 2025 at 08:30:39PM +0800, Huaitong Han wrote:
Stefano Garzarella <sgarz...@redhat.com> 于2025年5月20日周二 19:41写道：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") ^ nit: there should be a space here. If you need to resend, I think you can fix also the one in the description. >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) > { > 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; Why are we moving this check in every callback, can't we leave it as before in vhost.c and here return true? I mean here: if (msix_enabled(&proxy->pci_dev)) { return true; } else { return !pci_irq_disabled(&proxy->pci_dev); } and leave vhost.c untouched.Thanks for the suggestion — your approach indeed achieves the same effect while keeping the interface unchanged. However, I feel it might lead to some misunderstanding of the intended semantics of query_guest_notifiers. My original intent was for this callback to represent whether interrupts are actually in use by the guest, and in that sense, checking whether the queue uses a vector is part of that definition.
Okay, but IMHO these should be 2 patches, one that fixes the problem you mentioned (to be backported to stable, so with the Fixes tag), minimizing the changes. And another patch where you change the semantics.
By splitting the logic — checking msix_enabled and pci_irq_disabled inside the bus callback, and virtio_queue_vector() separately in vhost.c — the semantic boundary becomes less clear. While it works logically, it can reduce readability — particularly because the virtio_queue_vector() check semantically belongs under the msix_enabled() branch, and combining it with the pci_irq_disabled() case (INTx) could make the logic less clear to future readers. Additionally, the set_host_notifier_mr interface already includes an int n parameter, so adding it to query_guest_notifiers is accepted.
I'm not sure that delegating the call to virtio_queue_vector() to each bus is an improvement honestly. But we can discuss it on the patch.
Thanks, Stefano
