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

["Michael S. Tsirkin" <mst@redhat.com>]

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, oenhan@gmail.com wrote:
> From: Huaitong Han <hanht2@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 <yuanzhiyuan@chinatelecom.cn>
> Signed-off-by: Jidong Xia <xiajd@chinatelecom.cn>
> Signed-off-by: Huaitong Han <hanht2@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