Thanks for the patch!
I an waiting for Stefan's comments to be addressed.
Additionally, something to improve:
On Wed, Mar 26, 2025 at 04:25:37PM +0800, oen...@gmail.com wrote:
> From: Huaitong Han <han...@chinatelecom.cn>
>
> The vring call fd is set even when the guest does not use msix (e.g., in the
> case of virtio pmd), leading to unnecessary CPU overhead for processing
> interrupts. The previous patch optimized the condition where msix is enabled
> but the queue vector is unset. However, there is a additional case where the
> guest interrupt notifier is effectively unused and the vring call fd should
> also be cleared: the guest does not use msix and the INTX_DISABLED bit in the
> PCI
> config is set.
>
> Fixes: 96a3d98d2c ("vhost: don't set vring call if no vector")
this must be with no empty lines adjacent to the rest of trailers.
> Test result:
> https://raw.githubusercontent.com/oenhan/build_log/refs/heads/main/qemu/2503261015/build/meson-logs/testlog.txt
just include the summary here inline.
> Reported-by: Zhiyuan Yuan <yuanzhiy...@chinatelecom.cn>
> Signed-off-by: Jidong Xia <xi...@chinatelecom.cn>
> Signed-off-by: Huaitong Han <han...@chinatelecom.cn>
> ---
> hw/pci/pci.c | 2 +-
> hw/s390x/virtio-ccw.c | 9 ++++++---
> hw/virtio/vhost.c | 5 ++---
> hw/virtio/virtio-pci.c | 15 ++++++++++-----
> include/hw/pci/pci.h | 1 +
> include/hw/virtio/virtio-bus.h | 2 +-
> 6 files changed, 21 insertions(+), 13 deletions(-)
>
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 2844ec5556..503a897528 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -1719,7 +1719,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 43f3b162c8..af482a22cd 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_used(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)
> @@ -1270,7 +1273,7 @@ static void virtio_ccw_bus_class_init(ObjectClass
> *klass, void *data)
> bus_class->max_dev = 1;
> k->notify = virtio_ccw_notify;
> k->vmstate_change = virtio_ccw_vmstate_change;
> - k->query_guest_notifiers = virtio_ccw_query_guest_notifiers;
> + k->query_guest_notifiers_used = virtio_ccw_query_guest_notifiers_used;
> k->set_guest_notifiers = virtio_ccw_set_guest_notifiers;
> k->save_queue = virtio_ccw_save_queue;
> k->load_queue = virtio_ccw_load_queue;
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 6aa72fd434..32634da836 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -1341,9 +1341,8 @@ int vhost_virtqueue_start(struct vhost_dev *dev,
> vhost_virtqueue_mask(dev, vdev, idx, false);
> }
>
> - if (k->query_guest_notifiers &&
> - k->query_guest_notifiers(qbus->parent) &&
> - virtio_queue_vector(vdev, idx) == VIRTIO_NO_VECTOR) {
> + if (k->query_guest_notifiers_used &&
> + !k->query_guest_notifiers_used(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 3ca3f849d3..25ff869618 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -30,6 +30,7 @@
> #include "qemu/error-report.h"
> #include "qemu/log.h"
> #include "qemu/module.h"
> +#include "hw/pci/pci.h"
> #include "hw/pci/msi.h"
> #include "hw/pci/msix.h"
> #include "hw/loader.h"
> @@ -1031,7 +1032,7 @@ static void virtio_pci_one_vector_mask(VirtIOPCIProxy
> *proxy,
>
> /* If guest supports masking, keep irqfd but mask it.
> * Otherwise, clean it up now.
> - */
> + */
> if (vdev->use_guest_notifier_mask && k->guest_notifier_mask) {
> k->guest_notifier_mask(vdev, queue_no, true);
> } else {
> @@ -1212,10 +1213,15 @@ 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_used(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)
> @@ -2599,7 +2605,7 @@ static void virtio_pci_bus_class_init(ObjectClass
> *klass, void *data)
> k->save_extra_state = virtio_pci_save_extra_state;
> k->load_extra_state = virtio_pci_load_extra_state;
> k->has_extra_state = virtio_pci_has_extra_state;
> - k->query_guest_notifiers = virtio_pci_query_guest_notifiers;
> + k->query_guest_notifiers_used = virtio_pci_query_guest_notifiers_used;
> k->set_guest_notifiers = virtio_pci_set_guest_notifiers;
> k->set_host_notifier_mr = virtio_pci_set_host_notifier_mr;
> k->vmstate_change = virtio_pci_vmstate_change;
> @@ -2630,4 +2636,3 @@ static void virtio_pci_register_types(void)
> }
>
> type_init(virtio_pci_register_types)
> -
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index 822fbacdf0..de4ab28046 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -256,6 +256,7 @@ void pci_del_capability(PCIDevice *pci_dev, uint8_t
> cap_id, uint8_t cap_size);
>
> uint8_t pci_find_capability(PCIDevice *pci_dev, uint8_t cap_id);
>
> +int pci_irq_disabled(PCIDevice *d);
>
> uint32_t pci_default_read_config(PCIDevice *d,
> uint32_t address, int len);
> diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h
> index 7ab8c9dab0..de75a44895 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_used)(DeviceState *d, int n);
I don't see why we need to change the name, and you don't explain
in the commit log. The new name isn't really clear, either.
> 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
