[PATCH] s390/vfio-ap: Replace one-element array with flexible array member

[PATCH] s390/vfio-ap: Replace one-element array with flexible array member

[Thorsten Blum]

Replace the deprecated one-element array with a modern flexible array
member in the struct ap_matrix_dev.

Use struct_size() to calculate the number of bytes to allocate for
matrix_dev with a single mdev_type.

Link: https://github.com/KSPP/linux/issues/79
Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
---
 drivers/s390/crypto/vfio_ap_drv.c     | 2 +-
 drivers/s390/crypto/vfio_ap_private.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
index 67a807e2e75b..ea9ffa37f263 100644
--- a/drivers/s390/crypto/vfio_ap_drv.c
+++ b/drivers/s390/crypto/vfio_ap_drv.c
@@ -96,7 +96,7 @@ static int vfio_ap_matrix_dev_create(void)
 	if (ret)
 		goto bus_register_err;
 
-	matrix_dev = kzalloc(sizeof(*matrix_dev), GFP_KERNEL);
+	matrix_dev = kzalloc(struct_size(matrix_dev, mdev_types, 1), GFP_KERNEL);
 	if (!matrix_dev) {
 		ret = -ENOMEM;
 		goto matrix_alloc_err;
diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
index 437a161c8659..9aed8994f567 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.48.0

Re: [PATCH] s390/vfio-ap: Replace one-element array with flexible array member

[Alexander Gordeev]

On Thu, Jan 16, 2025 at 02:18:59PM +0100, Thorsten Blum wrote:
> Replace the deprecated one-element array with a modern flexible array
> member in the struct ap_matrix_dev.
> 
> Use struct_size() to calculate the number of bytes to allocate for
> matrix_dev with a single mdev_type.
> 
> Link: https://github.com/KSPP/linux/issues/79
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> ---
>  drivers/s390/crypto/vfio_ap_drv.c     | 2 +-
>  drivers/s390/crypto/vfio_ap_private.h | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)

Hi Tony, Halil, Jason,

Could you please take a look?

Thanks!

Re: [PATCH] s390/vfio-ap: Replace one-element array with flexible array member

[Anthony Krowiak]

On 1/16/25 8:18 AM, Thorsten Blum wrote:
> Replace the deprecated one-element array with a modern flexible array
> member in the struct ap_matrix_dev.
>
> Use struct_size() to calculate the number of bytes to allocate for
> matrix_dev with a single mdev_type.
>
> Link: https://github.com/KSPP/linux/issues/79

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

> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> ---
>   drivers/s390/crypto/vfio_ap_drv.c     | 2 +-
>   drivers/s390/crypto/vfio_ap_private.h | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
> index 67a807e2e75b..ea9ffa37f263 100644
> --- a/drivers/s390/crypto/vfio_ap_drv.c
> +++ b/drivers/s390/crypto/vfio_ap_drv.c
> @@ -96,7 +96,7 @@ static int vfio_ap_matrix_dev_create(void)
>   	if (ret)
>   		goto bus_register_err;
>   
> -	matrix_dev = kzalloc(sizeof(*matrix_dev), GFP_KERNEL);
> +	matrix_dev = kzalloc(struct_size(matrix_dev, mdev_types, 1), GFP_KERNEL);
>   	if (!matrix_dev) {
>   		ret = -ENOMEM;
>   		goto matrix_alloc_err;
> diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
> index 437a161c8659..9aed8994f567 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;

Re: [PATCH] s390/vfio-ap: Replace one-element array with flexible array member

[Alexander Gordeev]

On Thu, Jan 16, 2025 at 02:18:59PM +0100, Thorsten Blum wrote:
> Replace the deprecated one-element array with a modern flexible array
> member in the struct ap_matrix_dev.
> 
> Use struct_size() to calculate the number of bytes to allocate for
> matrix_dev with a single mdev_type.
> 
> Link: https://github.com/KSPP/linux/issues/79
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> ---
>  drivers/s390/crypto/vfio_ap_drv.c     | 2 +-
>  drivers/s390/crypto/vfio_ap_private.h | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/s390/crypto/vfio_ap_drv.c b/drivers/s390/crypto/vfio_ap_drv.c
> index 67a807e2e75b..ea9ffa37f263 100644
> --- a/drivers/s390/crypto/vfio_ap_drv.c
> +++ b/drivers/s390/crypto/vfio_ap_drv.c
> @@ -96,7 +96,7 @@ static int vfio_ap_matrix_dev_create(void)
>  	if (ret)
>  		goto bus_register_err;
>  
> -	matrix_dev = kzalloc(sizeof(*matrix_dev), GFP_KERNEL);
> +	matrix_dev = kzalloc(struct_size(matrix_dev, mdev_types, 1), GFP_KERNEL);
>  	if (!matrix_dev) {
>  		ret = -ENOMEM;
>  		goto matrix_alloc_err;
> diff --git a/drivers/s390/crypto/vfio_ap_private.h b/drivers/s390/crypto/vfio_ap_private.h
> index 437a161c8659..9aed8994f567 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;

Applied, thanks!

Re: [PATCH] s390/vfio-ap: Replace one-element array with flexible array member

[Halil Pasic]

On Thu, 16 Jan 2025 14:18:59 +0100
Thorsten Blum <thorsten.blum@linux.dev> wrote:

> Replace the deprecated one-element array with a modern flexible array
> member in the struct ap_matrix_dev.
> 

I'm not sure I understand the value of this. What we have here is not
a flexible array but a one element array. Something that in the generic
case could be many but particularly for vfio-ap is always one.

Imagine if we had exactly 2 supported mdev types. I guess you would not
come to the idea that the array of two needs to be changed to a modern
flexible array. Or am I wrong about that?

So I suppose the problem  here is that arrays of one are under general
suspicion of not actually being arrays of one but a pre C99 way of
doing flexible arrays. I kind of do understand that an array wih
one element is funny. But I think the current declaration of
struct ap_matrix_dev is more expressive the proposed one.

Now I understand that it is easy to grep for [1], but it is much
harder to tell.

I've checked in Documentation/process/coding-style.rst did not see this
documented. But then I checked in checkpatch.pl and it indeed seems to
warn about it and refer to
https://docs.kernel.org/process/deprecated.html#zero-length-and-one-element-arrays

But as far as I can tell it is also talking about the case where one
needs a trailing array of an at compile time unknown size, and not
about the case where all we need is just one element and it happens
to be convenient to have it in an array.

If there is community consensus that one sized arrays are bad regardless
of what they are used for, then the one sized array has to go.

But please tell me what speaks against replacing it with a single
pointer and then taking passing in that pointers address into
mdev_register_parent()?


> Use struct_size() to calculate the number of bytes to allocate for
> matrix_dev with a single mdev_type.
> 
> Link: https://github.com/KSPP/linux/issues/79
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>

Re: [PATCH] s390/vfio-ap: Replace one-element array with flexible array member

[Gustavo A. R. Silva]

On 30/01/25 07:08, Halil Pasic wrote:
> On Thu, 16 Jan 2025 14:18:59 +0100
> Thorsten Blum <thorsten.blum@linux.dev> wrote:
> 
>> Replace the deprecated one-element array with a modern flexible array
>> member in the struct ap_matrix_dev.
>>
> 
> I'm not sure I understand the value of this. What we have here is not
> a flexible array but a one element array. Something that in the generic
> case could be many but particularly for vfio-ap is always one.

You are correct. Only fake flexible arrays should be transformed into
C99 flex-array members [1].

Thanks
-Gustavo

[1] https://lwn.net/Articles/908817/

Re: [PATCH] s390/vfio-ap: Replace one-element array with flexible array member

[Halil Pasic]

On Thu, 30 Jan 2025 10:30:30 +1030
"Gustavo A. R. Silva" <gustavo@embeddedor.com> wrote:

> On 30/01/25 07:08, Halil Pasic wrote:
> > On Thu, 16 Jan 2025 14:18:59 +0100
> > Thorsten Blum <thorsten.blum@linux.dev> wrote:
> >   
> >> Replace the deprecated one-element array with a modern flexible array
> >> member in the struct ap_matrix_dev.
> >>  
> > 
> > I'm not sure I understand the value of this. What we have here is not
> > a flexible array but a one element array. Something that in the generic
> > case could be many but particularly for vfio-ap is always one.  
> 
> You are correct. Only fake flexible arrays should be transformed into
> C99 flex-array members [1].
> 
> Thanks
> -Gustavo
> 
> [1] https://lwn.net/Articles/908817/
> 

Thanks! Alex, what do we do with this then? I think you picked it up
yesterday late. And I think, it might make sense to make this look
less like a fake flex-array...

Re: [PATCH] s390/vfio-ap: Replace one-element array with flexible array member

[Alexander Gordeev]

On Thu, Jan 30, 2025 at 11:46:15AM +0100, Halil Pasic wrote:
> > > I'm not sure I understand the value of this. What we have here is not
> > > a flexible array but a one element array. Something that in the generic
> > > case could be many but particularly for vfio-ap is always one.  
> > 
> > You are correct. Only fake flexible arrays should be transformed into
> > C99 flex-array members [1].
> > 
> > Thanks
> > -Gustavo
> > 
> > [1] https://lwn.net/Articles/908817/
> > 
> 
> Thanks! Alex, what do we do with this then? I think you picked it up
> yesterday late. And I think, it might make sense to make this look
> less like a fake flex-array...

Dropped.
Thanks for looking into it!

Re: [PATCH] s390/vfio-ap: Replace one-element array with flexible array member

[Heiko Carstens]

On Thu, Jan 30, 2025 at 12:37:13PM +0100, Alexander Gordeev wrote:
> On Thu, Jan 30, 2025 at 11:46:15AM +0100, Halil Pasic wrote:
> > > > I'm not sure I understand the value of this. What we have here is not
> > > > a flexible array but a one element array. Something that in the generic
> > > > case could be many but particularly for vfio-ap is always one.  
> > > 
> > > You are correct. Only fake flexible arrays should be transformed into
> > > C99 flex-array members [1].
> > > 
> > > Thanks
> > > -Gustavo
> > > 
> > > [1] https://lwn.net/Articles/908817/
> > > 
> > 
> > Thanks! Alex, what do we do with this then? I think you picked it up
> > yesterday late. And I think, it might make sense to make this look
> > less like a fake flex-array...
> 
> Dropped.
> Thanks for looking into it!

Given that we already have 5dd4241964c8 ("vfio/ccw: replace one-element
array with flexible-array member") applied, we now end up with inconsistent
code. I'd prefer if we address _both_ code locations in a way that the code
looks similar, and people won't send similar patches again and again.

Halil, since you started this discussion, can you address this please?

Re: [PATCH] s390/vfio-ap: Replace one-element array with flexible array member

[Halil Pasic]

On Thu, 30 Jan 2025 13:46:47 +0100
Heiko Carstens <hca@linux.ibm.com> wrote:

> > > Thanks! Alex, what do we do with this then? I think you picked it up
> > > yesterday late. And I think, it might make sense to make this look
> > > less like a fake flex-array...  
> > 
> > Dropped.
> > Thanks for looking into it!  
> 
> Given that we already have 5dd4241964c8 ("vfio/ccw: replace one-element
> array with flexible-array member") applied, we now end up with inconsistent
> code. I'd prefer if we address _both_ code locations in a way that the code
> looks similar, and people won't send similar patches again and again.
> 

Yes, I agree. We should make this not look like a fake flex array.

> Halil, since you started this discussion, can you address this please?

Sure! But I won't get around before late in the evening.

Regards,
Halil