[S] Change in coreboot[main]: soc/intel/{adl,mtl}: Don't set up SPD on LPDDRx

[Arthur Heymans (Code Review)]

Attention is currently required from: Dinesh Gehlot, Eran Mitrani, Jakub Czapiga, Kapil Porwal, Nick Vaccaro, Rishika Raj, Subrata Banik, Tarun.

Arthur Heymans has posted comments on this change by Arthur Heymans. ( https://review.coreboot.org/c/coreboot/+/84205?usp=email )

Change subject: soc/intel/{adl,mtl}: Don't set up SPD on LPDDRx
......................................................................

Patch Set 3:

(1 comment)

File src/soc/intel/meteorlake/meminit.c:

https://review.coreboot.org/c/coreboot/+/84205/comment/8d14cb18_e4bae43b?usp... :
PS2, Line 209: has_spd = true;
> > > > > > > Can you help me understand the purpose of `has_spd`? Does it mean the mainboard has an SPD chip and we'll read the hex file using SMBUS?
> > > > > >
> > > > > > would need_spd be a better name? LPDDRx DRAM technology does not use SPD and the code for SPD in soc/intel/common is doing doing undefined behavior like overflowing an array.
> > > > >
> > > > > Yes, that name sounds better. I'd like to suggest adding a comment that explains the purpose of this variable. People often confuse the SPD chip with the SPD hex data file. The code should make it clear that the SPD chip isn't required to read the SPD hex file (need_spd).
> > > >
> > > > No actually it's about SPD data and this code is wrong. The soc/intel/common code is simply wrong for LPDDRx. The loop over channels and dimms per channel is overflowing. There are no DIMMs in LPDDRx which is what need to be addressed.
> > >
> > > okay, so when you set `has_spd` or `need_spd`, are you saying that we will read SPD data over SMBUS and for lpddrx, we shall pass SPD hex file ?
> >
> > No that's not the issue. CONFIG_DIMM_MAX is 4 which is how large the array is for containing SPD pointers. LPDDRx has 16 bit per channel, so the code thinks there are 8 channels in total. CONFIG_DIMM_PER_CHANNEL is set to 2. The common code loops over channels and dimms per channel so it's overflowing. What is the appropriate fix? do you think.
> >
> > https://qa.coreboot.org/job/coreboot-gerrit/263746/testReport/junit/(root)/g... is the compilation error with LTO which can detect the buffer overflow.
>
> will this work ?
>  git diff 
> diff --git a/src/soc/intel/common/block/memory/meminit.c b/src/soc/intel/common/block/memory/meminit.c
> index f583213254..cf12aff803 100644
> --- a/src/soc/intel/common/block/memory/meminit.c
> +++ b/src/soc/intel/common/block/memory/meminit.c
> @@ -189,12 +189,13 @@ void mem_populate_channel_data(FSPM_UPD *memupd, const struct soc_mem_cfg *soc_m
>                                 struct mem_channel_data *data)
>  {
>         size_t spd_md_len = 0, spd_dimm_len = 0;
> -       bool have_dimms;
> +       bool have_dimms = 0;
>
>         memset(data, 0, sizeof(*data));
>
>         read_spd_md(soc_mem_cfg, spd_info, half_populated, data, &spd_md_len);
> -       have_dimms = read_spd_dimm(memupd, soc_mem_cfg, spd_info, half_populated, data,
> +       if ((info->topo & MEM_TOPO_DIMM_MODULE)
> +                       have_dimms = read_spd_dimm(memupd, soc_mem_cfg, spd_info, half_populated, data,
>                                 &spd_dimm_len);
>
>         if (data->ch_population_flags == NO_CHANNEL_POPULATED)

I tried this. And looking more at it seems that this in fact a compiler bug. I'll play around with some options. It looks like even without lto this unused code would be in the binary. Maybe it's reasonable to move the memory type to Kconfig to optimize the code?