On 14 February 2016 at 06:44, Yao, Jiewen <[email protected]> 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? >
I am running into a problem with these patches. According to the spec, each entry in the Memory Attributes table shall have the same type as the region it was carved out of in the UEFI memory map. However, when I dump the table from Linux, it looks something like UEFI memory map: 0x00013c0e0000-0x00013c15ffff [Runtime Code |RUN| | | | | ] 0x00013c160000-0x00013c17ffff [Runtime Data |RUN| | | | | ] 0x00013c1a0000-0x00013c1dffff [Runtime Code |RUN| | | | | ] 0x00013c1e0000-0x00013c27ffff [Runtime Data |RUN| | | | | ] 0x00013c280000-0x00013c2cffff [Runtime Code |RUN| | | | | ] 0x00013c2d0000-0x00013c31ffff [Runtime Data |RUN| | | | | ] 0x00013c320000-0x00013c36ffff [Runtime Code |RUN| | | | | ] 0x00013f650000-0x00013f6dffff [Runtime Code |RUN| | | | | ] 0x00013f6f0000-0x00013f80ffff [Runtime Data |RUN| | | | | ] Memory Attributes Table: 0x00013c0e0000-0x00013c0effff [Runtime Data |RUN| |XP| | | ] 0x00013c0f0000-0x00013c0fffff [Runtime Code |RUN| | | | |RO] 0x00013c100000-0x00013c12ffff [Runtime Data |RUN| |XP| | | ] 0x00013c130000-0x00013c13ffff [Runtime Code |RUN| | | | |RO] 0x00013c140000-0x00013c15ffff [Runtime Data |RUN| |XP| | | ] 0x00013c160000-0x00013c17ffff [Runtime Data |RUN| |XP| | | ] 0x00013c1a0000-0x00013c1affff [Runtime Data |RUN| |XP| | | ] 0x00013c1b0000-0x00013c1bffff [Runtime Code |RUN| | | | |RO] 0x00013c1c0000-0x00013c1dffff [Runtime Data |RUN| |XP| | | ] 0x00013c1e0000-0x00013c27ffff [Runtime Data |RUN| |XP| | | ] 0x00013c280000-0x00013c28ffff [Runtime Data |RUN| |XP| | | ] 0x00013c290000-0x00013c29ffff [Runtime Code |RUN| | | | |RO] 0x00013c2a0000-0x00013c2cffff [Runtime Data |RUN| |XP| | | ] 0x00013c2d0000-0x00013c31ffff [Runtime Data |RUN| |XP| | | ] 0x00013c320000-0x00013c32ffff [Runtime Data |RUN| |XP| | | ] 0x00013c330000-0x00013c33ffff [Runtime Code |RUN| | | | |RO] 0x00013c340000-0x00013c36ffff [Runtime Data |RUN| |XP| | | ] 0x00013f650000-0x00013f65ffff [Runtime Data |RUN| |XP| | | ] 0x00013f660000-0x00013f66ffff [Runtime Code |RUN| | | | |RO] 0x00013f670000-0x00013f69ffff [Runtime Data |RUN| |XP| | | ] 0x00013f6a0000-0x00013f6affff [Runtime Code |RUN| | | | |RO] 0x00013f6b0000-0x00013f6dffff [Runtime Data |RUN| |XP| | | ] 0x00013f6f0000-0x00013f80ffff [Runtime Data |RUN| |XP| | | ] So what you see here is that the entry types in the Memory Attribute table are set according to the RO/XP attribute, and not according to the original region in the UEFI memory map. In other words, what I would expect here is that the first five entries are all 'Runtime Code', with the same permissions as are listed currently. Literally, the spec describes this requirement as follows: """ Irrespective of the memory protections implied by Attribute, the EFI_MEMORY_DESCRIPTOR.Type field should match the type of the memory in enclosing SetMemoryMap() entry. """ > > 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:[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
