[edk2] [edk2-staging] Create new edk2-test branch

[Kinney, Michael D]

I am creating a new branch in edk2-staging called edk2-test.

The purpose of this branch is to develop a test harness, 
test case SDK, and library of test cases that can be used 
as part of edk2 validation.

The initial version of this test harness is compatible with
binary releases of the PI SCTs and UEFI SCTs, are native
edk2 packages with no dependencies on the EdkCompatibilityPkg,
and the test harness runs using the latest version of the
UEFI Shell.  

Additional work items:
* Update to take advantage of latest edk2 features/libraries.
* Update for all supported CPU types
* Update for all supported compilers
* Review initial test harness features and determine
  what features should be dropped and what new features
  should be added. 
* Determine where the test harness, test case SDK, and 
  test cases should live once the initial functional and 
  quality criteria are met.  Could be packages in the 
  edk2 repo or packages in a new edk2-test repo.  Other 
  options???
* Resolve compatibility issues with binary releases of the
  PI SCTs and UEFI SCTs.
* Update test harness to support PEI tests
* Update test harness to support Runtime tests
* Update test harness to support SMM tests
* Optimize performance of the test harness and tests.

Please contact me if you are interested in helping with the 
test harness, the test case SDK, or the development of 
test cases.

Thanks,

Mike

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

Re: [edk2] [PATCH v2 1/1] MdePkg: Refine casting expression result to bigger size

[Gao, Liming]

Got it. Thanks for your clarification. 

