HI Thanks to discuss this properties table issue. The purpose of this patch is to *add* UEFI2.6 memory attributes table. This patch does not handle any UEFI2.5 properties table. If we want to change EDKII code for properties table, I suggest we separate it from this patch.
Is there any comment for adding UEFI2.6 memory attributes table? For UEFI2.5 properties table, let me summarize. Currently, we have several options: 0) Keep it as is. Current setting is PcdPropertiesTableEnable TRUE in MdeModulePkg.DEC. 1) Update PcdPropertiesTableEnable to FALSE in XXXPlatform.DSC file. (Platform choice. It can be static PCD or dynamic PCD.) 2) Update PcdPropertiesTableEnable to FALSE in MdeModulePkg.DEC file. (Disable Default. BIOS will not publish this table by default. If platform wants this table, it can set PCD to true.) 3) Remove PcdPropertiesTableEnable, and remove PropertiesTable support code in DxeCore. (PropertiesTable support is removed.) Personally, I do not suggest we choose 3), because this table is still defined in UEFI specification. We can remove it after UEFI spec removes it later. I do not have strong opinion for other options. Thank you Yao Jiewen -----Original Message----- From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org] Sent: Monday, February 01, 2016 2:28 PM To: Laszlo Ersek Cc: Yao, Jiewen; edk2-de...@ml01.01.org; Gao, Liming Subject: Re: [edk2] [patch 0/7] Add UEFI2.6 MemoryAttributesTable support. On 31 January 2016 at 21:36, Laszlo Ersek <ler...@redhat.com> wrote: > On 01/30/16 11:25, Ard Biesheuvel wrote: >> On 30 January 2016 at 04:17, Yao, Jiewen <jiewen....@intel.com> wrote: >>> Thanks for the clarification. I think you are right. >>> >>> I said "This table is used to retire old PropertiesTable", because UEFI2.6 >>> does not recommend using PropertiesTable to report RT information. >>> The future BIOS/OS should always produce/consume MemoryAttributesTable as >>> better solution. >>> >> >> I strongly feel we should remove the original PropertiesTable. It >> breaks backward compatibility, and was replaced by the >> MemoryAttributesTable for a good reason. The OS side will not be >> implemented in Linux (i.e., it will ignore the RO and XP attributes >> in the standard UEFI memory map), and the only reason the >> PropertiesTable was not removed entirely from the specification is >> because of business interests of one of the participating OS vendors. >> >> I understand that we need to keep the underlying machinery to produce >> either table. But we should remove the functionality that results in >> each PE/COFF image to be split into several regions marked RO or XP >> in the ordinary UEFI memory map. > > We can mitigate this by setting PcdPropertiesTableEnable to FALSE in > "ArmVirt.dsc.inc" as well. > No, we should remove it. > (Hmmm, I forget why SVN r18140 and r18566 don't already result in a > problematic memmap for Linux... Probably due to your kernel commit > 0ce3cc008ec0.) > It will result in a problematic memmap for older Linux kernels but also for less recent versions of Windows 8, for instance. And the only OS version that actually consumes the properties table is Windows 10, which will we updated in the future to use the MemoryAttributesTable instead. >>> -----Original Message----- >>> From: Laszlo Ersek [mailto:ler...@redhat.com] >>> Sent: Saturday, January 30, 2016 10:24 AM >>> To: Yao, Jiewen >>> Cc: edk2-de...@ml01.01.org; Gao, Liming >>> Subject: Re: [edk2] [patch 0/7] Add UEFI2.6 MemoryAttributesTable support. >>> >>> On 01/30/16 01:31, Yao, Jiewen wrote: >>>> Comment below: >>>> >>>> -----Original Message----- >>>> From: Laszlo Ersek [mailto:ler...@redhat.com] >>>> Sent: Saturday, January 30, 2016 1:13 AM >>>> To: Yao, Jiewen >>>> Cc: edk2-de...@ml01.01.org; Gao, Liming >>>> Subject: Re: [edk2] [patch 0/7] Add UEFI2.6 MemoryAttributesTable support. >>>> >>>> Hello Jiewen, >>>> >>>> On 01/29/16 13:12, jiewen yao wrote: >>>>> This series patches add UEFI2.6 MemoryAttributesTable support. >>>>> This table is used to retire old PropertiesTable. >>>>> This is standalone table published by DxeCore, so there is no >>>>> compatibility issue. >>>>> >>>>> The patch is validated with or without properties table. >>>> >>>> Do you mean, tested with PcdPropertiesTableEnable set to FALSE vs. TRUE? >>>> [Jiewen] Yes, you are right. No matter PcdPropertiestableEnable set to >>>> FALSE or TRUE, MemoryAttributesTable is always published. >>>> That is one compatibility we want to maintain. >>>> >>>> >>>> I skimmed the commit messages, and it looks like the properties table is >>>> preserved, but now it serves only as foundation for the memory attributes >>>> table. >>>> [Jiewen] Yes, the DxeCore global variable - properties table is always >>>> preserved. And the system UEFI configuration table for properties table is >>>> controlled by PcdPropertiestableEnable. >>>> No matter properties table or memory attributes table, we always need a >>>> flag to record if RT image is 4K aligned or not, so I just choose the >>>> original global variable. >>>> >>>> >>>> In OVMF we set PcdPropertiesTableEnable to FALSE by default, but flip it >>>> dynamically to TRUE in PlatformPei, if the user requests the feature on >>>> the QEMU command line. This is being done for avoiding regressions with >>>> OSes that don't know about the properties table, and its impact on the >>>> UEFI memmap. >>>> [Jiewen] That is good design. I believe most real platforms do similar >>>> thing. >>>> >>>> >>>> However, if the memory attributes table is safe for OSes that don't >>>> know about it, I think we can eliminate the above "FALSE" default, >>>> and dynamism, from OVMF, and just inherit the >>>> PcdPropertiesTableEnable=TRUE setting from "MdeModulePkg.dec". >>>> [Jiewen] I am sorry that I am not clear on your suggestion. What do you >>>> mean "eliminate the above "FALSE" default, and dynamism, from OVMF"? >>>> Do you mean you suggest to change PcdPropertiesTableEnable=FALSE as >>>> default value in MdeModulePkg.dec file? >>>> Or do you want to change OVMF.dsc file? >>> >>> Sorry, I should have educated myself on the memory attributes table first, >>> in UEFI 2.6. I have now carefully read the EFI_MEMORY_ATTRIBUTES_TABLE >>> section, and I've also noticed the new last paragraph in the >>> EFI_PROPERTIES_TABLE section. >>> >>> What I was missing when I asked my question is that >>> EFI_MEMORY_ATTRIBUTES_TABLE was an *additional* feature. >>> >>> Your cover letter said "This table is used to retire old PropertiesTable". >>> So I thought, the OS-visible properties table would be completely removed >>> from edk2, and the DXE core internals would only be reused for computing >>> the new memory attributes table. In other words, I thought that the memory >>> attributes table would *replace* the properties table, and the original >>> PcdPropertiesTableEnable PCD would now control the presence of the memory >>> attributes table instead. >>> >>> This is why I asked if we should change OVMF to remove its FALSE default >>> for the PCD, alongside the possibility for the QEMU user to change the PCD >>> dynamically, on the command line. Because, the memory attributes table >>> would *always* be safe to install, so we should just stick, in OVMF, with >>> the TRUE default for the PCD, from MdeModulePkg.dec. >>> >>> I do understand now that the memory attributes table is an additional >>> feature. It will always be exposed to the OS. *Independently*, the >>> properties table will continue to behave the same as before -- if the PCD >>> is TRUE, then it will be installed; if the PCD is FALSE, then it won't. >>> >>> In the end, there's nothing to do under OvmfPkg/. We'll let the QEMU user >>> control the properties table same as before (defaulting to FALSE), and >>> orthogonally, the new memory attributes table will always be installed >>> (with your patches in place), because it's safe. >>> >>> Thanks, and sorry about the confusion. >>> Laszlo >>> >>>> >>>> >>>> >>>> Does it sound reasonable to you? >>>> >>>> Thanks! >>>> Laszlo >>>> >>>>> >>>>> Contributed-under: TianoCore Contribution Agreement 1.0 >>>>> Signed-off-by: "Yao, Jiewen" <jiewen....@intel.com> >>>>> Cc: "Gao, Liming" <liming....@intel.com> >>>>> >>>>> jiewen yao (7): >>>>> MdePkg: Add UEFI2.6 MemoryAttributes Table definition. >>>>> MdePkg: Add UEFI2.6 MemoryAttributesTable GUID >>>>> MdeModulePkg: Add MemoryAttributesTable generation. >>>>> MdeModulePkg: Update PropertiesTable for MemoryAttributesTable. >>>>> MdeModulePkg: Add CoreInitializeMemoryAttributesTable() to header >>>>> file. >>>>> MdePkg: Call CoreInitializeMemoryAttributesTable() in DXE Entrypoint. >>>>> MdePkg: Update DxeCore INF for MemoryAttributesTable. >>>>> >>>>> MdeModulePkg/Core/Dxe/DxeMain.h | 11 +- >>>>> MdeModulePkg/Core/Dxe/DxeMain.inf | 4 +- >>>>> MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c | 3 +- >>>>> MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c | 214 >>>>> +++++++++++++++++++++ >>>>> MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c | 100 +++++++++- >>>>> MdePkg/Include/Guid/MemoryAttributesTable.h | 34 ++++ >>>>> MdePkg/MdePkg.dec | 11 +- >>>>> 7 files changed, 362 insertions(+), 15 deletions(-) create mode >>>>> 100644 MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c >>>>> create mode 100644 MdePkg/Include/Guid/MemoryAttributesTable.h >>>>> >>>> >>> >>> _______________________________________________ >>> edk2-devel mailing list >>> edk2-devel@lists.01.org >>> https://lists.01.org/mailman/listinfo/edk2-devel > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
