[coreboot] Re: Memory Down approach Error on intel Denverton board

[Angel Pons]

Hi,

On Mon, Jan 24, 2022 at 8:26 AM Ganesh Kumar C via coreboot
 wrote:
> Hi MariuszX,
>
> Thanks for you time .
>
> Yes . I have added the below  memoryDownConfig struct in
>
> src/mainboard/intel/harcuvar/romstage.c file .
>
> const MEMORY_DOWN_CONFIG mMemoryDownConfig = {
>         .SlotState = {
>                 {STATE_MEMORY_DOWN, STATE_MEMORY_SLOT},
>                 {STATE_MEMORY_SLOT, STATE_MEMORY_SLOT}
>         },
>         .SpdDataLen = MAX_SPD_BYTES, //512
>         .SpdDataPtr = {
>                 {(void *)CONFIG_SPD_LOC, (void *)NULL},
>                 {(void *)NULL, (void *)NULL}
>         }
> };

Looks like the Harcuvar memory down code has never been tested,
there's no way it can work as-is. Moreover, documenting code changes
in comments is a terrible idea, because comments aren't compiled.

It's not a good idea to directly access files inside CBFS (for
example, spd.bin is in CBFS) via a hardcoded address (`CONFIG_SPD_LOC`
in this case), as it completely bypasses the CBFS API: nothing
guarantees that the expected data will always be at that address,
there's no way to know the size of the file at runtime and prevents
making use of CBFS security features such as file measurement and
TOCTOU safety (IIRC, it's still work in progress). The right way to
fetch the SPD data using the CBFS API is already implemented in
`src/mainboard/intel/harcuvar/spd/spd.c` function
`mainboard_find_spd_data()`, but the returned pointer is not used in
the code.

I just made https://review.coreboot.org/61341 to show how to do it The
Right Way. Let me know if the implementation from my change works for
you.
> Regards
>
> Ganeshc

Best regards,
Angel