[PATCH v1 0/7] s390: vfio: ap: Using GISA for AP Interrupt

[PATCH v1 0/7] s390: vfio: ap: Using GISA for AP Interrupt

[Pierre Morel]

This is the first version to handle AP Interrupt
using the GISA facility.
The patch series is based on 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.


Pierre Morel (7):
  vfio: ap: Add AP Queue Interruption Control facility
  vfio: ap: VFIO AP Queue Interrupt Control
  vfio: ap: AP Queue Interrupt structures definitions
  vfio: ap: AP Queue Interrupt Control VFIO ioctl calls
  s390: kvm: export GIB registration
  vfio: ap: register guest ISC with GISA and GIB
  s390: kvm: Handle all GISA IPM bits through GISA

 arch/s390/include/asm/kvm_host.h      |   3 +
 arch/s390/kvm/interrupt.c             |   3 +-
 arch/s390/tools/gen_facilities.c      |   1 +
 drivers/s390/crypto/vfio_ap_ops.c     | 105 ++++++++++++++++++++++++++
 drivers/s390/crypto/vfio_ap_private.h |  77 +++++++++++++++++++
 include/uapi/linux/vfio.h             |  22 ++++++
 6 files changed, 209 insertions(+), 2 deletions(-)

-- 
2.17.0

[PATCH v1 1/7] 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>
---
 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 fd788e0f2e5b..18d317da02f5 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.17.0

Re: [PATCH v1 1/7] vfio: ap: Add AP Queue Interruption Control facility

[Tony Krowiak]

On 10/31/18 2:12 PM, Pierre Morel wrote:
> 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>
> ---
>   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 fd788e0f2e5b..18d317da02f5 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 */
>   		}

Reviewed-by: Tony Krowiak <akrowiak@linux.ibm.com>

> 

[PATCH v1 2/7] vfio: ap: VFIO 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 | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index f378b9802d8b..f13079bb28af 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -816,6 +816,28 @@ 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;
+	/* in */
+	__u64 cmd;
+	__u64 status;
+	__u64 nib;
+	__u32 adapter_id;
+};
+#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.17.0

[PATCH v1 3/7] vfio: ap: AP Queue Interrupt structures definitions

[Pierre Morel]

We define all the structures we need to let GISA handle
the AP Queues Interrupt.

This patch defines the inline assembler for AP Queue Interrupt
Control instruction with GISA, some utilities to manipulate
the data in the registers used by this instruction.

We also define new ap_matrix components to handle interruptions
and adapter mapping.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 drivers/s390/crypto/vfio_ap_private.h | 77 +++++++++++++++++++++++++++
 1 file changed, 77 insertions(+)

diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
index 5675492233c7..45103865bd7f 100644
--- a/drivers/s390/crypto/vfio_ap_private.h
+++ b/drivers/s390/crypto/vfio_ap_private.h
@@ -74,15 +74,92 @@ struct ap_matrix {
  * @group_notifier: notifier block used for specifying callback function for
  *		    handling the VFIO_GROUP_NOTIFY_SET_KVM event
  * @kvm:	the struct holding guest's state
+ * @map:	the adapter information for QEMU mapping
+ * @gisc:	the Guest ISC
  */
 struct ap_matrix_mdev {
 	struct list_head node;
 	struct ap_matrix matrix;
 	struct notifier_block group_notifier;
 	struct kvm *kvm;
+	struct s390_map_info *map;
+	unsigned char gisc;
 };
 
 extern int vfio_ap_mdev_register(void);
 extern void vfio_ap_mdev_unregister(void);
 
+/* AP Queue Interrupt Control associated structures and functions */
+struct aqic_gisa {
+	uint8_t  rzone;
+	uint8_t  izone;
+	unsigned	ir:1;
+	unsigned	reserved1:4;
+	unsigned	gisc:3;
+	unsigned	reserved2:6;
+	unsigned	f:2;
+	unsigned	reserved3:1;
+	unsigned	gisao:27;
+	unsigned	t:1;
+	unsigned	isc:3;
+}  __packed __aligned(8);
+
+struct ap_status {
+	unsigned	e:1;
+	unsigned	r:1;
+	unsigned	f:1;
+	unsigned	reserved:4;
+	unsigned	i:1;
+	unsigned	rc:8;
+	unsigned	pad:16;
+}  __packed __aligned(4);
+
+static inline uint32_t status2reg(struct ap_status a)
+{
+	return *(uint32_t *)(&a);
+}
+
+static inline struct ap_status reg2status(uint32_t r)
+{
+	return *(struct ap_status *)(&r);
+}
+
+static inline struct aqic_gisa reg2aqic(uint64_t r)
+{
+	return *((struct aqic_gisa *)&r);
+}
+
+static inline uint64_t aqic2reg(struct aqic_gisa a)
+{
+	return *((uint64_t *)&a);
+}
+
+/**
+ * ap_host_aqic - Issue the host AQIC instruction.
+ * @apqn is the AP queue number
+ * @gr1  the caller must have setup the register
+ *       with GISA address and format, with interrupt
+ *       request, ISC and guest ISC
+ * @gr2  the caller must have setup the register
+ *       to the guest NIB physical address
+ *
+ * issue the AQIC PQAP instruction and return the AP status
+ * word
+ */
+static inline uint32_t ap_host_aqic(uint64_t apqn, uint64_t gr1,
+				    uint64_t gr2)
+{
+	register unsigned long reg0 asm ("0") = apqn | (3UL << 24);
+	register unsigned long reg1_in asm ("1") = gr1;
+	register uint32_t reg1_out asm ("1");
+	register unsigned long reg2 asm ("2") = gr2;
+
+	asm volatile(
+		".long 0xb2af0000"	  /* PQAP(AQIC) */
+		: "+d" (reg0), "+d" (reg1_in), "=d" (reg1_out), "+d" (reg2)
+		:
+		: "cc");
+	return reg1_out;
+}
+
 #endif /* _VFIO_AP_PRIVATE_H_ */
-- 
2.17.0

Re: [PATCH v1 3/7] vfio: ap: AP Queue Interrupt structures definitions

[Tony Krowiak]

On 10/31/18 2:12 PM, Pierre Morel wrote:
> We define all the structures we need to let GISA handle
> the AP Queues Interrupt.
> 
> This patch defines the inline assembler for AP Queue Interrupt
> Control instruction with GISA, some utilities to manipulate
> the data in the registers used by this instruction.
> 
> We also define new ap_matrix components to handle interruptions
> and adapter mapping.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>   drivers/s390/crypto/vfio_ap_private.h | 77 +++++++++++++++++++++++++++
>   1 file changed, 77 insertions(+)
> 
> diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
> index 5675492233c7..45103865bd7f 100644
> --- a/drivers/s390/crypto/vfio_ap_private.h
> +++ b/drivers/s390/crypto/vfio_ap_private.h
> @@ -74,15 +74,92 @@ struct ap_matrix {
>    * @group_notifier: notifier block used for specifying callback function for
>    *		    handling the VFIO_GROUP_NOTIFY_SET_KVM event
>    * @kvm:	the struct holding guest's state
> + * @map:	the adapter information for QEMU mapping
> + * @gisc:	the Guest ISC
>    */
>   struct ap_matrix_mdev {
>   	struct list_head node;
>   	struct ap_matrix matrix;
>   	struct notifier_block group_notifier;
>   	struct kvm *kvm;
> +	struct s390_map_info *map;
> +	unsigned char gisc;
>   };
>   
>   extern int vfio_ap_mdev_register(void);
>   extern void vfio_ap_mdev_unregister(void);
>   
> +/* AP Queue Interrupt Control associated structures and functions */
> +struct aqic_gisa {
> +	uint8_t  rzone;
> +	uint8_t  izone;
> +	unsigned	ir:1;
> +	unsigned	reserved1:4;
> +	unsigned	gisc:3;
> +	unsigned	reserved2:6;
> +	unsigned	f:2;
> +	unsigned	reserved3:1;
> +	unsigned	gisao:27;
> +	unsigned	t:1;
> +	unsigned	isc:3;
> +}  __packed __aligned(8);

This struct is redundant with the 'struct ap_qirq_ctrl'
defined in arch/s390/include/asm/ap.h. That file also includes
the asm for the ap_aqic function.

> +
> +struct ap_status {
> +	unsigned	e:1;
> +	unsigned	r:1;
> +	unsigned	f:1;
> +	unsigned	reserved:4;
> +	unsigned	i:1;
> +	unsigned	rc:8;
> +	unsigned	pad:16;
> +}  __packed __aligned(4);
> +
> +static inline uint32_t status2reg(struct ap_status a)
> +{
> +	return *(uint32_t *)(&a);
> +}
> +
> +static inline struct ap_status reg2status(uint32_t r)
> +{
> +	return *(struct ap_status *)(&r);
> +}
> +
> +static inline struct aqic_gisa reg2aqic(uint64_t r)
> +{
> +	return *((struct aqic_gisa *)&r);
> +}
> +
> +static inline uint64_t aqic2reg(struct aqic_gisa a)
> +{
> +	return *((uint64_t *)&a);
> +}
> +
> +/**
> + * ap_host_aqic - Issue the host AQIC instruction.
> + * @apqn is the AP queue number
> + * @gr1  the caller must have setup the register
> + *       with GISA address and format, with interrupt
> + *       request, ISC and guest ISC
> + * @gr2  the caller must have setup the register
> + *       to the guest NIB physical address
> + *
> + * issue the AQIC PQAP instruction and return the AP status
> + * word
> + */
> +static inline uint32_t ap_host_aqic(uint64_t apqn, uint64_t gr1,
> +				    uint64_t gr2)
> +

> +	register unsigned long reg0 asm ("0") = apqn | (3UL << 24);
> +	register unsigned long reg1_in asm ("1") = gr1;
> +	register uint32_t reg1_out asm ("1");
> +	register unsigned long reg2 asm ("2") = gr2;
> +
> +	asm volatile(
> +		".long 0xb2af0000"	  /* PQAP(AQIC) */
> +		: "+d" (reg0), "+d" (reg1_in), "=d" (reg1_out), "+d" (reg2)
> +		:
> +		: "cc");
> +	return reg1_out;
> +}

This function is redundant to the ap_aqic function in
arch/s390/include/asm/ap.h

> +
>   #endif /* _VFIO_AP_PRIVATE_H_ */
> 

Re: [PATCH v1 3/7] vfio: ap: AP Queue Interrupt structures definitions

[Pierre Morel]

On 02/11/2018 16:14, Tony Krowiak wrote:
> On 10/31/18 2:12 PM, Pierre Morel wrote:
>> We define all the structures we need to let GISA handle
>> the AP Queues Interrupt.

...

>> +struct aqic_gisa {
>> +    uint8_t  rzone;
>> +    uint8_t  izone;
>> +    unsigned    ir:1;
>> +    unsigned    reserved1:4;
>> +    unsigned    gisc:3;
>> +    unsigned    reserved2:6;
>> +    unsigned    f:2;
>> +    unsigned    reserved3:1;
>> +    unsigned    gisao:27;
>> +    unsigned    t:1;
>> +    unsigned    isc:3;
>> +}  __packed __aligned(8);
> 
> This struct is redundant with the 'struct ap_qirq_ctrl'
> defined in arch/s390/include/asm/ap.h. That file also includes
> the asm for the ap_aqic function.

Exact.
My code is quite old and I forgot to adapt it to the new definitions.
I will do it for this, aqic, the inline assembler and the status.

Thanks,

Pierre

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

[PATCH v1 4/7] 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 | 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,
+			   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;
+	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)
+{
+	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;
+		/* find the adapter */
+		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;
+		ret = ap_ioctl_setirq(matrix_mdev, &parm);
+		parm.status &= 0x00000000ffffffffUL;
+		if (copy_to_user((void __user *)arg, &parm, sizeof(parm)))
+			return -EFAULT;
+
+		break;
+	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;
-- 
2.17.0

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2707 bytes --]

