* Re: [PATCH][next] drm/i915/gvt: Avoid -Wflex-array-member-not-at-end warning
2026-04-28 7:53 ` Jani Nikula
@ 2026-04-28 16:32 ` Gustavo A. R. Silva
0 siblings, 0 replies; 3+ messages in thread
From: Gustavo A. R. Silva @ 2026-04-28 16:32 UTC (permalink / raw)
To: Jani Nikula, Gustavo A. R. Silva, Zhenyu Wang, Zhi Wang,
Joonas Lahtinen, Rodrigo Vivi, Tvrtko Ursulin, David Airlie,
Simona Vetter
Cc: intel-gfx, dri-devel, linux-kernel, linux-hardening
On 4/28/26 01:53, Jani Nikula wrote:
> On Mon, 27 Apr 2026, "Gustavo A. R. Silva" <gustavoars@kernel.org> wrote:
>> -Wflex-array-member-not-at-end was introduced in GCC-14, and we are
>> getting ready to enable it, globally.
>>
>> Use the TRAILING_OVERLAP() helper to fix the following warning:
>>
>> drivers/gpu/drm/i915/gvt/opregion.c:126:40: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
>>
>> This helper creates a union between a flexible-array member (FAM)
>> and a set of members that would otherwise follow it. This overlays
>> the trailing members onto the FAM while preserving the original
>> memory layout.
>>
>> Lastly, the static_assert() ensures the alignment between the FAM and
>> struct efp_child_device_config child0; is not inadvertently changed,
>> and it's intentionally placed inmediately after the related structure
>> (that is, no blank line in between).
>>
>> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
>> ---
>> drivers/gpu/drm/i915/gvt/opregion.c | 20 ++++++++++++--------
>> 1 file changed, 12 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gvt/opregion.c b/drivers/gpu/drm/i915/gvt/opregion.c
>> index d6e76ba31d60..efe457c02788 100644
>> --- a/drivers/gpu/drm/i915/gvt/opregion.c
>> +++ b/drivers/gpu/drm/i915/gvt/opregion.c
>> @@ -122,17 +122,21 @@ struct vbt {
>> struct bdb_data_header general_features_header;
>> struct bdb_general_features general_features;
>>
>> - struct bdb_data_header general_definitions_header;
>> - struct bdb_general_definitions general_definitions;
>> -
>> - struct efp_child_device_config child0;
>> - struct efp_child_device_config child1;
>> - struct efp_child_device_config child2;
>> - struct efp_child_device_config child3;
>> -
>> struct bdb_data_header driver_features_header;
>> struct bdb_driver_features driver_features;
>> +
>> + struct bdb_data_header general_definitions_header;
>> +
>> + /* Must be last as it ends in a flexible-array member. */
>> + TRAILING_OVERLAP(struct bdb_general_definitions, general_definitions, devices,
>> + struct efp_child_device_config child0;
>> + struct efp_child_device_config child1;
>> + struct efp_child_device_config child2;
>> + struct efp_child_device_config child3;
>> + );
>
> So this impacts the generation of a binary blob, parsed by the client OS
> driver. In theory, the order of the BDB blocks shouldn't matter, but who
> knows.
>
> Anyway, I'm more worried about inadvertent padding potentially being
> introduced. struct vbt should have __packed attribute, which is missing,
> but I also think the union and the struct within TRAILING_OVERLAP()
> should also have __packed.
>
> Like, if struct efp_child_device_config gets extended by one byte,
> what's going to happen with padding? It's __packed on its own, but IIUC
> that doesn't automatically apply to the enclosing structs or unions.
We have __TRAILING_OVERLAP() to add attributes like __packed to the
overlapping group of MEMBERS.
So, the patch would look as follows (including the addition of __packed to
struct vbt):
diff --git a/drivers/gpu/drm/i915/gvt/opregion.c b/drivers/gpu/drm/i915/gvt/opregion.c
index d6e76ba31d60..f4fabba56a1b 100644
--- a/drivers/gpu/drm/i915/gvt/opregion.c
+++ b/drivers/gpu/drm/i915/gvt/opregion.c
@@ -122,17 +122,21 @@ struct vbt {
struct bdb_data_header general_features_header;
struct bdb_general_features general_features;
- struct bdb_data_header general_definitions_header;
- struct bdb_general_definitions general_definitions;
-
- struct efp_child_device_config child0;
- struct efp_child_device_config child1;
- struct efp_child_device_config child2;
- struct efp_child_device_config child3;
-
struct bdb_data_header driver_features_header;
struct bdb_driver_features driver_features;
-};
+
+ struct bdb_data_header general_definitions_header;
+
+ /* Must be last as it ends in a flexible-array member. */
+ __TRAILING_OVERLAP(struct bdb_general_definitions, general_definitions, devices, __packed,
+ struct efp_child_device_config child0;
+ struct efp_child_device_config child1;
+ struct efp_child_device_config child2;
+ struct efp_child_device_config child3;
+ );
+} __packed;
+static_assert(offsetof(struct vbt, general_definitions.devices) ==
+ offsetof(struct vbt, child0));
However, Sashiko says this[1]:
"Does moving these fields physically change the byte-for-byte layout and
block sequence of the VBT exposed to the guest VM?
struct vbt represents the exact layout of the synthetic VBT exposed to the
guest VM via the OpRegion. In intel_vgpu_init_opregion(), the structure is
directly copied to guest memory:
drivers/gpu/drm/i915/gvt/opregion.c:intel_vgpu_init_opregion() {
...
memcpy(buf + INTEL_GVT_OPREGION_VBT_OFFSET, &v, sizeof(struct vbt));
...
}"
If shuffling fields around actually causes any issues, I can use a different
approach, like the one below (thanks to -fms-extensions):
diff --git a/drivers/gpu/drm/i915/display/intel_vbt_defs.h b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
index 0dc13d080e8a..b238636e315e 100644
--- a/drivers/gpu/drm/i915/display/intel_vbt_defs.h
+++ b/drivers/gpu/drm/i915/display/intel_vbt_defs.h
@@ -568,7 +568,7 @@ struct child_device_config {
u32 edp_data_rate_override_reserved:20; /* 263+ */
} __packed;
-struct bdb_general_definitions {
+struct bdb_general_definitions_hdr {
/* DDC GPIO */
u8 crt_ddc_gmbus_pin;
@@ -581,7 +581,10 @@ struct bdb_general_definitions {
/* boot device bits */
u8 boot_display[2];
u8 child_dev_size;
+} __packed;
+struct bdb_general_definitions {
+ struct bdb_general_definitions_hdr;
/*
* Device info:
* If TV is present, it'll be at devices[0].
diff --git a/drivers/gpu/drm/i915/gvt/opregion.c b/drivers/gpu/drm/i915/gvt/opregion.c
index d6e76ba31d60..3ebdc4c28c5b 100644
--- a/drivers/gpu/drm/i915/gvt/opregion.c
+++ b/drivers/gpu/drm/i915/gvt/opregion.c
@@ -123,7 +123,7 @@ struct vbt {
struct bdb_general_features general_features;
struct bdb_data_header general_definitions_header;
- struct bdb_general_definitions general_definitions;
+ struct bdb_general_definitions_hdr general_definitions;
struct efp_child_device_config child0;
struct efp_child_device_config child1;
@@ -132,7 +132,7 @@ struct vbt {
struct bdb_data_header driver_features_header;
struct bdb_driver_features driver_features;
-};
+} __packed;
static void virt_vbt_generation(struct vbt *v)
{
However, in this particular case, __TRAILING_OVERLAP() is more robust.
Thanks for the feedback!
-Gustavo
[1] https://sashiko.dev/#/patchset/ae_4GkBsNl_0SYTm%40kspp
^ permalink raw reply related [flat|nested] 3+ messages in thread