Re: [PATCH v1 4/7] vfio: ap: AP Queue Interrupt Control VFIO ioctl calls

[Pierre Morel]

On 06/11/2018 21:21, Tony Krowiak wrote:
> On 10/31/18 2:12 PM, Pierre Morel wrote:
> > This is the implementation of the VFIO ioctl calls to handle
> > the AQIC interception and use GISA to handle interrupts.
> >
> > Signed-off-by: Pierre Morel <pmorel@xxxxxxxxxxxxx>
> > ---
> > Â drivers/s390/crypto/vfio_ap_ops.c | 95 +++++++++++++++++++++++++++++++
> > Â 1 file changed, 95 insertions(+)
> >
> > diff --git a/drivers/s390/crypto/vfio_ap_ops.c 
> > b/drivers/s390/crypto/vfio_ap_ops.c
> > index 272ef427dcc0..f68102163bf4 100644
> > --- a/drivers/s390/crypto/vfio_ap_ops.c
> > +++ b/drivers/s390/crypto/vfio_ap_ops.c
> > @@ -895,12 +895,107 @@ static int 
> > vfio_ap_mdev_get_device_info(unsigned long arg)
> > ÂÂÂÂÂ return copy_to_user((void __user *)arg, &info, minsz);
> > Â }
> > +static int ap_ioctl_setirq(struct ap_matrix_mdev *matrix_mdev,
>
> In keeping with the other function names in this file, how about
> naming this function vfio_ap_mdev_setirq???

OK, agreed.

> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct vfio_ap_aqic *parm)
> > +{
> > +ÂÂÂ struct aqic_gisa aqic_gisa = reg2aqic(0);
> > +ÂÂÂ struct kvm_s390_gisa *gisa = matrix_mdev->kvm->arch.gisa;
> > +ÂÂÂ struct ap_status ap_status = reg2status(0);
> > +ÂÂÂ unsigned long p;
> > +ÂÂÂ int ret = -1;
> > +ÂÂÂ int apqn;
> > +ÂÂÂ uint32_t gd;
> > +
> > +ÂÂÂ apqn = (int)(parm->cmd & 0xffff);
> > +
> > +ÂÂÂ gd = matrix_mdev->kvm->vcpus[0]->arch.sie_block->gd;
> > +ÂÂÂ if (gd & 0x01)
> > +ÂÂÂÂÂÂÂ aqic_gisa.f = 1;
> > +ÂÂÂ aqic_gisa.gisc = matrix_mdev->gisc;
>
> Can't you get this value from parm? I don't see any relationship
> between the mdev device and gisc, why store it there?

The idea is that we may want to report this value to the admin or as 
debug information, so I wanted to keep track of it.

> > +ÂÂÂ aqic_gisa.isc = GAL_ISC;
> > +ÂÂÂ aqic_gisa.ir = 1;
> > +ÂÂÂ aqic_gisa.gisao = gisa->next_alert >> 4;
> > +
> > +ÂÂÂ p = (unsigned long) page_address(matrix_mdev->map->page);
> > +ÂÂÂ p += (matrix_mdev->map->guest_addr & 0x0fff);
> > +
> > +ÂÂÂ ret = ap_host_aqic((uint64_t)apqn, aqic2reg(aqic_gisa), p);
> > +ÂÂÂ parm->status = ret;
> > +
> > +ÂÂÂ ap_status = reg2status(ret);
> > +ÂÂÂ return (ap_status.rc) ? -EIO : 0;
> > +}
> > +
> > +static int ap_ioctl_clrirq(struct ap_matrix_mdev *matrix_mdev,
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct vfio_ap_aqic *parm)
>
> In keeping with the other function names in this file, how about
> naming this function vfio_ap_mdev_clrirq, or better yet,
> vfio_ap_mdev_clear_irq???

agreed too.

