vfio: ap: ioctl definitions for AP Queue Interrupt Control

Libudev relies on having a subsystem link for non-root devices. To avoid libudev (and potentially other userspace tools) choking on the matrix device let us introduce a vfio_ap bus and with that the vfio_ap bus subsytem, and make the matrix device reside within it.

We restrict the number of allowed devices to a single one.

Doing this we need to suppress the forced link from the matrix device to the vfio_ap driver and we suppress the device_type we do not need anymore.

Since the associated matrix driver is not the vfio_ap driver any more, we have to change the search for the devices on the vfio_ap driver in the function vfio_ap_verify_queue_reserved.

Signed-off-by Pierre Morel <[email protected]>

---
 drivers/s390/crypto/vfio_ap_drv.c     | 63 +++++++++++++++++++++++++++++++----
 drivers/s390/crypto/vfio_ap_ops.c     |  4 +--
 drivers/s390/crypto/vfio_ap_private.h |  1 +
 3 files changed, 60 insertions(+), 8 deletions(-)

drivers/s390/crypto/vfio_ap_drv.c +0 -0

index 31c6c84..1fd5fe6 100644

drivers/s390/crypto/vfio_ap_drv.c +39 -0

 };
 
 struct ap_matrix_dev *matrix_dev;

@@ -62,6 +63,41 @@ static void vfio_ap_matrix_dev_release(struct device *dev)
 	kfree(matrix_dev);
 }
 
