[PATCH v1 0/7] vfio-ccw parent rework

[PATCH v1 0/7] vfio-ccw parent rework

[Eric Farman]

Hi all,

There have been discussions and attempts [1][2] to rework the vfio-ccw device
lifecycle to better align with the needs/expectations of vfio and mdev.
While those languished, commit cb9ff3f3b84c ("vfio: Add helpers for unifying
vfio_device life cycle") implemented a couple of tricks that help vfio and the
drivers that interact with it, while keeping vfio-ccw as-is. A handful of
commits titled "vfio/xxx: Use the new device life cycle helpers" implemented
those tricks for each of the drivers.

This series attempts to address the oddities/shortcomings of vfio-ccw, such
that vfio-ccw can use the same helpers as everyone else, and the tricks that
were implemented by the other drivers can be removed. It is built on 6.1-rc1,
and thus includes the various changes [3][4] that have occurred in and around
these parts.

Patches 1-5 rework the behavior of the vfio-ccw driver's private struct.
In summary, the mdev pieces are split out of vfio_ccw_private and into a
new vfio_ccw_parent struct that will continue to follow today's lifecycle.
The remainder (bulk) of the private struct moves to follow the mdev
probe/remove pair. There's opportunity for further separation of the
things in the private struct, which would simplify some of the vfio-ccw
code, but it got too hairy as I started that. Once vfio-ccw is no longer
considered unique, those cleanups can happen at our leisure. 

Patch 6 removes the trickery where vfio-ccw uses vfio_init_device instead of
vfio_alloc_device, and thus removes vfio_init_device from the outside world.

Patch 7 removes vfio_free_device from vfio-ccw and the other drivers (hello,
CC list!), letting it be handled by vfio_device_release directly.

Looking forward to the feedback.

Thanks,
Eric

[1] https://lore.kernel.org/kvm/0-v3-57c1502c62fd+2190-ccw_mdev_jgg@nvidia.com/
[2] https://lore.kernel.org/kvm/20220602171948.2790690-1-farman@linux.ibm.com/
[3] https://lore.kernel.org/kvm/20220923092652.100656-1-hch@lst.de/
[4] https://lore.kernel.org/kvm/20220921104401.38898-1-kevin.tian@intel.com/

Eric Farman (7):
  vfio/ccw: create a parent struct
  vfio/ccw: remove private->sch
  vfio/ccw: move private initialization to callback
  vfio/ccw: move private to mdev lifecycle
  vfio/ccw: remove release completion
  vfio/ccw: replace vfio_init_device with _alloc_
  vfio: Remove vfio_free_device

 drivers/gpu/drm/i915/gvt/kvmgt.c      |   1 -
 drivers/s390/cio/vfio_ccw_chp.c       |   5 +-
 drivers/s390/cio/vfio_ccw_drv.c       | 182 ++++++++++----------------
 drivers/s390/cio/vfio_ccw_fsm.c       |  27 ++--
 drivers/s390/cio/vfio_ccw_ops.c       | 108 ++++++++++-----
 drivers/s390/cio/vfio_ccw_parent.h    |  28 ++++
 drivers/s390/cio/vfio_ccw_private.h   |  22 ++--
 drivers/s390/crypto/vfio_ap_ops.c     |   6 -
 drivers/vfio/fsl-mc/vfio_fsl_mc.c     |   1 -
 drivers/vfio/pci/vfio_pci_core.c      |   1 -
 drivers/vfio/platform/vfio_amba.c     |   1 -
 drivers/vfio/platform/vfio_platform.c |   1 -
 drivers/vfio/vfio_main.c              |  25 +---
 include/linux/vfio.h                  |   1 -
 samples/vfio-mdev/mbochs.c            |   1 -
 samples/vfio-mdev/mdpy.c              |   1 -
 samples/vfio-mdev/mtty.c              |   1 -
 17 files changed, 204 insertions(+), 208 deletions(-)
 create mode 100644 drivers/s390/cio/vfio_ccw_parent.h

-- 
2.34.1

[PATCH v1 1/7] vfio/ccw: create a parent struct

[Eric Farman]

Move the stuff associated with the mdev parent (and thus the
subchannel struct) into its own struct, and leave the rest in
the existing private structure.

The subchannel will point to the parent, and the parent will point
to the private, for the areas where one or both are needed. Further
separation of these structs will follow.

Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_drv.c     | 104 ++++++++++++++++++++--------
 drivers/s390/cio/vfio_ccw_ops.c     |   9 ++-
 drivers/s390/cio/vfio_ccw_parent.h  |  28 ++++++++
 drivers/s390/cio/vfio_ccw_private.h |   5 --
 4 files changed, 112 insertions(+), 34 deletions(-)
 create mode 100644 drivers/s390/cio/vfio_ccw_parent.h

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 7f5402fe857a..634760ca0dea 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -20,6 +20,7 @@
 #include "chp.h"
 #include "ioasm.h"
 #include "css.h"
+#include "vfio_ccw_parent.h"
 #include "vfio_ccw_private.h"
 
 struct workqueue_struct *vfio_ccw_work_q;
@@ -36,7 +37,8 @@ debug_info_t *vfio_ccw_debug_trace_id;
  */
 int vfio_ccw_sch_quiesce(struct subchannel *sch)
 {
-	struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev);
+	struct vfio_ccw_parent *parent = dev_get_drvdata(&sch->dev);
+	struct vfio_ccw_private *private = dev_get_drvdata(&parent->dev);
 	DECLARE_COMPLETION_ONSTACK(completion);
 	int iretry, ret = 0;
 
@@ -51,19 +53,21 @@ int vfio_ccw_sch_quiesce(struct subchannel *sch)
 			break;
 		}
 
-		/*
-		 * Flush all I/O and wait for
-		 * cancel/halt/clear completion.
-		 */
-		private->completion = &completion;
-		spin_unlock_irq(sch->lock);
+		if (private) {
+			/*
+			 * Flush all I/O and wait for
+			 * cancel/halt/clear completion.
+			 */
+			private->completion = &completion;
+			spin_unlock_irq(sch->lock);
 
-		if (ret == -EBUSY)
-			wait_for_completion_timeout(&completion, 3*HZ);
+			if (ret == -EBUSY)
+				wait_for_completion_timeout(&completion, 3*HZ);
 
-		private->completion = NULL;
-		flush_workqueue(vfio_ccw_work_q);
-		spin_lock_irq(sch->lock);
+			private->completion = NULL;
+			flush_workqueue(vfio_ccw_work_q);
+			spin_lock_irq(sch->lock);
+		}
 		ret = cio_disable_subchannel(sch);
 	} while (ret == -EBUSY);
 
@@ -121,7 +125,22 @@ static void vfio_ccw_crw_todo(struct work_struct *work)
  */
 static void vfio_ccw_sch_irq(struct subchannel *sch)
 {
-	struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev);
+	struct vfio_ccw_parent *parent = dev_get_drvdata(&sch->dev);
+	struct vfio_ccw_private *private = dev_get_drvdata(&parent->dev);
+
+	/*
+	 * The subchannel should still be disabled at this point,
+	 * so an interrupt would be quite surprising. As with an
+	 * interrupt while the FSM is closed, let's attempt to
+	 * disable the subchannel again.
+	 */
+	if (!private) {
+		VFIO_CCW_MSG_EVENT(2, "sch %x.%x.%04x: unexpected interrupt\n",
+			sch->schid.cssid, sch->schid.ssid, sch->schid.sch_no);
+
+		cio_disable_subchannel(sch);
+		return;
+	}
 
 	inc_irq_stat(IRQIO_CIO);
 	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_INTERRUPT);
@@ -201,10 +220,19 @@ static void vfio_ccw_free_private(struct vfio_ccw_private *private)
 	mutex_destroy(&private->io_mutex);
 	kfree(private);
 }
+
+static void vfio_ccw_free_parent(struct device *dev)
+{
+	struct vfio_ccw_parent *parent = container_of(dev, struct vfio_ccw_parent, dev);
+
+	kfree(parent);
+}
+
 static int vfio_ccw_sch_probe(struct subchannel *sch)
 {
 	struct pmcw *pmcw = &sch->schib.pmcw;
 	struct vfio_ccw_private *private;
+	struct vfio_ccw_parent *parent;
 	int ret = -ENOMEM;
 
 	if (pmcw->qf) {
@@ -213,18 +241,28 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
 		return -ENODEV;
 	}
 
+	parent = kzalloc(sizeof(*parent), GFP_KERNEL);
+	if (IS_ERR(parent))
+		return PTR_ERR(parent);
+
+	parent->dev.release = &vfio_ccw_free_parent;
+	device_initialize(&parent->dev);
+
 	private = vfio_ccw_alloc_private(sch);
-	if (IS_ERR(private))
+	if (IS_ERR(private)) {
+		put_device(&parent->dev);
 		return PTR_ERR(private);
+	}
 
-	dev_set_drvdata(&sch->dev, private);
+	dev_set_drvdata(&sch->dev, parent);
+	dev_set_drvdata(&parent->dev, private);
 
-	private->mdev_type.sysfs_name = "io";
-	private->mdev_type.pretty_name = "I/O subchannel (Non-QDIO)";
-	private->mdev_types[0] = &private->mdev_type;
-	ret = mdev_register_parent(&private->parent, &sch->dev,
+	parent->mdev_type.sysfs_name = "io";
+	parent->mdev_type.pretty_name = "I/O subchannel (Non-QDIO)";
+	parent->mdev_types[0] = &parent->mdev_type;
+	ret = mdev_register_parent(&parent->parent, &sch->dev,
 				   &vfio_ccw_mdev_driver,
-				   private->mdev_types, 1);
+				   parent->mdev_types, 1);
 	if (ret)
 		goto out_free;
 
@@ -234,20 +272,24 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
 	return 0;
 
 out_free:
+	dev_set_drvdata(&parent->dev, NULL);
 	dev_set_drvdata(&sch->dev, NULL);
 	vfio_ccw_free_private(private);
+	put_device(&parent->dev);
 	return ret;
 }
 
 static void vfio_ccw_sch_remove(struct subchannel *sch)
 {
-	struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev);
+	struct vfio_ccw_parent *parent = dev_get_drvdata(&sch->dev);
+	struct vfio_ccw_private *private = dev_get_drvdata(&parent->dev);
 
-	mdev_unregister_parent(&private->parent);
+	mdev_unregister_parent(&parent->parent);
 
 	dev_set_drvdata(&sch->dev, NULL);
 
 	vfio_ccw_free_private(private);
+	put_device(&parent->dev);
 
 	VFIO_CCW_MSG_EVENT(4, "unbound from subchannel %x.%x.%04x\n",
 			   sch->schid.cssid, sch->schid.ssid,
@@ -256,7 +298,11 @@ static void vfio_ccw_sch_remove(struct subchannel *sch)
 
 static void vfio_ccw_sch_shutdown(struct subchannel *sch)
 {
-	struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev);
+	struct vfio_ccw_parent *parent = dev_get_drvdata(&sch->dev);
+	struct vfio_ccw_private *private = dev_get_drvdata(&parent->dev);
+
+	if (WARN_ON(!private))
+		return;
 
 	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_CLOSE);
 	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_NOT_OPER);
@@ -274,7 +320,8 @@ static void vfio_ccw_sch_shutdown(struct subchannel *sch)
  */
 static int vfio_ccw_sch_event(struct subchannel *sch, int process)
 {
-	struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev);
+	struct vfio_ccw_parent *parent = dev_get_drvdata(&sch->dev);
+	struct vfio_ccw_private *private = dev_get_drvdata(&parent->dev);
 	unsigned long flags;
 	int rc = -EAGAIN;
 
@@ -287,8 +334,10 @@ static int vfio_ccw_sch_event(struct subchannel *sch, int process)
 
 	rc = 0;
 
-	if (cio_update_schib(sch))
-		vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_NOT_OPER);
+	if (cio_update_schib(sch)) {
+		if (private)
+			vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_NOT_OPER);
+	}
 
 out_unlock:
 	spin_unlock_irqrestore(sch->lock, flags);
