Re: [Qemu-devel] [PATCH RFC v2 11/15] vfio: ccw: introduce ioctls to get/set VFIO_CCW_IO_IRQ

[Dong Jia Shi]

* Alex Williamson <address@hidden> [2017-01-17 14:02:40 -0700]:

> On Thu, 12 Jan 2017 08:19:43 +0100
> Dong Jia Shi <address@hidden> wrote:
> 
> > Realize VFIO_DEVICE_GET_IRQ_INFO ioctl to retrieve
> > VFIO_CCW_IO_IRQ information.
> > 
> > Realize VFIO_DEVICE_SET_IRQS ioctl to set an eventfd fd for
> > VFIO_CCW_IO_IRQ. Once a write operation to the ccw_io_region
> > was performed, trigger a signal on this fd.
> > 
> > Signed-off-by: Dong Jia Shi <address@hidden>
> > Reviewed-by: Pierre Morel <address@hidden>
> > ---
> >  drivers/s390/cio/vfio_ccw_ops.c     | 125 
> > +++++++++++++++++++++++++++++++++++-
> >  drivers/s390/cio/vfio_ccw_private.h |   4 ++
> >  include/uapi/linux/vfio.h           |  10 ++-
> >  3 files changed, 136 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/s390/cio/vfio_ccw_ops.c 
> > b/drivers/s390/cio/vfio_ccw_ops.c
> > index b702735..3c47eb6 100644
> > --- a/drivers/s390/cio/vfio_ccw_ops.c
> > +++ b/drivers/s390/cio/vfio_ccw_ops.c
> > @@ -203,6 +203,9 @@ static ssize_t vfio_ccw_mdev_write(struct mdev_device 
> > *mdev,
> >     if (region->ret_code != 0)
> >             return region->ret_code;
> >  
> > +   if (private->io_trigger)
> > +           eventfd_signal(private->io_trigger, 1);
> > +
> >     return count;
> >  }
> >  
> > @@ -211,7 +214,7 @@ static int vfio_ccw_mdev_get_device_info(struct 
> > mdev_device *mdev,
> >  {
> >     info->flags = VFIO_DEVICE_FLAGS_CCW | VFIO_DEVICE_FLAGS_RESET;
> >     info->num_regions = VFIO_CCW_NUM_REGIONS;
> > -   info->num_irqs = 0;
> > +   info->num_irqs = VFIO_CCW_NUM_IRQS;
> >  
> >     return 0;
> >  }
> > @@ -233,6 +236,84 @@ static int vfio_ccw_mdev_get_region_info(struct 
> > mdev_device *mdev,
> >     }
> >  }
> >  
> > +int vfio_ccw_mdev_get_irq_info(struct mdev_device *mdev,
> > +                          struct vfio_irq_info *info)
> > +{
> > +   if (info->index != VFIO_CCW_IO_IRQ_INDEX)
> > +           return -EINVAL;
> > +
> > +   info->count = VFIO_CCW_NUM_IRQS;
> > +   info->flags = VFIO_IRQ_INFO_EVENTFD | VFIO_IRQ_INFO_NORESIZE;
> 
> 
> VFIO_CCW_NUM_IRQS is not being used correctly here, info->count is the
> number of interrupts within this index, VFIO_CCW_NUM_IRQS is the number
> of indexes.  This is meant to handle things like PCI where we have 3
> different interrupts types (INTx, MSI, MSI-X) and some of those (MSI/X)
> support multiple vectors.  In this case I think you want info->count =
> 1 and you don't need the NORESIZE flag since that only makes sense for
> describing indexes where a subset of the available vectors may be
> enabled.  So info->count comes out to the same thing, but should not
> use the same macro to get there.
> 
Hi Alex,

I indeed use VFIO_CCW_NUM_IRQS here in a wrong way. Thanks for your
explanation. I will change it to:
        info->count = 1;

> > +
> > +   return 0;
> > +}
> > +
[...]

> > @@ -281,6 +362,48 @@ static ssize_t vfio_ccw_mdev_ioctl(struct mdev_device 
> > *mdev,
> >  
> >             return copy_to_user((void __user *)arg, &info, minsz);
> >     }
> > +   case VFIO_DEVICE_GET_IRQ_INFO:
> > +   {
> > +           struct vfio_irq_info info;
> > +
> > +           minsz = offsetofend(struct vfio_irq_info, count);
> > +
> > +           if (copy_from_user(&info, (void __user *)arg, minsz))
> > +                   return -EFAULT;
> > +
> > +           if (info.argsz < minsz || info.index >= VFIO_CCW_NUM_IRQS)
> > +                   return -EINVAL;
> > +
> > +           ret = vfio_ccw_mdev_get_irq_info(mdev, &info);
> > +           if (ret)
> > +                   return ret;
> > +
> > +           if (info.count == -1)
> > +                   return -EINVAL;
> > +
> > +           return copy_to_user((void __user *)arg, &info, minsz);
> > +   }
> > +   case VFIO_DEVICE_SET_IRQS:
> > +   {
> > +           struct vfio_irq_set hdr;
> > +           size_t data_size;
> > +           void __user *data;
> > +
> > +           minsz = offsetofend(struct vfio_irq_set, count);
> > +
> > +           if (copy_from_user(&hdr, (void __user *)arg, minsz))
> > +                   return -EFAULT;
> > +
> > +           ret = vfio_set_irqs_validate_and_prepare(&hdr,
> > +                                                    VFIO_CCW_NUM_IRQS,
> > +                                                    VFIO_CCW_NUM_IRQS,
> > +                                                    &data_size);
> 
> 
> This is another instance, max_irq_type is referring to the index while
> num_irqs is referring to the count of vectors within this index.
> VFIO_CCW_NUM_IRQS should only be used for max_irq_type.

Ok. Will use 1 instead of VFIO_CCW_NUM_IRQS for @num_irqs.

> 
> > +           if (ret)
> > +                   return ret;
> > +
> > +           data = (void __user *)(arg + minsz);
> > +           return vfio_ccw_mdev_set_irqs(mdev, hdr.flags, data);
> > +   }
> >     case VFIO_DEVICE_RESET:
> >             return vfio_ccw_mdev_reset(mdev);
> >     default:
[...]

-- 
Dong Jia