Hi Stefano,
I’ve implemented the version based on your suggestion. The core logic
now looks like this:
if (k->query_guest_notifiers &&
!k->query_guest_notifiers(qbus->parent) &&
virtio_queue_vector(vdev, idx) == VIRTIO_NO_VECTOR) {
...
}
And in virtio_pci_query_guest_notifiers():
if (msix_enabled(&proxy->pci_dev)) {
return false;
} else {
return !pci_irq_disabled(&proxy->pci_dev);
}
Although this works and preserves the original interface, I personally
find the logic less intuitive to read.
if you're fine with this version, I’ll go ahead and send v3 based on it.
Thanks.
Huaitong Han
Stefano Garzarella <sgarz...@redhat.com> 于2025年5月20日周二 20:53写道：
>
> 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
>
