On 14 February 2016 at 07:51, Laszlo Ersek <[email protected]> wrote: > 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). >
I disagree. Not only was the PropertiesTable superseded by the MemoryAttributes table in 2.6, the recommendation not to use the PropertiesTable has also been added to 2.5 errata A, while it was originally defined in 2.5 This means effectively that it has been retracted, and so it does not matter if you claim to implement UEFI 2.4, 2.5 or 2.6, in none of these cases should the PropertiesTable be published. The reason that the definition remains in the text is for the OS side, not for the reference implementation of the firmware side, which should steer clear from it entirely. -- Ard. >> -----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
