[coreboot-gerrit] Change in coreboot[master]: mrc_cache: Update mrc_cache data in romstage

[Shelley Chen (Code Review)]

Shelley Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44196 )

Change subject: mrc_cache: Update mrc_cache data in romstage
......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/c/coreboot/+/44196/7/src/drivers/mrc_cache/mrc_c... 
File src/drivers/mrc_cache/mrc_cache.c:

https://review.coreboot.org/c/coreboot/+/44196/7/src/drivers/mrc_cache/mrc_c... 
PS7, Line 668: #if CONFIG(MRC_WRITE_NV_LATE)
> Is there still a reason to differentiate this or can we just always call finalize in RESUME_CHECK? ( […]

Yes, it was discussed.  Here's your comment:

I would suggest organizing it like this:

mrc_cache_stash_data(type, version, data, size):
   if WRITE_NV_LATE:
     add stuff to cbmem for type
   else:
     update_mrc_cache_by_type(type, data, size)
 update_mrc_cache_from_cbmem(type):
   cr = lookup_region(type)
   entry = cbmem_entry_find(cr->cbmem_id)
   ...error handling as appropriate...
   update_mrc_cache_by_type(type, cbmem_entry_start(entry), cbmem_entry_size(entry))
 BOOT_STATE_INIT_ENTRY(finalize_mrc_cache)
 finalize_mrc_cache(void *unused):
   if WRITE_NV_LATE:
     update_mrc_cache_from_cbmem(MRC_TRAINING_DATA)
     if VARIABLE_DATA:
       update_mrc_cache_from_cbmem(MRC_VARIABLE_DATA)
   if CLEAR_ON_RETRAIN:
     invalidate_normal_cache()
   protect_mrc_region()
So basically, I would leave the BOOT_STATE callback in there for all configs, but it would only take the stash from CBMEM if WRITE_NV_LATE is set. In the other case all it does is handle the CLEAR_ON_RETRAIN and the protection stuff. The reason for that is that mrc_cache_stash_data() may be called multiple times in romstage (for multiple types of MRC cache), so you can't really be sure at what point you're done writing MRC caches and ready to enable the flash protection. Pushing that to ramstage is an easy solution (and shouldn't really cost anything... I'm just trying to get rid of all this copying through and reserving memory in CBMEM, having a boot stage callback that does basically nothing in ramstage doesn't really hurt).