[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH for-4.1] vfio/common: Introduce vfio_set_irq_sig
From: |
Alex Williamson |
Subject: |
Re: [Qemu-devel] [PATCH for-4.1] vfio/common: Introduce vfio_set_irq_signaling helper |
Date: |
Wed, 15 May 2019 16:52:58 -0600 |
On Tue, 9 Apr 2019 17:58:31 +0200
Eric Auger <address@hidden> wrote:
> The code used to assign an interrupt index/subindex to an
> eventfd is duplicated many times. Let's introduce an helper that
> allows to set/unset the signaling for an ACTION_TRIGGER or
> ACTION_UNMASK action.
>
> Signed-off-by: Eric Auger <address@hidden>
>
> ---
>
> This is a follow-up to
> [PATCH v2 0/2] vfio-pci: Introduce vfio_set_event_handler().
> It looks to me that introducing vfio_set_irq_signaling() has more
> benefits in term of code reduction and the helper abstraction
> looks cleaner.
> ---
> hw/vfio/common.c | 61 +++++++++
> hw/vfio/pci.c | 224 ++++++++--------------------------
> hw/vfio/platform.c | 55 +++------
> include/hw/vfio/vfio-common.h | 2 +
> 4 files changed, 134 insertions(+), 208 deletions(-)
>
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 4374cc6176..f88fd10ca3 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -95,6 +95,67 @@ void vfio_mask_single_irqindex(VFIODevice *vbasedev, int
> index)
> ioctl(vbasedev->fd, VFIO_DEVICE_SET_IRQS, &irq_set);
> }
>
> +static inline const char *action_to_str(int action)
> +{
> + switch (action) {
> + case VFIO_IRQ_SET_ACTION_MASK:
> + return "MASK";
> + case VFIO_IRQ_SET_ACTION_UNMASK:
> + return "UNMASK";
> + case VFIO_IRQ_SET_ACTION_TRIGGER:
> + return "TRIGGER";
> + default:
> + return "UNKNOWN ACTION";
> + }
> +}
> +
> +int vfio_set_irq_signaling(VFIODevice *vbasedev, int index, int subindex,
> + int action, int fd, Error **errp)
> +{
> + struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info),
> + .index = index };
> + struct vfio_irq_set *irq_set;
> + int argsz, ret = 0;
> + int32_t *pfd;
> +
> + ret = ioctl(vbasedev->fd, VFIO_DEVICE_GET_IRQ_INFO, &irq_info);
> + if (ret < 0) {
> + error_setg_errno(errp, errno, "index %d does not exist", index);
> + goto error;
> + }
> + if (irq_info.count < subindex + 1) {
> + error_setg_errno(errp, errno, "subindex %d does not exist",
> subindex);
> + goto error;
> + }
> +
> + argsz = sizeof(*irq_set) + sizeof(*pfd);
> +
> + irq_set = g_malloc0(argsz);
> + irq_set->argsz = argsz;
> + irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD | action;
> + irq_set->index = index;
> + irq_set->start = subindex;
> + irq_set->count = 1;
> + pfd = (int32_t *)&irq_set->data;
> + *pfd = fd;
> +
> + ret = ioctl(vbasedev->fd, VFIO_DEVICE_SET_IRQS, irq_set);
Hi Eric,
Sorry for the long delay. While I like the code reduction and
simplification, is it really acceptable that every SET_IRQS ioctl is
now a GET_IRQ_INFO + SET_IRQS? Are we trying to protect against
devices dynamically changing their interrupt support? Do we not trust
the callers?
> +
> + g_free(irq_set);
> +
> + if (ret) {
> + error_setg_errno(errp, -ret, "VFIO_DEVICE_SET_IRQS failure");
> + goto error;
> + }
> + return 0;
> +error:
> + error_prepend(errp,
> + "Failed to %s %s eventfd signaling for interrupt [%d,%d]:
> ",
> + fd < 0 ? "tear down" : "set up", action_to_str(action),
> + index, subindex);
Maybe icing on the cake, but this leaves me wishing it printed "MSIX-3"
rather than "[2,3]" for a PCI device ;)
> + return ret;
> +}
> +
> /*
> * IO Port/MMIO - Beware of the endians, VFIO is always little endian
> */
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 504019c458..cd93ff6fa3 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
[snip]
> @@ -2718,77 +2630,43 @@ static void vfio_req_notifier_handler(void *opaque)
>
> static void vfio_register_req_notifier(VFIOPCIDevice *vdev)
> {
> - struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info),
> - .index = VFIO_PCI_REQ_IRQ_INDEX };
> - int argsz;
> - struct vfio_irq_set *irq_set;
> - int32_t *pfd;
> + Error *err = NULL;
> + int32_t fd;
>
> if (!(vdev->features & VFIO_FEATURE_ENABLE_REQ)) {
> return;
> }
>
> - if (ioctl(vdev->vbasedev.fd,
> - VFIO_DEVICE_GET_IRQ_INFO, &irq_info) < 0 || irq_info.count <
> 1) {
> - return;
> - }
Here we used GET_IRQ_INFO to quietly only enable the request notifier
when it's available, now it looks like this is no longer quiet if that
support is unavailable. Is that intentional? Thanks,
Alex
> -
> if (event_notifier_init(&vdev->req_notifier, 0)) {
> error_report("vfio: Unable to init event notifier for device
> request");
> return;
> }
>
> - argsz = sizeof(*irq_set) + sizeof(*pfd);
> + fd = event_notifier_get_fd(&vdev->req_notifier);
> + qemu_set_fd_handler(fd, vfio_req_notifier_handler, NULL, vdev);
>
> - irq_set = g_malloc0(argsz);
> - irq_set->argsz = argsz;
> - irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD |
> - VFIO_IRQ_SET_ACTION_TRIGGER;
> - irq_set->index = VFIO_PCI_REQ_IRQ_INDEX;
> - irq_set->start = 0;
> - irq_set->count = 1;
> - pfd = (int32_t *)&irq_set->data;
> -
> - *pfd = event_notifier_get_fd(&vdev->req_notifier);
> - qemu_set_fd_handler(*pfd, vfio_req_notifier_handler, NULL, vdev);
> -
> - if (ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set)) {
> - error_report("vfio: Failed to set up device request notification");
> - qemu_set_fd_handler(*pfd, NULL, NULL, vdev);
> + if (vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_REQ_IRQ_INDEX, 0,
> + VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err)) {
> + error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
> + qemu_set_fd_handler(fd, NULL, NULL, vdev);
> event_notifier_cleanup(&vdev->req_notifier);
> } else {
> vdev->req_enabled = true;
> }
> -
> - g_free(irq_set);
> }
- Re: [Qemu-devel] [PATCH for-4.1] vfio/common: Introduce vfio_set_irq_signaling helper,
Alex Williamson <=