Re: [edk2-devel] Questions about UEFI MAT / PcdPropertiesTableEnable

["Laszlo Ersek" <lersek@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3462 bytes --]

On 03/25/20 06:17, Ni, Ray wrote:
>>
>> The properties table should not be used. It has been superseded by the memory attributes table, per spec.
>>
>> In edk2, the properties table is controlled by the PCD, regardless of the memory attributes table.
>>
>> In edk2, the memory attributes table is always produced, regardless of the properties table.
>>
>> Please see the discussion under:
>>
>> [edk2] [patch 0/7] Add UEFI2.6 MemoryAttributesTable support.
>> http://mid.mail-archive.com/1454069539-4056-1-git-send-email-jiewen.yao@intel.com
>>
> 
> Laszlo,
> I cannot open the URL above.

I'm really sorry about that!

Unfortunately, the discussion thread linked above goes back to January /
February 2016.

At that time, we were using <edk2-devel@lists.01.org> for the mailing
list address. And *normally*, you would expect the mailing list archive
at 01.org to contain the discussion, right?

Except, a few months ago, the 01.org admins replaced the Mailman2 list
software with Mailman3 / HyperKitty, on 01.org. As a result, the old
list archive links into 01.org are all broken. :(

However, that's still not the worst. Because the old 01.org archive is
still exposed to the world, only through a new User Interface. And the
new User Interface (called HyperKitty) is still searchable -- so if you
know at least the subject line of a message or thread, you can still
find it, here:

  https://lists.01.org/hyperkitty/list/edk2-devel@lists.01.org/

But then, what *is* the worst, is that the new Mailman3 / HyperKitty
archive does not go back far enough in time. In other words, when the
01.org admins replaced Mailman2 with Mailman3 on 01.org, they didn't
import *all* of the old archive. If you click the link above and select
2016 in the left sidebar, you can see the new archive only goes back to
July 2016. And the subject discussion occurred in Jan/Feb 2016.

This is why I *absolutely* insist on keeping our discussions on mailing
lists, because mailing lists can be federated. If we hadn't had
<mail-archive.com> subscribe to the original 01.org-based edk2-devel
mailing list, then now we wouldn't have *any* public record whatsoever,
of the thread in question!

In other words, I'm just explaining to you why the *only* link I have
provided here is for <mail-archive.com> (which you regrettably cannot
open, from behind the Great Firewall). The reason is that there *is* no
other link. Otherwise, if there are multiple links, I always provide at
least two -- one into the official archive (which in this case would be
01.org), and another into a secondary archive (such as mail-archive.com).

So... in this reply, let me attach the relevant part of the 2016
discussion for you (and others that cannot read <mail-archive.com>) --
18 messages.

> Do you think we could remove properties table?

Yes, that's exactly what Ard requested, as soon as Jiewen posted the MAT
series. Back then, Jiewen said that some production OSes were still
using the properties table, and would need time to migrate to MAT.

The agreement -- four years ago! -- seemed to be that the UEFI spec
should drop the properties table definition in some time, and then edk2
could remove the reference implementation too.

See the attached discussion.

Given that the properties table had been deprecated in the UEFI spec
even in Feb 2016, I think it's now high time to remove it altogether
(both spec and edk2).

> The existence of both is confusing.

Yes, very much.

Thanks
Laszlo

[-- Attachment #2: Attached Message --]
[-- Type: message/rfc822, Size: 4667 bytes --]

From: jiewen yao <jiewen.yao@intel.com>
To: edk2-devel@ml01.01.org
Cc: "Yao, Jiewen" <jiewen.yao@intel.com>, "Gao, Liming" <liming.gao@intel.com>
Subject: [edk2] [patch 0/7] Add UEFI2.6 MemoryAttributesTable support.
Date: Fri, 29 Jan 2016 20:12:12 +0800
Message-ID: <1454069539-4056-1-git-send-email-jiewen.yao@intel.com>

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.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: "Yao, Jiewen" <jiewen.yao@intel.com>
Cc: "Gao, Liming" <liming.gao@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

-- 
1.9.5.msysgit.0

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[-- Attachment #3: Attached Message --]
[-- Type: message/rfc822, Size: 6015 bytes --]

From: Laszlo Ersek <lersek@redhat.com>
To: jiewen yao <jiewen.yao@intel.com>
Cc: edk2-devel@ml01.01.org, "Gao, Liming" <liming.gao@intel.com>
Subject: Re: [edk2] [patch 0/7] Add UEFI2.6 MemoryAttributesTable support.
Date: Fri, 29 Jan 2016 18:12:53 +0100
Message-ID: <56AB9D95.3070402@redhat.com>

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?

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.

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.

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".

Does it sound reasonable to you?

Thanks!
Laszlo

> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: "Yao, Jiewen" <jiewen.yao@intel.com>
> Cc: "Gao, Liming" <liming.gao@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

[-- Attachment #4: Attached Message --]
[-- Type: message/rfc822, Size: 8199 bytes --]

From: "Yao, Jiewen" <jiewen.yao@intel.com>
To: Laszlo Ersek <lersek@redhat.com>
Cc: "edk2-devel@ml01.01.org" <edk2-devel@ml01.01.org>, "Gao, Liming" <liming.gao@intel.com>
Subject: Re: [edk2] [patch 0/7] Add UEFI2.6 MemoryAttributesTable support.
Date: Sat, 30 Jan 2016 00:31:42 +0000
Message-ID: <74D8A39837DF1E4DA445A8C0B3885C500275227B@shsmsx102.ccr.corp.intel.com>

Comment below:

-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Saturday, January 30, 2016 1:13 AM
To: Yao, Jiewen
Cc: edk2-devel@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?



Does it sound reasonable to you?

Thanks!
Laszlo

> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: "Yao, Jiewen" <jiewen.yao@intel.com>
> Cc: "Gao, Liming" <liming.gao@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

[-- Attachment #5: Attached Message --]
[-- Type: message/rfc822, Size: 9226 bytes --]

From: Laszlo Ersek <lersek@redhat.com>
To: "Yao, Jiewen" <jiewen.yao@intel.com>
Cc: "edk2-devel@ml01.01.org" <edk2-devel@ml01.01.org>, "Gao, Liming" <liming.gao@intel.com>
Subject: Re: [edk2] [patch 0/7] Add UEFI2.6 MemoryAttributesTable support.
Date: Sat, 30 Jan 2016 03:24:02 +0100
Message-ID: <56AC1EC2.4020700@redhat.com>

On 01/30/16 01:31, Yao, Jiewen wrote:
> Comment below:
> 
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com] 
> Sent: Saturday, January 30, 2016 1:13 AM
> To: Yao, Jiewen
> Cc: edk2-devel@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.yao@intel.com>
>> Cc: "Gao, Liming" <liming.gao@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

[-- Attachment #6: Attached Message --]
[-- Type: message/rfc822, Size: 10603 bytes --]

From: "Yao, Jiewen" <jiewen.yao@intel.com>
To: Laszlo Ersek <lersek@redhat.com>
Cc: "edk2-devel@ml01.01.org" <edk2-devel@ml01.01.org>, "Gao, Liming" <liming.gao@intel.com>
Subject: Re: [edk2] [patch 0/7] Add UEFI2.6 MemoryAttributesTable support.
Date: Sat, 30 Jan 2016 03:17:22 +0000
Message-ID: <74D8A39837DF1E4DA445A8C0B3885C5002752311@shsmsx102.ccr.corp.intel.com>

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.

Thank you
Yao Jiewen


-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Saturday, January 30, 2016 10:24 AM
To: Yao, Jiewen
Cc: edk2-devel@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:lersek@redhat.com]
> Sent: Saturday, January 30, 2016 1:13 AM
> To: Yao, Jiewen
> Cc: edk2-devel@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.yao@intel.com>
>> Cc: "Gao, Liming" <liming.gao@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

[-- Attachment #7: Attached Message --]
[-- Type: message/rfc822, Size: 12005 bytes --]

From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
To: "Yao, Jiewen" <jiewen.yao@intel.com>
Cc: "edk2-devel@ml01.01.org" <edk2-devel@ml01.01.org>, Laszlo Ersek <lersek@redhat.com>, "Gao, Liming" <liming.gao@intel.com>
Subject: Re: [edk2] [patch 0/7] Add UEFI2.6 MemoryAttributesTable support.
Date: Sat, 30 Jan 2016 11:25:19 +0100
Message-ID: <CAKv+Gu8iG4DNCzAWMmRzXgvH4PeE09qK7i=fzvff9Fujmy7F0g@mail.gmail.com>

On 30 January 2016 at 04:17, Yao, Jiewen <jiewen.yao@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.

Regards,
Ard.


> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Saturday, January 30, 2016 10:24 AM
> To: Yao, Jiewen
> Cc: edk2-devel@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:lersek@redhat.com]
>> Sent: Saturday, January 30, 2016 1:13 AM
>> To: Yao, Jiewen
>> Cc: edk2-devel@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.yao@intel.com>
>>> Cc: "Gao, Liming" <liming.gao@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

[-- Attachment #8: Attached Message --]
[-- Type: message/rfc822, Size: 11564 bytes --]

From: Laszlo Ersek <lersek@redhat.com>
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: "edk2-devel@ml01.01.org" <edk2-devel@ml01.01.org>, "Yao, Jiewen" <jiewen.yao@intel.com>, "Gao, Liming" <liming.gao@intel.com>
Subject: Re: [edk2] [patch 0/7] Add UEFI2.6 MemoryAttributesTable support.
Date: Sun, 31 Jan 2016 21:36:16 +0100
Message-ID: <56AE7040.4060004@redhat.com>

On 01/30/16 11:25, Ard Biesheuvel wrote:
> On 30 January 2016 at 04:17, Yao, Jiewen <jiewen.yao@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:lersek@redhat.com]
>> Sent: Saturday, January 30, 2016 10:24 AM
>> To: Yao, Jiewen
>> Cc: edk2-devel@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:lersek@redhat.com]
>>> Sent: Saturday, January 30, 2016 1:13 AM
>>> To: Yao, Jiewen
>>> Cc: edk2-devel@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.yao@intel.com>
>>>> Cc: "Gao, Liming" <liming.gao@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

[-- Attachment #9: Attached Message --]
[-- Type: message/rfc822, Size: 13005 bytes --]

From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
To: Laszlo Ersek <lersek@redhat.com>
Cc: "edk2-devel@ml01.01.org" <edk2-devel@ml01.01.org>, "Yao, Jiewen" <jiewen.yao@intel.com>, "Gao, Liming" <liming.gao@intel.com>
Subject: Re: [edk2] [patch 0/7] Add UEFI2.6 MemoryAttributesTable support.
Date: Mon, 1 Feb 2016 07:27:47 +0100
Message-ID: <CAKv+Gu9ixCzswHbjpT0Xa+KFQbXxNEiA1Utb2ayqz3PEeh9zyg@mail.gmail.com>

On 31 January 2016 at 21:36, Laszlo Ersek <lersek@redhat.com> wrote:
> On 01/30/16 11:25, Ard Biesheuvel wrote:
>> On 30 January 2016 at 04:17, Yao, Jiewen <jiewen.yao@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:lersek@redhat.com]
>>> Sent: Saturday, January 30, 2016 10:24 AM
>>> To: Yao, Jiewen
>>> Cc: edk2-devel@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:lersek@redhat.com]
>>>> Sent: Saturday, January 30, 2016 1:13 AM
>>>> To: Yao, Jiewen
>>>> Cc: edk2-devel@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.yao@intel.com>
>>>>> Cc: "Gao, Liming" <liming.gao@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

[-- Attachment #10: Attached Message --]
[-- Type: message/rfc822, Size: 14551 bytes --]

From: "Yao, Jiewen" <jiewen.yao@intel.com>
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>, Laszlo Ersek <lersek@redhat.com>
Cc: "edk2-devel@ml01.01.org" <edk2-devel@ml01.01.org>, "Gao, Liming" <liming.gao@intel.com>
Subject: Re: [edk2] [patch 0/7] Add UEFI2.6 MemoryAttributesTable support.
Date: Sun, 14 Feb 2016 05:44:45 +0000
Message-ID: <74D8A39837DF1E4DA445A8C0B3885C5002759103@shsmsx102.ccr.corp.intel.com>

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?


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.biesheuvel@linaro.org] 
Sent: Monday, February 01, 2016 2:28 PM
To: Laszlo Ersek
Cc: Yao, Jiewen; edk2-devel@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 <lersek@redhat.com> wrote:
> On 01/30/16 11:25, Ard Biesheuvel wrote:
>> On 30 January 2016 at 04:17, Yao, Jiewen <jiewen.yao@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:lersek@redhat.com]
>>> Sent: Saturday, January 30, 2016 10:24 AM
>>> To: Yao, Jiewen
>>> Cc: edk2-devel@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:lersek@redhat.com]
>>>> Sent: Saturday, January 30, 2016 1:13 AM
>>>> To: Yao, Jiewen
>>>> Cc: edk2-devel@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.yao@intel.com>
>>>>> Cc: "Gao, Liming" <liming.gao@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