> > +{
> > +ÂÂÂ struct aqic_gisa aqic_gisa = reg2aqic(0);
> > +ÂÂÂ struct ap_status ap_status = reg2status(0) > +ÂÂÂ int apqn;
> > +ÂÂÂ int retval;
> > +ÂÂÂ uint32_t gd;
> > +
> > +ÂÂÂ apqn = (int)(parm->cmd & 0xffff);
> > +
> > +ÂÂÂ gd = matrix_mdev->kvm->vcpus[0]->arch.sie_block->gd;
> > +ÂÂÂ if (gd & 0x01)
> > +ÂÂÂÂÂÂÂ aqic_gisa.f = 1;
> > +ÂÂÂ aqic_gisa.ir = 0;
> > +
> > +ÂÂÂ retval = ap_host_aqic((uint64_t)apqn, aqic2reg(aqic_gisa), 0);
> > +ÂÂÂ parm->status = retval;
> > +
> > +ÂÂÂ ap_status = reg2status(retval);
> > +ÂÂÂ return (ap_status.rc) ? -EIO : 0;
> > +}
> > +
> > Â static ssize_t vfio_ap_mdev_ioctl(struct mdev_device *mdev,
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ unsigned int cmd, unsigned long arg)
> > Â {
> > ÂÂÂÂÂ int ret;
> > +ÂÂÂ struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
> > +ÂÂÂ struct s390_io_adapter *adapter;
> > +ÂÂÂ struct vfio_ap_aqic parm;
> > +ÂÂÂ struct s390_map_info *map;
> > +ÂÂÂ int apqn, found = 0;
> > ÂÂÂÂÂ switch (cmd) {
> > +ÂÂÂ case VFIO_AP_SET_IRQ:
> > +ÂÂÂÂÂÂÂ if (copy_from_user(&parm, (void __user *)arg, sizeof(parm)))
> > +ÂÂÂÂÂÂÂÂÂÂÂ return -EFAULT;
> > +ÂÂÂÂÂÂÂ apqn = (int)(parm.cmd & 0xffff);
> > +ÂÂÂÂÂÂÂ parm.status &= 0x00000000ffffffffUL;
> > +ÂÂÂÂÂÂÂ matrix_mdev->gisc = parm.status & 0x07;
>
> It seems that the only reason for the 'gisc' field in matrix_mdev
> is to pass the value to the ap_ioctl_setirq() function. Since the
> gisc has nothing to do with the mdev device and the 'parm' is being
> passed to ap_ioctl_setirq(), why not just eliminate the
> matrix_mdev->gisc field and get it from the 'parm' parameter in
> ap_ioctl_setirq()?

OK, seems better.

> > +ÂÂÂÂÂÂÂ /* find the adapter */ap_ioctl_setirq()
> > +ÂÂÂÂÂÂÂ adapter = matrix_mdev->kvm->arch.adapters[parm.adapter_id];
> > +ÂÂÂÂÂÂÂ if (!adapter)
> > +ÂÂÂÂÂÂÂÂÂÂÂ return -ENOENT;
> > +ÂÂÂÂÂÂÂ down_write(&adapter->maps_lock);
> > +ÂÂÂÂÂÂÂ list_for_each_entry(map, &adapter->maps, list) {
> > +ÂÂÂÂÂÂÂÂÂÂÂ if (map->guest_addr == parm.nib) {
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ found = 1;
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ break;
> > +ÂÂÂÂÂÂÂÂÂÂÂ }
> > +ÂÂÂÂÂÂÂ }
> > +ÂÂÂÂÂÂÂ up_write(&adapter->maps_lock);
> > +
> > +ÂÂÂÂÂÂÂ if (!found)
> > +ÂÂÂÂÂÂÂÂÂÂÂ return -EINVAL;
> > +
> > +ÂÂÂÂÂÂÂ matrix_mdev->map = map;
>
> See my comment above about matrix_mdev->gisc. Can't we just get rid
> of the matrix_mdev->map field and pass the map into the
> ap_ioctl_setirq() function?

or calculate it from parm... as we give parm as argument to this function

> > +ÂÂÂÂÂÂÂ ret = ap_ioctl_setirq(matrix_mdev, &parm);
> > +ÂÂÂÂÂÂÂ parm.status &= 0x00000000ffffffffUL;
> > +ÂÂÂÂÂÂÂ if (copy_to_user((void __user *)arg, &parm, sizeof(parm)))
> > +ÂÂÂÂÂÂÂÂÂÂÂ return -EFAULT;
> > +
> > +ÂÂÂÂÂÂÂ break;
>
> IMHO, the case statements should only determine which ioctl is being
> invoked and call the appropriate function to handle it. All of the above
> code could be in an intermediate function called from this case
> statement, thus reducing the case to calling the intermediate function.

OK, I can do so, however I would like to let the __user access here.

> > +ÂÂÂ case VFIO_AP_CLEAR_IRQ:
> > +ÂÂÂÂÂÂÂ if (copy_from_user(&parm, (void __user *)arg, sizeof(parm)))
> > +ÂÂÂÂÂÂÂÂÂÂÂ return -EFAULT;
> > +ÂÂÂÂÂÂÂ ret = ap_ioctl_clrirq(matrix_mdev, &parm);
> > +ÂÂÂÂÂÂÂ if (copy_to_user((void __user *)arg, &parm, sizeof(parm)))
> > +ÂÂÂÂÂÂÂÂÂÂÂ return -EFAULT;
> > +ÂÂÂÂÂÂÂ break;
> > ÂÂÂÂÂ case VFIO_DEVICE_GET_INFO:
> > ÂÂÂÂÂÂÂÂÂ ret = vfio_ap_mdev_get_device_info(arg);
> > ÂÂÂÂÂÂÂÂÂ break;

Thanks
Pierre

--
Pierre Morel
Linux/KVM/QEMU in BÃblingen - Germany