[edk2] [patch] MdePkg/PropertiesTable support, MdeModulePkg/DxeCore support UEFI2.5 properties table

Discussion:

Yao, Jiewen

2015-06-04 14:34:31 UTC

Hi
Here is patch to support UEFI2.5, chapter 4, section 4.6 Properties Table feature.

MdePkg - add Properties table definition.
MdeModulePkg - add properties table support in DXE core.
MdeModulePkg - add PropertiesTableAttributesDxe driver to set ACPINvs/Reserved memory type to be XP.

Signed-off-by: Yao, Jiewen ***@intel.com<mailto:***@intel.com>
Reviewed-by: Zeng, Star ***@intel.com<mailto:***@intel.com>
Reviewed-by: Gao, Liming ***@intel.com<mailto:***@intel.com>

Thank you
Yao Jiewen

Laszlo Ersek

2015-06-04 17:47:37 UTC

Permalink

Jiewen,

On 06/04/15 16:34, Yao, Jiewen wrote:> Hi

Post by Yao, Jiewen
Here is patch to support UEFI2.5, chapter 4, section 4.6 Properties Table feature.
MdePkg \u2013 add Properties table definition.
MdeModulePkg \u2013 add properties table support in DXE core.
MdeModulePkg \u2013 add PropertiesTableAttributesDxe driver to set
ACPINvs/Reserved memory type to be XP.

Honest question: do you ever intend to abandon the following bad
practices:
- extremely long lines,
- huge code bombs in a few patches,
- posting patches as attachments?

The diffstat for the second attachment is:

/PropertiesTableAttributesDxe/PropertiesTableAttributesDxe.uni | 1
/PropertiesTableAttributesDxe/PropertiesTableAttributesDxeExtra.uni | 1
Core/Dxe/DxeMain.h | 29
Core/Dxe/DxeMain.inf | 4
Core/Dxe/DxeMain/DxeMain.c | 2
Core/Dxe/Image/Image.c | 2
Core/Dxe/Misc/PropertiesTable.c | 1418 ++++++++++
MdeModulePkg.dec | 22
MdeModulePkg.dsc | 2
Universal/PropertiesTableAttributesDxe/PropertiesTableAttributesDxe.c | 412 ++
Universal/PropertiesTableAttributesDxe/PropertiesTableAttributesDxe.inf | 114
Universal/PropertiesTableAttributesDxe/PropertiesTableAttributesDxe.uni | 1
Universal/PropertiesTableAttributesDxe/PropertiesTableAttributesDxeExtra.uni | 1
13 files changed, 2009 insertions(+)

Also, the longest line in the patch is 214 characters.

Do you ever intend to adopt inline patches, to break up features into
series of patches, and to follow the line length hints of the edk2
coding style?

Honestly, I have the impression about these postings that they are an
after-the-fact (non-)courtesy to the mailing list. "We wrote this at
Intel, it has been reviewed, it's going in, we'll ignore your formatting
requests that you've repeated many times; if you are unable to review it
as-is, your loss".

If you are committed to ignore these bugs in your patches in the long
term, I'd like to know that.

Thanks
Laszlo

------------------------------------------------------------------------------

Jordan Justen

2015-06-09 00:13:58 UTC

Permalink

Post by Laszlo Ersek
Jiewen,
On 06/04/15 16:34, Yao, Jiewen wrote:> Hi

Honest question: do you ever intend to abandon the following bad
- extremely long lines,
- huge code bombs in a few patches,
- posting patches as attachments?
/PropertiesTableAttributesDxe/PropertiesTableAttributesDxe.uni | 1
/PropertiesTableAttributesDxe/PropertiesTableAttributesDxeExtra.uni | 1
Core/Dxe/DxeMain.h | 29
Core/Dxe/DxeMain.inf | 4
Core/Dxe/DxeMain/DxeMain.c | 2
Core/Dxe/Image/Image.c | 2
Core/Dxe/Misc/PropertiesTable.c | 1418 ++++++++++
MdeModulePkg.dec | 22
MdeModulePkg.dsc | 2
Universal/PropertiesTableAttributesDxe/PropertiesTableAttributesDxe.c | 412 ++
Universal/PropertiesTableAttributesDxe/PropertiesTableAttributesDxe.inf | 114
Universal/PropertiesTableAttributesDxe/PropertiesTableAttributesDxe.uni | 1
Universal/PropertiesTableAttributesDxe/PropertiesTableAttributesDxeExtra.uni | 1
13 files changed, 2009 insertions(+)
Also, the longest line in the patch is 214 characters.
Do you ever intend to adopt inline patches, to break up features into
series of patches, and to follow the line length hints of the edk2
coding style?
Honestly, I have the impression about these postings that they are an
after-the-fact (non-)courtesy to the mailing list. "We wrote this at
Intel, it has been reviewed, it's going in, we'll ignore your formatting
requests that you've repeated many times; if you are unable to review it
as-is, your loss".
If you are committed to ignore these bugs in your patches in the long
term, I'd like to know that.

Jiewen, it seems like Laszlo expressed some (valid) concerns with your
patch, but you didn't respond, and checked them in anyway.

Considering how much effort it is for people to code review patches,
it is not very friendly to ignore their feedback.

Occasionally, in urgent cases, we can disagree and still move forward
even when there is disagreement, but I don't think it is good to
ignore the discussion entirely and move forward with no regard for the
concerns.

Did you intend this edk2-devel "code-review" to actually be "We wrote
this at Intel, it has been reviewed, it's going in" as was Laszlo's
concern?

I think the real start of the issue is that this should have been
posted to edk2-devel before there was any Reviewed-by. And, following
the posting, all feedback from the community should be considered.

Mike, do you have any thoughts on how we might be able to address
these issues? Maybe we could consider removing commit privileges for
developers that don't follow the process? Is there someone else on the
team besides you that I should ask this question to?

Thanks,

-Jordan