Hi Pierre,

I love your patch! Yet something to improve:

[auto build test ERROR on s390/features]
[also build test ERROR on next-20181101]
[cannot apply to v4.19]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Pierre-Morel/s390-vfio-ap-Using-GISA-for-AP-Interrupt/20181102-010854
base:   https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git features
config: s390-allmodconfig (attached as .config)
compiler: s390x-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.2.0 make.cross ARCH=s390 

Note: the linux-review/Pierre-Morel/s390-vfio-ap-Using-GISA-for-AP-Interrupt/20181102-010854 HEAD 1235cf4914e223e3da89385619976de8eea4e9db builds fine.
      It only hurts bisectibility.

All errors (new ones prefixed by >>):

   drivers/s390/crypto/vfio_ap_ops.c: In function 'ap_ioctl_setirq':
>> drivers/s390/crypto/vfio_ap_ops.c:915:18: error: 'GAL_ISC' undeclared (first use in this function); did you mean 'MAX_ISC'?
     aqic_gisa.isc = GAL_ISC;
                     ^~~~~~~
                     MAX_ISC
   drivers/s390/crypto/vfio_ap_ops.c:915:18: note: each undeclared identifier is reported only once for each function it appears in

vim +915 drivers/s390/crypto/vfio_ap_ops.c

   897	
   898	static int ap_ioctl_setirq(struct ap_matrix_mdev *matrix_mdev,
   899				   struct vfio_ap_aqic *parm)
   900	{
   901		struct aqic_gisa aqic_gisa = reg2aqic(0);
   902		struct kvm_s390_gisa *gisa = matrix_mdev->kvm->arch.gisa;
   903		struct ap_status ap_status = reg2status(0);
   904		unsigned long p;
   905		int ret = -1;
   906		int apqn;
   907		uint32_t gd;
   908	
   909		apqn = (int)(parm->cmd & 0xffff);
   910	
   911		gd = matrix_mdev->kvm->vcpus[0]->arch.sie_block->gd;
   912		if (gd & 0x01)
   913			aqic_gisa.f = 1;
   914		aqic_gisa.gisc = matrix_mdev->gisc;
 > 915		aqic_gisa.isc = GAL_ISC;
   916		aqic_gisa.ir = 1;
   917		aqic_gisa.gisao = gisa->next_alert >> 4;
   918	
   919		p = (unsigned long) page_address(matrix_mdev->map->page);
   920		p += (matrix_mdev->map->guest_addr & 0x0fff);
   921	
   922		ret = ap_host_aqic((uint64_t)apqn, aqic2reg(aqic_gisa), p);
   923		parm->status = ret;
   924	
   925		ap_status = reg2status(ret);
   926		return (ap_status.rc) ? -EIO : 0;
   927	}
   928	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 51778 bytes --]

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

