[PATCH v2 0/3] s390: vfio: ap: Using GISA for AP Interrupt

[PATCH v2 0/3] s390: vfio: ap: Using GISA for AP Interrupt

[Pierre Morel]

This patch series handles AP Interrupt using the GISA facility.
The patch series is based above the GISA+GIB patch series
sent by Michael Mueller.

- We define a new VFIO ioctl to ask the vfio_ap driver to register
  a interruption for the guest.
- We define the assembler code to register a GISA based interruption
  for the guest.
- We register to the GIB Alert mechanism and suppress the GISA
  (firmware) based interrupts from the list of software handled
  interrupts.

This patch series has been tested with success on z13 and z14
with adjunct processors CEX5[ACP] and CEX6C.

To use, you, of course, need the QEMU patches, which will be sent
separately.
Take care of the incompatbility in the ioctl parameters.

Pierre Morel (3):
  vfio: ap: Add AP Queue Interruption Control facility
  vfio: ap: ioctl definitions for AP Queue Interrupt Control
  vfio: ap: AP Queue Interrupt Control VFIO ioctl calls

 arch/s390/tools/gen_facilities.c  |   1 +
 drivers/s390/crypto/vfio_ap_ops.c | 110 +++++++++++++++++++++++++++++++++++++-
 include/uapi/linux/vfio.h         |  25 +++++++++
 3 files changed, 135 insertions(+), 1 deletion(-)

-- 
2.7.4

The goal of the first serie was merely to provide the possibility
to test GISA, and was quite a mess.
This serie is reworked with a simplier interface and almost
the half of LOCs.

Changelog:
(Thanks to Tony:)
- Use explicitely ISC as ioctl parameters
- Use ISC parameter in IRQ disable
- No more changes to the ap_matrix_mdev structure (was isc and map)
- suppress redondant functions
- use standard vfio_ap prefix for functions
- in ioctl use one line to call dedicated ioctl function
(Thanks to Conny:)
- no more complicated structure to int conversions
- Use explicitely apqn as ioctl parameters
 

[PATCH v2 1/3] vfio: ap: Add AP Queue Interruption Control facility

[Pierre Morel]

AP Queue Interruption Control (AQIC) facility gives
the guest the possibility to control interruption for
the Cryptographic Adjunct Processor queues

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
Reviewed-by: Tony Krowiak <akrowiak@linux.ibm.com>
---
 arch/s390/tools/gen_facilities.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/s390/tools/gen_facilities.c b/arch/s390/tools/gen_facilities.c
