* [Qemu-devel] [PATCH for-4.1] vfio/common: Introduce vfio_set_irq_signaling helper
@ 2019-04-09 15:58 Eric Auger
2019-04-12 11:31 ` Cornelia Huck
2019-05-15 22:52 ` Alex Williamson
0 siblings, 2 replies; 6+ messages in thread
From: Eric Auger @ 2019-04-09 15:58 UTC (permalink / raw)
To: eric.auger.pro, eric.auger, qemu-devel, alex.williamson; +Cc: aik, cohuck
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 <eric.auger@redhat.com>
---
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);
+
+ 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);
+ 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
@@ -113,9 +113,7 @@ static void vfio_intx_enable_kvm(VFIOPCIDevice *vdev, Error **errp)
.gsi = vdev->intx.route.irq,
.flags = KVM_IRQFD_FLAG_RESAMPLE,
};
- struct vfio_irq_set *irq_set;
- int ret, argsz;
- int32_t *pfd;
+ Error *err = NULL;
if (vdev->no_kvm_intx || !kvm_irqfds_enabled() ||
vdev->intx.route.mode != PCI_INTX_ENABLED ||
@@ -143,22 +141,10 @@ static void vfio_intx_enable_kvm(VFIOPCIDevice *vdev, Error **errp)
goto fail_irqfd;
}
- argsz = sizeof(*irq_set) + sizeof(*pfd);
-
- irq_set = g_malloc0(argsz);
- irq_set->argsz = argsz;
- irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD | VFIO_IRQ_SET_ACTION_UNMASK;
- irq_set->index = VFIO_PCI_INTX_IRQ_INDEX;
- irq_set->start = 0;
- irq_set->count = 1;
- pfd = (int32_t *)&irq_set->data;
-
- *pfd = irqfd.resamplefd;
-
- ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
- g_free(irq_set);
- if (ret) {
- error_setg_errno(errp, -ret, "failed to setup INTx unmask fd");
+ if (vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_INTX_IRQ_INDEX, 0,
+ VFIO_IRQ_SET_ACTION_UNMASK,
+ irqfd.resamplefd, &err)) {
+ error_propagate(errp, err);
goto fail_vfio;
}
@@ -262,10 +248,10 @@ static void vfio_intx_update(PCIDevice *pdev)
static int vfio_intx_enable(VFIOPCIDevice *vdev, Error **errp)
{
uint8_t pin = vfio_pci_read_config(&vdev->pdev, PCI_INTERRUPT_PIN, 1);
- int ret, argsz, retval = 0;
- struct vfio_irq_set *irq_set;
- int32_t *pfd;
Error *err = NULL;
+ int32_t fd;
+ int ret;
+
if (!pin) {
return 0;
@@ -292,27 +278,15 @@ static int vfio_intx_enable(VFIOPCIDevice *vdev, Error **errp)
error_setg_errno(errp, -ret, "event_notifier_init failed");
return ret;
}
+ fd = event_notifier_get_fd(&vdev->intx.interrupt);
+ qemu_set_fd_handler(fd, vfio_intx_interrupt, NULL, vdev);
- argsz = sizeof(*irq_set) + sizeof(*pfd);
-
- 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_INTX_IRQ_INDEX;
- irq_set->start = 0;
- irq_set->count = 1;
- pfd = (int32_t *)&irq_set->data;
-
- *pfd = event_notifier_get_fd(&vdev->intx.interrupt);
- qemu_set_fd_handler(*pfd, vfio_intx_interrupt, NULL, vdev);
-
- ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
- if (ret) {
- error_setg_errno(errp, -ret, "failed to setup INTx fd");
- qemu_set_fd_handler(*pfd, NULL, NULL, vdev);
+ if (vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_INTX_IRQ_INDEX, 0,
+ VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err)) {
+ error_propagate(errp, err);
+ qemu_set_fd_handler(fd, NULL, NULL, vdev);
event_notifier_cleanup(&vdev->intx.interrupt);
- retval = -errno;
- goto cleanup;
+ return -errno;
}
vfio_intx_enable_kvm(vdev, &err);
@@ -323,11 +297,7 @@ static int vfio_intx_enable(VFIOPCIDevice *vdev, Error **errp)
vdev->interrupt = VFIO_INT_INTx;
trace_vfio_intx_enable(vdev->vbasedev.name);
-
-cleanup:
- g_free(irq_set);
-
- return retval;
+ return 0;
}
static void vfio_intx_disable(VFIOPCIDevice *vdev)
@@ -530,31 +500,19 @@ static int vfio_msix_vector_do_use(PCIDevice *pdev, unsigned int nr,
error_report("vfio: failed to enable vectors, %d", ret);
}
} else {
- int argsz;
- struct vfio_irq_set *irq_set;
- int32_t *pfd;
-
- argsz = sizeof(*irq_set) + sizeof(*pfd);
-
- 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_MSIX_IRQ_INDEX;
- irq_set->start = nr;
- irq_set->count = 1;
- pfd = (int32_t *)&irq_set->data;
+ Error *err = NULL;
+ int32_t fd;
if (vector->virq >= 0) {
- *pfd = event_notifier_get_fd(&vector->kvm_interrupt);
+ fd = event_notifier_get_fd(&vector->kvm_interrupt);
} else {
- *pfd = event_notifier_get_fd(&vector->interrupt);
+ fd = event_notifier_get_fd(&vector->interrupt);
}
- ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
- g_free(irq_set);
- if (ret) {
- error_report("vfio: failed to modify vector, %d", ret);
+ if (vfio_set_irq_signaling(&vdev->vbasedev,
+ VFIO_PCI_MSIX_IRQ_INDEX, nr,
+ VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err)) {
+ error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
}
}
@@ -591,26 +549,10 @@ static void vfio_msix_vector_release(PCIDevice *pdev, unsigned int nr)
* be re-asserted on unmask. Nothing to do if already using QEMU mode.
*/
if (vector->virq >= 0) {
- int argsz;
- struct vfio_irq_set *irq_set;
- int32_t *pfd;
+ int32_t fd = event_notifier_get_fd(&vector->interrupt);
- argsz = sizeof(*irq_set) + sizeof(*pfd);
-
- 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_MSIX_IRQ_INDEX;
- irq_set->start = nr;
- irq_set->count = 1;
- pfd = (int32_t *)&irq_set->data;
-
- *pfd = event_notifier_get_fd(&vector->interrupt);
-
- ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
-
- g_free(irq_set);
+ vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_MSIX_IRQ_INDEX, nr,
+ VFIO_IRQ_SET_ACTION_TRIGGER, fd, NULL);
}
}
@@ -2629,10 +2571,8 @@ static void vfio_err_notifier_handler(void *opaque)
*/
static void vfio_register_err_notifier(VFIOPCIDevice *vdev)
{
- int ret;
- int argsz;
- struct vfio_irq_set *irq_set;
- int32_t *pfd;
+ Error *err = NULL;
+ int32_t fd;
if (!vdev->pci_aer) {
return;
@@ -2644,58 +2584,30 @@ static void vfio_register_err_notifier(VFIOPCIDevice *vdev)
return;
}
- argsz = sizeof(*irq_set) + sizeof(*pfd);
+ fd = event_notifier_get_fd(&vdev->err_notifier);
+ qemu_set_fd_handler(fd, vfio_err_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_ERR_IRQ_INDEX;
- irq_set->start = 0;
- irq_set->count = 1;
- pfd = (int32_t *)&irq_set->data;
-
- *pfd = event_notifier_get_fd(&vdev->err_notifier);
- qemu_set_fd_handler(*pfd, vfio_err_notifier_handler, NULL, vdev);
-
- ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
- if (ret) {
- error_report("vfio: Failed to set up error notification");
- qemu_set_fd_handler(*pfd, NULL, NULL, vdev);
+ if (vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_ERR_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->err_notifier);
vdev->pci_aer = false;
}
- g_free(irq_set);
}
static void vfio_unregister_err_notifier(VFIOPCIDevice *vdev)
{
- int argsz;
- struct vfio_irq_set *irq_set;
- int32_t *pfd;
- int ret;
+ Error *err = NULL;
if (!vdev->pci_aer) {
return;
}
- argsz = sizeof(*irq_set) + sizeof(*pfd);
-
- 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_ERR_IRQ_INDEX;
- irq_set->start = 0;
- irq_set->count = 1;
- pfd = (int32_t *)&irq_set->data;
- *pfd = -1;
-
- ret = ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set);
- if (ret) {
- error_report("vfio: Failed to de-assign error fd: %m");
+ if (vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_ERR_IRQ_INDEX, 0,
+ VFIO_IRQ_SET_ACTION_TRIGGER, -1, &err)) {
+ error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
}
- g_free(irq_set);
qemu_set_fd_handler(event_notifier_get_fd(&vdev->err_notifier),
NULL, NULL, vdev);
event_notifier_cleanup(&vdev->err_notifier);
@@ -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;
- }
-
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);
}
static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev)
{
- int argsz;
- struct vfio_irq_set *irq_set;
- int32_t *pfd;
+ Error *err = NULL;
if (!vdev->req_enabled) {
return;
}
- argsz = sizeof(*irq_set) + sizeof(*pfd);
-
- 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 = -1;
-
- if (ioctl(vdev->vbasedev.fd, VFIO_DEVICE_SET_IRQS, irq_set)) {
- error_report("vfio: Failed to de-assign device request fd: %m");
+ if (vfio_set_irq_signaling(&vdev->vbasedev, VFIO_PCI_REQ_IRQ_INDEX, 0,
+ VFIO_IRQ_SET_ACTION_TRIGGER, -1, &err)) {
+ error_reportf_err(err, VFIO_MSG_PREFIX, vdev->vbasedev.name);
}
- g_free(irq_set);
qemu_set_fd_handler(event_notifier_get_fd(&vdev->req_notifier),
NULL, NULL, vdev);
event_notifier_cleanup(&vdev->req_notifier);
diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
index 398db38f14..d60f391439 100644
--- a/hw/vfio/platform.c
+++ b/hw/vfio/platform.c
@@ -106,26 +106,19 @@ static int vfio_set_trigger_eventfd(VFIOINTp *intp,
eventfd_user_side_handler_t handler)
{
VFIODevice *vbasedev = &intp->vdev->vbasedev;
- struct vfio_irq_set *irq_set;
- int argsz, ret;
- int32_t *pfd;
+ int32_t fd = event_notifier_get_fd(intp->interrupt);
+ Error *err = NULL;
+ int ret;
- argsz = sizeof(*irq_set) + sizeof(*pfd);
- 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 = intp->pin;
- irq_set->start = 0;
- irq_set->count = 1;
- pfd = (int32_t *)&irq_set->data;
- *pfd = event_notifier_get_fd(intp->interrupt);
- qemu_set_fd_handler(*pfd, (IOHandler *)handler, NULL, intp);
- ret = ioctl(vbasedev->fd, VFIO_DEVICE_SET_IRQS, irq_set);
- if (ret < 0) {
- error_report("vfio: Failed to set trigger eventfd: %m");
- qemu_set_fd_handler(*pfd, NULL, NULL, NULL);
+ qemu_set_fd_handler(fd, (IOHandler *)handler, NULL, intp);
+
+ ret = vfio_set_irq_signaling(vbasedev, intp->pin, 0,
+ VFIO_IRQ_SET_ACTION_TRIGGER, fd, &err);
+ if (ret) {
+ error_reportf_err(err, VFIO_MSG_PREFIX, vbasedev->name);
+ qemu_set_fd_handler(fd, NULL, NULL, NULL);
}
- g_free(irq_set);
+
return ret;
}
@@ -361,26 +354,18 @@ static void vfio_start_eventfd_injection(SysBusDevice *sbdev, qemu_irq irq)
*/
static int vfio_set_resample_eventfd(VFIOINTp *intp)
{
+ int32_t fd = event_notifier_get_fd(intp->unmask);
VFIODevice *vbasedev = &intp->vdev->vbasedev;
- struct vfio_irq_set *irq_set;
- int argsz, ret;
- int32_t *pfd;
+ Error *err = NULL;
+ int ret;
- argsz = sizeof(*irq_set) + sizeof(*pfd);
- irq_set = g_malloc0(argsz);
- irq_set->argsz = argsz;
- irq_set->flags = VFIO_IRQ_SET_DATA_EVENTFD | VFIO_IRQ_SET_ACTION_UNMASK;
- irq_set->index = intp->pin;
- irq_set->start = 0;
- irq_set->count = 1;
- pfd = (int32_t *)&irq_set->data;
- *pfd = event_notifier_get_fd(intp->unmask);
- qemu_set_fd_handler(*pfd, NULL, NULL, NULL);
- ret = ioctl(vbasedev->fd, VFIO_DEVICE_SET_IRQS, irq_set);
- g_free(irq_set);
- if (ret < 0) {
- error_report("vfio: Failed to set resample eventfd: %m");
+ qemu_set_fd_handler(fd, NULL, NULL, NULL);
+ ret = vfio_set_irq_signaling(vbasedev, intp->pin, 0,
+ VFIO_IRQ_SET_ACTION_UNMASK, fd, &err);
+ if (ret) {
+ error_reportf_err(err, VFIO_MSG_PREFIX, vbasedev->name);
}
+
return ret;
}
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index 1155b79678..686d99ff8c 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -167,6 +167,8 @@ void vfio_put_base_device(VFIODevice *vbasedev);
void vfio_disable_irqindex(VFIODevice *vbasedev, int index);
void vfio_unmask_single_irqindex(VFIODevice *vbasedev, int index);
void vfio_mask_single_irqindex(VFIODevice *vbasedev, int index);
+int vfio_set_irq_signaling(VFIODevice *vbasedev, int index, int subindex,
+ int action, int fd, Error **errp);
void vfio_region_write(void *opaque, hwaddr addr,
uint64_t data, unsigned size);
uint64_t vfio_region_read(void *opaque,
--
2.20.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH for-4.1] vfio/common: Introduce vfio_set_irq_signaling helper
@ 2019-04-12 11:31 ` Cornelia Huck
0 siblings, 0 replies; 6+ messages in thread
From: Cornelia Huck @ 2019-04-12 11:31 UTC (permalink / raw)
To: Eric Auger; +Cc: eric.auger.pro, qemu-devel, alex.williamson, aik
On Tue, 9 Apr 2019 17:58:31 +0200
Eric Auger <eric.auger@redhat.com> 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.
I like that, and it looks like ccw can use the new function as well. (I
can do a patch on top.)
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>
> ---
>
> 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(-)
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH for-4.1] vfio/common: Introduce vfio_set_irq_signaling helper
@ 2019-04-12 11:31 ` Cornelia Huck
0 siblings, 0 replies; 6+ messages in thread
From: Cornelia Huck @ 2019-04-12 11:31 UTC (permalink / raw)
To: Eric Auger; +Cc: aik, alex.williamson, qemu-devel, eric.auger.pro
On Tue, 9 Apr 2019 17:58:31 +0200
Eric Auger <eric.auger@redhat.com> 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.
I like that, and it looks like ccw can use the new function as well. (I
can do a patch on top.)
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>
> ---
>
> 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(-)
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH for-4.1] vfio/common: Introduce vfio_set_irq_signaling helper
2019-04-12 11:31 ` Cornelia Huck
(?)
@ 2019-04-23 15:11 ` Auger Eric
-1 siblings, 0 replies; 6+ messages in thread
From: Auger Eric @ 2019-04-23 15:11 UTC (permalink / raw)
To: Cornelia Huck; +Cc: aik, alex.williamson, qemu-devel, eric.auger.pro
Hi Cornelia
On 4/12/19 1:31 PM, Cornelia Huck wrote:
> On Tue, 9 Apr 2019 17:58:31 +0200
> Eric Auger <eric.auger@redhat.com> 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.
>
> I like that, and it looks like ccw can use the new function as well. (I
> can do a patch on top.)
Thanks! Yes, feel free to proceed with the ccw patch once this gets
merged.
>
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>
>> ---
>>
>> 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(-)
>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Thanks
Eric
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH for-4.1] vfio/common: Introduce vfio_set_irq_signaling helper
2019-04-09 15:58 [Qemu-devel] [PATCH for-4.1] vfio/common: Introduce vfio_set_irq_signaling helper Eric Auger
2019-04-12 11:31 ` Cornelia Huck
@ 2019-05-15 22:52 ` Alex Williamson
2019-05-20 14:17 ` Auger Eric
1 sibling, 1 reply; 6+ messages in thread
From: Alex Williamson @ 2019-05-15 22:52 UTC (permalink / raw)
To: Eric Auger; +Cc: aik, cohuck, qemu-devel, eric.auger.pro
On Tue, 9 Apr 2019 17:58:31 +0200
Eric Auger <eric.auger@redhat.com> 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 <eric.auger@redhat.com>
>
> ---
>
> 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);
> }
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH for-4.1] vfio/common: Introduce vfio_set_irq_signaling helper
2019-05-15 22:52 ` Alex Williamson
@ 2019-05-20 14:17 ` Auger Eric
0 siblings, 0 replies; 6+ messages in thread
From: Auger Eric @ 2019-05-20 14:17 UTC (permalink / raw)
To: Alex Williamson; +Cc: aik, cohuck, qemu-devel, eric.auger.pro
Hi Alex,
On 5/16/19 12:52 AM, Alex Williamson wrote:
> On Tue, 9 Apr 2019 17:58:31 +0200
> Eric Auger <eric.auger@redhat.com> 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 <eric.auger@redhat.com>
>>
>> ---
>>
>> 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?
I agree this is generally not needed. I will remove the check.
>
>> +
>> + 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 ;)
OK
>
>
>> + 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,
not really I acknowledge ;-) I will restore that quiet check here.
Thanks
Eric
>
> 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);
>> }
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-05-20 14:20 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-09 15:58 [Qemu-devel] [PATCH for-4.1] vfio/common: Introduce vfio_set_irq_signaling helper Eric Auger
2019-04-12 11:31 ` Cornelia Huck
2019-04-12 11:31 ` Cornelia Huck
2019-04-23 15:11 ` Auger Eric
2019-05-15 22:52 ` Alex Williamson
2019-05-20 14:17 ` Auger Eric
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.