* [Qemu-devel] [PATCH v5] vfio/common: Introduce vfio_set_irq_signaling helper
@ 2019-06-12 12:40 Eric Auger
2019-06-12 16:46 ` Cornelia Huck
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Eric Auger @ 2019-06-12 12:40 UTC (permalink / raw)
To: eric.auger.pro, eric.auger, qemu-devel, alex.williamson; +Cc: cohuck, liq3ea
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,
ACTION_MASK or ACTION_UNMASK action.
In the error message, we now use errno in case of any
VFIO_DEVICE_SET_IRQS ioctl failure.
Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
v4 -> v5:
- output errno instead of ioctl returned value. Issue
reported by Li
- vfio_set_irq_signaling now returns -errno in case
of failure
- update the commit message
v3 -> v4:
- renamed irq_to_str into index_to_str
- avoid usage of g_strdup_printf
- print both the index and subindex in generic case
v2 -> v3:
- irq_to_str() simply outputs the IRQ index if the VFIO device is
not of PCI type
- removed "vfio: failed to start eventfd signaling ../.." in vfio_platform
v1 -> v2:
- don't call GET_IRQ_INFO in vfio_set_irq_signaling()
and restore quiet check in vfio_register_req_notifier.
Nicer display of the IRQ name.
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 | 78 ++++++++++++
hw/vfio/pci.c | 217 ++++++++--------------------------
hw/vfio/platform.c | 62 ++++------
include/hw/vfio/vfio-common.h | 2 +
4 files changed, 151 insertions(+), 208 deletions(-)
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 4374cc6176..a859298fda 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -95,6 +95,84 @@ 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";
+ }
+}
+
+static const char *index_to_str(VFIODevice *vbasedev, int index)
+{
+ if (vbasedev->type != VFIO_DEVICE_TYPE_PCI) {
+ return NULL;
+ }
+
+ switch (index) {
+ case VFIO_PCI_INTX_IRQ_INDEX:
+ return "INTX";
+ case VFIO_PCI_MSI_IRQ_INDEX:
+ return "MSI";
+ case VFIO_PCI_MSIX_IRQ_INDEX:
+ return "MSIX";
+ case VFIO_PCI_ERR_IRQ_INDEX:
+ return "ERR";
+ case VFIO_PCI_REQ_IRQ_INDEX:
+ return "REQ";
+ default:
+ return NULL;
+ }
+}
+
+int vfio_set_irq_signaling(VFIODevice *vbasedev, int index, int subindex,
+ int action, int fd, Error **errp)
+{
+ struct vfio_irq_set *irq_set;
+ int argsz, ret = 0;
+ const char *name;
+ 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 | action;
+ irq_set->index = index;
+ irq_set->start = subindex;
+ irq_set->count = 1;
+ pfd = (int32_t *)&irq_set->data;
+ *pfd = fd;
+
+ if (ioctl(vbasedev->fd, VFIO_DEVICE_SET_IRQS, irq_set)) {
+ ret = -errno;
+ }
+ g_free(irq_set);
+
+ if (!ret) {
+ return 0;
+ }
+
+ error_setg_errno(errp, -ret, "VFIO_DEVICE_SET_IRQS failure");
+
+ name = index_to_str(vbasedev, index);
+ if (name) {
+ error_prepend(errp, "%s-%d: ", name, subindex);
+ } else {
+ error_prepend(errp, "index %d-%d: ", index, subindex);
+ }
+ error_prepend(errp,
+ "Failed to %s %s eventfd signaling for interrupt ",
+ fd < 0 ? "tear down" : "set up", action_to_str(action));
+ 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 2a4091d216..65c2e17028 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);
}
}
@@ -2636,10 +2578,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;
@@ -2651,58 +2591,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);
@@ -2727,9 +2639,8 @@ 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;
@@ -2745,57 +2656,31 @@ static void vfio_register_req_notifier(VFIOPCIDevice *vdev)
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 d52d6552e0..8d6012857e 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;
}
@@ -330,7 +323,6 @@ static void vfio_platform_eoi(VFIODevice *vbasedev)
static void vfio_start_eventfd_injection(SysBusDevice *sbdev, qemu_irq irq)
{
- int ret;
VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(sbdev);
VFIOINTp *intp;
@@ -341,10 +333,7 @@ static void vfio_start_eventfd_injection(SysBusDevice *sbdev, qemu_irq irq)
}
assert(intp);
- ret = vfio_set_trigger_eventfd(intp, vfio_intp_interrupt);
- if (ret) {
- error_report("vfio: failed to start eventfd signaling for IRQ %d: %m",
- intp->pin);
+ if (vfio_set_trigger_eventfd(intp, vfio_intp_interrupt)) {
abort();
}
}
@@ -361,25 +350,16 @@ 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;
}
@@ -435,8 +415,6 @@ static void vfio_start_irqfd_injection(SysBusDevice *sbdev, qemu_irq irq)
return;
fail_vfio:
kvm_irqchip_remove_irqfd_notifier(kvm_state, intp->interrupt, irq);
- error_report("vfio: failed to start eventfd signaling for IRQ %d: %m",
- intp->pin);
abort();
fail_irqfd:
vfio_start_eventfd_injection(sbdev, irq);
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 v5] vfio/common: Introduce vfio_set_irq_signaling helper
2019-06-12 12:40 [Qemu-devel] [PATCH v5] vfio/common: Introduce vfio_set_irq_signaling helper Eric Auger
@ 2019-06-12 16:46 ` Cornelia Huck
2019-06-13 10:05 ` Auger Eric
2019-06-12 23:40 ` Li Qiang
2019-06-13 21:35 ` Alex Williamson
2 siblings, 1 reply; 6+ messages in thread
From: Cornelia Huck @ 2019-06-12 16:46 UTC (permalink / raw)
To: Eric Auger; +Cc: alex.williamson, liq3ea, qemu-devel, eric.auger.pro
On Wed, 12 Jun 2019 14:40:04 +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,
> ACTION_MASK or ACTION_UNMASK action.
>
> In the error message, we now use errno in case of any
> VFIO_DEVICE_SET_IRQS ioctl failure.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
Looks good to me.
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
I currently have the following, still untested patch on top (vfio-ap
doesn't use this ioctl):
From 9c2efe73d6139e8c7b2109ac2df79fe073d942fb Mon Sep 17 00:00:00 2001
From: Cornelia Huck <cohuck@redhat.com>
Date: Wed, 12 Jun 2019 18:42:29 +0200
Subject: [PATCH] vfio-ccw: use vfio_set_irq_signaling()
Make use of the new helper.
Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
hw/vfio/ccw.c | 50 +++++++++++++-------------------------------------
1 file changed, 13 insertions(+), 37 deletions(-)
diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
index 03a2becb3ec9..3643be0ee254 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -198,9 +198,9 @@ static void vfio_ccw_register_io_notifier(VFIOCCWDevice *vcdev, Error **errp)
{
VFIODevice *vdev = &vcdev->vdev;
struct vfio_irq_info *irq_info;
- struct vfio_irq_set *irq_set;
size_t argsz;
- int32_t *pfd;
+ int fd;
+ Error *local_err = NULL;
if (vdev->num_irqs < VFIO_CCW_IO_IRQ_INDEX + 1) {
error_setg(errp, "vfio: unexpected number of io irqs %u",
@@ -224,56 +224,32 @@ static void vfio_ccw_register_io_notifier(VFIOCCWDevice *vcdev, Error **errp)
goto out_free_info;
}
- 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_CCW_IO_IRQ_INDEX;
- irq_set->start = 0;
- irq_set->count = 1;
- pfd = (int32_t *) &irq_set->data;
-
- *pfd = event_notifier_get_fd(&vcdev->io_notifier);
- qemu_set_fd_handler(*pfd, vfio_ccw_io_notifier_handler, NULL, vcdev);
- if (ioctl(vdev->fd, VFIO_DEVICE_SET_IRQS, irq_set)) {
- error_setg(errp, "vfio: Failed to set up io notification");
- qemu_set_fd_handler(*pfd, NULL, NULL, vcdev);
+ fd = event_notifier_get_fd(&vcdev->io_notifier);
+ qemu_set_fd_handler(fd, vfio_ccw_io_notifier_handler, NULL, vcdev);
+ if (vfio_set_irq_signaling(vdev, VFIO_CCW_IO_IRQ_INDEX, 0,
+ VFIO_IRQ_SET_ACTION_TRIGGER, fd, &local_err)) {
+ error_propagate(errp, local_err);
+ qemu_set_fd_handler(fd, NULL, NULL, vcdev);
event_notifier_cleanup(&vcdev->io_notifier);
}
- g_free(irq_set);
-
out_free_info:
g_free(irq_info);
}
static void vfio_ccw_unregister_io_notifier(VFIOCCWDevice *vcdev)
{
- struct vfio_irq_set *irq_set;
- size_t argsz;
- 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_CCW_IO_IRQ_INDEX;
- irq_set->start = 0;
- irq_set->count = 1;
- pfd = (int32_t *) &irq_set->data;
- *pfd = -1;
-
- if (ioctl(vcdev->vdev.fd, VFIO_DEVICE_SET_IRQS, irq_set)) {
- error_report("vfio: Failed to de-assign device io fd: %m");
+ Error *local_err = NULL;
+
+ if (vfio_set_irq_signaling(&vcdev->vdev, VFIO_CCW_IO_IRQ_INDEX, 0,
+ VFIO_IRQ_SET_ACTION_TRIGGER, -1, &local_err)) {
+ error_report_err(local_err);
}
qemu_set_fd_handler(event_notifier_get_fd(&vcdev->io_notifier),
NULL, NULL, vcdev);
event_notifier_cleanup(&vcdev->io_notifier);
- g_free(irq_set);
}
static void vfio_ccw_get_region(VFIOCCWDevice *vcdev, Error **errp)
--
2.20.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v5] vfio/common: Introduce vfio_set_irq_signaling helper
2019-06-12 12:40 [Qemu-devel] [PATCH v5] vfio/common: Introduce vfio_set_irq_signaling helper Eric Auger
2019-06-12 16:46 ` Cornelia Huck
@ 2019-06-12 23:40 ` Li Qiang
2019-06-13 10:06 ` Auger Eric
2019-06-13 21:35 ` Alex Williamson
2 siblings, 1 reply; 6+ messages in thread
From: Li Qiang @ 2019-06-12 23:40 UTC (permalink / raw)
To: Eric Auger; +Cc: Alex Williamson, cohuck, Qemu Developers, eric.auger.pro
Eric Auger <eric.auger@redhat.com> 于2019年6月12日周三 下午8:40写道：
> 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,
> ACTION_MASK or ACTION_UNMASK action.
>
> In the error message, we now use errno in case of any
> VFIO_DEVICE_SET_IRQS ioctl failure.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>
>
Reviewed-by: Li Qiang <liq3ea@gmail.com>
> ---
>
> v4 -> v5:
> - output errno instead of ioctl returned value. Issue
> reported by Li
> - vfio_set_irq_signaling now returns -errno in case
> of failure
> - update the commit message
>
> v3 -> v4:
> - renamed irq_to_str into index_to_str
> - avoid usage of g_strdup_printf
> - print both the index and subindex in generic case
>
> v2 -> v3:
> - irq_to_str() simply outputs the IRQ index if the VFIO device is
> not of PCI type
> - removed "vfio: failed to start eventfd signaling ../.." in vfio_platform
>
> v1 -> v2:
> - don't call GET_IRQ_INFO in vfio_set_irq_signaling()
> and restore quiet check in vfio_register_req_notifier.
> Nicer display of the IRQ name.
>
> 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 | 78 ++++++++++++
> hw/vfio/pci.c | 217 ++++++++--------------------------
> hw/vfio/platform.c | 62 ++++------
> include/hw/vfio/vfio-common.h | 2 +
> 4 files changed, 151 insertions(+), 208 deletions(-)
>
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 4374cc6176..a859298fda 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -95,6 +95,84 @@ 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";
> + }
> +}
> +
> +static const char *index_to_str(VFIODevice *vbasedev, int index)
> +{
> + if (vbasedev->type != VFIO_DEVICE_TYPE_PCI) {
> + return NULL;
> + }
> +
> + switch (index) {
> + case VFIO_PCI_INTX_IRQ_INDEX:
> + return "INTX";
> + case VFIO_PCI_MSI_IRQ_INDEX:
> + return "MSI";
> + case VFIO_PCI_MSIX_IRQ_INDEX:
> + return "MSIX";
> + case VFIO_PCI_ERR_IRQ_INDEX:
> + return "ERR";
> + case VFIO_PCI_REQ_IRQ_INDEX:
> + return "REQ";
> + default:
> + return NULL;
> + }
> +}
> +
> +int vfio_set_irq_signaling(VFIODevice *vbasedev, int index, int subindex,
> + int action, int fd, Error **errp)
> +{
> + struct vfio_irq_set *irq_set;
> + int argsz, ret = 0;
> + const char *name;
> + 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 | action;
> + irq_set->index = index;
> + irq_set->start = subindex;
> + irq_set->count = 1;
> + pfd = (int32_t *)&irq_set->data;
> + *pfd = fd;
> +
> + if (ioctl(vbasedev->fd, VFIO_DEVICE_SET_IRQS, irq_set)) {
> + ret = -errno;
> + }
> + g_free(irq_set);
> +
> + if (!ret) {
> + return 0;
> + }
> +
> + error_setg_errno(errp, -ret, "VFIO_DEVICE_SET_IRQS failure");
> +
> + name = index_to_str(vbasedev, index);
> + if (name) {
> + error_prepend(errp, "%s-%d: ", name, subindex);
> + } else {
> + error_prepend(errp, "index %d-%d: ", index, subindex);
> + }
> + error_prepend(errp,
> + "Failed to %s %s eventfd signaling for interrupt ",
> + fd < 0 ? "tear down" : "set up", action_to_str(action));
> + 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 2a4091d216..65c2e17028 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);
> }
> }
>
> @@ -2636,10 +2578,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;
> @@ -2651,58 +2591,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);
> @@ -2727,9 +2639,8 @@ 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;
> @@ -2745,57 +2656,31 @@ static void
> vfio_register_req_notifier(VFIOPCIDevice *vdev)
> 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 d52d6552e0..8d6012857e 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;
> }
>
> @@ -330,7 +323,6 @@ static void vfio_platform_eoi(VFIODevice *vbasedev)
>
> static void vfio_start_eventfd_injection(SysBusDevice *sbdev, qemu_irq
> irq)
> {
> - int ret;
> VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(sbdev);
> VFIOINTp *intp;
>
> @@ -341,10 +333,7 @@ static void vfio_start_eventfd_injection(SysBusDevice
> *sbdev, qemu_irq irq)
> }
> assert(intp);
>
> - ret = vfio_set_trigger_eventfd(intp, vfio_intp_interrupt);
> - if (ret) {
> - error_report("vfio: failed to start eventfd signaling for IRQ %d:
> %m",
> - intp->pin);
> + if (vfio_set_trigger_eventfd(intp, vfio_intp_interrupt)) {
> abort();
> }
> }
> @@ -361,25 +350,16 @@ 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;
> }
> @@ -435,8 +415,6 @@ static void vfio_start_irqfd_injection(SysBusDevice
> *sbdev, qemu_irq irq)
> return;
> fail_vfio:
> kvm_irqchip_remove_irqfd_notifier(kvm_state, intp->interrupt, irq);
> - error_report("vfio: failed to start eventfd signaling for IRQ %d: %m",
> - intp->pin);
> abort();
> fail_irqfd:
> vfio_start_eventfd_injection(sbdev, irq);
> 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 [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v5] vfio/common: Introduce vfio_set_irq_signaling helper
2019-06-12 16:46 ` Cornelia Huck
@ 2019-06-13 10:05 ` Auger Eric
0 siblings, 0 replies; 6+ messages in thread
From: Auger Eric @ 2019-06-13 10:05 UTC (permalink / raw)
To: Cornelia Huck; +Cc: alex.williamson, liq3ea, qemu-devel, eric.auger.pro
Hi Connie,
On 6/12/19 6:46 PM, Cornelia Huck wrote:
> On Wed, 12 Jun 2019 14:40:04 +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,
>> ACTION_MASK or ACTION_UNMASK action.
>>
>> In the error message, we now use errno in case of any
>> VFIO_DEVICE_SET_IRQS ioctl failure.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>
> Looks good to me.
>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
thank you!
>
> I currently have the following, still untested patch on top (vfio-ap
> doesn't use this ioctl):
>
> From 9c2efe73d6139e8c7b2109ac2df79fe073d942fb Mon Sep 17 00:00:00 2001
> From: Cornelia Huck <cohuck@redhat.com>
> Date: Wed, 12 Jun 2019 18:42:29 +0200
> Subject: [PATCH] vfio-ccw: use vfio_set_irq_signaling()
>
> Make use of the new helper.
>
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
> hw/vfio/ccw.c | 50 +++++++++++++-------------------------------------
> 1 file changed, 13 insertions(+), 37 deletions(-)
>
> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
> index 03a2becb3ec9..3643be0ee254 100644
> --- a/hw/vfio/ccw.c
> +++ b/hw/vfio/ccw.c
> @@ -198,9 +198,9 @@ static void vfio_ccw_register_io_notifier(VFIOCCWDevice *vcdev, Error **errp)
> {
> VFIODevice *vdev = &vcdev->vdev;
> struct vfio_irq_info *irq_info;
> - struct vfio_irq_set *irq_set;
> size_t argsz;
> - int32_t *pfd;
> + int fd;
> + Error *local_err = NULL;
>
> if (vdev->num_irqs < VFIO_CCW_IO_IRQ_INDEX + 1) {
> error_setg(errp, "vfio: unexpected number of io irqs %u",
> @@ -224,56 +224,32 @@ static void vfio_ccw_register_io_notifier(VFIOCCWDevice *vcdev, Error **errp)
> goto out_free_info;
> }
>
> - 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_CCW_IO_IRQ_INDEX;
> - irq_set->start = 0;
> - irq_set->count = 1;
> - pfd = (int32_t *) &irq_set->data;
> -
> - *pfd = event_notifier_get_fd(&vcdev->io_notifier);
> - qemu_set_fd_handler(*pfd, vfio_ccw_io_notifier_handler, NULL, vcdev);
> - if (ioctl(vdev->fd, VFIO_DEVICE_SET_IRQS, irq_set)) {
> - error_setg(errp, "vfio: Failed to set up io notification");
> - qemu_set_fd_handler(*pfd, NULL, NULL, vcdev);
> + fd = event_notifier_get_fd(&vcdev->io_notifier);
> + qemu_set_fd_handler(fd, vfio_ccw_io_notifier_handler, NULL, vcdev);
> + if (vfio_set_irq_signaling(vdev, VFIO_CCW_IO_IRQ_INDEX, 0,
> + VFIO_IRQ_SET_ACTION_TRIGGER, fd, &local_err)) {
> + error_propagate(errp, local_err);
> + qemu_set_fd_handler(fd, NULL, NULL, vcdev);
> event_notifier_cleanup(&vcdev->io_notifier);
> }
>
> - g_free(irq_set);
> -
> out_free_info:
> g_free(irq_info);
> }
>
> static void vfio_ccw_unregister_io_notifier(VFIOCCWDevice *vcdev)
> {
> - struct vfio_irq_set *irq_set;
> - size_t argsz;
> - 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_CCW_IO_IRQ_INDEX;
> - irq_set->start = 0;
> - irq_set->count = 1;
> - pfd = (int32_t *) &irq_set->data;
> - *pfd = -1;
> -
> - if (ioctl(vcdev->vdev.fd, VFIO_DEVICE_SET_IRQS, irq_set)) {
> - error_report("vfio: Failed to de-assign device io fd: %m");
> + Error *local_err = NULL;
> +
> + if (vfio_set_irq_signaling(&vcdev->vdev, VFIO_CCW_IO_IRQ_INDEX, 0,
> + VFIO_IRQ_SET_ACTION_TRIGGER, -1, &local_err)) {
> + error_report_err(local_err);
> }
>
> qemu_set_fd_handler(event_notifier_get_fd(&vcdev->io_notifier),
> NULL, NULL, vcdev);
> event_notifier_cleanup(&vcdev->io_notifier);
>
> - g_free(irq_set);
> }
>
> static void vfio_ccw_get_region(VFIOCCWDevice *vcdev, Error **errp)
>
Looks good. Looking forward to reviewing it formally.
Best Regards
Eric
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v5] vfio/common: Introduce vfio_set_irq_signaling helper
2019-06-12 23:40 ` Li Qiang
@ 2019-06-13 10:06 ` Auger Eric
0 siblings, 0 replies; 6+ messages in thread
From: Auger Eric @ 2019-06-13 10:06 UTC (permalink / raw)
To: Li Qiang; +Cc: Alex Williamson, cohuck, Qemu Developers, eric.auger.pro
Hi Li,
On 6/13/19 1:40 AM, Li Qiang wrote:
> Eric Auger <eric.auger@redhat.com> 于2019年6月12日周三 下午8:40写道：
>
>> 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,
>> ACTION_MASK or ACTION_UNMASK action.
>>
>> In the error message, we now use errno in case of any
>> VFIO_DEVICE_SET_IRQS ioctl failure.
>>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>>
>>
>
> Reviewed-by: Li Qiang <liq3ea@gmail.com>
Thanks for the errno issue report and for the review!
Best Regards
Eric
>
>
>
>> ---
>>
>> v4 -> v5:
>> - output errno instead of ioctl returned value. Issue
>> reported by Li
>> - vfio_set_irq_signaling now returns -errno in case
>> of failure
>> - update the commit message
>>
>> v3 -> v4:
>> - renamed irq_to_str into index_to_str
>> - avoid usage of g_strdup_printf
>> - print both the index and subindex in generic case
>>
>> v2 -> v3:
>> - irq_to_str() simply outputs the IRQ index if the VFIO device is
>> not of PCI type
>> - removed "vfio: failed to start eventfd signaling ../.." in vfio_platform
>>
>> v1 -> v2:
>> - don't call GET_IRQ_INFO in vfio_set_irq_signaling()
>> and restore quiet check in vfio_register_req_notifier.
>> Nicer display of the IRQ name.
>>
>> 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 | 78 ++++++++++++
>> hw/vfio/pci.c | 217 ++++++++--------------------------
>> hw/vfio/platform.c | 62 ++++------
>> include/hw/vfio/vfio-common.h | 2 +
>> 4 files changed, 151 insertions(+), 208 deletions(-)
>>
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index 4374cc6176..a859298fda 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -95,6 +95,84 @@ 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";
>> + }
>> +}
>> +
>> +static const char *index_to_str(VFIODevice *vbasedev, int index)
>> +{
>> + if (vbasedev->type != VFIO_DEVICE_TYPE_PCI) {
>> + return NULL;
>> + }
>> +
>> + switch (index) {
>> + case VFIO_PCI_INTX_IRQ_INDEX:
>> + return "INTX";
>> + case VFIO_PCI_MSI_IRQ_INDEX:
>> + return "MSI";
>> + case VFIO_PCI_MSIX_IRQ_INDEX:
>> + return "MSIX";
>> + case VFIO_PCI_ERR_IRQ_INDEX:
>> + return "ERR";
>> + case VFIO_PCI_REQ_IRQ_INDEX:
>> + return "REQ";
>> + default:
>> + return NULL;
>> + }
>> +}
>> +
>> +int vfio_set_irq_signaling(VFIODevice *vbasedev, int index, int subindex,
>> + int action, int fd, Error **errp)
>> +{
>> + struct vfio_irq_set *irq_set;
>> + int argsz, ret = 0;
>> + const char *name;
>> + 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 | action;
>> + irq_set->index = index;
>> + irq_set->start = subindex;
>> + irq_set->count = 1;
>> + pfd = (int32_t *)&irq_set->data;
>> + *pfd = fd;
>> +
>> + if (ioctl(vbasedev->fd, VFIO_DEVICE_SET_IRQS, irq_set)) {
>> + ret = -errno;
>> + }
>> + g_free(irq_set);
>> +
>> + if (!ret) {
>> + return 0;
>> + }
>> +
>> + error_setg_errno(errp, -ret, "VFIO_DEVICE_SET_IRQS failure");
>> +
>> + name = index_to_str(vbasedev, index);
>> + if (name) {
>> + error_prepend(errp, "%s-%d: ", name, subindex);
>> + } else {
>> + error_prepend(errp, "index %d-%d: ", index, subindex);
>> + }
>> + error_prepend(errp,
>> + "Failed to %s %s eventfd signaling for interrupt ",
>> + fd < 0 ? "tear down" : "set up", action_to_str(action));
>> + 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 2a4091d216..65c2e17028 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);
>> }
>> }
>>
>> @@ -2636,10 +2578,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;
>> @@ -2651,58 +2591,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);
>> @@ -2727,9 +2639,8 @@ 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;
>> @@ -2745,57 +2656,31 @@ static void
>> vfio_register_req_notifier(VFIOPCIDevice *vdev)
>> 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 d52d6552e0..8d6012857e 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;
>> }
>>
>> @@ -330,7 +323,6 @@ static void vfio_platform_eoi(VFIODevice *vbasedev)
>>
>> static void vfio_start_eventfd_injection(SysBusDevice *sbdev, qemu_irq
>> irq)
>> {
>> - int ret;
>> VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(sbdev);
>> VFIOINTp *intp;
>>
>> @@ -341,10 +333,7 @@ static void vfio_start_eventfd_injection(SysBusDevice
>> *sbdev, qemu_irq irq)
>> }
>> assert(intp);
>>
>> - ret = vfio_set_trigger_eventfd(intp, vfio_intp_interrupt);
>> - if (ret) {
>> - error_report("vfio: failed to start eventfd signaling for IRQ %d:
>> %m",
>> - intp->pin);
>> + if (vfio_set_trigger_eventfd(intp, vfio_intp_interrupt)) {
>> abort();
>> }
>> }
>> @@ -361,25 +350,16 @@ 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;
>> }
>> @@ -435,8 +415,6 @@ static void vfio_start_irqfd_injection(SysBusDevice
>> *sbdev, qemu_irq irq)
>> return;
>> fail_vfio:
>> kvm_irqchip_remove_irqfd_notifier(kvm_state, intp->interrupt, irq);
>> - error_report("vfio: failed to start eventfd signaling for IRQ %d: %m",
>> - intp->pin);
>> abort();
>> fail_irqfd:
>> vfio_start_eventfd_injection(sbdev, irq);
>> 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 [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v5] vfio/common: Introduce vfio_set_irq_signaling helper
2019-06-12 12:40 [Qemu-devel] [PATCH v5] vfio/common: Introduce vfio_set_irq_signaling helper Eric Auger
2019-06-12 16:46 ` Cornelia Huck
2019-06-12 23:40 ` Li Qiang
@ 2019-06-13 21:35 ` Alex Williamson
2 siblings, 0 replies; 6+ messages in thread
From: Alex Williamson @ 2019-06-13 21:35 UTC (permalink / raw)
To: Eric Auger; +Cc: cohuck, liq3ea, qemu-devel, eric.auger.pro
On Wed, 12 Jun 2019 14:40:04 +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,
> ACTION_MASK or ACTION_UNMASK action.
>
> In the error message, we now use errno in case of any
> VFIO_DEVICE_SET_IRQS ioctl failure.
>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>
> ---
Thanks Eric, I just sent a pull request including this, with Li's and
Connie's R-b. Thanks,
Alex
>
> v4 -> v5:
> - output errno instead of ioctl returned value. Issue
> reported by Li
> - vfio_set_irq_signaling now returns -errno in case
> of failure
> - update the commit message
>
> v3 -> v4:
> - renamed irq_to_str into index_to_str
> - avoid usage of g_strdup_printf
> - print both the index and subindex in generic case
>
> v2 -> v3:
> - irq_to_str() simply outputs the IRQ index if the VFIO device is
> not of PCI type
> - removed "vfio: failed to start eventfd signaling ../.." in vfio_platform
>
> v1 -> v2:
> - don't call GET_IRQ_INFO in vfio_set_irq_signaling()
> and restore quiet check in vfio_register_req_notifier.
> Nicer display of the IRQ name.
>
> 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 | 78 ++++++++++++
> hw/vfio/pci.c | 217 ++++++++--------------------------
> hw/vfio/platform.c | 62 ++++------
> include/hw/vfio/vfio-common.h | 2 +
> 4 files changed, 151 insertions(+), 208 deletions(-)
>
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 4374cc6176..a859298fda 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -95,6 +95,84 @@ 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";
> + }
> +}
> +
> +static const char *index_to_str(VFIODevice *vbasedev, int index)
> +{
> + if (vbasedev->type != VFIO_DEVICE_TYPE_PCI) {
> + return NULL;
> + }
> +
> + switch (index) {
> + case VFIO_PCI_INTX_IRQ_INDEX:
> + return "INTX";
> + case VFIO_PCI_MSI_IRQ_INDEX:
> + return "MSI";
> + case VFIO_PCI_MSIX_IRQ_INDEX:
> + return "MSIX";
> + case VFIO_PCI_ERR_IRQ_INDEX:
> + return "ERR";
> + case VFIO_PCI_REQ_IRQ_INDEX:
> + return "REQ";
> + default:
> + return NULL;
> + }
> +}
> +
> +int vfio_set_irq_signaling(VFIODevice *vbasedev, int index, int subindex,
> + int action, int fd, Error **errp)
> +{
> + struct vfio_irq_set *irq_set;
> + int argsz, ret = 0;
> + const char *name;
> + 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 | action;
> + irq_set->index = index;
> + irq_set->start = subindex;
> + irq_set->count = 1;
> + pfd = (int32_t *)&irq_set->data;
> + *pfd = fd;
> +
> + if (ioctl(vbasedev->fd, VFIO_DEVICE_SET_IRQS, irq_set)) {
> + ret = -errno;
> + }
> + g_free(irq_set);
> +
> + if (!ret) {
> + return 0;
> + }
> +
> + error_setg_errno(errp, -ret, "VFIO_DEVICE_SET_IRQS failure");
> +
> + name = index_to_str(vbasedev, index);
> + if (name) {
> + error_prepend(errp, "%s-%d: ", name, subindex);
> + } else {
> + error_prepend(errp, "index %d-%d: ", index, subindex);
> + }
> + error_prepend(errp,
> + "Failed to %s %s eventfd signaling for interrupt ",
> + fd < 0 ? "tear down" : "set up", action_to_str(action));
> + 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 2a4091d216..65c2e17028 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);
> }
> }
>
> @@ -2636,10 +2578,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;
> @@ -2651,58 +2591,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);
> @@ -2727,9 +2639,8 @@ 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;
> @@ -2745,57 +2656,31 @@ static void vfio_register_req_notifier(VFIOPCIDevice *vdev)
> 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 d52d6552e0..8d6012857e 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;
> }
>
> @@ -330,7 +323,6 @@ static void vfio_platform_eoi(VFIODevice *vbasedev)
>
> static void vfio_start_eventfd_injection(SysBusDevice *sbdev, qemu_irq irq)
> {
> - int ret;
> VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(sbdev);
> VFIOINTp *intp;
>
> @@ -341,10 +333,7 @@ static void vfio_start_eventfd_injection(SysBusDevice *sbdev, qemu_irq irq)
> }
> assert(intp);
>
> - ret = vfio_set_trigger_eventfd(intp, vfio_intp_interrupt);
> - if (ret) {
> - error_report("vfio: failed to start eventfd signaling for IRQ %d: %m",
> - intp->pin);
> + if (vfio_set_trigger_eventfd(intp, vfio_intp_interrupt)) {
> abort();
> }
> }
> @@ -361,25 +350,16 @@ 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;
> }
> @@ -435,8 +415,6 @@ static void vfio_start_irqfd_injection(SysBusDevice *sbdev, qemu_irq irq)
> return;
> fail_vfio:
> kvm_irqchip_remove_irqfd_notifier(kvm_state, intp->interrupt, irq);
> - error_report("vfio: failed to start eventfd signaling for IRQ %d: %m",
> - intp->pin);
> abort();
> fail_irqfd:
> vfio_start_eventfd_injection(sbdev, irq);
> 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,
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-06-13 21:52 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-06-12 12:40 [Qemu-devel] [PATCH v5] vfio/common: Introduce vfio_set_irq_signaling helper Eric Auger
2019-06-12 16:46 ` Cornelia Huck
2019-06-13 10:05 ` Auger Eric
2019-06-12 23:40 ` Li Qiang
2019-06-13 10:06 ` Auger Eric
2019-06-13 21:35 ` Alex Williamson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).