[PATCH v1 00/18] VFIO ccw/mdev rework

[PATCH v1 00/18] VFIO ccw/mdev rework

[Eric Farman]

Last autumn, Jason Gunthorpe proposed some rework of vfio-ccw [1],
to better fit with the new mdev API (thank you!). Part of that
series was pulled for kernel 5.16 [2], but the complexities of
the remaining patches got them hung up behind other work.

This series attempts to dust off and complete that, with the
goal of untangling the lifecycle of a s390 subchannel when
bound to vfio-ccw instead of the usual io_subchannel driver.

Patches 1-8 are inspired by and/or split out from that series,
in order to be consumable on their own (backports, etc.).

Patches 9-12 handle the goal of making the FSM complete,
and synchronizing the subchannel's life with that of the mdev.
(This was the goal of patch 5 of the larger series [3].)

Patches 13-14 are pulled directly from the earlier series.
As these patches hit some other of the consumers of vfio,
those on CC who are unfamiliar with vfio-ccw probably only
care about these. :)

Patches 15-18 links the lifecycle of the vfio_ccw_private struct
with the mdev via a vfio reference. (Patch 17 was also pulled
directly from the earlier series.)

In the end, the subchannel probe/remove callbacks from the css
driver simply register/unregister with vfio-mdev. The communication
with the subchannel is delayed until the mdev routines, which
handles all the vfio-related memory and subchannel enablement.
There's no longer a configuration where the mdev is closed while
the subchannel remains enabled, since that's weird.

@Jason: I carried the S-o-b/r-b tags on patches 13, 14, and 17,
as they were cherry-picked straight from your v3.
If you'd prefer your S-o-b on others, please let me know.

[1] https://lore.kernel.org/r/0-v3-57c1502c62fd+2190-ccw_mdev_jgg@nvidia.com/
[2] https://lore.kernel.org/r/0-v4-cea4f5bd2c00+b52-ccw_mdev_jgg@nvidia.com/
[3] https://lore.kernel.org/r/5-v3-57c1502c62fd+2190-ccw_mdev_jgg@nvidia.com/

Cc: Kirti Wankhede <kwankhede@nvidia.com>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
Cc: Zhi Wang <zhi.a.wang@intel.com>
Cc: intel-gvt-dev@lists.freedesktop.org
Cc: Tony Krowiak <akrowiak@linux.ibm.com>
Cc: Jason Herne <jjherne@linux.ibm.com>

Eric Farman (14):
  vfio/ccw: Fix FSM state if mdev probe fails
  vfio/ccw: Ensure mdev->dev is cleared on mdev remove
  vfio/ccw: Do not change FSM state in subchannel event
  vfio/ccw: Remove private->mdev
  vfio/ccw: Pass enum to FSM event jumptable
  vfio/ccw: Flatten MDEV device (un)register
  vfio/ccw: Check that private pointer is not NULL
  vfio/ccw: Create an OPEN FSM Event
  vfio/ccw: Create a CLOSE FSM event
  vfio/ccw: Refactor vfio_ccw_mdev_reset
  vfio/ccw: Move FSM open/close to MDEV open/close
  vfio/ccw: Manage private with mdev
  vfio/ccw: Create a get_private routine
  vfio/ccw: Manage ccw/mdev reference counts

Jason Gunthorpe (3):
  vfio/mdev: Consolidate all the device_api sysfs into the core code
  vfio/mdev: Add mdev available instance checking to the core
  vfio: Export vfio_device_try_get()

Michael Kawano (1):
  vfio/ccw: Remove UUID from s390 debug log

 .../driver-api/vfio-mediated-device.rst       |   8 +-
 drivers/gpu/drm/i915/gvt/kvmgt.c              |   9 +-
 drivers/s390/cio/vfio_ccw_async.c             |   1 -
 drivers/s390/cio/vfio_ccw_drv.c               | 114 ++++++--------
 drivers/s390/cio/vfio_ccw_fsm.c               |  91 +++++++++--
 drivers/s390/cio/vfio_ccw_ops.c               | 145 ++++++------------
 drivers/s390/cio/vfio_ccw_private.h           |  33 +++-
 drivers/s390/crypto/vfio_ap_ops.c             |  41 ++---
 drivers/s390/crypto/vfio_ap_private.h         |   2 -
 drivers/vfio/mdev/mdev_core.c                 |  13 +-
 drivers/vfio/mdev/mdev_private.h              |   2 +
 drivers/vfio/mdev/mdev_sysfs.c                |  64 +++++++-
 drivers/vfio/vfio.c                           |   3 +-
 include/linux/mdev.h                          |  13 +-
 include/linux/vfio.h                          |   1 +
 samples/vfio-mdev/mbochs.c                    |   9 +-
 samples/vfio-mdev/mdpy.c                      |  31 +---
 samples/vfio-mdev/mtty.c                      |  10 +-
 18 files changed, 300 insertions(+), 290 deletions(-)

-- 
2.32.0

[PATCH v1 01/18] vfio/ccw: Remove UUID from s390 debug log

[Eric Farman]

From: Michael Kawano <mkawano@linux.ibm.com>

As vfio-ccw devices are created/destroyed, the uuid of the associated
mdevs that are recorded in $S390DBF/vfio_ccw_msg/sprintf get lost as
they are created using pointers passed by reference.

This is a deliberate design point of s390dbf, but it leaves the uuid
in these traces less than useful. Since the subchannels are more
constant, and are mapped 1:1 with the mdevs, the associated mdev can
be discerned by looking at the device configuration (e.g., mdevctl)
and places, such as kernel messages, where it is statically stored.

Thus, let's just remove the uuid from s390dbf traces. As we were
the only consumer of mdev_uuid(), remove that too.

Cc: Kirti Wankhede <kwankhede@nvidia.com>
Signed-off-by: Michael Kawano <mkawano@linux.ibm.com>
Fixes: 60e05d1cf0875 ("vfio-ccw: add some logging")
Fixes: b7701dfbf9832 ("vfio-ccw: Register a chp_event callback for vfio-ccw")
[farman: reworded commit message, added Fixes: tags]
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_drv.c |  5 ++---
 drivers/s390/cio/vfio_ccw_fsm.c | 24 ++++++++++++------------
 drivers/s390/cio/vfio_ccw_ops.c |  8 ++++----
 include/linux/mdev.h            |  4 ----
 4 files changed, 18 insertions(+), 23 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index ee182cfb467d..35055eb94115 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -14,7 +14,6 @@
 #include <linux/init.h>
 #include <linux/device.h>
 #include <linux/slab.h>
-#include <linux/uuid.h>
 #include <linux/mdev.h>
 
 #include <asm/isc.h>
@@ -358,8 +357,8 @@ static int vfio_ccw_chp_event(struct subchannel *sch,
 		return 0;
 
 	trace_vfio_ccw_chp_event(private->sch->schid, mask, event);
-	VFIO_CCW_MSG_EVENT(2, "%pUl (%x.%x.%04x): mask=0x%x event=%d\n",
-			   mdev_uuid(private->mdev), sch->schid.cssid,
+	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,
 			   mask, event);
 
diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
index e435a9cd92da..86b23732d899 100644
--- a/drivers/s390/cio/vfio_ccw_fsm.c
+++ b/drivers/s390/cio/vfio_ccw_fsm.c
@@ -256,8 +256,8 @@ static void fsm_io_request(struct vfio_ccw_private *private,
 		if (orb->tm.b) {
 			io_region->ret_code = -EOPNOTSUPP;
 			VFIO_CCW_MSG_EVENT(2,
-					   "%pUl (%x.%x.%04x): transport mode\n",
-					   mdev_uuid(mdev), schid.cssid,
+					   "sch %x.%x.%04x: transport mode\n",
+					   schid.cssid,
 					   schid.ssid, schid.sch_no);
 			errstr = "transport mode";
 			goto err_out;
@@ -266,8 +266,8 @@ static void fsm_io_request(struct vfio_ccw_private *private,
 					      orb);
 		if (io_region->ret_code) {
 			VFIO_CCW_MSG_EVENT(2,
-					   "%pUl (%x.%x.%04x): cp_init=%d\n",
-					   mdev_uuid(mdev), schid.cssid,
+					   "sch %x.%x.%04x: cp_init=%d\n",
+					   schid.cssid,
 					   schid.ssid, schid.sch_no,
 					   io_region->ret_code);
 			errstr = "cp init";
@@ -277,8 +277,8 @@ static void fsm_io_request(struct vfio_ccw_private *private,
 		io_region->ret_code = cp_prefetch(&private->cp);
 		if (io_region->ret_code) {
 			VFIO_CCW_MSG_EVENT(2,
-					   "%pUl (%x.%x.%04x): cp_prefetch=%d\n",
-					   mdev_uuid(mdev), schid.cssid,
+					   "sch %x.%x.%04x: cp_prefetch=%d\n",
+					   schid.cssid,
 					   schid.ssid, schid.sch_no,
 					   io_region->ret_code);
 			errstr = "cp prefetch";
@@ -290,8 +290,8 @@ static void fsm_io_request(struct vfio_ccw_private *private,
 		io_region->ret_code = fsm_io_helper(private);
 		if (io_region->ret_code) {
 			VFIO_CCW_MSG_EVENT(2,
-					   "%pUl (%x.%x.%04x): fsm_io_helper=%d\n",
-					   mdev_uuid(mdev), schid.cssid,
+					   "sch %x.%x.%04x: fsm_io_helper=%d\n",
+					   schid.cssid,
 					   schid.ssid, schid.sch_no,
 					   io_region->ret_code);
 			errstr = "cp fsm_io_helper";
@@ -301,16 +301,16 @@ static void fsm_io_request(struct vfio_ccw_private *private,
 		return;
 	} else if (scsw->cmd.fctl & SCSW_FCTL_HALT_FUNC) {
 		VFIO_CCW_MSG_EVENT(2,
-				   "%pUl (%x.%x.%04x): halt on io_region\n",
-				   mdev_uuid(mdev), schid.cssid,
+				   "sch %x.%x.%04x: halt on io_region\n",
+				   schid.cssid,
 				   schid.ssid, schid.sch_no);
 		/* halt is handled via the async cmd region */
 		io_region->ret_code = -EOPNOTSUPP;
 		goto err_out;
 	} else if (scsw->cmd.fctl & SCSW_FCTL_CLEAR_FUNC) {
 		VFIO_CCW_MSG_EVENT(2,
-				   "%pUl (%x.%x.%04x): clear on io_region\n",
-				   mdev_uuid(mdev), schid.cssid,
+				   "sch %x.%x.%04x: clear on io_region\n",
+				   schid.cssid,
 				   schid.ssid, schid.sch_no);
 		/* clear is handled via the async cmd region */
 		io_region->ret_code = -EOPNOTSUPP;
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index d8589afac272..bebae21228aa 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -131,8 +131,8 @@ static int vfio_ccw_mdev_probe(struct mdev_device *mdev)
 	private->mdev = mdev;
 	private->state = VFIO_CCW_STATE_IDLE;
 
-	VFIO_CCW_MSG_EVENT(2, "mdev %pUl, sch %x.%x.%04x: create\n",
-			   mdev_uuid(mdev), private->sch->schid.cssid,
+	VFIO_CCW_MSG_EVENT(2, "sch %x.%x.%04x: create\n",
+			   private->sch->schid.cssid,
 			   private->sch->schid.ssid,
 			   private->sch->schid.sch_no);
 
@@ -154,8 +154,8 @@ static void vfio_ccw_mdev_remove(struct mdev_device *mdev)
 {
 	struct vfio_ccw_private *private = dev_get_drvdata(mdev->dev.parent);
 
-	VFIO_CCW_MSG_EVENT(2, "mdev %pUl, sch %x.%x.%04x: remove\n",
-			   mdev_uuid(mdev), private->sch->schid.cssid,
+	VFIO_CCW_MSG_EVENT(2, "sch %x.%x.%04x: remove\n",
+			   private->sch->schid.cssid,
 			   private->sch->schid.ssid,
 			   private->sch->schid.sch_no);
 
diff --git a/include/linux/mdev.h b/include/linux/mdev.h
index 15d03f6532d0..a5788f592817 100644
--- a/include/linux/mdev.h
+++ b/include/linux/mdev.h
@@ -139,10 +139,6 @@ static inline void mdev_set_drvdata(struct mdev_device *mdev, void *data)
 {
 	mdev->driver_data = data;
 }
-static inline const guid_t *mdev_uuid(struct mdev_device *mdev)
-{
-	return &mdev->uuid;
-}
 
 extern struct bus_type mdev_bus_type;
 
-- 
2.32.0

[PATCH v1 02/18] vfio/ccw: Fix FSM state if mdev probe fails

[Eric Farman]

The FSM is in STANDBY state when arriving in vfio_ccw_mdev_probe(),
and this routine converts it to IDLE as part of its processing.
The error exit sets it to IDLE (again) but clears the private->mdev
pointer.

The FSM should of course be managing the state itself, but the
correct thing for vfio_ccw_mdev_probe() to do would be to put
the state back the way it found it.

The corresponding check of private->mdev in vfio_ccw_sch_io_todo()
can be removed, since the distinction is unnecessary at this point.

Fixes: 3bf1311f351ef ("vfio/ccw: Convert to use vfio_register_emulated_iommu_dev()")
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_drv.c | 2 +-
 drivers/s390/cio/vfio_ccw_ops.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 35055eb94115..b18b4582bc8b 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -108,7 +108,7 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work)
 	 * has finished. Do not overwrite a possible processing
 	 * state if the final interrupt was for HSCH or CSCH.
 	 */
-	if (private->mdev && cp_is_finished)
+	if (cp_is_finished)
 		private->state = VFIO_CCW_STATE_IDLE;
 
 	if (private->io_trigger)
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index bebae21228aa..a403d059a4e6 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -146,7 +146,7 @@ static int vfio_ccw_mdev_probe(struct mdev_device *mdev)
 	vfio_uninit_group_dev(&private->vdev);
 	atomic_inc(&private->avail);
 	private->mdev = NULL;
-	private->state = VFIO_CCW_STATE_IDLE;
+	private->state = VFIO_CCW_STATE_STANDBY;
 	return ret;
 }
 
-- 
2.32.0

[PATCH v1 03/18] vfio/ccw: Ensure mdev->dev is cleared on mdev remove

[Eric Farman]

The mdev is linked with the vfio_ccw_private pointer when the mdev
is probed, but it's not cleared once the mdev is removed.

This isn't much of a concern based on the current device lifecycle,
but fix it so that things make sense in later shuffling.

Fixes: 3bf1311f351ef ("vfio/ccw: Convert to use vfio_register_emulated_iommu_dev()")
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_ops.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index a403d059a4e6..a0a3200b0b04 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -159,6 +159,7 @@ static void vfio_ccw_mdev_remove(struct mdev_device *mdev)
 			   private->sch->schid.ssid,
 			   private->sch->schid.sch_no);
 
+	dev_set_drvdata(&mdev->dev, NULL);
 	vfio_unregister_group_dev(&private->vdev);
 
 	if ((private->state != VFIO_CCW_STATE_NOT_OPER) &&
-- 
2.32.0

[PATCH v1 04/18] vfio/ccw: Do not change FSM state in subchannel event

[Eric Farman]

The routine vfio_ccw_sch_event() is tasked with handling subchannel events,
specifically machine checks, on behalf of vfio-ccw. It correctly calls
cio_update_schib(), and if that fails (meaning the subchannel is gone)
it makes an FSM event call to mark the subchannel Not Operational.

If that worked, however, then it decides that if the FSM state was already
Not Operational (implying the subchannel just came back), then it should
simply change the FSM to partially- or fully-open.

Remove this trickery, since a subchannel returning will require more
probing than simply "oh all is well again" to ensure it works correctly.

Fixes: bbe37e4cb8970 ("vfio: ccw: introduce a finite state machine")
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_drv.c | 14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index b18b4582bc8b..9d817aa2f1c4 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -300,19 +300,11 @@ static int vfio_ccw_sch_event(struct subchannel *sch, int process)
 	if (work_pending(&sch->todo_work))
 		goto out_unlock;
 
-	if (cio_update_schib(sch)) {
-		vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_NOT_OPER);
-		rc = 0;
-		goto out_unlock;
-	}
-
-	private = dev_get_drvdata(&sch->dev);
-	if (private->state == VFIO_CCW_STATE_NOT_OPER) {
-		private->state = private->mdev ? VFIO_CCW_STATE_IDLE :
-				 VFIO_CCW_STATE_STANDBY;
-	}
 	rc = 0;
 
+	if (cio_update_schib(sch))
+		vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_NOT_OPER);
+
 out_unlock:
 	spin_unlock_irqrestore(sch->lock, flags);
 
-- 
2.32.0

[PATCH v1 05/18] vfio/ccw: Remove private->mdev

[Eric Farman]

The only remaining user of private->mdev gets the struct device in
order to copy channel program data. The same pointer is available
from private->vdev, which is managed by vfio_(un)init_group_dev().

It doesn't make sense to carry multiple pointers to the same thing,
and risk them getting out of sync. So convert that user, and then
private->mdev can be removed entirely.

Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_async.c   | 1 -
 drivers/s390/cio/vfio_ccw_fsm.c     | 4 +---
 drivers/s390/cio/vfio_ccw_ops.c     | 3 ---
 drivers/s390/cio/vfio_ccw_private.h | 2 --
 4 files changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_async.c b/drivers/s390/cio/vfio_ccw_async.c
index 7a838e3d7c0f..420d89ba7f83 100644
--- a/drivers/s390/cio/vfio_ccw_async.c
+++ b/drivers/s390/cio/vfio_ccw_async.c
@@ -8,7 +8,6 @@
  */
 
 #include <linux/vfio.h>
-#include <linux/mdev.h>
 
 #include "vfio_ccw_private.h"
 
diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
index 86b23732d899..3e1d7744c292 100644
--- a/drivers/s390/cio/vfio_ccw_fsm.c
+++ b/drivers/s390/cio/vfio_ccw_fsm.c
@@ -10,7 +10,6 @@
  */
 
 #include <linux/vfio.h>
-#include <linux/mdev.h>
 
 #include "ioasm.h"
 #include "vfio_ccw_private.h"
@@ -242,7 +241,6 @@ static void fsm_io_request(struct vfio_ccw_private *private,
 	union orb *orb;
 	union scsw *scsw = &private->scsw;
 	struct ccw_io_region *io_region = private->io_region;
-	struct mdev_device *mdev = private->mdev;
 	char *errstr = "request";
 	struct subchannel_id schid = get_schid(private);
 
@@ -262,7 +260,7 @@ static void fsm_io_request(struct vfio_ccw_private *private,
 			errstr = "transport mode";
 			goto err_out;
 		}
-		io_region->ret_code = cp_init(&private->cp, mdev_dev(mdev),
+		io_region->ret_code = cp_init(&private->cp, private->vdev.dev,
 					      orb);
 		if (io_region->ret_code) {
 			VFIO_CCW_MSG_EVENT(2,
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index a0a3200b0b04..4a64c176facb 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -128,7 +128,6 @@ static int vfio_ccw_mdev_probe(struct mdev_device *mdev)
 	vfio_init_group_dev(&private->vdev, &mdev->dev,
 			    &vfio_ccw_dev_ops);
 
-	private->mdev = mdev;
 	private->state = VFIO_CCW_STATE_IDLE;
 
 	VFIO_CCW_MSG_EVENT(2, "sch %x.%x.%04x: create\n",
@@ -145,7 +144,6 @@ static int vfio_ccw_mdev_probe(struct mdev_device *mdev)
 err_atomic:
 	vfio_uninit_group_dev(&private->vdev);
 	atomic_inc(&private->avail);
-	private->mdev = NULL;
 	private->state = VFIO_CCW_STATE_STANDBY;
 	return ret;
 }
@@ -171,7 +169,6 @@ static void vfio_ccw_mdev_remove(struct mdev_device *mdev)
 
 	vfio_uninit_group_dev(&private->vdev);
 	cp_free(&private->cp);
-	private->mdev = NULL;
 	atomic_inc(&private->avail);
 }
 
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index 7272eb788612..12b5537d478f 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -73,7 +73,6 @@ struct vfio_ccw_crw {
  * @state: internal state of the device
  * @completion: synchronization helper of the I/O completion
  * @avail: available for creating a mediated device
- * @mdev: pointer to the mediated device
  * @nb: notifier for vfio events
  * @io_region: MMIO region to input/output I/O arguments/results
  * @io_mutex: protect against concurrent update of I/O regions
@@ -97,7 +96,6 @@ struct vfio_ccw_private {
 	int			state;
 	struct completion	*completion;
 	atomic_t		avail;
-	struct mdev_device	*mdev;
 	struct notifier_block	nb;
 	struct ccw_io_region	*io_region;
 	struct mutex		io_mutex;
-- 
2.32.0

[PATCH v1 06/18] vfio/ccw: Pass enum to FSM event jumptable

[Eric Farman]

The FSM has an enumerated list of events defined.
Use that as the argument passed to the jump table,
instead of a regular int.

Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Fixes: bbe37e4cb8970 ("vfio: ccw: introduce a finite state machine")
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_private.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index 12b5537d478f..5c128eec596b 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -156,7 +156,7 @@ typedef void (fsm_func_t)(struct vfio_ccw_private *, enum vfio_ccw_event);
 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,
-				     int event)
+				      enum vfio_ccw_event event)
 {
 	trace_vfio_ccw_fsm_event(private->sch->schid, private->state, event);
 	vfio_ccw_jumptable[private->state][event](private, event);
-- 
2.32.0

[PATCH v1 07/18] vfio/ccw: Flatten MDEV device (un)register

[Eric Farman]

The vfio_ccw_mdev_(un)reg routines are merely vfio-ccw routines that
pass control to mdev_(un)register_device. Since there's only one
caller of each, let's just call the mdev routines directly.

Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_drv.c     |  4 ++--
 drivers/s390/cio/vfio_ccw_ops.c     | 12 +-----------
 drivers/s390/cio/vfio_ccw_private.h |  4 +---
 3 files changed, 4 insertions(+), 16 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 9d817aa2f1c4..3784eb4cda85 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -239,7 +239,7 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
 
 	private->state = VFIO_CCW_STATE_STANDBY;
 
-	ret = vfio_ccw_mdev_reg(sch);
+	ret = mdev_register_device(&sch->dev, &vfio_ccw_mdev_ops);
 	if (ret)
 		goto out_disable;
 
@@ -261,7 +261,7 @@ static void vfio_ccw_sch_remove(struct subchannel *sch)
 	struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev);
 
 	vfio_ccw_sch_quiesce(sch);
-	vfio_ccw_mdev_unreg(sch);
+	mdev_unregister_device(&sch->dev);
 
 	dev_set_drvdata(&sch->dev, NULL);
 
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index 4a64c176facb..497e1b7ffd61 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -656,18 +656,8 @@ struct mdev_driver vfio_ccw_mdev_driver = {
 	.remove = vfio_ccw_mdev_remove,
 };
 
-static const struct mdev_parent_ops vfio_ccw_mdev_ops = {
+const struct mdev_parent_ops vfio_ccw_mdev_ops = {
 	.owner			= THIS_MODULE,
 	.device_driver		= &vfio_ccw_mdev_driver,
 	.supported_type_groups  = mdev_type_groups,
 };
-
-int vfio_ccw_mdev_reg(struct subchannel *sch)
-{
-	return mdev_register_device(&sch->dev, &vfio_ccw_mdev_ops);
-}
-
-void vfio_ccw_mdev_unreg(struct subchannel *sch)
-{
-	mdev_unregister_device(&sch->dev);
-}
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index 5c128eec596b..2e0744ac6492 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -117,12 +117,10 @@ struct vfio_ccw_private {
 	struct work_struct	crw_work;
 } __aligned(8);
 
-extern int vfio_ccw_mdev_reg(struct subchannel *sch);
-extern void vfio_ccw_mdev_unreg(struct subchannel *sch);
-
 extern int vfio_ccw_sch_quiesce(struct subchannel *sch);
 
 extern struct mdev_driver vfio_ccw_mdev_driver;
+extern const struct mdev_parent_ops vfio_ccw_mdev_ops;
 
 /*
  * States of the device statemachine.
-- 
2.32.0

[PATCH v1 08/18] vfio/ccw: Check that private pointer is not NULL

[Eric Farman]

The subchannel->dev->drvdata pointer is set in vfio_ccw_sch_probe()
and cleared in vfio_ccw_sch_remove(). In a purely defensive move,
let's check that the resultant pointer exists before operating on it
any way, since some lifecycle changes are forthcoming.

Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_drv.c | 12 ++++++++++++
 drivers/s390/cio/vfio_ccw_ops.c |  3 +++
 2 files changed, 15 insertions(+)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 3784eb4cda85..f8226db26e54 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -41,6 +41,9 @@ int vfio_ccw_sch_quiesce(struct subchannel *sch)
 	DECLARE_COMPLETION_ONSTACK(completion);
 	int iretry, ret = 0;
 
+	if (!private)
+		return 0;
+
 	spin_lock_irq(sch->lock);
 	if (!sch->schib.pmcw.ena)
 		goto out_unlock;
@@ -132,6 +135,9 @@ static void vfio_ccw_sch_irq(struct subchannel *sch)
 {
 	struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev);
 
+	if (WARN_ON(!private))
+		return;
+
 	inc_irq_stat(IRQIO_CIO);
 	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_INTERRUPT);
 }
@@ -260,6 +266,9 @@ static void vfio_ccw_sch_remove(struct subchannel *sch)
 {
 	struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev);
 
+	if (!private)
+		return;
+
 	vfio_ccw_sch_quiesce(sch);
 	mdev_unregister_device(&sch->dev);
 
@@ -293,6 +302,9 @@ static int vfio_ccw_sch_event(struct subchannel *sch, int process)
 	unsigned long flags;
 	int rc = -EAGAIN;
 
+	if (!private)
+		return 0;
+
 	spin_lock_irqsave(sch->lock, flags);
 	if (!device_is_registered(&sch->dev))
 		goto out_unlock;
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index 497e1b7ffd61..9e5c184eab89 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -152,6 +152,9 @@ static void vfio_ccw_mdev_remove(struct mdev_device *mdev)
 {
 	struct vfio_ccw_private *private = dev_get_drvdata(mdev->dev.parent);
 
+	if (!private)
+		return;
+
 	VFIO_CCW_MSG_EVENT(2, "sch %x.%x.%04x: remove\n",
 			   private->sch->schid.cssid,
 			   private->sch->schid.ssid,
-- 
2.32.0

[PATCH v1 09/18] vfio/ccw: Create an OPEN FSM Event

[Eric Farman]

Move the process of enabling a subchannel for use by vfio-ccw
into the FSM, such that it can manage the sequence of lifecycle
events for the device.

That is, if the FSM state is NOT_OPER(erational), then do the work
that would enable the subchannel and move the FSM to STANDBY state.
An attempt to perform this event again from any of the other operating
states (IDLE, CP_PROCESSING, CP_PENDING) will convert the device back
to NOT_OPER so the configuration process can be started again.

Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_drv.c     |  9 ++-------
 drivers/s390/cio/vfio_ccw_fsm.c     | 21 +++++++++++++++++++++
 drivers/s390/cio/vfio_ccw_private.h |  1 +
 3 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index f8226db26e54..2d816dd01713 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -236,15 +236,10 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
 
 	dev_set_drvdata(&sch->dev, private);
 
-	spin_lock_irq(sch->lock);
-	sch->isc = VFIO_CCW_ISC;
-	ret = cio_enable_subchannel(sch, (u32)(unsigned long)sch);
-	spin_unlock_irq(sch->lock);
-	if (ret)
+	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_OPEN);
+	if (private->state == VFIO_CCW_STATE_NOT_OPER)
 		goto out_free;
 
-	private->state = VFIO_CCW_STATE_STANDBY;
-
 	ret = mdev_register_device(&sch->dev, &vfio_ccw_mdev_ops);
 	if (ret)
 		goto out_disable;
diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
index 3e1d7744c292..25db8534ded0 100644
--- a/drivers/s390/cio/vfio_ccw_fsm.c
+++ b/drivers/s390/cio/vfio_ccw_fsm.c
@@ -11,6 +11,8 @@
 
 #include <linux/vfio.h>
 
+#include <asm/isc.h>
+
 #include "ioasm.h"
 #include "vfio_ccw_private.h"
 
@@ -365,6 +367,20 @@ static void fsm_irq(struct vfio_ccw_private *private,
 		complete(private->completion);
 }
 
+static void fsm_open(struct vfio_ccw_private *private,
+		     enum vfio_ccw_event event)
+{
+	struct subchannel *sch = private->sch;
+	int ret;
+
+	spin_lock_irq(sch->lock);
+	sch->isc = VFIO_CCW_ISC;
+	ret = cio_enable_subchannel(sch, (u32)(unsigned long)sch);
+	if (!ret)
+		private->state = VFIO_CCW_STATE_STANDBY;
+	spin_unlock_irq(sch->lock);
+}
+
 /*
  * Device statemachine
  */
@@ -374,29 +390,34 @@ fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS] = {
 		[VFIO_CCW_EVENT_IO_REQ]		= fsm_io_error,
 		[VFIO_CCW_EVENT_ASYNC_REQ]	= fsm_async_error,
 		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_disabled_irq,
+		[VFIO_CCW_EVENT_OPEN]		= fsm_open,
 	},
 	[VFIO_CCW_STATE_STANDBY] = {
 		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
 		[VFIO_CCW_EVENT_IO_REQ]		= fsm_io_error,
 		[VFIO_CCW_EVENT_ASYNC_REQ]	= fsm_async_error,
 		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
+		[VFIO_CCW_EVENT_OPEN]		= fsm_nop,
 	},
 	[VFIO_CCW_STATE_IDLE] = {
 		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
 		[VFIO_CCW_EVENT_IO_REQ]		= fsm_io_request,
 		[VFIO_CCW_EVENT_ASYNC_REQ]	= fsm_async_request,
 		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
+		[VFIO_CCW_EVENT_OPEN]		= fsm_notoper,
 	},
 	[VFIO_CCW_STATE_CP_PROCESSING] = {
 		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
 		[VFIO_CCW_EVENT_IO_REQ]		= fsm_io_retry,
 		[VFIO_CCW_EVENT_ASYNC_REQ]	= fsm_async_retry,
 		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
+		[VFIO_CCW_EVENT_OPEN]		= fsm_notoper,
 	},
 	[VFIO_CCW_STATE_CP_PENDING] = {
 		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
 		[VFIO_CCW_EVENT_IO_REQ]		= fsm_io_busy,
 		[VFIO_CCW_EVENT_ASYNC_REQ]	= fsm_async_request,
 		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
+		[VFIO_CCW_EVENT_OPEN]		= fsm_notoper,
 	},
 };
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index 2e0744ac6492..d7ba6846e5c5 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -143,6 +143,7 @@ enum vfio_ccw_event {
 	VFIO_CCW_EVENT_IO_REQ,
 	VFIO_CCW_EVENT_INTERRUPT,
 	VFIO_CCW_EVENT_ASYNC_REQ,
+	VFIO_CCW_EVENT_OPEN,
 	/* last element! */
 	NR_VFIO_CCW_EVENTS
 };
-- 
2.32.0

[PATCH v1 10/18] vfio/ccw: Create a CLOSE FSM event

[Eric Farman]

Refactor the vfio_ccw_sch_quiesce() routine to extract the bit that
disables the subchannel and affects the FSM state. Use this to form
the basis of a CLOSE event that will mirror the OPEN event, and move
the subchannel back to NOT_OPER state.

A key difference with that mirroring is that while OPEN handles the
transition from NOT_OPER => STANDBY, the later probing of the mdev
handles the transition from STANDBY => IDLE. On the other hand,
the CLOSE event will move from one of the operating states {IDLE,
CP_PROCESSING, CP_PENDING} => NOT_OPER. That is, there is no stop
in a STANDBY state on the deconfigure path.

Add a call to cp_free() in this event, such that it is captured for
the various permutations of this event.

In the unlikely event that cio_disable_subchannel() returns -EBUSY,
the remaining logic of vfio_ccw_sch_quiesce() can still be used.

Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_drv.c     | 20 ++++++++------------
 drivers/s390/cio/vfio_ccw_fsm.c     | 26 ++++++++++++++++++++++++++
 drivers/s390/cio/vfio_ccw_ops.c     | 14 ++------------
 drivers/s390/cio/vfio_ccw_private.h |  1 +
 4 files changed, 37 insertions(+), 24 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 2d816dd01713..34995ad00332 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -44,13 +44,6 @@ int vfio_ccw_sch_quiesce(struct subchannel *sch)
 	if (!private)
 		return 0;
 
-	spin_lock_irq(sch->lock);
-	if (!sch->schib.pmcw.ena)
-		goto out_unlock;
-	ret = cio_disable_subchannel(sch);
-	if (ret != -EBUSY)
-		goto out_unlock;
-
 	iretry = 255;
 	do {
 
@@ -77,9 +70,7 @@ int vfio_ccw_sch_quiesce(struct subchannel *sch)
 		spin_lock_irq(sch->lock);
 		ret = cio_disable_subchannel(sch);
 	} while (ret == -EBUSY);
-out_unlock:
-	private->state = VFIO_CCW_STATE_NOT_OPER;
-	spin_unlock_irq(sch->lock);
+
 	return ret;
 }
 
@@ -264,7 +255,7 @@ static void vfio_ccw_sch_remove(struct subchannel *sch)
 	if (!private)
 		return;
 
-	vfio_ccw_sch_quiesce(sch);
+	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_CLOSE);
 	mdev_unregister_device(&sch->dev);
 
 	dev_set_drvdata(&sch->dev, NULL);
@@ -278,7 +269,12 @@ static void vfio_ccw_sch_remove(struct subchannel *sch)
 
 static void vfio_ccw_sch_shutdown(struct subchannel *sch)
 {
-	vfio_ccw_sch_quiesce(sch);
+	struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev);
+
+	if (!private)
+		return;
+
+	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_CLOSE);
 }
 
 /**
diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
index 25db8534ded0..926ae618c3fa 100644
--- a/drivers/s390/cio/vfio_ccw_fsm.c
+++ b/drivers/s390/cio/vfio_ccw_fsm.c
@@ -381,6 +381,27 @@ static void fsm_open(struct vfio_ccw_private *private,
 	spin_unlock_irq(sch->lock);
 }
 
+static void fsm_close(struct vfio_ccw_private *private,
+		      enum vfio_ccw_event event)
+{
+	struct subchannel *sch = private->sch;
+	int ret;
+
+	spin_lock_irq(sch->lock);
+
+	if (!sch->schib.pmcw.ena)
+		goto out_unlock;
+
+	ret = cio_disable_subchannel(sch);
+	if (ret == -EBUSY)
+		vfio_ccw_sch_quiesce(sch);
+
+out_unlock:
+	private->state = VFIO_CCW_STATE_NOT_OPER;
+	spin_unlock_irq(sch->lock);
+	cp_free(&private->cp);
+}
+
 /*
  * Device statemachine
  */
@@ -391,6 +412,7 @@ fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS] = {
 		[VFIO_CCW_EVENT_ASYNC_REQ]	= fsm_async_error,
 		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_disabled_irq,
 		[VFIO_CCW_EVENT_OPEN]		= fsm_open,
+		[VFIO_CCW_EVENT_CLOSE]		= fsm_nop,
 	},
 	[VFIO_CCW_STATE_STANDBY] = {
 		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
@@ -398,6 +420,7 @@ fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS] = {
 		[VFIO_CCW_EVENT_ASYNC_REQ]	= fsm_async_error,
 		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
 		[VFIO_CCW_EVENT_OPEN]		= fsm_nop,
+		[VFIO_CCW_EVENT_CLOSE]		= fsm_notoper,
 	},
 	[VFIO_CCW_STATE_IDLE] = {
 		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
@@ -405,6 +428,7 @@ fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS] = {
 		[VFIO_CCW_EVENT_ASYNC_REQ]	= fsm_async_request,
 		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
 		[VFIO_CCW_EVENT_OPEN]		= fsm_notoper,
+		[VFIO_CCW_EVENT_CLOSE]		= fsm_close,
 	},
 	[VFIO_CCW_STATE_CP_PROCESSING] = {
 		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
@@ -412,6 +436,7 @@ fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS] = {
 		[VFIO_CCW_EVENT_ASYNC_REQ]	= fsm_async_retry,
 		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
 		[VFIO_CCW_EVENT_OPEN]		= fsm_notoper,
+		[VFIO_CCW_EVENT_CLOSE]		= fsm_close,
 	},
 	[VFIO_CCW_STATE_CP_PENDING] = {
 		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
@@ -419,5 +444,6 @@ fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS] = {
 		[VFIO_CCW_EVENT_ASYNC_REQ]	= fsm_async_request,
 		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
 		[VFIO_CCW_EVENT_OPEN]		= fsm_notoper,
+		[VFIO_CCW_EVENT_CLOSE]		= fsm_close,
 	},
 };
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index 9e5c184eab89..38c715da61c6 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -33,9 +33,7 @@ static int vfio_ccw_mdev_reset(struct vfio_ccw_private *private)
 	 * There are still a lot more instructions need to be handled. We
 	 * should come back here later.
 	 */
-	ret = vfio_ccw_sch_quiesce(sch);
-	if (ret)
-		return ret;
+	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_CLOSE);
 
 	ret = cio_enable_subchannel(sch, (u32)(unsigned long)sch);
 	if (!ret)
@@ -64,7 +62,6 @@ static int vfio_ccw_mdev_notifier(struct notifier_block *nb,
 		if (vfio_ccw_mdev_reset(private))
 			return NOTIFY_BAD;
 
-		cp_free(&private->cp);
 		return NOTIFY_OK;
 	}
 
@@ -163,15 +160,9 @@ static void vfio_ccw_mdev_remove(struct mdev_device *mdev)
 	dev_set_drvdata(&mdev->dev, NULL);
 	vfio_unregister_group_dev(&private->vdev);
 
-	if ((private->state != VFIO_CCW_STATE_NOT_OPER) &&
-	    (private->state != VFIO_CCW_STATE_STANDBY)) {
-		if (!vfio_ccw_sch_quiesce(private->sch))
-			private->state = VFIO_CCW_STATE_STANDBY;
-		/* The state will be NOT_OPER on error. */
-	}
+	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_CLOSE);
 
 	vfio_uninit_group_dev(&private->vdev);
-	cp_free(&private->cp);
 	atomic_inc(&private->avail);
 }
 
@@ -222,7 +213,6 @@ static void vfio_ccw_mdev_close_device(struct vfio_device *vdev)
 		/* The state will be NOT_OPER on error. */
 	}
 
