Understood. I think the 4G for PEI x64 makes sense.
I checked OVMF code. In OvmfPkg\PlatformPei\Platform.c(44)
EFI_MEMORY_TYPE_INFORMATION mDefaultMemoryTypeInformation[] = {
{ EfiACPIMemoryNVS, 0x004 },
{ EfiACPIReclaimMemory, 0x008 },
{ EfiReservedMemoryType, 0x004 },
{ EfiRuntimeServicesData, 0x024 },
{ EfiRuntimeServicesCode, 0x030 },
{ EfiBootServicesCode, 0x180 },
{ EfiBootServicesData, 0xF00 },
{ EfiMaxMemoryType, 0x000 }
};
This is memory type information I mentioned. OVMF already has it.
(Although the table itself is wrong, the BootServicesCode and BootServicesData
should be removed.)
Also OvmfPkg\PlatformPei has AddUntestedMemoryBaseSizeHob (BASE_4GB,
UpperMemorySize);
It means above 4G is untested.
In conclusion, I am 100% sure that the ACPI allocation will still always be
Below4GB after we adopt the change, because that is controlled by DXE CORE.
(Unless there is bug, and we need fix the bug.)
Then I believe you will not see any problem on below configuration.
> Consider a platform with the following characteristics:
> - PEI is X64
> - DXE is X64
> - hence PcdDxeIplSwitchToLongMode is FALSE
> - PEI has page tables only for the first 4GB of RAM
> - S3 is supported
I agree PCD is good way for platform to configure core module behavior. And we
also try to avoid PCD abuse.
In this case, I think Ard gave reasonable justification for ACPI table
allocation on any memory (instead of below 4G) to avoid fragmentation. I
believe PCD is good way to resolve the problem, and I do not have any idea on
how to resolve it. That is why we choose PCD solution – PcdAcpiTableVersion.
4G x64 PEI can be resolved and is already resolved without a new PCD. So I do
not see the immediate need to have a new one.
I fully agree with you that any MdeModulePkg patch should still keep OVMF work.
That is Core’s responsibility to make compatible change.
If you find 4G X64 PEI S3 is broken later, please let me know. I will be happy
to discuss with you on the solution. For now, I believe it should work well.
Thank you
Yao Jiewen
From: edk2-devel [mailto:[email protected]] On Behalf Of Laszlo
Ersek
Sent: Tuesday, February 23, 2016 8:59 PM
To: Yao, Jiewen; Ard Biesheuvel
Cc: Ni, Ruiyu; Tian, Feng; [email protected]; Justen, Jordan L;
[email protected]; [email protected]; Gao, Liming; Fan, Jeff; Zeng,
Star
Subject: Re: [edk2] [PATCH v3 2/4] IntelFrameworkModulePkg: BdsDxe: only
allocate below 4 GB if needed
On 02/23/16 11:42, Yao, Jiewen wrote:
> Laszlo
>
> I do not understand why you only set 4G page table for X64 PEI.
Two reasons.
* First, OVMF's PEI has minimal memory needs. The permanent PEI RAM we install
it is normally capped at 64MB. If the guest has large RAM, then we install more
PEI RAM, but only so that the DXE IPL can build page tables for the entirety of
the guest RAM. (In other words, in such cases the permanent PEI RAM size is
dictated by / dominated by the DXE IPL's page table allocation.)
* Second, OVMF is special in that it doesn't need to configure DRAM hardware
before it becomes usable. Therefore only the reset vector and the SEC phase run
from flash. SEC decompresses both PEI and DXE firmware volumes into normal RAM,
and PEI runs from RAM.
In order to make this possible, originally OVMF's earliest page tables were
pre-built by the build process -- those page tables were statically embedded
into the read-only flash image, and SEC would only point CR3 at them. These
page tables mapped RAM up to 4GB.
Later this broke on some KVM versions: even if we pre-set the Accessed and
Dirty bits in the statically pre-built page table entries, the processor wanted
to rewrite those PTEs (in read-only flash) when accessing the mapped pages
themselves. Since the PTEs existed in read-only memory, this broke. (I don't
recall the exact symptoms, but it didn't work.)
So Jordan rewrote the creation of the earliest page tables, for the X64 SEC/PEI
case: it is now being done in the reset vector code. The reset vector code and
SEC continue to run from flash (located just below 4GB).
The reset vector creates the page tables (mapping memory up to 4GB)
dynamically, located in real RAM, at 8MB. (The size of the page tables is 24KB.)
The SEC phase is entered with paging enabled (using the tables created above),
and those same tables are used in PEI as well. (Until the DXE IPL maps the full
memory.)
> But if X64 PEI only fills 4G page table by default, you can setup page
> fault handler to dynamic fix the address.
>
> That is the way how we handle full memory access in memory constrain
> environment, like SMM.
I'm certain this is technically solvable, but let's make it clear that the
burden to make OVMF cope with such a change is not on us.
OvmfPkg lives in the upstream edk2 repository. In any open source project,
in-tree client code that depends on central infrastructure is not left behind
when the infrastructure changes, and the burden of adapting the client code to
the infrastructure is also not on the maintainers of the client code. Whoever
is updating the infrastructure is responsible for bringing forward all
dependent client code, with a bisectable series.
> Also, this platform can choose not report above 4G memory as tested
> for DxeCore
OVMF's PlatformPei already reports >=4GB memory as untested.
> and report big enough ACPI/Reserved memory in MemoryTypeInformation
> table.
How big is big enough?
> Then DxeCore will allocate BIN below 4G and all ACPI/Reserved memory
> allocation will happen below 4G.
Honestly, I find this way more complicated than a new PCD. A PCD is explicit
about its purpose.
Whereas your suggestion, if I understand it correctly, would require us to
measure actual memory usage for the Memory Type Information HOB, and keep it
up-to-date as development progresses, new drivers are pulled in, old drivers
are modified, and so on.
Also, the same OVMF binary is expected to run on QEMU when S3 support is
disabled (on the QEMU command line). In this case, some of the foundational
S3-related protocols are not installed, which prevents the rest of the
S3-related drivers from working. This is intentional, and it impacts the amount
of ACPI / Reserved memory needed.
> We do not want to introduce a new PCD, because it is unnecessary
> burden to maintain a new one, especially we already have solution to
> handle most cases.
The way I see it, support for a new platform family is being introduced.
A platform family that invalidates some of the key assumptions held (and
hard-coded!) in a number of core modules. I don't think a preexistent,
well-working platform with a non-negligible user base should be broken for the
newcomer's sake.
PCDs exist for platform configuration. That's what the name says:
Platform Configuration Database. If a new platform is being accommodated in the
core modules, I think it is quite natural to reckon with some additional
customization and maintenance.
For example, I would think the following library instances present some
additional maintenance burden too:
- MdePkg/Library/BaseCpuLib/AArch64
- MdePkg/Library/BaseLib/AArch64
- MdePkg/Library/BaseSynchronizationLib/AArch64
I don't debate your claim that a new PCD presents new maintenance burden
-- it does. I do question your claim that the burden is unnecessary.
I've put insane amounts of work into OVMF's S3. If someone submits bisectable,
regression-conscious patches for OVMF that adapt OVMF to the proposed
repurposing of PcdDxeIplSwitchToLongMode, I will do my best to test and review
those patches. If no such patches will be submitted, I will continue NACKing
the repurposing of PcdDxeIplSwitchToLongMode.
(Obviously, I'm also open to arguments that prove me wrong.)
Thanks
Laszlo
>
>
>
> Thank you
>
> Yao Jiewen
>
>
>
> *From:*Laszlo Ersek [mailto:[email protected]]
> *Sent:* Tuesday, February 23, 2016 5:04 PM
> *To:* Yao, Jiewen; Ard Biesheuvel
> *Cc:* Ni, Ruiyu; [email protected]<mailto:[email protected]>; Tian,
> Feng; Zeng, Star;
> [email protected]<mailto:[email protected]>;
> [email protected]<mailto:[email protected]>; Fan, Jeff; Gao,
> Liming
> *Subject:* Re: [edk2] [PATCH v3 2/4] IntelFrameworkModulePkg: BdsDxe:
> only allocate below 4 GB if needed
>
>
>
> On 02/23/16 05:31, Yao, Jiewen wrote:
>> I discussed with Module owner. Below is summary for MdeModulePkg:
>>
>> 1) PCD Name: We suggest to use PcdDxeIplSwitchToLongMode as indicator
>> that PEI is 32 bit and DXE is 64 bit for X86 platform. It might be
>> not clear stated in PCD description, but it is the intent when this
>> is introduced. We suggest to define this to be FALSE for ARM, so that
>> our core module can use it. (Confirmed with Liming Gao)
>>
>> 2) AcpiTableDxe: As Star Zeng mentioned, we need make sure FACS is
>> allocate below 4G when PcdDxeIplSwitchToLongMode is set, because it
>> is used by S3Resume.
>
> This is not good enough. PEI might be restricted to the first 4GB of
> memory *even though* it is built for X64.
>
> Consider a platform with the following characteristics:
> - PEI is X64
> - DXE is X64
> - hence PcdDxeIplSwitchToLongMode is FALSE
> - PEI has page tables only for the first 4GB of RAM
> - S3 is supported
>
> If AcpiTableDxe keyed off of PcdDxeIplSwitchToLongMode *solely*, in
> order to see whether the FACS needs to be allocated under 4GB, then
> said platform would break.
>
> The most frequently used build of OVMF is exactly like written above.
>
>>
>> 3) UefiBootManagerLib: As Ruiyu Ni mentioned, we need make allocate
>> L"PerfDataMemAddr" below 4G for when PcdDxeIplSwitchToLongMode is
>> set, because it is used by S3Resume.
>
> My counter-argument is the same as above, I think.
>
>>
>>
>>
>> 4) BootGraphicsResourceTableDxe: There is no need to add 4G
>> limitation, we can remove that directly. (Confirmed with Chao Zhang)
>>
>> 5) FirmwarePerformanceDataTableDxe: We can check
>> PcdDxeIplSwitchToLongMode to allocate Below4G memory for
>> mAcpiS3PerformanceTable which is used in S3.
>
> Same counter-argument again.
>
>> And we can remove 4G
>> limitation for mAcpiBootPerformanceTable directly. (Confirmed with
>> Chao
>> Zhang)
>>
>> 6) PiDxeS3BootScriptLib: We can check PcdDxeIplSwitchToLongMode for
>> S3TableBase and NewS3TableBase which is used in S3. (Confirmed with
>> Star
>> Zeng)
>
> Same counter-argument.
>
>>
>> 7) BootScriptExecutorDxe: We can check PcdDxeIplSwitchToLongMode for
>> FfsBuffer and BootScriptExecutorBuffer which is used in S3.
>> (Confirmed with Star Zeng)
>
> Same counter-argument.
>
>>
>> 8) CapsuleRuntimeDxe: Already check PcdDxeIplSwitchToLongMode.No
>> update needed. (Confirmed with Star Zeng)
>
> The only reason this doesn't break under OVMF is because OVMF doesn't
> implement capsule updates.
>
> In summary, PcdDxeIplSwitchToLongMode==TRUE implies, but is not
> equivalent to, the requirement to allocate under 4GB for S3 purposes.
> A platform may have *both* S3 *and* a 64-bit PEI phase without access
> to
>>= 4GB memory.
>
> That's why I proposed a brand new PCD. PcdDxeIplSwitchToLongMode would
> imply that PCD, but the new PCD wouldn't necessarily imply
> PcdDxeIplSwitchToLongMode.
>
>> BTW: We are trying not use IntelFrameworkModulePkg. If there is
>> replacement in MdeModulePkg, we suggest use the one in MdeModulePkg,
>> for example, BDS or ACPI.
>
> Absolutely, but migrating a platform from IntelFrameworkModulePkg BDS
> to MdeModulePkg BDS is not trivial, due to lack of documentation and
> examples; plus some functionality that is publicly available from
> GenericBdsLib (and is used by PlatformBdsLib instances) needs to be
> exported from the new boot manager library.
>
> Thanks
> Laszlo
>
>>
>>
>>
>> Thank you
>>
>> Yao Jiewen
>>
>>
>>
>>
>>
>> *From:*Ard Biesheuvel [mailto:[email protected]]
>> *Sent:* Monday, February 22, 2016 8:43 PM
>> *To:* Yao, Jiewen
>> *Cc:* Ni, Ruiyu; Laszlo Ersek;
>> [email protected]<mailto:[email protected]>
>> ; Tian, Feng; Zeng, Star;
>> [email protected]<mailto:[email protected]> ;
> [email protected]<mailto:[email protected]> ; Fan,
>> Jeff; Gao, Liming
>> *Subject:* Re: [edk2] [PATCH v3 2/4] IntelFrameworkModulePkg: BdsDxe:
>> only allocate below 4 GB if needed
>>
>>
>>
>> On 22 February 2016 at 13:40, Yao, Jiewen wrote:
>>> I did a search on current MdeModulePkg. I found there are more
>>> modules allocating Below4G memory.
>>>
>>> Besides BDS, we have BootGraphicsResourceTableDxe,
>>> BootScriptExecutorDxe, FirmwarePerformanceDataTableDxe,
>>> PiDxeS3BootScriptLib, CapsuleRuntimeDxe.
>>>
>>>
>>>
>>> BootGraphicsResourceTableDxe – I am not clear.
>>>
>>> CapsuleRuntimeDxe – Below4G is designed for PEI capsule. Or we do
>>> not need such capability.
>>>
>>> BootScriptExecutorDxe, FirmwarePerformanceDataTableDxe,
>>> PiDxeS3BootScriptLib
>>> - data need to be access in PEI phase.
>>>
>>>
>>>
>>> I think we might need more clean up for 32bit PEI.
>>>
>>
>> Agreed.
>>
>>> I am still thinking if we can reuse old Pcd – we can clarify in PCD
>>> description – it is only for 32bit PEI and 64bit DXE.
>>>
>>> Or a new PCD, and name to Pcd32BitPei? Please allow me collect more
>>> feedback on that.
>>>
>>
>> OK
>>
>>> In order to separate 32bit PEI issue from ACPI issue, I think we can
>>> check in ACPI patch at first, if you want.
>>>
>>
>> Should I wait for the maintainers of MdeModulePkg to give their
>> Reviewed-by ?
>>
>
_______________________________________________
edk2-devel mailing list
[email protected]<mailto:[email protected]>
https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
