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