[-- Attachment #11: Attached Message --]
[-- Type: message/rfc822, Size: 17995 bytes --]

From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
To: "Yao, Jiewen" <jiewen.yao@intel.com>
Cc: "edk2-devel@ml01.01.org" <edk2-devel@ml01.01.org>, Laszlo Ersek <lersek@redhat.com>, "Gao, Liming" <liming.gao@intel.com>
Subject: Re: [edk2] [patch 0/7] Add UEFI2.6 MemoryAttributesTable support.
Date: Thu, 18 Feb 2016 18:44:30 +0100
Message-ID: <CAKv+Gu8RzfPM00q3PnwHTvXEYyC8fQb9Hym9i-P+apUYWDPmsQ@mail.gmail.com>

On 14 February 2016 at 06:44, Yao, Jiewen <jiewen.yao@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.biesheuvel@linaro.org]
> Sent: Monday, February 01, 2016 2:28 PM
> To: Laszlo Ersek
> Cc: Yao, Jiewen; edk2-devel@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 <lersek@redhat.com> wrote:
>> On 01/30/16 11:25, Ard Biesheuvel wrote:
>>> On 30 January 2016 at 04:17, Yao, Jiewen <jiewen.yao@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:lersek@redhat.com]
>>>> Sent: Saturday, January 30, 2016 10:24 AM
>>>> To: Yao, Jiewen
>>>> Cc: edk2-devel@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:lersek@redhat.com]
>>>>> Sent: Saturday, January 30, 2016 1:13 AM
>>>>> To: Yao, Jiewen
>>>>> Cc: edk2-devel@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.yao@intel.com>
>>>>>> Cc: "Gao, Liming" <liming.gao@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