------------------------------------------------------------------------------

Yao, Jiewen

2015-06-09 01:20:02 UTC

Permalink

HI Laszlo
Thanks for your feedback. It reminds us to use GIT.
I am sorry that I missed your feedback before. I search it in my mailbox again today, but I fail to find it.
Not sure why. :-(

Currently we are still using SVN on some machines. Sorry to bring trouble for GIT user.
We are moving towards GIT, and we are involving community for more discussion.

Thank you
Yao Jiewen

-----Original Message-----
From: Justen, Jordan L
Sent: Tuesday, June 09, 2015 8:14 AM
To: edk2-***@lists.sourceforge.net; Yao, Jiewen; Laszlo Ersek; Kinney, Michael D
Subject: Re: [edk2] [patch] MdePkg/PropertiesTable support, MdeModulePkg/DxeCore support UEFI2.5 properties table

Post by Laszlo Ersek
Jiewen,
On 06/04/15 16:34, Yao, Jiewen wrote:> Hi

Honest question: do you ever intend to abandon the following bad
- extremely long lines,
- huge code bombs in a few patches,
- posting patches as attachments?
/PropertiesTableAttributesDxe/PropertiesTableAttributesDxe.uni | 1
/PropertiesTableAttributesDxe/PropertiesTableAttributesDxeExtra.uni | 1
Core/Dxe/DxeMain.h | 29
Core/Dxe/DxeMain.inf | 4
Core/Dxe/DxeMain/DxeMain.c | 2
Core/Dxe/Image/Image.c | 2
Core/Dxe/Misc/PropertiesTable.c | 1418 ++++++++++
MdeModulePkg.dec | 22
MdeModulePkg.dsc | 2
Universal/PropertiesTableAttributesDxe/PropertiesTableAttributesDxe.c | 412 ++
Universal/PropertiesTableAttributesDxe/PropertiesTableAttributesDxe.inf | 114
Universal/PropertiesTableAttributesDxe/PropertiesTableAttributesDxe.uni | 1
Universal/PropertiesTableAttributesDxe/PropertiesTableAttributesDxeExtra.uni | 1
13 files changed, 2009 insertions(+)
Also, the longest line in the patch is 214 characters.
Do you ever intend to adopt inline patches, to break up features into
series of patches, and to follow the line length hints of the edk2
coding style?
Honestly, I have the impression about these postings that they are an
after-the-fact (non-)courtesy to the mailing list. "We wrote this at
Intel, it has been reviewed, it's going in, we'll ignore your
formatting requests that you've repeated many times; if you are unable
to review it as-is, your loss".
If you are committed to ignore these bugs in your patches in the long
term, I'd like to know that.

Jiewen, it seems like Laszlo expressed some (valid) concerns with your patch, but you didn't respond, and checked them in anyway.

Considering how much effort it is for people to code review patches, it is not very friendly to ignore their feedback.

Occasionally, in urgent cases, we can disagree and still move forward even when there is disagreement, but I don't think it is good to ignore the discussion entirely and move forward with no regard for the concerns.

Did you intend this edk2-devel "code-review" to actually be "We wrote this at Intel, it has been reviewed, it's going in" as was Laszlo's concern?

I think the real start of the issue is that this should have been posted to edk2-devel before there was any Reviewed-by. And, following the posting, all feedback from the community should be considered.

Mike, do you have any thoughts on how we might be able to address these issues? Maybe we could consider removing commit privileges for developers that don't follow the process? Is there someone else on the team besides you that I should ask this question to?

Thanks,

-Jordan
------------------------------------------------------------------------------

Laszlo Ersek

2015-06-09 07:45:08 UTC

Permalink

Post by Yao, Jiewen
HI Laszlo
Thanks for your feedback. It reminds us to use GIT.
I am sorry that I missed your feedback before. I search it in my mailbox again today, but I fail to find it.
Not sure why. :-(
Currently we are still using SVN on some machines. Sorry to bring trouble for GIT user.
We are moving towards GIT, and we are involving community for more discussion.

Works for me, thank you. I just hope that eventually everyone on the
Intel firmware team will move to git. (I do realize there's the other
discussion about submodules / subtrees, which could be a blocker for you
guys.)

Thanks
Laszlo

Post by Yao, Jiewen
Thank you
Yao Jiewen
-----Original Message-----
From: Justen, Jordan L
Sent: Tuesday, June 09, 2015 8:14 AM
Subject: Re: [edk2] [patch] MdePkg/PropertiesTable support, MdeModulePkg/DxeCore support UEFI2.5 properties table

Post by Laszlo Ersek
Jiewen,
On 06/04/15 16:34, Yao, Jiewen wrote:> Hi

Honest question: do you ever intend to abandon the following bad
- extremely long lines,
- huge code bombs in a few patches,
- posting patches as attachments?
/PropertiesTableAttributesDxe/PropertiesTableAttributesDxe.uni | 1
/PropertiesTableAttributesDxe/PropertiesTableAttributesDxeExtra.uni | 1
Core/Dxe/DxeMain.h | 29
Core/Dxe/DxeMain.inf | 4
Core/Dxe/DxeMain/DxeMain.c | 2
Core/Dxe/Image/Image.c | 2
Core/Dxe/Misc/PropertiesTable.c | 1418 ++++++++++
MdeModulePkg.dec | 22
MdeModulePkg.dsc | 2
Universal/PropertiesTableAttributesDxe/PropertiesTableAttributesDxe.c | 412 ++
Universal/PropertiesTableAttributesDxe/PropertiesTableAttributesDxe.inf | 114
Universal/PropertiesTableAttributesDxe/PropertiesTableAttributesDxe.uni | 1
Universal/PropertiesTableAttributesDxe/PropertiesTableAttributesDxeExtra.uni | 1
13 files changed, 2009 insertions(+)
Also, the longest line in the patch is 214 characters.
Do you ever intend to adopt inline patches, to break up features into
series of patches, and to follow the line length hints of the edk2
coding style?
Honestly, I have the impression about these postings that they are an
after-the-fact (non-)courtesy to the mailing list. "We wrote this at
Intel, it has been reviewed, it's going in, we'll ignore your
formatting requests that you've repeated many times; if you are unable
to review it as-is, your loss".
If you are committed to ignore these bugs in your patches in the long
term, I'd like to know that.

Jiewen, it seems like Laszlo expressed some (valid) concerns with your patch, but you didn't respond, and checked them in anyway.
Considering how much effort it is for people to code review patches, it is not very friendly to ignore their feedback.
Occasionally, in urgent cases, we can disagree and still move forward even when there is disagreement, but I don't think it is good to ignore the discussion entirely and move forward with no regard for the concerns.
Did you intend this edk2-devel "code-review" to actually be "We wrote this at Intel, it has been reviewed, it's going in" as was Laszlo's concern?
I think the real start of the issue is that this should have been posted to edk2-devel before there was any Reviewed-by. And, following the posting, all feedback from the community should be considered.
Mike, do you have any thoughts on how we might be able to address these issues? Maybe we could consider removing commit privileges for developers that don't follow the process? Is there someone else on the team besides you that I should ask this question to?
Thanks,
-Jordan

------------------------------------------------------------------------------

Ard Biesheuvel

2015-06-05 13:12:15 UTC

Permalink

Post by Yao, Jiewen
Hi
Here is patch to support UEFI2.5, chapter 4, section 4.6 Properties Table feature.
MdePkg – add Properties table definition.
MdeModulePkg – add properties table support in DXE core.
MdeModulePkg – add PropertiesTableAttributesDxe driver to set
ACPINvs/Reserved memory type to be XP.

This patch breaks the build for GCC on 64-bit ARM

13:03:43 /home/buildslave/workspace/leg-virt-tianocore-edk2-upstream/edk2/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c:
In function ‘InsertImageRecord’:
13:03:43 /home/buildslave/workspace/leg-virt-tianocore-edk2-upstream/edk2/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c:1113:10:
error: variable ‘Magic’ set but not used
[-Werror=unused-but-set-variable]
13:03:43 UINT16 Magic;
13:03:43 ^
13:03:43 /home/buildslave/workspace/leg-virt-tianocore-edk2-upstream/edk2/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c:1110:10:
error: variable ‘ImageType’ set but not used
[-Werror=unused-but-set-variable]
13:03:43 UINT16 ImageType;
13:03:43 ^

and honestly, i can't be bothered to look at the code myself,
considering the way it was dumped into the mailing list and the
repository.

Please get this fixed

Regards,
Ard.

Post by Yao, Jiewen
------------------------------------------------------------------------------
_______________________________________________
edk2-devel mailing list
https://lists.sourceforge.net/lists/listinfo/edk2-devel

------------------------------------------------------------------------------

Yao, Jiewen

2015-06-05 13:21:33 UTC

Permalink

Hi Ard
Thanks to report this. I have fixed this in 17565.

Thank you
Yao Jiewen

-----Original Message-----
From: Ard Biesheuvel [mailto:***@linaro.org]
Sent: Friday, June 05, 2015 9:12 PM
To: edk2-***@lists.sourceforge.net; Yao, Jiewen; Zeng, Star; Gao, Liming
Subject: Re: [edk2] [patch] MdePkg/PropertiesTable support, MdeModulePkg/DxeCore support UEFI2.5 properties table

This patch breaks the build for GCC on 64-bit ARM

13:03:43 /home/buildslave/workspace/leg-virt-tianocore-edk2-upstream/edk2/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c:
In function ‘InsertImageRecord’:
13:03:43 /home/buildslave/workspace/leg-virt-tianocore-edk2-upstream/edk2/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c:1113:10:
error: variable ‘Magic’ set but not used [-Werror=unused-but-set-variable]
13:03:43 UINT16 Magic;
13:03:43 ^
13:03:43 /home/buildslave/workspace/leg-virt-tianocore-edk2-upstream/edk2/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c:1110:10:
error: variable ‘ImageType’ set but not used [-Werror=unused-but-set-variable]
13:03:43 UINT16 ImageType;
13:03:43 ^

and honestly, i can't be bothered to look at the code myself, considering the way it was dumped into the mailing list and the repository.

Please get this fixed

Regards,
Ard.

Post by Yao, Jiewen
----------------------------------------------------------------------
--------
_______________________________________________
edk2-devel mailing list
https://lists.sourceforge.net/lists/listinfo/edk2-devel

------------------------------------------------------------------------------

Ard Biesheuvel

2015-06-05 13:22:40 UTC

Permalink

Post by Yao, Jiewen
Hi Ard
Thanks to report this. I have fixed this in 17565.

Wow, that was quick! Thanks!

Post by Yao, Jiewen
-----Original Message-----
Sent: Friday, June 05, 2015 9:12 PM
Subject: Re: [edk2] [patch] MdePkg/PropertiesTable support, MdeModulePkg/DxeCore support UEFI2.5 properties table

This patch breaks the build for GCC on 64-bit ARM
error: variable ‘Magic’ set but not used [-Werror=unused-but-set-variable]
13:03:43 UINT16 Magic;
13:03:43 ^
error: variable ‘ImageType’ set but not used [-Werror=unused-but-set-variable]
13:03:43 UINT16 ImageType;
13:03:43 ^
and honestly, i can't be bothered to look at the code myself, considering the way it was dumped into the mailing list and the repository.
Please get this fixed
Regards,
Ard.

Post by Yao, Jiewen
----------------------------------------------------------------------
--------
_______________________________________________
edk2-devel mailing list
https://lists.sourceforge.net/lists/listinfo/edk2-devel

------------------------------------------------------------------------------

Ard Biesheuvel

2015-06-09 12:27:52 UTC

Permalink

Post by Ard Biesheuvel

Post by Yao, Jiewen
Hi Ard
Thanks to report this. I have fixed this in 17565.

Wow, that was quick! Thanks!

You have fixed the build, but the copy-pasted comments are still
there, which make no sense at all anymore, since they refer to the
Magic variable, which has been removed.

Next time, could you please send your patches (including fixes like
these) to the mailing list first? That way, people have a chance to
see what's going in before its gets merged.

Thanks,
Ard.

Post by Ard Biesheuvel

Post by Yao, Jiewen
-----Original Message-----
Sent: Friday, June 05, 2015 9:12 PM
Subject: Re: [edk2] [patch] MdePkg/PropertiesTable support, MdeModulePkg/DxeCore support UEFI2.5 properties table

Post by Yao, Jiewen
----------------------------------------------------------------------
--------
_______________________________________________
edk2-devel mailing list
https://lists.sourceforge.net/lists/listinfo/edk2-devel

------------------------------------------------------------------------------

Yao, Jiewen

2015-06-09 12:42:26 UTC

Permalink

Hi Ard
You are right. I happen to find a logic error, too.

Attach patch for your review.

-----Original Message-----
From: Ard Biesheuvel [mailto:***@linaro.org]
Sent: Tuesday, June 09, 2015 8:28 PM
To: Yao, Jiewen
Cc: edk2-***@lists.sourceforge.net; Zeng, Star; Gao, Liming
Subject: Re: [edk2] [patch] MdePkg/PropertiesTable support, MdeModulePkg/DxeCore support UEFI2.5 properties table

Post by Ard Biesheuvel

Post by Yao, Jiewen
Hi Ard
Thanks to report this. I have fixed this in 17565.

Wow, that was quick! Thanks!

You have fixed the build, but the copy-pasted comments are still there, which make no sense at all anymore, since they refer to the Magic variable, which has been removed.

Next time, could you please send your patches (including fixes like
these) to the mailing list first? That way, people have a chance to see what's going in before its gets merged.

Thanks,
Ard.

Post by Ard Biesheuvel

Post by Yao, Jiewen
-----Original Message-----
Sent: Friday, June 05, 2015 9:12 PM
Subject: Re: [edk2] [patch] MdePkg/PropertiesTable support,
MdeModulePkg/DxeCore support UEFI2.5 properties table

Post by Yao, Jiewen
Hi
Here is patch to support UEFI2.5, chapter 4, section 4.6 Properties Table feature.
MdePkg â add Properties table definition.
MdeModulePkg â add properties table support in DXE core.
MdeModulePkg â add PropertiesTableAttributesDxe driver to set
ACPINvs/Reserved memory type to be XP.

This patch breaks the build for GCC on 64-bit ARM
error: variable âMagicâ set but not used [-Werror=unused-but-set-variable]
13:03:43 UINT16 Magic;
13:03:43 ^
error: variable âImageTypeâ set but not used [-Werror=unused-but-set-variable]
13:03:43 UINT16 ImageType;
13:03:43 ^
and honestly, i can't be bothered to look at the code myself, considering the way it was dumped into the mailing list and the repository.
Please get this fixed
Regards,
Ard.

Post by Yao, Jiewen
--------------------------------------------------------------------
--
--------
_______________________________________________
edk2-devel mailing list
https://lists.sourceforge.net/lists/listinfo/edk2-devel

Ard Biesheuvel

2015-06-09 13:22:07 UTC

Permalink

Post by Yao, Jiewen
Hi Ard
You are right. I happen to find a logic error, too.
Attach patch for your review.

Yes, that looks fine to me, although it is unfortunate that can't just
use PeCoffLoaderGetPeHeaderMagicValue () directly.
It builds fine on GCC/AArch64

Reviewed-by: Ard Biesheuvel <***@linaro.org>

Thanks,
Ard.

Post by Yao, Jiewen
-----Original Message-----
Sent: Tuesday, June 09, 2015 8:28 PM
To: Yao, Jiewen
Subject: Re: [edk2] [patch] MdePkg/PropertiesTable support, MdeModulePkg/DxeCore support UEFI2.5 properties table

Post by Ard Biesheuvel

Post by Yao, Jiewen
Hi Ard
Thanks to report this. I have fixed this in 17565.

Wow, that was quick! Thanks!

You have fixed the build, but the copy-pasted comments are still there, which make no sense at all anymore, since they refer to the Magic variable, which has been removed.
Next time, could you please send your patches (including fixes like
these) to the mailing list first? That way, people have a chance to see what's going in before its gets merged.
Thanks,
Ard.

Post by Ard Biesheuvel

Post by Yao, Jiewen
-----Original Message-----
Sent: Friday, June 05, 2015 9:12 PM
Subject: Re: [edk2] [patch] MdePkg/PropertiesTable support,
MdeModulePkg/DxeCore support UEFI2.5 properties table

Post by Yao, Jiewen
--------------------------------------------------------------------
--
--------
_______________________________________________
edk2-devel mailing list
https://lists.sourceforge.net/lists/listinfo/edk2-devel

------------------------------------------------------------------------------

Yao, Jiewen

2015-06-09 13:34:11 UTC

Permalink

Thank you! Agree, I have similar concern on PeCoffLoaderGetPeHeaderMagicValue().

I have seen duplicated code in below modules:
1) SecurityPkg\Library\DxeImageVerificationLib\DxeImageVerificationLib.c(363): if (mNtHeader.Pe32->FileHeader.Machine == IMAGE_FILE_MACHINE_IA64 && mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
2) SecurityPkg\Library\DxeImageVerificationLib\DxeImageVerificationLib.c(1690): if (mNtHeader.Pe32->FileHeader.Machine == IMAGE_FILE_MACHINE_IA64 && mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
3) SecurityPkg\Library\DxeTpmMeasureBootLib\DxeTpmMeasureBootLib.c(419): if (Hdr.Pe32->FileHeader.Machine == IMAGE_FILE_MACHINE_IA64 && Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
4) SecurityPkg\Tcg\TrEEDxe\MeasureBootPeCoff.c(114): if (Hdr.Pe32->FileHeader.Machine == IMAGE_FILE_MACHINE_IA64 && Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
5) SecurityPkg\VariableAuthenticated\SecureBootConfigDxe\SecureBootConfigImpl.c(1734): if (mNtHeader.Pe32->FileHeader.Machine == IMAGE_FILE_MACHINE_IA64 && mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {

How I wish it is exposed!

Thank you
Yao Jiewen

-----Original Message-----
From: Ard Biesheuvel [mailto:***@linaro.org]
Sent: Tuesday, June 09, 2015 9:22 PM
To: Yao, Jiewen
Cc: edk2-***@lists.sourceforge.net; Zeng, Star; Gao, Liming
Subject: Re: [edk2] [patch] MdePkg/PropertiesTable support, MdeModulePkg/DxeCore support UEFI2.5 properties table

Post by Yao, Jiewen
Hi Ard
You are right. I happen to find a logic error, too.
Attach patch for your review.

Yes, that looks fine to me, although it is unfortunate that can't just use PeCoffLoaderGetPeHeaderMagicValue () directly.
It builds fine on GCC/AArch64

Reviewed-by: Ard Biesheuvel <***@linaro.org>

Thanks,
Ard.

Post by Yao, Jiewen
-----Original Message-----
Sent: Tuesday, June 09, 2015 8:28 PM
To: Yao, Jiewen
Subject: Re: [edk2] [patch] MdePkg/PropertiesTable support,
MdeModulePkg/DxeCore support UEFI2.5 properties table

Post by Ard Biesheuvel

Post by Yao, Jiewen
Hi Ard
Thanks to report this. I have fixed this in 17565.

Wow, that was quick! Thanks!

You have fixed the build, but the copy-pasted comments are still there, which make no sense at all anymore, since they refer to the Magic variable, which has been removed.
Next time, could you please send your patches (including fixes like
these) to the mailing list first? That way, people have a chance to see what's going in before its gets merged.
Thanks,
Ard.

Post by Ard Biesheuvel

Post by Yao, Jiewen
-----Original Message-----
Sent: Friday, June 05, 2015 9:12 PM
Subject: Re: [edk2] [patch] MdePkg/PropertiesTable support,
MdeModulePkg/DxeCore support UEFI2.5 properties table

Post by Yao, Jiewen
-------------------------------------------------------------------
-
--
--------
_______________________________________________
edk2-devel mailing list
https://lists.sourceforge.net/lists/listinfo/edk2-devel

------------------------------------------------------------------------------

Ard Biesheuvel

2015-06-09 13:55:50 UTC

Permalink

Post by Yao, Jiewen
Thank you! Agree, I have similar concern on PeCoffLoaderGetPeHeaderMagicValue().
1) SecurityPkg\Library\DxeImageVerificationLib\DxeImageVerificationLib.c(363): if (mNtHeader.Pe32->FileHeader.Machine == IMAGE_FILE_MACHINE_IA64 && mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
2) SecurityPkg\Library\DxeImageVerificationLib\DxeImageVerificationLib.c(1690): if (mNtHeader.Pe32->FileHeader.Machine == IMAGE_FILE_MACHINE_IA64 && mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
3) SecurityPkg\Library\DxeTpmMeasureBootLib\DxeTpmMeasureBootLib.c(419): if (Hdr.Pe32->FileHeader.Machine == IMAGE_FILE_MACHINE_IA64 && Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
4) SecurityPkg\Tcg\TrEEDxe\MeasureBootPeCoff.c(114): if (Hdr.Pe32->FileHeader.Machine == IMAGE_FILE_MACHINE_IA64 && Hdr.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
5) SecurityPkg\VariableAuthenticated\SecureBootConfigDxe\SecureBootConfigImpl.c(1734): if (mNtHeader.Pe32->FileHeader.Machine == IMAGE_FILE_MACHINE_IA64 && mNtHeader.Pe32->OptionalHeader.Magic == EFI_IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
How I wish it is exposed!

Actually, I do think this is somewhat of a concern, considering that
is the SecurityPkg. Having these kinds of exceptions on all
architectures, and in various places in the code, just because one
architecture needs it is not very elegant, especially since only IA64
builds are ever expected to try and load IA64 images in the first
place.

Is there any way we could export PeCoffLoaderGetPeHeaderMagicValue ()
from BasePecoffLib, and special case the implementation so the hack
only gets included on IPF builds? Or will we violate some spec by
doing that?

Regards,
Ard.

Post by Yao, Jiewen
-----Original Message-----
Sent: Tuesday, June 09, 2015 9:22 PM
To: Yao, Jiewen
Subject: Re: [edk2] [patch] MdePkg/PropertiesTable support, MdeModulePkg/DxeCore support UEFI2.5 properties table

Post by Yao, Jiewen
Hi Ard
You are right. I happen to find a logic error, too.
Attach patch for your review.

Yes, that looks fine to me, although it is unfortunate that can't just use PeCoffLoaderGetPeHeaderMagicValue () directly.
It builds fine on GCC/AArch64
Thanks,
Ard.

Post by Yao, Jiewen
-----Original Message-----
Sent: Tuesday, June 09, 2015 8:28 PM
To: Yao, Jiewen
Subject: Re: [edk2] [patch] MdePkg/PropertiesTable support,
MdeModulePkg/DxeCore support UEFI2.5 properties table

Post by Ard Biesheuvel

Post by Yao, Jiewen
Hi Ard
Thanks to report this. I have fixed this in 17565.

Wow, that was quick! Thanks!

You have fixed the build, but the copy-pasted comments are still there, which make no sense at all anymore, since they refer to the Magic variable, which has been removed.
Next time, could you please send your patches (including fixes like
these) to the mailing list first? That way, people have a chance to see what's going in before its gets merged.
Thanks,
Ard.

Post by Ard Biesheuvel

Post by Yao, Jiewen
-----Original Message-----
Sent: Friday, June 05, 2015 9:12 PM
Subject: Re: [edk2] [patch] MdePkg/PropertiesTable support,
MdeModulePkg/DxeCore support UEFI2.5 properties table

Post by Yao, Jiewen
-------------------------------------------------------------------
-
--
--------
_______________________________________________
edk2-devel mailing list
https://lists.sourceforge.net/lists/listinfo/edk2-devel

------------------------------------------------------------------------------

Ard Biesheuvel

2015-06-09 14:56:45 UTC

Permalink

Post by Ard Biesheuvel

Actually, I do think this is somewhat of a concern, considering that
is the SecurityPkg. Having these kinds of exceptions on all
architectures, and in various places in the code, just because one
architecture needs it is not very elegant, especially since only IA64
builds are ever expected to try and load IA64 images in the first
place.
Is there any way we could export PeCoffLoaderGetPeHeaderMagicValue ()
from BasePecoffLib, and special case the implementation so the hack
only gets included on IPF builds? Or will we violate some spec by
doing that?

... or perhaps get rid of the special case entirely? ELILO is dead,
and I doubt anyone using IA64 in production using an affected version
of ELILO would be interested in running the latest Tianocore.
--
Ard.

Post by Ard Biesheuvel

Post by Yao, Jiewen
Hi Ard
You are right. I happen to find a logic error, too.
Attach patch for your review.

Yes, that looks fine to me, although it is unfortunate that can't just use PeCoffLoaderGetPeHeaderMagicValue () directly.
It builds fine on GCC/AArch64
Thanks,
Ard.

Post by Yao, Jiewen
-----Original Message-----
Sent: Tuesday, June 09, 2015 8:28 PM
To: Yao, Jiewen
Subject: Re: [edk2] [patch] MdePkg/PropertiesTable support,
MdeModulePkg/DxeCore support UEFI2.5 properties table

Post by Ard Biesheuvel

Post by Yao, Jiewen
Hi Ard
Thanks to report this. I have fixed this in 17565.

Wow, that was quick! Thanks!

You have fixed the build, but the copy-pasted comments are still there, which make no sense at all anymore, since they refer to the Magic variable, which has been removed.
Next time, could you please send your patches (including fixes like
these) to the mailing list first? That way, people have a chance to see what's going in before its gets merged.
Thanks,
Ard.

Post by Ard Biesheuvel

Post by Yao, Jiewen
-----Original Message-----
Sent: Friday, June 05, 2015 9:12 PM
Subject: Re: [edk2] [patch] MdePkg/PropertiesTable support,
MdeModulePkg/DxeCore support UEFI2.5 properties table

Post by Yao, Jiewen
Hi
Here is patch to support UEFI2.5, chapter 4, section 4.6 Properties
Table feature.
MdePkg – add Properties table definition.
MdeModulePkg – add properties table support in DXE core.
MdeModulePkg – add PropertiesTableAttributesDxe driver to set
ACPINvs/Reserved memory type to be XP.

Post by Yao, Jiewen
-------------------------------------------------------------------
-
--
--------
_______________________________________________
edk2-devel mailing list
https://lists.sourceforge.net/lists/listinfo/edk2-devel

------------------------------------------------------------------------------

Yao, Jiewen

2015-06-09 12:46:16 UTC

Permalink

The correct log should be:
- The SectionAlignment is got from Magic number.
- The Magic number is got from PE header and machine type.

The original code mix them.

Thank you
Yao Jiewen

-----Original Message-----
From: Yao, Jiewen
Sent: Tuesday, June 09, 2015 8:42 PM
To: 'Ard Biesheuvel'
Cc: edk2-***@lists.sourceforge.net; Zeng, Star; Gao, Liming
Subject: RE: [edk2] [patch] MdePkg/PropertiesTable support, MdeModulePkg/DxeCore support UEFI2.5 properties table

Hi Ard
You are right. I happen to find a logic error, too.

Attach patch for your review.

-----Original Message-----
From: Ard Biesheuvel [mailto:***@linaro.org]
Sent: Tuesday, June 09, 2015 8:28 PM
To: Yao, Jiewen
Cc: edk2-***@lists.sourceforge.net; Zeng, Star; Gao, Liming
Subject: Re: [edk2] [patch] MdePkg/PropertiesTable support, MdeModulePkg/DxeCore support UEFI2.5 properties table

Post by Ard Biesheuvel

Post by Yao, Jiewen
Hi Ard
Thanks to report this. I have fixed this in 17565.

Wow, that was quick! Thanks!

You have fixed the build, but the copy-pasted comments are still there, which make no sense at all anymore, since they refer to the Magic variable, which has been removed.

Next time, could you please send your patches (including fixes like
these) to the mailing list first? That way, people have a chance to see what's going in before its gets merged.

Thanks,
Ard.

Post by Ard Biesheuvel

Post by Yao, Jiewen
-----Original Message-----
Sent: Friday, June 05, 2015 9:12 PM
Subject: Re: [edk2] [patch] MdePkg/PropertiesTable support,
MdeModulePkg/DxeCore support UEFI2.5 properties table

Post by Yao, Jiewen
--------------------------------------------------------------------
--
--------
_______________________________________________
edk2-devel mailing list
https://lists.sourceforge.net/lists/listinfo/edk2-devel

------------------------------------------------------------------------------

Laszlo Ersek

2015-06-08 10:34:02 UTC

Permalink

+ SetPropertiesTableSectionAlignment (SectionAlignment);
+ if ((SectionAlignment & (SIZE_4KB - 1)) != 0) {
+ DEBUG ((EFI_D_ERROR, "!!!!!!!! InsertImageRecord - Section Alignment(0x%x) is not 4K !!!!!!!!\n", SectionAlignment));

I just noticed that the above message is printed under OVMF for all
DXE_RUNTIME_DRIVER modules. Why does that happen?

Thanks
Laszlo

+ PdbPointer = PeCoffLoaderGetPdbPointer ((VOID*) (UINTN) ImageAddress);
+ if (PdbPointer != NULL) {
+ DEBUG ((EFI_D_ERROR, "!!!!!!!! Image - %a !!!!!!!!\n", PdbPointer));
+ }
+ goto Finish;
+ }

------------------------------------------------------------------------------

Yao, Jiewen

2015-06-08 13:56:25 UTC

Permalink

Hi Laszlo
This is for UEFI2.5 Properties Table feature. See UEFI 2.5 spec, 4.6 EFI Configuration Table & Properties Table, page 105.

//
// Memory attribute (Not defined bits are reserved)
//
#define EFI_PROPERTIES_RUNTIME_MEMORY_PROTECTION_NON_EXECUTABLE_PE_DATA 0x1
// BIT 0 - description - implies the runtime data is separated from the code
This bit implies that the UEFI runtime code and data sections of the executable image are separate
and aligned to at least a 4KiB boundary. This bit also implies that the data pages do no have any
executable code.

A platform may use gEfiMdeModulePkgTokenSpaceGuid.PropertiesTableEnable to control enable or disable this feature.
In order to meet "secure by default" rule, we define it TRUE as default configuration in MdeModulePkg.

If a platform does not want to enable this feature, it can override this PCD to be FALSE.
If a platform wants to enable this feature, it can override link option to make code/data 4K aligned for any runtime driver.

Thank you
Yao Jiewen

-----Original Message-----
From: Laszlo Ersek [mailto:***@redhat.com]
Sent: Monday, June 08, 2015 6:34 PM
To: Yao, Jiewen
Cc: edk2-***@lists.sourceforge.net
Subject: Re: [edk2] [patch] MdePkg/PropertiesTable support, MdeModulePkg/DxeCore support UEFI2.5 properties table

+ SetPropertiesTableSectionAlignment (SectionAlignment); if
+ ((SectionAlignment & (SIZE_4KB - 1)) != 0) {
+ DEBUG ((EFI_D_ERROR, "!!!!!!!! InsertImageRecord - Section
+ Alignment(0x%x) is not 4K !!!!!!!!\n", SectionAlignment));

I just noticed that the above message is printed under OVMF for all DXE_RUNTIME_DRIVER modules. Why does that happen?

Thanks
Laszlo

+ PdbPointer = PeCoffLoaderGetPdbPointer ((VOID*) (UINTN) ImageAddress);
+ if (PdbPointer != NULL) {
+ DEBUG ((EFI_D_ERROR, "!!!!!!!! Image - %a !!!!!!!!\n", PdbPointer));
+ }
+ goto Finish;
+ }

------------------------------------------------------------------------------

Laszlo Ersek

2015-06-08 14:14:50 UTC

Permalink

Post by Yao, Jiewen
Hi Laszlo
This is for UEFI2.5 Properties Table feature. See UEFI 2.5 spec, 4.6 EFI Configuration Table & Properties Table, page 105.
//
// Memory attribute (Not defined bits are reserved)
//
#define EFI_PROPERTIES_RUNTIME_MEMORY_PROTECTION_NON_EXECUTABLE_PE_DATA 0x1
// BIT 0 - description - implies the runtime data is separated from the code
This bit implies that the UEFI runtime code and data sections of the executable image are separate
and aligned to at least a 4KiB boundary. This bit also implies that the data pages do no have any
executable code.
A platform may use gEfiMdeModulePkgTokenSpaceGuid.PropertiesTableEnable to control enable or disable this feature.
In order to meet "secure by default" rule, we define it TRUE as default configuration in MdeModulePkg.
If a platform does not want to enable this feature, it can override this PCD to be FALSE.
If a platform wants to enable this feature, it can override link option to make code/data 4K aligned for any runtime driver.

I think the last option would be preferable for OVMF as well -- where
can we change that link option? Does it belong with BaseTools, or with
the rules in the FDF file?

Thanks!
Laszlo

Post by Yao, Jiewen
Thank you
Yao Jiewen
-----Original Message-----
Sent: Monday, June 08, 2015 6:34 PM
To: Yao, Jiewen
Subject: Re: [edk2] [patch] MdePkg/PropertiesTable support, MdeModulePkg/DxeCore support UEFI2.5 properties table

I just noticed that the above message is printed under OVMF for all DXE_RUNTIME_DRIVER modules. Why does that happen?
Thanks
Laszlo

+ PdbPointer = PeCoffLoaderGetPdbPointer ((VOID*) (UINTN) ImageAddress);
+ if (PdbPointer != NULL) {
+ DEBUG ((EFI_D_ERROR, "!!!!!!!! Image - %a !!!!!!!!\n", PdbPointer));
+ }
+ goto Finish;
+ }

------------------------------------------------------------------------------

Yao, Jiewen

2015-06-08 14:24:36 UTC

Permalink

That is good question. :-)

1) For VisualStudio, the link option can be changed directly - /ALIGN:4096.
And we need update RELEASE_*_*_DLINK_FLAGS = /MERGE:.data=.text /MERGE:.rdata=.text, to RELEASE_*_*_DLINK_FLAGS = /MERGE:.rdata=.data

2) For GCC, we need another link script, and set .text ALIGN(0x1000) and .data ALIGN(0x1000)
We also need update GenFw/Elf32Convert.c, GenFw/Elf64Convert.c.
================
UINT32 mCoffAlignment = 0x20;
if ((mEhdr->e_entry & 0xFFF) == 0) {
mCoffAlignment = 0x1000;
}
================

3) For Build tool, we will add format: [BuildOptions.$(arch).CodeBase.$(MODULE_TYPE)]
So that a platform may override rule for runtime driver [BuildOptions.common.EDKII.DXE_RUNTIME_DRIVER]

