xen/include/public/io/cameraif.h | 2 +- xen/include/public/io/displif.h | 2 +- xen/include/public/io/fsif.h | 4 ++-- xen/include/public/io/pvcalls.h | 2 +- xen/include/public/io/ring.h | 5 +++-- xen/include/public/io/sndif.h | 2 +- xen/include/public/xen-compat.h | 2 +- xen/include/public/xen.h | 6 ++++++ 8 files changed, 16 insertions(+), 9 deletions(-)
Flexible arrays in public headers can be problematic with some
compilers.
With XEN_FLEX_ARRAY_DIM there is a mechanism available to deal with
this issue, but care must be taken to not change the affected structs
in an incompatible way.
So bump __XEN_LATEST_INTERFACE_VERSION__ and introduce a new macro
XENPV_FLEX_ARRAY_DIM which will be XENPV_FLEX_ARRAY_DIM with the
interface version being new enough and "1" (the value used today in
the affected headers) when the interface version is an old one.
Replace the arr[1] instances (this includes the ones seen to be
problematic in recent Linux kernels [1]) with arr[XENPV_FLEX_ARRAY_DIM]
in order to avoid compilation errors.
[1]: https://bugzilla.kernel.org/show_bug.cgi?id=217693
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- bump interface version and make change only for new version
(Jan Beulich)
---
xen/include/public/io/cameraif.h | 2 +-
xen/include/public/io/displif.h | 2 +-
xen/include/public/io/fsif.h | 4 ++--
xen/include/public/io/pvcalls.h | 2 +-
xen/include/public/io/ring.h | 5 +++--
xen/include/public/io/sndif.h | 2 +-
xen/include/public/xen-compat.h | 2 +-
xen/include/public/xen.h | 6 ++++++
8 files changed, 16 insertions(+), 9 deletions(-)
diff --git a/xen/include/public/io/cameraif.h b/xen/include/public/io/cameraif.h
index 13763abef9..a389443769 100644
--- a/xen/include/public/io/cameraif.h
+++ b/xen/include/public/io/cameraif.h
@@ -763,7 +763,7 @@ struct xencamera_buf_create_req {
*/
struct xencamera_page_directory {
grant_ref_t gref_dir_next_page;
- grant_ref_t gref[1]; /* Variable length */
+ grant_ref_t gref[XENPV_FLEX_ARRAY_DIM];
};
/*
diff --git a/xen/include/public/io/displif.h b/xen/include/public/io/displif.h
index 73d0cbdf15..132c96fa5c 100644
--- a/xen/include/public/io/displif.h
+++ b/xen/include/public/io/displif.h
@@ -537,7 +537,7 @@ struct xendispl_dbuf_create_req {
struct xendispl_page_directory {
grant_ref_t gref_dir_next_page;
- grant_ref_t gref[1]; /* Variable length */
+ grant_ref_t gref[XENPV_FLEX_ARRAY_DIM];
};
/*
diff --git a/xen/include/public/io/fsif.h b/xen/include/public/io/fsif.h
index ec57850233..dcade1c698 100644
--- a/xen/include/public/io/fsif.h
+++ b/xen/include/public/io/fsif.h
@@ -40,7 +40,7 @@ struct fsif_read_request {
int32_t pad;
uint64_t len;
uint64_t offset;
- grant_ref_t grefs[1]; /* Variable length */
+ grant_ref_t grefs[XENPV_FLEX_ARRAY_DIM];
};
struct fsif_write_request {
@@ -48,7 +48,7 @@ struct fsif_write_request {
int32_t pad;
uint64_t len;
uint64_t offset;
- grant_ref_t grefs[1]; /* Variable length */
+ grant_ref_t grefs[XENPV_FLEX_ARRAY_DIM];
};
struct fsif_stat_request {
diff --git a/xen/include/public/io/pvcalls.h b/xen/include/public/io/pvcalls.h
index 230b0719e3..af0e9abd13 100644
--- a/xen/include/public/io/pvcalls.h
+++ b/xen/include/public/io/pvcalls.h
@@ -30,7 +30,7 @@ struct pvcalls_data_intf {
uint8_t pad2[52];
RING_IDX ring_order;
- grant_ref_t ref[];
+ grant_ref_t ref[XENPV_FLEX_ARRAY_DIM];
};
DEFINE_XEN_FLEX_RING(pvcalls);
diff --git a/xen/include/public/io/ring.h b/xen/include/public/io/ring.h
index 0cae4367be..955fe5a1fc 100644
--- a/xen/include/public/io/ring.h
+++ b/xen/include/public/io/ring.h
@@ -25,6 +25,7 @@
* and grant_table.h from the Xen public headers.
*/
+#include "../xen.h"
#include "../xen-compat.h"
#if __XEN_INTERFACE_VERSION__ < 0x00030208
@@ -110,7 +111,7 @@ struct __name##_sring { \
uint8_t pvt_pad[4]; \
} pvt; \
uint8_t __pad[44]; \
- union __name##_sring_entry ring[1]; /* variable-length */ \
+ union __name##_sring_entry ring[XENPV_FLEX_ARRAY_DIM]; \
}; \
\
/* "Front" end's private variables */ \
@@ -479,7 +480,7 @@ struct name##_data_intf { \
uint8_t pad2[56]; \
\
RING_IDX ring_order; \
- grant_ref_t ref[]; \
+ grant_ref_t ref[XENPV_FLEX_ARRAY_DIM]; \
}; \
DEFINE_XEN_FLEX_RING(name)
diff --git a/xen/include/public/io/sndif.h b/xen/include/public/io/sndif.h
index 4234a47c87..cce1459b7b 100644
--- a/xen/include/public/io/sndif.h
+++ b/xen/include/public/io/sndif.h
@@ -659,7 +659,7 @@ struct xensnd_open_req {
struct xensnd_page_directory {
grant_ref_t gref_dir_next_page;
- grant_ref_t gref[1]; /* Variable length */
+ grant_ref_t gref[XENPV_FLEX_ARRAY_DIM];
};
/*
diff --git a/xen/include/public/xen-compat.h b/xen/include/public/xen-compat.h
index 97fe698498..078b796664 100644
--- a/xen/include/public/xen-compat.h
+++ b/xen/include/public/xen-compat.h
@@ -10,7 +10,7 @@
#ifndef __XEN_PUBLIC_XEN_COMPAT_H__
#define __XEN_PUBLIC_XEN_COMPAT_H__
-#define __XEN_LATEST_INTERFACE_VERSION__ 0x00040e00
+#define __XEN_LATEST_INTERFACE_VERSION__ 0x00041300
#if defined(__XEN__) || defined(__XEN_TOOLS__)
/* Xen is built with matching headers and implements the latest interface. */
diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
index b812a0a324..62c4b04fa2 100644
--- a/xen/include/public/xen.h
+++ b/xen/include/public/xen.h
@@ -46,6 +46,12 @@ DEFINE_XEN_GUEST_HANDLE(xen_ulong_t);
#else
#define XEN_FLEX_ARRAY_DIM 1 /* variable size */
#endif
+/* Some PV I/O interfaces need a compatibility variant. */
+#if __XEN_INTERFACE_VERSION__ < 0x00041300
+#define XENPV_FLEX_ARRAY_DIM 1 /* variable size */
+#else
+#define XENPV_FLEX_ARRAY_DIM XEN_FLEX_ARRAY_DIM
+#endif
/* Turn a plain number into a C unsigned (long (long)) constant. */
#define __xen_mk_uint(x) x ## U
--
2.35.3On 30.11.2023 10:21, Juergen Gross wrote: > --- a/xen/include/public/xen.h > +++ b/xen/include/public/xen.h > @@ -46,6 +46,12 @@ DEFINE_XEN_GUEST_HANDLE(xen_ulong_t); > #else > #define XEN_FLEX_ARRAY_DIM 1 /* variable size */ > #endif > +/* Some PV I/O interfaces need a compatibility variant. */ > +#if __XEN_INTERFACE_VERSION__ < 0x00041300 > +#define XENPV_FLEX_ARRAY_DIM 1 /* variable size */ > +#else > +#define XENPV_FLEX_ARRAY_DIM XEN_FLEX_ARRAY_DIM > +#endif May I suggest that this live in io/ring.h, for the lack of a better place under io/? Jan
On 30.11.23 14:14, Jan Beulich wrote: > On 30.11.2023 10:21, Juergen Gross wrote: >> --- a/xen/include/public/xen.h >> +++ b/xen/include/public/xen.h >> @@ -46,6 +46,12 @@ DEFINE_XEN_GUEST_HANDLE(xen_ulong_t); >> #else >> #define XEN_FLEX_ARRAY_DIM 1 /* variable size */ >> #endif >> +/* Some PV I/O interfaces need a compatibility variant. */ >> +#if __XEN_INTERFACE_VERSION__ < 0x00041300 >> +#define XENPV_FLEX_ARRAY_DIM 1 /* variable size */ >> +#else >> +#define XENPV_FLEX_ARRAY_DIM XEN_FLEX_ARRAY_DIM >> +#endif > > May I suggest that this live in io/ring.h, for the lack of a better place > under io/? Yes, fine with me. Juergen
On 30/11/2023 9:21 am, Juergen Gross wrote: > Flexible arrays in public headers can be problematic with some > compilers. > > With XEN_FLEX_ARRAY_DIM there is a mechanism available to deal with > this issue, but care must be taken to not change the affected structs > in an incompatible way. > > So bump __XEN_LATEST_INTERFACE_VERSION__ and introduce a new macro > XENPV_FLEX_ARRAY_DIM which will be XENPV_FLEX_ARRAY_DIM with the > interface version being new enough and "1" (the value used today in > the affected headers) when the interface version is an old one. > > Replace the arr[1] instances (this includes the ones seen to be > problematic in recent Linux kernels [1]) with arr[XENPV_FLEX_ARRAY_DIM] > in order to avoid compilation errors. > > [1]: https://bugzilla.kernel.org/show_bug.cgi?id=217693 > > Signed-off-by: Juergen Gross <jgross@suse.com> CHANGELOG please. This change, however it ends up looking, absolutely needs covering. ~Andrew
On 30.11.23 12:34, Andrew Cooper wrote: > On 30/11/2023 9:21 am, Juergen Gross wrote: >> Flexible arrays in public headers can be problematic with some >> compilers. >> >> With XEN_FLEX_ARRAY_DIM there is a mechanism available to deal with >> this issue, but care must be taken to not change the affected structs >> in an incompatible way. >> >> So bump __XEN_LATEST_INTERFACE_VERSION__ and introduce a new macro >> XENPV_FLEX_ARRAY_DIM which will be XENPV_FLEX_ARRAY_DIM with the >> interface version being new enough and "1" (the value used today in >> the affected headers) when the interface version is an old one. >> >> Replace the arr[1] instances (this includes the ones seen to be >> problematic in recent Linux kernels [1]) with arr[XENPV_FLEX_ARRAY_DIM] >> in order to avoid compilation errors. >> >> [1]: https://bugzilla.kernel.org/show_bug.cgi?id=217693 >> >> Signed-off-by: Juergen Gross <jgross@suse.com> > > CHANGELOG please. This change, however it ends up looking, absolutely > needs covering. Would this be fine: ### Changed + - Changed flexible array definitions in public I/O interface headers to not + use "1" as the number of array elements. Juergen
Hi Juergen,
On 30/11/2023 09:21, Juergen Gross wrote:
> ---
> V2:
> - bump interface version and make change only for new version
> (Jan Beulich)
> ---
> xen/include/public/io/cameraif.h | 2 +-
> xen/include/public/io/displif.h | 2 +-
> xen/include/public/io/fsif.h | 4 ++--
> xen/include/public/io/pvcalls.h | 2 +-
> xen/include/public/io/ring.h | 5 +++--
> xen/include/public/io/sndif.h | 2 +-
> xen/include/public/xen-compat.h | 2 +-
> xen/include/public/xen.h | 6 ++++++
> 8 files changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/xen/include/public/io/cameraif.h b/xen/include/public/io/cameraif.h
> index 13763abef9..a389443769 100644
> --- a/xen/include/public/io/cameraif.h
> +++ b/xen/include/public/io/cameraif.h
> @@ -763,7 +763,7 @@ struct xencamera_buf_create_req {
> */
> struct xencamera_page_directory {
> grant_ref_t gref_dir_next_page;
> - grant_ref_t gref[1]; /* Variable length */
> + grant_ref_t gref[XENPV_FLEX_ARRAY_DIM];
> };
>
> /*
> diff --git a/xen/include/public/io/displif.h b/xen/include/public/io/displif.h
> index 73d0cbdf15..132c96fa5c 100644
> --- a/xen/include/public/io/displif.h
> +++ b/xen/include/public/io/displif.h
> @@ -537,7 +537,7 @@ struct xendispl_dbuf_create_req {
>
> struct xendispl_page_directory {
> grant_ref_t gref_dir_next_page;
> - grant_ref_t gref[1]; /* Variable length */
> + grant_ref_t gref[XENPV_FLEX_ARRAY_DIM];
> };
>
> /*
> diff --git a/xen/include/public/io/fsif.h b/xen/include/public/io/fsif.h
> index ec57850233..dcade1c698 100644
> --- a/xen/include/public/io/fsif.h
> +++ b/xen/include/public/io/fsif.h
> @@ -40,7 +40,7 @@ struct fsif_read_request {
> int32_t pad;
> uint64_t len;
> uint64_t offset;
> - grant_ref_t grefs[1]; /* Variable length */
> + grant_ref_t grefs[XENPV_FLEX_ARRAY_DIM];
> };
>
> struct fsif_write_request {
> @@ -48,7 +48,7 @@ struct fsif_write_request {
> int32_t pad;
> uint64_t len;
> uint64_t offset;
> - grant_ref_t grefs[1]; /* Variable length */
> + grant_ref_t grefs[XENPV_FLEX_ARRAY_DIM];
> };
>
> struct fsif_stat_request {
> diff --git a/xen/include/public/io/pvcalls.h b/xen/include/public/io/pvcalls.h
> index 230b0719e3..af0e9abd13 100644
> --- a/xen/include/public/io/pvcalls.h
> +++ b/xen/include/public/io/pvcalls.h
> @@ -30,7 +30,7 @@ struct pvcalls_data_intf {
> uint8_t pad2[52];
>
> RING_IDX ring_order;
> - grant_ref_t ref[];
> + grant_ref_t ref[XENPV_FLEX_ARRAY_DIM];
I am probably missing something. In the commit message, you suggested
that XENPV_FLEX_ARRAY_DIM will use 1 for older interface version for
compatibility reason.
Yet, if I am not mistaken, [] is not equivalent to [1]. So aren't you
effectively breaking the compatibility?
Cheers,
--
Julien Grall
On 30.11.23 12:33, Julien Grall wrote:
> Hi Juergen,
>
> On 30/11/2023 09:21, Juergen Gross wrote:
>> ---
>> V2:
>> - bump interface version and make change only for new version
>> (Jan Beulich)
>> ---
>> xen/include/public/io/cameraif.h | 2 +-
>> xen/include/public/io/displif.h | 2 +-
>> xen/include/public/io/fsif.h | 4 ++--
>> xen/include/public/io/pvcalls.h | 2 +-
>> xen/include/public/io/ring.h | 5 +++--
>> xen/include/public/io/sndif.h | 2 +-
>> xen/include/public/xen-compat.h | 2 +-
>> xen/include/public/xen.h | 6 ++++++
>> 8 files changed, 16 insertions(+), 9 deletions(-)
>>
>> diff --git a/xen/include/public/io/cameraif.h b/xen/include/public/io/cameraif.h
>> index 13763abef9..a389443769 100644
>> --- a/xen/include/public/io/cameraif.h
>> +++ b/xen/include/public/io/cameraif.h
>> @@ -763,7 +763,7 @@ struct xencamera_buf_create_req {
>> */
>> struct xencamera_page_directory {
>> grant_ref_t gref_dir_next_page;
>> - grant_ref_t gref[1]; /* Variable length */
>> + grant_ref_t gref[XENPV_FLEX_ARRAY_DIM];
>> };
>> /*
>> diff --git a/xen/include/public/io/displif.h b/xen/include/public/io/displif.h
>> index 73d0cbdf15..132c96fa5c 100644
>> --- a/xen/include/public/io/displif.h
>> +++ b/xen/include/public/io/displif.h
>> @@ -537,7 +537,7 @@ struct xendispl_dbuf_create_req {
>> struct xendispl_page_directory {
>> grant_ref_t gref_dir_next_page;
>> - grant_ref_t gref[1]; /* Variable length */
>> + grant_ref_t gref[XENPV_FLEX_ARRAY_DIM];
>> };
>> /*
>> diff --git a/xen/include/public/io/fsif.h b/xen/include/public/io/fsif.h
>> index ec57850233..dcade1c698 100644
>> --- a/xen/include/public/io/fsif.h
>> +++ b/xen/include/public/io/fsif.h
>> @@ -40,7 +40,7 @@ struct fsif_read_request {
>> int32_t pad;
>> uint64_t len;
>> uint64_t offset;
>> - grant_ref_t grefs[1]; /* Variable length */
>> + grant_ref_t grefs[XENPV_FLEX_ARRAY_DIM];
>> };
>> struct fsif_write_request {
>> @@ -48,7 +48,7 @@ struct fsif_write_request {
>> int32_t pad;
>> uint64_t len;
>> uint64_t offset;
>> - grant_ref_t grefs[1]; /* Variable length */
>> + grant_ref_t grefs[XENPV_FLEX_ARRAY_DIM];
>> };
>> struct fsif_stat_request {
>> diff --git a/xen/include/public/io/pvcalls.h b/xen/include/public/io/pvcalls.h
>> index 230b0719e3..af0e9abd13 100644
>> --- a/xen/include/public/io/pvcalls.h
>> +++ b/xen/include/public/io/pvcalls.h
>> @@ -30,7 +30,7 @@ struct pvcalls_data_intf {
>> uint8_t pad2[52];
>> RING_IDX ring_order;
>> - grant_ref_t ref[];
>> + grant_ref_t ref[XENPV_FLEX_ARRAY_DIM];
>
> I am probably missing something. In the commit message, you suggested that
> XENPV_FLEX_ARRAY_DIM will use 1 for older interface version for compatibility
> reason.
>
> Yet, if I am not mistaken, [] is not equivalent to [1]. So aren't you
> effectively breaking the compatibility?
Oh, thanks for catching this one!
I agree, this should be XEN_FLEX_ARRAY_DIM instead.
Juergen
© 2016 - 2024 Red Hat, Inc.