[PATCH 0/2] s390/vfio-*: make mdev_types unlike a fake flex array

[PATCH 0/2] s390/vfio-*: make mdev_types unlike a fake flex array

[Halil Pasic]

One sized trailing array members can look like fake flex arrays and
confuse people. Let us try to make the mdev_types member of the parent
devices in vfio-ap and vfio-ccw less confusing.


Halil Pasic (2):
  s390/vfio-ap: make mdev_types not look like a fake flex array
  s390/vfio-ccw: make mdev_types not look like a fake flex array

 drivers/s390/cio/vfio_ccw_drv.c       | 6 +++---
 drivers/s390/cio/vfio_ccw_private.h   | 2 +-
 drivers/s390/crypto/vfio_ap_ops.c     | 4 ++--
 drivers/s390/crypto/vfio_ap_private.h | 2 +-
 4 files changed, 7 insertions(+), 7 deletions(-)


base-commit: 0ad2507d5d93f39619fc42372c347d6006b64319
-- 
2.45.2

[PATCH 1/2] s390/vfio-ap: make mdev_types not look like a fake flex array

[Halil Pasic]

The vfio-ap driver and the vfio parent device provided by it
(matrix_dev) support just a single mdev_type, and this is not likely to
change any time soon.  Despite that matrix_dev->mdev_types started out
as a C99 flexible array presumably as a typo, and since the typo messed
up the allocation, commit e2c8cee9f489 ("s390/vfio-ap: Fix memory
allocation for mdev_types array") changed it to an array of size 1. And
to make things worse mdev_types happens to be the last member of struct
ap_matrix_dev.

Now the problem with that is that before C99 the usual way to get
something similar to a flexible array member was to use a trailing array of
size 0 or 1. This is what I called fake flex array. For a while now the
community is trying to get rid of fake flex arrays. And while mdev_types
is not a fake flex array but an array of size one (to match the mdev
interfaces nicer), it can easily be and was mistaken for a fake flex
array.

So, let us make mdev_types a pointer to struct mdev_type and pass in the
address of that pointer as the 4th formal parameter of
mdev_register_parent().

Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
Reviewed-by: Anthony Krowiak <akrowiak@linux.ibm.com>
Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
Reviewed-by: Eric Farman <farman@linux.ibm.com>
Tested-by: Anthony Krowiak <akrowiak@linux.ibm.com>

---

I've also considered switching up the order in which the members
mdev_types and mdev_type are defined in  struct ap_matrix_dev but
decided against that because that could look to somebody like
well known mistake that can be made when using fake flex arrays.
---
 drivers/s390/crypto/vfio_ap_ops.c     | 4 ++--
 drivers/s390/crypto/vfio_ap_private.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
index a52c2690933f..5212b3863aff 100644
--- a/drivers/s390/crypto/vfio_ap_ops.c
+++ b/drivers/s390/crypto/vfio_ap_ops.c
@@ -2316,10 +2316,10 @@ int vfio_ap_mdev_register(void)
 
 	matrix_dev->mdev_type.sysfs_name = VFIO_AP_MDEV_TYPE_HWVIRT;
 	matrix_dev->mdev_type.pretty_name = VFIO_AP_MDEV_NAME_HWVIRT;
-	matrix_dev->mdev_types[0] = &matrix_dev->mdev_type;
+	matrix_dev->mdev_types = &matrix_dev->mdev_type;
 	ret = mdev_register_parent(&matrix_dev->parent, &matrix_dev->device,
 				   &vfio_ap_matrix_driver,
-				   matrix_dev->mdev_types, 1);
+				   &matrix_dev->mdev_types, 1);
 	if (ret)
 		goto err_driver;
 	return 0;
diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
index 437a161c8659..9d16321777c8 100644
--- a/drivers/s390/crypto/vfio_ap_private.h
+++ b/drivers/s390/crypto/vfio_ap_private.h
@@ -53,7 +53,7 @@ struct ap_matrix_dev {
 	struct mutex guests_lock; /* serializes access to each KVM guest */
 	struct mdev_parent parent;
 	struct mdev_type mdev_type;
-	struct mdev_type *mdev_types[1];
+	struct mdev_type *mdev_types;
 };
 
 extern struct ap_matrix_dev *matrix_dev;
-- 
2.45.2

[PATCH 2/2] s390/vfio-ccw: make mdev_types not look like a fake flex array

[Halil Pasic]