[Tony Krowiak]

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@linux.ibm.com>
> ---
>   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???

> +			   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?

> +	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???

> +{
> +	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()?

> +		/* 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?

> +		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.

> +	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;
> 

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@linux.ibm.com>
>> ---
>>   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

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

[Tony Krowiak]

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@linux.ibm.com>
>>> ---
>>>   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
> 

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

[Cornelia Huck]

On Wed, 31 Oct 2018 19:12:54 +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 | 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,
> +			   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);

It seems you always use cmd & 0xffff only. What if there is other stuff
in the remaining bits of cmd? Do you plan to ignore it in any case, or
should you actively check that there is nothing in it?

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

[Pierre Morel]

On 07/11/2018 10:46, Cornelia Huck wrote:
> On Wed, 31 Oct 2018 19:12:54 +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 | 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,
>> +			   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);
> 
> It seems you always use cmd & 0xffff only. What if there is other stuff
> in the remaining bits of cmd? Do you plan to ignore it in any case, or
> should you actively check that there is nothing in it?
> 

I do not think that the ioctl interface should reflect the hardware 
interface.
The ioctl interface ignores the remaining bits.
We ignore the FC because we obviously want to make a AQIC FC=3
We ignore the T bit.

But we receive the information from the intercepting software, i.e. QEMU 
which should I think do the checks before using the ioctl interface.

It seemed easier to me to pass the complete registers and to ignore some 
bits in them. In case we get any change in the future
But we could also only pass the APQN

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

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

[Cornelia Huck]

On Wed, 7 Nov 2018 23:23:40 +0100
Pierre Morel <pmorel@linux.ibm.com> wrote:

> On 07/11/2018 10:46, Cornelia Huck wrote:
> > On Wed, 31 Oct 2018 19:12:54 +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 | 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,
> >> +			   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);  
> > 
> > It seems you always use cmd & 0xffff only. What if there is other stuff
> > in the remaining bits of cmd? Do you plan to ignore it in any case, or
> > should you actively check that there is nothing in it?
> >   
> 
> I do not think that the ioctl interface should reflect the hardware 
> interface.
> The ioctl interface ignores the remaining bits.
> We ignore the FC because we obviously want to make a AQIC FC=3
> We ignore the T bit.
> 
> But we receive the information from the intercepting software, i.e. QEMU 
> which should I think do the checks before using the ioctl interface.

Yes, it should; but you still can't know whether it actually did...

> 
> It seemed easier to me to pass the complete registers and to ignore some 
> bits in them. In case we get any change in the future
> But we could also only pass the APQN

I'd prefer to use a well-defined structure that explicitly handles the
userspace<->kernel communication. Not that we start relying on implicit
assumptions and then things break when userspace does something
different...

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

[Pierre Morel]

On 08/11/2018 10:14, Cornelia Huck wrote:
> On Wed, 7 Nov 2018 23:23:40 +0100
> Pierre Morel <pmorel@linux.ibm.com> wrote:
> 
>> On 07/11/2018 10:46, Cornelia Huck wrote:
>>> On Wed, 31 Oct 2018 19:12:54 +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 | 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,
>>>> +			   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);
>>>
>>> It seems you always use cmd & 0xffff only. What if there is other stuff
>>> in the remaining bits of cmd? Do you plan to ignore it in any case, or
>>> should you actively check that there is nothing in it?
>>>    
>>
>> I do not think that the ioctl interface should reflect the hardware
>> interface.
>> The ioctl interface ignores the remaining bits.
>> We ignore the FC because we obviously want to make a AQIC FC=3
>> We ignore the T bit.
>>
>> But we receive the information from the intercepting software, i.e. QEMU
>> which should I think do the checks before using the ioctl interface.
> 
> Yes, it should; but you still can't know whether it actually did...

I do not care, I just ignore these bits.

> 
>>
>> It seemed easier to me to pass the complete registers and to ignore some
>> bits in them. In case we get any change in the future
>> But we could also only pass the APQN
> 
> I'd prefer to use a well-defined structure that explicitly handles the
> userspace<->kernel communication. Not that we start relying on implicit
> assumptions and then things break when userspace does something
> different...
> 

OK, I can pass a u16 in the ioctl parameters and explicitly reserve the 
ignored bits.

Thanks for the review.

Regards,
Pierre

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

[PATCH v1 5/7] s390: kvm: export GIB registration

[Pierre Morel]

Define the GIB Alert registration for external users
inside the standrd kvm_host.h header file.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 arch/s390/include/asm/kvm_host.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index 512ea9f200fa..7e76a56da72d 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -902,4 +902,7 @@ static inline void kvm_arch_vcpu_unblocking(struct kvm_vcpu *vcpu) {}
 
 void kvm_arch_vcpu_block_finish(struct kvm_vcpu *vcpu);
 
+extern int kvm_s390_gisc_register(struct kvm *kvm, u32 gisc);
+extern int kvm_s390_gisc_unregister(struct kvm *kvm, u32 gisc);
+
 #endif
-- 
2.17.0

[PATCH v1 6/7] vfio: ap: register guest ISC with GISA and GIB

[Pierre Morel]

Register to the GIB Alert list and retrieve the GAL_ISC
to pass to the GISA registration.

Unregister on error and when clearing the interrupt.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
---
 drivers/s390/crypto/vfio_ap_ops.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index f68102163bf4..232168797fb8 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -903,16 +903,20 @@ static int ap_ioctl_setirq(struct ap_matrix_mdev *matrix_mdev,
 	struct ap_status ap_status = reg2status(0);
 	unsigned long p;
 	int ret = -1;
-	int apqn;
+	int apqn, gal_isc;
 	uint32_t gd;
 
+	gal_isc = kvm_s390_gisc_register(matrix_mdev->kvm, matrix_mdev->gisc);
+	if (gal_isc < 0)
+		return -EIO;
+
 	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;
-	aqic_gisa.isc = GAL_ISC;
+	aqic_gisa.isc = gal_isc;
 	aqic_gisa.ir = 1;
 	aqic_gisa.gisao = gisa->next_alert >> 4;
 
@@ -923,7 +927,11 @@ static int ap_ioctl_setirq(struct ap_matrix_mdev *matrix_mdev,
 	parm->status = ret;
 
 	ap_status = reg2status(ret);
-	return (ap_status.rc) ? -EIO : 0;
+	if (ap_status.rc) {
+		kvm_s390_gisc_unregister(matrix_mdev->kvm, matrix_mdev->gisc);
+		return -EIO;
+	}
+	return 0;
 }
 
 static int ap_ioctl_clrirq(struct ap_matrix_mdev *matrix_mdev,
@@ -946,6 +954,8 @@ static int ap_ioctl_clrirq(struct ap_matrix_mdev *matrix_mdev,
 	parm->status = retval;
 
 	ap_status = reg2status(retval);
+	/* unregister the IAM from the GIB anyway! */
+	kvm_s390_gisc_unregister(matrix_mdev->kvm, matrix_mdev->gisc);
 	return (ap_status.rc) ? -EIO : 0;
 }
 