index fd788e0..18d317d 100644
--- a/arch/s390/tools/gen_facilities.c
+++ b/arch/s390/tools/gen_facilities.c
@@ -108,6 +108,7 @@ static struct facility_def facility_defs[] = {
 		.bits = (int[]){
 			12, /* AP Query Configuration Information */
 			15, /* AP Facilities Test */
+			65, /* AP Queue Interruption Control */
 			156, /* etoken facility */
 			-1  /* END */
 		}
-- 
2.7.4

[PATCH v2 2/3] vfio: ap: ioctl definitions for AP Queue Interrupt Control

[Pierre Morel]

We define two VFIO ioctl command to setup and clear
the AP Queues interrupt.

Arguments passed by the guest are:
- the apqn, AP queue number
- the Notification by address
- the identifier of the previously associated adapter

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 include/uapi/linux/vfio.h | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index 8131028..9a1b350 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -866,6 +866,31 @@ struct vfio_iommu_spapr_tce_remove {
 };
 #define VFIO_IOMMU_SPAPR_TCE_REMOVE	_IO(VFIO_TYPE, VFIO_BASE + 20)
 
+/**
+ * VFIO_AP_SET_IRQ - _IOWR(VFIO_TYPE, VFIO_BASE + 21, struct vfio_ap_aqic)
+ *
+ * Setup IRQ for an AP Queue
+ * @cmd contains the AP queue number (apqn)
+ * @status receives the resulting status of the command
+ * @nib is the Notification Indicator byte address
+ * @adapter_id allows to retrieve the associated adapter
+ */
+struct vfio_ap_aqic {
+	__u32   argsz;
+	__u32   flags;
+	/* out */
+	__u32 status;
+	/* in */
+	__u32 adapter_id;
+	__u64 nib;
+	__u16 apqn;
+	__u8 isc;
+	__u8 reserved[5];
+};
+#define VFIO_AP_SET_IRQ			_IO(VFIO_TYPE, VFIO_BASE + 21)
+#define VFIO_AP_CLEAR_IRQ		_IO(VFIO_TYPE, VFIO_BASE + 22)
+
 /* ***************************************************************** */
 
+
 #endif /* _UAPIVFIO_H */
-- 
2.7.4

Re: [PATCH v2 2/3] vfio: ap: ioctl definitions for AP Queue Interrupt Control

[Alex Williamson]

On Thu, 22 Nov 2018 18:11:14 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> We define two VFIO ioctl command to setup and clear
> the AP Queues interrupt.
> 
> Arguments passed by the guest are:
> - the apqn, AP queue number
> - the Notification by address
> - the identifier of the previously associated adapter


We have an extensible VFIO_DEVICE_SET_IRQS ioctl already, why does AP
need its own?


> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  include/uapi/linux/vfio.h | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index 8131028..9a1b350 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -866,6 +866,31 @@ struct vfio_iommu_spapr_tce_remove {
>  };
>  #define VFIO_IOMMU_SPAPR_TCE_REMOVE	_IO(VFIO_TYPE, VFIO_BASE + 20)
>  
> +/**
> + * VFIO_AP_SET_IRQ - _IOWR(VFIO_TYPE, VFIO_BASE + 21, struct vfio_ap_aqic)
> + *
> + * Setup IRQ for an AP Queue
> + * @cmd contains the AP queue number (apqn)
> + * @status receives the resulting status of the command
> + * @nib is the Notification Indicator byte address
> + * @adapter_id allows to retrieve the associated adapter
> + */
> +struct vfio_ap_aqic {
> +	__u32   argsz;
> +	__u32   flags;
> +	/* out */
> +	__u32 status;
> +	/* in */
> +	__u32 adapter_id;
> +	__u64 nib;
> +	__u16 apqn;
> +	__u8 isc;
> +	__u8 reserved[5];
> +};
> +#define VFIO_AP_SET_IRQ			_IO(VFIO_TYPE, VFIO_BASE + 21)
> +#define VFIO_AP_CLEAR_IRQ		_IO(VFIO_TYPE, VFIO_BASE + 22)
> +
>  /* ***************************************************************** */
>  
> +
>  #endif /* _UAPIVFIO_H */

Re: [PATCH v2 2/3] vfio: ap: ioctl definitions for AP Queue Interrupt Control

[Pierre Morel]

On 27/11/2018 18:22, Alex Williamson wrote:
> On Thu, 22 Nov 2018 18:11:14 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> We define two VFIO ioctl command to setup and clear
>> the AP Queues interrupt.
>>
>> Arguments passed by the guest are:
>> - the apqn, AP queue number
>> - the Notification by address
>> - the identifier of the previously associated adapter
> 
> 
> We have an extensible VFIO_DEVICE_SET_IRQS ioctl already, why does AP
> need its own?

For no good reason.
Sorry for this.
I will change for the use of the standard VFIO_DEVICE_SET_IRQS.

Thanks Alex.

Regards,
Pierre

> 
> 
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   include/uapi/linux/vfio.h | 25 +++++++++++++++++++++++++
>>   1 file changed, 25 insertions(+)
>>
>> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
>> index 8131028..9a1b350 100644
>> --- a/include/uapi/linux/vfio.h
>> +++ b/include/uapi/linux/vfio.h
>> @@ -866,6 +866,31 @@ struct vfio_iommu_spapr_tce_remove {
>>   };
>>   #define VFIO_IOMMU_SPAPR_TCE_REMOVE	_IO(VFIO_TYPE, VFIO_BASE + 20)
>>   
>> +/**
>> + * VFIO_AP_SET_IRQ - _IOWR(VFIO_TYPE, VFIO_BASE + 21, struct vfio_ap_aqic)
>> + *
>> + * Setup IRQ for an AP Queue
>> + * @cmd contains the AP queue number (apqn)
>> + * @status receives the resulting status of the command
>> + * @nib is the Notification Indicator byte address
>> + * @adapter_id allows to retrieve the associated adapter
>> + */
>> +struct vfio_ap_aqic {
>> +	__u32   argsz;
>> +	__u32   flags;
>> +	/* out */
>> +	__u32 status;
>> +	/* in */
>> +	__u32 adapter_id;
>> +	__u64 nib;
>> +	__u16 apqn;
>> +	__u8 isc;
>> +	__u8 reserved[5];
>> +};
>> +#define VFIO_AP_SET_IRQ			_IO(VFIO_TYPE, VFIO_BASE + 21)
>> +#define VFIO_AP_CLEAR_IRQ		_IO(VFIO_TYPE, VFIO_BASE + 22)
>> +
>>   /* ***************************************************************** */
>>   
>> +
>>   #endif /* _UAPIVFIO_H */
> 


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

[PATCH v2 3/3] vfio: ap: AP Queue Interrupt Control VFIO ioctl calls

[Pierre Morel]

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@linux.ibm.com>
---
 drivers/s390/crypto/vfio_ap_ops.c | 110 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 109 insertions(+), 1 deletion(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 272ef42..f6e942f 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -895,12 +895,121 @@ static int vfio_ap_mdev_get_device_info(unsigned long arg)
 	return copy_to_user((void __user *)arg, &info, minsz);
 }
 
+static unsigned long vfio_ap_get_nib(struct kvm *kvm, struct vfio_ap_aqic *parm)
+{
+	struct s390_io_adapter *adapter;
+	struct s390_map_info *map;
+	unsigned long nib;
+	int found = 0;
+
+	/* find the adapter */
+	if (parm->adapter_id > MAX_S390_IO_ADAPTERS)
+		return 0;
+
+	adapter = kvm->arch.adapters[parm->adapter_id];
+	if (!adapter)
+		return 0;
+
+	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 0;
+
+	nib = (unsigned long) page_address(map->page);
+	nib += (map->guest_addr & 0x0fff);
+
+	return nib;
+}
+
+static int vfio_ap_ioctl_setirq(struct mdev_device *mdev, unsigned long arg)
+{
+	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
+	struct vfio_ap_aqic parm;
+	struct ap_qirq_ctrl aqic_gisa = {};
+	struct kvm *kvm = matrix_mdev->kvm;
+	struct kvm_s390_gisa *gisa = kvm->arch.gisa;
+	struct ap_queue_status ap_status;
+	unsigned long nib;
+
+	if (copy_from_user(&parm, (void __user *)arg, sizeof(parm)))
+		return -EFAULT;
+
+	if (parm.isc > MAX_ISC)
+		return -EINVAL;
+
+	if (kvm->vcpus[0]->arch.sie_block->gd & 0x01)
+		aqic_gisa.gf = 1;
+
+	nib = vfio_ap_get_nib(kvm, &parm);
+	if (!nib)
+		return -ENODEV;
+
+	aqic_gisa.gisc = parm.isc;
+	aqic_gisa.isc = kvm_s390_gisc_register(kvm, parm.isc);
+	aqic_gisa.ir = 1;
+	aqic_gisa.gisa = gisa->next_alert >> 4;
+
+	ap_status = ap_aqic(parm.apqn, aqic_gisa, (void *)nib);
+	parm.status = *(uint32_t *)(&ap_status);
+
+	if (copy_to_user((void __user *)arg, &parm, sizeof(parm))) {
+		kvm_s390_gisc_unregister(kvm, parm.isc);
+		return -EFAULT;
+	}
+
+	if (ap_status.response_code) {
+		kvm_s390_gisc_unregister(kvm, parm.isc);
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static int vfio_ap_ioctl_clrirq(struct mdev_device *mdev, unsigned long arg)
+{
+	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
+	struct vfio_ap_aqic parm;
+	struct ap_qirq_ctrl aqic_gisa = {};
+	struct ap_queue_status ap_status;
+	struct kvm *kvm = matrix_mdev->kvm;
+
+	if (copy_from_user(&parm, (void __user *)arg, sizeof(parm)))
+		return -EFAULT;
+
+	if (kvm->vcpus[0]->arch.sie_block->gd & 0x01)
+		aqic_gisa.gf = 1;
+	aqic_gisa.ir = 0;
+
+	ap_status = ap_aqic(parm.apqn, aqic_gisa, NULL);
+	parm.status = *(uint32_t *)(&ap_status);
+
+	kvm_s390_gisc_unregister(kvm, parm.isc);
+
+	if (copy_to_user((void __user *)arg, &parm, sizeof(parm)))
+		return -EFAULT;
+
+	return (ap_status.response_code) ? -EIO : 0;
+}
+
 static ssize_t vfio_ap_mdev_ioctl(struct mdev_device *mdev,
 				    unsigned int cmd, unsigned long arg)
 {
 	int ret;
 
 	switch (cmd) {
+	case VFIO_AP_SET_IRQ:
+		ret = vfio_ap_ioctl_setirq(mdev, arg);
+		break;
+	case VFIO_AP_CLEAR_IRQ:
+		ret = vfio_ap_ioctl_clrirq(mdev, arg);
+		break;
 	case VFIO_DEVICE_GET_INFO:
 		ret = vfio_ap_mdev_get_device_info(arg);
 		break;
@@ -911,7 +1020,6 @@ static ssize_t vfio_ap_mdev_ioctl(struct mdev_device *mdev,
 		ret = -EOPNOTSUPP;
 		break;
 	}
-
 	return ret;
 }
 
-- 
2.7.4

Re: [PATCH v2 3/3] vfio: ap: AP Queue Interrupt Control VFIO ioctl calls

[Cornelia Huck]

On Thu, 22 Nov 2018 18:11:15 +0100
Pierre Morel <pmorel@linux.ibm.com> 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@linux.ibm.com>
> ---
>  drivers/s390/crypto/vfio_ap_ops.c | 110 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 109 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 272ef42..f6e942f 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -895,12 +895,121 @@ static int vfio_ap_mdev_get_device_info(unsigned long arg)
>  	return copy_to_user((void __user *)arg, &info, minsz);
>  }
>  
> +static unsigned long vfio_ap_get_nib(struct kvm *kvm, struct vfio_ap_aqic *parm)
> +{
> +	struct s390_io_adapter *adapter;
> +	struct s390_map_info *map;
> +	unsigned long nib;
> +	int found = 0;
> +
> +	/* find the adapter */
> +	if (parm->adapter_id > MAX_S390_IO_ADAPTERS)
> +		return 0;
> +
> +	adapter = kvm->arch.adapters[parm->adapter_id];
> +	if (!adapter)
> +		return 0;
> +
> +	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);

Regardless of which user space interface you ultimately use: I think
you should leave poking the adapter handling innards to the adapter
code and instead create and use an interface to look up the mapping
from the guest address.

> +
> +	if (!found)
> +		return 0;
> +
> +	nib = (unsigned long) page_address(map->page);
> +	nib += (map->guest_addr & 0x0fff);
> +
> +	return nib;
> +}

Re: [PATCH v2 3/3] vfio: ap: AP Queue Interrupt Control VFIO ioctl calls

[Pierre Morel]

On 29/11/2018 12:37, Cornelia Huck wrote:
> On Thu, 22 Nov 2018 18:11:15 +0100
> Pierre Morel <pmorel@linux.ibm.com> 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@linux.ibm.com>
>> ---
>>   drivers/s390/crypto/vfio_ap_ops.c | 110 +++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 109 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
>> index 272ef42..f6e942f 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -895,12 +895,121 @@ static int vfio_ap_mdev_get_device_info(unsigned long arg)
>>   	return copy_to_user((void __user *)arg, &info, minsz);
>>   }
>>   
>> +static unsigned long vfio_ap_get_nib(struct kvm *kvm, struct vfio_ap_aqic *parm)
>> +{
>> +	struct s390_io_adapter *adapter;
>> +	struct s390_map_info *map;
>> +	unsigned long nib;
>> +	int found = 0;
>> +
>> +	/* find the adapter */
>> +	if (parm->adapter_id > MAX_S390_IO_ADAPTERS)
>> +		return 0;
>> +
>> +	adapter = kvm->arch.adapters[parm->adapter_id];
>> +	if (!adapter)
>> +		return 0;
>> +
>> +	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);
> 
> Regardless of which user space interface you ultimately use: I think
> you should leave poking the adapter handling innards to the adapter
> code and instead create and use an interface to look up the mapping
> from the guest address.

Right...
Thanks

Pierre

> 
>> +
>> +	if (!found)
>> +		return 0;
>> +
>> +	nib = (unsigned long) page_address(map->page);
>> +	nib += (map->guest_addr & 0x0fff);
>> +
>> +	return nib;
>> +}
> 


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

