* [Qemu-devel] [PATCH] vfio-ccw: use vfio_set_irq_signaling
@ 2019-06-14 9:27 Cornelia Huck
2019-06-14 13:28 ` Auger Eric
2019-06-14 14:30 ` Eric Farman
0 siblings, 2 replies; 6+ messages in thread
From: Cornelia Huck @ 2019-06-14 9:27 UTC (permalink / raw)
To: Eric Farman, Farhan Ali; +Cc: Eric Auger, qemu-s390x, Cornelia Huck, qemu-devel
Use the new helper.
Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
hw/vfio/ccw.c | 68 +++++++++++----------------------------------------
1 file changed, 14 insertions(+), 54 deletions(-)
diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
index 03a2becb3ec9..3dc08721a3db 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -197,10 +197,7 @@ read_err:
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;
if (vdev->num_irqs < VFIO_CCW_IO_IRQ_INDEX + 1) {
error_setg(errp, "vfio: unexpected number of io irqs %u",
@@ -208,72 +205,35 @@ static void vfio_ccw_register_io_notifier(VFIOCCWDevice *vcdev, Error **errp)
return;
}
- argsz = sizeof(*irq_info);
- irq_info = g_malloc0(argsz);
- irq_info->index = VFIO_CCW_IO_IRQ_INDEX;
- irq_info->argsz = argsz;
- if (ioctl(vdev->fd, VFIO_DEVICE_GET_IRQ_INFO,
- irq_info) < 0 || irq_info->count < 1) {
- error_setg_errno(errp, errno, "vfio: Error getting irq info");
- goto out_free_info;
- }
-
if (event_notifier_init(&vcdev->io_notifier, 0)) {
error_setg_errno(errp, errno,
"vfio: Unable to init event notifier for IO");
- goto out_free_info;
+ 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_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, errp)) {
+ 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 *err = NULL;
+
+ vfio_set_irq_signaling(&vcdev->vdev, VFIO_CCW_IO_IRQ_INDEX, 0,
+ VFIO_IRQ_SET_ACTION_TRIGGER, -1, &err);
+ if (err) {
+ error_reportf_err(err, VFIO_MSG_PREFIX, vcdev->vdev.name);
}
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] vfio-ccw: use vfio_set_irq_signaling
2019-06-14 9:27 [Qemu-devel] [PATCH] vfio-ccw: use vfio_set_irq_signaling Cornelia Huck
@ 2019-06-14 13:28 ` Auger Eric
2019-06-14 14:30 ` Eric Farman
1 sibling, 0 replies; 6+ messages in thread
From: Auger Eric @ 2019-06-14 13:28 UTC (permalink / raw)
To: Cornelia Huck, Eric Farman, Farhan Ali; +Cc: qemu-s390x, qemu-devel
Hi Connie,
On 6/14/19 11:27 AM, Cornelia Huck wrote:
> Use the new helper.
>
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Thanks
Eric
> ---
> hw/vfio/ccw.c | 68 +++++++++++----------------------------------------
> 1 file changed, 14 insertions(+), 54 deletions(-)
>
> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
> index 03a2becb3ec9..3dc08721a3db 100644
> --- a/hw/vfio/ccw.c
> +++ b/hw/vfio/ccw.c
> @@ -197,10 +197,7 @@ read_err:
> 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;
>
> if (vdev->num_irqs < VFIO_CCW_IO_IRQ_INDEX + 1) {
> error_setg(errp, "vfio: unexpected number of io irqs %u",
> @@ -208,72 +205,35 @@ static void vfio_ccw_register_io_notifier(VFIOCCWDevice *vcdev, Error **errp)
> return;
> }
>
> - argsz = sizeof(*irq_info);
> - irq_info = g_malloc0(argsz);
> - irq_info->index = VFIO_CCW_IO_IRQ_INDEX;
> - irq_info->argsz = argsz;
> - if (ioctl(vdev->fd, VFIO_DEVICE_GET_IRQ_INFO,
> - irq_info) < 0 || irq_info->count < 1) {
> - error_setg_errno(errp, errno, "vfio: Error getting irq info");
> - goto out_free_info;
> - }
> -
> if (event_notifier_init(&vcdev->io_notifier, 0)) {
> error_setg_errno(errp, errno,
> "vfio: Unable to init event notifier for IO");
> - goto out_free_info;
> + 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_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, errp)) {
> + 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 *err = NULL;
> +
> + vfio_set_irq_signaling(&vcdev->vdev, VFIO_CCW_IO_IRQ_INDEX, 0,
> + VFIO_IRQ_SET_ACTION_TRIGGER, -1, &err);
> + if (err) {
> + error_reportf_err(err, VFIO_MSG_PREFIX, vcdev->vdev.name);
> }
>
> 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)
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] vfio-ccw: use vfio_set_irq_signaling
2019-06-14 9:27 [Qemu-devel] [PATCH] vfio-ccw: use vfio_set_irq_signaling Cornelia Huck
2019-06-14 13:28 ` Auger Eric
@ 2019-06-14 14:30 ` Eric Farman
2019-06-14 15:06 ` Auger Eric
1 sibling, 1 reply; 6+ messages in thread
From: Eric Farman @ 2019-06-14 14:30 UTC (permalink / raw)
To: Cornelia Huck, Farhan Ali; +Cc: Eric Auger, qemu-s390x, qemu-devel
On 6/14/19 5:27 AM, Cornelia Huck wrote:
> Use the new helper.
>
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
> hw/vfio/ccw.c | 68 +++++++++++----------------------------------------
> 1 file changed, 14 insertions(+), 54 deletions(-)
>
> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
> index 03a2becb3ec9..3dc08721a3db 100644
> --- a/hw/vfio/ccw.c
> +++ b/hw/vfio/ccw.c
> @@ -197,10 +197,7 @@ read_err:
> 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;
>
> if (vdev->num_irqs < VFIO_CCW_IO_IRQ_INDEX + 1) {
> error_setg(errp, "vfio: unexpected number of io irqs %u",
> @@ -208,72 +205,35 @@ static void vfio_ccw_register_io_notifier(VFIOCCWDevice *vcdev, Error **errp)
> return;
> }
>
> - argsz = sizeof(*irq_info);
> - irq_info = g_malloc0(argsz);
> - irq_info->index = VFIO_CCW_IO_IRQ_INDEX;
> - irq_info->argsz = argsz;
> - if (ioctl(vdev->fd, VFIO_DEVICE_GET_IRQ_INFO,
> - irq_info) < 0 || irq_info->count < 1) {
> - error_setg_errno(errp, errno, "vfio: Error getting irq info");
> - goto out_free_info;
> - }
> -
Don't we still need this hunk? (And the out_free_info label stuff that
cleans it up.) I don't see vfio_set_irq_signaling() covering it.
> if (event_notifier_init(&vcdev->io_notifier, 0)) {
> error_setg_errno(errp, errno,
> "vfio: Unable to init event notifier for IO");
> - goto out_free_info;
> + 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_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, errp)) {
> + qemu_set_fd_handler(fd, NULL, NULL, vcdev);
This sure looks nice though. :)
> 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 *err = NULL;
> +
> + vfio_set_irq_signaling(&vcdev->vdev, VFIO_CCW_IO_IRQ_INDEX, 0,
> + VFIO_IRQ_SET_ACTION_TRIGGER, -1, &err);
> + if (err) {
> + error_reportf_err(err, VFIO_MSG_PREFIX, vcdev->vdev.name);
> }
>
> 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)
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] vfio-ccw: use vfio_set_irq_signaling
2019-06-14 14:30 ` Eric Farman
@ 2019-06-14 15:06 ` Auger Eric
2019-06-14 15:41 ` Eric Farman
0 siblings, 1 reply; 6+ messages in thread
From: Auger Eric @ 2019-06-14 15:06 UTC (permalink / raw)
To: Eric Farman, Cornelia Huck, Farhan Ali; +Cc: qemu-s390x, qemu-devel
Hi Eric,
On 6/14/19 4:30 PM, Eric Farman wrote:
>
>
> On 6/14/19 5:27 AM, Cornelia Huck wrote:
>> Use the new helper.
>>
>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>> ---
>> hw/vfio/ccw.c | 68 +++++++++++----------------------------------------
>> 1 file changed, 14 insertions(+), 54 deletions(-)
>>
>> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
>> index 03a2becb3ec9..3dc08721a3db 100644
>> --- a/hw/vfio/ccw.c
>> +++ b/hw/vfio/ccw.c
>> @@ -197,10 +197,7 @@ read_err:
>> 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;
>>
>> if (vdev->num_irqs < VFIO_CCW_IO_IRQ_INDEX + 1) {
>> error_setg(errp, "vfio: unexpected number of io irqs %u",
>> @@ -208,72 +205,35 @@ static void vfio_ccw_register_io_notifier(VFIOCCWDevice *vcdev, Error **errp)
>> return;
>> }
>>
>> - argsz = sizeof(*irq_info);
>> - irq_info = g_malloc0(argsz);
>> - irq_info->index = VFIO_CCW_IO_IRQ_INDEX;
>> - irq_info->argsz = argsz;
>> - if (ioctl(vdev->fd, VFIO_DEVICE_GET_IRQ_INFO,
>> - irq_info) < 0 || irq_info->count < 1) {
>> - error_setg_errno(errp, errno, "vfio: Error getting irq info");
>> - goto out_free_info;
>> - }
>> -
>
> Don't we still need this hunk? (And the out_free_info label stuff that
> cleans it up.) I don't see vfio_set_irq_signaling() covering it.
Looks this IRQ index is always implemented and exposed by the driver,
isn't it? In such a case it shouldn't be needed to test its presence?
Thanks
Eric
>
>> if (event_notifier_init(&vcdev->io_notifier, 0)) {
>> error_setg_errno(errp, errno,
>> "vfio: Unable to init event notifier for IO");
>> - goto out_free_info;
>> + 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_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, errp)) {
>> + qemu_set_fd_handler(fd, NULL, NULL, vcdev);
>
> This sure looks nice though. :)
>
>> 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 *err = NULL;
>> +
>> + vfio_set_irq_signaling(&vcdev->vdev, VFIO_CCW_IO_IRQ_INDEX, 0,
>> + VFIO_IRQ_SET_ACTION_TRIGGER, -1, &err);
>> + if (err) {
>> + error_reportf_err(err, VFIO_MSG_PREFIX, vcdev->vdev.name);
>> }
>>
>> 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)
>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] vfio-ccw: use vfio_set_irq_signaling
2019-06-14 15:06 ` Auger Eric
@ 2019-06-14 15:41 ` Eric Farman
2019-06-17 9:58 ` Cornelia Huck
0 siblings, 1 reply; 6+ messages in thread
From: Eric Farman @ 2019-06-14 15:41 UTC (permalink / raw)
To: Auger Eric, Cornelia Huck, Farhan Ali; +Cc: qemu-s390x, qemu-devel
On 6/14/19 11:06 AM, Auger Eric wrote:
> Hi Eric,
>
> On 6/14/19 4:30 PM, Eric Farman wrote:
>>
>>
>> On 6/14/19 5:27 AM, Cornelia Huck wrote:
>>> Use the new helper.
>>>
>>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
>>> ---
>>> hw/vfio/ccw.c | 68 +++++++++++----------------------------------------
>>> 1 file changed, 14 insertions(+), 54 deletions(-)
>>>
>>> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
>>> index 03a2becb3ec9..3dc08721a3db 100644
>>> --- a/hw/vfio/ccw.c
>>> +++ b/hw/vfio/ccw.c
>>> @@ -197,10 +197,7 @@ read_err:
>>> 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;
>>>
>>> if (vdev->num_irqs < VFIO_CCW_IO_IRQ_INDEX + 1) {
>>> error_setg(errp, "vfio: unexpected number of io irqs %u",
>>> @@ -208,72 +205,35 @@ static void vfio_ccw_register_io_notifier(VFIOCCWDevice *vcdev, Error **errp)
>>> return;
>>> }
>>>
>>> - argsz = sizeof(*irq_info);
>>> - irq_info = g_malloc0(argsz);
>>> - irq_info->index = VFIO_CCW_IO_IRQ_INDEX;
>>> - irq_info->argsz = argsz;
>>> - if (ioctl(vdev->fd, VFIO_DEVICE_GET_IRQ_INFO,
>>> - irq_info) < 0 || irq_info->count < 1) {
>>> - error_setg_errno(errp, errno, "vfio: Error getting irq info");
>>> - goto out_free_info;
>>> - }
>>> -
>>
>> Don't we still need this hunk? (And the out_free_info label stuff that
>> cleans it up.) I don't see vfio_set_irq_signaling() covering it.
>
> Looks this IRQ index is always implemented and exposed by the driver,
> isn't it? In such a case it shouldn't be needed to test its presence?
Right; if we were running on an old kernel, both ioctl's would fail the
same way since they were added concurrently. So the check today doesn't
seem very useful.
But since it's there, and we're taking it out, it got me wondering
whether there are/were intentions to expand GET_IRQ_INFO in the future.
I'm not aware of any reason vfio-ccw would need to, but want some
confirmation that I'm not overlooking anything.
Cheers,
Other Eric
>
> Thanks
>
> Eric
>>
>>> if (event_notifier_init(&vcdev->io_notifier, 0)) {
>>> error_setg_errno(errp, errno,
>>> "vfio: Unable to init event notifier for IO");
>>> - goto out_free_info;
>>> + 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_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, errp)) {
>>> + qemu_set_fd_handler(fd, NULL, NULL, vcdev);
>>
>> This sure looks nice though. :)
>>
>>> 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 *err = NULL;
>>> +
>>> + vfio_set_irq_signaling(&vcdev->vdev, VFIO_CCW_IO_IRQ_INDEX, 0,
>>> + VFIO_IRQ_SET_ACTION_TRIGGER, -1, &err);
>>> + if (err) {
>>> + error_reportf_err(err, VFIO_MSG_PREFIX, vcdev->vdev.name);
>>> }
>>>
>>> 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)
>>>
>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] vfio-ccw: use vfio_set_irq_signaling
2019-06-14 15:41 ` Eric Farman
@ 2019-06-17 9:58 ` Cornelia Huck
0 siblings, 0 replies; 6+ messages in thread
From: Cornelia Huck @ 2019-06-17 9:58 UTC (permalink / raw)
To: Eric Farman; +Cc: Auger Eric, qemu-s390x, Farhan Ali, qemu-devel
On Fri, 14 Jun 2019 11:41:41 -0400
Eric Farman <farman@linux.ibm.com> wrote:
> On 6/14/19 11:06 AM, Auger Eric wrote:
> > Hi Eric,
> >
> > On 6/14/19 4:30 PM, Eric Farman wrote:
> >>
> >>
> >> On 6/14/19 5:27 AM, Cornelia Huck wrote:
> >>> Use the new helper.
> >>>
> >>> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> >>> ---
> >>> hw/vfio/ccw.c | 68 +++++++++++----------------------------------------
> >>> 1 file changed, 14 insertions(+), 54 deletions(-)
> >>>
> >>> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
> >>> index 03a2becb3ec9..3dc08721a3db 100644
> >>> --- a/hw/vfio/ccw.c
> >>> +++ b/hw/vfio/ccw.c
> >>> @@ -197,10 +197,7 @@ read_err:
> >>> 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;
> >>>
> >>> if (vdev->num_irqs < VFIO_CCW_IO_IRQ_INDEX + 1) {
> >>> error_setg(errp, "vfio: unexpected number of io irqs %u",
> >>> @@ -208,72 +205,35 @@ static void vfio_ccw_register_io_notifier(VFIOCCWDevice *vcdev, Error **errp)
> >>> return;
> >>> }
> >>>
> >>> - argsz = sizeof(*irq_info);
> >>> - irq_info = g_malloc0(argsz);
> >>> - irq_info->index = VFIO_CCW_IO_IRQ_INDEX;
> >>> - irq_info->argsz = argsz;
> >>> - if (ioctl(vdev->fd, VFIO_DEVICE_GET_IRQ_INFO,
> >>> - irq_info) < 0 || irq_info->count < 1) {
> >>> - error_setg_errno(errp, errno, "vfio: Error getting irq info");
> >>> - goto out_free_info;
> >>> - }
> >>> -
> >>
> >> Don't we still need this hunk? (And the out_free_info label stuff that
> >> cleans it up.) I don't see vfio_set_irq_signaling() covering it.
> >
> > Looks this IRQ index is always implemented and exposed by the driver,
> > isn't it? In such a case it shouldn't be needed to test its presence?
>
> Right; if we were running on an old kernel, both ioctl's would fail the
> same way since they were added concurrently. So the check today doesn't
> seem very useful.
>
> But since it's there, and we're taking it out, it got me wondering
> whether there are/were intentions to expand GET_IRQ_INFO in the future.
> I'm not aware of any reason vfio-ccw would need to, but want some
> confirmation that I'm not overlooking anything.
It seems I actually was a bit too delete-happy... I did not intend to
rip out that code at all, it just somehow was suddenly gone :)
Checking is probably not strictly needed, but let me send a v2 that
does not delete it (and only converts to the new interface).
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-06-17 10:00 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-14 9:27 [Qemu-devel] [PATCH] vfio-ccw: use vfio_set_irq_signaling Cornelia Huck
2019-06-14 13:28 ` Auger Eric
2019-06-14 14:30 ` Eric Farman
2019-06-14 15:06 ` Auger Eric
2019-06-14 15:41 ` Eric Farman
2019-06-17 9:58 ` Cornelia Huck
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.