[coreboot-gerrit] Change in coreboot[master]: drivers/intel/fsp2_0: Change boot state notification

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43979 )

Change subject: drivers/intel/fsp2_0: Change boot state notification
......................................................................

Patch Set 4:
> Patch Set 4:
> > Patch Set 4:
> > > Patch Set 4:
> > > > Patch Set 4:
> > > >
> > > > (2 comments)
> > > >
> > > > Does anyone know why we load the payload into memory and then notify FSP? Doesn't it make sense to:
> > > > notify FSP of boot state init
> > > > load payload into memory
> > > > run payload
> > >
> > > we have 2 FSP callback
> > > Post PCI enumeration
> > > Ready to Boot
> > >
> > > I guess the purpose of NotifyPhaseApi() - Begin  [Phase: 00000040] 
> > > FSP Ready To Boot ... event to ensure we have a done with all operation and bootloader is ready to handoff to payload/os
> >
> > Subrata,
> > Is it possible for FSP to overwrite memory where the payload is loaded? That's what I'm seeing in some cases. Hence, I wanted to change the order in which FSP is being notified just prior to the payload being loaded.
>
> I still suspect that FSP might be outputting multiple reserved memory HOBs, and we only account for the first one.

+1. Either that or FSP actually has a bug where it is writing to memory it does not reserve. Both are bad since it can lead to OS memory corruption as well in case of S3 resume. You can enable CONFIG_DISPLAY_HOBS to check the HOBs that are provided by FSP. From FSP spec, any memory reserved by FSP would be reported as FSP_RESERVED_MEMORY_RESOURCE_HOB.