On 19 February 2016 at 01:43, Yao, Jiewen <jiewen....@intel.com> wrote: > Thanks. It seems my misunderstanding. > > Just want to double confirm: It is expected to see memory attribute table > like below. Right? >
Correct. > 0x00013c0e0000-0x00013c0effff [Runtime Code |RUN| |XP| | | ] > 0x00013c0f0000-0x00013c0fffff [Runtime Code |RUN| | | | |RO] > 0x00013c100000-0x00013c12ffff [Runtime Code |RUN| |XP| | | ] > 0x00013c130000-0x00013c13ffff [Runtime Code |RUN| | | | |RO] > 0x00013c140000-0x00013c15ffff [Runtime Code |RUN| |XP| | | ] > > 0x00013c160000-0x00013c17ffff [Runtime Data |RUN| |XP| | | ] > > 0x00013c1a0000-0x00013c1affff [Runtime Code |RUN| |XP| | | ] > 0x00013c1b0000-0x00013c1bffff [Runtime Code |RUN| | | | |RO] > 0x00013c1c0000-0x00013c1dffff [Runtime Code |RUN| |XP| | | ] > > 0x00013c1e0000-0x00013c27ffff [Runtime Data |RUN| |XP| | | ] > > 0x00013c280000-0x00013c28ffff [Runtime Code |RUN| |XP| | | ] > 0x00013c290000-0x00013c29ffff [Runtime Code |RUN| | | | |RO] > 0x00013c2a0000-0x00013c2cffff [Runtime Code |RUN| |XP| | | ] > > 0x00013c2d0000-0x00013c31ffff [Runtime Data |RUN| |XP| | | ] > > 0x00013c320000-0x00013c32ffff [Runtime Code |RUN| |XP| | | ] > 0x00013c330000-0x00013c33ffff [Runtime Code |RUN| | | | |RO] > 0x00013c340000-0x00013c36ffff [Runtime Code |RUN| |XP| | | ] > > 0x00013f650000-0x00013f65ffff [Runtime Code |RUN| |XP| | | ] > 0x00013f660000-0x00013f66ffff [Runtime Code |RUN| | | | |RO] > 0x00013f670000-0x00013f69ffff [Runtime Code |RUN| |XP| | | ] > 0x00013f6a0000-0x00013f6affff [Runtime Code |RUN| | | | |RO] > 0x00013f6b0000-0x00013f6dffff [Runtime Code |RUN| |XP| | | ] > > 0x00013f6f0000-0x00013f80ffff [Runtime Data |RUN| |XP| | | ] > > > > -----Original Message----- > From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org] > Sent: Friday, February 19, 2016 1:44 AM > To: Yao, Jiewen > Cc: Laszlo Ersek; edk2-de...@ml01.01.org; Gao, Liming > Subject: Re: [edk2] [patch 0/7] Add UEFI2.6 MemoryAttributesTable support. > > On 14 February 2016 at 06:44, Yao, Jiewen <jiewen....@intel.com> 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:ard.biesheu...@linaro.org] >> Sent: Monday, February 01, 2016 2:28 PM >> To: Laszlo Ersek >> Cc: Yao, Jiewen; edk2-de...@ml01.01.org; Gao, Liming >> Subject: Re: [edk2] [patch 0/7] Add UEFI2.6 MemoryAttributesTable support. >> >> On 31 January 2016 at 21:36, Laszlo Ersek <ler...@redhat.com> wrote: >>> 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. >>> >> >> 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: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
