[U-Boot] [PATCH 1/3] x86: ivybridge: Fix saving mrc cache and enable it

[Bin Meng]

Hi Simon,

On Mon, Oct 19, 2015 at 4:27 AM, Simon Glass <sjg at chromium.org> wrote:
> Hi Bin,
>
> On 12 October 2015 at 02:30, Bin Meng <bmeng.cn at gmail.com> wrote:
>> Currently sdram_initialise() saves pei_data->mrc_output directly to
>> gd->arch.mrc_output. This is incorrect as pei_data->mrc_output points
>> to an address on the stack whose content is no longer valid when we
>> call mrccache_reserve(). To fix this, save it on the heap instead.
>>
>> Signed-off-by: Bin Meng <bmeng.cn at gmail.com>
>> ---
>>
>>  arch/x86/cpu/ivybridge/sdram.c | 20 ++++++++++----------
>>  1 file changed, 10 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/x86/cpu/ivybridge/sdram.c b/arch/x86/cpu/ivybridge/sdram.c
>> index fc66a3c..f3d97ca 100644
>> --- a/arch/x86/cpu/ivybridge/sdram.c
>> +++ b/arch/x86/cpu/ivybridge/sdram.c
>> @@ -151,14 +151,8 @@ static int prepare_mrc_cache(struct pei_data *pei_data)
>>         if (!mrc_cache)
>>                 return -ENOENT;
>>
>> -       /*
>> -        * TODO(sjg at chromium.org): Skip this for now as it causes boot
>> -        * problems
>> -        */
>> -       if (0) {
>> -               pei_data->mrc_input = mrc_cache->data;
>> -               pei_data->mrc_input_len = mrc_cache->data_size;
>> -       }
>> +       pei_data->mrc_input = mrc_cache->data;
>> +       pei_data->mrc_input_len = mrc_cache->data_size;
>>         debug("%s: at %p, size %x checksum %04x\n", __func__,
>>               pei_data->mrc_input, pei_data->mrc_input_len,
>>               mrc_cache->checksum);
>> @@ -289,6 +283,7 @@ int sdram_initialise(struct pei_data *pei_data)
>>         unsigned version;
>>         const char *data;
>>         uint16_t done;
>> +       char *cache;
>>         int ret;
>>
>>         report_platform_info();
>> @@ -386,8 +381,13 @@ int sdram_initialise(struct pei_data *pei_data)
>>                  * This will be copied to SDRAM in reserve_arch(), then written
>>                  * to SPI flash in mrccache_save()
>>                  */
>> -               gd->arch.mrc_output = (char *)pei_data->mrc_output;
>> -               gd->arch.mrc_output_len = pei_data->mrc_output_len;
>> +               cache = malloc(pei_data->mrc_output_len);
>> +               if (cache) {
>> +                       memcpy(cache, pei_data->mrc_output,
>> +                              pei_data->mrc_output_len);
>> +                       gd->arch.mrc_output = cache;
>> +                       gd->arch.mrc_output_len = pei_data->mrc_output_len;
>> +               }
>
> This isn't really any better than what is there. The malloc() region
> is in CAR memory, just a different part of it. The function
> reserve_arch() copies it to SDRAM.

So where does this pei_data->mrc_input point to? Is it some place that
is malloced by the MRC itself and in a place that does not get
overwritten?

>
> I think with FSP this does not work but for ivybridge it seems OK.
>
> I'll resend your patch with this part removed. Your comments spurred
> me to take another look at why MRC was broken on ivybridge, and I
> found that car_uninit() was wrong. I'll send a series to fix it.
>
>>                 ret = write_seeds_to_cmos(pei_data);
>>                 if (ret)
>>                         debug("Failed to write seeds to CMOS: %d\n", ret);
>> --
>> 1.8.2.1
>>
>
> Regards,
> Simon

Regards,
Bin