-- 
2.17.0

Re: [PATCH v1 6/7] vfio: ap: register guest ISC with GISA and GIB

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1253 bytes --]

Hi Pierre,

I love your patch! Yet something to improve:

[auto build test ERROR on s390/features]
[also build test ERROR on next-20181101]
[cannot apply to v4.19]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Pierre-Morel/s390-vfio-ap-Using-GISA-for-AP-Interrupt/20181102-010854
base:   https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git features
config: s390-allmodconfig (attached as .config)
compiler: s390x-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.2.0 make.cross ARCH=s390 

All errors (new ones prefixed by >>):

>> ERROR: "kvm_s390_gisc_unregister" [drivers/s390/crypto/vfio_ap.ko] undefined!
>> ERROR: "kvm_s390_gisc_register" [drivers/s390/crypto/vfio_ap.ko] undefined!
   ERROR: "__node_distance" [drivers/nvme/host/nvme-core.ko] undefined!

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 51718 bytes --]

Re: [PATCH v1 6/7] vfio: ap: register guest ISC with GISA and GIB

[Tony Krowiak]

On 10/31/18 2:12 PM, Pierre Morel wrote:
> Register to the GIB Alert list and retrieve the GAL_ISC
> to pass to the GISA registration.
> 
> Unregister on error and when clearing the interrupt.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
> ---
>   drivers/s390/crypto/vfio_ap_ops.c | 16 +++++++++++++---
>   1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index f68102163bf4..232168797fb8 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -903,16 +903,20 @@ static int ap_ioctl_setirq(struct ap_matrix_mdev *matrix_mdev,
>   	struct ap_status ap_status = reg2status(0);
>   	unsigned long p;
>   	int ret = -1;
> -	int apqn;
> +	int apqn, gal_isc;
>   	uint32_t gd;
>   
> +	gal_isc = kvm_s390_gisc_register(matrix_mdev->kvm, matrix_mdev->gisc);
> +	if (gal_isc < 0)
> +		return -EIO;
> +
>   	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;
> -	aqic_gisa.isc = GAL_ISC;
> +	aqic_gisa.isc = gal_isc;
>   	aqic_gisa.ir = 1;
>   	aqic_gisa.gisao = gisa->next_alert >> 4;
>   
> @@ -923,7 +927,11 @@ static int ap_ioctl_setirq(struct ap_matrix_mdev *matrix_mdev,
>   	parm->status = ret;
>   
>   	ap_status = reg2status(ret);
> -	return (ap_status.rc) ? -EIO : 0;
> +	if (ap_status.rc) {
> +		kvm_s390_gisc_unregister(matrix_mdev->kvm, matrix_mdev->gisc);
> +		return -EIO;
> +	}
> +	return 0;
>   }
>   
>   static int ap_ioctl_clrirq(struct ap_matrix_mdev *matrix_mdev,
> @@ -946,6 +954,8 @@ static int ap_ioctl_clrirq(struct ap_matrix_mdev *matrix_mdev,
>   	parm->status = retval;
>   
>   	ap_status = reg2status(retval);
> +	/* unregister the IAM from the GIB anyway! */
> +	kvm_s390_gisc_unregister(matrix_mdev->kvm, matrix_mdev->gisc);

The case statement in patch 4 does not set mdev->gisc, so the
presumption here is that VFIO_AP_SET_IRQ has been previously called and
has set the value for matrix_mdev->gisc. Is it possible for
VFIO_AP_CLEAR_IRQ to get invoked without a prior call to
VFIO_AP_SET_IRQ? In any case, shouldn't the GISC value be taken from
bits 61-63 of 'parm'?

>   	return (ap_status.rc) ? -EIO : 0;
>   }
>   
> 

Re: [PATCH v1 6/7] vfio: ap: register guest ISC with GISA and GIB

[Pierre Morel]

On 06/11/2018 21:21, Tony Krowiak wrote:
> On 10/31/18 2:12 PM, Pierre Morel wrote:
>> Register to the GIB Alert list and retrieve the GAL_ISC
>> to pass to the GISA registration.
>>
>> Unregister on error and when clearing the interrupt.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> Signed-off-by: Michael Mueller <mimu@linux.ibm.com>
>> ---
>>   drivers/s390/crypto/vfio_ap_ops.c | 16 +++++++++++++---
>>   1 file changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/s390/crypto/vfio_ap_ops.c 
>> b/drivers/s390/crypto/vfio_ap_ops.c
>> index f68102163bf4..232168797fb8 100644
>> --- a/drivers/s390/crypto/vfio_ap_ops.c
>> +++ b/drivers/s390/crypto/vfio_ap_ops.c
>> @@ -903,16 +903,20 @@ static int ap_ioctl_setirq(struct ap_matrix_mdev 
>> *matrix_mdev,
>>       struct ap_status ap_status = reg2status(0);
>>       unsigned long p;
>>       int ret = -1;
>> -    int apqn;
>> +    int apqn, gal_isc;
>>       uint32_t gd;
>> +    gal_isc = kvm_s390_gisc_register(matrix_mdev->kvm, 
>> matrix_mdev->gisc);
>> +    if (gal_isc < 0)
>> +        return -EIO;
>> +
>>       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;
>> -    aqic_gisa.isc = GAL_ISC;
>> +    aqic_gisa.isc = gal_isc;
>>       aqic_gisa.ir = 1;
>>       aqic_gisa.gisao = gisa->next_alert >> 4;
>> @@ -923,7 +927,11 @@ static int ap_ioctl_setirq(struct ap_matrix_mdev 
>> *matrix_mdev,
>>       parm->status = ret;
>>       ap_status = reg2status(ret);
>> -    return (ap_status.rc) ? -EIO : 0;
>> +    if (ap_status.rc) {
>> +        kvm_s390_gisc_unregister(matrix_mdev->kvm, matrix_mdev->gisc);
>> +        return -EIO;
>> +    }
>> +    return 0;
>>   }
>>   static int ap_ioctl_clrirq(struct ap_matrix_mdev *matrix_mdev,
>> @@ -946,6 +954,8 @@ static int ap_ioctl_clrirq(struct ap_matrix_mdev 
>> *matrix_mdev,
>>       parm->status = retval;
>>       ap_status = reg2status(retval);
>> +    /* unregister the IAM from the GIB anyway! */
>> +    kvm_s390_gisc_unregister(matrix_mdev->kvm, matrix_mdev->gisc);
> 
> The case statement in patch 4 does not set mdev->gisc, so the
> presumption here is that VFIO_AP_SET_IRQ has been previously called and
> has set the value for matrix_mdev->gisc. Is it possible for
> VFIO_AP_CLEAR_IRQ to get invoked without a prior call to

right, I will check this.

However if the IRQ is not registered there is no problem as we will get 
an error when unregistering the IRQ
and unregistering the gisc 0 which was not registered will fail too.

But I think we better check before in case these functions change.

> VFIO_AP_SET_IRQ? In any case, shouldn't the GISC value be taken from
> bits 61-63 of 'parm'?

No this is not possible, the ISC field is not relevant when clearing 
interrupts.

> 
>>       return (ap_status.rc) ? -EIO : 0;
>>   }
>>
> 


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

[PATCH v1 7/7] s390: kvm: Handle all GISA IPM bits through GISA

[Pierre Morel]

Now that we use GISA and GIB we can handle all IPM bits from GISA
directly from firmware.
They will be interpreted on SIE entry or during guest run.

We remove them from the pending_irqs() test.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 arch/s390/kvm/interrupt.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index 6d0193173388..3174d9946523 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -248,8 +248,7 @@ static inline unsigned long pending_irqs_no_gisa(struct kvm_vcpu *vcpu)
 
 static inline unsigned long pending_irqs(struct kvm_vcpu *vcpu)
 {
-	return pending_irqs_no_gisa(vcpu) |
-		kvm_s390_gisa_get_ipm(vcpu->kvm->arch.gisa) << IRQ_PEND_IO_ISC_7;
+	return pending_irqs_no_gisa(vcpu);
 }
 
 static inline int isc_to_irq_type(unsigned long isc)
-- 
2.17.0

Re: [PATCH v1 7/7] s390: kvm: Handle all GISA IPM bits through GISA

[David Hildenbrand]

On 31.10.18 19:12, Pierre Morel wrote:
> Now that we use GISA and GIB we can handle all IPM bits from GISA
> directly from firmware.
> They will be interpreted on SIE entry or during guest run.
> 
> We remove them from the pending_irqs() test.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  arch/s390/kvm/interrupt.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
> index 6d0193173388..3174d9946523 100644
> --- a/arch/s390/kvm/interrupt.c
> +++ b/arch/s390/kvm/interrupt.c
> @@ -248,8 +248,7 @@ static inline unsigned long pending_irqs_no_gisa(struct kvm_vcpu *vcpu)
>  
>  static inline unsigned long pending_irqs(struct kvm_vcpu *vcpu)
>  {
> -	return pending_irqs_no_gisa(vcpu) |
> -		kvm_s390_gisa_get_ipm(vcpu->kvm->arch.gisa) << IRQ_PEND_IO_ISC_7;
> +	return pending_irqs_no_gisa(vcpu);
>  }
>  
>  static inline int isc_to_irq_type(unsigned long isc)
> 

(only looking at this very patch with no background information, so just
some notes)

1. e.g. kvm_s390_vcpu_has_irq() has to check all possible paths for
pending interrupts. It uses pending_irqs().

2. kvm_s390_deliver_pending_interrupts() delivers interrupts in the
order of priority. It could be that leaving it completely to the
hardware will result in some priority changes that could theoretically
be observed by the guest

(will have to study the GIB patches)

-- 

Thanks,

David / dhildenb