On 01/11/14 08:48, Jordan Justen wrote:
> On Thu, Jan 9, 2014 at 10:57 AM, Laszlo Ersek <[email protected]> wrote:
>> comments below
>>
>> On 01/09/14 01:44, Jordan Justen wrote:
>>> @@ -33,26 +31,44 @@ PeiFvInitialization (
>>> VOID
>>> )
>>> {
>>> - DEBUG ((EFI_D_ERROR, "Platform PEI Firmware Volume Initialization\n"));
>>> + DEBUG ((EFI_D_INFO, "Platform PEI Firmware Volume Initialization\n"));
>>>
>>> - DEBUG (
>>> - (EFI_D_ERROR, "Firmware Volume HOB: 0x%x 0x%x\n",
>>> - PcdGet32 (PcdOvmfMemFvBase),
>>> - PcdGet32 (PcdOvmfMemFvSize)
>>> - )
>>> + //
>>> + // Create a memory allocation HOB for the PEI FV.
>>> + //
>>> + // This is marked as ACPI NVS so it will still be available on S3 resume.
>>> + //
>>> + BuildMemoryAllocationHob (
>>> + PcdGet32 (PcdOvmfPeiMemFvBase),
>>> + PcdGet32 (PcdOvmfPeiMemFvSize),
>>> + EfiACPIMemoryNVS
>>> );
>>>
>>> - BuildFvHob (PcdGet32 (PcdOvmfMemFvBase), PcdGet32 (PcdOvmfMemFvSize));
>>> + //
>>> + // Let DXE know about the DXE FV
>>> + //
>>> + BuildFvHob (PcdGet32 (PcdOvmfDxeMemFvBase), PcdGet32
>>> (PcdOvmfDxeMemFvSize));
>>>
>>> //
>>> - // Create a memory allocation HOB.
>>> + // Create a memory allocation HOB for the DXE FV.
>>> //
>>> BuildMemoryAllocationHob (
>>> - PcdGet32 (PcdOvmfMemFvBase),
>>> - PcdGet32 (PcdOvmfMemFvSize),
>>> + PcdGet32 (PcdOvmfDxeMemFvBase),
>>> + PcdGet32 (PcdOvmfDxeMemFvSize),
>>> EfiBootServicesData
>>> );
>>>
>>> + //
>>> + // Let PEI know about the DXE FV so it can find the DXE Core
>>> + //
>>> + PeiServicesInstallFvInfoPpi (
>>> + NULL,
>>> + (VOID *)(UINTN) PcdGet32 (PcdOvmfDxeMemFvBase),
>>> + PcdGet32 (PcdOvmfDxeMemFvSize),
>>> + NULL,
>>> + NULL
>>> + );
>>> +
>>
>> Hm. I wonder why we didn't need this before.
>>
>> - before: BuildFvHob() covering entire FV, no FvInfoPpi
>> - after: BuildFvHob() covering DXE, FvInfoPpi also covering DXE
>>
>> Can you provide the source code or PI spec reference requiring the
>> FvInfo PPI? I vaguely remembered (and now very superficially "confirmed"
>> by grepping) that FindNextCoreFvHandle() in
>> "MdeModulePkg/Core/Pei/FwVol/FwVol.c" calls
>> PeiServicesInstallFvInfoPpi(). The FV hob should suffice, no?
>
> I couldn't find anything spec wise to refer you to...
>
> PEI Core picks up the boot firmware volume sent in from SEC and will
> then find items from that FV. That is why we didn't need the FvInfo
> previously.
>
> For additional FVs, MdeModulePkg/Core/Pei/FwVol/FwVol.c seems to get
> notified when FvInfo is installed, and then discovers new FVs.
>
> If I enable gEfiMdeModulePkgTokenSpaceGuid.PcdFrameworkCompatibilitySupport,
> then the HOB seems to be enough. We don't want to enable this.
>
> So, PEI needs the BFV from SEC for the first PEI FV + FvInfo for
> additional PEI FVs. DXE needs the HOB.
>
> This hand-waving "explanation" enough for an r-b? :)
I found the description in the PI specification, volume 1 (Pre-EFI
Initialization Core Interface), 5.8 Dispatch Algorithmm, 5.8.1.2
Multiple Firmware Volume Support.
The algorithm specified there seems to match what you have derived from
the code and described above. So the FvInfo is justified.
However (according to the spec, I didn't check the code) BuildFvHob()
should be automatically called by the dispatch algorithm, when it sees
the FvInfo.
Also there seems to be a requirement to create a memory resource
descriptor covering the new firmware volume if the latter falls outside
of permanent PEI memory. Which it does in our case I think, because the
permanent memory is installed above
PcdOvmfDxeMemFvBase+PcdOvmfDxeMemFvSize on the normal path, and in
another distinct range on the S3 path.
Anyway this is very confusing and I won't claim I can see through it
100% :) I don't want to obsess about the FV HOBs: if we install the FV
HOB ourselves, and then the PEI dispatcher installs another identical
one in response to the FvInfo, that shouldn't hurt. After all it works
in practice.
So yes, the explanation does convince me that we need FvInfo, which was
my main question.
I'm OK with this patch if you address the other stuff you said you
would, plus I think I had one other comment below, about updating the
leading comment of DecompressMemFvs():
>>> diff --git a/OvmfPkg/Sec/SecMain.c b/OvmfPkg/Sec/SecMain.c
>>> index 902ac85..21c2a36 100644
>>> --- a/OvmfPkg/Sec/SecMain.c
>>> +++ b/OvmfPkg/Sec/SecMain.c
>>> @@ -319,8 +319,8 @@ FindFfsFileAndSection (
>>>
>>> **/
>>> EFI_STATUS
>>> -DecompressGuidedFv (
>>> - IN OUT EFI_FIRMWARE_VOLUME_HEADER **Fv
>>> +DecompressMemFvs (
>>> + IN OUT EFI_FIRMWARE_VOLUME_HEADER **PeiFv
>>> )
>>> {
>>> EFI_STATUS Status;
>>
>> The leading comment should be updated perhaps? On output, *PeiFv points
>> to the decompressed PEI PV (not main FV).
Thanks!
Laszlo
------------------------------------------------------------------------------
CenturyLink Cloud: The Leader in Enterprise Cloud Services.
Learn Why More Businesses Are Choosing CenturyLink Cloud For
Critical Workloads, Development Environments & Everything In Between.
Get a Quote or Start a Free Trial Today.
http://pubads.g.doubleclick.net/gampad/clk?id=119420431&iu=/4140/ostg.clktrk
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/edk2-devel