Tool team will prepare all patch for base tool later.

Thank you
Yao Jiewen

-----Original Message-----
From: Laszlo Ersek [mailto:***@redhat.com]
Sent: Monday, June 08, 2015 10:15 PM
To: Yao, Jiewen
Cc: edk2-***@lists.sourceforge.net
Subject: Re: [edk2] [patch] MdePkg/PropertiesTable support, MdeModulePkg/DxeCore support UEFI2.5 properties table

Post by Yao, Jiewen
Hi Laszlo
This is for UEFI2.5 Properties Table feature. See UEFI 2.5 spec, 4.6 EFI Configuration Table & Properties Table, page 105.
//
// Memory attribute (Not defined bits are reserved) // #define
EFI_PROPERTIES_RUNTIME_MEMORY_PROTECTION_NON_EXECUTABLE_PE_DATA 0x1 //
BIT 0 - description - implies the runtime data is separated from the
code This bit implies that the UEFI runtime code and data sections of
the executable image are separate and aligned to at least a 4KiB
boundary. This bit also implies that the data pages do no have any executable code.
A platform may use gEfiMdeModulePkgTokenSpaceGuid.PropertiesTableEnable to control enable or disable this feature.
In order to meet "secure by default" rule, we define it TRUE as default configuration in MdeModulePkg.
If a platform does not want to enable this feature, it can override this PCD to be FALSE.
If a platform wants to enable this feature, it can override link option to make code/data 4K aligned for any runtime driver.