[-- Attachment #12: Attached Message --]
[-- Type: message/rfc822, Size: 20122 bytes --]

From: "Yao, Jiewen" <jiewen.yao@intel.com>
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: "Yao, Jiewen" <jiewen.yao@intel.com>, "edk2-devel@ml01.01.org" <edk2-devel@ml01.01.org>, Laszlo Ersek <lersek@redhat.com>, "Gao, Liming" <liming.gao@intel.com>
Subject: Re: [edk2] [patch 0/7] Add UEFI2.6 MemoryAttributesTable support.
Date: Fri, 19 Feb 2016 00:43:12 +0000
Message-ID: <74D8A39837DF1E4DA445A8C0B3885C500275CB21@shsmsx102.ccr.corp.intel.com>

Thanks. It seems my misunderstanding.

Just want to double confirm: It is expected to see memory attribute table like below. Right?

  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.biesheuvel@linaro.org] 
Sent: Friday, February 19, 2016 1:44 AM
To: Yao, Jiewen
Cc: Laszlo Ersek; edk2-devel@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.yao@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.biesheuvel@linaro.org]
> Sent: Monday, February 01, 2016 2:28 PM
> To: Laszlo Ersek
> Cc: Yao, Jiewen; edk2-devel@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 <lersek@redhat.com> wrote:
>> On 01/30/16 11:25, Ard Biesheuvel wrote:
>>> On 30 January 2016 at 04:17, Yao, Jiewen <jiewen.yao@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:lersek@redhat.com]
>>>> Sent: Saturday, January 30, 2016 10:24 AM
>>>> To: Yao, Jiewen
>>>> Cc: edk2-devel@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:lersek@redhat.com]
>>>>> Sent: Saturday, January 30, 2016 1:13 AM
>>>>> To: Yao, Jiewen
>>>>> Cc: edk2-devel@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.yao@intel.com>
>>>>>> Cc: "Gao, Liming" <liming.gao@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

