On 02/14/16 06:44, Yao, Jiewen wrote: > 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?
Not from my side, thanks. > 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. I agree the implementation should follow the spec -- at least offer the feature as long as the spec defines it. My vote would be (2). Thanks Laszlo > > Thank you > Yao Jiewen > > -----Original Message----- > From: Ard Biesheuvel [mailto:[email protected]] > Sent: Monday, February 01, 2016 2:28 PM > To: Laszlo Ersek > Cc: Yao, Jiewen; [email protected]; Gao, Liming > Subject: Re: [edk2] [patch 0/7] Add UEFI2.6 MemoryAttributesTable support. > > On 31 January 2016 at 21:36, Laszlo Ersek <[email protected]> wrote: >> On 01/30/16 11:25, Ard Biesheuvel wrote: >>> On 30 January 2016 at 04:17, Yao, Jiewen <[email protected]> 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:[email protected]] >>>> Sent: Saturday, January 30, 2016 10:24 AM >>>> To: Yao, Jiewen >>>> Cc: [email protected]; 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:[email protected]] >>>>> Sent: Saturday, January 30, 2016 1:13 AM >>>>> To: Yao, Jiewen >>>>> Cc: [email protected]; 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" <[email protected]> >>>>>> Cc: "Gao, Liming" <[email protected]> >>>>>> >>>>>> 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 >>>> [email protected] >>>> https://lists.01.org/mailman/listinfo/edk2-devel >> _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
