[coreboot] Re: Memory Down approach Error on intel Denverton board
Hi Angel Pons,
Thanks for suggesting the changes . I have removed spd eeprom and verified the
changes . It is working .
Thank you all for the quick support .
Regards,
GaneshC
From: Ganesh Kumar C
Sent: Monday, January 24, 2022 6:13 PM
To: Angel Pons
Cc: Szafranski, MariuszX; Coreboot
Subject: Re: [coreboot] Re: Memory Down approach Error on intel Denverton board
Sure, i will try these changes and let you know shortly .
Thanks,
Ganesh
From: Angel Pons
Sent: Monday, January 24, 2022 4:09 PM
To: Ganesh Kumar C
Cc: Szafranski, MariuszX; Coreboot
Subject: Re: [coreboot] Re: Memory Down approach Error on intel Denverton board
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
CAUTION: This email originated from outside your organization. Do not click
links or open attachments unless you recognize the sender and know the content
is safe.
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org
[coreboot] Re: Memory Down approach Error on intel Denverton board
Sure, i will try these changes and let you know shortly .
Thanks,
Ganesh
From: Angel Pons
Sent: Monday, January 24, 2022 4:09 PM
To: Ganesh Kumar C
Cc: Szafranski, MariuszX; Coreboot
Subject: Re: [coreboot] Re: Memory Down approach Error on intel Denverton board
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
CAUTION: This email originated from outside your organization. Do not click
links or open attachments unless you recognize the sender and know the content
is safe.
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org
[coreboot] Re: Memory Down approach Error on intel Denverton board
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
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org
[coreboot] Re: Memory Down approach Error on intel Denverton board
Also double check if .SpdDataPtr structure member for memory down slot has correct pointer to spd.bin content. >> Yes , I have verified this part and also printed the spd values getting from spd.bin . ___ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-le...@coreboot.org
[coreboot] Re: Memory Down approach Error on intel Denverton board
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}
}
};
Regards
Ganeshc
From: Szafranski, MariuszX
Sent: Thursday, January 20, 2022 6:55 PM
To: Ganesh Kumar C; Coreboot
Subject: RE: Memory Down approach Error on intel Denverton board
Hi,
Did you adjusted mMemoryDownConfig structure in mainboard`s romstage.c file to
much your memory down configuration?
Refer to commented out example just above structure definition.
Also double check if .SpdDataPtr structure member for memory down slot has
correct pointer to spd.bin content.
Mariusz
--
Intel Research and Development Ireland Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263
This e-mail and any attachments may contain confidential material for the sole
use of the intended recipient(s). Any review or distribution by others is
strictly prohibited. If you are not the intended recipient, please contact the
sender and delete all copies.
CAUTION: This email originated from outside your organization. Do not click
links or open attachments unless you recognize the sender and know the content
is safe.
___
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org
[coreboot] Re: Memory Down approach Error on intel Denverton board
Hi, Did you adjusted mMemoryDownConfig structure in mainboard`s romstage.c file to much your memory down configuration? Refer to commented out example just above structure definition. Also double check if .SpdDataPtr structure member for memory down slot has correct pointer to spd.bin content. Mariusz -- Intel Research and Development Ireland Limited Registered in Ireland Registered Office: Collinstown Industrial Park, Leixlip, County Kildare Registered Number: 308263 This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies. ___ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-le...@coreboot.org