[-- Attachment #13: Attached Message --]
[-- Type: message/rfc822, Size: 20583 bytes --]

From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
To: "Yao, Jiewen" <jiewen.yao@intel.com>
Cc: "edk2-devel@ml01.01.org" <edk2-devel@ml01.01.org>, Laszlo Ersek <lersek@redhat.com>, "Gao, Liming" <liming.gao@intel.com>
Subject: Re: [edk2] [patch 0/7] Add UEFI2.6 MemoryAttributesTable support.
Date: Fri, 19 Feb 2016 07:27:00 +0100
Message-ID: <CAKv+Gu-spLpL1S5671kokJoSBRy4apV5zB5QaETJH34AC52CSQ@mail.gmail.com>

On 19 February 2016 at 01:43, Yao, Jiewen <jiewen.yao@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.biesheuvel@linaro.org]
> Sent: Friday, February 19, 2016 1:44 AM
> To: Yao, Jiewen
> Cc: Laszlo Ersek; edk2-devel@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.yao@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.biesheuvel@linaro.org]
>> Sent: Monday, February 01, 2016 2:28 PM
>> To: Laszlo Ersek
>> Cc: Yao, Jiewen; edk2-devel@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 <lersek@redhat.com> wrote:
>>> On 01/30/16 11:25, Ard Biesheuvel wrote:
>>>> On 30 January 2016 at 04:17, Yao, Jiewen <jiewen.yao@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:lersek@redhat.com]
>>>>> Sent: Saturday, January 30, 2016 10:24 AM
>>>>> To: Yao, Jiewen
>>>>> Cc: edk2-devel@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:lersek@redhat.com]
>>>>>> Sent: Saturday, January 30, 2016 1:13 AM
>>>>>> To: Yao, Jiewen
>>>>>> Cc: edk2-devel@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.yao@intel.com>
>>>>>>> Cc: "Gao, Liming" <liming.gao@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

