Stefano Garzarella <[email protected]> 于2025年5月20日周二 19:41写道：
>
> On Tue, May 13, 2025 at 07:28:25PM +0800, [email protected] wrote:
> >From: Huaitong Han <[email protected]>
> >
> >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 <[email protected]>
> >Signed-off-by: Jidong Xia <[email protected]>
> >Signed-off-by: Huaitong Han <[email protected]>
> >---
> >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
> >
>
