Re: [edk2] [PATCH] MdeModulePkg/Core: fix feature conflict between NX and NULL detection

[Wang, Jian J]

Ok. Thanks for the comments.

Regards,
Jian

> -Original Message-
> From: Ni, Ruiyu
> Sent: Thursday, February 01, 2018 1:54 PM
> To: Wang, Jian J ; edk2-devel@lists.01.org
> Cc: Yao, Jiewen ; Dong, Eric ;
> Zeng, Star 
> Subject: Re: [edk2] [PATCH] MdeModulePkg/Core: fix feature conflict between
> NX and NULL detection
> 
> On 2/1/2018 1:33 PM, Ni, Ruiyu wrote:
> > On 2/1/2018 9:17 AM, Wang, Jian J wrote:
> >> You're right. Using a mask or separating the API into two
> >> (SetMemoryAttributes/ClearMemoryAttributes)
> >> is much better and can avoid many potential issues.
> >>
> >> Regards,
> >> Jian
> >>
> >
> > For now the patch is good enough to leave NULL pointer detection
> > feature enabled.
> >
> > Reviewed-by: Ruiyu Ni 
> >
> >
> >>
> >>> -Original Message-
> >>> From: Ni, Ruiyu
> >>> Sent: Tuesday, January 30, 2018 12:38 PM
> >>> To: Wang, Jian J ; edk2-devel@lists.01.org
> >>> Cc: Zeng, Star ; Dong, Eric
> >>> ; Yao,
> >>> Jiewen 
> >>> Subject: Re: [PATCH] MdeModulePkg/Core: fix feature conflict between
> >>> NX and
> >>> NULL detection
> >>>
> >>> On 1/29/2018 7:09 PM, Jian J Wang wrote:
>  If enabled, NX memory protection feature will mark all free memory as
>  NX (non-executable), including page 0. This will overwrite the
>  attributes
>  of page 0 if NULL pointer detection feature is also enabled and then
>  compromise the functionality of it. The solution is skipping the NX
>  attributes setting to page 0 if NULL pointer detection feature is
>  enabled.
> 
>  Cc: Star Zeng 
>  Cc: Eric Dong 
>  Cc: Jiewen Yao 
>  Cc: Ruiyu Ni 
>  Contributed-under: TianoCore Contribution Agreement 1.1
>  Signed-off-by: Jian J Wang 
>  ---
>     MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 20
> >>> 
>     1 file changed, 16 insertions(+), 4 deletions(-)
> 
>  diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> >>> b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
>  index 862593f562..150167bf66 100644
>  --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
>  +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
>  @@ -845,10 +845,22 @@ InitializeDxeNxMemoryProtectionPolicy (
> 
>     Attributes = GetPermissionAttributeForMemoryType
>  (MemoryMapEntry-
>  Type);
>     if (Attributes != 0) {
>  -  SetUefiImageMemoryAttributes (
>  -    MemoryMapEntry->PhysicalStart,
>  -    LShiftU64 (MemoryMapEntry->NumberOfPages, EFI_PAGE_SHIFT),
>  -    Attributes);
>  +  if (MemoryMapEntry->PhysicalStart == 0 &&
>  +  PcdGet8 (PcdNullPointerDetectionPropertyMask) != 0) {
>  +    //
>  +    // Skip page 0 if NULL pointer detection is enabled to
>  avoid attributes
>  +    // overwritten.
>  +    //
> 
> By the way, could you please add an assertion here?
> ASSERT (MemoryMapEntry->NumberOfPages != 0);
>  +    SetUefiImageMemoryAttributes (
>  +  MemoryMapEntry->PhysicalStart + EFI_PAGE_SIZE,
>  +  LShiftU64 (MemoryMapEntry->NumberOfPages - 1,
>  EFI_PAGE_SHIFT),
>  +  Attributes);
>  +  } else {
>  +    SetUefiImageMemoryAttributes (
>  +  MemoryMapEntry->PhysicalStart,
>  +  LShiftU64 (MemoryMapEntry->NumberOfPages, EFI_PAGE_SHIFT),
>  +  Attributes);
>  +  }
>     }
>     MemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry,
> >>> DescriptorSize);
>   }
> 
> >>> Does this bug expose an API-level issue?
> >>> SetUefiImageMemoryAttributes () should also accept a Mask value?
> >>>
> >>> --
> >>> Thanks,
> >>> Ray
> >
> >
> 
> 
> --
> Thanks,
> Ray
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] MdeModulePkg/Core: fix feature conflict between NX and NULL detection

[Ni, Ruiyu]

On 2/1/2018 1:33 PM, Ni, Ruiyu wrote:

On 2/1/2018 9:17 AM, Wang, Jian J wrote:
You're right. Using a mask or separating the API into two 
(SetMemoryAttributes/ClearMemoryAttributes)

is much better and can avoid many potential issues.

Regards,
Jian



For now the patch is good enough to leave NULL pointer detection
feature enabled.

Reviewed-by: Ruiyu Ni 





-Original Message-
From: Ni, Ruiyu
Sent: Tuesday, January 30, 2018 12:38 PM
To: Wang, Jian J ; edk2-devel@lists.01.org
Cc: Zeng, Star ; Dong, Eric 
; Yao,

Jiewen 
Subject: Re: [PATCH] MdeModulePkg/Core: fix feature conflict between 
NX and

NULL detection

On 1/29/2018 7:09 PM, Jian J Wang wrote:

If enabled, NX memory protection feature will mark all free memory as
NX (non-executable), including page 0. This will overwrite the 
attributes

of page 0 if NULL pointer detection feature is also enabled and then
compromise the functionality of it. The solution is skipping the NX
attributes setting to page 0 if NULL pointer detection feature is 
enabled.


Cc: Star Zeng 
Cc: Eric Dong 
Cc: Jiewen Yao 
Cc: Ruiyu Ni 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang 
---
   MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 20



   1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c

b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c

index 862593f562..150167bf66 100644
--- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
+++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
@@ -845,10 +845,22 @@ InitializeDxeNxMemoryProtectionPolicy (

   Attributes = GetPermissionAttributeForMemoryType 
(MemoryMapEntry-

Type);
   if (Attributes != 0) {
-  SetUefiImageMemoryAttributes (
-    MemoryMapEntry->PhysicalStart,
-    LShiftU64 (MemoryMapEntry->NumberOfPages, EFI_PAGE_SHIFT),
-    Attributes);
+  if (MemoryMapEntry->PhysicalStart == 0 &&
+  PcdGet8 (PcdNullPointerDetectionPropertyMask) != 0) {
+    //
+    // Skip page 0 if NULL pointer detection is enabled to 
avoid attributes

+    // overwritten.
+    //


By the way, could you please add an assertion here?
ASSERT (MemoryMapEntry->NumberOfPages != 0);

+    SetUefiImageMemoryAttributes (
+  MemoryMapEntry->PhysicalStart + EFI_PAGE_SIZE,
+  LShiftU64 (MemoryMapEntry->NumberOfPages - 1, 
EFI_PAGE_SHIFT),

+  Attributes);
+  } else {
+    SetUefiImageMemoryAttributes (
+  MemoryMapEntry->PhysicalStart,
+  LShiftU64 (MemoryMapEntry->NumberOfPages, EFI_PAGE_SHIFT),
+  Attributes);
+  }
   }
   MemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry,

DescriptorSize);

 }


Does this bug expose an API-level issue?
SetUefiImageMemoryAttributes () should also accept a Mask value?

--
Thanks,
Ray






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

Re: [edk2] [PATCH] MdeModulePkg/Core: fix feature conflict between NX and heap guard

[Ni, Ruiyu]

On 1/29/2018 8:09 PM, Jian J Wang wrote:

Considering following scenario (both NX memory protection and heap guard
are enabled):

1. Allocate 3 pages. The attributes of adjacent memory pages will be

   |NOT-PRESENT|  present  |  present  |  present  |NOT-PRESENT|

2. Free the middle page. The attributes of adjacent memory pages should
   be

   |NOT-PRESENT|  present  |NOT-PRESENT|  present  |NOT-PRESENT|

   But the NX feature will overwrite the attributes of middle page. So
   it looks still like below, which is wrong.

   |NOT-PRESENT|  present  |  PRESENT  |  present  |NOT-PRESENT|

The solution is checking the first and/or last page of a memory block to be
marked as NX, and skipping them if they are Guard pages.

Cc: Star Zeng 
Cc: Eric Dong 
Cc: Jiewen Yao 
Cc: Ruiyu Ni 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang 
---
  MdeModulePkg/Core/Dxe/Mem/HeapGuard.c | 14 ++
  MdeModulePkg/Core/Dxe/Mem/HeapGuard.h | 10 ++
  MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 22 ++
  3 files changed, 46 insertions(+)

diff --git a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c 
b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
index 392aeb8a02..d7906e08c5 100644
--- a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
+++ b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
@@ -728,6 +728,20 @@ IsPageTypeToGuard (
return IsMemoryTypeToGuard (MemoryType, AllocateType, GUARD_HEAP_TYPE_PAGE);
  }
  
+/**

+  Check to see if the heap guard is enabled for page and/or pool allocation.
+
+  @return TRUE/FALSE.
+**/
+BOOLEAN
+IsHeapGuardEnabled (
+  VOID
+  )
+{
+  return IsMemoryTypeToGuard (EfiMaxMemoryType, AllocateAnyPages,
+  GUARD_HEAP_TYPE_POOL|GUARD_HEAP_TYPE_PAGE);
+}
+
  /**
Set head Guard and tail Guard for the given memory range.
  
diff --git a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.h b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.h

index 30ac0e678f..7208ab1437 100644
--- a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.h
+++ b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.h
@@ -389,6 +389,16 @@ AdjustPoolHeadF (
IN EFI_PHYSICAL_ADDRESSMemory
);
  
+/**

+  Check to see if the heap guard is enabled for page and/or pool allocation.
+
+  @return TRUE/FALSE.
+**/
+BOOLEAN
+IsHeapGuardEnabled (
+  VOID
+  );
+
  extern BOOLEAN mOnGuarding;
  
  #endif

diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c 
b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
index 150167bf66..877e6e5025 100644
--- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
+++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
@@ -48,6 +48,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
  #include 
  
  #include "DxeMain.h"

+#include "Mem/HeapGuard.h"
  
  #define CACHE_ATTRIBUTE_MASK   (EFI_MEMORY_UC | EFI_MEMORY_WC | EFI_MEMORY_WT | EFI_MEMORY_WB | EFI_MEMORY_UCE | EFI_MEMORY_WP)

  #define MEMORY_ATTRIBUTE_MASK  (EFI_MEMORY_RP | EFI_MEMORY_XP | EFI_MEMORY_RO)
@@ -1200,6 +1201,27 @@ ApplyMemoryProtectionPolicy (
  return EFI_SUCCESS;
}
  
+  //

+  // Don't overwrite Guard pages, which should be the first and/or last page,
+  // if any.
+  //
+  if (IsHeapGuardEnabled ()) {
+if (IsGuardPage (Memory))  {
+  Memory += EFI_PAGE_SIZE;
+  Length -= EFI_PAGE_SIZE;
+  if (Length == 0) {
+return EFI_SUCCESS;
+  }
+}
+
+if (IsGuardPage (Memory + Length - EFI_PAGE_SIZE))  {
+  Length -= EFI_PAGE_SIZE;
+  if (Length == 0) {
+return EFI_SUCCESS;
+  }
+}
+  }
+
//
// Update the executable permissions according to the DXE memory
// protection policy, but only if


Reviewed-by: Ruiyu Ni 

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

Re: [edk2] [PATCH] MdeModulePkg/Core: fix guard page missing issue

[Ni, Ruiyu]

On 1/29/2018 12:52 PM, Jian J Wang wrote:

This issue is a regression one caused by a patch at

 425d25699be83c35e12df8470b827d7fbcef3bce

That fix didn't take the 0 page to free into account, which still
needs to call UnsetGuardPage() even no memory needs to free.

The fix is just moving the calling of UnsetGuardPage() to the place
right after calling AdjustMemoryF().

Cc: Ruiyu Ni 
Cc: Star Zeng 
Cc: Eric Dong 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang 
---
  MdeModulePkg/Core/Dxe/Mem/HeapGuard.c |  7 +++
  MdeModulePkg/Core/Dxe/Mem/Pool.c  | 16 
  2 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c 