The vfio-ccw driver and the vfio parent device provided by it (parent)
support just a single mdev_type, and this is not likely to change any
time soon. To match the mdev interfaces nicely initially the choice was
made that mdev_types (which gets passed into mdev_register_parent())
shall be an array of pointers to struct mdev_type with a single element,
and to make things worse it ended up being the last member.

Now the problem with that is that before C99 the usual way to get
something similar to a flexible array member was to use a trailing array
of size 0 or 1. This is what I called fake flex array. For a while now
the community is trying to get rid of fake flex arrays. And while
mdev_types was not a fake flex array but an array of size one, because
it can easily be and probably was mistaken for a fake flex array it got
converted into a real C99 flex array with a compile time known constant
size of one.

As per [1] it was established that "only fake flexible arrays should be
transformed into C99 flex-array members". Since IMHO the entire point of
flex arrays is being flexible about the array size at run time, a C99
flex array is a poor fit for mdev_types.  But an array of a size one is
a poor fit as well for the reason stated above, let us try to get rid of
the flex array without introducing back the one sized array.

So, lets make mdev_types a pointer to struct mdev_type and pass in the
address of that pointer as the 4th formal parameter of
mdev_register_parent().

[1] https://lore.kernel.org/lkml/85863d7a-2d8b-4c1b-b76a-e2f40834a7a8@embeddedor.com/

Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
Reviewed-by: Matthew Rosato <mjrosato@linux.ibm.com>
Tested-by: Eric Farman <farman@linux.ibm.com>

---

I've also considered switching up the order in which the members
mdev_types and mdev_type are defined in struct vfio_ccw_parent but
decided against that because that could look to somebody like
well known mistake that can be made when using fake flex arrays.
---
 drivers/s390/cio/vfio_ccw_drv.c     | 6 +++---
 drivers/s390/cio/vfio_ccw_private.h | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
index 914dde041675..6ff5c9cfb7ed 100644
--- a/drivers/s390/cio/vfio_ccw_drv.c
+++ b/drivers/s390/cio/vfio_ccw_drv.c
@@ -171,7 +171,7 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
 		return -ENODEV;
 	}
 
-	parent = kzalloc(struct_size(parent, mdev_types, 1), GFP_KERNEL);
+	parent = kzalloc(sizeof(*parent), GFP_KERNEL);
 	if (!parent)
 		return -ENOMEM;
 
@@ -186,10 +186,10 @@ static int vfio_ccw_sch_probe(struct subchannel *sch)
 
 	parent->mdev_type.sysfs_name = "io";
 	parent->mdev_type.pretty_name = "I/O subchannel (Non-QDIO)";
-	parent->mdev_types[0] = &parent->mdev_type;
+	parent->mdev_types = &parent->mdev_type;
 	ret = mdev_register_parent(&parent->parent, &sch->dev,
 				   &vfio_ccw_mdev_driver,
-				   parent->mdev_types, 1);
+				   &parent->mdev_types, 1);
 	if (ret)
 		goto out_unreg;
 
diff --git a/drivers/s390/cio/vfio_ccw_private.h b/drivers/s390/cio/vfio_ccw_private.h
index b62bbc5c6376..0501d4bbcdbd 100644
--- a/drivers/s390/cio/vfio_ccw_private.h
+++ b/drivers/s390/cio/vfio_ccw_private.h
@@ -79,7 +79,7 @@ struct vfio_ccw_parent {
 
 	struct mdev_parent	parent;
 	struct mdev_type	mdev_type;
-	struct mdev_type	*mdev_types[];
+	struct mdev_type	*mdev_types;
 };
 
 /**
-- 
2.45.2

Re: [PATCH 0/2] s390/vfio-*: make mdev_types unlike a fake flex array

[Vasily Gorbik]

On Mon, Feb 17, 2025 at 11:06:12AM +0100, Halil Pasic wrote:
> 
> One sized trailing array members can look like fake flex arrays and
> confuse people. Let us try to make the mdev_types member of the parent
> devices in vfio-ap and vfio-ccw less confusing.
> 
> 
> Halil Pasic (2):
>   s390/vfio-ap: make mdev_types not look like a fake flex array
>   s390/vfio-ccw: make mdev_types not look like a fake flex array
> 
>  drivers/s390/cio/vfio_ccw_drv.c       | 6 +++---
>  drivers/s390/cio/vfio_ccw_private.h   | 2 +-
>  drivers/s390/crypto/vfio_ap_ops.c     | 4 ++--
>  drivers/s390/crypto/vfio_ap_private.h | 2 +-
>  4 files changed, 7 insertions(+), 7 deletions(-)

Applied, thank you!