Re: [PATCH v2 3/3] vfio: ap: AP Queue Interrupt Control VFIO ioctl calls

[Cornelia Huck]

On Thu, 22 Nov 2018 18:11:15 +0100
Pierre Morel <pmorel@linux.ibm.com> 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@linux.ibm.com>
> ---
>  drivers/s390/crypto/vfio_ap_ops.c | 110 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 109 insertions(+), 1 deletion(-)
> 

> +static int vfio_ap_ioctl_setirq(struct mdev_device *mdev, unsigned long arg)
> +{
> +	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
> +	struct vfio_ap_aqic parm;
> +	struct ap_qirq_ctrl aqic_gisa = {};
> +	struct kvm *kvm = matrix_mdev->kvm;
> +	struct kvm_s390_gisa *gisa = kvm->arch.gisa;
> +	struct ap_queue_status ap_status;
> +	unsigned long nib;
> +
> +	if (copy_from_user(&parm, (void __user *)arg, sizeof(parm)))
> +		return -EFAULT;
> +
> +	if (parm.isc > MAX_ISC)
> +		return -EINVAL;
> +
> +	if (kvm->vcpus[0]->arch.sie_block->gd & 0x01)
> +		aqic_gisa.gf = 1;
> +
> +	nib = vfio_ap_get_nib(kvm, &parm);
> +	if (!nib)
> +		return -ENODEV;
> +
> +	aqic_gisa.gisc = parm.isc;
> +	aqic_gisa.isc = kvm_s390_gisc_register(kvm, parm.isc);

Oh, and as I just looked at the callers of these functions: You'll want
to check the return code here, even though the function should not fail
with the checking you did beforehand.

[I assume you'll have similar code even when using a different
interface.]

> +	aqic_gisa.ir = 1;
> +	aqic_gisa.gisa = gisa->next_alert >> 4;
> +
> +	ap_status = ap_aqic(parm.apqn, aqic_gisa, (void *)nib);
> +	parm.status = *(uint32_t *)(&ap_status);
> +
> +	if (copy_to_user((void __user *)arg, &parm, sizeof(parm))) {
> +		kvm_s390_gisc_unregister(kvm, parm.isc);
> +		return -EFAULT;
> +	}
> +
> +	if (ap_status.response_code) {
> +		kvm_s390_gisc_unregister(kvm, parm.isc);
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +static int vfio_ap_ioctl_clrirq(struct mdev_device *mdev, unsigned long arg)
> +{
> +	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
> +	struct vfio_ap_aqic parm;
> +	struct ap_qirq_ctrl aqic_gisa = {};
> +	struct ap_queue_status ap_status;
> +	struct kvm *kvm = matrix_mdev->kvm;
> +
> +	if (copy_from_user(&parm, (void __user *)arg, sizeof(parm)))
> +		return -EFAULT;
> +
> +	if (kvm->vcpus[0]->arch.sie_block->gd & 0x01)
> +		aqic_gisa.gf = 1;
> +	aqic_gisa.ir = 0;
> +
> +	ap_status = ap_aqic(parm.apqn, aqic_gisa, NULL);
> +	parm.status = *(uint32_t *)(&ap_status);
> +
> +	kvm_s390_gisc_unregister(kvm, parm.isc);

Here, you don't seem to verify the sanity of parm.isc beforehand...
luckily the function can deal with that :)

> +
> +	if (copy_to_user((void __user *)arg, &parm, sizeof(parm)))
> +		return -EFAULT;
> +
> +	return (ap_status.response_code) ? -EIO : 0;
> +}
> +

Re: [PATCH v2 3/3] vfio: ap: AP Queue Interrupt Control VFIO ioctl calls

[Pierre Morel]

On 03/12/2018 11:04, Cornelia Huck wrote:
> On Thu, 22 Nov 2018 18:11:15 +0100
> Pierre Morel <pmorel@linux.ibm.com> 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@linux.ibm.com>
>> ---
>>   drivers/s390/crypto/vfio_ap_ops.c | 110 +++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 109 insertions(+), 1 deletion(-)
>>
> 
>> +static int vfio_ap_ioctl_setirq(struct mdev_device *mdev, unsigned long arg)
>> +{
>> +	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
>> +	struct vfio_ap_aqic parm;
>> +	struct ap_qirq_ctrl aqic_gisa = {};
>> +	struct kvm *kvm = matrix_mdev->kvm;
>> +	struct kvm_s390_gisa *gisa = kvm->arch.gisa;
>> +	struct ap_queue_status ap_status;
>> +	unsigned long nib;
>> +
>> +	if (copy_from_user(&parm, (void __user *)arg, sizeof(parm)))
>> +		return -EFAULT;
>> +
>> +	if (parm.isc > MAX_ISC)
>> +		return -EINVAL;
>> +
>> +	if (kvm->vcpus[0]->arch.sie_block->gd & 0x01)
>> +		aqic_gisa.gf = 1;
>> +
>> +	nib = vfio_ap_get_nib(kvm, &parm);
>> +	if (!nib)
>> +		return -ENODEV;
>> +
>> +	aqic_gisa.gisc = parm.isc;
>> +	aqic_gisa.isc = kvm_s390_gisc_register(kvm, parm.isc);
> 
> Oh, and as I just looked at the callers of these functions: You'll want
> to check the return code here, even though the function should not fail
> with the checking you did beforehand.
> 

I should check.

> [I assume you'll have similar code even when using a different
> interface.]

Yes.


> 
>> +	aqic_gisa.ir = 1;
>> +	aqic_gisa.gisa = gisa->next_alert >> 4;
>> +
>> +	ap_status = ap_aqic(parm.apqn, aqic_gisa, (void *)nib);
>> +	parm.status = *(uint32_t *)(&ap_status);
>> +
>> +	if (copy_to_user((void __user *)arg, &parm, sizeof(parm))) {
>> +		kvm_s390_gisc_unregister(kvm, parm.isc);
>> +		return -EFAULT;
>> +	}
>> +
>> +	if (ap_status.response_code) {
>> +		kvm_s390_gisc_unregister(kvm, parm.isc);
>> +		return -EIO;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int vfio_ap_ioctl_clrirq(struct mdev_device *mdev, unsigned long arg)
>> +{
>> +	struct ap_matrix_mdev *matrix_mdev = mdev_get_drvdata(mdev);
>> +	struct vfio_ap_aqic parm;
>> +	struct ap_qirq_ctrl aqic_gisa = {};
>> +	struct ap_queue_status ap_status;
>> +	struct kvm *kvm = matrix_mdev->kvm;
>> +
>> +	if (copy_from_user(&parm, (void __user *)arg, sizeof(parm)))
>> +		return -EFAULT;
>> +
>> +	if (kvm->vcpus[0]->arch.sie_block->gd & 0x01)
>> +		aqic_gisa.gf = 1;
>> +	aqic_gisa.ir = 0;
>> +
>> +	ap_status = ap_aqic(parm.apqn, aqic_gisa, NULL);
>> +	parm.status = *(uint32_t *)(&ap_status);
>> +
>> +	kvm_s390_gisc_unregister(kvm, parm.isc);
> 
> Here, you don't seem to verify the sanity of parm.isc beforehand...
> luckily the function can deal with that :)

You are right.
Anyway I will change this, because this relies on user's code which is 
not right.


> 
>> +
>> +	if (copy_to_user((void __user *)arg, &parm, sizeof(parm)))
>> +		return -EFAULT;
>> +
>> +	return (ap_status.response_code) ? -EIO : 0;
>> +}
>> +
> 


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