Yes, thanks to point out this. Now I am thinking the problem becomes much broader. For example, do we need support a platform only set 2G page table in PEI phase? Or 1G? Or do we need support a platform only set 0~1G and 3G~4G page table? Or even more fragmented?
I do not think a generic core module can handle the case that a platform only set 0~1G and 3G~4G page table. There must be some rule pre-defined in specification, instead of using PCD. (Or we need a PCD to define the range?) PI specification might be a good place to clarify that rule. Please allow me to ask. Thank you Yao Jiewen From: edk2-devel [mailto:[email protected]] On Behalf Of Laszlo Ersek Sent: Tuesday, February 23, 2016 9:41 PM To: Ard Biesheuvel Cc: Ni, Ruiyu; Tian, Feng; [email protected]; Justen, Jordan L; [email protected]; [email protected]; Yao, Jiewen; 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 14:27, Ard Biesheuvel wrote: > On 23 February 2016 at 13:58, Laszlo Ersek wrote: >> On 02/23/16 11:42, Yao, Jiewen wrote: >>> Laszlo >>> >>> I do not understand why you only set 4G page table for X64 PEI. >> > > Actually, that question is not even relevant. The reality is simply > that the value of PcdDxeIplSwitchToLongMode is not sufficient to > decide whether PEI will have access to all of memory, and we need some > other way to convey the PEI memory limit. Note that, if OVMF > implements it that way, there may be other platforms in the wild that > do the same, so 'fixing' OVMF does not help us at all. Thanks for pointing this out. > So we are back to a PCD that sets a memory limit for PEI. I suppose > having a simple feature PCD should be sufficient here? Any suggestions > for names? PcdFavor64BitPeiMemory? Thanks Laszlo > > > > >> 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