-	cp_free(&private->cp);
 	vfio_ccw_unregister_dev_regions(private);
 	vfio_unregister_notifier(vdev->dev, VFIO_IOMMU_NOTIFY, &private->nb);
 }
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index d7ba6846e5c5..02a4a5edd00c 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -144,6 +144,7 @@ enum vfio_ccw_event {
 	VFIO_CCW_EVENT_INTERRUPT,
 	VFIO_CCW_EVENT_ASYNC_REQ,
 	VFIO_CCW_EVENT_OPEN,
+	VFIO_CCW_EVENT_CLOSE,
 	/* last element! */
 	NR_VFIO_CCW_EVENTS
 };
-- 
2.32.0

[PATCH v1 11/18] vfio/ccw: Refactor vfio_ccw_mdev_reset

[Eric Farman]

Use both the FSM Close and Open events when resetting an mdev,
rather than making a separate call to cio_enable_subchannel().

Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_ops.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index 38c715da61c6..386eb74ce219 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -21,25 +21,21 @@ static const struct vfio_device_ops vfio_ccw_dev_ops;
 
 static int vfio_ccw_mdev_reset(struct vfio_ccw_private *private)
 {
-	struct subchannel *sch;
-	int ret;
-
-	sch = private->sch;
 	/*
-	 * TODO:
-	 * In the cureent stage, some things like "no I/O running" and "no
-	 * interrupt pending" are clear, but we are not sure what other state
-	 * we need to care about.
-	 * There are still a lot more instructions need to be handled. We
-	 * should come back here later.
+	 * If the FSM state is seen as Not Operational after closing
+	 * and re-opening the mdev, return an error.
+	 *
+	 * Otherwise, change the FSM from STANDBY to IDLE which is
+	 * normally done by vfio_ccw_mdev_probe() in current lifecycle.
 	 */
 	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_CLOSE);
+	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_OPEN);
+	if (private->state == VFIO_CCW_STATE_NOT_OPER)
+		return -EINVAL;
 
-	ret = cio_enable_subchannel(sch, (u32)(unsigned long)sch);
-	if (!ret)
-		private->state = VFIO_CCW_STATE_IDLE;
+	private->state = VFIO_CCW_STATE_IDLE;
 
-	return ret;
+	return 0;
 }
 
 static int vfio_ccw_mdev_notifier(struct notifier_block *nb,
-- 
2.32.0

[PATCH v1 12/18] vfio/ccw: Move FSM open/close to MDEV open/close

[Eric Farman]

Part of the confusion that has existed is the FSM lifecycle of
subchannels between the common CSS driver and the vfio-ccw driver.
During configuration, the FSM state goes from NOT_OPER to STANDBY
to IDLE, but then back to NOT_OPER. For example:

	vfio_ccw_sch_probe:		VFIO_CCW_STATE_NOT_OPER
	vfio_ccw_sch_probe:		VFIO_CCW_STATE_STANDBY
	vfio_ccw_mdev_probe:		VFIO_CCW_STATE_IDLE
	vfio_ccw_mdev_remove:		VFIO_CCW_STATE_NOT_OPER
	vfio_ccw_sch_remove:		VFIO_CCW_STATE_NOT_OPER
	vfio_ccw_sch_shutdown:		VFIO_CCW_STATE_NOT_OPER

Rearrange the open/close events to align with the mdev open/close,
to better manage the memory and state of the devices as time
progresses. Specifically, make mdev_open() perform the FSM open,
and mdev_close() perform the FSM close instead of reset (which is
both close and open).

This makes the NOT_OPER state a dead-end path, indicating the
device is probably not recoverable without fully probing and
re-configuring the device.

This has the nice side-effect of removing a number of special-cases
where the FSM state is managed outside of the FSM itself (such as
the aforementioned mdev_close() routine).

Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_drv.c | 11 +++--------
 drivers/s390/cio/vfio_ccw_fsm.c | 30 ++++++++++++++++++++++--------
 drivers/s390/cio/vfio_ccw_ops.c | 26 +++++++++++---------------
 3 files changed, 36 insertions(+), 31 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 34995ad00332..e4b285953a45 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -143,7 +143,7 @@ static struct vfio_ccw_private *vfio_ccw_alloc_private(struct subchannel *sch)
 
 	private->sch = sch;
 	mutex_init(&private->io_mutex);
-	private->state = VFIO_CCW_STATE_NOT_OPER;
+	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);
@@ -227,21 +227,15 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
 
 	dev_set_drvdata(&sch->dev, private);
 
-	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_OPEN);
-	if (private->state == VFIO_CCW_STATE_NOT_OPER)
-		goto out_free;
-
 	ret = mdev_register_device(&sch->dev, &vfio_ccw_mdev_ops);
 	if (ret)
-		goto out_disable;
+		goto out_free;
 
 	VFIO_CCW_MSG_EVENT(4, "bound to subchannel %x.%x.%04x\n",
 			   sch->schid.cssid, sch->schid.ssid,
 			   sch->schid.sch_no);
 	return 0;
 
-out_disable:
-	cio_disable_subchannel(sch);
 out_free:
 	dev_set_drvdata(&sch->dev, NULL);
 	vfio_ccw_free_private(private);
@@ -275,6 +269,7 @@ static void vfio_ccw_sch_shutdown(struct subchannel *sch)
 		return;
 
 	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_CLOSE);
+	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_NOT_OPER);
 }
 
 /**
diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
index 926ae618c3fa..7ec64c1e076c 100644
--- a/drivers/s390/cio/vfio_ccw_fsm.c
+++ b/drivers/s390/cio/vfio_ccw_fsm.c
@@ -171,6 +171,7 @@ static void fsm_notoper(struct vfio_ccw_private *private,
 	 */
 	css_sched_sch_todo(sch, SCH_TODO_UNREG);
 	private->state = VFIO_CCW_STATE_NOT_OPER;
+	cp_free(&private->cp);
 }
 
 /*
@@ -376,9 +377,16 @@ static void fsm_open(struct vfio_ccw_private *private,
 	spin_lock_irq(sch->lock);
 	sch->isc = VFIO_CCW_ISC;
 	ret = cio_enable_subchannel(sch, (u32)(unsigned long)sch);
-	if (!ret)
-		private->state = VFIO_CCW_STATE_STANDBY;
+	if (ret)
+		goto err_unlock;
+
+	private->state = VFIO_CCW_STATE_IDLE;
 	spin_unlock_irq(sch->lock);
+	return;
+
+err_unlock:
+	spin_unlock_irq(sch->lock);
+	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_NOT_OPER);
 }
 
 static void fsm_close(struct vfio_ccw_private *private,
@@ -390,16 +398,22 @@ static void fsm_close(struct vfio_ccw_private *private,
 	spin_lock_irq(sch->lock);
 
 	if (!sch->schib.pmcw.ena)
-		goto out_unlock;
+		goto err_unlock;
 
 	ret = cio_disable_subchannel(sch);
 	if (ret == -EBUSY)
 		vfio_ccw_sch_quiesce(sch);
+	if (ret)
+		goto err_unlock;
 
-out_unlock:
-	private->state = VFIO_CCW_STATE_NOT_OPER;
+	private->state = VFIO_CCW_STATE_STANDBY;
 	spin_unlock_irq(sch->lock);
 	cp_free(&private->cp);
+	return;
+
+err_unlock:
+	spin_unlock_irq(sch->lock);
+	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_NOT_OPER);
 }
 
 /*
@@ -411,15 +425,15 @@ fsm_func_t *vfio_ccw_jumptable[NR_VFIO_CCW_STATES][NR_VFIO_CCW_EVENTS] = {
 		[VFIO_CCW_EVENT_IO_REQ]		= fsm_io_error,
 		[VFIO_CCW_EVENT_ASYNC_REQ]	= fsm_async_error,
 		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_disabled_irq,
-		[VFIO_CCW_EVENT_OPEN]		= fsm_open,
+		[VFIO_CCW_EVENT_OPEN]		= fsm_nop,
 		[VFIO_CCW_EVENT_CLOSE]		= fsm_nop,
 	},
 	[VFIO_CCW_STATE_STANDBY] = {
 		[VFIO_CCW_EVENT_NOT_OPER]	= fsm_notoper,
 		[VFIO_CCW_EVENT_IO_REQ]		= fsm_io_error,
 		[VFIO_CCW_EVENT_ASYNC_REQ]	= fsm_async_error,
-		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_irq,
-		[VFIO_CCW_EVENT_OPEN]		= fsm_nop,
+		[VFIO_CCW_EVENT_INTERRUPT]	= fsm_disabled_irq,
+		[VFIO_CCW_EVENT_OPEN]		= fsm_open,
 		[VFIO_CCW_EVENT_CLOSE]		= fsm_notoper,
 	},
 	[VFIO_CCW_STATE_IDLE] = {
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index 386eb74ce219..2afb8f13739f 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -24,17 +24,12 @@ static int vfio_ccw_mdev_reset(struct vfio_ccw_private *private)
 	/*
 	 * If the FSM state is seen as Not Operational after closing
 	 * and re-opening the mdev, return an error.
-	 *
-	 * Otherwise, change the FSM from STANDBY to IDLE which is
-	 * normally done by vfio_ccw_mdev_probe() in current lifecycle.
 	 */
 	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_CLOSE);
 	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_OPEN);
 	if (private->state == VFIO_CCW_STATE_NOT_OPER)
 		return -EINVAL;
 
-	private->state = VFIO_CCW_STATE_IDLE;
-
 	return 0;
 }
 
@@ -121,8 +116,6 @@ static int vfio_ccw_mdev_probe(struct mdev_device *mdev)
 	vfio_init_group_dev(&private->vdev, &mdev->dev,
 			    &vfio_ccw_dev_ops);
 
-	private->state = VFIO_CCW_STATE_IDLE;
-
 	VFIO_CCW_MSG_EVENT(2, "sch %x.%x.%04x: create\n",
 			   private->sch->schid.cssid,
 			   private->sch->schid.ssid,
@@ -137,7 +130,6 @@ static int vfio_ccw_mdev_probe(struct mdev_device *mdev)
 err_atomic:
 	vfio_uninit_group_dev(&private->vdev);
 	atomic_inc(&private->avail);
-	private->state = VFIO_CCW_STATE_STANDBY;
 	return ret;
 }
 
@@ -169,6 +161,10 @@ static int vfio_ccw_mdev_open_device(struct vfio_device *vdev)
 	unsigned long events = VFIO_IOMMU_NOTIFY_DMA_UNMAP;
 	int ret;
 
+	/* Device cannot simply be opened again from this state */
+	if (private->state == VFIO_CCW_STATE_NOT_OPER)
+		return -EINVAL;
+
 	private->nb.notifier_call = vfio_ccw_mdev_notifier;
 
 	ret = vfio_register_notifier(vdev->dev, VFIO_IOMMU_NOTIFY,
@@ -188,6 +184,12 @@ static int vfio_ccw_mdev_open_device(struct vfio_device *vdev)
 	if (ret)
 		goto out_unregister;
 
+	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_OPEN);
+	if (private->state == VFIO_CCW_STATE_NOT_OPER) {
+		ret = -EINVAL;
+		goto out_unregister;
+	}
+
 	return ret;
 
 out_unregister:
@@ -202,13 +204,7 @@ static void vfio_ccw_mdev_close_device(struct vfio_device *vdev)
 	struct vfio_ccw_private *private =
 		container_of(vdev, struct vfio_ccw_private, vdev);
 
-	if ((private->state != VFIO_CCW_STATE_NOT_OPER) &&
-	    (private->state != VFIO_CCW_STATE_STANDBY)) {
-		if (!vfio_ccw_mdev_reset(private))
-			private->state = VFIO_CCW_STATE_STANDBY;
-		/* The state will be NOT_OPER on error. */
-	}
-
+	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_CLOSE);
 	vfio_ccw_unregister_dev_regions(private);
 	vfio_unregister_notifier(vdev->dev, VFIO_IOMMU_NOTIFY, &private->nb);
 }
-- 
2.32.0

[PATCH v1 13/18] vfio/mdev: Consolidate all the device_api sysfs into the core code

[Eric Farman]

From: Jason Gunthorpe <jgg@nvidia.com>

Every driver just emits a static string, simply feed it through the ops
and provide a standard sysfs show function.

Cc: Kirti Wankhede <kwankhede@nvidia.com>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
Cc: Zhi Wang <zhi.a.wang@intel.com>
Cc: Tony Krowiak <akrowiak@linux.ibm.com>
Cc: Jason Herne <jjherne@linux.ibm.com>
Cc: intel-gvt-dev@lists.freedesktop.org
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
Link: https://lore.kernel.org/r/6-v3-57c1502c62fd+2190-ccw_mdev_jgg@nvidia.com/
[farman: added Cc: tags]
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 .../driver-api/vfio-mediated-device.rst       |  4 ++-
 drivers/gpu/drm/i915/gvt/kvmgt.c              |  9 +------
 drivers/s390/cio/vfio_ccw_ops.c               |  9 +------
 drivers/s390/crypto/vfio_ap_ops.c             |  9 +------
 drivers/vfio/mdev/mdev_core.c                 |  2 +-
 drivers/vfio/mdev/mdev_sysfs.c                | 27 ++++++++++++++++---
 include/linux/mdev.h                          |  7 ++---
 samples/vfio-mdev/mbochs.c                    |  9 +------
 samples/vfio-mdev/mdpy.c                      |  9 +------
 samples/vfio-mdev/mtty.c                      | 10 +------
 10 files changed, 36 insertions(+), 59 deletions(-)

diff --git a/Documentation/driver-api/vfio-mediated-device.rst b/Documentation/driver-api/vfio-mediated-device.rst
index 9f26079cacae..f410a1cd98bb 100644
--- a/Documentation/driver-api/vfio-mediated-device.rst
+++ b/Documentation/driver-api/vfio-mediated-device.rst
@@ -137,6 +137,7 @@ The structures in the mdev_parent_ops structure are as follows:
 * mdev_attr_groups: attributes of the mediated device
 * supported_config: attributes to define supported configurations
 * device_driver: device driver to bind for mediated device instances
+* device_api: String to pass through the sysfs file below
 
 The mdev_parent_ops also still has various functions pointers.  Theses exist
 for historical reasons only and shall not be used for new drivers.
@@ -225,7 +226,8 @@ Directories and files under the sysfs for Each Physical Device
 * device_api
 
   This attribute should show which device API is being created, for example,
-  "vfio-pci" for a PCI device.
+  "vfio-pci" for a PCI device. The core code maintins this sysfs using the
+  device_api member of mdev_parent_ops.
 
 * available_instances
 
diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
index 057ec4490104..752d7a1211e6 100644
--- a/drivers/gpu/drm/i915/gvt/kvmgt.c
+++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
@@ -163,12 +163,6 @@ static ssize_t available_instances_show(struct mdev_type *mtype,
 	return sprintf(buf, "%u\n", num);
 }
 
