Re: [qemu-s390x] [PATCH] vfio-ccw: use vfio_set_irq_signaling

[Cornelia Huck]

On Fri, 14 Jun 2019 11:41:41 -0400
Eric Farman <address@hidden> 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 <address@hidden>
> >>> ---
> >>>  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).