I think the last option would be preferable for OVMF as well -- where can we change that link option? Does it belong with BaseTools, or with the rules in the FDF file?

Thanks!
Laszlo

Post by Yao, Jiewen
Thank you
Yao Jiewen
-----Original Message-----
Sent: Monday, June 08, 2015 6:34 PM
To: Yao, Jiewen
Subject: Re: [edk2] [patch] MdePkg/PropertiesTable support,
MdeModulePkg/DxeCore support UEFI2.5 properties table

I just noticed that the above message is printed under OVMF for all DXE_RUNTIME_DRIVER modules. Why does that happen?
Thanks
Laszlo

+ PdbPointer = PeCoffLoaderGetPdbPointer ((VOID*) (UINTN) ImageAddress);
+ if (PdbPointer != NULL) {
+ DEBUG ((EFI_D_ERROR, "!!!!!!!! Image - %a !!!!!!!!\n", PdbPointer));
+ }
+ goto Finish;
+ }

------------------------------------------------------------------------------

Laszlo Ersek

2015-06-08 22:38:56 UTC

Permalink

Post by Yao, Jiewen
That is good question. :-)
1) For VisualStudio, the link option can be changed directly - /ALIGN:4096.
And we need update RELEASE_*_*_DLINK_FLAGS = /MERGE:.data=.text /MERGE:.rdata=.text, to RELEASE_*_*_DLINK_FLAGS = /MERGE:.rdata=.data
2) For GCC, we need another link script, and set .text ALIGN(0x1000) and .data ALIGN(0x1000)
We also need update GenFw/Elf32Convert.c, GenFw/Elf64Convert.c.
================
UINT32 mCoffAlignment = 0x20;
if ((mEhdr->e_entry & 0xFFF) == 0) {
mCoffAlignment = 0x1000;
}
================
3) For Build tool, we will add format: [BuildOptions.$(arch).CodeBase.$(MODULE_TYPE)]
So that a platform may override rule for runtime driver [BuildOptions.common.EDKII.DXE_RUNTIME_DRIVER]
Tool team will prepare all patch for base tool later.