-static ssize_t device_api_show(struct mdev_type *mtype,
-			       struct mdev_type_attribute *attr, char *buf)
-{
-	return sprintf(buf, "%s\n", VFIO_DEVICE_API_PCI_STRING);
-}
-
 static ssize_t description_show(struct mdev_type *mtype,
 				struct mdev_type_attribute *attr, char *buf)
 {
@@ -202,13 +196,11 @@ static ssize_t name_show(struct mdev_type *mtype,
 }
 
 static MDEV_TYPE_ATTR_RO(available_instances);
-static MDEV_TYPE_ATTR_RO(device_api);
 static MDEV_TYPE_ATTR_RO(description);
 static MDEV_TYPE_ATTR_RO(name);
 
 static struct attribute *gvt_type_attrs[] = {
 	&mdev_type_attr_available_instances.attr,
-	&mdev_type_attr_device_api.attr,
 	&mdev_type_attr_description.attr,
 	&mdev_type_attr_name.attr,
 	NULL,
@@ -1767,6 +1759,7 @@ static const struct attribute_group *intel_vgpu_groups[] = {
 
 static struct mdev_parent_ops intel_vgpu_ops = {
 	.mdev_attr_groups       = intel_vgpu_groups,
+	.device_api		= VFIO_DEVICE_API_PCI_STRING,
 	.create			= intel_vgpu_create,
 	.remove			= intel_vgpu_remove,
 
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index 2afb8f13739f..6793c8b3c58b 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -66,13 +66,6 @@ static ssize_t name_show(struct mdev_type *mtype,
 }
 static MDEV_TYPE_ATTR_RO(name);
 
-static ssize_t device_api_show(struct mdev_type *mtype,
-			       struct mdev_type_attribute *attr, char *buf)
-{
-	return sprintf(buf, "%s\n", VFIO_DEVICE_API_CCW_STRING);
-}
-static MDEV_TYPE_ATTR_RO(device_api);
-
 static ssize_t available_instances_show(struct mdev_type *mtype,
 					struct mdev_type_attribute *attr,
 					char *buf)
@@ -86,7 +79,6 @@ static MDEV_TYPE_ATTR_RO(available_instances);
 
 static struct attribute *mdev_types_attrs[] = {
 	&mdev_type_attr_name.attr,
-	&mdev_type_attr_device_api.attr,
 	&mdev_type_attr_available_instances.attr,
 	NULL,
 };
@@ -644,5 +636,6 @@ struct mdev_driver vfio_ccw_mdev_driver = {
 const struct mdev_parent_ops vfio_ccw_mdev_ops = {
 	.owner			= THIS_MODULE,
 	.device_driver		= &vfio_ccw_mdev_driver,
+	.device_api		= VFIO_DEVICE_API_CCW_STRING,
 	.supported_type_groups  = mdev_type_groups,
 };
diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 6e08d04b605d..838b1a3eac8a 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -530,17 +530,9 @@ static ssize_t available_instances_show(struct mdev_type *mtype,
 
 static MDEV_TYPE_ATTR_RO(available_instances);
 
-static ssize_t device_api_show(struct mdev_type *mtype,
-			       struct mdev_type_attribute *attr, char *buf)
-{
-	return sprintf(buf, "%s\n", VFIO_DEVICE_API_AP_STRING);
-}
-
-static MDEV_TYPE_ATTR_RO(device_api);
 
 static struct attribute *vfio_ap_mdev_type_attrs[] = {
 	&mdev_type_attr_name.attr,
-	&mdev_type_attr_device_api.attr,
 	&mdev_type_attr_available_instances.attr,
 	NULL,
 };
@@ -1501,6 +1493,7 @@ static struct mdev_driver vfio_ap_matrix_driver = {
 static const struct mdev_parent_ops vfio_ap_matrix_ops = {
 	.owner			= THIS_MODULE,
 	.device_driver		= &vfio_ap_matrix_driver,
+	.device_api		= VFIO_DEVICE_API_AP_STRING,
 	.supported_type_groups	= vfio_ap_mdev_type_groups,
 };
 
diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
index b314101237fe..c3018e8e6d32 100644
--- a/drivers/vfio/mdev/mdev_core.c
+++ b/drivers/vfio/mdev/mdev_core.c
@@ -129,7 +129,7 @@ int mdev_register_device(struct device *dev, const struct mdev_parent_ops *ops)
 	char *envp[] = { env_string, NULL };
 
 	/* check for mandatory ops */
-	if (!ops || !ops->supported_type_groups)
+	if (!ops || !ops->supported_type_groups || !ops->device_api)
 		return -EINVAL;
 	if (!ops->device_driver && (!ops->create || !ops->remove))
 		return -EINVAL;
diff --git a/drivers/vfio/mdev/mdev_sysfs.c b/drivers/vfio/mdev/mdev_sysfs.c
index f5cf1931c54e..d4b99440d19e 100644
--- a/drivers/vfio/mdev/mdev_sysfs.c
+++ b/drivers/vfio/mdev/mdev_sysfs.c
@@ -74,9 +74,30 @@ static ssize_t create_store(struct mdev_type *mtype,
 
 	return count;
 }
-
 static MDEV_TYPE_ATTR_WO(create);
 
+static ssize_t device_api_show(struct mdev_type *mtype,
+			       struct mdev_type_attribute *attr, char *buf)
+{
+	return sysfs_emit(buf, "%s\n", mtype->parent->ops->device_api);
+}
+static MDEV_TYPE_ATTR_RO(device_api);
+
+static struct attribute *mdev_types_std_attrs[] = {
+	&mdev_type_attr_create.attr,
+	&mdev_type_attr_device_api.attr,
+	NULL,
+};
+
+static struct attribute_group mdev_type_std_group = {
+	.attrs = mdev_types_std_attrs,
+};
+
+static const struct attribute_group *mdev_type_groups[] = {
+	&mdev_type_std_group,
+	NULL,
+};
+
 static void mdev_type_release(struct kobject *kobj)
 {
 	struct mdev_type *type = to_mdev_type(kobj);
@@ -123,7 +144,7 @@ static struct mdev_type *add_mdev_supported_type(struct mdev_parent *parent,
 		return ERR_PTR(ret);
 	}
 
-	ret = sysfs_create_file(&type->kobj, &mdev_type_attr_create.attr);
+	ret = sysfs_create_groups(&type->kobj, mdev_type_groups);
 	if (ret)
 		goto attr_create_failed;
 
@@ -144,7 +165,7 @@ static struct mdev_type *add_mdev_supported_type(struct mdev_parent *parent,
 attrs_failed:
 	kobject_put(type->devices_kobj);
 attr_devices_failed:
-	sysfs_remove_file(&type->kobj, &mdev_type_attr_create.attr);
+	sysfs_remove_groups(&type->kobj, mdev_type_groups);
 attr_create_failed:
 	kobject_del(&type->kobj);
 	kobject_put(&type->kobj);
diff --git a/include/linux/mdev.h b/include/linux/mdev.h
index a5788f592817..14655215417b 100644
--- a/include/linux/mdev.h
+++ b/include/linux/mdev.h
@@ -36,6 +36,7 @@ struct device *mtype_get_parent_dev(struct mdev_type *mtype);
  *
  * @owner:		The module owner.
  * @device_driver:	Which device driver to probe() on newly created devices
+ * @device_api:		String to return for the device_api sysfs
  * @dev_attr_groups:	Attributes of the parent device.
  * @mdev_attr_groups:	Attributes of the mediated device.
  * @supported_type_groups: Attributes to define supported types. It is mandatory
@@ -80,6 +81,7 @@ struct device *mtype_get_parent_dev(struct mdev_type *mtype);
 struct mdev_parent_ops {
 	struct module   *owner;
 	struct mdev_driver *device_driver;
+	const char *device_api;
 	const struct attribute_group **dev_attr_groups;
 	const struct attribute_group **mdev_attr_groups;
 	struct attribute_group **supported_type_groups;
@@ -108,11 +110,6 @@ struct mdev_type_attribute {
 			 size_t count);
 };
 
-#define MDEV_TYPE_ATTR(_name, _mode, _show, _store)		\
-struct mdev_type_attribute mdev_type_attr_##_name =		\
-	__ATTR(_name, _mode, _show, _store)
-#define MDEV_TYPE_ATTR_RW(_name) \
-	struct mdev_type_attribute mdev_type_attr_##_name = __ATTR_RW(_name)
 #define MDEV_TYPE_ATTR_RO(_name) \
 	struct mdev_type_attribute mdev_type_attr_##_name = __ATTR_RO(_name)
 #define MDEV_TYPE_ATTR_WO(_name) \
diff --git a/samples/vfio-mdev/mbochs.c b/samples/vfio-mdev/mbochs.c
index e90c8552cc31..8d3ae97d9d6e 100644
--- a/samples/vfio-mdev/mbochs.c
+++ b/samples/vfio-mdev/mbochs.c
@@ -1358,17 +1358,9 @@ static ssize_t available_instances_show(struct mdev_type *mtype,
 }
 static MDEV_TYPE_ATTR_RO(available_instances);
 
-static ssize_t device_api_show(struct mdev_type *mtype,
-			       struct mdev_type_attribute *attr, char *buf)
-{
-	return sprintf(buf, "%s\n", VFIO_DEVICE_API_PCI_STRING);
-}
-static MDEV_TYPE_ATTR_RO(device_api);
-
 static struct attribute *mdev_types_attrs[] = {
 	&mdev_type_attr_name.attr,
 	&mdev_type_attr_description.attr,
-	&mdev_type_attr_device_api.attr,
 	&mdev_type_attr_available_instances.attr,
 	NULL,
 };
@@ -1417,6 +1409,7 @@ static struct mdev_driver mbochs_driver = {
 static const struct mdev_parent_ops mdev_fops = {
 	.owner			= THIS_MODULE,
 	.device_driver		= &mbochs_driver,
+	.device_api		= VFIO_DEVICE_API_PCI_STRING,
 	.supported_type_groups	= mdev_type_groups,
 };
 
diff --git a/samples/vfio-mdev/mdpy.c b/samples/vfio-mdev/mdpy.c
index fe5d43e797b6..402a7ebe6563 100644
--- a/samples/vfio-mdev/mdpy.c
+++ b/samples/vfio-mdev/mdpy.c
@@ -670,17 +670,9 @@ static ssize_t available_instances_show(struct mdev_type *mtype,
 }
 static MDEV_TYPE_ATTR_RO(available_instances);
 
-static ssize_t device_api_show(struct mdev_type *mtype,
-			       struct mdev_type_attribute *attr, char *buf)
-{
-	return sprintf(buf, "%s\n", VFIO_DEVICE_API_PCI_STRING);
-}
-static MDEV_TYPE_ATTR_RO(device_api);
-
 static struct attribute *mdev_types_attrs[] = {
 	&mdev_type_attr_name.attr,
 	&mdev_type_attr_description.attr,
-	&mdev_type_attr_device_api.attr,
 	&mdev_type_attr_available_instances.attr,
 	NULL,
 };
@@ -728,6 +720,7 @@ static struct mdev_driver mdpy_driver = {
 static const struct mdev_parent_ops mdev_fops = {
 	.owner			= THIS_MODULE,
 	.device_driver          = &mdpy_driver,
+	.device_api		= VFIO_DEVICE_API_PCI_STRING,
 	.supported_type_groups	= mdev_type_groups,
 };
 
diff --git a/samples/vfio-mdev/mtty.c b/samples/vfio-mdev/mtty.c
index a0e1a469bd47..5dc1b6a4c02c 100644
--- a/samples/vfio-mdev/mtty.c
+++ b/samples/vfio-mdev/mtty.c
@@ -1281,17 +1281,8 @@ static ssize_t available_instances_show(struct mdev_type *mtype,
 
 static MDEV_TYPE_ATTR_RO(available_instances);
 
-static ssize_t device_api_show(struct mdev_type *mtype,
-			       struct mdev_type_attribute *attr, char *buf)
-{
-	return sprintf(buf, "%s\n", VFIO_DEVICE_API_PCI_STRING);
-}
-
-static MDEV_TYPE_ATTR_RO(device_api);
-
 static struct attribute *mdev_types_attrs[] = {
 	&mdev_type_attr_name.attr,
-	&mdev_type_attr_device_api.attr,
 	&mdev_type_attr_available_instances.attr,
 	NULL,
 };
@@ -1333,6 +1324,7 @@ static struct mdev_driver mtty_driver = {
 static const struct mdev_parent_ops mdev_fops = {
 	.owner                  = THIS_MODULE,
 	.device_driver		= &mtty_driver,
+	.device_api		= VFIO_DEVICE_API_PCI_STRING,
 	.dev_attr_groups        = mtty_dev_groups,
 	.supported_type_groups  = mdev_type_groups,
 };
-- 
2.32.0

[PATCH v1 14/18] vfio/mdev: Add mdev available instance checking to the core

[Eric Farman]

From: Jason Gunthorpe <jgg@nvidia.com>

Many of the mdev drivers use a simple counter for keeping track of the
available instances. Move this code to the core code and store the counter
in the mdev_type. Implement it using correct locking, fixing mdpy.

Drivers provide a get_available() callback to set the number of available
instances for their mtypes which is fixed at registration time. The core
provides a standard sysfs attribute to return the available_instances.

Cc: Kirti Wankhede <kwankhede@nvidia.com>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Tony Krowiak <akrowiak@linux.ibm.com>
Cc: Jason Herne <jjherne@linux.ibm.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
Link: https://lore.kernel.org/r/7-v3-57c1502c62fd+2190-ccw_mdev_jgg@nvidia.com/
[farman: added Cc: tags]
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 .../driver-api/vfio-mediated-device.rst       |  4 +-
 drivers/s390/cio/vfio_ccw_drv.c               |  1 -
 drivers/s390/cio/vfio_ccw_ops.c               | 26 ++++---------
 drivers/s390/cio/vfio_ccw_private.h           |  2 -
 drivers/s390/crypto/vfio_ap_ops.c             | 32 ++++------------
 drivers/s390/crypto/vfio_ap_private.h         |  2 -
 drivers/vfio/mdev/mdev_core.c                 | 11 +++++-
 drivers/vfio/mdev/mdev_private.h              |  2 +
 drivers/vfio/mdev/mdev_sysfs.c                | 37 +++++++++++++++++++
 include/linux/mdev.h                          |  2 +
 samples/vfio-mdev/mdpy.c                      | 22 +++--------
 11 files changed, 76 insertions(+), 65 deletions(-)

diff --git a/Documentation/driver-api/vfio-mediated-device.rst b/Documentation/driver-api/vfio-mediated-device.rst
index f410a1cd98bb..a4f7f1362fa8 100644
--- a/Documentation/driver-api/vfio-mediated-device.rst
+++ b/Documentation/driver-api/vfio-mediated-device.rst
@@ -106,6 +106,7 @@ structure to represent a mediated device's driver::
 	     int  (*probe)  (struct mdev_device *dev);
 	     void (*remove) (struct mdev_device *dev);
 	     struct device_driver    driver;
+	     unsigned int (*get_available)(struct mdev_type *mtype);
      };
 
 A mediated bus driver for mdev should use this structure in the function calls
@@ -232,7 +233,8 @@ Directories and files under the sysfs for Each Physical Device
 * available_instances
 
   This attribute should show the number of devices of type <type-id> that can be
-  created.
+  created. Drivers can supply a get_available() function pointer to have the
+  core code create and maintain this sysfs automatically.
 
 * [device]
 
diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index e4b285953a45..6dca35f3ceba 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -147,7 +147,6 @@ static struct vfio_ccw_private *vfio_ccw_alloc_private(struct subchannel *sch)
 	INIT_LIST_HEAD(&private->crw);
 	INIT_WORK(&private->io_work, vfio_ccw_sch_io_todo);
 	INIT_WORK(&private->crw_work, vfio_ccw_crw_todo);
-	atomic_set(&private->avail, 1);
 
 	private->cp.guest_cp = kcalloc(CCWCHAIN_LEN_MAX, sizeof(struct ccw1),
 				       GFP_KERNEL);
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index 6793c8b3c58b..60a4855e8ecb 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -66,20 +66,9 @@ static ssize_t name_show(struct mdev_type *mtype,
 }
 static MDEV_TYPE_ATTR_RO(name);
 
-static ssize_t available_instances_show(struct mdev_type *mtype,
-					struct mdev_type_attribute *attr,
-					char *buf)
-{
-	struct vfio_ccw_private *private =
-		dev_get_drvdata(mtype_get_parent_dev(mtype));
-
-	return sprintf(buf, "%d\n", atomic_read(&private->avail));
-}
-static MDEV_TYPE_ATTR_RO(available_instances);
 
 static struct attribute *mdev_types_attrs[] = {
 	&mdev_type_attr_name.attr,
-	&mdev_type_attr_available_instances.attr,
 	NULL,
 };
 
@@ -101,9 +90,6 @@ static int vfio_ccw_mdev_probe(struct mdev_device *mdev)
 	if (private->state == VFIO_CCW_STATE_NOT_OPER)
 		return -ENODEV;
 
-	if (atomic_dec_if_positive(&private->avail) < 0)
-		return -EPERM;
-
 	memset(&private->vdev, 0, sizeof(private->vdev));
 	vfio_init_group_dev(&private->vdev, &mdev->dev,
 			    &vfio_ccw_dev_ops);
@@ -115,13 +101,12 @@ static int vfio_ccw_mdev_probe(struct mdev_device *mdev)
 
 	ret = vfio_register_emulated_iommu_dev(&private->vdev);
 	if (ret)
-		goto err_atomic;
+		goto err_init;
 	dev_set_drvdata(&mdev->dev, private);
 	return 0;
 
-err_atomic:
+err_init:
 	vfio_uninit_group_dev(&private->vdev);
-	atomic_inc(&private->avail);
 	return ret;
 }
 
@@ -143,7 +128,6 @@ static void vfio_ccw_mdev_remove(struct mdev_device *mdev)
 	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_CLOSE);
 
 	vfio_uninit_group_dev(&private->vdev);
-	atomic_inc(&private->avail);
 }
 
 static int vfio_ccw_mdev_open_device(struct vfio_device *vdev)
@@ -614,6 +598,11 @@ static void vfio_ccw_mdev_request(struct vfio_device *vdev, unsigned int count)
 	}
 }
 
+static unsigned int vfio_ccw_get_available(struct mdev_type *mtype)
+{
+	return 1;
+}
+
 static const struct vfio_device_ops vfio_ccw_dev_ops = {
 	.open_device = vfio_ccw_mdev_open_device,
 	.close_device = vfio_ccw_mdev_close_device,
@@ -631,6 +620,7 @@ struct mdev_driver vfio_ccw_mdev_driver = {
 	},
 	.probe = vfio_ccw_mdev_probe,
 	.remove = vfio_ccw_mdev_remove,
+	.get_available = vfio_ccw_get_available,
 };
 
 const struct mdev_parent_ops vfio_ccw_mdev_ops = {
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index 02a4a5edd00c..e568fd6bcf2a 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -72,7 +72,6 @@ struct vfio_ccw_crw {
  * @sch: pointer to the subchannel
  * @state: internal state of the device
  * @completion: synchronization helper of the I/O completion
- * @avail: available for creating a mediated device
  * @nb: notifier for vfio events
  * @io_region: MMIO region to input/output I/O arguments/results
  * @io_mutex: protect against concurrent update of I/O regions
@@ -95,7 +94,6 @@ struct vfio_ccw_private {
 	struct subchannel	*sch;
 	int			state;
 	struct completion	*completion;
-	atomic_t		avail;
 	struct notifier_block	nb;
 	struct ccw_io_region	*io_region;
 	struct mutex		io_mutex;
diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index 838b1a3eac8a..4c62ba9c72d8 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -462,14 +462,9 @@ static int vfio_ap_mdev_probe(struct mdev_device *mdev)
 	struct ap_matrix_mdev *matrix_mdev;
 	int ret;
 
-	if ((atomic_dec_if_positive(&matrix_dev->available_instances) < 0))
-		return -EPERM;
-
 	matrix_mdev = kzalloc(sizeof(*matrix_mdev), GFP_KERNEL);
-	if (!matrix_mdev) {
-		ret = -ENOMEM;
-		goto err_dec_available;
-	}
+	if (!matrix_mdev)
+		return -ENOMEM;
 	vfio_init_group_dev(&matrix_mdev->vdev, &mdev->dev,
 			    &vfio_ap_matrix_dev_ops);
 
@@ -492,8 +487,6 @@ static int vfio_ap_mdev_probe(struct mdev_device *mdev)
 	mutex_unlock(&matrix_dev->lock);
 	vfio_uninit_group_dev(&matrix_mdev->vdev);
 	kfree(matrix_mdev);
-err_dec_available:
-	atomic_inc(&matrix_dev->available_instances);
 	return ret;
 }
 
@@ -509,7 +502,6 @@ static void vfio_ap_mdev_remove(struct mdev_device *mdev)
 	mutex_unlock(&matrix_dev->lock);
 	vfio_uninit_group_dev(&matrix_mdev->vdev);
 	kfree(matrix_mdev);
-	atomic_inc(&matrix_dev->available_instances);
 }
 
 static ssize_t name_show(struct mdev_type *mtype,
@@ -520,20 +512,8 @@ static ssize_t name_show(struct mdev_type *mtype,
 
 static MDEV_TYPE_ATTR_RO(name);
 
-static ssize_t available_instances_show(struct mdev_type *mtype,
-					struct mdev_type_attribute *attr,
-					char *buf)
-{
-	return sprintf(buf, "%d\n",
-		       atomic_read(&matrix_dev->available_instances));
-}
-
-static MDEV_TYPE_ATTR_RO(available_instances);
-
-
 static struct attribute *vfio_ap_mdev_type_attrs[] = {
 	&mdev_type_attr_name.attr,
-	&mdev_type_attr_available_instances.attr,
 	NULL,
 };
 
@@ -1473,6 +1453,11 @@ static ssize_t vfio_ap_mdev_ioctl(struct vfio_device *vdev,
 	return ret;
 }
 
+static unsigned int vfio_ap_mdev_get_available(struct mdev_type *mtype)
+{
+	return MAX_ZDEV_ENTRIES_EXT;
+}
+
 static const struct vfio_device_ops vfio_ap_matrix_dev_ops = {
 	.open_device = vfio_ap_mdev_open_device,
 	.close_device = vfio_ap_mdev_close_device,
@@ -1488,6 +1473,7 @@ static struct mdev_driver vfio_ap_matrix_driver = {
 	},
 	.probe = vfio_ap_mdev_probe,
 	.remove = vfio_ap_mdev_remove,
+	.get_available = vfio_ap_mdev_get_available,
 };
 
 static const struct mdev_parent_ops vfio_ap_matrix_ops = {
@@ -1501,8 +1487,6 @@ int vfio_ap_mdev_register(void)
 {
 	int ret;
 
-	atomic_set(&matrix_dev->available_instances, MAX_ZDEV_ENTRIES_EXT);
-
 	ret = mdev_register_driver(&vfio_ap_matrix_driver);
 	if (ret)
 		return ret;
diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
index 648fcaf8104a..5c0cbf03074b 100644
--- a/drivers/s390/crypto/vfio_ap_private.h
+++ b/drivers/s390/crypto/vfio_ap_private.h
@@ -29,7 +29,6 @@
  * struct ap_matrix_dev - Contains the data for the matrix device.
  *
  * @device:	generic device structure associated with the AP matrix device
- * @available_instances: number of mediated matrix devices that can be created
  * @info:	the struct containing the output from the PQAP(QCI) instruction
  * @mdev_list:	the list of mediated matrix devices created
  * @lock:	mutex for locking the AP matrix device. This lock will be
@@ -41,7 +40,6 @@
  */
 struct ap_matrix_dev {
 	struct device device;
-	atomic_t available_instances;
 	struct ap_config_info info;
 	struct list_head mdev_list;
 	struct mutex lock;
diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
index c3018e8e6d32..bb27ca0db948 100644
--- a/drivers/vfio/mdev/mdev_core.c
+++ b/drivers/vfio/mdev/mdev_core.c
@@ -25,7 +25,7 @@ static DEFINE_MUTEX(parent_list_lock);
 static struct class_compat *mdev_bus_compat_class;
 
 static LIST_HEAD(mdev_list);
-static DEFINE_MUTEX(mdev_list_lock);
+DEFINE_MUTEX(mdev_list_lock);
 
 struct device *mdev_parent_dev(struct mdev_device *mdev)
 {
@@ -245,6 +245,7 @@ static void mdev_device_release(struct device *dev)
 
 	mutex_lock(&mdev_list_lock);
 	list_del(&mdev->next);
+	mdev->type->available++;
 	mutex_unlock(&mdev_list_lock);
 
 	dev_dbg(&mdev->dev, "MDEV: destroying\n");
@@ -268,6 +269,14 @@ int mdev_device_create(struct mdev_type *type, const guid_t *uuid)
 		}
 	}
 
+	if (drv && drv->get_available) {
+		if (!type->available) {
+			mutex_unlock(&mdev_list_lock);
+			return -EUSERS;
+		}
+		type->available--;
+	}
+
 	mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
 	if (!mdev) {
 		mutex_unlock(&mdev_list_lock);
diff --git a/drivers/vfio/mdev/mdev_private.h b/drivers/vfio/mdev/mdev_private.h
index afbad7b0a14a..83586b070233 100644
--- a/drivers/vfio/mdev/mdev_private.h
+++ b/drivers/vfio/mdev/mdev_private.h
@@ -29,6 +29,7 @@ struct mdev_type {
 	struct kobject *devices_kobj;
 	struct mdev_parent *parent;
 	struct list_head next;
+	unsigned int available;
 	unsigned int type_group_id;
 };
 
@@ -38,6 +39,7 @@ struct mdev_type {
 	container_of(_kobj, struct mdev_type, kobj)
 
 extern struct mdev_driver vfio_mdev_driver;
+extern struct mutex mdev_list_lock;
 
 int  parent_create_sysfs_files(struct mdev_parent *parent);
 void parent_remove_sysfs_files(struct mdev_parent *parent);
diff --git a/drivers/vfio/mdev/mdev_sysfs.c b/drivers/vfio/mdev/mdev_sysfs.c
index d4b99440d19e..b3129dfc27ef 100644
--- a/drivers/vfio/mdev/mdev_sysfs.c
+++ b/drivers/vfio/mdev/mdev_sysfs.c
@@ -93,8 +93,41 @@ static struct attribute_group mdev_type_std_group = {
 	.attrs = mdev_types_std_attrs,
 };
 
+/* mdev_type attribute used by drivers that have an get_available() op */
+static ssize_t available_instances_show(struct mdev_type *mtype,
+					struct mdev_type_attribute *attr,
+					char *buf)
+{
+	unsigned int available;
+
+	mutex_lock(&mdev_list_lock);
+	available = mtype->available;
+	mutex_unlock(&mdev_list_lock);
+
+	return sysfs_emit(buf, "%u\n", available);
+}
+static MDEV_TYPE_ATTR_RO(available_instances);
+static umode_t available_instances_is_visible(struct kobject *kobj,
+					      struct attribute *attr, int n)
+{
+	struct mdev_type *type = to_mdev_type(kobj);
+
+	if (!type->parent->ops->device_driver->get_available)
+		return 0;
+	return attr->mode;
+}
+static struct attribute *mdev_types_name_attrs[] = {
+	&mdev_type_attr_available_instances.attr,
+	NULL,
+};
+static struct attribute_group mdev_type_available_instances_group = {
+	.attrs = mdev_types_name_attrs,
+	.is_visible = available_instances_is_visible,
+};
+
 static const struct attribute_group *mdev_type_groups[] = {
 	&mdev_type_std_group,
+	&mdev_type_available_instances_group,
 	NULL,
 };
 
@@ -136,6 +169,10 @@ static struct mdev_type *add_mdev_supported_type(struct mdev_parent *parent,
 	mdev_get_parent(parent);
 	type->type_group_id = type_group_id;
 
+	if (parent->ops->device_driver->get_available)
+		type->available =
+			parent->ops->device_driver->get_available(type);
+
 	ret = kobject_init_and_add(&type->kobj, &mdev_type_ktype, NULL,
 				   "%s-%s", dev_driver_string(parent->dev),
 				   group->name);
diff --git a/include/linux/mdev.h b/include/linux/mdev.h
index 14655215417b..0ce1bb3dabd0 100644
--- a/include/linux/mdev.h
+++ b/include/linux/mdev.h
@@ -120,12 +120,14 @@ struct mdev_type_attribute {
  * @probe: called when new device created
  * @remove: called when device removed
  * @driver: device driver structure
+ * @get_available: Return the max number of instances that can be created
  *
  **/
 struct mdev_driver {
 	int (*probe)(struct mdev_device *dev);
 	void (*remove)(struct mdev_device *dev);
 	struct device_driver driver;
+	unsigned int (*get_available)(struct mdev_type *mtype);
 };
 
 static inline void *mdev_get_drvdata(struct mdev_device *mdev)
diff --git a/samples/vfio-mdev/mdpy.c b/samples/vfio-mdev/mdpy.c
index 402a7ebe6563..d7da6ed35657 100644
--- a/samples/vfio-mdev/mdpy.c
+++ b/samples/vfio-mdev/mdpy.c
@@ -84,7 +84,6 @@ static dev_t		mdpy_devt;
 static struct class	*mdpy_class;
 static struct cdev	mdpy_cdev;
 static struct device	mdpy_dev;
-static u32		mdpy_count;
 static const struct vfio_device_ops mdpy_dev_ops;
 
 /* State of each mdev device */
@@ -225,9 +224,6 @@ static int mdpy_probe(struct mdev_device *mdev)
 	u32 fbsize;
 	int ret;
 
-	if (mdpy_count >= max_devices)
-		return -ENOMEM;
-
 	mdev_state = kzalloc(sizeof(struct mdev_state), GFP_KERNEL);
 	if (mdev_state == NULL)
 		return -ENOMEM;
@@ -256,8 +252,6 @@ static int mdpy_probe(struct mdev_device *mdev)
 	mdpy_create_config_space(mdev_state);
 	mdpy_reset(mdev_state);
 
-	mdpy_count++;
-
 	ret = vfio_register_emulated_iommu_dev(&mdev_state->vdev);
 	if (ret)
 		goto err_mem;
@@ -284,8 +278,6 @@ static void mdpy_remove(struct mdev_device *mdev)
 	kfree(mdev_state->vconfig);
 	vfio_uninit_group_dev(&mdev_state->vdev);
 	kfree(mdev_state);
-
-	mdpy_count--;
 }
 
 static ssize_t mdpy_read(struct vfio_device *vdev, char __user *buf,
@@ -662,18 +654,10 @@ static ssize_t description_show(struct mdev_type *mtype,
 }
 static MDEV_TYPE_ATTR_RO(description);
 
-static ssize_t available_instances_show(struct mdev_type *mtype,
-					struct mdev_type_attribute *attr,
-					char *buf)
-{
-	return sprintf(buf, "%d\n", max_devices - mdpy_count);
-}
-static MDEV_TYPE_ATTR_RO(available_instances);
 
 static struct attribute *mdev_types_attrs[] = {
 	&mdev_type_attr_name.attr,
 	&mdev_type_attr_description.attr,
-	&mdev_type_attr_available_instances.attr,
 	NULL,
 };
 
@@ -706,6 +690,11 @@ static const struct vfio_device_ops mdpy_dev_ops = {
 	.mmap = mdpy_mmap,
 };
 
+static unsigned int mdpy_get_available(struct mdev_type *mtype)
+{
+	return max_devices;
+}
+
 static struct mdev_driver mdpy_driver = {
 	.driver = {
 		.name = "mdpy",
@@ -715,6 +704,7 @@ static struct mdev_driver mdpy_driver = {
 	},
 	.probe = mdpy_probe,
 	.remove	= mdpy_remove,
+	.get_available = mdpy_get_available,
 };
 
 static const struct mdev_parent_ops mdev_fops = {
-- 
2.32.0

[PATCH v1 15/18] vfio/ccw: Manage private with mdev

[Eric Farman]

Simplify the lifecycle of a vfio-ccw device by moving the alloc/free
of the vfio-ccw private struct to the mdev probe/remove processing,
rather than the subchannel.

Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_drv.c     | 35 ++++++++---------------------
 drivers/s390/cio/vfio_ccw_ops.c     | 35 +++++++++++++----------------
 drivers/s390/cio/vfio_ccw_private.h |  5 +++++
 3 files changed, 29 insertions(+), 46 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 6dca35f3ceba..5928460854ec 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -133,7 +133,8 @@ 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 mdev_device *mdev,
+						struct subchannel *sch)
 {
 	struct vfio_ccw_private *private;
 
@@ -141,6 +142,8 @@ static struct vfio_ccw_private *vfio_ccw_alloc_private(struct subchannel *sch)
 	if (!private)
 		return ERR_PTR(-ENOMEM);
 
+	vfio_init_group_dev(&private->vdev, &mdev->dev, &vfio_ccw_dev_ops);
+
 	private->sch = sch;
 	mutex_init(&private->io_mutex);
 	private->state = VFIO_CCW_STATE_STANDBY;
@@ -186,11 +189,12 @@ static struct vfio_ccw_private *vfio_ccw_alloc_private(struct subchannel *sch)
 	kfree(private->cp.guest_cp);
 out_free_private:
 	mutex_destroy(&private->io_mutex);
+	vfio_uninit_group_dev(&private->vdev);
 	kfree(private);
 	return ERR_PTR(-ENOMEM);
 }
 
-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;
 
@@ -205,14 +209,14 @@ static void vfio_ccw_free_private(struct vfio_ccw_private *private)
 	kmem_cache_free(vfio_ccw_io_region, private->io_region);
 	kfree(private->cp.guest_cp);
 	mutex_destroy(&private->io_mutex);
+	vfio_uninit_group_dev(&private->vdev);
 	kfree(private);
 }
 
 static int vfio_ccw_sch_probe(struct subchannel *sch)
 {
 	struct pmcw *pmcw = &sch->schib.pmcw;
-	struct vfio_ccw_private *private;
-	int ret = -ENOMEM;
+	int ret;
 
 	if (pmcw->qf) {
 		dev_warn(&sch->dev, "vfio: ccw: does not support QDIO: %s\n",
@@ -220,41 +224,20 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
 		return -ENODEV;
 	}
 
-	private = vfio_ccw_alloc_private(sch);
-	if (IS_ERR(private))
-		return PTR_ERR(private);
-
-	dev_set_drvdata(&sch->dev, private);
-
 	ret = mdev_register_device(&sch->dev, &vfio_ccw_mdev_ops);
 	if (ret)
-		goto out_free;
+		return ret;
 
 	VFIO_CCW_MSG_EVENT(4, "bound to subchannel %x.%x.%04x\n",
 			   sch->schid.cssid, sch->schid.ssid,
 			   sch->schid.sch_no);
 	return 0;
-
-out_free:
-	dev_set_drvdata(&sch->dev, NULL);
-	vfio_ccw_free_private(private);
-	return ret;
 }
 
 static void vfio_ccw_sch_remove(struct subchannel *sch)
 {
-	struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev);
-
-	if (!private)
-		return;
-
-	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_CLOSE);
 	mdev_unregister_device(&sch->dev);
 
-	dev_set_drvdata(&sch->dev, NULL);
-
-	vfio_ccw_free_private(private);
-
 	VFIO_CCW_MSG_EVENT(4, "unbound from subchannel %x.%x.%04x\n",
 			   sch->schid.cssid, sch->schid.ssid,
 			   sch->schid.sch_no);
diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
index 60a4855e8ecb..1ce7079c9dbb 100644
--- a/drivers/s390/cio/vfio_ccw_ops.c
+++ b/drivers/s390/cio/vfio_ccw_ops.c
@@ -17,8 +17,6 @@
 
 #include "vfio_ccw_private.h"
 
-static const struct vfio_device_ops vfio_ccw_dev_ops;
-
 static int vfio_ccw_mdev_reset(struct vfio_ccw_private *private)
 {
 	/*
@@ -84,29 +82,28 @@ static struct attribute_group *mdev_type_groups[] = {
 
 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_private *private;
 	int ret;
 
-	if (private->state == VFIO_CCW_STATE_NOT_OPER)
-		return -ENODEV;
-
-	memset(&private->vdev, 0, sizeof(private->vdev));
-	vfio_init_group_dev(&private->vdev, &mdev->dev,
-			    &vfio_ccw_dev_ops);
+	private = vfio_ccw_alloc_private(mdev, sch);
+	if (IS_ERR(private))
+		return PTR_ERR(private);
 
 	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)
-		goto err_init;
+		goto err_alloc;
 	dev_set_drvdata(&mdev->dev, private);
+	dev_set_drvdata(&sch->dev, private);
 	return 0;
 
-err_init:
-	vfio_uninit_group_dev(&private->vdev);
+err_alloc:
+	vfio_ccw_free_private(private);
 	return ret;
 }
 
@@ -122,12 +119,10 @@ static void vfio_ccw_mdev_remove(struct mdev_device *mdev)
 			   private->sch->schid.ssid,
 			   private->sch->schid.sch_no);
 
+	dev_set_drvdata(&private->sch->dev, NULL);
 	dev_set_drvdata(&mdev->dev, NULL);
 	vfio_unregister_group_dev(&private->vdev);
-
-	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_CLOSE);
-
-	vfio_uninit_group_dev(&private->vdev);
+	vfio_ccw_free_private(private);
 }
 
 static int vfio_ccw_mdev_open_device(struct vfio_device *vdev)
@@ -603,7 +598,7 @@ static unsigned int vfio_ccw_get_available(struct mdev_type *mtype)
 	return 1;
 }
 
-static const struct vfio_device_ops vfio_ccw_dev_ops = {
+const struct vfio_device_ops vfio_ccw_dev_ops = {
 	.open_device = vfio_ccw_mdev_open_device,
 	.close_device = vfio_ccw_mdev_close_device,
 	.read = vfio_ccw_mdev_read,
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index e568fd6bcf2a..bf11ebd0d32a 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -117,8 +117,13 @@ struct vfio_ccw_private {
 
 extern int vfio_ccw_sch_quiesce(struct subchannel *sch);
 
+struct vfio_ccw_private *vfio_ccw_alloc_private(struct mdev_device *mdev,
+						struct subchannel *sch);
+void vfio_ccw_free_private(struct vfio_ccw_private *private);
+
 extern struct mdev_driver vfio_ccw_mdev_driver;
 extern const struct mdev_parent_ops vfio_ccw_mdev_ops;
+extern const struct vfio_device_ops vfio_ccw_dev_ops;
 
 /*
  * States of the device statemachine.
-- 
2.32.0

[PATCH v1 16/18] vfio/ccw: Create a get_private routine

[Eric Farman]

Refactor out the logic of getting the vfio_ccw_private struct from
the subchannel, so that it can be synchronized with vfio better.

Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_drv.c     | 10 +++++-----
 drivers/s390/cio/vfio_ccw_private.h | 14 ++++++++++++++
 2 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 5928460854ec..c8cbbad6e1c5 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -37,7 +37,7 @@ 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_private *private = vfio_ccw_get_private(sch);
 	DECLARE_COMPLETION_ONSTACK(completion);
 	int iretry, ret = 0;
 
@@ -124,7 +124,7 @@ 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_private *private = vfio_ccw_get_private(sch);
 
 	if (WARN_ON(!private))
 		return;
@@ -245,7 +245,7 @@ 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_private *private = vfio_ccw_get_private(sch);
 
 	if (!private)
 		return;
@@ -266,7 +266,7 @@ 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_private *private = vfio_ccw_get_private(sch);
 	unsigned long flags;
 	int rc = -EAGAIN;
 
@@ -321,7 +321,7 @@ 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_private *private = vfio_ccw_get_private(sch);
 	int mask = chp_ssd_get_mask(&sch->ssd_info, link);
 	int retry = 255;
 
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index bf11ebd0d32a..3833204bd388 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -24,6 +24,8 @@
 #include "css.h"
 #include "vfio_ccw_cp.h"
 
+struct mdev_device;
+
 #define VFIO_CCW_OFFSET_SHIFT   10
 #define VFIO_CCW_OFFSET_TO_INDEX(off)	(off >> VFIO_CCW_OFFSET_SHIFT)
 #define VFIO_CCW_INDEX_TO_OFFSET(index)	((u64)(index) << VFIO_CCW_OFFSET_SHIFT)
@@ -125,6 +127,18 @@ extern struct mdev_driver vfio_ccw_mdev_driver;
 extern const struct mdev_parent_ops vfio_ccw_mdev_ops;
 extern const struct vfio_device_ops vfio_ccw_dev_ops;
 
+static inline struct vfio_ccw_private *vfio_ccw_get_private(struct subchannel *sch)
+{
+	struct vfio_ccw_private *private;
+
+	if (!sch)
+		return NULL;
+
+	private = dev_get_drvdata(&sch->dev);
+
+	return private;
+}
+
 /*
  * States of the device statemachine.
  */
-- 
2.32.0

[PATCH v1 17/18] vfio: Export vfio_device_try_get()

[Eric Farman]

From: Jason Gunthorpe <jgg@nvidia.com>

vfio_ccw will need it.

Cc: Alex Williamson <alex.williamson@redhat.com>
Cc: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
Link: https://lore.kernel.org/r/9-v3-57c1502c62fd+2190-ccw_mdev_jgg@nvidia.com/
[farman: added Cc: tags]
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/vfio/vfio.c  | 3 ++-
 include/linux/vfio.h | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
index a4555014bd1e..a1f5904b5ea3 100644
--- a/drivers/vfio/vfio.c
+++ b/drivers/vfio/vfio.c
@@ -499,10 +499,11 @@ void vfio_device_put(struct vfio_device *device)
 }
 EXPORT_SYMBOL_GPL(vfio_device_put);
 
-static bool vfio_device_try_get(struct vfio_device *device)
+bool vfio_device_try_get(struct vfio_device *device)
 {
 	return refcount_inc_not_zero(&device->refcount);
 }
+EXPORT_SYMBOL_GPL(vfio_device_try_get);
 
 static struct vfio_device *vfio_group_get_device(struct vfio_group *group,
 						 struct device *dev)
diff --git a/include/linux/vfio.h b/include/linux/vfio.h
index 66dda06ec42d..bcd6eaaea9a5 100644
--- a/include/linux/vfio.h
+++ b/include/linux/vfio.h
@@ -126,6 +126,7 @@ int vfio_register_group_dev(struct vfio_device *device);
 int vfio_register_emulated_iommu_dev(struct vfio_device *device);
 void vfio_unregister_group_dev(struct vfio_device *device);
 extern struct vfio_device *vfio_device_get_from_dev(struct device *dev);
+bool vfio_device_try_get(struct vfio_device *device);
 extern void vfio_device_put(struct vfio_device *device);
 
 int vfio_assign_device_set(struct vfio_device *device, void *set_id);
-- 
2.32.0

[PATCH v1 18/18] vfio/ccw: Manage ccw/mdev reference counts

[Eric Farman]

Take a reference to the vfio device when looking at the
corresponding vfio_ccw_private struct, and put it back
once we are finished.

Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Eric Farman <farman@linux.ibm.com>
---
 drivers/s390/cio/vfio_ccw_drv.c     | 9 ++++++++-
 drivers/s390/cio/vfio_ccw_private.h | 2 ++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index c8cbbad6e1c5..2e55fa45f76c 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -71,6 +71,7 @@ int vfio_ccw_sch_quiesce(struct subchannel *sch)
 		ret = cio_disable_subchannel(sch);
 	} while (ret == -EBUSY);
 
+	vfio_device_put(&private->vdev);
 	return ret;
 }
 
@@ -131,6 +132,7 @@ static void vfio_ccw_sch_irq(struct subchannel *sch)
 
 	inc_irq_stat(IRQIO_CIO);
 	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_INTERRUPT);
+	vfio_device_put(&private->vdev);
 }
 
 struct vfio_ccw_private *vfio_ccw_alloc_private(struct mdev_device *mdev,
@@ -252,6 +254,7 @@ static void vfio_ccw_sch_shutdown(struct subchannel *sch)
 
 	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_CLOSE);
 	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_NOT_OPER);
+	vfio_device_put(&private->vdev);
 }
 
 /**
@@ -287,6 +290,7 @@ static int vfio_ccw_sch_event(struct subchannel *sch, int process)
 
 out_unlock:
 	spin_unlock_irqrestore(sch->lock, flags);
+	vfio_device_put(&private->vdev);
 
 	return rc;
 }
@@ -334,8 +338,10 @@ static int vfio_ccw_chp_event(struct subchannel *sch,
 			   sch->schid.ssid, sch->schid.sch_no,
 			   mask, event);
 
-	if (cio_update_schib(sch))
+	if (cio_update_schib(sch)) {
+		vfio_device_put(&private->vdev);
 		return -ENODEV;
+	}
 
 	switch (event) {
 	case CHP_VARY_OFF:
@@ -365,6 +371,7 @@ static int vfio_ccw_chp_event(struct subchannel *sch,
 		break;
 	}
 
+	vfio_device_put(&private->vdev);
 	return 0;
 }
 
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index 3833204bd388..be238cf277ff 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -135,6 +135,8 @@ static inline struct vfio_ccw_private *vfio_ccw_get_private(struct subchannel *s
 		return NULL;
 
 	private = dev_get_drvdata(&sch->dev);
+	if (private && !vfio_device_try_get(&private->vdev))
+		private = NULL;
 
 	return private;
 }
-- 
2.32.0

Re: [PATCH v1 01/18] vfio/ccw: Remove UUID from s390 debug log

[Jason Gunthorpe]

On Thu, Jun 02, 2022 at 07:19:31PM +0200, Eric Farman wrote:
> From: Michael Kawano <mkawano@linux.ibm.com>
> 
> As vfio-ccw devices are created/destroyed, the uuid of the associated
> mdevs that are recorded in $S390DBF/vfio_ccw_msg/sprintf get lost as
> they are created using pointers passed by reference.
> 
> This is a deliberate design point of s390dbf, but it leaves the uuid
> in these traces less than useful. Since the subchannels are more
> constant, and are mapped 1:1 with the mdevs, the associated mdev can
> be discerned by looking at the device configuration (e.g., mdevctl)
> and places, such as kernel messages, where it is statically stored.

I don't quite understand these two paragraphs, but the change looks OK

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

Jason

Re: [PATCH v1 02/18] vfio/ccw: Fix FSM state if mdev probe fails

[Jason Gunthorpe]

On Thu, Jun 02, 2022 at 07:19:32PM +0200, Eric Farman wrote:
> The FSM is in STANDBY state when arriving in vfio_ccw_mdev_probe(),
> and this routine converts it to IDLE as part of its processing.
> The error exit sets it to IDLE (again) but clears the private->mdev
> pointer.
> 
> The FSM should of course be managing the state itself, but the
> correct thing for vfio_ccw_mdev_probe() to do would be to put
> the state back the way it found it.
> 
> The corresponding check of private->mdev in vfio_ccw_sch_io_todo()
> can be removed, since the distinction is unnecessary at this point.
> 
> Fixes: 3bf1311f351ef ("vfio/ccw: Convert to use vfio_register_emulated_iommu_dev()")
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>  drivers/s390/cio/vfio_ccw_drv.c | 2 +-
>  drivers/s390/cio/vfio_ccw_ops.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)

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

Jason

Re: [PATCH v1 05/18] vfio/ccw: Remove private->mdev

[Jason Gunthorpe]

On Thu, Jun 02, 2022 at 07:19:35PM +0200, Eric Farman wrote:
> @@ -262,7 +260,7 @@ static void fsm_io_request(struct vfio_ccw_private *private,
>  			errstr = "transport mode";
>  			goto err_out;
>  		}
> -		io_region->ret_code = cp_init(&private->cp, mdev_dev(mdev),
> +		io_region->ret_code = cp_init(&private->cp, private->vdev.dev,
>  					      orb);

You'll need to rebase this series, I already did this hunk in v5.19:

commit 0a58795647cd4300470788ffdbff6b29b5f00632
Author: Jason Gunthorpe <jgg@ziepe.ca>
Date:   Wed May 11 13:12:59 2022 -0600

    vfio/ccw: Remove mdev from struct channel_program

Jason

Re: [PATCH v1 06/18] vfio/ccw: Pass enum to FSM event jumptable

[Jason Gunthorpe]

On Thu, Jun 02, 2022 at 07:19:36PM +0200, Eric Farman wrote:
> The FSM has an enumerated list of events defined.
> Use that as the argument passed to the jump table,
> instead of a regular int.
> 
> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> Fixes: bbe37e4cb8970 ("vfio: ccw: introduce a finite state machine")
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>  drivers/s390/cio/vfio_ccw_private.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

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

Jason

Re: [PATCH v1 07/18] vfio/ccw: Flatten MDEV device (un)register

[Jason Gunthorpe]

On Thu, Jun 02, 2022 at 07:19:37PM +0200, Eric Farman wrote:
> The vfio_ccw_mdev_(un)reg routines are merely vfio-ccw routines that
> pass control to mdev_(un)register_device. Since there's only one
> caller of each, let's just call the mdev routines directly.
> 
> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>  drivers/s390/cio/vfio_ccw_drv.c     |  4 ++--
>  drivers/s390/cio/vfio_ccw_ops.c     | 12 +-----------
>  drivers/s390/cio/vfio_ccw_private.h |  4 +---
>  3 files changed, 4 insertions(+), 16 deletions(-)

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

Jason

Re: [PATCH v1 08/18] vfio/ccw: Check that private pointer is not NULL

[Matthew Rosato]

On 6/2/22 1:19 PM, Eric Farman wrote:
> The subchannel->dev->drvdata pointer is set in vfio_ccw_sch_probe()
> and cleared in vfio_ccw_sch_remove(). In a purely defensive move,
> let's check that the resultant pointer exists before operating on it
> any way, since some lifecycle changes are forthcoming.
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>   drivers/s390/cio/vfio_ccw_drv.c | 12 ++++++++++++
>   drivers/s390/cio/vfio_ccw_ops.c |  3 +++
>   2 files changed, 15 insertions(+)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> index 3784eb4cda85..f8226db26e54 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> @@ -41,6 +41,9 @@ int vfio_ccw_sch_quiesce(struct subchannel *sch)
>   	DECLARE_COMPLETION_ONSTACK(completion);
>   	int iretry, ret = 0;
>   
> +	if (!private)
> +		return 0;
> +
>   	spin_lock_irq(sch->lock);
>   	if (!sch->schib.pmcw.ena)
>   		goto out_unlock;
> @@ -132,6 +135,9 @@ static void vfio_ccw_sch_irq(struct subchannel *sch)
>   {
>   	struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev);
>   
> +	if (WARN_ON(!private))
> +		return;
> +

I wonder, why a warning here vs quietly ignoring in all the other cases? 
(maybe add a small comment)

Also, kind of wondering what serialization is protecting the state of 
private (e.g. is it possible that you got a nonzero pointer from 
dev_get_drvdata but now it's been freed by a remove)


>   	inc_irq_stat(IRQIO_CIO);
>   	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_INTERRUPT);
>   }
> @@ -260,6 +266,9 @@ static void vfio_ccw_sch_remove(struct subchannel *sch)
>   {
>   	struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev);
>   
> +	if (!private)
> +		return;
> +
>   	vfio_ccw_sch_quiesce(sch);
>   	mdev_unregister_device(&sch->dev);
>   
> @@ -293,6 +302,9 @@ static int vfio_ccw_sch_event(struct subchannel *sch, int process)
>   	unsigned long flags;
>   	int rc = -EAGAIN;
>   
> +	if (!private)
> +		return 0;
> +
>   	spin_lock_irqsave(sch->lock, flags);
>   	if (!device_is_registered(&sch->dev))
>   		goto out_unlock;
> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index 497e1b7ffd61..9e5c184eab89 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
> @@ -152,6 +152,9 @@ static void vfio_ccw_mdev_remove(struct mdev_device *mdev)
>   {
>   	struct vfio_ccw_private *private = dev_get_drvdata(mdev->dev.parent);
>   
> +	if (!private)
> +		return;
> +
>   	VFIO_CCW_MSG_EVENT(2, "sch %x.%x.%04x: remove\n",
>   			   private->sch->schid.cssid,
>   			   private->sch->schid.ssid,

Re: [PATCH v1 08/18] vfio/ccw: Check that private pointer is not NULL

[Jason Gunthorpe]

On Thu, Jun 02, 2022 at 07:19:38PM +0200, Eric Farman wrote:
> The subchannel->dev->drvdata pointer is set in vfio_ccw_sch_probe()
> and cleared in vfio_ccw_sch_remove(). In a purely defensive move,
> let's check that the resultant pointer exists before operating on it
> any way, since some lifecycle changes are forthcoming.

There should be no possibility for the drvdata to be NULL in any of
these routines, if that happens there is some kernel race bug with
removal.

Thus all of these should be WARN_ON to document that they cannot happen.

Jason

Re: [PATCH v1 09/18] vfio/ccw: Create an OPEN FSM Event

[Jason Gunthorpe]

On Thu, Jun 02, 2022 at 07:19:39PM +0200, Eric Farman wrote:
> Move the process of enabling a subchannel for use by vfio-ccw
> into the FSM, such that it can manage the sequence of lifecycle
> events for the device.
> 
> That is, if the FSM state is NOT_OPER(erational), then do the work
> that would enable the subchannel and move the FSM to STANDBY state.
> An attempt to perform this event again from any of the other operating
> states (IDLE, CP_PROCESSING, CP_PENDING) will convert the device back
> to NOT_OPER so the configuration process can be started again.
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>  drivers/s390/cio/vfio_ccw_drv.c     |  9 ++-------
>  drivers/s390/cio/vfio_ccw_fsm.c     | 21 +++++++++++++++++++++
>  drivers/s390/cio/vfio_ccw_private.h |  1 +
>  3 files changed, 24 insertions(+), 7 deletions(-)

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

Jason

Re: [PATCH v1 10/18] vfio/ccw: Create a CLOSE FSM event

[Jason Gunthorpe]

On Thu, Jun 02, 2022 at 07:19:40PM +0200, Eric Farman wrote:

> @@ -278,7 +269,12 @@ static void vfio_ccw_sch_remove(struct subchannel *sch)
>  
>  static void vfio_ccw_sch_shutdown(struct subchannel *sch)
>  {
> -	vfio_ccw_sch_quiesce(sch);
> +	struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev);
> +
> +	if (!private)
> +		return;

WARN_ON here too

Looks Ok otherwise

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

Jason

Re: [PATCH v1 11/18] vfio/ccw: Refactor vfio_ccw_mdev_reset

[Jason Gunthorpe]

On Thu, Jun 02, 2022 at 07:19:41PM +0200, Eric Farman wrote:
> Use both the FSM Close and Open events when resetting an mdev,
> rather than making a separate call to cio_enable_subchannel().
> 
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>  drivers/s390/cio/vfio_ccw_ops.c | 24 ++++++++++--------------
>  1 file changed, 10 insertions(+), 14 deletions(-)

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

Jason

Re: [PATCH v1 05/18] vfio/ccw: Remove private->mdev

[Eric Farman]

On Thu, 2022-06-02 at 16:02 -0300, Jason Gunthorpe wrote:
> On Thu, Jun 02, 2022 at 07:19:35PM +0200, Eric Farman wrote:
> > @@ -262,7 +260,7 @@ static void fsm_io_request(struct
> > vfio_ccw_private *private,
> >  			errstr = "transport mode";
> >  			goto err_out;
> >  		}
> > -		io_region->ret_code = cp_init(&private->cp,
> > mdev_dev(mdev),
> > +		io_region->ret_code = cp_init(&private->cp, private-
> > >vdev.dev,
> >  					      orb);
> 
> You'll need to rebase this series, I already did this hunk in v5.19:
> 
> commit 0a58795647cd4300470788ffdbff6b29b5f00632
> Author: Jason Gunthorpe <jgg@ziepe.ca>
> Date:   Wed May 11 13:12:59 2022 -0600
> 
>     vfio/ccw: Remove mdev from struct channel_program

Ah, yes. I hadn't gotten this sent out before the vfio pull request
landed yesterday, so a rebase is of course warranted. Thanks!

Eric

> 
> Jason

Re: [PATCH v1 12/18] vfio/ccw: Move FSM open/close to MDEV open/close

[Jason Gunthorpe]

On Thu, Jun 02, 2022 at 07:19:42PM +0200, Eric Farman wrote:
> Part of the confusion that has existed is the FSM lifecycle of
> subchannels between the common CSS driver and the vfio-ccw driver.
> During configuration, the FSM state goes from NOT_OPER to STANDBY
> to IDLE, but then back to NOT_OPER. For example:
> 
> 	vfio_ccw_sch_probe:		VFIO_CCW_STATE_NOT_OPER
> 	vfio_ccw_sch_probe:		VFIO_CCW_STATE_STANDBY
> 	vfio_ccw_mdev_probe:		VFIO_CCW_STATE_IDLE
> 	vfio_ccw_mdev_remove:		VFIO_CCW_STATE_NOT_OPER
> 	vfio_ccw_sch_remove:		VFIO_CCW_STATE_NOT_OPER
> 	vfio_ccw_sch_shutdown:		VFIO_CCW_STATE_NOT_OPER
> 
> Rearrange the open/close events to align with the mdev open/close,
> to better manage the memory and state of the devices as time
> progresses. Specifically, make mdev_open() perform the FSM open,
> and mdev_close() perform the FSM close instead of reset (which is
> both close and open).
> 
> This makes the NOT_OPER state a dead-end path, indicating the
> device is probably not recoverable without fully probing and
> re-configuring the device.
> 
> This has the nice side-effect of removing a number of special-cases
> where the FSM state is managed outside of the FSM itself (such as
> the aforementioned mdev_close() routine).
> 
> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>  drivers/s390/cio/vfio_ccw_drv.c | 11 +++--------
>  drivers/s390/cio/vfio_ccw_fsm.c | 30 ++++++++++++++++++++++--------
>  drivers/s390/cio/vfio_ccw_ops.c | 26 +++++++++++---------------
>  3 files changed, 36 insertions(+), 31 deletions(-)

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

Jason

Re: [PATCH v1 07/18] vfio/ccw: Flatten MDEV device (un)register

[Matthew Rosato]

On 6/2/22 1:19 PM, Eric Farman wrote:
> The vfio_ccw_mdev_(un)reg routines are merely vfio-ccw routines that
> pass control to mdev_(un)register_device. Since there's only one
> caller of each, let's just call the mdev routines directly.

I'd reword slightly to reference the ops extern

".. caller of each, externalize vfio_ccw_mdev_ops and call..."

regardless,

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

> 
> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>   drivers/s390/cio/vfio_ccw_drv.c     |  4 ++--
>   drivers/s390/cio/vfio_ccw_ops.c     | 12 +-----------
>   drivers/s390/cio/vfio_ccw_private.h |  4 +---
>   3 files changed, 4 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> index 9d817aa2f1c4..3784eb4cda85 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> @@ -239,7 +239,7 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
>   
>   	private->state = VFIO_CCW_STATE_STANDBY;
>   
> -	ret = vfio_ccw_mdev_reg(sch);
> +	ret = mdev_register_device(&sch->dev, &vfio_ccw_mdev_ops);
>   	if (ret)
>   		goto out_disable;
>   
> @@ -261,7 +261,7 @@ static void vfio_ccw_sch_remove(struct subchannel *sch)
>   	struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev);
>   
>   	vfio_ccw_sch_quiesce(sch);
> -	vfio_ccw_mdev_unreg(sch);
> +	mdev_unregister_device(&sch->dev);
>   
>   	dev_set_drvdata(&sch->dev, NULL);
>   
> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index 4a64c176facb..497e1b7ffd61 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
> @@ -656,18 +656,8 @@ struct mdev_driver vfio_ccw_mdev_driver = {
>   	.remove = vfio_ccw_mdev_remove,
>   };
>   
> -static const struct mdev_parent_ops vfio_ccw_mdev_ops = {
> +const struct mdev_parent_ops vfio_ccw_mdev_ops = {
>   	.owner			= THIS_MODULE,
>   	.device_driver		= &vfio_ccw_mdev_driver,
>   	.supported_type_groups  = mdev_type_groups,
>   };
> -
> -int vfio_ccw_mdev_reg(struct subchannel *sch)
> -{
> -	return mdev_register_device(&sch->dev, &vfio_ccw_mdev_ops);
> -}
> -
> -void vfio_ccw_mdev_unreg(struct subchannel *sch)
> -{
> -	mdev_unregister_device(&sch->dev);
> -}
> diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
> index 5c128eec596b..2e0744ac6492 100644
> --- a/drivers/s390/cio/vfio_ccw_private.h
> +++ b/drivers/s390/cio/vfio_ccw_private.h
> @@ -117,12 +117,10 @@ struct vfio_ccw_private {
>   	struct work_struct	crw_work;
>   } __aligned(8);
>   
> -extern int vfio_ccw_mdev_reg(struct subchannel *sch);
> -extern void vfio_ccw_mdev_unreg(struct subchannel *sch);
> -
>   extern int vfio_ccw_sch_quiesce(struct subchannel *sch);
>   
>   extern struct mdev_driver vfio_ccw_mdev_driver;
> +extern const struct mdev_parent_ops vfio_ccw_mdev_ops;
>   
>   /*
>    * States of the device statemachine.

Re: [PATCH v1 16/18] vfio/ccw: Create a get_private routine

[Jason Gunthorpe]

On Thu, Jun 02, 2022 at 07:19:46PM +0200, Eric Farman wrote:

> @@ -125,6 +127,18 @@ extern struct mdev_driver vfio_ccw_mdev_driver;
>  extern const struct mdev_parent_ops vfio_ccw_mdev_ops;
>  extern const struct vfio_device_ops vfio_ccw_dev_ops;
>  
> +static inline struct vfio_ccw_private *vfio_ccw_get_private(struct subchannel *sch)
> +{
> +	struct vfio_ccw_private *private;
> +
> +	if (!sch)
> +		return NULL;

WARN_ON?

Jason

Re: [PATCH v1 18/18] vfio/ccw: Manage ccw/mdev reference counts

[Jason Gunthorpe]

On Thu, Jun 02, 2022 at 07:19:48PM +0200, Eric Farman wrote:
> diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
> index 3833204bd388..be238cf277ff 100644
> +++ b/drivers/s390/cio/vfio_ccw_private.h
> @@ -135,6 +135,8 @@ static inline struct vfio_ccw_private *vfio_ccw_get_private(struct subchannel *s
>  		return NULL;
>  
>  	private = dev_get_drvdata(&sch->dev);
> +	if (private && !vfio_device_try_get(&private->vdev))
> +		private = NULL;

I didn't try to check everything, but obviously any of the null checks
in prior patches that are eventually converted to this call should not
be made into WARN_ON's..

This looks OK too

Jason

Re: [PATCH v1 06/18] vfio/ccw: Pass enum to FSM event jumptable

[Matthew Rosato]

On 6/2/22 1:19 PM, Eric Farman wrote:
> The FSM has an enumerated list of events defined.
> Use that as the argument passed to the jump table,
> instead of a regular int.
> 
> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> Fixes: bbe37e4cb8970 ("vfio: ccw: introduce a finite state machine")

Not sure if this change really merits a fixes tag

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

> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>   drivers/s390/cio/vfio_ccw_private.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
> index 12b5537d478f..5c128eec596b 100644
> --- a/drivers/s390/cio/vfio_ccw_private.h
> +++ b/drivers/s390/cio/vfio_ccw_private.h
> @@ -156,7 +156,7 @@ typedef void (fsm_func_t)(struct vfio_ccw_private *, enum vfio_ccw_event);
>   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,
> -				     int event)
> +				      enum vfio_ccw_event event)
>   {
>   	trace_vfio_ccw_fsm_event(private->sch->schid, private->state, event);
>   	vfio_ccw_jumptable[private->state][event](private, event);

Re: [PATCH v1 00/18] VFIO ccw/mdev rework

[Jason Gunthorpe]

On Thu, Jun 02, 2022 at 07:19:30PM +0200, Eric Farman wrote:
> Last autumn, Jason Gunthorpe proposed some rework of vfio-ccw [1],
> to better fit with the new mdev API (thank you!). Part of that
> series was pulled for kernel 5.16 [2], but the complexities of
> the remaining patches got them hung up behind other work.
> 
> This series attempts to dust off and complete that, with the
> goal of untangling the lifecycle of a s390 subchannel when
> bound to vfio-ccw instead of the usual io_subchannel driver.
> 
> Patches 1-8 are inspired by and/or split out from that series,
> in order to be consumable on their own (backports, etc.).
> 
> Patches 9-12 handle the goal of making the FSM complete,
> and synchronizing the subchannel's life with that of the mdev.
> (This was the goal of patch 5 of the larger series [3].)
> 
> Patches 13-14 are pulled directly from the earlier series.
> As these patches hit some other of the consumers of vfio,
> those on CC who are unfamiliar with vfio-ccw probably only
> care about these. :)
> 
> Patches 15-18 links the lifecycle of the vfio_ccw_private struct
> with the mdev via a vfio reference. (Patch 17 was also pulled
> directly from the earlier series.)
> 
> In the end, the subchannel probe/remove callbacks from the css
> driver simply register/unregister with vfio-mdev. The communication
> with the subchannel is delayed until the mdev routines, which
> handles all the vfio-related memory and subchannel enablement.
> There's no longer a configuration where the mdev is closed while
> the subchannel remains enabled, since that's weird.

This all looks great thanks!

I would like this to go through the VFIO tree once you repost it on v5.19-rc1

> @Jason: I carried the S-o-b/r-b tags on patches 13, 14, and 17,
> as they were cherry-picked straight from your v3.
> If you'd prefer your S-o-b on others, please let me know.

It is OK, you did a lot of work splitting things up

Thanks,
Jason

Re: [PATCH v1 01/18] vfio/ccw: Remove UUID from s390 debug log

[Matthew Rosato]

On 6/2/22 1:19 PM, Eric Farman wrote:
> From: Michael Kawano <mkawano@linux.ibm.com>
> 
> As vfio-ccw devices are created/destroyed, the uuid of the associated
> mdevs that are recorded in $S390DBF/vfio_ccw_msg/sprintf get lost as
> they are created using pointers passed by reference.
> 
> This is a deliberate design point of s390dbf, but it leaves the uuid

This wording is confusing, maybe some re-wording would help here.

Basically, s390dbf doesn't support values passed by reference today 
(e.g. %pUl), it will just store that pointer (e.g. &mdev->uuid) and not 
its contents -- so a subsequent viewing of the s390dbf log at any point 
in the future will go peek at that referenced memory -- which might have 
been freed (e.g. mdev was removed).  So this change will fix potential 
garbage data viewed from the log or worse an oops when viewing the log 
-- the latter of which should probably be mentioned in the commit message.

I'm not sure if it was a deliberate design decision of s390dbf or just a 
feature that was never implemented, so I'd omit that altogether -- but 
it IS pointed out in the s390dbf documentation as a limitation anyway.

The code itself is fine:

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

> in these traces less than useful. Since the subchannels are more
> constant, and are mapped 1:1 with the mdevs, the associated mdev can
> be discerned by looking at the device configuration (e.g., mdevctl)
> and places, such as kernel messages, where it is statically stored.
> 
> Thus, let's just remove the uuid from s390dbf traces. As we were
> the only consumer of mdev_uuid(), remove that too.
> 
> Cc: Kirti Wankhede <kwankhede@nvidia.com>
> Signed-off-by: Michael Kawano <mkawano@linux.ibm.com>
> Fixes: 60e05d1cf0875 ("vfio-ccw: add some logging")
> Fixes: b7701dfbf9832 ("vfio-ccw: Register a chp_event callback for vfio-ccw")
> [farman: reworded commit message, added Fixes: tags]
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>   drivers/s390/cio/vfio_ccw_drv.c |  5 ++---
>   drivers/s390/cio/vfio_ccw_fsm.c | 24 ++++++++++++------------
>   drivers/s390/cio/vfio_ccw_ops.c |  8 ++++----
>   include/linux/mdev.h            |  4 ----
>   4 files changed, 18 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> index ee182cfb467d..35055eb94115 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> @@ -14,7 +14,6 @@
>   #include <linux/init.h>
>   #include <linux/device.h>
>   #include <linux/slab.h>
> -#include <linux/uuid.h>
>   #include <linux/mdev.h>
>   
>   #include <asm/isc.h>
> @@ -358,8 +357,8 @@ static int vfio_ccw_chp_event(struct subchannel *sch,
>   		return 0;
>   
>   	trace_vfio_ccw_chp_event(private->sch->schid, mask, event);
> -	VFIO_CCW_MSG_EVENT(2, "%pUl (%x.%x.%04x): mask=0x%x event=%d\n",
> -			   mdev_uuid(private->mdev), sch->schid.cssid,
> +	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,
>   			   mask, event);
>   
> diff --git a/drivers/s390/cio/vfio_ccw_fsm.c b/drivers/s390/cio/vfio_ccw_fsm.c
> index e435a9cd92da..86b23732d899 100644
> --- a/drivers/s390/cio/vfio_ccw_fsm.c
> +++ b/drivers/s390/cio/vfio_ccw_fsm.c
> @@ -256,8 +256,8 @@ static void fsm_io_request(struct vfio_ccw_private *private,
>   		if (orb->tm.b) {
>   			io_region->ret_code = -EOPNOTSUPP;
>   			VFIO_CCW_MSG_EVENT(2,
> -					   "%pUl (%x.%x.%04x): transport mode\n",
> -					   mdev_uuid(mdev), schid.cssid,
> +					   "sch %x.%x.%04x: transport mode\n",
> +					   schid.cssid,
>   					   schid.ssid, schid.sch_no);
>   			errstr = "transport mode";
>   			goto err_out;
> @@ -266,8 +266,8 @@ static void fsm_io_request(struct vfio_ccw_private *private,
>   					      orb);
>   		if (io_region->ret_code) {
>   			VFIO_CCW_MSG_EVENT(2,
> -					   "%pUl (%x.%x.%04x): cp_init=%d\n",
> -					   mdev_uuid(mdev), schid.cssid,
> +					   "sch %x.%x.%04x: cp_init=%d\n",
> +					   schid.cssid,
>   					   schid.ssid, schid.sch_no,
>   					   io_region->ret_code);
>   			errstr = "cp init";
> @@ -277,8 +277,8 @@ static void fsm_io_request(struct vfio_ccw_private *private,
>   		io_region->ret_code = cp_prefetch(&private->cp);
>   		if (io_region->ret_code) {
>   			VFIO_CCW_MSG_EVENT(2,
> -					   "%pUl (%x.%x.%04x): cp_prefetch=%d\n",
> -					   mdev_uuid(mdev), schid.cssid,
> +					   "sch %x.%x.%04x: cp_prefetch=%d\n",
> +					   schid.cssid,
>   					   schid.ssid, schid.sch_no,
>   					   io_region->ret_code);
>   			errstr = "cp prefetch";
> @@ -290,8 +290,8 @@ static void fsm_io_request(struct vfio_ccw_private *private,
>   		io_region->ret_code = fsm_io_helper(private);
>   		if (io_region->ret_code) {
>   			VFIO_CCW_MSG_EVENT(2,
> -					   "%pUl (%x.%x.%04x): fsm_io_helper=%d\n",
> -					   mdev_uuid(mdev), schid.cssid,
> +					   "sch %x.%x.%04x: fsm_io_helper=%d\n",
> +					   schid.cssid,
>   					   schid.ssid, schid.sch_no,
>   					   io_region->ret_code);
>   			errstr = "cp fsm_io_helper";
> @@ -301,16 +301,16 @@ static void fsm_io_request(struct vfio_ccw_private *private,
>   		return;
>   	} else if (scsw->cmd.fctl & SCSW_FCTL_HALT_FUNC) {
>   		VFIO_CCW_MSG_EVENT(2,
> -				   "%pUl (%x.%x.%04x): halt on io_region\n",
> -				   mdev_uuid(mdev), schid.cssid,
> +				   "sch %x.%x.%04x: halt on io_region\n",
> +				   schid.cssid,
>   				   schid.ssid, schid.sch_no);
>   		/* halt is handled via the async cmd region */
>   		io_region->ret_code = -EOPNOTSUPP;
>   		goto err_out;
>   	} else if (scsw->cmd.fctl & SCSW_FCTL_CLEAR_FUNC) {
>   		VFIO_CCW_MSG_EVENT(2,
> -				   "%pUl (%x.%x.%04x): clear on io_region\n",
> -				   mdev_uuid(mdev), schid.cssid,
> +				   "sch %x.%x.%04x: clear on io_region\n",
> +				   schid.cssid,
>   				   schid.ssid, schid.sch_no);
>   		/* clear is handled via the async cmd region */
>   		io_region->ret_code = -EOPNOTSUPP;
> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index d8589afac272..bebae21228aa 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
> @@ -131,8 +131,8 @@ static int vfio_ccw_mdev_probe(struct mdev_device *mdev)
>   	private->mdev = mdev;
>   	private->state = VFIO_CCW_STATE_IDLE;
>   
> -	VFIO_CCW_MSG_EVENT(2, "mdev %pUl, sch %x.%x.%04x: create\n",
> -			   mdev_uuid(mdev), private->sch->schid.cssid,
> +	VFIO_CCW_MSG_EVENT(2, "sch %x.%x.%04x: create\n",
> +			   private->sch->schid.cssid,
>   			   private->sch->schid.ssid,
>   			   private->sch->schid.sch_no);
>   
> @@ -154,8 +154,8 @@ static void vfio_ccw_mdev_remove(struct mdev_device *mdev)
>   {
>   	struct vfio_ccw_private *private = dev_get_drvdata(mdev->dev.parent);
>   
> -	VFIO_CCW_MSG_EVENT(2, "mdev %pUl, sch %x.%x.%04x: remove\n",
> -			   mdev_uuid(mdev), private->sch->schid.cssid,
> +	VFIO_CCW_MSG_EVENT(2, "sch %x.%x.%04x: remove\n",
> +			   private->sch->schid.cssid,
>   			   private->sch->schid.ssid,
>   			   private->sch->schid.sch_no);
>   
> diff --git a/include/linux/mdev.h b/include/linux/mdev.h
> index 15d03f6532d0..a5788f592817 100644
> --- a/include/linux/mdev.h
> +++ b/include/linux/mdev.h
> @@ -139,10 +139,6 @@ static inline void mdev_set_drvdata(struct mdev_device *mdev, void *data)
>   {
>   	mdev->driver_data = data;
>   }
> -static inline const guid_t *mdev_uuid(struct mdev_device *mdev)
> -{
> -	return &mdev->uuid;
> -}
>   
>   extern struct bus_type mdev_bus_type;
>   

Re: [PATCH v1 13/18] vfio/mdev: Consolidate all the device_api sysfs into the core code

[Christoph Hellwig]

On Thu, Jun 02, 2022 at 07:19:43PM +0200, Eric Farman wrote:
> From: Jason Gunthorpe <jgg@nvidia.com>
> 
> Every driver just emits a static string, simply feed it through the ops
> and provide a standard sysfs show function.

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v1 17/18] vfio: Export vfio_device_try_get()

[Cornelia Huck]

On Thu, Jun 02 2022, Eric Farman <farman@linux.ibm.com> wrote:

> From: Jason Gunthorpe <jgg@nvidia.com>
>
> vfio_ccw will need it.
>
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Cc: Cornelia Huck <cohuck@redhat.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> Link: https://lore.kernel.org/r/9-v3-57c1502c62fd+2190-ccw_mdev_jgg@nvidia.com/
> [farman: added Cc: tags]
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>  drivers/vfio/vfio.c  | 3 ++-
>  include/linux/vfio.h | 1 +
>  2 files changed, 3 insertions(+), 1 deletion(-)

Acked-by: Cornelia Huck <cohuck@redhat.com>

Re: [PATCH v1 02/18] vfio/ccw: Fix FSM state if mdev probe fails

[Matthew Rosato]

On 6/2/22 1:19 PM, Eric Farman wrote:
> The FSM is in STANDBY state when arriving in vfio_ccw_mdev_probe(),
> and this routine converts it to IDLE as part of its processing.
> The error exit sets it to IDLE (again) but clears the private->mdev
> pointer.
> 
> The FSM should of course be managing the state itself, but the
> correct thing for vfio_ccw_mdev_probe() to do would be to put
> the state back the way it found it.
> 
> The corresponding check of private->mdev in vfio_ccw_sch_io_todo()
> can be removed, since the distinction is unnecessary at this point.
> 
> Fixes: 3bf1311f351ef ("vfio/ccw: Convert to use vfio_register_emulated_iommu_dev()")
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>   drivers/s390/cio/vfio_ccw_drv.c | 2 +-
>   drivers/s390/cio/vfio_ccw_ops.c | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> index 35055eb94115..b18b4582bc8b 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> @@ -108,7 +108,7 @@ static void vfio_ccw_sch_io_todo(struct work_struct *work)
>   	 * has finished. Do not overwrite a possible processing
>   	 * state if the final interrupt was for HSCH or CSCH.
>   	 */
> -	if (private->mdev && cp_is_finished)
> +	if (cp_is_finished)
>   		private->state = VFIO_CCW_STATE_IDLE;

Took me a bit to convince myself this was OK, mainly because AFAICT 
despite the change below the fsm jumptable would still allow you to 
reach this code when in STANDBY.  But, it should only be possible for an 
unsolicited interrupt (e.g. unsolicited implies !cp_is_finished) so we 
would still avoid a STANDBY->IDLE transition on accident.

Maybe work unsolicited interrupt into the comment block above along with 
HSCH/CSCH?

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

>   
>   	if (private->io_trigger)
> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index bebae21228aa..a403d059a4e6 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
> @@ -146,7 +146,7 @@ static int vfio_ccw_mdev_probe(struct mdev_device *mdev)
>   	vfio_uninit_group_dev(&private->vdev);
>   	atomic_inc(&private->avail);
>   	private->mdev = NULL;
> -	private->state = VFIO_CCW_STATE_IDLE;
> +	private->state = VFIO_CCW_STATE_STANDBY;

Re: [PATCH v1 03/18] vfio/ccw: Ensure mdev->dev is cleared on mdev remove

[Matthew Rosato]

On 6/2/22 1:19 PM, Eric Farman wrote:
> The mdev is linked with the vfio_ccw_private pointer when the mdev
> is probed, but it's not cleared once the mdev is removed.
> 
> This isn't much of a concern based on the current device lifecycle,
> but fix it so that things make sense in later shuffling.
> 
> Fixes: 3bf1311f351ef ("vfio/ccw: Convert to use vfio_register_emulated_iommu_dev()")
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>   drivers/s390/cio/vfio_ccw_ops.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index a403d059a4e6..a0a3200b0b04 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
> @@ -159,6 +159,7 @@ static void vfio_ccw_mdev_remove(struct mdev_device *mdev)
>   			   private->sch->schid.ssid,
>   			   private->sch->schid.sch_no);
>   
> +	dev_set_drvdata(&mdev->dev, NULL);
>   	vfio_unregister_group_dev(&private->vdev);
>   
>   	if ((private->state != VFIO_CCW_STATE_NOT_OPER) &&

Seems harmless enough.

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

But is this just precautionary or is it fixing a real problem (if the 
former I don't think a fixes tag makes sense)

I also ask because I note vfio-ap clears its driver_data in mdev_remove 
but also leaves the pointer set, meaning they might need a similar 
cleanup and should probably have a look (CC Tony & Jason H)

Re: [PATCH v1 03/18] vfio/ccw: Ensure mdev->dev is cleared on mdev remove

[Jason Gunthorpe]

On Fri, Jun 03, 2022 at 09:25:19AM -0400, Matthew Rosato wrote:
> On 6/2/22 1:19 PM, Eric Farman wrote:
> > The mdev is linked with the vfio_ccw_private pointer when the mdev
> > is probed, but it's not cleared once the mdev is removed.
> > 
> > This isn't much of a concern based on the current device lifecycle,
> > but fix it so that things make sense in later shuffling.
> > 
> > Fixes: 3bf1311f351ef ("vfio/ccw: Convert to use vfio_register_emulated_iommu_dev()")
> > Signed-off-by: Eric Farman <farman@linux.ibm.com>
> >   drivers/s390/cio/vfio_ccw_ops.c | 1 +
> >   1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> > index a403d059a4e6..a0a3200b0b04 100644
> > +++ b/drivers/s390/cio/vfio_ccw_ops.c
> > @@ -159,6 +159,7 @@ static void vfio_ccw_mdev_remove(struct mdev_device *mdev)
> >   			   private->sch->schid.ssid,
> >   			   private->sch->schid.sch_no);
> > +	dev_set_drvdata(&mdev->dev, NULL);
> >   	vfio_unregister_group_dev(&private->vdev);
> >   	if ((private->state != VFIO_CCW_STATE_NOT_OPER) &&
> 
> Seems harmless enough.
> 
> Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
> 
> But is this just precautionary or is it fixing a real problem (if the former
> I don't think a fixes tag makes sense)
> 
> I also ask because I note vfio-ap clears its driver_data in mdev_remove but
> also leaves the pointer set, meaning they might need a similar cleanup and
> should probably have a look (CC Tony & Jason H)

There should be no reason to clear the drvdata on remove - the driver
must be designed to guarentee all references to the dev stop before
the remove function returns.

Jason

Re: [PATCH v1 13/18] vfio/mdev: Consolidate all the device_api sysfs into the core code

[Tony Krowiak]

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

On 6/2/22 1:19 PM, Eric Farman wrote:
> From: Jason Gunthorpe <jgg@nvidia.com>
>
> Every driver just emits a static string, simply feed it through the ops
> and provide a standard sysfs show function.
>
> Cc: Kirti Wankhede <kwankhede@nvidia.com>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
> Cc: Zhi Wang <zhi.a.wang@intel.com>
> Cc: Tony Krowiak <akrowiak@linux.ibm.com>
> Cc: Jason Herne <jjherne@linux.ibm.com>
> Cc: intel-gvt-dev@lists.freedesktop.org
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> Link: https://lore.kernel.org/r/6-v3-57c1502c62fd+2190-ccw_mdev_jgg@nvidia.com/
> [farman: added Cc: tags]
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>   .../driver-api/vfio-mediated-device.rst       |  4 ++-
>   drivers/gpu/drm/i915/gvt/kvmgt.c              |  9 +------
>   drivers/s390/cio/vfio_ccw_ops.c               |  9 +------
>   drivers/s390/crypto/vfio_ap_ops.c             |  9 +------
>   drivers/vfio/mdev/mdev_core.c                 |  2 +-
>   drivers/vfio/mdev/mdev_sysfs.c                | 27 ++++++++++++++++---
>   include/linux/mdev.h                          |  7 ++---
>   samples/vfio-mdev/mbochs.c                    |  9 +------
>   samples/vfio-mdev/mdpy.c                      |  9 +------
>   samples/vfio-mdev/mtty.c                      | 10 +------
>   10 files changed, 36 insertions(+), 59 deletions(-)
>
> diff --git a/Documentation/driver-api/vfio-mediated-device.rst b/Documentation/driver-api/vfio-mediated-device.rst
> index 9f26079cacae..f410a1cd98bb 100644
> --- a/Documentation/driver-api/vfio-mediated-device.rst
> +++ b/Documentation/driver-api/vfio-mediated-device.rst
> @@ -137,6 +137,7 @@ The structures in the mdev_parent_ops structure are as follows:
>   * mdev_attr_groups: attributes of the mediated device
>   * supported_config: attributes to define supported configurations
>   * device_driver: device driver to bind for mediated device instances
> +* device_api: String to pass through the sysfs file below
>   
>   The mdev_parent_ops also still has various functions pointers.  Theses exist
>   for historical reasons only and shall not be used for new drivers.
> @@ -225,7 +226,8 @@ Directories and files under the sysfs for Each Physical Device
>   * device_api
>   
>     This attribute should show which device API is being created, for example,
> -  "vfio-pci" for a PCI device.
> +  "vfio-pci" for a PCI device. The core code maintins this sysfs using the
> +  device_api member of mdev_parent_ops.
>   
>   * available_instances
>   
> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
> index 057ec4490104..752d7a1211e6 100644
> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> @@ -163,12 +163,6 @@ static ssize_t available_instances_show(struct mdev_type *mtype,
>   	return sprintf(buf, "%u\n", num);
>   }
>   
> -static ssize_t device_api_show(struct mdev_type *mtype,
> -			       struct mdev_type_attribute *attr, char *buf)
> -{
> -	return sprintf(buf, "%s\n", VFIO_DEVICE_API_PCI_STRING);
> -}
> -
>   static ssize_t description_show(struct mdev_type *mtype,
>   				struct mdev_type_attribute *attr, char *buf)
>   {
> @@ -202,13 +196,11 @@ static ssize_t name_show(struct mdev_type *mtype,
>   }
>   
>   static MDEV_TYPE_ATTR_RO(available_instances);
> -static MDEV_TYPE_ATTR_RO(device_api);
>   static MDEV_TYPE_ATTR_RO(description);
>   static MDEV_TYPE_ATTR_RO(name);
>   
>   static struct attribute *gvt_type_attrs[] = {
>   	&mdev_type_attr_available_instances.attr,
> -	&mdev_type_attr_device_api.attr,
>   	&mdev_type_attr_description.attr,
>   	&mdev_type_attr_name.attr,
>   	NULL,
> @@ -1767,6 +1759,7 @@ static const struct attribute_group *intel_vgpu_groups[] = {
>   
>   static struct mdev_parent_ops intel_vgpu_ops = {
>   	.mdev_attr_groups       = intel_vgpu_groups,
> +	.device_api		= VFIO_DEVICE_API_PCI_STRING,
>   	.create			= intel_vgpu_create,
>   	.remove			= intel_vgpu_remove,
>   
> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index 2afb8f13739f..6793c8b3c58b 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
> @@ -66,13 +66,6 @@ static ssize_t name_show(struct mdev_type *mtype,
>   }
>   static MDEV_TYPE_ATTR_RO(name);
>   
> -static ssize_t device_api_show(struct mdev_type *mtype,
> -			       struct mdev_type_attribute *attr, char *buf)
> -{
> -	return sprintf(buf, "%s\n", VFIO_DEVICE_API_CCW_STRING);
> -}
> -static MDEV_TYPE_ATTR_RO(device_api);
> -
>   static ssize_t available_instances_show(struct mdev_type *mtype,
>   					struct mdev_type_attribute *attr,
>   					char *buf)
> @@ -86,7 +79,6 @@ static MDEV_TYPE_ATTR_RO(available_instances);
>   
>   static struct attribute *mdev_types_attrs[] = {
>   	&mdev_type_attr_name.attr,
> -	&mdev_type_attr_device_api.attr,
>   	&mdev_type_attr_available_instances.attr,
>   	NULL,
>   };
> @@ -644,5 +636,6 @@ struct mdev_driver vfio_ccw_mdev_driver = {
>   const struct mdev_parent_ops vfio_ccw_mdev_ops = {
>   	.owner			= THIS_MODULE,
>   	.device_driver		= &vfio_ccw_mdev_driver,
> +	.device_api		= VFIO_DEVICE_API_CCW_STRING,
>   	.supported_type_groups  = mdev_type_groups,
>   };
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 6e08d04b605d..838b1a3eac8a 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -530,17 +530,9 @@ static ssize_t available_instances_show(struct mdev_type *mtype,
>   
>   static MDEV_TYPE_ATTR_RO(available_instances);
>   
> -static ssize_t device_api_show(struct mdev_type *mtype,
> -			       struct mdev_type_attribute *attr, char *buf)
> -{
> -	return sprintf(buf, "%s\n", VFIO_DEVICE_API_AP_STRING);
> -}
> -
> -static MDEV_TYPE_ATTR_RO(device_api);
>   
>   static struct attribute *vfio_ap_mdev_type_attrs[] = {
>   	&mdev_type_attr_name.attr,
> -	&mdev_type_attr_device_api.attr,
>   	&mdev_type_attr_available_instances.attr,
>   	NULL,
>   };
> @@ -1501,6 +1493,7 @@ static struct mdev_driver vfio_ap_matrix_driver = {
>   static const struct mdev_parent_ops vfio_ap_matrix_ops = {
>   	.owner			= THIS_MODULE,
>   	.device_driver		= &vfio_ap_matrix_driver,
> +	.device_api		= VFIO_DEVICE_API_AP_STRING,
>   	.supported_type_groups	= vfio_ap_mdev_type_groups,
>   };
>   
> diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
> index b314101237fe..c3018e8e6d32 100644
> --- a/drivers/vfio/mdev/mdev_core.c
> +++ b/drivers/vfio/mdev/mdev_core.c
> @@ -129,7 +129,7 @@ int mdev_register_device(struct device *dev, const struct mdev_parent_ops *ops)
>   	char *envp[] = { env_string, NULL };
>   
>   	/* check for mandatory ops */
> -	if (!ops || !ops->supported_type_groups)
> +	if (!ops || !ops->supported_type_groups || !ops->device_api)
>   		return -EINVAL;
>   	if (!ops->device_driver && (!ops->create || !ops->remove))
>   		return -EINVAL;
> diff --git a/drivers/vfio/mdev/mdev_sysfs.c b/drivers/vfio/mdev/mdev_sysfs.c
> index f5cf1931c54e..d4b99440d19e 100644
> --- a/drivers/vfio/mdev/mdev_sysfs.c
> +++ b/drivers/vfio/mdev/mdev_sysfs.c
> @@ -74,9 +74,30 @@ static ssize_t create_store(struct mdev_type *mtype,
>   
>   	return count;
>   }
> -
>   static MDEV_TYPE_ATTR_WO(create);
>   
> +static ssize_t device_api_show(struct mdev_type *mtype,
> +			       struct mdev_type_attribute *attr, char *buf)
> +{
> +	return sysfs_emit(buf, "%s\n", mtype->parent->ops->device_api);
> +}
> +static MDEV_TYPE_ATTR_RO(device_api);
> +
> +static struct attribute *mdev_types_std_attrs[] = {
> +	&mdev_type_attr_create.attr,
> +	&mdev_type_attr_device_api.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group mdev_type_std_group = {
> +	.attrs = mdev_types_std_attrs,
> +};
> +
> +static const struct attribute_group *mdev_type_groups[] = {
> +	&mdev_type_std_group,
> +	NULL,
> +};
> +
>   static void mdev_type_release(struct kobject *kobj)
>   {
>   	struct mdev_type *type = to_mdev_type(kobj);
> @@ -123,7 +144,7 @@ static struct mdev_type *add_mdev_supported_type(struct mdev_parent *parent,
>   		return ERR_PTR(ret);
>   	}
>   
> -	ret = sysfs_create_file(&type->kobj, &mdev_type_attr_create.attr);
> +	ret = sysfs_create_groups(&type->kobj, mdev_type_groups);
>   	if (ret)
>   		goto attr_create_failed;
>   
> @@ -144,7 +165,7 @@ static struct mdev_type *add_mdev_supported_type(struct mdev_parent *parent,
>   attrs_failed:
>   	kobject_put(type->devices_kobj);
>   attr_devices_failed:
> -	sysfs_remove_file(&type->kobj, &mdev_type_attr_create.attr);
> +	sysfs_remove_groups(&type->kobj, mdev_type_groups);
>   attr_create_failed:
>   	kobject_del(&type->kobj);
>   	kobject_put(&type->kobj);
> diff --git a/include/linux/mdev.h b/include/linux/mdev.h
> index a5788f592817..14655215417b 100644
> --- a/include/linux/mdev.h
> +++ b/include/linux/mdev.h
> @@ -36,6 +36,7 @@ struct device *mtype_get_parent_dev(struct mdev_type *mtype);
>    *
>    * @owner:		The module owner.
>    * @device_driver:	Which device driver to probe() on newly created devices
> + * @device_api:		String to return for the device_api sysfs
>    * @dev_attr_groups:	Attributes of the parent device.
>    * @mdev_attr_groups:	Attributes of the mediated device.
>    * @supported_type_groups: Attributes to define supported types. It is mandatory
> @@ -80,6 +81,7 @@ struct device *mtype_get_parent_dev(struct mdev_type *mtype);
>   struct mdev_parent_ops {
>   	struct module   *owner;
>   	struct mdev_driver *device_driver;
> +	const char *device_api;
>   	const struct attribute_group **dev_attr_groups;
>   	const struct attribute_group **mdev_attr_groups;
>   	struct attribute_group **supported_type_groups;
> @@ -108,11 +110,6 @@ struct mdev_type_attribute {
>   			 size_t count);
>   };
>   
> -#define MDEV_TYPE_ATTR(_name, _mode, _show, _store)		\
> -struct mdev_type_attribute mdev_type_attr_##_name =		\
> -	__ATTR(_name, _mode, _show, _store)
> -#define MDEV_TYPE_ATTR_RW(_name) \
> -	struct mdev_type_attribute mdev_type_attr_##_name = __ATTR_RW(_name)
>   #define MDEV_TYPE_ATTR_RO(_name) \
>   	struct mdev_type_attribute mdev_type_attr_##_name = __ATTR_RO(_name)
>   #define MDEV_TYPE_ATTR_WO(_name) \
> diff --git a/samples/vfio-mdev/mbochs.c b/samples/vfio-mdev/mbochs.c
> index e90c8552cc31..8d3ae97d9d6e 100644
> --- a/samples/vfio-mdev/mbochs.c
> +++ b/samples/vfio-mdev/mbochs.c
> @@ -1358,17 +1358,9 @@ static ssize_t available_instances_show(struct mdev_type *mtype,
>   }
>   static MDEV_TYPE_ATTR_RO(available_instances);
>   
> -static ssize_t device_api_show(struct mdev_type *mtype,
> -			       struct mdev_type_attribute *attr, char *buf)
> -{
> -	return sprintf(buf, "%s\n", VFIO_DEVICE_API_PCI_STRING);
> -}
> -static MDEV_TYPE_ATTR_RO(device_api);
> -
>   static struct attribute *mdev_types_attrs[] = {
>   	&mdev_type_attr_name.attr,
>   	&mdev_type_attr_description.attr,
> -	&mdev_type_attr_device_api.attr,
>   	&mdev_type_attr_available_instances.attr,
>   	NULL,
>   };
> @@ -1417,6 +1409,7 @@ static struct mdev_driver mbochs_driver = {
>   static const struct mdev_parent_ops mdev_fops = {
>   	.owner			= THIS_MODULE,
>   	.device_driver		= &mbochs_driver,
> +	.device_api		= VFIO_DEVICE_API_PCI_STRING,
>   	.supported_type_groups	= mdev_type_groups,
>   };
>   
> diff --git a/samples/vfio-mdev/mdpy.c b/samples/vfio-mdev/mdpy.c
> index fe5d43e797b6..402a7ebe6563 100644
> --- a/samples/vfio-mdev/mdpy.c
> +++ b/samples/vfio-mdev/mdpy.c
> @@ -670,17 +670,9 @@ static ssize_t available_instances_show(struct mdev_type *mtype,
>   }
>   static MDEV_TYPE_ATTR_RO(available_instances);
>   
> -static ssize_t device_api_show(struct mdev_type *mtype,
> -			       struct mdev_type_attribute *attr, char *buf)
> -{
> -	return sprintf(buf, "%s\n", VFIO_DEVICE_API_PCI_STRING);
> -}
> -static MDEV_TYPE_ATTR_RO(device_api);
> -
>   static struct attribute *mdev_types_attrs[] = {
>   	&mdev_type_attr_name.attr,
>   	&mdev_type_attr_description.attr,
> -	&mdev_type_attr_device_api.attr,
>   	&mdev_type_attr_available_instances.attr,
>   	NULL,
>   };
> @@ -728,6 +720,7 @@ static struct mdev_driver mdpy_driver = {
>   static const struct mdev_parent_ops mdev_fops = {
>   	.owner			= THIS_MODULE,
>   	.device_driver          = &mdpy_driver,
> +	.device_api		= VFIO_DEVICE_API_PCI_STRING,
>   	.supported_type_groups	= mdev_type_groups,
>   };
>   
> diff --git a/samples/vfio-mdev/mtty.c b/samples/vfio-mdev/mtty.c
> index a0e1a469bd47..5dc1b6a4c02c 100644
> --- a/samples/vfio-mdev/mtty.c
> +++ b/samples/vfio-mdev/mtty.c
> @@ -1281,17 +1281,8 @@ static ssize_t available_instances_show(struct mdev_type *mtype,
>   
>   static MDEV_TYPE_ATTR_RO(available_instances);
>   
> -static ssize_t device_api_show(struct mdev_type *mtype,
> -			       struct mdev_type_attribute *attr, char *buf)
> -{
> -	return sprintf(buf, "%s\n", VFIO_DEVICE_API_PCI_STRING);
> -}
> -
> -static MDEV_TYPE_ATTR_RO(device_api);
> -
>   static struct attribute *mdev_types_attrs[] = {
>   	&mdev_type_attr_name.attr,
> -	&mdev_type_attr_device_api.attr,
>   	&mdev_type_attr_available_instances.attr,
>   	NULL,
>   };
> @@ -1333,6 +1324,7 @@ static struct mdev_driver mtty_driver = {
>   static const struct mdev_parent_ops mdev_fops = {
>   	.owner                  = THIS_MODULE,
>   	.device_driver		= &mtty_driver,
> +	.device_api		= VFIO_DEVICE_API_PCI_STRING,
>   	.dev_attr_groups        = mtty_dev_groups,
>   	.supported_type_groups  = mdev_type_groups,
>   };

Re: [PATCH v1 14/18] vfio/mdev: Add mdev available instance checking to the core

[Tony Krowiak]

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

On 6/2/22 1:19 PM, Eric Farman wrote:
> From: Jason Gunthorpe <jgg@nvidia.com>
>
> Many of the mdev drivers use a simple counter for keeping track of the
> available instances. Move this code to the core code and store the counter
> in the mdev_type. Implement it using correct locking, fixing mdpy.
>
> Drivers provide a get_available() callback to set the number of available
> instances for their mtypes which is fixed at registration time. The core
> provides a standard sysfs attribute to return the available_instances.
>
> Cc: Kirti Wankhede <kwankhede@nvidia.com>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Tony Krowiak <akrowiak@linux.ibm.com>
> Cc: Jason Herne <jjherne@linux.ibm.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> Link: https://lore.kernel.org/r/7-v3-57c1502c62fd+2190-ccw_mdev_jgg@nvidia.com/
> [farman: added Cc: tags]
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>   .../driver-api/vfio-mediated-device.rst       |  4 +-
>   drivers/s390/cio/vfio_ccw_drv.c               |  1 -
>   drivers/s390/cio/vfio_ccw_ops.c               | 26 ++++---------
>   drivers/s390/cio/vfio_ccw_private.h           |  2 -
>   drivers/s390/crypto/vfio_ap_ops.c             | 32 ++++------------
>   drivers/s390/crypto/vfio_ap_private.h         |  2 -
>   drivers/vfio/mdev/mdev_core.c                 | 11 +++++-
>   drivers/vfio/mdev/mdev_private.h              |  2 +
>   drivers/vfio/mdev/mdev_sysfs.c                | 37 +++++++++++++++++++
>   include/linux/mdev.h                          |  2 +
>   samples/vfio-mdev/mdpy.c                      | 22 +++--------
>   11 files changed, 76 insertions(+), 65 deletions(-)
>
> diff --git a/Documentation/driver-api/vfio-mediated-device.rst b/Documentation/driver-api/vfio-mediated-device.rst
> index f410a1cd98bb..a4f7f1362fa8 100644
> --- a/Documentation/driver-api/vfio-mediated-device.rst
> +++ b/Documentation/driver-api/vfio-mediated-device.rst
> @@ -106,6 +106,7 @@ structure to represent a mediated device's driver::
>   	     int  (*probe)  (struct mdev_device *dev);
>   	     void (*remove) (struct mdev_device *dev);
>   	     struct device_driver    driver;
> +	     unsigned int (*get_available)(struct mdev_type *mtype);
>        };
>   
>   A mediated bus driver for mdev should use this structure in the function calls
> @@ -232,7 +233,8 @@ Directories and files under the sysfs for Each Physical Device
>   * available_instances
>   
>     This attribute should show the number of devices of type <type-id> that can be
> -  created.
> +  created. Drivers can supply a get_available() function pointer to have the
> +  core code create and maintain this sysfs automatically.
>   
>   * [device]
>   
> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> index e4b285953a45..6dca35f3ceba 100644
> --- a/drivers/s390/cio/vfio_ccw_drv.c
> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> @@ -147,7 +147,6 @@ static struct vfio_ccw_private *vfio_ccw_alloc_private(struct subchannel *sch)
>   	INIT_LIST_HEAD(&private->crw);
>   	INIT_WORK(&private->io_work, vfio_ccw_sch_io_todo);
>   	INIT_WORK(&private->crw_work, vfio_ccw_crw_todo);
> -	atomic_set(&private->avail, 1);
>   
>   	private->cp.guest_cp = kcalloc(CCWCHAIN_LEN_MAX, sizeof(struct ccw1),
>   				       GFP_KERNEL);
> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index 6793c8b3c58b..60a4855e8ecb 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
> @@ -66,20 +66,9 @@ static ssize_t name_show(struct mdev_type *mtype,
>   }
>   static MDEV_TYPE_ATTR_RO(name);
>   
> -static ssize_t available_instances_show(struct mdev_type *mtype,
> -					struct mdev_type_attribute *attr,
> -					char *buf)
> -{
> -	struct vfio_ccw_private *private =
> -		dev_get_drvdata(mtype_get_parent_dev(mtype));
> -
> -	return sprintf(buf, "%d\n", atomic_read(&private->avail));
> -}
> -static MDEV_TYPE_ATTR_RO(available_instances);
>   
>   static struct attribute *mdev_types_attrs[] = {
>   	&mdev_type_attr_name.attr,
> -	&mdev_type_attr_available_instances.attr,
>   	NULL,
>   };
>   
> @@ -101,9 +90,6 @@ static int vfio_ccw_mdev_probe(struct mdev_device *mdev)
>   	if (private->state == VFIO_CCW_STATE_NOT_OPER)
>   		return -ENODEV;
>   
> -	if (atomic_dec_if_positive(&private->avail) < 0)
> -		return -EPERM;
> -
>   	memset(&private->vdev, 0, sizeof(private->vdev));
>   	vfio_init_group_dev(&private->vdev, &mdev->dev,
>   			    &vfio_ccw_dev_ops);
> @@ -115,13 +101,12 @@ static int vfio_ccw_mdev_probe(struct mdev_device *mdev)
>   
>   	ret = vfio_register_emulated_iommu_dev(&private->vdev);
>   	if (ret)
> -		goto err_atomic;
> +		goto err_init;
>   	dev_set_drvdata(&mdev->dev, private);
>   	return 0;
>   
> -err_atomic:
> +err_init:
>   	vfio_uninit_group_dev(&private->vdev);
> -	atomic_inc(&private->avail);
>   	return ret;
>   }
>   
> @@ -143,7 +128,6 @@ static void vfio_ccw_mdev_remove(struct mdev_device *mdev)
>   	vfio_ccw_fsm_event(private, VFIO_CCW_EVENT_CLOSE);
>   
>   	vfio_uninit_group_dev(&private->vdev);
> -	atomic_inc(&private->avail);
>   }
>   
>   static int vfio_ccw_mdev_open_device(struct vfio_device *vdev)
> @@ -614,6 +598,11 @@ static void vfio_ccw_mdev_request(struct vfio_device *vdev, unsigned int count)
>   	}
>   }
>   
> +static unsigned int vfio_ccw_get_available(struct mdev_type *mtype)
> +{
> +	return 1;
> +}
> +
>   static const struct vfio_device_ops vfio_ccw_dev_ops = {
>   	.open_device = vfio_ccw_mdev_open_device,
>   	.close_device = vfio_ccw_mdev_close_device,
> @@ -631,6 +620,7 @@ struct mdev_driver vfio_ccw_mdev_driver = {
>   	},
>   	.probe = vfio_ccw_mdev_probe,
>   	.remove = vfio_ccw_mdev_remove,
> +	.get_available = vfio_ccw_get_available,
>   };
>   
>   const struct mdev_parent_ops vfio_ccw_mdev_ops = {
> diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
> index 02a4a5edd00c..e568fd6bcf2a 100644
> --- a/drivers/s390/cio/vfio_ccw_private.h
> +++ b/drivers/s390/cio/vfio_ccw_private.h
> @@ -72,7 +72,6 @@ struct vfio_ccw_crw {
>    * @sch: pointer to the subchannel
>    * @state: internal state of the device
>    * @completion: synchronization helper of the I/O completion
> - * @avail: available for creating a mediated device
>    * @nb: notifier for vfio events
>    * @io_region: MMIO region to input/output I/O arguments/results
>    * @io_mutex: protect against concurrent update of I/O regions
> @@ -95,7 +94,6 @@ struct vfio_ccw_private {
>   	struct subchannel	*sch;
>   	int			state;
>   	struct completion	*completion;
> -	atomic_t		avail;
>   	struct notifier_block	nb;
>   	struct ccw_io_region	*io_region;
>   	struct mutex		io_mutex;
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 838b1a3eac8a..4c62ba9c72d8 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -462,14 +462,9 @@ static int vfio_ap_mdev_probe(struct mdev_device *mdev)
>   	struct ap_matrix_mdev *matrix_mdev;
>   	int ret;
>   
> -	if ((atomic_dec_if_positive(&matrix_dev->available_instances) < 0))
> -		return -EPERM;
> -
>   	matrix_mdev = kzalloc(sizeof(*matrix_mdev), GFP_KERNEL);
> -	if (!matrix_mdev) {
> -		ret = -ENOMEM;
> -		goto err_dec_available;
> -	}
> +	if (!matrix_mdev)
> +		return -ENOMEM;
>   	vfio_init_group_dev(&matrix_mdev->vdev, &mdev->dev,
>   			    &vfio_ap_matrix_dev_ops);
>   
> @@ -492,8 +487,6 @@ static int vfio_ap_mdev_probe(struct mdev_device *mdev)
>   	mutex_unlock(&matrix_dev->lock);
>   	vfio_uninit_group_dev(&matrix_mdev->vdev);
>   	kfree(matrix_mdev);
> -err_dec_available:
> -	atomic_inc(&matrix_dev->available_instances);
>   	return ret;
>   }
>   
> @@ -509,7 +502,6 @@ static void vfio_ap_mdev_remove(struct mdev_device *mdev)
>   	mutex_unlock(&matrix_dev->lock);
>   	vfio_uninit_group_dev(&matrix_mdev->vdev);
>   	kfree(matrix_mdev);
> -	atomic_inc(&matrix_dev->available_instances);
>   }
>   
>   static ssize_t name_show(struct mdev_type *mtype,
> @@ -520,20 +512,8 @@ static ssize_t name_show(struct mdev_type *mtype,
>   
>   static MDEV_TYPE_ATTR_RO(name);
>   
> -static ssize_t available_instances_show(struct mdev_type *mtype,
> -					struct mdev_type_attribute *attr,
> -					char *buf)
> -{
> -	return sprintf(buf, "%d\n",
> -		       atomic_read(&matrix_dev->available_instances));
> -}
> -
> -static MDEV_TYPE_ATTR_RO(available_instances);
> -
> -
>   static struct attribute *vfio_ap_mdev_type_attrs[] = {
>   	&mdev_type_attr_name.attr,
> -	&mdev_type_attr_available_instances.attr,
>   	NULL,
>   };
>   
> @@ -1473,6 +1453,11 @@ static ssize_t vfio_ap_mdev_ioctl(struct vfio_device *vdev,
>   	return ret;
>   }
>   
> +static unsigned int vfio_ap_mdev_get_available(struct mdev_type *mtype)
> +{
> +	return MAX_ZDEV_ENTRIES_EXT;
> +}
> +
>   static const struct vfio_device_ops vfio_ap_matrix_dev_ops = {
>   	.open_device = vfio_ap_mdev_open_device,
>   	.close_device = vfio_ap_mdev_close_device,
> @@ -1488,6 +1473,7 @@ static struct mdev_driver vfio_ap_matrix_driver = {
>   	},
>   	.probe = vfio_ap_mdev_probe,
>   	.remove = vfio_ap_mdev_remove,
> +	.get_available = vfio_ap_mdev_get_available,
>   };
>   
>   static const struct mdev_parent_ops vfio_ap_matrix_ops = {
> @@ -1501,8 +1487,6 @@ int vfio_ap_mdev_register(void)
>   {
>   	int ret;
>   
> -	atomic_set(&matrix_dev->available_instances, MAX_ZDEV_ENTRIES_EXT);
> -
>   	ret = mdev_register_driver(&vfio_ap_matrix_driver);
>   	if (ret)
>   		return ret;
> diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
> index 648fcaf8104a..5c0cbf03074b 100644
> --- a/drivers/s390/crypto/vfio_ap_private.h
> +++ b/drivers/s390/crypto/vfio_ap_private.h
> @@ -29,7 +29,6 @@
>    * struct ap_matrix_dev - Contains the data for the matrix device.
>    *
>    * @device:	generic device structure associated with the AP matrix device
> - * @available_instances: number of mediated matrix devices that can be created
>    * @info:	the struct containing the output from the PQAP(QCI) instruction
>    * @mdev_list:	the list of mediated matrix devices created
>    * @lock:	mutex for locking the AP matrix device. This lock will be
> @@ -41,7 +40,6 @@
>    */
>   struct ap_matrix_dev {
>   	struct device device;
> -	atomic_t available_instances;
>   	struct ap_config_info info;
>   	struct list_head mdev_list;
>   	struct mutex lock;
> diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
> index c3018e8e6d32..bb27ca0db948 100644
> --- a/drivers/vfio/mdev/mdev_core.c
> +++ b/drivers/vfio/mdev/mdev_core.c
> @@ -25,7 +25,7 @@ static DEFINE_MUTEX(parent_list_lock);
>   static struct class_compat *mdev_bus_compat_class;
>   
>   static LIST_HEAD(mdev_list);
> -static DEFINE_MUTEX(mdev_list_lock);
> +DEFINE_MUTEX(mdev_list_lock);
>   
>   struct device *mdev_parent_dev(struct mdev_device *mdev)
>   {
> @@ -245,6 +245,7 @@ static void mdev_device_release(struct device *dev)
>   
>   	mutex_lock(&mdev_list_lock);
>   	list_del(&mdev->next);
> +	mdev->type->available++;
>   	mutex_unlock(&mdev_list_lock);
>   
>   	dev_dbg(&mdev->dev, "MDEV: destroying\n");
> @@ -268,6 +269,14 @@ int mdev_device_create(struct mdev_type *type, const guid_t *uuid)
>   		}
>   	}
>   
> +	if (drv && drv->get_available) {
> +		if (!type->available) {
> +			mutex_unlock(&mdev_list_lock);
> +			return -EUSERS;
> +		}
> +		type->available--;
> +	}
> +
>   	mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
>   	if (!mdev) {
>   		mutex_unlock(&mdev_list_lock);
> diff --git a/drivers/vfio/mdev/mdev_private.h b/drivers/vfio/mdev/mdev_private.h
> index afbad7b0a14a..83586b070233 100644
> --- a/drivers/vfio/mdev/mdev_private.h
> +++ b/drivers/vfio/mdev/mdev_private.h
> @@ -29,6 +29,7 @@ struct mdev_type {
>   	struct kobject *devices_kobj;
>   	struct mdev_parent *parent;
>   	struct list_head next;
> +	unsigned int available;
>   	unsigned int type_group_id;
>   };
>   
> @@ -38,6 +39,7 @@ struct mdev_type {
>   	container_of(_kobj, struct mdev_type, kobj)
>   
>   extern struct mdev_driver vfio_mdev_driver;
> +extern struct mutex mdev_list_lock;
>   
>   int  parent_create_sysfs_files(struct mdev_parent *parent);
>   void parent_remove_sysfs_files(struct mdev_parent *parent);
> diff --git a/drivers/vfio/mdev/mdev_sysfs.c b/drivers/vfio/mdev/mdev_sysfs.c
> index d4b99440d19e..b3129dfc27ef 100644
> --- a/drivers/vfio/mdev/mdev_sysfs.c
> +++ b/drivers/vfio/mdev/mdev_sysfs.c
> @@ -93,8 +93,41 @@ static struct attribute_group mdev_type_std_group = {
>   	.attrs = mdev_types_std_attrs,
>   };
>   
> +/* mdev_type attribute used by drivers that have an get_available() op */
> +static ssize_t available_instances_show(struct mdev_type *mtype,
> +					struct mdev_type_attribute *attr,
> +					char *buf)
> +{
> +	unsigned int available;
> +
> +	mutex_lock(&mdev_list_lock);
> +	available = mtype->available;
> +	mutex_unlock(&mdev_list_lock);
> +
> +	return sysfs_emit(buf, "%u\n", available);
> +}
> +static MDEV_TYPE_ATTR_RO(available_instances);
> +static umode_t available_instances_is_visible(struct kobject *kobj,
> +					      struct attribute *attr, int n)
> +{
> +	struct mdev_type *type = to_mdev_type(kobj);
> +
> +	if (!type->parent->ops->device_driver->get_available)
> +		return 0;
> +	return attr->mode;
> +}
> +static struct attribute *mdev_types_name_attrs[] = {
> +	&mdev_type_attr_available_instances.attr,
> +	NULL,
> +};
> +static struct attribute_group mdev_type_available_instances_group = {
> +	.attrs = mdev_types_name_attrs,
> +	.is_visible = available_instances_is_visible,
> +};
> +
>   static const struct attribute_group *mdev_type_groups[] = {
>   	&mdev_type_std_group,
> +	&mdev_type_available_instances_group,
>   	NULL,
>   };
>   
> @@ -136,6 +169,10 @@ static struct mdev_type *add_mdev_supported_type(struct mdev_parent *parent,
>   	mdev_get_parent(parent);
>   	type->type_group_id = type_group_id;
>   
> +	if (parent->ops->device_driver->get_available)
> +		type->available =
> +			parent->ops->device_driver->get_available(type);
> +
>   	ret = kobject_init_and_add(&type->kobj, &mdev_type_ktype, NULL,
>   				   "%s-%s", dev_driver_string(parent->dev),
>   				   group->name);
> diff --git a/include/linux/mdev.h b/include/linux/mdev.h
> index 14655215417b..0ce1bb3dabd0 100644
> --- a/include/linux/mdev.h
> +++ b/include/linux/mdev.h
> @@ -120,12 +120,14 @@ struct mdev_type_attribute {
>    * @probe: called when new device created
>    * @remove: called when device removed
>    * @driver: device driver structure
> + * @get_available: Return the max number of instances that can be created
>    *
>    **/
>   struct mdev_driver {
>   	int (*probe)(struct mdev_device *dev);
>   	void (*remove)(struct mdev_device *dev);
>   	struct device_driver driver;
> +	unsigned int (*get_available)(struct mdev_type *mtype);
>   };
>   
>   static inline void *mdev_get_drvdata(struct mdev_device *mdev)
> diff --git a/samples/vfio-mdev/mdpy.c b/samples/vfio-mdev/mdpy.c
> index 402a7ebe6563..d7da6ed35657 100644
> --- a/samples/vfio-mdev/mdpy.c
> +++ b/samples/vfio-mdev/mdpy.c
> @@ -84,7 +84,6 @@ static dev_t		mdpy_devt;
>   static struct class	*mdpy_class;
>   static struct cdev	mdpy_cdev;
>   static struct device	mdpy_dev;
> -static u32		mdpy_count;
>   static const struct vfio_device_ops mdpy_dev_ops;
>   
>   /* State of each mdev device */
> @@ -225,9 +224,6 @@ static int mdpy_probe(struct mdev_device *mdev)
>   	u32 fbsize;
>   	int ret;
>   
> -	if (mdpy_count >= max_devices)
> -		return -ENOMEM;
> -
>   	mdev_state = kzalloc(sizeof(struct mdev_state), GFP_KERNEL);
>   	if (mdev_state == NULL)
>   		return -ENOMEM;
> @@ -256,8 +252,6 @@ static int mdpy_probe(struct mdev_device *mdev)
>   	mdpy_create_config_space(mdev_state);
>   	mdpy_reset(mdev_state);
>   
> -	mdpy_count++;
> -
>   	ret = vfio_register_emulated_iommu_dev(&mdev_state->vdev);
>   	if (ret)
>   		goto err_mem;
> @@ -284,8 +278,6 @@ static void mdpy_remove(struct mdev_device *mdev)
>   	kfree(mdev_state->vconfig);
>   	vfio_uninit_group_dev(&mdev_state->vdev);
>   	kfree(mdev_state);
> -
> -	mdpy_count--;
>   }
>   
>   static ssize_t mdpy_read(struct vfio_device *vdev, char __user *buf,
> @@ -662,18 +654,10 @@ static ssize_t description_show(struct mdev_type *mtype,
>   }
>   static MDEV_TYPE_ATTR_RO(description);
>   
> -static ssize_t available_instances_show(struct mdev_type *mtype,
> -					struct mdev_type_attribute *attr,
> -					char *buf)
> -{
> -	return sprintf(buf, "%d\n", max_devices - mdpy_count);
> -}
> -static MDEV_TYPE_ATTR_RO(available_instances);
>   
>   static struct attribute *mdev_types_attrs[] = {
>   	&mdev_type_attr_name.attr,
>   	&mdev_type_attr_description.attr,
> -	&mdev_type_attr_available_instances.attr,
>   	NULL,
>   };
>   
> @@ -706,6 +690,11 @@ static const struct vfio_device_ops mdpy_dev_ops = {
>   	.mmap = mdpy_mmap,
>   };
>   
> +static unsigned int mdpy_get_available(struct mdev_type *mtype)
> +{
> +	return max_devices;
> +}
> +
>   static struct mdev_driver mdpy_driver = {
>   	.driver = {
>   		.name = "mdpy",
> @@ -715,6 +704,7 @@ static struct mdev_driver mdpy_driver = {
>   	},
>   	.probe = mdpy_probe,
>   	.remove	= mdpy_remove,
> +	.get_available = mdpy_get_available,
>   };
>   
>   static const struct mdev_parent_ops mdev_fops = {

Re: [PATCH v1 03/18] vfio/ccw: Ensure mdev->dev is cleared on mdev remove

[Tony Krowiak]

On 6/3/22 9:37 AM, Jason Gunthorpe wrote:
> On Fri, Jun 03, 2022 at 09:25:19AM -0400, Matthew Rosato wrote:
>> On 6/2/22 1:19 PM, Eric Farman wrote:
>>> The mdev is linked with the vfio_ccw_private pointer when the mdev
>>> is probed, but it's not cleared once the mdev is removed.
>>>
>>> This isn't much of a concern based on the current device lifecycle,
>>> but fix it so that things make sense in later shuffling.
>>>
>>> Fixes: 3bf1311f351ef ("vfio/ccw: Convert to use vfio_register_emulated_iommu_dev()")
>>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
>>>    drivers/s390/cio/vfio_ccw_ops.c | 1 +
>>>    1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
>>> index a403d059a4e6..a0a3200b0b04 100644
>>> +++ b/drivers/s390/cio/vfio_ccw_ops.c
>>> @@ -159,6 +159,7 @@ static void vfio_ccw_mdev_remove(struct mdev_device *mdev)
>>>    			   private->sch->schid.ssid,
>>>    			   private->sch->schid.sch_no);
>>> +	dev_set_drvdata(&mdev->dev, NULL);
>>>    	vfio_unregister_group_dev(&private->vdev);
>>>    	if ((private->state != VFIO_CCW_STATE_NOT_OPER) &&
>> Seems harmless enough.
>>
>> Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
>>
>> But is this just precautionary or is it fixing a real problem (if the former
>> I don't think a fixes tag makes sense)
>>
>> I also ask because I note vfio-ap clears its driver_data in mdev_remove but
>> also leaves the pointer set, meaning they might need a similar cleanup and
>> should probably have a look (CC Tony & Jason H)
> There should be no reason to clear the drvdata on remove - the driver
> must be designed to guarentee all references to the dev stop before
> the remove function returns.

Each function that gets the driver data
backs a sysfs attribute of the mdev. If the mdev is removed, these
attributes will not exist. The mdev remove callback must get the
same locks acquired by the sysfs attribute functions before clearing
the driver data, so I believe this guarantees that all references to
the dev stop before the remove function returns.

>
> Jason

Re: [PATCH v1 01/18] vfio/ccw: Remove UUID from s390 debug log

[Eric Farman]

On Thu, 2022-06-02 at 15:51 -0400, Matthew Rosato wrote:
> On 6/2/22 1:19 PM, Eric Farman wrote:
> > From: Michael Kawano <mkawano@linux.ibm.com>
> > 
> > As vfio-ccw devices are created/destroyed, the uuid of the
> > associated
> > mdevs that are recorded in $S390DBF/vfio_ccw_msg/sprintf get lost
> > as
> > they are created using pointers passed by reference.
> > 
> > This is a deliberate design point of s390dbf, but it leaves the
> > uuid
> 
> This wording is confusing, maybe some re-wording would help here.
> 
> Basically, s390dbf doesn't support values passed by reference today 
> (e.g. %pUl), it will just store that pointer (e.g. &mdev->uuid) and
> not 
> its contents -- so a subsequent viewing of the s390dbf log at any
> point 
> in the future will go peek at that referenced memory -- which might
> have 
> been freed (e.g. mdev was removed).  So this change will fix
> potential 
> garbage data viewed from the log or worse an oops when viewing the
> log 
> -- the latter of which should probably be mentioned in the commit
> message.
> 
> I'm not sure if it was a deliberate design decision of s390dbf or
> just a 
> feature that was never implemented, so I'd omit that altogether --
> but 
> it IS pointed out in the s390dbf documentation as a limitation
> anyway.

@Jason, @Matt...  All fair, I obviously got too verbose in whatever I
was writing at the time. I've changed this to:

As vfio-ccw devices are created/destroyed, the uuid of the associated
mdevs that are recorded in $S390DBF/vfio_ccw_msg/sprintf get lost.
This is because a pointer to the UUID is stored instead of the UUID
itself, and that memory may have been repurposed if/when the logs are
examined. The result is usually garbage UUID data in the logs, though
there is an outside chance of an oops happening here.

Simply remove the UUID from the traces, as the subchannel number will
provide useful configuration information for problem determination,
and is stored directly into the log instead of a pointer.

As we were the only consumer of mdev_uuid(), remove that too.

> 
> The code itself is fine:
> 
> Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
> 
> > in these traces less than useful. Since the subchannels are more
> > constant, and are mapped 1:1 with the mdevs, the associated mdev
> > can
> > be discerned by looking at the device configuration (e.g., mdevctl)
> > and places, such as kernel messages, where it is statically stored.
> > 
> > Thus, let's just remove the uuid from s390dbf traces. As we were
> > the only consumer of mdev_uuid(), remove that too.
> > 
> > Cc: Kirti Wankhede <kwankhede@nvidia.com>
> > Signed-off-by: Michael Kawano <mkawano@linux.ibm.com>
> > Fixes: 60e05d1cf0875 ("vfio-ccw: add some logging")
> > Fixes: b7701dfbf9832 ("vfio-ccw: Register a chp_event callback for
> > vfio-ccw")
> > [farman: reworded commit message, added Fixes: tags]
> > Signed-off-by: Eric Farman <farman@linux.ibm.com>
> > ---
> >   drivers/s390/cio/vfio_ccw_drv.c |  5 ++---
> >   drivers/s390/cio/vfio_ccw_fsm.c | 24 ++++++++++++------------
> >   drivers/s390/cio/vfio_ccw_ops.c |  8 ++++----
> >   include/linux/mdev.h            |  4 ----
> >   4 files changed, 18 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/s390/cio/vfio_ccw_drv.c
> > b/drivers/s390/cio/vfio_ccw_drv.c
> > index ee182cfb467d..35055eb94115 100644
> > --- a/drivers/s390/cio/vfio_ccw_drv.c
> > +++ b/drivers/s390/cio/vfio_ccw_drv.c
> > @@ -14,7 +14,6 @@
> >   #include <linux/init.h>
> >   #include <linux/device.h>
> >   #include <linux/slab.h>
> > -#include <linux/uuid.h>
> >   #include <linux/mdev.h>
> >   
> >   #include <asm/isc.h>
> > @@ -358,8 +357,8 @@ static int vfio_ccw_chp_event(struct subchannel
> > *sch,
> >   		return 0;
> >   
> >   	trace_vfio_ccw_chp_event(private->sch->schid, mask, event);
> > -	VFIO_CCW_MSG_EVENT(2, "%pUl (%x.%x.%04x): mask=0x%x
> > event=%d\n",
> > -			   mdev_uuid(private->mdev), sch->schid.cssid,
> > +	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,
> >   			   mask, event);
> >   
> > diff --git a/drivers/s390/cio/vfio_ccw_fsm.c
> > b/drivers/s390/cio/vfio_ccw_fsm.c
> > index e435a9cd92da..86b23732d899 100644
> > --- a/drivers/s390/cio/vfio_ccw_fsm.c
> > +++ b/drivers/s390/cio/vfio_ccw_fsm.c
> > @@ -256,8 +256,8 @@ static void fsm_io_request(struct
> > vfio_ccw_private *private,
> >   		if (orb->tm.b) {
> >   			io_region->ret_code = -EOPNOTSUPP;
> >   			VFIO_CCW_MSG_EVENT(2,
> > -					   "%pUl (%x.%x.%04x):
> > transport mode\n",
> > -					   mdev_uuid(mdev),
> > schid.cssid,
> > +					   "sch %x.%x.%04x: transport
> > mode\n",
> > +					   schid.cssid,
> >   					   schid.ssid, schid.sch_no);
> >   			errstr = "transport mode";
> >   			goto err_out;
> > @@ -266,8 +266,8 @@ static void fsm_io_request(struct
> > vfio_ccw_private *private,
> >   					      orb);
> >   		if (io_region->ret_code) {
> >   			VFIO_CCW_MSG_EVENT(2,
> > -					   "%pUl (%x.%x.%04x):
> > cp_init=%d\n",
> > -					   mdev_uuid(mdev),
> > schid.cssid,
> > +					   "sch %x.%x.%04x:
> > cp_init=%d\n",
> > +					   schid.cssid,
> >   					   schid.ssid, schid.sch_no,
> >   					   io_region->ret_code);
> >   			errstr = "cp init";
> > @@ -277,8 +277,8 @@ static void fsm_io_request(struct
> > vfio_ccw_private *private,
> >   		io_region->ret_code = cp_prefetch(&private->cp);
> >   		if (io_region->ret_code) {
> >   			VFIO_CCW_MSG_EVENT(2,
> > -					   "%pUl (%x.%x.%04x):
> > cp_prefetch=%d\n",
> > -					   mdev_uuid(mdev),
> > schid.cssid,
> > +					   "sch %x.%x.%04x:
> > cp_prefetch=%d\n",
> > +					   schid.cssid,
> >   					   schid.ssid, schid.sch_no,
> >   					   io_region->ret_code);
> >   			errstr = "cp prefetch";
> > @@ -290,8 +290,8 @@ static void fsm_io_request(struct
> > vfio_ccw_private *private,
> >   		io_region->ret_code = fsm_io_helper(private);
> >   		if (io_region->ret_code) {
> >   			VFIO_CCW_MSG_EVENT(2,
> > -					   "%pUl (%x.%x.%04x):
> > fsm_io_helper=%d\n",
> > -					   mdev_uuid(mdev),
> > schid.cssid,
> > +					   "sch %x.%x.%04x:
> > fsm_io_helper=%d\n",
> > +					   schid.cssid,
> >   					   schid.ssid, schid.sch_no,
> >   					   io_region->ret_code);
> >   			errstr = "cp fsm_io_helper";
> > @@ -301,16 +301,16 @@ static void fsm_io_request(struct
> > vfio_ccw_private *private,
> >   		return;
> >   	} else if (scsw->cmd.fctl & SCSW_FCTL_HALT_FUNC) {
> >   		VFIO_CCW_MSG_EVENT(2,
> > -				   "%pUl (%x.%x.%04x): halt on
> > io_region\n",
> > -				   mdev_uuid(mdev), schid.cssid,
> > +				   "sch %x.%x.%04x: halt on
> > io_region\n",
> > +				   schid.cssid,
> >   				   schid.ssid, schid.sch_no);
> >   		/* halt is handled via the async cmd region */
> >   		io_region->ret_code = -EOPNOTSUPP;
> >   		goto err_out;
> >   	} else if (scsw->cmd.fctl & SCSW_FCTL_CLEAR_FUNC) {
> >   		VFIO_CCW_MSG_EVENT(2,
> > -				   "%pUl (%x.%x.%04x): clear on
> > io_region\n",
> > -				   mdev_uuid(mdev), schid.cssid,
> > +				   "sch %x.%x.%04x: clear on
> > io_region\n",
> > +				   schid.cssid,
> >   				   schid.ssid, schid.sch_no);
> >   		/* clear is handled via the async cmd region */
> >   		io_region->ret_code = -EOPNOTSUPP;
> > diff --git a/drivers/s390/cio/vfio_ccw_ops.c
> > b/drivers/s390/cio/vfio_ccw_ops.c
> > index d8589afac272..bebae21228aa 100644
> > --- a/drivers/s390/cio/vfio_ccw_ops.c
> > +++ b/drivers/s390/cio/vfio_ccw_ops.c
> > @@ -131,8 +131,8 @@ static int vfio_ccw_mdev_probe(struct
> > mdev_device *mdev)
> >   	private->mdev = mdev;
> >   	private->state = VFIO_CCW_STATE_IDLE;
> >   
> > -	VFIO_CCW_MSG_EVENT(2, "mdev %pUl, sch %x.%x.%04x: create\n",
> > -			   mdev_uuid(mdev), private->sch->schid.cssid,
> > +	VFIO_CCW_MSG_EVENT(2, "sch %x.%x.%04x: create\n",
> > +			   private->sch->schid.cssid,
> >   			   private->sch->schid.ssid,
> >   			   private->sch->schid.sch_no);
> >   
> > @@ -154,8 +154,8 @@ static void vfio_ccw_mdev_remove(struct
> > mdev_device *mdev)
> >   {
> >   	struct vfio_ccw_private *private = dev_get_drvdata(mdev-
> > >dev.parent);
> >   
> > -	VFIO_CCW_MSG_EVENT(2, "mdev %pUl, sch %x.%x.%04x: remove\n",
> > -			   mdev_uuid(mdev), private->sch->schid.cssid,
> > +	VFIO_CCW_MSG_EVENT(2, "sch %x.%x.%04x: remove\n",
> > +			   private->sch->schid.cssid,
> >   			   private->sch->schid.ssid,
> >   			   private->sch->schid.sch_no);
> >   
> > diff --git a/include/linux/mdev.h b/include/linux/mdev.h
> > index 15d03f6532d0..a5788f592817 100644
> > --- a/include/linux/mdev.h
> > +++ b/include/linux/mdev.h
> > @@ -139,10 +139,6 @@ static inline void mdev_set_drvdata(struct
> > mdev_device *mdev, void *data)
> >   {
> >   	mdev->driver_data = data;
> >   }
> > -static inline const guid_t *mdev_uuid(struct mdev_device *mdev)
> > -{
> > -	return &mdev->uuid;
> > -}
> >   
> >   extern struct bus_type mdev_bus_type;
> >   

Re: [PATCH v1 02/18] vfio/ccw: Fix FSM state if mdev probe fails

[Eric Farman]

On Fri, 2022-06-03 at 09:21 -0400, Matthew Rosato wrote:
> On 6/2/22 1:19 PM, Eric Farman wrote:
> > The FSM is in STANDBY state when arriving in vfio_ccw_mdev_probe(),
> > and this routine converts it to IDLE as part of its processing.
> > The error exit sets it to IDLE (again) but clears the private->mdev
> > pointer.
> > 
> > The FSM should of course be managing the state itself, but the
> > correct thing for vfio_ccw_mdev_probe() to do would be to put
> > the state back the way it found it.
> > 
> > The corresponding check of private->mdev in vfio_ccw_sch_io_todo()
> > can be removed, since the distinction is unnecessary at this point.
> > 
> > Fixes: 3bf1311f351ef ("vfio/ccw: Convert to use
> > vfio_register_emulated_iommu_dev()")
> > Signed-off-by: Eric Farman <farman@linux.ibm.com>
> > ---
> >   drivers/s390/cio/vfio_ccw_drv.c | 2 +-
> >   drivers/s390/cio/vfio_ccw_ops.c | 2 +-
> >   2 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/s390/cio/vfio_ccw_drv.c
> > b/drivers/s390/cio/vfio_ccw_drv.c
> > index 35055eb94115..b18b4582bc8b 100644
> > --- a/drivers/s390/cio/vfio_ccw_drv.c
> > +++ b/drivers/s390/cio/vfio_ccw_drv.c
> > @@ -108,7 +108,7 @@ static void vfio_ccw_sch_io_todo(struct
> > work_struct *work)
> >   	 * has finished. Do not overwrite a possible processing
> >   	 * state if the final interrupt was for HSCH or CSCH.
> >   	 */
> > -	if (private->mdev && cp_is_finished)
> > +	if (cp_is_finished)
> >   		private->state = VFIO_CCW_STATE_IDLE;
> 
> Took me a bit to convince myself this was OK

Me too. :)

> , mainly because AFAICT 
> despite the change below the fsm jumptable would still allow you to 
> reach this code when in STANDBY.  But, it should only be possible for
> an 
> unsolicited interrupt (e.g. unsolicited implies !cp_is_finished) so
> we 
> would still avoid a STANDBY->IDLE transition on accident.
> 
> Maybe work unsolicited interrupt into the comment block above along
> with 
> HSCH/CSCH?

Good idea. How about:

        /*
         * Reset to IDLE only if
processing of a channel program
         * has finished. Do not
overwrite a possible processing
         * state if the interrupt was
unsolicited, or if the final
         * interrupt was for HSCH or CSCH.
 
        */

> 
> Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
> 
> >   
> >   	if (private->io_trigger)
> > diff --git a/drivers/s390/cio/vfio_ccw_ops.c
> > b/drivers/s390/cio/vfio_ccw_ops.c
> > index bebae21228aa..a403d059a4e6 100644
> > --- a/drivers/s390/cio/vfio_ccw_ops.c
> > +++ b/drivers/s390/cio/vfio_ccw_ops.c
> > @@ -146,7 +146,7 @@ static int vfio_ccw_mdev_probe(struct
> > mdev_device *mdev)
> >   	vfio_uninit_group_dev(&private->vdev);
> >   	atomic_inc(&private->avail);
> >   	private->mdev = NULL;
> > -	private->state = VFIO_CCW_STATE_IDLE;
> > +	private->state = VFIO_CCW_STATE_STANDBY;

Re: [PATCH v1 07/18] vfio/ccw: Flatten MDEV device (un)register

[Eric Farman]

On Thu, 2022-06-02 at 15:14 -0400, Matthew Rosato wrote:
> On 6/2/22 1:19 PM, Eric Farman wrote:
> > The vfio_ccw_mdev_(un)reg routines are merely vfio-ccw routines
> > that
> > pass control to mdev_(un)register_device. Since there's only one
> > caller of each, let's just call the mdev routines directly.
> 
> I'd reword slightly to reference the ops extern
> 
> ".. caller of each, externalize vfio_ccw_mdev_ops and call..."

Of course vfio_ccw_mdev_ops was removed in 5.19 via commit 6b42f491e17c
("vfio/mdev: Remove mdev_parent_ops"), so the extern doesn't happen
now.

> 
> regardless,
> 
> Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
> 
> > Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> > Signed-off-by: Eric Farman <farman@linux.ibm.com>
> > ---
> >   drivers/s390/cio/vfio_ccw_drv.c     |  4 ++--
> >   drivers/s390/cio/vfio_ccw_ops.c     | 12 +-----------
> >   drivers/s390/cio/vfio_ccw_private.h |  4 +---
> >   3 files changed, 4 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/s390/cio/vfio_ccw_drv.c
> > b/drivers/s390/cio/vfio_ccw_drv.c
> > index 9d817aa2f1c4..3784eb4cda85 100644
> > --- a/drivers/s390/cio/vfio_ccw_drv.c
> > +++ b/drivers/s390/cio/vfio_ccw_drv.c
> > @@ -239,7 +239,7 @@ static int vfio_ccw_sch_probe(struct subchannel
> > *sch)
> >   
> >   	private->state = VFIO_CCW_STATE_STANDBY;
> >   
> > -	ret = vfio_ccw_mdev_reg(sch);
> > +	ret = mdev_register_device(&sch->dev, &vfio_ccw_mdev_ops);
> >   	if (ret)
> >   		goto out_disable;
> >   
> > @@ -261,7 +261,7 @@ static void vfio_ccw_sch_remove(struct
> > subchannel *sch)
> >   	struct vfio_ccw_private *private = dev_get_drvdata(&sch->dev);
> >   
> >   	vfio_ccw_sch_quiesce(sch);
> > -	vfio_ccw_mdev_unreg(sch);
> > +	mdev_unregister_device(&sch->dev);
> >   
> >   	dev_set_drvdata(&sch->dev, NULL);
> >   
> > diff --git a/drivers/s390/cio/vfio_ccw_ops.c
> > b/drivers/s390/cio/vfio_ccw_ops.c
> > index 4a64c176facb..497e1b7ffd61 100644
> > --- a/drivers/s390/cio/vfio_ccw_ops.c
> > +++ b/drivers/s390/cio/vfio_ccw_ops.c
> > @@ -656,18 +656,8 @@ struct mdev_driver vfio_ccw_mdev_driver = {
> >   	.remove = vfio_ccw_mdev_remove,
> >   };
> >   
> > -static const struct mdev_parent_ops vfio_ccw_mdev_ops = {
> > +const struct mdev_parent_ops vfio_ccw_mdev_ops = {
> >   	.owner			= THIS_MODULE,
> >   	.device_driver		= &vfio_ccw_mdev_driver,
> >   	.supported_type_groups  = mdev_type_groups,
> >   };
> > -
> > -int vfio_ccw_mdev_reg(struct subchannel *sch)
> > -{
> > -	return mdev_register_device(&sch->dev, &vfio_ccw_mdev_ops);
> > -}
> > -
> > -void vfio_ccw_mdev_unreg(struct subchannel *sch)
> > -{
> > -	mdev_unregister_device(&sch->dev);
> > -}
> > diff --git a/drivers/s390/cio/vfio_ccw_private.h
> > b/drivers/s390/cio/vfio_ccw_private.h
> > index 5c128eec596b..2e0744ac6492 100644
> > --- a/drivers/s390/cio/vfio_ccw_private.h
> > +++ b/drivers/s390/cio/vfio_ccw_private.h
> > @@ -117,12 +117,10 @@ struct vfio_ccw_private {
> >   	struct work_struct	crw_work;
> >   } __aligned(8);
> >   
> > -extern int vfio_ccw_mdev_reg(struct subchannel *sch);
> > -extern void vfio_ccw_mdev_unreg(struct subchannel *sch);
> > -
> >   extern int vfio_ccw_sch_quiesce(struct subchannel *sch);
> >   
> >   extern struct mdev_driver vfio_ccw_mdev_driver;
> > +extern const struct mdev_parent_ops vfio_ccw_mdev_ops;
> >   
> >   /*
> >    * States of the device statemachine.

Re: [PATCH v1 13/18] vfio/mdev: Consolidate all the device_api sysfs into the core code

[Kirti Wankhede]

Reviewed By: Kirti Wankhede <kwankhede@nvidia.com>

On 6/2/2022 10:49 PM, Eric Farman wrote:
> From: Jason Gunthorpe <jgg@nvidia.com>
> 
> Every driver just emits a static string, simply feed it through the ops
> and provide a standard sysfs show function.
> 
> Cc: Kirti Wankhede <kwankhede@nvidia.com>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
> Cc: Zhi Wang <zhi.a.wang@intel.com>
> Cc: Tony Krowiak <akrowiak@linux.ibm.com>
> Cc: Jason Herne <jjherne@linux.ibm.com>
> Cc: intel-gvt-dev@lists.freedesktop.org
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> Link: https://lore.kernel.org/r/6-v3-57c1502c62fd+2190-ccw_mdev_jgg@nvidia.com/
> [farman: added Cc: tags]
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>   .../driver-api/vfio-mediated-device.rst       |  4 ++-
>   drivers/gpu/drm/i915/gvt/kvmgt.c              |  9 +------
>   drivers/s390/cio/vfio_ccw_ops.c               |  9 +------
>   drivers/s390/crypto/vfio_ap_ops.c             |  9 +------
>   drivers/vfio/mdev/mdev_core.c                 |  2 +-
>   drivers/vfio/mdev/mdev_sysfs.c                | 27 ++++++++++++++++---
>   include/linux/mdev.h                          |  7 ++---
>   samples/vfio-mdev/mbochs.c                    |  9 +------
>   samples/vfio-mdev/mdpy.c                      |  9 +------
>   samples/vfio-mdev/mtty.c                      | 10 +------
>   10 files changed, 36 insertions(+), 59 deletions(-)
> 
> diff --git a/Documentation/driver-api/vfio-mediated-device.rst b/Documentation/driver-api/vfio-mediated-device.rst
> index 9f26079cacae..f410a1cd98bb 100644
> --- a/Documentation/driver-api/vfio-mediated-device.rst
> +++ b/Documentation/driver-api/vfio-mediated-device.rst
> @@ -137,6 +137,7 @@ The structures in the mdev_parent_ops structure are as follows:
>   * mdev_attr_groups: attributes of the mediated device
>   * supported_config: attributes to define supported configurations
>   * device_driver: device driver to bind for mediated device instances
> +* device_api: String to pass through the sysfs file below
>   
>   The mdev_parent_ops also still has various functions pointers.  Theses exist
>   for historical reasons only and shall not be used for new drivers.
> @@ -225,7 +226,8 @@ Directories and files under the sysfs for Each Physical Device
>   * device_api
>   
>     This attribute should show which device API is being created, for example,
> -  "vfio-pci" for a PCI device.
> +  "vfio-pci" for a PCI device. The core code maintins this sysfs using the
> +  device_api member of mdev_parent_ops.
>   
>   * available_instances
>   
> diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c
> index 057ec4490104..752d7a1211e6 100644
> --- a/drivers/gpu/drm/i915/gvt/kvmgt.c
> +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c
> @@ -163,12 +163,6 @@ static ssize_t available_instances_show(struct mdev_type *mtype,
>   	return sprintf(buf, "%u\n", num);
>   }
>   
> -static ssize_t device_api_show(struct mdev_type *mtype,
> -			       struct mdev_type_attribute *attr, char *buf)
> -{
> -	return sprintf(buf, "%s\n", VFIO_DEVICE_API_PCI_STRING);
> -}
> -
>   static ssize_t description_show(struct mdev_type *mtype,
>   				struct mdev_type_attribute *attr, char *buf)
>   {
> @@ -202,13 +196,11 @@ static ssize_t name_show(struct mdev_type *mtype,
>   }
>   
>   static MDEV_TYPE_ATTR_RO(available_instances);
> -static MDEV_TYPE_ATTR_RO(device_api);
>   static MDEV_TYPE_ATTR_RO(description);
>   static MDEV_TYPE_ATTR_RO(name);
>   
>   static struct attribute *gvt_type_attrs[] = {
>   	&mdev_type_attr_available_instances.attr,
> -	&mdev_type_attr_device_api.attr,
>   	&mdev_type_attr_description.attr,
>   	&mdev_type_attr_name.attr,
>   	NULL,
> @@ -1767,6 +1759,7 @@ static const struct attribute_group *intel_vgpu_groups[] = {
>   
>   static struct mdev_parent_ops intel_vgpu_ops = {
>   	.mdev_attr_groups       = intel_vgpu_groups,
> +	.device_api		= VFIO_DEVICE_API_PCI_STRING,
>   	.create			= intel_vgpu_create,
>   	.remove			= intel_vgpu_remove,
>   
> diff --git a/drivers/s390/cio/vfio_ccw_ops.c b/drivers/s390/cio/vfio_ccw_ops.c
> index 2afb8f13739f..6793c8b3c58b 100644
> --- a/drivers/s390/cio/vfio_ccw_ops.c
> +++ b/drivers/s390/cio/vfio_ccw_ops.c
> @@ -66,13 +66,6 @@ static ssize_t name_show(struct mdev_type *mtype,
>   }
>   static MDEV_TYPE_ATTR_RO(name);
>   
> -static ssize_t device_api_show(struct mdev_type *mtype,
> -			       struct mdev_type_attribute *attr, char *buf)
> -{
> -	return sprintf(buf, "%s\n", VFIO_DEVICE_API_CCW_STRING);
> -}
> -static MDEV_TYPE_ATTR_RO(device_api);
> -
>   static ssize_t available_instances_show(struct mdev_type *mtype,
>   					struct mdev_type_attribute *attr,
>   					char *buf)
> @@ -86,7 +79,6 @@ static MDEV_TYPE_ATTR_RO(available_instances);
>   
>   static struct attribute *mdev_types_attrs[] = {
>   	&mdev_type_attr_name.attr,
> -	&mdev_type_attr_device_api.attr,
>   	&mdev_type_attr_available_instances.attr,
>   	NULL,
>   };
> @@ -644,5 +636,6 @@ struct mdev_driver vfio_ccw_mdev_driver = {
>   const struct mdev_parent_ops vfio_ccw_mdev_ops = {
>   	.owner			= THIS_MODULE,
>   	.device_driver		= &vfio_ccw_mdev_driver,
> +	.device_api		= VFIO_DEVICE_API_CCW_STRING,
>   	.supported_type_groups  = mdev_type_groups,
>   };
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 6e08d04b605d..838b1a3eac8a 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -530,17 +530,9 @@ static ssize_t available_instances_show(struct mdev_type *mtype,
>   
>   static MDEV_TYPE_ATTR_RO(available_instances);
>   
> -static ssize_t device_api_show(struct mdev_type *mtype,
> -			       struct mdev_type_attribute *attr, char *buf)
> -{
> -	return sprintf(buf, "%s\n", VFIO_DEVICE_API_AP_STRING);
> -}
> -
> -static MDEV_TYPE_ATTR_RO(device_api);
>   
>   static struct attribute *vfio_ap_mdev_type_attrs[] = {
>   	&mdev_type_attr_name.attr,
> -	&mdev_type_attr_device_api.attr,
>   	&mdev_type_attr_available_instances.attr,
>   	NULL,
>   };
> @@ -1501,6 +1493,7 @@ static struct mdev_driver vfio_ap_matrix_driver = {
>   static const struct mdev_parent_ops vfio_ap_matrix_ops = {
>   	.owner			= THIS_MODULE,
>   	.device_driver		= &vfio_ap_matrix_driver,
> +	.device_api		= VFIO_DEVICE_API_AP_STRING,
>   	.supported_type_groups	= vfio_ap_mdev_type_groups,
>   };
>   
> diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
> index b314101237fe..c3018e8e6d32 100644
> --- a/drivers/vfio/mdev/mdev_core.c
> +++ b/drivers/vfio/mdev/mdev_core.c
> @@ -129,7 +129,7 @@ int mdev_register_device(struct device *dev, const struct mdev_parent_ops *ops)
>   	char *envp[] = { env_string, NULL };
>   
>   	/* check for mandatory ops */
> -	if (!ops || !ops->supported_type_groups)
> +	if (!ops || !ops->supported_type_groups || !ops->device_api)
>   		return -EINVAL;
>   	if (!ops->device_driver && (!ops->create || !ops->remove))
>   		return -EINVAL;
> diff --git a/drivers/vfio/mdev/mdev_sysfs.c b/drivers/vfio/mdev/mdev_sysfs.c
> index f5cf1931c54e..d4b99440d19e 100644
> --- a/drivers/vfio/mdev/mdev_sysfs.c
> +++ b/drivers/vfio/mdev/mdev_sysfs.c
> @@ -74,9 +74,30 @@ static ssize_t create_store(struct mdev_type *mtype,
>   
>   	return count;
>   }
> -
>   static MDEV_TYPE_ATTR_WO(create);
>   
> +static ssize_t device_api_show(struct mdev_type *mtype,
> +			       struct mdev_type_attribute *attr, char *buf)
> +{
> +	return sysfs_emit(buf, "%s\n", mtype->parent->ops->device_api);
> +}
> +static MDEV_TYPE_ATTR_RO(device_api);
> +
> +static struct attribute *mdev_types_std_attrs[] = {
> +	&mdev_type_attr_create.attr,
> +	&mdev_type_attr_device_api.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group mdev_type_std_group = {
> +	.attrs = mdev_types_std_attrs,
> +};
> +
> +static const struct attribute_group *mdev_type_groups[] = {
> +	&mdev_type_std_group,
> +	NULL,
> +};
> +
>   static void mdev_type_release(struct kobject *kobj)
>   {
>   	struct mdev_type *type = to_mdev_type(kobj);
> @@ -123,7 +144,7 @@ static struct mdev_type *add_mdev_supported_type(struct mdev_parent *parent,
>   		return ERR_PTR(ret);
>   	}
>   
> -	ret = sysfs_create_file(&type->kobj, &mdev_type_attr_create.attr);
> +	ret = sysfs_create_groups(&type->kobj, mdev_type_groups);
>   	if (ret)
>   		goto attr_create_failed;
>   
> @@ -144,7 +165,7 @@ static struct mdev_type *add_mdev_supported_type(struct mdev_parent *parent,
>   attrs_failed:
>   	kobject_put(type->devices_kobj);
>   attr_devices_failed:
> -	sysfs_remove_file(&type->kobj, &mdev_type_attr_create.attr);
> +	sysfs_remove_groups(&type->kobj, mdev_type_groups);
>   attr_create_failed:
>   	kobject_del(&type->kobj);
>   	kobject_put(&type->kobj);
> diff --git a/include/linux/mdev.h b/include/linux/mdev.h
> index a5788f592817..14655215417b 100644
> --- a/include/linux/mdev.h
> +++ b/include/linux/mdev.h
> @@ -36,6 +36,7 @@ struct device *mtype_get_parent_dev(struct mdev_type *mtype);
>    *
>    * @owner:		The module owner.
>    * @device_driver:	Which device driver to probe() on newly created devices
> + * @device_api:		String to return for the device_api sysfs
>    * @dev_attr_groups:	Attributes of the parent device.
>    * @mdev_attr_groups:	Attributes of the mediated device.
>    * @supported_type_groups: Attributes to define supported types. It is mandatory
> @@ -80,6 +81,7 @@ struct device *mtype_get_parent_dev(struct mdev_type *mtype);
>   struct mdev_parent_ops {
>   	struct module   *owner;
>   	struct mdev_driver *device_driver;
> +	const char *device_api;
>   	const struct attribute_group **dev_attr_groups;
>   	const struct attribute_group **mdev_attr_groups;
>   	struct attribute_group **supported_type_groups;
> @@ -108,11 +110,6 @@ struct mdev_type_attribute {
>   			 size_t count);
>   };
>   
> -#define MDEV_TYPE_ATTR(_name, _mode, _show, _store)		\
> -struct mdev_type_attribute mdev_type_attr_##_name =		\
> -	__ATTR(_name, _mode, _show, _store)
> -#define MDEV_TYPE_ATTR_RW(_name) \
> -	struct mdev_type_attribute mdev_type_attr_##_name = __ATTR_RW(_name)
>   #define MDEV_TYPE_ATTR_RO(_name) \
>   	struct mdev_type_attribute mdev_type_attr_##_name = __ATTR_RO(_name)
>   #define MDEV_TYPE_ATTR_WO(_name) \
> diff --git a/samples/vfio-mdev/mbochs.c b/samples/vfio-mdev/mbochs.c
> index e90c8552cc31..8d3ae97d9d6e 100644
> --- a/samples/vfio-mdev/mbochs.c
> +++ b/samples/vfio-mdev/mbochs.c
> @@ -1358,17 +1358,9 @@ static ssize_t available_instances_show(struct mdev_type *mtype,
>   }
>   static MDEV_TYPE_ATTR_RO(available_instances);
>   
> -static ssize_t device_api_show(struct mdev_type *mtype,
> -			       struct mdev_type_attribute *attr, char *buf)
> -{
> -	return sprintf(buf, "%s\n", VFIO_DEVICE_API_PCI_STRING);
> -}
> -static MDEV_TYPE_ATTR_RO(device_api);
> -
>   static struct attribute *mdev_types_attrs[] = {
>   	&mdev_type_attr_name.attr,
>   	&mdev_type_attr_description.attr,
> -	&mdev_type_attr_device_api.attr,
>   	&mdev_type_attr_available_instances.attr,
>   	NULL,
>   };
> @@ -1417,6 +1409,7 @@ static struct mdev_driver mbochs_driver = {
>   static const struct mdev_parent_ops mdev_fops = {
>   	.owner			= THIS_MODULE,
>   	.device_driver		= &mbochs_driver,
> +	.device_api		= VFIO_DEVICE_API_PCI_STRING,
>   	.supported_type_groups	= mdev_type_groups,
>   };
>   
> diff --git a/samples/vfio-mdev/mdpy.c b/samples/vfio-mdev/mdpy.c
> index fe5d43e797b6..402a7ebe6563 100644
> --- a/samples/vfio-mdev/mdpy.c
> +++ b/samples/vfio-mdev/mdpy.c
> @@ -670,17 +670,9 @@ static ssize_t available_instances_show(struct mdev_type *mtype,
>   }
>   static MDEV_TYPE_ATTR_RO(available_instances);
>   
> -static ssize_t device_api_show(struct mdev_type *mtype,
> -			       struct mdev_type_attribute *attr, char *buf)
> -{
> -	return sprintf(buf, "%s\n", VFIO_DEVICE_API_PCI_STRING);
> -}
> -static MDEV_TYPE_ATTR_RO(device_api);
> -
>   static struct attribute *mdev_types_attrs[] = {
>   	&mdev_type_attr_name.attr,
>   	&mdev_type_attr_description.attr,
> -	&mdev_type_attr_device_api.attr,
>   	&mdev_type_attr_available_instances.attr,
>   	NULL,
>   };
> @@ -728,6 +720,7 @@ static struct mdev_driver mdpy_driver = {
>   static const struct mdev_parent_ops mdev_fops = {
>   	.owner			= THIS_MODULE,
>   	.device_driver          = &mdpy_driver,
> +	.device_api		= VFIO_DEVICE_API_PCI_STRING,
>   	.supported_type_groups	= mdev_type_groups,
>   };
>   
> diff --git a/samples/vfio-mdev/mtty.c b/samples/vfio-mdev/mtty.c
> index a0e1a469bd47..5dc1b6a4c02c 100644
> --- a/samples/vfio-mdev/mtty.c
> +++ b/samples/vfio-mdev/mtty.c
> @@ -1281,17 +1281,8 @@ static ssize_t available_instances_show(struct mdev_type *mtype,
>   
>   static MDEV_TYPE_ATTR_RO(available_instances);
>   
> -static ssize_t device_api_show(struct mdev_type *mtype,
> -			       struct mdev_type_attribute *attr, char *buf)
> -{
> -	return sprintf(buf, "%s\n", VFIO_DEVICE_API_PCI_STRING);
> -}
> -
> -static MDEV_TYPE_ATTR_RO(device_api);
> -
>   static struct attribute *mdev_types_attrs[] = {
>   	&mdev_type_attr_name.attr,
> -	&mdev_type_attr_device_api.attr,
>   	&mdev_type_attr_available_instances.attr,
>   	NULL,
>   };
> @@ -1333,6 +1324,7 @@ static struct mdev_driver mtty_driver = {
>   static const struct mdev_parent_ops mdev_fops = {
>   	.owner                  = THIS_MODULE,
>   	.device_driver		= &mtty_driver,
> +	.device_api		= VFIO_DEVICE_API_PCI_STRING,
>   	.dev_attr_groups        = mtty_dev_groups,
>   	.supported_type_groups  = mdev_type_groups,
>   };

Re: [PATCH v1 14/18] vfio/mdev: Add mdev available instance checking to the core

[Kirti Wankhede]

On 6/2/2022 10:49 PM, Eric Farman wrote:
> From: Jason Gunthorpe <jgg@nvidia.com>
> 
> Many of the mdev drivers use a simple counter for keeping track of the
> available instances. Move this code to the core code and store the counter
> in the mdev_type. Implement it using correct locking, fixing mdpy.
> 
> Drivers provide a get_available() callback to set the number of available
> instances for their mtypes which is fixed at registration time. The core
> provides a standard sysfs attribute to return the available_instances.
> 
> Cc: Kirti Wankhede <kwankhede@nvidia.com>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Tony Krowiak <akrowiak@linux.ibm.com>
> Cc: Jason Herne <jjherne@linux.ibm.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> Link: https://lore.kernel.org/r/7-v3-57c1502c62fd+2190-ccw_mdev_jgg@nvidia.com/
> [farman: added Cc: tags]
> Signed-off-by: Eric Farman <farman@linux.ibm.com>
> ---
>   .../driver-api/vfio-mediated-device.rst       |  4 +-
>   drivers/s390/cio/vfio_ccw_drv.c               |  1 -
>   drivers/s390/cio/vfio_ccw_ops.c               | 26 ++++---------
>   drivers/s390/cio/vfio_ccw_private.h           |  2 -
>   drivers/s390/crypto/vfio_ap_ops.c             | 32 ++++------------
>   drivers/s390/crypto/vfio_ap_private.h         |  2 -
>   drivers/vfio/mdev/mdev_core.c                 | 11 +++++-
>   drivers/vfio/mdev/mdev_private.h              |  2 +
>   drivers/vfio/mdev/mdev_sysfs.c                | 37 +++++++++++++++++++
>   include/linux/mdev.h                          |  2 +
>   samples/vfio-mdev/mdpy.c                      | 22 +++--------
>   11 files changed, 76 insertions(+), 65 deletions(-)
> 
> diff --git a/Documentation/driver-api/vfio-mediated-device.rst b/Documentation/driver-api/vfio-mediated-device.rst
> index f410a1cd98bb..a4f7f1362fa8 100644
> --- a/Documentation/driver-api/vfio-mediated-device.rst
> +++ b/Documentation/driver-api/vfio-mediated-device.rst
> @@ -106,6 +106,7 @@ structure to represent a mediated device's driver::
>   	     int  (*probe)  (struct mdev_device *dev);
>   	     void (*remove) (struct mdev_device *dev);
>   	     struct device_driver    driver;
> +	     unsigned int (*get_available)(struct mdev_type *mtype);
>        };
>

This patch conflicts with Christoph Hellwig's patch. I see 
'supported_type_groups' is not is above structure, I beleive that your 
patch is applied on top of Christoph's patch series.

but then in below part of code, 'add_mdev_supported_type' has also being 
removed in Christoph's patch. So this patch would not get applied cleanly.

Thanks,
Kirti

> +/* mdev_type attribute used by drivers that have an get_available() op */
> +static ssize_t available_instances_show(struct mdev_type *mtype,
> +					struct mdev_type_attribute *attr,
> +					char *buf)
> +{
> +	unsigned int available;
> +
> +	mutex_lock(&mdev_list_lock);
> +	available = mtype->available;
> +	mutex_unlock(&mdev_list_lock);
> +
> +	return sysfs_emit(buf, "%u\n", available);
> +}
> +static MDEV_TYPE_ATTR_RO(available_instances);
> +static umode_t available_instances_is_visible(struct kobject *kobj,
> +					      struct attribute *attr, int n)
> +{
> +	struct mdev_type *type = to_mdev_type(kobj);
> +
> +	if (!type->parent->ops->device_driver->get_available)
> +		return 0;
> +	return attr->mode;
> +}
> +static struct attribute *mdev_types_name_attrs[] = {
> +	&mdev_type_attr_available_instances.attr,
> +	NULL,
> +};
> +static struct attribute_group mdev_type_available_instances_group = {
> +	.attrs = mdev_types_name_attrs,
> +	.is_visible = available_instances_is_visible,
> +};
> +
>   static const struct attribute_group *mdev_type_groups[] = {
>   	&mdev_type_std_group,
> +	&mdev_type_available_instances_group,
>   	NULL,
>   };
>   
> @@ -136,6 +169,10 @@ static struct mdev_type *add_mdev_supported_type(struct mdev_parent *parent,
>   	mdev_get_parent(parent);
>   	type->type_group_id = type_group_id;
>   
> +	if (parent->ops->device_driver->get_available)
> +		type->available =
> +			parent->ops->device_driver->get_available(type);
> +
>   	ret = kobject_init_and_add(&type->kobj, &mdev_type_ktype, NULL,
>   				   "%s-%s", dev_driver_string(parent->dev),
>   				   group->name);
> diff --git a/include/linux/mdev.h b/include/linux/mdev.h
> index 14655215417b..0ce1bb3dabd0 100644
> --- a/include/linux/mdev.h
> +++ b/include/linux/mdev.h
> @@ -120,12 +120,14 @@ struct mdev_type_attribute {
>    * @probe: called when new device created
>    * @remove: called when device removed
>    * @driver: device driver structure
> + * @get_available: Return the max number of instances that can be created
>    *
>    **/
>   struct mdev_driver {
>   	int (*probe)(struct mdev_device *dev);
>   	void (*remove)(struct mdev_device *dev);
>   	struct device_driver driver;
> +	unsigned int (*get_available)(struct mdev_type *mtype);
>   };
>   

Re: [PATCH v1 14/18] vfio/mdev: Add mdev available instance checking to the core

[Eric Farman]

On Tue, 2022-06-07 at 01:32 +0530, Kirti Wankhede wrote:
> 
> On 6/2/2022 10:49 PM, Eric Farman wrote:
> > From: Jason Gunthorpe <jgg@nvidia.com>
> > 
> > Many of the mdev drivers use a simple counter for keeping track of
> > the
> > available instances. Move this code to the core code and store the
> > counter
> > in the mdev_type. Implement it using correct locking, fixing mdpy.
> > 
> > Drivers provide a get_available() callback to set the number of
> > available
> > instances for their mtypes which is fixed at registration time. The
> > core
> > provides a standard sysfs attribute to return the
> > available_instances.
> > 
> > Cc: Kirti Wankhede <kwankhede@nvidia.com>
> > Cc: Jonathan Corbet <corbet@lwn.net>
> > Cc: Tony Krowiak <akrowiak@linux.ibm.com>
> > Cc: Jason Herne <jjherne@linux.ibm.com>
> > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> > Link: 
> > https://lore.kernel.org/r/7-v3-57c1502c62fd+2190-ccw_mdev_jgg@nvidia.com/
> > [farman: added Cc: tags]
> > Signed-off-by: Eric Farman <farman@linux.ibm.com>
> > ---
> >   .../driver-api/vfio-mediated-device.rst       |  4 +-
> >   drivers/s390/cio/vfio_ccw_drv.c               |  1 -
> >   drivers/s390/cio/vfio_ccw_ops.c               | 26 ++++---------
> >   drivers/s390/cio/vfio_ccw_private.h           |  2 -
> >   drivers/s390/crypto/vfio_ap_ops.c             | 32 ++++--------
> > ----
> >   drivers/s390/crypto/vfio_ap_private.h         |  2 -
> >   drivers/vfio/mdev/mdev_core.c                 | 11 +++++-
> >   drivers/vfio/mdev/mdev_private.h              |  2 +
> >   drivers/vfio/mdev/mdev_sysfs.c                | 37
> > +++++++++++++++++++
> >   include/linux/mdev.h                          |  2 +
> >   samples/vfio-mdev/mdpy.c                      | 22 +++--------
> >   11 files changed, 76 insertions(+), 65 deletions(-)
> > 
> > diff --git a/Documentation/driver-api/vfio-mediated-device.rst
> > b/Documentation/driver-api/vfio-mediated-device.rst
> > index f410a1cd98bb..a4f7f1362fa8 100644
> > --- a/Documentation/driver-api/vfio-mediated-device.rst
> > +++ b/Documentation/driver-api/vfio-mediated-device.rst
> > @@ -106,6 +106,7 @@ structure to represent a mediated device's
> > driver::
> >   	     int  (*probe)  (struct mdev_device *dev);
> >   	     void (*remove) (struct mdev_device *dev);
> >   	     struct device_driver    driver;
> > +	     unsigned int (*get_available)(struct mdev_type *mtype);
> >        };
> > 
> 
> This patch conflicts with Christoph Hellwig's patch. I see 
> 'supported_type_groups' is not is above structure, I beleive that
> your 
> patch is applied on top of Christoph's patch series.
> 
> but then in below part of code, 'add_mdev_supported_type' has also
> being 
> removed in Christoph's patch. So this patch would not get applied
> cleanly.

Apologies. This series was fit to 5.18 as the merge window progressed.
Both this patch and the previous one have to adjust to the removal of
mdev_parent_ops that came about from 

commit 6b42f491e17ce13f5ff7f2d1f49c73a0f4c47b20
Author: Jason Gunthorpe <jgg@ziepe.ca>
Date:   Mon Apr 11 16:14:01 2022 +0200

    vfio/mdev: Remove mdev_parent_ops

I have this rebased for v2.

Eric

> 
> Thanks,
> Kirti
> 
> > +/* mdev_type attribute used by drivers that have an
> > get_available() op */
> > +static ssize_t available_instances_show(struct mdev_type *mtype,
> > +					struct mdev_type_attribute
> > *attr,
> > +					char *buf)
> > +{
> > +	unsigned int available;
> > +
> > +	mutex_lock(&mdev_list_lock);
> > +	available = mtype->available;
> > +	mutex_unlock(&mdev_list_lock);
> > +
> > +	return sysfs_emit(buf, "%u\n", available);
> > +}
> > +static MDEV_TYPE_ATTR_RO(available_instances);
> > +static umode_t available_instances_is_visible(struct kobject
> > *kobj,
> > +					      struct attribute *attr,
> > int n)
> > +{
> > +	struct mdev_type *type = to_mdev_type(kobj);
> > +
> > +	if (!type->parent->ops->device_driver->get_available)
> > +		return 0;
> > +	return attr->mode;
> > +}
> > +static struct attribute *mdev_types_name_attrs[] = {
> > +	&mdev_type_attr_available_instances.attr,
> > +	NULL,
> > +};
> > +static struct attribute_group mdev_type_available_instances_group
> > = {
> > +	.attrs = mdev_types_name_attrs,
> > +	.is_visible = available_instances_is_visible,
> > +};
> > +
> >   static const struct attribute_group *mdev_type_groups[] = {
> >   	&mdev_type_std_group,
> > +	&mdev_type_available_instances_group,
> >   	NULL,
> >   };
> >   
> > @@ -136,6 +169,10 @@ static struct mdev_type
> > *add_mdev_supported_type(struct mdev_parent *parent,
> >   	mdev_get_parent(parent);
> >   	type->type_group_id = type_group_id;
> >   
> > +	if (parent->ops->device_driver->get_available)
> > +		type->available =
> > +			parent->ops->device_driver-
> > >get_available(type);
> > +
> >   	ret = kobject_init_and_add(&type->kobj, &mdev_type_ktype, NULL,
> >   				   "%s-%s", dev_driver_string(parent-
> > >dev),
> >   				   group->name);
> > diff --git a/include/linux/mdev.h b/include/linux/mdev.h
> > index 14655215417b..0ce1bb3dabd0 100644
> > --- a/include/linux/mdev.h
> > +++ b/include/linux/mdev.h
> > @@ -120,12 +120,14 @@ struct mdev_type_attribute {
> >    * @probe: called when new device created
> >    * @remove: called when device removed
> >    * @driver: device driver structure
> > + * @get_available: Return the max number of instances that can be
> > created
> >    *
> >    **/
> >   struct mdev_driver {
> >   	int (*probe)(struct mdev_device *dev);
> >   	void (*remove)(struct mdev_device *dev);
> >   	struct device_driver driver;
> > +	unsigned int (*get_available)(struct mdev_type *mtype);
> >   };
> >   

Re: [PATCH v1 14/18] vfio/mdev: Add mdev available instance checking to the core

[Matthew Rosato]

On 6/6/22 4:23 PM, Eric Farman wrote:
> On Tue, 2022-06-07 at 01:32 +0530, Kirti Wankhede wrote:
>>
>> On 6/2/2022 10:49 PM, Eric Farman wrote:
>>> From: Jason Gunthorpe <jgg@nvidia.com>
>>>
>>> Many of the mdev drivers use a simple counter for keeping track of
>>> the
>>> available instances. Move this code to the core code and store the
>>> counter
>>> in the mdev_type. Implement it using correct locking, fixing mdpy.
>>>
>>> Drivers provide a get_available() callback to set the number of
>>> available
>>> instances for their mtypes which is fixed at registration time. The
>>> core
>>> provides a standard sysfs attribute to return the
>>> available_instances.
>>>
>>> Cc: Kirti Wankhede <kwankhede@nvidia.com>
>>> Cc: Jonathan Corbet <corbet@lwn.net>
>>> Cc: Tony Krowiak <akrowiak@linux.ibm.com>
>>> Cc: Jason Herne <jjherne@linux.ibm.com>
>>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>>> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
>>> Link:
>>> https://lore.kernel.org/r/7-v3-57c1502c62fd+2190-ccw_mdev_jgg@nvidia.com/
>>> [farman: added Cc: tags]
>>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
>>> ---
>>>    .../driver-api/vfio-mediated-device.rst       |  4 +-
>>>    drivers/s390/cio/vfio_ccw_drv.c               |  1 -
>>>    drivers/s390/cio/vfio_ccw_ops.c               | 26 ++++---------
>>>    drivers/s390/cio/vfio_ccw_private.h           |  2 -
>>>    drivers/s390/crypto/vfio_ap_ops.c             | 32 ++++--------
>>> ----
>>>    drivers/s390/crypto/vfio_ap_private.h         |  2 -
>>>    drivers/vfio/mdev/mdev_core.c                 | 11 +++++-
>>>    drivers/vfio/mdev/mdev_private.h              |  2 +
>>>    drivers/vfio/mdev/mdev_sysfs.c                | 37
>>> +++++++++++++++++++
>>>    include/linux/mdev.h                          |  2 +
>>>    samples/vfio-mdev/mdpy.c                      | 22 +++--------
>>>    11 files changed, 76 insertions(+), 65 deletions(-)
>>>
>>> diff --git a/Documentation/driver-api/vfio-mediated-device.rst
>>> b/Documentation/driver-api/vfio-mediated-device.rst
>>> index f410a1cd98bb..a4f7f1362fa8 100644
>>> --- a/Documentation/driver-api/vfio-mediated-device.rst
>>> +++ b/Documentation/driver-api/vfio-mediated-device.rst
>>> @@ -106,6 +106,7 @@ structure to represent a mediated device's
>>> driver::
>>>    	     int  (*probe)  (struct mdev_device *dev);
>>>    	     void (*remove) (struct mdev_device *dev);
>>>    	     struct device_driver    driver;
>>> +	     unsigned int (*get_available)(struct mdev_type *mtype);
>>>         };
>>>
>>
>> This patch conflicts with Christoph Hellwig's patch. I see
>> 'supported_type_groups' is not is above structure, I beleive that
>> your
>> patch is applied on top of Christoph's patch series.
>>
>> but then in below part of code, 'add_mdev_supported_type' has also
>> being
>> removed in Christoph's patch. So this patch would not get applied
>> cleanly.
> 
> Apologies. This series was fit to 5.18 as the merge window progressed.
> Both this patch and the previous one have to adjust to the removal of
> mdev_parent_ops that came about from
> 
> commit 6b42f491e17ce13f5ff7f2d1f49c73a0f4c47b20
> Author: Jason Gunthorpe <jgg@ziepe.ca>
> Date:   Mon Apr 11 16:14:01 2022 +0200
> 
>      vfio/mdev: Remove mdev_parent_ops
> 
> I have this rebased for v2.
> 
> Eric

Thanks Eric -- FYI, I'm planning to review the entire series but will 
likely hold off until v2 on a number of patches at this point as there 
are a few other collisions with 5.19

Re: [PATCH v1 02/18] vfio/ccw: Fix FSM state if mdev probe fails

[Matthew Rosato]

On 6/3/22 3:12 PM, Eric Farman wrote:
> On Fri, 2022-06-03 at 09:21 -0400, Matthew Rosato wrote:
>> On 6/2/22 1:19 PM, Eric Farman wrote:
>>> The FSM is in STANDBY state when arriving in vfio_ccw_mdev_probe(),
>>> and this routine converts it to IDLE as part of its processing.
>>> The error exit sets it to IDLE (again) but clears the private->mdev
>>> pointer.
>>>
>>> The FSM should of course be managing the state itself, but the
>>> correct thing for vfio_ccw_mdev_probe() to do would be to put
>>> the state back the way it found it.
>>>
>>> The corresponding check of private->mdev in vfio_ccw_sch_io_todo()
>>> can be removed, since the distinction is unnecessary at this point.
>>>
>>> Fixes: 3bf1311f351ef ("vfio/ccw: Convert to use
>>> vfio_register_emulated_iommu_dev()")
>>> Signed-off-by: Eric Farman <farman@linux.ibm.com>
>>> ---
>>>    drivers/s390/cio/vfio_ccw_drv.c | 2 +-
>>>    drivers/s390/cio/vfio_ccw_ops.c | 2 +-
>>>    2 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/s390/cio/vfio_ccw_drv.c
>>> b/drivers/s390/cio/vfio_ccw_drv.c
>>> index 35055eb94115..b18b4582bc8b 100644
>>> --- a/drivers/s390/cio/vfio_ccw_drv.c
>>> +++ b/drivers/s390/cio/vfio_ccw_drv.c
>>> @@ -108,7 +108,7 @@ static void vfio_ccw_sch_io_todo(struct
>>> work_struct *work)
>>>    	 * has finished. Do not overwrite a possible processing
>>>    	 * state if the final interrupt was for HSCH or CSCH.
>>>    	 */
>>> -	if (private->mdev && cp_is_finished)
>>> +	if (cp_is_finished)
>>>    		private->state = VFIO_CCW_STATE_IDLE;
>>
>> Took me a bit to convince myself this was OK
> 
> Me too. :)
> 
>> , mainly because AFAICT
>> despite the change below the fsm jumptable would still allow you to
>> reach this code when in STANDBY.  But, it should only be possible for
>> an
>> unsolicited interrupt (e.g. unsolicited implies !cp_is_finished) so
>> we
>> would still avoid a STANDBY->IDLE transition on accident.
>>
>> Maybe work unsolicited interrupt into the comment block above along
>> with
>> HSCH/CSCH?
> 
> Good idea. How about:
> 
>          /*
>           * Reset to IDLE only if
> processing of a channel program
>           * has finished. Do not
> overwrite a possible processing
>           * state if the interrupt was
> unsolicited, or if the final
>           * interrupt was for HSCH or CSCH.
>   
>          */
> 

Sounds good to me

Re: [PATCH v1 01/18] vfio/ccw: Remove UUID from s390 debug log

[Matthew Rosato]

On 6/3/22 3:03 PM, Eric Farman wrote:
> On Thu, 2022-06-02 at 15:51 -0400, Matthew Rosato wrote:
>> On 6/2/22 1:19 PM, Eric Farman wrote:
>>> From: Michael Kawano <mkawano@linux.ibm.com>
>>>
>>> As vfio-ccw devices are created/destroyed, the uuid of the
>>> associated
>>> mdevs that are recorded in $S390DBF/vfio_ccw_msg/sprintf get lost
>>> as
>>> they are created using pointers passed by reference.
>>>
>>> This is a deliberate design point of s390dbf, but it leaves the
>>> uuid
>>
>> This wording is confusing, maybe some re-wording would help here.
>>
>> Basically, s390dbf doesn't support values passed by reference today
>> (e.g. %pUl), it will just store that pointer (e.g. &mdev->uuid) and
>> not
>> its contents -- so a subsequent viewing of the s390dbf log at any
>> point
>> in the future will go peek at that referenced memory -- which might
>> have
>> been freed (e.g. mdev was removed).  So this change will fix
>> potential
>> garbage data viewed from the log or worse an oops when viewing the
>> log
>> -- the latter of which should probably be mentioned in the commit
>> message.
>>
>> I'm not sure if it was a deliberate design decision of s390dbf or
>> just a
>> feature that was never implemented, so I'd omit that altogether --
>> but
>> it IS pointed out in the s390dbf documentation as a limitation
>> anyway.
> 
> @Jason, @Matt...  All fair, I obviously got too verbose in whatever I
> was writing at the time. I've changed this to:
> 
> As vfio-ccw devices are created/destroyed, the uuid of the associated
> mdevs that are recorded in $S390DBF/vfio_ccw_msg/sprintf get lost.
> This is because a pointer to the UUID is stored instead of the UUID
> itself, and that memory may have been repurposed if/when the logs are
> examined. The result is usually garbage UUID data in the logs, though
> there is an outside chance of an oops happening here.
> 
> Simply remove the UUID from the traces, as the subchannel number will
> provide useful configuration information for problem determination,
> and is stored directly into the log instead of a pointer.
> 
> As we were the only consumer of mdev_uuid(), remove that too.
> 

Sounds good

Re: [PATCH v1 00/18] VFIO ccw/mdev rework

[Yi Liu]

Hi Eric,

On 2022/6/3 01:19, Eric Farman wrote:
> Last autumn, Jason Gunthorpe proposed some rework of vfio-ccw [1],
> to better fit with the new mdev API (thank you!). Part of that
> series was pulled for kernel 5.16 [2], but the complexities of
> the remaining patches got them hung up behind other work.
> 
> This series attempts to dust off and complete that, with the
> goal of untangling the lifecycle of a s390 subchannel when
> bound to vfio-ccw instead of the usual io_subchannel driver.
> 
> Patches 1-8 are inspired by and/or split out from that series,
> in order to be consumable on their own (backports, etc.).
> 
> Patches 9-12 handle the goal of making the FSM complete,
> and synchronizing the subchannel's life with that of the mdev.
> (This was the goal of patch 5 of the larger series [3].)
> 
> Patches 13-14 are pulled directly from the earlier series.
> As these patches hit some other of the consumers of vfio,
> those on CC who are unfamiliar with vfio-ccw probably only
> care about these. :)
> 
> Patches 15-18 links the lifecycle of the vfio_ccw_private struct
> with the mdev via a vfio reference. (Patch 17 was also pulled
> directly from the earlier series.)
> 
> In the end, the subchannel probe/remove callbacks from the css
> driver simply register/unregister with vfio-mdev. The communication
> with the subchannel is delayed until the mdev routines, which
> handles all the vfio-related memory and subchannel enablement.
> There's no longer a configuration where the mdev is closed while
> the subchannel remains enabled, since that's weird.
> 
> @Jason: I carried the S-o-b/r-b tags on patches 13, 14, and 17,
> as they were cherry-picked straight from your v3.
> If you'd prefer your S-o-b on others, please let me know.

very nice to see it. do you have a 5.19 based branch including these
changes on github? I'd like to rebase my vfio cdev patches on top of
your changes. :-)

> [1] https://lore.kernel.org/r/0-v3-57c1502c62fd+2190-ccw_mdev_jgg@nvidia.com/
> [2] https://lore.kernel.org/r/0-v4-cea4f5bd2c00+b52-ccw_mdev_jgg@nvidia.com/
> [3] https://lore.kernel.org/r/5-v3-57c1502c62fd+2190-ccw_mdev_jgg@nvidia.com/
> 
> Cc: Kirti Wankhede <kwankhede@nvidia.com>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
> Cc: Zhi Wang <zhi.a.wang@intel.com>
> Cc: intel-gvt-dev@lists.freedesktop.org
> Cc: Tony Krowiak <akrowiak@linux.ibm.com>
> Cc: Jason Herne <jjherne@linux.ibm.com>
> 
> Eric Farman (14):
>    vfio/ccw: Fix FSM state if mdev probe fails
>    vfio/ccw: Ensure mdev->dev is cleared on mdev remove
>    vfio/ccw: Do not change FSM state in subchannel event
>    vfio/ccw: Remove private->mdev
>    vfio/ccw: Pass enum to FSM event jumptable
>    vfio/ccw: Flatten MDEV device (un)register
>    vfio/ccw: Check that private pointer is not NULL
>    vfio/ccw: Create an OPEN FSM Event
>    vfio/ccw: Create a CLOSE FSM event
>    vfio/ccw: Refactor vfio_ccw_mdev_reset
>    vfio/ccw: Move FSM open/close to MDEV open/close
>    vfio/ccw: Manage private with mdev
>    vfio/ccw: Create a get_private routine
>    vfio/ccw: Manage ccw/mdev reference counts
> 
> Jason Gunthorpe (3):
>    vfio/mdev: Consolidate all the device_api sysfs into the core code
>    vfio/mdev: Add mdev available instance checking to the core
>    vfio: Export vfio_device_try_get()
> 
> Michael Kawano (1):
>    vfio/ccw: Remove UUID from s390 debug log
> 
>   .../driver-api/vfio-mediated-device.rst       |   8 +-
>   drivers/gpu/drm/i915/gvt/kvmgt.c              |   9 +-
>   drivers/s390/cio/vfio_ccw_async.c             |   1 -
>   drivers/s390/cio/vfio_ccw_drv.c               | 114 ++++++--------
>   drivers/s390/cio/vfio_ccw_fsm.c               |  91 +++++++++--
>   drivers/s390/cio/vfio_ccw_ops.c               | 145 ++++++------------
>   drivers/s390/cio/vfio_ccw_private.h           |  33 +++-
>   drivers/s390/crypto/vfio_ap_ops.c             |  41 ++---
>   drivers/s390/crypto/vfio_ap_private.h         |   2 -
>   drivers/vfio/mdev/mdev_core.c                 |  13 +-
>   drivers/vfio/mdev/mdev_private.h              |   2 +
>   drivers/vfio/mdev/mdev_sysfs.c                |  64 +++++++-
>   drivers/vfio/vfio.c                           |   3 +-
>   include/linux/mdev.h                          |  13 +-
>   include/linux/vfio.h                          |   1 +
>   samples/vfio-mdev/mbochs.c                    |   9 +-
>   samples/vfio-mdev/mdpy.c                      |  31 +---
>   samples/vfio-mdev/mtty.c                      |  10 +-
>   18 files changed, 300 insertions(+), 290 deletions(-)
> 

-- 
Regards,
Yi Liu

RE: [PATCH v1 13/18] vfio/mdev: Consolidate all the device_api sysfs into the core code

[Tian, Kevin]

> From: Eric Farman
> Sent: Friday, June 3, 2022 1:20 AM
> 
> From: Jason Gunthorpe <jgg@nvidia.com>
> 
> Every driver just emits a static string, simply feed it through the ops
> and provide a standard sysfs show function.
> 
> Cc: Kirti Wankhede <kwankhede@nvidia.com>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Zhenyu Wang <zhenyuw@linux.intel.com>
> Cc: Zhi Wang <zhi.a.wang@intel.com>
> Cc: Tony Krowiak <akrowiak@linux.ibm.com>
> Cc: Jason Herne <jjherne@linux.ibm.com>
> Cc: intel-gvt-dev@lists.freedesktop.org
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> Link: https://lore.kernel.org/r/6-v3-57c1502c62fd+2190-
> ccw_mdev_jgg@nvidia.com/
> [farman: added Cc: tags]
> Signed-off-by: Eric Farman <farman@linux.ibm.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>, with one nit:

> @@ -225,7 +226,8 @@ Directories and files under the sysfs for Each Physical
> Device
>  * device_api
> 
>    This attribute should show which device API is being created, for example,
> -  "vfio-pci" for a PCI device.
> +  "vfio-pci" for a PCI device. The core code maintins this sysfs using the
> +  device_api member of mdev_parent_ops.
> 

s/maintins/maintains/

RE: [PATCH v1 14/18] vfio/mdev: Add mdev available instance checking to the core

[Tian, Kevin]

> From: Eric Farman <farman@linux.ibm.com>
> Sent: Tuesday, June 7, 2022 4:24 AM
> 
> On Tue, 2022-06-07 at 01:32 +0530, Kirti Wankhede wrote:
> >
> > On 6/2/2022 10:49 PM, Eric Farman wrote:
> > > From: Jason Gunthorpe <jgg@nvidia.com>
> > > @@ -106,6 +106,7 @@ structure to represent a mediated device's
> > > driver::
> > >   	     int  (*probe)  (struct mdev_device *dev);
> > >   	     void (*remove) (struct mdev_device *dev);
> > >   	     struct device_driver    driver;
> > > +	     unsigned int (*get_available)(struct mdev_type *mtype);
> > >        };
> > >
> >
> > This patch conflicts with Christoph Hellwig's patch. I see
> > 'supported_type_groups' is not is above structure, I beleive that
> > your
> > patch is applied on top of Christoph's patch series.
> >
> > but then in below part of code, 'add_mdev_supported_type' has also
> > being
> > removed in Christoph's patch. So this patch would not get applied
> > cleanly.
> 
> Apologies. This series was fit to 5.18 as the merge window progressed.
> Both this patch and the previous one have to adjust to the removal of
> mdev_parent_ops that came about from
> 
> commit 6b42f491e17ce13f5ff7f2d1f49c73a0f4c47b20
> Author: Jason Gunthorpe <jgg@ziepe.ca>
> Date:   Mon Apr 11 16:14:01 2022 +0200
> 
>     vfio/mdev: Remove mdev_parent_ops
> 
> I have this rebased for v2.
> 

btw with those latest changes [1] we don't need .get_available() then,
as mdev type is now added by mdev driver one-by-one then the
available instance can be provided directly in that path.

[1] https://lore.kernel.org/all/20220607055653.GA8848@lst.de/T/#m82caa423d728bce0729b8c9e8d8aff76250ec9ac

Re: [PATCH v1 14/18] vfio/mdev: Add mdev available instance checking to the core

[Christoph Hellwig]

On Fri, Jun 10, 2022 at 07:43:46AM +0000, Tian, Kevin wrote:
> btw with those latest changes [1] we don't need .get_available() then,
> as mdev type is now added by mdev driver one-by-one then the
> available instance can be provided directly in that path.

Yes, we can probably add a helper to add the vailable attibrute, which
takes the number of instances.  Is it ok if I just add a version of this
patch and the device_api one to my series, and we rebase this series
on top of it?  I'll try to get out a new version ASAP.

Re: [PATCH v1 14/18] vfio/mdev: Add mdev available instance checking to the core

[Eric Farman]

On Mon, 2022-06-13 at 08:46 +0200, Christoph Hellwig wrote:
> On Fri, Jun 10, 2022 at 07:43:46AM +0000, Tian, Kevin wrote:
> > btw with those latest changes [1] we don't need .get_available()
> > then,
> > as mdev type is now added by mdev driver one-by-one then the
> > available instance can be provided directly in that path.
> 
> Yes, we can probably add a helper to add the vailable attibrute,
> which
> takes the number of instances.  Is it ok if I just add a version of
> this
> patch and the device_api one to my series, and we rebase this series
> on top of it?  I'll try to get out a new version ASAP.

That's fine with me. Thanks!

Eric