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. (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.) Would you like me to submit a patch for the dsc include file? Thanks Laszlo > Regards, > Ard. > > >> -----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
