Re: [PATCH v1 4/7] vfio: ap: AP Queue Interrupt Control VFIO ioctl calls
From: Tony Krowiak
Date: Tue Nov 13 2018 - 10:41:08 EST
On 11/7/18 5:31 PM, Pierre Morel wrote:
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.
It can be added if/when that is implemented. As of now, it is not needed.
+ÂÂÂ 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
Better yet.
+ÂÂÂÂÂÂÂ 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.
I can live with that although I prefer the one liner 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