+static int matrix_bus_match(struct device *dev, struct device_driver *drv)
+{
+	return 1;
+}
+
+static struct bus_type matrix_bus = {
+	.name = "vfio_ap",
+	.match = &matrix_bus_match,
+};
+
+static int matrix_probe(struct device *dev);
+static int matrix_remove(struct device *dev);
+static struct matrix_driver matrix_driver = {
+	.drv = {
+		.name = "vfio_ap",
+		.bus = &matrix_bus,
+		.probe = matrix_probe,
+		.remove = matrix_remove,
+	},
+};
+
+static int matrix_probe(struct device *dev)
+{
+	if (matrix_driver.device_count)
+		return -EEXIST;
+	matrix_driver.device_count++;
+	return 0;
+}
+
+static int matrix_remove(struct device *dev)
+{
+	matrix_driver.device_count--;
+	return 0;
+}
+
 static int vfio_ap_matrix_dev_create(void)
 {
 	int ret;

@@ -71,6 +107,10 @@ static int vfio_ap_matrix_dev_create(void)
 	if (IS_ERR(root_device))
 		return PTR_ERR(root_device);
 
+	ret = bus_register(&matrix_bus);
+	if (ret)
+		goto bus_register_err;
+
 	matrix_dev = kzalloc(sizeof(*matrix_dev), GFP_KERNEL);
 	if (!matrix_dev) {
 		ret = -ENOMEM;

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

 	return 0;
 
+matrix_drv_err:
+	device_unregister(&matrix_dev->device);
 matrix_reg_err:
 	put_device(&matrix_dev->device);
 matrix_alloc_err:
+	bus_unregister(&matrix_bus);
+bus_register_err:
 	root_device_unregister(root_device);
-
 	return ret;
 }
 
 static void vfio_ap_matrix_dev_destroy(void)
 {
+	struct device *root_device = matrix_dev->device.parent;
+
+	driver_unregister(&matrix_driver.drv);
 	device_unregister(&matrix_dev->device);
-	root_device_unregister(matrix_dev->device.parent);
+	bus_unregister(&matrix_bus);
+	root_device_unregister(root_device);
 }
 
 static int __init vfio_ap_init(void)

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

index 272ef42..900b9cf 100644
@@ -198,8 +198,8 @@ static int vfio_ap_verify_queue_reserved(unsigned long *apid,
 	qres.apqi = apqi;
 	qres.reserved = false;
 
-	ret = driver_for_each_device(matrix_dev->device.driver, NULL, &qres,
-				     vfio_ap_has_queue);
+	ret = driver_for_each_device(&matrix_dev->vfio_ap_drv->driver, NULL,
+				     &qres, vfio_ap_has_queue);
 	if (ret)
 		return ret;
 

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

index 5675492..76b7f98 100644

On Thu, 14 Feb 2019 14:51:01 +0100
Pierre Morel <[email protected]> wrote:

Quoted 5 lines

Libudev relies on having a subsystem link for non-root devices. To avoid libudev (and potentially other userspace tools) choking on the matrix device let us introduce a vfio_ap bus and with that the vfio_ap bus subsytem, and make the matrix device reside within it.

How does libudev choke on this? It feels wrong to introduce a bus that basically does nothing...

Quoted 24 lines

We restrict the number of allowed devices to a single one.

Doing this we need to suppress the forced link from the matrix device to the vfio_ap driver and we suppress the device_type we do not need anymore.

Since the associated matrix driver is not the vfio_ap driver any more, we have to change the search for the devices on the vfio_ap driver in the function vfio_ap_verify_queue_reserved.

Signed-off-by Pierre Morel <[email protected]>

---
 drivers/s390/crypto/vfio_ap_drv.c     | 63 +++++++++++++++++++++++++++++++----
 drivers/s390/crypto/vfio_ap_ops.c     |  4 +--
 drivers/s390/crypto/vfio_ap_private.h |  1 +
 3 files changed, 60 insertions(+), 8 deletions(-)

drivers/s390/crypto/vfio_ap_drv.c +0 -0

index 31c6c84..1fd5fe6 100644

drivers/s390/crypto/vfio_ap_drv.c MODULE_LICENSE("GPL v2"); · line 29

Quoted 8 lines

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

@@ -24,8 +24,9 @@ MODULE_LICENSE("GPL v2");
 
 static struct ap_driver vfio_ap_drv;
 
-static struct device_type vfio_ap_dev_type = {
-	.name = VFIO_AP_DEV_TYPE_NAME,
+struct matrix_driver {
+	struct device_driver drv;
+	int device_count;

This counter basically ensures that at most one device may bind with this driver... you'd still have that device on the bus, though.

Quoted 57 lines

drivers/s390/crypto/vfio_ap_drv.c +39 -0

 };
 
 struct ap_matrix_dev *matrix_dev;

@@ -62,6 +63,41 @@ static void vfio_ap_matrix_dev_release(struct device *dev)
 	kfree(matrix_dev);
 }
 
+static int matrix_bus_match(struct device *dev, struct device_driver *drv)
+{
+	return 1;
+}
+
+static struct bus_type matrix_bus = {
+	.name = "vfio_ap",
+	.match = &matrix_bus_match,
+};
+
+static int matrix_probe(struct device *dev);
+static int matrix_remove(struct device *dev);
+static struct matrix_driver matrix_driver = {
+	.drv = {
+		.name = "vfio_ap",
+		.bus = &matrix_bus,
+		.probe = matrix_probe,
+		.remove = matrix_remove,
+	},
+};
+
+static int matrix_probe(struct device *dev)
+{
+	if (matrix_driver.device_count)
+		return -EEXIST;
+	matrix_driver.device_count++;
+	return 0;
+}
+
+static int matrix_remove(struct device *dev)
+{
+	matrix_driver.device_count--;
+	return 0;
+}
+
 static int vfio_ap_matrix_dev_create(void)
 {
 	int ret;

@@ -71,6 +107,10 @@ static int vfio_ap_matrix_dev_create(void)
 	if (IS_ERR(root_device))
 		return PTR_ERR(root_device);
 
+	ret = bus_register(&matrix_bus);
+	if (ret)
+		goto bus_register_err;
+
 	matrix_dev = kzalloc(sizeof(*matrix_dev), GFP_KERNEL);
 	if (!matrix_dev) {
 		ret = -ENOMEM;

drivers/s390/crypto/vfio_ap_drv.c static int vfio_ap_matrix_dev_create(void) · line 134

Quoted 10 lines

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

@@ -87,30 +127,41 @@ static int vfio_ap_matrix_dev_create(void)
 	mutex_init(&matrix_dev->lock);
 	INIT_LIST_HEAD(&matrix_dev->mdev_list);
 
-	matrix_dev->device.type = &vfio_ap_dev_type;
 	dev_set_name(&matrix_dev->device, "%s", VFIO_AP_DEV_NAME);
 	matrix_dev->device.parent = root_device;
+	matrix_dev->device.bus = &matrix_bus;
 	matrix_dev->device.release = vfio_ap_matrix_dev_release;
-	matrix_dev->device.driver = &vfio_ap_drv.driver;
+	matrix_dev->vfio_ap_drv = &vfio_ap_drv;

Can't you get that structure through matrix_dev->device.driver instead when you need it in the function below?

drivers/s390/crypto/vfio_ap_drv.c static int vfio_ap_matrix_dev_create(void) · line 143

Quoted 9 lines

drivers/s390/crypto/vfio_ap_drv.c +4 -0

 	ret = device_register(&matrix_dev->device);
 	if (ret)
 		goto matrix_reg_err;
 
+	ret = driver_register(&matrix_driver.drv);
+	if (ret)
+		goto matrix_drv_err;
+

As you already have several structures that can be registered exactly once (the root device, the bus, the driver, ...), you can already be sure that there's only one device on the bus, can't you?

Quoted 46 lines

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

 	return 0;
 
+matrix_drv_err:
+	device_unregister(&matrix_dev->device);
 matrix_reg_err:
 	put_device(&matrix_dev->device);
 matrix_alloc_err:
+	bus_unregister(&matrix_bus);
+bus_register_err:
 	root_device_unregister(root_device);
-
 	return ret;
 }
 
 static void vfio_ap_matrix_dev_destroy(void)
 {
+	struct device *root_device = matrix_dev->device.parent;
+
+	driver_unregister(&matrix_driver.drv);
 	device_unregister(&matrix_dev->device);
-	root_device_unregister(matrix_dev->device.parent);
+	bus_unregister(&matrix_bus);
+	root_device_unregister(root_device);
 }
 
 static int __init vfio_ap_init(void)

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

index 272ef42..900b9cf 100644
@@ -198,8 +198,8 @@ static int vfio_ap_verify_queue_reserved(unsigned long *apid,
 	qres.apqi = apqi;
 	qres.reserved = false;
 
-	ret = driver_for_each_device(matrix_dev->device.driver, NULL, &qres,
-				     vfio_ap_has_queue);
+	ret = driver_for_each_device(&matrix_dev->vfio_ap_drv->driver, NULL,
+				     &qres, vfio_ap_has_queue);
 	if (ret)
 		return ret;
 

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

index 5675492..76b7f98 100644

drivers/s390/crypto/vfio_ap_private.h struct ap_matrix_dev { · line 46

Quoted 7 lines

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

@@ -40,6 +40,7 @@ struct ap_matrix_dev {
 	struct ap_config_info info;
 	struct list_head mdev_list;
 	struct mutex lock;
+	struct ap_driver  *vfio_ap_drv;
 };
 
 extern struct ap_matrix_dev *matrix_dev;

This feels like a lot of boilerplate code, just to create a bus that basically doesn't do anything. I'm surprised that libudev can't deal with bus-less devices properly...

Quoted 2 lines · Cornelia Huck

On Thu, 14 Feb 2019 14:51:01 +0100
Pierre Morel <[email protected]> wrote:

Pierre,
this is independent from this series and should have been sent separately. In the end (when we have the final solution) this will require cc stable.

Quoted 8 lines

Quoted 5 lines

Libudev relies on having a subsystem link for non-root devices. To avoid libudev (and potentially other userspace tools) choking on the matrix device let us introduce a vfio_ap bus and with that the vfio_ap bus subsytem, and make the matrix device reside within it.

How does libudev choke on this? It feels wrong to introduce a bus that basically does nothing...

I have seen libvirt looping when a matrix device was available before the libvirt start.
Marc Hartmayer debugged this and circumvented this in libvirt:
https://www.redhat.com/archives/libvir-list/2019-February/msg00837.html

Still libudev expects a subsystem link in the matrix folder when doing the udev_enumerate_scan_devices call.

Having a bus is one way of adding a subsystem link.

Quoted 2 lines · Cornelia Huck

On Thu, 14 Feb 2019 14:51:01 +0100
Pierre Morel <[email protected]> wrote:

Pierre,
this is independent from this series and should have been sent separately. In the end (when we have the final solution) this will require cc stable.

Quoted 8 lines

Quoted 5 lines

Libudev relies on having a subsystem link for non-root devices. To avoid libudev (and potentially other userspace tools) choking on the matrix device let us introduce a vfio_ap bus and with that the vfio_ap bus subsytem, and make the matrix device reside within it.

How does libudev choke on this? It feels wrong to introduce a bus that basically does nothing...

I have seen libvirt looping when a matrix device was available before the libvirt start.
Marc Hartmayer debugged this and circumvented this in libvirt:
https://www.redhat.com/archives/libvir-list/2019-February/msg00837.html

Still libudev expects a subsystem link in the matrix folder when doing the udev_enumerate_scan_devices call.

Having a bus is one way of adding a subsystem link.

Quoted 179 lines

Quoted 24 lines

We restrict the number of allowed devices to a single one.

Doing this we need to suppress the forced link from the matrix device to the vfio_ap driver and we suppress the device_type we do not need anymore.

Since the associated matrix driver is not the vfio_ap driver any more, we have to change the search for the devices on the vfio_ap driver in the function vfio_ap_verify_queue_reserved.

Signed-off-by Pierre Morel <[email protected]>

---
  drivers/s390/crypto/vfio_ap_drv.c     | 63 +++++++++++++++++++++++++++++++----
  drivers/s390/crypto/vfio_ap_ops.c     |  4 +--
  drivers/s390/crypto/vfio_ap_private.h |  1 +
  3 files changed, 60 insertions(+), 8 deletions(-)

drivers/s390/crypto/vfio_ap_drv.c +0 -0

index 31c6c84..1fd5fe6 100644

drivers/s390/crypto/vfio_ap_drv.c MODULE_LICENSE("GPL v2"); · line 29

Quoted 8 lines

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

@@ -24,8 +24,9 @@ MODULE_LICENSE("GPL v2");
 
  static struct ap_driver vfio_ap_drv;
 
-static struct device_type vfio_ap_dev_type = {
-	.name = VFIO_AP_DEV_TYPE_NAME,
+struct matrix_driver {
+	struct device_driver drv;
+	int device_count;

This counter basically ensures that at most one device may bind with this driver... you'd still have that device on the bus, though.

Quoted 57 lines

drivers/s390/crypto/vfio_ap_drv.c +39 -0

  };
 
  struct ap_matrix_dev *matrix_dev;

@@ -62,6 +63,41 @@ static void vfio_ap_matrix_dev_release(struct device *dev)
  	kfree(matrix_dev);
  }
 
+static int matrix_bus_match(struct device *dev, struct device_driver *drv)
+{
+	return 1;
+}
+
+static struct bus_type matrix_bus = {
+	.name = "vfio_ap",
+	.match = &matrix_bus_match,
+};
+
+static int matrix_probe(struct device *dev);
+static int matrix_remove(struct device *dev);
+static struct matrix_driver matrix_driver = {
+	.drv = {
+		.name = "vfio_ap",
+		.bus = &matrix_bus,
+		.probe = matrix_probe,
+		.remove = matrix_remove,
+	},
+};
+
+static int matrix_probe(struct device *dev)
+{
+	if (matrix_driver.device_count)
+		return -EEXIST;
+	matrix_driver.device_count++;
+	return 0;
+}
+
+static int matrix_remove(struct device *dev)
+{
+	matrix_driver.device_count--;
+	return 0;
+}
+
  static int vfio_ap_matrix_dev_create(void)
  {
  	int ret;

@@ -71,6 +107,10 @@ static int vfio_ap_matrix_dev_create(void)
  	if (IS_ERR(root_device))
  		return PTR_ERR(root_device);
 
+	ret = bus_register(&matrix_bus);
+	if (ret)
+		goto bus_register_err;
+
  	matrix_dev = kzalloc(sizeof(*matrix_dev), GFP_KERNEL);
  	if (!matrix_dev) {
  		ret = -ENOMEM;

drivers/s390/crypto/vfio_ap_drv.c static int vfio_ap_matrix_dev_create(void) · line 134

Quoted 10 lines

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

@@ -87,30 +127,41 @@ static int vfio_ap_matrix_dev_create(void)
  	mutex_init(&matrix_dev->lock);
  	INIT_LIST_HEAD(&matrix_dev->mdev_list);
 
-	matrix_dev->device.type = &vfio_ap_dev_type;
  	dev_set_name(&matrix_dev->device, "%s", VFIO_AP_DEV_NAME);
  	matrix_dev->device.parent = root_device;
+	matrix_dev->device.bus = &matrix_bus;
  	matrix_dev->device.release = vfio_ap_matrix_dev_release;
-	matrix_dev->device.driver = &vfio_ap_drv.driver;
+	matrix_dev->vfio_ap_drv = &vfio_ap_drv;

Can't you get that structure through matrix_dev->device.driver instead when you need it in the function below?

drivers/s390/crypto/vfio_ap_drv.c static int vfio_ap_matrix_dev_create(void) · line 143

Quoted 9 lines

drivers/s390/crypto/vfio_ap_drv.c +4 -0

  	ret = device_register(&matrix_dev->device);
  	if (ret)
  		goto matrix_reg_err;
 
+	ret = driver_register(&matrix_driver.drv);
+	if (ret)
+		goto matrix_drv_err;
+

As you already have several structures that can be registered exactly once (the root device, the bus, the driver, ...), you can already be sure that there's only one device on the bus, can't you?

Quoted 46 lines

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

  	return 0;
 
+matrix_drv_err:
+	device_unregister(&matrix_dev->device);
  matrix_reg_err:
  	put_device(&matrix_dev->device);
  matrix_alloc_err:
+	bus_unregister(&matrix_bus);
+bus_register_err:
  	root_device_unregister(root_device);
-
  	return ret;
  }
 
  static void vfio_ap_matrix_dev_destroy(void)
  {
+	struct device *root_device = matrix_dev->device.parent;
+
+	driver_unregister(&matrix_driver.drv);
  	device_unregister(&matrix_dev->device);
-	root_device_unregister(matrix_dev->device.parent);
+	bus_unregister(&matrix_bus);
+	root_device_unregister(root_device);
  }
 
  static int __init vfio_ap_init(void)

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

index 272ef42..900b9cf 100644
@@ -198,8 +198,8 @@ static int vfio_ap_verify_queue_reserved(unsigned long *apid,
  	qres.apqi = apqi;
  	qres.reserved = false;
 
-	ret = driver_for_each_device(matrix_dev->device.driver, NULL, &qres,
-				     vfio_ap_has_queue);
+	ret = driver_for_each_device(&matrix_dev->vfio_ap_drv->driver, NULL,
+				     &qres, vfio_ap_has_queue);
  	if (ret)
  		return ret;
 

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

index 5675492..76b7f98 100644

drivers/s390/crypto/vfio_ap_private.h struct ap_matrix_dev { · line 46

Quoted 7 lines

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

@@ -40,6 +40,7 @@ struct ap_matrix_dev {
  	struct ap_config_info info;
  	struct list_head mdev_list;
  	struct mutex lock;
+	struct ap_driver  *vfio_ap_drv;
  };
 
  extern struct ap_matrix_dev *matrix_dev;

This feels like a lot of boilerplate code, just to create a bus that basically doesn't do anything. I'm surprised that libudev can't deal with bus-less devices properly...

Quoted 2 lines · Cornelia Huck

On Thu, 14 Feb 2019 14:51:01 +0100
Pierre Morel <[email protected]> wrote:

Pierre,
this is independent from this series and should have been sent separately. In the end (when we have the final solution) this will require cc stable.

Quoted 8 lines

Quoted 5 lines

Libudev relies on having a subsystem link for non-root devices. To avoid libudev (and potentially other userspace tools) choking on the matrix device let us introduce a vfio_ap bus and with that the vfio_ap bus subsytem, and make the matrix device reside within it.

How does libudev choke on this? It feels wrong to introduce a bus that basically does nothing...

I have seen libvirt looping when a matrix device was available before the libvirt start.
Marc Hartmayer debugged this and circumvented this in libvirt:
https://www.redhat.com/archives/libvir-list/2019-February/msg00837.html

Still libudev expects a subsystem link in the matrix folder when doing the udev_enumerate_scan_devices call.

Having a bus is one way of adding a subsystem link.

Quoted 179 lines

Quoted 24 lines

We restrict the number of allowed devices to a single one.

Doing this we need to suppress the forced link from the matrix device to the vfio_ap driver and we suppress the device_type we do not need anymore.

Since the associated matrix driver is not the vfio_ap driver any more, we have to change the search for the devices on the vfio_ap driver in the function vfio_ap_verify_queue_reserved.

Signed-off-by Pierre Morel <[email protected]>

---
  drivers/s390/crypto/vfio_ap_drv.c     | 63 +++++++++++++++++++++++++++++++----
  drivers/s390/crypto/vfio_ap_ops.c     |  4 +--
  drivers/s390/crypto/vfio_ap_private.h |  1 +
  3 files changed, 60 insertions(+), 8 deletions(-)

drivers/s390/crypto/vfio_ap_drv.c +0 -0

index 31c6c84..1fd5fe6 100644

drivers/s390/crypto/vfio_ap_drv.c MODULE_LICENSE("GPL v2"); · line 29

Quoted 8 lines

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

@@ -24,8 +24,9 @@ MODULE_LICENSE("GPL v2");
 
  static struct ap_driver vfio_ap_drv;
 
-static struct device_type vfio_ap_dev_type = {
-	.name = VFIO_AP_DEV_TYPE_NAME,
+struct matrix_driver {
+	struct device_driver drv;
+	int device_count;

This counter basically ensures that at most one device may bind with this driver... you'd still have that device on the bus, though.

Quoted 57 lines

drivers/s390/crypto/vfio_ap_drv.c +39 -0

  };
 
  struct ap_matrix_dev *matrix_dev;

@@ -62,6 +63,41 @@ static void vfio_ap_matrix_dev_release(struct device *dev)
  	kfree(matrix_dev);
  }
 
+static int matrix_bus_match(struct device *dev, struct device_driver *drv)
+{
+	return 1;
+}
+
+static struct bus_type matrix_bus = {
+	.name = "vfio_ap",
+	.match = &matrix_bus_match,
+};
+
+static int matrix_probe(struct device *dev);
+static int matrix_remove(struct device *dev);
+static struct matrix_driver matrix_driver = {
+	.drv = {
+		.name = "vfio_ap",
+		.bus = &matrix_bus,
+		.probe = matrix_probe,
+		.remove = matrix_remove,
+	},
+};
+
+static int matrix_probe(struct device *dev)
+{
+	if (matrix_driver.device_count)
+		return -EEXIST;
+	matrix_driver.device_count++;
+	return 0;
+}
+
+static int matrix_remove(struct device *dev)
+{
+	matrix_driver.device_count--;
+	return 0;
+}
+
  static int vfio_ap_matrix_dev_create(void)
  {
  	int ret;

@@ -71,6 +107,10 @@ static int vfio_ap_matrix_dev_create(void)
  	if (IS_ERR(root_device))
  		return PTR_ERR(root_device);
 
+	ret = bus_register(&matrix_bus);
+	if (ret)
+		goto bus_register_err;
+
  	matrix_dev = kzalloc(sizeof(*matrix_dev), GFP_KERNEL);
  	if (!matrix_dev) {
  		ret = -ENOMEM;

drivers/s390/crypto/vfio_ap_drv.c static int vfio_ap_matrix_dev_create(void) · line 134

Quoted 10 lines

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

@@ -87,30 +127,41 @@ static int vfio_ap_matrix_dev_create(void)
  	mutex_init(&matrix_dev->lock);
  	INIT_LIST_HEAD(&matrix_dev->mdev_list);
 
-	matrix_dev->device.type = &vfio_ap_dev_type;
  	dev_set_name(&matrix_dev->device, "%s", VFIO_AP_DEV_NAME);
  	matrix_dev->device.parent = root_device;
+	matrix_dev->device.bus = &matrix_bus;
  	matrix_dev->device.release = vfio_ap_matrix_dev_release;
-	matrix_dev->device.driver = &vfio_ap_drv.driver;
+	matrix_dev->vfio_ap_drv = &vfio_ap_drv;

Can't you get that structure through matrix_dev->device.driver instead when you need it in the function below?

drivers/s390/crypto/vfio_ap_drv.c static int vfio_ap_matrix_dev_create(void) · line 143

Quoted 9 lines

drivers/s390/crypto/vfio_ap_drv.c +4 -0

  	ret = device_register(&matrix_dev->device);
  	if (ret)
  		goto matrix_reg_err;
 
+	ret = driver_register(&matrix_driver.drv);
+	if (ret)
+		goto matrix_drv_err;
+

As you already have several structures that can be registered exactly once (the root device, the bus, the driver, ...), you can already be sure that there's only one device on the bus, can't you?

Quoted 46 lines

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

  	return 0;
 
+matrix_drv_err:
+	device_unregister(&matrix_dev->device);
  matrix_reg_err:
  	put_device(&matrix_dev->device);
  matrix_alloc_err:
+	bus_unregister(&matrix_bus);
+bus_register_err:
  	root_device_unregister(root_device);
-
  	return ret;
  }
 
  static void vfio_ap_matrix_dev_destroy(void)
  {
+	struct device *root_device = matrix_dev->device.parent;
+
+	driver_unregister(&matrix_driver.drv);
  	device_unregister(&matrix_dev->device);
-	root_device_unregister(matrix_dev->device.parent);
+	bus_unregister(&matrix_bus);
+	root_device_unregister(root_device);
  }
 
  static int __init vfio_ap_init(void)

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

index 272ef42..900b9cf 100644
@@ -198,8 +198,8 @@ static int vfio_ap_verify_queue_reserved(unsigned long *apid,
  	qres.apqi = apqi;
  	qres.reserved = false;
 
-	ret = driver_for_each_device(matrix_dev->device.driver, NULL, &qres,
-				     vfio_ap_has_queue);
+	ret = driver_for_each_device(&matrix_dev->vfio_ap_drv->driver, NULL,
+				     &qres, vfio_ap_has_queue);
  	if (ret)
  		return ret;
 

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

index 5675492..76b7f98 100644

drivers/s390/crypto/vfio_ap_private.h struct ap_matrix_dev { · line 46

Quoted 7 lines

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

@@ -40,6 +40,7 @@ struct ap_matrix_dev {
  	struct ap_config_info info;
  	struct list_head mdev_list;
  	struct mutex lock;
+	struct ap_driver  *vfio_ap_drv;
  };
 
  extern struct ap_matrix_dev *matrix_dev;

This feels like a lot of boilerplate code, just to create a bus that basically doesn't do anything. I'm surprised that libudev can't deal with bus-less devices properly...

Quoted 4 lines · On Thu, 14 Feb 2019 14:51:01 +0100 Pierre Morel <[email protected]> wrote:

Libudev relies on having a subsystem link for non-root devices. To avoid libudev (and potentially other userspace tools) choking on the matrix device let us introduce a vfio_ap bus and with that the vfio_ap bus subsytem, and make the matrix device reside within it.

How does libudev choke on this? It feels wrong to introduce a bus that basically does nothing...

Quoted 4 lines

We restrict the number of allowed devices to a single one.

Quoted 5 lines

+++ b/drivers/s390/crypto/vfio_ap_drv.c

drivers/s390/crypto/vfio_ap_drv.c +0 -0

@@ -24,8 +24,9 @@ MODULE_LICENSE("GPL v2");
 
  static struct ap_driver vfio_ap_drv;
 

drivers/s390/crypto/vfio_ap_drv.c MODULE_LICENSE("GPL v2"); · line 29

Quoted 5 lines

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

-static struct device_type vfio_ap_dev_type = {
-	.name = VFIO_AP_DEV_TYPE_NAME,
+struct matrix_driver {
+	struct device_driver drv;
+	int device_count;

This counter basically ensures that at most one device may bind with this driver... you'd still have that device on the bus, though.

Quoted 6 lines

drivers/s390/crypto/vfio_ap_drv.c +0 -0

  };
 
  struct ap_matrix_dev *matrix_dev;

drivers/s390/crypto/vfio_ap_drv.c static int vfio_ap_matrix_dev_create(void) · line 134

Quoted 7 lines

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

-	matrix_dev->device.type = &vfio_ap_dev_type;
  	dev_set_name(&matrix_dev->device, "%s", VFIO_AP_DEV_NAME);
  	matrix_dev->device.parent = root_device;
+	matrix_dev->device.bus = &matrix_bus;
  	matrix_dev->device.release = vfio_ap_matrix_dev_release;
-	matrix_dev->device.driver = &vfio_ap_drv.driver;
+	matrix_dev->vfio_ap_drv = &vfio_ap_drv;

Can't you get that structure through matrix_dev->device.driver instead when you need it in the function below?

Quoted 6 lines

  
  	ret = device_register(&matrix_dev->device);
  	if (ret)
  		goto matrix_reg_err;

drivers/s390/crypto/vfio_ap_drv.c static int vfio_ap_matrix_dev_create(void) · line 143

Quoted 4 lines

drivers/s390/crypto/vfio_ap_drv.c +4 -0

+	ret = driver_register(&matrix_driver.drv);
+	if (ret)
+		goto matrix_drv_err;
+

As you already have several structures that can be registered exactly once (the root device, the bus, the driver, ...), you can already be sure that there's only one device on the bus, can't you?

Quoted 8 lines · Cornelia Huck

Quoted 5 lines · On Thu, 14 Feb 2019 14:51:01 +0100 Pierre Morel <[email protected]> wrote:

+++ b/drivers/s390/crypto/vfio_ap_drv.c

drivers/s390/crypto/vfio_ap_drv.c +0 -0

@@ -24,8 +24,9 @@ MODULE_LICENSE("GPL v2");
 
  static struct ap_driver vfio_ap_drv;
 

Quoted 8 lines

drivers/s390/crypto/vfio_ap_drv.c MODULE_LICENSE("GPL v2"); · line 29

Quoted 5 lines

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

-static struct device_type vfio_ap_dev_type = {
-	.name = VFIO_AP_DEV_TYPE_NAME,
+struct matrix_driver {
+	struct device_driver drv;
+	int device_count;  

This counter basically ensures that at most one device may bind with this driver... you'd still have that device on the bus, though.

yes, this is what is wanted: this driver can only support one device. May be another matrix driver can support one or more other devices.

I should update comment message my be.

Quoted 6 lines

Quoted 6 lines

drivers/s390/crypto/vfio_ap_drv.c +0 -0

  };
 
  struct ap_matrix_dev *matrix_dev;  

Quoted 10 lines

drivers/s390/crypto/vfio_ap_drv.c static int vfio_ap_matrix_dev_create(void) · line 134

Quoted 7 lines

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

-	matrix_dev->device.type = &vfio_ap_dev_type;
  	dev_set_name(&matrix_dev->device, "%s", VFIO_AP_DEV_NAME);
  	matrix_dev->device.parent = root_device;
+	matrix_dev->device.bus = &matrix_bus;
  	matrix_dev->device.release = vfio_ap_matrix_dev_release;
-	matrix_dev->device.driver = &vfio_ap_drv.driver;
+	matrix_dev->vfio_ap_drv = &vfio_ap_drv;  

Can't you get that structure through matrix_dev->device.driver instead when you need it in the function below?

Not anymore.
We have two different drivers and devices
matrix_drv <-> matrix_dev
and
vfio_ap_drv <-> ap_devices

The driver behind the matrix_dev->dev->driver is matrix_drv what is needed here is vfio_ap_drv.

Quoted 7 lines

Quoted 7 lines

  
  
  	ret = device_register(&matrix_dev->device);
  	if (ret)
  		goto matrix_reg_err;

Quoted 8 lines

drivers/s390/crypto/vfio_ap_drv.c static int vfio_ap_matrix_dev_create(void) · line 143

Quoted 4 lines

drivers/s390/crypto/vfio_ap_drv.c +4 -0

+	ret = driver_register(&matrix_driver.drv);
+	if (ret)
+		goto matrix_drv_err;
+  

As you already have several structures that can be registered exactly once (the root device, the bus, the driver, ...), you can already be sure that there's only one device on the bus, can't you?

hum, no I don't think so, no device can register before this module is loaded, but what does prevent a device to register later from another module?

Quoted 38 lines · On Thu, 14 Feb 2019 16:47:30 +0100 Pierre Morel <[email protected]> wrote:

Quoted 8 lines · Cornelia Huck

Quoted 5 lines · On Thu, 14 Feb 2019 14:51:01 +0100 Pierre Morel <[email protected]> wrote:

+++ b/drivers/s390/crypto/vfio_ap_drv.c @@ -24,8 +24,9 @@ MODULE_LICENSE("GPL v2");

static struct ap_driver vfio_ap_drv;

Quoted 7 lines

Quoted 3 lines

-static struct device_type vfio_ap_dev_type = { - .name =
VFIO_AP_DEV_TYPE_NAME, +struct matrix_driver { + struct
device_driver drv; + int device_count;

This counter basically ensures that at most one device may bind with this driver... you'd still have that device on the bus, though.

yes, this is what is wanted: this driver can only support one device. May be another matrix driver can support one or more other devices.

I should update comment message my be.

Quoted 14 lines

Quoted 11 lines

};

struct ap_matrix_dev *matrix_dev;

drivers/s390/crypto/vfio_ap_drv.c +0 -1

-	matrix_dev->device.type = &vfio_ap_dev_type; 

dev_set_name(&matrix_dev->device, "%s", VFIO_AP_DEV_NAME); matrix_dev->device.parent = root_device; + matrix_dev->device.bus = &matrix_bus; matrix_dev->device.release = vfio_ap_matrix_dev_release; - matrix_dev->device.driver = &vfio_ap_drv.driver; +
matrix_dev->vfio_ap_drv = &vfio_ap_drv;

Can't you get that structure through matrix_dev->device.driver instead when you need it in the function below?

Quoted 5 lines

Not anymore. We have two different drivers and devices matrix_drv <-> matrix_dev and vfio_ap_drv <-> ap_devices

The driver behind the matrix_dev->dev->driver is matrix_drv what is needed here is vfio_ap_drv.

Wait, we had tacked a driver for ap devices unto a matrix device, which is not on the ap bus?

Maybe that's what trips libudev? >
(And reading further in the current code, it seems we clear that structure _after_ the matrix device had been setup, so how can that even work? Where am I confused?)

Quoted 15 lines

Quoted 14 lines

Quoted 9 lines

ret = device_register(&matrix_dev->device); if (ret) goto matrix_reg_err;

(quoted diff) +1 -0

+	ret = driver_register(&matrix_driver.drv); +	if (ret) +		goto

matrix_drv_err; +

As you already have several structures that can be registered exactly once (the root device, the bus, the driver, ...), you can already be sure that there's only one device on the bus, can't you?

Quoted 3 lines

hum, no I don't think so, no device can register before this module is loaded, but what does prevent a device to register later from another module?

Not unless you export the interface, I guess.

Quoted 41 lines · Cornelia Huck

Quoted 38 lines · On Thu, 14 Feb 2019 16:47:30 +0100 Pierre Morel <[email protected]> wrote:

Quoted 8 lines · Cornelia Huck

Quoted 5 lines · On Thu, 14 Feb 2019 14:51:01 +0100 Pierre Morel <[email protected]> wrote:

+++ b/drivers/s390/crypto/vfio_ap_drv.c @@ -24,8 +24,9 @@ MODULE_LICENSE("GPL v2");

static struct ap_driver vfio_ap_drv;

Quoted 7 lines

Quoted 3 lines

-static struct device_type vfio_ap_dev_type = { -    .name = VFIO_AP_DEV_TYPE_NAME, +struct matrix_driver { +    struct device_driver drv; +    int device_count;

This counter basically ensures that at most one device may bind with this driver... you'd still have that device on the bus, though.

yes, this is what is wanted: this driver can only support one device. May be another matrix driver can support one or more other devices.

I should update comment message my be.

Quoted 14 lines

Quoted 11 lines

};

struct ap_matrix_dev *matrix_dev;

-    matrix_dev->device.type = &vfio_ap_dev_type;
dev_set_name(&matrix_dev->device, "%s", VFIO_AP_DEV_NAME); matrix_dev->device.parent = root_device; + matrix_dev->device.bus = &matrix_bus; matrix_dev->device.release = vfio_ap_matrix_dev_release; - matrix_dev->device.driver = &vfio_ap_drv.driver; +
matrix_dev->vfio_ap_drv = &vfio_ap_drv;

Can't you get that structure through matrix_dev->device.driver instead when you need it in the function below?

Quoted 8 lines

Quoted 5 lines

Not anymore. We have two different drivers and devices matrix_drv <-> matrix_dev and vfio_ap_drv <-> ap_devices

The driver behind the matrix_dev->dev->driver is matrix_drv what is needed here is vfio_ap_drv.

Wait, we had tacked a driver for ap devices unto a matrix device, which is not on the ap bus?

...yes -(

Quoted 2 lines

Maybe that's what trips libudev? >
(And reading further in the current code, it seems we clear that structure _after_ the matrix device had been setup, so how can that even work? Where am I confused?)

On device_register there were no bus, so the core just do not look for a driver and this field was nor tested nor overwritten.

Quoted 16 lines

Quoted 16 lines

Quoted 15 lines

Quoted 10 lines

ret = device_register(&matrix_dev->device); if (ret) goto matrix_reg_err;

+    ret = driver_register(&matrix_driver.drv); +    if (ret) +        goto
matrix_drv_err; +

As you already have several structures that can be registered exactly once (the root device, the bus, the driver, ...), you can already be sure that there's only one device on the bus, can't you?

Quoted 6 lines

Quoted 3 lines

hum, no I don't think so, no device can register before this module is loaded, but what does prevent a device to register later from another module?

Not unless you export the interface, I guess.

:) definitively right
thanks, this will simplify the code in the next version.
I will take the patch away from this series to get the way to stable as Christian requested.

Regards,
Pierre

Quoted 19 lines · Pierre Morel

Quoted 18 lines · Cornelia Huck

Quoted 15 lines · On Thu, 14 Feb 2019 16:47:30 +0100 Pierre Morel <[email protected]> wrote:

Quoted 13 lines · Cornelia Huck

Quoted 7 lines · On Thu, 14 Feb 2019 14:51:01 +0100 Pierre Morel <[email protected]> wrote:

-    matrix_dev->device.type = &vfio_ap_dev_type; dev_set_name(&matrix_dev->device, "%s", VFIO_AP_DEV_NAME); matrix_dev->device.parent = root_device; + matrix_dev->device.bus = &matrix_bus; matrix_dev->device.release = vfio_ap_matrix_dev_release; -
matrix_dev->device.driver = &vfio_ap_drv.driver; + matrix_dev->vfio_ap_drv = &vfio_ap_drv;

Can't you get that structure through matrix_dev->device.driver instead when you need it in the function below?

Quoted 8 lines

Quoted 8 lines

Quoted 5 lines

Not anymore. We have two different drivers and devices matrix_drv <-> matrix_dev and vfio_ap_drv <-> ap_devices

The driver behind the matrix_dev->dev->driver is matrix_drv what is needed here is vfio_ap_drv.

Wait, we had tacked a driver for ap devices unto a matrix device, which is not on the ap bus?

It's really a bit more complicated than that. Without going into a lengthy description of the history of AP passthrough support, suffice it to say that we needed a device to serve as the parent of each mediated device used to configure a matrix of AP adapter IDs and domain indexes identifying the devices to which a guest would be granted access. The AP devices themselves are attached to the AP bus, but the matrix device is an artificial (virtual?) device whose sole purpose in life is to serve as an anchor for the mediated devices whose sysfs interfaces are created and managed by the vfio_ap device driver. The matrix device itself is created by the vfio_ap device driver - when it is initialized
- for that purpose. In hindsight, maybe there was a better way to implement this, but neither this patch nor this discussion belongs in this series. It distracts from discussion of interrupt support which is the sole purpose of the patch series.

Quoted 11 lines

...yes -(

Quoted 4 lines

Maybe that's what trips libudev? >
(And reading further in the current code, it seems we clear that structure _after_ the matrix device had been setup, so how can that even work? Where am I confused?)

On device_register there were no bus, so the core just do not look for a driver and this field was nor tested nor overwritten.

Quoted 21 lines

Quoted 21 lines

Quoted 20 lines

Quoted 8 lines

Quoted 8 lines

ret = device_register(&matrix_dev->device); if (ret) goto matrix_reg_err;

Quoted 8 lines

Quoted 3 lines

+    ret = driver_register(&matrix_driver.drv); +    if (ret) +        goto
matrix_drv_err; +

As you already have several structures that can be registered exactly once (the root device, the bus, the driver, ...), you can already be sure that there's only one device on the bus, can't you?

hum, no I don't think so, no device can register before this module is loaded, but what does prevent a device to register later from another module?

Quoted 7 lines

Quoted 2 lines

Not unless you export the interface, I guess.

:) definitively right
thanks, this will simplify the code in the next version.
I will take the patch away from this series to get the way to stable as Christian requested.

Quoted 29 lines · On Thu, 14 Feb 2019 13:30:59 -0500 Tony Krowiak <[email protected]> wrote:

Quoted 19 lines · Pierre Morel

Quoted 18 lines · Cornelia Huck

Quoted 15 lines · On Thu, 14 Feb 2019 16:47:30 +0100 Pierre Morel <[email protected]> wrote:

Quoted 13 lines · Cornelia Huck

Quoted 7 lines · On Thu, 14 Feb 2019 14:51:01 +0100 Pierre Morel <[email protected]> wrote:

-    matrix_dev->device.type = &vfio_ap_dev_type; dev_set_name(&matrix_dev->device, "%s", VFIO_AP_DEV_NAME); matrix_dev->device.parent = root_device; + matrix_dev->device.bus = &matrix_bus; matrix_dev->device.release = vfio_ap_matrix_dev_release; -
matrix_dev->device.driver = &vfio_ap_drv.driver; + matrix_dev->vfio_ap_drv = &vfio_ap_drv;

Can't you get that structure through matrix_dev->device.driver instead when you need it in the function below?

Quoted 8 lines

Quoted 8 lines

Quoted 5 lines

Not anymore. We have two different drivers and devices matrix_drv <-> matrix_dev and vfio_ap_drv <-> ap_devices

The driver behind the matrix_dev->dev->driver is matrix_drv what is needed here is vfio_ap_drv.

Wait, we had tacked a driver for ap devices unto a matrix device, which is not on the ap bus?

Quoted 14 lines

It's really a bit more complicated than that. Without going into a lengthy description of the history of AP passthrough support, suffice it to say that we needed a device to serve as the parent of each mediated device used to configure a matrix of AP adapter IDs and domain indexes identifying the devices to which a guest would be granted access. The AP devices themselves are attached to the AP bus, but the matrix device is an artificial (virtual?) device whose sole purpose in life is to serve as an anchor for the mediated devices whose sysfs interfaces are created and managed by the vfio_ap device driver. The matrix device itself is created by the vfio_ap device driver - when it is initialized
- for that purpose. In hindsight, maybe there was a better way to implement this, but neither this patch nor this discussion belongs in this series. It distracts from discussion of interrupt support which is the sole purpose of the patch series.

The we-need-a-parent part is fine; but whatever we're doing with that driver just looks wrong, so that even the new bus that basically does nothing looks better...

Quoted 10 lines

Quoted 10 lines

...yes -(

Quoted 4 lines

Maybe that's what trips libudev? >
(And reading further in the current code, it seems we clear that structure _after_ the matrix device had been setup, so how can that even work? Where am I confused?)

Quoted 2 lines

Quoted 2 lines

On device_register there were no bus, so the core just do not look for a driver and this field was nor tested nor overwritten.

Hm... so has the callback in driver_for_each_device() in vfio_ap_verify_queue_reserved() ever been invoked at all? It seems this patch fixes more than just libudev issues...

Quoted 22 lines

Quoted 22 lines

Quoted 22 lines

Quoted 21 lines

Quoted 9 lines

Quoted 9 lines

ret = device_register(&matrix_dev->device); if (ret) goto matrix_reg_err;

Quoted 8 lines

Quoted 3 lines

+    ret = driver_register(&matrix_driver.drv); +    if (ret) +        goto
matrix_drv_err; +

As you already have several structures that can be registered exactly once (the root device, the bus, the driver, ...), you can already be sure that there's only one device on the bus, can't you?

hum, no I don't think so, no device can register before this module is loaded, but what does prevent a device to register later from another module?

Quoted 7 lines

Quoted 7 lines

Quoted 2 lines

Not unless you export the interface, I guess.

:) definitively right
thanks, this will simplify the code in the next version.
I will take the patch away from this series to get the way to stable as Christian requested.

Yeah, makes sense.

Quoted 13 lines · Cornelia Huck

Quoted 9 lines · On Thu, 14 Feb 2019 13:30:59 -0500 Tony Krowiak <[email protected]> wrote:

Quoted 8 lines · Pierre Morel

Quoted 3 lines · Cornelia Huck

(And reading further in the current code, it seems we clear that structure _after_ the matrix device had been setup, so how can that even work? Where am I confused?)

On device_register there were no bus, so the core just do not look for a driver and this field was nor tested nor overwritten.

Quoted 3 lines

Hm... so has the callback in driver_for_each_device() in vfio_ap_verify_queue_reserved() ever been invoked at all? It seems this patch fixes more than just libudev issues...

It is this patch that rendered the driver_for_each_device() in vfio_ap_verify_queue_reserved() erroneous. That function gets called every time an adapter or domain is assigned to the mdev. This patch introduced the problem with driver_for_each_device().

Quoted 18 lines · On Fri, 15 Feb 2019 16:59:33 -0500 Tony Krowiak <[email protected]> wrote:

Quoted 16 lines · Cornelia Huck

Quoted 9 lines · On Thu, 14 Feb 2019 13:30:59 -0500 Tony Krowiak <[email protected]> wrote:

Quoted 8 lines · Pierre Morel

Quoted 3 lines · Cornelia Huck

(And reading further in the current code, it seems we clear that structure _after_ the matrix device had been setup, so how can that even work? Where am I confused?)

On device_register there were no bus, so the core just do not look for a driver and this field was nor tested nor overwritten.

Hm... so has the callback in driver_for_each_device() in vfio_ap_verify_queue_reserved() ever been invoked at all? It seems this patch fixes more than just libudev issues...

Quoted 4 lines

It is this patch that rendered the driver_for_each_device() in vfio_ap_verify_queue_reserved() erroneous. That function gets called every time an adapter or domain is assigned to the mdev. This patch introduced the problem with driver_for_each_device().

So, does this function need to be removed or called from another place, then? (It looks like it was dead code before.)

Quoted 21 lines · Cornelia Huck

Quoted 18 lines · On Fri, 15 Feb 2019 16:59:33 -0500 Tony Krowiak <[email protected]> wrote:

Quoted 16 lines · Cornelia Huck

Quoted 9 lines · On Thu, 14 Feb 2019 13:30:59 -0500 Tony Krowiak <[email protected]> wrote:

Quoted 8 lines · Pierre Morel

Quoted 3 lines · Cornelia Huck

(And reading further in the current code, it seems we clear that structure _after_ the matrix device had been setup, so how can that even work? Where am I confused?)

On device_register there were no bus, so the core just do not look for a driver and this field was nor tested nor overwritten.

Hm... so has the callback in driver_for_each_device() in vfio_ap_verify_queue_reserved() ever been invoked at all? It seems this patch fixes more than just libudev issues...

Quoted 7 lines

Quoted 4 lines

It is this patch that rendered the driver_for_each_device() in vfio_ap_verify_queue_reserved() erroneous. That function gets called every time an adapter or domain is assigned to the mdev. This patch introduced the problem with driver_for_each_device().

So, does this function need to be removed or called from another place, then? (It looks like it was dead code before.)

I don't see why you think it's dead code:

assign_adapter_store
==> vfio_ap_mdev_verify_queues_reserved_for_apid

     ==> vfio_ap_verify_queue_reserved
         ==> driver_for_each_device  

The only way that the vfio_ap_verify_queue_reserved - the function that calls driver_for_each_device - does not get called is if no bits have yet been set in matrix_mdev->matrix.aqm.

Quoted 37 lines · On Mon, 18 Feb 2019 11:35:45 -0500 Tony Krowiak <[email protected]> wrote:

Quoted 21 lines · Cornelia Huck

Quoted 18 lines · On Fri, 15 Feb 2019 16:59:33 -0500 Tony Krowiak <[email protected]> wrote:

Quoted 16 lines · Cornelia Huck

Quoted 9 lines · On Thu, 14 Feb 2019 13:30:59 -0500 Tony Krowiak <[email protected]> wrote:

Quoted 8 lines · Pierre Morel

Quoted 3 lines · Cornelia Huck

(And reading further in the current code, it seems we clear that structure _after_ the matrix device had been setup, so how can that even work? Where am I confused?)

On device_register there were no bus, so the core just do not look for a driver and this field was nor tested nor overwritten.

Hm... so has the callback in driver_for_each_device() in vfio_ap_verify_queue_reserved() ever been invoked at all? It seems this patch fixes more than just libudev issues...

Quoted 7 lines

Quoted 4 lines

It is this patch that rendered the driver_for_each_device() in vfio_ap_verify_queue_reserved() erroneous. That function gets called every time an adapter or domain is assigned to the mdev. This patch introduced the problem with driver_for_each_device().

So, does this function need to be removed or called from another place, then? (It looks like it was dead code before.)

I don't see why you think it's dead code:

assign_adapter_store
==> vfio_ap_mdev_verify_queues_reserved_for_apid

      ==> vfio_ap_verify_queue_reserved
          ==> driver_for_each_device

Quoted 3 lines

The only way that the vfio_ap_verify_queue_reserved - the function that calls driver_for_each_device - does not get called is if no bits have yet been set in matrix_mdev->matrix.aqm.

What I don't see is how this can be called if no device has been, in fact, bound to the driver in the driver core...

Quoted 40 lines · Cornelia Huck

Quoted 37 lines · On Mon, 18 Feb 2019 11:35:45 -0500 Tony Krowiak <[email protected]> wrote:

Quoted 21 lines · Cornelia Huck

Quoted 18 lines · On Fri, 15 Feb 2019 16:59:33 -0500 Tony Krowiak <[email protected]> wrote:

Quoted 16 lines · Cornelia Huck

Quoted 9 lines · On Thu, 14 Feb 2019 13:30:59 -0500 Tony Krowiak <[email protected]> wrote:

Quoted 8 lines · Pierre Morel

Quoted 3 lines · Cornelia Huck

(And reading further in the current code, it seems we clear that structure _after_ the matrix device had been setup, so how can that even work? Where am I confused?)

On device_register there were no bus, so the core just do not look for a driver and this field was nor tested nor overwritten.

Hm... so has the callback in driver_for_each_device() in vfio_ap_verify_queue_reserved() ever been invoked at all? It seems this patch fixes more than just libudev issues...

Quoted 7 lines

Quoted 4 lines

It is this patch that rendered the driver_for_each_device() in vfio_ap_verify_queue_reserved() erroneous. That function gets called every time an adapter or domain is assigned to the mdev. This patch introduced the problem with driver_for_each_device().

So, does this function need to be removed or called from another place, then? (It looks like it was dead code before.)

I don't see why you think it's dead code:

assign_adapter_store
==> vfio_ap_mdev_verify_queues_reserved_for_apid

      ==> vfio_ap_verify_queue_reserved
          ==> driver_for_each_device  

Quoted 6 lines

Quoted 3 lines

The only way that the vfio_ap_verify_queue_reserved - the function that calls driver_for_each_device - does not get called is if no bits have yet been set in matrix_mdev->matrix.aqm.

What I don't see is how this can be called if no device has been, in fact, bound to the driver in the driver core...

Let's start with the fact that one can create an mdev device regardless of whether a queue has been bound to the vfio_ap driver. Once an mdev device is created, one can start assigning adapters, domains and control domains to it. Let's say the admin now attempts to assign an adapter, in which case the assign_adapter_store() function is invoked. After verifying that the APID passed in is a valid adapter number, the vfio_ap_mdev_verify_queues_reserved_for_apid() function is called. This function first checks if any domains have been assigned and if not, calls vfio_ap_verify_queue_reserved(&apid, NULL). It is in this function that the driver_for_each_device() function is called. Since there are no devices bound to the vfio_ap device driver, the callback passed in to the driver_for_each_device() function will never get called, so the vfio_ap_mdev_verify_queues_reserved_for_apid() function will return -EADDRNOTAVAIL. A similar flow will occur if the first assignment is for a domain. The bottom line is, the driver_for_each_device() function is called every time an adapter or domain is assigned.

Libudev relies on having a subsystem link for non-root devices. To avoid libudev (and potentially other userspace tools) choking on the matrix device let us introduce a vfio_ap bus and with that the vfio_ap bus subsytem, and make the matrix device reside within it.

We restrict the number of allowed devices to a single one.

Doing this we need to suppress the forced link from the matrix device to the vfio_ap driver and we suppress the device_type we do not need anymore.

Since the associated matrix driver is not the vfio_ap driver any more, we have to change the search for the devices on the vfio_ap driver in the function vfio_ap_verify_queue_reserved.

Signed-off-by Pierre Morel <[email protected]>

---

 drivers/s390/crypto/vfio_ap_drv.c     | 63 +++++++++++++++++++++++++++++++----
 drivers/s390/crypto/vfio_ap_ops.c     |  4 +--
 drivers/s390/crypto/vfio_ap_private.h |  1 +
 3 files changed, 60 insertions(+), 8 deletions(-)

drivers/s390/crypto/vfio_ap_drv.c +57 -6

index 31c6c84..1fd5fe6 100644
@@ -24,8 +24,9 @@ MODULE_LICENSE("GPL v2");
 
 static struct ap_driver vfio_ap_drv;
 
-static struct device_type vfio_ap_dev_type = {
-	.name = VFIO_AP_DEV_TYPE_NAME,
+struct matrix_driver {
+	struct device_driver drv;
+	int device_count;
 };
 
 struct ap_matrix_dev *matrix_dev;

@@ -62,6 +63,41 @@ static void vfio_ap_matrix_dev_release(struct device *dev)
 	kfree(matrix_dev);
 }
 
+static int matrix_bus_match(struct device *dev, struct device_driver *drv)
+{
+	return 1;
+}
+
+static struct bus_type matrix_bus = {
+	.name = "vfio_ap",
+	.match = &matrix_bus_match,
+};
+
+static int matrix_probe(struct device *dev);
+static int matrix_remove(struct device *dev);
+static struct matrix_driver matrix_driver = {
+	.drv = {
+		.name = "vfio_ap",
+		.bus = &matrix_bus,
+		.probe = matrix_probe,
+		.remove = matrix_remove,
+	},
+};
+
+static int matrix_probe(struct device *dev)
+{
+	if (matrix_driver.device_count)
+		return -EEXIST;
+	matrix_driver.device_count++;
+	return 0;
+}
+
+static int matrix_remove(struct device *dev)
+{
+	matrix_driver.device_count--;
+	return 0;
+}
+
 static int vfio_ap_matrix_dev_create(void)
 {
 	int ret;

@@ -71,6 +107,10 @@ static int vfio_ap_matrix_dev_create(void)
 	if (IS_ERR(root_device))
 		return PTR_ERR(root_device);
 
+	ret = bus_register(&matrix_bus);
+	if (ret)
+		goto bus_register_err;
+
 	matrix_dev = kzalloc(sizeof(*matrix_dev), GFP_KERNEL);
 	if (!matrix_dev) {
 		ret = -ENOMEM;

@@ -87,30 +127,41 @@ static int vfio_ap_matrix_dev_create(void)
 	mutex_init(&matrix_dev->lock);
 	INIT_LIST_HEAD(&matrix_dev->mdev_list);
 
-	matrix_dev->device.type = &vfio_ap_dev_type;
 	dev_set_name(&matrix_dev->device, "%s", VFIO_AP_DEV_NAME);
 	matrix_dev->device.parent = root_device;
+	matrix_dev->device.bus = &matrix_bus;
 	matrix_dev->device.release = vfio_ap_matrix_dev_release;
-	matrix_dev->device.driver = &vfio_ap_drv.driver;
+	matrix_dev->vfio_ap_drv = &vfio_ap_drv;
 
 	ret = device_register(&matrix_dev->device);
 	if (ret)
 		goto matrix_reg_err;
 
+	ret = driver_register(&matrix_driver.drv);
+	if (ret)
+		goto matrix_drv_err;
+
 	return 0;
 
+matrix_drv_err:
+	device_unregister(&matrix_dev->device);
 matrix_reg_err:
 	put_device(&matrix_dev->device);
 matrix_alloc_err:
+	bus_unregister(&matrix_bus);
+bus_register_err:
 	root_device_unregister(root_device);
-
 	return ret;
 }
 
 static void vfio_ap_matrix_dev_destroy(void)
 {
+	struct device *root_device = matrix_dev->device.parent;
+
+	driver_unregister(&matrix_driver.drv);
 	device_unregister(&matrix_dev->device);
-	root_device_unregister(matrix_dev->device.parent);
+	bus_unregister(&matrix_bus);
+	root_device_unregister(root_device);
 }
 
 static int __init vfio_ap_init(void)

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

index 272ef42..900b9cf 100644
@@ -198,8 +198,8 @@ static int vfio_ap_verify_queue_reserved(unsigned long *apid,
 	qres.apqi = apqi;
 	qres.reserved = false;
 
-	ret = driver_for_each_device(matrix_dev->device.driver, NULL, &qres,
-				     vfio_ap_has_queue);
+	ret = driver_for_each_device(&matrix_dev->vfio_ap_drv->driver, NULL,
+				     &qres, vfio_ap_has_queue);
 	if (ret)
 		return ret;
 

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

index 5675492..76b7f98 100644
@@ -40,6 +40,7 @@ struct ap_matrix_dev {
 	struct ap_config_info info;
 	struct list_head mdev_list;
 	struct mutex lock;
+	struct ap_driver  *vfio_ap_drv;
 };
 
 extern struct ap_matrix_dev *matrix_dev;

Quoted 11 lines · Pierre Morel

Libudev relies on having a subsystem link for non-root devices. To avoid libudev (and potentially other userspace tools) choking on the matrix device let us introduce a vfio_ap bus and with that the vfio_ap bus subsytem, and make the matrix device reside within it.

We restrict the number of allowed devices to a single one.

Doing this we need to suppress the forced link from the matrix device to the vfio_ap driver and we suppress the device_type we do not need anymore.

Quoted 5 lines

Since the associated matrix driver is not the vfio_ap driver any more, we have to change the search for the devices on the vfio_ap driver in the function vfio_ap_verify_queue_reserved.

Signed-off-by Pierre Morel <[email protected]>

Didnt you want to a add a reported-by?

Quoted 151 lines

---

  drivers/s390/crypto/vfio_ap_drv.c     | 63 +++++++++++++++++++++++++++++++----
  drivers/s390/crypto/vfio_ap_ops.c     |  4 +--
  drivers/s390/crypto/vfio_ap_private.h |  1 +
  3 files changed, 60 insertions(+), 8 deletions(-)

drivers/s390/crypto/vfio_ap_drv.c +57 -6

index 31c6c84..1fd5fe6 100644
@@ -24,8 +24,9 @@ MODULE_LICENSE("GPL v2");
 
  static struct ap_driver vfio_ap_drv;
 
-static struct device_type vfio_ap_dev_type = {
-	.name = VFIO_AP_DEV_TYPE_NAME,
+struct matrix_driver {
+	struct device_driver drv;
+	int device_count;
  };
 
  struct ap_matrix_dev *matrix_dev;

@@ -62,6 +63,41 @@ static void vfio_ap_matrix_dev_release(struct device *dev)
  	kfree(matrix_dev);
  }
 
+static int matrix_bus_match(struct device *dev, struct device_driver *drv)
+{
+	return 1;
+}
+
+static struct bus_type matrix_bus = {
+	.name = "vfio_ap",
+	.match = &matrix_bus_match,
+};
+
+static int matrix_probe(struct device *dev);
+static int matrix_remove(struct device *dev);
+static struct matrix_driver matrix_driver = {
+	.drv = {
+		.name = "vfio_ap",
+		.bus = &matrix_bus,
+		.probe = matrix_probe,
+		.remove = matrix_remove,
+	},
+};
+
+static int matrix_probe(struct device *dev)
+{
+	if (matrix_driver.device_count)
+		return -EEXIST;
+	matrix_driver.device_count++;
+	return 0;
+}
+
+static int matrix_remove(struct device *dev)
+{
+	matrix_driver.device_count--;
+	return 0;
+}
+
  static int vfio_ap_matrix_dev_create(void)
  {
  	int ret;

@@ -71,6 +107,10 @@ static int vfio_ap_matrix_dev_create(void)
  	if (IS_ERR(root_device))
  		return PTR_ERR(root_device);
 
+	ret = bus_register(&matrix_bus);
+	if (ret)
+		goto bus_register_err;
+
  	matrix_dev = kzalloc(sizeof(*matrix_dev), GFP_KERNEL);
  	if (!matrix_dev) {
  		ret = -ENOMEM;

@@ -87,30 +127,41 @@ static int vfio_ap_matrix_dev_create(void)
  	mutex_init(&matrix_dev->lock);
  	INIT_LIST_HEAD(&matrix_dev->mdev_list);
 
-	matrix_dev->device.type = &vfio_ap_dev_type;
  	dev_set_name(&matrix_dev->device, "%s", VFIO_AP_DEV_NAME);
  	matrix_dev->device.parent = root_device;
+	matrix_dev->device.bus = &matrix_bus;
  	matrix_dev->device.release = vfio_ap_matrix_dev_release;
-	matrix_dev->device.driver = &vfio_ap_drv.driver;
+	matrix_dev->vfio_ap_drv = &vfio_ap_drv;
 
  	ret = device_register(&matrix_dev->device);
  	if (ret)
  		goto matrix_reg_err;
 
+	ret = driver_register(&matrix_driver.drv);
+	if (ret)
+		goto matrix_drv_err;
+
  	return 0;
 
+matrix_drv_err:
+	device_unregister(&matrix_dev->device);
  matrix_reg_err:
  	put_device(&matrix_dev->device);
  matrix_alloc_err:
+	bus_unregister(&matrix_bus);
+bus_register_err:
  	root_device_unregister(root_device);
-
  	return ret;
  }
 
  static void vfio_ap_matrix_dev_destroy(void)
  {
+	struct device *root_device = matrix_dev->device.parent;
+
+	driver_unregister(&matrix_driver.drv);
  	device_unregister(&matrix_dev->device);
-	root_device_unregister(matrix_dev->device.parent);
+	bus_unregister(&matrix_bus);
+	root_device_unregister(root_device);
  }
 
  static int __init vfio_ap_init(void)

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

index 272ef42..900b9cf 100644
@@ -198,8 +198,8 @@ static int vfio_ap_verify_queue_reserved(unsigned long *apid,
  	qres.apqi = apqi;
  	qres.reserved = false;
 
-	ret = driver_for_each_device(matrix_dev->device.driver, NULL, &qres,
-				     vfio_ap_has_queue);
+	ret = driver_for_each_device(&matrix_dev->vfio_ap_drv->driver, NULL,
+				     &qres, vfio_ap_has_queue);
  	if (ret)
  		return ret;
 

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

index 5675492..76b7f98 100644
@@ -40,6 +40,7 @@ struct ap_matrix_dev {
  	struct ap_config_info info;
  	struct list_head mdev_list;
  	struct mutex lock;
+	struct ap_driver  *vfio_ap_drv;
  };
 
  extern struct ap_matrix_dev *matrix_dev;

This patch adds interception code for the PQAP instructions, and a callback inside the KVM arch structure for s390.

If a VFIO-AP drivers needs to intercept PQAP/AQIC or PQAP/TAPQ

instructions, the driver will initialize the callback inside the kvm_arch structure to be called when the interception of a PQAP instruction occurs.

If the callback is not initialized, the code still returns -EOPNOTSUPP to let userland handle the instruction as it used to.

Signed-off-by Pierre Morel <[email protected]>

Reviewed-by Tony Krowiak <[email protected]>

---
 arch/s390/include/asm/kvm_host.h |  1 +
 arch/s390/kvm/priv.c             | 50 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+)

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

index c5f5156..49cc8b0 100644
@@ -719,6 +719,7 @@ struct kvm_s390_cpu_model {
 
 struct kvm_s390_crypto {
 	struct kvm_s390_crypto_cb *crycb;
+	int (*pqap_hook)(struct kvm_vcpu *vcpu);
 	__u32 crycbd;
 	__u8 aes_kw;
 	__u8 dea_kw;

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

index 8679bd7..72fdc21 100644
@@ -27,6 +27,7 @@
 #include <asm/io.h>
 #include <asm/ptrace.h>
 #include <asm/sclp.h>
+#include <asm/ap.h>
 #include "gaccess.h"
 #include "kvm-s390.h"
 #include "trace.h"

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

+ * - AP instructions are not available or not available to the guest
+ * - the instruction is not PQAP with function code indicating
+ *   AQIC do not change the previous behavior.
+ *
+ * For PQAP/AQIC instruction, verify privilege and specifications
+ *
+ * return the value returned by the callback.
+ */
+static int handle_pqap(struct kvm_vcpu *vcpu)
+{
+	uint8_t fc;
+
+	/* Verify that the hook callback is registered */
+	if (!vcpu->kvm->arch.crypto.pqap_hook)
+		return -EOPNOTSUPP;
+	/* Verify that the AP instruction are available */
+	if (!ap_instructions_available())
+		return -EOPNOTSUPP;
+	/* Verify that the guest is allowed to use AP instructions */
+	if (!(vcpu->arch.sie_block->eca & ECA_APIE))
+		return -EOPNOTSUPP;
+	/* Verify that the function code is AQIC */
+	fc = vcpu->run->s.regs.gprs[0] >> 24;
+	if (fc != 0x03)

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

+}
+
 static int handle_stfl(struct kvm_vcpu *vcpu)
 {
 	int rc;

@@ -878,6 +926,8 @@ int kvm_s390_handle_b2(struct kvm_vcpu *vcpu)
 		return handle_sthyi(vcpu);
 	case 0x7d:
 		return handle_stsi(vcpu);
+	case 0xaf:
+		return handle_pqap(vcpu);
 	case 0xb1:
 		return handle_stfl(vcpu);
 	case 0xb2:

Quoted 4 lines · On Thu, 14 Feb 2019 14:51:02 +0100 Pierre Morel <[email protected]> wrote:

This patch adds interception code for the PQAP instructions, and a callback inside the KVM arch structure for s390.

If a VFIO-AP drivers needs to intercept PQAP/AQIC or PQAP/TAPQ

s/drivers/driver/

arch/s390/kvm/priv.c static int handle_io_inst(struct kvm_vcpu *vcpu) · line 598

Quoted 9 lines

  #include "kvm-s390.h"
  #include "trace.h"

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

@@ -592,6 +593,53 @@ static int handle_io_inst(struct kvm_vcpu *vcpu)
  	}
  }
 
+/*
+ * handle_pqap: Handling pqap interception
+ * @vcpu: the vcpu having issue the pqap instruction

s/issue/issued/

arch/s390/kvm/priv.c static int handle_io_inst(struct kvm_vcpu *vcpu) · line 600

Quoted 3 lines

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

+ *
+ * This callback only handles PQAP/AQIC instruction and

Here you only talk about PQAP/AQIC, what about PQAP/TAPQ mentioned in the patch description?

arch/s390/kvm/priv.c static int handle_io_inst(struct kvm_vcpu *vcpu) · line 606

Quoted 7 lines

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

+ * calls a dedicated callback for this instruction if
+ * a driver did register one in the CRYPTO satellite of the
+ * SIE block.
+ *
+ * Do not change the behavior if, return -EOPNOTSUPP if:
+ * - the hook is not used do not change the behavior.

The hook is not used? Or not set?

(I don't think you need to repeat "do
not change the behavior".)

Quoted 30 lines

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

+ * - AP instructions are not available or not available to the guest
+ * - the instruction is not PQAP with function code indicating
+ *   AQIC do not change the previous behavior.
+ *
+ * For PQAP/AQIC instruction, verify privilege and specifications
+ *
+ * return the value returned by the callback.
+ */
+static int handle_pqap(struct kvm_vcpu *vcpu)
+{
+	uint8_t fc;
+
+	/* Verify that the hook callback is registered */
+	if (!vcpu->kvm->arch.crypto.pqap_hook)
+		return -EOPNOTSUPP;
+	/* Verify that the AP instruction are available */
+	if (!ap_instructions_available())
+		return -EOPNOTSUPP;
+	/* Verify that the guest is allowed to use AP instructions */
+	if (!(vcpu->arch.sie_block->eca & ECA_APIE))
+		return -EOPNOTSUPP;
+	/* Verify that the function code is AQIC */
+	fc = vcpu->run->s.regs.gprs[0] >> 24;
+	if (fc != 0x03)
+		return -EOPNOTSUPP;
+
+	/* PQAP instructions are allowed for guest kernel only */
+	if (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PSTATE)
+		return kvm_s390_inject_program_int(vcpu, PGM_PRIVILEGED_OP);

arch/s390/kvm/priv.c static int handle_io_inst(struct kvm_vcpu *vcpu) · line 640

Quoted 5 lines

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

+	/* AQIC instruction is allowed only if facility 65 is available */
+	if (!test_kvm_facility(vcpu->kvm, 65))
+		return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
+	/* All right, call the callback */
+	return vcpu->kvm->arch.crypto.pqap_hook(vcpu);

Can that callback also return -EOPNOTSUPP to order to drop to user space?

Quoted 15 lines

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

+}
+
  static int handle_stfl(struct kvm_vcpu *vcpu)
  {
  	int rc;

@@ -878,6 +926,8 @@ int kvm_s390_handle_b2(struct kvm_vcpu *vcpu)
  		return handle_sthyi(vcpu);
  	case 0x7d:
  		return handle_stsi(vcpu);
+	case 0xaf:
+		return handle_pqap(vcpu);
  	case 0xb1:
  		return handle_stfl(vcpu);
  	case 0xb2:

Quoted 9 lines · Cornelia Huck

Quoted 4 lines · On Thu, 14 Feb 2019 14:51:02 +0100 Pierre Morel <[email protected]> wrote:

This patch adds interception code for the PQAP instructions, and a callback inside the KVM arch structure for s390.

If a VFIO-AP drivers needs to intercept PQAP/AQIC or PQAP/TAPQ

s/drivers/driver/

thanks. OK

Quoted 5 lines

Quoted 2 lines

+ *
+ * This callback only handles PQAP/AQIC instruction and

Here you only talk about PQAP/AQIC, what about PQAP/TAPQ mentioned in the patch description?

I can add "for now" or "in this patch" or suppress the reference to PAPQ/TAPQ

Quoted 9 lines

Quoted 8 lines

+ * calls a dedicated callback for this instruction if
+ * a driver did register one in the CRYPTO satellite of the
+ * SIE block.
+ *
+ * Do not change the behavior if, return -EOPNOTSUPP if:
+ * - the hook is not used do not change the behavior.

Quoted 1 line

The hook is not used? Or not set?

I think "is not set" is better.

Quoted 3 lines

(I don't think you need to repeat "do
not change the behavior".)

OK

Quoted 30 lines

Quoted 30 lines

+ * - AP instructions are not available or not available to the guest
+ * - the instruction is not PQAP with function code indicating
+ * AQIC do not change the previous behavior.
+ *
+ * For PQAP/AQIC instruction, verify privilege and specifications
+ *
+ * return the value returned by the callback.
+ */

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

+static int handle_pqap(struct kvm_vcpu *vcpu)
+{
+	uint8_t fc;
+
+	/* Verify that the hook callback is registered */
+	if (!vcpu->kvm->arch.crypto.pqap_hook)
+		return -EOPNOTSUPP;
+	/* Verify that the AP instruction are available */
+	if (!ap_instructions_available())
+		return -EOPNOTSUPP;
+	/* Verify that the guest is allowed to use AP instructions */
+	if (!(vcpu->arch.sie_block->eca & ECA_APIE))
+		return -EOPNOTSUPP;
+	/* Verify that the function code is AQIC */
+	fc = vcpu->run->s.regs.gprs[0] >> 24;
+	if (fc != 0x03)
+		return -EOPNOTSUPP;
+
+	/* PQAP instructions are allowed for guest kernel only */
+	if (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PSTATE)
+		return kvm_s390_inject_program_int(vcpu, PGM_PRIVILEGED_OP);

Quoted 8 lines

arch/s390/kvm/priv.c static int handle_io_inst(struct kvm_vcpu *vcpu) · line 640

Quoted 5 lines

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

+	/* AQIC instruction is allowed only if facility 65 is available */
+	if (!test_kvm_facility(vcpu->kvm, 65))
+		return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
+	/* All right, call the callback */
+	return vcpu->kvm->arch.crypto.pqap_hook(vcpu);  

Can that callback also return -EOPNOTSUPP to order to drop to user space?

Yes.
Why not?

Quoted 20 lines · On Thu, 14 Feb 2019 17:45:06 +0100 Pierre Morel <[email protected]> wrote:

Quoted 9 lines · Cornelia Huck

Quoted 4 lines · On Thu, 14 Feb 2019 14:51:02 +0100 Pierre Morel <[email protected]> wrote:

This patch adds interception code for the PQAP instructions, and a callback inside the KVM arch structure for s390.

If a VFIO-AP drivers needs to intercept PQAP/AQIC or PQAP/TAPQ

s/drivers/driver/

thanks. OK

Quoted 5 lines

Quoted 2 lines

+ *
+ * This callback only handles PQAP/AQIC instruction and

Here you only talk about PQAP/AQIC, what about PQAP/TAPQ mentioned in the patch description?

Quoted 2 lines

I can add "for now" or "in this patch" or suppress the reference to PAPQ/TAPQ

I'd just add a note to the patch description that this patch only handles PQAP/AQCI and that handling PQAP/TAPQ is something for a follow-on patch.

Quoted 3 lines

Quoted 3 lines

Quoted 12 lines

Quoted 11 lines

arch/s390/kvm/priv.c static int handle_io_inst(struct kvm_vcpu *vcpu) · line 640

Quoted 8 lines

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

+	/* PQAP instructions are allowed for guest kernel only */
+	if (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PSTATE)
+		return kvm_s390_inject_program_int(vcpu, PGM_PRIVILEGED_OP);
+	/* AQIC instruction is allowed only if facility 65 is available */
+	if (!test_kvm_facility(vcpu->kvm, 65))
+		return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
+	/* All right, call the callback */
+	return vcpu->kvm->arch.crypto.pqap_hook(vcpu);

Can that callback also return -EOPNOTSUPP to order to drop to user space?

Quoted 2 lines

Yes.
Why not?

Maybe also mention that in the function description?

This patch adds interception code for the PQAP instructions, and a callback inside the KVM arch structure for s390.

If a VFIO-AP drivers needs to intercept PQAP/AQIC or PQAP/TAPQ instructions, the driver will initialize the callback inside the kvm_arch structure to be called when the interception of a PQAP instruction occurs.

If the callback is not initialized, the code still returns -EOPNOTSUPP to let userland handle the instruction as it used to.

Signed-off-by Pierre Morel <[email protected]>

Reviewed-by Tony Krowiak <[email protected]>

---
  arch/s390/include/asm/kvm_host.h |  1 +
  arch/s390/kvm/priv.c             | 50 ++++++++++++++++++++++++++++++++++++++++
  2 files changed, 51 insertions(+)

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

index c5f5156..49cc8b0 100644
@@ -719,6 +719,7 @@ struct kvm_s390_cpu_model {
 
  struct kvm_s390_crypto {
  	struct kvm_s390_crypto_cb *crycb;
+	int (*pqap_hook)(struct kvm_vcpu *vcpu);
  	__u32 crycbd;
  	__u8 aes_kw;
  	__u8 dea_kw;

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

index 8679bd7..72fdc21 100644
@@ -27,6 +27,7 @@
  #include <asm/io.h>
  #include <asm/ptrace.h>
  #include <asm/sclp.h>
+#include <asm/ap.h>
  #include "gaccess.h"
  #include "kvm-s390.h"
  #include "trace.h"

@@ -592,6 +593,53 @@ static int handle_io_inst(struct kvm_vcpu *vcpu)
  	}
  }
 
+/*
+ * handle_pqap: Handling pqap interception
+ * @vcpu: the vcpu having issue the pqap instruction
+ *
+ * This callback only handles PQAP/AQIC instruction and
+ * calls a dedicated callback for this instruction if
+ * a driver did register one in the CRYPTO satellite of the
+ * SIE block.
+ *
+ * Do not change the behavior if, return -EOPNOTSUPP if:
+ * - the hook is not used do not change the behavior.
+ * - AP instructions are not available or not available to the guest
+ * - the instruction is not PQAP with function code indicating
+ *   AQIC do not change the previous behavior.
+ *
+ * For PQAP/AQIC instruction, verify privilege and specifications
+ *
+ * return the value returned by the callback.
+ */
+static int handle_pqap(struct kvm_vcpu *vcpu)
+{
+	uint8_t fc;
+
+	/* Verify that the hook callback is registered */
+	if (!vcpu->kvm->arch.crypto.pqap_hook)
+		return -EOPNOTSUPP;

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

+
+	/* PQAP instructions are allowed for guest kernel only */
+	if (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PSTATE)
+		return kvm_s390_inject_program_int(vcpu, PGM_PRIVILEGED_OP);
+	/* AQIC instruction is allowed only if facility 65 is available */
+	if (!test_kvm_facility(vcpu->kvm, 65))
+		return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
+	/* All right, call the callback */
+	return vcpu->kvm->arch.crypto.pqap_hook(vcpu);
+}
+
  static int handle_stfl(struct kvm_vcpu *vcpu)
  {
  	int rc;

@@ -878,6 +926,8 @@ int kvm_s390_handle_b2(struct kvm_vcpu *vcpu)
  		return handle_sthyi(vcpu);
  	case 0x7d:
  		return handle_stsi(vcpu);
+	case 0xaf:
+		return handle_pqap(vcpu);
  	case 0xb1:
  		return handle_stfl(vcpu);
  	case 0xb2:

Quoted 77 lines · Pierre Morel

This patch adds interception code for the PQAP instructions, and a callback inside the KVM arch structure for s390.

If a VFIO-AP drivers needs to intercept PQAP/AQIC or PQAP/TAPQ instructions, the driver will initialize the callback inside the kvm_arch structure to be called when the interception of a PQAP instruction occurs.

If the callback is not initialized, the code still returns -EOPNOTSUPP to let userland handle the instruction as it used to.

Signed-off-by Pierre Morel <[email protected]>

Reviewed-by Tony Krowiak <[email protected]>

---
  arch/s390/include/asm/kvm_host.h |  1 +
  arch/s390/kvm/priv.c             | 50
++++++++++++++++++++++++++++++++++++++++

  2 files changed, 51 insertions(+)

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

index c5f5156..49cc8b0 100644
@@ -719,6 +719,7 @@ struct kvm_s390_cpu_model {
   struct kvm_s390_crypto {
       struct kvm_s390_crypto_cb *crycb;
+    int (*pqap_hook)(struct kvm_vcpu *vcpu);
       __u32 crycbd;
       __u8 aes_kw;
       __u8 dea_kw;

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

index 8679bd7..72fdc21 100644
@@ -27,6 +27,7 @@
   #include <asm/io.h>
   #include <asm/ptrace.h>
   #include <asm/sclp.h>
+#include <asm/ap.h>
   #include "gaccess.h"
   #include "kvm-s390.h"
   #include "trace.h"

@@ -592,6 +593,53 @@ static int handle_io_inst(struct kvm_vcpu *vcpu)
       }
   }
+/*
+ * handle_pqap: Handling pqap interception
+ * @vcpu: the vcpu having issue the pqap instruction
+ *
+ * This callback only handles PQAP/AQIC instruction and
+ * calls a dedicated callback for this instruction if
+ * a driver did register one in the CRYPTO satellite of the
+ * SIE block.
+ *
+ * Do not change the behavior if, return -EOPNOTSUPP if:
+ * - the hook is not used do not change the behavior.
+ * - AP instructions are not available or not available to the guest
+ * - the instruction is not PQAP with function code indicating
+ *   AQIC do not change the previous behavior.
+ *
+ * For PQAP/AQIC instruction, verify privilege and specifications
+ *
+ * return the value returned by the callback.
+ */
+static int handle_pqap(struct kvm_vcpu *vcpu)
+{
+    uint8_t fc;
+
+    /* Verify that the hook callback is registered */
+    if (!vcpu->kvm->arch.crypto.pqap_hook)
+        return -EOPNOTSUPP;
+    /* Verify that the AP instruction are available */
+    if (!ap_instructions_available())
+        return -EOPNOTSUPP;
+    /* Verify that the guest is allowed to use AP instructions */
+    if (!(vcpu->arch.sie_block->eca & ECA_APIE))
+        return -EOPNOTSUPP;

arch/s390/kvm/priv.c static int handle_io_inst(struct kvm_vcpu *vcpu) · line 631

Quoted 4 lines

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

+    /* Verify that the function code is AQIC */
+    fc = vcpu->run->s.regs.gprs[0] >> 24;
+    if (fc != 0x03)
+        return -EOPNOTSUPP;

This does not belong here. Function code 3 is one of 7 function codes that can be sent with the PQAP instruction. This belongs in the PQAP hook code.

Quoted 25 lines

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

+
+    /* PQAP instructions are allowed for guest kernel only */
+    if (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PSTATE)
+        return kvm_s390_inject_program_intkvm_s390_inject_program_int(vcpu, PGM_PRIVILEGED_OP);
+    /* AQIC instruction is allowed only if facility 65 is available */
+    if (!test_kvm_facility(vcpu->kvm, 65))
+        return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
+    /* All right, call the callback */
+    return vcpu->kvm->arch.crypto.pqap_hook(vcpu);
+}
+
   static int handle_stfl(struct kvm_vcpu *vcpu)
   {
       int rc;

@@ -878,6 +926,8 @@ int kvm_s390_handle_b2(struct kvm_vcpu *vcpu)
           return handle_sthyi(vcpu);

      case 0x7d:
          return handle_stsi(vcpu);
+    case 0xaf:
+        return handle_pqap(vcpu);
      case 0xb1:
          return handle_stfl(vcpu);
      case 0xb2:

Quoted 34 lines · Tony Krowiak

Quoted 32 lines · Pierre Morel

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

+/*
+ * handle_pqap: Handling pqap interception
+ * @vcpu: the vcpu having issue the pqap instruction
+ *
+ * This callback only handles PQAP/AQIC instruction and
+ * calls a dedicated callback for this instruction if
+ * a driver did register one in the CRYPTO satellite of the
+ * SIE block.
+ *
+ * Do not change the behavior if, return -EOPNOTSUPP if:
+ * - the hook is not used do not change the behavior.
+ * - AP instructions are not available or not available to the guest
+ * - the instruction is not PQAP with function code indicating
+ *   AQIC do not change the previous behavior.
+ *
+ * For PQAP/AQIC instruction, verify privilege and specifications
+ *
+ * return the value returned by the callback.
+ */
+static int handle_pqap(struct kvm_vcpu *vcpu)
+{
+    uint8_t fc;
+
+    /* Verify that the hook callback is registered */
+    if (!vcpu->kvm->arch.crypto.pqap_hook)
+        return -EOPNOTSUPP;
+    /* Verify that the AP instruction are available */
+    if (!ap_instructions_available())
+        return -EOPNOTSUPP;
+    /* Verify that the guest is allowed to use AP instructions */
+    if (!(vcpu->arch.sie_block->eca & ECA_APIE))
+        return -EOPNOTSUPP;

Quoted 8 lines

Quoted 4 lines

+    /* Verify that the function code is AQIC */
+    fc = vcpu->run->s.regs.gprs[0] >> 24;
+    if (fc != 0x03)
+        return -EOPNOTSUPP;

This does not belong here. Function code 3 is one of 7 function codes that can be sent with the PQAP instruction. This belongs in the PQAP hook code.

On one hand, effectively I would prefer to put the code in the VFIO driver code.
On the other hand, doing this would lead to export the code for test_kvm_facility() and kvm_s390_inject_program_int() from the kvm-s390.h

I choose not to export these functions from the KVM code.

Would like opinion from KVM maintainers?

Quoted 27 lines

Quoted 26 lines

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

+
+    /* PQAP instructions are allowed for guest kernel only */
+    if (vcpu->arch.sie_block->gpsw.mask & PSW_MASK_PSTATE)
+        return kvm_s390_inject_program_intkvm_s390_inject_program_int(vcpu, PGM_PRIVILEGED_OP);
+    /* AQIC instruction is allowed only if facility 65 is available */
+    if (!test_kvm_facility(vcpu->kvm, 65))
+        return kvm_s390_inject_program_int(vcpu, PGM_SPECIFICATION);
+    /* All right, call the callback */
+    return vcpu->kvm->arch.crypto.pqap_hook(vcpu);
+}
+

  static int handle_stfl(struct kvm_vcpu *vcpu)
  {
      int rc;

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

@@ -878,6 +926,8 @@ int kvm_s390_handle_b2(struct kvm_vcpu *vcpu)
           return handle_sthyi(vcpu);

      case 0x7d:
          return handle_stsi(vcpu);
+    case 0xaf:
+        return handle_pqap(vcpu);
      case 0xb1:
          return handle_stfl(vcpu);
      case 0xb2:

Quoted 51 lines · On Mon, 18 Feb 2019 19:29:10 +0100 Pierre Morel <[email protected]> wrote:

Quoted 34 lines · Tony Krowiak

Quoted 32 lines · Pierre Morel

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

+/*
+ * handle_pqap: Handling pqap interception
+ * @vcpu: the vcpu having issue the pqap instruction
+ *
+ * This callback only handles PQAP/AQIC instruction and
+ * calls a dedicated callback for this instruction if
+ * a driver did register one in the CRYPTO satellite of the
+ * SIE block.
+ *
+ * Do not change the behavior if, return -EOPNOTSUPP if:
+ * - the hook is not used do not change the behavior.
+ * - AP instructions are not available or not available to the guest
+ * - the instruction is not PQAP with function code indicating
+ *   AQIC do not change the previous behavior.
+ *
+ * For PQAP/AQIC instruction, verify privilege and specifications
+ *
+ * return the value returned by the callback.
+ */
+static int handle_pqap(struct kvm_vcpu *vcpu)
+{
+    uint8_t fc;
+
+    /* Verify that the hook callback is registered */
+    if (!vcpu->kvm->arch.crypto.pqap_hook)
+        return -EOPNOTSUPP;
+    /* Verify that the AP instruction are available */
+    if (!ap_instructions_available())
+        return -EOPNOTSUPP;
+    /* Verify that the guest is allowed to use AP instructions */
+    if (!(vcpu->arch.sie_block->eca & ECA_APIE))
+        return -EOPNOTSUPP;

Quoted 8 lines

Quoted 4 lines

+    /* Verify that the function code is AQIC */
+    fc = vcpu->run->s.regs.gprs[0] >> 24;
+    if (fc != 0x03)
+        return -EOPNOTSUPP;

This does not belong here. Function code 3 is one of 7 function codes that can be sent with the PQAP instruction. This belongs in the PQAP hook code.

On one hand, effectively I would prefer to put the code in the VFIO driver code.
On the other hand, doing this would lead to export the code for test_kvm_facility() and kvm_s390_inject_program_int() from the kvm-s390.h

I choose not to export these functions from the KVM code.

Quoted 1 line

Would like opinion from KVM maintainers?

Looking at this (and without access to the specification...), I think the check for problem state makes sense in here (if this applies to all PQAP functions equally, which seems likely). The check for the facility makes more sense in the handler. You can probably still inject the specification exception here if you use a clever return code.

Another option: Provide a way to register a callback per function code; this allows you to still do the check here and extend it later for other function codes (which will probably be indicated by another facility).

Quoted 56 lines · Cornelia Huck

Quoted 52 lines · On Mon, 18 Feb 2019 19:29:10 +0100 Pierre Morel <[email protected]> wrote:

Quoted 34 lines · Tony Krowiak

Quoted 32 lines · Pierre Morel

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

+/*
+ * handle_pqap: Handling pqap interception
+ * @vcpu: the vcpu having issue the pqap instruction
+ *
+ * This callback only handles PQAP/AQIC instruction and
+ * calls a dedicated callback for this instruction if
+ * a driver did register one in the CRYPTO satellite of the
+ * SIE block.
+ *
+ * Do not change the behavior if, return -EOPNOTSUPP if:
+ * - the hook is not used do not change the behavior.
+ * - AP instructions are not available or not available to the guest
+ * - the instruction is not PQAP with function code indicating
+ *   AQIC do not change the previous behavior.
+ *
+ * For PQAP/AQIC instruction, verify privilege and specifications
+ *
+ * return the value returned by the callback.
+ */
+static int handle_pqap(struct kvm_vcpu *vcpu)
+{
+    uint8_t fc;
+
+    /* Verify that the hook callback is registered */
+    if (!vcpu->kvm->arch.crypto.pqap_hook)
+        return -EOPNOTSUPP;
+    /* Verify that the AP instruction are available */
+    if (!ap_instructions_available())
+        return -EOPNOTSUPP;
+    /* Verify that the guest is allowed to use AP instructions */
+    if (!(vcpu->arch.sie_block->eca & ECA_APIE))
+        return -EOPNOTSUPP;

Quoted 8 lines

Quoted 4 lines

+    /* Verify that the function code is AQIC */
+    fc = vcpu->run->s.regs.gprs[0] >> 24;
+    if (fc != 0x03)
+        return -EOPNOTSUPP;

This does not belong here. Function code 3 is one of 7 function codes that can be sent with the PQAP instruction. This belongs in the PQAP hook code.

On one hand, effectively I would prefer to put the code in the VFIO driver code.
On the other hand, doing this would lead to export the code for test_kvm_facility() and kvm_s390_inject_program_int() from the kvm-s390.h

I choose not to export these functions from the KVM code.

Would like opinion from KVM maintainers?

Quoted 6 lines

Looking at this (and without access to the specification...), I think the check for problem state makes sense in here (if this applies to all PQAP functions equally, which seems likely). The check for the facility makes more sense in the handler. You can probably still inject the specification exception here if you use a clever return code.

If there is no objection on exporting the KVM functions... I can do this.

Quoted 5 lines

Another option: Provide a way to register a callback per function code; this allows you to still do the check here and extend it later for other function codes (which will probably be indicated by another facility).

I like this option even better.

Regards,
Pierre

Quoted 64 lines · Pierre Morel

Quoted 62 lines · Cornelia Huck

Quoted 44 lines · On Mon, 18 Feb 2019 19:29:10 +0100 Pierre Morel <[email protected]> wrote:

Quoted 34 lines · Tony Krowiak

Quoted 32 lines · Pierre Morel

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

+/*
+ * handle_pqap: Handling pqap interception
+ * @vcpu: the vcpu having issue the pqap instruction
+ *
+ * This callback only handles PQAP/AQIC instruction and
+ * calls a dedicated callback for this instruction if
+ * a driver did register one in the CRYPTO satellite of the
+ * SIE block.
+ *
+ * Do not change the behavior if, return -EOPNOTSUPP if:
+ * - the hook is not used do not change the behavior.
+ * - AP instructions are not available or not available to the guest
+ * - the instruction is not PQAP with function code indicating
+ *   AQIC do not change the previous behavior.
+ *
+ * For PQAP/AQIC instruction, verify privilege and specifications
+ *
+ * return the value returned by the callback.
+ */
+static int handle_pqap(struct kvm_vcpu *vcpu)
+{
+    uint8_t fc;
+
+    /* Verify that the hook callback is registered */
+    if (!vcpu->kvm->arch.crypto.pqap_hook)
+        return -EOPNOTSUPP;
+    /* Verify that the AP instruction are available */
+    if (!ap_instructions_available())
+        return -EOPNOTSUPP;
+    /* Verify that the guest is allowed to use AP instructions */
+    if (!(vcpu->arch.sie_block->eca & ECA_APIE))
+        return -EOPNOTSUPP;

Quoted 8 lines

Quoted 4 lines

+    /* Verify that the function code is AQIC */
+    fc = vcpu->run->s.regs.gprs[0] >> 24;
+    if (fc != 0x03)
+        return -EOPNOTSUPP;

This does not belong here. Function code 3 is one of 7 function codes that can be sent with the PQAP instruction. This belongs in the PQAP hook code.

Quoted 8 lines

On one hand, effectively I would prefer to put the code in the VFIO driver code.
On the other hand, doing this would lead to export the code for test_kvm_facility() and kvm_s390_inject_program_int() from the kvm-s390.h

I choose not to export these functions from the KVM code.

Would like opinion from KVM maintainers?

Looking at this (and without access to the specification...), I think the check for problem state makes sense in here (if this applies to all PQAP functions equally, which seems likely). The check for the facility makes more sense in the handler. You can probably still inject the specification exception here if you use a clever return code.

Quoted 1 line

If there is no objection on exporting the KVM functions... I can do this.

I do not understand why you would have to export KVM functions to place the check for FC 0x03 in the pqap hook? What am I missing here? Maybe you misunderstood my comment?

Quoted 56 lines · Cornelia Huck

Quoted 52 lines · On Mon, 18 Feb 2019 19:29:10 +0100 Pierre Morel <[email protected]> wrote:

Quoted 34 lines · Tony Krowiak

Quoted 32 lines · Pierre Morel

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

+/*
+ * handle_pqap: Handling pqap interception
+ * @vcpu: the vcpu having issue the pqap instruction
+ *
+ * This callback only handles PQAP/AQIC instruction and
+ * calls a dedicated callback for this instruction if
+ * a driver did register one in the CRYPTO satellite of the
+ * SIE block.
+ *
+ * Do not change the behavior if, return -EOPNOTSUPP if:
+ * - the hook is not used do not change the behavior.
+ * - AP instructions are not available or not available to the guest
+ * - the instruction is not PQAP with function code indicating
+ *   AQIC do not change the previous behavior.
+ *
+ * For PQAP/AQIC instruction, verify privilege and specifications
+ *
+ * return the value returned by the callback.
+ */
+static int handle_pqap(struct kvm_vcpu *vcpu)
+{
+    uint8_t fc;
+
+    /* Verify that the hook callback is registered */
+    if (!vcpu->kvm->arch.crypto.pqap_hook)
+        return -EOPNOTSUPP;
+    /* Verify that the AP instruction are available */
+    if (!ap_instructions_available())
+        return -EOPNOTSUPP;
+    /* Verify that the guest is allowed to use AP instructions */
+    if (!(vcpu->arch.sie_block->eca & ECA_APIE))
+        return -EOPNOTSUPP;

Quoted 8 lines

Quoted 4 lines

+    /* Verify that the function code is AQIC */
+    fc = vcpu->run->s.regs.gprs[0] >> 24;
+    if (fc != 0x03)
+        return -EOPNOTSUPP;

This does not belong here. Function code 3 is one of 7 function codes that can be sent with the PQAP instruction. This belongs in the PQAP hook code.

On one hand, effectively I would prefer to put the code in the VFIO driver code.
On the other hand, doing this would lead to export the code for test_kvm_facility() and kvm_s390_inject_program_int() from the kvm-s390.h

I choose not to export these functions from the KVM code.

Would like opinion from KVM maintainers?

Quoted 6 lines

Looking at this (and without access to the specification...), I think the check for problem state makes sense in here (if this applies to all PQAP functions equally, which seems likely). The check for the facility makes more sense in the handler. You can probably still inject the specification exception here if you use a clever return code.

If there is no objection on exporting the KVM functions... I can do this.

Quoted 5 lines

Another option: Provide a way to register a callback per function code; this allows you to still do the check here and extend it later for other function codes (which will probably be indicated by another facility).

I like this option even better.

Regards,
Pierre

The AP interruptions are assigned on a queue basis and we need some information neloging to the queue

to handle interuptions.

We add a new structure vfio_ap_queue, to hold per
queue information useful to handle interruptions.

- apqn: AP queue number (defined here)
- isc : Interrupt subclass (defined later)
- nib : notification information byte (defined later)

The vfio_ap_queue structure is allocated when the vfio_ap_driver is probed and added as driver data to the ap_queue device. It is free on remove.

The structure is linked to the matrix_dev host device.

Signed-off-by Pierre Morel <[email protected]>

Reviewed-by Tony Krowiak <[email protected]>

---
 drivers/s390/crypto/vfio_ap_drv.c     | 23 ++++++++++++++++++++++-
 drivers/s390/crypto/vfio_ap_private.h |  3 +++
 2 files changed, 25 insertions(+), 1 deletion(-)

Quoted 2 lines · On Thu, 14 Feb 2019 14:51:03 +0100 Pierre Morel <[email protected]> wrote:

The AP interruptions are assigned on a queue basis and we need some information neloging to the queue

s/neloging/belonging/

Quoted 1 line

to handle interuptions.

s/interuptions/interruptions/

Quoted 7 lines

We add a new structure vfio_ap_queue, to hold per
queue information useful to handle interruptions.

- apqn: AP queue number (defined here)
- isc : Interrupt subclass (defined later)
- nib : notification information byte (defined later)

So, you basically simply want quick access to these? Makes sense.

Quoted 4 lines

The vfio_ap_queue structure is allocated when the vfio_ap_driver is probed and added as driver data to the ap_queue device. It is free on remove.

s/free/freed/

Quoted 2 lines

The structure is linked to the matrix_dev host device.

Hm, I don't quite follow?

Quoted 8 lines

Signed-off-by Pierre Morel <[email protected]>

Reviewed-by Tony Krowiak <[email protected]>

---
  drivers/s390/crypto/vfio_ap_drv.c     | 23 ++++++++++++++++++++++-
  drivers/s390/crypto/vfio_ap_private.h |  3 +++
  2 files changed, 25 insertions(+), 1 deletion(-)

Patch looks reasonable; might make sense to squash it with later patches (still need to read).

We need to find the queue with a specific APQN during the handling of the interception of the PQAP/AQIC instruction.

To handle the AP associated device reference count we keep track of it in the vfio_ap_queue until we put the device.

Signed-off-by Pierre Morel <[email protected]>

---
 drivers/s390/crypto/vfio_ap_ops.c     | 54 +++++++++++++++++++++++++++++++++++
 drivers/s390/crypto/vfio_ap_private.h |  1 +
 2 files changed, 55 insertions(+)

drivers/s390/crypto/vfio_ap_ops.c +17 -0

index 900b9cf..2a52c9b 100644
@@ -24,6 +24,60 @@
 #define VFIO_AP_MDEV_TYPE_HWVIRT "passthrough"
 #define VFIO_AP_MDEV_NAME_HWVIRT "VFIO AP Passthrough Device"
 
+/**
+ * vfio_ap_check_apqn: check if a ap_queue is of a given APQN
+ *
+ * Returns 1 if we have a match.
+ * Otherwise returns 0.
+ */
+static int vfio_ap_check_apqn(struct device *dev, void *data)
+{
+	struct vfio_ap_queue *q = dev_get_drvdata(dev);
+
+	return (q->apqn == *(int *)data);
+}
+
+/**
+ * vfio_ap_get_queue: Retrieve a queue with a specific APQN
+ * @apqn: The queue APQN
+ *

drivers/s390/crypto/vfio_ap_ops.c +27 -0

+	struct vfio_ap_queue *vfio_ap_get_queue(int apqn)
+{
+	struct device *dev;
+	struct vfio_ap_queue *q;
+
+	dev = driver_find_device(&matrix_dev->vfio_ap_drv->driver, NULL, &apqn,
+				 vfio_ap_check_apqn);
+	if (!dev)
+		return NULL;
+	q = dev_get_drvdata(dev);
+	q->dev = dev;
+	return q;
+}
+
+/**
+ * vfio_ap_put_queue: lower device reference count for a queue
+ * @q: The queue
+ *
+ * put the associated device
+ *
+ */
+static  __attribute__((unused)) void vfio_ap_put_queue(struct vfio_ap_queue *q)
+{
+	put_device(q->dev);
+	q->dev = NULL;
+}
+
 static void vfio_ap_matrix_init(struct ap_config_info *info,
 				struct ap_matrix *matrix)
 {

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

index 8836f01..081f0d7 100644
@@ -87,6 +87,7 @@ extern int vfio_ap_mdev_register(void);
 extern void vfio_ap_mdev_unregister(void);
 
 struct vfio_ap_queue {
+	struct device *dev;
 	int	apqn;
 };
 #endif /* _VFIO_AP_PRIVATE_H_ */

Quoted 3 lines · On Thu, 14 Feb 2019 14:51:04 +0100 Pierre Morel <[email protected]> wrote:

We need to find the queue with a specific APQN during the handling of the interception of the PQAP/AQIC instruction.

Quoted 2 lines

To handle the AP associated device reference count we keep track of it in the vfio_ap_queue until we put the device.

So, the relationship is
(struct ap_device)--(driver_data)-->(struct vfio_ap_queue)--(pointer)-->(struct ap_device) ? IOW, a backlink?

If so, can't you already set that up during probe?

Or am I confused by the various similar devices again? Maybe a diagram would help...

Quoted 37 lines

Signed-off-by Pierre Morel <[email protected]>

---
  drivers/s390/crypto/vfio_ap_ops.c     | 54 +++++++++++++++++++++++++++++++++++
  drivers/s390/crypto/vfio_ap_private.h |  1 +
  2 files changed, 55 insertions(+)

drivers/s390/crypto/vfio_ap_ops.c +21 -0

index 900b9cf..2a52c9b 100644
@@ -24,6 +24,60 @@
  #define VFIO_AP_MDEV_TYPE_HWVIRT "passthrough"
  #define VFIO_AP_MDEV_NAME_HWVIRT "VFIO AP Passthrough Device"
 
+/**
+ * vfio_ap_check_apqn: check if a ap_queue is of a given APQN
+ *
+ * Returns 1 if we have a match.
+ * Otherwise returns 0.
+ */
+static int vfio_ap_check_apqn(struct device *dev, void *data)
+{
+	struct vfio_ap_queue *q = dev_get_drvdata(dev);
+
+	return (q->apqn == *(int *)data);
+}
+
+/**
+ * vfio_ap_get_queue: Retrieve a queue with a specific APQN
+ * @apqn: The queue APQN
+ *
+ * Retrieve a queue with a specific APQN from the list of the
+ * devices associated to the vfio_ap_driver.
+ *
+ * The vfio_ap_queue has been already associated with the device

drivers/s390/crypto/vfio_ap_ops.c line 53

Quoted 6 lines

drivers/s390/crypto/vfio_ap_ops.c +6 -0

+ * during the probe.
+ * Store the associated device for reference counting
+ *
+ * Returns the pointer to the associated vfio_ap_queue
+ */
+static  __attribute__((unused))

Eww. Can you get rid of that by reordering or squashing patches?

Quoted 6 lines · Cornelia Huck

Quoted 3 lines · On Thu, 14 Feb 2019 14:51:04 +0100 Pierre Morel <[email protected]> wrote:

We need to find the queue with a specific APQN during the handling of the interception of the PQAP/AQIC instruction.

Quoted 8 lines

Quoted 2 lines

To handle the AP associated device reference count we keep track of it in the vfio_ap_queue until we put the device.

So, the relationship is
(struct ap_device)--(driver_data)-->(struct vfio_ap_queue)--(pointer)-->(struct ap_device) ? IOW, a backlink?

If so, can't you already set that up during probe?

Will do.

Quoted 3 lines

Or am I confused by the various similar devices again? Maybe a diagram would help...

No you are right.

Quoted 39 lines

Quoted 39 lines

Signed-off-by Pierre Morel <[email protected]>

---
  drivers/s390/crypto/vfio_ap_ops.c     | 54 +++++++++++++++++++++++++++++++++++
  drivers/s390/crypto/vfio_ap_private.h |  1 +
  2 files changed, 55 insertions(+)

drivers/s390/crypto/vfio_ap_ops.c +21 -0

index 900b9cf..2a52c9b 100644
@@ -24,6 +24,60 @@
  #define VFIO_AP_MDEV_TYPE_HWVIRT "passthrough"
  #define VFIO_AP_MDEV_NAME_HWVIRT "VFIO AP Passthrough Device"
 
+/**
+ * vfio_ap_check_apqn: check if a ap_queue is of a given APQN
+ *
+ * Returns 1 if we have a match.
+ * Otherwise returns 0.
+ */
+static int vfio_ap_check_apqn(struct device *dev, void *data)
+{
+	struct vfio_ap_queue *q = dev_get_drvdata(dev);
+
+	return (q->apqn == *(int *)data);
+}
+
+/**
+ * vfio_ap_get_queue: Retrieve a queue with a specific APQN
+ * @apqn: The queue APQN
+ *
+ * Retrieve a queue with a specific APQN from the list of the
+ * devices associated to the vfio_ap_driver.
+ *
+ * The vfio_ap_queue has been already associated with the device

Quoted 8 lines

drivers/s390/crypto/vfio_ap_ops.c line 53

Quoted 6 lines

drivers/s390/crypto/vfio_ap_ops.c +6 -0

+ * during the probe.
+ * Store the associated device for reference counting
+ *
+ * Returns the pointer to the associated vfio_ap_queue
+ */
+static  __attribute__((unused))  

Eww. Can you get rid of that by reordering or squashing patches?

I did this to avoid posting a very big patch.
I will of course squash 4 and 5 with patch 6, when the two patches 4 and 5 are reviewed.

If you think it brings more clarity to squash all for the next iteration I will do.

We need to find the queue with a specific APQN during the handling of the interception of the PQAP/AQIC instruction.

To handle the AP associated device reference count we keep track of it in the vfio_ap_queue until we put the device.

Signed-off-by Pierre Morel <[email protected]>

---
  drivers/s390/crypto/vfio_ap_ops.c     | 54 +++++++++++++++++++++++++++++++++++
  drivers/s390/crypto/vfio_ap_private.h |  1 +
  2 files changed, 55 insertions(+)

drivers/s390/crypto/vfio_ap_ops.c +18 -0

index 900b9cf..2a52c9b 100644
@@ -24,6 +24,60 @@
  #define VFIO_AP_MDEV_TYPE_HWVIRT "passthrough"
  #define VFIO_AP_MDEV_NAME_HWVIRT "VFIO AP Passthrough Device"
 
+/**
+ * vfio_ap_check_apqn: check if a ap_queue is of a given APQN
+ *
+ * Returns 1 if we have a match.
+ * Otherwise returns 0.
+ */
+static int vfio_ap_check_apqn(struct device *dev, void *data)
+{
+	struct vfio_ap_queue *q = dev_get_drvdata(dev);
+
+	return (q->apqn == *(int *)data);
+}
+
+/**
+ * vfio_ap_get_queue: Retrieve a queue with a specific APQN
+ * @apqn: The queue APQN
+ *
+ * Retrieve a queue with a specific APQN from the list of the

drivers/s390/crypto/vfio_ap_ops.c +5 -0

+	return q;
+}
+
+/**
+ * vfio_ap_put_queue: lower device reference count for a queue

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

  	int	apqn;
  };
  #endif /* _VFIO_AP_PRIVATE_H_ */

Quoted 17 lines · Pierre Morel

We need to find the queue with a specific APQN during the handling of the interception of the PQAP/AQIC instruction.

To handle the AP associated device reference count we keep track of it in the vfio_ap_queue until we put the device.

Signed-off-by Pierre Morel <[email protected]>

---
  drivers/s390/crypto/vfio_ap_ops.c     | 54 +++++++++++++++++++++++++++++++++++
  drivers/s390/crypto/vfio_ap_private.h |  1 +
  2 files changed, 55 insertions(+)

drivers/s390/crypto/vfio_ap_ops.c +5 -0

+/**
+ * vfio_ap_get_queue: Retrieve a queue with a specific APQN
+ * @apqn: The queue APQN
+ *
+ * Retrieve a queue with a specific APQN from the list of the

drivers/s390/crypto/vfio_ap_ops.c line 54

Quoted 10 lines

drivers/s390/crypto/vfio_ap_ops.c +10 -0

+ * devices associated to the vfio_ap_driver.
+ *
+ * The vfio_ap_queue has been already associated with the device
+ * during the probe.
+ * Store the associated device for reference counting
+ *
+ * Returns the pointer to the associated vfio_ap_queue
+ */
+static  __attribute__((unused))
+	struct vfio_ap_queue *vfio_ap_get_queue(int apqn)  

I think you should change this signature for the reasons I stated below:

struct device *vfio_ap_get_queue_dev(int apqn)

drivers/s390/crypto/vfio_ap_ops.c line 64

Quoted 10 lines

drivers/s390/crypto/vfio_ap_ops.c +10 -0

+{
+	struct device *dev;
+	struct vfio_ap_queue *q;
+
+	dev = driver_find_device(&matrix_dev->vfio_ap_drv->driver, NULL, &apqn,
+				 vfio_ap_check_apqn);
+	if (!dev)
+		return NULL;
+	q = dev_get_drvdata(dev);
+	q->dev = dev;  

Why store the device with the vfio_ap_queue object? Why not just return the device. The caller can get the vfio_ap_queue from the device's driver data. It seems the only reason for the 'dev' field is to temporarily hold a ref to the device so it can be put later. Why not just put the device.

Quoted 34 lines · On Fri, 15 Feb 2019 17:13:21 -0500 Tony Krowiak <[email protected]> wrote:

Quoted 17 lines · Pierre Morel

We need to find the queue with a specific APQN during the handling of the interception of the PQAP/AQIC instruction.

To handle the AP associated device reference count we keep track of it in the vfio_ap_queue until we put the device.

Signed-off-by Pierre Morel <[email protected]>

---
   drivers/s390/crypto/vfio_ap_ops.c     | 54 +++++++++++++++++++++++++++++++++++
   drivers/s390/crypto/vfio_ap_private.h |  1 +
   2 files changed, 55 insertions(+)

drivers/s390/crypto/vfio_ap_ops.c +5 -0

+/**
+ * vfio_ap_get_queue: Retrieve a queue with a specific APQN
+ * @apqn: The queue APQN
+ *
+ * Retrieve a queue with a specific APQN from the list of the

drivers/s390/crypto/vfio_ap_ops.c line 54

Quoted 10 lines

drivers/s390/crypto/vfio_ap_ops.c +10 -0

+ * devices associated to the vfio_ap_driver.
+ *
+ * The vfio_ap_queue has been already associated with the device
+ * during the probe.
+ * Store the associated device for reference counting
+ *
+ * Returns the pointer to the associated vfio_ap_queue
+ */
+static  __attribute__((unused))
+	struct vfio_ap_queue *vfio_ap_get_queue(int apqn)

I think you should change this signature for the reasons I stated below:

struct device *vfio_ap_get_queue_dev(int apqn)

Quoted 16 lines

drivers/s390/crypto/vfio_ap_ops.c line 64

Quoted 10 lines

drivers/s390/crypto/vfio_ap_ops.c +10 -0

+{
+	struct device *dev;
+	struct vfio_ap_queue *q;
+
+	dev = driver_find_device(&matrix_dev->vfio_ap_drv->driver, NULL, &apqn,
+				 vfio_ap_check_apqn);
+	if (!dev)
+		return NULL;
+	q = dev_get_drvdata(dev);
+	q->dev = dev;

Why store the device with the vfio_ap_queue object? Why not just return the device. The caller can get the vfio_ap_queue from the device's driver data. It seems the only reason for the 'dev' field is to temporarily hold a ref to the device so it can be put later. Why not just put the device.