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

[Bin Meng]

Hi Simon,

On Mon, Oct 19, 2015 at 10:51 AM, Simon Glass <sjg at chromium.org> wrote:
> 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.
>

Thanks, this explains why malloc() is not needed, since it is reserved
and not the same stack that U-Boot uses in the CAR.

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