[coreboot] CBMEM Console corruption with fsp_baytrail in coreboot 4.2 (fixed)

[Ben Gardner]

Hi all,

A fix to the timestamp/console issue is available here:
http://review.coreboot.org/#/c/12511/

Would you mind taking a look?
Thanks
Ben

On Sat, Nov 21, 2015 at 10:14 PM, Ben Gardner <gardner.ben at gmail.com> wrote:
> I think I found the issue, but won't be able to test for another week.
>
> car_get_var_ptr() assumes that the migrated base is the same as _car_data_start.
> On FSP 1.0, that isn't true.  _car_data_start is 0x0c00 bytes past
> migrated base.
>
> The last two lines of car_get_var_ptr() read as follows:
>    offset = (char *)var - (char *)_car_data_start;
>    return &migrated_base[offset];
>
> However, migrated base matches 0xfef00000 (DCACHE_RAM_BASE), while
> _car_data_start is 0xfef00c00.
> So, when the data is migrated, the migrated offset is wrong and
> cbmem_console_p is fairly random.
>
> It worked when I moved _preram_cbmem_console because that put
> _car_data_start at 0xfef00000 again.
> A simple fix would be to use CONFIG_DCACHE_RAM_BASE instead of
> _car_data_start when calculating the base.
>
> I think we may also need Aaron's earlier patch proposed in this
> thread, as _preram_cbmem_console is not between _car_data_start and
> _car_data_end.
>
> The fix might be something like the following: (gmail ate my tabs).
>
> What do you think?
>
> --- a/src/cpu/x86/car.c
> +++ b/src/cpu/x86/car.c
> @@ -63,15 +63,15 @@ void *car_get_var_ptr(void *var)
>  #if IS_ENABLED(CONFIG_PLATFORM_USES_FSP1_0)
>         migrated_base=(char *)find_saved_temp_mem(
>                         *(void **)CBMEM_FSP_HOB_PTR);
> +       offset = (char *)var - (char *)CONFIG_DCACHE_RAM_BASE;
>  #else
>         migrated_base = cbmem_find(CBMEM_ID_CAR_GLOBALS);
> +       offset = (char *)var - (char *)_car_start;
>  #endif
>
>         if (migrated_base == NULL)
>                 die( "CAR: Could not find migration base!\n");
>
> -       offset = (char *)var - (char *)_car_start;
> -
>         return &migrated_base[offset];
>  }
>
>
> On Fri, Nov 20, 2015 at 5:11 PM, Ben Gardner <gardner.ben at gmail.com> wrote:
>> On Fri, Nov 20, 2015 at 4:56 PM, Ben Gardner <gardner.ben at gmail.com> wrote:
>>> Ug. I was looking at the wrong log. Time for a break.
>>>
>>> The output from the log was:
>>> Stack: fef03fc4 or fef03fcc
>>>
>>> That seems to match the settings:
>>> CONFIG_DCACHE_RAM_BASE=0xfef00000
>>> CONFIG_DCACHE_RAM_SIZE=0x4000
>>
>> For what it is worth, I verified that ECX = 0xfef00000 and EDX =
>> 0xfef04000 when the FSP's TempRamIinitEntry() function returns.
>> The CAR global data makes more sense now, as that is mapped to the
>> bottom of the temporary stack area.
>> The CAR data location is correct and the stack data does indeed appear
>> to have been copied correctly to 0x7ae230a0.
>> I think I'm looking in the wrong area.
>>
>> Oh, well.  I'll look at it again after the Thanksgiving holiday.
>>
>> Ben