[-- Attachment #14: Attached Message --]
[-- Type: message/rfc822, Size: 14028 bytes --]

From: Laszlo Ersek <lersek@redhat.com>
To: "Yao, Jiewen" <jiewen.yao@intel.com>, Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: "edk2-devel@ml01.01.org" <edk2-devel@ml01.01.org>, "Gao, Liming" <liming.gao@intel.com>
Subject: Re: [edk2] [patch 0/7] Add UEFI2.6 MemoryAttributesTable support.
Date: Sun, 14 Feb 2016 07:51:39 +0100
Message-ID: <56C023FB.60904@redhat.com>

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).

Thanks
Laszlo

> 
> Thank you
> Yao Jiewen
> 
> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org] 
> Sent: Monday, February 01, 2016 2:28 PM
> To: Laszlo Ersek
> Cc: Yao, Jiewen; edk2-devel@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 <lersek@redhat.com> wrote:
>> On 01/30/16 11:25, Ard Biesheuvel wrote:
>>> On 30 January 2016 at 04:17, Yao, Jiewen <jiewen.yao@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:lersek@redhat.com]
>>>> Sent: Saturday, January 30, 2016 10:24 AM
>>>> To: Yao, Jiewen
>>>> Cc: edk2-devel@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:lersek@redhat.com]
>>>>> Sent: Saturday, January 30, 2016 1:13 AM
>>>>> To: Yao, Jiewen
>>>>> Cc: edk2-devel@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.yao@intel.com>
>>>>>> Cc: "Gao, Liming" <liming.gao@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

[-- Attachment #15: Attached Message --]
[-- Type: message/rfc822, Size: 15822 bytes --]

From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
To: Laszlo Ersek <lersek@redhat.com>
Cc: "edk2-devel@ml01.01.org" <edk2-devel@ml01.01.org>, "Yao, Jiewen" <jiewen.yao@intel.com>, "Gao, Liming" <liming.gao@intel.com>
Subject: Re: [edk2] [patch 0/7] Add UEFI2.6 MemoryAttributesTable support.
Date: Sun, 14 Feb 2016 10:22:04 +0100
Message-ID: <CAKv+Gu9ixB2N-Cbgh3pjqmoS=pbDuTEToZFX0RPCRT5f4f=qBw@mail.gmail.com>

On 14 February 2016 at 07:51, Laszlo Ersek <lersek@redhat.com> 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:ard.biesheuvel@linaro.org]
>> Sent: Monday, February 01, 2016 2:28 PM
>> To: Laszlo Ersek
>> Cc: Yao, Jiewen; edk2-devel@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 <lersek@redhat.com> wrote:
>>> On 01/30/16 11:25, Ard Biesheuvel wrote:
>>>> On 30 January 2016 at 04:17, Yao, Jiewen <jiewen.yao@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:lersek@redhat.com]
>>>>> Sent: Saturday, January 30, 2016 10:24 AM
>>>>> To: Yao, Jiewen
>>>>> Cc: edk2-devel@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:lersek@redhat.com]
>>>>>> Sent: Saturday, January 30, 2016 1:13 AM
>>>>>> To: Yao, Jiewen
>>>>>> Cc: edk2-devel@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.yao@intel.com>
>>>>>>> Cc: "Gao, Liming" <liming.gao@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

