lkml.org 
[lkml]   [2022]   [Dec]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] [next] i915/gvt: Replace one-element array with flexible-array member
On 2022.12.20 16:41:15 +1300, Paulo Miguel Almeida wrote:
> One-element arrays are deprecated, and we are replacing them with
> flexible array members instead. So, replace one-element array with
> flexible-array member in struct gvt_firmware_header and refactor the
> rest of the code accordingly.
>
> Additionally, previous implementation was allocating 8 bytes more than
> required to represent firmware_header + cfg_space data + mmio data.
>
> This helps with the ongoing efforts to tighten the FORTIFY_SOURCE
> routines on memcpy() and help us make progress towards globally
> enabling -fstrict-flex-arrays=3 [1].
>
> Link: https://github.com/KSPP/linux/issues/79
> Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101836 [1]
> Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
> ---
> To make reviewing this patch easier, I'm pasting before/after struct
> sizes.
>
> pahole -C gvt_firmware_header before/drivers/gpu/drm/i915/gvt/firmware.o
> struct gvt_firmware_header {
> u64 magic; /* 0 8 */
> u32 crc32; /* 8 4 */
> u32 version; /* 12 4 */
> u64 cfg_space_size; /* 16 8 */
> u64 cfg_space_offset; /* 24 8 */
> u64 mmio_size; /* 32 8 */
> u64 mmio_offset; /* 40 8 */
> unsigned char data[1]; /* 48 1 */
>
> /* size: 56, cachelines: 1, members: 8 */
> /* padding: 7 */
> /* last cacheline: 56 bytes */
> };
>
> pahole -C gvt_firmware_header after/drivers/gpu/drm/i915/gvt/firmware.o
> struct gvt_firmware_header {
> u64 magic; /* 0 8 */
> u32 crc32; /* 8 4 */
> u32 version; /* 12 4 */
> u64 cfg_space_size; /* 16 8 */
> u64 cfg_space_offset; /* 24 8 */
> u64 mmio_size; /* 32 8 */
> u64 mmio_offset; /* 40 8 */
> unsigned char data[]; /* 48 0 */
>
> /* size: 48, cachelines: 1, members: 8 */
> /* last cacheline: 48 bytes */
> };
>
> As you can see the additional byte of the fake-flexible array (data[1])
> forced the compiler to pad the struct but those bytes aren't actually used
> as first & last bytes (of both cfg_space and mmio) are controlled by the
> <>_size and <>_offset members present in the gvt_firmware_header struct.
> ---
> drivers/gpu/drm/i915/gvt/firmware.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gvt/firmware.c b/drivers/gpu/drm/i915/gvt/firmware.c
> index a683c22d5b64..dce93738e98a 100644
> --- a/drivers/gpu/drm/i915/gvt/firmware.c
> +++ b/drivers/gpu/drm/i915/gvt/firmware.c
> @@ -45,7 +45,7 @@ struct gvt_firmware_header {
> u64 cfg_space_offset; /* offset in the file */
> u64 mmio_size;
> u64 mmio_offset; /* offset in the file */
> - unsigned char data[1];
> + unsigned char data[];
> };
>
> #define dev_to_drm_minor(d) dev_get_drvdata((d))
> @@ -77,7 +77,7 @@ static int expose_firmware_sysfs(struct intel_gvt *gvt)
> unsigned long size, crc32_start;
> int ret;
>
> - size = sizeof(*h) + info->mmio_size + info->cfg_space_size;
> + size = offsetof(struct gvt_firmware_header, data) + info->mmio_size + info->cfg_space_size;
> firmware = vzalloc(size);
> if (!firmware)
> return -ENOMEM;
> --

Looks good to me.
Reviewed-by: Zhenyu Wang <zhenyuw@linux.intel.com>

Thanks!
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2023-03-26 23:15    [W:0.037 / U:0.036 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site