Great, thank you.
Laszlo

Post by Yao, Jiewen
-----Original Message-----
Sent: Monday, June 08, 2015 10:15 PM
To: Yao, Jiewen
Subject: Re: [edk2] [patch] MdePkg/PropertiesTable support, MdeModulePkg/DxeCore support UEFI2.5 properties table

Post by Yao, Jiewen
Hi Laszlo
This is for UEFI2.5 Properties Table feature. See UEFI 2.5 spec, 4.6 EFI Configuration Table & Properties Table, page 105.
//
// Memory attribute (Not defined bits are reserved) // #define
EFI_PROPERTIES_RUNTIME_MEMORY_PROTECTION_NON_EXECUTABLE_PE_DATA 0x1 //
BIT 0 - description - implies the runtime data is separated from the
code This bit implies that the UEFI runtime code and data sections of
the executable image are separate and aligned to at least a 4KiB
boundary. This bit also implies that the data pages do no have any executable code.
A platform may use gEfiMdeModulePkgTokenSpaceGuid.PropertiesTableEnable to control enable or disable this feature.
In order to meet "secure by default" rule, we define it TRUE as default configuration in MdeModulePkg.
If a platform does not want to enable this feature, it can override this PCD to be FALSE.
If a platform wants to enable this feature, it can override link option to make code/data 4K aligned for any runtime driver.

I think the last option would be preferable for OVMF as well -- where can we change that link option? Does it belong with BaseTools, or with the rules in the FDF file?
Thanks!
Laszlo

Post by Yao, Jiewen
Thank you
Yao Jiewen
-----Original Message-----
Sent: Monday, June 08, 2015 6:34 PM
To: Yao, Jiewen
Subject: Re: [edk2] [patch] MdePkg/PropertiesTable support,
MdeModulePkg/DxeCore support UEFI2.5 properties table

I just noticed that the above message is printed under OVMF for all DXE_RUNTIME_DRIVER modules. Why does that happen?
Thanks
Laszlo

+ PdbPointer = PeCoffLoaderGetPdbPointer ((VOID*) (UINTN) ImageAddress);
+ if (PdbPointer != NULL) {
+ DEBUG ((EFI_D_ERROR, "!!!!!!!! Image - %a !!!!!!!!\n", PdbPointer));
+ }
+ goto Finish;
+ }

------------------------------------------------------------------------------