[-- Attachment #16: Attached Message --]
[-- Type: message/rfc822, Size: 6797 bytes --]

From: Laszlo Ersek <lersek@redhat.com>
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: "edk2-devel@ml01.01.org" <edk2-devel@ml01.01.org>, "Yao, Jiewen" <jiewen.yao@intel.com>, "Gao, Liming" <liming.gao@intel.com>
Subject: Re: [edk2] [patch 0/7] Add UEFI2.6 MemoryAttributesTable support.
Date: Sun, 14 Feb 2016 11:52:32 +0100
Message-ID: <56C05C70.5000700@redhat.com>

On 02/14/16 10:22, Ard Biesheuvel wrote:
> On 14 February 2016 at 07:51, Laszlo Ersek <lersek@redhat.com> 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.

Well, why not drop the text from the spec completely then?

> 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.

Okay, but in that case, I sort of think of the edk2 code as something
that would let you test an operating system's support for this (mis)feature.

Anyway, I don't disagree with (3) either. If that is what gets
implemented, I'll be fine with it. As long as there's a way to disable
the (mis)feature -- by way of PCD or complete removal --, I'm okay.

Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[-- Attachment #17: Attached Message --]
[-- Type: message/rfc822, Size: 8074 bytes --]

From: "Yao, Jiewen" <jiewen.yao@intel.com>
To: Laszlo Ersek <lersek@redhat.com>, Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: "edk2-devel@ml01.01.org" <edk2-devel@ml01.01.org>, "Gao, Liming" <liming.gao@intel.com>
Subject: Re: [edk2] [patch 0/7] Add UEFI2.6 MemoryAttributesTable support.
Date: Tue, 16 Feb 2016 00:51:17 +0000
Message-ID: <74D8A39837DF1E4DA445A8C0B3885C5002759A22@shsmsx102.ccr.corp.intel.com>

Thanks. I think it is easy to just choose 2).
For 3), we need start evaluating if there is any impact to any production.

Thank you
Yao Jiewen

-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Sunday, February 14, 2016 6:53 PM
To: Ard Biesheuvel
Cc: Yao, Jiewen; edk2-devel@ml01.01.org; Gao, Liming
Subject: Re: [edk2] [patch 0/7] Add UEFI2.6 MemoryAttributesTable support.

On 02/14/16 10:22, Ard Biesheuvel wrote:
> On 14 February 2016 at 07:51, Laszlo Ersek <lersek@redhat.com> 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.

Well, why not drop the text from the spec completely then?

> 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.

Okay, but in that case, I sort of think of the edk2 code as something that would let you test an operating system's support for this (mis)feature.

Anyway, I don't disagree with (3) either. If that is what gets implemented, I'll be fine with it. As long as there's a way to disable the (mis)feature -- by way of PCD or complete removal --, I'm okay.

Thanks
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[-- Attachment #18: Attached Message --]
[-- Type: message/rfc822, Size: 9408 bytes --]

From: "Yao, Jiewen" <jiewen.yao@intel.com>
To: "Yao, Jiewen" <jiewen.yao@intel.com>, Laszlo Ersek <lersek@redhat.com>, Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: "edk2-devel@ml01.01.org" <edk2-devel@ml01.01.org>, "Yao, Jiewen" <jiewen.yao@intel.com>, "Gao, Liming" <liming.gao@intel.com>
Subject: Re: [edk2] [patch 0/7] Add UEFI2.6 MemoryAttributesTable support.
Date: Wed, 17 Feb 2016 12:22:44 +0000
Message-ID: <74D8A39837DF1E4DA445A8C0B3885C500275A94E@shsmsx102.ccr.corp.intel.com>

I collected Intel internal feedback.

1) We observed the shipping production still using this Properties Table, and UEFI 2.6 specification still keeps the text.  So we want to keep code to give production a way to enable this feature.
2) Since UEFI specification marks this as "not recommended", we agree to change default value to FALSE in DEC declaration.

