[edk2] [PATCH v2] ShellPkg/UefiShellDebug1CommandsLib: Remove the unused function CharToUpper

[Shenglei Zhang]

CharToUpper is an unused function, so it will be removed.
https://bugzilla.tianocore.org/show_bug.cgi?id=1399

v2:Update the title.

Cc: Laszlo Ersek 
Cc: Jaben Carsey 
Cc: Ruiyu Ni 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Shenglei Zhang 
---
 .../UefiShellDebug1CommandsLib.c  | 28 ---
 1 file changed, 28 deletions(-)

diff --git 
a/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib.c 
b/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib.c
index bd4dfa98f7..480441b0f9 100644
--- a/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib.c
+++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib.c
@@ -113,34 +113,6 @@ UefiShellDebug1CommandsLibDestructor (
   return (EFI_SUCCESS);
 }
 
-/**
-  Convert a Unicode character to upper case only if
-  it maps to a valid small-case ASCII character.
-
-  This internal function only deal with Unicode character
-  which maps to a valid small-case ASCII character, i.e.
-  L'a' to L'z'. For other Unicode character, the input character
-  is returned directly.
-
-  @param  Char  The character to convert.
-
-  @retval LowerCharacter   If the Char is with range L'a' to L'z'.
-  @retval UnchangedOtherwise.
-
-
-  //Stolen from MdePkg Baselib
-**/
-CHAR16
-CharToUpper (
-  IN  CHAR16Char
-  )
-{
-  if (Char >= L'a' && Char <= L'z') {
-return (CHAR16) (Char - (L'a' - L'A'));
-  }
-
-  return Char;
-}
 
 /**
   Function returns a system configuration table that is stored in the
-- 
2.18.0.windows.1

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

Re: [edk2] [PATCH v3 1/1] ArmVirtPkg: Replace obsoleted network drivers from platform DSC/FDF.

[Ard Biesheuvel]

On Fri, 14 Dec 2018 at 07:11, Fu, Siyuan  wrote:
>
> Hi, Arb
>
> Do you think if it's ok to commit this patch to ArmVirtPkg now?
>

Sure, please go ahead

> > -Original Message-
> > From: Fu, Siyuan
> > Sent: Wednesday, November 7, 2018 8:59 AM
> > To: Ard Biesheuvel 
> > Cc: edk2-devel@lists.01.org; Julien Grall ; Laszlo
> > Ersek 
> > Subject: RE: [PATCH v3 1/1] ArmVirtPkg: Replace obsoleted network drivers 
> > from
> > platform DSC/FDF.
> >
> > Hi, Arb
> >
> > > -Original Message-
> > > From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> > > Sent: Wednesday, November 7, 2018 12:39 AM
> > > To: Laszlo Ersek 
> > > Cc: Fu, Siyuan ; edk2-devel@lists.01.org; Julien
> > > Grall 
> > > Subject: Re: [PATCH v3 1/1] ArmVirtPkg: Replace obsoleted network drivers
> > > from platform DSC/FDF.
> > >
> > > On 6 November 2018 at 16:24, Laszlo Ersek  wrote:
> > > > On 11/06/18 13:32, Ard Biesheuvel wrote:
> > > >> On 6 November 2018 at 02:24, Fu Siyuan  wrote:
> > > >>> V3:
> > > >>> Remove duplicate library added in v2 patch, since ArmVirtPkg.dsc.inc
> > > >>> already have them. Just remove the if...end there is enough.
> > > >>>
> > > >>> V2:
> > > >>> Add missing library instance for NetworkPkg iSCSI driver.
> > > >>>
> > > >>
> > > >> Please don't put the patch revision history in the commit log. Put it
> > > >> below the ---
> > > >>
> > > >>> This patch replaces the MdeModulePkg TCP, PXE and iSCSI driver with
> > > those
> > > >>> ones in NetworkPkg. These 3 drivers in MdeModulePkg are not being
> > > actively
> > > >>> maintained and will be removed from edk2 master soon.
> > > >>>
> > > >>> Cc: Laszlo Ersek 
> > > >>> Cc: Ard Biesheuvel 
> > > >>> Cc: Julien Grall 
> > > >>> Contributed-under: TianoCore Contribution Agreement 1.1
> > > >>> Signed-off-by: Fu Siyuan 
> > > >>> ---
> > > >>
> > > >> ... here ...
> > > >>
> > > >> The patch looks fine to me
> > > >>
> > > >> Reviewed-by: Ard Biesheuvel 
> > > >>
> > > >> but please don't merge it until after the next stable tag has been
> > > created
> > > >
> > > > This is not a bad idea (see also your discussion with Leif); however it
> > > > does create a bit of inconsistency with how the other platform DSC/FDF
> > > > files have been handled. (The changes have been pushed for those.)
> > > >
> > > > Again, I don't disagree, and I don't mind if ArmVirt is handled
> > > > differently. It's just that we should have handled this more uniformly,
> > > > I believe.
> > > >
> > >
> > > Yes - as I replied to Leif, I am not going to obsess about this. But
> > > the point of stable tags is not to rush things in at the last minute.
> >
> > OK I will not commit this change to ArmVirt until the stable tag is made.
> > Sorry for the last minute notification of this change, and I can fully
> > understand your concern, that's why we accepted Leif that still keep
> > these MdeModulePkg drivers in this stable tag.
> >
> > Thanks for review.
> > Siyuan
> >
> > >
> > > >
> > > > In retrospect, I would have also appreciated if the patches had
> > > > referenced , even
> > > > though they only implement "prep" work for now, on the platform DSC/FDF
> > > > level, and not the actual driver removal.
> > > >
> > > > For example, the important explanation about MdeModulePkg's iSCSI driver
> > > > implementing its own MD5 algo cannot be connected to the OVMF commit now
> > > > (d2f1f6423bd1). I have copied the most relevant passage from the cover
> > > > letter of this series into TianoCore BZ#1278, but the commit in question
> > > > doesn't reference any BZ, so the link cannot be established.
> > > >
> > > > Thanks
> > > > Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [edk2-test][Patch] uefi-sct/SctPkg:Correct macro name style in HwErrRecVariable Test

[Jin, Eric]

Hello Supreeth,

I use "Apply Patch Serial" operation provided by TortoiseGit to do the applying.
The operation is equivalent to "git.exe am --3way --ignore-space-change 
--keep-cr Fix.patch"

What is your command to apply patch?

I observe the same failure with "git.exe am Fix.patch" and 
"--ignore-space-change " is the key.
I am not sure if it is the mail-patch-conversion cause or not.

And the patch "[edk2][edk2-test][PATCH v1 1/1] uefi-sct: Change line endings to 
CR LF. "  also has the same failure behavior without "--ignore-space-change " 
on my side. 

Best Regards
Eric

-Original Message-
From: Supreeth Venkatesh  
Sent: Thursday, December 13, 2018 5:07 AM
To: Jin, Eric ; edk2-devel@lists.01.org
Cc: Leif Lindholm 
Subject: Re: [edk2-test][Patch] uefi-sct/SctPkg:Correct macro name style in 
HwErrRecVariable Test


Eric,

Nothing wrong with code.

However, when applying this patch with git am, I encounter the below errors. 
(not sure if it is related to mailbox configuration).
Not sure if it is my mailbox, could you please test it on your side using git 
am and let me know?

git am
./patches/0001_SctPkg\:Correct_macro_name_style_in_HwErrRecVariable_Te
st.mbox
Applying: uefi-sct/SctPkg:Correct macro name style in HwErrRecVariable Test
error: patch failed: uefi-
sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBoxT
est/VariableServicesBBTestMain.h:131
error: uefi-
sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBoxT
est/VariableServicesBBTestMain.h: patch does not apply
error: patch failed: uefi-
sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBoxT
est/VariableServicesBBTestFunction.c:2855
error: uefi-
sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBoxT
est/VariableServicesBBTestFunction.c: patch does not apply Patch failed at 0001 
uefi-sct/SctPkg:Correct macro name style in HwErrRecVariable Test Use 'git am 
--show-current-patch' to see the failed patch When you have resolved this 
problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

Thanks,
Supreeth
On Wed, 2018-12-12 at 11:32 +0800, Eric Jin wrote:
> Name macros appropriately to follow the rule in coding standards 
> specification.
> Change the following macro from variable style 
> HwErrRecVariableNameLength HwErrRecVariableNamePrefixLength 
> HwErrRecVariableNameIndexLength to macro style.
> HW_ERR_REC_VARIABLE_NAME_LEN
> HW_ERR_REC_VARIABLE_NAME_PREFIX_LEN
> HW_ERR_REC_VARIABLE_NAME_INDEX_LEN
> 
> Cc: Leif Lindholm 
> Cc: Supreeth Venkatesh 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Jin 
> ---
>  uefi-
> sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBo
> xTest/VariableServicesBBTestMain.h |  6 +++---
>  uefi-
> sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBo
> xTest/VariableServicesBBTestFunction.c | 22 +++---
>  2 files changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/uefi-
> sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBo
> xTest/VariableServicesBBTestMain.h b/uefi- 
> sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBo
> xTest/VariableServicesBBTestMain.h
> index 426b762..7eaa56d 100644
> --- a/uefi-
> sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBo
> xTest/VariableServicesBBTestMain.h
> +++ b/uefi-
> sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBo
> xTest/VariableServicesBBTestMain.h
> @@ -131,9 +131,9 @@ Abstract:
>  // The prefix length is 8, index length is 4.
>  // Consider the tail of string, the name length is 13.
>  //
> -#define HwErrRecVariableNameLength   13
> -#define HwErrRecVariableNamePrefixLength 8 -#define 
> HwErrRecVariableNameIndexLength  4
> +#define HW_ERR_REC_VARIABLE_NAME_LEN   13
> +#define HW_ERR_REC_VARIABLE_NAME_PREFIX_LEN8
> +#define HW_ERR_REC_VARIABLE_NAME_INDEX_LEN 4
>  
>  //
>  // Global Variables
> diff --git a/uefi-
> sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBo
> xTest/VariableServicesBBTestFunction.c b/uefi- 
> sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBo
> xTest/VariableServicesBBTestFunction.c
> index a016476..015a78a 100644
> --- a/uefi-
> sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBo
> xTest/VariableServicesBBTestFunction.c
> +++ b/uefi-
> sct/SctPkg/TestCase/UEFI/EFI/RuntimeServices/VariableServices/BlackBo
> xTest/VariableServicesBBTestFunction.c
> @@ -2855,7 +2855,7 @@ HardwareErrorRecordFuncTest (
>UINT64RemainingVariableStorageSize;
>UINT64MaximumVariableSize;
>
> -  CHAR16HwErrRecVariableName[HwErrRecVariableNameLen
> gth];
> +  CHAR16HwErrRecVariableName[HW_ERR_REC_VARIABLE_NAM
> E_LEN];
>CHAR16HwErrRecVariable[] = L"

[edk2] [PATCH] Edk2Platforms: Replace MdeModulePkg PXE/iSCSI/TCP with NetworkPkg Drivers.

[Fu Siyuan]

The PXE/iSCSI/TCP drivers in MdeModulePkg are going to be deprecated. All
platform DSC/FDF files should be updated to use the dual-stack drivers in
NetworkPkg.

Cc: Michael A Kubacki 
Cc: Jiewen Yao 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Fu Siyuan 
---
 Platform/Intel/AdvancedFeaturePkg/Include/Dsc/CoreAdvancedDxeInclude.dsc  | 7 
++-
 Platform/Intel/AdvancedFeaturePkg/Include/Fdf/CoreAdvancedLateInclude.fdf | 7 
++-
 Platform/Intel/MinPlatformPkg/Include/Dsc/CoreCommonLib.dsc   | 3 
+--
 3 files changed, 5 insertions(+), 12 deletions(-)

diff --git 
a/Platform/Intel/AdvancedFeaturePkg/Include/Dsc/CoreAdvancedDxeInclude.dsc 
b/Platform/Intel/AdvancedFeaturePkg/Include/Dsc/CoreAdvancedDxeInclude.dsc
index 4d70db6062..6764d46131 100644
--- a/Platform/Intel/AdvancedFeaturePkg/Include/Dsc/CoreAdvancedDxeInclude.dsc
+++ b/Platform/Intel/AdvancedFeaturePkg/Include/Dsc/CoreAdvancedDxeInclude.dsc
@@ -1,7 +1,7 @@
 ## @file
 #  Platform description.
 #
-# Copyright (c) 2017, Intel Corporation. All rights reserved.
+# Copyright (c) 2017 - 2018, Intel Corporation. All rights reserved.
 #
 # This program and the accompanying materials are licensed and made available 
under
 # the terms and conditions of the BSD License which accompanies this 
distribution.
@@ -26,10 +26,7 @@
   MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Dxe.inf
   MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Dxe.inf
   MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Dxe.inf
-  MdeModulePkg/Universal/Network/Tcp4Dxe/Tcp4Dxe.inf
   MdeModulePkg/Universal/Network/Udp4Dxe/Udp4Dxe.inf
-  MdeModulePkg/Universal/Network/IScsiDxe/IScsiDxe.inf
-  #MdeModulePkg/Universal/Network/UefiPxeBcDxe/UefiPxeBcDxe.inf
 
   NetworkPkg/Ip6Dxe/Ip6Dxe.inf
   NetworkPkg/TcpDxe/TcpDxe.inf
@@ -42,7 +39,7 @@
   NetworkPkg/HttpUtilitiesDxe/HttpUtilitiesDxe.inf
   NetworkPkg/HttpBootDxe/HttpBootDxe.inf
 
-  #NetworkPkg/IScsiDxe/IScsiDxe.inf
+  NetworkPkg/IScsiDxe/IScsiDxe.inf
   NetworkPkg/UefiPxeBcDxe/UefiPxeBcDxe.inf
 !endif
 
diff --git 
a/Platform/Intel/AdvancedFeaturePkg/Include/Fdf/CoreAdvancedLateInclude.fdf 
b/Platform/Intel/AdvancedFeaturePkg/Include/Fdf/CoreAdvancedLateInclude.fdf
index 0be408d13b..64f1dd5872 100644
--- a/Platform/Intel/AdvancedFeaturePkg/Include/Fdf/CoreAdvancedLateInclude.fdf
+++ b/Platform/Intel/AdvancedFeaturePkg/Include/Fdf/CoreAdvancedLateInclude.fdf
@@ -1,7 +1,7 @@
 ## @file
 #  FDF file of Platform.
 #
-# Copyright (c) 2017, Intel Corporation. All rights reserved.
+# Copyright (c) 2017 - 2018, Intel Corporation. All rights reserved.
 #
 # This program and the accompanying materials are licensed and made available 
under
 # the terms and conditions of the BSD License which accompanies this 
distribution.
@@ -27,9 +27,6 @@ INF  MdeModulePkg/Universal/Network/Dhcp4Dxe/Dhcp4Dxe.inf
 INF  MdeModulePkg/Universal/Network/Ip4Dxe/Ip4Dxe.inf
 INF  MdeModulePkg/Universal/Network/Mtftp4Dxe/Mtftp4Dxe.inf
 INF  MdeModulePkg/Universal/Network/Udp4Dxe/Udp4Dxe.inf
-#INF  MdeModulePkg/Universal/Network/Tcp4Dxe/Tcp4Dxe.inf
-INF  MdeModulePkg/Universal/Network/IScsiDxe/IScsiDxe.inf
-#INF  MdeModulePkg/Universal/Network/UefiPxeBcDxe/UefiPxeBcDxe.inf
 
 INF  NetworkPkg/Ip6Dxe/Ip6Dxe.inf
 INF  NetworkPkg/TcpDxe/TcpDxe.inf
@@ -42,7 +39,7 @@ INF  NetworkPkg/HttpDxe/HttpDxe.inf
 INF  NetworkPkg/HttpUtilitiesDxe/HttpUtilitiesDxe.inf
 INF  NetworkPkg/HttpBootDxe/HttpBootDxe.inf
 
-#INF  NetworkPkg/IScsiDxe/IScsiDxe.inf
+INF  NetworkPkg/IScsiDxe/IScsiDxe.inf
 INF  NetworkPkg/UefiPxeBcDxe/UefiPxeBcDxe.inf
 !endif
 
diff --git a/Platform/Intel/MinPlatformPkg/Include/Dsc/CoreCommonLib.dsc 
b/Platform/Intel/MinPlatformPkg/Include/Dsc/CoreCommonLib.dsc
index 2174eaa609..dd0173a1af 100644
--- a/Platform/Intel/MinPlatformPkg/Include/Dsc/CoreCommonLib.dsc
+++ b/Platform/Intel/MinPlatformPkg/Include/Dsc/CoreCommonLib.dsc
@@ -1,7 +1,7 @@
 ## @file
 #  Platform description.
 #
-# Copyright (c) 2017, Intel Corporation. All rights reserved.
+# Copyright (c) 2017 - 2018, Intel Corporation. All rights reserved.
 #
 # This program and the accompanying materials are licensed and made available 
under
 # the terms and conditions of the BSD License which accompanies this 
distribution.
@@ -83,7 +83,6 @@
   UdpIoLib|MdeModulePkg/Library/DxeUdpIoLib/DxeUdpIoLib.inf
   TcpIoLib|MdeModulePkg/Library/DxeTcpIoLib/DxeTcpIoLib.inf
   DpcLib|MdeModulePkg/Library/DxeDpcLib/DxeDpcLib.inf
-  TcpIoLib|MdeModulePkg/Library/DxeTcpIoLib/DxeTcpIoLib.inf
   
OemHookStatusCodeLib|MdeModulePkg/Library/OemHookStatusCodeLibNull/OemHookStatusCodeLibNull.inf
   CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.inf
 
-- 
2.19.1.windows.1

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

Re: [edk2] [PATCH 0/2] Add two public functions

[Zhang, Shenglei]

Hi Leif

Thanks for your constructive guide. I 'll improve my later patches.

Thanks,
Shenglei

> -Original Message-
> From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
> Sent: Thursday, December 13, 2018 6:52 PM
> To: Zhang, Shenglei 
> Cc: edk2-devel@lists.01.org; Kinney, Michael D
> ; Gao, Liming 
> Subject: Re: [edk2] [PATCH 0/2] Add two public functions
> 
> Please let the subject line give some sort of hint of what is being
> done, and to what. "Add two functions" is not substantially more
> descriptive than "add 572 characters".
> 
> In this case, your're moving previously internal string helper functions
> to BaseLib.
> 
> On Thu, Dec 13, 2018 at 04:34:37PM +0800, Shenglei Zhang wrote:
> > Add two public functions,CharToUpper and AsciiToUpper,and
> > remove the same functional functions,InternalCharToUpper
> > and InternalBaseLibAsciiToUpper.
> >
> > Cc: Michael D Kinney 
> > Cc: Liming Gao 
> > Cc: Bob Feng 
> > Cc: Yonghong Zhu 
> > Shenglei Zhang (2):
> >   MdePkg/BaseLib: Add two public functions
> >   BaseTools/Common: Remove InternalCharToUpper
> >
> >  BaseTools/Source/C/Common/CommonLib.c | 16 ++---
> >  BaseTools/Source/C/Common/CommonLib.h |  4 ---
> >  MdePkg/Include/Library/BaseLib.h  | 40 +
> >  MdePkg/Library/BaseLib/BaseLibInternals.h | 42 ---
> >  MdePkg/Library/BaseLib/SafeString.c   |  8 ++---
> >  MdePkg/Library/BaseLib/String.c   | 35 ---
> >  6 files changed, 53 insertions(+), 92 deletions(-)
> >
> > --
> > 2.18.0.windows.1
> >
> > ___
> > 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

Re: [edk2] [PATCH v3 1/1] ArmVirtPkg: Replace obsoleted network drivers from platform DSC/FDF.

[Fu, Siyuan]

Hi, Arb

Do you think if it's ok to commit this patch to ArmVirtPkg now?

BestRegards
Fu Siyuan

> -Original Message-
> From: Fu, Siyuan
> Sent: Wednesday, November 7, 2018 8:59 AM
> To: Ard Biesheuvel 
> Cc: edk2-devel@lists.01.org; Julien Grall ; Laszlo
> Ersek 
> Subject: RE: [PATCH v3 1/1] ArmVirtPkg: Replace obsoleted network drivers from
> platform DSC/FDF.
> 
> Hi, Arb
> 
> > -Original Message-
> > From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> > Sent: Wednesday, November 7, 2018 12:39 AM
> > To: Laszlo Ersek 
> > Cc: Fu, Siyuan ; edk2-devel@lists.01.org; Julien
> > Grall 
> > Subject: Re: [PATCH v3 1/1] ArmVirtPkg: Replace obsoleted network drivers
> > from platform DSC/FDF.
> >
> > On 6 November 2018 at 16:24, Laszlo Ersek  wrote:
> > > On 11/06/18 13:32, Ard Biesheuvel wrote:
> > >> On 6 November 2018 at 02:24, Fu Siyuan  wrote:
> > >>> V3:
> > >>> Remove duplicate library added in v2 patch, since ArmVirtPkg.dsc.inc
> > >>> already have them. Just remove the if...end there is enough.
> > >>>
> > >>> V2:
> > >>> Add missing library instance for NetworkPkg iSCSI driver.
> > >>>
> > >>
> > >> Please don't put the patch revision history in the commit log. Put it
> > >> below the ---
> > >>
> > >>> This patch replaces the MdeModulePkg TCP, PXE and iSCSI driver with
> > those
> > >>> ones in NetworkPkg. These 3 drivers in MdeModulePkg are not being
> > actively
> > >>> maintained and will be removed from edk2 master soon.
> > >>>
> > >>> Cc: Laszlo Ersek 
> > >>> Cc: Ard Biesheuvel 
> > >>> Cc: Julien Grall 
> > >>> Contributed-under: TianoCore Contribution Agreement 1.1
> > >>> Signed-off-by: Fu Siyuan 
> > >>> ---
> > >>
> > >> ... here ...
> > >>
> > >> The patch looks fine to me
> > >>
> > >> Reviewed-by: Ard Biesheuvel 
> > >>
> > >> but please don't merge it until after the next stable tag has been
> > created
> > >
> > > This is not a bad idea (see also your discussion with Leif); however it
> > > does create a bit of inconsistency with how the other platform DSC/FDF
> > > files have been handled. (The changes have been pushed for those.)
> > >
> > > Again, I don't disagree, and I don't mind if ArmVirt is handled
> > > differently. It's just that we should have handled this more uniformly,
> > > I believe.
> > >
> >
> > Yes - as I replied to Leif, I am not going to obsess about this. But
> > the point of stable tags is not to rush things in at the last minute.
> 
> OK I will not commit this change to ArmVirt until the stable tag is made.
> Sorry for the last minute notification of this change, and I can fully
> understand your concern, that's why we accepted Leif that still keep
> these MdeModulePkg drivers in this stable tag.
> 
> Thanks for review.
> Siyuan
> 
> >
> > >
> > > In retrospect, I would have also appreciated if the patches had
> > > referenced , even
> > > though they only implement "prep" work for now, on the platform DSC/FDF
> > > level, and not the actual driver removal.
> > >
> > > For example, the important explanation about MdeModulePkg's iSCSI driver
> > > implementing its own MD5 algo cannot be connected to the OVMF commit now
> > > (d2f1f6423bd1). I have copied the most relevant passage from the cover
> > > letter of this series into TianoCore BZ#1278, but the commit in question
> > > doesn't reference any BZ, so the link cannot be established.
> > >
> > > Thanks
> > > Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [PATCH] BaseTools: Add $(INC)-like support when compiling .nasm files

[Yonghong Zhu]

From: zhijufan 

current edk2\BaseTools\Conf\build_rule.template, the compile of nasm
source files does not have the $(INC) support.

The '-I' option only includes the directory of the nasm source file
(${s_path}(+)). Hence, it will be impossible for nasm files to include
files outside of the nasm source file directory.

As a comparison, the compile of both .s and .asm have $(INC) support
in their compile commands.

Fixes: https://bugzilla.tianocore.org/show_bug.cgi?id=1085
Cc: Liming Gao 
Cc: Yonghong Zhu 
Cc: Bob Feng 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Zhiju.Fan 
---
 BaseTools/Source/Python/AutoGen/GenMake.py | 20 +++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/BaseTools/Source/Python/AutoGen/GenMake.py 
b/BaseTools/Source/Python/AutoGen/GenMake.py
index d94d8f9368..ef7bc845d0 100644
--- a/BaseTools/Source/Python/AutoGen/GenMake.py
+++ b/BaseTools/Source/Python/AutoGen/GenMake.py
@@ -167,7 +167,7 @@ class BuildFile(object):
 "gmake" :   "include"
 }
 
-_INC_FLAG_ = {TAB_COMPILER_MSFT : "/I", "GCC" : "-I", "INTEL" : "-I", 
"RVCT" : "-I"}
+_INC_FLAG_ = {TAB_COMPILER_MSFT : "/I", "GCC" : "-I", "INTEL" : "-I", 
"RVCT" : "-I", "NASM" : "-I"}
 
 ## Constructor of BuildFile
 #
@@ -596,6 +596,24 @@ cleanlib:
 }
 )
 FileMacroList.append(FileMacro)
+# Add support when compiling .nasm source files
+for File in self.FileCache.keys():
+if not str(File).endswith('.nasm'):
+continue
+IncludePathList = []
+for P in  MyAgo.IncludePathList:
+IncludePath = self._INC_FLAG_['NASM'] + self.PlaceMacro(P, 
self.Macros)
+if IncludePath.endswith(os.sep):
+IncludePath = IncludePath.rstrip(os.sep)
+# When compiling .nasm files, need to add a literal backslash 
at each path
+# To specify a literal backslash at the end of the line, 
precede it with a caret (^)
+if P == MyAgo.IncludePathList[-1] and os.sep == '\\':
+IncludePath = ''.join([IncludePath, '^', os.sep])
+else:
+IncludePath = os.path.join(IncludePath, '')
+IncludePathList.append(IncludePath)
+
FileMacroList.append(self._FILE_MACRO_TEMPLATE.Replace({"macro_name": 
"NASM_INC", "source_file": IncludePathList}))
+break
 
 # Generate macros used to represent files containing list of input 
files
 for ListFileMacro in self.ListFileMacros:
-- 
2.18.0.windows.1

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

[edk2] [PATCH 2/2] BaseTools: Update nasm file build rule to support $(INC)

[Yonghong Zhu]

From: zhijufan 

Update the build rule to:
"$(NASM)" -I${s_path}(+) $(NASM_INC) $(NASM_FLAGS)
-o $dst ${d_path}(+)${s_base}.iii

Cc: Liming Gao 
Cc: Yonghong Zhu 
Cc: Bob Feng 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Zhiju.Fan 
---
 BaseTools/Conf/build_rule.template | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/BaseTools/Conf/build_rule.template 
b/BaseTools/Conf/build_rule.template
index 3ab560603f..2a53d7ed63 100755
--- a/BaseTools/Conf/build_rule.template
+++ b/BaseTools/Conf/build_rule.template
@@ -62,6 +62,7 @@
 #   $(BIN_DIR)  Common directory for executable files
 #   $(FV_DIR)   Directory to store flash image files
 #   $(INC)  Search path of current module
+#   $(NASM_INC) Search nasm file path of current module
 #   $(INC_LIST) A file containing search pathes of current module
 #   $(LIBS) Static library files of current module
 #   $(_FLAGS) Tools flags of current module
@@ -225,7 +226,7 @@
 
 "$(PP)" $(PP_FLAGS) $(INC) ${src} > ${d_path}(+)${s_base}.i
 Trim --trim-long --source-code -o ${d_path}(+)${s_base}.iii 
${d_path}(+)${s_base}.i
-"$(NASM)" -I${s_path}(+) $(NASM_FLAGS) -o $dst 
${d_path}(+)${s_base}.iii
+"$(NASM)" -I${s_path}(+) $(NASM_INC) $(NASM_FLAGS) -o $dst 
${d_path}(+)${s_base}.iii
 
 [Device-Tree-Source-File]
 
-- 
2.18.0.windows.1

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

[edk2] [Patch 0/2] Update .nasm to support $(INC)-like support

[Yonghong Zhu]

From: zhijufan 

Fixes: https://bugzilla.tianocore.org/show_bug.cgi?id=1085
Cc: Liming Gao 
Cc: Yonghong Zhu 
Cc: Bob Feng 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Zhiju.Fan 

zhijufan (2):
  BaseTools: Add $(INC)-like support when compiling .nasm files
  BaseTools: Update nasm file build rule to support $(INC)

 BaseTools/Conf/build_rule.template |  3 +-
 BaseTools/Source/Python/AutoGen/GenMake.py | 20 +-
 2 files changed, 21 insertions(+), 2 deletions(-)

-- 
2.6.1.windows.1

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

[edk2] [PATCH] BaseTools:Break build if same [LibraryClasses] name is used

[Yonghong Zhu]

From: zhijufan 

For example if both PackageA and PackageB declare the
[LibraryClasses] name of MyLibClass, that is mapped to
an include file in each package:

PackageA.dec

[LibraryClasses]
  MyLibClass|Include/Library/MyLibClass.h

PackageB.dec

[LibraryClasses]
  MyLibClass|Include/Library/MyLibClass.h

Cc: Liming Gao 
Cc: Yonghong Zhu 
Cc: Bob Feng 
Fixes: https://bugzilla.tianocore.org/show_bug.cgi?id=954
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Zhiju.Fan 
---
 .../Python/Workspace/WorkspaceCommon.py   | 19 ++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/BaseTools/Source/Python/Workspace/WorkspaceCommon.py 
b/BaseTools/Source/Python/Workspace/WorkspaceCommon.py
index 55d01fa4b2..b05ed9 100644
--- a/BaseTools/Source/Python/Workspace/WorkspaceCommon.py
+++ b/BaseTools/Source/Python/Workspace/WorkspaceCommon.py
@@ -20,7 +20,8 @@ from Workspace.BuildClassObject import StructurePcd
 from Common.BuildToolError import RESOURCE_NOT_AVAILABLE
 from Common.BuildToolError import OPTION_MISSING
 from Common.BuildToolError import BUILD_ERROR
-
+from Common.BuildToolError import OPTION_CONFLICT
+from Common import EdkLogger as EdkLogger
 class OrderedListDict(OrderedDict):
 def __init__(self, *args, **kwargs):
 super(OrderedListDict, self).__init__(*args, **kwargs)
@@ -48,6 +49,21 @@ def GetPackageList(Platform, BuildDatabase, Arch, Target, 
Toolchain):
 PkgSet.update(Lib.Packages)
 return list(PkgSet)
 
+# Check that the LibraryClasses name are the same
+def CheckLibraryClassesName(PkgList):
+LibraryDict = {}
+for Pkg in PkgList:
+LibCls = Pkg.LibraryClasses
+for Lib in LibCls:
+if Lib in LibraryDict:
+if LibraryDict[Lib] != str(Pkg):
+EdkLogger.error("build", OPTION_CONFLICT,
+"ClassName is conflicted,Please change the 
ClassName",
+ExtraData="in [%s] [%s]\n\tClassName is 
[%s]" % (
+LibraryDict[Lib], str(Pkg), Lib))
+else:
+LibraryDict[Lib] = str(Pkg)
+
 ## Get all declared PCD from platform for specified arch, target and toolchain
 #
 #  @param Platform: DscBuildData instance
@@ -60,6 +76,7 @@ def GetPackageList(Platform, BuildDatabase, Arch, Target, 
Toolchain):
 #
 def GetDeclaredPcd(Platform, BuildDatabase, Arch, Target, Toolchain, 
additionalPkgs):
 PkgList = GetPackageList(Platform, BuildDatabase, Arch, Target, Toolchain)
+CheckLibraryClassesName(PkgList)
 PkgList = set(PkgList)
 PkgList |= additionalPkgs
 DecPcds = {}
-- 
2.18.0.windows.1

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

Re: [edk2] [PATCH] Maintainers.txt: Change package maintainer and reviewer of CryptoPkg.

[Ye, Ting]

Thanks all for the info and follow up. Laszlo, please let me know if there is 
additional step you think I need follow, thanks.

Best Regards,
Ting

From: Long, Qin
Sent: Friday, December 14, 2018 2:30 AM
To: Gao, Liming ; Laszlo Ersek ; Ye, 
Ting 
Cc: edk2-devel@lists.01.org
Subject: RE: [edk2] [PATCH] Maintainers.txt: Change package maintainer and 
reviewer of CryptoPkg.

Confirmed by Long, Qin mailto:qin.l...@intel.com>>

(And sorry for this rule breaking caused by me. I didn't notice this updates.)

Best Regards & Thanks,
LONG, Qin

From: Gao, Liming
Sent: Thursday, December 13, 2018 9:15 PM
To: Laszlo Ersek mailto:ler...@redhat.com>>; Ye, Ting 
mailto:ting...@intel.com>>; Long, Qin 
mailto:qin.l...@intel.com>>
Cc: edk2-devel@lists.01.org
Subject: RE: [edk2] [PATCH] Maintainers.txt: Change package maintainer and 
reviewer of CryptoPkg.

Laszlo:
  Yes. Long, Qin should send this patch. Because Long, Qin changes to another 
work group for a while, he doesn't work on edk2 project. Ting directly sends 
the patch to remove his name. I just include Long, Qin, and let him confirm 
this change.

Thanks
Liming
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo 
> Ersek
> Sent: Thursday, December 13, 2018 6:38 PM
> To: Ye, Ting mailto:ting...@intel.com>>
> Cc: edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH] Maintainers.txt: Change package maintainer and 
> reviewer of CryptoPkg.
>
> Hi Ting,
>
> On 12/13/18 08:41, tye1 wrote:
> > Cc: Gang Wei mailto:gang@intel.com>>
> > Cc: Jian Wang mailto:jian.j.w...@intel.com>>
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Ting Ye mailto:ting...@intel.com>>
> > ---
> >  Maintainers.txt | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/Maintainers.txt b/Maintainers.txt
> > index 001d8ba010..d5cb305da9 100644
> > --- a/Maintainers.txt
> > +++ b/Maintainers.txt
> > @@ -102,8 +102,9 @@ S: Maintained
> >
> >  CryptoPkg
> >  W: https://github.com/tianocore/tianocore.github.io/wiki/CryptoPkg
> > -M: Qin Long mailto:qin.l...@intel.com>>
> >  M: Ting Ye mailto:ting...@intel.com>>
> > +R: Gang Wei mailto:gang@intel.com>>
> > +R: Jian Wang mailto:jian.j.w...@intel.com>>
> >
> >  DynamicTablesPkg
> >  W: https://github.com/tianocore/tianocore.github.io/wiki/DynamicTablesPkg
> >
>
> This patch does not conform to the rule that we added lately; please see
> commit 9ebef6c0a7d3 ("Maintainers.txt: Add the rule to hand over the
> package maintain role", 2018-11-29).
>
> In other words, the patch should be sent out by Qin Long. Even though
> you co-maintain CryptoPkg with Qin Long, you shouldn't be able to
> deprive Qin Long from the role.
>
> 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

[edk2] [PATCH v3] ArmPkg/ArmScmiDxe: Add clock enable function

[Jeff Brasen]

Add function to allow enabling and disabling of the clock using the SCMI
interface. Add gArmScmiClock2ProtocolGuid to distinguish platforms that
support new API from those that just have the older protocol.

SCMI_CLOCK2_PROTOCOL also adds a version parameter to allow for future
changes. It is placed after the functions that are present in the
existing protocol to allow SCMI_CLOCK2_PROTOCOL to be cast to
SCMI_CLOCK_PROTOCOL so that only a single implementation of those
function are needed.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jeff Brasen 
Cc: Ard Biesheuvel 
Cc: Leif Lindholm 
Cc: Grish Pathak 
---
 ArmPkg/ArmPkg.dec  |   1 +
 .../ArmScmiDxe/ArmScmiClockProtocolPrivate.h   |   7 +
 ArmPkg/Drivers/ArmScmiDxe/ArmScmiDxe.inf   |   1 +
 ArmPkg/Drivers/ArmScmiDxe/ScmiClockProtocol.c  |  62 +++
 ArmPkg/Include/Protocol/ArmScmiClock2Protocol.h| 198 +
 5 files changed, 269 insertions(+)
 create mode 100644 ArmPkg/Include/Protocol/ArmScmiClock2Protocol.h

diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec
index d99eb67..2f5e5b3 100644
--- a/ArmPkg/ArmPkg.dec
+++ b/ArmPkg/ArmPkg.dec
@@ -59,6 +59,7 @@
   ## Arm System Control and Management Interface(SCMI) Clock management 
protocol
   ## ArmPkg/Include/Protocol/ArmScmiClockProtocol.h
   gArmScmiClockProtocolGuid = { 0x91ce67a8, 0xe0aa, 0x4012, { 0xb9, 0x9f, 
0xb6, 0xfc, 0xf3, 0x4, 0x8e, 0xaa } }
+  gArmScmiClock2ProtocolGuid = { 0xb8d8caf2, 0x9e94, 0x462c, { 0xa8, 0x34, 
0x6c, 0x99, 0xfc, 0x05, 0xef, 0xcf } }
 
   ## Arm System Control and Management Interface(SCMI) Clock management 
protocol
   ## ArmPkg/Include/Protocol/ArmScmiPerformanceProtocol.h
diff --git a/ArmPkg/Drivers/ArmScmiDxe/ArmScmiClockProtocolPrivate.h 
b/ArmPkg/Drivers/ArmScmiDxe/ArmScmiClockProtocolPrivate.h
index 0d1ec6f..c135bac 100644
--- a/ArmPkg/Drivers/ArmScmiDxe/ArmScmiClockProtocolPrivate.h
+++ b/ArmPkg/Drivers/ArmScmiDxe/ArmScmiClockProtocolPrivate.h
@@ -59,6 +59,13 @@ typedef struct {
   CLOCK_RATE_DWORD Rate;
 } CLOCK_RATE_SET_ATTRIBUTES;
 
+
+// Message parameters for CLOCK_CONFIG_SET command.
+typedef struct {
+  UINT32 ClockId;
+  UINT32 Attributes;
+} CLOCK_CONFIG_SET_ATTRIBUTES;
+
 //  if ClockAttr Bit[0] is set then clock device is enabled.
 #define CLOCK_ENABLE_MASK 0x1
 #define CLOCK_ENABLED(ClockAttr)  ((ClockAttr & CLOCK_ENABLE_MASK) == 1)
diff --git a/ArmPkg/Drivers/ArmScmiDxe/ArmScmiDxe.inf 
b/ArmPkg/Drivers/ArmScmiDxe/ArmScmiDxe.inf
index 05ce9c0..9b29b9f 100644
--- a/ArmPkg/Drivers/ArmScmiDxe/ArmScmiDxe.inf
+++ b/ArmPkg/Drivers/ArmScmiDxe/ArmScmiDxe.inf
@@ -46,6 +46,7 @@
 [Protocols]
   gArmScmiBaseProtocolGuid
   gArmScmiClockProtocolGuid
+  gArmScmiClock2ProtocolGuid
   gArmScmiPerformanceProtocolGuid
 
 [Depex]
diff --git a/ArmPkg/Drivers/ArmScmiDxe/ScmiClockProtocol.c 
b/ArmPkg/Drivers/ArmScmiDxe/ScmiClockProtocol.c
index 64d2afa..c7f27a3 100644
--- a/ArmPkg/Drivers/ArmScmiDxe/ScmiClockProtocol.c
+++ b/ArmPkg/Drivers/ArmScmiDxe/ScmiClockProtocol.c
@@ -19,6 +19,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "ArmScmiClockProtocolPrivate.h"
 #include "ScmiPrivate.h"
@@ -388,6 +389,53 @@ ClockRateSet (
   return Status;
 }
 
+/** Enable/Disable specified clock.
+
+  @param[in]  ThisA Pointer to SCMI_CLOCK_PROTOCOL Instance.
+  @param[in]  ClockId Identifier for the clock device.
+  @param[in]  Enable  TRUE to enable, FALSE to disable.
+
+  @retval EFI_SUCCESS  Clock enable/disable successful.
+  @retval EFI_DEVICE_ERROR SCP returns an SCMI error.
+  @retval !(EFI_SUCCESS)   Other errors.
+**/
+STATIC
+EFI_STATUS
+ClockEnable (
+  IN SCMI_CLOCK2_PROTOCOL *This,
+  IN UINT32   ClockId,
+  IN BOOLEAN  Enable
+  )
+{
+  EFI_STATUS  Status;
+  CLOCK_CONFIG_SET_ATTRIBUTES *ClockConfigSetAttributes;
+  SCMI_COMMANDCmd;
+  UINT32  PayloadLength;
+
+  Status = ScmiCommandGetPayload ((UINT32**)&ClockConfigSetAttributes);
+  if (EFI_ERROR (Status)) {
+return Status;
+  }
+
+  // Fill arguments for clock protocol command.
+  ClockConfigSetAttributes->ClockId= ClockId;
+  ClockConfigSetAttributes->Attributes = Enable ? BIT0 : 0;
+
+  Cmd.ProtocolId = SCMI_PROTOCOL_ID_CLOCK;
+  Cmd.MessageId  = SCMI_MESSAGE_ID_CLOCK_CONFIG_SET;
+
+  PayloadLength = sizeof (CLOCK_CONFIG_SET_ATTRIBUTES);
+
+  // Execute and wait for response on a SCMI channel.
+  Status = ScmiCommandExecute (
+ &Cmd,
+ &PayloadLength,
+ NULL
+ );
+
+  return Status;
+}
+
 // Instance of the SCMI clock management protocol.
 STATIC CONST SCMI_CLOCK_PROTOCOL ScmiClockProtocol = {
   ClockGetVersion,
@@ -398,6 +446,18 @@ STATIC CONST SCMI_CLOCK_PROTOCOL ScmiClockProtocol = {
   ClockRateSet
  };
 
+// Instance of the SCMI clock management protocol.
+STATIC CONST SCMI_CLOCK2_PROTOCOL ScmiClock2Protocol = {
+  (SCMI_CLOCK2_GET_VERSION)Clo

Re: [edk2] [PATCH v2] ArmPkg/ArmScmiDxe: Add clock enable function

[Jeff Brasen]

-Original Message-
From: Leif Lindholm  
Sent: Wednesday, December 12, 2018 11:49 AM
To: Ard Biesheuvel 
Cc: Jeff Brasen ; edk2-devel@lists.01.org; Girish Pathak 

Subject: Re: [PATCH v2] ArmPkg/ArmScmiDxe: Add clock enable function

On Thu, Dec 06, 2018 at 06:09:26PM +0100, Ard Biesheuvel wrote:
> > -Original Message-
> > From: Ard Biesheuvel 
> > Sent: Thursday, December 6, 2018 9:54 AM
> > To: Jeff Brasen 
> > Cc: edk2-devel@lists.01.org; Leif Lindholm 
> > ; Girish Pathak 
> > Subject: Re: [PATCH v2] ArmPkg/ArmScmiDxe: Add clock enable function
> >
> > On Thu, 6 Dec 2018 at 01:37, Jeff Brasen  wrote:
> > >
> > > Leif/Ard,
> > >
> > >
> > >   Any comments on this v2 patch for this?
> > >
> > >
> >
> > Hi Jeff,
> >
> > I'm not sure what level of bikeshedding is justified when it comes 
> > to a driver such as this one, which is very recent, and mostly for 
> > platform internal use. However, I will note that the current 
> > versioning approach permits a *client* of the old 
> > SCMI_CLOCK_PROTOCOL to be built that invokes ->Enable(), which is 
> > not defined for it. This somewhat defeats the purpose of the 
> > versioning, since the whole point is to avoid invoking ->Enable() on 
> > older implementations of the protocol.
> >
> > I'd be fine with just modifying the protocol, but if we decide we 
> > need versioning, we should not modify the public interface of the 
> > old one.
> > How the driver reuses one implementation to back the other is another 
> > matter, of course.
> > [JMB] I can either just change without versioning (that was my 
> > original approach but I also changed the guid which would primarily 
> > catch new clients running on old platforms from calling an undefined 
> > function), I am fine with either that (with maybe a switch back to 
> > original guid if we are not concerned about that
> > issue) or a future update that creates a full v2 version of the 
> > protocol in the header.
> 
> Maybe Leif disagrees, but I am not too concerned about just changing 
> it. This is not a protocol that 3rd party drivers would invoke, right?

It's a protocol that a 3rd party driver _could_ invoke.
Whether that is a likely thing to happen, I just don't know.
Or whether BIOS vendors cherry-picking things badly would cause interesting 
things to happen.

On the one hand, I would prefer to see a complete version duplication of the 
protocol, just so we _won't_ let existing apps/drivers call the Enable function.

On the other hand, I don't think this would be the last protocol update we 
would ever see, and moving to an internally versioned interface may make more 
sense (i.e. adding Version and possibly Size
fields) would be more resilient for that. But that would still require a full 
Protocol2 implementation.

At which point, if we don't want to add the Version field, we may just change 
the original and see if we break anything?
[JMB] If we add a version to this we should probably add one to the other SCMI 
protocols as well for consistency right? I'll make a cleaner version of v2 that 
separates the protocols better and upload that

/
Leif
---
This email message is for the sole use of the intended recipient(s) and may 
contain
confidential information.  Any unauthorized review, use, disclosure or 
distribution
is prohibited.  If you are not the intended recipient, please contact the 
sender by
reply email and destroy all copies of the original message.
---
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] Obtaining TCG final events on systems without TCG2 log support

[Matthew Garrett]

On Thu, Dec 13, 2018 at 01:36:09PM +0100, Laszlo Ersek wrote:

> (2) EFI_TCG2_FINAL_EVENTS_TABLE is defined with TCG_PCR_EVENT2 entries
> *only*. TCG_PCR_EVENT is not accommodated.
> 
> 
> That's the contradiction. If a platform is unable to produce
> TCG_PCR_EVENT2 entries in GetEventLog(), it is fairly certainly also
> unable to produce them in the final events table.

If a platform is unable to produce them in the final events table then 
it's violating the spec. If the platform only offers the 1.2 log format 
then it seems reasonable to expect that the events in the final events 
table would only contain a SHA1, but a TCG_PCR_EVENT2 structure that 
only contains SHA1s isn't significantly more complicated than an old 
style event.

-- 
Matthew Garrett | mj...@srcf.ucam.org
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] Obtaining TCG final events on systems without TCG2 log support

