[RFC] Approaches to deal with a struct with multiple fake flexible arrays members

[RFC] Approaches to deal with a struct with multiple fake flexible arrays members

[Paulo Miguel Almeida]

Hi KSPP community,

I've been working on replacing 1-element arrays with flex array members
on the drm/amdgpu files. I came across one insteresting case which I
may need to pick your brains to find a solution for it.

The structure below has two fake flexible arrays but I would get an
error if I try make them both FAM. How should/could I deal with the
asRegIndexBuf in this case? In theory, DECLARE_FLEX_ARRAY would "work"
but that doesn't seem to be its intended usage as far I've searched.
(unless I got it wrong, if that's the case, feel free to set me straight)

Any ideas? 

struct _ATOM_INIT_REG_BLOCK {
	USHORT                     usRegIndexTblSize;    /*     0     2 */
	USHORT                     usRegDataBlkSize;     /*     2     2 */
	ATOM_INIT_REG_INDEX_FORMAT asRegIndexBuf[1];     /*     4     3 */
	ATOM_MEMORY_SETTING_DATA_BLOCK asRegDataBuf[1];  /*     7     8 */

	/* size: 15, cachelines: 1, members: 4 */
	/* last cacheline: 15 bytes */
} __attribute__((__packed__));

thanks!

- Paulo A.

Re: [RFC] Approaches to deal with a struct with multiple fake flexible arrays members

[Gustavo A. R. Silva]

On Wed, Nov 09, 2022 at 04:45:42PM +1300, Paulo Miguel Almeida wrote:

Adding Alex, Christian and DRM lists to the thread.

> Hi KSPP community,
> 
> I've been working on replacing 1-element arrays with flex array members
> on the drm/amdgpu files. I came across one insteresting case which I
> may need to pick your brains to find a solution for it.
> 
> The structure below has two fake flexible arrays but I would get an
> error if I try make them both FAM. How should/could I deal with the
> asRegIndexBuf in this case? In theory, DECLARE_FLEX_ARRAY would "work"
> but that doesn't seem to be its intended usage as far I've searched.
> (unless I got it wrong, if that's the case, feel free to set me straight)
> 
> Any ideas? 
> 
> struct _ATOM_INIT_REG_BLOCK {
> 	USHORT                     usRegIndexTblSize;    /*     0     2 */
> 	USHORT                     usRegDataBlkSize;     /*     2     2 */
> 	ATOM_INIT_REG_INDEX_FORMAT asRegIndexBuf[1];     /*     4     3 */
> 	ATOM_MEMORY_SETTING_DATA_BLOCK asRegDataBuf[1];  /*     7     8 */

I didn't find evidence that asRegDataBuf is used anywhere in the
codebase[1].

> 
> 	/* size: 15, cachelines: 1, members: 4 */
> 	/* last cacheline: 15 bytes */
> } __attribute__((__packed__));

Alex, Christian,

It looks like this structure is only being used as a template to populate
instances of struct atom_mc_reg_table[2] and that these[3] are the only
places where asRegIndexBuf[] is being used. Apparently, this array is only
being used to retrieve it's address so that a pointer can jump[4] in chucks
of size sizeof(ATOM_INIT_REG_INDEX_FORMAT):

drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c:1461:
1461:	format = (ATOM_INIT_REG_INDEX_FORMAT *)
1462:		((u8 *)format + sizeof(ATOM_INIT_REG_INDEX_FORMAT));

up to (VBIOS_MC_REGISTER_ARRAY_SIZE * sizeof(ATOM_INIT_REG_INDEX_FORMAT))[5],

As I pointed out above, I don't see asRegDataBuf[] being used in the
codebase (of course it may describe firmware memory layout). Instead,
there is this jump to a block of data past asRegIndexBuf[]:

drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c:1444:
1444:	ATOM_MEMORY_SETTING_DATA_BLOCK *reg_data =
1445:		(ATOM_MEMORY_SETTING_DATA_BLOCK *)
1446:		((u8 *)reg_block + (2 * sizeof(u16)) +
1447:			 le16_to_cpu(reg_block->usRegIndexTblSize));

So, it seems the one relevant array, from the kernel side, is
asRegIndexBuf[]. I wonder if we really need asRegDataBuf[] in that
structure... or if we could try modifying that struct and only have
asRegIndexBuf[] as last member? and then we can transform it into a
flex-array member.

If for any strong reasong we cannot remove asRegDataBuf[] then I think we
could give it a try and use DECLARE_FLEX_ARRAY() to declare both arrays
in the structure.

But first, of course, Alex, Christian, it'd be really great if we can
have your input and feedback. :)

Thanks!
--
Gustavo

[1] https://elixir.bootlin.com/linux/latest/C/ident/asRegDataBuf
[2] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c#L1441
[3] https://elixir.bootlin.com/linux/latest/C/ident/asRegIndexBuf

Re: [RFC] Approaches to deal with a struct with multiple fake flexible arrays members

[Paulo Miguel Almeida]

On Wed, Nov 09, 2022 at 06:45:57PM -0600, Gustavo A. R. Silva wrote:
> On Wed, Nov 09, 2022 at 04:45:42PM +1300, Paulo Miguel Almeida wrote:
> 
> Adding Alex, Christian and DRM lists to the thread.

Thanks so much for your reply Gustavo 
Yep, that's a good idea.

> 
> > struct _ATOM_INIT_REG_BLOCK {
> > 	USHORT                     usRegIndexTblSize;    /*     0     2 */
> > 	USHORT                     usRegDataBlkSize;     /*     2     2 */
> > 	ATOM_INIT_REG_INDEX_FORMAT asRegIndexBuf[1];     /*     4     3 */
> > 	ATOM_MEMORY_SETTING_DATA_BLOCK asRegDataBuf[1];  /*     7     8 */
> 
> I didn't find evidence that asRegDataBuf is used anywhere in the
> codebase[1].
> ...
> <snip> 
> ...
> As I pointed out above, I don't see asRegDataBuf[] being used in the
> codebase (of course it may describe firmware memory layout). Instead,
> there is this jump to a block of data past asRegIndexBuf[]:
> 
> drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c:1444:
> 1444:	ATOM_MEMORY_SETTING_DATA_BLOCK *reg_data =
> 1445:		(ATOM_MEMORY_SETTING_DATA_BLOCK *)
> 1446:		((u8 *)reg_block + (2 * sizeof(u16)) +
> 1447:			 le16_to_cpu(reg_block->usRegIndexTblSize));
> 
> So, it seems the one relevant array, from the kernel side, is
> asRegIndexBuf[]. I wonder if we really need asRegDataBuf[] in that
> structure... or if we could try modifying that struct and only have
> asRegIndexBuf[] as last member? and then we can transform it into a
> flex-array member.

I saw that one too. That would be the way it's currently accessing
asRegDataBuf member as the existing struct would make asRegDataBuf[0] point 
to some index within the asRegIndexBuf member (as you probably got it too)

you are right... asRegDataBuff isn't used from a static analysis
point of view but removing it make the code a bit cryptic, right?

That's pickle, ay? :-)

> 
> If for any strong reasong we cannot remove asRegDataBuf[] then I think we
> could give it a try and use DECLARE_FLEX_ARRAY() to declare both arrays
> in the structure.
> 

Out of curiosity, why both rather than just asRegIndexBuf?

> But first, of course, Alex, Christian, it'd be really great if we can
> have your input and feedback. :)
> 
> Thanks!
> --
> Gustavo
> 

- Paulo A.

Re: [RFC] Approaches to deal with a struct with multiple fake flexible arrays members

[Alex Deucher]

On Wed, Nov 9, 2022 at 8:31 PM Paulo Miguel Almeida
<paulo.miguel.almeida.rodenas@gmail.com> wrote:
>
> On Wed, Nov 09, 2022 at 06:45:57PM -0600, Gustavo A. R. Silva wrote:
> > On Wed, Nov 09, 2022 at 04:45:42PM +1300, Paulo Miguel Almeida wrote:
> >
> > Adding Alex, Christian and DRM lists to the thread.
>
> Thanks so much for your reply Gustavo
> Yep, that's a good idea.
>
> >
> > > struct _ATOM_INIT_REG_BLOCK {
> > >     USHORT                     usRegIndexTblSize;    /*     0     2 */
> > >     USHORT                     usRegDataBlkSize;     /*     2     2 */
> > >     ATOM_INIT_REG_INDEX_FORMAT asRegIndexBuf[1];     /*     4     3 */
> > >     ATOM_MEMORY_SETTING_DATA_BLOCK asRegDataBuf[1];  /*     7     8 */
> >
> > I didn't find evidence that asRegDataBuf is used anywhere in the
> > codebase[1].
> > ...
> > <snip>
> > ...
> > As I pointed out above, I don't see asRegDataBuf[] being used in the
> > codebase (of course it may describe firmware memory layout). Instead,
> > there is this jump to a block of data past asRegIndexBuf[]:
> >
> > drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c:1444:
> > 1444: ATOM_MEMORY_SETTING_DATA_BLOCK *reg_data =
> > 1445:         (ATOM_MEMORY_SETTING_DATA_BLOCK *)
> > 1446:         ((u8 *)reg_block + (2 * sizeof(u16)) +
> > 1447:                  le16_to_cpu(reg_block->usRegIndexTblSize));
> >
> > So, it seems the one relevant array, from the kernel side, is
> > asRegIndexBuf[]. I wonder if we really need asRegDataBuf[] in that
> > structure... or if we could try modifying that struct and only have
> > asRegIndexBuf[] as last member? and then we can transform it into a
> > flex-array member.
>
> I saw that one too. That would be the way it's currently accessing
> asRegDataBuf member as the existing struct would make asRegDataBuf[0] point
> to some index within the asRegIndexBuf member (as you probably got it too)
>
> you are right... asRegDataBuff isn't used from a static analysis
> point of view but removing it make the code a bit cryptic, right?
>
> That's pickle, ay? :-)
>
> >
> > If for any strong reasong we cannot remove asRegDataBuf[] then I think we
> > could give it a try and use DECLARE_FLEX_ARRAY() to declare both arrays
> > in the structure.
> >
>
> Out of curiosity, why both rather than just asRegIndexBuf?
>
> > But first, of course, Alex, Christian, it'd be really great if we can
> > have your input and feedback. :)

This header describes the layout of memory stored in the ROM on the
GPU.  It's shared between vbios and driver so some parts aren't
necessarily directly used by the driver.  As for what to do about it,
I'm not sure.  This structure stores a set of register offsets
(asRegIndexBuf) and data values (asRegDataBuf) for specific operations
(e.g., walk this structure and program register X with value Y.  For a
little background on atombios, it's a set of data and command tables
stored in the vbios ROM image.  The driver has an interpreter for the
command tables (see atom.c) so it can parse the command tables to
initialize the asic to a usable state.  The various programming
sequences vary depending on the components the AIB/OEM uses for the
board (different vram vendors, different clock/voltage settings,
etc.).  The legacy VGA/VBE and the GOP driver and the OS driver can
use these tables, so this allows us to initialize any GPU in a generic
way on any architecture even if the platform firmware doesn't post the
card.  For the most part the driver doesn't have to deal with these
particular tables directly, other than for some very specific cases
where the driver needs to grab some elements from the tables to
populate the power management controller for GPU memory reclocking
parameters.  However, the command tables as interpreted by the parser
very often will directly parse these tables.  So you might have a
command table that the driver executes to initialize some part of the
GPU which ultimately fetches the table from the ROM image and walks it
programming registers based on the offset and values in that table.
So if you were debugging something that involves the atombios parser
and walking through one of the command tables, you may be confused if
the data tables don't match what header says.  So ideally, we'd keep
both arrays.

Alex

> >
> > Thanks!
> > --
> > Gustavo
> >
>
> - Paulo A.

Re: [RFC] Approaches to deal with a struct with multiple fake flexible arrays members

[Gustavo A. R. Silva]

On Wed, Nov 09, 2022 at 10:20:34PM -0500, Alex Deucher wrote:
> On Wed, Nov 9, 2022 at 8:31 PM Paulo Miguel Almeida
> <paulo.miguel.almeida.rodenas@gmail.com> wrote:
> >
> > On Wed, Nov 09, 2022 at 06:45:57PM -0600, Gustavo A. R. Silva wrote:
> > > On Wed, Nov 09, 2022 at 04:45:42PM +1300, Paulo Miguel Almeida wrote:
> > >
> > > Adding Alex, Christian and DRM lists to the thread.
> >
> > Thanks so much for your reply Gustavo
> > Yep, that's a good idea.
> >
> > >
> > > > struct _ATOM_INIT_REG_BLOCK {
> > > >     USHORT                     usRegIndexTblSize;    /*     0     2 */
> > > >     USHORT                     usRegDataBlkSize;     /*     2     2 */
> > > >     ATOM_INIT_REG_INDEX_FORMAT asRegIndexBuf[1];     /*     4     3 */
> > > >     ATOM_MEMORY_SETTING_DATA_BLOCK asRegDataBuf[1];  /*     7     8 */
> > >
> > > I didn't find evidence that asRegDataBuf is used anywhere in the
> > > codebase[1].
> > > ...
> > > <snip>
> > > ...
> > > As I pointed out above, I don't see asRegDataBuf[] being used in the
> > > codebase (of course it may describe firmware memory layout). Instead,
> > > there is this jump to a block of data past asRegIndexBuf[]:
> > >
> > > drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c:1444:
> > > 1444: ATOM_MEMORY_SETTING_DATA_BLOCK *reg_data =
> > > 1445:         (ATOM_MEMORY_SETTING_DATA_BLOCK *)
> > > 1446:         ((u8 *)reg_block + (2 * sizeof(u16)) +
> > > 1447:                  le16_to_cpu(reg_block->usRegIndexTblSize));
> > >
> > > So, it seems the one relevant array, from the kernel side, is
> > > asRegIndexBuf[]. I wonder if we really need asRegDataBuf[] in that
> > > structure... or if we could try modifying that struct and only have
> > > asRegIndexBuf[] as last member? and then we can transform it into a
> > > flex-array member.
> >
> > I saw that one too. That would be the way it's currently accessing
> > asRegDataBuf member as the existing struct would make asRegDataBuf[0] point
> > to some index within the asRegIndexBuf member (as you probably got it too)
> >
> > you are right... asRegDataBuff isn't used from a static analysis
> > point of view but removing it make the code a bit cryptic, right?
> >
> > That's pickle, ay? :-)
> >
> > >
> > > If for any strong reasong we cannot remove asRegDataBuf[] then I think we
> > > could give it a try and use DECLARE_FLEX_ARRAY() to declare both arrays
> > > in the structure.
> > >
> >
> > Out of curiosity, why both rather than just asRegIndexBuf?

Because if I understand the code and Alex's reply below correctly, both
arrays are being used to describe data of variable size, and both arrays
need to stay. The situation is that it'd be _strange_ to transform just
one of them into a flex-array member and not the other, and at the same
time a flex-array member may only appear as the last member of a
struct, and it's not _great_ to have more than one flex-array member
in a struct --in fact, we can't.

DECLARE_FLEX_ARRAY() was originally designed to have flex-array members
in unions. This is, we can declare multiple flex-array members as long as
they all share the same address. Unfortunately, this is not the case with
asRegIndexBuf[] and asRegDataBuf[], and I don't see[1] DECLARE_FLEX_ARRAY()
working in this case. So, nope, we cannot use it to declare both arrays
and we also cannot have a flex-array in the middle of a struct, so we
cannot use it to declare asRegIndexBuf[] alone. :/

On the other hand, I fail to see how the current state of the code is
problematic for things like -fstrict-flex-arrays, right now. asRegDataBuf[]
is not being used for anything in the kernel, and asRegIndexBuf[] is
correctly being accessed through it's only valid index zero, after which
is decays into a pointer[2].

That struct is definitely not great (I don't love it), but we might try
to explore other cases while we determine how and if we ultimately need
to modify it.

I'll open an issue for this in the bug tracker, so we keep an eye on it.
:)

> >
> > > But first, of course, Alex, Christian, it'd be really great if we can
> > > have your input and feedback. :)
> 
> This header describes the layout of memory stored in the ROM on the
> GPU.  It's shared between vbios and driver so some parts aren't
> necessarily directly used by the driver.  As for what to do about it,
> I'm not sure.  This structure stores a set of register offsets
> (asRegIndexBuf) and data values (asRegDataBuf) for specific operations
> (e.g., walk this structure and program register X with value Y.  For a
> little background on atombios, it's a set of data and command tables
> stored in the vbios ROM image.  The driver has an interpreter for the
> command tables (see atom.c) so it can parse the command tables to
> initialize the asic to a usable state.  The various programming
> sequences vary depending on the components the AIB/OEM uses for the
> board (different vram vendors, different clock/voltage settings,
> etc.).  The legacy VGA/VBE and the GOP driver and the OS driver can
> use these tables, so this allows us to initialize any GPU in a generic
> way on any architecture even if the platform firmware doesn't post the
> card.  For the most part the driver doesn't have to deal with these
> particular tables directly, other than for some very specific cases
> where the driver needs to grab some elements from the tables to
> populate the power management controller for GPU memory reclocking
> parameters.  However, the command tables as interpreted by the parser
> very often will directly parse these tables.  So you might have a
> command table that the driver executes to initialize some part of the
> GPU which ultimately fetches the table from the ROM image and walks it
> programming registers based on the offset and values in that table.
> So if you were debugging something that involves the atombios parser
> and walking through one of the command tables, you may be confused if
> the data tables don't match what header says.  So ideally, we'd keep
> both arrays.

Thanks a lot for the input, Alex.
--
Gustavo

[1] https://godbolt.org/z/sxa7K9Erd
[2] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c#L1448

Re: [RFC] Approaches to deal with a struct with multiple fake flexible arrays members

[Paulo Miguel Almeida]

On Thu, Nov 10, 2022 at 11:39:02PM -0600, Gustavo A. R. Silva wrote:
> On Wed, Nov 09, 2022 at 10:20:34PM -0500, Alex Deucher wrote:
> > On Wed, Nov 9, 2022 at 8:31 PM Paulo Miguel Almeida
> > <paulo.miguel.almeida.rodenas@gmail.com> wrote:
> > >
> > > On Wed, Nov 09, 2022 at 06:45:57PM -0600, Gustavo A. R. Silva wrote:
> > > > On Wed, Nov 09, 2022 at 04:45:42PM +1300, Paulo Miguel Almeida wrote:
> > > >
> > > > Adding Alex, Christian and DRM lists to the thread.
> > >
> > > Thanks so much for your reply Gustavo
> > > Yep, that's a good idea.
> > >
> > > >
> > > > > struct _ATOM_INIT_REG_BLOCK {
> > > > >     USHORT                     usRegIndexTblSize;    /*     0     2 */
> > > > >     USHORT                     usRegDataBlkSize;     /*     2     2 */
> > > > >     ATOM_INIT_REG_INDEX_FORMAT asRegIndexBuf[1];     /*     4     3 */
> > > > >     ATOM_MEMORY_SETTING_DATA_BLOCK asRegDataBuf[1];  /*     7     8 */
> > > >
> > > > I didn't find evidence that asRegDataBuf is used anywhere in the
> > > > codebase[1].
> > > > ...
> > > > <snip>
> > > > ...
> > > > As I pointed out above, I don't see asRegDataBuf[] being used in the
> > > > codebase (of course it may describe firmware memory layout). Instead,
> > > > there is this jump to a block of data past asRegIndexBuf[]:
> > > >
> > > > drivers/gpu/drm/amd/amdgpu/amdgpu_atombios.c:1444:
> > > > 1444: ATOM_MEMORY_SETTING_DATA_BLOCK *reg_data =
> > > > 1445:         (ATOM_MEMORY_SETTING_DATA_BLOCK *)
> > > > 1446:         ((u8 *)reg_block + (2 * sizeof(u16)) +
> > > > 1447:                  le16_to_cpu(reg_block->usRegIndexTblSize));
> > > >
> > > > So, it seems the one relevant array, from the kernel side, is
> > > > asRegIndexBuf[]. I wonder if we really need asRegDataBuf[] in that
> > > > structure... or if we could try modifying that struct and only have
> > > > asRegIndexBuf[] as last member? and then we can transform it into a
> > > > flex-array member.
> > >
> > > I saw that one too. That would be the way it's currently accessing
> > > asRegDataBuf member as the existing struct would make asRegDataBuf[0] point
> > > to some index within the asRegIndexBuf member (as you probably got it too)
> > >
> > > you are right... asRegDataBuff isn't used from a static analysis
> > > point of view but removing it make the code a bit cryptic, right?
> > >
> > > That's pickle, ay? :-)
> > >
> > > >
> > > > If for any strong reasong we cannot remove asRegDataBuf[] then I think we
> > > > could give it a try and use DECLARE_FLEX_ARRAY() to declare both arrays
> > > > in the structure.
> > > >
> > >
> > > Out of curiosity, why both rather than just asRegIndexBuf?
> 
> Because if I understand the code and Alex's reply below correctly, both
> arrays are being used to describe data of variable size, and both arrays
> need to stay. The situation is that it'd be _strange_ to transform just
> one of them into a flex-array member and not the other, and at the same

My apologies, I tried being succinct and I ended up mistakenly leading you 
to understand a different thing. I will be more careful next time :-)

What I meant was why would you use DECLARE_FLEX_ARRAY macro for both
members instead of the following ?

typedef struct _ATOM_INIT_REG_BLOCK{
   USHORT                           usRegIndexTblSize;
   USHORT                           usRegDataBlkSize;
   DECLARE_FLEX_ARRAY(ATOM_INIT_REG_INDEX_FORMAT, asRegIndexBuf);  // Macro needed
   ATOM_MEMORY_SETTING_DATA_BLOCK   asRegDataBuf[];		   // Regular FMA
}ATOM_INIT_REG_BLOCK;

> On the other hand, I fail to see how the current state of the code is
> problematic for things like -fstrict-flex-arrays, right now. asRegDataBuf[]
> is not being used for anything in the kernel, and asRegIndexBuf[] is
> correctly being accessed through it's only valid index zero, after which
> is decays into a pointer[2].
> 
> That struct is definitely not great (I don't love it), but we might try
> to explore other cases while we determine how and if we ultimately need
> to modify it.
> 
> I'll open an issue for this in the bug tracker, so we keep an eye on it.
> :)

Fair enough. Thanks heaps Gustavo, I will move on to the other fake flex
array occurences and circle back to it once a decision in made. Please
count on me to make the changes :-)

> 
> > >
> > > > But first, of course, Alex, Christian, it'd be really great if we can
> > > > have your input and feedback. :)
> > 
> > This header describes the layout of memory stored in the ROM on the
> > GPU.  It's shared between vbios and driver so some parts aren't
> > necessarily directly used by the driver.  As for what to do about it,
> > I'm not sure.  This structure stores a set of register offsets
> > (asRegIndexBuf) and data values (asRegDataBuf) for specific operations
> > (e.g., walk this structure and program register X with value Y.  For a
> > little background on atombios, it's a set of data and command tables
> > stored in the vbios ROM image.  The driver has an interpreter for the
> > command tables (see atom.c) so it can parse the command tables to
> > initialize the asic to a usable state.  The various programming
> > sequences vary depending on the components the AIB/OEM uses for the
> > board (different vram vendors, different clock/voltage settings,
> > etc.).  The legacy VGA/VBE and the GOP driver and the OS driver can
> > use these tables, so this allows us to initialize any GPU in a generic
> > way on any architecture even if the platform firmware doesn't post the
> > card.  For the most part the driver doesn't have to deal with these
> > particular tables directly, other than for some very specific cases
> > where the driver needs to grab some elements from the tables to
> > populate the power management controller for GPU memory reclocking
> > parameters.  However, the command tables as interpreted by the parser
> > very often will directly parse these tables.  So you might have a
> > command table that the driver executes to initialize some part of the
> > GPU which ultimately fetches the table from the ROM image and walks it
> > programming registers based on the offset and values in that table.
> > So if you were debugging something that involves the atombios parser
> > and walking through one of the command tables, you may be confused if
> > the data tables don't match what header says.  So ideally, we'd keep
> > both arrays.
> 
> Thanks a lot for the input, Alex.
> --
> Gustavo
> 

Same here, thanks heaps Alex!

- Paulo A.