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

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

[oenhan]

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)
 {
     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

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

[Stefano Garzarella]

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?

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
>

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

[Huaitong Han]

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.

> 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
> >
>

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

[Stefano Garzarella]

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

>
>> 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
>> >
>>
>

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

[Stefano Garzarella]

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")
                   ^
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 <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)
> {
>     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,
Stefano

>+    } 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
>

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

[Michael S. Tsirkin]

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
> > > >
> > > 
> > 

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

[Huaitong Han]

Stefano Garzarella <sgarzare@redhat.com> 于2025年5月20日周二 19:41写道：
>
> 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")
>                    ^
> 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 <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)
> > {
> >     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.
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.

Thanks.
Huaitong Han

> Thanks,
> Stefano
>
> >+    } 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
> >
>

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

[Stefano Garzarella]

On Tue, May 20, 2025 at 08:30:39PM +0800, Huaitong Han wrote:
>Stefano Garzarella <sgarzare@redhat.com> 于2025年5月20日周二 19:41写道：
>>
>> 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")
>>                    ^
>> 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 <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)
>> > {
>> >     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

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

[Huaitong Han]

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 <sgarzare@redhat.com> 于2025年5月20日周二 20:53写道：
>
> On Tue, May 20, 2025 at 08:30:39PM +0800, Huaitong Han wrote:
> >Stefano Garzarella <sgarzare@redhat.com> 于2025年5月20日周二 19:41写道：
> >>
> >> 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")
> >>                    ^
> >> 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 <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)
> >> > {
> >> >     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
>

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

[Stefano Garzarella]

Hi Huaitong Han,

On Thu, 22 May 2025 at 05:39, Huaitong Han <oenhan@gmail.com> wrote:
>
> 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) {
>     ...
> }

Which is the way it was before, right?

>
> 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.

I think I've already explained the reason for my request, but I'll try
to explain myself better.

Since we are fixing a problem, I think the patch should be as least
intrusive as possible to avoid new problems and to simplify the
backport.

So IMHO changes such as a readability improvement are not something to
be included in a patch that fixes a problem, but in a separate patch.

> if you're fine with this version, I’ll go ahead and send v3 based on it.

Yep, and if you want you can send another patch, or put both in a
series, to improve the readability. But again, I'm not sure if I
followed you about that, so if you will include the second patch,
please explain why it improves the readability.

Thanks,
Stefano