[Matthew Garrett]

I don't see how that follows - regardless of whether or not we'd like to 
deprecate SHA1 support, people use it. There's little value in having an 
incomplete event log.

On Thu, Dec 13, 2018 at 01:23:35PM +, Yao, Jiewen wrote:
> Right.
> I think we are trying to deprecate the old SHA1 support, because SHA1 is 
> considered as unsecure algorithm.
> We are moving to crypto agile. As such, we do not see the need to support old 
> style event log.
> 
> Thank you
> Yao Jiewen
> 
> 
> > -Original Message-
> > From: Laszlo Ersek [mailto:ler...@redhat.com]
> > Sent: Thursday, December 13, 2018 8:36 PM
> > To: Matthew Garrett 
> > Cc: edk2-devel@lists.01.org; Yao, Jiewen ;
> > Marc-André Lureau ; Stefan Berger
> > 
> > Subject: Re: [edk2] Obtaining TCG final events on systems without TCG2 log
> > support
> > 
> > + Jiewen, Marc-André, Stefan
> > 
> > On 12/13/18 02:17, Matthew Garrett wrote:
> > > SetupEventLog() in Tcg2Dxe.c only installs the final event log
> > > configuration table if SupportedEventLogs includes the TCG2 log format.
> > > If the platform only supports the TCG1.2 log format then the final
> > > events table isn't installed. However, ExitBootServices() should
> > > generate an event even on systems that don't support the TCG2 log
> > > format. How is an OS supposed to obtain the log of the
> > > ExitBootServices() events in that case?
> > >
> > 
> > I don't think it can.
> > 
> > You probably refer to the code below the comment "No need to handle
> > EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2", in SetupEventLog(). This code
> > dates
> > back to commit fd46e831bc33 ("SecurityPkg: Update final event log
> > calculation.", 2016-01-18). And the commit message says, "... there is
> > no need to record TCG12 format log to final event log area ...".
> > 
> > Hence, the code is intentional. I even think the code is valid
> > (according to the spec [*]); I just think the commit message should have
> > said, "there is no *way* to record TCG12 format log to final event log
> > area". Because, IMO, the bug is in the spec.
> > 
> > [*] TCG EFI Protocol Specification
> > Family “2.0”
> > Level 00 Revision 00.13
> > March 30, 2016
> > 
> > Here's why I think it's a spec bug:
> > 
> > 
> > (1) If EFI_TCG2_EVENT_LOG_FORMAT_TCG_2 is *clear* in
> > SupportedEventLogs,
> > then the platform advertizes GetEventLog() as unable to produce the
> > crypto agile log format.
> > 
> > In other words, the platform is unable to produce a log which consists
> > of TCG_PCR_EVENT2 entries, beyond the sole TCG_PCR_EVENT ("SHA1
> > format")
> > header entry.
> > 
> > Accordingly, GetEventLog() will fail with EFI_INVALID_PARAMETER, when
> > called with EventLogFormat=EFI_TCG2_EVENT_LOG_FORMAT_TCG_2. (BTW,
> > I
> > think EFI_UNSUPPORTED would have been better for this, but I digress.)
> > 
> > (2) EFI_TCG2_FINAL_EVENTS_TABLE is defined with TCG_PCR_EVENT2
> > entries
> > *only*. TCG_PCR_EVENT is not accommodated.
> > 
> > 
> > That's the contradiction. If a platform is unable to produce
> > TCG_PCR_EVENT2 entries in GetEventLog(), it is fairly certainly also
> > unable to produce them in the final events table.
> > 
> > And, while the first *instance* of the limitation is conformant, via
> > SupportedEventLogs, the second instance of the same limitation isn't.
> > 
> > Thanks,
> > Laszlo
-- 
Matthew Garrett | mj...@srcf.ucam.org
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] Maintainers.txt: Change package maintainer and reviewer of CryptoPkg.

[Long, Qin]

Confirmed by Long, Qin mailto:qin.l...@intel.com>>

(And sorry for this rule breaking caused by me. I didn't notice this updates.)

Best Regards & Thanks,
LONG, Qin

From: Gao, Liming
Sent: Thursday, December 13, 2018 9:15 PM
To: Laszlo Ersek ; Ye, Ting ; Long, Qin 

Cc: edk2-devel@lists.01.org
Subject: RE: [edk2] [PATCH] Maintainers.txt: Change package maintainer and 
reviewer of CryptoPkg.

Laszlo:
  Yes. Long, Qin should send this patch. Because Long, Qin changes to another 
work group for a while, he doesn't work on edk2 project. Ting directly sends 
the patch to remove his name. I just include Long, Qin, and let him confirm 
this change.

Thanks
Liming
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo 
> Ersek
> Sent: Thursday, December 13, 2018 6:38 PM
> To: Ye, Ting mailto:ting...@intel.com>>
> Cc: edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH] Maintainers.txt: Change package maintainer and 
> reviewer of CryptoPkg.
>
> Hi Ting,
>
> On 12/13/18 08:41, tye1 wrote:
> > Cc: Gang Wei mailto:gang@intel.com>>
> > Cc: Jian Wang mailto:jian.j.w...@intel.com>>
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Ting Ye mailto:ting...@intel.com>>
> > ---
> >  Maintainers.txt | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/Maintainers.txt b/Maintainers.txt
> > index 001d8ba010..d5cb305da9 100644
> > --- a/Maintainers.txt
> > +++ b/Maintainers.txt
> > @@ -102,8 +102,9 @@ S: Maintained
> >
> >  CryptoPkg
> >  W: https://github.com/tianocore/tianocore.github.io/wiki/CryptoPkg
> > -M: Qin Long mailto:qin.l...@intel.com>>
> >  M: Ting Ye mailto:ting...@intel.com>>
> > +R: Gang Wei mailto:gang@intel.com>>
> > +R: Jian Wang mailto:jian.j.w...@intel.com>>
> >
> >  DynamicTablesPkg
> >  W: https://github.com/tianocore/tianocore.github.io/wiki/DynamicTablesPkg
> >
>
> This patch does not conform to the rule that we added lately; please see
> commit 9ebef6c0a7d3 ("Maintainers.txt: Add the rule to hand over the
> package maintain role", 2018-11-29).
>
> In other words, the patch should be sent out by Qin Long. Even though
> you co-maintain CryptoPkg with Qin Long, you shouldn't be able to
> deprive Qin Long from the role.
>
> 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

Re: [edk2] [PATCH edk2-platforms 16/27] Silicon/NXP: Add i.MX6 Timer DXE driver

[Leif Lindholm]

On Fri, Sep 21, 2018 at 08:26:08AM +, Chris Co wrote:
> This adds DXE support for EPIT timer on NXP i.MX6 SoCs.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Christopher Co 
> Cc: Ard Biesheuvel 
> Cc: Leif Lindholm 
> Cc: Michael D Kinney 
> ---
>  Silicon/NXP/iMX6Pkg/Drivers/TimerDxe/Timer.c  | 278 
>  Silicon/NXP/iMX6Pkg/Drivers/TimerDxe/TimerDxe.inf |  55 
>  2 files changed, 333 insertions(+)
> 
> diff --git a/Silicon/NXP/iMX6Pkg/Drivers/TimerDxe/Timer.c 
> b/Silicon/NXP/iMX6Pkg/Drivers/TimerDxe/Timer.c
> new file mode 100644
> index ..6b4db6185b48
> --- /dev/null
> +++ b/Silicon/NXP/iMX6Pkg/Drivers/TimerDxe/Timer.c
> @@ -0,0 +1,278 @@
> +/** @file
> +*
> +*  Copyright (c) 2018 Microsoft Corporation. All rights reserved.
> +*
> +*  This program and the accompanying materials
> +*  are licensed and made available under the terms and conditions of the BSD 
> License
> +*  which accompanies this distribution.  The full text of the license may be 
> found at
> +*  http://opensource.org/licenses/bsd-license.php
> +*
> +*  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +*  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
> IMPLIED.
> +*
> +**/
> +
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +
> +// The notification function to call on every timer interrupt.
> +volatile EFI_TIMER_NOTIFY mTimerNotifyFunction = (EFI_TIMER_NOTIFY) NULL;
> +EFI_EVENT EfiExitBootServicesEvent = (EFI_EVENT) NULL;
> +
> +// Cached copy of the Hardware Interrupt protocol instance
> +EFI_HARDWARE_INTERRUPT_PROTOCOL *gInterrupt = NULL;
> +
> +// Cached interrupt vector
> +volatile UINTN  mVector;
> +UINT64 mCurrentTimerPeriod;
> +
> +EFI_STATUS
> +EFIAPI
> +TimerDriverRegisterHandler (
> +  IN EFI_TIMER_ARCH_PROTOCOL  *This,
> +  IN EFI_TIMER_NOTIFY NotifyFunction
> +  )
> +{
> +  DEBUG ((DEBUG_VERBOSE, "++TimerDriverRegisterHandler()\n"));

Please use %a and __FUNCTION__ rather than manually typing the
function name. (Throughout the set.)

> +  if ((NotifyFunction == NULL) && (mTimerNotifyFunction == NULL)) {
> +return EFI_INVALID_PARAMETER;
> +  }
> +
> +  if ((NotifyFunction != NULL) && (mTimerNotifyFunction != NULL)) {
> +return EFI_ALREADY_STARTED;
> +  }
> +
> +  mTimerNotifyFunction = NotifyFunction;
> +  DEBUG ((DEBUG_VERBOSE, "--TimerDriverRegisterHandler()=ok\n"));
> +  return EFI_SUCCESS;
> +}
> +
> +EFI_STATUS
> +EFIAPI
> +TimerDriverSetTimerPeriod (
> +  IN EFI_TIMER_ARCH_PROTOCOL  *This,
> +  IN UINT64   TimerPeriod
> +  )
> +{
> +  PCSP_EPIT_REG   pEpit;
> +  UINT16  EpitPreScalar;
> +  EFI_STATUS  Status;
> +  UINT32  TimerCount;
> +  UINT32  Value;
> +
> +  DEBUG ((DEBUG_VERBOSE, "++TimerDriverSetTimerPeriod(%d)\n", TimerPeriod));
> +
> +  pEpit = (PCSP_EPIT_REG) CSP_BASE_REG_PA_EPIT1;
> +  DEBUG ((DEBUG_VERBOSE,
> +  "TimerDriverSetTimerPeriod() disable timer. EPIT_REG adr=%p\n", 
> pEpit));
> +
> +  // First stop the timer.
> +  Value = MmioRead32 ((UINTN)&pEpit->CR);
> +  Value &= ~(((1 << EPIT_CR_EN_WID) - 1) << EPIT_CR_EN_LSH);

Just to clarify comment from last email: I did mean global
search/replace on _LSH to _SHIFT for the entire set.

> +  Value |= (EPIT_CR_EN_DISABLE << EPIT_CR_EN_LSH);
> +  MmioWrite32 ((UINTN)&pEpit->CR, Value);
> +
> +  if (TimerPeriod == 0) {
> +Status = gInterrupt->DisableInterruptSource (gInterrupt, mVector);
> +mCurrentTimerPeriod = 0;
> +DEBUG ((DEBUG_VERBOSE, "--TimerDriverSetTimerPeriod() Timer 
> Disabled\n"));
> +return Status;
> +  }
> +
> +  // Configure EPIT to be sourced from iMX6 24 MHz crystal oscialltor
> +  // Aim to have UEFI tick counting at 1 MHz clock or another frequency as 
> set in pcd
> +  EpitPreScalar = 68;

That 68 needs a #define.

> +  DEBUG ((DEBUG_VERBOSE,
> +  "TimerDriverSetTimerPeriod() using corrected EPIT prescalar=%d\n",
> +  EpitPreScalar));
> +
> +  MmioWrite32 ((UINTN)&pEpit->CR,
> +(EPIT_CR_ENMOD_LOAD << EPIT_CR_ENMOD_LSH) |
> +(EPIT_CR_OCIEN_ENABLE << EPIT_CR_OCIEN_LSH) |
> +(EPIT_CR_RLD_RELOAD << EPIT_CR_RLD_LSH) |
> +((EpitPreScalar - 1) << EPIT_CR_PRESCALAR_LSH) |
> +(EPIT_CR_SWR_NORESET << EPIT_CR_SWR_LSH) |
> +(EPIT_CR_IOVW_OVR << EPIT_CR_IOVW_LSH) |
> +(EPIT_CR_DBGEN_ACTIVE << EPIT_CR_DBGEN_LSH) |
> +(EPIT_CR_WAITEN_ENABLE << EPIT_CR_WAITEN_LSH) |
> +(EPIT_CR_DOZEN_ENABLE << EPIT_CR_DOZEN_LSH) |
> +(EPIT_CR_STOPEN_ENABLE << EPIT_CR_STOPEN_LSH) |
> +(EPIT_CR_OM_DICONNECT << EPIT_CR_OM_LSH) |
> +(EPIT_CR_CLKSRC_IPGCLK << EPIT_CR_CLKSRC_LSH));
> +
> +  // Clear timer compare interrupt flag (write-1-clear)
> +  MmioWrite32 ((UINTN)&pEpit->SR, ((1 << EPIT_SR_OCI

Re: [edk2] [PATCH edk2-platforms 15/27] Silicon/NXP: Add i.MX6 GPT Timer library

[Leif Lindholm]

On Fri, Sep 21, 2018 at 08:26:07AM +, Chris Co wrote:
> This adds support for GPT Timer on NXP i.MX6 SoCs.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Christopher Co 
> Cc: Ard Biesheuvel 
> Cc: Leif Lindholm 
> Cc: Michael D Kinney 
> ---
>  Silicon/NXP/iMX6Pkg/Include/iMX6Timer.h   |  24 ++
>  Silicon/NXP/iMX6Pkg/Library/TimerLib/TimerLib.c   | 246 
>  Silicon/NXP/iMX6Pkg/Library/TimerLib/TimerLib.inf |  45 
>  3 files changed, 315 insertions(+)
> 
> diff --git a/Silicon/NXP/iMX6Pkg/Include/iMX6Timer.h 
> b/Silicon/NXP/iMX6Pkg/Include/iMX6Timer.h
> new file mode 100644
> index ..fbac9d2a61c0
> --- /dev/null
> +++ b/Silicon/NXP/iMX6Pkg/Include/iMX6Timer.h
> @@ -0,0 +1,24 @@
> +/** @file
> +*
> +*  Copyright (c) 2018 Microsoft Corporation. All rights reserved.
> +*
> +*  This program and the accompanying materials
> +*  are licensed and made available under the terms and conditions of the BSD 
> License
> +*  which accompanies this distribution.  The full text of the license may be 
> found at
> +*  http://opensource.org/licenses/bsd-license.php
> +*
> +*  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +*  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
> IMPLIED.
> +*
> +**/
> +
> +#ifndef _IMX6_TIMER_H_
> +#define _IMX6_TIMER_H_
> +
> +RETURN_STATUS
> +EFIAPI
> +TimerConstructor (
> +  VOID
> +  );
> +
> +#endif  /* _IMX6_TIMER_H_ */
> diff --git a/Silicon/NXP/iMX6Pkg/Library/TimerLib/TimerLib.c 
> b/Silicon/NXP/iMX6Pkg/Library/TimerLib/TimerLib.c
> new file mode 100644
> index ..fa55cee242ef
> --- /dev/null
> +++ b/Silicon/NXP/iMX6Pkg/Library/TimerLib/TimerLib.c
> @@ -0,0 +1,246 @@
> +/** @file
> +*
> +*  Copyright (c) 2018 Microsoft Corporation. All rights reserved.
> +*
> +*  This program and the accompanying materials
> +*  are licensed and made available under the terms and conditions of the BSD 
> License
> +*  which accompanies this distribution.  The full text of the license may be 
> found at
> +*  http://opensource.org/licenses/bsd-license.php
> +*
> +*  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +*  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
> IMPLIED.
> +*
> +**/
> +
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +
> +RETURN_STATUS
> +EFIAPI
> +TimerConstructor (
> +  VOID
> +  )
> +{
> +  PCSP_GPT_REGS pGpt;
> +  UINT32 FreqPreScale;
> +
> +  pGpt = (PCSP_GPT_REGS)CSP_BASE_REG_PA_GPT;
> +
> +  ASSERT (SOC_OSC_FREQUENCY_REF_HZ >= PcdGet32 (PcdArmArchTimerFreqInHz));

This line strikes me as slightly counterintuitive.
Are you reusing the Arch Timer Pcd for a custom timer?

If this is the case, we should still be able to share some of the code
from the ArmPkg ArmArchTimerLib by breaking it out and sharing it
(like we did with TimeBaseLib).

> +
> +  // Calculate the scale factor since we are using the 24Mhz oscillator
> +  // as reference.
> +  FreqPreScale = SOC_OSC_FREQUENCY_REF_HZ / PcdGet32 
> (PcdArmArchTimerFreqInHz);
> +  ASSERT (FreqPreScale <= (1 << GPT_PR_PRESCALER_WID));
> +
> +  // Set the frequency scale
> +  MmioWrite32 ((UINTN)&pGpt->PR, FreqPreScale - 1);
> +
> +#if defined(CPU_IMX6DQ) || defined (CPU_IMX6DQP)
> +  // Set GPT configuration:
> +  // - GPT Enabled
> +  // - Use the 24Mhz oscillator source
> +  MmioWrite32 ((UINTN)&pGpt->CR,
> +(GPT_CR_EN_ENABLE << GPT_CR_EN_LSH) |

Can you do a global search and replace _LSH/_SHIFT?

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

Re: [edk2] [PATCH edk2-platforms 11/27] Silicon/NXP: Add i.MX6 SoC header files

[Leif Lindholm]

On Fri, Sep 21, 2018 at 08:26:02AM +, Chris Co wrote:
> This adds includes for NXP i.MX6 SoC family, specifically Dual/Quad,
> Solo/DualLite, SoloX, DualPlus/QuadPlus families.
> These are the header files for managing clocks, IoMux, and general
> SoC register layout information.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Christopher Co 
> Cc: Ard Biesheuvel 
> Cc: Leif Lindholm 
> Cc: Michael D Kinney 
> ---
>  Silicon/NXP/iMX6Pkg/Include/iMX6.h   |   39 +
>  Silicon/NXP/iMX6Pkg/Include/iMX6BoardLib.h   |   55 +
>  Silicon/NXP/iMX6Pkg/Include/iMX6ClkPwr.h |  105 +
>  Silicon/NXP/iMX6Pkg/Include/iMX6ClkPwr_DQ.h  |  181 ++
>  Silicon/NXP/iMX6Pkg/Include/iMX6ClkPwr_SDL.h |  176 ++
>  Silicon/NXP/iMX6Pkg/Include/iMX6ClkPwr_SX.h  |  190 ++
>  Silicon/NXP/iMX6Pkg/Include/iMX6IoMux.h  |  202 ++
>  Silicon/NXP/iMX6Pkg/Include/iMX6IoMux_DQP.h  | 2466 
>  Silicon/NXP/iMX6Pkg/Include/iMX6IoMux_SDL.h  | 1875 +++
>  Silicon/NXP/iMX6Pkg/Include/iMX6IoMux_SX.h   | 2270 ++
>  Silicon/NXP/iMX6Pkg/Include/iMX6_DQ.h|  332 +++
>  Silicon/NXP/iMX6Pkg/Include/iMX6_DQP.h   |  335 +++
>  Silicon/NXP/iMX6Pkg/Include/iMX6_SDL.h   |  301 +++
>  Silicon/NXP/iMX6Pkg/Include/iMX6_SX.h| 1730 ++
>  Silicon/NXP/iMX6Pkg/Include/iMX6_common.h| 1350 +++
>  15 files changed, 11607 insertions(+)
> 

> diff --git a/Silicon/NXP/iMX6Pkg/Include/iMX6.h 
> b/Silicon/NXP/iMX6Pkg/Include/iMX6.h
> new file mode 100644
> index ..ded03eced048
> --- /dev/null
> +++ b/Silicon/NXP/iMX6Pkg/Include/iMX6.h
> @@ -0,0 +1,39 @@
> +/** @file
> +*
> +*  Copyright (c) 2018 Microsoft Corporation. All rights reserved.
> +*
> +*  This program and the accompanying materials
> +*  are licensed and made available under the terms and conditions of the BSD 
> License
> +*  which accompanies this distribution.  The full text of the license may be 
> found at
> +*  http://opensource.org/licenses/bsd-license.php
> +*
> +*  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +*  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
> IMPLIED.
> +*
> +**/
> +
> +#ifndef __IMX6_H__
> +#define __IMX6_H__
> +
> +// Platform specific definition
> +#define EFI_ACPI_OEM_TABLE_ID  
> SIGNATURE_64('I','M','X','6','E','D','K','2')
> +#define EFI_ACPI_OEM_REVISION  0x01000101
> +#define EFI_ACPI_CREATOR_IDSIGNATURE_32('I','M','X','6')
> +#define EFI_ACPI_CREATOR_REVISION  0x0001
> +
> +#if defined(CPU_IMX6DQ)
> +#include "iMX6_DQ.h"
> +#elif defined(CPU_IMX6DQP)
> +#include "iMX6_DQP.h"
> +#elif defined(CPU_IMX6SDL)
> +#include "iMX6_SDL.h"
> +#elif defined(CPU_IMX6SX)
> +#include "iMX6_SX.h"
> +#else
> +#error iMX6 CPU Type Not Defined! (Preprocessor Flag)
> +#endif
> +
> +#define SERIAL_DEBUG_PORT_INIT_MSG "\r\nDebug Serial Port Init\r\n"
> +#define SERIAL_PORT_INIT_MSG "UART"
> +
> +#endif // __IMX6_H__
> diff --git a/Silicon/NXP/iMX6Pkg/Include/iMX6BoardLib.h 
> b/Silicon/NXP/iMX6Pkg/Include/iMX6BoardLib.h
> new file mode 100644
> index ..7997ebc72897
> --- /dev/null
> +++ b/Silicon/NXP/iMX6Pkg/Include/iMX6BoardLib.h
> @@ -0,0 +1,55 @@
> +/** @file
> +*
> +*  Copyright (c) 2018 Microsoft Corporation. All rights reserved.
> +*
> +*  This program and the accompanying materials
> +*  are licensed and made available under the terms and conditions of the BSD 
> License
> +*  which accompanies this distribution.  The full text of the license may be 
> found at
> +*  http://opensource.org/licenses/bsd-license.php
> +*
> +*  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +*  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
> IMPLIED.
> +*
> +**/
> +
> +#ifndef _IMX6_BOARD_LIB_H_
> +#define _IMX6_BOARD_LIB_H_
> +
> +/*
> +  Mandatory functions to implement by the board library.
> +*/
> +
> +VOID
> +ImxClkPwrInit (
> +  );
> +
> +/*
> +  Optional functions to implement by the board library.
> +  The default implementation of these functions if not overridden is NOOP.
> +*/
> +
> +VOID
> +SdhcInit (
> +  );
> +
> +VOID
> +EhciInit (
> +  );
> +
> +VOID
> +I2cInit (
> +  );
> +
> +VOID
> +SpiInit (
> +  );
> +
> +VOID
> +PcieInit (
> +  );
> +
> +VOID
> +SetupAudio (
> +  );
> +
> +#endif // _IMX6_BOARD_LIB_H_
> diff --git a/Silicon/NXP/iMX6Pkg/Include/iMX6ClkPwr.h 
> b/Silicon/NXP/iMX6Pkg/Include/iMX6ClkPwr.h
> new file mode 100644
> index ..18262751c443
> --- /dev/null
> +++ b/Silicon/NXP/iMX6Pkg/Include/iMX6ClkPwr.h
> @@ -0,0 +1,105 @@
> +/** @file
> +*
> +*  Copyright (c) 2018 Microsoft Corporation. All rights reserved.
> +*  Copyright 2018 NXP
> +*
> +*  This program and the accompanying materials
> +*  are licensed and made available under the terms and conditions of the BSD 
> License
> +*  which accompanies this distribution.  The full text of the license may be 
> found at
> +*  http://opensource.org/l

Re: [edk2] [RFC PATCH v4 00/12] Extend secure variable service to be usable from Standalone MM

[Gao, Liming]

I add my comments. 

> -Original Message-
> From: Jagadeesh Ujja [mailto:jagadeesh.u...@arm.com]
> Sent: Thursday, December 13, 2018 8:00 PM
> To: Gao, Liming 
> Cc: edk2-devel@lists.01.org; Zhang, Chao B ; Leif 
> Lindholm 
> Subject: Re: [edk2] [RFC PATCH v4 00/12] Extend secure variable service to be 
> usable from Standalone MM
> 
> Hi Liming
> 
> On Wed, Dec 12, 2018 at 8:44 PM Gao, Liming  wrote:
> >
> > This version is better. I have some comments on edk2 coding style.
> 
> Thank you for your review. Please see reply to your comments below.
> 
> >
> > 1. This patch set can't be applied in edk2 trunk. Seemly, they base on 
> > previous version edk2.
> 
> The v4 patchset was based on the tip of the edk2 master branch on the
> day it was posted. The commit id on which this series was based is
> "f7f94ffe".
> 
So, can you fork edk2 tree and upload these changes into your branch in fork 
edk2 tree? If so, it will be easy for review. 

> > 2. Pcd is for Standalone MM Code, not specific for Variable. So, I suggest 
> > to use the generic name PcdStandaloneMmCodeEnabled. Its
> description is also required to be updated.
> 
> The intention of the changes done in the patchset is to reuse the
> variable service driver in MM_STANDALONE mode. There could be
> platforms that enable Standalone MM mode but would not want a  secure
> storage for EFI variables. In which case, the PCD named
> PcdStandaloneMmCodeEnabled would not be sufficient. And this the
> reason it was named " PcdStandaloneMmVariableEnabled".
> 
I see this PCD is also used in 
CryptoPkg/Library/BaseCryptLib/SysCall/TimerWrapper.c. So, I understand it is 
general purpose, not only for Variable. 
If it is for Variable only, please define this PCD into MdeModulePkg instead of 
MdePkg. 

> > 3. Library header file name (StandaloneMmServicesTableLib.h) is also 
> > library class name. Library class name and header file mapping
> is required to be listed in MdePkg.dec file [LibraryClasses] section. And, 
> this header file doesn't need to include Library/DebugLib.h,
> because it doesn't depend on it.
> > 4. Library implementation INF file (StandaloneMmRuntimeDxe.inf) should list 
> > its library class name in LIBRARY_CLASS of [Defines]
> section. Its library class name is StandaloneMmServicesTableLib. And, MdePkg 
> library implementation depends on MdePkg.dec only in
> [Packages] section.
> > 5. Library implementation should implement all interfaces defined in 
> > library class header file. StandaloneMmRuntimeDxe library
> should initialize gMmst as NULL if it has no real value. 
> StandaloneMmRuntimeDxe library doesn't depend on any other library class. It
> doesn't need to list other library class in its [LibraryClasses] section of 
> INF file.
> 
> Point 3, 4 and 5 will be fixed
> 
> > 6. When other module depends on this library class header file, it should 
> > list StandaloneMmServicesTableLib in its [LibraryClasses]
> section of INF file.
> > 7. Platform DSC also needs to list LibraryClassName|Library implementation 
> > INF in [LibraryClasses] section.
> 
> Points 6 and 7 are taken care and are part of edk2platform specific
> changes, will post those changes soon
> 
> > 8. I don't suggest to add AsmLfence API in BaseLib for AArch64, because it 
> > is X86 specific API. I suggest to update Variable driver with
> the wrapper function FenceFunc() for AsmLfence() and MemoryFence(). FenceFunc 
> can be implemented for the different arch in
> Variable driver. Variable driver will call FenceFunc() instead of 
> AsmLfence(). So, only variable driver is required to be updated. There is
> no change in BaseLib.
> >
> Okay, the variable driver can be updated to call a wrapper
> "FenceFunc()" but wouldn't it be useful to add the architecture
> specific implantation of this in BaseLib. In that way, not just the
> variable driver but other drivers can use this implementation of
> "FenceFunc()". For instance,
> FaultTolerantWriteDxe/FaultTolerantWriteSmm.c does calls to
> AsmLfence() and an architecture specific implementation of
> "FenceFunc()" in BaseLib can be reused in FaultTolerantWriteDxe driver
> as well.
> 
Now, there are two drivers VariableSmm and FaultTolerantWriteSmm that have this 
usage. For sharing the code, I prefer not to add new API, and 
change AsmLfence() as the generic API for all processors. It is similar to 
current patch. But, I would like to see one AsmLfence() declaration in 
BaseLib.h.
Its description can list the different behavior for the different processors. 
Then, you add its implementation for Arm and AArch64 arch. 

> > > -Original Message-
> > > From: Jagadeesh Ujja [mailto:jagadeesh.u...@arm.com]
> > > Sent: Tuesday, December 11, 2018 2:22 PM
> > > To: edk2-devel@lists.01.org; Gao, Liming ; Zhang, 
> > > Chao B ;
> leif.lindh...@linaro.org
> > > Subject: [RFC PATCH v4 00/12] Extend secure variable service to be usable 
> > > from Standalone MM
> > >
> > > Changes since v3:
> > > - Addressed all the comments f

Re: [edk2] [Patch] BaseTools: Optimize string concatenation

[Gao, Liming]

Leif:
  Agree your point to prepare the data before the patch instead of after the 
patch. 

Thanks
Liming
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Leif 
> Lindholm
> Sent: Thursday, December 13, 2018 6:09 PM
> To: Gao, Liming 
> Cc: Carsey, Jaben ; edk2-devel@lists.01.org
> Subject: Re: [edk2] [Patch] BaseTools: Optimize string concatenation
> 
> Hi Liming,
> 
> Yes, this is fine.
> But for future submissions, I would like for this sort of information
> to be provided at the time of posting.
> 
> Not the full breakdown, but "reduceces build time of platform XXX on
> hardware YYY by A%/B seconds".
> 
> Ideally, more than one platform and more than one hardware should be
> provided, but at least during this initial improvement phase I'm also
> happy for the assumption being that unless someone else complains,
> it's fine on others.
> 
> Regards,
> 
> Leif
> 
> On Thu, Dec 13, 2018 at 01:50:35AM +, Gao, Liming wrote:
> > Leif:
> >   Kabylake platform is the real Intel hardware.  The MinKabylake is the 
> > minimal feature of the Kabylake BIOS. Here is MinKabylake
> BIOS code https://github.com/tianocore/edk2-platforms/tree/devel-MinPlatform
> >   Bob adds the build performance data of MinKabylake into 
> > https://bugzilla.tianocore.org/show_bug.cgi?id=1288.
> >
> > The original build performance data:
> > Build Duration:   00:02:23
> > AutoGen Duration: 00:00:42
> > Make Duration:00:01:12
> > GenFds Duration:  00:00:27
> >
> > After apply the patch, clean build performance is reduced from 2:23 to 
> > 1:57. So, I think his patch improves build performance.
> > Build Duration:   00:01:57
> > AutoGen Duration: 00:00:23
> > Make Duration:00:01:12
> > GenFds Duration:  00:00:21
> >
> > Thanks
> > Liming
> > >-Original Message-
> > >From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Leif
> > >Lindholm
> > >Sent: Thursday, December 13, 2018 2:37 AM
> > >To: Feng, Bob C 
> > >Cc: Carsey, Jaben ; edk2-devel@lists.01.org; Gao,
> > >Liming 
> > >Subject: Re: [edk2] [Patch] BaseTools: Optimize string concatenation
> > >
> > >On Tue, Dec 11, 2018 at 08:48:19AM +, Feng, Bob C wrote:
> > >> Hi Leif,
> > >>
> > >> I understand your concern.
> > >>
> > >> I collected another performance data set based on open source
> > >> MinKabylake platform and updated the BZ
> > >> https://bugzilla.tianocore.org/show_bug.cgi?id=1288. The data looks
> > >> better than Ovmf. It enabled multiple SKU.
> > >>
> > >> Before I sent those patch, I did verify them on intel real
> > >> platforms. It improves the build performance. But it's not
> > >> convenient to share those data.
> > >
> > >So, I have two comments on this:
> > >1) How can it be inconvenient to share information on build times? I
> > >   don't even care what the names or codenames for those platforms
> > >   are. If you are unable to tell us why what you have done matters,
> > >   the code changes do not belong in the public tree.
> > >   Clearly having good performance numbers for public platforms is the
> > >   easiest solution for this problem.
> > >2) Submissions of improvements to build system performance should be
> > >   verified building real platforms. It should not be a question of
> > >   "find some other platform to get numbers from once we have improved
> > >   performance for building our confidential platforms".
> > >
> > >Regards,
> > >
> > >Leif
> > >>
> > >> Thanks,
> > >> Bob
> > >>
> > >> -Original Message-
> > >> From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
> > >> Sent: Monday, December 10, 2018 8:36 PM
> > >> To: Feng, Bob C 
> > >> Cc: edk2-devel@lists.01.org; Carsey, Jaben ; Gao,
> > >Liming 
> > >> Subject: Re: [edk2] [Patch] BaseTools: Optimize string concatenation
> > >>
> > >> On Mon, Dec 10, 2018 at 12:09:23PM +, Feng, Bob C wrote:
> > >> > For the "customized deepcopy" and "cache for uni file parser" data,
> > >> > you can see the AutoGen is not slower. The whole Build Duration is
> > >> > longer because Make Duration is longer while Make Duration time
> > >> > depends on the external make, compiler and linker. So it's not the
> > >> > patch make the build slow down.
> > >> >
> > >> > Yes,  it's not faster either. I think that because the Ovmf platform
> > >> > is relatively simple.  From the build tool source code point of view,
> > >> > the customized deepcopy will take effect if the platform enabled
> > >> > multiple SKU or there are many expressions in metadata file to be
> > >> > evaluated. And the "cache for uni file parser" needs there are many
> > >> > uni files.  The Ovmf platform looks not a good platform to demo the
> > >> > effect of this patch.
> > >>
> > >> But surely we should not introduce patches said to improve performance
> > >when the only data we have available shows that they slow things down?
> > >>
> > >> If the performance data is not representative, then it is worthle

Re: [edk2] Obtaining TCG final events on systems without TCG2 log support

[Yao, Jiewen]

Right.
I think we are trying to deprecate the old SHA1 support, because SHA1 is 
considered as unsecure algorithm.
We are moving to crypto agile. As such, we do not see the need to support old 
style event log.

Thank you
Yao Jiewen


> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Thursday, December 13, 2018 8:36 PM
> To: Matthew Garrett 
> Cc: edk2-devel@lists.01.org; Yao, Jiewen ;
> Marc-André Lureau ; Stefan Berger
> 
> Subject: Re: [edk2] Obtaining TCG final events on systems without TCG2 log
> support
> 
> + Jiewen, Marc-André, Stefan
> 
> On 12/13/18 02:17, Matthew Garrett wrote:
> > SetupEventLog() in Tcg2Dxe.c only installs the final event log
> > configuration table if SupportedEventLogs includes the TCG2 log format.
> > If the platform only supports the TCG1.2 log format then the final
> > events table isn't installed. However, ExitBootServices() should
> > generate an event even on systems that don't support the TCG2 log
> > format. How is an OS supposed to obtain the log of the
> > ExitBootServices() events in that case?
> >
> 
> I don't think it can.
> 
> You probably refer to the code below the comment "No need to handle
> EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2", in SetupEventLog(). This code
> dates
> back to commit fd46e831bc33 ("SecurityPkg: Update final event log
> calculation.", 2016-01-18). And the commit message says, "... there is
> no need to record TCG12 format log to final event log area ...".
> 
> Hence, the code is intentional. I even think the code is valid
> (according to the spec [*]); I just think the commit message should have
> said, "there is no *way* to record TCG12 format log to final event log
> area". Because, IMO, the bug is in the spec.
> 
> [*] TCG EFI Protocol Specification
> Family “2.0”
> Level 00 Revision 00.13
> March 30, 2016
> 
> Here's why I think it's a spec bug:
> 
> 
> (1) If EFI_TCG2_EVENT_LOG_FORMAT_TCG_2 is *clear* in
> SupportedEventLogs,
> then the platform advertizes GetEventLog() as unable to produce the
> crypto agile log format.
> 
> In other words, the platform is unable to produce a log which consists
> of TCG_PCR_EVENT2 entries, beyond the sole TCG_PCR_EVENT ("SHA1
> format")
> header entry.
> 
> Accordingly, GetEventLog() will fail with EFI_INVALID_PARAMETER, when
> called with EventLogFormat=EFI_TCG2_EVENT_LOG_FORMAT_TCG_2. (BTW,
> I
> think EFI_UNSUPPORTED would have been better for this, but I digress.)
> 
> (2) EFI_TCG2_FINAL_EVENTS_TABLE is defined with TCG_PCR_EVENT2
> entries
> *only*. TCG_PCR_EVENT is not accommodated.
> 
> 
> That's the contradiction. If a platform is unable to produce
> TCG_PCR_EVENT2 entries in GetEventLog(), it is fairly certainly also
> unable to produce them in the final events table.
> 
> And, while the first *instance* of the limitation is conformant, via
> SupportedEventLogs, the second instance of the same limitation isn't.
> 
> Thanks,
> Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] Maintainers.txt: Change package maintainer and reviewer of CryptoPkg.

[Gao, Liming]

Laszlo:
  Yes. Long, Qin should send this patch. Because Long, Qin changes to another 
work group for a while, he doesn't work on edk2 project. Ting directly sends 
the patch to remove his name. I just include Long, Qin, and let him confirm 
this change. 

Thanks
Liming
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo 
> Ersek
> Sent: Thursday, December 13, 2018 6:38 PM
> To: Ye, Ting 
> Cc: edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH] Maintainers.txt: Change package maintainer and 
> reviewer of CryptoPkg.
> 
> Hi Ting,
> 
> On 12/13/18 08:41, tye1 wrote:
> > Cc: Gang Wei 
> > Cc: Jian Wang 
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Ting Ye 
> > ---
> >  Maintainers.txt | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/Maintainers.txt b/Maintainers.txt
> > index 001d8ba010..d5cb305da9 100644
> > --- a/Maintainers.txt
> > +++ b/Maintainers.txt
> > @@ -102,8 +102,9 @@ S: Maintained
> >
> >  CryptoPkg
> >  W: https://github.com/tianocore/tianocore.github.io/wiki/CryptoPkg
> > -M: Qin Long 
> >  M: Ting Ye 
> > +R: Gang Wei 
> > +R: Jian Wang 
> >
> >  DynamicTablesPkg
> >  W: https://github.com/tianocore/tianocore.github.io/wiki/DynamicTablesPkg
> >
> 
> This patch does not conform to the rule that we added lately; please see
> commit 9ebef6c0a7d3 ("Maintainers.txt: Add the rule to hand over the
> package maintain role", 2018-11-29).
> 
> In other words, the patch should be sent out by Qin Long. Even though
> you co-maintain CryptoPkg with Qin Long, you shouldn't be able to
> deprive Qin Long from the role.
> 
> 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

Re: [edk2] [PATCH] MdeModulePkg/PciBus: Fix system hang when no PCI Option ROM exists

[Wu, Hao A]

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Ruiyu Ni
> Sent: Wednesday, December 12, 2018 11:10 PM
> To: edk2-devel@lists.01.org
> Cc: Wu, Hao A
> Subject: [edk2] [PATCH] MdeModulePkg/PciBus: Fix system hang when no
> PCI Option ROM exists
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1394
> 
> When there is no PCI option ROM exists, today's logic still creates
> virtual BAR for option ROM using Length = 0, Alignment = (-1).
> It causes the final MEM32 alignment requirement is as big as
> 0x_.
> 
> The patch fixes this issue by only creating virtual BAR for option
> ROM when there is PCI option ROM.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ruiyu Ni 
> Cc: Chiu Chasel 
> Cc: Hao A Wu 
> Cc: Jian J Wang 
> ---
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c | 10 ++
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> index 7255bcfbbc..ee5c77147e 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> @@ -515,10 +515,12 @@ PciHostBridgeResourceAllocator (
>// All devices' Option ROM share the same MEM32 resource.
>//
>MaxOptionRomSize = GetMaxOptionRomSize (RootBridgeDev);
> -  RootBridgeDev->PciBar[0].BarType   = PciBarTypeOpRom;
> -  RootBridgeDev->PciBar[0].Length= MaxOptionRomSize;
> -  RootBridgeDev->PciBar[0].Alignment = MaxOptionRomSize - 1;
> -  GetResourceFromDevice (RootBridgeDev, IoBridge, Mem32Bridge,
> PMem32Bridge, Mem64Bridge, PMem64Bridge);
> +  if (MaxOptionRomSize != 0) {
> +RootBridgeDev->PciBar[0].BarType   = PciBarTypeOpRom;
> +RootBridgeDev->PciBar[0].Length= MaxOptionRomSize;
> +RootBridgeDev->PciBar[0].Alignment = MaxOptionRomSize - 1;
> +GetResourceFromDevice (RootBridgeDev, IoBridge, Mem32Bridge,
> PMem32Bridge, Mem64Bridge, PMem64Bridge);
> +  }

Reviewed-by: Hao Wu 

Best Regards,
Hao Wu

> 
>//
>// Create resourcemap by going through all the devices subject to this
> root bridge
> --
> 2.16.1.windows.1
> 
> ___
> 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

Re: [edk2] Obtaining TCG final events on systems without TCG2 log support

[Laszlo Ersek]

+ Jiewen, Marc-André, Stefan

On 12/13/18 02:17, Matthew Garrett wrote:
> SetupEventLog() in Tcg2Dxe.c only installs the final event log 
> configuration table if SupportedEventLogs includes the TCG2 log format. 
> If the platform only supports the TCG1.2 log format then the final 
> events table isn't installed. However, ExitBootServices() should 
> generate an event even on systems that don't support the TCG2 log 
> format. How is an OS supposed to obtain the log of the 
> ExitBootServices() events in that case?
> 

I don't think it can.

You probably refer to the code below the comment "No need to handle
EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2", in SetupEventLog(). This code dates
back to commit fd46e831bc33 ("SecurityPkg: Update final event log
calculation.", 2016-01-18). And the commit message says, "... there is
no need to record TCG12 format log to final event log area ...".

Hence, the code is intentional. I even think the code is valid
(according to the spec [*]); I just think the commit message should have
said, "there is no *way* to record TCG12 format log to final event log
area". Because, IMO, the bug is in the spec.

[*] TCG EFI Protocol Specification
Family “2.0”
Level 00 Revision 00.13
March 30, 2016

Here's why I think it's a spec bug:


(1) If EFI_TCG2_EVENT_LOG_FORMAT_TCG_2 is *clear* in SupportedEventLogs,
then the platform advertizes GetEventLog() as unable to produce the
crypto agile log format.

In other words, the platform is unable to produce a log which consists
of TCG_PCR_EVENT2 entries, beyond the sole TCG_PCR_EVENT ("SHA1 format")
header entry.

Accordingly, GetEventLog() will fail with EFI_INVALID_PARAMETER, when
called with EventLogFormat=EFI_TCG2_EVENT_LOG_FORMAT_TCG_2. (BTW, I
think EFI_UNSUPPORTED would have been better for this, but I digress.)

(2) EFI_TCG2_FINAL_EVENTS_TABLE is defined with TCG_PCR_EVENT2 entries
*only*. TCG_PCR_EVENT is not accommodated.


That's the contradiction. If a platform is unable to produce
TCG_PCR_EVENT2 entries in GetEventLog(), it is fairly certainly also
unable to produce them in the final events table.

And, while the first *instance* of the limitation is conformant, via
SupportedEventLogs, the second instance of the same limitation isn't.

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

Re: [edk2] [RFC PATCH v4 00/12] Extend secure variable service to be usable from Standalone MM

[Jagadeesh Ujja]

Hi Liming

On Wed, Dec 12, 2018 at 8:44 PM Gao, Liming  wrote:
>
> This version is better. I have some comments on edk2 coding style.

Thank you for your review. Please see reply to your comments below.

>
> 1. This patch set can't be applied in edk2 trunk. Seemly, they base on 
> previous version edk2.

The v4 patchset was based on the tip of the edk2 master branch on the
day it was posted. The commit id on which this series was based is
"f7f94ffe".

> 2. Pcd is for Standalone MM Code, not specific for Variable. So, I suggest to 
> use the generic name PcdStandaloneMmCodeEnabled. Its description is also 
> required to be updated.

The intention of the changes done in the patchset is to reuse the
variable service driver in MM_STANDALONE mode. There could be
platforms that enable Standalone MM mode but would not want a  secure
storage for EFI variables. In which case, the PCD named
PcdStandaloneMmCodeEnabled would not be sufficient. And this the
reason it was named " PcdStandaloneMmVariableEnabled".

> 3. Library header file name (StandaloneMmServicesTableLib.h) is also library 
> class name. Library class name and header file mapping is required to be 
> listed in MdePkg.dec file [LibraryClasses] section. And, this header file 
> doesn't need to include Library/DebugLib.h, because it doesn't depend on it.
> 4. Library implementation INF file (StandaloneMmRuntimeDxe.inf) should list 
> its library class name in LIBRARY_CLASS of [Defines] section. Its library 
> class name is StandaloneMmServicesTableLib. And, MdePkg library 
> implementation depends on MdePkg.dec only in [Packages] section.
> 5. Library implementation should implement all interfaces defined in library 
> class header file. StandaloneMmRuntimeDxe library should initialize gMmst as 
> NULL if it has no real value. StandaloneMmRuntimeDxe library doesn't depend 
> on any other library class. It doesn't need to list other library class in 
> its [LibraryClasses] section of INF file.

Point 3, 4 and 5 will be fixed

> 6. When other module depends on this library class header file, it should 
> list StandaloneMmServicesTableLib in its [LibraryClasses] section of INF file.
> 7. Platform DSC also needs to list LibraryClassName|Library implementation 
> INF in [LibraryClasses] section.

Points 6 and 7 are taken care and are part of edk2platform specific
changes, will post those changes soon

> 8. I don't suggest to add AsmLfence API in BaseLib for AArch64, because it is 
> X86 specific API. I suggest to update Variable driver with the wrapper 
> function FenceFunc() for AsmLfence() and MemoryFence(). FenceFunc can be 
> implemented for the different arch in Variable driver. Variable driver will 
> call FenceFunc() instead of AsmLfence(). So, only variable driver is required 
> to be updated. There is no change in BaseLib.
>
Okay, the variable driver can be updated to call a wrapper
"FenceFunc()" but wouldn't it be useful to add the architecture
specific implantation of this in BaseLib. In that way, not just the
variable driver but other drivers can use this implementation of
"FenceFunc()". For instance,
FaultTolerantWriteDxe/FaultTolerantWriteSmm.c does calls to
AsmLfence() and an architecture specific implementation of
"FenceFunc()" in BaseLib can be reused in FaultTolerantWriteDxe driver
as well.

> > -Original Message-
> > From: Jagadeesh Ujja [mailto:jagadeesh.u...@arm.com]
> > Sent: Tuesday, December 11, 2018 2:22 PM
> > To: edk2-devel@lists.01.org; Gao, Liming ; Zhang, 
> > Chao B ; leif.lindh...@linaro.org
> > Subject: [RFC PATCH v4 00/12] Extend secure variable service to be usable 
> > from Standalone MM
> >
> > Changes since v3:
> > - Addressed all the comments from Liming Gao
> >   - Added a AArch64 implementation of AsmLfence which is a wrapper for
> > MemoryFence. The changes in variable service driver in v3 of this
> > patchset that used MemoryFence instead of AsmLfence have been removed.
> >   - Added StandaloneMmServicesTableLib.h and StandaloneMmRuntimeDxe
> > library into MdePkg.
> >   - Renamed PcdStandaloneMmEnable as PcdStandaloneMmVariableEnabled and
> > added to in to MdePkg.
> >   - Now with above changes, edk2 packages don't need to depend on
> > StandaloneMmPkg/StandaloneMmPkg.dec
> > - Addressed comments from Ting Ye
> >   - Removed the hacks in the v3 version.
> >   - Will relook into the “TimerWrapp.c” file and add a appropriate
> > implementation of this for MM Standalone mode code.
> >
> > Changes since v2:
> > - Added 'Contributed-under' tag, removed Change-ID tag and
> >   maintained a single signed-off-by for the all the patches.
> >
> > Changes since v1:
> > - Addressed all the comments from Liming Gao
> >   - Removed the use of #ifdef/#else/#endif and used a Pcd instead to
> > select between MM and non-MM paths.
> >   - Removed all dependencies on edk2-platforms.
> >   - Dropped the use of mMmst and used gSmst instead.
> >   - Added a dummy implementation UefiRuntimeServiceTableL

[edk2] [PATCH edk2-platforms 3/3] Platform/ARM/SgiPkg: Enable MmCommunication module on the platform

[Sughosh Ganu]

The ArmMmCommunication module is used for communication between
non-secure and secure world using Arm's Management Mode
Specification. Enable this module on Sgi platforms. This would be used
subsequently by the RAS and SecureBoot features, support for which
is to be added on the platform..

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Sughosh Ganu 
---
 Platform/ARM/SgiPkg/SgiPlatform.dsc | 20 
 Platform/ARM/SgiPkg/SgiPlatform.fdf |  5 +
 2 files changed, 25 insertions(+)

diff --git a/Platform/ARM/SgiPkg/SgiPlatform.dsc 
b/Platform/ARM/SgiPkg/SgiPlatform.dsc
index 19d2ac3a656a..44f769947d53 100644
--- a/Platform/ARM/SgiPkg/SgiPlatform.dsc
+++ b/Platform/ARM/SgiPkg/SgiPlatform.dsc
@@ -185,6 +185,22 @@ [PcdsFixedAtBuild.common]
   # Ethernet
   gEmbeddedTokenSpaceGuid.PcdLan91xDxeBaseAddress|0x1800
 
+!if $(ARM_STANDALONE_MM_ENABLE) == TRUE
+  #
+  # Set the base address and size of the buffer used
+  # for communication between the Normal world edk2
+  # with StandaloneMm image at S-EL0 through MM_COMMUNICATE.
+  # This buffer gets allocated in ATF and since we do not have
+  # a mechanism currently to communicate the base address and
+  # size of this buffer from ATF, hard-code it here
+  #
+
+  ## MM Communicate
+  gArmTokenSpaceGuid.PcdMmBufferBase|0xFF60
+  gArmTokenSpaceGuid.PcdMmBufferSize|0x1
+
+!endif
+
 

 #
 # Components Section - list of all EDK II Modules needed by this Platform
@@ -316,3 +332,7 @@ [Components.common]
   # SATA Controller
   #
   MdeModulePkg/Bus/Pci/SataControllerDxe/SataControllerDxe.inf
+
+!if $(ARM_STANDALONE_MM_ENABLE) == TRUE
+  ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf
+!endif
diff --git a/Platform/ARM/SgiPkg/SgiPlatform.fdf 
b/Platform/ARM/SgiPkg/SgiPlatform.fdf
index 9c0ec1fa43a6..4145bc50ea04 100644
--- a/Platform/ARM/SgiPkg/SgiPlatform.fdf
+++ b/Platform/ARM/SgiPkg/SgiPlatform.fdf
@@ -169,6 +169,11 @@ [FV.FvMain]
   #
   INF  ShellPkg/Application/Shell/Shell.inf
 
+!if $(ARM_STANDALONE_MM_ENABLE) == TRUE
+  # MM Communicate
+  INF ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf
+!endif
+
   #
   # Platform driver
   #
-- 
2.7.4

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

Re: [edk2] [PATCH 0/3] fixes for CLANG35 on ARM

[Ard Biesheuvel]

On Thu, 13 Dec 2018 at 12:42, Gao, Liming  wrote:
>
> Yes. Reviewed-by: Liming Gao 
>

Thanks all

Pushed as 580f4539dfbb..36deafb838d0

> > -Original Message-
> > From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> > Sent: Thursday, December 13, 2018 6:49 PM
> > To: Gao, Liming 
> > Cc: Kinney, Michael D ; 
> > edk2-devel@lists.01.org; Laszlo Ersek 
> > Subject: Re: [edk2] [PATCH 0/3] fixes for CLANG35 on ARM
> >
> > On Wed, 12 Dec 2018 at 15:19, Ard Biesheuvel  
> > wrote:
> > >
> > > On Wed, 12 Dec 2018 at 15:19, Gao, Liming  wrote:
> > > >
> > > > Make sense. So, CLANG35 tool chain can also be used by CLANG 3.5 and 
> > > > above version compiler, like GCC49?
> > > >
> > >
> > > Exactly
> > >
> >
> > Liming,
> >
> > Are you happy with the MdePkg and BaseTools changes in this series?
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [PATCH edk2-platforms 2/3] Platform/ARM/SgiPkg: Setup memory buffers for MmCommunicate

[Sughosh Ganu]

Add memory regions for MmCommuncate buffers into the virtual memory
table.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Sughosh Ganu 
---
 Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf  | 4 
 Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLibMem.c | 6 ++
 2 files changed, 10 insertions(+)

diff --git a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf 
b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf
index 260be58fb38c..c0fcc8198201 100644
--- a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf
+++ b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf
@@ -25,6 +25,7 @@ [Packages]
   MdeModulePkg/MdeModulePkg.dec
   MdePkg/MdePkg.dec
   Platform/ARM/SgiPkg/SgiPlatform.dec
+  StandaloneMmPkg/StandaloneMmPkg.dec
 
 [LibraryClasses]
   ArmLib
@@ -62,6 +63,9 @@ [FixedPcd]
   gArmTokenSpaceGuid.PcdPciMmio64Base
   gArmTokenSpaceGuid.PcdPciMmio64Size
 
+  gArmTokenSpaceGuid.PcdMmBufferBase
+  gArmTokenSpaceGuid.PcdMmBufferSize
+
   gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
 
 [Guids]
diff --git a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLibMem.c 
b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLibMem.c
index 6ec2e8a7096d..11e0811fccc8 100644
--- a/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLibMem.c
+++ b/Platform/ARM/SgiPkg/Library/PlatformLib/PlatformLibMem.c
@@ -136,6 +136,12 @@ ArmPlatformGetVirtualMemoryMap (
SIZE_1MB;
   VirtualMemoryTable[Index].Attributes  = 
ARM_MEMORY_REGION_ATTRIBUTE_DEVICE;
 
+ // MM Memory Space
+  VirtualMemoryTable[++Index].PhysicalBase  = PcdGet64 (PcdMmBufferBase);
+  VirtualMemoryTable[Index].VirtualBase = PcdGet64 (PcdMmBufferBase);
+  VirtualMemoryTable[Index].Length  = PcdGet64 (PcdMmBufferSize);
+  VirtualMemoryTable[Index].Attributes  = 
ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED;
+
   // End of Table
   VirtualMemoryTable[++Index].PhysicalBase  = 0;
   VirtualMemoryTable[Index].VirtualBase = 0;
-- 
2.7.4

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

[edk2] [PATCH edk2-platforms 1/3] Platform/ARM/SgiPkg: Build infrastructure for StandaloneMm image

[Sughosh Ganu]

Add the build infrastructure for compilation of StandaloneMm image
files and FD file.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Sughosh Ganu 
---
 Platform/ARM/SgiPkg/PlatformStandaloneMm.dsc | 130 +++
 Platform/ARM/SgiPkg/PlatformStandaloneMm.fdf | 100 +
 2 files changed, 230 insertions(+)
 create mode 100644 Platform/ARM/SgiPkg/PlatformStandaloneMm.dsc
 create mode 100644 Platform/ARM/SgiPkg/PlatformStandaloneMm.fdf

diff --git a/Platform/ARM/SgiPkg/PlatformStandaloneMm.dsc 
b/Platform/ARM/SgiPkg/PlatformStandaloneMm.dsc
new file mode 100644
index ..4615c383c46a
--- /dev/null
+++ b/Platform/ARM/SgiPkg/PlatformStandaloneMm.dsc
@@ -0,0 +1,130 @@
+#
+#  Copyright (c) 2018, ARM Limited. All rights reserved.
+#
+#  This program and the accompanying materials are licensed and made available
+#  under the terms and conditions of the BSD License which accompanies this
+#  distribution.  The full text of the license may be found at
+#  http://opensource.org/licenses/bsd-license.php
+#
+#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
IMPLIED.
+#
+
+
+#
+# Defines Section - statements that will be processed to create a Makefile.
+#
+
+[Defines]
+  PLATFORM_NAME  = sgi_mm_standalone
+  PLATFORM_GUID  = 34B78C8F-CFD5-49D5-8360-E91143F6106D
+  PLATFORM_VERSION   = 1.0
+  DSC_SPECIFICATION  = 0x00010011
+  OUTPUT_DIRECTORY   = Build/$(PLATFORM_NAME)
+  SUPPORTED_ARCHITECTURES= AARCH64|ARM
+  BUILD_TARGETS  = DEBUG|RELEASE
+  SKUID_IDENTIFIER   = DEFAULT
+  FLASH_DEFINITION   = Platform/ARM/SgiPkg/PlatformStandaloneMm.fdf
+  DEFINE DEBUG_MESSAGE   = TRUE
+
+  # LzmaF86
+  DEFINE COMPRESSION_TOOL_GUID   = D42AE6BD-1352-4bfb-909A-CA72A6EAE889
+
+
+#
+# Library Class section - list of all Library Classes needed by this Platform.
+#
+
+[LibraryClasses]
+  #
+  # Basic
+  #
+  BaseLib|MdePkg/Library/BaseLib/BaseLib.inf
+  BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
+  DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
+  
DebugPrintErrorLevelLib|MdePkg/Library/BaseDebugPrintErrorLevelLib/BaseDebugPrintErrorLevelLib.inf
+  FvLib|StandaloneMmPkg/Library/FvLib/FvLib.inf
+  
HobLib|StandaloneMmPkg/Library/StandaloneMmCoreHobLib/StandaloneMmCoreHobLib.inf
+  IoLib|MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
+  MemLib|StandaloneMmPkg/Library/StandaloneMmMemLib/StandaloneMmMemLib.inf
+  
MemoryAllocationLib|StandaloneMmPkg/Library/StandaloneMmCoreMemoryAllocationLib/StandaloneMmCoreMemoryAllocationLib.inf
+  PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
+  PeCoffLib|MdePkg/Library/BasePeCoffLib/BasePeCoffLib.inf
+  PrintLib|MdePkg/Library/BasePrintLib/BasePrintLib.inf
+  
ReportStatusCodeLib|MdePkg/Library/BaseReportStatusCodeLibNull/BaseReportStatusCodeLibNull.inf
+
+  #
+  # Entry point
+  #
+  
StandaloneMmDriverEntryPoint|StandaloneMmPkg/Library/StandaloneMmDriverEntryPoint/StandaloneMmDriverEntryPoint.inf
+
+[LibraryClasses.AARCH64]
+  ArmLib|ArmPkg/Library/ArmLib/ArmBaseLib.inf
+  
StandaloneMmMmuLib|ArmPkg/Library/StandaloneMmMmuLib/ArmMmuStandaloneMmLib.inf
+  ArmSvcLib|ArmPkg/Library/ArmSvcLib/ArmSvcLib.inf
+  
CacheMaintenanceLib|ArmPkg/Library/ArmCacheMaintenanceLib/ArmCacheMaintenanceLib.inf
+  
PeCoffExtraActionLib|StandaloneMmPkg/Library/StandaloneMmPeCoffExtraActionLib/StandaloneMmPeCoffExtraActionLib.inf
+
+  # ARM PL011 UART Driver
+  
PL011UartClockLib|ArmPlatformPkg/Library/PL011UartClockLib/PL011UartClockLib.inf
+  PL011UartLib|ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.inf
+  
SerialPortLib|ArmPlatformPkg/Library/PL011SerialPortLib/PL011SerialPortLib.inf
+
+  
StandaloneMmCoreEntryPoint|StandaloneMmPkg/Library/StandaloneMmCoreEntryPoint/StandaloneMmCoreEntryPoint.inf
+
+
+#
+# Pcd Section - list of all EDK II PCD Entries defined by this Platform
+#
+
+[PcdsFeatureFlag]
+  gStandaloneMmPkgTokenSpaceGuid.PcdStandaloneMmEnable|TRUE
+
+[PcdsFixedAtBuild]
+  gEfiMdePkgTokenSpaceGuid.PcdDebugPrintErrorLevel|0x80CF
+  gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask|0xff
+  gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x0f
+
+[PcdsFixedAtBuild.AARCH64]
+  ## PL011 - Serial Terminal
+  gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterBase|0x7FF7
+  gEfiMdePkgTok

[edk2] [PATCH edk2-platforms 0/3] Platform/ARM/SgiPkg: Enable StandaloneMm on Sgi platforms

[Sughosh Ganu]

The following patches enable building of StandaloneMm image on Sgi
platforms.

The first patch adds build infra for StandaloneMm image. The second
patch of the series adds memory buffer required for communication
between the non-secure world with StandaloneMm image using MM
Communicate. The third patch of the series enables MmCommunication
module on the platform for it subsequent use by features like
Secure-Boot and Error Injection and Handling for RAS.


Sughosh Ganu (3):
  Platform/ARM/SgiPkg: Build infrastructure for StandaloneMm image
  Platform/ARM/SgiPkg: Setup memory buffers for MmCommunicate
  Platform/ARM/SgiPkg: Enable MmCommunication module on the platform

 Platform/ARM/SgiPkg/PlatformStandaloneMm.dsc   | 130 +
 Platform/ARM/SgiPkg/SgiPlatform.dsc|  20 
 Platform/ARM/SgiPkg/PlatformStandaloneMm.fdf   | 100 
 Platform/ARM/SgiPkg/SgiPlatform.fdf|   5 +
 .../ARM/SgiPkg/Library/PlatformLib/PlatformLib.inf |   4 +
 .../SgiPkg/Library/PlatformLib/PlatformLibMem.c|   6 +
 6 files changed, 265 insertions(+)
 create mode 100644 Platform/ARM/SgiPkg/PlatformStandaloneMm.dsc
 create mode 100644 Platform/ARM/SgiPkg/PlatformStandaloneMm.fdf

-- 
2.7.4


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

Re: [edk2] [PATCH 0/3] fixes for CLANG35 on ARM

[Gao, Liming]

Yes. Reviewed-by: Liming Gao 

> -Original Message-
> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> Sent: Thursday, December 13, 2018 6:49 PM
> To: Gao, Liming 
> Cc: Kinney, Michael D ; edk2-devel@lists.01.org; 
> Laszlo Ersek 
> Subject: Re: [edk2] [PATCH 0/3] fixes for CLANG35 on ARM
> 
> On Wed, 12 Dec 2018 at 15:19, Ard Biesheuvel  
> wrote:
> >
> > On Wed, 12 Dec 2018 at 15:19, Gao, Liming  wrote:
> > >
> > > Make sense. So, CLANG35 tool chain can also be used by CLANG 3.5 and 
> > > above version compiler, like GCC49?
> > >
> >
> > Exactly
> >
> 
> Liming,
> 
> Are you happy with the MdePkg and BaseTools changes in this series?
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH edk2-platforms 00/11] final set of Styx cleanups

[Ard Biesheuvel]

On Wed, 12 Dec 2018 at 23:23, Leif Lindholm  wrote:
>
> On Tue, Dec 11, 2018 at 07:35:03PM +0100, Ard Biesheuvel wrote:
> > I promise :-)
>
> Yeah, right :)
>
> > This gets rid of the last build config options passed via the command
> > line into the preprocessor, and replaces them with PCD references.
> >
> > Ard Biesheuvel (11):
> >   Silicon/AMD/Styx/Iort: drop conditionally included XGBE nodes
> >   Platform/SoftIron/Overdrive1000Board: remove dead XGBE references
> >   Silicon/AMD/Styx/AcpiPlatformDxe: replace XGBE CPP conditional with
> > PCD
> >   Silicon/AMD/Styx/PlatInitPei: replace XGBE CPP conditional with PCD
> >   Silicon/AMD/Styx/StyxDtbLoaderLib: replace XGBE CPP conditional with
> > PCD
> >   Platform/AMD/OverdriveBoard: drop DO_XGBE C preprocessor defines
> >   Silicon/AMD/Styx: introduce boolean PCD for KCS/IPMI support
> >   Silicon/AMD/Styx/AcpiPlatformDxe: move IPMI/KCS device into separate
> > SSDT
> >   Silicon/AMD/Styx/StyxDtbLoaderLib: replace DO_KCS macro reference with
> > PCD
> >   Platform/Styx: get rid of DO_KCS preprocessor macro
> >   Silicon/AMD/Styx/AcpiPlatformDxe: disable KCS on pre-B1 silicon
>
> Nice bit of cleanup.
> Reviewed-by: Leif Lindholm 
>


Thanks

Pushed as  572a554e1db4..80dfc2ce18c6
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2 edk2-platforms 03/20] Platform/Broadcom/RPi3: Add GPIO and RTC libraries

[Leif Lindholm]

On Thu, Dec 13, 2018 at 10:49:05AM +, Pete Batard wrote:
> > > diff --git a/Platform/Broadcom/Bcm283x/Include/Utils.h 
> > > b/Platform/Broadcom/Bcm283x/Include/Utils.h
> > > new file mode 100644
> > > index ..47713275de3f
> > > --- /dev/null
> > > +++ b/Platform/Broadcom/Bcm283x/Include/Utils.h
> > > @@ -0,0 +1,33 @@
> > > +/** @file
> > > + *
> > > + *  Copyright (c) 2018, Andrei Warkentin 
> > > + *
> > > + *  This program and the accompanying materials
> > > + *  are licensed and made available under the terms and conditions of 
> > > the BSD License
> > > + *  which accompanies this distribution.  The full text of the license 
> > > may be found at
> > > + *  http://opensource.org/licenses/bsd-license.php
> > > + *
> > > + *  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> > > + *  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
> > > IMPLIED.
> > > + *
> > > + **/
> > > +
> > > +#ifndef UTILS_H
> > > +#define UTILS_H
> > > +
> > > +#define _IX_BITS(sm, bg) ((bg) - (sm) + 1)
> > > +#define _IX_MASK(sm, bg) ((1ul << _IX_BITS((sm), (bg))) - 1)
> > > +#define _X(val, sm, bg) ((val) >> (sm)) & _IX_MASK((sm), (bg))
> > > +#define X(val, ix1, ix2) (((ix1) < (ix2)) ? _X((val), (ix1), (ix2)) :   \
> > > +  _X((val), (ix2), (ix1)))
> > > +
> > > +#define _I(val, sm, bg)  (((val) & _IX_MASK((sm), (bg))) << (sm))
> > > +#define I(val, ix1, ix2) (((ix1) < (ix2)) ? _I((val), (ix1), (ix2)) :   \
> > > +  _I((val), (ix2), (ix1)))
> > > +#define _M(val, sm, bg)  ((val) & (_IX_MASK((sm), (bg)) << (sm)))
> > > +#define M(val, ix1, ix2) (((ix1) < (ix2)) ? _M((val), (ix1), (ix2)) :   \
> > > +  _M((val), (ix2), (ix1)))
> > > +
> > > +#define ELES(x) (sizeof((x)) / sizeof((x)[0]))
> > > +
> > 
> > The name of the file suggests that these are generic utility macros,
> > and they use single letter identifiers. Not great for grep, so please
> > improve the names (assuming that they are actually used anywhere)
> 
> These macros are used. But I agree their naming should be better.
> 
> Also, I'll double check if these macros are actually used in more than one
> source, or if we can remove utils.h altogether and just move these macros
> into the driver/library that requires them.

ELES (if used) can be replaced with ARRAY_SIZE from Base.h.

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

Re: [edk2] [PATCH v2 edk2-platforms 07/20] Platform/Broadcom/RPi3: Add Firmware driver

[Pete Batard]

On 2018.12.12 21:17, Ard Biesheuvel wrote:

On Mon, 10 Dec 2018 at 13:39, Pete Batard  wrote:


Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Pete Batard 
---
  Platform/Broadcom/Bcm283x/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c   | 1085 

  Platform/Broadcom/Bcm283x/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.inf |   49 +
  Platform/Broadcom/Bcm283x/Include/Protocol/RaspberryPiFirmware.h|  131 +++
  3 files changed, 1265 insertions(+)



Reviewed-by: Ard Biesheuvel 

Please move this patch forward in the series. It is depended upon by
earlier patches


I'll move the patch forward. Thanks for the reviews.


diff --git a/Platform/Broadcom/Bcm283x/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c 
b/Platform/Broadcom/Bcm283x/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
new file mode 100644
index ..50f3ed3f1e36
--- /dev/null
+++ b/Platform/Broadcom/Bcm283x/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
@@ -0,0 +1,1085 @@
+/** @file
+ *
+ *  Copyright (c) 2017-2018, Andrei Warkentin 
+ *  Copyright (c) 2016, Linaro, Ltd. All rights reserved.
+ *
+ *  This program and the accompanying materials
+ *  are licensed and made available under the terms and conditions of the BSD 
License
+ *  which accompanies this distribution.  The full text of the license may be 
found at
+ *  http://opensource.org/licenses/bsd-license.php
+ *
+ *  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+ *  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
IMPLIED.
+ *
+ **/
+
+#include 
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+
+#include 
+
+//
+// The number of statically allocated buffer pages
+//
+#define NUM_PAGES   1
+
+//
+// The number of iterations to perform when waiting for the mailbox
+// status to change
+//
+#define MAX_TRIES   0x10
+
+STATIC VOID  *mDmaBuffer;
+STATIC VOID  *mDmaBufferMapping;
+STATIC UINTN mDmaBufferBusAddress;
+
+STATIC SPIN_LOCK mMailboxLock;
+
+STATIC
+BOOLEAN
+DrainMailbox (
+  VOID
+  )
+{
+  INTNTries;
+  UINT32  Val;
+
+  //
+  // Get rid of stale response data in the mailbox
+  //
+  Tries = 0;
+  do {
+Val = MmioRead32 (BCM2836_MBOX_BASE_ADDRESS + BCM2836_MBOX_STATUS_OFFSET);
+if (Val & (1U << BCM2836_MBOX_STATUS_EMPTY)) {
+  return TRUE;
+}
+ArmDataSynchronizationBarrier ();
+MmioRead32 (BCM2836_MBOX_BASE_ADDRESS + BCM2836_MBOX_READ_OFFSET);
+  } while (++Tries < MAX_TRIES);
+
+  return FALSE;
+}
+
+STATIC
+BOOLEAN
+MailboxWaitForStatusCleared (
+  IN  UINTN   StatusMask
+  )
+{
+  INTNTries;
+  UINT32  Val;
+
+  //
+  // Get rid of stale response data in the mailbox
+  //
+  Tries = 0;
+  do {
+Val = MmioRead32 (BCM2836_MBOX_BASE_ADDRESS + BCM2836_MBOX_STATUS_OFFSET);
+if ((Val & StatusMask) == 0) {
+  return TRUE;
+}
+ArmDataSynchronizationBarrier ();
+  } while (++Tries < MAX_TRIES);
+
+  return FALSE;
+}
+
+STATIC
+EFI_STATUS
+MailboxTransaction (
+  INUINTN   Length,
+  INUINTN   Channel,
+  OUT   UINT32  *Result
+  )
+{
+  if (Channel >= BCM2836_MBOX_NUM_CHANNELS) {
+return EFI_INVALID_PARAMETER;
+  }
+
+  //
+  // Get rid of stale response data in the mailbox
+  //
+  if (!DrainMailbox ()) {
+DEBUG ((DEBUG_ERROR, "%a: timeout waiting for mailbox to drain\n",
+  __FUNCTION__));
+return EFI_TIMEOUT;
+  }
+
+  //
+  // Wait for the 'output register full' bit to become clear
+  //
+  if (!MailboxWaitForStatusCleared (1U << BCM2836_MBOX_STATUS_FULL)) {
+DEBUG ((DEBUG_ERROR, "%a: timeout waiting for outbox to become empty\n",
+  __FUNCTION__));
+return EFI_TIMEOUT;
+  }
+
+  ArmDataSynchronizationBarrier ();
+
+  //
+  // Start the mailbox transaction
+  //
+  MmioWrite32 (BCM2836_MBOX_BASE_ADDRESS + BCM2836_MBOX_WRITE_OFFSET,
+(UINT32)((UINTN)mDmaBufferBusAddress | Channel));
+
+  ArmDataSynchronizationBarrier ();
+
+  //
+  // Wait for the 'input register empty' bit to clear
+  //
+  if (!MailboxWaitForStatusCleared (1U << BCM2836_MBOX_STATUS_EMPTY)) {
+DEBUG ((DEBUG_ERROR, "%a: timeout waiting for inbox to become full\n",
+  __FUNCTION__));
+return EFI_TIMEOUT;
+  }
+
+  //
+  // Read back the result
+  //
+  ArmDataSynchronizationBarrier ();
+  *Result = MmioRead32 (BCM2836_MBOX_BASE_ADDRESS + BCM2836_MBOX_READ_OFFSET);
+  ArmDataSynchronizationBarrier ();
+
+  return EFI_SUCCESS;
+}
+
+#pragma pack(1)
+typedef struct {
+  UINT32BufferSize;
+  UINT32Response;
+} RPI_FW_BUFFER_HEAD;
+
+typedef struct {
+  UINT32TagId;
+  UINT32TagSize;
+  UINT32TagValueSize;
+} RPI_FW_TAG_HEAD;
+
+typedef struct {
+  UINT32  DeviceId;
+  UINT32  PowerState;
+} RPI_FW_POWER_STATE_TAG;
+
+typedef struct {
+  RPI_FW_BUFFER_HEADBufferHead;
+  RPI_FW_TAG_HEAD   TagHead;
+  RPI_FW_POWER_STATE_TAGTagBody;
+  UINT32EndTag;
+} RPI_FW_SET_POWER_STATE_CMD;
+#p

Re: [edk2] [PATCH 0/2] Add two public functions

[Leif Lindholm]

Please let the subject line give some sort of hint of what is being
done, and to what. "Add two functions" is not substantially more
descriptive than "add 572 characters".

In this case, your're moving previously internal string helper functions
to BaseLib.

On Thu, Dec 13, 2018 at 04:34:37PM +0800, Shenglei Zhang wrote:
> Add two public functions,CharToUpper and AsciiToUpper,and
> remove the same functional functions,InternalCharToUpper
> and InternalBaseLibAsciiToUpper.
> 
> Cc: Michael D Kinney 
> Cc: Liming Gao 
> Cc: Bob Feng 
> Cc: Yonghong Zhu 
> Shenglei Zhang (2):
>   MdePkg/BaseLib: Add two public functions
>   BaseTools/Common: Remove InternalCharToUpper
> 
>  BaseTools/Source/C/Common/CommonLib.c | 16 ++---
>  BaseTools/Source/C/Common/CommonLib.h |  4 ---
>  MdePkg/Include/Library/BaseLib.h  | 40 +
>  MdePkg/Library/BaseLib/BaseLibInternals.h | 42 ---
>  MdePkg/Library/BaseLib/SafeString.c   |  8 ++---
>  MdePkg/Library/BaseLib/String.c   | 35 ---
>  6 files changed, 53 insertions(+), 92 deletions(-)
> 
> -- 
> 2.18.0.windows.1
> 
> ___
> 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

Re: [edk2] [PATCH v2 edk2-platforms 06/20] Platform/Broadcom/RPi3: Add Interrupt and Device Tree drivers

[Pete Batard]

On 2018.12.12 21:09, Ard Biesheuvel wrote:

On Mon, 10 Dec 2018 at 13:39, Pete Batard  wrote:


Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Pete Batard 
---
  Platform/Broadcom/Bcm283x/Drivers/Bcm2836InterruptDxe/Bcm2836InterruptDxe.c   
| 367 +++
  Platform/Broadcom/Bcm283x/Drivers/Bcm2836InterruptDxe/Bcm2836InterruptDxe.inf 
|  48 +++


These look mostly fine, but please check for cosmetic issues (space
before (, no space after a cast)


Sure.




  Platform/Broadcom/Bcm283x/Drivers/RpiFdtDxe/RpiFdtDxe.c   
| 370 
  Platform/Broadcom/Bcm283x/Drivers/RpiFdtDxe/RpiFdtDxe.inf 
|  53 +++


Some comments below


  4 files changed, 838 insertions(+)

diff --git 
a/Platform/Broadcom/Bcm283x/Drivers/Bcm2836InterruptDxe/Bcm2836InterruptDxe.c 
b/Platform/Broadcom/Bcm283x/Drivers/Bcm2836InterruptDxe/Bcm2836InterruptDxe.c
new file mode 100644
index ..dda61665031d
--- /dev/null
+++ 
b/Platform/Broadcom/Bcm283x/Drivers/Bcm2836InterruptDxe/Bcm2836InterruptDxe.c
@@ -0,0 +1,367 @@
+/** @file
+ *
+ *  Copyright (c) 2016, Linaro, Ltd. All rights reserved.
+ *
+ *  This program and the accompanying materials
+ *  are licensed and made available under the terms and conditions of the BSD 
License
+ *  which accompanies this distribution.  The full text of the license may be 
found at
+ *  http://opensource.org/licenses/bsd-license.php
+ *
+ *  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+ *  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
IMPLIED.
+ *
+ **/
+
+#include 
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+
+#include 
+#include 
+
+//
+// This currently only implements support for the architected timer interrupts
+// on the per-CPU interrupt controllers.
+//
+#define NUM_IRQS(4)
+
+#ifdef MDE_CPU_AARCH64
+#define ARM_ARCH_EXCEPTION_IRQ  EXCEPT_AARCH64_IRQ
+#else
+#define ARM_ARCH_EXCEPTION_IRQ  EXCEPT_ARM_IRQ
+#endif
+
+STATIC CONST
+EFI_PHYSICAL_ADDRESS RegBase = FixedPcdGet32 (PcdInterruptBaseAddress);
+
+//
+// Notifications
+//
+STATIC EFI_EVENTmExitBootServicesEvent;
+STATIC HARDWARE_INTERRUPT_HANDLER   mRegisteredInterruptHandlers[NUM_IRQS];
+
+/**
+  Shutdown our hardware
+
+  DXE Core will disable interrupts and turn off the timer and disable 
interrupts
+  after all the event handlers have run.
+
+  @param[in]  Event   The Event that is being processed
+  @param[in]  Context Event Context
+**/
+STATIC
+VOID
+EFIAPI
+ExitBootServicesEvent (
+  IN EFI_EVENT  Event,
+  IN VOID   *Context
+  )
+{
+  // Disable all interrupts
+  MmioWrite32 (RegBase + BCM2836_INTC_TIMER_CONTROL_OFFSET, 0);
+}
+
+/**
+  Enable interrupt source Source.
+
+  @param This Instance pointer for this protocol
+  @param Source   Hardware source of the interrupt
+
+  @retval EFI_SUCCESS   Source interrupt enabled.
+  @retval EFI_DEVICE_ERROR  Hardware could not be programmed.
+
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+EnableInterruptSource (
+  IN EFI_HARDWARE_INTERRUPT_PROTOCOL*This,
+  IN HARDWARE_INTERRUPT_SOURCE  Source
+  )
+{
+  if (Source >= NUM_IRQS) {
+ASSERT(FALSE);
+return EFI_UNSUPPORTED;
+  }
+
+  MmioOr32 (RegBase + BCM2836_INTC_TIMER_CONTROL_OFFSET, 1 << Source);
+
+  return EFI_SUCCESS;
+}
+
+
+/**
+  Disable interrupt source Source.
+
+  @param This Instance pointer for this protocol
+  @param Source   Hardware source of the interrupt
+
+  @retval EFI_SUCCESS   Source interrupt disabled.
+  @retval EFI_DEVICE_ERROR  Hardware could not be programmed.
+
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+DisableInterruptSource (
+  IN EFI_HARDWARE_INTERRUPT_PROTOCOL*This,
+  IN HARDWARE_INTERRUPT_SOURCE  Source
+  )
+{
+  if (Source >= NUM_IRQS) {
+ASSERT(FALSE);
+return EFI_UNSUPPORTED;
+  }
+
+  MmioAnd32 (RegBase + BCM2836_INTC_TIMER_CONTROL_OFFSET, ~(1 << Source));
+
+  return EFI_SUCCESS;
+}
+
+/**
+  Register Handler for the specified interrupt source.
+
+  @param This Instance pointer for this protocol
+  @param Source   Hardware source of the interrupt
+  @param Handler  Callback for interrupt. NULL to unregister
+
+  @retval EFI_SUCCESS Source was updated to support Handler.
+  @retval EFI_DEVICE_ERROR  Hardware could not be programmed.
+
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+RegisterInterruptSource (
+  IN EFI_HARDWARE_INTERRUPT_PROTOCOL*This,
+  IN HARDWARE_INTERRUPT_SOURCE  Source,
+  IN HARDWARE_INTERRUPT_HANDLER Handler
+  )
+{
+  if (Source >= NUM_IRQS) {
+ASSERT (FALSE);
+return EFI_UNSUPPORTED;
+  }
+
+  if (Handler == NULL && mRegisteredInterruptHandlers[Source] == NULL) {
+return EFI_INVALID_PARAMETER;
+  }
+
+  if (Handler != NULL && mRegisteredInterruptHandlers[Source] != NULL) {
+return EFI_ALREADY_STARTED;
+  }
+
+  mRegisteredInterruptHandlers[Source] = Handler;
+  return EnableInterruptSource(This,

Re: [edk2] [PATCH 0/3] fixes for CLANG35 on ARM

[Ard Biesheuvel]

On Wed, 12 Dec 2018 at 15:19, Ard Biesheuvel  wrote:
>
> On Wed, 12 Dec 2018 at 15:19, Gao, Liming  wrote:
> >
> > Make sense. So, CLANG35 tool chain can also be used by CLANG 3.5 and above 
> > version compiler, like GCC49?
> >
>
> Exactly
>

Liming,

Are you happy with the MdePkg and BaseTools changes in this series?
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2 edk2-platforms 04/20] Platform/Broadcom/RPi3: Add ACPI Tables

[Pete Batard]

On 2018.12.12 20:52, Ard Biesheuvel wrote:

On Mon, 10 Dec 2018 at 13:39, Pete Batard  wrote:


Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Pete Batard 
---
  Platform/Broadcom/Bcm283x/AcpiTables/AcpiTables.inf |  51 ++
  Platform/Broadcom/Bcm283x/AcpiTables/Csrt.aslc  | 337 +
  Platform/Broadcom/Bcm283x/AcpiTables/Dbg2.aslc  |  32 ++
  Platform/Broadcom/Bcm283x/AcpiTables/Dsdt.asl   | 523 
  Platform/Broadcom/Bcm283x/AcpiTables/Fadt.aslc  |  50 ++
  Platform/Broadcom/Bcm283x/AcpiTables/Gtdt.aslc  |  31 ++
  Platform/Broadcom/Bcm283x/AcpiTables/Madt.aslc  |  60 +++
  Platform/Broadcom/Bcm283x/AcpiTables/Pep.asl|  92 
  Platform/Broadcom/Bcm283x/AcpiTables/Pep.c  |  84 
  Platform/Broadcom/Bcm283x/AcpiTables/Pep.h  | 126 +
  Platform/Broadcom/Bcm283x/AcpiTables/Platform.h |  82 +++
  Platform/Broadcom/Bcm283x/AcpiTables/Rhpx.asl   | 201 
  Platform/Broadcom/Bcm283x/AcpiTables/Sdhc.asl   | 105 
  Platform/Broadcom/Bcm283x/AcpiTables/Spcr.asl   |  53 ++
  Platform/Broadcom/Bcm283x/AcpiTables/Uart.asl   | 155 ++
  15 files changed, 1982 insertions(+)



I'm not crazt about the hexdump-style files in this patch but I can
live with it.

One remark below.


diff --git a/Platform/Broadcom/Bcm283x/AcpiTables/AcpiTables.inf 
b/Platform/Broadcom/Bcm283x/AcpiTables/AcpiTables.inf
new file mode 100644
index ..db0270270cf9
--- /dev/null
+++ b/Platform/Broadcom/Bcm283x/AcpiTables/AcpiTables.inf
@@ -0,0 +1,51 @@
+#/** @file
+#
+#  ACPI table data and ASL sources required to boot the platform.
+#
+#  Copyright (c) 2017, Andrey Warkentin 
+#  Copyright (c) Microsoft Corporation. All rights reserved.
+#
+#  This program and the accompanying materials
+#  are licensed and made available under the terms and conditions of the BSD 
License
+#  which accompanies this distribution.  The full text of the license may be 
found at
+#  http://opensource.org/licenses/bsd-license.php
+#
+#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
IMPLIED.
+#
+#**/
+
+[Defines]
+  INF_VERSION= 0x00010005


I'll mention it here but it applies everywhere: please use 0x0001001A
as the INF version.
(In general, use the latest published version of each file type)


Good point. We'll fix that.


+  BASE_NAME  = AcpiTables
+  FILE_GUID  = 7E374E25-8E01-4FEE-87F2-390C23C606CD
+  MODULE_TYPE= USER_DEFINED
+  VERSION_STRING = 1.0
+
+#
+# The following information is for reference only and not required by the 
build tools.
+#
+#  VALID_ARCHITECTURES   = AARCH64
+#
+
+[Sources]
+  Platform.h
+  Madt.aslc
+  Fadt.aslc
+  Dbg2.aslc
+  Gtdt.aslc
+  Dsdt.asl
+  Csrt.aslc
+  Spcr.asl
+
+[Packages]
+  MdePkg/MdePkg.dec
+  EmbeddedPkg/EmbeddedPkg.dec
+
+[FixedPcd]
+  gEmbeddedTokenSpaceGuid.PcdInterruptBaseAddress
+
+[BuildOptions]
+  # The default '-mcmodel=small' used with DEBUG produces a GenFw error when 
compiling CSRT.acpi:
+  # "AARCH64 small code model requires identical ELF and PE/COFF section offsets 
modulo 4 KB."
+  GCC:DEBUG_*_AARCH64_CC_FLAGS = -mcmodel=tiny
diff --git a/Platform/Broadcom/Bcm283x/AcpiTables/Csrt.aslc 
b/Platform/Broadcom/Bcm283x/AcpiTables/Csrt.aslc
new file mode 100644
index ..926942ec8da3
--- /dev/null
+++ b/Platform/Broadcom/Bcm283x/AcpiTables/Csrt.aslc
@@ -0,0 +1,337 @@
+/** @file
+ *
+ *  Core System Resource Table (CSRT)
+ *
+ *  Copyright (c) Microsoft Corporation. All rights reserved.
+ *
+ *  This program and the accompanying materials
+ *  are licensed and made available under the terms and conditions of the BSD 
License
+ *  which accompanies this distribution.  The full text of the license may be 
found at
+ *  http://opensource.org/licenses/bsd-license.php
+ *
+ *  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+ *  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
IMPLIED.
+ *
+ **/
+
+#include "Platform.h"
+
+#define DMA_MAX_REQ_LINES 32
+
+#pragma pack(push, 1)
+
+//
+// DMA Controller Vendor Data for RPi3
+//
+typedef struct
+{
+UINT32 Length;
+UINT32 Type;
+UINT64 ChannelsBaseAddress;
+UINT32 ChannelsBaseSize;
+UINT64 ControllerBaseAddress;
+UINT32 ControllerBaseSize;
+UINT32 ChannelCount;
+UINT32 ControllerInterrupt;
+UINT32 MinimumRequestLine;
+UINT32 MaximumRequestLine;
+BOOLEAN CacheCoherent;
+} DMA_CONTROLLER_VENDOR_DATA;
+
+//
+// DMA Controller on RPi3
+//
+typedef stru

Re: [edk2] [PATCH v2 edk2-platforms 03/20] Platform/Broadcom/RPi3: Add GPIO and RTC libraries

[Pete Batard]

On 2018.12.12 20:50, Ard Biesheuvel wrote:

On Mon, 10 Dec 2018 at 13:39, Pete Batard  wrote:


Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Pete Batard 
---
  Platform/Broadcom/Bcm283x/Include/IndustryStandard/Bcm2837Gpio.h  
|  50 +
  Platform/Broadcom/Bcm283x/Include/Library/GpioLib.h   
|  33 +++
  Platform/Broadcom/Bcm283x/Include/Utils.h 
|  33 +++
  Platform/Broadcom/Bcm283x/Library/GpioLib/GpioLib.c   
|  79 +++
  Platform/Broadcom/Bcm283x/Library/GpioLib/GpioLib.inf 
|  39 
  
Platform/Broadcom/Bcm283x/Library/VirtualRealTimeClockLib/VirtualRealTimeClockLib.c
   | 222 
  
Platform/Broadcom/Bcm283x/Library/VirtualRealTimeClockLib/VirtualRealTimeClockLib.inf
 |  43 
  7 files changed, 499 insertions(+)

diff --git a/Platform/Broadcom/Bcm283x/Include/IndustryStandard/Bcm2837Gpio.h 
b/Platform/Broadcom/Bcm283x/Include/IndustryStandard/Bcm2837Gpio.h
new file mode 100644
index ..27e6665f1745
--- /dev/null
+++ b/Platform/Broadcom/Bcm283x/Include/IndustryStandard/Bcm2837Gpio.h
@@ -0,0 +1,50 @@
+/** @file
+ *
+ *  Copyright (c) 2018, Andrei Warkentin 
+ *  Copyright (c) Microsoft Corporation. All rights reserved.
+ *
+ *  This program and the accompanying materials
+ *  are licensed and made available under the terms and conditions of the BSD 
License
+ *  which accompanies this distribution.  The full text of the license may be 
found at
+ *  http://opensource.org/licenses/bsd-license.php
+ *
+ *  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+ *  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
IMPLIED.
+ *
+ **/
+
+#ifndef __BCM2837GPIO_H__
+#define __BCM2837GPIO_H__
+
+#define GPIO_BASE_ADDRESS  (BCM2836_SOC_REGISTERS + 0x0020)
+
+#define GPIO_GPFSEL0   (GPIO_BASE_ADDRESS + 0x00)
+#define GPIO_GPFSEL1   (GPIO_BASE_ADDRESS + 0x04)
+#define GPIO_GPFSEL2   (GPIO_BASE_ADDRESS + 0x08)
+#define GPIO_GPFSEL3   (GPIO_BASE_ADDRESS + 0x0C)
+#define GPIO_GPFSEL4   (GPIO_BASE_ADDRESS + 0x10)
+#define GPIO_GPFSEL5   (GPIO_BASE_ADDRESS + 0x14)
+
+#define GPIO_GPCLR0(GPIO_BASE_ADDRESS + 0x28)
+#define GPIO_GPCLR1(GPIO_BASE_ADDRESS + 0x2C)
+
+#define GPIO_GPSET0(GPIO_BASE_ADDRESS + 0x1C)
+#define GPIO_GPSET1(GPIO_BASE_ADDRESS + 0x20)
+
+#define GPIO_FSEL_INPUT0x0
+#define GPIO_FSEL_OUTPUT   0x1
+#define GPIO_FSEL_ALT0 0x4
+#define GPIO_FSEL_ALT1 0x5
+#define GPIO_FSEL_ALT2 0x6
+#define GPIO_FSEL_ALT3 0x7
+#define GPIO_FSEL_ALT4 0x3
+#define GPIO_FSEL_ALT5 0x2
+
+#define GPIO_FSEL_PINS_PER_REGISTER 10
+#define GPIO_FSEL_BITS_PER_PIN  3
+#define GPIO_FSEL_MASK  ((1 << GPIO_FSEL_BITS_PER_PIN) - 1)
+
+#define GPIO_PINS  54
+
+#endif // __BCM2837GPIO_H__
+
diff --git a/Platform/Broadcom/Bcm283x/Include/Library/GpioLib.h 
b/Platform/Broadcom/Bcm283x/Include/Library/GpioLib.h
new file mode 100644
index ..ca9da4b11a87
--- /dev/null
+++ b/Platform/Broadcom/Bcm283x/Include/Library/GpioLib.h
@@ -0,0 +1,33 @@
+/** @file
+ *
+ *  GPIO manipulation.
+ *
+ *  Copyright (c) 2018, Andrei Warkentin 
+ *
+ *  This program and the accompanying materials
+ *  are licensed and made available under the terms and conditions of the BSD 
License
+ *  which accompanies this distribution.  The full text of the license may be 
found at
+ *  http://opensource.org/licenses/bsd-license.php
+ *
+ *  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+ *  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
IMPLIED.
+ *
+ **/
+
+#ifndef __GPIO_LIB__
+#define __GPIO_LIB__
+
+#include 
+
+VOID
+GpioPinFuncSet(


Space before ( please [throughout]


Okay. I'll spend some time going over our files to make sure the 
formatting is consistent with the EDK2 coding style.



+  IN  UINTN Pin,
+  IN  UINTN Function
+  );
+
+UINTN
+GpioPinFuncGet(
+  IN  UINTN Pin
+  );
+
+#endif /* __GPIO_LIB__ */
diff --git a/Platform/Broadcom/Bcm283x/Include/Utils.h 
b/Platform/Broadcom/Bcm283x/Include/Utils.h
new file mode 100644
index ..47713275de3f
--- /dev/null
+++ b/Platform/Broadcom/Bcm283x/Include/Utils.h
@@ -0,0 +1,33 @@
+/** @file
+ *
+ *  Copyright (c) 2018, Andrei Warkentin 
+ *
+ *  This program and the accompanying materials
+ *  are licensed and made available under the terms and conditions of the BSD 
License
+ *  which accompanies this distribution.  The full text of the license may be 
found at
+ *  http://opensource.org/licenses/bsd-license.php
+ *
+ *  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+ *  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
IMPLIED.
+ *
+ **/
+
+#ifndef UTILS_H
+#define UTILS_H
+
+#define _IX_BITS(sm, bg) ((bg) - (sm) + 1)
+#define _IX_MASK(sm

Re: [edk2] [PATCH 1/2] MdePkg/BaseLib: Add two public functions

[Leif Lindholm]

Hi Zhenglei,

Please change the title to
MdePkg/BaseLib: add common versions of CharToUpper and AsciiToUpper
and split the change/deletion in String/SafeString/BaseLibInternals
into a separate patch, following after this one.

Other than that, glad to see this consolidation.

Regards,

Leif

On Thu, Dec 13, 2018 at 04:34:38PM +0800, Shenglei Zhang wrote:
> Add two public functions, CharToUpper and AsciiToUpper.
> InternalCharToUpper and InternalBaseLibAsciiToUpper have the same
> functions as CharToUpper and AsciiToUpper, but they are internal ones.
> So the internal ones are removed and replace them with public ones
> in other places.
> https://bugzilla.tianocore.org/show_bug.cgi?id=1369
> 
> Cc: Michael D Kinney 
> Cc: Liming Gao 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Shenglei Zhang 
> ---
>  MdePkg/Include/Library/BaseLib.h  | 40 +
>  MdePkg/Library/BaseLib/BaseLibInternals.h | 42 ---
>  MdePkg/Library/BaseLib/SafeString.c   |  8 ++---
>  MdePkg/Library/BaseLib/String.c   | 35 ---
>  4 files changed, 51 insertions(+), 74 deletions(-)
> 
> diff --git a/MdePkg/Include/Library/BaseLib.h 
> b/MdePkg/Include/Library/BaseLib.h
> index 8cc086983d..b861d82287 100644
> --- a/MdePkg/Include/Library/BaseLib.h
> +++ b/MdePkg/Include/Library/BaseLib.h
> @@ -2720,6 +2720,46 @@ AsciiStrnToUnicodeStrS (
>OUT UINTN *DestinationLength
>);
>  
> +/**
> +  Convert a Unicode character to upper case only if
> +  it maps to a valid small-case ASCII character.
> +
> +  This internal function only deal with Unicode character
> +  which maps to a valid small-case ASCII character, i.e.
> +  L'a' to L'z'. For other Unicode character, the input character
> +  is returned directly.
> +
> +  @param  Char  The character to convert.
> +
> +  @retval LowerCharacter   If the Char is with range L'a' to L'z'.
> +  @retval UnchangedOtherwise.
> +
> +**/
> +CHAR16
> +EFIAPI
> +CharToUpper (
> +  IN  CHAR16Char
> +  );
> +
> +/**
> +  Converts a lowercase Ascii character to upper one.
> +
> +  If Chr is lowercase Ascii character, then converts it to upper one.
> +
> +  If Value >= 0xA0, then ASSERT().
> +  If (Value & 0x0F) >= 0x0A, then ASSERT().
> +
> +  @param  Chr   one Ascii character
> +
> +  @return The uppercase value of Ascii character
> +
> +**/
> +CHAR8
> +EFIAPI
> +AsciiToUpper (
> +  IN  CHAR8 Chr
> +  );
> +
>  /**
>Converts an 8-bit value to an 8-bit BCD value.
>  
> diff --git a/MdePkg/Library/BaseLib/BaseLibInternals.h 
> b/MdePkg/Library/BaseLib/BaseLibInternals.h
> index 8855231c1a..9db925b157 100644
> --- a/MdePkg/Library/BaseLib/BaseLibInternals.h
> +++ b/MdePkg/Library/BaseLib/BaseLibInternals.h
> @@ -469,28 +469,6 @@ InternalIsDecimalDigitCharacter (
>);
>  
>  
> -/**
> -  Convert a Unicode character to upper case only if
> -  it maps to a valid small-case ASCII character.
> -
> -  This internal function only deal with Unicode character
> -  which maps to a valid small-case ASCII character, i.e.
> -  L'a' to L'z'. For other Unicode character, the input character
> -  is returned directly.
> -
> -  @param  Char  The character to convert.
> -
> -  @retval LowerCharacter   If the Char is with range L'a' to L'z'.
> -  @retval UnchangedOtherwise.
> -
> -**/
> -CHAR16
> -EFIAPI
> -InternalCharToUpper (
> -  IN  CHAR16Char
> -  );
> -
> -
>  /**
>Convert a Unicode character to numerical value.
>  
> @@ -552,26 +530,6 @@ InternalAsciiIsDecimalDigitCharacter (
>);
>  
>  
> -/**
> -  Converts a lowercase Ascii character to upper one.
> -
> -  If Chr is lowercase Ascii character, then converts it to upper one.
> -
> -  If Value >= 0xA0, then ASSERT().
> -  If (Value & 0x0F) >= 0x0A, then ASSERT().
> -
> -  @param  Chr   one Ascii character
> -
> -  @return The uppercase value of Ascii character
> -
> -**/
> -CHAR8
> -EFIAPI
> -InternalBaseLibAsciiToUpper (
> -  IN  CHAR8 Chr
> -  );
> -
> -
>  /**
>Check if a ASCII character is a hexadecimal character.
>  
> diff --git a/MdePkg/Library/BaseLib/SafeString.c 
> b/MdePkg/Library/BaseLib/SafeString.c
> index 417497cbc9..17f88b46d8 100644
> --- a/MdePkg/Library/BaseLib/SafeString.c
> +++ b/MdePkg/Library/BaseLib/SafeString.c
> @@ -905,7 +905,7 @@ StrHexToUintnS (
>  String++;
>}
>  
> -  if (InternalCharToUpper (*String) == L'X') {
> +  if (CharToUpper (*String) == L'X') {
>  if (*(String - 1) != L'0') {
>*Data = 0;
>return RETURN_SUCCESS;
> @@ -1036,7 +1036,7 @@ StrHexToUint64S (
>  String++;
>}
>  
> -  if (InternalCharToUpper (*String) == L'X') {
> +  if (CharToUpper (*String) == L'X') {
>  if (*(String - 1) != L'0') {
>*Data = 0;
>return RETURN_SUCCESS;
> @@ -2459,7 +2459,7 @@ AsciiStrHexToUintnS (
>  String++;
>}
>  
> -  if (InternalBaseLibAsc

Re: [edk2] Secureboot enable with OVMF

[Laszlo Ersek]

On 12/13/18 02:25, Park, Kyung Min wrote:
> Hi,
> 
> I'm trying to enable the secureboot with OVMF. I followed the steps as below.
> But When I executed LockDown.efi, it gives me an error which says, "Failed to 
> enroll PK: 26".
> According to UEFI spec, the 26 means EFI_SECURITY_VIOLATION, but I don't 
> understand why I got this error.
> Before I ran the LockDown.efi, the secureboot was disabled by default and the 
> PK key was not enrolled.
> 
> 1. Build OVMF with secureboot enable
> https://wiki.ubuntu.com/UEFI/EDK2

Please know that, if you build OVMF with *just* SECURE_BOOT_ENABLE, but
without SMM_REQUIRE, then a malicious guest OS may modify the pflash
chip with direct hardware access that contains the authenticated UEFI
variables. In other words, a malicious guest OS may circumvent Secure Boot.

If that's OK for your use case (it could be), then it's OK to use just
SECURE_BOOT_ENABLE; but it should be a conscious decision.

Regarding SMM_REQUIRE, please see OvmfPkg/README, section "SMM support".

> 2. Generate/Execute LockDown.efi to enroll PK/KEK/DB keys
> git://git.kernel.org/pub/scm/linux/kernel/git/jejb/efitools.git
> 
> I would appreciate any useful information about this error.

You'll have to dig into LockDown.efi for that. If you are convinced
LockDown.efi does the right thing, then you'll have to add debug
messages to the edk2 stack that handles authenticated variables (the
variable driver, some SecurityPkg / CryptoPkg libraries, etc). This is
usually quite time consuming.

As an alternative, you might be able to use
"/usr/share/edk2/ovmf/EnrollDefaultKeys.efi", from the "edk2-ovmf"
subpackage package, from

  https://koji.fedoraproject.org/koji/packageinfo?packageID=16183

(You can find the source code in the SRPM.)

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

Re: [edk2] [PATCH v2 edk2-platforms 01/20] Platform/Broadcom/RPi3: Add Reset and Memory Init libraries

[Pete Batard]

On 2018.12.12 20:43, Ard Biesheuvel wrote:

On Mon, 10 Dec 2018 at 13:39, Pete Batard  wrote:


Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Pete Batard 
---
  Platform/Broadcom/Bcm283x/Library/MemoryInitPeiLib/MemoryInitPeiLib.c   | 183 

  Platform/Broadcom/Bcm283x/Library/MemoryInitPeiLib/MemoryInitPeiLib.inf |  51 
++
  Platform/Broadcom/Bcm283x/Library/ResetLib/ResetLib.c   | 104 
+++
  Platform/Broadcom/Bcm283x/Library/ResetLib/ResetLib.inf |  46 
+
  4 files changed, 384 insertions(+)

diff --git 
a/Platform/Broadcom/Bcm283x/Library/MemoryInitPeiLib/MemoryInitPeiLib.c 
b/Platform/Broadcom/Bcm283x/Library/MemoryInitPeiLib/MemoryInitPeiLib.c
new file mode 100644
index ..81d810b5d428
--- /dev/null
+++ b/Platform/Broadcom/Bcm283x/Library/MemoryInitPeiLib/MemoryInitPeiLib.c
@@ -0,0 +1,183 @@
+/** @file
+ *
+ *  Copyright (c) 2017-2018, Andrey Warkentin 
+ *  Copyright (c) 2011-2015, ARM Limited. All rights reserved.
+ *
+ *  This program and the accompanying materials
+ *  are licensed and made available under the terms and conditions of the BSD 
License
+ *  which accompanies this distribution.  The full text of the license may be 
found at
+ *  http://opensource.org/licenses/bsd-license.php
+ *
+ *  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+ *  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
IMPLIED.
+ *
+ **/
+
+#include 
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+extern UINT64 mSystemMemoryEnd;
+
+VOID
+BuildMemoryTypeInformationHob (
+  VOID
+  );
+
+STATIC
+VOID
+InitMmu (
+  IN ARM_MEMORY_REGION_DESCRIPTOR  *MemoryTable
+  )
+{
+
+  VOID  *TranslationTableBase;
+  UINTN TranslationTableSize;


You can drop these


+  RETURN_STATUS Status;
+
+  //Note: Because we called PeiServicesInstallPeiMemory() before to call 
InitMmu() the MMU Page Table resides in
+  //  DRAM (even at the top of DRAM as it is the first permanent memory 
allocation)
+  Status = ArmConfigureMmu (MemoryTable, &TranslationTableBase, 
&TranslationTableSize);


... given that they are only passed here, and are actually OPTIONAL


Good point. We'll drop these and pass VOID instead.


+  if (EFI_ERROR (Status)) {
+DEBUG ((DEBUG_ERROR, "Error: Failed to enable MMU\n"));
+  }
+}
+
+STATIC
+VOID
+AddAndRTSData(ARM_MEMORY_REGION_DESCRIPTOR *Desc)


What does AddAnd mean?


I'm not sure myself, so I agree this is confusing and needs to be changed.


Can we improve the naming in the file?


We'll rename all the AddAnd... function calls to something that is more 
explicit wrt the goal of these functions.



Also, please use a space before (


Okay.


+{
+  BuildResourceDescriptorHob (
+  EFI_RESOURCE_SYSTEM_MEMORY,
+  EFI_RESOURCE_ATTRIBUTE_PRESENT |
+  EFI_RESOURCE_ATTRIBUTE_INITIALIZED |
+  EFI_RESOURCE_ATTRIBUTE_WRITE_COMBINEABLE |
+  EFI_RESOURCE_ATTRIBUTE_WRITE_THROUGH_CACHEABLE |
+  EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE |
+  EFI_RESOURCE_ATTRIBUTE_TESTED,
+  Desc->PhysicalBase,
+  Desc->Length
+  );
+
+  BuildMemoryAllocationHob (
+Desc->PhysicalBase,
+Desc->Length,
+EfiRuntimeServicesData
+);
+}
+
+STATIC
+VOID
+AddAndReserved(ARM_MEMORY_REGION_DESCRIPTOR *Desc)
+{
+  BuildResourceDescriptorHob (
+  EFI_RESOURCE_SYSTEM_MEMORY,
+  EFI_RESOURCE_ATTRIBUTE_PRESENT |
+  EFI_RESOURCE_ATTRIBUTE_INITIALIZED |
+  EFI_RESOURCE_ATTRIBUTE_WRITE_COMBINEABLE |
+  EFI_RESOURCE_ATTRIBUTE_WRITE_THROUGH_CACHEABLE |
+  EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE |
+  EFI_RESOURCE_ATTRIBUTE_TESTED,
+  Desc->PhysicalBase,
+  Desc->Length
+  );
+
+  BuildMemoryAllocationHob (
+Desc->PhysicalBase,
+Desc->Length,
+EfiReservedMemoryType
+);
+}
+
+STATIC
+VOID
+AddAndMmio(ARM_MEMORY_REGION_DESCRIPTOR *Desc)
+{
+  BuildResourceDescriptorHob (
+  EFI_RESOURCE_SYSTEM_MEMORY,
+  (EFI_RESOURCE_ATTRIBUTE_PRESENT|
+   EFI_RESOURCE_ATTRIBUTE_INITIALIZED |
+   EFI_RESOURCE_ATTRIBUTE_UNCACHEABLE |
+

Re: [edk2] [PATCH] ShellPkg/UefiShellDebug1CommandsLib: Remove an unused function

[Laszlo Ersek]

On 12/13/18 06:33, Shenglei Zhang wrote:
> CharToUpper is an unused internal function, so it will be removed.
> 
> Cc: Jaben Carsey 
> Cc: Ruiyu Ni 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Shenglei Zhang 
> ---
>  .../UefiShellDebug1CommandsLib.c  | 28 ---
>  1 file changed, 28 deletions(-)

Please change the subject line to:

ShellPkg/UefiShellDebug1CommandsLib: remove unused function CharToUpper

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

Re: [edk2] [PATCH] Maintainers.txt: Change package maintainer and reviewer of CryptoPkg.

[Laszlo Ersek]

Hi Ting,

On 12/13/18 08:41, tye1 wrote:
> Cc: Gang Wei 
> Cc: Jian Wang 
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ting Ye 
> ---
>  Maintainers.txt | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/Maintainers.txt b/Maintainers.txt
> index 001d8ba010..d5cb305da9 100644
> --- a/Maintainers.txt
> +++ b/Maintainers.txt
> @@ -102,8 +102,9 @@ S: Maintained
>  
>  CryptoPkg
>  W: https://github.com/tianocore/tianocore.github.io/wiki/CryptoPkg
> -M: Qin Long 
>  M: Ting Ye 
> +R: Gang Wei 
> +R: Jian Wang 
>  
>  DynamicTablesPkg
>  W: https://github.com/tianocore/tianocore.github.io/wiki/DynamicTablesPkg
> 

This patch does not conform to the rule that we added lately; please see
commit 9ebef6c0a7d3 ("Maintainers.txt: Add the rule to hand over the
package maintain role", 2018-11-29).

In other words, the patch should be sent out by Qin Long. Even though
you co-maintain CryptoPkg with Qin Long, you shouldn't be able to
deprive Qin Long from the role.

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

Re: [edk2] [PATCH 1/2] MdePkg/BaseLib: Add two public functions

[Laszlo Ersek]

On 12/13/18 09:34, Shenglei Zhang wrote:
> Add two public functions, CharToUpper and AsciiToUpper.
> InternalCharToUpper and InternalBaseLibAsciiToUpper have the same
> functions as CharToUpper and AsciiToUpper, but they are internal ones.
> So the internal ones are removed and replace them with public ones
> in other places.
> https://bugzilla.tianocore.org/show_bug.cgi?id=1369
> 
> Cc: Michael D Kinney 
> Cc: Liming Gao 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Shenglei Zhang 
> ---
>  MdePkg/Include/Library/BaseLib.h  | 40 +
>  MdePkg/Library/BaseLib/BaseLibInternals.h | 42 ---
>  MdePkg/Library/BaseLib/SafeString.c   |  8 ++---
>  MdePkg/Library/BaseLib/String.c   | 35 ---
>  4 files changed, 51 insertions(+), 74 deletions(-)

I have no comment on the code, but please change the subject line to:

  MdePkg/BaseLib: introduce CharToUpper() and AsciiToUpper() publicly

or something similar.

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

Re: [edk2] [Patch] BaseTools: Optimize string concatenation

[Leif Lindholm]

Hi Liming,

Yes, this is fine.
But for future submissions, I would like for this sort of information
to be provided at the time of posting.

Not the full breakdown, but "reduceces build time of platform XXX on
hardware YYY by A%/B seconds".

Ideally, more than one platform and more than one hardware should be
provided, but at least during this initial improvement phase I'm also
happy for the assumption being that unless someone else complains,
it's fine on others.

Regards,

Leif

On Thu, Dec 13, 2018 at 01:50:35AM +, Gao, Liming wrote:
> Leif:
>   Kabylake platform is the real Intel hardware.  The MinKabylake is the 
> minimal feature of the Kabylake BIOS. Here is MinKabylake BIOS code 
> https://github.com/tianocore/edk2-platforms/tree/devel-MinPlatform
>   Bob adds the build performance data of MinKabylake into 
> https://bugzilla.tianocore.org/show_bug.cgi?id=1288. 
> 
> The original build performance data:
> Build Duration:   00:02:23
> AutoGen Duration: 00:00:42
> Make Duration:00:01:12
> GenFds Duration:  00:00:27
> 
> After apply the patch, clean build performance is reduced from 2:23 to 1:57. 
> So, I think his patch improves build performance. 
> Build Duration:   00:01:57
> AutoGen Duration: 00:00:23
> Make Duration:00:01:12
> GenFds Duration:  00:00:21
> 
> Thanks
> Liming
> >-Original Message-
> >From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Leif
> >Lindholm
> >Sent: Thursday, December 13, 2018 2:37 AM
> >To: Feng, Bob C 
> >Cc: Carsey, Jaben ; edk2-devel@lists.01.org; Gao,
> >Liming 
> >Subject: Re: [edk2] [Patch] BaseTools: Optimize string concatenation
> >
> >On Tue, Dec 11, 2018 at 08:48:19AM +, Feng, Bob C wrote:
> >> Hi Leif,
> >>
> >> I understand your concern.
> >>
> >> I collected another performance data set based on open source
> >> MinKabylake platform and updated the BZ
> >> https://bugzilla.tianocore.org/show_bug.cgi?id=1288. The data looks
> >> better than Ovmf. It enabled multiple SKU.
> >>
> >> Before I sent those patch, I did verify them on intel real
> >> platforms. It improves the build performance. But it's not
> >> convenient to share those data.
> >
> >So, I have two comments on this:
> >1) How can it be inconvenient to share information on build times? I
> >   don't even care what the names or codenames for those platforms
> >   are. If you are unable to tell us why what you have done matters,
> >   the code changes do not belong in the public tree.
> >   Clearly having good performance numbers for public platforms is the
> >   easiest solution for this problem.
> >2) Submissions of improvements to build system performance should be
> >   verified building real platforms. It should not be a question of
> >   "find some other platform to get numbers from once we have improved
> >   performance for building our confidential platforms".
> >
> >Regards,
> >
> >Leif
> >>
> >> Thanks,
> >> Bob
> >>
> >> -Original Message-
> >> From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
> >> Sent: Monday, December 10, 2018 8:36 PM
> >> To: Feng, Bob C 
> >> Cc: edk2-devel@lists.01.org; Carsey, Jaben ; Gao,
> >Liming 
> >> Subject: Re: [edk2] [Patch] BaseTools: Optimize string concatenation
> >>
> >> On Mon, Dec 10, 2018 at 12:09:23PM +, Feng, Bob C wrote:
> >> > For the "customized deepcopy" and "cache for uni file parser" data,
> >> > you can see the AutoGen is not slower. The whole Build Duration is
> >> > longer because Make Duration is longer while Make Duration time
> >> > depends on the external make, compiler and linker. So it's not the
> >> > patch make the build slow down.
> >> >
> >> > Yes,  it's not faster either. I think that because the Ovmf platform
> >> > is relatively simple.  From the build tool source code point of view,
> >> > the customized deepcopy will take effect if the platform enabled
> >> > multiple SKU or there are many expressions in metadata file to be
> >> > evaluated. And the "cache for uni file parser" needs there are many
> >> > uni files.  The Ovmf platform looks not a good platform to demo the
> >> > effect of this patch.
> >>
> >> But surely we should not introduce patches said to improve performance
> >when the only data we have available shows that they slow things down?
> >>
> >> If the performance data is not representative, then it is worthless.
> >>
> >> Don't get me wrong - if you say "and for this secret platform I can't share
> >with you, it improves build performance by X", then I may be OK with a minor
> >slowdown on the platforms I do have available to test, if X is not minor.
> >>
> >> But if the improvement is only theoretical, and we have no evidence that it
> >helps real platforms, it should not be committed.
> >>
> >> Regards,
> >>
> >> Leif
> >___
> >edk2-devel mailing list
> >edk2-devel@lists.01.org
> >https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [PATCH 1/2] MdePkg/BaseLib: Add two public functions

[Shenglei Zhang]

Add two public functions, CharToUpper and AsciiToUpper.
InternalCharToUpper and InternalBaseLibAsciiToUpper have the same
functions as CharToUpper and AsciiToUpper, but they are internal ones.
So the internal ones are removed and replace them with public ones
in other places.
https://bugzilla.tianocore.org/show_bug.cgi?id=1369

Cc: Michael D Kinney 
Cc: Liming Gao 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Shenglei Zhang 
---
 MdePkg/Include/Library/BaseLib.h  | 40 +
 MdePkg/Library/BaseLib/BaseLibInternals.h | 42 ---
 MdePkg/Library/BaseLib/SafeString.c   |  8 ++---
 MdePkg/Library/BaseLib/String.c   | 35 ---
 4 files changed, 51 insertions(+), 74 deletions(-)

diff --git a/MdePkg/Include/Library/BaseLib.h b/MdePkg/Include/Library/BaseLib.h
index 8cc086983d..b861d82287 100644
--- a/MdePkg/Include/Library/BaseLib.h
+++ b/MdePkg/Include/Library/BaseLib.h
@@ -2720,6 +2720,46 @@ AsciiStrnToUnicodeStrS (
   OUT UINTN *DestinationLength
   );
 
+/**
+  Convert a Unicode character to upper case only if
+  it maps to a valid small-case ASCII character.
+
+  This internal function only deal with Unicode character
+  which maps to a valid small-case ASCII character, i.e.
+  L'a' to L'z'. For other Unicode character, the input character
+  is returned directly.
+
+  @param  Char  The character to convert.
+
+  @retval LowerCharacter   If the Char is with range L'a' to L'z'.
+  @retval UnchangedOtherwise.
+
+**/
+CHAR16
+EFIAPI
+CharToUpper (
+  IN  CHAR16Char
+  );
+
+/**
+  Converts a lowercase Ascii character to upper one.
+
+  If Chr is lowercase Ascii character, then converts it to upper one.
+
+  If Value >= 0xA0, then ASSERT().
+  If (Value & 0x0F) >= 0x0A, then ASSERT().
+
+  @param  Chr   one Ascii character
+
+  @return The uppercase value of Ascii character
+
+**/
+CHAR8
+EFIAPI
+AsciiToUpper (
+  IN  CHAR8 Chr
+  );
+
 /**
   Converts an 8-bit value to an 8-bit BCD value.
 
diff --git a/MdePkg/Library/BaseLib/BaseLibInternals.h 
b/MdePkg/Library/BaseLib/BaseLibInternals.h
index 8855231c1a..9db925b157 100644
--- a/MdePkg/Library/BaseLib/BaseLibInternals.h
+++ b/MdePkg/Library/BaseLib/BaseLibInternals.h
@@ -469,28 +469,6 @@ InternalIsDecimalDigitCharacter (
   );
 
 
-/**
-  Convert a Unicode character to upper case only if
-  it maps to a valid small-case ASCII character.
-
-  This internal function only deal with Unicode character
-  which maps to a valid small-case ASCII character, i.e.
-  L'a' to L'z'. For other Unicode character, the input character
-  is returned directly.
-
-  @param  Char  The character to convert.
-
-  @retval LowerCharacter   If the Char is with range L'a' to L'z'.
-  @retval UnchangedOtherwise.
-
-**/
-CHAR16
-EFIAPI
-InternalCharToUpper (
-  IN  CHAR16Char
-  );
-
-
 /**
   Convert a Unicode character to numerical value.
 
@@ -552,26 +530,6 @@ InternalAsciiIsDecimalDigitCharacter (
   );
 
 
-/**
-  Converts a lowercase Ascii character to upper one.
-
-  If Chr is lowercase Ascii character, then converts it to upper one.
-
-  If Value >= 0xA0, then ASSERT().
-  If (Value & 0x0F) >= 0x0A, then ASSERT().
-
-  @param  Chr   one Ascii character
-
-  @return The uppercase value of Ascii character
-
-**/
-CHAR8
-EFIAPI
-InternalBaseLibAsciiToUpper (
-  IN  CHAR8 Chr
-  );
-
-
 /**
   Check if a ASCII character is a hexadecimal character.
 
diff --git a/MdePkg/Library/BaseLib/SafeString.c 
b/MdePkg/Library/BaseLib/SafeString.c
index 417497cbc9..17f88b46d8 100644
--- a/MdePkg/Library/BaseLib/SafeString.c
+++ b/MdePkg/Library/BaseLib/SafeString.c
@@ -905,7 +905,7 @@ StrHexToUintnS (
 String++;
   }
 
-  if (InternalCharToUpper (*String) == L'X') {
+  if (CharToUpper (*String) == L'X') {
 if (*(String - 1) != L'0') {
   *Data = 0;
   return RETURN_SUCCESS;
@@ -1036,7 +1036,7 @@ StrHexToUint64S (
 String++;
   }
 
-  if (InternalCharToUpper (*String) == L'X') {
+  if (CharToUpper (*String) == L'X') {
 if (*(String - 1) != L'0') {
   *Data = 0;
   return RETURN_SUCCESS;
@@ -2459,7 +2459,7 @@ AsciiStrHexToUintnS (
 String++;
   }
 
-  if (InternalBaseLibAsciiToUpper (*String) == 'X') {
+  if (AsciiToUpper (*String) == 'X') {
 if (*(String - 1) != '0') {
   *Data = 0;
   return RETURN_SUCCESS;
@@ -2586,7 +2586,7 @@ AsciiStrHexToUint64S (
 String++;
   }
 
-  if (InternalBaseLibAsciiToUpper (*String) == 'X') {
+  if (AsciiToUpper (*String) == 'X') {
 if (*(String - 1) != '0') {
   *Data = 0;
   return RETURN_SUCCESS;
diff --git a/MdePkg/Library/BaseLib/String.c b/MdePkg/Library/BaseLib/String.c
index e6df12797d..ced1b3f975 100644
--- a/MdePkg/Library/BaseLib/String.c
+++ b/MdePkg/Library/BaseLib/String.c
@@ -552,7 +552,7 @@ InternalIsDecimalDigitCharacter (
 **/
 CHAR16
 EFIAPI
-InternalCharToU

[edk2] [PATCH 2/2] BaseTools/Common: Remove InternalCharToUpper

[Shenglei Zhang]

InternalCharToUpper is an internal function.
So remove InternalCharToUpper and replace it with a public
function CharToUpper which has the same function as the internal one
in all places.
https://bugzilla.tianocore.org/show_bug.cgi?id=1369

Cc: Bob Feng 
Cc: Liming Gao 
Cc: Yonghong Zhu 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Shenglei Zhang 
---
 BaseTools/Source/C/Common/CommonLib.c | 16 ++--
 BaseTools/Source/C/Common/CommonLib.h |  4 
 2 files changed, 2 insertions(+), 18 deletions(-)

diff --git a/BaseTools/Source/C/Common/CommonLib.c 
b/BaseTools/Source/C/Common/CommonLib.c
index 5c40fdb5fd..878b593a4e 100644
--- a/BaseTools/Source/C/Common/CommonLib.c
+++ b/BaseTools/Source/C/Common/CommonLib.c
@@ -740,18 +740,6 @@ Returns:
 #endif
 }
 
-CHAR16
-InternalCharToUpper (
-CHAR16Char
-  )
-{
-  if (Char >= L'a' && Char <= L'z') {
-return (CHAR16) (Char - (L'a' - L'A'));
-  }
-
-  return Char;
-}
-
 UINTN
 StrnLenS (
CONST CHAR16  *String,
@@ -1089,7 +1077,7 @@ StrHexToUint64S (
 String++;
   }
 
-  if (InternalCharToUpper (*String) == L'X') {
+  if (CharToUpper (*String) == L'X') {
 if (*(String - 1) != L'0') {
   *Data = 0;
   return RETURN_SUCCESS;
@@ -1264,7 +1252,7 @@ InternalHexCharToUintn (
 return Char - L'0';
   }
 
-  return (10 + InternalCharToUpper (Char) - L'A');
+  return (10 + CharToUpper (Char) - L'A');
 }
 
 
diff --git a/BaseTools/Source/C/Common/CommonLib.h 
b/BaseTools/Source/C/Common/CommonLib.h
index 4e1541bc70..b81584c7d4 100644
--- a/BaseTools/Source/C/Common/CommonLib.h
+++ b/BaseTools/Source/C/Common/CommonLib.h
@@ -304,10 +304,6 @@ StrnLenS (
UINTN MaxSize
   );
 
-CHAR16
-InternalCharToUpper (
-CHAR16Char
-  );
 
 INTN
 StrCmp (
-- 
2.18.0.windows.1

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

[edk2] [PATCH 0/2] Add two public functions

[Shenglei Zhang]

Add two public functions,CharToUpper and AsciiToUpper,and
remove the same functional functions,InternalCharToUpper
and InternalBaseLibAsciiToUpper.

Cc: Michael D Kinney 
Cc: Liming Gao 
Cc: Bob Feng 
Cc: Yonghong Zhu 
Shenglei Zhang (2):
  MdePkg/BaseLib: Add two public functions
  BaseTools/Common: Remove InternalCharToUpper

 BaseTools/Source/C/Common/CommonLib.c | 16 ++---
 BaseTools/Source/C/Common/CommonLib.h |  4 ---
 MdePkg/Include/Library/BaseLib.h  | 40 +
 MdePkg/Library/BaseLib/BaseLibInternals.h | 42 ---
 MdePkg/Library/BaseLib/SafeString.c   |  8 ++---
 MdePkg/Library/BaseLib/String.c   | 35 ---
 6 files changed, 53 insertions(+), 92 deletions(-)

-- 
2.18.0.windows.1

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

Re: [edk2] [PATCH] Maintainers.txt: Change package maintainer and reviewer of CryptoPkg.

[Wei, Gang]

Reviewed-by: Gang Wei 

> -Original Message-
> From: Wang, Jian J
> Sent: Thursday, December 13, 2018 3:59 PM
> To: Ye, Ting ; edk2-devel@lists.01.org
> Cc: Wei, Gang 
> Subject: RE: [PATCH] Maintainers.txt: Change package maintainer and reviewer
> of CryptoPkg.
> 
> Reviewed-by: Jian J Wang 
> 
> > -Original Message-
> > From: Ye, Ting
> > Sent: Thursday, December 13, 2018 3:41 PM
> > To: edk2-devel@lists.01.org
> > Cc: Wei, Gang ; Wang, Jian J ;
> > Ye, Ting 
> > Subject: [PATCH] Maintainers.txt: Change package maintainer and reviewer of
> > CryptoPkg.
> >
> > Cc: Gang Wei 
> > Cc: Jian Wang 
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Ting Ye 
> > ---
> >  Maintainers.txt | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/Maintainers.txt b/Maintainers.txt
> > index 001d8ba010..d5cb305da9 100644
> > --- a/Maintainers.txt
> > +++ b/Maintainers.txt
> > @@ -102,8 +102,9 @@ S: Maintained
> >
> >  CryptoPkg
> >  W: https://github.com/tianocore/tianocore.github.io/wiki/CryptoPkg
> > -M: Qin Long 
> >  M: Ting Ye 
> > +R: Gang Wei 
> > +R: Jian Wang 
> >
> >  DynamicTablesPkg
> >  W: https://github.com/tianocore/tianocore.github.io/wiki/DynamicTablesPkg
> > --
> > 2.16.2.windows.1

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