s390: vfio-ap: dynamic configuration support

Introduces a new driver callback to prevent a root user from unbinding an AP queue from its device driver if the queue is in use. This prevents a root user from inadvertently taking a queue away from a guest and giving it to the host, or vice versa. The callback will be invoked whenever a change to the AP bus's apmask or aqmask sysfs interfaces may result in one or more AP queues being removed from its driver. If the callback responds in the affirmative for any driver queried, the change to the apmask or aqmask will be rejected with a device in use error.

For this patch, only non-default drivers will be queried. Currently, there is only one non-default driver, the vfio_ap device driver. The vfio_ap device driver manages AP queues passed through to one or more guests and we don't want to unexpectedly take AP resources away from guests which are most likely independently administered.

Signed-off-by Tony Krowiak <[email protected]>

---
 drivers/s390/crypto/ap_bus.c | 138 +++++++++++++++++++++++++++++++++++++++++--
 drivers/s390/crypto/ap_bus.h |   3 +
 2 files changed, 135 insertions(+), 6 deletions(-)

drivers/s390/crypto/ap_bus.c +132 -6

index 1546389d71db..66a5a9d9fae6 100644
@@ -35,6 +35,7 @@
 #include <linux/mod_devicetable.h>
 #include <linux/debugfs.h>
 #include <linux/ctype.h>
+#include <linux/module.h>
 
 #include "ap_bus.h"
 #include "ap_debug.h"

