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