>-Original Message-
>From: Wu, Hao A
>Sent: Wednesday, January 25, 2017 2:17 PM
>To: Gao, Liming 
>Cc: edk2-de...@ml01.01.org
>Subject: RE: [edk2] [PATCH v2 1/1] MdePkg: Refine casting expression result
>to bigger size
>
>> -Original Message-
>> From: Gao, Liming
>> Sent: Wednesday, January 25, 2017 1:58 PM
>> To: Wu, Hao A; Laszlo Ersek
>> Cc: edk2-de...@ml01.01.org
>> Subject: RE: [edk2] [PATCH v2 1/1] MdePkg: Refine casting expression result
>to
>> bigger size
>>
>> Hao:
>>   For PCILIB_TO_COMMON_ADDRESS, we can't assume its usage in the
>> consumer code. There may be some usage in other projects. So, I suggest to
>> provide the safe fix.
>>
>
>Hi Liming,
>
>The definition "PCILIB_TO_COMMON_ADDRESS" is defined in
>MdePkg/Library/BaseS3PciLib/S3PciLib.c. It will not be consumed outside.
>
>Best Regards,
>Hao Wu
>
>> Thanks
>> Liming
>> >-Original Message-
>> >From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>Wu,
>> >Hao A
>> >Sent: Wednesday, January 25, 2017 8:26 AM
>> >To: Laszlo Ersek 
>> >Cc: edk2-de...@ml01.01.org
>> >Subject: Re: [edk2] [PATCH v2 1/1] MdePkg: Refine casting expression
>result
>> >to bigger size
>> >
>> >> -Original Message-
>> >> From: Laszlo Ersek [mailto:ler...@redhat.com]
>> >> Sent: Tuesday, January 24, 2017 5:54 PM
>> >> To: Wu, Hao A
>> >> Cc: edk2-de...@ml01.01.org
>> >> Subject: Re: [edk2] [PATCH v2 1/1] MdePkg: Refine casting expression
>result
>> >to
>> >> bigger size
>> >>
>> >> On 01/24/17 08:25, Hao Wu wrote:
>> >> > There are cases that the operands of an expression are all with rank
>less
>> >> > than UINT64/INT64 and the result of the expression is explicitly casted
>to
>> >> > UINT64/INT64 to fit the target size.
>> >> >
>> >> > An example will be:
>> >> > UINT32 a,b;
>> >> > // a and b can be any unsigned int type with rank less than UINT64, like
>> >> > // UINT8, UINT16, etc.
>> >> > UINT64 c;
>> >> > c = (UINT64) (a + b);
>> >> >
>> >> > Some static code checkers may warn that the expression result might
>> >> > overflow within the rank of "int" (integer promotions) and the result is
>> >> > then cast to a bigger size.
>> >> >
>> >> > The commit refines codes by the following rules:
>> >> > 1). When the expression will not overflow within the rank of "int",
>remove
>> >> > the explicit type casts:
>> >> > c = a + b;
>> >> >
>> >> > 2). When the expression is possible to overflow the range of unsigned
>int/
>> >> > int:
>> >> > c = (UINT64)a + b;
>> >> >
>> >> > Contributed-under: TianoCore Contribution Agreement 1.0
>> >> > Signed-off-by: Hao Wu 
>> >> > ---
>> >> >  MdePkg/Library/BaseLib/String.c  |  4 ++--
>> >> >  MdePkg/Library/BasePeCoffLib/BasePeCoff.c| 12 
>> >> > +-
>--
>> >> >  MdePkg/Library/BaseS3PciLib/S3PciLib.c   |  4 ++--
>> >> >  MdePkg/Library/SmmMemoryAllocationLib/MemoryAllocationLib.c  |
>4
>> >++--
>> >> >  MdePkg/Library/UefiMemoryAllocationLib/MemoryAllocationLib.c |  4
>> >++--
>> >> >  5 files changed, 13 insertions(+), 15 deletions(-)
>> >> >
>> >> > diff --git a/MdePkg/Library/BaseLib/String.c
>> >> b/MdePkg/Library/BaseLib/String.c
>> >> > index e84bf50..4151e0e 100644
>> >> > --- a/MdePkg/Library/BaseLib/String.c
>> >> > +++ b/MdePkg/Library/BaseLib/String.c
>> >> > @@ -586,7 +586,7 @@ InternalHexCharToUintn (
>> >> >  return Char - L'0';
>> >> >}
>> >> >
>> >> > -  return (UINTN) (10 + InternalCharToUpper (Char) - L'A');
>> >> > +  return (10 + InternalCharToUpper (Char) - L'A');
>> >> >  }
>> >> >
>> >> >  /**
>> >> > @@ -1211,7 +1211,7 @@ InternalAsciiHexCharToUintn (
>> >> >  return Char - '0';
>> >> >}
>> >> >
>> >> > -  return (UINTN) (10 + InternalBaseLibAsciiToUpper (Char) - 'A');
>> >> > +  return (10 + InternalBaseLibAsciiToUpper (Char) - 'A');
>> >> >  }
>> >> >
>> >> >
>> >> > diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
>> >> b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
>> >> > index 33cad23..8d1daba 100644
>> >> > --- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
>> >> > +++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
>> >> > @@ -15,7 +15,7 @@
>> >> >PeCoffLoaderGetPeHeader() routine will do basic check for PE/COFF
>> >header.
>> >> >PeCoffLoaderGetImageInfo() routine will do basic check for whole
>> >PE/COFF
>> >> image.
>> >> >
>> >> > -  Copyright (c) 2006 - 2014, Intel Corporation. All rights 
>> >> > reserved.
>> >> > +  Copyright (c) 2006 - 2017, Intel Corporation. All rights 
>> >> > reserved.
>> >> >Portions copyright (c) 2008 - 2009, Apple Inc. All rights 
>> >> > reserved.
>> >> >This program and the accompanying materials
>> >> >are licensed and made available under the terms and conditions of
>the
>> >BSD
>> >> License
>> >> > @@ -703,12 +703,10 @@ PeCoffLoaderGetImageInfo (
>> >> >//
>> >> >DebugDirectoryEntryFileOffset 

Re: [edk2] [PATCH v2 1/1] MdePkg: Refine casting expression result to bigger size

[Wu, Hao A]

> -Original Message-
> From: Gao, Liming
> Sent: Wednesday, January 25, 2017 1:58 PM
> To: Wu, Hao A; Laszlo Ersek
> Cc: edk2-de...@ml01.01.org
> Subject: RE: [edk2] [PATCH v2 1/1] MdePkg: Refine casting expression result to
> bigger size
> 
> Hao:
>   For PCILIB_TO_COMMON_ADDRESS, we can't assume its usage in the
> consumer code. There may be some usage in other projects. So, I suggest to
> provide the safe fix.
> 

Hi Liming,

The definition "PCILIB_TO_COMMON_ADDRESS" is defined in
MdePkg/Library/BaseS3PciLib/S3PciLib.c. It will not be consumed outside.

Best Regards,
Hao Wu

> Thanks
> Liming
> >-Original Message-
> >From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Wu,
> >Hao A
> >Sent: Wednesday, January 25, 2017 8:26 AM
> >To: Laszlo Ersek 
> >Cc: edk2-de...@ml01.01.org
> >Subject: Re: [edk2] [PATCH v2 1/1] MdePkg: Refine casting expression result
> >to bigger size
> >
> >> -Original Message-
> >> From: Laszlo Ersek [mailto:ler...@redhat.com]
> >> Sent: Tuesday, January 24, 2017 5:54 PM
> >> To: Wu, Hao A
> >> Cc: edk2-de...@ml01.01.org
> >> Subject: Re: [edk2] [PATCH v2 1/1] MdePkg: Refine casting expression result
> >to
> >> bigger size
> >>
> >> On 01/24/17 08:25, Hao Wu wrote:
> >> > There are cases that the operands of an expression are all with rank less
> >> > than UINT64/INT64 and the result of the expression is explicitly casted 
> >> > to
> >> > UINT64/INT64 to fit the target size.
> >> >
> >> > An example will be:
> >> > UINT32 a,b;
> >> > // a and b can be any unsigned int type with rank less than UINT64, like
> >> > // UINT8, UINT16, etc.
> >> > UINT64 c;
> >> > c = (UINT64) (a + b);
> >> >
> >> > Some static code checkers may warn that the expression result might
> >> > overflow within the rank of "int" (integer promotions) and the result is
> >> > then cast to a bigger size.
> >> >
> >> > The commit refines codes by the following rules:
> >> > 1). When the expression will not overflow within the rank of "int", 
> >> > remove
> >> > the explicit type casts:
> >> > c = a + b;
> >> >
> >> > 2). When the expression is possible to overflow the range of unsigned 
> >> > int/
> >> > int:
> >> > c = (UINT64)a + b;
> >> >
> >> > Contributed-under: TianoCore Contribution Agreement 1.0
> >> > Signed-off-by: Hao Wu 
> >> > ---
> >> >  MdePkg/Library/BaseLib/String.c  |  4 ++--
> >> >  MdePkg/Library/BasePeCoffLib/BasePeCoff.c| 12 
> >> > +---
> >> >  MdePkg/Library/BaseS3PciLib/S3PciLib.c   |  4 ++--
> >> >  MdePkg/Library/SmmMemoryAllocationLib/MemoryAllocationLib.c  |  4
> >++--
> >> >  MdePkg/Library/UefiMemoryAllocationLib/MemoryAllocationLib.c |  4
> >++--
> >> >  5 files changed, 13 insertions(+), 15 deletions(-)
> >> >
> >> > diff --git a/MdePkg/Library/BaseLib/String.c
> >> b/MdePkg/Library/BaseLib/String.c
> >> > index e84bf50..4151e0e 100644
> >> > --- a/MdePkg/Library/BaseLib/String.c
> >> > +++ b/MdePkg/Library/BaseLib/String.c
> >> > @@ -586,7 +586,7 @@ InternalHexCharToUintn (
> >> >  return Char - L'0';
> >> >}
> >> >
> >> > -  return (UINTN) (10 + InternalCharToUpper (Char) - L'A');
> >> > +  return (10 + InternalCharToUpper (Char) - L'A');
> >> >  }
> >> >
> >> >  /**
> >> > @@ -1211,7 +1211,7 @@ InternalAsciiHexCharToUintn (
> >> >  return Char - '0';
> >> >}
> >> >
> >> > -  return (UINTN) (10 + InternalBaseLibAsciiToUpper (Char) - 'A');
> >> > +  return (10 + InternalBaseLibAsciiToUpper (Char) - 'A');
> >> >  }
> >> >
> >> >
> >> > diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> >> b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> >> > index 33cad23..8d1daba 100644
> >> > --- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> >> > +++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> >> > @@ -15,7 +15,7 @@
> >> >PeCoffLoaderGetPeHeader() routine will do basic check for PE/COFF
> >header.
> >> >PeCoffLoaderGetImageInfo() routine will do basic check for whole
> >PE/COFF
> >> image.
> >> >
> >> > -  Copyright (c) 2006 - 2014, Intel Corporation. All rights reserved.
> >> > +  Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.
> >> >Portions copyright (c) 2008 - 2009, Apple Inc. All rights 
> >> > reserved.
> >> >This program and the accompanying materials
> >> >are licensed and made available under the terms and conditions of the
> >BSD
> >> License
> >> > @@ -703,12 +703,10 @@ PeCoffLoaderGetImageInfo (
> >> >//
> >> >DebugDirectoryEntryFileOffset = 0;
> >> >
> >> > -  SectionHeaderOffset = (UINTN)(
> >> > -   ImageContext->PeCoffHeaderOffset +
> >> > -   sizeof (UINT32) +
> >> > -   sizeof (EFI_IMAGE_FILE_HEADER) +
> >> > -   Hdr.Pe32->FileHeader.SizeOfOptionalHeader
> >> > -   );
> >> > +  SectionHeaderOffset = 

Re: [edk2] [PATCH v2 1/1] MdePkg: Refine casting expression result to bigger size

[Gao, Liming]

Hao:
  For PCILIB_TO_COMMON_ADDRESS, we can't assume its usage in the consumer code. 
There may be some usage in other projects. So, I suggest to provide the safe 
fix. 

Thanks
Liming
>-Original Message-
>From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Wu,
>Hao A
>Sent: Wednesday, January 25, 2017 8:26 AM
>To: Laszlo Ersek 
>Cc: edk2-de...@ml01.01.org
>Subject: Re: [edk2] [PATCH v2 1/1] MdePkg: Refine casting expression result
>to bigger size
>
>> -Original Message-
>> From: Laszlo Ersek [mailto:ler...@redhat.com]
>> Sent: Tuesday, January 24, 2017 5:54 PM
>> To: Wu, Hao A
>> Cc: edk2-de...@ml01.01.org
>> Subject: Re: [edk2] [PATCH v2 1/1] MdePkg: Refine casting expression result
>to
>> bigger size
>>
>> On 01/24/17 08:25, Hao Wu wrote:
>> > There are cases that the operands of an expression are all with rank less
>> > than UINT64/INT64 and the result of the expression is explicitly casted to
>> > UINT64/INT64 to fit the target size.
>> >
>> > An example will be:
>> > UINT32 a,b;
>> > // a and b can be any unsigned int type with rank less than UINT64, like
>> > // UINT8, UINT16, etc.
>> > UINT64 c;
>> > c = (UINT64) (a + b);
>> >
>> > Some static code checkers may warn that the expression result might
>> > overflow within the rank of "int" (integer promotions) and the result is
>> > then cast to a bigger size.
>> >
>> > The commit refines codes by the following rules:
>> > 1). When the expression will not overflow within the rank of "int", remove
>> > the explicit type casts:
>> > c = a + b;
>> >
>> > 2). When the expression is possible to overflow the range of unsigned int/
>> > int:
>> > c = (UINT64)a + b;
>> >
>> > Contributed-under: TianoCore Contribution Agreement 1.0
>> > Signed-off-by: Hao Wu 
>> > ---
>> >  MdePkg/Library/BaseLib/String.c  |  4 ++--
>> >  MdePkg/Library/BasePeCoffLib/BasePeCoff.c| 12 
>> > +---
>> >  MdePkg/Library/BaseS3PciLib/S3PciLib.c   |  4 ++--
>> >  MdePkg/Library/SmmMemoryAllocationLib/MemoryAllocationLib.c  |  4
>++--
>> >  MdePkg/Library/UefiMemoryAllocationLib/MemoryAllocationLib.c |  4
>++--
>> >  5 files changed, 13 insertions(+), 15 deletions(-)
>> >
>> > diff --git a/MdePkg/Library/BaseLib/String.c
>> b/MdePkg/Library/BaseLib/String.c
>> > index e84bf50..4151e0e 100644
>> > --- a/MdePkg/Library/BaseLib/String.c
>> > +++ b/MdePkg/Library/BaseLib/String.c
>> > @@ -586,7 +586,7 @@ InternalHexCharToUintn (
>> >  return Char - L'0';
>> >}
>> >
>> > -  return (UINTN) (10 + InternalCharToUpper (Char) - L'A');
>> > +  return (10 + InternalCharToUpper (Char) - L'A');
>> >  }
>> >
>> >  /**
>> > @@ -1211,7 +1211,7 @@ InternalAsciiHexCharToUintn (
>> >  return Char - '0';
>> >}
>> >
>> > -  return (UINTN) (10 + InternalBaseLibAsciiToUpper (Char) - 'A');
>> > +  return (10 + InternalBaseLibAsciiToUpper (Char) - 'A');
>> >  }
>> >
>> >
>> > diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
>> b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
>> > index 33cad23..8d1daba 100644
>> > --- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
>> > +++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
>> > @@ -15,7 +15,7 @@
>> >PeCoffLoaderGetPeHeader() routine will do basic check for PE/COFF
>header.
>> >PeCoffLoaderGetImageInfo() routine will do basic check for whole
>PE/COFF
>> image.
>> >
>> > -  Copyright (c) 2006 - 2014, Intel Corporation. All rights reserved.
>> > +  Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.
>> >Portions copyright (c) 2008 - 2009, Apple Inc. All rights reserved.
>> >This program and the accompanying materials
>> >are licensed and made available under the terms and conditions of the
>BSD
>> License
>> > @@ -703,12 +703,10 @@ PeCoffLoaderGetImageInfo (
>> >//
>> >DebugDirectoryEntryFileOffset = 0;
>> >
>> > -  SectionHeaderOffset = (UINTN)(
>> > -   ImageContext->PeCoffHeaderOffset +
>> > -   sizeof (UINT32) +
>> > -   sizeof (EFI_IMAGE_FILE_HEADER) +
>> > -   Hdr.Pe32->FileHeader.SizeOfOptionalHeader
>> > -   );
>> > +  SectionHeaderOffset = ImageContext->PeCoffHeaderOffset +
>> > +sizeof (UINT32) +
>> > +sizeof (EFI_IMAGE_FILE_HEADER) +
>> > +Hdr.Pe32->FileHeader.SizeOfOptionalHeader;
>> >
>> >for (Index = 0; Index < Hdr.Pe32->FileHeader.NumberOfSections;
>Index++)
>> {
>> >  //
>> > diff --git a/MdePkg/Library/BaseS3PciLib/S3PciLib.c
>> b/MdePkg/Library/BaseS3PciLib/S3PciLib.c
>> > index e29f7fe..27342b0 100644
>> > --- a/MdePkg/Library/BaseS3PciLib/S3PciLib.c
>> > +++ b/MdePkg/Library/BaseS3PciLib/S3PciLib.c
>> > @@ -3,7 +3,7 @@
>> >the PCI operations to be replayed during an S3 resume. This library 

[edk2] [PATCH] SecurityPkg: Tcg2Dxe: Update PCR[4] measure logic

[Zhang, Chao B]

Update PCR[4] measure logic for each boot attempt.
1. Measure event to PCR[4] instead of PCR[5]
2. Measure “Calling UEFI Application from Boot Option”
http://www.trustedcomputinggroup.org/wp-content/uploads/PC-ClientSpecific_Platform_Profile_for_TPM_2p0_Systems_v21.pdf

Cc: Star Zeng 
Cc: Yao Jiewen 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Chao Zhang 
---
 SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.c | 25 ++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.c 
b/SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.c
index 9aa16dc..860ee59 100644
--- a/SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.c
+++ b/SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.c
@@ -1648,8 +1648,9 @@ SetupEventLog (
 }
 
 /**
-  Measure and log an action string, and extend the measurement result into 
PCR[5].
+  Measure and log an action string, and extend the measurement result into 
PCR[PCRIndex].
 
+  @param[in] PCRIndex PCRIndex to extend
   @param[in] String   A specific string that indicates an Action 
event.  
   
   @retval EFI_SUCCESS Operation completed successfully.
@@ -1658,12 +1659,13 @@ SetupEventLog (
 **/
 EFI_STATUS
 TcgMeasureAction (
-  IN  CHAR8 *String
+  IN  TPM_PCRINDEX   PCRIndex,
+  IN  CHAR8  *String
   )
 {
   TCG_PCR_EVENT_HDR TcgEvent;
 
-  TcgEvent.PCRIndex  = 5;
+  TcgEvent.PCRIndex  = PCRIndex;
   TcgEvent.EventType = EV_EFI_ACTION;
   TcgEvent.EventSize = (UINT32)AsciiStrLen (String);
   return TcgDxeHashLogExtendEvent (
@@ -2180,6 +2182,7 @@ OnReadyToBoot (
 // 1. This is the first boot attempt.
 //
 Status = TcgMeasureAction (
+   4,
EFI_CALLING_EFI_APPLICATION
);
 if (EFI_ERROR (Status)) {
@@ -2213,11 +2216,24 @@ OnReadyToBoot (
 // 6. Not first attempt, meaning a return from last attempt
 //
 Status = TcgMeasureAction (
+   4,
EFI_RETURNING_FROM_EFI_APPLICATOIN
);
 if (EFI_ERROR (Status)) {
   DEBUG ((EFI_D_ERROR, "%a not Measured. Error!\n", 
EFI_RETURNING_FROM_EFI_APPLICATOIN));
 }
+
+//
+// 7. Next boot attempt, measure "Calling EFI Application from Boot 
Option" again
+// TCG PC Client PFP spec Section 2.4.4.5 Step 4
+//
+Status = TcgMeasureAction (
+   4,
+   EFI_CALLING_EFI_APPLICATION
+   );
+if (EFI_ERROR (Status)) {
+  DEBUG ((EFI_D_ERROR, "%a not Measured. Error!\n", 
EFI_CALLING_EFI_APPLICATION));
+}
   }
 
   DEBUG ((EFI_D_INFO, "TPM2 Tcg2Dxe Measure Data when ReadyToBoot\n"));
@@ -2250,6 +2266,7 @@ OnExitBootServices (
   // Measure invocation of ExitBootServices,
   //
   Status = TcgMeasureAction (
+ 5,
  EFI_EXIT_BOOT_SERVICES_INVOCATION
  );
   if (EFI_ERROR (Status)) {
@@ -2260,6 +2277,7 @@ OnExitBootServices (
   // Measure success of ExitBootServices
   //
   Status = TcgMeasureAction (
+ 5,
  EFI_EXIT_BOOT_SERVICES_SUCCEEDED
  );
   if (EFI_ERROR (Status)) {
@@ -2289,6 +2307,7 @@ OnExitBootServicesFailed (
   // Measure Failure of ExitBootServices,
   //
   Status = TcgMeasureAction (
+ 5,
  EFI_EXIT_BOOT_SERVICES_FAILED
  );
   if (EFI_ERROR (Status)) {
-- 
1.9.5.msysgit.1

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

Re: [edk2] [PATCH v2 0/6] Refine type cast for pointer subtraction

[Laszlo Ersek]

On 01/25/17 06:28, Hao Wu wrote:
> Please note that this patch is maily for feedback collection and only the
> MdeModulePkg part of the series is sent out firstly.
> 
> 
> V2 refines the commit messages:
> For pointer subtraction, the result is of type "ptrdiff_t". According to
> the C11 standard (Committee Draft - April 12, 2011):
> 
> "When two pointers are subtracted, both shall point to elements of the
> same array object, or one past the last element of the array object; the
> result is the difference of the subscripts of the two array elements. The
> size of the result is implementation-defined, and its type (a signed
> integer type) is ptrdiff_t defined in the  header. If the result
> is not representable in an object of that type, the behavior is
> undefined."
> 
> In our codes, there are cases that the pointer subtraction is not
> performed by pointers to elements of the same array object. This might
> lead to potential issues, since the behavior is undefined according to C11
> standard.
> 
> Also, since the size of type "ptrdiff_t" is implementation-defined. Some
> static code checkers may warn that the pointer subtraction might underflow
> first and then being cast to a bigger size. For example:
> 
> UINT8  *Ptr1, *Ptr2;
> UINTN  PtrDiff;
> ...
> PtrDiff = (UINTN) (Ptr1 - Ptr2);
> 
> The commit will refine the pointer subtraction expressions by casting each
> pointer to UINTN first and then perform the subtraction:
> 
> PtrDiff = (UINTN) Ptr1 - (UINTN) Ptr2;
> 

This commit message update looks good to me.

For all patches in the series that use this commit message (and perform
the matching source code changes):

Acked-by: Laszlo Ersek 

Module owners should of course review the patches in detail.

Thanks,
Laszlo

> 
> V1:
> For pointer subtraction, the result is of type "ptrdiff_t". According to
> the C11 spec, ptrdiff_t is a signed integer type but its size is
> implementation-defined.
> 
> There are cases that the result of pointer subtraction is type casted to
> UINTN, like:
> 
> UINT8  *Ptr1, *Ptr2;
> UINTN  PtrDiff;
> ...
> PtrDiff = (UINTN) (Ptr1 - Ptr2);
> 
> Some static code checkers may warn that the pointer subtraction might
> overflow first and then the result is being cast to a bigger size.
> 
> The series will cast each pointer to UINTN first and then perform the
> subtraction:
> 
> PtrDiff = (UINTN) Ptr1 - (UINTN) Ptr2;
> 
> 
> Cc: Laszlo Ersek 
> 
> Hao Wu (6):
>   MdeModulePkg: Refine type cast for pointer subtraction
>   CryptoPkg: Refine type cast for pointer subtraction
>   IntelFrameworkModulePkg: Refine type cast for pointer subtraction
>   NetworkPkg: Refine type cast for pointer subtraction
>   SecurityPkg: Refine type cast for pointer subtraction
>   ShellPkg: Refine type cast for pointer subtraction
> 
>  CryptoPkg/Library/BaseCryptLib/SysCall/RuntimeMemAllocation.c
> |  6 +++---
>  IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyPci.c
> |  4 ++--
>  IntelFrameworkModulePkg/Library/GenericBdsLib/BdsBoot.c  
> |  4 ++--
>  IntelFrameworkModulePkg/Library/GenericBdsLib/BdsConsole.c   
> |  4 ++--
>  IntelFrameworkModulePkg/Library/GenericBdsLib/BdsMisc.c  
> |  4 ++--
>  IntelFrameworkModulePkg/Library/LegacyBootManagerLib/LegacyBm.c  
> |  4 ++--
>  IntelFrameworkModulePkg/Universal/BdsDxe/BootMaint/BootOption.c  
> |  6 +++---
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c 
> |  4 ++--
>  MdeModulePkg/Include/Library/NetLib.h
> |  6 +++---
>  MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleLib.c
> |  4 ++--
>  MdeModulePkg/Library/DxePrintLibPrint2Protocol/PrintLib.c
> |  2 +-
>  MdeModulePkg/Library/FileExplorerLib/FileExplorer.c  
> |  6 +++---
>  MdeModulePkg/Library/PiDxeS3BootScriptLib/BootScriptSave.c   
> |  4 ++--
>  MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c   
> |  4 ++--
>  MdeModulePkg/Universal/DebugPortDxe/DebugPort.c  
> |  4 ++--
>  MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWrite.c
> |  4 ++--
>  MdeModulePkg/Universal/HiiDatabaseDxe/Image.c
> |  4 ++--
>  MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> | 10 +-
>  NetworkPkg/HttpDxe/HttpImpl.c
> |  6 +++---
>  SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> | 18 +-
>  SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c  
> | 10 +-
>  SecurityPkg/Library/Tpm2CommandLib/Tpm2Help.c
> |  6 +++---
>  SecurityPkg/Tcg/Tcg2Dxe/MeasureBootPeCoff.c  
> | 10 

[edk2] [PATCH v2 1/6] MdeModulePkg: Refine type cast for pointer subtraction

[Hao Wu]

For pointer subtraction, the result is of type "ptrdiff_t". According to
the C11 standard (Committee Draft - April 12, 2011):

"When two pointers are subtracted, both shall point to elements of the
same array object, or one past the last element of the array object; the
result is the difference of the subscripts of the two array elements. The
size of the result is implementation-defined, and its type (a signed
integer type) is ptrdiff_t defined in the  header. If the result
is not representable in an object of that type, the behavior is
undefined."

In our codes, there are cases that the pointer subtraction is not
performed by pointers to elements of the same array object. This might
lead to potential issues, since the behavior is undefined according to C11
standard.

Also, since the size of type "ptrdiff_t" is implementation-defined. Some
static code checkers may warn that the pointer subtraction might underflow
first and then being cast to a bigger size. For example:

UINT8  *Ptr1, *Ptr2;
UINTN  PtrDiff;
...
PtrDiff = (UINTN) (Ptr1 - Ptr2);

The commit will refine the pointer subtraction expressions by casting each
pointer to UINTN first and then perform the subtraction:

PtrDiff = (UINTN) Ptr1 - (UINTN) Ptr2;

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Hao Wu 
---
 MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c  |  4 ++--
 MdeModulePkg/Include/Library/NetLib.h |  6 +++---
 MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleLib.c |  4 ++--
 MdeModulePkg/Library/DxePrintLibPrint2Protocol/PrintLib.c |  2 +-
 MdeModulePkg/Library/FileExplorerLib/FileExplorer.c   |  6 +++---
 MdeModulePkg/Library/PiDxeS3BootScriptLib/BootScriptSave.c|  4 ++--
 MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c|  4 ++--
 MdeModulePkg/Universal/DebugPortDxe/DebugPort.c   |  4 ++--
 MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWrite.c |  4 ++--
 MdeModulePkg/Universal/HiiDatabaseDxe/Image.c |  4 ++--
 MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c | 10 
+-
 11 files changed, 26 insertions(+), 26 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c 
b/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
index 2bc4f8c..d2ad94e 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
@@ -1,7 +1,7 @@
 /** @file
   PCI Rom supporting funtions implementation for PCI Bus module.
 
-Copyright (c) 2006 - 2015, Intel Corporation. All rights reserved.
+Copyright (c) 2006 - 2017, 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.  The full text of the license may be 
found at
@@ -776,7 +776,7 @@ ProcessOpRomImage (
 NextImage:
 RomBarOffset += ImageSize;
 
-  } while (((Indicator & 0x80) == 0x00) && ((UINTN) (RomBarOffset - (UINT8 *) 
RomBar) < PciDevice->RomSize));
+  } while (((Indicator & 0x80) == 0x00) && (((UINTN) RomBarOffset - (UINTN) 
RomBar) < PciDevice->RomSize));
 
   return RetStatus;
 }
diff --git a/MdeModulePkg/Include/Library/NetLib.h 
b/MdeModulePkg/Include/Library/NetLib.h
index 09ead09..3b8ff1a 100644
--- a/MdeModulePkg/Include/Library/NetLib.h
+++ b/MdeModulePkg/Include/Library/NetLib.h
@@ -2,7 +2,7 @@
   This library is only intended to be used by UEFI network stack modules.
   It provides basic functions for the UEFI network stack.
 
-Copyright (c) 2005 - 2016, Intel Corporation. All rights reserved.
+Copyright (c) 2005 - 2017, 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.  The full text of the license may be 
found at
@@ -1610,10 +1610,10 @@ typedef struct {
   (sizeof (NET_BUF) + ((BlockOpNum) - 1) * sizeof (NET_BLOCK_OP))
 
 #define NET_HEADSPACE(BlockOp)  \
-  (UINTN)((BlockOp)->Head - (BlockOp)->BlockHead)
+  ((UINTN)((BlockOp)->Head) - (UINTN)((BlockOp)->BlockHead))
 
 #define NET_TAILSPACE(BlockOp)  \
-  (UINTN)((BlockOp)->BlockTail - (BlockOp)->Tail)
+  ((UINTN)((BlockOp)->BlockTail) - (UINTN)((BlockOp)->Tail))
 
 /**
   Allocate a single block NET_BUF. Upon allocation, all the
diff --git a/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleLib.c 
b/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleLib.c
index 71e05bd..d7abcc8 100644
--- a/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleLib.c
+++ b/MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleLib.c
@@ -10,7 +10,7 @@
   ValidateFmpCapsule(), DisplayCapsuleImage(), ConvertBmpToGopBlt() will
   receive untrusted input and do basic validation.
 
-  Copyright (c) 2016, Intel Corporation. All rights reserved.
+  Copyright (c) 2016 - 2017, 

[edk2] [PATCH v2 0/6] Refine type cast for pointer subtraction

[Hao Wu]

Please note that this patch is maily for feedback collection and only the
MdeModulePkg part of the series is sent out firstly.


V2 refines the commit messages:
For pointer subtraction, the result is of type "ptrdiff_t". According to
the C11 standard (Committee Draft - April 12, 2011):

"When two pointers are subtracted, both shall point to elements of the
same array object, or one past the last element of the array object; the
result is the difference of the subscripts of the two array elements. The
size of the result is implementation-defined, and its type (a signed
integer type) is ptrdiff_t defined in the  header. If the result
is not representable in an object of that type, the behavior is
undefined."

In our codes, there are cases that the pointer subtraction is not
performed by pointers to elements of the same array object. This might
lead to potential issues, since the behavior is undefined according to C11
standard.

Also, since the size of type "ptrdiff_t" is implementation-defined. Some
static code checkers may warn that the pointer subtraction might underflow
first and then being cast to a bigger size. For example:

UINT8  *Ptr1, *Ptr2;
UINTN  PtrDiff;
...
PtrDiff = (UINTN) (Ptr1 - Ptr2);

The commit will refine the pointer subtraction expressions by casting each
pointer to UINTN first and then perform the subtraction:

PtrDiff = (UINTN) Ptr1 - (UINTN) Ptr2;


V1:
For pointer subtraction, the result is of type "ptrdiff_t". According to
the C11 spec, ptrdiff_t is a signed integer type but its size is
implementation-defined.

There are cases that the result of pointer subtraction is type casted to
UINTN, like:

UINT8  *Ptr1, *Ptr2;
UINTN  PtrDiff;
...
PtrDiff = (UINTN) (Ptr1 - Ptr2);

Some static code checkers may warn that the pointer subtraction might
overflow first and then the result is being cast to a bigger size.

The series will cast each pointer to UINTN first and then perform the
subtraction:

PtrDiff = (UINTN) Ptr1 - (UINTN) Ptr2;


Cc: Laszlo Ersek 

Hao Wu (6):
  MdeModulePkg: Refine type cast for pointer subtraction
  CryptoPkg: Refine type cast for pointer subtraction
  IntelFrameworkModulePkg: Refine type cast for pointer subtraction
  NetworkPkg: Refine type cast for pointer subtraction
  SecurityPkg: Refine type cast for pointer subtraction
  ShellPkg: Refine type cast for pointer subtraction

 CryptoPkg/Library/BaseCryptLib/SysCall/RuntimeMemAllocation.c| 
 6 +++---
 IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyPci.c| 
 4 ++--
 IntelFrameworkModulePkg/Library/GenericBdsLib/BdsBoot.c  | 
 4 ++--
 IntelFrameworkModulePkg/Library/GenericBdsLib/BdsConsole.c   | 
 4 ++--
 IntelFrameworkModulePkg/Library/GenericBdsLib/BdsMisc.c  | 
 4 ++--
 IntelFrameworkModulePkg/Library/LegacyBootManagerLib/LegacyBm.c  | 
 4 ++--
 IntelFrameworkModulePkg/Universal/BdsDxe/BootMaint/BootOption.c  | 
 6 +++---
 MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c | 
 4 ++--
 MdeModulePkg/Include/Library/NetLib.h| 
 6 +++---
 MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleLib.c| 
 4 ++--
 MdeModulePkg/Library/DxePrintLibPrint2Protocol/PrintLib.c| 
 2 +-
 MdeModulePkg/Library/FileExplorerLib/FileExplorer.c  | 
 6 +++---
 MdeModulePkg/Library/PiDxeS3BootScriptLib/BootScriptSave.c   | 
 4 ++--
 MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c   | 
 4 ++--
 MdeModulePkg/Universal/DebugPortDxe/DebugPort.c  | 
 4 ++--
 MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWrite.c| 
 4 ++--
 MdeModulePkg/Universal/HiiDatabaseDxe/Image.c| 
 4 ++--
 MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c| 
10 +-
 NetworkPkg/HttpDxe/HttpImpl.c| 
 6 +++---
 SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c| 
18 +-
 SecurityPkg/Library/DxeTpmMeasureBootLib/DxeTpmMeasureBootLib.c  | 
10 +-
 SecurityPkg/Library/Tpm2CommandLib/Tpm2Help.c| 
 6 +++---
 SecurityPkg/Tcg/Tcg2Dxe/MeasureBootPeCoff.c  | 
10 +-
 SecurityPkg/Tcg/TrEEDxe/MeasureBootPeCoff.c  | 
10 +-
 SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c | 
14 +++---
 ShellPkg/Application/Shell/ShellParametersProtocol.c | 
 6 +++---
 ShellPkg/Library/UefiShellDebug1CommandsLib/DmpStore.c   | 
 4 ++--
 ShellPkg/Library/UefiShellDebug1CommandsLib/SmbiosView/SmbiosView.c  | 
 4 ++--
 ShellPkg/Library/UefiShellDriver1CommandsLib/Dh.c

Re: [edk2] [PATCH v1 1/1] BaseTools: AutoGen: Expand macros in INF files

[Gao, Liming]

Hi, 
  This usage will break UDP (UEFI Platform Initialization Distribution 
Packaging Specification).  It can be downloaded from 
http://www.uefi.org/specifications. This spec defines the packaging format. It 
defines SourceFiles.Filename as the path (relative to the Module "root" 
directory) and filename of the file (as specified in the Distribution Content 
File.) That means source files must be in the module directory. 
  And, EDK2 BaseTools UPT tool follows UDP to create Package.dist based on edk2 
source Package, and install Package.dist to edk2 source package. If source 
package has no change, UPT should always create the same package.dist file. 
But, this feature will break it. So, I don't suggest to add this support. 

Thanks
Liming
>-Original Message-
>From: kyletp...@gmail.com [mailto:kyletp...@gmail.com]
>Sent: Wednesday, January 25, 2017 3:16 AM
>To: edk2-devel@lists.01.org
>Cc: Laszlo Ersek ; Zhu, Yonghong
>; Gao, Liming 
>Subject: [PATCH v1 1/1] BaseTools: AutoGen: Expand macros in INF files
>
>From: apianti 
>
>Expand environment macros used for paths and names in INF files
>
>Cc: Laszlo Ersek 
>Cc: Yonghong Zhu 
>Cc: Liming Gao 
>Contributed-under: TianoCore Contribution Agreement 1.0
>Signed-off-by: apianti 
>---
> BaseTools/Source/Python/AutoGen/GenC.py| 4 ++--
> BaseTools/Source/Python/AutoGen/GenMake.py | 2 +-
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
>diff --git a/BaseTools/Source/Python/AutoGen/GenC.py
>b/BaseTools/Source/Python/AutoGen/GenC.py
>index 63cfe0422bbc..866e906f2f89 100644
>--- a/BaseTools/Source/Python/AutoGen/GenC.py
>+++ b/BaseTools/Source/Python/AutoGen/GenC.py
>@@ -21,7 +21,7 @@ from Common import EdkLogger
> from Common.BuildToolError import *
> from Common.DataType import *
> from Common.Misc import *
>-from Common.String import StringToArray
>+from Common.String import StringToArray, ReplaceMacro
> from StrGather import *
> from GenPcdDb import CreatePcdDatabaseCode
> from IdfClassObject import *
>@@ -1911,7 +1911,7 @@ def CreateHeaderCode(Info, AutoGenC, AutoGenH):
> # Publish the CallerId Guid
> #
> AutoGenC.Append('\nGLOBAL_REMOVE_IF_UNREFERENCED GUID
>gEfiCallerIdGuid = %s;\n' % GuidStringToGuidStructureString(Info.Guid))
>-AutoGenC.Append('\nGLOBAL_REMOVE_IF_UNREFERENCED CHAR8
>*gEfiCallerBaseName = "%s";\n' % Info.Name)
>+AutoGenC.Append('\nGLOBAL_REMOVE_IF_UNREFERENCED CHAR8
>*gEfiCallerBaseName = "%s";\n' % ReplaceMacro(Info.Name, os.environ))
>
> ## Create common code for header file
> #
>diff --git a/BaseTools/Source/Python/AutoGen/GenMake.py
>b/BaseTools/Source/Python/AutoGen/GenMake.py
>index 51c5238fd17e..7ec59fca5271 100644
>--- a/BaseTools/Source/Python/AutoGen/GenMake.py
>+++ b/BaseTools/Source/Python/AutoGen/GenMake.py
>@@ -587,7 +587,7 @@ cleanlib:
> FileMacroList.append("%s = %s" % (ListFileMacro, ListFileName))
> SaveFileOnChange(
> ListFileName,
>-"\n".join(self.ListFileMacros[ListFileMacro]),
>+"\n".join(ReplaceMacros(self.ListFileMacros[ListFileMacro],
>os.environ)),
> False
> )
>
>--
>2.11.0.windows.1

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

Re: [edk2] SetUnicodeMem in DisplayEngineDxe seems unsafe

[Dong, Eric]

Xiaofeng,

Thanks, we will follow up to fix it.

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of wang 
> xiaofeng
> Sent: Wednesday, January 25, 2017 10:49 AM
> To: Dong, Eric
> Cc: edk2-devel@lists.01.org; Gao, Liming
> Subject: Re: [edk2] SetUnicodeMem in DisplayEngineDxe seems unsafe
> 
> Hi Eric and Liming,
>Bug 358 is submitted for this issue.
> 
> 
> 
> 
> 
> 
> 
> 
> At 2017-01-25 10:29:52, "Dong, Eric"  wrote:
> >Xiaofeng,
> >
> >BugZillar link is: https://bugzilla.tianocore.org/
> >
> >Thanks,
> >Eric
> >> -Original Message-
> >> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of 
> >> wang xiaofeng
> >> Sent: Wednesday, January 25, 2017 10:22 AM
> >> To: Gao, Liming
> >> Cc: edk2-devel@lists.01.org; Dong, Eric
> >> Subject: Re: [edk2] SetUnicodeMem in DisplayEngineDxe seems unsafe
> >>
> >> Hi Liming,
> >> Where is the BugZillar link? I will try if I can submit it. But not 
> >> sure where I can quickly apply for an account.
> >>
> >>
> >>
> >>
> >>
> >>
> >>
> >>
> >> At 2017-01-25 08:54:47, "Gao, Liming"  wrote:
> >> >Xiaofeng:
> >> >   Yes. This is a potential issue. This API should be updated with 
> >> > original Buffer Size. Could you help submit this issue in BugZillar?
> >> >
> >> >Thanks
> >> >Liming
> >> >>-Original Message-
> >> >>From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> >> >>wang xiaofeng
> >> >>Sent: Tuesday, January 24, 2017 3:56 PM
> >> >>To: edk2-devel@lists.01.org; Dong, Eric 
> >> >>Subject: [edk2] SetUnicodeMem in DisplayEngineDxe seems unsafe
> >> >>
> >> >>Hi DisplayEngineDxe  Owner,
> >> >>   SetUnicodeMem seems unsafe  since the buffer may overflow if the input
> >> >>Size is bigger than buffer size.Do we think about improve the function
> >> >>
> >> >>
> >> >>/**
> >> >>  Set Buffer to Value for Size bytes.
> >> >>
> >> >>
> >> >>  @param  Buffer Memory to set.
> >> >>  @param  Size   Number of bytes to set
> >> >>  @param  Value  Value of the set operation.
> >> >>
> >> >>
> >> >>**/
> >> >>VOID
> >> >>SetUnicodeMem (
> >> >>  IN VOID   *Buffer,
> >> >>  IN UINTN  Size,
> >> >>  IN CHAR16 Value
> >> >>  )
> >> >>{
> >> >>  CHAR16  *Ptr;
> >> >>
> >> >>
> >> >>  Ptr = Buffer;
> >> >>  while ((Size--)  != 0) {
> >> >>*(Ptr++) = Value;
> >> >>  }
> >> >>}
> >> >>
> >> >>   The problem I meet is liking the following screen. Year in main page 
> >> >> shows
> >> >>incorrect char randomly.
> >> >>
> >> >>   If I turn off GetNumericInput optimize with #pragma optimize( "", off 
> >> >> ) in
> >> >>InputHandler.c , or swtich to use StrCpyS like this. The problem 
> >> >>disappear. This
> >> >>issue cannot be seen in OVMF ,but it can be reproduced in our own 
> >> >>platform
> >> >>with a rate of 30%.
> >> >>
> >> >>
> >> >>
> >> >>___
> >> >>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-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-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] SetUnicodeMem in DisplayEngineDxe seems unsafe

[wang xiaofeng]

Hi Eric and Liming,
   Bug 358 is submitted for this issue. 








At 2017-01-25 10:29:52, "Dong, Eric"  wrote:
>Xiaofeng,
>
>BugZillar link is: https://bugzilla.tianocore.org/
>
>Thanks,
>Eric
>> -Original Message-
>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of wang 
>> xiaofeng
>> Sent: Wednesday, January 25, 2017 10:22 AM
>> To: Gao, Liming
>> Cc: edk2-devel@lists.01.org; Dong, Eric
>> Subject: Re: [edk2] SetUnicodeMem in DisplayEngineDxe seems unsafe
>> 
>> Hi Liming,
>> Where is the BugZillar link? I will try if I can submit it. But not sure 
>> where I can quickly apply for an account.
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> At 2017-01-25 08:54:47, "Gao, Liming"  wrote:
>> >Xiaofeng:
>> >   Yes. This is a potential issue. This API should be updated with original 
>> > Buffer Size. Could you help submit this issue in BugZillar?
>> >
>> >Thanks
>> >Liming
>> >>-Original Message-
>> >>From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>> >>wang xiaofeng
>> >>Sent: Tuesday, January 24, 2017 3:56 PM
>> >>To: edk2-devel@lists.01.org; Dong, Eric 
>> >>Subject: [edk2] SetUnicodeMem in DisplayEngineDxe seems unsafe
>> >>
>> >>Hi DisplayEngineDxe  Owner,
>> >>   SetUnicodeMem seems unsafe  since the buffer may overflow if the input
>> >>Size is bigger than buffer size.Do we think about improve the function
>> >>
>> >>
>> >>/**
>> >>  Set Buffer to Value for Size bytes.
>> >>
>> >>
>> >>  @param  Buffer Memory to set.
>> >>  @param  Size   Number of bytes to set
>> >>  @param  Value  Value of the set operation.
>> >>
>> >>
>> >>**/
>> >>VOID
>> >>SetUnicodeMem (
>> >>  IN VOID   *Buffer,
>> >>  IN UINTN  Size,
>> >>  IN CHAR16 Value
>> >>  )
>> >>{
>> >>  CHAR16  *Ptr;
>> >>
>> >>
>> >>  Ptr = Buffer;
>> >>  while ((Size--)  != 0) {
>> >>*(Ptr++) = Value;
>> >>  }
>> >>}
>> >>
>> >>   The problem I meet is liking the following screen. Year in main page 
>> >> shows
>> >>incorrect char randomly.
>> >>
>> >>   If I turn off GetNumericInput optimize with #pragma optimize( "", off ) 
>> >> in
>> >>InputHandler.c , or swtich to use StrCpyS like this. The problem 
>> >>disappear. This
>> >>issue cannot be seen in OVMF ,but it can be reproduced in our own platform
>> >>with a rate of 30%.
>> >>
>> >>
>> >>
>> >>___
>> >>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-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-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] SetUnicodeMem in DisplayEngineDxe seems unsafe

[Dong, Eric]

Xiaofeng,

BugZillar link is: https://bugzilla.tianocore.org/

Thanks,
Eric
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of wang 
> xiaofeng
> Sent: Wednesday, January 25, 2017 10:22 AM
> To: Gao, Liming
> Cc: edk2-devel@lists.01.org; Dong, Eric
> Subject: Re: [edk2] SetUnicodeMem in DisplayEngineDxe seems unsafe
> 
> Hi Liming,
> Where is the BugZillar link? I will try if I can submit it. But not sure 
> where I can quickly apply for an account.
> 
> 
> 
> 
> 
> 
> 
> 
> At 2017-01-25 08:54:47, "Gao, Liming"  wrote:
> >Xiaofeng:
> >   Yes. This is a potential issue. This API should be updated with original 
> > Buffer Size. Could you help submit this issue in BugZillar?
> >
> >Thanks
> >Liming
> >>-Original Message-
> >>From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> >>wang xiaofeng
> >>Sent: Tuesday, January 24, 2017 3:56 PM
> >>To: edk2-devel@lists.01.org; Dong, Eric 
> >>Subject: [edk2] SetUnicodeMem in DisplayEngineDxe seems unsafe
> >>
> >>Hi DisplayEngineDxe  Owner,
> >>   SetUnicodeMem seems unsafe  since the buffer may overflow if the input
> >>Size is bigger than buffer size.Do we think about improve the function
> >>
> >>
> >>/**
> >>  Set Buffer to Value for Size bytes.
> >>
> >>
> >>  @param  Buffer Memory to set.
> >>  @param  Size   Number of bytes to set
> >>  @param  Value  Value of the set operation.
> >>
> >>
> >>**/
> >>VOID
> >>SetUnicodeMem (
> >>  IN VOID   *Buffer,
> >>  IN UINTN  Size,
> >>  IN CHAR16 Value
> >>  )
> >>{
> >>  CHAR16  *Ptr;
> >>
> >>
> >>  Ptr = Buffer;
> >>  while ((Size--)  != 0) {
> >>*(Ptr++) = Value;
> >>  }
> >>}
> >>
> >>   The problem I meet is liking the following screen. Year in main page 
> >> shows
> >>incorrect char randomly.
> >>
> >>   If I turn off GetNumericInput optimize with #pragma optimize( "", off ) 
> >> in
> >>InputHandler.c , or swtich to use StrCpyS like this. The problem disappear. 
> >>This
> >>issue cannot be seen in OVMF ,but it can be reproduced in our own platform
> >>with a rate of 30%.
> >>
> >>
> >>
> >>___
> >>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-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 1/6] MdeModulePkg: Refine type cast for pointer subtraction

[Wu, Hao A]

> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Tuesday, January 24, 2017 6:15 PM
> To: Wu, Hao A
> Cc: edk2-de...@ml01.01.org
> Subject: Re: [edk2] [PATCH 1/6] MdeModulePkg: Refine type cast for pointer
> subtraction
> 
> On 01/24/17 08:50, Hao Wu wrote:
> > For pointer subtraction, the result is of type "ptrdiff_t". According to
> > the C11 spec, ptrdiff_t is a signed integer type but its size is
> > implementation-defined.
> >
> > There are cases that the result of pointer subtraction is type casted to
> > UINTN, like:
> >
> > UINT8  *Ptr1, *Ptr2;
> > UINTN  PtrDiff;
> > ...
> > PtrDiff = (UINTN) (Ptr1 - Ptr2);
> >
> > Some static code checkers may warn that the pointer subtraction might
> > overflow first and then the result is being cast to a bigger size.
> >
> > The commit will cast each pointer to UINTN first and then perform the
> > subtraction:
> >
> > PtrDiff = (UINTN) Ptr1 - (UINTN) Ptr2;
> 
> The main point here is that pointer subtraction is only allowed between
> pointers to elements of the same (single-dimensional) array. For the
> purposes of this requirement, the subtrahend and/or the minuend can
> point one past the last element in the array (but such a pointer cannot
> be dereferenced). Also, a non-array object is treated identically to an
> array with 1 element.
> 
> In the above permitted (defined) cases, no overflow will ever occur.
> That leaves us with the following concerns:
> 
> - The well-defined difference (of type ptrdiff_t) might be negative;
> casting that to UINTN probably doesn't have the intended result. This
> can be excluded by verifying (manually) that the minuend pointer always
> points past the subtrahend pointer.
> 
> - If the difference is not defined (because the above requirement is not
> satisfied), then the pointers should indeed be converted to some integer
> type before the subtraction. This raises further questions:
> 
>   - if the integer type we choose is signed, then the subtraction
> (between the signed integers) could underflow, which would be
> undefined.
> 
>   - if the integer type we choose is unsigned (and has at least the
> rank of int), then the subtraction could underflow with
> well-defined behavior, but the result would still be non-intuitive.
> 
> Considering our implementation details, casting both operands to UINTN
> and then doing the subtraction seems fine, as long as we can ensure that
> the subtraction doesn't wrap around (in order to prevent non-intuitive
> results).
> 
> ... I wrote up this wall of text only to suggest a more precise commit
> message. If you have access to the C11 std (or a late enough draft of
> it), then it's best to quote the standard directly. My point is that in
> the following example,
> 
>   UINT8 Blah[2][2];
>   UINTN Diff;
> 
>   Diff = [1][1] - [0][0];
> 
> there is no underflow of the specific kind that the commit message
> suggests, but the subtraction is still undefined, because the operands
> don't point into (or one past) the same array. The minuend points into
> the Blah[1] array, while the subtrahend points into the Blah[0] array.
> 

Yes, the above example is an undefined case for pointer subtraction that
the commit message missed mentioning. I will add more details in the
commit log about the pointer subtraction related content from the C11
spec.

> Not volunteering to review this patch though :)
> 

Many thanks for the feedbacks. Really appreciate the effort.

Best Regards,
Hao Wu

> Thanks
> Laszlo
> 
> 
> >
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Hao Wu 
> > ---
> >  MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c   |  4 ++--
> >  MdeModulePkg/Include/Library/NetLib.h  |  6 +++---
> >  MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleLib.c  |  4 ++--
> >  MdeModulePkg/Library/DxePrintLibPrint2Protocol/PrintLib.c  |  2 +-
> >  MdeModulePkg/Library/FileExplorerLib/FileExplorer.c|  6 +++---
> >  MdeModulePkg/Library/PiDxeS3BootScriptLib/BootScriptSave.c |  4 ++--
> >  MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c |  4 ++--
> >  MdeModulePkg/Universal/DebugPortDxe/DebugPort.c|  4 ++--
> >  .../Universal/FaultTolerantWriteDxe/FaultTolerantWrite.c   |  4 ++--
> >  MdeModulePkg/Universal/HiiDatabaseDxe/Image.c  |  4 ++--
> >  MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c  | 10
> +-
> >  11 files changed, 26 insertions(+), 26 deletions(-)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
> > index 2bc4f8c..d2ad94e 100644
> > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
> > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
> > @@ -1,7 +1,7 @@
> >  /** @file
> >PCI Rom supporting funtions implementation for PCI Bus module.
> >
> > -Copyright (c) 2006 - 

Re: [edk2] SetUnicodeMem in DisplayEngineDxe seems unsafe

[Gao, Liming]

https://bugzilla.tianocore.org/

Thanks
Liming
>-Original Message-
>From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>wang xiaofeng
>Sent: Wednesday, January 25, 2017 10:22 AM
>To: Gao, Liming 
>Cc: edk2-devel@lists.01.org; Dong, Eric 
>Subject: Re: [edk2] SetUnicodeMem in DisplayEngineDxe seems unsafe
>
>Hi Liming,
>Where is the BugZillar link? I will try if I can submit it. But not sure 
> where I
>can quickly apply for an account.
>
>
>
>
>
>
>
>
>At 2017-01-25 08:54:47, "Gao, Liming"  wrote:
>>Xiaofeng:
>>   Yes. This is a potential issue. This API should be updated with original 
>> Buffer
>Size. Could you help submit this issue in BugZillar?
>>
>>Thanks
>>Liming
>>>-Original Message-
>>>From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>>>wang xiaofeng
>>>Sent: Tuesday, January 24, 2017 3:56 PM
>>>To: edk2-devel@lists.01.org; Dong, Eric 
>>>Subject: [edk2] SetUnicodeMem in DisplayEngineDxe seems unsafe
>>>
>>>Hi DisplayEngineDxe  Owner,
>>>   SetUnicodeMem seems unsafe  since the buffer may overflow if the
>input
>>>Size is bigger than buffer size.Do we think about improve the function
>>>
>>>
>>>/**
>>>  Set Buffer to Value for Size bytes.
>>>
>>>
>>>  @param  Buffer Memory to set.
>>>  @param  Size   Number of bytes to set
>>>  @param  Value  Value of the set operation.
>>>
>>>
>>>**/
>>>VOID
>>>SetUnicodeMem (
>>>  IN VOID   *Buffer,
>>>  IN UINTN  Size,
>>>  IN CHAR16 Value
>>>  )
>>>{
>>>  CHAR16  *Ptr;
>>>
>>>
>>>  Ptr = Buffer;
>>>  while ((Size--)  != 0) {
>>>*(Ptr++) = Value;
>>>  }
>>>}
>>>
>>>   The problem I meet is liking the following screen. Year in main page shows
>>>incorrect char randomly.
>>>
>>>   If I turn off GetNumericInput optimize with #pragma optimize( "", off ) in
>>>InputHandler.c , or swtich to use StrCpyS like this. The problem disappear.
>This
>>>issue cannot be seen in OVMF ,but it can be reproduced in our own
>platform
>>>with a rate of 30%.
>>>
>>>
>>>
>>>___
>>>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-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] SetUnicodeMem in DisplayEngineDxe seems unsafe

[wang xiaofeng]

Hi Liming,
Where is the BugZillar link? I will try if I can submit it. But not sure 
where I can quickly apply for an account.








At 2017-01-25 08:54:47, "Gao, Liming"  wrote:
>Xiaofeng:
>   Yes. This is a potential issue. This API should be updated with original 
> Buffer Size. Could you help submit this issue in BugZillar? 
>
>Thanks
>Liming
>>-Original Message-
>>From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>>wang xiaofeng
>>Sent: Tuesday, January 24, 2017 3:56 PM
>>To: edk2-devel@lists.01.org; Dong, Eric 
>>Subject: [edk2] SetUnicodeMem in DisplayEngineDxe seems unsafe
>>
>>Hi DisplayEngineDxe  Owner,
>>   SetUnicodeMem seems unsafe  since the buffer may overflow if the input
>>Size is bigger than buffer size.Do we think about improve the function
>>
>>
>>/**
>>  Set Buffer to Value for Size bytes.
>>
>>
>>  @param  Buffer Memory to set.
>>  @param  Size   Number of bytes to set
>>  @param  Value  Value of the set operation.
>>
>>
>>**/
>>VOID
>>SetUnicodeMem (
>>  IN VOID   *Buffer,
>>  IN UINTN  Size,
>>  IN CHAR16 Value
>>  )
>>{
>>  CHAR16  *Ptr;
>>
>>
>>  Ptr = Buffer;
>>  while ((Size--)  != 0) {
>>*(Ptr++) = Value;
>>  }
>>}
>>
>>   The problem I meet is liking the following screen. Year in main page shows
>>incorrect char randomly.
>>
>>   If I turn off GetNumericInput optimize with #pragma optimize( "", off ) in
>>InputHandler.c , or swtich to use StrCpyS like this. The problem disappear. 
>>This
>>issue cannot be seen in OVMF ,but it can be reproduced in our own platform
>>with a rate of 30%.
>>
>>
>>
>>___
>>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-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 2/3] MdePkg: VariableDxe: Use UEFI_VARIABLE_DATA

[Yao, Jiewen]

Thank you.

I also think this is a right way to go.

One observation I have is that we need use 
sizeof(TREE_BOOT_SERVICE_CAPABILITY_1_0) in Tcg2Dxe.
I think we can use OFFSET_OF(EFI_TCG2_BOOT_SERVICE_CAPABILITY , 
NumberOfPCRBanks) to remove TrEE reference in Tcg2.

Thank you
Yao Jiewen


> -Original Message-
> From: Zhang, Chao B
> Sent: Wednesday, January 25, 2017 10:13 AM
> To: Zeng, Star ; edk2-devel@lists.01.org
> Cc: Yao, Jiewen 
> Subject: RE: [edk2] [PATCH 2/3] MdePkg: VariableDxe: Use UEFI_VARIABLE_DATA
> 
> Star:
>I agree. I will clean up other TrEEProtocol reference later on.
> 
> -Original Message-
> From: Zeng, Star
> Sent: Tuesday, January 24, 2017 4:33 PM
> To: Zhang, Chao B ; edk2-devel@lists.01.org
> Cc: Yao, Jiewen ; Zeng, Star 
> Subject: RE: [edk2] [PATCH 2/3] MdePkg: VariableDxe: Use UEFI_VARIABLE_DATA
> 
> Could we remove " #include " in Measurement.c?
> 
> Thanks,
> Star
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Zhang,
> Chao B
> Sent: Tuesday, January 24, 2017 3:58 PM
> To: edk2-devel@lists.01.org
> Cc: Yao, Jiewen ; Zhang, Chao B
> ; Zeng, Star 
> Subject: [edk2] [PATCH 2/3] MdePkg: VariableDxe: Use UEFI_VARIABLE_DATA
> 
> Use UEFI_VARIABLE_DATA data structure according to TCG PC-Client PFP Spec
> 00.21.
> http://www.trustedcomputinggroup.org/wp-content/uploads/PC-ClientSpecific
> _Platform_Profile_for_TPM_2p0_Systems_v21.pdf
> 
> Cc: Star Zeng 
> Cc: Yao Jiewen 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Chao Zhang 
> ---
>  MdeModulePkg/Universal/Variable/RuntimeDxe/Measurement.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Measurement.c
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/Measurement.c
> index 309521f..b9febac 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Measurement.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Measurement.c
> @@ -96,7 +96,7 @@ MeasureVariable (
>  {
>EFI_STATUSStatus;
>UINTN VarNameLength;
> -  EFI_VARIABLE_DATA_TREE*VarLog;
> +  UEFI_VARIABLE_DATA*VarLog;
>UINT32VarLogSize;
> 
>ASSERT ((VarSize == 0 && VarData == NULL) || (VarSize != 0 && VarData !=
> NULL)); @@ -105,7 +105,7 @@ MeasureVariable (
>VarLogSize = (UINT32)(sizeof (*VarLog) + VarNameLength * sizeof (*VarName)
> + VarSize
>  - sizeof (VarLog->UnicodeName) - sizeof
> (VarLog->VariableData));
> 
> -  VarLog = (EFI_VARIABLE_DATA_TREE *) AllocateZeroPool (VarLogSize);
> +  VarLog = (UEFI_VARIABLE_DATA *) AllocateZeroPool (VarLogSize);
>if (VarLog == NULL) {
>  return EFI_OUT_OF_RESOURCES;
>}
> --
> 1.9.5.msysgit.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 2/3] MdePkg: VariableDxe: Use UEFI_VARIABLE_DATA

[Zhang, Chao B]

Star: 
   I agree. I will clean up other TrEEProtocol reference later on.

-Original Message-
From: Zeng, Star 
Sent: Tuesday, January 24, 2017 4:33 PM
To: Zhang, Chao B ; edk2-devel@lists.01.org
Cc: Yao, Jiewen ; Zeng, Star 
Subject: RE: [edk2] [PATCH 2/3] MdePkg: VariableDxe: Use UEFI_VARIABLE_DATA

Could we remove " #include " in Measurement.c?

Thanks,
Star
-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Zhang, 
Chao B
Sent: Tuesday, January 24, 2017 3:58 PM
To: edk2-devel@lists.01.org
Cc: Yao, Jiewen ; Zhang, Chao B ; 
Zeng, Star 
Subject: [edk2] [PATCH 2/3] MdePkg: VariableDxe: Use UEFI_VARIABLE_DATA

Use UEFI_VARIABLE_DATA data structure according to TCG PC-Client PFP Spec 00.21.
http://www.trustedcomputinggroup.org/wp-content/uploads/PC-ClientSpecific_Platform_Profile_for_TPM_2p0_Systems_v21.pdf

Cc: Star Zeng 
Cc: Yao Jiewen 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Chao Zhang 
---
 MdeModulePkg/Universal/Variable/RuntimeDxe/Measurement.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Measurement.c 
b/MdeModulePkg/Universal/Variable/RuntimeDxe/Measurement.c
index 309521f..b9febac 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Measurement.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Measurement.c
@@ -96,7 +96,7 @@ MeasureVariable (
 {
   EFI_STATUSStatus;
   UINTN VarNameLength;
-  EFI_VARIABLE_DATA_TREE*VarLog;
+  UEFI_VARIABLE_DATA*VarLog;
   UINT32VarLogSize;
 
   ASSERT ((VarSize == 0 && VarData == NULL) || (VarSize != 0 && VarData != 
NULL)); @@ -105,7 +105,7 @@ MeasureVariable (
   VarLogSize = (UINT32)(sizeof (*VarLog) + VarNameLength * sizeof (*VarName) + 
VarSize
 - sizeof (VarLog->UnicodeName) - sizeof 
(VarLog->VariableData));
 
-  VarLog = (EFI_VARIABLE_DATA_TREE *) AllocateZeroPool (VarLogSize);
+  VarLog = (UEFI_VARIABLE_DATA *) AllocateZeroPool (VarLogSize);
   if (VarLog == NULL) {
 return EFI_OUT_OF_RESOURCES;
   }
--
1.9.5.msysgit.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] SetUnicodeMem in DisplayEngineDxe seems unsafe

[Gao, Liming]

Xiaofeng:
   Yes. This is a potential issue. This API should be updated with original 
Buffer Size. Could you help submit this issue in BugZillar? 

Thanks
Liming
>-Original Message-
>From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>wang xiaofeng
>Sent: Tuesday, January 24, 2017 3:56 PM
>To: edk2-devel@lists.01.org; Dong, Eric 
>Subject: [edk2] SetUnicodeMem in DisplayEngineDxe seems unsafe
>
>Hi DisplayEngineDxe  Owner,
>   SetUnicodeMem seems unsafe  since the buffer may overflow if the input
>Size is bigger than buffer size.Do we think about improve the function
>
>
>/**
>  Set Buffer to Value for Size bytes.
>
>
>  @param  Buffer Memory to set.
>  @param  Size   Number of bytes to set
>  @param  Value  Value of the set operation.
>
>
>**/
>VOID
>SetUnicodeMem (
>  IN VOID   *Buffer,
>  IN UINTN  Size,
>  IN CHAR16 Value
>  )
>{
>  CHAR16  *Ptr;
>
>
>  Ptr = Buffer;
>  while ((Size--)  != 0) {
>*(Ptr++) = Value;
>  }
>}
>
>   The problem I meet is liking the following screen. Year in main page shows
>incorrect char randomly.
>
>   If I turn off GetNumericInput optimize with #pragma optimize( "", off ) in
>InputHandler.c , or swtich to use StrCpyS like this. The problem disappear. 
>This
>issue cannot be seen in OVMF ,but it can be reproduced in our own platform
>with a rate of 30%.
>
>
>
>___
>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 1/1] MdePkg: Refine casting expression result to bigger size

[Wu, Hao A]

> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Tuesday, January 24, 2017 5:54 PM
> To: Wu, Hao A
> Cc: edk2-de...@ml01.01.org
> Subject: Re: [edk2] [PATCH v2 1/1] MdePkg: Refine casting expression result to
> bigger size
> 
> On 01/24/17 08:25, Hao Wu wrote:
> > There are cases that the operands of an expression are all with rank less
> > than UINT64/INT64 and the result of the expression is explicitly casted to
> > UINT64/INT64 to fit the target size.
> >
> > An example will be:
> > UINT32 a,b;
> > // a and b can be any unsigned int type with rank less than UINT64, like
> > // UINT8, UINT16, etc.
> > UINT64 c;
> > c = (UINT64) (a + b);
> >
> > Some static code checkers may warn that the expression result might
> > overflow within the rank of "int" (integer promotions) and the result is
> > then cast to a bigger size.
> >
> > The commit refines codes by the following rules:
> > 1). When the expression will not overflow within the rank of "int", remove
> > the explicit type casts:
> > c = a + b;
> >
> > 2). When the expression is possible to overflow the range of unsigned int/
> > int:
> > c = (UINT64)a + b;
> >
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Hao Wu 
> > ---
> >  MdePkg/Library/BaseLib/String.c  |  4 ++--
> >  MdePkg/Library/BasePeCoffLib/BasePeCoff.c| 12 
> > +---
> >  MdePkg/Library/BaseS3PciLib/S3PciLib.c   |  4 ++--
> >  MdePkg/Library/SmmMemoryAllocationLib/MemoryAllocationLib.c  |  4 ++--
> >  MdePkg/Library/UefiMemoryAllocationLib/MemoryAllocationLib.c |  4 ++--
> >  5 files changed, 13 insertions(+), 15 deletions(-)
> >
> > diff --git a/MdePkg/Library/BaseLib/String.c
> b/MdePkg/Library/BaseLib/String.c
> > index e84bf50..4151e0e 100644
> > --- a/MdePkg/Library/BaseLib/String.c
> > +++ b/MdePkg/Library/BaseLib/String.c
> > @@ -586,7 +586,7 @@ InternalHexCharToUintn (
> >  return Char - L'0';
> >}
> >
> > -  return (UINTN) (10 + InternalCharToUpper (Char) - L'A');
> > +  return (10 + InternalCharToUpper (Char) - L'A');
> >  }
> >
> >  /**
> > @@ -1211,7 +1211,7 @@ InternalAsciiHexCharToUintn (
> >  return Char - '0';
> >}
> >
> > -  return (UINTN) (10 + InternalBaseLibAsciiToUpper (Char) - 'A');
> > +  return (10 + InternalBaseLibAsciiToUpper (Char) - 'A');
> >  }
> >
> >
> > diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> > index 33cad23..8d1daba 100644
> > --- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> > +++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> > @@ -15,7 +15,7 @@
> >PeCoffLoaderGetPeHeader() routine will do basic check for PE/COFF header.
> >PeCoffLoaderGetImageInfo() routine will do basic check for whole PE/COFF
> image.
> >
> > -  Copyright (c) 2006 - 2014, Intel Corporation. All rights reserved.
> > +  Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.
> >Portions copyright (c) 2008 - 2009, Apple Inc. All rights reserved.
> >This program and the accompanying materials
> >are licensed and made available under the terms and conditions of the BSD
> License
> > @@ -703,12 +703,10 @@ PeCoffLoaderGetImageInfo (
> >//
> >DebugDirectoryEntryFileOffset = 0;
> >
> > -  SectionHeaderOffset = (UINTN)(
> > -   ImageContext->PeCoffHeaderOffset +
> > -   sizeof (UINT32) +
> > -   sizeof (EFI_IMAGE_FILE_HEADER) +
> > -   Hdr.Pe32->FileHeader.SizeOfOptionalHeader
> > -   );
> > +  SectionHeaderOffset = ImageContext->PeCoffHeaderOffset +
> > +sizeof (UINT32) +
> > +sizeof (EFI_IMAGE_FILE_HEADER) +
> > +Hdr.Pe32->FileHeader.SizeOfOptionalHeader;
> >
> >for (Index = 0; Index < Hdr.Pe32->FileHeader.NumberOfSections; 
> > Index++)
> {
> >  //
> > diff --git a/MdePkg/Library/BaseS3PciLib/S3PciLib.c
> b/MdePkg/Library/BaseS3PciLib/S3PciLib.c
> > index e29f7fe..27342b0 100644
> > --- a/MdePkg/Library/BaseS3PciLib/S3PciLib.c
> > +++ b/MdePkg/Library/BaseS3PciLib/S3PciLib.c
> > @@ -3,7 +3,7 @@
> >the PCI operations to be replayed during an S3 resume. This library class
> >maps directly on top of the PciLib class.
> >
> > -  Copyright (c) 2006 - 2012, Intel Corporation. All rights reserved.
> > +  Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.
> >
> >This program and the accompanying materials
> >are licensed and made available under the terms and conditions
> > @@ -25,7 +25,7 @@
> >  #include 
> >
> >  #define PCILIB_TO_COMMON_ADDRESS(Address) \
> > -((UINT64) UINTN) ((Address>>20) & 0xff)) << 24) + (((UINTN)
> ((Address>>15) & 0x1f)) << 16) + (((UINTN) ((Address>>12) & 0x07)) << 8) +
> ((UINTN) (Address & 0xfff 
> 

Re: [edk2] [PATCH] ShellPkg/pci: Support interpreting specific PCIE ext cap thru "-ec"

[Carsey, Jaben]

Reviewed-by: Jaben Carsey 

> -Original Message-
> From: Ni, Ruiyu
> Sent: Monday, January 23, 2017 10:43 PM
> To: edk2-devel@lists.01.org
> Cc: Carsey, Jaben 
> Subject: [PATCH] ShellPkg/pci: Support interpreting specific PCIE ext cap thru
> "-ec"
> Importance: High
> 
> The implementation was already there but through a private flag
> "-_e". The patch removes "-_e" support and add "-ec" support.
> Removing old "-_e" support makes the pci command more clean.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ruiyu Ni 
> Cc: Jaben Carsey 
> ---
>  ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c  | 61 +---
> --
>  .../UefiShellDebug1CommandsLib.uni | 15 +++---
>  2 files changed, 34 insertions(+), 42 deletions(-)
> 
> diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c
> b/ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c
> index 9cc4f5c..99e6caf 100644
> --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c
> +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c
> @@ -2370,7 +2370,7 @@ PCI_CONFIG_SPACE  *mConfigSpace = NULL;
>  STATIC CONST SHELL_PARAM_ITEM ParamList[] = {
>{L"-s", TypeValue},
>{L"-i", TypeFlag},
> -  {L"-_e", TypeValue},
> +  {L"-ec", TypeValue},
>{NULL, TypeMax}
>};
> 
> @@ -2516,6 +2516,11 @@ ShellCommandRunPci (
>ShellStatus = SHELL_INVALID_PARAMETER;
>goto Done;
>  }
> +if (ShellCommandLineGetFlag(Package, L"-ec") &&
> ShellCommandLineGetValue(Package, L"-ec") == NULL) {
> +  ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN (STR_GEN_NO_VALUE),
> gShellDebug1HiiHandle,  L"pci", L"-ec");
> +  ShellStatus = SHELL_INVALID_PARAMETER;
> +  goto Done;
> +}
>  if (ShellCommandLineGetFlag(Package, L"-s") &&
> ShellCommandLineGetValue(Package, L"-s") == NULL) {
>ShellPrintHiiEx(-1, -1, NULL, STRING_TOKEN (STR_GEN_NO_VALUE),
> gShellDebug1HiiHandle,  L"pci", L"-s");
>ShellStatus = SHELL_INVALID_PARAMETER;
> @@ -2878,13 +2883,11 @@ ShellCommandRunPci (
>  // If "-i" appears in command line, interpret data in configuration space
>  //
>  if (ExplainData) {
> -  EnhancedDump = 0;
> -  if (ShellCommandLineGetFlag(Package, L"-_e")) {
> -EnhancedDump = 0x;
> -Temp = ShellCommandLineGetValue(Package, L"-_e");
> -if (Temp != NULL) {
> -  EnhancedDump = (UINT16) ShellHexStrToUintn (Temp);
> -}
> +  EnhancedDump = 0x;
> +  if (ShellCommandLineGetFlag(Package, L"-ec")) {
> +Temp = ShellCommandLineGetValue(Package, L"-ec");
> +ASSERT (Temp != NULL);
> +EnhancedDump = (UINT16) ShellHexStrToUintn (Temp);
>}
>Status = PciExplainData (, Address, IoDev, EnhancedDump);
>  }
> @@ -5829,39 +5832,25 @@ PciExplainPciExpress (
>  return EFI_UNSUPPORTED;
>}
> 
> -  if (EnhancedDump == 0) {
> +  ExtHdr = (PCI_EXP_EXT_HDR*)ExRegBuffer;
> +  while (ExtHdr->CapabilityId != 0 && ExtHdr->CapabilityVersion != 0) {
>  //
> -// Print the PciEx extend space in raw bytes ( 0xFF-0xFFF)
> +// Process this item
>  //
> -ShellPrintEx (-1, -1, L"\r\n%HStart dumping PCIex extended configuration
> space (0x100 - 0xFFF).%N\r\n\r\n");
> -
> -DumpHex (
> -  2,
> -  EFI_PCIE_CAPABILITY_BASE_OFFSET,
> -  ExtendRegSize,
> -  (VOID *) (ExRegBuffer)
> -  );
> -  } else {
> -ExtHdr = (PCI_EXP_EXT_HDR*)ExRegBuffer;
> -while (ExtHdr->CapabilityId != 0 && ExtHdr->CapabilityVersion != 0) {
> +if (EnhancedDump == 0x || EnhancedDump == ExtHdr->CapabilityId)
> {
>//
> -  // Process this item
> +  // Print this item
>//
> -  if (EnhancedDump == 0x || EnhancedDump == ExtHdr->CapabilityId)
> {
> -//
> -// Print this item
> -//
> -PrintPciExtendedCapabilityDetails((PCI_EXP_EXT_HDR*)ExRegBuffer,
> ExtHdr, );
> -  }
> +  PrintPciExtendedCapabilityDetails((PCI_EXP_EXT_HDR*)ExRegBuffer,
> ExtHdr, );
> +}
> 
> -  //
> -  // Advance to the next item if it exists
> -  //
> -  if (ExtHdr->NextCapabilityOffset != 0) {
> -ExtHdr = (PCI_EXP_EXT_HDR*)((UINT8*)ExRegBuffer + ExtHdr-
> >NextCapabilityOffset - EFI_PCIE_CAPABILITY_BASE_OFFSET);
> -  } else {
> -break;
> -  }
> +//
> +// Advance to the next item if it exists
> +//
> +if (ExtHdr->NextCapabilityOffset != 0) {
> +  ExtHdr = (PCI_EXP_EXT_HDR*)((UINT8*)ExRegBuffer + ExtHdr-
> >NextCapabilityOffset - EFI_PCIE_CAPABILITY_BASE_OFFSET);
> +} else {
> +  break;
>  }
>}
>SHELL_FREE_NON_NULL(ExRegBuffer);
> diff --git
> a/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1Comman
> dsLib.uni
> b/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1Comman
> dsLib.uni
> index 06865a4..324e233f 100644
> ---
> 

Re: [edk2] [PATCH] Fix edit on screens with more than 200 columns

[Carsey, Jaben]

I didn't mean a 400 static, I meant start by allocating 400 and simplify the 
end of the function...

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Witt, Sebastian
> Sent: Tuesday, January 24, 2017 8:48 AM
> To: Carsey, Jaben ; edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH] Fix edit on screens with more than 200 columns
> Importance: High
> 
> Only performance. But I haven't measured if there is a big difference
> between static buffer and AllocateZeroPool. I wouldn't use a fixed value.
> There may be a display device with more than 400 columns. Otherwise one
> can always allocate the [LastCol + 1] buffer.
> 
> -Original Message-
> From: Carsey, Jaben [mailto:jaben.car...@intel.com]
> Sent: Dienstag, 24. Januar 2017 17:27
> To: Witt, Sebastian (DF FA AS DH KHE 1); edk2-devel@lists.01.org
> Cc: Carsey, Jaben
> Subject: RE: [PATCH] Fix edit on screens with more than 200 columns
> 
> Is there a reason to not just always start with allocating the 400 and then we
> don't need to complicate the end to conditionally free the buffer?
> 
> > -Original Message-
> > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> > Witt, Sebastian
> > Sent: Tuesday, January 24, 2017 5:14 AM
> > To: edk2-devel@lists.01.org
> > Subject: [edk2] [PATCH] Fix edit on screens with more than 200 columns
> > Importance: High
> >
> > If the shell edit command is used on a screen with more than
> > 200 columns, we get a buffer overflow. This increases the default
> > buffer size to
> > 400 columns and allocates a pool when this is not enough.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Sebastian Witt 
> >
> > ---
> >  .../UefiShellDebug1CommandsLib.c  | 15 
> > ++-
> >  1 file changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git
> >
> a/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1Comman
> dsL
> > i
> > b.c
> >
> b/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1Comman
> dsL
> > i
> > b.c
> > index 6ebf002..d81dd01 100644
> > ---
> >
> a/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1Comman
> dsL
> > i
> > b.c
> > +++
> >
> b/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1Comman
> d
> > +++ sLib.c
> > @@ -302,12 +302,21 @@ EditorClearLine (
> >IN UINTN LastRow
> >)
> >  {
> > -  CHAR16 Line[200];
> > +  CHAR16 Buffer[400];
> > +  CHAR16 *Line = Buffer;
> >
> >if (Row == 0) {
> >  Row = 1;
> >}
> >
> > +  // If there are more columns than our buffer can contain, allocate
> > + new buffer  if (LastCol >= (sizeof (Buffer) / sizeof (CHAR16))) {
> > +Line = AllocateZeroPool (LastCol*(sizeof(CHAR16) + 1));
> > +if (Line == NULL) {
> > +  return;
> > +}
> > +  }
> > +
> >//
> >// prepare a blank line
> >//
> > @@ -326,6 +335,10 @@ EditorClearLine (
> >// print out the blank line
> >//
> >ShellPrintEx (0, ((INT32)Row) - 1, Line);
> > +
> > +  // Free if allocated
> > +  if (Line != Buffer)
> > +FreePool (Line);
> >  }
> >
> >  /**
> > --
> > 2.1.4
> >
> > With best regards,
> > Sebastian Witt
> >
> > Siemens AG
> > Digital Factory Division
> > Factory Automation
> > Automation Products and Systems
> > DF FA AS DH KHE 1
> > Oestliche Rheinbrueckenstr. 50
> > 76187 Karlsruhe, Germany
> > Tel.: +49 721 595-5326
> > mailto:sebastian.w...@siemens.com
> >
> > www.siemens.com/ingenuityforlife
> >
> > Siemens Aktiengesellschaft: Chairman of the Supervisory Board: Gerhard
> > Cromme; Managing Board: Joe Kaeser, Chairman, President and Chief
> > Executive Officer; Roland Busch, Lisa Davis, Klaus Helmrich, Janina
> > Kugel, Siegfried Russwurm, Ralf P. Thomas; Registered offices: Berlin
> > and Munich, Germany; Commercial registries: Berlin Charlottenburg, HRB
> > 12300, Munich, HRB 6684; WEEE-Reg.-No. DE 23691322
> > ___
> > 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-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] Fix edit on screens with more than 200 columns

[Witt, Sebastian]

Only performance. But I haven't measured if there is a big difference between 
static buffer and AllocateZeroPool. I wouldn't use a fixed value. There may be 
a display device with more than 400 columns. Otherwise one can always allocate 
the [LastCol + 1] buffer.

-Original Message-
From: Carsey, Jaben [mailto:jaben.car...@intel.com] 
Sent: Dienstag, 24. Januar 2017 17:27
To: Witt, Sebastian (DF FA AS DH KHE 1); edk2-devel@lists.01.org
Cc: Carsey, Jaben
Subject: RE: [PATCH] Fix edit on screens with more than 200 columns

Is there a reason to not just always start with allocating the 400 and then we 
don't need to complicate the end to conditionally free the buffer?

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of 
> Witt, Sebastian
> Sent: Tuesday, January 24, 2017 5:14 AM
> To: edk2-devel@lists.01.org
> Subject: [edk2] [PATCH] Fix edit on screens with more than 200 columns
> Importance: High
> 
> If the shell edit command is used on a screen with more than
> 200 columns, we get a buffer overflow. This increases the default 
> buffer size to
> 400 columns and allocates a pool when this is not enough.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Sebastian Witt 
> 
> ---
>  .../UefiShellDebug1CommandsLib.c  | 15 
> ++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git
> a/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsL
> i
> b.c
> b/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsL
> i
> b.c
> index 6ebf002..d81dd01 100644
> ---
> a/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsL
> i
> b.c
> +++
> b/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1Command
> +++ sLib.c
> @@ -302,12 +302,21 @@ EditorClearLine (
>IN UINTN LastRow
>)
>  {
> -  CHAR16 Line[200];
> +  CHAR16 Buffer[400];
> +  CHAR16 *Line = Buffer;
> 
>if (Row == 0) {
>  Row = 1;
>}
> 
> +  // If there are more columns than our buffer can contain, allocate 
> + new buffer  if (LastCol >= (sizeof (Buffer) / sizeof (CHAR16))) {
> +Line = AllocateZeroPool (LastCol*(sizeof(CHAR16) + 1));
> +if (Line == NULL) {
> +  return;
> +}
> +  }
> +
>//
>// prepare a blank line
>//
> @@ -326,6 +335,10 @@ EditorClearLine (
>// print out the blank line
>//
>ShellPrintEx (0, ((INT32)Row) - 1, Line);
> +
> +  // Free if allocated
> +  if (Line != Buffer)
> +FreePool (Line);
>  }
> 
>  /**
> --
> 2.1.4
> 
> With best regards,
> Sebastian Witt
> 
> Siemens AG
> Digital Factory Division
> Factory Automation
> Automation Products and Systems
> DF FA AS DH KHE 1
> Oestliche Rheinbrueckenstr. 50
> 76187 Karlsruhe, Germany
> Tel.: +49 721 595-5326
> mailto:sebastian.w...@siemens.com
> 
> www.siemens.com/ingenuityforlife
> 
> Siemens Aktiengesellschaft: Chairman of the Supervisory Board: Gerhard 
> Cromme; Managing Board: Joe Kaeser, Chairman, President and Chief 
> Executive Officer; Roland Busch, Lisa Davis, Klaus Helmrich, Janina 
> Kugel, Siegfried Russwurm, Ralf P. Thomas; Registered offices: Berlin 
> and Munich, Germany; Commercial registries: Berlin Charlottenburg, HRB 
> 12300, Munich, HRB 6684; WEEE-Reg.-No. DE 23691322 
> ___
> 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] ShellPkg/pci: Fix extended register dumping for MFVC capability

[Carsey, Jaben]

Reviewed-by: Jaben Carsey 

> -Original Message-
> From: Ni, Ruiyu
> Sent: Monday, January 23, 2017 10:43 PM
> To: edk2-devel@lists.01.org
> Cc: Carsey, Jaben 
> Subject: [PATCH] ShellPkg/pci: Fix extended register dumping for MFVC
> capability
> Importance: High
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=355
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ruiyu Ni 
> Cc: Jaben Carsey 
> ---
>  ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c
> b/ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c
> index 99e6caf..fb7561f 100644
> --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c
> +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c
> @@ -5505,7 +5505,8 @@ PrintInterpretedExtendedCompatibilityVirtualChannel
> (
>DumpHex (
>  4,
>  EFI_PCIE_CAPABILITY_BASE_OFFSET + ((UINT8*)HeaderAddress -
> (UINT8*)HeadersBaseAddress),
> -sizeof(PCI_EXPRESS_EXTENDED_CAPABILITIES_VIRTUAL_CHANNEL_VC) +
> (Header->ExtendedVcCount - 1) *
> sizeof(PCI_EXPRESS_EXTENDED_CAPABILITIES_VIRTUAL_CHANNEL_CAPABILITY
> ),
> +sizeof
> (PCI_EXPRESS_EXTENDED_CAPABILITIES_VIRTUAL_CHANNEL_CAPABILITY)
> ++ Header->ExtendedVcCount * sizeof
> (PCI_EXPRESS_EXTENDED_CAPABILITIES_VIRTUAL_CHANNEL_VC),
>  (VOID *) (HeaderAddress)
>  );
> 
> --
> 2.9.0.windows.1

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

Re: [edk2] [PATCH] ShellPkg/pci: Use PCI definitions defined in MdePkg

[Carsey, Jaben]

Reviewed-by: Jaben Carsey 

> -Original Message-
> From: Ni, Ruiyu
> Sent: Monday, January 23, 2017 10:52 PM
> To: edk2-devel@lists.01.org
> Cc: Carsey, Jaben 
> Subject: [PATCH] ShellPkg/pci: Use PCI definitions defined in MdePkg
> Importance: High
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=354
> 
> The patch removes the local PCI definitions and uses the definitions
> defined in MdePkg/Include/IndustryStandard folder.
> There is no functionality impact.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ruiyu Ni 
> Cc: Jaben Carsey 
> ---
>  ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c | 582 ++-
> ---
>  ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.h | 423 +---
>  2 files changed, 284 insertions(+), 721 deletions(-)
> 
> diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c
> b/ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c
> index 5302051..9cc4f5c 100644
> --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c
> +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c
> @@ -1,7 +1,7 @@
>  /** @file
>Main file for Pci shell Debug1 function.
> 
> -  Copyright (c) 2005 - 2016, Intel Corporation. All rights reserved.
> +  Copyright (c) 2005 - 2017, Intel Corporation. All rights reserved.
>(C) Copyright 2013-2015 Hewlett-Packard Development Company, L.P.
>(C) Copyright 2016 Hewlett Packard Enterprise Development LP
>This program and the accompanying materials
> @@ -1928,7 +1928,7 @@ PciExplainData (
>  **/
>  EFI_STATUS
>  PciExplainDeviceData (
> -  IN PCI_DEVICE_HEADER  *Device,
> +  IN PCI_DEVICE_HEADER_TYPE_REGION  *Device,
>IN UINT64 Address,
>IN EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL*IoDev
>);
> @@ -1944,7 +1944,7 @@ PciExplainDeviceData (
>  **/
>  EFI_STATUS
>  PciExplainBridgeData (
> -  IN  PCI_BRIDGE_HEADER *Bridge,
> +  IN  PCI_BRIDGE_CONTROL_REGISTER   *Bridge,
>IN  UINT64Address,
>IN  EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL   *IoDev
>);
> @@ -1980,7 +1980,7 @@ PciExplainBar (
>  **/
>  EFI_STATUS
>  PciExplainCardBusData (
> -  IN PCI_CARDBUS_HEADER *CardBus,
> +  IN PCI_CARDBUS_CONTROL_REGISTER   *CardBus,
>IN UINT64 Address,
>IN EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL*IoDev
>);
> @@ -2075,7 +2075,7 @@ PciExplainPciExpress (
>  **/
>  EFI_STATUS
>  ExplainPcieCapReg (
> -  IN PCIE_CAP_STRUCTURE *PciExpressCap
> +  IN PCI_CAPABILITY_PCIEXP *PciExpressCap
>);
> 
>  /**
> @@ -2087,7 +2087,7 @@ ExplainPcieCapReg (
>  **/
>  EFI_STATUS
>  ExplainPcieDeviceCap (
> -  IN PCIE_CAP_STRUCTURE *PciExpressCap
> +  IN PCI_CAPABILITY_PCIEXP *PciExpressCap
>);
> 
>  /**
> @@ -2099,7 +2099,7 @@ ExplainPcieDeviceCap (
>  **/
>  EFI_STATUS
>  ExplainPcieDeviceControl (
> -  IN PCIE_CAP_STRUCTURE *PciExpressCap
> +  IN PCI_CAPABILITY_PCIEXP *PciExpressCap
>);
> 
>  /**
> @@ -2111,7 +2111,7 @@ ExplainPcieDeviceControl (
>  **/
>  EFI_STATUS
>  ExplainPcieDeviceStatus (
> -  IN PCIE_CAP_STRUCTURE *PciExpressCap
> +  IN PCI_CAPABILITY_PCIEXP *PciExpressCap
>);
> 
>  /**
> @@ -2123,7 +2123,7 @@ ExplainPcieDeviceStatus (
>  **/
>  EFI_STATUS
>  ExplainPcieLinkCap (
> -  IN PCIE_CAP_STRUCTURE *PciExpressCap
> +  IN PCI_CAPABILITY_PCIEXP *PciExpressCap
>);
> 
>  /**
> @@ -2135,7 +2135,7 @@ ExplainPcieLinkCap (
>  **/
>  EFI_STATUS
>  ExplainPcieLinkControl (
> -  IN PCIE_CAP_STRUCTURE *PciExpressCap
> +  IN PCI_CAPABILITY_PCIEXP *PciExpressCap
>);
> 
>  /**
> @@ -2147,7 +2147,7 @@ ExplainPcieLinkControl (
>  **/
>  EFI_STATUS
>  ExplainPcieLinkStatus (
> -  IN PCIE_CAP_STRUCTURE *PciExpressCap
> +  IN PCI_CAPABILITY_PCIEXP *PciExpressCap
>);
> 
>  /**
> @@ -2159,7 +2159,7 @@ ExplainPcieLinkStatus (
>  **/
>  EFI_STATUS
>  ExplainPcieSlotCap (
> -  IN PCIE_CAP_STRUCTURE *PciExpressCap
> +  IN PCI_CAPABILITY_PCIEXP *PciExpressCap
>);
> 
>  /**
> @@ -2171,7 +2171,7 @@ ExplainPcieSlotCap (
>  **/
>  EFI_STATUS
>  ExplainPcieSlotControl (
> -  IN PCIE_CAP_STRUCTURE *PciExpressCap
> +  IN PCI_CAPABILITY_PCIEXP *PciExpressCap
>);
> 
>  /**
> @@ -2183,7 +2183,7 @@ ExplainPcieSlotControl (
>  **/
>  EFI_STATUS
>  ExplainPcieSlotStatus (
> -  IN PCIE_CAP_STRUCTURE *PciExpressCap
> +  IN PCI_CAPABILITY_PCIEXP *PciExpressCap
>);
> 
>  /**
> @@ -2195,7 +2195,7 @@ ExplainPcieSlotStatus (
>  **/
>  EFI_STATUS
>  ExplainPcieRootControl (
> -  IN PCIE_CAP_STRUCTURE *PciExpressCap
> +  IN PCI_CAPABILITY_PCIEXP *PciExpressCap
>);
> 
>  /**
> @@ -2207,7 +2207,7 @@ ExplainPcieRootControl (
>  **/
>  EFI_STATUS
>  ExplainPcieRootCap (
> -  IN PCIE_CAP_STRUCTURE *PciExpressCap
> +  IN PCI_CAPABILITY_PCIEXP *PciExpressCap
>);
> 
>  /**
> @@ -2219,10 +2219,10 @@ ExplainPcieRootCap (
>  **/

Re: [edk2] [PATCH] Fix edit on screens with more than 200 columns

[Carsey, Jaben]

Is there a reason to not just always start with allocating the 400 and then we 
don't need to complicate the end to conditionally free the buffer?

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Witt,
> Sebastian
> Sent: Tuesday, January 24, 2017 5:14 AM
> To: edk2-devel@lists.01.org
> Subject: [edk2] [PATCH] Fix edit on screens with more than 200 columns
> Importance: High
> 
> If the shell edit command is used on a screen with more than
> 200 columns, we get a buffer overflow. This increases the default buffer size 
> to
> 400 columns and allocates a pool when this is not enough.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Sebastian Witt 
> 
> ---
>  .../UefiShellDebug1CommandsLib.c  | 15 
> ++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git
> a/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLi
> b.c
> b/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLi
> b.c
> index 6ebf002..d81dd01 100644
> ---
> a/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLi
> b.c
> +++
> b/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1Command
> +++ sLib.c
> @@ -302,12 +302,21 @@ EditorClearLine (
>IN UINTN LastRow
>)
>  {
> -  CHAR16 Line[200];
> +  CHAR16 Buffer[400];
> +  CHAR16 *Line = Buffer;
> 
>if (Row == 0) {
>  Row = 1;
>}
> 
> +  // If there are more columns than our buffer can contain, allocate
> + new buffer  if (LastCol >= (sizeof (Buffer) / sizeof (CHAR16))) {
> +Line = AllocateZeroPool (LastCol*(sizeof(CHAR16) + 1));
> +if (Line == NULL) {
> +  return;
> +}
> +  }
> +
>//
>// prepare a blank line
>//
> @@ -326,6 +335,10 @@ EditorClearLine (
>// print out the blank line
>//
>ShellPrintEx (0, ((INT32)Row) - 1, Line);
> +
> +  // Free if allocated
> +  if (Line != Buffer)
> +FreePool (Line);
>  }
> 
>  /**
> --
> 2.1.4
> 
> With best regards,
> Sebastian Witt
> 
> Siemens AG
> Digital Factory Division
> Factory Automation
> Automation Products and Systems
> DF FA AS DH KHE 1
> Oestliche Rheinbrueckenstr. 50
> 76187 Karlsruhe, Germany
> Tel.: +49 721 595-5326
> mailto:sebastian.w...@siemens.com
> 
> www.siemens.com/ingenuityforlife
> 
> Siemens Aktiengesellschaft: Chairman of the Supervisory Board: Gerhard
> Cromme; Managing Board: Joe Kaeser, Chairman, President and Chief Executive
> Officer; Roland Busch, Lisa Davis, Klaus Helmrich, Janina Kugel, Siegfried
> Russwurm, Ralf P. Thomas; Registered offices: Berlin and Munich, Germany;
> Commercial registries: Berlin Charlottenburg, HRB 12300, Munich, HRB 6684;
> WEEE-Reg.-No. DE 23691322
> ___
> 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] ShellPkg/pci: Use PCI definitions defined in MdePkg

[Carsey, Jaben]

Reviewed-by: Jaben Carsey 

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Ruiyu
> Ni
> Sent: Monday, January 23, 2017 10:42 PM
> To: edk2-devel@lists.01.org
> Cc: Jaben Carsey 
> Subject: [edk2] [PATCH] ShellPkg/pci: Use PCI definitions defined in MdePkg
> Importance: High
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=354
> 
> The patch removes the local PCI definitions and uses the definitions
> defined in MdePkg/Include/IndustryStandard folder.
> There is no functionality impact.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ruiyu Ni 
> Cc: Jaben Carsey 
> ---
>  ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c | 582 ++-
> ---
>  ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.h | 423 +---
>  2 files changed, 284 insertions(+), 721 deletions(-)
> 
> diff --git a/ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c
> b/ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c
> index 5302051..9cc4f5c 100644
> --- a/ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c
> +++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/Pci.c
> @@ -1,7 +1,7 @@
>  /** @file
>Main file for Pci shell Debug1 function.
> 
> -  Copyright (c) 2005 - 2016, Intel Corporation. All rights reserved.
> +  Copyright (c) 2005 - 2017, Intel Corporation. All rights reserved.
>(C) Copyright 2013-2015 Hewlett-Packard Development Company, L.P.
>(C) Copyright 2016 Hewlett Packard Enterprise Development LP
>This program and the accompanying materials
> @@ -1928,7 +1928,7 @@ PciExplainData (
>  **/
>  EFI_STATUS
>  PciExplainDeviceData (
> -  IN PCI_DEVICE_HEADER  *Device,
> +  IN PCI_DEVICE_HEADER_TYPE_REGION  *Device,
>IN UINT64 Address,
>IN EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL*IoDev
>);
> @@ -1944,7 +1944,7 @@ PciExplainDeviceData (
>  **/
>  EFI_STATUS
>  PciExplainBridgeData (
> -  IN  PCI_BRIDGE_HEADER *Bridge,
> +  IN  PCI_BRIDGE_CONTROL_REGISTER   *Bridge,
>IN  UINT64Address,
>IN  EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL   *IoDev
>);
> @@ -1980,7 +1980,7 @@ PciExplainBar (
>  **/
>  EFI_STATUS
>  PciExplainCardBusData (
> -  IN PCI_CARDBUS_HEADER *CardBus,
> +  IN PCI_CARDBUS_CONTROL_REGISTER   *CardBus,
>IN UINT64 Address,
>IN EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL*IoDev
>);
> @@ -2075,7 +2075,7 @@ PciExplainPciExpress (
>  **/
>  EFI_STATUS
>  ExplainPcieCapReg (
> -  IN PCIE_CAP_STRUCTURE *PciExpressCap
> +  IN PCI_CAPABILITY_PCIEXP *PciExpressCap
>);
> 
>  /**
> @@ -2087,7 +2087,7 @@ ExplainPcieCapReg (
>  **/
>  EFI_STATUS
>  ExplainPcieDeviceCap (
> -  IN PCIE_CAP_STRUCTURE *PciExpressCap
> +  IN PCI_CAPABILITY_PCIEXP *PciExpressCap
>);
> 
>  /**
> @@ -2099,7 +2099,7 @@ ExplainPcieDeviceCap (
>  **/
>  EFI_STATUS
>  ExplainPcieDeviceControl (
> -  IN PCIE_CAP_STRUCTURE *PciExpressCap
> +  IN PCI_CAPABILITY_PCIEXP *PciExpressCap
>);
> 
>  /**
> @@ -2111,7 +2111,7 @@ ExplainPcieDeviceControl (
>  **/
>  EFI_STATUS
>  ExplainPcieDeviceStatus (
> -  IN PCIE_CAP_STRUCTURE *PciExpressCap
> +  IN PCI_CAPABILITY_PCIEXP *PciExpressCap
>);
> 
>  /**
> @@ -2123,7 +2123,7 @@ ExplainPcieDeviceStatus (
>  **/
>  EFI_STATUS
>  ExplainPcieLinkCap (
> -  IN PCIE_CAP_STRUCTURE *PciExpressCap
> +  IN PCI_CAPABILITY_PCIEXP *PciExpressCap
>);
> 
>  /**
> @@ -2135,7 +2135,7 @@ ExplainPcieLinkCap (
>  **/
>  EFI_STATUS
>  ExplainPcieLinkControl (
> -  IN PCIE_CAP_STRUCTURE *PciExpressCap
> +  IN PCI_CAPABILITY_PCIEXP *PciExpressCap
>);
> 
>  /**
> @@ -2147,7 +2147,7 @@ ExplainPcieLinkControl (
>  **/
>  EFI_STATUS
>  ExplainPcieLinkStatus (
> -  IN PCIE_CAP_STRUCTURE *PciExpressCap
> +  IN PCI_CAPABILITY_PCIEXP *PciExpressCap
>);
> 
>  /**
> @@ -2159,7 +2159,7 @@ ExplainPcieLinkStatus (
>  **/
>  EFI_STATUS
>  ExplainPcieSlotCap (
> -  IN PCIE_CAP_STRUCTURE *PciExpressCap
> +  IN PCI_CAPABILITY_PCIEXP *PciExpressCap
>);
> 
>  /**
> @@ -2171,7 +2171,7 @@ ExplainPcieSlotCap (
>  **/
>  EFI_STATUS
>  ExplainPcieSlotControl (
> -  IN PCIE_CAP_STRUCTURE *PciExpressCap
> +  IN PCI_CAPABILITY_PCIEXP *PciExpressCap
>);
> 
>  /**
> @@ -2183,7 +2183,7 @@ ExplainPcieSlotControl (
>  **/
>  EFI_STATUS
>  ExplainPcieSlotStatus (
> -  IN PCIE_CAP_STRUCTURE *PciExpressCap
> +  IN PCI_CAPABILITY_PCIEXP *PciExpressCap
>);
> 
>  /**
> @@ -2195,7 +2195,7 @@ ExplainPcieSlotStatus (
>  **/
>  EFI_STATUS
>  ExplainPcieRootControl (
> -  IN PCIE_CAP_STRUCTURE *PciExpressCap
> +  IN PCI_CAPABILITY_PCIEXP *PciExpressCap
>);
> 
>  /**
> @@ -2207,7 +2207,7 @@ ExplainPcieRootControl (
>  **/
>  EFI_STATUS
>  ExplainPcieRootCap (
> -  IN PCIE_CAP_STRUCTURE *PciExpressCap
> +  IN PCI_CAPABILITY_PCIEXP 

[edk2] [PATCH] Fix edit on screens with more than 200 columns

[Witt, Sebastian]

If the shell edit command is used on a screen with more than
200 columns, we get a buffer overflow. This increases the default buffer size 
to 400 columns and allocates a pool when this is not enough.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Sebastian Witt 

---
 .../UefiShellDebug1CommandsLib.c  | 15 ++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git 
a/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib.c 
b/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib.c
index 6ebf002..d81dd01 100644
--- a/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib.c
+++ b/ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib.c
@@ -302,12 +302,21 @@ EditorClearLine (
   IN UINTN LastRow
   )
 {
-  CHAR16 Line[200];
+  CHAR16 Buffer[400];
+  CHAR16 *Line = Buffer;
 
   if (Row == 0) {
 Row = 1;
   }
 
+  // If there are more columns than our buffer can contain, allocate new buffer
+  if (LastCol >= (sizeof (Buffer) / sizeof (CHAR16))) {
+Line = AllocateZeroPool (LastCol*(sizeof(CHAR16) + 1));
+if (Line == NULL) {
+  return;
+}
+  }
+  
   //
   // prepare a blank line
   //
@@ -326,6 +335,10 @@ EditorClearLine (
   // print out the blank line
   //
   ShellPrintEx (0, ((INT32)Row) - 1, Line);
+  
+  // Free if allocated
+  if (Line != Buffer)
+FreePool (Line);
 }
 
 /**
-- 
2.1.4

With best regards,
Sebastian Witt

Siemens AG
Digital Factory Division
Factory Automation
Automation Products and Systems
DF FA AS DH KHE 1
Oestliche Rheinbrueckenstr. 50
76187 Karlsruhe, Germany
Tel.: +49 721 595-5326
mailto:sebastian.w...@siemens.com

www.siemens.com/ingenuityforlife

Siemens Aktiengesellschaft: Chairman of the Supervisory Board: Gerhard Cromme; 
Managing Board: Joe Kaeser, Chairman, President and Chief Executive Officer; 
Roland Busch, Lisa Davis, Klaus Helmrich, Janina Kugel, Siegfried Russwurm, 
Ralf P. Thomas; Registered offices: Berlin and Munich, Germany; Commercial 
registries: Berlin Charlottenburg, HRB 12300, Munich, HRB 6684; WEEE-Reg.-No. 
DE 23691322
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe: Fixed crash on Juno R0

[Ryan Harkin]

On 24 January 2017 at 11:05, Ryan Harkin  wrote:
> On 24 January 2017 at 02:19, Daniil Egranov  wrote:
>> Hi Ryan,
>>
>>
>>
>> On 01/23/2017 06:56 AM, Ryan Harkin wrote:
>>>
>>> On 20 January 2017 at 20:57, Daniil Egranov 
>>> wrote:

 Hi Ryan,


 On 01/20/2017 04:30 AM, Ryan Harkin wrote:

 On 20 January 2017 at 01:34, Daniil Egranov 
 wrote:

 Hi Leif, Ryan


 On 01/19/2017 09:13 AM, Leif Lindholm wrote:

 On Thu, Jan 19, 2017 at 01:49:04PM +, Ryan Harkin wrote:

 On 18 January 2017 at 23:27, Daniil Egranov 
 wrote:

 The Marvell Yukon MAC address load supported only on Juno R1 and R2.
 It disabled for Juno R0 due to PCI issues on this board.

 Contributed-under: TianoCore Contribution Agreement 1.0
 Signed-off-by: Daniil Egranov 

 Tested-by: Ryan Harkin 

 ---
   ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c | 9 +++--
   1 file changed, 7 insertions(+), 2 deletions(-)

 diff --git a/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
 b/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
 index 47ff587..e9e6990 100644
 --- a/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
 +++ b/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
 @@ -378,6 +378,7 @@ OnEndOfDxe (
 EFI_DEVICE_PATH_PROTOCOL* PciRootComplexDevicePath;
 EFI_HANDLEHandle;
 EFI_STATUSStatus;
 +  UINT32JunoRevision;

 //
 // PCI Root Complex initialization
 @@ -393,8 +394,12 @@ OnEndOfDxe (
 Status = gBS->ConnectController (Handle, NULL,
 PciRootComplexDevicePath,
 FALSE);
 ASSERT_EFI_ERROR (Status);

 -  Status = ArmJunoSetNicMacAddress ();
 -  ASSERT_EFI_ERROR (Status);
 +  GetJunoRevision (JunoRevision);
 +
 +  if (JunoRevision != JUNO_REVISION_R0) {
 +Status = ArmJunoSetNicMacAddress ();
 +ASSERT_EFI_ERROR (Status);

 This is just an FYI, but I stacked your patch on top of mainline, like
 this:

 5f81f61  2017-01-18  ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe:
 Fixed crash on Juno R0   [Daniil Egranov]
 19ca06b  2017-01-19  OvmfPkg: Remove superfluous return statements.
   [Thomas Huth]

 The first time I ran this, Juno R0 worked fine, but on R1 and R2, the
 assert triggered:

 UEFI firmware (version 5f81f61 built at 11:56:52 on Jan 19 2017)
 [snip]
 ASSERT_EFI_ERROR (Status = Not Found)
 ASSERT [ArmJunoDxe]

 /linaro/platforms/uefi/edk2/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c(401):
 !EFI_ERROR (Status)

 I worked out what is happening. And it's not to do with this patch.
 It's another fall-out from the re-work you did to the previous patch.
 It's also ultimately due to a bug the firmware.

 With the initial version of your "Set Marvell Yukon MAC address"
 patch, this hang didn't happen. I suspect that was because your error
 checking was weaker and certain PCIe failures didn't trigger the
 assert.

 To reproduce the error with this commit:
 1) power on and boot R1 or R2 into Shell
I do this by interrupting the boot by pressing ESCAPE and using the
 boot
 menu
 2) At the Shell prompt, run "reset -s" to shutdown
 3) At the ARM Boot Loader "Cmd>" prompt, run "reboot"
 4) the board will hang while booting UEFI, assuming the board firmware
 doesn't die with constant messages like this:

  ERROR: PCIe CSR read failed to respond
  ERROR: SMBus transaction not claimed

 Assuming the problem is firmware, not EDK2, what should we do about it?

 OK, so instinctively, my reaction was that "the reset -s bug is a
 system controller firmware bug and we shouldn't work around
 it". However, since it is actually disrupting Ryan's workflow, which
 frequently doesn't touch PCI at all, I think downgrading the ASSERT to
 an error message is a good idea short-term.

 Daniil - could you make that change please?

 /
  Leif


 I've been able to reproduce "PCIe CSR read failed to respond" and "SMBus
 transaction not claimed" errors on my Juno R2. I disabled Marvell Yukon
 driver (.dsc/.fdf) and removed ArmJunoDxe patch but still see the same
 error
 messages during the initial boot.

 Testing motherboard interfaces (FPGA build 118)...
 SRAM 32MB test: PASSED
 LAN9118   test: PASSED
 KMI1/2test: PASSED
 MMC   test: PASSED
 PB/LEDs   test: PASSED
 FPGA UART test: PASSED
 ERROR: PCIe CSR read failed to respond
 ERROR: 

[edk2] [PATCH 5/5] BaseTools: add scripts to generate EBC call signatures

[Pete Batard]

* EBC binaries meant to be compatible with the ARM EBC VM
  require the insertion of call signatures at the locations
  where the BREAK 5 offsets are stored.
* This patch adds 2 new Python applications, as well as the
  required modifications for build_rule and tools_def templates:
  - GenEbcSignature is a script that parses a preprocessed C
source, along with a binary object file, as generated by the
intel EBC compiler, to create the signature data, which it
stores into a .sig file.
  - PatchEbcSignature is a script that processes the .sig files
as well as the .map data and the .efi binary, to insert the
signatures at the required locations.
* Note that the insertion of signatures into EBC binaries has
  absolutely no impact for EBC execution on non ARM platforms,
  as it applies to data parts that are ignored on these VMs.
* Also note the GenEbcSignature is designed to be very basic
  with regards to the detection of call signatures that have
  straight 64-bit parameters, as it is meant to be used as a
  stopgap solution until the intel EBC compiler (which does
  has easy access to the data required to do so, and is best
  placed for such processing) is updated to generate .sig files
  on its own.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Pete Batard 
---
 .../BinWrappers/WindowsLike/GenEbcSignature.bat|   3 +
 .../BinWrappers/WindowsLike/PatchEbcSignature.bat  |   3 +
 BaseTools/Conf/build_rule.template |  72 +++--
 BaseTools/Conf/tools_def.template  |  12 +
 .../Python/GenEbcSignature/GenEbcSignature.py  | 306 +
 .../Source/Python/GenEbcSignature/__init__.py  |  15 +
 .../Python/PatchEbcSignature/PatchEbcSignature.py  | 226 +++
 .../Source/Python/PatchEbcSignature/__init__.py|  15 +
 8 files changed, 633 insertions(+), 19 deletions(-)
 create mode 100644 BaseTools/BinWrappers/WindowsLike/GenEbcSignature.bat
 create mode 100644 BaseTools/BinWrappers/WindowsLike/PatchEbcSignature.bat
 create mode 100644 BaseTools/Source/Python/GenEbcSignature/GenEbcSignature.py
 create mode 100644 BaseTools/Source/Python/GenEbcSignature/__init__.py
 create mode 100644 
BaseTools/Source/Python/PatchEbcSignature/PatchEbcSignature.py
 create mode 100644 BaseTools/Source/Python/PatchEbcSignature/__init__.py

diff --git a/BaseTools/BinWrappers/WindowsLike/GenEbcSignature.bat 
b/BaseTools/BinWrappers/WindowsLike/GenEbcSignature.bat
new file mode 100644
index ..9fbb704a6eb0
--- /dev/null
+++ b/BaseTools/BinWrappers/WindowsLike/GenEbcSignature.bat
@@ -0,0 +1,3 @@
+@setlocal
+@set ToolName=%~n0%
+@%PYTHON_HOME%\python.exe 
%BASE_TOOLS_PATH%\Source\Python\%ToolName%\%ToolName%.py %*
diff --git a/BaseTools/BinWrappers/WindowsLike/PatchEbcSignature.bat 
b/BaseTools/BinWrappers/WindowsLike/PatchEbcSignature.bat
new file mode 100644
index ..9fbb704a6eb0
--- /dev/null
+++ b/BaseTools/BinWrappers/WindowsLike/PatchEbcSignature.bat
@@ -0,0 +1,3 @@
+@setlocal
+@set ToolName=%~n0%
+@%PYTHON_HOME%\python.exe 
%BASE_TOOLS_PATH%\Source\Python\%ToolName%\%ToolName%.py %*
diff --git a/BaseTools/Conf/build_rule.template 
b/BaseTools/Conf/build_rule.template
index 1db94b696f89..3408f507e78b 100755
--- a/BaseTools/Conf/build_rule.template
+++ b/BaseTools/Conf/build_rule.template
@@ -161,6 +161,27 @@
 "$(CC)" $(CC_FLAGS) -c -o ${dst} $(INC) ${src}
 "$(SYMRENAME)" $(SYMRENAME_FLAGS) ${dst}
 
+[C-Code-File.COMMON.EBC]
+
+?.c
+?.C
+?.cc
+?.CC
+?.cpp
+?.Cpp
+?.CPP
+
+
+$(MAKE_FILE)
+
+
+$(OUTPUT_DIR)(+)${s_dir}(+)${s_base}.obj
+
+
+"$(CC)" /Fo${dst} $(CC_FLAGS) $(INC) ${src}
+# Parse preprocessed sources to generate the EBC call signatures
+"$(CC)" /E /EP $(CC_FLAGS) $(INC) ${src} | "$(GENEBC)" -v -o ${dst} -s 
$(OUTPUT_DIR)(+)$(MODULE_NAME).sig
+
 
[C-Code-File.BASE.AARCH64,C-Code-File.SEC.AARCH64,C-Code-File.PEI_CORE.AARCH64,C-Code-File.PEIM.AARCH64,C-Code-File.BASE.ARM,C-Code-File.SEC.ARM,C-Code-File.PEI_CORE.ARM,C-Code-File.PEIM.ARM]
 
 ?.c
@@ -267,10 +288,10 @@
 
 
 "$(SLINK)" cr ${dst} $(SLINK_FLAGS) @$(OBJECT_FILES_LIST)
-
+
 
 "$(SLINK)" $(SLINK_FLAGS) ${dst} --via $(OBJECT_FILES_LIST)
-
+
 
 # $(OBJECT_FILES_LIST) has wrong paths for cygwin
 "$(SLINK)" $(SLINK_FLAGS) ${dst} $(OBJECT_FILES)
@@ -308,8 +329,8 @@
 
 
 "$(DLINK)" $(DLINK_FLAGS) -o ${dst} $(DLINK_SPATH) -filelist 
$(STATIC_LIBRARY_FILES_LIST)  $(DLINK2_FLAGS)
-
-
+
+
 [Static-Library-File.SEC.AARCH64, Static-Library-File.PEI_CORE.AARCH64, 
Static-Library-File.PEIM.AARCH64,Static-Library-File.SEC.ARM, 
Static-Library-File.PEI_CORE.ARM, Static-Library-File.PEIM.ARM]
 
 *.lib
@@ -353,8 +374,8 @@
 
 
 "$(DLINK)" -o ${dst} 

[edk2] [PATCH 4/5] MdeModulePkg/EbcDxe: add call signatures for ARM native->EBC support

[Pete Batard]

* This patch fixes the call from native into EBC ARM calling convention
  issue by introducing support for call signatures when invoking BREAK 5.
* These call signatures are provided as the high 32 bit word of the
  64-bit longword to which R7 points during BREAK 5 invocation (the lowest
  32-bit being left untouched, as the "offset from self").
* Within this word the upper 16-bits should be set to an EBC_CALL_SIGNATURE
  marker and the lower 16 bits to the signature itself, with each bit
  indicating if the corresponding argument is 64-bit (1) or not (0).
* The compiler toolchain is expected to automatically fill these signatures
  when producing binaries.
* None of the changes introduced by this patch have any bearing on the
  behaviour of existing EBC VMs or EBC applications, except for ARM.
  Especially, existing EBC code that is missing signatures still works
  exactly as it did on the updated versions of IA32, X64 and AARCH64 VMs,
  and EBC code that includes signatures also works just the same on non
  signature-aware VMs.
  The presence or absence of signature has only an impact on ARM platforms.
* However, because this feature requires a minor specs change and because
  an EBC application that provides call signatures may want to detect if
  the VM is signature aware, we also bump the global VM version to 1.1.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Pete Batard 
---
 MdeModulePkg/Include/Protocol/EbcVmTest.h   |  2 +-
 MdeModulePkg/Universal/EbcDxe/Arm/EbcLowLevel.S |  8 ++-
 MdeModulePkg/Universal/EbcDxe/Arm/EbcSupport.c  | 85 +++--
 MdeModulePkg/Universal/EbcDxe/EbcExecute.c  | 27 +++-
 MdeModulePkg/Universal/EbcDxe/EbcInt.h  |  7 +-
 5 files changed, 102 insertions(+), 27 deletions(-)

diff --git a/MdeModulePkg/Include/Protocol/EbcVmTest.h 
b/MdeModulePkg/Include/Protocol/EbcVmTest.h
index 12d5e5217059..5b6aee704795 100644
--- a/MdeModulePkg/Include/Protocol/EbcVmTest.h
+++ b/MdeModulePkg/Include/Protocol/EbcVmTest.h
@@ -34,7 +34,7 @@ typedef struct _EFI_EBC_VM_TEST_PROTOCOL 
EFI_EBC_VM_TEST_PROTOCOL;
 // VM major/minor version
 //
 #define VM_MAJOR_VERSION  1
-#define VM_MINOR_VERSION  0
+#define VM_MINOR_VERSION  1
 
 //
 // Bits in the VM->StopFlags field
diff --git a/MdeModulePkg/Universal/EbcDxe/Arm/EbcLowLevel.S 
b/MdeModulePkg/Universal/EbcDxe/Arm/EbcLowLevel.S
index dc9c3946ced2..2325dd07a472 100644
--- a/MdeModulePkg/Universal/EbcDxe/Arm/EbcLowLevel.S
+++ b/MdeModulePkg/Universal/EbcDxe/Arm/EbcLowLevel.S
@@ -129,16 +129,17 @@ ASM_PFX(EbcLLCALLEXNativeArm):
 //
 // EbcLLEbcInterpret
 //
-// This function is called by the thunk code to handle an Native to EBC call
-// This can handle up to 16 arguments (1-4 on in r0-r3, 5-16 are on the stack)
+// This function is called by the thunk code to handle a Native to EBC call
+// This can handle up to 16 arguments (args 1-2/1-4 in r0-r3, rest onstack)
 // ip contains the Entry point that will be the first argument when
 // EBCInterpret is called.
 //
 //
 ASM_PFX(EbcLLEbcInterpret):
+
 stmdb   sp!, {r4, lr}
 
-// push the entry point and the address of args #5 - #16 onto the stack
+// push the entry point and the address of non register args on the stack
 add r4, sp, #8
 str ip, [sp, #-8]!
 str r4, [sp, #4]
@@ -180,3 +181,4 @@ ASM_PFX(mEbcInstructionBufferTemplate):
 
 .long   0   // EBC_ENTRYPOINT_SIGNATURE
 0:  .long   0   // EBC_LL_EBC_ENTRYPOINT_SIGNATURE
+.long   0   // EBC_CALL_SIGNATURE
diff --git a/MdeModulePkg/Universal/EbcDxe/Arm/EbcSupport.c 
b/MdeModulePkg/Universal/EbcDxe/Arm/EbcSupport.c
index 2e308772b477..99bed1697748 100644
--- a/MdeModulePkg/Universal/EbcDxe/Arm/EbcSupport.c
+++ b/MdeModulePkg/Universal/EbcDxe/Arm/EbcSupport.c
@@ -32,6 +32,7 @@ typedef struct {
   UINT32Magic;
   UINT32EbcEntryPoint;
   UINT32EbcLlEntryPoint;
+  UINT32EbcCallSignature;
 } EBC_INSTRUCTION_BUFFER;
 #pragma pack()
 
@@ -158,7 +159,7 @@ PushU32 (
   @param  Arg3  The 3rd argument.
   @param  Arg4  The 4th argument.
   @param  InstructionBuffer A pointer to the thunk instruction buffer.
-  @param  Args5_16[]Array containing arguments #5 to #16.
+  @param  Args5To32[]   Array containing arguments #5 to #16.
 
   @return The value returned by the EBC application we're going to run.
 
@@ -171,7 +172,7 @@ EbcInterpret (
   IN UINTN  Arg3,
   IN UINTN  Arg4,
   IN EBC_INSTRUCTION_BUFFER *InstructionBuffer,
-  IN UINTN  Args5_16[]
+  IN UINTN  Args5To32[]
   )
 {
   //
@@ -179,11 +180,23 @@ EbcInterpret (
   //
   VM_CONTEXT  VmContext;
   UINTN   Addr;
+  UINT32  Mask;
   

[edk2] [PATCH 0/5] MdeModulePkg/EbcDxe: add ARM support

[Pete Batard]

(This e-mail is fairly lengthy, so an Executive Summary is provided, for 
those who don't want to go through a wall of text).


0. Executive Summary


0.1 Preamble


One of the most vexing aspect of EFI Byte Code (EBC) proposal from the 
UEFI specs is that its EDK2 implementation has somewhat fallen short of 
its implicit goal of universality, due to the non availability of an EBC 
VM for all supported architectures.


As a consequence, we feel that this has resulted in major backtracking 
on EBC, such as EBC not being made a mandatory part of UEFI firmware 
implementations (in the same way as FAT) or not having a default 
provision for EBC bootloaders (e.g. /efi/boot/bootebc.arm).


Shortly after Ard Biesheuvel provided an EBC implementation for ARM64, 
last August, questions were raised with regards to being able to do the 
same for ARM due to issues with trying to work with the calling 
convention on that platform, and more specifically, with the 64-bit 
parameter marshalling that is required there. At the time, these 
problems were deemed difficult to tackle without the collaboration of 
third parties (such as external toolchain developers) or without having 
to restrict the scope of what EBC applications could do (such as 
limiting their access to only "known" EDK2 interfaces, for which 
parameter marshalling specifics would have been added).


This series of patches attempts to remedy all that, by proposing an ARM 
EBC implementation that solves the issues mentioned above in a generic 
and entirely self contained manner (i.e. within the EDK2). With this, 
the EDK2 should finally enable the execution of the same EBC binary 
across ALL supported UEFI architectures, and thus complete the implicit 
goal of EBC.


0.2 Solution Overview
-

The gist of our marshalling solution can be summarized with being able 
to access a 16-bit value, at runtime, for native <-> EBC layer 
transition calls, that indicates which of (up to) 16 function call 
parameters is 64-bit. In turn, this enables the ARM EBC VM to "realign" 
said parameters to a 64-bit boundary as needed.


Hereafter, we will refer to these 16-bit values as a "call signatures".

Now, whereas this solution is self-contained, it does entail a minor 
change to the UEFI EBC specs, that mandates the insertion of call 
signatures at compilation time into the unused part of the 64-bit 
function pointer data object used by BREAK 5 (see 3.3).


However, this non breaking change is both backward and forward 
compatible. Especially, once the specs change is effected, and for *ALL* 
current EBC archs (IA32, IA64, X64, AARCH64):


a) EBC executables that were produced with the older version of the 
specs will run exactly as they did on EBC VMs that comply with the newer 
version of the specs


b) EBC executables that are produced with the newer version of the specs 
will run and perform the same, on EBC VMs that comply with either the 
older or newer version of the specs.


Also, and specifically for ARM (since other platforms are unaffected by 
such concerns), this whole proposal makes it possible for:


- Existing EBC binaries, that do not invoke BREAK 5 (i.e. no native to 
EBC calls) to run onto the proposed ARM EBC VM without any changes. 
Especially, these applications can perform EBC to native calls, on ARM, 
with no adverse effects.


- Existing EBC binaries, that do invoke BREAK 5, but that don't include 
signatures, to partially run on ARM. In this case, the ARM EBC VM will 
return an error status for calls issued from native to the EBC, allowing 
a native app to acknowledge incompatibility issues and potentially let 
the developer know that the adding of signature is needed.


- Existing EBC binaries, that do invoke BREAK 5, to be (relatively 
easily) patched for ARM EBC compatibility. This, for instance, is 
demonstrated with the EDK2 FAT EBC binary in 4.3.


- The updated EDK2 EBC toolchain to produce EBC applications that run on 
*ALL* EBC VMs, including the newly added ARM, as well as other VMs.


0.3 Patch Overview
--

The patch series is broken down into 5 parts:

- 1/5 relates to preliminary changes, within the common EBC code, that 
enable VmReadIndex##() functions to optionally return the decoded Const 
and Natural parts, as we need this data for our proposed Stack Tracker. 
It is done by adding an optional pointer to a {Const, Natural} struct, 
that is to be filled when the pointer is not NULL. With the introduction 
of this change, the patch sets all of the new optional pointers to NULL, 
so no actual behavioural change occurs.


- 2/5 introduces the basic ARM EBC VM, as was proposed by Ard as a PoC 
port of the ARRCH64 version. Note that this change alone is enough to 
get standalone EBC code to run on ARM, but may result in the parameter 
marshalling issues we mentioned above, for any call that transitions 
between ARM native and EBC, due to potential "misaligned" 64-bit 

[edk2] [PATCH 1/5] MdeModulePkg/EbcDxe: allow VmReadIndex##() to return a decoded index

[Pete Batard]

* The VmReadIndex## function now take an optional pointer to an index
  pair structure which, when not NULL, is filled with the decoded const
  and natural values.
* This feature is needed by the ARM EBC VM.
* For now, the new parameters is set to NULL, so as not to change
  existing behaviour with current EBC VM platforms.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Pete Batard 
---
 MdeModulePkg/Universal/EbcDxe/EbcExecute.c | 116 +
 MdeModulePkg/Universal/EbcDxe/EbcExecute.h |   8 ++
 2 files changed, 93 insertions(+), 31 deletions(-)

diff --git a/MdeModulePkg/Universal/EbcDxe/EbcExecute.c 
b/MdeModulePkg/Universal/EbcDxe/EbcExecute.c
index e5d290a2fec6..2d21c3364e0d 100644
--- a/MdeModulePkg/Universal/EbcDxe/EbcExecute.c
+++ b/MdeModulePkg/Universal/EbcDxe/EbcExecute.c
@@ -61,6 +61,8 @@ UINT64
   @param  VmPtr A pointer to VM context.
   @param  CodeOffsetOffset from IP of the location of the 16-bit index
 to decode.
+  @param  IndexPtr  An optional pointer where the decoded index pair
+values can be written.
 
   @return The decoded offset.
 
@@ -68,7 +70,8 @@ UINT64
 INT16
 VmReadIndex16 (
   IN VM_CONTEXT *VmPtr,
-  IN UINT32 CodeOffset
+  IN UINT32 CodeOffset,
+  OUT EBC_INDEX *IndexPtr OPTIONAL
   );
 
 /**
@@ -77,6 +80,8 @@ VmReadIndex16 (
   @param  VmPtr A pointer to VM context.
   @param  CodeOffsetOffset from IP of the location of the 32-bit index
 to decode.
+  @param  IndexPtr  An optional pointer where the decoded index pair
+values can be written.
 
   @return Converted index per EBC VM specification.
 
@@ -84,7 +89,8 @@ VmReadIndex16 (
 INT32
 VmReadIndex32 (
   IN VM_CONTEXT *VmPtr,
-  IN UINT32 CodeOffset
+  IN UINT32 CodeOffset,
+  OUT EBC_INDEX *IndexPtr OPTIONAL
   );
 
 /**
@@ -93,6 +99,8 @@ VmReadIndex32 (
   @param  VmPtr A pointer to VM context.s
   @param  CodeOffsetOffset from IP of the location of the 64-bit index
 to decode.
+  @param  IndexPtr  An optional pointer where the decoded index pair
+values can be written.
 
   @return Converted index per EBC VM specification
 
@@ -100,7 +108,8 @@ VmReadIndex32 (
 INT64
 VmReadIndex64 (
   IN VM_CONTEXT *VmPtr,
-  IN UINT32 CodeOffset
+  IN UINT32 CodeOffset,
+  OUT EBC_INDEX *IndexPtr OPTIONAL
   );
 
 /**
@@ -1600,13 +1609,13 @@ ExecuteMOVxx (
   // Get one or both index values.
   //
   if ((Opcode & OPCODE_M_IMMED_OP1) != 0) {
-Index16 = VmReadIndex16 (VmPtr, 2);
+Index16 = VmReadIndex16 (VmPtr, 2, NULL);
 Index64Op1  = (INT64) Index16;
 Size += sizeof (UINT16);
   }
 
   if ((Opcode & OPCODE_M_IMMED_OP2) != 0) {
-Index16 = VmReadIndex16 (VmPtr, Size);
+Index16 = VmReadIndex16 (VmPtr, Size, NULL);
 Index64Op2  = (INT64) Index16;
 Size += sizeof (UINT16);
   }
@@ -1615,13 +1624,13 @@ ExecuteMOVxx (
   // MOVBD, MOVWD, MOVDD, MOVQD, and MOVND have 32-bit immediate index
   //
   if ((Opcode & OPCODE_M_IMMED_OP1) != 0) {
-Index32 = VmReadIndex32 (VmPtr, 2);
+Index32 = VmReadIndex32 (VmPtr, 2, NULL);
 Index64Op1  = (INT64) Index32;
 Size += sizeof (UINT32);
   }
 
   if ((Opcode & OPCODE_M_IMMED_OP2) != 0) {
-Index32 = VmReadIndex32 (VmPtr, Size);
+Index32 = VmReadIndex32 (VmPtr, Size, NULL);
 Index64Op2  = (INT64) Index32;
 Size += sizeof (UINT32);
   }
@@ -1630,12 +1639,12 @@ ExecuteMOVxx (
   // MOVqq -- only form with a 64-bit index
   //
   if ((Opcode & OPCODE_M_IMMED_OP1) != 0) {
-Index64Op1 = VmReadIndex64 (VmPtr, 2);
+Index64Op1 = VmReadIndex64 (VmPtr, 2, NULL);
 Size += sizeof (UINT64);
   }
 
   if ((Opcode & OPCODE_M_IMMED_OP2) != 0) {
-Index64Op2 = VmReadIndex64 (VmPtr, Size);
+Index64Op2 = VmReadIndex64 (VmPtr, Size, NULL);
 Size += sizeof (UINT64);
   }
 } else {
@@ -2042,7 +2051,7 @@ ExecuteJMP (
   //
   if ((Opcode & OPCODE_M_IMMDATA) != 0) {
 if (OPERAND1_INDIRECT (Operand)) {
-  Index32 = VmReadIndex32 (VmPtr, 2);
+  Index32 = VmReadIndex32 (VmPtr, 2, NULL);
 } else {
   Index32 = VmReadImmed32 (VmPtr, 2);
 }
@@ -2210,7 +2219,7 @@ ExecuteMOVI (
   // Get the index (16-bit) if present
   //
   if ((Operands & MOVI_M_IMMDATA) != 0) {
-Index16 = VmReadIndex16 (VmPtr, 2);
+Index16 = VmReadIndex16 (VmPtr, 2, NULL);
 Size= 4;
   } else {
 Index16 = 0;
@@ -2329,7 +2338,7 @@ ExecuteMOVIn (
   // Get the operand1 index (16-bit) if present
   //
   if ((Operands & MOVI_M_IMMDATA) != 0) {
-Index16 = 

[edk2] [PATCH 2/5] MdeModulePkg/EbcDxe: add ARM support

[Pete Batard]

From: Ard Biesheuvel 

* This is a port of the AARCH64 implementation of the EBC runtime to ARM.
* Note that, on its own, this patch only allows running self contained EBC
  applications, such as ones that don't issue calls into the native
  platform, or that aren't being called from native.
* Support for EBC -> native and native -> EBC calls will be added in
  subsequent patches.
* Because EFI32 may not be defined for ARM compilation, this patch also
  alters the EBC Debugger to use MDE_CPU_### macros for address display.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel 
Signed-off-by: Pete Batard 
---
 ArmVirtPkg/ArmVirt.dsc.inc |   6 +-
 ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc   |  10 +-
 ArmVirtPkg/ArmVirtXen.fdf  |  10 +-
 MdeModulePkg/MdeModulePkg.dsc  |   4 +-
 MdeModulePkg/Universal/EbcDxe/Arm/EbcLowLevel.S| 147 +++
 MdeModulePkg/Universal/EbcDxe/Arm/EbcSupport.c | 470 +
 MdeModulePkg/Universal/EbcDxe/EbcDebugger.inf  |   6 +-
 .../EbcDxe/EbcDebugger/EdbDisasmSupport.h  |   4 +-
 .../Universal/EbcDxe/EbcDebuggerConfig.inf |   2 +-
 MdeModulePkg/Universal/EbcDxe/EbcDxe.inf   |   6 +-
 10 files changed, 648 insertions(+), 17 deletions(-)
 create mode 100644 MdeModulePkg/Universal/EbcDxe/Arm/EbcLowLevel.S
 create mode 100644 MdeModulePkg/Universal/EbcDxe/Arm/EbcSupport.c

diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
index dbd6678accde..3fa016b501c5 100644
--- a/ArmVirtPkg/ArmVirt.dsc.inc
+++ b/ArmVirtPkg/ArmVirt.dsc.inc
@@ -402,6 +402,11 @@ [Components.common]
   MdeModulePkg/Universal/Disk/RamDiskDxe/RamDiskDxe.inf
 
   #
+  # EBC support
+  #
+  MdeModulePkg/Universal/EbcDxe/EbcDxe.inf
+
+  #
   # UEFI application (Shell Embedded Boot Loader)
   #
   ShellPkg/Application/Shell/Shell.inf {
@@ -430,4 +435,3 @@ [Components.AARCH64]
   # ACPI Support
   #
   MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf
-  MdeModulePkg/Universal/EbcDxe/EbcDxe.inf
diff --git a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc 
b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
index cc5d12aaefea..5145a86a0f33 100644
--- a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
+++ b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
@@ -140,6 +140,11 @@ [FV.FvMain]
   INF MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf
   INF OvmfPkg/SmbiosPlatformDxe/SmbiosPlatformDxe.inf
 
+  #
+  # EBC support
+  #
+  INF MdeModulePkg/Universal/EbcDxe/EbcDxe.inf
+
 !if $(ARCH) == AARCH64
   #
   # ACPI Support
@@ -147,11 +152,6 @@ [FV.FvMain]
   INF MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf
   INF 
MdeModulePkg/Universal/Acpi/BootGraphicsResourceTableDxe/BootGraphicsResourceTableDxe.inf
   INF OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpiPlatformDxe.inf
-
-  #
-  # EBC support
-  #
-  INF MdeModulePkg/Universal/EbcDxe/EbcDxe.inf
 !endif
 
   #
diff --git a/ArmVirtPkg/ArmVirtXen.fdf b/ArmVirtPkg/ArmVirtXen.fdf
index c997251b12b8..ccbd8baa5f82 100644
--- a/ArmVirtPkg/ArmVirtXen.fdf
+++ b/ArmVirtPkg/ArmVirtXen.fdf
@@ -170,6 +170,11 @@ [FV.FvMain]
   INF ShellPkg/Application/Shell/Shell.inf
 
   #
+  # EBC support
+  #
+  INF MdeModulePkg/Universal/EbcDxe/EbcDxe.inf
+
+  #
   # Bds
   #
   INF MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf
@@ -186,11 +191,6 @@ [FV.FvMain]
 !if $(ARCH) == AARCH64
   INF MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf
   INF ArmVirtPkg/XenAcpiPlatformDxe/XenAcpiPlatformDxe.inf
-
-  #
-  # EBC support
-  #
-  INF MdeModulePkg/Universal/EbcDxe/EbcDxe.inf
 !endif
 
  #
diff --git a/MdeModulePkg/MdeModulePkg.dsc b/MdeModulePkg/MdeModulePkg.dsc
index 5996fe50f680..35094bab9e9a 100644
--- a/MdeModulePkg/MdeModulePkg.dsc
+++ b/MdeModulePkg/MdeModulePkg.dsc
@@ -430,8 +430,10 @@ [Components]
   MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleLib.inf
   MdeModulePkg/Library/DxeCapsuleLibFmp/DxeRuntimeCapsuleLib.inf
 
-[Components.IA32, Components.X64, Components.IPF, Components.AARCH64]
+[Components.IA32, Components.X64, Components.IPF]
   MdeModulePkg/Universal/Network/UefiPxeBcDxe/UefiPxeBcDxe.inf
+
+[Components.IA32, Components.X64, Components.IPF, Components.ARM, 
Components.AARCH64]
   MdeModulePkg/Universal/DebugSupportDxe/DebugSupportDxe.inf
   MdeModulePkg/Universal/EbcDxe/EbcDxe.inf
   MdeModulePkg/Universal/EbcDxe/EbcDebugger.inf
diff --git a/MdeModulePkg/Universal/EbcDxe/Arm/EbcLowLevel.S 
b/MdeModulePkg/Universal/EbcDxe/Arm/EbcLowLevel.S
new file mode 100644
index ..cb3c11f71673
--- /dev/null
+++ b/MdeModulePkg/Universal/EbcDxe/Arm/EbcLowLevel.S
@@ -0,0 +1,147 @@
+///** @file
+//
+//  This code provides low level routines that support the Virtual Machine
+//  for option ROMs.
+//
+//  Copyright (c) 2016, Linaro, Ltd. All rights reserved.
+//  Copyright (c) 2015, The Linux Foundation. All rights reserved.
+//  Copyright (c) 2007 - 2014, Intel 

[edk2] [PATCH 3/5] MdeModulePkg/EbcDxe: add a stack tracker for ARM EBC->native support

[Pete Batard]

* This patch fixes the CALLEX to native vs ARM calling convention issue by
  tracking whether each potential call argument is a natural or a 64-bit
  value.
* This is accomplished by monitoring stack pointer operations, including
  arithmetic or single stack buffer reallocation, through a separate stack
  tracker. The stack tracker is then used, at calltime to determine 64-bit
  arguments that need to be aligned.
* While currently ARM specific, the stack tracking is added in a generic
  manner, so that any future platform that requires a similar mechanism may
  do so with minimal changes.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Pete Batard 
---
 MdeModulePkg/Include/Protocol/EbcVmTest.h  |   2 +
 MdeModulePkg/Universal/EbcDxe/Arm/EbcLowLevel.S| 121 ++--
 .../Universal/EbcDxe/Arm/EbcStackTracker.c | 634 +
 MdeModulePkg/Universal/EbcDxe/Arm/EbcSupport.c | 124 +++-
 MdeModulePkg/Universal/EbcDxe/EbcDebugger.inf  |   4 +
 MdeModulePkg/Universal/EbcDxe/EbcDxe.inf   |   4 +
 MdeModulePkg/Universal/EbcDxe/EbcExecute.c | 155 -
 MdeModulePkg/Universal/EbcDxe/EbcStackTracker.c|  65 +++
 8 files changed, 1038 insertions(+), 71 deletions(-)
 create mode 100644 MdeModulePkg/Universal/EbcDxe/Arm/EbcStackTracker.c
 create mode 100644 MdeModulePkg/Universal/EbcDxe/EbcStackTracker.c

diff --git a/MdeModulePkg/Include/Protocol/EbcVmTest.h 
b/MdeModulePkg/Include/Protocol/EbcVmTest.h
index 9eedca1906a2..12d5e5217059 100644
--- a/MdeModulePkg/Include/Protocol/EbcVmTest.h
+++ b/MdeModulePkg/Include/Protocol/EbcVmTest.h
@@ -111,6 +111,8 @@ typedef struct {
   UINTN ImageBase;
   VOID  *StackPool;
   VOID  *StackTop;
+  VOID  *StackTracker;  ///< pointer to an optional, 
opaque and arch-specific
+///  structure, which may be used 
to track stack ops.
 } VM_CONTEXT;
 
 /**
diff --git a/MdeModulePkg/Universal/EbcDxe/Arm/EbcLowLevel.S 
b/MdeModulePkg/Universal/EbcDxe/Arm/EbcLowLevel.S
index cb3c11f71673..dc9c3946ced2 100644
--- a/MdeModulePkg/Universal/EbcDxe/Arm/EbcLowLevel.S
+++ b/MdeModulePkg/Universal/EbcDxe/Arm/EbcLowLevel.S
@@ -3,6 +3,7 @@
 //  This code provides low level routines that support the Virtual Machine
 //  for option ROMs.
 //
+//  Copyright (c) 2016, Pete Batard. All rights reserved.
 //  Copyright (c) 2016, Linaro, Ltd. All rights reserved.
 //  Copyright (c) 2015, The Linux Foundation. All rights reserved.
 //  Copyright (c) 2007 - 2014, Intel Corporation. All rights reserved.
@@ -20,11 +21,11 @@
 .thumb
 .syntax unified
 
-ASM_GLOBAL ASM_PFX(EbcLLCALLEXNative)
+ASM_GLOBAL ASM_PFX(EbcLLCALLEXNativeArm)
 ASM_GLOBAL ASM_PFX(EbcLLEbcInterpret)
 ASM_GLOBAL ASM_PFX(EbcLLExecuteEbcImageEntryPoint)
 
-INTERWORK_FUNC(EbcLLCALLEXNative)
+INTERWORK_FUNC(EbcLLCALLEXNativeArm)
 INTERWORK_FUNC(EbcLLEbcInterpret)
 INTERWORK_FUNC(EbcLLExecuteEbcImageEntryPoint)
 
@@ -34,62 +35,96 @@ ASM_GLOBAL ASM_PFX(mEbcInstructionBufferTemplate)
 // EbcLLCALLEX
 //
 // This function is called to execute an EBC CALLEX instruction.
-// This instruction requires that we thunk out to external native
-// code. For ARM, we copy the VM stack into the main stack and then pop
-// the first 4 arguments off according to the ARM Procedure Call Standard
-// On return, we restore the stack pointer to its original location.
+// This instruction requires that we thunk out to external native code.
+// Note that due to the ARM Procedure Call Standard, we may have to align
+// 64-bit arguments to an even register or dword aligned stack address,
+// which is what the extra ArgLayout parameter is used for.
+// Also, to optimize for speed, we arbitrarily limit to 16 the maximum
+// number of arguments a native call can have.
 //
 //
-// UINTN EbcLLCALLEXNative(UINTN FuncAddr, UINTN NewStackPointer, VOID 
*FramePtr)
-ASM_PFX(EbcLLCALLEXNative):
-mov ip, r0  // Preserve r0
+// UINTN EbcLLCALLEXNativeArm(UINTN FuncAddr, UINTN NewStackPointer,
+//VOID *FramePtr, UINT16 ArgLayout)
+ASM_PFX(EbcLLCALLEXNativeArm):
+
+mov ip, r1  // Use ip as our argument walker
+push{r0, r4-r6}
+mov r4, r2  // Keep a copy of FramePtr
+mov r5, r3  // Keep a copy of ArgLayout
+mov r6, #2  // Arg counter (2 for r0 and r2)
 
 //
-// If the EBC stack frame is smaller than or equal to 16 bytes, we know 
there
-// are no stacked arguments #5 and beyond that we need to copy to the 
native
-// stack. In this case, we can perform a tail call which is much more
-// efficient, since there is no need to touch the native stack at all.
+// Process the register arguments, skipping r1 and r3
+// as needed, according to the 

Re: [edk2] [PATCH v2] ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe: Fixed crash on Juno R0

[Ryan Harkin]

On 24 January 2017 at 02:01, Daniil Egranov  wrote:
> The Marvell Yukon MAC address load supported only on Juno R1 and R2.
> It disabled for Juno R0 due to PCI issues on this board.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Daniil Egranov 

Tested-by: Ryan Harkin 

> ---
> Changelog:
>
> v2
> Replaced ASSERT with the error message in case Marvell MAC address
> set has failed
>

^ Thanks for doing that, it's much more usable in my setup now.


>  ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c | 11 +--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c 
> b/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
> index 47ff587..0193b98 100644
> --- a/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
> +++ b/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
> @@ -378,6 +378,7 @@ OnEndOfDxe (
>EFI_DEVICE_PATH_PROTOCOL* PciRootComplexDevicePath;
>EFI_HANDLEHandle;
>EFI_STATUSStatus;
> +  UINT32JunoRevision;
>
>//
>// PCI Root Complex initialization
> @@ -393,8 +394,14 @@ OnEndOfDxe (
>Status = gBS->ConnectController (Handle, NULL, PciRootComplexDevicePath, 
> FALSE);
>ASSERT_EFI_ERROR (Status);
>
> -  Status = ArmJunoSetNicMacAddress ();
> -  ASSERT_EFI_ERROR (Status);
> +  GetJunoRevision (JunoRevision);
> +
> +  if (JunoRevision != JUNO_REVISION_R0) {
> +Status = ArmJunoSetNicMacAddress ();
> +if (EFI_ERROR (Status)) {
> +  DEBUG ((DEBUG_ERROR, "ArmJunoDxe: Failed to set Marvell Yukon NIC MAC 
> address\n"));
> +}
> +  }
>  }
>
>  STATIC
> --
> 2.7.4
>
> ___
> 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] ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe: Fixed crash on Juno R0

[Ryan Harkin]

On 24 January 2017 at 02:19, Daniil Egranov  wrote:
> Hi Ryan,
>
>
>
> On 01/23/2017 06:56 AM, Ryan Harkin wrote:
>>
>> On 20 January 2017 at 20:57, Daniil Egranov 
>> wrote:
>>>
>>> Hi Ryan,
>>>
>>>
>>> On 01/20/2017 04:30 AM, Ryan Harkin wrote:
>>>
>>> On 20 January 2017 at 01:34, Daniil Egranov 
>>> wrote:
>>>
>>> Hi Leif, Ryan
>>>
>>>
>>> On 01/19/2017 09:13 AM, Leif Lindholm wrote:
>>>
>>> On Thu, Jan 19, 2017 at 01:49:04PM +, Ryan Harkin wrote:
>>>
>>> On 18 January 2017 at 23:27, Daniil Egranov 
>>> wrote:
>>>
>>> The Marvell Yukon MAC address load supported only on Juno R1 and R2.
>>> It disabled for Juno R0 due to PCI issues on this board.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Daniil Egranov 
>>>
>>> Tested-by: Ryan Harkin 
>>>
>>> ---
>>>   ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c | 9 +++--
>>>   1 file changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
>>> b/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
>>> index 47ff587..e9e6990 100644
>>> --- a/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
>>> +++ b/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c
>>> @@ -378,6 +378,7 @@ OnEndOfDxe (
>>> EFI_DEVICE_PATH_PROTOCOL* PciRootComplexDevicePath;
>>> EFI_HANDLEHandle;
>>> EFI_STATUSStatus;
>>> +  UINT32JunoRevision;
>>>
>>> //
>>> // PCI Root Complex initialization
>>> @@ -393,8 +394,12 @@ OnEndOfDxe (
>>> Status = gBS->ConnectController (Handle, NULL,
>>> PciRootComplexDevicePath,
>>> FALSE);
>>> ASSERT_EFI_ERROR (Status);
>>>
>>> -  Status = ArmJunoSetNicMacAddress ();
>>> -  ASSERT_EFI_ERROR (Status);
>>> +  GetJunoRevision (JunoRevision);
>>> +
>>> +  if (JunoRevision != JUNO_REVISION_R0) {
>>> +Status = ArmJunoSetNicMacAddress ();
>>> +ASSERT_EFI_ERROR (Status);
>>>
>>> This is just an FYI, but I stacked your patch on top of mainline, like
>>> this:
>>>
>>> 5f81f61  2017-01-18  ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe:
>>> Fixed crash on Juno R0   [Daniil Egranov]
>>> 19ca06b  2017-01-19  OvmfPkg: Remove superfluous return statements.
>>>   [Thomas Huth]
>>>
>>> The first time I ran this, Juno R0 worked fine, but on R1 and R2, the
>>> assert triggered:
>>>
>>> UEFI firmware (version 5f81f61 built at 11:56:52 on Jan 19 2017)
>>> [snip]
>>> ASSERT_EFI_ERROR (Status = Not Found)
>>> ASSERT [ArmJunoDxe]
>>>
>>> /linaro/platforms/uefi/edk2/ArmPlatformPkg/ArmJunoPkg/Drivers/ArmJunoDxe/ArmJunoDxe.c(401):
>>> !EFI_ERROR (Status)
>>>
>>> I worked out what is happening. And it's not to do with this patch.
>>> It's another fall-out from the re-work you did to the previous patch.
>>> It's also ultimately due to a bug the firmware.
>>>
>>> With the initial version of your "Set Marvell Yukon MAC address"
>>> patch, this hang didn't happen. I suspect that was because your error
>>> checking was weaker and certain PCIe failures didn't trigger the
>>> assert.
>>>
>>> To reproduce the error with this commit:
>>> 1) power on and boot R1 or R2 into Shell
>>>I do this by interrupting the boot by pressing ESCAPE and using the
>>> boot
>>> menu
>>> 2) At the Shell prompt, run "reset -s" to shutdown
>>> 3) At the ARM Boot Loader "Cmd>" prompt, run "reboot"
>>> 4) the board will hang while booting UEFI, assuming the board firmware
>>> doesn't die with constant messages like this:
>>>
>>>  ERROR: PCIe CSR read failed to respond
>>>  ERROR: SMBus transaction not claimed
>>>
>>> Assuming the problem is firmware, not EDK2, what should we do about it?
>>>
>>> OK, so instinctively, my reaction was that "the reset -s bug is a
>>> system controller firmware bug and we shouldn't work around
>>> it". However, since it is actually disrupting Ryan's workflow, which
>>> frequently doesn't touch PCI at all, I think downgrading the ASSERT to
>>> an error message is a good idea short-term.
>>>
>>> Daniil - could you make that change please?
>>>
>>> /
>>>  Leif
>>>
>>>
>>> I've been able to reproduce "PCIe CSR read failed to respond" and "SMBus
>>> transaction not claimed" errors on my Juno R2. I disabled Marvell Yukon
>>> driver (.dsc/.fdf) and removed ArmJunoDxe patch but still see the same
>>> error
>>> messages during the initial boot.
>>>
>>> Testing motherboard interfaces (FPGA build 118)...
>>> SRAM 32MB test: PASSED
>>> LAN9118   test: PASSED
>>> KMI1/2test: PASSED
>>> MMC   test: PASSED
>>> PB/LEDs   test: PASSED
>>> FPGA UART test: PASSED
>>> ERROR: PCIe CSR read failed to respond
>>> ERROR: SMBus transaction not claimed
>>> ERROR: PCIe CSR read failed to respond
>>> ...
>>>
>>> Once it went through reporting these errors, the UEFI starts loading but
>>> still fails in OnEndOfDxe():
>>> 

Re: [edk2] [PATCH 1/6] MdeModulePkg: Refine type cast for pointer subtraction

[Laszlo Ersek]

On 01/24/17 08:50, Hao Wu wrote:
> For pointer subtraction, the result is of type "ptrdiff_t". According to
> the C11 spec, ptrdiff_t is a signed integer type but its size is
> implementation-defined.
> 
> There are cases that the result of pointer subtraction is type casted to
> UINTN, like:
> 
> UINT8  *Ptr1, *Ptr2;
> UINTN  PtrDiff;
> ...
> PtrDiff = (UINTN) (Ptr1 - Ptr2);
> 
> Some static code checkers may warn that the pointer subtraction might
> overflow first and then the result is being cast to a bigger size.
> 
> The commit will cast each pointer to UINTN first and then perform the
> subtraction:
> 
> PtrDiff = (UINTN) Ptr1 - (UINTN) Ptr2;

The main point here is that pointer subtraction is only allowed between
pointers to elements of the same (single-dimensional) array. For the
purposes of this requirement, the subtrahend and/or the minuend can
point one past the last element in the array (but such a pointer cannot
be dereferenced). Also, a non-array object is treated identically to an
array with 1 element.

In the above permitted (defined) cases, no overflow will ever occur.
That leaves us with the following concerns:

- The well-defined difference (of type ptrdiff_t) might be negative;
casting that to UINTN probably doesn't have the intended result. This
can be excluded by verifying (manually) that the minuend pointer always
points past the subtrahend pointer.

- If the difference is not defined (because the above requirement is not
satisfied), then the pointers should indeed be converted to some integer
type before the subtraction. This raises further questions:

  - if the integer type we choose is signed, then the subtraction
(between the signed integers) could underflow, which would be
undefined.

  - if the integer type we choose is unsigned (and has at least the
rank of int), then the subtraction could underflow with
well-defined behavior, but the result would still be non-intuitive.

Considering our implementation details, casting both operands to UINTN
and then doing the subtraction seems fine, as long as we can ensure that
the subtraction doesn't wrap around (in order to prevent non-intuitive
results).

... I wrote up this wall of text only to suggest a more precise commit
message. If you have access to the C11 std (or a late enough draft of
it), then it's best to quote the standard directly. My point is that in
the following example,

  UINT8 Blah[2][2];
  UINTN Diff;

  Diff = [1][1] - [0][0];

there is no underflow of the specific kind that the commit message
suggests, but the subtraction is still undefined, because the operands
don't point into (or one past) the same array. The minuend points into
the Blah[1] array, while the subtrahend points into the Blah[0] array.

Not volunteering to review this patch though :)

Thanks
Laszlo


> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Hao Wu 
> ---
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c   |  4 ++--
>  MdeModulePkg/Include/Library/NetLib.h  |  6 +++---
>  MdeModulePkg/Library/DxeCapsuleLibFmp/DxeCapsuleLib.c  |  4 ++--
>  MdeModulePkg/Library/DxePrintLibPrint2Protocol/PrintLib.c  |  2 +-
>  MdeModulePkg/Library/FileExplorerLib/FileExplorer.c|  6 +++---
>  MdeModulePkg/Library/PiDxeS3BootScriptLib/BootScriptSave.c |  4 ++--
>  MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c |  4 ++--
>  MdeModulePkg/Universal/DebugPortDxe/DebugPort.c|  4 ++--
>  .../Universal/FaultTolerantWriteDxe/FaultTolerantWrite.c   |  4 ++--
>  MdeModulePkg/Universal/HiiDatabaseDxe/Image.c  |  4 ++--
>  MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c  | 10 
> +-
>  11 files changed, 26 insertions(+), 26 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c 
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
> index 2bc4f8c..d2ad94e 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
> @@ -1,7 +1,7 @@
>  /** @file
>PCI Rom supporting funtions implementation for PCI Bus module.
>  
> -Copyright (c) 2006 - 2015, Intel Corporation. All rights reserved.
> +Copyright (c) 2006 - 2017, 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.  The full text of the license may be 
> found at
> @@ -776,7 +776,7 @@ ProcessOpRomImage (
>  NextImage:
>  RomBarOffset += ImageSize;
>  
> -  } while (((Indicator & 0x80) == 0x00) && ((UINTN) (RomBarOffset - (UINT8 
> *) RomBar) < PciDevice->RomSize));
> +  } while (((Indicator & 0x80) == 0x00) && (((UINTN) RomBarOffset - (UINTN) 
> RomBar) < PciDevice->RomSize));
>  
>return RetStatus;
>  }
> diff --git a/MdeModulePkg/Include/Library/NetLib.h 
> 

[edk2] [PATCH] SecurityPkg HashLibRouter: Avoid incorrect PcdTcg2HashAlgorithmBitmap

[Star Zeng]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=244

Currently, when software HashLib (HashLibBaseCryptoRouter) and related
HashInstanceLib instances are used, PcdTcg2HashAlgorithmBitmap is
expected to be configured to 0 in platform dsc.
But PcdTcg2HashAlgorithmBitmap has default value 0x in
SecurityPkg.dec, and some platforms forget to configure it to 0 or
still configure it to 0x in platform dsc, that will make final
PcdTcg2HashAlgorithmBitmap value incorrect.

This patch is to add CONSTRUCTOR in HashLib (HashLibBaseCryptoRouter)
and PcdTcg2HashAlgorithmBitmap will be set to 0 in the CONSTRUCTOR.

Current HASH_LIB_PEI_ROUTER_GUID HOB created in
HashLibBaseCryptoRouterPei is shared between modules that links
HashLibBaseCryptoRouterPei.
To avoid mutual interference, separated HASH_LIB_PEI_ROUTER_GUID HOBs
with gEfiCallerIdGuid Identifier will be created for those modules.

This patch is also to add check in HashLib (HashLibBaseCryptoRouter)
for the mismatch of supported HashMask between modules that may link
different HashInstanceLib instances, warning will be reported if
mismatch is found.

Cc: Yao Jiewen 
Cc: Chao Zhang 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Star Zeng 
---
 .../HashLibBaseCryptoRouterDxe.c   |  80 -
 .../HashLibBaseCryptoRouterDxe.inf |   3 +-
 .../HashLibBaseCryptoRouterPei.c   | 190 +
 .../HashLibBaseCryptoRouterPei.inf |   8 +-
 SecurityPkg/SecurityPkg.dec|   4 +
 SecurityPkg/SecurityPkg.uni|   8 +-
 6 files changed, 254 insertions(+), 39 deletions(-)

diff --git 
a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterDxe.c 
b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterDxe.c
index 3250c3a01a0c..4775cfee2d7a 100644
--- a/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterDxe.c
+++ b/SecurityPkg/Library/HashLibBaseCryptoRouter/HashLibBaseCryptoRouterDxe.c
@@ -3,7 +3,7 @@
   hash handler registerd, such as SHA1, SHA256.
   Platform can use PcdTpm2HashMask to mask some hash engines.
 
-Copyright (c) 2013 - 2016, Intel Corporation. All rights reserved. 
+Copyright (c) 2013 - 2017, 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.  The full text of the license may be 
found at
@@ -28,6 +28,30 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
 HASH_INTERFACE   mHashInterface[HASH_COUNT] = {{{0}, NULL, NULL, NULL}};
 UINTNmHashInterfaceCount = 0;
 
+UINT32   mSupportedHashMaskLast = 0;
+UINT32   mSupportedHashMaskCurrent = 0;
+
+/**
+  Check mismatch of supported HashMask between modules
+  that may link different HashInstanceLib instances.
+
+**/
+VOID
+CheckSupportedHashMaskMismatch (
+  VOID
+  )
+{
+  if (mSupportedHashMaskCurrent != mSupportedHashMaskLast) {
+DEBUG ((
+  DEBUG_WARN,
+  "WARNING: There is mismatch of supported HashMask (0x%x - 0x%x) between 
modules\n",
+  mSupportedHashMaskCurrent,
+  mSupportedHashMaskLast
+  ));
+DEBUG ((DEBUG_WARN, "that are linking different HashInstanceLib 
instances!\n"));
+  }
+}
+
 /**
   Start hash sequence.
 
@@ -50,6 +74,8 @@ HashStart (
 return EFI_UNSUPPORTED;
   }
 
+  CheckSupportedHashMaskMismatch ();
+
   HashCtx = AllocatePool (sizeof(*HashCtx) * mHashInterfaceCount);
   ASSERT (HashCtx != NULL);
 
@@ -90,6 +116,8 @@ HashUpdate (
 return EFI_UNSUPPORTED;
   }
 
+  CheckSupportedHashMaskMismatch ();
+
   HashCtx = (HASH_HANDLE *)HashHandle;
 
   for (Index = 0; Index < mHashInterfaceCount; Index++) {
@@ -133,6 +161,8 @@ HashCompleteAndExtend (
 return EFI_UNSUPPORTED;
   }
 
+  CheckSupportedHashMaskMismatch ();
+
   HashCtx = (HASH_HANDLE *)HashHandle;
   ZeroMem (DigestList, sizeof(*DigestList));
 
@@ -180,6 +210,8 @@ HashAndExtend (
 return EFI_UNSUPPORTED;
   }
 
+  CheckSupportedHashMaskMismatch ();
+
   HashStart ();
   HashUpdate (HashHandle, DataToHash, DataToHashLen);
   Status = HashCompleteAndExtend (HashHandle, PcrIndex, NULL, 0, DigestList);
@@ -204,7 +236,6 @@ RegisterHashInterfaceLib (
 {
   UINTN  Index;
   UINT32 HashMask;
-  UINT32 BiosSupportedHashMask;
   EFI_STATUS Status;
 
   //
@@ -218,21 +249,58 @@ RegisterHashInterfaceLib (
   if (mHashInterfaceCount >= sizeof(mHashInterface)/sizeof(mHashInterface[0])) 
{
 return EFI_OUT_OF_RESOURCES;
   }
-  BiosSupportedHashMask = PcdGet32 (PcdTcg2HashAlgorithmBitmap);
-  Status = PcdSet32S (PcdTcg2HashAlgorithmBitmap, BiosSupportedHashMask | 
HashMask);
-  ASSERT_EFI_ERROR (Status);
 
   //
   // Check duplication
   //
   for (Index = 0; Index < 

Re: [edk2] [PATCH v2 1/1] MdePkg: Refine casting expression result to bigger size

[Laszlo Ersek]

On 01/24/17 08:25, Hao Wu wrote:
> There are cases that the operands of an expression are all with rank less
> than UINT64/INT64 and the result of the expression is explicitly casted to
> UINT64/INT64 to fit the target size.
> 
> An example will be:
> UINT32 a,b;
> // a and b can be any unsigned int type with rank less than UINT64, like
> // UINT8, UINT16, etc.
> UINT64 c;
> c = (UINT64) (a + b);
> 
> Some static code checkers may warn that the expression result might
> overflow within the rank of "int" (integer promotions) and the result is
> then cast to a bigger size.
> 
> The commit refines codes by the following rules:
> 1). When the expression will not overflow within the rank of "int", remove
> the explicit type casts:
> c = a + b;
> 
> 2). When the expression is possible to overflow the range of unsigned int/
> int:
> c = (UINT64)a + b;
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Hao Wu 
> ---
>  MdePkg/Library/BaseLib/String.c  |  4 ++--
>  MdePkg/Library/BasePeCoffLib/BasePeCoff.c| 12 
> +---
>  MdePkg/Library/BaseS3PciLib/S3PciLib.c   |  4 ++--
>  MdePkg/Library/SmmMemoryAllocationLib/MemoryAllocationLib.c  |  4 ++--
>  MdePkg/Library/UefiMemoryAllocationLib/MemoryAllocationLib.c |  4 ++--
>  5 files changed, 13 insertions(+), 15 deletions(-)
> 
> diff --git a/MdePkg/Library/BaseLib/String.c b/MdePkg/Library/BaseLib/String.c
> index e84bf50..4151e0e 100644
> --- a/MdePkg/Library/BaseLib/String.c
> +++ b/MdePkg/Library/BaseLib/String.c
> @@ -586,7 +586,7 @@ InternalHexCharToUintn (
>  return Char - L'0';
>}
>  
> -  return (UINTN) (10 + InternalCharToUpper (Char) - L'A');
> +  return (10 + InternalCharToUpper (Char) - L'A');
>  }
>  
>  /**
> @@ -1211,7 +1211,7 @@ InternalAsciiHexCharToUintn (
>  return Char - '0';
>}
>  
> -  return (UINTN) (10 + InternalBaseLibAsciiToUpper (Char) - 'A');
> +  return (10 + InternalBaseLibAsciiToUpper (Char) - 'A');
>  }
>  
>  
> diff --git a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c 
> b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> index 33cad23..8d1daba 100644
> --- a/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> +++ b/MdePkg/Library/BasePeCoffLib/BasePeCoff.c
> @@ -15,7 +15,7 @@
>PeCoffLoaderGetPeHeader() routine will do basic check for PE/COFF header.
>PeCoffLoaderGetImageInfo() routine will do basic check for whole PE/COFF 
> image.
>  
> -  Copyright (c) 2006 - 2014, Intel Corporation. All rights reserved.
> +  Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.
>Portions copyright (c) 2008 - 2009, Apple Inc. All rights reserved.
>This program and the accompanying materials
>are licensed and made available under the terms and conditions of the BSD 
> License
> @@ -703,12 +703,10 @@ PeCoffLoaderGetImageInfo (
>//
>DebugDirectoryEntryFileOffset = 0;
>  
> -  SectionHeaderOffset = (UINTN)(
> -   ImageContext->PeCoffHeaderOffset +
> -   sizeof (UINT32) +
> -   sizeof (EFI_IMAGE_FILE_HEADER) +
> -   Hdr.Pe32->FileHeader.SizeOfOptionalHeader
> -   );
> +  SectionHeaderOffset = ImageContext->PeCoffHeaderOffset +
> +sizeof (UINT32) +
> +sizeof (EFI_IMAGE_FILE_HEADER) +
> +Hdr.Pe32->FileHeader.SizeOfOptionalHeader;
>  
>for (Index = 0; Index < Hdr.Pe32->FileHeader.NumberOfSections; 
> Index++) {
>  //
> diff --git a/MdePkg/Library/BaseS3PciLib/S3PciLib.c 
> b/MdePkg/Library/BaseS3PciLib/S3PciLib.c
> index e29f7fe..27342b0 100644
> --- a/MdePkg/Library/BaseS3PciLib/S3PciLib.c
> +++ b/MdePkg/Library/BaseS3PciLib/S3PciLib.c
> @@ -3,7 +3,7 @@
>the PCI operations to be replayed during an S3 resume. This library class
>maps directly on top of the PciLib class. 
>  
> -  Copyright (c) 2006 - 2012, Intel Corporation. All rights reserved.
> +  Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.
>  
>This program and the accompanying materials
>are licensed and made available under the terms and conditions
> @@ -25,7 +25,7 @@
>  #include 
>  
>  #define PCILIB_TO_COMMON_ADDRESS(Address) \
> -((UINT64) UINTN) ((Address>>20) & 0xff)) << 24) + (((UINTN) 
> ((Address>>15) & 0x1f)) << 16) + (((UINTN) ((Address>>12) & 0x07)) << 8) + 
> ((UINTN) (Address & 0xfff 
> +UINTN) ((Address>>20) & 0xff)) << 24) + (((UINTN) ((Address>>15) 
> & 0x1f)) << 16) + (((UINTN) ((Address>>12) & 0x07)) << 8) + ((UINTN) (Address 
> & 0xfff )))
>  
>  /**
>Saves a PCI configuration value to the boot script.

I think this change is potentially unsafe, without auditing all uses of
PCILIB_TO_COMMON_ADDRESS(). In a 32-bit build, the type of the result
will no longer be UINT64 but UINT32, and that can cause 

Re: [edk2] [Patch] SecurityPkg/Tpm12CommandLib: Always check response returnCode

[Zhang, Chao B]

Reviewed-by : Chao Zhang 

-Original Message-
From: Yao, Jiewen 
Sent: Tuesday, January 24, 2017 4:20 PM
To: Kinney, Michael D ; edk2-devel@lists.01.org
Cc: Zhang, Chao B 
Subject: RE: [Patch] SecurityPkg/Tpm12CommandLib: Always check response 
returnCode

Reviewed-by: jiewen@intel.com

> -Original Message-
> From: Kinney, Michael D
> Sent: Wednesday, January 11, 2017 2:23 AM
> To: edk2-devel@lists.01.org
> Cc: Zhang, Chao B ; Yao, Jiewen 
> 
> Subject: [Patch] SecurityPkg/Tpm12CommandLib: Always check response 
> returnCode
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=338
> 
> Update the Tpm12CommandLib to consistently check the returnCode field 
> of a response packet.  These checks are missing from the GetCapability 
> and SelfTest commands.  The functions Tpm12ContinueSelfTest(), 
> Tpm12GetCapabilityFlagPermanent(), and
> Tpm12GetCapabilityFlagVolatile() are updated to verify that the 
> response returnCode is not an error.
> 
> Cc: Chao Zhang 
> Cc: Jiewen Yao 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Michael Kinney 
> ---
>  SecurityPkg/Library/Tpm12CommandLib/Tpm12GetCapability.c | 12
> +++-
>  SecurityPkg/Library/Tpm12CommandLib/Tpm12SelfTest.c  | 16
> ++--
>  2 files changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/SecurityPkg/Library/Tpm12CommandLib/Tpm12GetCapability.c
> b/SecurityPkg/Library/Tpm12CommandLib/Tpm12GetCapability.c
> index c33746a..c6eb9e1 100644
> --- a/SecurityPkg/Library/Tpm12CommandLib/Tpm12GetCapability.c
> +++ b/SecurityPkg/Library/Tpm12CommandLib/Tpm12GetCapability.c
> @@ -1,7 +1,7 @@
>  /** @file
>Implement TPM1.2 Get Capabilities related commands.
> 
> -Copyright (c) 2016, Intel Corporation. All rights reserved. 
> +Copyright (c) 2016 - 2017, 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.  The full text of the license may be 
> found at @@ -79,6 +79,11 @@ Tpm12GetCapabilityFlagPermanent (
>  return Status;
>}
> 
> +  if (SwapBytes32 (Response.Hdr.returnCode) != TPM_SUCCESS) {
> +DEBUG ((DEBUG_ERROR, "Tpm12GetCapabilityFlagPermanent: Response
> Code error! 0x%08x\r\n", SwapBytes32 (Response.Hdr.returnCode)));
> +return EFI_DEVICE_ERROR;
> +  }
> +
>ZeroMem (TpmPermanentFlags, sizeof (*TpmPermanentFlags));
>CopyMem (TpmPermanentFlags, , MIN (sizeof 
> (*TpmPermanentFlags), Response.ResponseSize));
> 
> @@ -120,6 +125,11 @@ Tpm12GetCapabilityFlagVolatile (
>  return Status;
>}
> 
> +  if (SwapBytes32 (Response.Hdr.returnCode) != TPM_SUCCESS) {
> +DEBUG ((DEBUG_ERROR, "Tpm12GetCapabilityFlagVolatile: Response 
> + Code
> error! 0x%08x\r\n", SwapBytes32 (Response.Hdr.returnCode)));
> +return EFI_DEVICE_ERROR;
> +  }
> +
>ZeroMem (VolatileFlags, sizeof (*VolatileFlags));
>CopyMem (VolatileFlags, , MIN (sizeof 
> (*VolatileFlags), Response.ResponseSize));
> 
> diff --git a/SecurityPkg/Library/Tpm12CommandLib/Tpm12SelfTest.c
> b/SecurityPkg/Library/Tpm12CommandLib/Tpm12SelfTest.c
> index 8e232ee..579fed7 100644
> --- a/SecurityPkg/Library/Tpm12CommandLib/Tpm12SelfTest.c
> +++ b/SecurityPkg/Library/Tpm12CommandLib/Tpm12SelfTest.c
> @@ -1,7 +1,7 @@
>  /** @file
>Implement TPM1.2 NV Self Test related commands.
> 
> -Copyright (c) 2016, Intel Corporation. All rights reserved. 
> +Copyright (c) 2016 - 2017, Intel Corporation. All rights reserved. 
> +
>  (C) Copyright 2016 Hewlett Packard Enterprise Development LP  
> This program and the accompanying materials  are licensed and made 
> available under the terms and conditions of the BSD License @@ -16,6 
> +16,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
> EXPRESS OR IMPLIED.
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
> 
>  /**
> @@ -33,6 +34,7 @@ Tpm12ContinueSelfTest (
>VOID
>)
>  {
> +  EFI_STATUS   Status;
>TPM_RQU_COMMAND_HDR  Command;
>TPM_RSP_COMMAND_HDR  Response;
>UINT32   Length;
> @@ -44,5 +46,15 @@ Tpm12ContinueSelfTest (
>Command.paramSize = SwapBytes32 (sizeof (Command));
>Command.ordinal   = SwapBytes32 (TPM_ORD_ContinueSelfTest);
>Length = sizeof (Response);
> -  return Tpm12SubmitCommand (sizeof (Command), (UINT8 *), 
> , (UINT8 *));
> +  Status = Tpm12SubmitCommand (sizeof (Command), (UINT8 *),
> , (UINT8 *));
> +  if (EFI_ERROR (Status)) {
> +return Status;
> +  }
> +
> +  if (SwapBytes32 (Response.returnCode) != TPM_SUCCESS) {
> +DEBUG ((DEBUG_ERROR, "Tpm12ContinueSelfTest: Response Code error!
> 0x%08x\r\n", SwapBytes32 (Response.returnCode)));
> +return EFI_DEVICE_ERROR;
> +  }
> +
> +  return 

Re: [edk2] [PATCH 2/3] MdePkg: VariableDxe: Use UEFI_VARIABLE_DATA

[Zeng, Star]

Please update "MdePkg" to "MdeModulePkg" in the title.

Thanks,
Star
-Original Message-
From: Zeng, Star 
Sent: Tuesday, January 24, 2017 4:33 PM
To: Zhang, Chao B ; edk2-devel@lists.01.org
Cc: Yao, Jiewen ; Zeng, Star 
Subject: RE: [edk2] [PATCH 2/3] MdePkg: VariableDxe: Use UEFI_VARIABLE_DATA

Could we remove " #include " in Measurement.c?

Thanks,
Star
-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Zhang, 
Chao B
Sent: Tuesday, January 24, 2017 3:58 PM
To: edk2-devel@lists.01.org
Cc: Yao, Jiewen ; Zhang, Chao B ; 
Zeng, Star 
Subject: [edk2] [PATCH 2/3] MdePkg: VariableDxe: Use UEFI_VARIABLE_DATA

Use UEFI_VARIABLE_DATA data structure according to TCG PC-Client PFP Spec 00.21.
http://www.trustedcomputinggroup.org/wp-content/uploads/PC-ClientSpecific_Platform_Profile_for_TPM_2p0_Systems_v21.pdf

Cc: Star Zeng 
Cc: Yao Jiewen 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Chao Zhang 
---
 MdeModulePkg/Universal/Variable/RuntimeDxe/Measurement.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Measurement.c 
b/MdeModulePkg/Universal/Variable/RuntimeDxe/Measurement.c
index 309521f..b9febac 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Measurement.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Measurement.c
@@ -96,7 +96,7 @@ MeasureVariable (
 {
   EFI_STATUSStatus;
   UINTN VarNameLength;
-  EFI_VARIABLE_DATA_TREE*VarLog;
+  UEFI_VARIABLE_DATA*VarLog;
   UINT32VarLogSize;
 
   ASSERT ((VarSize == 0 && VarData == NULL) || (VarSize != 0 && VarData != 
NULL)); @@ -105,7 +105,7 @@ MeasureVariable (
   VarLogSize = (UINT32)(sizeof (*VarLog) + VarNameLength * sizeof (*VarName) + 
VarSize
 - sizeof (VarLog->UnicodeName) - sizeof 
(VarLog->VariableData));
 
-  VarLog = (EFI_VARIABLE_DATA_TREE *) AllocateZeroPool (VarLogSize);
+  VarLog = (UEFI_VARIABLE_DATA *) AllocateZeroPool (VarLogSize);
   if (VarLog == NULL) {
 return EFI_OUT_OF_RESOURCES;
   }
--
1.9.5.msysgit.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 2/3] MdePkg: VariableDxe: Use UEFI_VARIABLE_DATA

[Zeng, Star]

Could we remove " #include " in Measurement.c?

Thanks,
Star
-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Zhang, 
Chao B
Sent: Tuesday, January 24, 2017 3:58 PM
To: edk2-devel@lists.01.org
Cc: Yao, Jiewen ; Zhang, Chao B ; 
Zeng, Star 
Subject: [edk2] [PATCH 2/3] MdePkg: VariableDxe: Use UEFI_VARIABLE_DATA

Use UEFI_VARIABLE_DATA data structure according to TCG PC-Client PFP Spec 00.21.
http://www.trustedcomputinggroup.org/wp-content/uploads/PC-ClientSpecific_Platform_Profile_for_TPM_2p0_Systems_v21.pdf

Cc: Star Zeng 
Cc: Yao Jiewen 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Chao Zhang 
---
 MdeModulePkg/Universal/Variable/RuntimeDxe/Measurement.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Measurement.c 
b/MdeModulePkg/Universal/Variable/RuntimeDxe/Measurement.c
index 309521f..b9febac 100644
--- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Measurement.c
+++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Measurement.c
@@ -96,7 +96,7 @@ MeasureVariable (
 {
   EFI_STATUSStatus;
   UINTN VarNameLength;
-  EFI_VARIABLE_DATA_TREE*VarLog;
+  UEFI_VARIABLE_DATA*VarLog;
   UINT32VarLogSize;
 
   ASSERT ((VarSize == 0 && VarData == NULL) || (VarSize != 0 && VarData != 
NULL)); @@ -105,7 +105,7 @@ MeasureVariable (
   VarLogSize = (UINT32)(sizeof (*VarLog) + VarNameLength * sizeof (*VarName) + 
VarSize
 - sizeof (VarLog->UnicodeName) - sizeof 
(VarLog->VariableData));
 
-  VarLog = (EFI_VARIABLE_DATA_TREE *) AllocateZeroPool (VarLogSize);
+  VarLog = (UEFI_VARIABLE_DATA *) AllocateZeroPool (VarLogSize);
   if (VarLog == NULL) {
 return EFI_OUT_OF_RESOURCES;
   }
--
1.9.5.msysgit.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 1/3] MdePkg: UefiTcgPlatform.h: Add UEFI_VARIABLE_DATA

[Yao, Jiewen]

Series reviewed-by: jiewen@intel.com

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Zhang,
> Chao B
> Sent: Tuesday, January 24, 2017 3:58 PM
> To: edk2-devel@lists.01.org
> Cc: Yao, Jiewen ; Zhang, Chao B
> ; Zeng, Star 
> Subject: [edk2] [PATCH 1/3] MdePkg: UefiTcgPlatform.h: Add
> UEFI_VARIABLE_DATA
> 
> Add UEFI_VARIABLE_DATA according to TCG PC-Client PFP Spec 00.21.
> http://www.trustedcomputinggroup.org/wp-content/uploads/PC-ClientSpecific
> _Platform_Profile_for_TPM_2p0_Systems_v21.pdf
> 
> Cc: Star Zeng 
> Cc: Yao Jiewen 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Chao Zhang 
> ---
>  MdePkg/Include/IndustryStandard/UefiTcgPlatform.h | 17
> +
>  1 file changed, 17 insertions(+)
> 
> diff --git a/MdePkg/Include/IndustryStandard/UefiTcgPlatform.h
> b/MdePkg/Include/IndustryStandard/UefiTcgPlatform.h
> index 6ce808e..8a3e170 100644
> --- a/MdePkg/Include/IndustryStandard/UefiTcgPlatform.h
> +++ b/MdePkg/Include/IndustryStandard/UefiTcgPlatform.h
> @@ -151,6 +151,7 @@ typedef struct tdEFI_HANDOFF_TABLE_POINTERS {
>  /// This structure serves as the header for measuring variables. The name of 
> the
>  /// variable (in Unicode format) should immediately follow, then the variable
>  /// data.
> +/// This is defined in TCG EFI Platform Spec for TPM1.1 or 1.2 V1.22
>  ///
>  typedef struct tdEFI_VARIABLE_DATA {
>EFI_GUID  VariableName;
> @@ -160,6 +161,22 @@ typedef struct tdEFI_VARIABLE_DATA {
>INT8  VariableData[1];  ///< Driver or
> platform-specific data
>  } EFI_VARIABLE_DATA;
> 
> +///
> +/// UEFI_VARIABLE_DATA
> +///
> +/// This structure serves as the header for measuring variables. The name of 
> the
> +/// variable (in Unicode format) should immediately follow, then the variable
> +/// data.
> +/// This is defined in TCG PC Client Firmware Profile Spec 00.21
> +///
> +typedef struct tdUEFI_VARIABLE_DATA {
> +  EFI_GUID  VariableName;
> +  UINT64UnicodeNameLength;
> +  UINT64VariableDataLength;
> +  CHAR16UnicodeName[1];
> +  INT8  VariableData[1];  ///< Driver or
> platform-specific data
> +} UEFI_VARIABLE_DATA;
> +
>  //
>  // For TrEE1.0 compatibility
>  //
> --
> 1.9.5.msysgit.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] SecurityPkg/Tpm12CommandLib: Always check response returnCode

[Yao, Jiewen]

Reviewed-by: jiewen@intel.com

> -Original Message-
> From: Kinney, Michael D
> Sent: Wednesday, January 11, 2017 2:23 AM
> To: edk2-devel@lists.01.org
> Cc: Zhang, Chao B ; Yao, Jiewen
> 
> Subject: [Patch] SecurityPkg/Tpm12CommandLib: Always check response
> returnCode
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=338
> 
> Update the Tpm12CommandLib to consistently check the returnCode
> field of a response packet.  These checks are missing from the
> GetCapability and SelfTest commands.  The functions
> Tpm12ContinueSelfTest(), Tpm12GetCapabilityFlagPermanent(), and
> Tpm12GetCapabilityFlagVolatile() are updated to verify that the
> response returnCode is not an error.
> 
> Cc: Chao Zhang 
> Cc: Jiewen Yao 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Michael Kinney 
> ---
>  SecurityPkg/Library/Tpm12CommandLib/Tpm12GetCapability.c | 12
> +++-
>  SecurityPkg/Library/Tpm12CommandLib/Tpm12SelfTest.c  | 16
> ++--
>  2 files changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/SecurityPkg/Library/Tpm12CommandLib/Tpm12GetCapability.c
> b/SecurityPkg/Library/Tpm12CommandLib/Tpm12GetCapability.c
> index c33746a..c6eb9e1 100644
> --- a/SecurityPkg/Library/Tpm12CommandLib/Tpm12GetCapability.c
> +++ b/SecurityPkg/Library/Tpm12CommandLib/Tpm12GetCapability.c
> @@ -1,7 +1,7 @@
>  /** @file
>Implement TPM1.2 Get Capabilities related commands.
> 
> -Copyright (c) 2016, Intel Corporation. All rights reserved. 
> +Copyright (c) 2016 - 2017, 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.  The full text of the license may be 
> found
> at
> @@ -79,6 +79,11 @@ Tpm12GetCapabilityFlagPermanent (
>  return Status;
>}
> 
> +  if (SwapBytes32 (Response.Hdr.returnCode) != TPM_SUCCESS) {
> +DEBUG ((DEBUG_ERROR, "Tpm12GetCapabilityFlagPermanent: Response
> Code error! 0x%08x\r\n", SwapBytes32 (Response.Hdr.returnCode)));
> +return EFI_DEVICE_ERROR;
> +  }
> +
>ZeroMem (TpmPermanentFlags, sizeof (*TpmPermanentFlags));
>CopyMem (TpmPermanentFlags, , MIN (sizeof
> (*TpmPermanentFlags), Response.ResponseSize));
> 
> @@ -120,6 +125,11 @@ Tpm12GetCapabilityFlagVolatile (
>  return Status;
>}
> 
> +  if (SwapBytes32 (Response.Hdr.returnCode) != TPM_SUCCESS) {
> +DEBUG ((DEBUG_ERROR, "Tpm12GetCapabilityFlagVolatile: Response Code
> error! 0x%08x\r\n", SwapBytes32 (Response.Hdr.returnCode)));
> +return EFI_DEVICE_ERROR;
> +  }
> +
>ZeroMem (VolatileFlags, sizeof (*VolatileFlags));
>CopyMem (VolatileFlags, , MIN (sizeof (*VolatileFlags),
> Response.ResponseSize));
> 
> diff --git a/SecurityPkg/Library/Tpm12CommandLib/Tpm12SelfTest.c
> b/SecurityPkg/Library/Tpm12CommandLib/Tpm12SelfTest.c
> index 8e232ee..579fed7 100644
> --- a/SecurityPkg/Library/Tpm12CommandLib/Tpm12SelfTest.c
> +++ b/SecurityPkg/Library/Tpm12CommandLib/Tpm12SelfTest.c
> @@ -1,7 +1,7 @@
>  /** @file
>Implement TPM1.2 NV Self Test related commands.
> 
> -Copyright (c) 2016, Intel Corporation. All rights reserved. 
> +Copyright (c) 2016 - 2017, Intel Corporation. All rights reserved. 
>  (C) Copyright 2016 Hewlett Packard Enterprise Development LP
>  This program and the accompanying materials
>  are licensed and made available under the terms and conditions of the BSD
> License
> @@ -16,6 +16,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND,
> EITHER EXPRESS OR IMPLIED.
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
> 
>  /**
> @@ -33,6 +34,7 @@ Tpm12ContinueSelfTest (
>VOID
>)
>  {
> +  EFI_STATUS   Status;
>TPM_RQU_COMMAND_HDR  Command;
>TPM_RSP_COMMAND_HDR  Response;
>UINT32   Length;
> @@ -44,5 +46,15 @@ Tpm12ContinueSelfTest (
>Command.paramSize = SwapBytes32 (sizeof (Command));
>Command.ordinal   = SwapBytes32 (TPM_ORD_ContinueSelfTest);
>Length = sizeof (Response);
> -  return Tpm12SubmitCommand (sizeof (Command), (UINT8 *),
> , (UINT8 *));
> +  Status = Tpm12SubmitCommand (sizeof (Command), (UINT8 *),
> , (UINT8 *));
> +  if (EFI_ERROR (Status)) {
> +return Status;
> +  }
> +
> +  if (SwapBytes32 (Response.returnCode) != TPM_SUCCESS) {
> +DEBUG ((DEBUG_ERROR, "Tpm12ContinueSelfTest: Response Code error!
> 0x%08x\r\n", SwapBytes32 (Response.returnCode)));
> +return EFI_DEVICE_ERROR;
> +  }
> +
> +  return Status;
>  }
> --
> 2.6.3.windows.1

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