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

[Simon Glass]

Hi Bin,

On 18 October 2015 at 20:44, Bin Meng <bmeng.cn at gmail.com> wrote:
> 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 it points into the part of CAR that is reserved for use by the MRC.

>
>>
>> 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