Intel vote #2, at current point of time.
We vote #3 as long term plan, after UEFI 2.x specification removes the text later.

Thank you
Yao Jiewen

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Yao, Jiewen
Sent: Tuesday, February 16, 2016 8:51 AM
To: Laszlo Ersek; Ard Biesheuvel
Cc: edk2-devel@ml01.01.org; Gao, Liming
Subject: Re: [edk2] [patch 0/7] Add UEFI2.6 MemoryAttributesTable support.

Thanks. I think it is easy to just choose 2).
For 3), we need start evaluating if there is any impact to any production.

Thank you
Yao Jiewen

-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com]
Sent: Sunday, February 14, 2016 6:53 PM
To: Ard Biesheuvel
Cc: Yao, Jiewen; edk2-devel@ml01.01.org; Gao, Liming
Subject: Re: [edk2] [patch 0/7] Add UEFI2.6 MemoryAttributesTable support.

On 02/14/16 10:22, Ard Biesheuvel wrote:
> On 14 February 2016 at 07:51, Laszlo Ersek <lersek@redhat.com> 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.

Well, why not drop the text from the spec completely then?

> 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.

Okay, but in that case, I sort of think of the edk2 code as something that would let you test an operating system's support for this (mis)feature.

Anyway, I don't disagree with (3) either. If that is what gets implemented, I'll be fine with it. As long as there's a way to disable the (mis)feature -- by way of PCD or complete removal --, I'm okay.

Thanks
Laszlo
_______________________________________________
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

[-- Attachment #19: Attached Message --]
[-- Type: message/rfc822, Size: 9660 bytes --]

From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
To: "Yao, Jiewen" <jiewen.yao@intel.com>
Cc: "edk2-devel@ml01.01.org" <edk2-devel@ml01.01.org>, Laszlo Ersek <lersek@redhat.com>, "Gao, Liming" <liming.gao@intel.com>
Subject: Re: [edk2] [patch 0/7] Add UEFI2.6 MemoryAttributesTable support.
Date: Wed, 17 Feb 2016 13:24:18 +0100
Message-ID: <CAKv+Gu9N-J-038LOrux2dYr8Ju_f=GfsL1bNyJBu7jBnLMx1eA@mail.gmail.com>

On 17 February 2016 at 13:22, Yao, Jiewen <jiewen.yao@intel.com> wrote:
> I collected Intel internal feedback.
>
> 1) We observed the shipping production still using this Properties Table, and UEFI 2.6 specification still keeps the text.  So we want to keep code to give production a way to enable this feature.
> 2) Since UEFI specification marks this as "not recommended", we agree to change default value to FALSE in DEC declaration.
>
> Intel vote #2, at current point of time.
> We vote #3 as long term plan, after UEFI 2.x specification removes the text later.
>

OK, that is fine by me. As long as we agree that it is something that
is on its way to be removed, rather than a valid alternative for the
MemoryAttributes table.

Thanks,
Ard.


> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Yao, Jiewen
> Sent: Tuesday, February 16, 2016 8:51 AM
> To: Laszlo Ersek; Ard Biesheuvel
> Cc: edk2-devel@ml01.01.org; Gao, Liming
> Subject: Re: [edk2] [patch 0/7] Add UEFI2.6 MemoryAttributesTable support.
>
> Thanks. I think it is easy to just choose 2).
> For 3), we need start evaluating if there is any impact to any production.
>
> Thank you
> Yao Jiewen
>
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Sunday, February 14, 2016 6:53 PM
> To: Ard Biesheuvel
> Cc: Yao, Jiewen; edk2-devel@ml01.01.org; Gao, Liming
> Subject: Re: [edk2] [patch 0/7] Add UEFI2.6 MemoryAttributesTable support.
>
> On 02/14/16 10:22, Ard Biesheuvel wrote:
>> On 14 February 2016 at 07:51, Laszlo Ersek <lersek@redhat.com> 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.
>
> Well, why not drop the text from the spec completely then?
>
>> 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.
>
> Okay, but in that case, I sort of think of the edk2 code as something that would let you test an operating system's support for this (mis)feature.
>
> Anyway, I don't disagree with (3) either. If that is what gets implemented, I'll be fine with it. As long as there's a way to disable the (mis)feature -- by way of PCD or complete removal --, I'm okay.
>
> Thanks
> Laszlo
> _______________________________________________
> 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