Re: [PATCH][next] drm/i915/gvt: Avoid -Wflex-array-member-not-at-end warning

["Gustavo A. R. Silva" <gustavo@embeddedor.com>]

Hi all,

I wonder if you have any comments on this. :)

Thanks!
-Gustavo

>>> 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