@@ -980,9 +981,11 @@ int ap_parse_mask_str(const char *str,
 	newmap = kmalloc(size, GFP_KERNEL);
 	if (!newmap)
 		return -ENOMEM;
-	if (mutex_lock_interruptible(lock)) {
-		kfree(newmap);
-		return -ERESTARTSYS;
+	if (lock) {
+		if (mutex_lock_interruptible(lock)) {
+			kfree(newmap);
+			return -ERESTARTSYS;
+		}
 	}
 
 	if (*str == '+' || *str == '-') {

@@ -994,7 +997,10 @@ int ap_parse_mask_str(const char *str,
 	}
 	if (rc == 0)
 		memcpy(bitmap, newmap, size);
-	mutex_unlock(lock);
+
+	if (lock)
+		mutex_unlock(lock);
+
 	kfree(newmap);
 	return rc;
 }

@@ -1181,12 +1187,72 @@ static ssize_t apmask_show(struct bus_type *bus, char *buf)
 	return rc;
 }
 
+static int __verify_card_reservations(struct device_driver *drv, void *data)
+{
+	int rc = 0;
+	struct ap_driver *ap_drv = to_ap_drv(drv);
+	unsigned long *newapm = (unsigned long *)data;
+
+	/*
+	 * If the reserved bits do not identify cards reserved for use by the
+	 * non-default driver, there is no need to verify the driver is using
+	 * the queues.
+	 */
+	if (ap_drv->flags & AP_DRIVER_FLAG_DEFAULT)
+		return 0;
+
+	/* Pin the driver's module code */
+	if (!try_module_get(drv->owner))
+		return 0;
+
+	if (ap_drv->in_use)
+		if (ap_drv->in_use(newapm, ap_perms.aqm))
+			rc = -EADDRINUSE;
+
+	module_put(drv->owner);
+
+	return rc;
+}
+
+static int apmask_commit(unsigned long *newapm)
+{
+	int rc;
+	unsigned long reserved[BITS_TO_LONGS(AP_DEVICES)];
+
+	/*
+	 * Check if any bits in the apmask have been set which will
+	 * result in queues being removed from non-default drivers
+	 */
+	if (bitmap_andnot(reserved, newapm, ap_perms.apm, AP_DEVICES)) {
+		rc = (bus_for_each_drv(&ap_bus_type, NULL, reserved,
+				       __verify_card_reservations));
+		if (rc)
+			return rc;
+	}
+
+	memcpy(ap_perms.apm, newapm, APMASKSIZE);
+
+	return 0;
+}
+
 static ssize_t apmask_store(struct bus_type *bus, const char *buf,
 			    size_t count)
 {
 	int rc;
+	unsigned long newapm[BITS_TO_LONGS(AP_DEVICES)];
+
+	memcpy(newapm, ap_perms.apm, APMASKSIZE);
+
+	rc = ap_parse_mask_str(buf, newapm, AP_DEVICES, NULL);
+	if (rc)
+		return rc;
+
+	if (mutex_lock_interruptible(&ap_perms_mutex))
+		return -ERESTARTSYS;
+
+	rc = apmask_commit(newapm);
+	mutex_unlock(&ap_perms_mutex);
 
-	rc = ap_parse_mask_str(buf, ap_perms.apm, AP_DEVICES, &ap_perms_mutex);
 	if (rc)
 		return rc;
 

@@ -1212,12 +1278,72 @@ static ssize_t aqmask_show(struct bus_type *bus, char *buf)
 	return rc;
 }
 
+static int __verify_queue_reservations(struct device_driver *drv, void *data)
+{
+	int rc = 0;
+	struct ap_driver *ap_drv = to_ap_drv(drv);
+	unsigned long *newaqm = (unsigned long *)data;
+
+	/*
+	 * If the reserved bits do not identify queues reserved for use by the
+	 * non-default driver, there is no need to verify the driver is using
+	 * the queues.
+	 */
+	if (ap_drv->flags & AP_DRIVER_FLAG_DEFAULT)
+		return 0;
+
+	/* Pin the driver's module code */
+	if (!try_module_get(drv->owner))
+		return 0;
+
+	if (ap_drv->in_use)
+		if (ap_drv->in_use(ap_perms.apm, newaqm))
+			rc = -EADDRINUSE;
+
+	module_put(drv->owner);
+
+	return rc;
+}
+
+static int aqmask_commit(unsigned long *newaqm)
+{
+	int rc;
+	unsigned long reserved[BITS_TO_LONGS(AP_DOMAINS)];
+
+	/*
+	 * Check if any bits in the aqmask have been set which will
+	 * result in queues being removed from non-default drivers
+	 */
+	if (bitmap_andnot(reserved, newaqm, ap_perms.aqm, AP_DOMAINS)) {
+		rc = (bus_for_each_drv(&ap_bus_type, NULL, reserved,
+				       __verify_queue_reservations));
+		if (rc)
+			return rc;
+	}
+
+	memcpy(ap_perms.aqm, newaqm, APMASKSIZE);
+
+	return 0;
+}
+
 static ssize_t aqmask_store(struct bus_type *bus, const char *buf,
 			    size_t count)
 {
 	int rc;
+	unsigned long newaqm[BITS_TO_LONGS(AP_DOMAINS)];
+
+	memcpy(newaqm, ap_perms.aqm, AQMASKSIZE);
+
+	rc = ap_parse_mask_str(buf, newaqm, AP_DOMAINS, NULL);
+	if (rc)
+		return rc;
+
+	if (mutex_lock_interruptible(&ap_perms_mutex))
+		return -ERESTARTSYS;
+
+	rc = aqmask_commit(newaqm);
+	mutex_unlock(&ap_perms_mutex);
 
-	rc = ap_parse_mask_str(buf, ap_perms.aqm, AP_DOMAINS, &ap_perms_mutex);
 	if (rc)
 		return rc;
 

drivers/s390/crypto/ap_bus.h +3 -0

index 15a98a673c5c..b00952d5fdbb 100644
@@ -137,6 +137,7 @@ struct ap_driver {
 	void (*remove)(struct ap_device *);
 	void (*suspend)(struct ap_device *);
 	void (*resume)(struct ap_device *);
+	bool (*in_use)(unsigned long *apm, unsigned long *aqm);
 };
 
 #define to_ap_drv(x) container_of((x), struct ap_driver, driver)

@@ -262,6 +263,8 @@ void ap_queue_reinit_state(struct ap_queue *aq);
 struct ap_card *ap_card_create(int id, int queue_depth, int raw_device_type,
 			       int comp_device_type, unsigned int functions);
 
+#define APMASKSIZE (BITS_TO_LONGS(AP_DEVICES) * sizeof(unsigned long))
+#define AQMASKSIZE (BITS_TO_LONGS(AP_DOMAINS) * sizeof(unsigned long))
 struct ap_perms {
 	unsigned long ioctlm[BITS_TO_LONGS(AP_IOCTLS)];
 	unsigned long apm[BITS_TO_LONGS(AP_DEVICES)];

Quoted 15 lines · Tony Krowiak

Introduces a new driver callback to prevent a root user from unbinding an AP queue from its device driver if the queue is in use. This prevents a root user from inadvertently taking a queue away from a guest and giving it to the host, or vice versa. The callback will be invoked whenever a change to the AP bus's apmask or aqmask sysfs interfaces may result in one or more AP queues being removed from its driver. If the callback responds in the affirmative for any driver queried, the change to the apmask or aqmask will be rejected with a device in use error.

For this patch, only non-default drivers will be queried. Currently, there is only one non-default driver, the vfio_ap device driver. The vfio_ap device driver manages AP queues passed through to one or more guests and we don't want to unexpectedly take AP resources away from guests which are most likely independently administered.

Quoted 1 line

Signed-off-by Tony Krowiak <[email protected]>

Hello Tony

you are going out with your patches but ... what is the result of the discussions we had in the past ? Do we have a common understanding that we want to have it this way ? A driver which is able to claim resources and the bus code has lower precedence ?

Quoted 5 lines

---
 drivers/s390/crypto/ap_bus.c | 138 +++++++++++++++++++++++++++++++++++++++++--
 drivers/s390/crypto/ap_bus.h |   3 +
 2 files changed, 135 insertions(+), 6 deletions(-)

Quoted 16 lines · On Fri, 12 Apr 2019 08:54:54 +0200 Harald Freudenberger <[email protected]>…

Quoted 15 lines · Tony Krowiak

Introduces a new driver callback to prevent a root user from unbinding an AP queue from its device driver if the queue is in use. This prevents a root user from inadvertently taking a queue away from a guest and giving it to the host, or vice versa. The callback will be invoked whenever a change to the AP bus's apmask or aqmask sysfs interfaces may result in one or more AP queues being removed from its driver. If the callback responds in the affirmative for any driver queried, the change to the apmask or aqmask will be rejected with a device in use error.

For this patch, only non-default drivers will be queried. Currently, there is only one non-default driver, the vfio_ap device driver. The vfio_ap device driver manages AP queues passed through to one or more guests and we don't want to unexpectedly take AP resources away from guests which are most likely independently administered.

Quoted 8 lines

Quoted 1 line

Signed-off-by Tony Krowiak <[email protected]>

Hello Tony

you are going out with your patches but ... what is the result of the discussions we had in the past ? Do we have a common understanding that we want to have it this way ? A driver which is able to claim resources and the bus code has lower precedence ?

I don't know about previous discussions, but I'm curious how you arrived at this design. A driver being able to override the bus code seems odd. Restricting this to 'non-default' drivers looks even more odd.

What is this trying to solve? Traditionally, root has been able to shoot any appendages of their choice. I would rather expect that in a supported setup you'd have some management software keeping track of device usage and making sure that only unused queues can be unbound. Do we need to export more information to user space so that management software can make better choices?

Quoted 6 lines

Quoted 6 lines

---
  drivers/s390/crypto/ap_bus.c | 138 +++++++++++++++++++++++++++++++++++++++++--
  drivers/s390/crypto/ap_bus.h |   3 +
  2 files changed, 135 insertions(+), 6 deletions(-)

Quoted 19 lines · Cornelia Huck

Quoted 16 lines · On Fri, 12 Apr 2019 08:54:54 +0200 Harald Freudenberger <[email protected]>…

Quoted 15 lines · Tony Krowiak

Introduces a new driver callback to prevent a root user from unbinding an AP queue from its device driver if the queue is in use. This prevents a root user from inadvertently taking a queue away from a guest and giving it to the host, or vice versa. The callback will be invoked whenever a change to the AP bus's apmask or aqmask sysfs interfaces may result in one or more AP queues being removed from its driver. If the callback responds in the affirmative for any driver queried, the change to the apmask or aqmask will be rejected with a device in use error.

For this patch, only non-default drivers will be queried. Currently, there is only one non-default driver, the vfio_ap device driver. The vfio_ap device driver manages AP queues passed through to one or more guests and we don't want to unexpectedly take AP resources away from guests which are most likely independently administered.

Quoted 8 lines

Quoted 8 lines

Quoted 1 line

Signed-off-by Tony Krowiak <[email protected]>

Hello Tony

you are going out with your patches but ... what is the result of the discussions we had in the past ? Do we have a common understanding that we want to have it this way ? A driver which is able to claim resources and the bus code has lower precedence ?

This is what Reinhard suggested and what we agreed to as a team quite some time ago. I submitted three versions of this patch to our internal mailing list, all of which you reviewed, so I'm not sure why you are surprised by this now.

Quoted 6 lines

I don't know about previous discussions, but I'm curious how you arrived at this design. A driver being able to override the bus code seems odd. Restricting this to 'non-default' drivers looks even more odd.

Quoted 6 lines

What is this trying to solve? Traditionally, root has been able to shoot any appendages of their choice. I would rather expect that in a supported setup you'd have some management software keeping track of device usage and making sure that only unused queues can be unbound. Do we need to export more information to user space so that management software can make better choices?

Is there a reason other than tradition to prevent root from accidentally shooting himself in the foot or any other appendage? At present, sysfs is the only supported setup, so IMHO it makes sense to prevent as much accidentally caused damage as possible in the kernel.

Quoted 8 lines

Quoted 7 lines

Quoted 7 lines

  
 
---
  drivers/s390/crypto/ap_bus.c | 138 +++++++++++++++++++++++++++++++++++++++++--
  drivers/s390/crypto/ap_bus.h |   3 +
  2 files changed, 135 insertions(+), 6 deletions(-)  

Quoted 47 lines · On Fri, 12 Apr 2019 15:38:21 -0400 Tony Krowiak <[email protected]> wrote:

Quoted 19 lines · Cornelia Huck

Quoted 16 lines · On Fri, 12 Apr 2019 08:54:54 +0200 Harald Freudenberger <[email protected]>…

Quoted 15 lines · Tony Krowiak

Introduces a new driver callback to prevent a root user from unbinding an AP queue from its device driver if the queue is in use. This prevents a root user from inadvertently taking a queue away from a guest and giving it to the host, or vice versa. The callback will be invoked whenever a change to the AP bus's apmask or aqmask sysfs interfaces may result in one or more AP queues being removed from its driver. If the callback responds in the affirmative for any driver queried, the change to the apmask or aqmask will be rejected with a device in use error.

For this patch, only non-default drivers will be queried. Currently, there is only one non-default driver, the vfio_ap device driver. The vfio_ap device driver manages AP queues passed through to one or more guests and we don't want to unexpectedly take AP resources away from guests which are most likely independently administered.

Quoted 8 lines

Quoted 8 lines

Quoted 1 line

Signed-off-by Tony Krowiak <[email protected]>

Hello Tony

you are going out with your patches but ... what is the result of the discussions we had in the past ? Do we have a common understanding that we want to have it this way ? A driver which is able to claim resources and the bus code has lower precedence ?

This is what Reinhard suggested and what we agreed to as a team quite some time ago. I submitted three versions of this patch to our internal mailing list, all of which you reviewed, so I'm not sure why you are surprised by this now.

Quoted 6 lines

I don't know about previous discussions, but I'm curious how you arrived at this design. A driver being able to override the bus code seems odd. Restricting this to 'non-default' drivers looks even more odd.

Quoted 6 lines

What is this trying to solve? Traditionally, root has been able to shoot any appendages of their choice. I would rather expect that in a supported setup you'd have some management software keeping track of device usage and making sure that only unused queues can be unbound. Do we need to export more information to user space so that management software can make better choices?

Quoted 4 lines

Is there a reason other than tradition to prevent root from accidentally shooting himself in the foot or any other appendage? At present, sysfs is the only supported setup, so IMHO it makes sense to prevent as much accidentally caused damage as possible in the kernel.

I don't think anybody wants an interface where it is easy for root to accidentally cause damage... but from the patch description, it sounds like you're creating an interface which makes it easy for a badly-acting driver to hog resources without any way for root to remove them forcefully.

Therefore, again my question: What is this trying to solve? I see a management layer as a better place to make sure that queues are not accidentally yanked from guests that are using them. Does more information about queue usage need to be made available to userspace for this to be feasible? Is anything else missing?

Quoted 10 lines

Quoted 9 lines

Quoted 8 lines

Quoted 8 lines

   
  
---
   drivers/s390/crypto/ap_bus.c | 138 +++++++++++++++++++++++++++++++++++++++++--
   drivers/s390/crypto/ap_bus.h |   3 +
   2 files changed, 135 insertions(+), 6 deletions(-)

Quoted 61 lines · Cornelia Huck

Quoted 47 lines · On Fri, 12 Apr 2019 15:38:21 -0400 Tony Krowiak <[email protected]> wrote:

Quoted 19 lines · Cornelia Huck

Quoted 16 lines · On Fri, 12 Apr 2019 08:54:54 +0200 Harald Freudenberger <[email protected]>…

Quoted 15 lines · Tony Krowiak

Introduces a new driver callback to prevent a root user from unbinding an AP queue from its device driver if the queue is in use. This prevents a root user from inadvertently taking a queue away from a guest and giving it to the host, or vice versa. The callback will be invoked whenever a change to the AP bus's apmask or aqmask sysfs interfaces may result in one or more AP queues being removed from its driver. If the callback responds in the affirmative for any driver queried, the change to the apmask or aqmask will be rejected with a device in use error.

For this patch, only non-default drivers will be queried. Currently, there is only one non-default driver, the vfio_ap device driver. The vfio_ap device driver manages AP queues passed through to one or more guests and we don't want to unexpectedly take AP resources away from guests which are most likely independently administered.

Quoted 8 lines

Quoted 8 lines

Quoted 1 line

Signed-off-by Tony Krowiak <[email protected]>

Hello Tony

you are going out with your patches but ... what is the result of the discussions we had in the past ? Do we have a common understanding that we want to have it this way ? A driver which is able to claim resources and the bus code has lower precedence ?

This is what Reinhard suggested and what we agreed to as a team quite some time ago. I submitted three versions of this patch to our internal mailing list, all of which you reviewed, so I'm not sure why you are surprised by this now.

Quoted 6 lines

I don't know about previous discussions, but I'm curious how you arrived at this design. A driver being able to override the bus code seems odd. Restricting this to 'non-default' drivers looks even more odd.

Quoted 6 lines

What is this trying to solve? Traditionally, root has been able to shoot any appendages of their choice. I would rather expect that in a supported setup you'd have some management software keeping track of device usage and making sure that only unused queues can be unbound. Do we need to export more information to user space so that management software can make better choices?

Quoted 4 lines

Is there a reason other than tradition to prevent root from accidentally shooting himself in the foot or any other appendage? At present, sysfs is the only supported setup, so IMHO it makes sense to prevent as much accidentally caused damage as possible in the kernel.

I don't think anybody wants an interface where it is easy for root to accidentally cause damage... but from the patch description, it sounds like you're creating an interface which makes it easy for a badly-acting driver to hog resources without any way for root to remove them forcefully.

Quoted 5 lines

Therefore, again my question: What is this trying to solve? I see a management layer as a better place to make sure that queues are not accidentally yanked from guests that are using them. Does more information about queue usage need to be made available to userspace for this to be feasible? Is anything else missing?

Accidentally yanking queues from guests is only part of the equation. One has to consider the case where queues go away without root user intervention. Take, for example, the case where an AP card temporarily goes offline for some reason; possibly, due to glich or temporary hardware malfunction of some sort. When the AP bus scan subsequently runs, the card device and all associated queue devices will be unbound from their respective device drivers. If the card then comes back online, the next bus scan will recreate the card and queue devices and bind them to their respective device drivers. These patches ensure the queues are given back to the guest from which they were taken when unbound.

Having said that, I understand your concern about a driver hogging resources. I think I can provide a solution that serves both the purpose of preventing problems associated with accidental removal of AP resources as well as allowing root to remove them forcefully. I'll work on that for v2.

Quoted 12 lines

Quoted 11 lines

Quoted 10 lines

Quoted 9 lines

Quoted 9 lines

  
 
     
    
---
   drivers/s390/crypto/ap_bus.c | 138 +++++++++++++++++++++++++++++++++++++++++--
   drivers/s390/crypto/ap_bus.h |   3 +
   2 files changed, 135 insertions(+), 6 deletions(-)  

Having said that, I understand your concern about a driver hogging resources. I think I can provide a solution that serves both the purpose of preventing problems associated with accidental removal of AP resources as well as allowing root to remove them forcefully. I'll work on that for v2.

Quoted 5 lines · On Mon, 15 Apr 2019 12:51:23 -0400 Tony Krowiak <[email protected]> wrote:

Having said that, I understand your concern about a driver hogging resources. I think I can provide a solution that serves both the purpose of preventing problems associated with accidental removal of AP resources as well as allowing root to remove them forcefully. I'll work on that for v2.

Can you tell us some more about this solution? Should we stop reviewing v1 because v2 is going to be different anyway?

Regards,
Halil

Quoted 11 lines · Halil Pasic

Quoted 5 lines · On Mon, 15 Apr 2019 12:51:23 -0400 Tony Krowiak <[email protected]> wrote:

Having said that, I understand your concern about a driver hogging resources. I think I can provide a solution that serves both the purpose of preventing problems associated with accidental removal of AP resources as well as allowing root to remove them forcefully. I'll work on that for v2.

Can you tell us some more about this solution? Should we stop reviewing v1 because v2 is going to be different anyway?

Patch 1 and 2 will be removed. There will not be a major design change between these patches and v2. In order to avoid a long explanation of my proposed changes, I'd prefer to state that the patch set will establish and enforce the following rules:

1. An APQN can be assigned to an mdev device iff it is NOT reserved for use by a zcrypt driver and is not assigned to another mdev device.

2. Once an APQN is assigned to an mdev device, it will remain assigned until it is explicitly unassigned.

3. A queue's APQN can be set in the guest's CRYCB iff the APQN is assigned to the mdev device used by the guest; however, if the queue is also in the host configuration (i.e., online), it MUST also be bound to the vfio_ap device driver.

4. When a queue is bound to the vfio_ap driver and its APQN is assigned to an mdev device in use by a guest, the guest will be given access to the queue.

5. When a queue is unbound from the vfio_ap driver and its APQN is assigned to an mdev device in use by the guest, access to the card containing the queue will be removed from the guest. Keep in mind that we can not deny access to a specific queue due to the architecture (i.e., clearing a bit in the AQM removes access to the queue for all adapters)

6. When an adapter is assigned to an mdev device that is in use by a guest, the guest will be given access to the adapter.

7. When an adapter is unassigned from an mdev device that is in use by a guest, access to the adapter will removed from the guest.

8. When a domain is assigned to an mdev device that is in use by a guest, the guest will be given access to the domain.

9. When a domain is unassigned from an mdev device that is in use by a guest, access to the domain will removed from the guest.

Introduces a new driver callback to prevent a root user from unbinding an AP queue from its device driver if the queue is in use. This prevents a root user from inadvertently taking a queue away from a guest and giving it to the host, or vice versa. The callback will be invoked whenever a change to the AP bus's apmask or aqmask sysfs interfaces may result in one or more AP queues being removed from its driver. If the callback responds in the affirmative for any driver queried, the change to the apmask or aqmask will be rejected with a device in use error.

For this patch, only non-default drivers will be queried. Currently, there is only one non-default driver, the vfio_ap device driver. The vfio_ap device driver manages AP queues passed through to one or more guests and we don't want to unexpectedly take AP resources away from guests which are most likely independently administered.

Signed-off-by Tony Krowiak <[email protected]>

---
  drivers/s390/crypto/ap_bus.c | 138 +++++++++++++++++++++++++++++++++++++++++--
  drivers/s390/crypto/ap_bus.h |   3 +
  2 files changed, 135 insertions(+), 6 deletions(-)

drivers/s390/crypto/ap_bus.c +13 -4

index 1546389d71db..66a5a9d9fae6 100644
@@ -35,6 +35,7 @@
  #include <linux/mod_devicetable.h>
  #include <linux/debugfs.h>
  #include <linux/ctype.h>
+#include <linux/module.h>
 
  #include "ap_bus.h"
  #include "ap_debug.h"

@@ -980,9 +981,11 @@ int ap_parse_mask_str(const char *str,
  	newmap = kmalloc(size, GFP_KERNEL);
  	if (!newmap)
  		return -ENOMEM;
-	if (mutex_lock_interruptible(lock)) {
-		kfree(newmap);
-		return -ERESTARTSYS;
+	if (lock) {
+		if (mutex_lock_interruptible(lock)) {
+			kfree(newmap);
+			return -ERESTARTSYS;
+		}
  	}
 
  	if (*str == '+' || *str == '-') {

@@ -994,7 +997,10 @@ int ap_parse_mask_str(const char *str,
  	}
  	if (rc == 0)
  		memcpy(bitmap, newmap, size);
-	mutex_unlock(lock);
+
+	if (lock)
+		mutex_unlock(lock);
+
  	kfree(newmap);
  	return rc;
  }

@@ -1181,12 +1187,72 @@ static ssize_t apmask_show(struct bus_type *bus, char *buf)
  	return rc;
  }
 
+static int __verify_card_reservations(struct device_driver *drv, void *data)
+{
+	int rc = 0;

drivers/s390/crypto/ap_bus.c +14 -0

+
+	/* Pin the driver's module code */
+	if (!try_module_get(drv->owner))
+		return 0;
+
+	if (ap_drv->in_use)
+		if (ap_drv->in_use(newapm, ap_perms.aqm))
+			rc = -EADDRINUSE;
+
+	module_put(drv->owner);
+
+	return rc;
+}
+

Quoted 73 lines · Tony Krowiak

Introduces a new driver callback to prevent a root user from unbinding an AP queue from its device driver if the queue is in use. This prevents a root user from inadvertently taking a queue away from a guest and giving it to the host, or vice versa. The callback will be invoked whenever a change to the AP bus's apmask or aqmask sysfs interfaces may result in one or more AP queues being removed from its driver. If the callback responds in the affirmative for any driver queried, the change to the apmask or aqmask will be rejected with a device in use error.

For this patch, only non-default drivers will be queried. Currently, there is only one non-default driver, the vfio_ap device driver. The vfio_ap device driver manages AP queues passed through to one or more guests and we don't want to unexpectedly take AP resources away from guests which are most likely independently administered.

Signed-off-by Tony Krowiak <[email protected]>

---
  drivers/s390/crypto/ap_bus.c | 138
+++++++++++++++++++++++++++++++++++++++++--
  drivers/s390/crypto/ap_bus.h |   3 +

  2 files changed, 135 insertions(+), 6 deletions(-)

drivers/s390/crypto/ap_bus.c +18 -4

index 1546389d71db..66a5a9d9fae6 100644
@@ -35,6 +35,7 @@
   #include <linux/mod_devicetable.h>
   #include <linux/debugfs.h>
   #include <linux/ctype.h>
+#include <linux/module.h>
   #include "ap_bus.h"
   #include "ap_debug.h"

@@ -980,9 +981,11 @@ int ap_parse_mask_str(const char *str,
       newmap = kmalloc(size, GFP_KERNEL);
       if (!newmap)
           return -ENOMEM;
-    if (mutex_lock_interruptible(lock)) {
-        kfree(newmap);
-        return -ERESTARTSYS;
+    if (lock) {
+        if (mutex_lock_interruptible(lock)) {
+            kfree(newmap);
+            return -ERESTARTSYS;
+        }
       }
       if (*str == '+' || *str == '-') {

@@ -994,7 +997,10 @@ int ap_parse_mask_str(const char *str,
       }
       if (rc == 0)
           memcpy(bitmap, newmap, size);
-    mutex_unlock(lock);
+
+    if (lock)
+        mutex_unlock(lock);
+
       kfree(newmap);
       return rc;
   }

@@ -1181,12 +1187,72 @@ static ssize_t apmask_show(struct bus_type 
 *bus, char *buf)
       return rc;
   }
+static int __verify_card_reservations(struct device_driver *drv, void 
 *data)
+{
+    int rc = 0;
+    struct ap_driver *ap_drv = to_ap_drv(drv);
+    unsigned long *newapm = (unsigned long *)data;
+
+    /*
+     * If the reserved bits do not identify cards reserved for use by 

the

Quoted 6 lines

+     * non-default driver, there is no need to verify the driver is using
+     * the queues.
+     */
+    if (ap_drv->flags & AP_DRIVER_FLAG_DEFAULT)
+        return 0;

I prefer you suppress this asymmetry with the assumption that the "default driver" will not register a "in_use" callback.

Quoted 16 lines

drivers/s390/crypto/ap_bus.c +14 -0

+
+    /* Pin the driver's module code */
+    if (!try_module_get(drv->owner))
+        return 0;
+
+    if (ap_drv->in_use)
+        if (ap_drv->in_use(newapm, ap_perms.aqm))
+            rc = -EADDRINUSE;
+
+    module_put(drv->owner);
+
+    return rc;
+}
+

Quoted 74 lines · Pierre Morel

Quoted 73 lines · Tony Krowiak

Introduces a new driver callback to prevent a root user from unbinding an AP queue from its device driver if the queue is in use. This prevents a root user from inadvertently taking a queue away from a guest and giving it to the host, or vice versa. The callback will be invoked whenever a change to the AP bus's apmask or aqmask sysfs interfaces may result in one or more AP queues being removed from its driver. If the callback responds in the affirmative for any driver queried, the change to the apmask or aqmask will be rejected with a device in use error.

For this patch, only non-default drivers will be queried. Currently, there is only one non-default driver, the vfio_ap device driver. The vfio_ap device driver manages AP queues passed through to one or more guests and we don't want to unexpectedly take AP resources away from guests which are most likely independently administered.

Signed-off-by Tony Krowiak <[email protected]>

---
  drivers/s390/crypto/ap_bus.c | 138
+++++++++++++++++++++++++++++++++++++++++--
  drivers/s390/crypto/ap_bus.h |   3 +

  2 files changed, 135 insertions(+), 6 deletions(-)

drivers/s390/crypto/ap_bus.c +18 -4

index 1546389d71db..66a5a9d9fae6 100644
@@ -35,6 +35,7 @@
   #include <linux/mod_devicetable.h>
   #include <linux/debugfs.h>
   #include <linux/ctype.h>
+#include <linux/module.h>
   #include "ap_bus.h"
   #include "ap_debug.h"

@@ -980,9 +981,11 @@ int ap_parse_mask_str(const char *str,
       newmap = kmalloc(size, GFP_KERNEL);
       if (!newmap)
           return -ENOMEM;
-    if (mutex_lock_interruptible(lock)) {
-        kfree(newmap);
-        return -ERESTARTSYS;
+    if (lock) {
+        if (mutex_lock_interruptible(lock)) {
+            kfree(newmap);
+            return -ERESTARTSYS;
+        }
       }
       if (*str == '+' || *str == '-') {

@@ -994,7 +997,10 @@ int ap_parse_mask_str(const char *str,
       }
       if (rc == 0)
           memcpy(bitmap, newmap, size);
-    mutex_unlock(lock);
+
+    if (lock)
+        mutex_unlock(lock);
+
       kfree(newmap);
       return rc;
   }

@@ -1181,12 +1187,72 @@ static ssize_t apmask_show(struct bus_type 
 *bus, char *buf)
       return rc;
   }
+static int __verify_card_reservations(struct device_driver *drv, 
 void *data)
+{
+    int rc = 0;
+    struct ap_driver *ap_drv = to_ap_drv(drv);
+    unsigned long *newapm = (unsigned long *)data;
+
+    /*
+     * If the reserved bits do not identify cards reserved for use 

by the

Quoted 8 lines

Quoted 6 lines

+     * non-default driver, there is no need to verify the driver is using
+     * the queues.
+     */
+    if (ap_drv->flags & AP_DRIVER_FLAG_DEFAULT)
+        return 0;

I prefer you suppress this asymmetry with the assumption that the "default driver" will not register a "in_use" callback.

Based on comments by Connie, I plan on removing this patch from the next version.

Quoted 18 lines

Quoted 17 lines

drivers/s390/crypto/ap_bus.c +14 -0

+
+    /* Pin the driver's module code */
+    if (!try_module_get(drv->owner))
+        return 0;
+
+    if (ap_drv->in_use)
+        if (ap_drv->in_use(newapm, ap_perms.aqm))
+            rc = -EADDRINUSE;
+
+    module_put(drv->owner);
+
+    return rc;
+}
+

Once an APQN is assigned to an mdev device it will remained assigned until it is explicitly unassigned from the mdev device. The associated AP queue devices, however, can come and go due to failures or deliberate actions by a sysadmin. For example, a sysadmin can dynamically remove an AP adapter card using the SE or by executing an SCLP command. This patch refactors the probe and remove callbacks of the vfio_ap driver to handle dynamic changes as follows:

* Probe callback changes:

If the APQN of the queue being probed is assigned to an mdev device, the mdev device is in use by a guest, and the APQN is not set in the guest's CRYCB, the CRYCB will be dynamically updated to give the guest access to the queue.

* Remove callback changes:

If the APQN of the queue being removed is assigned to an mdev device, the mdev
device is in use by a guest, and the APQN is set in the guest's CRYCB, the CRYCB will be dynamically updated to remove the guest's access to the adapter card associated with the queue. Keep in mind, the architecture does not provide a way to remove access to a single queue unless only one queue is in the guest's configuration, so it was decided that it makes more sense to unplug the adapter from the guest.

The APQN of the queue being removed will remain assigned to the mdev device should the queue be dynamically returned to the configuration. The queue will also be reset prior to returning control to the caller (a.k.a., the AP bus).

Signed-off-by Tony Krowiak <[email protected]>

---
 arch/s390/include/asm/kvm_host.h      |  2 ++
 arch/s390/kvm/kvm-s390.c              | 37 +++++++++++++++++++
 drivers/s390/crypto/vfio_ap_drv.c     | 16 +++++++--
 drivers/s390/crypto/vfio_ap_ops.c     | 67 +++++++++++++++++++++++++++++++++--
 drivers/s390/crypto/vfio_ap_private.h |  2 ++
 5 files changed, 120 insertions(+), 4 deletions(-)

arch/s390/include/asm/kvm_host.h +2 -0

index c47e22bba87f..0ce5d9b0df59 100644
@@ -895,6 +895,8 @@ void kvm_arch_async_page_present(struct kvm_vcpu *vcpu,
 void kvm_arch_crypto_clear_masks(struct kvm *kvm);
 void kvm_arch_crypto_set_masks(struct kvm *kvm, unsigned long *apm,
 			       unsigned long *aqm, unsigned long *adm);
+int kvm_arch_crypto_test_masks(struct kvm *kvm, unsigned long *apm,
+			       unsigned long *aqm, unsigned long *adm);
 
 extern int sie64a(struct kvm_s390_sie_block *, u64 *);
 extern char sie_exit;

arch/s390/kvm/kvm-s390.c +37 -0

index 4638303ba6a8..5f423cdd29ba 100644
@@ -2217,6 +2217,43 @@ static void kvm_s390_set_crycb_format(struct kvm *kvm)
 		kvm->arch.crypto.crycbd |= CRYCB_FORMAT1;
 }
 
+int kvm_arch_crypto_test_masks(struct kvm *kvm, unsigned long *apm,
+			       unsigned long *aqm, unsigned long *adm)
+{
+	int ret;
+	struct kvm_s390_crypto_cb *crycb = kvm->arch.crypto.crycb;
+
+	switch (kvm->arch.crypto.crycbd & CRYCB_FORMAT_MASK) {
+	case CRYCB_FORMAT2: /* APCB1 use 256 bits */
+		ret = bitmap_equal(apm, (unsigned long *)crycb->apcb1.apm, 256);
+		VM_EVENT(kvm, 3, "TEST CRYCB: apm %016lx %016lx %016lx %016lx",
+			 apm[0], apm[1], apm[2], apm[3]);
+		ret &= bitmap_equal(aqm,
+				    (unsigned long *)crycb->apcb1.aqm, 256);
+		VM_EVENT(kvm, 3, "TEST CRYCB: aqm %016lx %016lx %016lx %016lx",
+			 aqm[0], aqm[1], aqm[2], aqm[3]);
+		ret &= bitmap_equal(adm,
+				    (unsigned long *)crycb->apcb1.adm, 256);
+		VM_EVENT(kvm, 3, "TEST CRYCB: adm %016lx %016lx %016lx %016lx",
+			 adm[0], adm[1], adm[2], adm[3]);
+		break;
+	case CRYCB_FORMAT1:
+	case CRYCB_FORMAT0: /* Fall through both use APCB0 */
+		ret = bitmap_equal(apm, (unsigned long *)crycb->apcb1.apm, 64);
+		ret &= bitmap_equal(aqm, (unsigned long *)crycb->apcb1.aqm, 16);
+		ret &= bitmap_equal(adm, (unsigned long *)crycb->apcb1.adm, 16);
+		VM_EVENT(kvm, 3, "TEST CRYCB: apm %016lx aqm %04x adm %04x",
+			 apm[0], *((unsigned short *)aqm),
+			 *((unsigned short *)adm));
+		break;
+	default:	/* Can not happen */
+		ret = 0;
+		break;
+	}
+	return ret;
+}
+EXPORT_SYMBOL_GPL(kvm_arch_crypto_test_masks);
+
 void kvm_arch_crypto_set_masks(struct kvm *kvm, unsigned long *apm,
 			       unsigned long *aqm, unsigned long *adm)
 {

drivers/s390/crypto/vfio_ap_drv.c +14 -2

index f340a28c1d65..2a79d27d9730 100644
@@ -42,12 +42,24 @@ MODULE_DEVICE_TABLE(vfio_ap, ap_queue_ids);
 
 static int vfio_ap_queue_dev_probe(struct ap_device *apdev)
 {
-	return 0;
+	struct ap_queue *apq = to_ap_queue(&apdev->device);
+	unsigned long apid = AP_QID_CARD(apq->qid);
+	unsigned long apqi = AP_QID_QUEUE(apq->qid);
+
+	mutex_lock(&matrix_dev->lock);
+	vfio_ap_mdev_probe_queue(apid, apqi);
+	mutex_unlock(&matrix_dev->lock);
 }
 
 static void vfio_ap_queue_dev_remove(struct ap_device *apdev)
 {
-	/* Nothing to do yet */
+	struct ap_queue *apq = to_ap_queue(&apdev->device);
+	unsigned long apid = AP_QID_CARD(apq->qid);
+	unsigned long apqi = AP_QID_QUEUE(apq->qid);
+
+	mutex_lock(&matrix_dev->lock);
+	vfio_ap_mdev_remove_queue(apid, apqi);
+	mutex_unlock(&matrix_dev->lock);
 }
 
 static void vfio_ap_matrix_dev_release(struct device *dev)

drivers/s390/crypto/vfio_ap_ops.c +65 -2

index ade2c150fe6b..8a70707bf870 100644
@@ -683,8 +683,8 @@ static void vfio_ap_mdev_wait_for_qempty(unsigned long apid, unsigned long apqi)
 			msleep(20);
 			break;
 		default:
-			pr_warn("%s: tapq err %02x: 0x%04x may not be empty\n",
-				__func__, status.response_code, q->apqn);
+			pr_warn("%s: tapq err %02x: %02lx%04lx may not be empty\n",
+				__func__, status.response_code, apid, apqi);
 			return;
 		}
 	} while (--retry);

@@ -840,3 +840,66 @@ void vfio_ap_mdev_unregister(void)
 {
 	mdev_unregister_device(&matrix_dev->device);
 }
+
+static struct ap_matrix_mdev *vfio_ap_mdev_find_matrix_mdev(unsigned long apid,
+							    unsigned long apqi)
+{
+	struct ap_matrix_mdev *matrix_mdev;
+
+	list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
+		if (test_bit_inv(apid, matrix_mdev->matrix.apm) &&
+		    test_bit_inv(apqi, matrix_mdev->matrix.aqm))
+			return matrix_mdev;
+	}
+
+	return NULL;
+}
+
+void vfio_ap_mdev_remove_queue(unsigned long apid, unsigned long apqi)
+{
+	struct ap_matrix_mdev *matrix_mdev;
+
+	matrix_mdev = vfio_ap_mdev_find_matrix_mdev(apid, apqi);
+
+	/*
+	 * If the queue is assigned to the mdev device and the mdev device
+	 * is in use by a guest
+	 */
+	if (matrix_mdev && matrix_mdev->kvm) {
+		/*
+		 * If the queue is plugged into the guest, unplug the adapter
+		 * but keep the adapter assigned to the mdev device
+		 */
+		if (!kvm_arch_crypto_test_masks(matrix_mdev->kvm,
+						matrix_mdev->matrix.apm,
+						matrix_mdev->matrix.aqm,
+						matrix_mdev->matrix.adm)) {
+			clear_bit_inv(apid, matrix_mdev->matrix.apm);
+			vfio_ap_mdev_update_crycb(matrix_mdev);
+			set_bit_inv(apid, matrix_mdev->matrix.apm);
+		}
+	}
+
+	vfio_ap_mdev_reset_queue(apid, apqi);
+}
+
+void vfio_ap_mdev_probe_queue(unsigned long apid, unsigned long apqi)
+{
+	struct ap_matrix_mdev *matrix_mdev;
+
+	matrix_mdev = vfio_ap_mdev_find_matrix_mdev(apid, apqi);
+
+	/*
+	 * If the queue is assigned to the mdev device and the mdev device
+	 * is in use by a guest
+	 */
+	if (matrix_mdev && matrix_mdev->kvm) {
+		/* If the queue is not plugged into the guest, plug it in */
+		if (!kvm_arch_crypto_test_masks(matrix_mdev->kvm,
+						matrix_mdev->matrix.apm,
+						matrix_mdev->matrix.aqm,
+						matrix_mdev->matrix.adm)) {
+			vfio_ap_mdev_update_crycb(matrix_mdev);
+		}
+	}
+}

drivers/s390/crypto/vfio_ap_private.h +2 -0

index 76b7f98e47e9..acdd5bfabaaf 100644
@@ -85,5 +85,7 @@ struct ap_matrix_mdev {
 
 extern int vfio_ap_mdev_register(void);
 extern void vfio_ap_mdev_unregister(void);
+void vfio_ap_mdev_remove_queue(unsigned long apid, unsigned long apqi);
+void vfio_ap_mdev_probe_queue(unsigned long apid, unsigned long apqi);
 
 #endif /* _VFIO_AP_PRIVATE_H_ */

Once an APQN is assigned to an mdev device it will remained assigned until it is explicitly unassigned from the mdev device. The associated AP queue devices, however, can come and go due to failures or deliberate actions by a sysadmin. For example, a sysadmin can dynamically remove an AP adapter card using the SE or by executing an SCLP command. This patch refactors the probe and remove callbacks of the vfio_ap driver to handle dynamic changes as follows:

* Probe callback changes:

If the APQN of the queue being probed is assigned to an mdev device, the mdev device is in use by a guest, and the APQN is not set in the guest's CRYCB, the CRYCB will be dynamically updated to give the guest access to the queue.

* Remove callback changes:

If the APQN of the queue being removed is assigned to an mdev device, the mdev
device is in use by a guest, and the APQN is set in the guest's CRYCB, the CRYCB will be dynamically updated to remove the guest's access to the adapter card associated with the queue. Keep in mind, the architecture does not provide a way to remove access to a single queue unless only one queue is in the guest's configuration, so it was decided that it makes more sense to unplug the adapter from the guest.

The APQN of the queue being removed will remain assigned to the mdev device should the queue be dynamically returned to the configuration. The queue will also be reset prior to returning control to the caller (a.k.a., the AP bus).

Signed-off-by Tony Krowiak <[email protected]>

---
  arch/s390/include/asm/kvm_host.h      |  2 ++
  arch/s390/kvm/kvm-s390.c              | 37 +++++++++++++++++++
  drivers/s390/crypto/vfio_ap_drv.c     | 16 +++++++--
  drivers/s390/crypto/vfio_ap_ops.c     | 67 +++++++++++++++++++++++++++++++++--
  drivers/s390/crypto/vfio_ap_private.h |  2 ++
  5 files changed, 120 insertions(+), 4 deletions(-)

arch/s390/include/asm/kvm_host.h +2 -0

index c47e22bba87f..0ce5d9b0df59 100644kvm_s390_crypto_cb
@@ -895,6 +895,8 @@ void kvm_arch_async_page_present(struct kvm_vcpu *vcpu,
  void kvm_arch_crypto_clear_masks(struct kvm *kvm);
  void kvm_arch_crypto_set_masks(struct kvm *kvm, unsigned long *apm,
  			       unsigned long *aqm, unsigned long *adm);
+int kvm_arch_crypto_test_masks(struct kvm *kvm, unsigned long *apm,
+			       unsigned long *aqm, unsigned long *adm);
 
  extern int sie64a(struct kvm_s390_sie_block *, u64 *);
  extern char sie_exit;

arch/s390/kvm/kvm-s390.c +0 -0

index 4638303ba6a8..5f423cdd29ba 100644

arch/s390/kvm/kvm-s390.c +26 -0

+int kvm_arch_crypto_test_masks(struct kvm *kvm, unsigned long *apm,
+			       unsigned long *aqm, unsigned long *adm)
+{
+	int ret;
+	struct kvm_s390_crypto_cb *crycb = kvm->arch.crypto.crycb;
+
+	switch (kvm->arch.crypto.crycbd & CRYCB_FORMAT_MASK) {
+	case CRYCB_FORMAT2: /* APCB1 use 256 bits */
+		ret = bitmap_equal(apm, (unsigned long *)crycb->apcb1.apm, 256);
+		VM_EVENT(kvm, 3, "TEST CRYCB: apm %016lx %016lx %016lx %016lx",
+			 apm[0], apm[1], apm[2], apm[3]);
+		ret &= bitmap_equal(aqm,
+				    (unsigned long *)crycb->apcb1.aqm, 256);
+		VM_EVENT(kvm, 3, "TEST CRYCB: aqm %016lx %016lx %016lx %016lx",
+			 aqm[0], aqm[1], aqm[2], aqm[3]);
+		ret &= bitmap_equal(adm,
+				    (unsigned long *)crycb->apcb1.adm, 256);
+		VM_EVENT(kvm, 3, "TEST CRYCB: adm %016lx %016lx %016lx %016lx",
+			 adm[0], adm[1], adm[2], adm[3]);
+		break;
+	case CRYCB_FORMAT1:
+	case CRYCB_FORMAT0: /* Fall through both use APCB0 */
+		ret = bitmap_equal(apm, (unsigned long *)crycb->apcb1.apm, 64);
+		ret &= bitmap_equal(aqm, (unsigned long *)crycb->apcb1.aqm, 16);
+		ret &= bitmap_equal(adm, (unsigned long *)crycb->apcb1.adm, 16);
+		VM_EVENT(kvm, 3, "TEST CRYCB: apm %016lx aqm %04x adm %04x",

Once an APQN is assigned to an mdev device it will remained assigned until it is explicitly unassigned from the mdev device. The associated AP queue devices, however, can come and go due to failures or deliberate actions by a sysadmin. For example, a sysadmin can dynamically remove an AP adapter card using the SE or by executing an SCLP command. This patch refactors the probe and remove callbacks of the vfio_ap driver to handle dynamic changes as follows:

* Probe callback changes:

If the APQN of the queue being probed is assigned to an mdev device, the mdev device is in use by a guest, and the APQN is not set in the guest's CRYCB, the CRYCB will be dynamically updated to give the guest access to the queue.

* Remove callback changes:

If the APQN of the queue being removed is assigned to an mdev device, the mdev
device is in use by a guest, and the APQN is set in the guest's CRYCB, the CRYCB will be dynamically updated to remove the guest's access to the adapter card associated with the queue. Keep in mind, the architecture does not provide a way to remove access to a single queue unless only one queue is in the guest's configuration, so it was decided that it makes more sense to unplug the adapter from the guest.

The APQN of the queue being removed will remain assigned to the mdev device should the queue be dynamically returned to the configuration. The queue will also be reset prior to returning control to the caller (a.k.a., the AP bus).

Signed-off-by Tony Krowiak <[email protected]>

---
  arch/s390/include/asm/kvm_host.h      |  2 ++
  arch/s390/kvm/kvm-s390.c              | 37 +++++++++++++++++++
  drivers/s390/crypto/vfio_ap_drv.c     | 16 +++++++--
  drivers/s390/crypto/vfio_ap_ops.c     | 67 +++++++++++++++++++++++++++++++++--
  drivers/s390/crypto/vfio_ap_private.h |  2 ++
  5 files changed, 120 insertions(+), 4 deletions(-)

arch/s390/include/asm/kvm_host.h +2 -0

index c47e22bba87f..0ce5d9b0df59 100644
@@ -895,6 +895,8 @@ void kvm_arch_async_page_present(struct kvm_vcpu *vcpu,
  void kvm_arch_crypto_clear_masks(struct kvm *kvm);
  void kvm_arch_crypto_set_masks(struct kvm *kvm, unsigned long *apm,
  			       unsigned long *aqm, unsigned long *adm);
+int kvm_arch_crypto_test_masks(struct kvm *kvm, unsigned long *apm,
+			       unsigned long *aqm, unsigned long *adm);
 
  extern int sie64a(struct kvm_s390_sie_block *, u64 *);
  extern char sie_exit;

arch/s390/kvm/kvm-s390.c +15 -0

index 4638303ba6a8..5f423cdd29ba 100644
@@ -2217,6 +2217,43 @@ static void kvm_s390_set_crycb_format(struct kvm *kvm)
  		kvm->arch.crypto.crycbd |= CRYCB_FORMAT1;
  }
 
+int kvm_arch_crypto_test_masks(struct kvm *kvm, unsigned long *apm,
+			       unsigned long *aqm, unsigned long *adm)
+{
+	int ret;
+	struct kvm_s390_crypto_cb *crycb = kvm->arch.crypto.crycb;
+
+	switch (kvm->arch.crypto.crycbd & CRYCB_FORMAT_MASK) {
+	case CRYCB_FORMAT2: /* APCB1 use 256 bits */
+		ret = bitmap_equal(apm, (unsigned long *)crycb->apcb1.apm, 256);
+		VM_EVENT(kvm, 3, "TEST CRYCB: apm %016lx %016lx %016lx %016lx",
+			 apm[0], apm[1], apm[2], apm[3]);
+		ret &= bitmap_equal(aqm,
+				    (unsigned long *)crycb->apcb1.aqm, 256);
+		VM_EVENT(kvm, 3, "TEST CRYCB: aqm %016lx %016lx %016lx %016lx",
+			 aqm[0], aqm[1], aqm[2], aqm[3]);

arch/s390/kvm/kvm-s390.c +12 -0

+		VM_EVENT(kvm, 3, "TEST CRYCB: apm %016lx aqm %04x adm %04x",
+			 apm[0], *((unsigned short *)aqm),
+			 *((unsigned short *)adm));
+		break;
+	default:	/* Can not happen */
+		ret = 0;
+		break;
+	}
+	return ret;
+}
+EXPORT_SYMBOL_GPL(kvm_arch_crypto_test_masks);
+
  void kvm_arch_crypto_set_masks(struct kvm *kvm, unsigned long *apm,
  			       unsigned long *aqm, unsigned long *adm)
  {

drivers/s390/crypto/vfio_ap_drv.c +14 -2

index f340a28c1d65..2a79d27d9730 100644
@@ -42,12 +42,24 @@ MODULE_DEVICE_TABLE(vfio_ap, ap_queue_ids);
 
  static int vfio_ap_queue_dev_probe(struct ap_device *apdev)
  {
-	return 0;
+	struct ap_queue *apq = to_ap_queue(&apdev->device);
+	unsigned long apid = AP_QID_CARD(apq->qid);
+	unsigned long apqi = AP_QID_QUEUE(apq->qid);
+
+	mutex_lock(&matrix_dev->lock);
+	vfio_ap_mdev_probe_queue(apid, apqi);
+	mutex_unlock(&matrix_dev->lock);
  }
 
  static void vfio_ap_queue_dev_remove(struct ap_device *apdev)
  {
-	/* Nothing to do yet */
+	struct ap_queue *apq = to_ap_queue(&apdev->device);
+	unsigned long apid = AP_QID_CARD(apq->qid);
+	unsigned long apqi = AP_QID_QUEUE(apq->qid);
+
+	mutex_lock(&matrix_dev->lock);
+	vfio_ap_mdev_remove_queue(apid, apqi);
+	mutex_unlock(&matrix_dev->lock);
  }
 
  static void vfio_ap_matrix_dev_release(struct device *dev)

drivers/s390/crypto/vfio_ap_ops.c +65 -2

index ade2c150fe6b..8a70707bf870 100644
@@ -683,8 +683,8 @@ static void vfio_ap_mdev_wait_for_qempty(unsigned long apid, unsigned long apqi)
  			msleep(20);
  			break;
  		default:
-			pr_warn("%s: tapq err %02x: 0x%04x may not be empty\n",
-				__func__, status.response_code, q->apqn);
+			pr_warn("%s: tapq err %02x: %02lx%04lx may not be empty\n",
+				__func__, status.response_code, apid, apqi);
  			return;
  		}
  	} while (--retry);

@@ -840,3 +840,66 @@ void vfio_ap_mdev_unregister(void)
  {
  	mdev_unregister_device(&matrix_dev->device);
  }
+
+static struct ap_matrix_mdev *vfio_ap_mdev_find_matrix_mdev(unsigned long apid,
+							    unsigned long apqi)
+{
+	struct ap_matrix_mdev *matrix_mdev;
+
+	list_for_each_entry(matrix_mdev, &matrix_dev->mdev_list, node) {
+		if (test_bit_inv(apid, matrix_mdev->matrix.apm) &&
+		    test_bit_inv(apqi, matrix_mdev->matrix.aqm))
+			return matrix_mdev;
+	}
+
+	return NULL;
+}
+
+void vfio_ap_mdev_remove_queue(unsigned long apid, unsigned long apqi)
+{
+	struct ap_matrix_mdev *matrix_mdev;
+
+	matrix_mdev = vfio_ap_mdev_find_matrix_mdev(apid, apqi);
+
+	/*
+	 * If the queue is assigned to the mdev device and the mdev device
+	 * is in use by a guest
+	 */
+	if (matrix_mdev && matrix_mdev->kvm) {
+		/*
+		 * If the queue is plugged into the guest, unplug the adapter
+		 * but keep the adapter assigned to the mdev device
+		 */
+		if (!kvm_arch_crypto_test_masks(matrix_mdev->kvm,
+						matrix_mdev->matrix.apm,
+						matrix_mdev->matrix.aqm,
+						matrix_mdev->matrix.adm)) {
+			clear_bit_inv(apid, matrix_mdev->matrix.apm);
+			vfio_ap_mdev_update_crycb(matrix_mdev);
+			set_bit_inv(apid, matrix_mdev->matrix.apm);
+		}
+	}
+
+	vfio_ap_mdev_reset_queue(apid, apqi);
+}
+
+void vfio_ap_mdev_probe_queue(unsigned long apid, unsigned long apqi)
+{
+	struct ap_matrix_mdev *matrix_mdev;
+
+	matrix_mdev = vfio_ap_mdev_find_matrix_mdev(apid, apqi);
+
+	/*
+	 * If the queue is assigned to the mdev device and the mdev device
+	 * is in use by a guest
+	 */
+	if (matrix_mdev && matrix_mdev->kvm) {
+		/* If the queue is not plugged into the guest, plug it in */
+		if (!kvm_arch_crypto_test_masks(matrix_mdev->kvm,
+						matrix_mdev->matrix.apm,
+						matrix_mdev->matrix.aqm,
+						matrix_mdev->matrix.adm)) {
+			vfio_ap_mdev_update_crycb(matrix_mdev);
+		}
+	}
+}

drivers/s390/crypto/vfio_ap_private.h +2 -0

index 76b7f98e47e9..acdd5bfabaaf 100644
@@ -85,5 +85,7 @@ struct ap_matrix_mdev {
 
  extern int vfio_ap_mdev_register(void);
  extern void vfio_ap_mdev_unregister(void);
+void vfio_ap_mdev_remove_queue(unsigned long apid, unsigned long apqi);
+void vfio_ap_mdev_probe_queue(unsigned long apid, unsigned long apqi);
 
  #endif /* _VFIO_AP_PRIVATE_H_ */