@@ -326,7 +375,8 @@ static void vfio_ccw_queue_crw(struct vfio_ccw_private *private,
 static int vfio_ccw_chp_event(struct subchannel *sch,
 			      struct chp_link *link, int event)
 {
-	struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev);
+	struct vfio_ccw_parent *parent = dev_get_drvdata(&sch->dev);
+	struct vfio_ccw_private *private = dev_get_drvdata(&parent->dev);
 	int mask = chp_ssd_get_mask(&sch->ssd_info, link);
 	int retry = 255;
 
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index 6ae4d012d800..b1cd89d900ab 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -14,6 +14,7 @@
 #include <linux/nospec.h>
 #include <linux/slab.h>
 
+#include "vfio_ccw_parent.h"
 #include "vfio_ccw_private.h"
 
 static const struct vfio_device_ops vfio_ccw_dev_ops;
@@ -55,7 +56,9 @@ static int vfio_ccw_mdev_init_dev(struct vfio_device *vdev)
 
 static int vfio_ccw_mdev_probe(struct mdev_device *mdev)
 {
-	struct vfio_ccw_private *private = dev_get_drvdata(mdev->dev.parent);
+	struct subchannel *sch = to_subchannel(mdev->dev.parent);
+	struct vfio_ccw_parent *parent = dev_get_drvdata(&sch->dev);
+	struct vfio_ccw_private *private = dev_get_drvdata(&parent->dev);
 	int ret;
 
 	if (private->state == VFIO_CCW_STATE_NOT_OPER)
@@ -100,7 +103,9 @@ static void vfio_ccw_mdev_release_dev(struct vfio_device *vdev)
 
 static void vfio_ccw_mdev_remove(struct mdev_device *mdev)
 {
-	struct vfio_ccw_private *private = dev_get_drvdata(mdev->dev.parent);
+	struct subchannel *sch = to_subchannel(mdev->dev.parent);
+	struct vfio_ccw_parent *parent = dev_get_drvdata(&sch->dev);
+	struct vfio_ccw_private *private = dev_get_drvdata(&parent->dev);
 
 	VFIO_CCW_MSG_EVENT(2, "sch %x.%x.%04x: remove\n",
 			   private->sch->schid.cssid,
diff --git a/drivers/s390/cio/vfio_ccw_parent.h b/drivers/s390/cio/vfio_ccw_parent.h
new file mode 100644
index 000000000000..834c00077802
--- /dev/null
+++ b/drivers/s390/cio/vfio_ccw_parent.h
@@ -0,0 +1,28 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * MDEV Parent contents for vfio_ccw driver
+ *
+ * Copyright IBM Corp. 2022
+ */
+
+#ifndef _VFIO_CCW_PARENT_H_
+#define _VFIO_CCW_PARENT_H_
+
+#include <linux/mdev.h>
+
+/**
+ * struct vfio_ccw_parent
+ *
+ * @dev: embedded device struct
+ * @parent: parent data structures for mdevs created
+ * @mdev_type(s): identifying information for mdevs created
+ */
+struct vfio_ccw_parent {
+	struct device		dev;
+
+	struct mdev_parent	parent;
+	struct mdev_type	mdev_type;
+	struct mdev_type	*mdev_types[1];
+};
+
+#endif
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index bd5fb81456af..673e9a81a172 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -89,7 +89,6 @@ struct vfio_ccw_crw {
  * @io_work: work for deferral process of I/O handling
  * @crw_work: work for deferral process of CRW handling
  * @release_comp: synchronization helper for vfio device release
- * @parent: parent data structures for mdevs created
  */
 struct vfio_ccw_private {
 	struct vfio_device vdev;
@@ -116,10 +115,6 @@ struct vfio_ccw_private {
 	struct work_struct	crw_work;
 
 	struct completion	release_comp;
-
-	struct mdev_parent	parent;
-	struct mdev_type	mdev_type;
-	struct mdev_type	*mdev_types[1];
 } __aligned(8);
 
 int vfio_ccw_sch_quiesce(struct subchannel *sch);
-- 
2.34.1

Re: [PATCH v1 1/7] vfio/ccw: create a parent struct

[Eric Farman]

On Wed, 2022-10-19 at 18:21 +0200, Eric Farman wrote:
> Move the stuff associated with the mdev parent (and thus the
> subchannel struct) into its own struct, and leave the rest in
> the existing private structure.
> 
> The subchannel will point to the parent, and the parent will point
> to the private, for the areas where one or both are needed. Further
> separation of these structs will follow.
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>  drivers/s390/cio/vfio_ccw_drv.c     | 104 ++++++++++++++++++++------
> --
>  drivers/s390/cio/vfio_ccw_ops.c     |   9 ++-
>  drivers/s390/cio/vfio_ccw_parent.h  |  28 ++++++++
>  drivers/s390/cio/vfio_ccw_private.h |   5 --
>  4 files changed, 112 insertions(+), 34 deletions(-)
>  create mode 100644 drivers/s390/cio/vfio_ccw_parent.h
> 
> diff --git a/drivers/s390/cio/vfio_ccw_drv.c
> b/drivers/s390/cio/vfio_ccw_drv.c
> index 7f5402fe857a..634760ca0dea 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c

...snip...

> @@ -213,18 +241,28 @@ static int vfio_ccw_sch_probe(struct subchannel
> *sch)
>                 return -ENODEV;
>         }
>  
> +       parent = kzalloc(sizeof(*parent), GFP_KERNEL);
> +       if (IS_ERR(parent))
> +               return PTR_ERR(parent);
> +
> +       parent->dev.release = &vfio_ccw_free_parent;
> +       device_initialize(&parent->dev);
> +

Oops. This should either be device_register, or I needed a device_add
somewhere along the way. There's no need to separate them, so the
former is probably better, but I still need some additional logic to
link this back to the css driver.

>         private = vfio_ccw_alloc_private(sch);
> -       if (IS_ERR(private))
> +       if (IS_ERR(private)) {
> +               put_device(&parent->dev);
>                 return PTR_ERR(private);
> +       }
>  
> -       dev_set_drvdata(&sch->dev, private);
> +       dev_set_drvdata(&sch->dev, parent);
> +       dev_set_drvdata(&parent->dev, private);
>  
> -       private->mdev_type.sysfs_name = "io";
> -       private->mdev_type.pretty_name = "I/O subchannel (Non-QDIO)";
> -       private->mdev_types[0] = &private->mdev_type;
> -       ret = mdev_register_parent(&private->parent, &sch->dev,
> +       parent->mdev_type.sysfs_name = "io";
> +       parent->mdev_type.pretty_name = "I/O subchannel (Non-QDIO)";
> +       parent->mdev_types[0] = &parent->mdev_type;
> +       ret = mdev_register_parent(&parent->parent, &sch->dev,
>                                    &vfio_ccw_mdev_driver,
> -                                  private->mdev_types, 1);
> +                                  parent->mdev_types, 1);
>         if (ret)
>                 goto out_free;
>  
> @@ -234,20 +272,24 @@ static int vfio_ccw_sch_probe(struct subchannel
> *sch)
>         return 0;
>  
>  out_free:
> +       dev_set_drvdata(&parent->dev, NULL);
>         dev_set_drvdata(&sch->dev, NULL);
>         vfio_ccw_free_private(private);
> +       put_device(&parent->dev);
>         return ret;
>  }
>  
>  static void vfio_ccw_sch_remove(struct subchannel *sch)
>  {
> -       struct vfio_ccw_private *private = dev_get_drvdata(&sch-
> >dev);
> +       struct vfio_ccw_parent *parent = dev_get_drvdata(&sch->dev);
> +       struct vfio_ccw_private *private = dev_get_drvdata(&parent-
> >dev);
>  
> -       mdev_unregister_parent(&private->parent);
> +       mdev_unregister_parent(&parent->parent);
>  
>         dev_set_drvdata(&sch->dev, NULL);
>  
>         vfio_ccw_free_private(private);
> +       put_device(&parent->dev);
>  
>         VFIO_CCW_MSG_EVENT(4, "unbound from subchannel %x.%x.%04x\n",
>                            sch->schid.cssid, sch->schid.ssid,
> @@ -256,7 +298,11 @@ static void vfio_ccw_sch_remove(struct
> subchannel *sch)
>  
>  static void vfio_ccw_sch_shutdown(struct subchannel *sch)
>  {
> -       struct vfio_ccw_private *private = dev_get_drvdata(&sch-
> >dev);
> +       struct vfio_ccw_parent *parent = dev_get_drvdata(&sch->dev);
> +       struct vfio_ccw_private *private = dev_get_drvdata(&parent-
> >dev);
> +
> +       if (WARN_ON(!private))
> +               return;
>  
>         vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_CLOSE);
>         vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_NOT_OPER);
> @@ -274,7 +320,8 @@ static void vfio_ccw_sch_shutdown(struct
> subchannel *sch)
>   */
>  static int vfio_ccw_sch_event(struct subchannel *sch, int process)
>  {
> -       struct vfio_ccw_private *private = dev_get_drvdata(&sch-
> >dev);
> +       struct vfio_ccw_parent *parent = dev_get_drvdata(&sch->dev);
> +       struct vfio_ccw_private *private = dev_get_drvdata(&parent-
> >dev);
>         unsigned long flags;
>         int rc = -EAGAIN;
>  
> @@ -287,8 +334,10 @@ static int vfio_ccw_sch_event(struct subchannel
> *sch, int process)
>  
>         rc = 0;
>  
> -       if (cio_update_schib(sch))
> -               vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_NOT_OPER);
> +       if (cio_update_schib(sch)) {
> +               if (private)
> +                       vfio_ccw_fsm_event(private,
> VFIO_CCW_EVENT_NOT_OPER);
> +       }
>  
>  out_unlock:
>         spin_unlock_irqrestore(sch->lock, flags);
> @@ -326,7 +375,8 @@ static void vfio_ccw_queue_crw(struct
> vfio_ccw_private *private,
>  static int vfio_ccw_chp_event(struct subchannel *sch,
>                               struct chp_link *link, int event)
>  {
> -       struct vfio_ccw_private *private = dev_get_drvdata(&sch-
> >dev);
> +       struct vfio_ccw_parent *parent = dev_get_drvdata(&sch->dev);
> +       struct vfio_ccw_private *private = dev_get_drvdata(&parent-
> >dev);
>         int mask = chp_ssd_get_mask(&sch->ssd_info, link);
>         int retry = 255;
>  
> diff --git a/drivers/s390/cio/vfio_ccw_ops.c
> b/drivers/s390/cio/vfio_ccw_ops.c
> index 6ae4d012d800..b1cd89d900ab 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
> @@ -14,6 +14,7 @@
>  #include <linux/nospec.h>
>  #include <linux/slab.h>
>  
> +#include "vfio_ccw_parent.h"
>  #include "vfio_ccw_private.h"
>  
>  static const struct vfio_device_ops vfio_ccw_dev_ops;
> @@ -55,7 +56,9 @@ static int vfio_ccw_mdev_init_dev(struct
> vfio_device *vdev)
>  
>  static int vfio_ccw_mdev_probe(struct mdev_device *mdev)
>  {
> -       struct vfio_ccw_private *private = dev_get_drvdata(mdev-
> >dev.parent);
> +       struct subchannel *sch = to_subchannel(mdev->dev.parent);
> +       struct vfio_ccw_parent *parent = dev_get_drvdata(&sch->dev);
> +       struct vfio_ccw_private *private = dev_get_drvdata(&parent-
> >dev);
>         int ret;
>  
>         if (private->state == VFIO_CCW_STATE_NOT_OPER)
> @@ -100,7 +103,9 @@ static void vfio_ccw_mdev_release_dev(struct
> vfio_device *vdev)
>  
>  static void vfio_ccw_mdev_remove(struct mdev_device *mdev)
>  {
> -       struct vfio_ccw_private *private = dev_get_drvdata(mdev-
> >dev.parent);
> +       struct subchannel *sch = to_subchannel(mdev->dev.parent);
> +       struct vfio_ccw_parent *parent = dev_get_drvdata(&sch->dev);
> +       struct vfio_ccw_private *private = dev_get_drvdata(&parent-
> >dev);
>  
>         VFIO_CCW_MSG_EVENT(2, "sch %x.%x.%04x: remove\n",
>                            private->sch->schid.cssid,
> diff --git a/drivers/s390/cio/vfio_ccw_parent.h
> b/drivers/s390/cio/vfio_ccw_parent.h
> new file mode 100644
> index 000000000000..834c00077802
> --- /dev/null
> +++ b/drivers/s390/cio/vfio_ccw_parent.h
> @@ -0,0 +1,28 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * MDEV Parent contents for vfio_ccw driver
> + *
> + * Copyright IBM Corp. 2022
> + */
> +
> +#ifndef _VFIO_CCW_PARENT_H_
> +#define _VFIO_CCW_PARENT_H_
> +
> +#include <linux/mdev.h>
> +
> +/**
> + * struct vfio_ccw_parent
> + *
> + * @dev: embedded device struct
> + * @parent: parent data structures for mdevs created
> + * @mdev_type(s): identifying information for mdevs created
> + */
> +struct vfio_ccw_parent {
> +       struct device           dev;
> +
> +       struct mdev_parent      parent;
> +       struct mdev_type        mdev_type;
> +       struct mdev_type        *mdev_types[1];
> +};
> +
> +#endif
> diff --git a/drivers/s390/cio/vfio_ccw_private.h
> b/drivers/s390/cio/vfio_ccw_private.h
> index bd5fb81456af..673e9a81a172 100644
> --- a/drivers/s390/cio/vfio_ccw_private.h
> +++ b/drivers/s390/cio/vfio_ccw_private.h
> @@ -89,7 +89,6 @@ struct vfio_ccw_crw {
>   * @io_work: work for deferral process of I/O handling
>   * @crw_work: work for deferral process of CRW handling
>   * @release_comp: synchronization helper for vfio device release
> - * @parent: parent data structures for mdevs created
>   */
>  struct vfio_ccw_private {
>         struct vfio_device vdev;
> @@ -116,10 +115,6 @@ struct vfio_ccw_private {
>         struct work_struct      crw_work;
>  
>         struct completion       release_comp;
> -
> -       struct mdev_parent      parent;
> -       struct mdev_type        mdev_type;
> -       struct mdev_type        *mdev_types[1];
>  } __aligned(8);
>  
>  int vfio_ccw_sch_quiesce(struct subchannel *sch);

Re: [PATCH v1 1/7] vfio/ccw: create a parent struct

[Matthew Rosato]

On 10/19/22 12:21 PM, Eric Farman wrote:
> Move the stuff associated with the mdev parent (and thus the
> subchannel struct) into its own struct, and leave the rest in
> the existing private structure.
> 
> The subchannel will point to the parent, and the parent will point
> to the private, for the areas where one or both are needed. Further
> separation of these structs will follow.
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>  drivers/s390/cio/vfio_ccw_drv.c     | 104 ++++++++++++++++++++--------
>  drivers/s390/cio/vfio_ccw_ops.c     |   9 ++-
>  drivers/s390/cio/vfio_ccw_parent.h  |  28 ++++++++
>  drivers/s390/cio/vfio_ccw_private.h |   5 --
>  4 files changed, 112 insertions(+), 34 deletions(-)
>  create mode 100644 drivers/s390/cio/vfio_ccw_parent.h
> 
> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> index 7f5402fe857a..634760ca0dea 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> @@ -20,6 +20,7 @@
>  #include "chp.h"
>  #include "ioasm.h"
>  #include "css.h"
> +#include "vfio_ccw_parent.h"
>  #include "vfio_ccw_private.h"
>  
>  struct workqueue_struct *vfio_ccw_work_q;
> @@ -36,7 +37,8 @@ debug_info_t *vfio_ccw_debug_trace_id;
>   */
>  int vfio_ccw_sch_quiesce(struct subchannel *sch)
>  {
> -	struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev);
> +	struct vfio_ccw_parent *parent = dev_get_drvdata(&sch->dev);
> +	struct vfio_ccw_private *private = dev_get_drvdata(&parent->dev);
>  	DECLARE_COMPLETION_ONSTACK(completion);
>  	int iretry, ret = 0;
>  
> @@ -51,19 +53,21 @@ int vfio_ccw_sch_quiesce(struct subchannel *sch)
>  			break;
>  		}
>  
> -		/*
> -		 * Flush all I/O and wait for
> -		 * cancel/halt/clear completion.
> -		 */
> -		private->completion = &completion;
> -		spin_unlock_irq(sch->lock);
> +		if (private) {

Is it valid to ever reach this code with private == NULL?  If no, then this should probably be a WARN_ON upfront?

> +			/*
> +			 * Flush all I/O and wait for
> +			 * cancel/halt/clear completion.
> +			 */
> +			private->completion = &completion;
> +			spin_unlock_irq(sch->lock);
>  
> -		if (ret == -EBUSY)
> -			wait_for_completion_timeout(&completion, 3*HZ);
> +			if (ret == -EBUSY)
> +				wait_for_completion_timeout(&completion, 3*HZ);
>  
> -		private->completion = NULL;
> -		flush_workqueue(vfio_ccw_work_q);
> -		spin_lock_irq(sch->lock);
> +			private->completion = NULL;
> +			flush_workqueue(vfio_ccw_work_q);
> +			spin_lock_irq(sch->lock);
> +		}
>  		ret = cio_disable_subchannel(sch);
>  	} while (ret == -EBUSY);
>  

.. snip ..

> diff --git a/drivers/s390/cio/vfio_ccw_parent.h b/drivers/s390/cio/vfio_ccw_parent.h
> new file mode 100644
> index 000000000000..834c00077802
> --- /dev/null
> +++ b/drivers/s390/cio/vfio_ccw_parent.h
> @@ -0,0 +1,28 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * MDEV Parent contents for vfio_ccw driver
> + *
> + * Copyright IBM Corp. 2022
> + */
> +
> +#ifndef _VFIO_CCW_PARENT_H_
> +#define _VFIO_CCW_PARENT_H_
> +
> +#include <linux/mdev.h>
> +
> +/**
> + * struct vfio_ccw_parent
> + *
> + * @dev: embedded device struct
> + * @parent: parent data structures for mdevs created
> + * @mdev_type(s): identifying information for mdevs created
> + */
> +struct vfio_ccw_parent {
> +	struct device		dev;
> +
> +	struct mdev_parent	parent;
> +	struct mdev_type	mdev_type;
> +	struct mdev_type	*mdev_types[1];
> +};

Structure itself seems fine, but any reason we need a new file for it?

Re: [PATCH v1 1/7] vfio/ccw: create a parent struct

[Eric Farman]

On Fri, 2022-10-28 at 12:51 -0400, Matthew Rosato wrote:
> On 10/19/22 12:21 PM, Eric Farman wrote:
> > Move the stuff associated with the mdev parent (and thus the
> > subchannel struct) into its own struct, and leave the rest in
> > the existing private structure.
> > 
> > The subchannel will point to the parent, and the parent will point
> > to the private, for the areas where one or both are needed. Further
> > separation of these structs will follow.
> > 
> > Signed-off-by: Eric Farman <farman@linux.ibm.com>
> > ---
> >  drivers/s390/cio/vfio_ccw_drv.c     | 104 ++++++++++++++++++++----
> > ----
> >  drivers/s390/cio/vfio_ccw_ops.c     |   9 ++-
> >  drivers/s390/cio/vfio_ccw_parent.h  |  28 ++++++++
> >  drivers/s390/cio/vfio_ccw_private.h |   5 --
> >  4 files changed, 112 insertions(+), 34 deletions(-)
> >  create mode 100644 drivers/s390/cio/vfio_ccw_parent.h
> > 
> > diff --git a/drivers/s390/cio/vfio_ccw_drv.c
> > b/drivers/s390/cio/vfio_ccw_drv.c
> > index 7f5402fe857a..634760ca0dea 100644
> > --- a/drivers/s390/cio/vfio_ccw_drv.c
> > +++ b/drivers/s390/cio/vfio_ccw_drv.c
> > @@ -20,6 +20,7 @@
> >  #include "chp.h"
> >  #include "ioasm.h"
> >  #include "css.h"
> > +#include "vfio_ccw_parent.h"
> >  #include "vfio_ccw_private.h"
> >  
> >  struct workqueue_struct *vfio_ccw_work_q;
> > @@ -36,7 +37,8 @@ debug_info_t *vfio_ccw_debug_trace_id;
> >   */
> >  int vfio_ccw_sch_quiesce(struct subchannel *sch)
> >  {
> > -       struct vfio_ccw_private *private = dev_get_drvdata(&sch-
> > >dev);
> > +       struct vfio_ccw_parent *parent = dev_get_drvdata(&sch-
> > >dev);
> > +       struct vfio_ccw_private *private = dev_get_drvdata(&parent-
> > >dev);
> >         DECLARE_COMPLETION_ONSTACK(completion);
> >         int iretry, ret = 0;
> >  
> > @@ -51,19 +53,21 @@ int vfio_ccw_sch_quiesce(struct subchannel
> > *sch)
> >                         break;
> >                 }
> >  
> > -               /*
> > -                * Flush all I/O and wait for
> > -                * cancel/halt/clear completion.
> > -                */
> > -               private->completion = &completion;
> > -               spin_unlock_irq(sch->lock);
> > +               if (private) {
> 
> Is it valid to ever reach this code with private == NULL?  If no,
> then this should probably be a WARN_ON upfront?

Hrm, the caller jumps from private -> subchannel, so it would be weird
if we couldn't then go back the other way. Probably impossible, I'll
unwind these whitespace changes and put the WARN_ON on top. Thanks for
the tip.

> 
> > +                       /*
> > +                        * Flush all I/O and wait for
> > +                        * cancel/halt/clear completion.
> > +                        */
> > +                       private->completion = &completion;
> > +                       spin_unlock_irq(sch->lock);
> >  
> > -               if (ret == -EBUSY)
> > -                       wait_for_completion_timeout(&completion,
> > 3*HZ);
> > +                       if (ret == -EBUSY)
> > +                               wait_for_completion_timeout(&comple
> > tion, 3*HZ);
> >  
> > -               private->completion = NULL;
> > -               flush_workqueue(vfio_ccw_work_q);
> > -               spin_lock_irq(sch->lock);
> > +                       private->completion = NULL;
> > +                       flush_workqueue(vfio_ccw_work_q);
> > +                       spin_lock_irq(sch->lock);
> > +               }
> >                 ret = cio_disable_subchannel(sch);
> >         } while (ret == -EBUSY);
> >  
> 
> .. snip ..
> 
> > diff --git a/drivers/s390/cio/vfio_ccw_parent.h
> > b/drivers/s390/cio/vfio_ccw_parent.h
> > new file mode 100644
> > index 000000000000..834c00077802
> > --- /dev/null
> > +++ b/drivers/s390/cio/vfio_ccw_parent.h
> > @@ -0,0 +1,28 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * MDEV Parent contents for vfio_ccw driver
> > + *
> > + * Copyright IBM Corp. 2022
> > + */
> > +
> > +#ifndef _VFIO_CCW_PARENT_H_
> > +#define _VFIO_CCW_PARENT_H_
> > +
> > +#include <linux/mdev.h>
> > +
> > +/**
> > + * struct vfio_ccw_parent
> > + *
> > + * @dev: embedded device struct
> > + * @parent: parent data structures for mdevs created
> > + * @mdev_type(s): identifying information for mdevs created
> > + */
> > +struct vfio_ccw_parent {
> > +       struct device           dev;
> > +
> > +       struct mdev_parent      parent;
> > +       struct mdev_type        mdev_type;
> > +       struct mdev_type        *mdev_types[1];
> > +};
> 
> Structure itself seems fine, but any reason we need a new file for
> it?
> 

Not really. I could leave it in _private.h, but that file is just a
dumping ground for everything so I thought this would be a good
opportunity to start to cleaning that up. But it wouldn't bother me to
leave that whole process to another day too.

[PATCH v1 2/7] vfio/ccw: remove private->sch

[Eric Farman]

These places all rely on the ability to jump from a private
struct back to the subchannel struct. Rather than keeping a
copy in our back pocket, let's use the relationship provided
by the vfio_device embedded within the private.

Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_chp.c     |  5 +++--
 drivers/s390/cio/vfio_ccw_drv.c     |  3 +--
 drivers/s390/cio/vfio_ccw_fsm.c     | 27 ++++++++++++---------------
 drivers/s390/cio/vfio_ccw_ops.c     | 12 ++++++------
 drivers/s390/cio/vfio_ccw_private.h |  7 ++++---
 5 files changed, 26 insertions(+), 28 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_chp.c b/drivers/s390/cio/vfio_ccw_chp.c
index 13b26a1c7988..d3f3a611f95b 100644
--- a/drivers/s390/cio/vfio_ccw_chp.c
+++ b/drivers/s390/cio/vfio_ccw_chp.c
@@ -16,6 +16,7 @@ static ssize_t vfio_ccw_schib_region_read(struct vfio_ccw_private *private,
 					  char __user *buf, size_t count,
 					  loff_t *ppos)
 {
+	struct subchannel *sch = to_subchannel(private->vdev.dev->parent);
 	unsigned int i = VFIO_CCW_OFFSET_TO_INDEX(*ppos) - VFIO_CCW_NUM_REGIONS;
 	loff_t pos = *ppos & VFIO_CCW_OFFSET_MASK;
 	struct ccw_schib_region *region;
@@ -27,12 +28,12 @@ static ssize_t vfio_ccw_schib_region_read(struct vfio_ccw_private *private,
 	mutex_lock(&private->io_mutex);
 	region = private->region[i].data;
 
-	if (cio_update_schib(private->sch)) {
+	if (cio_update_schib(sch)) {
 		ret = -ENODEV;
 		goto out;
 	}
 
-	memcpy(region, &private->sch->schib, sizeof(*region));
+	memcpy(region, &sch->schib, sizeof(*region));
 
 	if (copy_to_user(buf, (void *)region + pos, count)) {
 		ret = -EFAULT;
diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 634760ca0dea..4ee953c8ae39 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -154,7 +154,6 @@ static struct vfio_ccw_private *vfio_ccw_alloc_private(struct subchannel *sch)
 	if (!private)
 		return ERR_PTR(-ENOMEM);
 
-	private->sch = sch;
 	mutex_init(&private->io_mutex);
 	private->state = VFIO_CCW_STATE_STANDBY;
 	INIT_LIST_HEAD(&private->crw);
@@ -383,7 +382,7 @@ static int vfio_ccw_chp_event(struct subchannel *sch,
 	if (!private || !mask)
 		return 0;
 
-	trace_vfio_ccw_chp_event(private->sch->schid, mask, event);
+	trace_vfio_ccw_chp_event(sch->schid, mask, event);
 	VFIO_CCW_MSG_EVENT(2, "sch %x.%x.%04x: mask=0x%x event=%d\n",
 			   sch->schid.cssid,
 			   sch->schid.ssid, sch->schid.sch_no,
diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
index a59c758869f8..e67fad897af3 100644
--- a/drivers/s390/cio/vfio_ccw_fsm.c
+++ b/drivers/s390/cio/vfio_ccw_fsm.c
@@ -18,15 +18,13 @@
 
 static int fsm_io_helper(struct vfio_ccw_private *private)
 {
-	struct subchannel *sch;
+	struct subchannel *sch = to_subchannel(private->vdev.dev->parent);
 	union orb *orb;
 	int ccode;
 	__u8 lpm;
 	unsigned long flags;
 	int ret;
 
-	sch = private->sch;
-
 	spin_lock_irqsave(sch->lock, flags);
 
 	orb = cp_get_orb(&private->cp, (u32)(addr_t)sch, sch->lpm);
@@ -80,13 +78,11 @@ static int fsm_io_helper(struct vfio_ccw_private *private)
 
 static int fsm_do_halt(struct vfio_ccw_private *private)
 {
-	struct subchannel *sch;
+	struct subchannel *sch = to_subchannel(private->vdev.dev->parent);
 	unsigned long flags;
 	int ccode;
 	int ret;
 
-	sch = private->sch;
-
 	spin_lock_irqsave(sch->lock, flags);
 
 	VFIO_CCW_TRACE_EVENT(2, "haltIO");
@@ -121,13 +117,11 @@ static int fsm_do_halt(struct vfio_ccw_private *private)
 
 static int fsm_do_clear(struct vfio_ccw_private *private)
 {
-	struct subchannel *sch;
+	struct subchannel *sch = to_subchannel(private->vdev.dev->parent);
 	unsigned long flags;
 	int ccode;
 	int ret;
 
-	sch = private->sch;
-
 	spin_lock_irqsave(sch->lock, flags);
 
 	VFIO_CCW_TRACE_EVENT(2, "clearIO");
@@ -160,7 +154,7 @@ static int fsm_do_clear(struct vfio_ccw_private *private)
 static void fsm_notoper(struct vfio_ccw_private *private,
 			enum vfio_ccw_event event)
 {
-	struct subchannel *sch = private->sch;
+	struct subchannel *sch = to_subchannel(private->vdev.dev->parent);
 
 	VFIO_CCW_MSG_EVENT(2, "sch %x.%x.%04x: notoper event %x state %x\n",
 			   sch->schid.cssid,
@@ -228,7 +222,7 @@ static void fsm_async_retry(struct vfio_ccw_private *private,
 static void fsm_disabled_irq(struct vfio_ccw_private *private,
 			     enum vfio_ccw_event event)
 {
-	struct subchannel *sch = private->sch;
+	struct subchannel *sch = to_subchannel(private->vdev.dev->parent);
 
 	/*
 	 * An interrupt in a disabled state means a previous disable was not
@@ -238,7 +232,9 @@ static void fsm_disabled_irq(struct vfio_ccw_private *private,
 }
 inline struct subchannel_id get_schid(struct vfio_ccw_private *p)
 {
-	return p->sch->schid;
+	struct subchannel *sch = to_subchannel(p->vdev.dev->parent);
+
+	return sch->schid;
 }
 
 /*
@@ -360,10 +356,11 @@ static void fsm_async_request(struct vfio_ccw_private *private,
 static void fsm_irq(struct vfio_ccw_private *private,
 		    enum vfio_ccw_event event)
 {
+	struct subchannel *sch = to_subchannel(private->vdev.dev->parent);
 	struct irb *irb = this_cpu_ptr(&cio_irb);
 
 	VFIO_CCW_TRACE_EVENT(6, "IRQ");
-	VFIO_CCW_TRACE_EVENT(6, dev_name(&private->sch->dev));
+	VFIO_CCW_TRACE_EVENT(6, dev_name(&sch->dev));
 
 	memcpy(&private->irb, irb, sizeof(*irb));
 
@@ -376,7 +373,7 @@ static void fsm_irq(struct vfio_ccw_private *private,
 static void fsm_open(struct vfio_ccw_private *private,
 		     enum vfio_ccw_event event)
 {
-	struct subchannel *sch = private->sch;
+	struct subchannel *sch = to_subchannel(private->vdev.dev->parent);
 	int ret;
 
 	spin_lock_irq(sch->lock);
@@ -397,7 +394,7 @@ static void fsm_open(struct vfio_ccw_private *private,
 static void fsm_close(struct vfio_ccw_private *private,
 		      enum vfio_ccw_event event)
 {
-	struct subchannel *sch = private->sch;
+	struct subchannel *sch = to_subchannel(private->vdev.dev->parent);
 	int ret;
 
 	spin_lock_irq(sch->lock);
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index b1cd89d900ab..cf383c729d53 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -69,9 +69,9 @@ static int vfio_ccw_mdev_probe(struct mdev_device *mdev)
 		return ret;
 
 	VFIO_CCW_MSG_EVENT(2, "sch %x.%x.%04x: create\n",
-			   private->sch->schid.cssid,
-			   private->sch->schid.ssid,
-			   private->sch->schid.sch_no);
+			   sch->schid.cssid,
+			   sch->schid.ssid,
+			   sch->schid.sch_no);
 
 	ret = vfio_register_emulated_iommu_dev(&private->vdev);
 	if (ret)
@@ -108,9 +108,9 @@ static void vfio_ccw_mdev_remove(struct mdev_device *mdev)
 	struct vfio_ccw_private *private = dev_get_drvdata(&parent->dev);
 
 	VFIO_CCW_MSG_EVENT(2, "sch %x.%x.%04x: remove\n",
-			   private->sch->schid.cssid,
-			   private->sch->schid.ssid,
-			   private->sch->schid.sch_no);
+			   sch->schid.cssid,
+			   sch->schid.ssid,
+			   sch->schid.sch_no);
 
 	vfio_unregister_group_dev(&private->vdev);
 
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index 673e9a81a172..0fdff1435230 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -70,7 +70,6 @@ struct vfio_ccw_crw {
 /**
  * struct vfio_ccw_private
  * @vdev: Embedded VFIO device
- * @sch: pointer to the subchannel
  * @state: internal state of the device
  * @completion: synchronization helper of the I/O completion
  * @io_region: MMIO region to input/output I/O arguments/results
@@ -92,7 +91,6 @@ struct vfio_ccw_crw {
  */
 struct vfio_ccw_private {
 	struct vfio_device vdev;
-	struct subchannel	*sch;
 	int			state;
 	struct completion	*completion;
 	struct ccw_io_region	*io_region;
@@ -157,7 +155,10 @@ extern fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS];
 static inline void vfio_ccw_fsm_event(struct vfio_ccw_private *private,
 				      enum vfio_ccw_event event)
 {
-	trace_vfio_ccw_fsm_event(private->sch->schid, private->state, event);
+	struct subchannel *sch = to_subchannel(private->vdev.dev->parent);
+
+	if (sch)
+		trace_vfio_ccw_fsm_event(sch->schid, private->state, event);
 	vfio_ccw_jumptable[private->state][event](private, event);
 }
 
-- 
2.34.1

Re: [PATCH v1 2/7] vfio/ccw: remove private->sch

[Matthew Rosato]

On 10/19/22 12:21 PM, Eric Farman wrote:
> These places all rely on the ability to jump from a private
> struct back to the subchannel struct. Rather than keeping a
> copy in our back pocket, let's use the relationship provided
> by the vfio_device embedded within the private.
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>  drivers/s390/cio/vfio_ccw_chp.c     |  5 +++--
>  drivers/s390/cio/vfio_ccw_drv.c     |  3 +--
>  drivers/s390/cio/vfio_ccw_fsm.c     | 27 ++++++++++++---------------
>  drivers/s390/cio/vfio_ccw_ops.c     | 12 ++++++------
>  drivers/s390/cio/vfio_ccw_private.h |  7 ++++---
>  5 files changed, 26 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_chp.c b/drivers/s390/cio/vfio_ccw_chp.c
> index 13b26a1c7988..d3f3a611f95b 100644
> --- a/drivers/s390/cio/vfio_ccw_chp.c
> +++ b/drivers/s390/cio/vfio_ccw_chp.c
> @@ -16,6 +16,7 @@ static ssize_t vfio_ccw_schib_region_read(struct vfio_ccw_private *private,
>  					  char __user *buf, size_t count,
>  					  loff_t *ppos)
>  {
> +	struct subchannel *sch = to_subchannel(private->vdev.dev->parent);

I'm not a big fan of the amount of indirection there, but I prefer this over back-pocketing the subchannel in vfio_ccw_private anyway

Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>

[PATCH v1 3/7] vfio/ccw: move private initialization to callback

[Eric Farman]

There's already a device initialization callback that is
used to initialize the release completion workaround.

Move the other elements of the vfio_ccw_private struct that
require distinct initialization over to that routine.

Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_drv.c     | 57 +++--------------------------
 drivers/s390/cio/vfio_ccw_ops.c     | 43 ++++++++++++++++++++++
 drivers/s390/cio/vfio_ccw_private.h |  7 +++-
 3 files changed, 55 insertions(+), 52 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 4ee953c8ae39..cc9ed2fd970f 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -24,10 +24,10 @@
 #include "vfio_ccw_private.h"
 
 struct workqueue_struct *vfio_ccw_work_q;
-static struct kmem_cache *vfio_ccw_io_region;
-static struct kmem_cache *vfio_ccw_cmd_region;
-static struct kmem_cache *vfio_ccw_schib_region;
-static struct kmem_cache *vfio_ccw_crw_region;
+struct kmem_cache *vfio_ccw_io_region;
+struct kmem_cache *vfio_ccw_cmd_region;
+struct kmem_cache *vfio_ccw_schib_region;
+struct kmem_cache *vfio_ccw_crw_region;
 
 debug_info_t *vfio_ccw_debug_msg_id;
 debug_info_t *vfio_ccw_debug_trace_id;
@@ -74,7 +74,7 @@ int vfio_ccw_sch_quiesce(struct subchannel *sch)
 	return ret;
 }
 
-static void vfio_ccw_sch_io_todo(struct work_struct *work)
+void vfio_ccw_sch_io_todo(struct work_struct *work)
 {
 	struct vfio_ccw_private *private;
 	struct irb *irb;
@@ -110,7 +110,7 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work)
 		eventfd_signal(private->io_trigger, 1);
 }
 
-static void vfio_ccw_crw_todo(struct work_struct *work)
+void vfio_ccw_crw_todo(struct work_struct *work)
 {
 	struct vfio_ccw_private *private;
 
@@ -154,52 +154,7 @@ static struct vfio_ccw_private *vfio_ccw_alloc_private(struct subchannel *sch)
 	if (!private)
 		return ERR_PTR(-ENOMEM);
 
-	mutex_init(&private->io_mutex);
-	private->state = VFIO_CCW_STATE_STANDBY;
-	INIT_LIST_HEAD(&private->crw);
-	INIT_WORK(&private->io_work, vfio_ccw_sch_io_todo);
-	INIT_WORK(&private->crw_work, vfio_ccw_crw_todo);
-
-	private->cp.guest_cp = kcalloc(CCWCHAIN_LEN_MAX, sizeof(struct ccw1),
-				       GFP_KERNEL);
-	if (!private->cp.guest_cp)
-		goto out_free_private;
-
-	private->io_region = kmem_cache_zalloc(vfio_ccw_io_region,
-					       GFP_KERNEL | GFP_DMA);
-	if (!private->io_region)
-		goto out_free_cp;
-
-	private->cmd_region = kmem_cache_zalloc(vfio_ccw_cmd_region,
-						GFP_KERNEL | GFP_DMA);
-	if (!private->cmd_region)
-		goto out_free_io;
-
-	private->schib_region = kmem_cache_zalloc(vfio_ccw_schib_region,
-						  GFP_KERNEL | GFP_DMA);
-
-	if (!private->schib_region)
-		goto out_free_cmd;
-
-	private->crw_region = kmem_cache_zalloc(vfio_ccw_crw_region,
-						GFP_KERNEL | GFP_DMA);
-
-	if (!private->crw_region)
-		goto out_free_schib;
 	return private;
-
-out_free_schib:
-	kmem_cache_free(vfio_ccw_schib_region, private->schib_region);
-out_free_cmd:
-	kmem_cache_free(vfio_ccw_cmd_region, private->cmd_region);
-out_free_io:
-	kmem_cache_free(vfio_ccw_io_region, private->io_region);
-out_free_cp:
-	kfree(private->cp.guest_cp);
-out_free_private:
-	mutex_destroy(&private->io_mutex);
-	kfree(private);
-	return ERR_PTR(-ENOMEM);
 }
 
 static void vfio_ccw_free_private(struct vfio_ccw_private *private)
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index cf383c729d53..626b8eb3507b 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -50,8 +50,51 @@ static int vfio_ccw_mdev_init_dev(struct vfio_device *vdev)
 	struct vfio_ccw_private *private =
 		container_of(vdev, struct vfio_ccw_private, vdev);
 
+	mutex_init(&private->io_mutex);
+	private->state = VFIO_CCW_STATE_STANDBY;
+	INIT_LIST_HEAD(&private->crw);
+	INIT_WORK(&private->io_work, vfio_ccw_sch_io_todo);
+	INIT_WORK(&private->crw_work, vfio_ccw_crw_todo);
 	init_completion(&private->release_comp);
+
+	private->cp.guest_cp = kcalloc(CCWCHAIN_LEN_MAX, sizeof(struct ccw1),
+				       GFP_KERNEL);
+	if (!private->cp.guest_cp)
+		goto out_free_private;
+
+	private->io_region = kmem_cache_zalloc(vfio_ccw_io_region,
+					       GFP_KERNEL | GFP_DMA);
+	if (!private->io_region)
+		goto out_free_cp;
+
+	private->cmd_region = kmem_cache_zalloc(vfio_ccw_cmd_region,
+						GFP_KERNEL | GFP_DMA);
+	if (!private->cmd_region)
+		goto out_free_io;
+
+	private->schib_region = kmem_cache_zalloc(vfio_ccw_schib_region,
+						  GFP_KERNEL | GFP_DMA);
+	if (!private->schib_region)
+		goto out_free_cmd;
+
+	private->crw_region = kmem_cache_zalloc(vfio_ccw_crw_region,
+						GFP_KERNEL | GFP_DMA);
+	if (!private->crw_region)
+		goto out_free_schib;
+
 	return 0;
+
+out_free_schib:
+	kmem_cache_free(vfio_ccw_schib_region, private->schib_region);
+out_free_cmd:
+	kmem_cache_free(vfio_ccw_cmd_region, private->cmd_region);
+out_free_io:
+	kmem_cache_free(vfio_ccw_io_region, private->io_region);
+out_free_cp:
+	kfree(private->cp.guest_cp);
+out_free_private:
+	mutex_destroy(&private->io_mutex);
+	return -ENOMEM;
 }
 
 static int vfio_ccw_mdev_probe(struct mdev_device *mdev)
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index 0fdff1435230..b35940057073 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -116,6 +116,8 @@ struct vfio_ccw_private {
 } __aligned(8);
 
 int vfio_ccw_sch_quiesce(struct subchannel *sch);
+void vfio_ccw_sch_io_todo(struct work_struct *work);
+void vfio_ccw_crw_todo(struct work_struct *work);
 
 extern struct mdev_driver vfio_ccw_mdev_driver;
 
@@ -163,7 +165,10 @@ static inline void vfio_ccw_fsm_event(struct vfio_ccw_private *private,
 }
 
 extern struct workqueue_struct *vfio_ccw_work_q;
-
+extern struct kmem_cache *vfio_ccw_io_region;
+extern struct kmem_cache *vfio_ccw_cmd_region;
+extern struct kmem_cache *vfio_ccw_schib_region;
+extern struct kmem_cache *vfio_ccw_crw_region;
 
 /* s390 debug feature, similar to base cio */
 extern debug_info_t *vfio_ccw_debug_msg_id;
-- 
2.34.1

Re: [PATCH v1 3/7] vfio/ccw: move private initialization to callback

[Matthew Rosato]

On 10/19/22 12:21 PM, Eric Farman wrote:
> There's already a device initialization callback that is
> used to initialize the release completion workaround.

As discussed off-list, maybe clarify what callback you're talking about here and/or reference the commit that added it.

> 
> Move the other elements of the vfio_ccw_private struct that
> require distinct initialization over to that routine.
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>  drivers/s390/cio/vfio_ccw_drv.c     | 57 +++--------------------------
>  drivers/s390/cio/vfio_ccw_ops.c     | 43 ++++++++++++++++++++++
>  drivers/s390/cio/vfio_ccw_private.h |  7 +++-
>  3 files changed, 55 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> index 4ee953c8ae39..cc9ed2fd970f 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> @@ -24,10 +24,10 @@
>  #include "vfio_ccw_private.h"
>  
>  struct workqueue_struct *vfio_ccw_work_q;
> -static struct kmem_cache *vfio_ccw_io_region;
> -static struct kmem_cache *vfio_ccw_cmd_region;
> -static struct kmem_cache *vfio_ccw_schib_region;
> -static struct kmem_cache *vfio_ccw_crw_region;
> +struct kmem_cache *vfio_ccw_io_region;
> +struct kmem_cache *vfio_ccw_cmd_region;
> +struct kmem_cache *vfio_ccw_schib_region;
> +struct kmem_cache *vfio_ccw_crw_region;
>  
>  debug_info_t *vfio_ccw_debug_msg_id;
>  debug_info_t *vfio_ccw_debug_trace_id;
> @@ -74,7 +74,7 @@ int vfio_ccw_sch_quiesce(struct subchannel *sch)
>  	return ret;
>  }
>  
> -static void vfio_ccw_sch_io_todo(struct work_struct *work)
> +void vfio_ccw_sch_io_todo(struct work_struct *work)
>  {
>  	struct vfio_ccw_private *private;
>  	struct irb *irb;
> @@ -110,7 +110,7 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work)
>  		eventfd_signal(private->io_trigger, 1);
>  }
>  
> -static void vfio_ccw_crw_todo(struct work_struct *work)
> +void vfio_ccw_crw_todo(struct work_struct *work)
>  {
>  	struct vfio_ccw_private *private;
>  
> @@ -154,52 +154,7 @@ static struct vfio_ccw_private *vfio_ccw_alloc_private(struct subchannel *sch)
>  	if (!private)
>  		return ERR_PTR(-ENOMEM);

Not sure we really still need vfio_ccw_alloc_private() now or whether you can just kzalloc() inline right in vfio_ccw_sch_probe()

Either way:

Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>


>  
> -	mutex_init(&private->io_mutex);
> -	private->state = VFIO_CCW_STATE_STANDBY;
> -	INIT_LIST_HEAD(&private->crw);
> -	INIT_WORK(&private->io_work, vfio_ccw_sch_io_todo);
> -	INIT_WORK(&private->crw_work, vfio_ccw_crw_todo);
> -
> -	private->cp.guest_cp = kcalloc(CCWCHAIN_LEN_MAX, sizeof(struct ccw1),
> -				       GFP_KERNEL);
> -	if (!private->cp.guest_cp)
> -		goto out_free_private;
> -
> -	private->io_region = kmem_cache_zalloc(vfio_ccw_io_region,
> -					       GFP_KERNEL | GFP_DMA);
> -	if (!private->io_region)
> -		goto out_free_cp;
> -
> -	private->cmd_region = kmem_cache_zalloc(vfio_ccw_cmd_region,
> -						GFP_KERNEL | GFP_DMA);
> -	if (!private->cmd_region)
> -		goto out_free_io;
> -
> -	private->schib_region = kmem_cache_zalloc(vfio_ccw_schib_region,
> -						  GFP_KERNEL | GFP_DMA);
> -
> -	if (!private->schib_region)
> -		goto out_free_cmd;
> -
> -	private->crw_region = kmem_cache_zalloc(vfio_ccw_crw_region,
> -						GFP_KERNEL | GFP_DMA);
> -
> -	if (!private->crw_region)
> -		goto out_free_schib;
>  	return private;
> -
> -out_free_schib:
> -	kmem_cache_free(vfio_ccw_schib_region, private->schib_region);
> -out_free_cmd:
> -	kmem_cache_free(vfio_ccw_cmd_region, private->cmd_region);
> -out_free_io:
> -	kmem_cache_free(vfio_ccw_io_region, private->io_region);
> -out_free_cp:
> -	kfree(private->cp.guest_cp);
> -out_free_private:
> -	mutex_destroy(&private->io_mutex);
> -	kfree(private);
> -	return ERR_PTR(-ENOMEM);
>  }
>  
>  static void vfio_ccw_free_private(struct vfio_ccw_private *private)
> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index cf383c729d53..626b8eb3507b 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
> @@ -50,8 +50,51 @@ static int vfio_ccw_mdev_init_dev(struct vfio_device *vdev)
>  	struct vfio_ccw_private *private =
>  		container_of(vdev, struct vfio_ccw_private, vdev);
>  
> +	mutex_init(&private->io_mutex);
> +	private->state = VFIO_CCW_STATE_STANDBY;
> +	INIT_LIST_HEAD(&private->crw);
> +	INIT_WORK(&private->io_work, vfio_ccw_sch_io_todo);
> +	INIT_WORK(&private->crw_work, vfio_ccw_crw_todo);
>  	init_completion(&private->release_comp);
> +
> +	private->cp.guest_cp = kcalloc(CCWCHAIN_LEN_MAX, sizeof(struct ccw1),
> +				       GFP_KERNEL);
> +	if (!private->cp.guest_cp)
> +		goto out_free_private;
> +
> +	private->io_region = kmem_cache_zalloc(vfio_ccw_io_region,
> +					       GFP_KERNEL | GFP_DMA);
> +	if (!private->io_region)
> +		goto out_free_cp;
> +
> +	private->cmd_region = kmem_cache_zalloc(vfio_ccw_cmd_region,
> +						GFP_KERNEL | GFP_DMA);
> +	if (!private->cmd_region)
> +		goto out_free_io;
> +
> +	private->schib_region = kmem_cache_zalloc(vfio_ccw_schib_region,
> +						  GFP_KERNEL | GFP_DMA);
> +	if (!private->schib_region)
> +		goto out_free_cmd;
> +
> +	private->crw_region = kmem_cache_zalloc(vfio_ccw_crw_region,
> +						GFP_KERNEL | GFP_DMA);
> +	if (!private->crw_region)
> +		goto out_free_schib;
> +
>  	return 0;
> +
> +out_free_schib:
> +	kmem_cache_free(vfio_ccw_schib_region, private->schib_region);
> +out_free_cmd:
> +	kmem_cache_free(vfio_ccw_cmd_region, private->cmd_region);
> +out_free_io:
> +	kmem_cache_free(vfio_ccw_io_region, private->io_region);
> +out_free_cp:
> +	kfree(private->cp.guest_cp);
> +out_free_private:
> +	mutex_destroy(&private->io_mutex);
> +	return -ENOMEM;
>  }
>  
>  static int vfio_ccw_mdev_probe(struct mdev_device *mdev)
> diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
> index 0fdff1435230..b35940057073 100644
> --- a/drivers/s390/cio/vfio_ccw_private.h
> +++ b/drivers/s390/cio/vfio_ccw_private.h
> @@ -116,6 +116,8 @@ struct vfio_ccw_private {
>  } __aligned(8);
>  
>  int vfio_ccw_sch_quiesce(struct subchannel *sch);
> +void vfio_ccw_sch_io_todo(struct work_struct *work);
> +void vfio_ccw_crw_todo(struct work_struct *work);
>  
>  extern struct mdev_driver vfio_ccw_mdev_driver;
>  
> @@ -163,7 +165,10 @@ static inline void vfio_ccw_fsm_event(struct vfio_ccw_private *private,
>  }
>  
>  extern struct workqueue_struct *vfio_ccw_work_q;
> -
> +extern struct kmem_cache *vfio_ccw_io_region;
> +extern struct kmem_cache *vfio_ccw_cmd_region;
> +extern struct kmem_cache *vfio_ccw_schib_region;
> +extern struct kmem_cache *vfio_ccw_crw_region;
>  
>  /* s390 debug feature, similar to base cio */
>  extern debug_info_t *vfio_ccw_debug_msg_id;

Re: [PATCH v1 3/7] vfio/ccw: move private initialization to callback

[Eric Farman]

On Fri, 2022-10-28 at 14:52 -0400, Matthew Rosato wrote:
> On 10/19/22 12:21 PM, Eric Farman wrote:
> > There's already a device initialization callback that is
> > used to initialize the release completion workaround.
> 
> As discussed off-list, maybe clarify what callback you're talking
> about here and/or reference the commit that added it.

Agreed. Will point out that it's private->release_comp, introduced with
commit ebb72b765fb49 ("vfio/ccw: Use the new device life cycle
helpers")

> 
> > 
> > Move the other elements of the vfio_ccw_private struct that
> > require distinct initialization over to that routine.
> > 
> > Signed-off-by: Eric Farman <farman@linux.ibm.com>
> > ---
> >  drivers/s390/cio/vfio_ccw_drv.c     | 57 +++----------------------
> > ----
> >  drivers/s390/cio/vfio_ccw_ops.c     | 43 ++++++++++++++++++++++
> >  drivers/s390/cio/vfio_ccw_private.h |  7 +++-
> >  3 files changed, 55 insertions(+), 52 deletions(-)
> > 
> > diff --git a/drivers/s390/cio/vfio_ccw_drv.c
> > b/drivers/s390/cio/vfio_ccw_drv.c
> > index 4ee953c8ae39..cc9ed2fd970f 100644
> > --- a/drivers/s390/cio/vfio_ccw_drv.c
> > +++ b/drivers/s390/cio/vfio_ccw_drv.c
> > @@ -24,10 +24,10 @@
> >  #include "vfio_ccw_private.h"
> >  
> >  struct workqueue_struct *vfio_ccw_work_q;
> > -static struct kmem_cache *vfio_ccw_io_region;
> > -static struct kmem_cache *vfio_ccw_cmd_region;
> > -static struct kmem_cache *vfio_ccw_schib_region;
> > -static struct kmem_cache *vfio_ccw_crw_region;
> > +struct kmem_cache *vfio_ccw_io_region;
> > +struct kmem_cache *vfio_ccw_cmd_region;
> > +struct kmem_cache *vfio_ccw_schib_region;
> > +struct kmem_cache *vfio_ccw_crw_region;
> >  
> >  debug_info_t *vfio_ccw_debug_msg_id;
> >  debug_info_t *vfio_ccw_debug_trace_id;
> > @@ -74,7 +74,7 @@ int vfio_ccw_sch_quiesce(struct subchannel *sch)
> >         return ret;
> >  }
> >  
> > -static void vfio_ccw_sch_io_todo(struct work_struct *work)
> > +void vfio_ccw_sch_io_todo(struct work_struct *work)
> >  {
> >         struct vfio_ccw_private *private;
> >         struct irb *irb;
> > @@ -110,7 +110,7 @@ static void vfio_ccw_sch_io_todo(struct
> > work_struct *work)
> >                 eventfd_signal(private->io_trigger, 1);
> >  }
> >  
> > -static void vfio_ccw_crw_todo(struct work_struct *work)
> > +void vfio_ccw_crw_todo(struct work_struct *work)
> >  {
> >         struct vfio_ccw_private *private;
> >  
> > @@ -154,52 +154,7 @@ static struct vfio_ccw_private
> > *vfio_ccw_alloc_private(struct subchannel *sch)
> >         if (!private)
> >                 return ERR_PTR(-ENOMEM);
> 
> Not sure we really still need vfio_ccw_alloc_private() now or whether
> you can just kzalloc() inline right in vfio_ccw_sch_probe()

Fair. It ends up ends up getting scrapped in patch 6 anyway, but that
might clean things up just a smidge more. Will give it a whirl.

> Either way:
> 
> Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>

Thanks!

> 
> 
> >  
> > -       mutex_init(&private->io_mutex);
> > -       private->state = VFIO_CCW_STATE_STANDBY;
> > -       INIT_LIST_HEAD(&private->crw);
> > -       INIT_WORK(&private->io_work, vfio_ccw_sch_io_todo);
> > -       INIT_WORK(&private->crw_work, vfio_ccw_crw_todo);
> > -
> > -       private->cp.guest_cp = kcalloc(CCWCHAIN_LEN_MAX,
> > sizeof(struct ccw1),
> > -                                      GFP_KERNEL);
> > -       if (!private->cp.guest_cp)
> > -               goto out_free_private;
> > -
> > -       private->io_region = kmem_cache_zalloc(vfio_ccw_io_region,
> > -                                              GFP_KERNEL |
> > GFP_DMA);
> > -       if (!private->io_region)
> > -               goto out_free_cp;
> > -
> > -       private->cmd_region =
> > kmem_cache_zalloc(vfio_ccw_cmd_region,
> > -                                               GFP_KERNEL |
> > GFP_DMA);
> > -       if (!private->cmd_region)
> > -               goto out_free_io;
> > -
> > -       private->schib_region =
> > kmem_cache_zalloc(vfio_ccw_schib_region,
> > -                                                 GFP_KERNEL |
> > GFP_DMA);
> > -
> > -       if (!private->schib_region)
> > -               goto out_free_cmd;
> > -
> > -       private->crw_region =
> > kmem_cache_zalloc(vfio_ccw_crw_region,
> > -                                               GFP_KERNEL |
> > GFP_DMA);
> > -
> > -       if (!private->crw_region)
> > -               goto out_free_schib;
> >         return private;
> > -
> > -out_free_schib:
> > -       kmem_cache_free(vfio_ccw_schib_region, private-
> > >schib_region);
> > -out_free_cmd:
> > -       kmem_cache_free(vfio_ccw_cmd_region, private->cmd_region);
> > -out_free_io:
> > -       kmem_cache_free(vfio_ccw_io_region, private->io_region);
> > -out_free_cp:
> > -       kfree(private->cp.guest_cp);
> > -out_free_private:
> > -       mutex_destroy(&private->io_mutex);
> > -       kfree(private);
> > -       return ERR_PTR(-ENOMEM);
> >  }
> >  
> >  static void vfio_ccw_free_private(struct vfio_ccw_private
> > *private)
> > diff --git a/drivers/s390/cio/vfio_ccw_ops.c
> > b/drivers/s390/cio/vfio_ccw_ops.c
> > index cf383c729d53..626b8eb3507b 100644
> > --- a/drivers/s390/cio/vfio_ccw_ops.c
> > +++ b/drivers/s390/cio/vfio_ccw_ops.c
> > @@ -50,8 +50,51 @@ static int vfio_ccw_mdev_init_dev(struct
> > vfio_device *vdev)
> >         struct vfio_ccw_private *private =
> >                 container_of(vdev, struct vfio_ccw_private, vdev);
> >  
> > +       mutex_init(&private->io_mutex);
> > +       private->state = VFIO_CCW_STATE_STANDBY;
> > +       INIT_LIST_HEAD(&private->crw);
> > +       INIT_WORK(&private->io_work, vfio_ccw_sch_io_todo);
> > +       INIT_WORK(&private->crw_work, vfio_ccw_crw_todo);
> >         init_completion(&private->release_comp);
> > +
> > +       private->cp.guest_cp = kcalloc(CCWCHAIN_LEN_MAX,
> > sizeof(struct ccw1),
> > +                                      GFP_KERNEL);
> > +       if (!private->cp.guest_cp)
> > +               goto out_free_private;
> > +
> > +       private->io_region = kmem_cache_zalloc(vfio_ccw_io_region,
> > +                                              GFP_KERNEL |
> > GFP_DMA);
> > +       if (!private->io_region)
> > +               goto out_free_cp;
> > +
> > +       private->cmd_region =
> > kmem_cache_zalloc(vfio_ccw_cmd_region,
> > +                                               GFP_KERNEL |
> > GFP_DMA);
> > +       if (!private->cmd_region)
> > +               goto out_free_io;
> > +
> > +       private->schib_region =
> > kmem_cache_zalloc(vfio_ccw_schib_region,
> > +                                                 GFP_KERNEL |
> > GFP_DMA);
> > +       if (!private->schib_region)
> > +               goto out_free_cmd;
> > +
> > +       private->crw_region =
> > kmem_cache_zalloc(vfio_ccw_crw_region,
> > +                                               GFP_KERNEL |
> > GFP_DMA);
> > +       if (!private->crw_region)
> > +               goto out_free_schib;
> > +
> >         return 0;
> > +
> > +out_free_schib:
> > +       kmem_cache_free(vfio_ccw_schib_region, private-
> > >schib_region);
> > +out_free_cmd:
> > +       kmem_cache_free(vfio_ccw_cmd_region, private->cmd_region);
> > +out_free_io:
> > +       kmem_cache_free(vfio_ccw_io_region, private->io_region);
> > +out_free_cp:
> > +       kfree(private->cp.guest_cp);
> > +out_free_private:
> > +       mutex_destroy(&private->io_mutex);
> > +       return -ENOMEM;
> >  }
> >  
> >  static int vfio_ccw_mdev_probe(struct mdev_device *mdev)
> > diff --git a/drivers/s390/cio/vfio_ccw_private.h
> > b/drivers/s390/cio/vfio_ccw_private.h
> > index 0fdff1435230..b35940057073 100644
> > --- a/drivers/s390/cio/vfio_ccw_private.h
> > +++ b/drivers/s390/cio/vfio_ccw_private.h
> > @@ -116,6 +116,8 @@ struct vfio_ccw_private {
> >  } __aligned(8);
> >  
> >  int vfio_ccw_sch_quiesce(struct subchannel *sch);
> > +void vfio_ccw_sch_io_todo(struct work_struct *work);
> > +void vfio_ccw_crw_todo(struct work_struct *work);
> >  
> >  extern struct mdev_driver vfio_ccw_mdev_driver;
> >  
> > @@ -163,7 +165,10 @@ static inline void vfio_ccw_fsm_event(struct
> > vfio_ccw_private *private,
> >  }
> >  
> >  extern struct workqueue_struct *vfio_ccw_work_q;
> > -
> > +extern struct kmem_cache *vfio_ccw_io_region;
> > +extern struct kmem_cache *vfio_ccw_cmd_region;
> > +extern struct kmem_cache *vfio_ccw_schib_region;
> > +extern struct kmem_cache *vfio_ccw_crw_region;
> >  
> >  /* s390 debug feature, similar to base cio */
> >  extern debug_info_t *vfio_ccw_debug_msg_id;
> 

[PATCH v1 4/7] vfio/ccw: move private to mdev lifecycle

[Eric Farman]

Now that the mdev parent data is split out into its own struct,
it is safe to move the remaining private data to follow the
mdev probe/remove lifecycle. The mdev parent data will remain
where it is, and follow the subchannel and the css driver
interfaces.

Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_drv.c     | 17 ++---------------
 drivers/s390/cio/vfio_ccw_ops.c     | 26 +++++++++++++-------------
 drivers/s390/cio/vfio_ccw_private.h |  3 +++
 3 files changed, 18 insertions(+), 28 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index cc9ed2fd970f..686a9b9f6731 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -146,7 +146,7 @@ static void vfio_ccw_sch_irq(struct subchannel *sch)
 	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_INTERRUPT);
 }
 
-static struct vfio_ccw_private *vfio_ccw_alloc_private(struct subchannel *sch)
+struct vfio_ccw_private *vfio_ccw_alloc_private(struct subchannel *sch)
 {
 	struct vfio_ccw_private *private;
 
@@ -157,7 +157,7 @@ static struct vfio_ccw_private *vfio_ccw_alloc_private(struct subchannel *sch)
 	return private;
 }
 
-static void vfio_ccw_free_private(struct vfio_ccw_private *private)
+void vfio_ccw_free_private(struct vfio_ccw_private *private)
 {
 	struct vfio_ccw_crw *crw, *temp;
 
@@ -185,7 +185,6 @@ static void vfio_ccw_free_parent(struct device *dev)
 static int vfio_ccw_sch_probe(struct subchannel *sch)
 {
 	struct pmcw *pmcw = &sch->schib.pmcw;
-	struct vfio_ccw_private *private;
 	struct vfio_ccw_parent *parent;
 	int ret = -ENOMEM;
 
@@ -202,14 +201,7 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
 	parent->dev.release = &vfio_ccw_free_parent;
 	device_initialize(&parent->dev);
 
-	private = vfio_ccw_alloc_private(sch);
-	if (IS_ERR(private)) {
-		put_device(&parent->dev);
-		return PTR_ERR(private);
-	}
-
 	dev_set_drvdata(&sch->dev, parent);
-	dev_set_drvdata(&parent->dev, private);
 
 	parent->mdev_type.sysfs_name = "io";
 	parent->mdev_type.pretty_name = "I/O subchannel (Non-QDIO)";
@@ -226,9 +218,7 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
 	return 0;
 
 out_free:
-	dev_set_drvdata(&parent->dev, NULL);
 	dev_set_drvdata(&sch->dev, NULL);
-	vfio_ccw_free_private(private);
 	put_device(&parent->dev);
 	return ret;
 }
@@ -236,13 +226,10 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
 static void vfio_ccw_sch_remove(struct subchannel *sch)
 {
 	struct vfio_ccw_parent *parent = dev_get_drvdata(&sch->dev);
-	struct vfio_ccw_private *private = dev_get_drvdata(&parent->dev);
 
 	mdev_unregister_parent(&parent->parent);
 
 	dev_set_drvdata(&sch->dev, NULL);
-
-	vfio_ccw_free_private(private);
 	put_device(&parent->dev);
 
 	VFIO_CCW_MSG_EVENT(4, "unbound from subchannel %x.%x.%04x\n",
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index 626b8eb3507b..3e627b236241 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -101,15 +101,20 @@ static int vfio_ccw_mdev_probe(struct mdev_device *mdev)
 {
 	struct subchannel *sch = to_subchannel(mdev->dev.parent);
 	struct vfio_ccw_parent *parent = dev_get_drvdata(&sch->dev);
-	struct vfio_ccw_private *private = dev_get_drvdata(&parent->dev);
+	struct vfio_ccw_private *private;
 	int ret;
 
-	if (private->state == VFIO_CCW_STATE_NOT_OPER)
-		return -ENODEV;
+	private = vfio_ccw_alloc_private(sch);
+	if (!private)
+		return -ENOMEM;
 
 	ret = vfio_init_device(&private->vdev, &mdev->dev, &vfio_ccw_dev_ops);
-	if (ret)
+	if (ret) {
+		kfree(private);
 		return ret;
+	}
+
+	dev_set_drvdata(&parent->dev, private);
 
 	VFIO_CCW_MSG_EVENT(2, "sch %x.%x.%04x: create\n",
 			   sch->schid.cssid,
@@ -123,6 +128,7 @@ static int vfio_ccw_mdev_probe(struct mdev_device *mdev)
 	return 0;
 
 err_put_vdev:
+	dev_set_drvdata(&parent->dev, NULL);
 	vfio_put_device(&private->vdev);
 	return ret;
 }
@@ -132,15 +138,6 @@ static void vfio_ccw_mdev_release_dev(struct vfio_device *vdev)
 	struct vfio_ccw_private *private =
 		container_of(vdev, struct vfio_ccw_private, vdev);
 
-	/*
-	 * We cannot free vfio_ccw_private here because it includes
-	 * parent info which must be free'ed by css driver.
-	 *
-	 * Use a workaround by memset'ing the core device part and
-	 * then notifying the remove path that all active references
-	 * to this device have been released.
-	 */
-	memset(vdev, 0, sizeof(*vdev));
 	complete(&private->release_comp);
 }
 
@@ -157,6 +154,7 @@ static void vfio_ccw_mdev_remove(struct mdev_device *mdev)
 
 	vfio_unregister_group_dev(&private->vdev);
 
+	dev_set_drvdata(&parent->dev, NULL);
 	vfio_put_device(&private->vdev);
 	/*
 	 * Wait for all active references on mdev are released so it
@@ -167,6 +165,8 @@ static void vfio_ccw_mdev_remove(struct mdev_device *mdev)
 	 * cycle.
 	 */
 	wait_for_completion(&private->release_comp);
+
+	vfio_ccw_free_private(private);
 }
 
 static int vfio_ccw_mdev_open_device(struct vfio_device *vdev)
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index b35940057073..c1959b8bfe86 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -119,6 +119,9 @@ int vfio_ccw_sch_quiesce(struct subchannel *sch);
 void vfio_ccw_sch_io_todo(struct work_struct *work);
 void vfio_ccw_crw_todo(struct work_struct *work);
 
+struct vfio_ccw_private *vfio_ccw_alloc_private(struct subchannel *sch);
+void vfio_ccw_free_private(struct vfio_ccw_private *private);
+
 extern struct mdev_driver vfio_ccw_mdev_driver;
 
 /*
-- 
2.34.1

RE: [PATCH v1 4/7] vfio/ccw: move private to mdev lifecycle

[Tian, Kevin]

> From: Eric Farman <farman@linux.ibm.com>
> Sent: Thursday, October 20, 2022 12:22 AM
> 
> @@ -101,15 +101,20 @@ static int vfio_ccw_mdev_probe(struct
> mdev_device *mdev)
>  {
>  	struct subchannel *sch = to_subchannel(mdev->dev.parent);
>  	struct vfio_ccw_parent *parent = dev_get_drvdata(&sch->dev);
> -	struct vfio_ccw_private *private = dev_get_drvdata(&parent->dev);
> +	struct vfio_ccw_private *private;
>  	int ret;
> 
> -	if (private->state == VFIO_CCW_STATE_NOT_OPER)
> -		return -ENODEV;

Not familiar with ccw but just want to double confirm this removal
is intentional w/o side-effect?

> +	private = vfio_ccw_alloc_private(sch);
> +	if (!private)
> +		return -ENOMEM;
> 
>  	ret = vfio_init_device(&private->vdev, &mdev->dev,
> &vfio_ccw_dev_ops);
> -	if (ret)
> +	if (ret) {
> +		kfree(private);
>  		return ret;
> +	}
> +
> +	dev_set_drvdata(&parent->dev, private);
> 
>  	VFIO_CCW_MSG_EVENT(2, "sch %x.%x.%04x: create\n",
>  			   sch->schid.cssid,
> @@ -123,6 +128,7 @@ static int vfio_ccw_mdev_probe(struct mdev_device
> *mdev)
>  	return 0;
> 
>  err_put_vdev:
> +	dev_set_drvdata(&parent->dev, NULL);

No need to set drvdata to NULL, iiuc

Re: [PATCH v1 4/7] vfio/ccw: move private to mdev lifecycle

[Eric Farman]

On Tue, 2022-11-01 at 09:08 +0000, Tian, Kevin wrote:
> > From: Eric Farman <farman@linux.ibm.com>
> > Sent: Thursday, October 20, 2022 12:22 AM
> > 
> > @@ -101,15 +101,20 @@ static int vfio_ccw_mdev_probe(struct
> > mdev_device *mdev)
> >  {
> >         struct subchannel *sch = to_subchannel(mdev->dev.parent);
> >         struct vfio_ccw_parent *parent = dev_get_drvdata(&sch-
> > >dev);
> > -       struct vfio_ccw_private *private = dev_get_drvdata(&parent-
> > >dev);
> > +       struct vfio_ccw_private *private;
> >         int ret;
> > 
> > -       if (private->state == VFIO_CCW_STATE_NOT_OPER)
> > -               return -ENODEV;
> 
> Not familiar with ccw but just want to double confirm this removal
> is intentional w/o side-effect?

Right, it's intentional and fine. The concern previously was re-probing
the mdev when a device had gone into a non-recoverable state and the
private data was left hanging around. With the private now being
cleaned up in mdev_remove instead of the subchannel side, that's no
longer an issue.

> 
> > +       private = vfio_ccw_alloc_private(sch);
> > +       if (!private)
> > +               return -ENOMEM;
> > 
> >         ret = vfio_init_device(&private->vdev, &mdev->dev,
> > &vfio_ccw_dev_ops);
> > -       if (ret)
> > +       if (ret) {
> > +               kfree(private);
> >                 return ret;
> > +       }
> > +
> > +       dev_set_drvdata(&parent->dev, private);
> > 
> >         VFIO_CCW_MSG_EVENT(2, "sch %x.%x.%04x: create\n",
> >                            sch->schid.cssid,
> > @@ -123,6 +128,7 @@ static int vfio_ccw_mdev_probe(struct
> > mdev_device
> > *mdev)
> >         return 0;
> > 
> >  err_put_vdev:
> > +       dev_set_drvdata(&parent->dev, NULL);
> 
> No need to set drvdata to NULL, iiuc

Fair.

[PATCH v1 5/7] vfio/ccw: remove release completion

[Eric Farman]

There's enough separation between the parent and private structs now,
that it is fine to remove the release completion hack.

Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_ops.c     | 14 +-------------
 drivers/s390/cio/vfio_ccw_private.h |  3 ---
 2 files changed, 1 insertion(+), 16 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index 3e627b236241..261cb8150abb 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -55,7 +55,6 @@ static int vfio_ccw_mdev_init_dev(struct vfio_device *vdev)
 	INIT_LIST_HEAD(&private->crw);
 	INIT_WORK(&private->io_work, vfio_ccw_sch_io_todo);
 	INIT_WORK(&private->crw_work, vfio_ccw_crw_todo);
-	init_completion(&private->release_comp);
 
 	private->cp.guest_cp = kcalloc(CCWCHAIN_LEN_MAX, sizeof(struct ccw1),
 				       GFP_KERNEL);
@@ -138,7 +137,7 @@ static void vfio_ccw_mdev_release_dev(struct vfio_device *vdev)
 	struct vfio_ccw_private *private =
 		container_of(vdev, struct vfio_ccw_private, vdev);
 
-	complete(&private->release_comp);
+	vfio_ccw_free_private(private);
 }
 
 static void vfio_ccw_mdev_remove(struct mdev_device *mdev)
@@ -156,17 +155,6 @@ static void vfio_ccw_mdev_remove(struct mdev_device *mdev)
 
 	dev_set_drvdata(&parent->dev, NULL);
 	vfio_put_device(&private->vdev);
-	/*
-	 * Wait for all active references on mdev are released so it
-	 * is safe to defer kfree() to a later point.
-	 *
-	 * TODO: the clean fix is to split parent/mdev info from ccw
-	 * private structure so each can be managed in its own life
-	 * cycle.
-	 */
-	wait_for_completion(&private->release_comp);
-
-	vfio_ccw_free_private(private);
 }
 
 static int vfio_ccw_mdev_open_device(struct vfio_device *vdev)
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index c1959b8bfe86..ddaf6846022d 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -87,7 +87,6 @@ struct vfio_ccw_crw {
  * @req_trigger: eventfd ctx for signaling userspace to return device
  * @io_work: work for deferral process of I/O handling
  * @crw_work: work for deferral process of CRW handling
- * @release_comp: synchronization helper for vfio device release
  */
 struct vfio_ccw_private {
 	struct vfio_device vdev;
@@ -111,8 +110,6 @@ struct vfio_ccw_private {
 	struct eventfd_ctx	*req_trigger;
 	struct work_struct	io_work;
 	struct work_struct	crw_work;
-
-	struct completion	release_comp;
 } __aligned(8);
 
 int vfio_ccw_sch_quiesce(struct subchannel *sch);
-- 
2.34.1

RE: [PATCH v1 5/7] vfio/ccw: remove release completion

[Tian, Kevin]

> From: Eric Farman <farman@linux.ibm.com>
> Sent: Thursday, October 20, 2022 12:22 AM
> 
> There's enough separation between the parent and private structs now,
> that it is fine to remove the release completion hack.
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

[PATCH v1 6/7] vfio/ccw: replace vfio_init_device with _alloc_

[Eric Farman]

Now that we have a reasonable separation of structs that follow
the subchannel and mdev lifecycles, there's no reason we can't
call the official vfio_alloc_device routine for our private data,
and behave like everyone else.

Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_drv.c     | 29 -----------------------------
 drivers/s390/cio/vfio_ccw_ops.c     | 28 ++++++++++++++++++----------
 drivers/s390/cio/vfio_ccw_private.h |  3 ---
 drivers/vfio/vfio_main.c            |  3 ---
 4 files changed, 18 insertions(+), 45 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 686a9b9f6731..9bbf41315aca 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -146,35 +146,6 @@ static void vfio_ccw_sch_irq(struct subchannel *sch)
 	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_INTERRUPT);
 }
 
-struct vfio_ccw_private *vfio_ccw_alloc_private(struct subchannel *sch)
-{
-	struct vfio_ccw_private *private;
-
-	private = kzalloc(sizeof(*private), GFP_KERNEL);
-	if (!private)
-		return ERR_PTR(-ENOMEM);
-
-	return private;
-}
-
-void vfio_ccw_free_private(struct vfio_ccw_private *private)
-{
-	struct vfio_ccw_crw *crw, *temp;
-
-	list_for_each_entry_safe(crw, temp, &private->crw, next) {
-		list_del(&crw->next);
-		kfree(crw);
-	}
-
-	kmem_cache_free(vfio_ccw_crw_region, private->crw_region);
-	kmem_cache_free(vfio_ccw_schib_region, private->schib_region);
-	kmem_cache_free(vfio_ccw_cmd_region, private->cmd_region);
-	kmem_cache_free(vfio_ccw_io_region, private->io_region);
-	kfree(private->cp.guest_cp);
-	mutex_destroy(&private->io_mutex);
-	kfree(private);
-}
-
 static void vfio_ccw_free_parent(struct device *dev)
 {
 	struct vfio_ccw_parent *parent = container_of(dev, struct vfio_ccw_parent, dev);
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index 261cb8150abb..9619dc35080f 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -103,15 +103,10 @@ static int vfio_ccw_mdev_probe(struct mdev_device *mdev)
 	struct vfio_ccw_private *private;
 	int ret;
 
-	private = vfio_ccw_alloc_private(sch);
-	if (!private)
-		return -ENOMEM;
-
-	ret = vfio_init_device(&private->vdev, &mdev->dev, &vfio_ccw_dev_ops);
-	if (ret) {
-		kfree(private);
-		return ret;
-	}
+	private = vfio_alloc_device(vfio_ccw_private, vdev, &mdev->dev,
+				    &vfio_ccw_dev_ops);
+	if (IS_ERR(private))
+		return PTR_ERR(private);
 
 	dev_set_drvdata(&parent->dev, private);
 
@@ -136,8 +131,21 @@ static void vfio_ccw_mdev_release_dev(struct vfio_device *vdev)
 {
 	struct vfio_ccw_private *private =
 		container_of(vdev, struct vfio_ccw_private, vdev);
+	struct vfio_ccw_crw *crw, *temp;
+
+	list_for_each_entry_safe(crw, temp, &private->crw, next) {
+		list_del(&crw->next);
+		kfree(crw);
+	}
+
+	kmem_cache_free(vfio_ccw_crw_region, private->crw_region);
+	kmem_cache_free(vfio_ccw_schib_region, private->schib_region);
+	kmem_cache_free(vfio_ccw_cmd_region, private->cmd_region);
+	kmem_cache_free(vfio_ccw_io_region, private->io_region);
+	kfree(private->cp.guest_cp);
+	mutex_destroy(&private->io_mutex);
 
-	vfio_ccw_free_private(private);
+	vfio_free_device(vdev);
 }
 
 static void vfio_ccw_mdev_remove(struct mdev_device *mdev)
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index ddaf6846022d..2ca408881659 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -116,9 +116,6 @@ int vfio_ccw_sch_quiesce(struct subchannel *sch);
 void vfio_ccw_sch_io_todo(struct work_struct *work);
 void vfio_ccw_crw_todo(struct work_struct *work);
 
-struct vfio_ccw_private *vfio_ccw_alloc_private(struct subchannel *sch);
-void vfio_ccw_free_private(struct vfio_ccw_private *private);
-
 extern struct mdev_driver vfio_ccw_mdev_driver;
 
 /*
diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
index 2d168793d4e1..28f36c6d9d3f 100644
--- a/drivers/vfio/vfio_main.c
+++ b/drivers/vfio/vfio_main.c
@@ -386,8 +386,6 @@ EXPORT_SYMBOL_GPL(_vfio_alloc_device);
 
 /*
  * Initialize a vfio_device so it can be registered to vfio core.
- *
- * Only vfio-ccw driver should call this interface.
  */
 int vfio_init_device(struct vfio_device *device, struct device *dev,
 		     const struct vfio_device_ops *ops)
@@ -422,7 +420,6 @@ int vfio_init_device(struct vfio_device *device, struct device *dev,
 	ida_free(&vfio.device_ida, device->index);
 	return ret;
 }
-EXPORT_SYMBOL_GPL(vfio_init_device);
 
 /*
  * The helper called by driver @release callback to free the device
-- 
2.34.1

Re: [PATCH v1 6/7] vfio/ccw: replace vfio_init_device with _alloc_

[Jason Gunthorpe]

On Wed, Oct 19, 2022 at 06:21:34PM +0200, Eric Farman wrote:

>  /*
>   * Initialize a vfio_device so it can be registered to vfio core.
> - *
> - * Only vfio-ccw driver should call this interface.
>   */
>  int vfio_init_device(struct vfio_device *device, struct device *dev,
>  		     const struct vfio_device_ops *ops)
> @@ -422,7 +420,6 @@ int vfio_init_device(struct vfio_device *device, struct device *dev,
>  	ida_free(&vfio.device_ida, device->index);
>  	return ret;
>  }
> -EXPORT_SYMBOL_GPL(vfio_init_device);

Should be made static as well

Jason

Re: [PATCH v1 6/7] vfio/ccw: replace vfio_init_device with _alloc_

[Eric Farman]

On Wed, 2022-10-19 at 14:15 -0300, Jason Gunthorpe wrote:
> On Wed, Oct 19, 2022 at 06:21:34PM +0200, Eric Farman wrote:
> 
> >  /*
> >   * Initialize a vfio_device so it can be registered to vfio core.
> > - *
> > - * Only vfio-ccw driver should call this interface.
> >   */
> >  int vfio_init_device(struct vfio_device *device, struct device
> > *dev,
> >                      const struct vfio_device_ops *ops)
> > @@ -422,7 +420,6 @@ int vfio_init_device(struct vfio_device
> > *device, struct device *dev,
> >         ida_free(&vfio.device_ida, device->index);
> >         return ret;
> >  }
> > -EXPORT_SYMBOL_GPL(vfio_init_device);
> 
> Should be made static as well

Agreed. Only reason I didn't was there's a prototype in
include/linux/vfio.h to satisfy the call to vfio_init_device from
_vfio_alloc_device, and I didn't want to get into moving things around
if I didn't need to. I can do that on top, if you'd like.

Eric

Re: [PATCH v1 6/7] vfio/ccw: replace vfio_init_device with _alloc_

[Jason Gunthorpe]

On Wed, Oct 19, 2022 at 01:57:41PM -0400, Eric Farman wrote:
> On Wed, 2022-10-19 at 14:15 -0300, Jason Gunthorpe wrote:
> > On Wed, Oct 19, 2022 at 06:21:34PM +0200, Eric Farman wrote:
> > 
> > >  /*
> > >   * Initialize a vfio_device so it can be registered to vfio core.
> > > - *
> > > - * Only vfio-ccw driver should call this interface.
> > >   */
> > >  int vfio_init_device(struct vfio_device *device, struct device
> > > *dev,
> > >                      const struct vfio_device_ops *ops)
> > > @@ -422,7 +420,6 @@ int vfio_init_device(struct vfio_device
> > > *device, struct device *dev,
> > >         ida_free(&vfio.device_ida, device->index);
> > >         return ret;
> > >  }
> > > -EXPORT_SYMBOL_GPL(vfio_init_device);
> > 
> > Should be made static as well
> 
> Agreed. Only reason I didn't was there's a prototype in
> include/linux/vfio.h to satisfy the call to vfio_init_device from
> _vfio_alloc_device, and I didn't want to get into moving things around
> if I didn't need to. I can do that on top, if you'd like.

You can just add a one line forward static declaration at the top of
the file in this patch.

Jason

RE: [PATCH v1 6/7] vfio/ccw: replace vfio_init_device with _alloc_

[Tian, Kevin]

> From: Eric Farman
> Sent: Thursday, October 20, 2022 12:22 AM
> 
> Now that we have a reasonable separation of structs that follow
> the subchannel and mdev lifecycles, there's no reason we can't
> call the official vfio_alloc_device routine for our private data,
> and behave like everyone else.
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>

This looks good to me. With Jason's suggestion handled,

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

[PATCH v1 7/7] vfio: Remove vfio_free_device

[Eric Farman]

With the "mess" sorted out, we should be able to inline the
vfio_free_device call introduced by commit cb9ff3f3b84c
("vfio: Add helpers for unifying vfio_device life cycle")
and remove them from driver release callbacks.

Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/gpu/drm/i915/gvt/kvmgt.c      |  1 -
 drivers/s390/cio/vfio_ccw_ops.c       |  2 --
 drivers/s390/crypto/vfio_ap_ops.c     |  6 ------
 drivers/vfio/fsl-mc/vfio_fsl_mc.c     |  1 -
 drivers/vfio/pci/vfio_pci_core.c      |  1 -
 drivers/vfio/platform/vfio_amba.c     |  1 -
 drivers/vfio/platform/vfio_platform.c |  1 -
 drivers/vfio/vfio_main.c              | 22 ++++------------------
 include/linux/vfio.h                  |  1 -
 samples/vfio-mdev/mbochs.c            |  1 -
 samples/vfio-mdev/mdpy.c              |  1 -
 samples/vfio-mdev/mtty.c              |  1 -
 12 files changed, 4 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
index 7a45e5360caf..eee6805e67de 100644
--- a/drivers/gpu/drm/i915/gvt/kvmgt.c
+++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
@@ -1461,7 +1461,6 @@ static void intel_vgpu_release_dev(struct vfio_device *vfio_dev)
 	struct intel_vgpu *vgpu = vfio_dev_to_vgpu(vfio_dev);
 
 	intel_gvt_destroy_vgpu(vgpu);
-	vfio_free_device(vfio_dev);
 }
 
 static const struct vfio_device_ops intel_vgpu_dev_ops = {
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index 9619dc35080f..cfb2e5c8e717 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -144,8 +144,6 @@ static void vfio_ccw_mdev_release_dev(struct vfio_device *vdev)
 	kmem_cache_free(vfio_ccw_io_region, private->io_region);
 	kfree(private->cp.guest_cp);
 	mutex_destroy(&private->io_mutex);
-
-	vfio_free_device(vdev);
 }
 
 static void vfio_ccw_mdev_remove(struct mdev_device *mdev)
diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 0b4cc8c597ae..f108c0f14712 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -765,11 +765,6 @@ static void vfio_ap_mdev_unlink_fr_queues(struct ap_matrix_mdev *matrix_mdev)
 	}
 }
 
-static void vfio_ap_mdev_release_dev(struct vfio_device *vdev)
-{
-	vfio_free_device(vdev);
-}
-
 static void vfio_ap_mdev_remove(struct mdev_device *mdev)
 {
 	struct ap_matrix_mdev *matrix_mdev = dev_get_drvdata(&mdev->dev);
@@ -1784,7 +1779,6 @@ static const struct attribute_group vfio_queue_attr_group = {
 
 static const struct vfio_device_ops vfio_ap_matrix_dev_ops = {
 	.init = vfio_ap_mdev_init_dev,
-	.release = vfio_ap_mdev_release_dev,
 	.open_device = vfio_ap_mdev_open_device,
 	.close_device = vfio_ap_mdev_close_device,
 	.ioctl = vfio_ap_mdev_ioctl,
diff --git a/drivers/vfio/fsl-mc/vfio_fsl_mc.c b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
index b16874e913e4..7b8889f55007 100644
--- a/drivers/vfio/fsl-mc/vfio_fsl_mc.c
+++ b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
@@ -568,7 +568,6 @@ static void vfio_fsl_mc_release_dev(struct vfio_device *core_vdev)
 
 	vfio_fsl_uninit_device(vdev);
 	mutex_destroy(&vdev->igate);
-	vfio_free_device(core_vdev);
 }
 
 static int vfio_fsl_mc_remove(struct fsl_mc_device *mc_dev)
diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
index badc9d828cac..9be2d5be5d95 100644
--- a/drivers/vfio/pci/vfio_pci_core.c
+++ b/drivers/vfio/pci/vfio_pci_core.c
@@ -2109,7 +2109,6 @@ void vfio_pci_core_release_dev(struct vfio_device *core_vdev)
 	mutex_destroy(&vdev->vma_lock);
 	kfree(vdev->region);
 	kfree(vdev->pm_save);
-	vfio_free_device(core_vdev);
 }
 EXPORT_SYMBOL_GPL(vfio_pci_core_release_dev);
 
diff --git a/drivers/vfio/platform/vfio_amba.c b/drivers/vfio/platform/vfio_amba.c
index eaea63e5294c..18faf2678b99 100644
--- a/drivers/vfio/platform/vfio_amba.c
+++ b/drivers/vfio/platform/vfio_amba.c
@@ -95,7 +95,6 @@ static void vfio_amba_release_dev(struct vfio_device *core_vdev)
 
 	vfio_platform_release_common(vdev);
 	kfree(vdev->name);
-	vfio_free_device(core_vdev);
 }
 
 static void vfio_amba_remove(struct amba_device *adev)
diff --git a/drivers/vfio/platform/vfio_platform.c b/drivers/vfio/platform/vfio_platform.c
index 82cedcebfd90..9910451dc341 100644
--- a/drivers/vfio/platform/vfio_platform.c
+++ b/drivers/vfio/platform/vfio_platform.c
@@ -83,7 +83,6 @@ static void vfio_platform_release_dev(struct vfio_device *core_vdev)
 		container_of(core_vdev, struct vfio_platform_device, vdev);
 
 	vfio_platform_release_common(vdev);
-	vfio_free_device(core_vdev);
 }
 
 static int vfio_platform_remove(struct platform_device *pdev)
diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
index 28f36c6d9d3f..8a1d83cbb05a 100644
--- a/drivers/vfio/vfio_main.c
+++ b/drivers/vfio/vfio_main.c
@@ -339,13 +339,10 @@ static void vfio_device_release(struct device *dev)
 	vfio_release_device_set(device);
 	ida_free(&vfio.device_ida, device->index);
 
-	/*
-	 * kvfree() cannot be done here due to a life cycle mess in
-	 * vfio-ccw. Before the ccw part is fixed all drivers are
-	 * required to support @release and call vfio_free_device()
-	 * from there.
-	 */
-	device->ops->release(device);
+	if (device->ops->release)
+		device->ops->release(device);
+
+	kvfree(device);
 }
 
 /*
@@ -421,17 +418,6 @@ int vfio_init_device(struct vfio_device *device, struct device *dev,
 	return ret;
 }
 
-/*
- * The helper called by driver @release callback to free the device
- * structure. Drivers which don't have private data to clean can
- * simply use this helper as its @release.
- */
-void vfio_free_device(struct vfio_device *device)
-{
-	kvfree(device);
-}
-EXPORT_SYMBOL_GPL(vfio_free_device);
-
 static struct vfio_group *vfio_noiommu_group_alloc(struct device *dev,
 		enum vfio_group_type type)
 {
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index e7cebeb875dd..47d33d5af467 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -178,7 +178,6 @@ struct vfio_device *_vfio_alloc_device(size_t size, struct device *dev,
 
 int vfio_init_device(struct vfio_device *device, struct device *dev,
 		     const struct vfio_device_ops *ops);
-void vfio_free_device(struct vfio_device *device);
 static inline void vfio_put_device(struct vfio_device *device)
 {
 	put_device(&device->device);
diff --git a/samples/vfio-mdev/mbochs.c b/samples/vfio-mdev/mbochs.c
index 117a8d799f71..8b5a3a778a25 100644
--- a/samples/vfio-mdev/mbochs.c
+++ b/samples/vfio-mdev/mbochs.c
@@ -594,7 +594,6 @@ static void mbochs_release_dev(struct vfio_device *vdev)
 	atomic_add(mdev_state->type->mbytes, &mbochs_avail_mbytes);
 	kfree(mdev_state->pages);
 	kfree(mdev_state->vconfig);
-	vfio_free_device(vdev);
 }
 
 static void mbochs_remove(struct mdev_device *mdev)
diff --git a/samples/vfio-mdev/mdpy.c b/samples/vfio-mdev/mdpy.c
index 946e8cfde6fd..721fb06c6413 100644
--- a/samples/vfio-mdev/mdpy.c
+++ b/samples/vfio-mdev/mdpy.c
@@ -283,7 +283,6 @@ static void mdpy_release_dev(struct vfio_device *vdev)
 
 	vfree(mdev_state->memblk);
 	kfree(mdev_state->vconfig);
-	vfio_free_device(vdev);
 }
 
 static void mdpy_remove(struct mdev_device *mdev)
diff --git a/samples/vfio-mdev/mtty.c b/samples/vfio-mdev/mtty.c
index e72085fc1376..3c2a421b9b69 100644
--- a/samples/vfio-mdev/mtty.c
+++ b/samples/vfio-mdev/mtty.c
@@ -784,7 +784,6 @@ static void mtty_release_dev(struct vfio_device *vdev)
 
 	atomic_add(mdev_state->nr_ports, &mdev_avail_ports);
 	kfree(mdev_state->vconfig);
-	vfio_free_device(vdev);
 }
 
 static void mtty_remove(struct mdev_device *mdev)
-- 
2.34.1

Re: [PATCH v1 7/7] vfio: Remove vfio_free_device

[Jason Gunthorpe]

On Wed, Oct 19, 2022 at 06:21:35PM +0200, Eric Farman wrote:
> With the "mess" sorted out, we should be able to inline the
> vfio_free_device call introduced by commit cb9ff3f3b84c
> ("vfio: Add helpers for unifying vfio_device life cycle")
> and remove them from driver release callbacks.
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

RE: [PATCH v1 7/7] vfio: Remove vfio_free_device

[Tian, Kevin]

> From: Eric Farman <farman@linux.ibm.com>
> Sent: Thursday, October 20, 2022 12:22 AM
> 
> With the "mess" sorted out, we should be able to inline the
> vfio_free_device call introduced by commit cb9ff3f3b84c
> ("vfio: Add helpers for unifying vfio_device life cycle")
> and remove them from driver release callbacks.
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

Re: [PATCH v1 0/7] vfio-ccw parent rework

[Jason Gunthorpe]

On Wed, Oct 19, 2022 at 06:21:28PM +0200, Eric Farman wrote:

> This series attempts to address the oddities/shortcomings of vfio-ccw, such
> that vfio-ccw can use the same helpers as everyone else, and the tricks that
> were implemented by the other drivers can be removed. It is built on 6.1-rc1,
> and thus includes the various changes [3][4] that have occurred in and around
> these parts.

It looks good to me if you are happy with the ccw part

Thanks,
Jason