b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
index 92753c7269..392aeb8a02 100644
--- a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
+++ b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
@@ -1135,10 +1135,6 @@ CoreConvertPagesWithGuard (
  OldPages = NumberOfPages;
  
  AdjustMemoryF (, );

-if (NumberOfPages == 0) {
-  return EFI_SUCCESS;
-}
-
  //
  // It's safe to unset Guard page inside memory lock because there should
  // be no memory allocation occurred in updating memory page attribute at
@@ -1147,6 +1143,9 @@ CoreConvertPagesWithGuard (
  // marking it usable (from non-present to present).
  //
  UnsetGuardForMemory (OldStart, OldPages);
+if (NumberOfPages == 0) {
+  return EFI_SUCCESS;
+}
} else {
  AdjustMemoryA (, );
}
diff --git a/MdeModulePkg/Core/Dxe/Mem/Pool.c b/MdeModulePkg/Core/Dxe/Mem/Pool.c
index df9a1d28df..1ff2061f7f 100644
--- a/MdeModulePkg/Core/Dxe/Mem/Pool.c
+++ b/MdeModulePkg/Core/Dxe/Mem/Pool.c
@@ -642,15 +642,15 @@ CoreFreePoolPagesWithGuard (
NoPagesGuarded = NoPages;
  
AdjustMemoryF (, );

+  //
+  // It's safe to unset Guard page inside memory lock because there should
+  // be no memory allocation occurred in updating memory page attribute at
+  // this point. And unsetting Guard page before free will prevent Guard
+  // page just freed back to pool from being allocated right away before
+  // marking it usable (from non-present to present).
+  //
+  UnsetGuardForMemory (MemoryGuarded, NoPagesGuarded);
if (NoPages > 0) {
-//
-// It's safe to unset Guard page inside memory lock because there should
-// be no memory allocation occurred in updating memory page attribute at
-// this point. And unsetting Guard page before free will prevent Guard
-// page just freed back to pool from being allocated right away before
-// marking it usable (from non-present to present).
-//
-UnsetGuardForMemory (MemoryGuarded, NoPagesGuarded);
  CoreFreePoolPagesI (PoolType, Memory, NoPages);
}
  }


Reviewed-by: Ruiyu Ni 

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

Re: [edk2] [PATCH 2/6] MdeModulePkg/ConSplitter: ReadKeyStrokeEx always return key state

[Zeng, Star]

Hi Ruiyu,

One comment:
ConSplitterTextInPrivateReadKeyStroke() needs filter out partial key from 
ConSplitterTextInExDequeueKey().

With the comment above handled, Reviewed-by: Star Zeng  to 
this patch series.


Thanks,
Star
-Original Message-
From: Ni, Ruiyu 
Sent: Monday, January 22, 2018 4:10 PM
To: edk2-devel@lists.01.org
Cc: Zeng, Star ; Kinney, Michael D 

Subject: [PATCH 2/6] MdeModulePkg/ConSplitter: ReadKeyStrokeEx always return 
key state

Today's implementation only return key state when there is key.
But when user doesn't press any key, the key state cannot be
returned.

The patch changes the ReadKeyStrokeEx() to always return the
key state even there is no key pressed.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni 
Cc: Star Zeng 
Cc: Michael D Kinney 
---
 .../Universal/Console/ConSplitterDxe/ConSplitter.c | 164 ++---
 .../Universal/Console/ConSplitterDxe/ConSplitter.h |   4 +-
 2 files changed, 143 insertions(+), 25 deletions(-)

diff --git a/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c 
b/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
index e70ff6114a..022fca7cbb 100644
--- a/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
+++ b/MdeModulePkg/Universal/Console/ConSplitterDxe/ConSplitter.c
@@ -16,7 +16,7 @@
   never removed. Such design ensures sytem function well during none console
   device situation.
 
-Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.
+Copyright (c) 2006 - 2018, 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
@@ -67,6 +67,8 @@ GLOBAL_REMOVE_IF_UNREFERENCED TEXT_IN_SPLITTER_PRIVATE_DATA  
mConIn = {
 (LIST_ENTRY *) NULL,
 (LIST_ENTRY *) NULL
   },
+  (EFI_KEY_DATA *) NULL,
+  0,
   0,
   FALSE,
 
@@ -606,6 +608,7 @@ ConSplitterTextInConstructor (
   )
 {
   EFI_STATUS  Status;
+  UINTN   TextInExListCount;
 
   //
   // Allocate buffer for Simple Text Input device
@@ -631,6 +634,19 @@ ConSplitterTextInConstructor (
   );
   ASSERT_EFI_ERROR (Status);
 
+  //
+  // Allocate buffer for KeyQueue
+  //
+  TextInExListCount = ConInPrivate->TextInExListCount;
+  Status = ConSplitterGrowBuffer (
+ sizeof (EFI_KEY_DATA),
+ ,
+ (VOID **) >KeyQueue
+ );
+  if (EFI_ERROR (Status)) {
+return EFI_OUT_OF_RESOURCES;
+  }
+
   //
   // Allocate buffer for Simple Text Input Ex device
   //
@@ -1968,6 +1984,17 @@ ConSplitterTextInExAddDevice (
 return EFI_OUT_OF_RESOURCES;
   }
 }
+
+TextInExListCount = Private->TextInExListCount;
+Status = ConSplitterGrowBuffer (
+   sizeof (EFI_KEY_DATA),
+   ,
+   (VOID **) >KeyQueue
+   );
+if (EFI_ERROR (Status)) {
+  return EFI_OUT_OF_RESOURCES;
+}
+
 Status = ConSplitterGrowBuffer (
   sizeof (EFI_SIMPLE_TEXT_INPUT_EX_PROTOCOL *),
   >TextInExListCount,
@@ -3445,11 +3472,46 @@ ConSplitterTextInReset (
 
   if (!EFI_ERROR (ReturnStatus)) {
 ToggleStateSyncReInitialization (Private);
+//
+// Empty the key queue.
+//
+Private->CurrentNumberOfKeys = 0;
   }
 
   return ReturnStatus;
 }
 
+/**
+  Dequeue the saved key from internal key queue.
+
+  @param  Private  Protocol instance pointer.
+  @param  KeyData  A pointer to a buffer that is filled in 
with the
+   keystroke state data for the key that was
+   pressed.
+  @retval EFI_NOT_FOUNDQueue is empty.
+  @retval EFI_SUCCESS  First key is dequeued and returned.
+**/
+EFI_STATUS
+ConSplitterTextInExDequeueKey (
+  IN  TEXT_IN_SPLITTER_PRIVATE_DATA   *Private,
+  OUT EFI_KEY_DATA*KeyData
+  )
+{
+  if (Private->CurrentNumberOfKeys == 0) {
+return EFI_NOT_FOUND;
+  }
+  //
+  // Return the first saved key.
+  //
+  CopyMem (KeyData, >KeyQueue[0], sizeof (EFI_KEY_DATA));
+  Private->CurrentNumberOfKeys--;
+  CopyMem (
+>KeyQueue[0],
+>KeyQueue[1],
+Private->CurrentNumberOfKeys * sizeof (EFI_KEY_DATA)
+);
+  return EFI_SUCCESS;
+}
 
 /**
   Reads the next keystroke from the input device. The WaitForKey Event can
@@ -3473,7 +3535,13 @@ ConSplitterTextInPrivateReadKeyStroke (
 {
   EFI_STATUSStatus;
   UINTN Index;
-  EFI_INPUT_KEY CurrentKey;
+  EFI_KEY_DATA  KeyData;
+
+  Status = ConSplitterTextInExDequeueKey (Private, );
+  if (!EFI_ERROR (Status)) {
+CopyMem (Key, , sizeof (EFI_INPUT_KEY));
+return Status;
+  }
 
   Key->UnicodeChar  = 0;
   Key->ScanCode = SCAN_NULL;
@@ -3486,15 

Re: [edk2] [PATCH] MdeModulePkg/Core: fix feature conflict between NX and NULL detection

[Ni, Ruiyu]

On 2/1/2018 9:17 AM, Wang, Jian J wrote:

You're right. Using a mask or separating the API into two 
(SetMemoryAttributes/ClearMemoryAttributes)
is much better and can avoid many potential issues.

Regards,
Jian



For now the patch is good enough to leave NULL pointer detection
feature enabled.

Reviewed-by: Ruiyu Ni 





-Original Message-
From: Ni, Ruiyu
Sent: Tuesday, January 30, 2018 12:38 PM
To: Wang, Jian J ; edk2-devel@lists.01.org
Cc: Zeng, Star ; Dong, Eric ; Yao,
Jiewen 
Subject: Re: [PATCH] MdeModulePkg/Core: fix feature conflict between NX and
NULL detection

On 1/29/2018 7:09 PM, Jian J Wang wrote:

If enabled, NX memory protection feature will mark all free memory as
NX (non-executable), including page 0. This will overwrite the attributes
of page 0 if NULL pointer detection feature is also enabled and then
compromise the functionality of it. The solution is skipping the NX
attributes setting to page 0 if NULL pointer detection feature is enabled.

Cc: Star Zeng 
Cc: Eric Dong 
Cc: Jiewen Yao 
Cc: Ruiyu Ni 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang 
---
   MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 20



   1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c

b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c

index 862593f562..150167bf66 100644
--- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
+++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
@@ -845,10 +845,22 @@ InitializeDxeNxMemoryProtectionPolicy (

   Attributes = GetPermissionAttributeForMemoryType (MemoryMapEntry-
Type);
   if (Attributes != 0) {
-  SetUefiImageMemoryAttributes (
-MemoryMapEntry->PhysicalStart,
-LShiftU64 (MemoryMapEntry->NumberOfPages, EFI_PAGE_SHIFT),
-Attributes);
+  if (MemoryMapEntry->PhysicalStart == 0 &&
+  PcdGet8 (PcdNullPointerDetectionPropertyMask) != 0) {
+//
+// Skip page 0 if NULL pointer detection is enabled to avoid attributes
+// overwritten.
+//
+SetUefiImageMemoryAttributes (
+  MemoryMapEntry->PhysicalStart + EFI_PAGE_SIZE,
+  LShiftU64 (MemoryMapEntry->NumberOfPages - 1, EFI_PAGE_SHIFT),
+  Attributes);
+  } else {
+SetUefiImageMemoryAttributes (
+  MemoryMapEntry->PhysicalStart,
+  LShiftU64 (MemoryMapEntry->NumberOfPages, EFI_PAGE_SHIFT),
+  Attributes);
+  }
   }
   MemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry,

DescriptorSize);

 }


Does this bug expose an API-level issue?
SetUefiImageMemoryAttributes () should also accept a Mask value?

--
Thanks,
Ray



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

Re: [edk2] [PATCH] UefiCpuPkg: Enhance CPU feature dependency check

[Ni, Ruiyu]

On 1/31/2018 5:44 PM, Laszlo Ersek wrote:

On 01/31/18 08:00, Song, BinX wrote:

Current CPU feature dependency check will hang on when meet below or
similar case:
if (IsCpuFeatureSupported (CPU_FEATURE_AESNI)) {
   Status = RegisterCpuFeature (
  "AESNI",
  AesniGetConfigData,
  AesniSupport,
  AesniInitialize,
  CPU_FEATURE_AESNI,
  CPU_FEATURE_MWAIT | CPU_FEATURE_BEFORE,
  CPU_FEATURE_END
  );
   ASSERT_EFI_ERROR (Status);
}
if (IsCpuFeatureSupported (CPU_FEATURE_MWAIT)) {
   Status = RegisterCpuFeature (
  "MWAIT",
  NULL,
  MonitorMwaitSupport,
  MonitorMwaitInitialize,
  CPU_FEATURE_MWAIT,
  CPU_FEATURE_AESNI | CPU_FEATURE_BEFORE,
  CPU_FEATURE_END
  );
   ASSERT_EFI_ERROR (Status);
}

Solution is to separate current CPU feature dependency check into
sort and check two parts.

Sort function:
According to CPU feature's dependency, sort all CPU features.
Later dependency will override previous dependency if they are conflicted.

Check function:
Check sorted CPU features' relationship, ASSERT invalid relationship.

Cc: Eric Dong 
Cc: Laszlo Ersek 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Bell Song 
---
  .../RegisterCpuFeaturesLib/CpuFeaturesInitialize.c | 271 -
  .../RegisterCpuFeaturesLib/RegisterCpuFeatures.h   |   7 +
  .../RegisterCpuFeaturesLib.c   | 130 +-
  3 files changed, 278 insertions(+), 130 deletions(-)

diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c 
b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
index 4d75c07..2fd0d5f 100644
--- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
+++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
@@ -423,6 +423,271 @@ DumpRegisterTableOnProcessor (
  }
  
  /**

+  From FeatureBitMask, find the right feature entry in CPU feature list.
+
+  @param[in]  FeatureListThe pointer to CPU feature list.
+  @param[in]  CurrentFeature The pointer to current CPU feature.
+  @param[in]  BeforeFlag TRUE: BeforeFeatureBitMask; FALSE: 
AfterFeatureBitMask.
+
+  @return  The pointer to right CPU feature entry.
+**/
+LIST_ENTRY *
+FindFeatureInList(
+  IN LIST_ENTRY  *CpuFeatureList,
+  IN CPU_FEATURES_ENTRY  *CurrentCpuFeature,
+  IN BOOLEAN  BeforeFlag
+  )
+{
+  LIST_ENTRY *TempEntry;
+  CPU_FEATURES_ENTRY *TempFeature;
+  UINT8  *FeatureBitMask;
+
+  FeatureBitMask = BeforeFlag ? CurrentCpuFeature->BeforeFeatureBitMask : 
CurrentCpuFeature->AfterFeatureBitMask;
+  TempEntry = GetFirstNode (CpuFeatureList);
+  while (!IsNull (CpuFeatureList, TempEntry)) {
+TempFeature = CPU_FEATURE_ENTRY_FROM_LINK (TempEntry);
+if (IsBitMaskMatchCheck (FeatureBitMask, TempFeature->FeatureMask)){
+  return TempEntry;
+}
+TempEntry = TempEntry->ForwardLink;
+  }
+
+  DEBUG ((DEBUG_ERROR, "Error: Feature %a ", CurrentCpuFeature->FeatureName, BeforeFlag ? "before 
":"after ", "condition is invalid!\n"));


Hi, I skimmed this patch quickly -- I can tell that I can't really tell
what's going on. I don't know how the feature dependencies are defined
in the first place, and what the bug is.

However, I do see that the above DEBUG macro invocation is incorrect.
The format string has one (1) %a conversion specification, but we pass
three (3) arguments.

I think the last argument ("condition is invalid!\n") should actually be
part of the format string. And then, the "before"/"after" string has to
be printed somehow as well.

Another superficial observation below:


+/**
+  Check sorted CPU features' relationship, ASSERT invalid one.
+
+  @param[in]  FeatureList  The pointer to CPU feature list.
+**/
+VOID
+CheckCpuFeaturesRelationShip (


I don't think we should capitalize "Ship" in this identifier.

Third comment: there are several ways to define "sorting", so I'm not
sure my question applies, but: can we replace the manual sorting with
SortLib?


Laszlo,
I haven't checked the patch in details.
But regarding to the SortLib suggestion, the feature entry is chained in
linked list, while SortLib can only perform sorting in array.

Bin,
Can we have a simpler fix to this issue?
If my understanding is correct, the patch tries to fix the infinite loop
in code.
If that's true, can we just firstly calculate how many loops are
expected before looping, then exit when the maximum loop is met?
Upon that, when the sort hasn't been finished, a wrong dependency
exists.




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



--
Thanks,
Ray
___

Re: [edk2] [RFC] MdeModulePkg/PciHostBridgeDxe: Add support for address translation

[Ni, Ruiyu]

On 1/29/2018 4:50 PM, Guo Heyi wrote:

Sorry for the late; I caught cold and didn't work for several days last week :(
Please see my comments below:


On Mon, Jan 22, 2018 at 11:36:14AM +0800, Ni, Ruiyu wrote:

On 1/18/2018 9:26 AM, Guo Heyi wrote:

On Wed, Jan 17, 2018 at 02:08:06PM +, Ard Biesheuvel wrote:

On 15 January 2018 at 14:46, Heyi Guo  wrote:

This is the draft patch for the discussion posted in edk2-devel
mailing list:
https://lists.01.org/pipermail/edk2-devel/2017-December/019289.html

As discussed in the mailing list, we'd like to add support for PCI
address translation which is necessary for some non-x86 platforms. I
also want to minimize the changes to the generic host bridge driver
and platform PciHostBridgeLib implemetations, so additional two
interfaces are added to expose translation information of the
platform. To be generic, I add translation for each type of IO or
memory resources.

The patch is still a RFC, so I only passed the build for qemu64 and
the function has not been tested yet.

Please let me know your comments about it.

Thanks.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Heyi Guo 
Cc: Ruiyu Ni 
Cc: Ard Biesheuvel 
Cc: Star Zeng 
Cc: Eric Dong 
---
  .../FdtPciHostBridgeLib/FdtPciHostBridgeLib.c  |  19 
  .../Bus/Pci/PciHostBridgeDxe/PciHostBridge.c   |  53 ---
  .../Bus/Pci/PciHostBridgeDxe/PciRootBridge.h   |   8 +-
  .../Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 101 ++---
  MdeModulePkg/Include/Library/PciHostBridgeLib.h|  36 
  5 files changed, 192 insertions(+), 25 deletions(-)

diff --git a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c 
b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
index 5b9c887..0c8371a 100644
--- a/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
+++ b/ArmVirtPkg/Library/FdtPciHostBridgeLib/FdtPciHostBridgeLib.c
@@ -360,6 +360,16 @@ PciHostBridgeGetRootBridges (
return 
  }

+PCI_ROOT_BRIDGE_TRANSLATION *
+EFIAPI
+PciHostBridgeGetTranslations (
+  UINTN *Count
+  )
+{
+  *Count = 0;
+  return NULL;
+}
+
  /**
Free the root bridge instances array returned from
PciHostBridgeGetRootBridges().
@@ -377,6 +387,15 @@ PciHostBridgeFreeRootBridges (
ASSERT (Count == 1);
  }

+VOID
+EFIAPI
+PciHostBridgeFreeTranslations (
+  PCI_ROOT_BRIDGE_TRANSLATION *Translations,
+  UINTN   Count
+  )
+{
+}
+
  /**
Inform the platform that the resource conflict happens.

diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c 
b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
index 1494848..835e411 100644
--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
@@ -360,18 +360,38 @@ InitializePciHostBridge (
PCI_HOST_BRIDGE_INSTANCE*HostBridge;
PCI_ROOT_BRIDGE_INSTANCE*RootBridge;
PCI_ROOT_BRIDGE *RootBridges;
+  PCI_ROOT_BRIDGE_TRANSLATION *Translations;
UINTN   RootBridgeCount;
+  UINTN   TranslationCount;
UINTN   Index;
PCI_ROOT_BRIDGE_APERTURE*MemApertures[4];


Wouldn't it be much better to add a 'translation' member to
PCI_ROOT_BRIDGE_APERTURE? That way, existing code just default to a
translation of 0, and all the handling of the separate array can be
dropped.


Actually my first idea was the same, but when I looked at the implementation of
PciHostBridgeLib of Ovmf, I found it a little tedious to change the
existing code in this way. We'll need to check everywhere
PCI_ROOT_BRIDGE_APERTURE or PCI_ROOT_BRIDGE is used, to make sure the
translation field is initialized to be zero, e.g. line 235~245.

What I did in this RFC is not so straightforward indeed. So I can change the
code if we all agree to add Translation field into PCI_ROOT_BRIDGE_APERTURE
directly.

Thanks,

Gary (Heyi Guo)


I also agree to put the translation fields into the
PCI_ROOT_BRIDGE_APERTURE.


But I think we may have different understandings to the address
translation.
My understanding to the whole translation:
1. PciHostBridge.GetProposedResources () returns resource information
for a single root bridge. It only carries the address range in PCI
view.
The address range/resource is reported/submitted by PciHostBridgeLib.
Before the change, CPU view equals to PCI view. So PciHostBridgeDxe
driver directly adds the resource to GCD.
In your change, PciHostBridgeDxe assumes the source is in PCI view
and it adds offset to convert to CPU view.
CPU-address = PCI-address + offset. <-- Equation #A


2. PciRootBridgeIo.Configuration() returns the resource information
for a single root bridge. It carries the address range in CPU view,
and the translation offset.
However, per 

[edk2] Need help about a Hii tutorial.

[krishnaLee]

Hi,
I am learning Hii and I know few about Hii,luckly,I found a good tutorial here:
https://firmware.intel.com/sites/default/files/Lab_Material_FW.zip
source_code locate in:Lab_Material_FW.zip\FW\LabSampleCode\MyWizardDriver
https://firmware.intel.com/sites/default/files/Presentations161017.zip
Hii-tutorial locate in:Presentations161017.zip\FW\Presentations\_UEFI_Lab_Guide 
.docx,--> chapter 9.1


So I follow the chapter9.1, when I compile and build run,I find a entry at bios 
setup->Device Manager->My Wizard Driver Sample Formset,ok,
so I select it and press Enter,Error happened:
ASSERT!: [HiiDatabase] d:\edk2-vudk2017\MdePkg\Library\BaseLib\String.c (172): 
String != ((void *) 0)


I had tried more then 5 times form start to end,same failure.
I find the checkbox can't work, if I disable the VFR-file's checkbox and add a 
text,it worked:
//working VFR:
form formid = 1, title = STRING_TOKEN(STR_SAMPLE_FORM1_TITLE);
subtitle text = STRING_TOKEN(STR_SUBTITLE_TEXT);
subtitle text = STRING_TOKEN(STR_SUBTITLE_TEXT2);
  // Define a checkbox to enable / disable the device
  checkbox varid   = MWD_IfrNVData.MyWizardDriverChooseToEnable,
  //   prompt   = STRING_TOKEN(STR_CHECK_BOX_PROMPT),
  //   help = STRING_TOKEN(STR_CHECK_BOX_HELP),
  //   flags= CHECKBOX_DEFAULT ,
  //   key  = 0,
  //   default  = 1,
  //   endcheckbox;
  text
  help   = STRING_TOKEN(STR_CHECK_BOX_PROMPT),
  text   = STRING_TOKEN(STR_CHECK_BOX_HELP),
text   = STRING_TOKEN(STR_CHECK_BOX_HELP),
  flags  = 0,
  key= 0;
   endform;
endformset;


I think the tutorial has a bug,I don't how to contuine,some help will be great 
appreciate.
my platform:windows 10 x64+UDK2017+VS2015


by krishna



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

Re: [edk2] [Patch 1/1] MdePkg/UefiLib: Add EfiLocateProtocolBuffer()

[Zeng, Star]

Mike,

Depend on whether the allocated buffer will be returned to caller or not, and 
whether the implementation is matched with the function comments.
Some cases seem ok.
1. There are paired interfaces, for example AddUnicodeString and 
FreeUnicodeStringTable, etc.
2. There are clear comments, and implementation matches with comments, for 
example GetVariable2, etc.
" The returned buffer is allocated using AllocatePool().  The caller is 
responsible for freeing this buffer with FreePool(). "

More evaluation are needed for all the interfaces.


Thanks,
Star
-Original Message-
From: Kinney, Michael D 
Sent: Thursday, February 1, 2018 10:07 AM
To: Zeng, Star ; edk2-devel@lists.01.org; Kinney, Michael 
D 
Cc: Yao, Jiewen ; Gao, Liming 
Subject: RE: [edk2] [Patch 1/1] MdePkg/UefiLib: Add EfiLocateProtocolBuffer()

Star,

I agree with your observation.  And the API specifically says that 
EFI_BOOT_SERVICES.AllocatePool() is used, so I agree with your suggested change.

However, there are other APIs in the UefiLib implementation that use the 
MemoryAllocationLib services.  Should those be updated too?

Mike

> -Original Message-
> From: Zeng, Star
> Sent: Wednesday, January 31, 2018 5:32 PM
> To: Kinney, Michael D ; edk2- 
> de...@lists.01.org
> Cc: Kinney, Michael D ; Yao, Jiewen 
> ; Gao, Liming ; Zeng, Star 
> 
> Subject: RE: [edk2] [Patch 1/1] MdePkg/UefiLib: Add
> EfiLocateProtocolBuffer()
> 
> Hi Mike,
> 
> UefiLib is capable to be used by SMM Core or SMM driver before 
> SmmReadyToLock.
> 
> And the new interface has the comment below.
> 
> +The returned buffer is
> allocated using
> +
> EFI_BOOT_SERVICES.AllocatePool().  The caller is
> +responsible for freeing this
> buffer with
> +
> EFI_BOOT_SERVICES.FreePool().
> 
> But the implementation is using AllocateZeroPool() like below, that 
> will direct to gSmst->AllocatePool for SMM Core or SMM driver.
> 
> +  //
> +  // Allocate array of protocol instances  //  *Buffer = 
> + AllocateZeroPool (NoHandles * sizeof (VOID
> *));
> +  if (*Buffer == NULL) {
> +return EFI_OUT_OF_RESOURCES;
> +  }
> 
> Should the implementation use gBS->AllocatePool() +
> ZeroMem() instead of AllocateZeroPool()?
> 
> 
> Thanks,
> Star
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org]
> On Behalf Of Kinney, Michael D
> Sent: Thursday, February 1, 2018 7:33 AM
> To: edk2-devel@lists.01.org
> Cc: Kinney, Michael D ; Yao, Jiewen 
> ; Gao, Liming 
> Subject: [edk2] [Patch 1/1] MdePkg/UefiLib: Add
> EfiLocateProtocolBuffer()
> 
> From: Michael D Kinney 
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=838
> 
> Add new API to the UefiLib that locates and returns an array of 
> protocols instances that match a given protocol.
> 
> Cc: Sean Brogan 
> Cc: Jiewen Yao 
> Cc: Liming Gao 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Michael D Kinney
> 
> ---
>  MdePkg/Include/Library/UefiLib.h |  32 +++-  
> MdePkg/Library/UefiLib/UefiLib.c | 107
> ++-
>  2 files changed, 137 insertions(+), 2 deletions(-)
> 
> diff --git a/MdePkg/Include/Library/UefiLib.h
> b/MdePkg/Include/Library/UefiLib.h
> index 0b14792a0a..54bc2cc5a3 100644
> --- a/MdePkg/Include/Library/UefiLib.h
> +++ b/MdePkg/Include/Library/UefiLib.h
> @@ -12,7 +12,7 @@
>of size reduction when compiler optimization is disabled. If 
> MDEPKG_NDEBUG is
>defined, then debug and assert related macros wrapped by it are the 
> NULL implementations.
> 
> -Copyright (c) 2006 - 2017, Intel Corporation. All rights 
> reserved.
> +Copyright (c) 2006 - 2018, Intel Corporation. All rights
> reserved.
>  This program and the accompanying materials are licensed and made 
> available under  the terms and conditions of the BSD License that 
> accompanies this distribution.
>  The full text of the license may be found at @@ -1490,4 +1490,34 @@ 
> CatSPrint (
>...
>);
> 
> +/**
> +  Returns an array of protocol instance that matches the
> given protocol.
> +
> +  @param[in]  Protocol  Provides the protocol to
> search for.
> +  @param[out] NoProtocols   The number of protocols
> returned in Buffer.
> +  @param[out] BufferA pointer to the buffer to
> return the requested
> +array of protocol instances
> that match Protocol.
> +The returned buffer is
> allocated using
> +
> EFI_BOOT_SERVICES.AllocatePool().  The caller is
> +responsible for freeing 

Re: [edk2] [PATCH] UefiCpuPkg: Enhance CPU feature dependency check

[Song, BinX]

Hi Laszlo,

Thanks for your comments.
Explain the issue first:
In CpuCommonFeaturesLib.inf -> CpuCommonFeaturesLib.c -> 
CpuCommonFeaturesLibConstructor() function,
it invokes RegisterCpuFeature() to register CPU feature. Some original source 
codes is here.
  if (IsCpuFeatureSupported (CPU_FEATURE_AESNI)) {
Status = RegisterCpuFeature (
   "AESNI",
   AesniGetConfigData,
   AesniSupport,
   AesniInitialize,
   CPU_FEATURE_AESNI,
   CPU_FEATURE_END
   );
ASSERT_EFI_ERROR (Status);
  }
  if (IsCpuFeatureSupported (CPU_FEATURE_MWAIT)) {
Status = RegisterCpuFeature (
   "MWAIT",
   NULL,
   MonitorMwaitSupport,
   MonitorMwaitInitialize,
   CPU_FEATURE_MWAIT,
   CPU_FEATURE_END
   );
ASSERT_EFI_ERROR (Status);
  }

Then I update them to below.
  if (IsCpuFeatureSupported (CPU_FEATURE_AESNI)) {
Status = RegisterCpuFeature (
   "AESNI",
   AesniGetConfigData,
   AesniSupport,
   AesniInitialize,
   CPU_FEATURE_AESNI,
   CPU_FEATURE_MWAIT | CPU_FEATURE_BEFORE,
   CPU_FEATURE_END
   );
ASSERT_EFI_ERROR (Status);
  }
  if (IsCpuFeatureSupported (CPU_FEATURE_MWAIT)) {
Status = RegisterCpuFeature (
   "MWAIT",
   NULL,
   MonitorMwaitSupport,
   MonitorMwaitInitialize,
   CPU_FEATURE_MWAIT,
   CPU_FEATURE_AESNI | CPU_FEATURE_BEFORE,
   CPU_FEATURE_END
   );
ASSERT_EFI_ERROR (Status);
  }
Original function CheckCpuFeaturesDependency() will enter a dead loop and 
prompt nothing when checking and sorting them.
I think a better way is to detect this conflicted logic and give some hints to 
user, then assert(false).

For your three comments.
1. How about change to this?
  if (BeforeFlag) {
DEBUG ((DEBUG_ERROR, "Error: Feature %a before condition is invalid!", 
CurrentCpuFeature->FeatureName));
  } else {
DEBUG ((DEBUG_ERROR, "Error: Feature %a after condition is invalid!", 
CurrentCpuFeature->FeatureName));
  }
2. Will update it in V2 patch.
3. How about add a prefix before the name? 
RegisterCpuFeaturesLibSortCpuFeatures() will be unique.

Best Regards,
Bell Song

> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Wednesday, January 31, 2018 5:44 PM
> To: Song, BinX ; edk2-devel@lists.01.org
> Cc: Dong, Eric 
> Subject: Re: [PATCH] UefiCpuPkg: Enhance CPU feature dependency check
> 
> On 01/31/18 08:00, Song, BinX wrote:
> > Current CPU feature dependency check will hang on when meet below or
> > similar case:
> > if (IsCpuFeatureSupported (CPU_FEATURE_AESNI)) {
> >   Status = RegisterCpuFeature (
> >  "AESNI",
> >  AesniGetConfigData,
> >  AesniSupport,
> >  AesniInitialize,
> >  CPU_FEATURE_AESNI,
> >  CPU_FEATURE_MWAIT | CPU_FEATURE_BEFORE,
> >  CPU_FEATURE_END
> >  );
> >   ASSERT_EFI_ERROR (Status);
> > }
> > if (IsCpuFeatureSupported (CPU_FEATURE_MWAIT)) {
> >   Status = RegisterCpuFeature (
> >  "MWAIT",
> >  NULL,
> >  MonitorMwaitSupport,
> >  MonitorMwaitInitialize,
> >  CPU_FEATURE_MWAIT,
> >  CPU_FEATURE_AESNI | CPU_FEATURE_BEFORE,
> >  CPU_FEATURE_END
> >  );
> >   ASSERT_EFI_ERROR (Status);
> > }
> >
> > Solution is to separate current CPU feature dependency check into
> > sort and check two parts.
> >
> > Sort function:
> > According to CPU feature's dependency, sort all CPU features.
> > Later dependency will override previous dependency if they are conflicted.
> >
> > Check function:
> > Check sorted CPU features' relationship, ASSERT invalid relationship.
> >
> > Cc: Eric Dong 
> > Cc: Laszlo Ersek 
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Bell Song 
> > ---
> >  .../RegisterCpuFeaturesLib/CpuFeaturesInitialize.c | 271
> -
> >  .../RegisterCpuFeaturesLib/RegisterCpuFeatures.h   |   7 +
> >  .../RegisterCpuFeaturesLib.c   | 130 +-
> >  3 files changed, 278 insertions(+), 130 deletions(-)
> >
> > diff --git
> a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> > index 4d75c07..2fd0d5f 100644
> > --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> > +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> > @@ -423,6 +423,271 @@ DumpRegisterTableOnProcessor (
> >  }
> >
> >  /**
> > +  From FeatureBitMask, find the right feature entry in CPU feature list.
> > +
> > +  @param[in]  

Re: [edk2] [Patch 1/1] MdePkg/UefiLib: Add EfiLocateProtocolBuffer()

[Kinney, Michael D]

Star,

I agree with your observation.  And the API specifically says
that EFI_BOOT_SERVICES.AllocatePool() is used, so I agree with
your suggested change.

However, there are other APIs in the UefiLib implementation
that use the MemoryAllocationLib services.  Should those be 
updated too?

Mike

> -Original Message-
> From: Zeng, Star
> Sent: Wednesday, January 31, 2018 5:32 PM
> To: Kinney, Michael D ; edk2-
> de...@lists.01.org
> Cc: Kinney, Michael D ; Yao,
> Jiewen ; Gao, Liming
> ; Zeng, Star 
> Subject: RE: [edk2] [Patch 1/1] MdePkg/UefiLib: Add
> EfiLocateProtocolBuffer()
> 
> Hi Mike,
> 
> UefiLib is capable to be used by SMM Core or SMM driver
> before SmmReadyToLock.
> 
> And the new interface has the comment below.
> 
> +The returned buffer is
> allocated using
> +
> EFI_BOOT_SERVICES.AllocatePool().  The caller is
> +responsible for freeing this
> buffer with
> +
> EFI_BOOT_SERVICES.FreePool().
> 
> But the implementation is using AllocateZeroPool() like
> below, that will direct to gSmst->AllocatePool for SMM
> Core or SMM driver.
> 
> +  //
> +  // Allocate array of protocol instances
> +  //
> +  *Buffer = AllocateZeroPool (NoHandles * sizeof (VOID
> *));
> +  if (*Buffer == NULL) {
> +return EFI_OUT_OF_RESOURCES;
> +  }
> 
> Should the implementation use gBS->AllocatePool() +
> ZeroMem() instead of AllocateZeroPool()?
> 
> 
> Thanks,
> Star
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org]
> On Behalf Of Kinney, Michael D
> Sent: Thursday, February 1, 2018 7:33 AM
> To: edk2-devel@lists.01.org
> Cc: Kinney, Michael D ; Yao,
> Jiewen ; Gao, Liming
> 
> Subject: [edk2] [Patch 1/1] MdePkg/UefiLib: Add
> EfiLocateProtocolBuffer()
> 
> From: Michael D Kinney 
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=838
> 
> Add new API to the UefiLib that locates and returns
> an array of protocols instances that match a given
> protocol.
> 
> Cc: Sean Brogan 
> Cc: Jiewen Yao 
> Cc: Liming Gao 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Michael D Kinney
> 
> ---
>  MdePkg/Include/Library/UefiLib.h |  32 +++-
>  MdePkg/Library/UefiLib/UefiLib.c | 107
> ++-
>  2 files changed, 137 insertions(+), 2 deletions(-)
> 
> diff --git a/MdePkg/Include/Library/UefiLib.h
> b/MdePkg/Include/Library/UefiLib.h
> index 0b14792a0a..54bc2cc5a3 100644
> --- a/MdePkg/Include/Library/UefiLib.h
> +++ b/MdePkg/Include/Library/UefiLib.h
> @@ -12,7 +12,7 @@
>of size reduction when compiler optimization is
> disabled. If MDEPKG_NDEBUG is
>defined, then debug and assert related macros wrapped
> by it are the NULL implementations.
> 
> -Copyright (c) 2006 - 2017, Intel Corporation. All rights
> reserved.
> +Copyright (c) 2006 - 2018, Intel Corporation. All rights
> reserved.
>  This program and the accompanying materials are licensed
> and made available under
>  the terms and conditions of the BSD License that
> accompanies this distribution.
>  The full text of the license may be found at
> @@ -1490,4 +1490,34 @@ CatSPrint (
>...
>);
> 
> +/**
> +  Returns an array of protocol instance that matches the
> given protocol.
> +
> +  @param[in]  Protocol  Provides the protocol to
> search for.
> +  @param[out] NoProtocols   The number of protocols
> returned in Buffer.
> +  @param[out] BufferA pointer to the buffer to
> return the requested
> +array of protocol instances
> that match Protocol.
> +The returned buffer is
> allocated using
> +
> EFI_BOOT_SERVICES.AllocatePool().  The caller is
> +responsible for freeing this
> buffer with
> +
> EFI_BOOT_SERVICES.FreePool().
> +
> +  @retval EFI_SUCCESSThe array of protocols
> was returned in Buffer,
> + and the number of
> protocols in Buffer was
> + returned in
> NoProtocols.
> +  @retval EFI_NOT_FOUND  No protocols found.
> +  @retval EFI_OUT_OF_RESOURCES   There is not enough
> pool memory to store the
> + matching results.
> +  @retval EFI_INVALID_PARAMETER  Protocol is NULL.
> +  @retval EFI_INVALID_PARAMETER  NoProtocols is NULL.
> +  @retval EFI_INVALID_PARAMETER  Buffer is NULL.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +EfiLocateProtocolBuffer (
> +  IN  EFI_GUID  *Protocol,
> +  OUT UINTN *NoProtocols,
> +  OUT VOID  ***Buffer
> +  );
>  #endif
> diff --git a/MdePkg/Library/UefiLib/UefiLib.c
> b/MdePkg/Library/UefiLib/UefiLib.c
> index 

Re: [edk2] [Patch 0/1] MdePkg/UefiLib: Add EfiLocateProtocolBuffer()

[Kinney, Michael D]

Liming,

I looks like we still do updates if a new API is added
to the UefiLib class.

https://github.com/tianocore/edk2/commit/6212b9481d822a6765f8cd264ceb8454291948bd#diff-b3a4a95eeed10a8932c1738829d8cd7f

I will update the patch.

Mike


> -Original Message-
> From: Gao, Liming
> Sent: Wednesday, January 31, 2018 5:20 PM
> To: Kinney, Michael D ; edk2-
> de...@lists.01.org
> Cc: Sean Brogan ; Yao, Jiewen
> 
> Subject: RE: [Patch 0/1] MdePkg/UefiLib: Add
> EfiLocateProtocolBuffer()
> 
> Mike:
>   Just confirm. Do we still need to add new API in
> IntelFrameworkPkg FrameworkUefiLib? I think we don't need
> add new feature in IntelFrameworkPkg and
> IntelFrameworkModulePkg.
> 
> >-Original Message-
> >From: Kinney, Michael D
> >Sent: Thursday, February 01, 2018 7:33 AM
> >To: edk2-devel@lists.01.org
> >Cc: Sean Brogan ; Yao, Jiewen
> >; Gao, Liming
> ; Kinney,
> >Michael D 
> >Subject: [Patch 0/1] MdePkg/UefiLib: Add
> EfiLocateProtocolBuffer()
> >
> >https://bugzilla.tianocore.org/show_bug.cgi?id=838
> >
> >Add new API to the UefiLib that locates and returns
> >an array of protocols instances that match a given
> >protocol.
> >
> >Branch for review:
> >
> >https://github.com/mdkinney/edk2/tree/Bug_838_EfiLocateP
> rotocolBuffer_
> >V3
> >
> >Cc: Sean Brogan 
> >Cc: Jiewen Yao 
> >Cc: Liming Gao 
> >Contributed-under: TianoCore Contribution Agreement 1.1
> >Signed-off-by: Michael D Kinney
> 
> >
> >Michael D Kinney (1):
> >  MdePkg/UefiLib: Add EfiLocateProtocolBuffer()
> >
> > MdePkg/Include/Library/UefiLib.h |  32 +++-
> > MdePkg/Library/UefiLib/UefiLib.c | 107
> >++-
> > 2 files changed, 137 insertions(+), 2 deletions(-)
> >
> >--
> >2.14.2.windows.3

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

Re: [edk2] [PATCH] BaseTools/build_rule: pass contents of DTC_FLAGS device tree compiler

[Gao, Liming]

Ard:
  Do you consider to add DTC_FLAGS in tools_def.template file? Its value can be 
empty. So, user bases on tools_def.txt to know which option can be set. 

>-Original Message-
>From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
>Sent: Thursday, February 01, 2018 5:49 AM
>To: edk2-devel@lists.01.org
>Cc: Gao, Liming ; Zhu, Yonghong
>; ler...@redhat.com; Ard Biesheuvel
>
>Subject: [PATCH] BaseTools/build_rule: pass contents of DTC_FLAGS device
>tree compiler
>
>To allow device tree compilation to be customized at the platform or
>module level, pass the contents of the DTC_FLAGS variable on the
>dtc command line.
>
>Contributed-under: TianoCore Contribution Agreement 1.1
>Signed-off-by: Ard Biesheuvel 
>---
> BaseTools/Conf/build_rule.template | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/BaseTools/Conf/build_rule.template
>b/BaseTools/Conf/build_rule.template
>index 3e6aa8ff0f34..a5e471eb3c10 100755
>--- a/BaseTools/Conf/build_rule.template
>+++ b/BaseTools/Conf/build_rule.template
>@@ -250,7 +250,7 @@
>
> 
> "$(PP)" $(DTCPP_FLAGS) $(INC) ${src} > ${d_path}(+)${s_base}.i
>-"$(DTC)" -I dts -O dtb -o ${dst} ${d_path}(+)${s_base}.i
>+"$(DTC)" $(DTC_FLAGS) -I dts -O dtb -o ${dst} ${d_path}(+)${s_base}.i
>
> [Visual-Form-Representation-File]
> 
>--
>2.11.0

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

Re: [edk2] [Patch 1/1] MdePkg/UefiLib: Add EfiLocateProtocolBuffer()

[Zeng, Star]

Hi Mike,

UefiLib is capable to be used by SMM Core or SMM driver before SmmReadyToLock.

And the new interface has the comment below.

+The returned buffer is allocated using
+EFI_BOOT_SERVICES.AllocatePool().  The caller is
+responsible for freeing this buffer with
+EFI_BOOT_SERVICES.FreePool().

But the implementation is using AllocateZeroPool() like below, that will direct 
to gSmst->AllocatePool for SMM Core or SMM driver.

+  //
+  // Allocate array of protocol instances
+  //
+  *Buffer = AllocateZeroPool (NoHandles * sizeof (VOID *));
+  if (*Buffer == NULL) {
+return EFI_OUT_OF_RESOURCES;
+  }

Should the implementation use gBS->AllocatePool() + ZeroMem() instead of 
AllocateZeroPool()?


Thanks,
Star
-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Kinney, 
Michael D
Sent: Thursday, February 1, 2018 7:33 AM
To: edk2-devel@lists.01.org
Cc: Kinney, Michael D ; Yao, Jiewen 
; Gao, Liming 
Subject: [edk2] [Patch 1/1] MdePkg/UefiLib: Add EfiLocateProtocolBuffer()

From: Michael D Kinney 

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

Add new API to the UefiLib that locates and returns
an array of protocols instances that match a given
protocol.

Cc: Sean Brogan 
Cc: Jiewen Yao 
Cc: Liming Gao 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Michael D Kinney 
---
 MdePkg/Include/Library/UefiLib.h |  32 +++-
 MdePkg/Library/UefiLib/UefiLib.c | 107 ++-
 2 files changed, 137 insertions(+), 2 deletions(-)

diff --git a/MdePkg/Include/Library/UefiLib.h b/MdePkg/Include/Library/UefiLib.h
index 0b14792a0a..54bc2cc5a3 100644
--- a/MdePkg/Include/Library/UefiLib.h
+++ b/MdePkg/Include/Library/UefiLib.h
@@ -12,7 +12,7 @@
   of size reduction when compiler optimization is disabled. If MDEPKG_NDEBUG is
   defined, then debug and assert related macros wrapped by it are the NULL 
implementations.
 
-Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.
+Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.
 This program and the accompanying materials are licensed and made available 
under 
 the terms and conditions of the BSD License that accompanies this 
distribution.  
 The full text of the license may be found at
@@ -1490,4 +1490,34 @@ CatSPrint (
   ...
   );
 
+/**
+  Returns an array of protocol instance that matches the given protocol.
+
+  @param[in]  Protocol  Provides the protocol to search for.
+  @param[out] NoProtocols   The number of protocols returned in Buffer.
+  @param[out] BufferA pointer to the buffer to return the requested
+array of protocol instances that match Protocol.
+The returned buffer is allocated using
+EFI_BOOT_SERVICES.AllocatePool().  The caller is
+responsible for freeing this buffer with
+EFI_BOOT_SERVICES.FreePool().
+
+  @retval EFI_SUCCESSThe array of protocols was returned in Buffer,
+ and the number of protocols in Buffer was
+ returned in NoProtocols.
+  @retval EFI_NOT_FOUND  No protocols found.
+  @retval EFI_OUT_OF_RESOURCES   There is not enough pool memory to store the
+ matching results.
+  @retval EFI_INVALID_PARAMETER  Protocol is NULL.
+  @retval EFI_INVALID_PARAMETER  NoProtocols is NULL.
+  @retval EFI_INVALID_PARAMETER  Buffer is NULL.
+
+**/
+EFI_STATUS
+EFIAPI
+EfiLocateProtocolBuffer (
+  IN  EFI_GUID  *Protocol,
+  OUT UINTN *NoProtocols,
+  OUT VOID  ***Buffer
+  );
 #endif
diff --git a/MdePkg/Library/UefiLib/UefiLib.c b/MdePkg/Library/UefiLib/UefiLib.c
index a7eee01240..d86db4bd67 100644
--- a/MdePkg/Library/UefiLib/UefiLib.c
+++ b/MdePkg/Library/UefiLib/UefiLib.c
@@ -5,7 +5,7 @@
   EFI Driver Model related protocols, manage Unicode string tables for UEFI 
Drivers, 
   and print messages on the console output and standard error devices.
 
-  Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.
+  Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.
   This program and the accompanying materials
   are licensed and made available under the terms and conditions of the BSD 
License
   which accompanies this distribution.  The full text of the license may be 
found at
@@ -1605,3 +1605,108 @@ GetBestLanguage (
   //
   return NULL;
 }
+
+/**
+  Returns an array of protocol instance that matches the given protocol.
+
+  @param[in]  Protocol  Provides the protocol to search for.
+  @param[out] NoProtocols  

Re: [edk2] [Patch] BaseTools: Structure Pcd in CommandLine.

[Gao, Liming]

Reviewed-by: Liming Gao 

>-Original Message-
>From: Feng, Bob C
>Sent: Wednesday, January 31, 2018 4:49 PM
>To: edk2-devel@lists.01.org
>Cc: Feng, Bob C ; Gao, Liming 
>Subject: [Patch] BaseTools: Structure Pcd in CommandLine.
>
>Contributed-under: TianoCore Contribution Agreement 1.1
>Signed-off-by: Bob Feng 
>Cc: Liming Gao 
>---
> BaseTools/Source/Python/AutoGen/AutoGen.py| 110 +++-
> BaseTools/Source/Python/Common/Expression.py  |   2 +-
> BaseTools/Source/Python/Common/Misc.py|   2 +-
> BaseTools/Source/Python/GenFds/FfsInfStatement.py |   1 +
> BaseTools/Source/Python/GenFds/GenFds.py  |   4 +-
> BaseTools/Source/Python/Workspace/DscBuildData.py | 210
>+-
> BaseTools/Source/Python/build/build.py|   2 +-
> 7 files changed, 233 insertions(+), 98 deletions(-)
>
>diff --git a/BaseTools/Source/Python/AutoGen/AutoGen.py
>b/BaseTools/Source/Python/AutoGen/AutoGen.py
>index 1cf50e872f..405bfa145a 100644
>--- a/BaseTools/Source/Python/AutoGen/AutoGen.py
>+++ b/BaseTools/Source/Python/AutoGen/AutoGen.py
>@@ -396,100 +396,22 @@ class WorkspaceAutoGen(AutoGen):
>
> # apply SKU and inject PCDs from Flash Definition file
> for Arch in self.ArchList:
> Platform = self.BuildDatabase[self.MetaFile, Arch, Target, 
> Toolchain]
>
>-DecPcds = {}
>-DecPcdsKey = set()
>-PGen = PlatformAutoGen(self, self.MetaFile, Target, Toolchain, 
>Arch)
>-if GlobalData.BuildOptionPcd:
>-for i, pcd in enumerate(GlobalData.BuildOptionPcd):
>-if type(pcd) is tuple:
>-continue
>-(pcdname, pcdvalue) = pcd.split('=')
>-if not pcdvalue:
>-EdkLogger.error('build', AUTOGEN_ERROR, "No Value 
>specified
>for the PCD %s." % (pcdname))
>-if '.' in pcdname:
>-(TokenSpaceGuidCName, TokenCName) = pcdname.split('.')
>-HasTokenSpace = True
>-else:
>-TokenCName = pcdname
>-TokenSpaceGuidCName = ''
>-HasTokenSpace = False
>-TokenSpaceGuidCNameList = []
>-FoundFlag = False
>-PcdDatumType = ''
>-NewValue = ''
>-for package in PGen.PackageList:
>-Guids = package.Guids
>-self._GuidDict.update(Guids)
>-for package in PGen.PackageList:
>-for key in package.Pcds:
>-PcdItem = package.Pcds[key]
>-if HasTokenSpace:
>-if (PcdItem.TokenCName, 
>PcdItem.TokenSpaceGuidCName)
>== (TokenCName, TokenSpaceGuidCName):
>-PcdDatumType = PcdItem.DatumType
>-if pcdvalue.startswith('H'):
>-try:
>-pcdvalue = 
>ValueExpressionEx(pcdvalue[1:],
>PcdDatumType, self._GuidDict)(True)
>-except BadExpression, Value:
>-if Value.result > 1:
>-EdkLogger.error('Parser', 
>FORMAT_INVALID, 'PCD
>[%s.%s] Value "%s",  %s' %
>-
>(TokenSpaceGuidCName, TokenCName,
>pcdvalue, Value))
>-pcdvalue = 'H' + pcdvalue
>-NewValue =
>BuildOptionPcdValueFormat(TokenSpaceGuidCName, TokenCName,
>PcdDatumType, pcdvalue)
>-FoundFlag = True
>-else:
>-if PcdItem.TokenCName == TokenCName:
>-if not PcdItem.TokenSpaceGuidCName in
>TokenSpaceGuidCNameList:
>-if len (TokenSpaceGuidCNameList) < 1:
>-
>TokenSpaceGuidCNameList.append(PcdItem.TokenSpaceGuidCName)
>-PcdDatumType = PcdItem.DatumType
>-TokenSpaceGuidCName =
>PcdItem.TokenSpaceGuidCName
>-if pcdvalue.startswith('H'):
>-try:
>-pcdvalue = 
>ValueExpressionEx(pcdvalue[1:],
>PcdDatumType, self._GuidDict)(True)
>-except BadExpression, Value:
>-EdkLogger.error('Parser', 
>FORMAT_INVALID, 'PCD
>[%s.%s] Value "%s", %s' %
>- 

Re: [edk2] [Patch 0/1] MdePkg/UefiLib: Add EfiLocateProtocolBuffer()

[Gao, Liming]

Mike:
  Just confirm. Do we still need to add new API in IntelFrameworkPkg 
FrameworkUefiLib? I think we don't need add new feature in IntelFrameworkPkg 
and IntelFrameworkModulePkg. 

>-Original Message-
>From: Kinney, Michael D
>Sent: Thursday, February 01, 2018 7:33 AM
>To: edk2-devel@lists.01.org
>Cc: Sean Brogan ; Yao, Jiewen
>; Gao, Liming ; Kinney,
>Michael D 
>Subject: [Patch 0/1] MdePkg/UefiLib: Add EfiLocateProtocolBuffer()
>
>https://bugzilla.tianocore.org/show_bug.cgi?id=838
>
>Add new API to the UefiLib that locates and returns
>an array of protocols instances that match a given
>protocol.
>
>Branch for review:
>
>https://github.com/mdkinney/edk2/tree/Bug_838_EfiLocateProtocolBuffer_
>V3
>
>Cc: Sean Brogan 
>Cc: Jiewen Yao 
>Cc: Liming Gao 
>Contributed-under: TianoCore Contribution Agreement 1.1
>Signed-off-by: Michael D Kinney 
>
>Michael D Kinney (1):
>  MdePkg/UefiLib: Add EfiLocateProtocolBuffer()
>
> MdePkg/Include/Library/UefiLib.h |  32 +++-
> MdePkg/Library/UefiLib/UefiLib.c | 107
>++-
> 2 files changed, 137 insertions(+), 2 deletions(-)
>
>--
>2.14.2.windows.3

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

Re: [edk2] [PATCH] MdeModulePkg/Core: fix feature conflict between NX and NULL detection

[Wang, Jian J]

gCpu->SetMemoryAttributes() called by SetUefiImageMemoryAttributes() will return
without any problem if Length is 0.

Regards,
Jian


> -Original Message-
> From: Yao, Jiewen
> Sent: Tuesday, January 30, 2018 10:09 AM
> To: Wang, Jian J ; edk2-devel@lists.01.org
> Cc: Zeng, Star ; Dong, Eric ; Ni,
> Ruiyu 
> Subject: RE: [PATCH] MdeModulePkg/Core: fix feature conflict between NX and
> NULL detection
> 
> Hi Jian
> May I know how we handle MemoryMapEntry->NumberOfPages is 1?
> The lengh will be 0 in that case. Should we add additional check?
> 
> > +SetUefiImageMemoryAttributes (
> > +  MemoryMapEntry->PhysicalStart + EFI_PAGE_SIZE,
> > +  LShiftU64 (MemoryMapEntry->NumberOfPages - 1,
> > EFI_PAGE_SHIFT),
> > +  Attributes);
> 
> > -Original Message-
> > From: Wang, Jian J
> > Sent: Monday, January 29, 2018 7:10 PM
> > To: edk2-devel@lists.01.org
> > Cc: Zeng, Star ; Dong, Eric ; Yao,
> > Jiewen ; Ni, Ruiyu 
> > Subject: [PATCH] MdeModulePkg/Core: fix feature conflict between NX and
> > NULL detection
> >
> > If enabled, NX memory protection feature will mark all free memory as
> > NX (non-executable), including page 0. This will overwrite the attributes
> > of page 0 if NULL pointer detection feature is also enabled and then
> > compromise the functionality of it. The solution is skipping the NX
> > attributes setting to page 0 if NULL pointer detection feature is enabled.
> >
> > Cc: Star Zeng 
> > Cc: Eric Dong 
> > Cc: Jiewen Yao 
> > Cc: Ruiyu Ni 
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Jian J Wang 
> > ---
> >  MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 20
> > 
> >  1 file changed, 16 insertions(+), 4 deletions(-)
> >
> > diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> > b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> > index 862593f562..150167bf66 100644
> > --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> > +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> > @@ -845,10 +845,22 @@ InitializeDxeNxMemoryProtectionPolicy (
> >
> >  Attributes = GetPermissionAttributeForMemoryType
> > (MemoryMapEntry->Type);
> >  if (Attributes != 0) {
> > -  SetUefiImageMemoryAttributes (
> > -MemoryMapEntry->PhysicalStart,
> > -LShiftU64 (MemoryMapEntry->NumberOfPages, EFI_PAGE_SHIFT),
> > -Attributes);
> > +  if (MemoryMapEntry->PhysicalStart == 0 &&
> > +  PcdGet8 (PcdNullPointerDetectionPropertyMask) != 0) {
> > +//
> > +// Skip page 0 if NULL pointer detection is enabled to avoid 
> > attributes
> > +// overwritten.
> > +//
> > +SetUefiImageMemoryAttributes (
> > +  MemoryMapEntry->PhysicalStart + EFI_PAGE_SIZE,
> > +  LShiftU64 (MemoryMapEntry->NumberOfPages - 1,
> > EFI_PAGE_SHIFT),
> > +  Attributes);
> > +  } else {
> > +SetUefiImageMemoryAttributes (
> > +  MemoryMapEntry->PhysicalStart,
> > +  LShiftU64 (MemoryMapEntry->NumberOfPages, EFI_PAGE_SHIFT),
> > +  Attributes);
> > +  }
> >  }
> >  MemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry,
> > DescriptorSize);
> >}
> > --
> > 2.14.1.windows.1

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

Re: [edk2] [PATCH edk2-platforms v2 02/15] Hisilicon/D05: Add PPTT support

[Jeremy Linton]

Hi,


On 01/26/2018 02:00 AM, Ming Huang wrote:

Add Processor Properties Topology Table, PPTT include
Processor hierarchy node, Cache Type Structure and ID structure.

PPTT is needed for lscpu command to show socket information correctly.
https://bugs.linaro.org/show_bug.cgi?id=3206

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ming Huang 
Signed-off-by: Heyi Guo 
---
  Platform/Hisilicon/D05/D05.dsc |   1 +
  Platform/Hisilicon/D05/D05.fdf |   1 +
  Silicon/Hisilicon/Hi1616/Pptt/Pptt.c   | 540 
  Silicon/Hisilicon/Hi1616/Pptt/Pptt.h   |  88 
  Silicon/Hisilicon/Hi1616/Pptt/Pptt.inf |  48 ++
  5 files changed, 678 insertions(+)

diff --git a/Platform/Hisilicon/D05/D05.dsc b/Platform/Hisilicon/D05/D05.dsc
index 77a89fd..710339c 100644
--- a/Platform/Hisilicon/D05/D05.dsc
+++ b/Platform/Hisilicon/D05/D05.dsc
@@ -506,6 +506,7 @@
MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf
  
Silicon/Hisilicon/Hi1616/D05AcpiTables/AcpiTablesHi1616.inf

+  Silicon/Hisilicon/Hi1616/Pptt/Pptt.inf
Silicon/Hisilicon/Drivers/AcpiPlatformDxe/AcpiPlatformDxe.inf
  
#

diff --git a/Platform/Hisilicon/D05/D05.fdf b/Platform/Hisilicon/D05/D05.fdf
index 78ab0c8..97de4d2 100644
--- a/Platform/Hisilicon/D05/D05.fdf
+++ b/Platform/Hisilicon/D05/D05.fdf
@@ -241,6 +241,7 @@ READ_LOCK_STATUS   = TRUE
INF Silicon/Hisilicon/Drivers/HisiAcpiPlatformDxe/AcpiPlatformDxe.inf
  
INF RuleOverride=ACPITABLE Silicon/Hisilicon/Hi1616/D05AcpiTables/AcpiTablesHi1616.inf

+  INF Silicon/Hisilicon/Hi1616/Pptt/Pptt.inf
INF Silicon/Hisilicon/Drivers/AcpiPlatformDxe/AcpiPlatformDxe.inf
  
#

diff --git a/Silicon/Hisilicon/Hi1616/Pptt/Pptt.c 
b/Silicon/Hisilicon/Hi1616/Pptt/Pptt.c
new file mode 100644
index 000..71c456c
--- /dev/null
+++ b/Silicon/Hisilicon/Hi1616/Pptt/Pptt.c
@@ -0,0 +1,540 @@
+/** @file
+*
+*  Copyright (c) 2018, Hisilicon Limited. All rights reserved.
+*  Copyright (c) 2018, Linaro Limited. All rights reserved.
+*
+*  This program and the accompanying materials
+*  are licensed and made available under the terms and conditions of the BSD 
License
+*  which accompanies this distribution.  The full text of the license may be 
found at
+*  http://opensource.org/licenses/bsd-license.php
+*
+*  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+*  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
IMPLIED.
+*
+*  Based on the files under Platform/ARM/JunoPkg/AcpiTables/
+*
+**/
+
+#include "Pptt.h"
+
+EFI_ACPI_TABLE_PROTOCOL   *mAcpiTableProtocol = NULL;
+EFI_ACPI_SDT_PROTOCOL *mAcpiSdtProtocol   = NULL;
+
+EFI_ACPI_DESCRIPTION_HEADER mPpttHeader =
+  ARM_ACPI_HEADER (
+EFI_ACPI_6_2_PROCESSOR_PROPERTIES_TOPOLOGY_TABLE_STRUCTURE_SIGNATURE,
+EFI_ACPI_DESCRIPTION_HEADER,
+EFI_ACPI_6_2_PROCESSOR_PROPERTIES_TOPOLOGY_TABLE_REVISION
+  );
+
+EFI_ACPI_6_2_PPTT_STRUCTURE_ID mPpttSocketType2[PPTT_SOCKET_COMPONENT_NO] =
+{
+  {2, sizeof (EFI_ACPI_6_2_PPTT_STRUCTURE_ID), {0, 0}, 0, 0, 0, 0, 0, 0}
+};


I missed this the first time. I think at a minimum the VENDOR_ID needs 
to contain something other than 0 if your populating a type2 structure. 
Did I miss it getting overridden somewhere?


Checking the ACPI id, registry there is an existing entry for Hisilicon 
Technologies Co.., LTD and its 'HISI'.


I would suggest you use that, and come up with a plan for how the 
remaining fields are provided.




+
+EFI_ACPI_6_2_PPTT_STRUCTURE_CACHE mPpttCacheType1[PPTT_CACHE_NO];
+
+
+STATIC
+VOID
+InitCacheInfo (
+  VOID
+  )
+{
+  UINT8Index;
+  EFI_ACPI_6_2_PPTT_STRUCTURE_CACHE_ATTRIBUTES Type1Attributes;
+  CSSELR_DATA  CsselrData;
+  CCSIDR_DATA  CcsidrData;
+
+  for (Index = 0; Index < PPTT_CACHE_NO; Index++) {
+CsselrData.Data = 0;
+CcsidrData.Data = 0;
+SetMem (
+  ,
+  sizeof (EFI_ACPI_6_2_PPTT_STRUCTURE_CACHE_ATTRIBUTES),
+  0
+  );
+
+if (Index == 0) { //L1I
+  CsselrData.Bits.InD = 1;
+  CsselrData.Bits.Level = 0;
+  Type1Attributes.CacheType  = 1;
+} else if (Index == 1) {
+  Type1Attributes.CacheType  = 0;
+  CsselrData.Bits.Level = Index - 1;
+} else {
+  Type1Attributes.CacheType  = 2;
+  CsselrData.Bits.Level = Index - 1;
+}
+
+CcsidrData.Data = ReadCCSIDR (CsselrData.Data);
+
+if (CcsidrData.Bits.Wa == 1) {
+  Type1Attributes.AllocationType  = PPTT_TYPE1_ALLOCATION_WRITE;
+  if (CcsidrData.Bits.Ra == 1) {
+Type1Attributes.AllocationType  = PPTT_TYPE1_ALLOCATION_READ_WRITE;
+  }
+}
+
+if (CcsidrData.Bits.Wt == 1) {
+  Type1Attributes.WritePolicy = 1;
+}
+DEBUG ((DEBUG_INFO,
+"[Acpi PPTT] Level = %x!CcsidrData = %x!\n",
+CsselrData.Bits.Level,
+CcsidrData.Data));
+
+

[edk2] [staging/edk2-test Patch] MdePkgUnitTest: Add UefiLib unit tests

[Kinney, Michael D]

Add UefiLib unit tests for the new API
EfiLocateProtocolBuffer().

Cc: Sean Brogan 
Cc: Liming Gao 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Michael D Kinney 
---
 MdePkgUnitTest/MdePkgUnitTest.dsc   |   8 +-
 MdePkgUnitTest/UefiLib/UefiLibUnitTests.c   | 266 
 MdePkgUnitTest/UefiLib/UefiLibUnitTests.inf |  46 +
 3 files changed, 319 insertions(+), 1 deletion(-)
 create mode 100644 MdePkgUnitTest/UefiLib/UefiLibUnitTests.c
 create mode 100644 MdePkgUnitTest/UefiLib/UefiLibUnitTests.inf

diff --git a/MdePkgUnitTest/MdePkgUnitTest.dsc 
b/MdePkgUnitTest/MdePkgUnitTest.dsc
index d98483020b..3f021639b2 100644
--- a/MdePkgUnitTest/MdePkgUnitTest.dsc
+++ b/MdePkgUnitTest/MdePkgUnitTest.dsc
@@ -1,7 +1,7 @@
 ## @file
 # This Package provides unit tests for the MdePkg
 #
-# Copyright (c) 2017, Intel Corporation. All rights reserved.
+# Copyright (c) 2017 - 2018, Intel Corporation. All rights reserved.
 #
 # This program and the accompanying materials are licensed and made available 
under
 # the terms and conditions of the BSD License which accompanies this 
distribution.
@@ -33,6 +33,7 @@
   BaseLib|MdePkg/Library/BaseLib/BaseLib.inf
   BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
   PrintLib|MdePkg/Library/BasePrintLib/BasePrintLib.inf
+
   DebugLib|MdePkg/Library/UefiDebugLibStdErr/UefiDebugLibStdErr.inf
   
DebugPrintErrorLevelLib|MdePkg/Library/BaseDebugPrintErrorLevelLib/BaseDebugPrintErrorLevelLib.inf
   PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
@@ -50,8 +51,13 @@
   
UefiHiiServicesLib|MdeModulePkg/Library/UefiHiiServicesLib/UefiHiiServicesLib.inf
   SortLib|MdeModulePkg/Library/UefiSortLib/UefiSortLib.inf
 
+[PcdsFixedAtBuild]
+  gEfiMdePkgTokenSpaceGuid.PcdDebugPrintErrorLevel|0x8047
+  gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask|0x1f
+
 [Components]
   MdePkgUnitTest/SafeIntLib/SafeIntLibUnitTests.inf {
 
   SafeIntLib|MdePkg/Library/BaseSafeIntLib/BaseSafeIntLib.inf
   }
+  MdePkgUnitTest/UefiLib/UefiLibUnitTests.inf
diff --git a/MdePkgUnitTest/UefiLib/UefiLibUnitTests.c 
b/MdePkgUnitTest/UefiLib/UefiLibUnitTests.c
new file mode 100644
index 00..244715c2ca
--- /dev/null
+++ b/MdePkgUnitTest/UefiLib/UefiLibUnitTests.c
@@ -0,0 +1,266 @@
+/** @file
+  Uefi Shell based Application that Unit Tests the UefiLib
+
+  Copyright (c) 2018, Intel Corporation. All rights reserved.
+  This program and the accompanying materials
+  are licensed and made available under the terms and conditions of the BSD 
License
+  which accompanies this distribution.  The full text of the license may be 
found at
+  http://opensource.org/licenses/bsd-license.php
+
+  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+
+
+#define UNIT_TEST_APP_NAMEL"Uefi Lib Unit Test Application"
+#define UNIT_TEST_APP_VERSION L"0.1"
+
+EFI_GUID  mUnitTestProtocolGuid = { 0xe5f282af, 0x895c, 0x4ece, { 0xae, 0xf0, 
0x19, 0xbf, 0x6f, 0xdd, 0x41, 0x2 } };
+
+//
+// Conversion function tests:
+//
+UNIT_TEST_STATUS
+EFIAPI
+TestEfiLocateProtocolBuffer (
+  IN UNIT_TEST_FRAMEWORK_HANDLE  Framework,
+  IN UNIT_TEST_CONTEXT   Context
+  )
+{
+  EFI_STATUS  Status;
+  UINTN   NoProtocols;
+  VOID**Buffer;
+  EFI_HANDLE  Handle1;
+  EFI_HANDLE  Handle2;
+  EFI_HANDLE  Handle3;
+  UINT32  Instance2;
+  UINT32  Instance3;
+
+  //
+  // NULL Protocol should result in EFI_INVALID_PARAMETER
+  //
+  Status = EfiLocateProtocolBuffer (NULL, , );
+  UT_ASSERT_EQUAL (EFI_INVALID_PARAMETER, Status);
+
+  //
+  // NULL NoProtocols should result in EFI_INVALID_PARAMETER
+  //
+  Status = EfiLocateProtocolBuffer (, NULL, );
+  UT_ASSERT_EQUAL (EFI_INVALID_PARAMETER, Status);
+
+  //
+  // NULL Buffer should result in EFI_INVALID_PARAMETER
+  //
+  Status = EfiLocateProtocolBuffer (, , NULL);
+  UT_ASSERT_EQUAL (EFI_INVALID_PARAMETER, Status);
+
+  //
+  // All NULL should result in EFI_INVALID_PARAMETER
+  //
+  Status = EfiLocateProtocolBuffer (NULL, NULL, NULL);
+  UT_ASSERT_EQUAL (EFI_INVALID_PARAMETER, Status);
+
+  //
+  // Request for unknown protocol should result in EFI_NOT_FOUND
+  //
+  NoProtocols = 0;
+  Buffer = NULL;
+  Status = EfiLocateProtocolBuffer (, , 
);
+  UT_ASSERT_EQUAL (EFI_NOT_FOUND, Status);
+
+  //
+  // Request for Loaded Image Protocol should result in EFI_SUCCESS
+  //
+  NoProtocols = 0;
+  Buffer = NULL;
+  Status = EfiLocateProtocolBuffer (, 
, );
+  UT_ASSERT_EQUAL (EFI_SUCCESS, Status);
+  UT_ASSERT_TRUE (NoProtocols > 0);
+  UT_ASSERT_NOT_NULL (Buffer);
+
+  if (Buffer != NULL) {
+FreePool (Buffer);
+  }
+
+  //
+  // Install 1 instance of a protocol
+  //
+  Handle1 = 

[edk2] [Patch 0/1] MdePkg/UefiLib: Add EfiLocateProtocolBuffer()

[Kinney, Michael D]

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

Add new API to the UefiLib that locates and returns
an array of protocols instances that match a given
protocol.

Branch for review:

https://github.com/mdkinney/edk2/tree/Bug_838_EfiLocateProtocolBuffer_V3

Cc: Sean Brogan 
Cc: Jiewen Yao 
Cc: Liming Gao 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Michael D Kinney 

Michael D Kinney (1):
  MdePkg/UefiLib: Add EfiLocateProtocolBuffer()

 MdePkg/Include/Library/UefiLib.h |  32 +++-
 MdePkg/Library/UefiLib/UefiLib.c | 107 ++-
 2 files changed, 137 insertions(+), 2 deletions(-)

-- 
2.14.2.windows.3

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

[edk2] [Patch 1/1] MdePkg/UefiLib: Add EfiLocateProtocolBuffer()

[Kinney, Michael D]

From: Michael D Kinney 

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

Add new API to the UefiLib that locates and returns
an array of protocols instances that match a given
protocol.

Cc: Sean Brogan 
Cc: Jiewen Yao 
Cc: Liming Gao 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Michael D Kinney 
---
 MdePkg/Include/Library/UefiLib.h |  32 +++-
 MdePkg/Library/UefiLib/UefiLib.c | 107 ++-
 2 files changed, 137 insertions(+), 2 deletions(-)

diff --git a/MdePkg/Include/Library/UefiLib.h b/MdePkg/Include/Library/UefiLib.h
index 0b14792a0a..54bc2cc5a3 100644
--- a/MdePkg/Include/Library/UefiLib.h
+++ b/MdePkg/Include/Library/UefiLib.h
@@ -12,7 +12,7 @@
   of size reduction when compiler optimization is disabled. If MDEPKG_NDEBUG is
   defined, then debug and assert related macros wrapped by it are the NULL 
implementations.
 
-Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.
+Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.
 This program and the accompanying materials are licensed and made available 
under 
 the terms and conditions of the BSD License that accompanies this 
distribution.  
 The full text of the license may be found at
@@ -1490,4 +1490,34 @@ CatSPrint (
   ...
   );
 
+/**
+  Returns an array of protocol instance that matches the given protocol.
+
+  @param[in]  Protocol  Provides the protocol to search for.
+  @param[out] NoProtocols   The number of protocols returned in Buffer.
+  @param[out] BufferA pointer to the buffer to return the requested
+array of protocol instances that match Protocol.
+The returned buffer is allocated using
+EFI_BOOT_SERVICES.AllocatePool().  The caller is
+responsible for freeing this buffer with
+EFI_BOOT_SERVICES.FreePool().
+
+  @retval EFI_SUCCESSThe array of protocols was returned in Buffer,
+ and the number of protocols in Buffer was
+ returned in NoProtocols.
+  @retval EFI_NOT_FOUND  No protocols found.
+  @retval EFI_OUT_OF_RESOURCES   There is not enough pool memory to store the
+ matching results.
+  @retval EFI_INVALID_PARAMETER  Protocol is NULL.
+  @retval EFI_INVALID_PARAMETER  NoProtocols is NULL.
+  @retval EFI_INVALID_PARAMETER  Buffer is NULL.
+
+**/
+EFI_STATUS
+EFIAPI
+EfiLocateProtocolBuffer (
+  IN  EFI_GUID  *Protocol,
+  OUT UINTN *NoProtocols,
+  OUT VOID  ***Buffer
+  );
 #endif
diff --git a/MdePkg/Library/UefiLib/UefiLib.c b/MdePkg/Library/UefiLib/UefiLib.c
index a7eee01240..d86db4bd67 100644
--- a/MdePkg/Library/UefiLib/UefiLib.c
+++ b/MdePkg/Library/UefiLib/UefiLib.c
@@ -5,7 +5,7 @@
   EFI Driver Model related protocols, manage Unicode string tables for UEFI 
Drivers, 
   and print messages on the console output and standard error devices.
 
-  Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.
+  Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.
   This program and the accompanying materials
   are licensed and made available under the terms and conditions of the BSD 
License
   which accompanies this distribution.  The full text of the license may be 
found at
@@ -1605,3 +1605,108 @@ GetBestLanguage (
   //
   return NULL;
 }
+
+/**
+  Returns an array of protocol instance that matches the given protocol.
+
+  @param[in]  Protocol  Provides the protocol to search for.
+  @param[out] NoProtocols   The number of protocols returned in Buffer.
+  @param[out] BufferA pointer to the buffer to return the requested
+array of protocol instances that match Protocol.
+The returned buffer is allocated using
+EFI_BOOT_SERVICES.AllocatePool().  The caller is
+responsible for freeing this buffer with
+EFI_BOOT_SERVICES.FreePool().
+
+  @retval EFI_SUCCESSThe array of protocols was returned in Buffer,
+ and the number of protocols in Buffer was
+ returned in NoProtocols.
+  @retval EFI_NOT_FOUND  No protocols found.
+  @retval EFI_OUT_OF_RESOURCES   There is not enough pool memory to store the
+ matching results.
+  @retval EFI_INVALID_PARAMETER  Protocol is NULL.
+  @retval EFI_INVALID_PARAMETER  NoProtocols is NULL.
+  @retval EFI_INVALID_PARAMETER  Buffer is NULL.
+
+**/
+EFI_STATUS
+EFIAPI
+EfiLocateProtocolBuffer (
+  IN  EFI_GUID  *Protocol,
+  OUT UINTN *NoProtocols,
+  OUT VOID  ***Buffer
+  )
+{
+  EFI_STATUS  Status;
+  UINTN   

[edk2] [PATCH] BaseTools/build_rule: pass contents of DTC_FLAGS device tree compiler

[Ard Biesheuvel]

To allow device tree compilation to be customized at the platform or
module level, pass the contents of the DTC_FLAGS variable on the
dtc command line.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel 
---
 BaseTools/Conf/build_rule.template | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/BaseTools/Conf/build_rule.template 
b/BaseTools/Conf/build_rule.template
index 3e6aa8ff0f34..a5e471eb3c10 100755
--- a/BaseTools/Conf/build_rule.template
+++ b/BaseTools/Conf/build_rule.template
@@ -250,7 +250,7 @@
 
 
 "$(PP)" $(DTCPP_FLAGS) $(INC) ${src} > ${d_path}(+)${s_base}.i
-"$(DTC)" -I dts -O dtb -o ${dst} ${d_path}(+)${s_base}.i
+"$(DTC)" $(DTC_FLAGS) -I dts -O dtb -o ${dst} ${d_path}(+)${s_base}.i
 
 [Visual-Form-Representation-File]
 
-- 
2.11.0

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

[edk2] [PATCH v2 1/1] MdePkg/Include: Add management mode FV file type and depex.

[Supreeth Venkatesh]

As per PI specification v1.6,
As per section 2.1.4.1,
The following file types exist:
Table 3. Defined File Types
Name   Value
EFI_FV_FILETYPE_RAW0x01
EFI_FV_FILETYPE_FREEFORM   0x02
EFI_FV_FILETYPE_SECURITY_CORE  0x03
EFI_FV_FILETYPE_PEI_CORE   0x04
EFI_FV_FILETYPE_DXE_CORE   0x05
EFI_FV_FILETYPE_PEIM   0x06
EFI_FV_FILETYPE_DRIVER 0x07
EFI_FV_FILETYPE_COMBINED_PEIM_DRIVER   0x08
EFI_FV_FILETYPE_APPLICATION0x09
EFI_FV_FILETYPE_MM 0x0A
EFI_FV_FILETYPE_FIRMWARE_VOLUME_IMAGE  0x0B
EFI_FV_FILETYPE_COMBINED_MM_DXE0x0C
EFI_FV_FILETYPE_MM_CORE0x0D
EFI_FV_FILETYPE_MM_STANDALONE  0x0E
EFI_FV_FILETYPE_MM_CORE_STANDALONE 0x0F

The following new section type is added:
EFI_SECTION_MM_DEPEX

This patch adds the management mode FV file type and depex.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Achin Gupta 
Signed-off-by: Supreeth Venkatesh 
Reviewed-by: Jiewen Yao 
---
 MdePkg/Include/Pi/PiFirmwareFile.h | 16 
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/MdePkg/Include/Pi/PiFirmwareFile.h 
b/MdePkg/Include/Pi/PiFirmwareFile.h
index b982c9eda3..808202cd22 100644
--- a/MdePkg/Include/Pi/PiFirmwareFile.h
+++ b/MdePkg/Include/Pi/PiFirmwareFile.h
@@ -71,10 +71,17 @@ typedef UINT8 EFI_FFS_FILE_STATE;
 #define EFI_FV_FILETYPE_DRIVER0x07
 #define EFI_FV_FILETYPE_COMBINED_PEIM_DRIVER  0x08
 #define EFI_FV_FILETYPE_APPLICATION   0x09
-#define EFI_FV_FILETYPE_SMM   0x0A
+#define EFI_FV_FILETYPE_MM0x0A
+#define EFI_FV_FILETYPE_SMM   EFI_FV_FILETYPE_MM
 #define EFI_FV_FILETYPE_FIRMWARE_VOLUME_IMAGE 0x0B
-#define EFI_FV_FILETYPE_COMBINED_SMM_DXE  0x0C
-#define EFI_FV_FILETYPE_SMM_CORE  0x0D
+#define EFI_FV_FILETYPE_COMBINED_MM_DXE   0x0C
+#define EFI_FV_FILETYPE_COMBINED_SMM_DXE  EFI_FV_FILETYPE_COMBINED_MM_DXE
+#define EFI_FV_FILETYPE_MM_CORE   0x0D
+#define EFI_FV_FILETYPE_SMM_CORE  EFI_FV_FILETYPE_MM_CORE
+#define EFI_FV_FILETYPE_MM_STANDALONE 0x0E
+#define EFI_FV_FILETYPE_SMM_STANDALONEEFI_FV_FILETYPE_MM_STANDALONE
+#define EFI_FV_FILETYPE_MM_CORE_STANDALONE0x0F
+#define EFI_FV_FILETYPE_SMM_CORE_STANDALONE   
EFI_FV_FILETYPE_MM_CORE_STANDALONE
 #define EFI_FV_FILETYPE_OEM_MIN   0xc0
 #define EFI_FV_FILETYPE_OEM_MAX   0xdf
 #define EFI_FV_FILETYPE_DEBUG_MIN 0xe0
@@ -217,7 +224,8 @@ typedef UINT8 EFI_SECTION_TYPE;
 #define EFI_SECTION_FREEFORM_SUBTYPE_GUID 0x18
 #define EFI_SECTION_RAW   0x19
 #define EFI_SECTION_PEI_DEPEX 0x1B
-#define EFI_SECTION_SMM_DEPEX 0x1C
+#define EFI_SECTION_MM_DEPEX  0x1C
+#define EFI_SECTION_SMM_DEPEX EFI_SECTION_MM_DEPEX
 
 ///
 /// Common section header.
-- 
2.14.1

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

Re: [edk2] [Patch] MdePkg SafeIntLib: Update API definition to use the same output name

[Kinney, Michael D]

Reviewed-by: Michael D Kinney 

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org]
> On Behalf Of Liming Gao
> Sent: Wednesday, January 31, 2018 4:27 AM
> To: edk2-devel@lists.01.org
> Subject: [edk2] [Patch] MdePkg SafeIntLib: Update API
> definition to use the same output name
> 
> In SafeUintnToChar8(), update its output parameter name.
> Update pui8Result --> Result to match its library
> implementation
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Liming Gao 
> ---
>  MdePkg/Include/Library/SafeIntLib.h | 12 ++--
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/MdePkg/Include/Library/SafeIntLib.h
> b/MdePkg/Include/Library/SafeIntLib.h
> index 5839301..8c5003f 100644
> --- a/MdePkg/Include/Library/SafeIntLib.h
> +++ b/MdePkg/Include/Library/SafeIntLib.h
> @@ -375,7 +375,7 @@ RETURN_STATUS
>  EFIAPI
>  SafeInt16ToUint8 (
>IN INT16 Operand,
> -  OUT UINT8 *pui8Result
> +  OUT UINT8 *Result
>);
> 
>  /**
> @@ -564,7 +564,7 @@ RETURN_STATUS
>  EFIAPI
>  SafeUint16ToUint8 (
>IN UINT16 Operand,
> -  OUT UINT8 *pui8Result
> +  OUT UINT8 *Result
>);
> 
>  /**
> @@ -672,7 +672,7 @@ RETURN_STATUS
>  EFIAPI
>  SafeInt32ToUint8 (
>IN INT32 Operand,
> -  OUT UINT8 *pui8Result
> +  OUT UINT8 *Result
>);
> 
>  /**
> @@ -889,7 +889,7 @@ RETURN_STATUS
>  EFIAPI
>  SafeUint32ToUint8 (
>IN UINT32 Operand,
> -  OUT UINT8 *pui8Result
> +  OUT UINT8 *Result
>);
> 
>  /**
> @@ -1078,7 +1078,7 @@ RETURN_STATUS
>  EFIAPI
>  SafeIntnToUint8 (
>IN INTN Operand,
> -  OUT UINT8 *pui8Result
> +  OUT UINT8 *Result
>);
> 
>  /**
> @@ -1321,7 +1321,7 @@ RETURN_STATUS
>  EFIAPI
>  SafeUintnToUint8 (
>IN UINTN Operand,
> -  OUT UINT8 *pui8Result
> +  OUT UINT8 *Result
>);
> 
>  /**
> --
> 2.8.0.windows.1
> 
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [RFC v5 0/8] Stack trace support in X64 exception handling

[Paulo Alcantara]

"Yao, Jiewen"  writes:

Hi Jiewen,

> ===
> OVMF IA32: DXE worked, but SMM not.
>
> Since the page fault is only occurring in IA32 with no paging enabled 
> (default case), I suspect that when we don't have paging, we are unable 
> to successfully validate the memory address when it's i.e. outside SMRAM 
> - that is, we don't know when to stop unwinding the stack.
> ===
>
> For IA32 SMM, I am a little confused.
> We unconditionally setup page table for SMM, no matter it is IA32 or X64.
>
> If you find a SMM driver running in a page-disable environment, it means, the 
> SMM CORE load the driver in SMRAM, but the SMM CPU driver has not rebased the 
> CPU yet.
> SMM driver is still using the PageTable/GDT/IDT setup by DXE CPU
> driver, not SMM CPU driver.

OK - thanks for clarifying that.

> Would you please double confirm what you have observed?
>
> You can just check the boot log file to see if PiSmmCpu driver has run
> or not.

Sure. I will do it and then get back to you once I got the results.

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

[edk2] [Patch] DSC spec: Add flexible PCD value format into spec

[Yonghong Zhu]

Cc: Liming Gao 
Cc: Michael Kinney 
Cc: Kevin W Shaw 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Yonghong Zhu 
---
 3_edk_ii_dsc_file_format/310_pcd_sections.md   | 160 +
 .../33_platform_dsc_definition.md  |  74 +++---
 2 files changed, 157 insertions(+), 77 deletions(-)

diff --git a/3_edk_ii_dsc_file_format/310_pcd_sections.md 
b/3_edk_ii_dsc_file_format/310_pcd_sections.md
index 2af42cc..18a243d 100644
--- a/3_edk_ii_dsc_file_format/310_pcd_sections.md
+++ b/3_edk_ii_dsc_file_format/310_pcd_sections.md
@@ -98,13 +98,11 @@ is permissible to list multiple architectures in a single 
method section as in:
 It is permissible to list a PCD in a common architecture section and also list
 it in an architecturally modified section. In this case, the value in the
 architectural section overrides the value specified in the common section.
 
 The PCD values must match the datum type declared for a given PCD in the DEC
-file. While a PCD of datum type `BOOLEAN` is permitted to have a `1` or a `0`
-(instead of TRUE or FALSE) in the value field, a PCD of type UINT* cannot use
-`TRUE` or `FALSE` for values.
+file.
 
 PCDs with a data type of `VOID`* can optionally provide the maximum size of the
 value. If not provided, the maximum length will be calculated as the largest of
 the size of the data in the DSC file, the size of the data in the INF file or
 the size of the data in the DEC file that declares the PCD.
@@ -220,21 +218,24 @@ fields that are separated by the pipe character, "|".
 ::=  [ ]*
::= 
 ::= 
  ::= {} {} {}
   ::=   [ ] 
-  ::= if (pcddatumtype == "BOOLEAN"): {} {}
-elif (pcddatumtype == "UINT8"): {}
-{} elif (pcddatumtype == "UINT16"):
-{} {} elif (pcddatumtype ==
-"UINT32"): {} {} elif
-(pcddatumtype == "UINT64"): {} {}
+  ::= if (pcddatumtype == "BOOLEAN"):
+  {} {}
+elif (pcddatumtype == "UINT8"):
+  {} {}
+elif (pcddatumtype == "UINT16"):
+  {} {}
+elif (pcddatumtype == "UINT32"):
+  {} {}
+elif (pcddatumtype == "UINT64"):
+  {} {}
 else:
- []
+   []
::=  "VOID*"  {} {}
-   ::= {} {} {}
-{} {}
+   ::= {} {} {}
 ```
 
  Parameters
 
 **_Expression_**
@@ -325,21 +326,24 @@ of the DSC file.
 ::=  [ ]*
::= 
 ::= 
  ::= {} {} {}
   ::=   [ ] 
-  ::= if (pcddatumtype == "BOOLEAN"): {} {}
-elif (pcddatumtype == "UINT8"): {}
-{} elif (pcddatumtype == "UINT16"):
-{} {} elif (pcddatumtype ==
-"UINT32"): {} {} elif
-(pcddatumtype == "UINT64"): {} {}
+  ::= if (pcddatumtype == "BOOLEAN"):
+  {} {}
+elif (pcddatumtype == "UINT8"):
+  {} {}
+elif (pcddatumtype == "UINT16"):
+  {} {}
+elif (pcddatumtype == "UINT32"):
+  {} {}
+elif (pcddatumtype == "UINT64"):
+  {} {}
 else:
- []
+   []
::=  {} {}
-   ::= {} {} {}
-{} {}
+   ::= {} {} {}
 ```
 
  Parameters
 
 **_Expression_**
@@ -458,41 +462,58 @@ sections of the DSC file.
   ::= "."  ["." ]
  ::=  [ ]*
 ::= 
  ::= 
::=  [ ] 
-   ::= if (pcddatumtype == "BOOLEAN"): {} {}
- elif (pcddatumtype == "UINT8"): {}
- {} elif (pcddatumtype == "UINT16"):
- {} {} elif (pcddatumtype ==
- "UINT32"): {} {} elif
- (pcddatumtype == "UINT64"): {}
- {} else:
-  []
+   ::= if (pcddatumtype == "BOOLEAN"):
+   {} {}
+ elif (pcddatumtype == "UINT8"):
+   {} {}
+ elif (pcddatumtype == "UINT16"):
+   {} {}
+ elif (pcddatumtype == "UINT32"):
+   {} {}
+ elif (pcddatumtype == "UINT64"):
+   {} {}
+ else:
+[]
 ::=  "VOID*" [ ]
   ::= {} {}
-::= {} {} {} {}
- {}
+::= {} {} {}
::=[ ] 
   ::= {} {"*"}
-::= if (pcddatumtype == "BOOLEAN"): {} {}
- elif (pcddatumtype == "UINT8"): {}
- {} elif (pcddatumtype == "UINT16"):

[edk2] [Patch] INF spec: Add flexible PCD value format into spec

[Yonghong Zhu]

Cc: Liming Gao 
Cc: Michael Kinney 
Cc: Kevin W Shaw 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Yonghong Zhu 
---
 .../32_component_inf_definition.md | 71 +-
 3_edk_ii_inf_file_format/38_pcd_sections.md| 39 +++-
 2 files changed, 79 insertions(+), 31 deletions(-)

diff --git a/3_edk_ii_inf_file_format/32_component_inf_definition.md 
b/3_edk_ii_inf_file_format/32_component_inf_definition.md
index 72bcf2c..7af9072 100644
--- a/3_edk_ii_inf_file_format/32_component_inf_definition.md
+++ b/3_edk_ii_inf_file_format/32_component_inf_definition.md
@@ -111,15 +111,16 @@ The following are common definitions used by multiple 
section types.
   ::= (0-9)
::= (a-zA-Z_)
  ::=  *
   ::=  # A valid C variable name.
  ::= (0x21 - 0x7E)
- ::= [{0x21} {(0x23 - 0x5B)} {(0x5D - 0x7E)}
- {}]*
+ ::= [{0x21} {(0x23 - 0x26)} {(0x28 - 0x5B)}
+ {(0x5D - 0x7E)} {}]*
::= 0x22
+   ::= 0x27
  ::= "\" {"n"} {"t"} {"f"} {"r"} {"b"} {"0"} {"\"}
- {}
+ {} {}
::= {} {}
  ::= *
 ::= +
 ::= 0x09
   ::= 0x20
@@ -137,15 +138,16 @@ The following are common definitions used by multiple 
section types.
 ::= [ * * ]*
 ::= 
  ::= 
  ::= {} {}
::=  * 
-::= ["L"] 
+::=  * 
+::= {} {}
::=  [{} {}]+ 
   ::=  "#" [] +
 ::= "#"  +
-  ::= "L" 
+  ::= "L" {} {}
::= (a-fA-F0-9)
 ::= {"0x"} {"0X"}  
   ::= {"0x"} {"0X"} *
  ::= "0x"  
   ::= ? ? ?
@@ -181,14 +183,14 @@ The following are common definitions used by multiple 
section types.
   ::= {} {}
::= (\x1 - \x)
::= (1-18446744073709551615)
   ::= {} {}
 ::= {"TRUE"} {"true"} {"True"} {"0x1"} {"0x01"} {"1"}
-  {}
::= {"FALSE"} {"false"} {"False"} {"0x0"}
   {"0x00"} {"0"}
-::= {} {}
+ ::= {} {}
+::= {} {"{""}"}
::= (A-Z)(A-Z0-9_)*
 ::= "$("  ")"
  ::=  "." 
 ::= 
  ::= 
@@ -209,14 +211,45 @@ The following are common definitions used by multiple 
section types.

 ::= (0-255)
   ::= (0-65535)
  ::= (0-4294967295)
  ::= (0-18446744073709551615)
- ::= {} {}
-::= {} {}
-::= {} {}
-::= {} {}
+::= {} {} {}
+  {} {}
+   ::= {} {} {}
+  {} {}
+   ::= {} {} {}
+  {} {}
+   ::= {} {} {}
+  {} {}
+ ::= {} {"{""}"}
+::= {}
+  {"{" [ ]*"}"}
+::= {}
+  {"{" [ ]*"}"}
+::= {}
+  {"{" [ ]*"}"}
+   ::= {} {} {}
+   ::= "{"[]  
+   [ [] ]*"}"
+::= {} {} {} 
+::= {} {} {} {}
+  {} {}
+ ::= "GUID("  ")"
+ ::= {  }
+  {} {}
+  ::= "DEVICE_PATH("  ")"
+   ::= A double quoted string that follow the device path
+  as string format defined in UEFI Specification 2.6
+  Section 9.6
+ ::= {} {} {} 
{}
+::= "UINT8("  ")"
+   ::= "UINT16("  ")"
+   ::= "UINT32("  ")"
+   ::= "UINT64("  ")"
+   ::= "LABEL("  ")"
+  ::= "OFFSET_OF("  ")"
   ::= {"BASE"} {"SEC"} {"PEI_CORE"} {"PEIM"}
   {"DXE_CORE"} {"DXE_DRIVER"} {"SMM_CORE"}
   {"DXE_RUNTIME_DRIVER"} {"DXE_SAL_DRIVER"}
   {"DXE_SMM_DRIVER"} {"UEFI_DRIVER"}
   {"UEFI_APPLICATION"} {"USER_DEFINED"}
@@ -233,10 +266,18 @@ The following are common definitions used by multiple 
section types.
   {"SMM_CORE"} {"DXE_SMM_DRIVER"}
   {"UEFI_DRIVER"} {"UEFI_APPLICATION"}
 ```
 
 **
+**Note:** When using CString, UnicodeString or byte array format as
+UINT8/UINT16/UINT32/UINT64 values, please make sure they fit in the
+target type's size, otherwise tool would report failure.
+**
+**Note:** LABEL() macro in byte arrays to tag the byte offset of a
+location in a byte array. OFFSET_OF() macro in byte arrays that returns
+the byte offset of a LABEL() declared in a byte array.
+**
 **Note:** When using 

[edk2] [Patch] DEC spec: Add flexible PCD value format into spec

[Yonghong Zhu]

Cc: Liming Gao 
Cc: Michael Kinney 
Cc: Kevin W Shaw 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Yonghong Zhu 
---
 3_edk_ii_dec_file_format/310_pcd_sections.md   | 14 ++---
 .../32_package_declaration_dec_definitions.md  | 70 +-
 2 files changed, 63 insertions(+), 21 deletions(-)

diff --git a/3_edk_ii_dec_file_format/310_pcd_sections.md 
b/3_edk_ii_dec_file_format/310_pcd_sections.md
index 488f17f..36c32ff 100644
--- a/3_edk_ii_dec_file_format/310_pcd_sections.md
+++ b/3_edk_ii_dec_file_format/310_pcd_sections.md
@@ -113,24 +113,24 @@ PCDs listed in `PcdsFeatureFlag` sections must only be 
listed in
   [ ]
   [] 
 ::=  {} {} {}
  ::= {} {} {} {}
  ::=  
- ::=   "BOOLEAN"
+ ::=   "BOOLEAN"
  ::= {} {}
 ::=  
-::=   "UINT8"
+::= {} {}  "UINT8"
::=  
-   ::=   "UINT16"
+   ::= {} {}  "UINT16"
::=  
-   ::=   "UINT32"
+   ::= {} {}  "UINT32"
::=  
-   ::=   "UINT64"
+   ::= {} {}  "UINT64"
   ::=  
::=   "VOID*"
-  ::= {} {}
-   ::= 
+  ::= {} {}
+   ::= {} {}
   ::=  {+} {} {+}
   ::= "#"  "@Prompt   
::= "#"  "@ValidRange"   
 ::= [ ] 
 ::= "#"  "@ValidList"   
diff --git a/3_edk_ii_dec_file_format/32_package_declaration_dec_definitions.md 
b/3_edk_ii_dec_file_format/32_package_declaration_dec_definitions.md
index 8e473f2..d7f9067 100644
--- a/3_edk_ii_dec_file_format/32_package_declaration_dec_definitions.md
+++ b/3_edk_ii_dec_file_format/32_package_declaration_dec_definitions.md
@@ -86,15 +86,16 @@ DEC file (for example, `` statements are not 
permitted).
::= (0-9)
 ::= (a-zA-Z_)
   ::=  *
::=  # A valid C variable name.
   ::= (0x21 - 0x7E)
-  ::= [{0x21} {(0x23 - 0x5B)} {(0x5D - 0x7E)}
-  {}]*
+  ::= [{0x21} {(0x23 - 0x26)} {(0x28 - 0x5B)}
+  {(0x5D - 0x7E)} {}]*
 ::= 0x22
+::= 0x27
   ::= "\" {"n"} {"t"} {"f"} {"r"} {"b"} {"0"} {"\"}
-  {}
+  {} {}
 ::= {} {}
   ::= *
  ::= +
  ::= 0x09
::= 0x20
@@ -112,15 +113,16 @@ DEC file (for example, `` statements are not 
permitted).
  ::= [ * * ]*
  ::= 
   ::= 
   ::= {} {}
 ::=  * 
- ::= ["L"] 
+ ::=  * 
+ ::= {} {}
 ::=  [{} {}]+ 
::=  "#" [ ] +
  ::= "#"   +
-   ::= "L" 
+   ::= "L" {} {}
 ::= (a-fA-F0-9)
  ::= {"0x"} {"0X"} [] 
::= {"0x"} {"0X"} +
   ::= "0x" [0]*  
::= ? ? ?
@@ -157,11 +159,12 @@ DEC file (for example, `` statements are not 
permitted).
   ::= {} {}
 ::= {"TRUE"} {"true"} {"True"} {"0x1"}
   {"0x01"} {"1"}
::= {"FALSE"} {"false"} {"False"} {"0x0"}
   {"0x00"} {"0"}
-::= {} {}
+ ::= {} {}
+::= {} {"{""}"}
::= (A-Z)(A-Z0-9_)*
 ::= "$("  ")"
  ::=  "." 
 ::= 
  ::= 
@@ -182,14 +185,45 @@ DEC file (for example, `` statements are not 
permitted).

 ::= (0-255)
   ::= (0-65535)
  ::= (0-4294967295)
  ::= (0-18446744073709551615)
- ::= {} {}
-::= {} {}
-::= {} {}
-::= {} {}
+::= {} {} {}
+  {} {}
+   ::= {} {} {}
+  {} {}
+   ::= {} {} {}
+  {} {}
+   ::= {} {} {}
+  {} {}
+ ::= {} {"{""}"}
+::= {}
+  {"{" [ ]*"}"}
+::= {}
+  {"{" [ ]*"}"}
+::= {}
+  {"{" [ ]*"}"}
+   ::= {} {} {}
+   ::= "{"[]  
+   [ [] ]*"}"
+::= {} {} {} 
+::= {} {} {} {}
+  {} {}
+ ::= "GUID("  ")"
+ ::= {  }
+  {} {}
+  ::= "DEVICE_PATH("  ")"
+   ::= A double quoted string that follow the device path
+  as string format defined in UEFI Specification 2.6
+  Section 9.6
+ ::= {} {} {} 
{}
+::= "UINT8("  ")"
+   ::= "UINT16("  ")"
+   ::= "UINT32("  ")"
+   ::= "UINT64("  ")"
+   ::= "LABEL("  ")"
+  ::= "OFFSET_OF("  ")"
  

[edk2] [Patch] Build spec: Add flexible PCD value format into spec

[Yonghong Zhu]

Cc: Liming Gao 
Cc: Michael Kinney 
Cc: Kevin W Shaw 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Yonghong Zhu 
---
 7_build_environment/73_guided_tools.md |  8 +--
 .../82_auto-generation_process.md  | 18 +++
 appendix_d_buildexe_command/d4_usage.md| 60 +++---
 3 files changed, 65 insertions(+), 21 deletions(-)

diff --git a/7_build_environment/73_guided_tools.md 
b/7_build_environment/73_guided_tools.md
index a8881d3..50119f1 100644
--- a/7_build_environment/73_guided_tools.md
+++ b/7_build_environment/73_guided_tools.md
@@ -119,21 +119,21 @@ file required by the build system is provided in the 
appendix, VPD Tool.
 ```
 
   If using automatic offset feature, the build tools byte-align numeric values,
   while `VOID*` PCD types will be aligned using the following rules:
 
-  * ASCII strings, "string", will be byte aligned.
-  * Unicode strings, L"string" will be two-byte aligned.
+  * ASCII strings, "string" or 'string', will be byte aligned.
+  * Unicode strings, L"string" or L'string' will be two-byte aligned.
   * Byte arrays, {0x00, 0x01} will be 8-byte aligned.
 
   If the developer manually assigns offset values in the DSC file, the 
developer
   must follow the same rules.
 
   **
   **Note:** If a developer manually sets the offset of a `VOID*` PCD with
-  Unicode string, L"string", style to a value that is not 2-byte aligned, then
-  an error is generated and the build halts.
+  Unicode string, L"string"/L'string' style to a value that is not 2-byte 
aligned,
+  then an error is generated and the build halts.
   **
   **Note:** If a developer manually sets the offset of a `VOID*` PCD with byte
   array {} style to a value that is not 8-byte aligned, then a warning is
   generated, but the build will continue.
   **
diff --git a/8_pre-build_autogen_stage/82_auto-generation_process.md 
b/8_pre-build_autogen_stage/82_auto-generation_process.md
index f610185..5a950d7 100644
--- a/8_pre-build_autogen_stage/82_auto-generation_process.md
+++ b/8_pre-build_autogen_stage/82_auto-generation_process.md
@@ -875,41 +875,41 @@ A PCD value set on the command-line has the highest 
precedence. It overrides
 all instances of the PCD value specified in the DSC or FDF file. The following
 is the syntax to override the value of a PCD on the command line:
 
 `--pcd [.]=`
 
-For `VOID*` type PCDs, `` supports the following syntax:
+`` supports the following syntax:
 
-* ASCII string value for a `VOID*` PCD
+* ASCII string value for a PCD
 
   `--pcd  [.]="String"`
+  `--pcd  [.]='String'`
 
-*  Unicode string value for a `VOID*` PCD
+* Unicode string value for a PCD
 
   `--pcd  [.]=L"String"`
+  `--pcd  [.]=L'String'`
 
-*  Byte array value for a `VOID*` PCD
+* Byte array value for a PCD
 
   `--pcd  [.]= H"{0x1, 0x2}"`
 
 **
 **Note:** The EDK II meta-data specs have changed to permit a PCD entry (or any
 other entry) to be listed only one time per section.
 **
-**Caution:** Dynamic and DynamicEx `VOID*` VPD PCD array values must be hex 
byte
-arrays. Using a Registry or C format GUID value in the value field of a `VOID*`
-VPD PCD is not permitted.
-**
 
 If the maximum size of a `VOID*` PCD is not specified in the DSC file, then the
 maximum size is calculated based on the largest size of 1) the string or array
 in the DSC file, 2) the string or array in the INF file and 3) the string or
 array in the DEC file. If the value is a quoted text string, the size of the
 string will be incremented by one to handle string termination. If the quoted
 string is preceded by L, as in `L"This is a string"`, then the size of the 
string
 will be incremented by two to handle unicode string termination. If the value
-is a byte array, then the size of the byte array is not modified.
+is a byte array, then the size of the byte array is not modified. If the value 
is
+a single quoted string, as in 'string' or L'string', the size of the string 
doesn't
+need to include string null termination character.
 
 For example, if the string in the DSC file is `L"DSC Length"`, the INF file has
 `L"Module Length"` and the DEC file declares the default as `L"Length"`, then
 the maximum size that will be allocated for this PCD will be 28 bytes (`
 L"Module Length"` 26 bytes, 2 bytes for null termination character).
diff --git a/appendix_d_buildexe_command/d4_usage.md 
b/appendix_d_buildexe_command/d4_usage.md
index c901266..3dbfc33 100644
--- a/appendix_d_buildexe_command/d4_usage.md
+++ b/appendix_d_buildexe_command/d4_usage.md
@@ -195,24 +195,68 @@ precedence over PCD provided in DSC, FDF, INF, and DEC 
files.
 
 ```c
::= "--pcd"  ["=" ] 
   ::= 0x20
  ::= +
+  ::= *
+  ::= "," *
+::= (a-fA-F0-9)
+   ::= A valid C variable name.
  ::= [ 

Re: [edk2] [Patch] MdePkg SafeIntLib: Update API definition to use the same output name

[Laszlo Ersek]

On 01/31/18 13:27, Liming Gao wrote:
> In SafeUintnToChar8(), update its output parameter name.
> Update pui8Result --> Result to match its library implementation
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Liming Gao 
> ---
>  MdePkg/Include/Library/SafeIntLib.h | 12 ++--
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/MdePkg/Include/Library/SafeIntLib.h 
> b/MdePkg/Include/Library/SafeIntLib.h
> index 5839301..8c5003f 100644
> --- a/MdePkg/Include/Library/SafeIntLib.h
> +++ b/MdePkg/Include/Library/SafeIntLib.h
> @@ -375,7 +375,7 @@ RETURN_STATUS
>  EFIAPI
>  SafeInt16ToUint8 (
>IN INT16 Operand,
> -  OUT UINT8 *pui8Result
> +  OUT UINT8 *Result
>);
>  
>  /**
> @@ -564,7 +564,7 @@ RETURN_STATUS
>  EFIAPI
>  SafeUint16ToUint8 (
>IN UINT16 Operand,
> -  OUT UINT8 *pui8Result
> +  OUT UINT8 *Result
>);
>  
>  /**
> @@ -672,7 +672,7 @@ RETURN_STATUS
>  EFIAPI
>  SafeInt32ToUint8 (
>IN INT32 Operand,
> -  OUT UINT8 *pui8Result
> +  OUT UINT8 *Result
>);
>  
>  /**
> @@ -889,7 +889,7 @@ RETURN_STATUS
>  EFIAPI
>  SafeUint32ToUint8 (
>IN UINT32 Operand,
> -  OUT UINT8 *pui8Result
> +  OUT UINT8 *Result
>);
>  
>  /**
> @@ -1078,7 +1078,7 @@ RETURN_STATUS
>  EFIAPI
>  SafeIntnToUint8 (
>IN INTN Operand,
> -  OUT UINT8 *pui8Result
> +  OUT UINT8 *Result
>);
>  
>  /**
> @@ -1321,7 +1321,7 @@ RETURN_STATUS
>  EFIAPI
>  SafeUintnToUint8 (
>IN UINTN Operand,
> -  OUT UINT8 *pui8Result
> +  OUT UINT8 *Result
>);
>  
>  /**
> 

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

Re: [edk2] [RFC] Remove X86 .asm and .S assembly files in EDK2

[Laszlo Ersek]

On 01/31/18 12:06, Gao, Liming wrote:
> Edk2 has used nasm assembly file for all tool chains. So, IA32 and
> X64 .asm and .S assembly files can be removed if their nasm files are
> ready. It can save the maintain effort and avoid the confuse.
> 
> 
> 
> If you have any comments on this change, please let me know.

My comment is that I greatly welcome the removal of .S and .asm files :)

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

[edk2] [Patch] MdePkg SafeIntLib: Update API definition to use the same output name

[Liming Gao]

In SafeUintnToChar8(), update its output parameter name.
Update pui8Result --> Result to match its library implementation

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Liming Gao 
---
 MdePkg/Include/Library/SafeIntLib.h | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/MdePkg/Include/Library/SafeIntLib.h 
b/MdePkg/Include/Library/SafeIntLib.h
index 5839301..8c5003f 100644
--- a/MdePkg/Include/Library/SafeIntLib.h
+++ b/MdePkg/Include/Library/SafeIntLib.h
@@ -375,7 +375,7 @@ RETURN_STATUS
 EFIAPI
 SafeInt16ToUint8 (
   IN INT16 Operand,
-  OUT UINT8 *pui8Result
+  OUT UINT8 *Result
   );
 
 /**
@@ -564,7 +564,7 @@ RETURN_STATUS
 EFIAPI
 SafeUint16ToUint8 (
   IN UINT16 Operand,
-  OUT UINT8 *pui8Result
+  OUT UINT8 *Result
   );
 
 /**
@@ -672,7 +672,7 @@ RETURN_STATUS
 EFIAPI
 SafeInt32ToUint8 (
   IN INT32 Operand,
-  OUT UINT8 *pui8Result
+  OUT UINT8 *Result
   );
 
 /**
@@ -889,7 +889,7 @@ RETURN_STATUS
 EFIAPI
 SafeUint32ToUint8 (
   IN UINT32 Operand,
-  OUT UINT8 *pui8Result
+  OUT UINT8 *Result
   );
 
 /**
@@ -1078,7 +1078,7 @@ RETURN_STATUS
 EFIAPI
 SafeIntnToUint8 (
   IN INTN Operand,
-  OUT UINT8 *pui8Result
+  OUT UINT8 *Result
   );
 
 /**
@@ -1321,7 +1321,7 @@ RETURN_STATUS
 EFIAPI
 SafeUintnToUint8 (
   IN UINTN Operand,
-  OUT UINT8 *pui8Result
+  OUT UINT8 *Result
   );
 
 /**
-- 
2.8.0.windows.1

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

Re: [edk2] [Patch] BaseTools: Support multiple .h file

[Gao, Liming]

Reviewed-by: Liming Gao 

>-Original Message-
>From: Feng, Bob C
>Sent: Monday, January 29, 2018 2:10 PM
>To: edk2-devel@lists.01.org
>Cc: Feng, Bob C ; Gao, Liming 
>Subject: [Patch] BaseTools: Support multiple .h file
>
>for structure Pcd declaration in DEC file.
>
>Contributed-under: TianoCore Contribution Agreement 1.1
>Signed-off-by: Bob Feng 
>Cc: Liming Gao 
>---
> BaseTools/Source/Python/Workspace/BuildClassObject.py | 2 +-
> BaseTools/Source/Python/Workspace/DecBuildData.py | 2 +-
> BaseTools/Source/Python/Workspace/DscBuildData.py | 8 
> BaseTools/Source/Python/Workspace/MetaFileParser.py   | 4 +++-
> 4 files changed, 9 insertions(+), 7 deletions(-)
>
>diff --git a/BaseTools/Source/Python/Workspace/BuildClassObject.py
>b/BaseTools/Source/Python/Workspace/BuildClassObject.py
>index 3afb27a9c0..0e1161c96f 100644
>--- a/BaseTools/Source/Python/Workspace/BuildClassObject.py
>+++ b/BaseTools/Source/Python/Workspace/BuildClassObject.py
>@@ -113,11 +113,11 @@ class StructurePcd(PcdClassObject):
> if SkuInfoList is None: SkuInfoList={}
> if validateranges is None: validateranges=[]
> if validlists is None: validlists=[]
> if expressions is None : expressions=[]
> super(StructurePcd, self).__init__(Name, Guid, Type, DatumType, Value,
>Token, MaxDatumSize, SkuInfoList, IsOverrided, GuidValue, validateranges,
>validlists, expressions)
>-self.StructuredPcdIncludeFile = StructuredPcdIncludeFile
>+self.StructuredPcdIncludeFile = [] if StructuredPcdIncludeFile is None
>else StructuredPcdIncludeFile
> self.PackageDecs = Packages
> self.DefaultStoreName = [default_store]
> self.DefaultValues = collections.OrderedDict({})
> self.PcdMode = None
> self.SkuOverrideValues = collections.OrderedDict({})
>diff --git a/BaseTools/Source/Python/Workspace/DecBuildData.py
>b/BaseTools/Source/Python/Workspace/DecBuildData.py
>index f6b908dee6..2fd3820dcc 100644
>--- a/BaseTools/Source/Python/Workspace/DecBuildData.py
>+++ b/BaseTools/Source/Python/Workspace/DecBuildData.py
>@@ -374,11 +374,11 @@ class DecBuildData(PackageBuildClassObject):
> for pcdname in s_pcd_set:
> dep_pkgs = []
> struct_pcd = StructurePcd()
> for item,LineNo in s_pcd_set[pcdname]:
> if "" in item.TokenCName:
>-struct_pcd.StructuredPcdIncludeFile = item.DefaultValue
>+
>struct_pcd.StructuredPcdIncludeFile.append(item.DefaultValue)
> elif "" in item.TokenCName:
> dep_pkgs.append(item.DefaultValue)
> elif item.DatumType == item.TokenCName:
> struct_pcd.copy(item)
> struct_pcd.TokenValue = 
> struct_pcd.TokenValue.strip("{").strip()
>diff --git a/BaseTools/Source/Python/Workspace/DscBuildData.py
>b/BaseTools/Source/Python/Workspace/DscBuildData.py
>index 256fdd6875..0384b96997 100644
>--- a/BaseTools/Source/Python/Workspace/DscBuildData.py
>+++ b/BaseTools/Source/Python/Workspace/DscBuildData.py
>@@ -1669,14 +1669,14 @@ class DscBuildData(PlatformBuildClassObject):
> CApp = PcdMainCHeader
>
> Includes = {}
> for PcdName in StructuredPcds:
> Pcd = StructuredPcds[PcdName]
>-IncludeFile = Pcd.StructuredPcdIncludeFile
>-if IncludeFile not in Includes:
>-Includes[IncludeFile] = True
>-CApp = CApp + '#include <%s>\n' % (IncludeFile)
>+for IncludeFile in Pcd.StructuredPcdIncludeFile:
>+if IncludeFile not in Includes:
>+Includes[IncludeFile] = True
>+CApp = CApp + '#include <%s>\n' % (IncludeFile)
> CApp = CApp + '\n'
>
> for PcdName in StructuredPcds:
> Pcd = StructuredPcds[PcdName]
> if not Pcd.SkuOverrideValues:
>diff --git a/BaseTools/Source/Python/Workspace/MetaFileParser.py
>b/BaseTools/Source/Python/Workspace/MetaFileParser.py
>index 8f4b5e5cc1..4359ba4b33 100644
>--- a/BaseTools/Source/Python/Workspace/MetaFileParser.py
>+++ b/BaseTools/Source/Python/Workspace/MetaFileParser.py
>@@ -1889,26 +1889,28 @@ class DecParser(MetaFileParser):
> self._ValueList[0] = self._CurrentStructurePcdName
>
> if "|" not in self._CurrentLine:
> if "" == self._CurrentLine:
> self._include_flag = True
>+self._package_flag = False
> self._ValueList = None
> return
> if "" == self._CurrentLine:
> self._package_flag = True
> self._ValueList = None
>+self._include_flag = False
> return
>
> if self._include_flag:
> 

Re: [edk2] [Patch] BaseTool: Add comments in PcdValueInit.c.

[Gao, Liming]

Reviewed-by: Liming Gao 

>-Original Message-
>From: Feng, Bob C
>Sent: Wednesday, January 31, 2018 5:45 PM
>To: edk2-devel@lists.01.org
>Cc: Feng, Bob C ; Gao, Liming 
>Subject: [Patch] BaseTool: Add comments in PcdValueInit.c.
>
>Add Comments for __FLEXIBLE_SIZE () statement.
>
>Contributed-under: TianoCore Contribution Agreement 1.1
>Signed-off-by: Bob Feng 
>Cc: Liming Gao 
>---
> BaseTools/Source/Python/Workspace/DscBuildData.py | 10 ++
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
>diff --git a/BaseTools/Source/Python/Workspace/DscBuildData.py
>b/BaseTools/Source/Python/Workspace/DscBuildData.py
>index 012e16a488..214e4fd569 100644
>--- a/BaseTools/Source/Python/Workspace/DscBuildData.py
>+++ b/BaseTools/Source/Python/Workspace/DscBuildData.py
>@@ -1325,42 +1325,44 @@ class DscBuildData(PlatformBuildClassObject):
> for FieldName in FieldList:
> FieldName = "." + FieldName
> IsArray = 
> self.IsFieldValueAnArray(FieldList[FieldName.strip(".")][0])
> if IsArray:
> Value, ValueSize = ParseFieldValue
>(FieldList[FieldName.strip(".")][0])
>-CApp = CApp + '  __FLEXIBLE_SIZE(Size, %s, %s, %d /
>__ARRAY_ELEMENT_SIZE(%s, %s) + ((%d %%
>__ARRAY_ELEMENT_SIZE(%s, %s)) ? 1 : 0));\n' % (Pcd.DatumType,
>FieldName.strip("."), ValueSize, Pcd.DatumType, FieldName.strip("."),
>ValueSize, Pcd.DatumType, FieldName.strip("."));
>+CApp = CApp + '  __FLEXIBLE_SIZE(Size, %s, %s, %d /
>__ARRAY_ELEMENT_SIZE(%s, %s) + ((%d %%
>__ARRAY_ELEMENT_SIZE(%s, %s)) ? 1 : 0));  // From %s Line %d Value %s \n' %
>(Pcd.DatumType, FieldName.strip("."), ValueSize, Pcd.DatumType,
>FieldName.strip("."), ValueSize, Pcd.DatumType, FieldName.strip("."),
>FieldList[FieldName.strip(".")][1], FieldList[FieldName.strip(".")][2],
>FieldList[FieldName.strip(".")][0]);
> else:
> NewFieldName = ''
>+FieldName_ori = FieldName.strip('.')
> while '[' in  FieldName:
> NewFieldName = NewFieldName + 
> FieldName.split('[', 1)[0] +
>'[0]'
> ArrayIndex = int(FieldName.split('[', 
> 1)[1].split(']', 1)[0])
> FieldName = FieldName.split(']', 1)[1]
> FieldName = NewFieldName + FieldName
> while '[' in FieldName:
> FieldName = FieldName.rsplit('[', 1)[0]
>-CApp = CApp + '  __FLEXIBLE_SIZE(Size, %s, %s, 
>%d);\n' %
>(Pcd.DatumType, FieldName.strip("."), ArrayIndex + 1)
>+CApp = CApp + '  __FLEXIBLE_SIZE(Size, %s, %s, 
>%d); //
>From %s Line %d Value %s\n' % (Pcd.DatumType, FieldName.strip("."),
>ArrayIndex + 1, FieldList[FieldName_ori][1], FieldList[FieldName_ori][2],
>FieldList[FieldName_ori][0])
> for skuname in self.SkuIdMgr.GetSkuChain(SkuName):
> inherit_OverrideValues = Pcd.SkuOverrideValues[skuname]
> for FieldList in 
> [inherit_OverrideValues.get(DefaultStoreName)]:
> if not FieldList:
> continue
> for FieldName in FieldList:
> FieldName = "." + FieldName
> IsArray =
>self.IsFieldValueAnArray(FieldList[FieldName.strip(".")][0])
> if IsArray:
> Value, ValueSize = ParseFieldValue
>(FieldList[FieldName.strip(".")][0])
>-CApp = CApp + '  __FLEXIBLE_SIZE(Size, %s, %s, %d 
>/
>__ARRAY_ELEMENT_SIZE(%s, %s) + ((%d %%
>__ARRAY_ELEMENT_SIZE(%s, %s)) ? 1 : 0));\n' % (Pcd.DatumType,
>FieldName.strip("."), ValueSize, Pcd.DatumType, FieldName.strip("."),
>ValueSize, Pcd.DatumType, FieldName.strip("."));
>+CApp = CApp + '  __FLEXIBLE_SIZE(Size, %s, %s, %d 
>/
>__ARRAY_ELEMENT_SIZE(%s, %s) + ((%d %%
>__ARRAY_ELEMENT_SIZE(%s, %s)) ? 1 : 0)); // From %s Line %d Value %s\n' %
>(Pcd.DatumType, FieldName.strip("."), ValueSize, Pcd.DatumType,
>FieldName.strip("."), ValueSize, Pcd.DatumType, FieldName.strip("."),
>FieldList[FieldName.strip(".")][1], FieldList[FieldName.strip(".")][2],
>FieldList[FieldName.strip(".")][0]);
> else:
> NewFieldName = ''
>+FieldName_ori = FieldName.strip('.')
> while '[' in  FieldName:
> NewFieldName = NewFieldName + 
> FieldName.split('[', 1)[0] +
>'[0]'
> ArrayIndex = int(FieldName.split('[', 
> 1)[1].split(']', 1)[0])
> FieldName = FieldName.split(']', 1)[1]
> FieldName = NewFieldName 

Re: [edk2] [Patch] BaseTool: Enhance error handling.

[Gao, Liming]

Reviewed-by: Liming Gao 

>-Original Message-
>From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>BobCF
>Sent: Wednesday, January 31, 2018 5:32 PM
>To: edk2-devel@lists.01.org
>Cc: Gao, Liming 
>Subject: [edk2] [Patch] BaseTool: Enhance error handling.
>
>Contributed-under: TianoCore Contribution Agreement 1.1
>Signed-off-by: Bob Feng 
>Cc: Liming Gao 
>---
> BaseTools/Source/Python/Workspace/DscBuildData.py  | 116
>-
> .../Source/Python/Workspace/MetaFileParser.py  |   3 +
> 2 files changed, 70 insertions(+), 49 deletions(-)
>
>diff --git a/BaseTools/Source/Python/Workspace/DscBuildData.py
>b/BaseTools/Source/Python/Workspace/DscBuildData.py
>index bc77d1a2b4..9d8da6ed85 100644
>--- a/BaseTools/Source/Python/Workspace/DscBuildData.py
>+++ b/BaseTools/Source/Python/Workspace/DscBuildData.py
>@@ -856,11 +856,11 @@ class DscBuildData(PlatformBuildClassObject):
> if not Valid:
> EdkLogger.error('build', FORMAT_INVALID, ErrStr, 
> File=self.MetaFile,
>Line=LineNo,
> ExtraData="%s.%s" % (TokenSpaceGuid, 
> PcdCName))
> if PcdType in (MODEL_PCD_DYNAMIC_DEFAULT,
>MODEL_PCD_DYNAMIC_EX_DEFAULT):
> if self._DecPcds[PcdCName, TokenSpaceGuid].DatumType.strip() 
> !=
>ValueList[1].strip():
>-EdkLogger.error('build', FORMAT_INVALID, ErrStr ,
>File=self.MetaFile, Line=LineNo,
>+EdkLogger.error('build', FORMAT_INVALID, "Pcd datumtype 
>used
>in DSC file is not the same as its declaration in DEC file." , 
>File=self.MetaFile,
>Line=LineNo,
> ExtraData="%s.%s|%s" % (TokenSpaceGuid, 
> PcdCName,
>Setting))
> return ValueList
>
> def _FilterPcdBySkuUsage(self,Pcds):
> available_sku = self.SkuIdMgr.AvailableSkuIdSet
>@@ -1009,17 +1009,20 @@ class DscBuildData(PlatformBuildClassObject):
> StrPcdSet = self.GetStructurePcdInfo(S_PcdSet)
> S_pcd_set = {}
> for str_pcd in StrPcdSet:
> str_pcd_obj = Pcds.get((str_pcd[1], str_pcd[0]), None)
> str_pcd_dec = self._DecPcds.get((str_pcd[1], str_pcd[0]), None)
>+if not isinstance (str_pcd_dec, StructurePcd):
>+EdkLogger.error('build', PARSER_ERROR,
>+"Pcd (%s.%s) is not declared as Structure PCD in 
>DEC files. Arch:
>['%s']" % (str_pcd[0], str_pcd[1], self._Arch),
>+File=self.MetaFile,Line = 
>StrPcdSet[str_pcd][0][5])
> if str_pcd_dec:
> str_pcd_obj_str = StructurePcd()
> str_pcd_obj_str.copy(str_pcd_dec)
> if str_pcd_obj:
> str_pcd_obj_str.copy(str_pcd_obj)
>-if str_pcd_obj.DefaultValue:
>-str_pcd_obj_str.DefaultFromDSC = 
>str_pcd_obj.DefaultValue
>+str_pcd_obj_str.DefaultFromDSC = str_pcd_obj_str.DefaultValue
> for str_pcd_data in StrPcdSet[str_pcd]:
> if str_pcd_data[3] in SkuIds:
> str_pcd_obj_str.AddOverrideValue(str_pcd_data[2],
>str(str_pcd_data[6]), 'DEFAULT' if str_pcd_data[3] == 'COMMON' else
>str_pcd_data[3],'STANDARD' if str_pcd_data[4] == 'COMMON' else
>str_pcd_data[4], self.MetaFile.File,LineNo=str_pcd_data[5])
> S_pcd_set[str_pcd[1], str_pcd[0]] = str_pcd_obj_str
> else:
>@@ -1153,11 +1156,11 @@ class DscBuildData(PlatformBuildClassObject):
> if SkuName not in AvailableSkuIdSet:
> EdkLogger.error('build ', PARAMETER_INVALID, 'Sku %s is not 
> defined
>in [SkuIds] section' % SkuName,
> File=self.MetaFile, Line=Dummy5)
> if SkuName in (self.SkuIdMgr.SystemSkuId, 'DEFAULT', 'COMMON'):
> if "." not in TokenSpaceGuid:
>-PcdSet.add((PcdCName, TokenSpaceGuid, SkuName, Dummy4))
>+PcdSet.add((PcdCName, TokenSpaceGuid, SkuName, Dummy5))
> PcdDict[Arch, PcdCName, TokenSpaceGuid, SkuName] = Setting
>
> for PcdCName, TokenSpaceGuid, SkuName, Dummy4 in PcdSet:
> Setting = PcdDict[self._Arch, PcdCName, TokenSpaceGuid, SkuName]
> if Setting == None:
>@@ -1262,16 +1265,13 @@ class DscBuildData(PlatformBuildClassObject):
>
> def ExecuteCommand (self, Command):
> try:
> Process = subprocess.Popen(Command, stdout=subprocess.PIPE,
>stderr=subprocess.PIPE, shell=True)
> except:
>-print 'ERROR: Can not execute command:', Command
>-sys.exit(1)
>+EdkLogger.error('Build', COMMAND_FAILURE, 'Can not execute
>command: %s' % Command)
> Result = Process.communicate()
>-if Process.returncode <> 0:
>-print 'ERROR: Can not collect output from command:', 

Re: [edk2] [PATCH 0/3] UefiCpuPkg/PiSmmCpuDxeSmm: fix IA32 SmmStartup() regression on KVM

[Laszlo Ersek]

On 01/30/18 16:33, Laszlo Ersek wrote:
> Repo:   https://github.com/lersek/edk2.git
> Branch: smm_startup_ia32_nocond
>
> This small series fixes the IA32 SMM regression on KVM that I reported
> recently. The first two patches are cleanups (no functional changes),
> the third patch is the fix.
>
> Cc: Eric Dong 
> Cc: Jian J Wang 
> Cc: Jiewen Yao 
> Cc: Paolo Bonzini 
> Cc: Ruiyu Ni 
>
> Thanks
> Laszlo
>
> Laszlo Ersek (3):
>   UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()
>   UefiCpuPkg/PiSmmCpuDxeSmm: remove unneeded DBs from IA32 SmmStartup()
>   UefiCpuPkg/PiSmmCpuDxeSmm: eliminate conditional jump in IA32
> SmmStartup()
>
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm | 28 +---
>  1 file changed, 13 insertions(+), 15 deletions(-)
>

Code differences between the posted version and the pushed version,
according to Ray's comments for patch #3, displayed with "git diff
--word-diff" (look for [-...-] and {+...+} markers):

> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm 
> b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
> index 102e0bdbabc8..d64fcd48d03e 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
> @@ -47,8 +47,8 @@ ASM_PFX(SmmStartup):
> mov eax, 0x8001 ; read capability
> cpuid
> mov ebx, edx; rdmsr will change edx. keep it in 
> ebx.
> and ebx, BIT20  ; extract [-XD-]{+NX+} capability bit
> shr ebx, 9  ; shift bit to [-IA32_EFER 
> NXE-]{+IA32_EFER.NXE[BIT11]+} position
> DB  0x66, 0xb8  ; mov eax, imm32
> ASM_PFX(gSmmCr3): DD 0
> mov cr3, eax
> @@ -58,7 +58,7 @@ ASM_PFX(gSmmCr4): DD 0
> mov cr4, eax
> mov ecx, 0xc080 ; IA32_EFER MSR
> rdmsr
> or  eax, ebx; set NXE bit if [-XD-]{+NX+} is 
> available
> wrmsr
> DB  0x66, 0xb8  ; mov eax, imm32
> ASM_PFX(gSmmCr0): DD 0

Series pushed as d5988a8ac971..8d4d55b15b58.

Let's continue the PatchAssembly() discussion; once we reach an
agreement on that, I'll post patches for that too.

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

[edk2] [Patch 1/2] BaseTools GNUmakefile: Move HOST_ARCH detection into common makefile

[Liming Gao]

With this change, enter single tool directory, make can pass.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Liming Gao 
---
 BaseTools/Source/C/Makefiles/header.makefile | 28 ++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/BaseTools/Source/C/Makefiles/header.makefile 
b/BaseTools/Source/C/Makefiles/header.makefile
index e034da2..0976973 100644
--- a/BaseTools/Source/C/Makefiles/header.makefile
+++ b/BaseTools/Source/C/Makefiles/header.makefile
@@ -6,7 +6,7 @@
 # HOST_ARCH = ia64 or IA64 for IA64 build
 # HOST_ARCH = Arm or ARM for ARM build
 #
-# Copyright (c) 2007 - 2016, Intel Corporation. All rights reserved.
+# Copyright (c) 2007 - 2018, Intel Corporation. All rights reserved.
 # This program and the accompanying materials
 # are licensed and made available under the terms and conditions of the BSD 
License
 # which accompanies this distribution.The full text of the license may be 
found at
@@ -15,7 +15,31 @@
 # THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
 # WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 
-HOST_ARCH ?= IA32
+ifndef HOST_ARCH
+  #
+  # If HOST_ARCH is not defined, then we use 'uname -m' to attempt
+  # try to figure out the appropriate HOST_ARCH.
+  #
+  uname_m = $(shell uname -m)
+  $(info Attempting to detect HOST_ARCH from 'uname -m': $(uname_m))
+  ifneq (,$(strip $(filter $(uname_m), x86_64 amd64)))
+HOST_ARCH=X64
+  endif
+  ifeq ($(patsubst i%86,IA32,$(uname_m)),IA32)
+HOST_ARCH=IA32
+  endif
+  ifneq (,$(findstring aarch64,$(uname_m)))
+HOST_ARCH=AARCH64
+  endif
+  ifneq (,$(findstring arm,$(uname_m)))
+HOST_ARCH=ARM
+  endif
+  ifndef HOST_ARCH
+$(info Could not detected HOST_ARCH from uname results)
+$(error HOST_ARCH is not defined!)
+  endif
+  $(info Detected HOST_ARCH of $(HOST_ARCH) using uname.)
+endif
 
 CYGWIN:=$(findstring CYGWIN, $(shell uname -s))
 LINUX:=$(findstring Linux, $(shell uname -s))
-- 
2.8.0.windows.1

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

[edk2] [Patch 2/2] BaseTools GNUmakefile: Remove HOST_ARCH in every tool Makefile

[Liming Gao]

HOST_ARCH has been moved into the common header.makefile

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Liming Gao 
---
 BaseTools/Source/C/BootSectImage/GNUmakefile| 3 +--
 BaseTools/Source/C/BrotliCompress/GNUmakefile   | 3 +--
 BaseTools/Source/C/Common/GNUmakefile   | 3 +--
 BaseTools/Source/C/EfiLdrImage/GNUmakefile  | 3 +--
 BaseTools/Source/C/EfiRom/GNUmakefile   | 3 +--
 BaseTools/Source/C/GenCrc32/GNUmakefile | 3 +--
 BaseTools/Source/C/GenFfs/GNUmakefile   | 3 +--
 BaseTools/Source/C/GenFv/GNUmakefile| 3 +--
 BaseTools/Source/C/GenFw/GNUmakefile| 3 +--
 BaseTools/Source/C/GenPage/GNUmakefile  | 3 +--
 BaseTools/Source/C/GenSec/GNUmakefile   | 3 +--
 BaseTools/Source/C/GenVtf/GNUmakefile   | 3 +--
 BaseTools/Source/C/GnuGenBootSector/GNUmakefile | 3 +--
 BaseTools/Source/C/LzmaCompress/GNUmakefile | 3 +--
 BaseTools/Source/C/Split/GNUmakefile| 3 +--
 BaseTools/Source/C/TianoCompress/GNUmakefile| 3 +--
 BaseTools/Source/C/VfrCompile/GNUmakefile   | 3 +--
 BaseTools/Source/C/VolInfo/GNUmakefile  | 3 +--
 18 files changed, 18 insertions(+), 36 deletions(-)

diff --git a/BaseTools/Source/C/BootSectImage/GNUmakefile 
b/BaseTools/Source/C/BootSectImage/GNUmakefile
index 90800a4..f76beac 100644
--- a/BaseTools/Source/C/BootSectImage/GNUmakefile
+++ b/BaseTools/Source/C/BootSectImage/GNUmakefile
@@ -1,7 +1,7 @@
 ## @file
 # GNU/Linux makefile for 'BootSectImage' module build.
 #
-# Copyright (c) 2009 - 2010, Intel Corporation. All rights reserved.
+# Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved.
 # This program and the accompanying materials
 # are licensed and made available under the terms and conditions of the BSD 
License
 # which accompanies this distribution.  The full text of the license may be 
found at
@@ -10,7 +10,6 @@
 # THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
 # WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 #
-HOST_ARCH ?= IA32
 MAKEROOT ?= ..
 
 APPNAME = BootSectImage
diff --git a/BaseTools/Source/C/BrotliCompress/GNUmakefile 
b/BaseTools/Source/C/BrotliCompress/GNUmakefile
index 3426a00..1eaade7 100644
--- a/BaseTools/Source/C/BrotliCompress/GNUmakefile
+++ b/BaseTools/Source/C/BrotliCompress/GNUmakefile
@@ -1,7 +1,7 @@
 ## @file
 # GNU/Linux makefile for 'Brotli' module build.
 #
-# Copyright (c) 2017, Intel Corporation. All rights reserved.
+# Copyright (c) 2017 - 2018, Intel Corporation. All rights reserved.
 # This program and the accompanying materials
 # are licensed and made available under the terms and conditions of the BSD 
License
 # which accompanies this distribution.  The full text of the license may be 
found at
@@ -10,7 +10,6 @@
 # THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
 # WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 #
-HOST_ARCH ?= IA32
 MAKEROOT ?= ..
 
 APPNAME = Brotli
diff --git a/BaseTools/Source/C/Common/GNUmakefile 
b/BaseTools/Source/C/Common/GNUmakefile
index 66bd3a6..8214f18 100644
--- a/BaseTools/Source/C/Common/GNUmakefile
+++ b/BaseTools/Source/C/Common/GNUmakefile
@@ -1,7 +1,7 @@
 ## @file
 # GNU/Linux makefile for 'Common' module build.
 #
-# Copyright (c) 2007 - 2010, Intel Corporation. All rights reserved.
+# Copyright (c) 2007 - 2018, Intel Corporation. All rights reserved.
 # This program and the accompanying materials
 # are licensed and made available under the terms and conditions of the BSD 
License
 # which accompanies this distribution.  The full text of the license may be 
found at
@@ -10,7 +10,6 @@
 # THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
 # WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 #
-HOST_ARCH ?= IA32
 MAKEROOT ?= ..
 
 # VPATH = ..
diff --git a/BaseTools/Source/C/EfiLdrImage/GNUmakefile 
b/BaseTools/Source/C/EfiLdrImage/GNUmakefile
index 75c04ea..f5fe49c 100644
--- a/BaseTools/Source/C/EfiLdrImage/GNUmakefile
+++ b/BaseTools/Source/C/EfiLdrImage/GNUmakefile
@@ -1,7 +1,7 @@
 ## @file
 # GNU/Linux makefile for 'EfiLdrImage' module build.
 #
-# Copyright (c) 2007 - 2010, Intel Corporation. All rights reserved.
+# Copyright (c) 2007 - 2018, Intel Corporation. All rights reserved.
 # This program and the accompanying materials
 # are licensed and made available under the terms and conditions of the BSD 
License
 # which accompanies this distribution.  The full text of the license may be 
found at
@@ -10,7 +10,6 @@
 # THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
 # WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 #
-HOST_ARCH ?= IA32
 MAKEROOT ?= ..
 
 APPNAME = EfiLdrImage
diff --git a/BaseTools/Source/C/EfiRom/GNUmakefile 
b/BaseTools/Source/C/EfiRom/GNUmakefile
index a13111c..6a65ce8 100644
--- 

[edk2] [Patch 0/2] BaseTools GNUmakefile: Move HOST_ARCH detection into common makefile

[Liming Gao]

Liming Gao (2):
  BaseTools GNUmakefile: Move HOST_ARCH detection into common makefile
  BaseTools GNUmakefile: Remove HOST_ARCH in every tool Makefile

 BaseTools/Source/C/BootSectImage/GNUmakefile|  3 +--
 BaseTools/Source/C/BrotliCompress/GNUmakefile   |  3 +--
 BaseTools/Source/C/Common/GNUmakefile   |  3 +--
 BaseTools/Source/C/EfiLdrImage/GNUmakefile  |  3 +--
 BaseTools/Source/C/EfiRom/GNUmakefile   |  3 +--
 BaseTools/Source/C/GenCrc32/GNUmakefile |  3 +--
 BaseTools/Source/C/GenFfs/GNUmakefile   |  3 +--
 BaseTools/Source/C/GenFv/GNUmakefile|  3 +--
 BaseTools/Source/C/GenFw/GNUmakefile|  3 +--
 BaseTools/Source/C/GenPage/GNUmakefile  |  3 +--
 BaseTools/Source/C/GenSec/GNUmakefile   |  3 +--
 BaseTools/Source/C/GenVtf/GNUmakefile   |  3 +--
 BaseTools/Source/C/GnuGenBootSector/GNUmakefile |  3 +--
 BaseTools/Source/C/LzmaCompress/GNUmakefile |  3 +--
 BaseTools/Source/C/Makefiles/header.makefile| 28 +++--
 BaseTools/Source/C/Split/GNUmakefile|  3 +--
 BaseTools/Source/C/TianoCompress/GNUmakefile|  3 +--
 BaseTools/Source/C/VfrCompile/GNUmakefile   |  3 +--
 BaseTools/Source/C/VolInfo/GNUmakefile  |  3 +--
 19 files changed, 44 insertions(+), 38 deletions(-)

-- 
2.8.0.windows.1

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

Re: [edk2] [PATCH v2] PcAtChipsetPkg: Add PeiAcpiTimerLib to save PerformanceCounterFrequency in HOB

[Gao, Liming]

Star:
  Thanks for you feedback. I will update the patch and push it. 

>-Original Message-
>From: Zeng, Star
>Sent: Friday, January 26, 2018 1:57 PM
>To: Gao, Liming ; edk2-devel@lists.01.org
>Cc: Zeng, Star 
>Subject: RE: [edk2] [PATCH v2] PcAtChipsetPkg: Add PeiAcpiTimerLib to save
>PerformanceCounterFrequency in HOB
>
>Hi Liming,
>
>Reviewed-by: Star Zeng 
>
>Two minor comments. Just for your information.
>1. Notice the title, it may be too long.
>2. In PeiAcpiTimerLib.c, you may need to add ASSERT
>(PerformanceCounterFrequency != NULL) after BuildGuidHob.
>And you can move " PerformanceCounterFrequency =
>(UINT64*)GET_GUID_HOB_DATA (GuidHob); " to the else condition, then
>only one "return" is needed.
>
>+  PerformanceCounterFrequency = NULL;
>+  GuidHob = GetFirstGuidHob ();
>+  if (GuidHob == NULL) {
>+PerformanceCounterFrequency  =
>(UINT64*)BuildGuidHob(, sizeof
>(*PerformanceCounterFrequency));
>+*PerformanceCounterFrequency = InternalCalculateTscFrequency ();
>+return *PerformanceCounterFrequency;
>+  }
>+  PerformanceCounterFrequency = (UINT64*)GET_GUID_HOB_DATA
>(GuidHob);
>+
>+  return  *PerformanceCounterFrequency;
>+}
>
>
>Thanks,
>Star
>-Original Message-
>From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>Liming Gao
>Sent: Tuesday, January 23, 2018 10:24 AM
>To: edk2-devel@lists.01.org
>Cc: Zeng, Star 
>Subject: [edk2] [PATCH v2] PcAtChipsetPkg: Add PeiAcpiTimerLib to save
>PerformanceCounterFrequency in HOB
>
>In V2:
>1) Update PeiAcpiTimerLib base name to PeiAcpiTimerLib
>2) Update PeiAcpiTimerLib to add the missing constructor to enable ACPI IO
>space
>3) Update DxeAcpiTimerLib to cache frequency in constructor.
>
>PeiAcpiTimerLib caches PerformanceCounterFrequency in HOB, then Pei and
>Dxe
>AcpiTimerLib can share the same PerformanceCounterFrequency.
>
>Contributed-under: TianoCore Contribution Agreement 1.1
>Signed-off-by: Liming Gao 
>Cc: Star Zeng 
>---
> PcAtChipsetPkg/Library/AcpiTimerLib/AcpiTimerLib.c |  4 +-
> .../Library/AcpiTimerLib/DxeAcpiTimerLib.c | 57 +-
> .../Library/AcpiTimerLib/DxeAcpiTimerLib.inf   |  7 ++-
> .../Library/AcpiTimerLib/PeiAcpiTimerLib.c | 68
>++
> .../Library/AcpiTimerLib/PeiAcpiTimerLib.inf   | 56 ++
> .../Library/AcpiTimerLib/PeiAcpiTimerLib.uni   | 23 
> PcAtChipsetPkg/PcAtChipsetPkg.dsc  |  4 +-
> 7 files changed, 211 insertions(+), 8 deletions(-)
> create mode 100644 PcAtChipsetPkg/Library/AcpiTimerLib/PeiAcpiTimerLib.c
> create mode 100644
>PcAtChipsetPkg/Library/AcpiTimerLib/PeiAcpiTimerLib.inf
> create mode 100644
>PcAtChipsetPkg/Library/AcpiTimerLib/PeiAcpiTimerLib.uni
>
>diff --git a/PcAtChipsetPkg/Library/AcpiTimerLib/AcpiTimerLib.c
>b/PcAtChipsetPkg/Library/AcpiTimerLib/AcpiTimerLib.c
>index 792781a33f..2f3cb4bca4 100644
>--- a/PcAtChipsetPkg/Library/AcpiTimerLib/AcpiTimerLib.c
>+++ b/PcAtChipsetPkg/Library/AcpiTimerLib/AcpiTimerLib.c
>@@ -1,7 +1,7 @@
> /** @file
>   ACPI Timer implements one instance of Timer Library.
>
>-  Copyright (c) 2013 - 2016, Intel Corporation. All rights reserved.
>+  Copyright (c) 2013 - 2018, Intel Corporation. All rights reserved.
>   This program and the accompanying materials
>   are licensed and made available under the terms and conditions of the BSD
>License
>   which accompanies this distribution.  The full text of the license may be
>found at
>@@ -21,6 +21,8 @@
> #include 
> #include 
>
>+GUID mFrequencyHobGuid = { 0x3fca54f6, 0xe1a2, 0x4b20, { 0xbe, 0x76,
>0x92, 0x6b, 0x4b, 0x48, 0xbf, 0xaa }};
>+
> /**
>   Internal function to retrieves the 64-bit frequency in Hz.
>
>diff --git a/PcAtChipsetPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.c
>b/PcAtChipsetPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.c
>index b141c680fb..67e18a1360 100644
>--- a/PcAtChipsetPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.c
>+++ b/PcAtChipsetPkg/Library/AcpiTimerLib/DxeAcpiTimerLib.c
>@@ -12,9 +12,27 @@
>
> **/
>
>-#include 
>+#include 
> #include 
> #include 
>+#include 
>+
>+extern GUID mFrequencyHobGuid;
>+
>+/**
>+  The constructor function enables ACPI IO space.
>+
>+  If ACPI I/O space not enabled, this function will enable it.
>+  It will always return RETURN_SUCCESS.
>+
>+  @retval EFI_SUCCESS   The constructor always returns RETURN_SUCCESS.
>+
>+**/
>+RETURN_STATUS
>+EFIAPI
>+AcpiTimerLibConstructor (
>+  VOID
>+  );
>
> /**
>   Calculate TSC frequency.
>@@ -54,8 +72,41 @@ InternalGetPerformanceCounterFrequency (
>   VOID
>   )
> {
>-  if (mPerformanceCounterFrequency == 0) {
>+  return  mPerformanceCounterFrequency;
>+}
>+
>+/**
>+  The constructor function enables ACPI IO space, and caches
>PerformanceCounterFrequency.
>+
>+  @param  ImageHandle   The firmware allocated handle for the EFI image.
>+  @param  SystemTable   A pointer to the EFI System 

[edk2] [RFC] Remove X86 .asm and .S assembly files in EDK2

[Gao, Liming]

Edk2 has used nasm assembly file for all tool chains. So, IA32 and X64 .asm and 
.S assembly files can be removed if their nasm files are ready. It can save the 
maintain effort and avoid the confuse.



If you have any comments on this change, please let me know.

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

Re: [edk2] [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()

[Laszlo Ersek]

On 01/31/18 06:54, Ni, Ruiyu wrote:

> Laszlo, Mike,
> Considering this patch doesn't make the code worse,
> actually improved a tiny bit, can we firstly check in the three patches?

I agree; the PatchAssembly() discussion is taking quite a bit of
thought, meanwhile IA32 SMM is broken on KVM -- and even on QEMU! (Paolo
helped me test that, and yes, QEMU/TCG is affected the exact same way.)

I will go ahead and push the patches with the reviews from Paolo and
Ray, with the following small modifications:

* In the commit message of the first patch, I'll change

we can never remove

  to

we can't yet remove

  in the commit message.

* In the third patch, I will change the commit message from

SMM emulation under KVM

  to

SMM emulation under both KVM and QEMU (TCG)

* In the third patch, I will update the code comments as requested by
  Ray.

> Reviewed-by: Ruiyu Ni 

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

Re: [edk2] [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()

[Laszlo Ersek]

On 01/31/18 06:44, Ni, Ruiyu wrote:
> Mike, Laszlo,
> Does the patch only apply to the operand?
> If so, PatchAssembly looks too general.
> How about the name like PatchAssemblyOperand?

Originally I meant to call the function "PatchInstruction", but then I
thought it could be used for patching any other artifacts too, in object
code that comes from NASM.

I'm also fine calling it PatchAssemblyOperand, or even
PatchInstructionTrailingOperand :)

Thanks
Laszlo


> On 1/31/2018 6:25 AM, Kinney, Michael D wrote:
>> Laszlo,
>>
>> I agree that the function is better than a macro.
>>
>> I thought of the alignment issues as well.  CopyMem()
>> is a good solution.  We could also consider
>> WriteUnalignedxx() functions in BaseLib.
>>
>> I was originally thinking this functionality would go
>> into BaseLib.  But with the use of CopyMem(), we can't
>> do that.  Maybe we should use WriteUnalignedxx() and
>> add some ASSERT() checks.
>>
>> VOID
>> PatchAssembly (
>>    VOID    *BufferEnd,
>>    UINT64  PatchValue,
>>    UINTN   ValueSize
>>    )
>> {
>>    ASSERT ((UINTN)BufferEnd > ValueSize);
>>    switch (ValueSize) {
>>    case 1:
>>  ASSERT (PatchValue <= MAX_UINT8);
>>  *((UINT8 *)BufferEnd - 1) = (UINT8)PatchValue;
>>    case 2:
>>  ASSERT (PatchValue <= MAX_UINT16);
>>  WriteUnaligned16 ((UINT16 *)(BufferEnd) - 1, (UINT16)PatchValue));
>>  break;
>>    case 4:
>>  ASSERT (PatchValue <= MAX_UINT32);
>>  WriteUnaligned32 ((UINT32 *)(BufferEnd) - 1, (UINT32)PatchValue));
>>  break;
>>    case 8:
>>  WriteUnaligned64 ((UINT64 *)(BufferEnd) - 1, PatchValue));
>>  break;
>>    default:
>>  ASSERT (FALSE);
>>    }
>> }
>>
>> Mike
>>
>>> -Original Message-
>>> From: Laszlo Ersek [mailto:ler...@redhat.com]
>>> Sent: Tuesday, January 30, 2018 1:45 PM
>>> To: Kinney, Michael D ; edk2-
>>> devel-01 
>>> Cc: Ni, Ruiyu ; Paolo Bonzini
>>> ; Yao, Jiewen
>>> ; Dong, Eric 
>>> Subject: Re: [edk2] [PATCH 1/3]
>>> UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32
>>> SmmStartup()
>>>
>>> On 01/30/18 21:31, Kinney, Michael D wrote:
 Laszlo,

 We have already used this technique in other NASM files
 to remove DBs.
>>>
>>> OK.
>>>
 Let us know if you have suggestions on how to make the
 C code that performs the patches easier to read and
 maintain.
>>>
>>> How about this:
>>>
>>>    VOID
>>>    PatchAssembly (
>>>  VOID   *BufferEnd,
>>>  UINT64 PatchValue,
>>>  UINTN  ValueSize
>>>  )
>>>    {
>>>  CopyMem (
>>>    (VOID *)((UINTN)BufferEnd - ValueSize),
>>>    ,
>>>    ValueSize
>>>    );
>>>    }
>>>
>>>    extern UINT8 gAsmSmmCr0;
>>>    extern UINT8 gAsmSmmCr3;
>>>    extern UINT8 gAsmSmmCr4;
>>>
>>>    ...
>>>    {
>>>  PatchAssembly (, AsmReadCr0 (), 4);
>>>  PatchAssembly (, AsmReadCr3 (), 4);
>>>  PatchAssembly (, AsmReadCr4 (), 4);
>>>  ...
>>>    }
>>>
>>> (I think it's fine to open-code the last argument as "4",
>>> rather than
>>> "sizeof (UINT32)", because for patching, we must have
>>> intimate knowledge
>>> of the instruction anyway.)
>>>
>>> To me, this is easier to read, because:
>>>
>>> - there are no complex casts in the "business logic"
>>> - the size is spelled out once per patching
>>> - the function name and the variable names make it clear
>>> we are patching
>>>    separately compiled assembly code that was linked into
>>> the same
>>>    module.
>>>
>>> What do you think?
>>>
>>> Thanks!
>>> Laszlo
>>>
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-
>>> boun...@lists.01.org]
> On Behalf Of Laszlo Ersek
> Sent: Tuesday, January 30, 2018 10:17 AM
> To: Kinney, Michael D ;
>>> edk2-
> devel-01 
> Cc: Ni, Ruiyu ; Paolo Bonzini
> ; Yao, Jiewen
> ; Dong, Eric
>>> 
> Subject: Re: [edk2] [PATCH 1/3]
> UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32
> SmmStartup()
>
> On 01/30/18 18:22, Kinney, Michael D wrote:
>> Laszlo,
>>
>> The DBs can be removed if the label is moved after
>> the instruction and the patch is done to the label
>> minus the size of the patch value.
>
> Indeed I haven't thought of this.
>
> If I understand correctly, it means
>
>    extern UINT8 gSmmCr0;
>
>    *(UINT32*)( - sizeof (UINT32)) =
> (UINT32)AsmReadCr0 ();
>
> TBH, the DB feels less ugly to me than this :)
>
> Still, if you think it would be an acceptable price to
> pay for removing
> the remaining DBs, I can respin.
>
> Thanks
> Laszlo
>
>>> -Original Message-
>>> From: edk2-devel [mailto:edk2-devel-
> boun...@lists.01.org]
>>> On Behalf 

Re: [edk2] [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup()

[Laszlo Ersek]

On 01/30/18 23:25, Kinney, Michael D wrote:
> Laszlo,
> 
> I agree that the function is better than a macro.
> 
> I thought of the alignment issues as well.  CopyMem()
> is a good solution.  We could also consider
> WriteUnalignedxx() functions in BaseLib.

IMO, the WriteUnalignedxx functions are a bit pointless in the exact
form they are declared (this was discussed earlier esp. with regard to
aarch64). The functions take pointers to objects that already have the
target type, such as

UINT32
EFIAPI
WriteUnaligned32 (
  OUT UINT32*Buffer,
  IN  UINT32Value
  )

Here the type of Buffer should be (VOID *), not (UINT32 *). Otherwise,
the undefined behavior (due to mis-alignment) surfaces as soon as the
function is called with an unaligned pointer (i.e. before the target
area is actually written).

> I was originally thinking this functionality would go
> into BaseLib.  But with the use of CopyMem(), we can't
> do that.

Can we put it in BaseMemoryLib instead (which is where CopyMem() is
from)? That library class is still low-level enough. And, while I count
9 library instances, PatchAssembly() is not a large function, we could
tolerate adding it to all 9 instances, identically.

Let me also ask the opposite question: should we perhaps make the
PatchAssembly() API *less* abstract? (Also suggested by your naming of
the macro, PATCH_X86_ASM.) If the instruction encoding on e.g. AARCH64
doesn't lend itself to such patching (= expressed through the address
right after the instruction), then even BaseMemoryLib may be too generic
for the API.

> Maybe we should use WriteUnalignedxx() and
> add some ASSERT() checks.
> 
> VOID
> PatchAssembly (
>   VOID*BufferEnd,
>   UINT64  PatchValue,
>   UINTN   ValueSize
>   )
> {
>   ASSERT ((UINTN)BufferEnd > ValueSize);
>   switch (ValueSize) {
>   case 1:
> ASSERT (PatchValue <= MAX_UINT8);
> *((UINT8 *)BufferEnd - 1) = (UINT8)PatchValue;
>   case 2:
> ASSERT (PatchValue <= MAX_UINT16);
> WriteUnaligned16 ((UINT16 *)(BufferEnd) - 1, (UINT16)PatchValue));
> break;
>   case 4:
> ASSERT (PatchValue <= MAX_UINT32);
> WriteUnaligned32 ((UINT32 *)(BufferEnd) - 1, (UINT32)PatchValue));
> break;
>   case 8:
> WriteUnaligned64 ((UINT64 *)(BufferEnd) - 1, PatchValue));
> break;
>   default:
> ASSERT (FALSE);
>   }
> }

In my opinion:

- If Ard and Leif say that PatchAssembly() API makes sense for AARCH64,
  then I think we can go with the above generic implementation (for
  BaseLib).

- If Ard and Leif say the API is only useful on x86, then I suggest that
  we implement the API separately for all arches (still in BaseLib):

  - On x86, we should simply open-code the unaligned accesses (like you
originall suggested). The pointer arithmetic will look a bit wild,
but it's safely hidden behind a BaseLib API, so client code will
look nice.

  - On all other arches, we should implement the function with
ASSERT(FALSE).

Thanks!
Laszlo

> 
> Mike
> 
>> -Original Message-
>> From: Laszlo Ersek [mailto:ler...@redhat.com]
>> Sent: Tuesday, January 30, 2018 1:45 PM
>> To: Kinney, Michael D ; edk2-
>> devel-01 
>> Cc: Ni, Ruiyu ; Paolo Bonzini
>> ; Yao, Jiewen
>> ; Dong, Eric 
>> Subject: Re: [edk2] [PATCH 1/3]
>> UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32
>> SmmStartup()
>>
>> On 01/30/18 21:31, Kinney, Michael D wrote:
>>> Laszlo,
>>>
>>> We have already used this technique in other NASM files
>>> to remove DBs.
>>
>> OK.
>>
>>> Let us know if you have suggestions on how to make the
>>> C code that performs the patches easier to read and
>>> maintain.
>>
>> How about this:
>>
>>   VOID
>>   PatchAssembly (
>> VOID   *BufferEnd,
>> UINT64 PatchValue,
>> UINTN  ValueSize
>> )
>>   {
>> CopyMem (
>>   (VOID *)((UINTN)BufferEnd - ValueSize),
>>   ,
>>   ValueSize
>>   );
>>   }
>>
>>   extern UINT8 gAsmSmmCr0;
>>   extern UINT8 gAsmSmmCr3;
>>   extern UINT8 gAsmSmmCr4;
>>
>>   ...
>>   {
>> PatchAssembly (, AsmReadCr0 (), 4);
>> PatchAssembly (, AsmReadCr3 (), 4);
>> PatchAssembly (, AsmReadCr4 (), 4);
>> ...
>>   }
>>
>> (I think it's fine to open-code the last argument as "4",
>> rather than
>> "sizeof (UINT32)", because for patching, we must have
>> intimate knowledge
>> of the instruction anyway.)
>>
>> To me, this is easier to read, because:
>>
>> - there are no complex casts in the "business logic"
>> - the size is spelled out once per patching
>> - the function name and the variable names make it clear
>> we are patching
>>   separately compiled assembly code that was linked into
>> the same
>>   module.
>>
>> What do you think?
>>
>> Thanks!
>> Laszlo
>>
 -Original Message-
 From: edk2-devel [mailto:edk2-devel-
>> boun...@lists.01.org]
 On Behalf Of Laszlo Ersek
 

Re: [edk2] [Patch] BaseTools CommonLib: Remove the unnecessary print message in PcdValueCommon

[Zhu, Yonghong]

Reviewed-by: Yonghong Zhu  

Best Regards,
Zhu Yonghong

-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Liming 
Gao
Sent: Wednesday, January 31, 2018 5:04 PM
To: edk2-devel@lists.01.org
Subject: [edk2] [Patch] BaseTools CommonLib: Remove the unnecessary print 
message in PcdValueCommon

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Liming Gao 
---
 BaseTools/Source/C/Common/PcdValueCommon.c | 10 --
 1 file changed, 10 deletions(-)

diff --git a/BaseTools/Source/C/Common/PcdValueCommon.c 
b/BaseTools/Source/C/Common/PcdValueCommon.c
index 92328da..210f87b 100644
--- a/BaseTools/Source/C/Common/PcdValueCommon.c
+++ b/BaseTools/Source/C/Common/PcdValueCommon.c
@@ -330,9 +330,7 @@ Returns:
 break;
   case PcdDataTypePointer:
 Value = [Index].Value[1];
-printf ("Value = %s\n", PcdList[Index].Value);
 for (*Size = 0, Byte = (UINT8) strtoul(Value, , 16); Value != End; 
Byte = (UINT8) strtoul(Value, , 16), *Size = *Size + 1) {
-  printf("%x\n", Byte);
   Value = End + 1;
 }
 Buffer = malloc(*Size + 1);
@@ -401,7 +399,6 @@ Returns:
 PcdList[Index].Value = malloc(Size * 5 + 3);
 PcdList[Index].Value[0] = '{';
 for (ValueIndex = 0; ValueIndex < Size; ValueIndex++) {
-  printf("Value[%d] = %02x\n", ValueIndex, Value[ValueIndex]);
   sprintf([Index].Value[1 + ValueIndex * 5], "0x%02x,", 
Value[ValueIndex]);
 }
 PcdList[Index].Value[1 + Size * 5 - 1] = '}'; @@ -724,15 +721,11 @@ 
Returns:
   if (*InputFileName == NULL) {
 fprintf (stderr, "Missing option.  Input files is not specified\n");
 exit (EXIT_FAILURE);
-  } else {
-printf ("Input file name is %s\n", *InputFileName);
   }
 
   if (*OutputFileName == NULL) {
 fprintf (stderr, "Missing option.  Output file is not specified\n");
 exit (EXIT_FAILURE);
-  } else {
-printf ("Output file name is %s\n", *OutputFileName);
   }
 }
 
@@ -761,7 +754,6 @@ Returns:
   UINT8   *FileBuffer;
   UINT32  FileSize;
 
-  printf ("PCD tool start.\n");
   InputFileName = NULL;
   OutputFileName = NULL;
 
@@ -790,7 +782,5 @@ Returns:
   //
   WriteOutputFile (OutputFileName);
 
-  printf ("PCD tool done.\n");
-
   exit (EXIT_SUCCESS);
 }
--
2.8.0.windows.1

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

Re: [edk2] [Patch] UefiCpuPkg: Remove the unused file ResetVec.asm16

[Laszlo Ersek]

On 01/31/18 10:45, Liming Gao wrote:
> ResetVec.nasmb is used. ResetVec.asm16 can be retired.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Liming Gao 
> ---
>  UefiCpuPkg/SecCore/Ia32/ResetVec.asm16 | 106 
> -
>  1 file changed, 106 deletions(-)
>  delete mode 100644 UefiCpuPkg/SecCore/Ia32/ResetVec.asm16

Reviewed-by: Laszlo Ersek 

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

[edk2] [Patch] UefiCpuPkg: Remove the unused file ResetVec.asm16

[Liming Gao]

ResetVec.nasmb is used. ResetVec.asm16 can be retired.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Liming Gao 
---
 UefiCpuPkg/SecCore/Ia32/ResetVec.asm16 | 106 -
 1 file changed, 106 deletions(-)
 delete mode 100644 UefiCpuPkg/SecCore/Ia32/ResetVec.asm16

diff --git a/UefiCpuPkg/SecCore/Ia32/ResetVec.asm16 
b/UefiCpuPkg/SecCore/Ia32/ResetVec.asm16
deleted file mode 100644
index d90613c..000
--- a/UefiCpuPkg/SecCore/Ia32/ResetVec.asm16
+++ /dev/null
@@ -1,106 +0,0 @@
-;--
-;
-; Copyright (c) 2014, 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
-; http://opensource.org/licenses/bsd-license.php.
-;
-; THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
-; WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
-;
-; Module Name:
-;
-;  ResetVec.asm
-;
-; Abstract:
-;
-;  Reset Vector Data structure
-;  This structure is located at 0xFFC0
-;
-;--
-
-.model  tiny
-.686p
-.stack  0h
-.code
-
-;
-; The layout of this file is fixed. The build tool makes assumption of the 
layout.
-;
-
-ORG 0h
-;
-; Reserved
-;
-ReservedData DD 0h, 0h
-
-ORG 10h
-;
-; This is located at 0xFFD0h
-;
-mov di, "AP"
-jmp ApStartup
-
-ORG 20h
-;
-; Pointer to the entry point of the PEI core
-; It is located at 0xFFE0, and is fixed up by some build tool
-; So if the value 8..1 appears in the final FD image, tool failure occurs.
-;
-PeiCoreEntryPoint   DD  87654321h
-
-;
-; This is the handler for all kinds of exceptions. Since it's for debugging
-; purpose only, nothing except a dead loop would be done here. Developers could
-; analyze the cause of the exception if a debugger had been attached.
-;
-InterruptHandlerPROC
-jmp $
-iret
-InterruptHandlerENDP
-
-ORG 30h
-;
-; For IA32, the reset vector must be at 0xFFF0, i.e., 4G-16 byte
-; Execution starts here upon power-on/platform-reset.
-;
-ResetHandler:
-nop
-nop
-ApStartup:
-;
-; Jmp Rel16 instruction
-; Use machine code directly in case of the assembler optimization
-; SEC entry point relative address will be fixed up by some build tool.
-;
-; Typically, SEC entry point is the function _ModuleEntryPoint() defined in
-; SecEntry.asm
-;
-DB  0e9h
-DW  -3
-
-
-ORG 38h
-;
-; Ap reset vector segment address is at 0xFFF8
-; This will be fixed up by some build tool,
-; so if the value 1..8 appears in the final FD image,
-; tool failure occurs
-;
-ApSegAddressdd  12345678h
-
-ORG 3ch
-;
-; BFV Base is at 0xFFFC
-; This will be fixed up by some build tool,
-; so if the value 1..8 appears in the final FD image,
-; tool failure occurs.
-;
-BfvBase DD  12345678h
-
-;
-; Nothing can go here, otherwise the layout of this file would change.
-;
-
-END
-- 
2.8.0.windows.1

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

[edk2] [Patch] BaseTool: Add comments in PcdValueInit.c.

[BobCF]

Add Comments for __FLEXIBLE_SIZE () statement.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Bob Feng 
Cc: Liming Gao 
---
 BaseTools/Source/Python/Workspace/DscBuildData.py | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/BaseTools/Source/Python/Workspace/DscBuildData.py 
b/BaseTools/Source/Python/Workspace/DscBuildData.py
index 012e16a488..214e4fd569 100644
--- a/BaseTools/Source/Python/Workspace/DscBuildData.py
+++ b/BaseTools/Source/Python/Workspace/DscBuildData.py
@@ -1325,42 +1325,44 @@ class DscBuildData(PlatformBuildClassObject):
 for FieldName in FieldList:
 FieldName = "." + FieldName
 IsArray = 
self.IsFieldValueAnArray(FieldList[FieldName.strip(".")][0])
 if IsArray:
 Value, ValueSize = ParseFieldValue 
(FieldList[FieldName.strip(".")][0])
-CApp = CApp + '  __FLEXIBLE_SIZE(Size, %s, %s, %d / 
__ARRAY_ELEMENT_SIZE(%s, %s) + ((%d %% __ARRAY_ELEMENT_SIZE(%s, %s)) ? 1 : 
0));\n' % (Pcd.DatumType, FieldName.strip("."), ValueSize, Pcd.DatumType, 
FieldName.strip("."), ValueSize, Pcd.DatumType, FieldName.strip("."));
+CApp = CApp + '  __FLEXIBLE_SIZE(Size, %s, %s, %d / 
__ARRAY_ELEMENT_SIZE(%s, %s) + ((%d %% __ARRAY_ELEMENT_SIZE(%s, %s)) ? 1 : 0)); 
 // From %s Line %d Value %s \n' % (Pcd.DatumType, FieldName.strip("."), 
ValueSize, Pcd.DatumType, FieldName.strip("."), ValueSize, Pcd.DatumType, 
FieldName.strip("."), FieldList[FieldName.strip(".")][1], 
FieldList[FieldName.strip(".")][2], FieldList[FieldName.strip(".")][0]);
 else:
 NewFieldName = ''
+FieldName_ori = FieldName.strip('.')
 while '[' in  FieldName:
 NewFieldName = NewFieldName + FieldName.split('[', 
1)[0] + '[0]'
 ArrayIndex = int(FieldName.split('[', 
1)[1].split(']', 1)[0])
 FieldName = FieldName.split(']', 1)[1]
 FieldName = NewFieldName + FieldName
 while '[' in FieldName:
 FieldName = FieldName.rsplit('[', 1)[0]
-CApp = CApp + '  __FLEXIBLE_SIZE(Size, %s, %s, 
%d);\n' % (Pcd.DatumType, FieldName.strip("."), ArrayIndex + 1)
+CApp = CApp + '  __FLEXIBLE_SIZE(Size, %s, %s, 
%d); // From %s Line %d Value %s\n' % (Pcd.DatumType, FieldName.strip("."), 
ArrayIndex + 1, FieldList[FieldName_ori][1], FieldList[FieldName_ori][2], 
FieldList[FieldName_ori][0])
 for skuname in self.SkuIdMgr.GetSkuChain(SkuName):
 inherit_OverrideValues = Pcd.SkuOverrideValues[skuname]
 for FieldList in 
[inherit_OverrideValues.get(DefaultStoreName)]:
 if not FieldList:
 continue
 for FieldName in FieldList:
 FieldName = "." + FieldName
 IsArray = 
self.IsFieldValueAnArray(FieldList[FieldName.strip(".")][0])
 if IsArray:
 Value, ValueSize = ParseFieldValue 
(FieldList[FieldName.strip(".")][0])
-CApp = CApp + '  __FLEXIBLE_SIZE(Size, %s, %s, %d 
/ __ARRAY_ELEMENT_SIZE(%s, %s) + ((%d %% __ARRAY_ELEMENT_SIZE(%s, %s)) ? 1 : 
0));\n' % (Pcd.DatumType, FieldName.strip("."), ValueSize, Pcd.DatumType, 
FieldName.strip("."), ValueSize, Pcd.DatumType, FieldName.strip("."));
+CApp = CApp + '  __FLEXIBLE_SIZE(Size, %s, %s, %d 
/ __ARRAY_ELEMENT_SIZE(%s, %s) + ((%d %% __ARRAY_ELEMENT_SIZE(%s, %s)) ? 1 : 
0)); // From %s Line %d Value %s\n' % (Pcd.DatumType, FieldName.strip("."), 
ValueSize, Pcd.DatumType, FieldName.strip("."), ValueSize, Pcd.DatumType, 
FieldName.strip("."), FieldList[FieldName.strip(".")][1], 
FieldList[FieldName.strip(".")][2], FieldList[FieldName.strip(".")][0]);
 else:
 NewFieldName = ''
+FieldName_ori = FieldName.strip('.')
 while '[' in  FieldName:
 NewFieldName = NewFieldName + 
FieldName.split('[', 1)[0] + '[0]'
 ArrayIndex = int(FieldName.split('[', 
1)[1].split(']', 1)[0])
 FieldName = FieldName.split(']', 1)[1]
 FieldName = NewFieldName + FieldName
 while '[' in FieldName:
 FieldName = FieldName.rsplit('[', 1)[0]
-CApp = CApp + '  __FLEXIBLE_SIZE(Size, %s, %s, 
%d);\n' % (Pcd.DatumType, FieldName.strip("."), ArrayIndex + 1)
+CApp = CApp + '  __FLEXIBLE_SIZE(Size, %s, %s, 
%d); // From %s Line 

Re: [edk2] [PATCH] UefiCpuPkg: Enhance CPU feature dependency check

[Laszlo Ersek]

On 01/31/18 08:00, Song, BinX wrote:
> Current CPU feature dependency check will hang on when meet below or
> similar case:
> if (IsCpuFeatureSupported (CPU_FEATURE_AESNI)) {
>   Status = RegisterCpuFeature (
>  "AESNI",
>  AesniGetConfigData,
>  AesniSupport,
>  AesniInitialize,
>  CPU_FEATURE_AESNI,
>  CPU_FEATURE_MWAIT | CPU_FEATURE_BEFORE,
>  CPU_FEATURE_END
>  );
>   ASSERT_EFI_ERROR (Status);
> }
> if (IsCpuFeatureSupported (CPU_FEATURE_MWAIT)) {
>   Status = RegisterCpuFeature (
>  "MWAIT",
>  NULL,
>  MonitorMwaitSupport,
>  MonitorMwaitInitialize,
>  CPU_FEATURE_MWAIT,
>  CPU_FEATURE_AESNI | CPU_FEATURE_BEFORE,
>  CPU_FEATURE_END
>  );
>   ASSERT_EFI_ERROR (Status);
> }
> 
> Solution is to separate current CPU feature dependency check into
> sort and check two parts.
> 
> Sort function:
> According to CPU feature's dependency, sort all CPU features.
> Later dependency will override previous dependency if they are conflicted.
> 
> Check function:
> Check sorted CPU features' relationship, ASSERT invalid relationship.
> 
> Cc: Eric Dong 
> Cc: Laszlo Ersek 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Bell Song 
> ---
>  .../RegisterCpuFeaturesLib/CpuFeaturesInitialize.c | 271 
> -
>  .../RegisterCpuFeaturesLib/RegisterCpuFeatures.h   |   7 +
>  .../RegisterCpuFeaturesLib.c   | 130 +-
>  3 files changed, 278 insertions(+), 130 deletions(-)
> 
> diff --git 
> a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c 
> b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> index 4d75c07..2fd0d5f 100644
> --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> @@ -423,6 +423,271 @@ DumpRegisterTableOnProcessor (
>  }
>  
>  /**
> +  From FeatureBitMask, find the right feature entry in CPU feature list.
> +
> +  @param[in]  FeatureListThe pointer to CPU feature list.
> +  @param[in]  CurrentFeature The pointer to current CPU feature.
> +  @param[in]  BeforeFlag TRUE: BeforeFeatureBitMask; FALSE: 
> AfterFeatureBitMask.
> +
> +  @return  The pointer to right CPU feature entry.
> +**/
> +LIST_ENTRY *
> +FindFeatureInList(
> +  IN LIST_ENTRY  *CpuFeatureList,
> +  IN CPU_FEATURES_ENTRY  *CurrentCpuFeature,
> +  IN BOOLEAN  BeforeFlag
> +  )
> +{
> +  LIST_ENTRY *TempEntry;
> +  CPU_FEATURES_ENTRY *TempFeature;
> +  UINT8  *FeatureBitMask;
> +
> +  FeatureBitMask = BeforeFlag ? CurrentCpuFeature->BeforeFeatureBitMask : 
> CurrentCpuFeature->AfterFeatureBitMask;
> +  TempEntry = GetFirstNode (CpuFeatureList);
> +  while (!IsNull (CpuFeatureList, TempEntry)) {
> +TempFeature = CPU_FEATURE_ENTRY_FROM_LINK (TempEntry);
> +if (IsBitMaskMatchCheck (FeatureBitMask, TempFeature->FeatureMask)){
> +  return TempEntry;
> +}
> +TempEntry = TempEntry->ForwardLink;
> +  }
> +
> +  DEBUG ((DEBUG_ERROR, "Error: Feature %a ", CurrentCpuFeature->FeatureName, 
> BeforeFlag ? "before ":"after ", "condition is invalid!\n"));

Hi, I skimmed this patch quickly -- I can tell that I can't really tell
what's going on. I don't know how the feature dependencies are defined
in the first place, and what the bug is.

However, I do see that the above DEBUG macro invocation is incorrect.
The format string has one (1) %a conversion specification, but we pass
three (3) arguments.

I think the last argument ("condition is invalid!\n") should actually be
part of the format string. And then, the "before"/"after" string has to
be printed somehow as well.

Another superficial observation below:

> +/**
> +  Check sorted CPU features' relationship, ASSERT invalid one.
> +
> +  @param[in]  FeatureList  The pointer to CPU feature list.
> +**/
> +VOID
> +CheckCpuFeaturesRelationShip (

I don't think we should capitalize "Ship" in this identifier.

Third comment: there are several ways to define "sorting", so I'm not
sure my question applies, but: can we replace the manual sorting with
SortLib?

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

Re: [edk2] difference between asm16 and asm files

[Gao, Liming]

Tiger:
  Nasm compiler supports to generate 16bit raw code. User doesn't need to 
install the additional tool. And, the raw code is 16bit code. You need 16bit 
assembler. It is also nasm compiler. So, it is obvious to use nasm to directly 
do it.

Thanks
Liming
>-Original Message-
>From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>Tiger Liu(BJ-RD)
>Sent: Wednesday, January 31, 2018 1:18 PM
>To: Gao, Liming ; edk2-devel@lists.01.org
>Subject: Re: [edk2] difference between asm16 and asm files
>
>Hi, Liming:
>YES, its code need to be the raw execution code.
>I mean:
>1. We could use traditional asm code , compiled it, and use tools to translate
>them to the raw execution code.
>Is there any difficulty to do this?
>
>So, it would depreciate the asm16 or nasmb concept.
>
>Thanks
>-邮件原件-
>发件人: Tiger Liu(BJ-RD)
>发送时间: 2018年1月30日 9:45
>收件人: 'Gao, Liming' ; edk2-devel@lists.01.org
>主题: Re: [edk2] difference between asm16 and asm files
>
>Hi, Liming:
>Thanks for your reply!
>
>I have another question:
>Why not let sec code complied to obj file and linked to lib and EFI image?
>
>I met a problem:
>I tried to use IFDEF marco in an assembly inc file,  and this inc file would be
>included by ResetVec.asm16 But failed, it seemed asm 16 complier not
>recognize this marco exist.
>If I changed ResetVec.asm16 to ResetVec.asm, and compiled again, the
>complier will recognize this marco exist.
>But the last step failed, GenFds.exe seems search a binary file, and not found.
>
>Thanks
>-邮件原件-
>发件人: Gao, Liming [mailto:liming@intel.com]
>发送时间: 2018年1月29日 22:30
>收件人: Tiger Liu(BJ-RD) ; edk2-devel@lists.01.org
>主题: RE: [edk2] difference between asm16 and asm files
>
>Asm16 is compiled to the binary file. Asm is compiled to the obj file, and 
>linked
>into lib and EFI image.
>
>Now, asm16 is replaced by nasmb. Asm is replaced by nasm. If you check
>SecCore.inf, you will find ResetVec.asm16 is not used, and ResetVec.nasmb is
>used. I will send the patch to remove the unused ResetVec.asm16 to avoid
>the confuse.
>
>> -Original Message-
>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>> Tiger Liu(BJ-RD)
>> Sent: Monday, January 29, 2018 6:49 PM
>> To: edk2-devel@lists.01.org
>> Subject: [edk2] difference between asm16 and asm files
>>
>> Hi, experts:
>> I have a question about asm16 postfix and asm postfix.
>>
>> Such as:
>> UefiCpuPkg\SecCore\Ia32\ResetVec.asm16
>>
>> Why not use asm as postfix?
>>
>> Thanks
>>
>> Best wishes,
>>
>>
>> ?
>> ?
>> CONFIDENTIAL NOTE:
>> This email contains confidential or legally privileged information and
>> is for the sole use of its intended recipient. Any unauthorized review, use,
>copying or forwarding of this email or the content of this email is strictly
>prohibited.
>> ___
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
>
>
>保密声明：
>本邮件含有保密或专有信息，仅供指定收件人使用。严禁对本邮件或其
>内容做任何未经授权的查阅、使用、复制或转发。
>CONFIDENTIAL NOTE:
>This email contains confidential or legally privileged information and is for 
>the
>sole use of its intended recipient. Any unauthorized review, use, copying or
>forwarding of this email or the content of this email is strictly prohibited.
>___
>edk2-devel mailing list
>edk2-devel@lists.01.org
>https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [Patch] BaseTool: Enhance error handling.

[BobCF]

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Bob Feng 
Cc: Liming Gao 
---
 BaseTools/Source/Python/Workspace/DscBuildData.py  | 116 -
 .../Source/Python/Workspace/MetaFileParser.py  |   3 +
 2 files changed, 70 insertions(+), 49 deletions(-)

diff --git a/BaseTools/Source/Python/Workspace/DscBuildData.py 
b/BaseTools/Source/Python/Workspace/DscBuildData.py
index bc77d1a2b4..9d8da6ed85 100644
--- a/BaseTools/Source/Python/Workspace/DscBuildData.py
+++ b/BaseTools/Source/Python/Workspace/DscBuildData.py
@@ -856,11 +856,11 @@ class DscBuildData(PlatformBuildClassObject):
 if not Valid:
 EdkLogger.error('build', FORMAT_INVALID, ErrStr, 
File=self.MetaFile, Line=LineNo,
 ExtraData="%s.%s" % (TokenSpaceGuid, PcdCName))
 if PcdType in (MODEL_PCD_DYNAMIC_DEFAULT, 
MODEL_PCD_DYNAMIC_EX_DEFAULT):
 if self._DecPcds[PcdCName, TokenSpaceGuid].DatumType.strip() 
!= ValueList[1].strip():
-EdkLogger.error('build', FORMAT_INVALID, ErrStr , 
File=self.MetaFile, Line=LineNo,
+EdkLogger.error('build', FORMAT_INVALID, "Pcd datumtype 
used in DSC file is not the same as its declaration in DEC file." , 
File=self.MetaFile, Line=LineNo,
 ExtraData="%s.%s|%s" % (TokenSpaceGuid, 
PcdCName, Setting))
 return ValueList
 
 def _FilterPcdBySkuUsage(self,Pcds):
 available_sku = self.SkuIdMgr.AvailableSkuIdSet
@@ -1009,17 +1009,20 @@ class DscBuildData(PlatformBuildClassObject):
 StrPcdSet = self.GetStructurePcdInfo(S_PcdSet)
 S_pcd_set = {}
 for str_pcd in StrPcdSet:
 str_pcd_obj = Pcds.get((str_pcd[1], str_pcd[0]), None)
 str_pcd_dec = self._DecPcds.get((str_pcd[1], str_pcd[0]), None)
+if not isinstance (str_pcd_dec, StructurePcd):
+EdkLogger.error('build', PARSER_ERROR,
+"Pcd (%s.%s) is not declared as Structure PCD in 
DEC files. Arch: ['%s']" % (str_pcd[0], str_pcd[1], self._Arch),
+File=self.MetaFile,Line = StrPcdSet[str_pcd][0][5])
 if str_pcd_dec:
 str_pcd_obj_str = StructurePcd()
 str_pcd_obj_str.copy(str_pcd_dec)
 if str_pcd_obj:
 str_pcd_obj_str.copy(str_pcd_obj)
-if str_pcd_obj.DefaultValue:
-str_pcd_obj_str.DefaultFromDSC = 
str_pcd_obj.DefaultValue
+str_pcd_obj_str.DefaultFromDSC = str_pcd_obj_str.DefaultValue
 for str_pcd_data in StrPcdSet[str_pcd]:
 if str_pcd_data[3] in SkuIds:
 str_pcd_obj_str.AddOverrideValue(str_pcd_data[2], 
str(str_pcd_data[6]), 'DEFAULT' if str_pcd_data[3] == 'COMMON' else 
str_pcd_data[3],'STANDARD' if str_pcd_data[4] == 'COMMON' else str_pcd_data[4], 
self.MetaFile.File,LineNo=str_pcd_data[5])
 S_pcd_set[str_pcd[1], str_pcd[0]] = str_pcd_obj_str
 else:
@@ -1153,11 +1156,11 @@ class DscBuildData(PlatformBuildClassObject):
 if SkuName not in AvailableSkuIdSet:
 EdkLogger.error('build ', PARAMETER_INVALID, 'Sku %s is not 
defined in [SkuIds] section' % SkuName,
 File=self.MetaFile, Line=Dummy5)
 if SkuName in (self.SkuIdMgr.SystemSkuId, 'DEFAULT', 'COMMON'):
 if "." not in TokenSpaceGuid:
-PcdSet.add((PcdCName, TokenSpaceGuid, SkuName, Dummy4))
+PcdSet.add((PcdCName, TokenSpaceGuid, SkuName, Dummy5))
 PcdDict[Arch, PcdCName, TokenSpaceGuid, SkuName] = Setting
 
 for PcdCName, TokenSpaceGuid, SkuName, Dummy4 in PcdSet:
 Setting = PcdDict[self._Arch, PcdCName, TokenSpaceGuid, SkuName]
 if Setting == None:
@@ -1262,16 +1265,13 @@ class DscBuildData(PlatformBuildClassObject):
 
 def ExecuteCommand (self, Command):
 try:
 Process = subprocess.Popen(Command, stdout=subprocess.PIPE, 
stderr=subprocess.PIPE, shell=True)
 except:
-print 'ERROR: Can not execute command:', Command
-sys.exit(1)
+EdkLogger.error('Build', COMMAND_FAILURE, 'Can not execute 
command: %s' % Command)
 Result = Process.communicate()
-if Process.returncode <> 0:
-print 'ERROR: Can not collect output from command:', Command
-return Result[0], Result[1]
+return Process.returncode, Result[0], Result[1]
 
 def IntToCString(self, Value, ValueSize):
 Result = '"'
 if not isinstance (Value, str):
 for Index in range(0, ValueSize):
@@ -1383,11 +1383,11 @@ class DscBuildData(PlatformBuildClassObject):
 for FieldName in FieldList:
 IsArray = 

[edk2] [Patch] BaseTools CommonLib: Remove the unnecessary print message in PcdValueCommon

[Liming Gao]

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Liming Gao 
---
 BaseTools/Source/C/Common/PcdValueCommon.c | 10 --
 1 file changed, 10 deletions(-)

diff --git a/BaseTools/Source/C/Common/PcdValueCommon.c 
b/BaseTools/Source/C/Common/PcdValueCommon.c
index 92328da..210f87b 100644
--- a/BaseTools/Source/C/Common/PcdValueCommon.c
+++ b/BaseTools/Source/C/Common/PcdValueCommon.c
@@ -330,9 +330,7 @@ Returns:
 break;
   case PcdDataTypePointer:
 Value = [Index].Value[1];
-printf ("Value = %s\n", PcdList[Index].Value);
 for (*Size = 0, Byte = (UINT8) strtoul(Value, , 16); Value != End; 
Byte = (UINT8) strtoul(Value, , 16), *Size = *Size + 1) {
-  printf("%x\n", Byte);
   Value = End + 1;
 }
 Buffer = malloc(*Size + 1);
@@ -401,7 +399,6 @@ Returns:
 PcdList[Index].Value = malloc(Size * 5 + 3);
 PcdList[Index].Value[0] = '{';
 for (ValueIndex = 0; ValueIndex < Size; ValueIndex++) {
-  printf("Value[%d] = %02x\n", ValueIndex, Value[ValueIndex]);
   sprintf([Index].Value[1 + ValueIndex * 5], "0x%02x,", 
Value[ValueIndex]);
 }
 PcdList[Index].Value[1 + Size * 5 - 1] = '}';
@@ -724,15 +721,11 @@ Returns:
   if (*InputFileName == NULL) {
 fprintf (stderr, "Missing option.  Input files is not specified\n");
 exit (EXIT_FAILURE);
-  } else {
-printf ("Input file name is %s\n", *InputFileName);
   }
 
   if (*OutputFileName == NULL) {
 fprintf (stderr, "Missing option.  Output file is not specified\n");
 exit (EXIT_FAILURE);
-  } else {
-printf ("Output file name is %s\n", *OutputFileName);
   }
 }
 
@@ -761,7 +754,6 @@ Returns:
   UINT8   *FileBuffer;
   UINT32  FileSize;
 
-  printf ("PCD tool start.\n");
   InputFileName = NULL;
   OutputFileName = NULL;
 
@@ -790,7 +782,5 @@ Returns:
   //
   WriteOutputFile (OutputFileName);
 
-  printf ("PCD tool done.\n");
-
   exit (EXIT_SUCCESS);
 }
-- 
2.8.0.windows.1

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

Re: [edk2] [Patch] DSC spec: Update Skuid value to support Hex number

[Gao, Liming]

Reviewed-by: Liming Gao 

>-Original Message-
>From: Zhu, Yonghong
>Sent: Tuesday, January 23, 2018 11:18 AM
>To: edk2-devel@lists.01.org
>Cc: Gao, Liming ; Kinney, Michael D
>; Shaw, Kevin W 
>Subject: [Patch] DSC spec: Update Skuid value to support Hex number
>
>Original Skuid value only support to use integer, this patch extend it
>to also support hex number.
>
>Cc: Liming Gao 
>Cc: Michael Kinney 
>Cc: Kevin W Shaw 
>Contributed-under: TianoCore Contribution Agreement 1.1
>Signed-off-by: Yonghong Zhu 
>---
> 2_dsc_overview/25_[skuids]_section_processing.md | 2 +-
> 3_edk_ii_dsc_file_format/37_[skuids]_section.md  | 4 ++--
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
>diff --git a/2_dsc_overview/25_[skuids]_section_processing.md
>b/2_dsc_overview/25_[skuids]_section_processing.md
>index 428f7cf..afd102e 100644
>--- a/2_dsc_overview/25_[skuids]_section_processing.md
>+++ b/2_dsc_overview/25_[skuids]_section_processing.md
>@@ -35,11 +35,11 @@ The contents of this section are used to define valid
>`SKUID_IDENTIFIER` names.
> Since a platform may support different SKUs, and different SKUs may
>implement
> different methods for handing platform configuration data (PCD) the user can
> define, in this section, tag names to use. Use `0` for the `DEFAULT` SKU
> identifier. Each entry below the section header is of the form:
>
>-`integer | word`
>+`Number | word`
>
> The following is an example of a `[SkuIds]` section:
>
> ```
> [SkuIds]
>diff --git a/3_edk_ii_dsc_file_format/37_[skuids]_section.md
>b/3_edk_ii_dsc_file_format/37_[skuids]_section.md
>index 97642e9..756ded2 100644
>--- a/3_edk_ii_dsc_file_format/37_[skuids]_section.md
>+++ b/3_edk_ii_dsc_file_format/37_[skuids]_section.md
>@@ -42,18 +42,18 @@ recommended that a ".txt" extension be used.
>
> If this section is not specified, the parsing tools will assume a SkuId of 0,
> with a `UiName` of "DEFAULT". The default entry must not be re-defined.
>
> The `!include` file can only contain an ASCII (not Unicode) list of
>-"integer|UiSkuName" statements.
>+"Number|UiSkuName" statements.
>
>  Prototype
>
> ```c
>  ::= "[SkuIds]" 
> {*} {}
>- ::= 
>+ ::= 
> ::= 
> ```
>
>  Example
>
>--
>2.6.1.windows.1

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

Re: [edk2] [Patch] BaseTools: Update BPDG to support L'' and '' format as VPD Pcd Value

[Gao, Liming]

Reviewed-by: Liming Gao 

>-Original Message-
>From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>Yonghong Zhu
>Sent: Tuesday, January 30, 2018 11:20 PM
>To: edk2-devel@lists.01.org
>Subject: [edk2] [Patch] BaseTools: Update BPDG to support L'' and '' format as
>VPD Pcd Value
>
>Current Pcd value support flexible format, this patch add support for
>BPDG Tool to support L'' and '' format.
>
>Contributed-under: TianoCore Contribution Agreement 1.1
>Signed-off-by: Yonghong Zhu 
>---
> BaseTools/Source/Python/BPDG/GenVpd.py | 32 ++
>--
> BaseTools/Source/Python/Common/Misc.py |  6 +++---
> 2 files changed, 21 insertions(+), 17 deletions(-)
>
>diff --git a/BaseTools/Source/Python/BPDG/GenVpd.py
>b/BaseTools/Source/Python/BPDG/GenVpd.py
>index 9861e7d..612400d 100644
>--- a/BaseTools/Source/Python/BPDG/GenVpd.py
>+++ b/BaseTools/Source/Python/BPDG/GenVpd.py
>@@ -59,11 +59,11 @@ class PcdEntry:
> EdkLogger.error("BPDG", BuildToolError.FORMAT_INVALID,
> "Invalid PCD format(Name: %s File: %s Line: %s), 
> no PcdSize
>specified!" % (self.PcdCName, self.FileName, self.Lineno))
>
> self._GenOffsetValue ()
>
>-## Analyze the string value to judge the PCD's datum type euqal to Boolean
>or not.
>+## Analyze the string value to judge the PCD's datum type equal to
>Boolean or not.
> #
> #  @param   ValueString  PCD's value
> #  @param   Size PCD's size
> #
> #  @retval  True   PCD's datum type is Boolean
>@@ -162,42 +162,45 @@ class PcdEntry:
> "Invalid size or value for PCD %s to pack(File: 
> %s Line: %s)." %
>(self.PcdCName, self.FileName, self.Lineno))
>
> ## Pack VOID* type VPD PCD's value form string to binary type.
> #
> #  The VOID* type of string divided into 3 sub-type:
>-#1:L"String", Unicode type string.
>-#2:"String",  Ascii type string.
>+#1:L"String"/L'String', Unicode type string.
>+#2:"String"/'String',  Ascii type string.
> #3:{bytearray}, only support byte-array.
> #
> #  @param ValueString The Integer type string for pack.
> #
> def _PackPtrValue(self, ValueString, Size):
>-if ValueString.startswith('L"'):
>+if ValueString.startswith('L"') or ValueString.startswith("L'"):
> self._PackUnicode(ValueString, Size)
> elif ValueString.startswith('{') and ValueString.endswith('}'):
> self._PackByteArray(ValueString, Size)
>-elif ValueString.startswith('"') and ValueString.endswith('"'):
>+elif (ValueString.startswith('"') and ValueString.endswith('"')) or
>(ValueString.startswith("'") and ValueString.endswith("'")):
> self._PackString(ValueString, Size)
> else:
> EdkLogger.error("BPDG", BuildToolError.FORMAT_INVALID,
> "Invalid VOID* type PCD %s value %s (File: %s 
> Line: %s)" %
>(self.PcdCName, ValueString, self.FileName, self.Lineno))
>
> ## Pack an Ascii PCD value.
> #
>-#  An Ascii string for a PCD should be in format as  "".
>+#  An Ascii string for a PCD should be in format as  ""/''.
> #
> def _PackString(self, ValueString, Size):
> if (Size < 0):
> EdkLogger.error("BPDG", BuildToolError.FORMAT_INVALID,
> "Invalid parameter Size %s of PCD %s!(File: %s 
> Line: %s)" %
>(self.PcdBinSize, self.PcdCName, self.FileName, self.Lineno))
> if (ValueString == ""):
> EdkLogger.error("BPDG", BuildToolError.FORMAT_INVALID, "Invalid
>parameter ValueString %s of PCD %s!(File: %s Line: %s)" %
>(self.PcdUnpackValue, self.PcdCName, self.FileName, self.Lineno))
>-if (len(ValueString) < 2):
>-EdkLogger.error("BPDG", BuildToolError.FORMAT_INVALID, "For
>PCD: %s ,ASCII string %s at least contains two!(File: %s Line: %s)" %
>(self.PcdCName, self.PcdUnpackValue, self.FileName, self.Lineno))
>+
>+QuotedFlag = True
>+if ValueString.startswith("'"):
>+QuotedFlag = False
>
> ValueString = ValueString[1:-1]
>-if len(ValueString) + 1 > Size:
>+# No null-terminator in 'string'
>+if (QuotedFlag and len(ValueString) + 1 > Size) or (not QuotedFlag and
>len(ValueString) > Size):
> EdkLogger.error("BPDG", BuildToolError.RESOURCE_OVERFLOW,
> "PCD value string %s is exceed to size %d(File: 
> %s Line: %s)" %
>(ValueString, Size, self.FileName, self.Lineno))
> try:
> self.PcdValue = pack('%ds' % Size, ValueString)
> except:
>@@ -256,23 +259,24 @@ class PcdEntry:
>
> self.PcdValue = ReturnArray.tolist()
>
> ## Pack a unicode PCD value into byte array.
> #
>-#  A unicode string for a PCD should be in format as  L"".
>+#  A unicode string for a PCD 

Re: [edk2] [Patch] BaseTools: Fix the bug to align VPD PCD based on value type

[Gao, Liming]

Reviewed-by: Liming Gao 

>-Original Message-
>From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>Yonghong Zhu
>Sent: Tuesday, January 30, 2018 2:57 PM
>To: edk2-devel@lists.01.org
>Subject: [edk2] [Patch] BaseTools: Fix the bug to align VPD PCD based on
>value type
>
>Spec required for VOID* VPD Pcd, Ascii string use byte alignment, byte
>array use 8-byte alignment, unicode string use 2-byte alignment.
>while when the VPD pcd offset use *, the offset generated in the .map
>file not follow this rule.
>
>Contributed-under: TianoCore Contribution Agreement 1.1
>Signed-off-by: Yonghong Zhu 
>---
> BaseTools/Source/Python/BPDG/GenVpd.py | 14 --
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
>diff --git a/BaseTools/Source/Python/BPDG/GenVpd.py
>b/BaseTools/Source/Python/BPDG/GenVpd.py
>index 9861e7d..ec4da23 100644
>--- a/BaseTools/Source/Python/BPDG/GenVpd.py
>+++ b/BaseTools/Source/Python/BPDG/GenVpd.py
>@@ -1,10 +1,10 @@
> ## @file
> #  This file include GenVpd class for fix the Vpd type PCD offset, and 
> PcdEntry
>for describe
> #  and process each entry of vpd type PCD.
> #
>-#  Copyright (c) 2010 - 2016, Intel Corporation. All rights reserved.
>+#  Copyright (c) 2010 - 2018, Intel Corporation. All rights reserved.
> #
> #  This program and the accompanying materials
> #  are licensed and made available under the terms and conditions of the BSD
>License
> #  which accompanies this distribution.  The full text of the license may be
>found at
> #  http://opensource.org/licenses/bsd-license.php
>@@ -33,21 +33,22 @@ _FORMAT_CHAR = {1: 'B',
> #
> #  This class contain method to format and pack pcd's value.
> #
> class PcdEntry:
> def __init__(self, PcdCName, SkuId,PcdOffset, PcdSize, PcdValue,
>Lineno=None, FileName=None, PcdUnpackValue=None,
>- PcdBinOffset=None, PcdBinSize=None):
>+ PcdBinOffset=None, PcdBinSize=None, Alignment=None):
> self.PcdCName   = PcdCName.strip()
> self.SkuId  = SkuId.strip()
> self.PcdOffset  = PcdOffset.strip()
> self.PcdSize= PcdSize.strip()
> self.PcdValue   = PcdValue.strip()
> self.Lineno = Lineno.strip()
> self.FileName   = FileName.strip()
> self.PcdUnpackValue = PcdUnpackValue
> self.PcdBinOffset   = PcdBinOffset
> self.PcdBinSize = PcdBinSize
>+self.Alignment   = Alignment
>
> if self.PcdValue == '' :
> EdkLogger.error("BPDG", BuildToolError.FORMAT_INVALID,
> "Invalid PCD format(Name: %s File: %s line: %s) , 
> no Value
>specified!" % (self.PcdCName, self.FileName, self.Lineno))
>
>@@ -432,20 +433,22 @@ class GenVPD :
> elif PCD.PcdUnpackValue.startswith("L"):
> Alignment = 2
> else:
> Alignment = 1
>
>+PCD.Alignment = Alignment
> if PCD.PcdOffset != '*':
> if PCD.PcdOccupySize % Alignment != 0:
> if PCD.PcdUnpackValue.startswith("{"):
> EdkLogger.warn("BPDG", "The offset value of PCD 
> %s is not 8-
>byte aligned!" %(PCD.PcdCName), File=self.InputFileName)
> else:
> EdkLogger.error("BPDG", 
> BuildToolError.FORMAT_INVALID,
>'The offset value of PCD %s should be %s-byte aligned.' % (PCD.PcdCName,
>Alignment))
> else:
> if PCD.PcdOccupySize % Alignment != 0:
> PCD.PcdOccupySize = (PCD.PcdOccupySize / Alignment + 
> 1) *
>Alignment
>
>+PackSize = PCD.PcdOccupySize
> if PCD._IsBoolean(PCD.PcdValue, PCD.PcdSize):
> PCD._PackBooleanValue(PCD.PcdValue)
> self.FileLinesList[count] = PCD
> count += 1
> continue
>@@ -516,10 +519,12 @@ class GenVPD :
> #
> if (len(self.PcdFixedOffsetSizeList) == 0) and
>(len(self.PcdUnknownOffsetList) != 0) :
> # The offset start from 0
> NowOffset = 0
> for Pcd in self.PcdUnknownOffsetList :
>+if NowOffset % Pcd.Alignment != 0:
>+NowOffset = (NowOffset/ Pcd.Alignment + 1) * Pcd.Alignment
> Pcd.PcdBinOffset = NowOffset
> Pcd.PcdOffset= str(hex(Pcd.PcdBinOffset))
> NowOffset   += Pcd.PcdOccupySize
>
> self.PcdFixedOffsetSizeList = self.PcdUnknownOffsetList
>@@ -578,10 +583,12 @@ class GenVPD :
> while(countOfUnfixedList < lenOfUnfixedList) :
> eachUnfixedPcd  =
>self.PcdUnknownOffsetList[countOfUnfixedList]
> needFixPcdSize  = eachUnfixedPcd.PcdOccupySize
> # Not been fixed
> 

[edk2] [Patch] BaseTools: Structure Pcd in CommandLine.

[BobCF]

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Bob Feng 
Cc: Liming Gao 
---
 BaseTools/Source/Python/AutoGen/AutoGen.py| 110 +++-
 BaseTools/Source/Python/Common/Expression.py  |   2 +-
 BaseTools/Source/Python/Common/Misc.py|   2 +-
 BaseTools/Source/Python/GenFds/FfsInfStatement.py |   1 +
 BaseTools/Source/Python/GenFds/GenFds.py  |   4 +-
 BaseTools/Source/Python/Workspace/DscBuildData.py | 210 +-
 BaseTools/Source/Python/build/build.py|   2 +-
 7 files changed, 233 insertions(+), 98 deletions(-)

diff --git a/BaseTools/Source/Python/AutoGen/AutoGen.py 
b/BaseTools/Source/Python/AutoGen/AutoGen.py
index 1cf50e872f..405bfa145a 100644
--- a/BaseTools/Source/Python/AutoGen/AutoGen.py
+++ b/BaseTools/Source/Python/AutoGen/AutoGen.py
@@ -396,100 +396,22 @@ class WorkspaceAutoGen(AutoGen):
 
 # apply SKU and inject PCDs from Flash Definition file
 for Arch in self.ArchList:
 Platform = self.BuildDatabase[self.MetaFile, Arch, Target, 
Toolchain]
 
-DecPcds = {}
-DecPcdsKey = set()
-PGen = PlatformAutoGen(self, self.MetaFile, Target, Toolchain, 
Arch)
-if GlobalData.BuildOptionPcd:
-for i, pcd in enumerate(GlobalData.BuildOptionPcd):
-if type(pcd) is tuple:
-continue
-(pcdname, pcdvalue) = pcd.split('=')
-if not pcdvalue:
-EdkLogger.error('build', AUTOGEN_ERROR, "No Value 
specified for the PCD %s." % (pcdname))
-if '.' in pcdname:
-(TokenSpaceGuidCName, TokenCName) = pcdname.split('.')
-HasTokenSpace = True
-else:
-TokenCName = pcdname
-TokenSpaceGuidCName = ''
-HasTokenSpace = False
-TokenSpaceGuidCNameList = []
-FoundFlag = False
-PcdDatumType = ''
-NewValue = ''
-for package in PGen.PackageList:
-Guids = package.Guids
-self._GuidDict.update(Guids)
-for package in PGen.PackageList:
-for key in package.Pcds:
-PcdItem = package.Pcds[key]
-if HasTokenSpace:
-if (PcdItem.TokenCName, 
PcdItem.TokenSpaceGuidCName) == (TokenCName, TokenSpaceGuidCName):
-PcdDatumType = PcdItem.DatumType
-if pcdvalue.startswith('H'):
-try:
-pcdvalue = 
ValueExpressionEx(pcdvalue[1:], PcdDatumType, self._GuidDict)(True)
-except BadExpression, Value:
-if Value.result > 1:
-EdkLogger.error('Parser', 
FORMAT_INVALID, 'PCD [%s.%s] Value "%s",  %s' %
-
(TokenSpaceGuidCName, TokenCName, pcdvalue, Value))
-pcdvalue = 'H' + pcdvalue
-NewValue = 
BuildOptionPcdValueFormat(TokenSpaceGuidCName, TokenCName, PcdDatumType, 
pcdvalue)
-FoundFlag = True
-else:
-if PcdItem.TokenCName == TokenCName:
-if not PcdItem.TokenSpaceGuidCName in 
TokenSpaceGuidCNameList:
-if len (TokenSpaceGuidCNameList) < 1:
-
TokenSpaceGuidCNameList.append(PcdItem.TokenSpaceGuidCName)
-PcdDatumType = PcdItem.DatumType
-TokenSpaceGuidCName = 
PcdItem.TokenSpaceGuidCName
-if pcdvalue.startswith('H'):
-try:
-pcdvalue = 
ValueExpressionEx(pcdvalue[1:], PcdDatumType, self._GuidDict)(True)
-except BadExpression, Value:
-EdkLogger.error('Parser', 
FORMAT_INVALID, 'PCD [%s.%s] Value "%s", %s' %
-
(TokenSpaceGuidCName, TokenCName, pcdvalue, Value))
-pcdvalue = 'H' + pcdvalue
-NewValue = 
BuildOptionPcdValueFormat(TokenSpaceGuidCName, TokenCName, PcdDatumType, 
pcdvalue)
-