[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 at 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.