date:20171205

[edk2] [PATCH 3/3] IntelFrameworkModulePkg/KeyboardDxe: Use macro to enable/disable page 0

2017-12-05 Thread Jian J Wang

Current implementation uses following two methods

EnableNullDetection()
DisableNullDetection()

to enable/disable page 0. These two methods will check PCD
PcdNullPointerDetectionPropertyMask to know if the page 0 is disabled or not.
This is due to the fact that old GCD service doesn't provide paging related
attributes of memory block. Since this issue has been fixed, GCD services
can be used to determine the paging status of page 0. This is also make it
possible to just use a new macro

ACCESS_PAGE0_CODE(
  {
  
  }
);

to replace above methods to do the same job, which also makes code more
readability.

Cc: Liming Gao 
Cc: Michael D Kinney 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang 
---
 .../Csm/BiosThunk/KeyboardDxe/BiosKeyboard.c   | 118 ++---
 .../Csm/BiosThunk/KeyboardDxe/KeyboardDxe.inf  |   1 -
 2 files changed, 10 insertions(+), 109 deletions(-)

diff --git a/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/BiosKeyboard.c 
b/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/BiosKeyboard.c
index ebf03d30c1..c7797acfb8 100644
--- a/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/BiosKeyboard.c
+++ b/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/BiosKeyboard.c
@@ -1732,98 +1732,6 @@ CheckKeyboardConnect (
   }
 }
 
-/**
-  Disable NULL pointer detection.
-**/
-VOID
-DisableNullDetection (
-  VOID
-  )
-{
-  EFI_STATUSStatus;
-  EFI_GCD_MEMORY_SPACE_DESCRIPTOR   Desc;
-
-  if ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & BIT0) == 0) {
-return;
-  }
-
-  //
-  // Check current capabilities and attributes
-  //
-  Status = gDS->GetMemorySpaceDescriptor (0, );
-  ASSERT_EFI_ERROR (Status);
-
-  //
-  // Try to add EFI_MEMORY_RP support if necessary
-  //
-  if ((Desc.Capabilities & EFI_MEMORY_RP) == 0) {
-Desc.Capabilities |= EFI_MEMORY_RP;
-Status = gDS->SetMemorySpaceCapabilities (0, EFI_PAGES_TO_SIZE(1),
-  Desc.Capabilities);
-ASSERT_EFI_ERROR (Status);
-if (EFI_ERROR (Status)) {
-  return;
-}
-  }
-
-  //
-  // Don't bother if EFI_MEMORY_RP is already cleared.
-  //
-  if ((Desc.Attributes & EFI_MEMORY_RP) != 0) {
-Desc.Attributes &= ~EFI_MEMORY_RP;
-Status = gDS->SetMemorySpaceAttributes (0, EFI_PAGES_TO_SIZE(1),
-Desc.Attributes);
-ASSERT_EFI_ERROR (Status);
-  } else {
-DEBUG ((DEBUG_WARN, "!!! Page 0 is supposed to be disabled !!!\r\n"));
-  }
-}
-
-/**
-   Enable NULL pointer detection.
-**/
-VOID
-EnableNullDetection (
-  VOID
-  )
-{
-  EFI_STATUSStatus;
-  EFI_GCD_MEMORY_SPACE_DESCRIPTOR   Desc;
-
-  if ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & BIT0) == 0) {
-return;
-  }
-
-  //
-  // Check current capabilities and attributes
-  //
-  Status = gDS->GetMemorySpaceDescriptor (0, );
-  ASSERT_EFI_ERROR (Status);
-
-  //
-  // Try to add EFI_MEMORY_RP support if necessary
-  //
-  if ((Desc.Capabilities & EFI_MEMORY_RP) == 0) {
-Desc.Capabilities |= EFI_MEMORY_RP;
-Status = gDS->SetMemorySpaceCapabilities (0, EFI_PAGES_TO_SIZE(1),
-  Desc.Capabilities);
-ASSERT_EFI_ERROR (Status);
-if (EFI_ERROR (Status)) {
-  return;
-}
-  }
-
-  //
-  // Don't bother if EFI_MEMORY_RP is already set.
-  //
-  if ((Desc.Attributes & EFI_MEMORY_RP) == 0) {
-Desc.Attributes |= EFI_MEMORY_RP;
-Status = gDS->SetMemorySpaceAttributes (0, EFI_PAGES_TO_SIZE(1),
-Desc.Attributes);
-ASSERT_EFI_ERROR (Status);
-  }
-}
-
 /**
   Timer event handler: read a series of key stroke from 8042
   and put them into memory key buffer. 
@@ -1931,16 +1839,13 @@ BiosKeyboardTimerHandler (
   //   0 Right Shift pressed
 
 
-  //
-  // Disable NULL pointer detection temporarily
-  //
-  DisableNullDetection ();
-
   //
   // Clear the CTRL and ALT BDA flag
   //
-  KbFlag1 = *((UINT8 *) (UINTN) 0x417);  // read the STATUS FLAGS 1
-  KbFlag2 = *((UINT8 *) (UINTN) 0x418); // read STATUS FLAGS 2
+  ACCESS_PAGE0_CODE ({
+KbFlag1 = *((UINT8 *) (UINTN) 0x417); // read the STATUS FLAGS 1
+KbFlag2 = *((UINT8 *) (UINTN) 0x418); // read STATUS FLAGS 2
+  });
 
   DEBUG_CODE (
 {
@@ -2008,15 +1913,12 @@ BiosKeyboardTimerHandler (
   //
   // Clear left alt and left ctrl BDA flag
   //
-  KbFlag2 &= ~(KB_LEFT_ALT_PRESSED | KB_LEFT_CTRL_PRESSED);
-  *((UINT8 *) (UINTN) 0x418) = KbFlag2;
-  KbFlag1 &= ~0x0C;  
-  *((UINT8 *) (UINTN) 0x417) = KbFlag1; 
-
-  //
-  // Restore NULL pointer detection
-  //
-  EnableNullDetection ();
+  ACCESS_PAGE0_CODE ({
+KbFlag2 &= ~(KB_LEFT_ALT_PRESSED | KB_LEFT_CTRL_PRESSED);
+*((UINT8 *) (UINTN) 0x418) = KbFlag2;
+KbFlag1 &= ~0x0C;
+*((UINT8 *) (UINTN) 0x417) =

[edk2] [PATCH 2/3] IntelFrameworkModulePkg/LegacyBiosDxe: Use macro to enable/disable page 0

2017-12-05 Thread Jian J Wang

Current implementation uses following two methods

EnableNullDetection()
DisableNullDetection()

to enable/disable page 0. These two methods will check PCD
PcdNullPointerDetectionPropertyMask to know if the page 0 is disabled or not.
This is due to the fact that old GCD service doesn't provide paging related
attributes of memory block. Since this issue has been fixed, GCD services
can be used to determine the paging status of page 0. This is also make it
possible to just use a new macro

ACCESS_PAGE0_CODE(
  {
  
  }
);

to replace above methods to do the same job, which also makes code more
readability.

Cc: Liming Gao 
Cc: Michael D Kinney 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang 
---
 .../Csm/LegacyBiosDxe/LegacyBda.c  |  53 
 .../Csm/LegacyBiosDxe/LegacyBios.c | 135 ++---
 .../Csm/LegacyBiosDxe/LegacyBiosDxe.inf|   1 -
 .../Csm/LegacyBiosDxe/LegacyBiosInterface.h|  16 ---
 .../Csm/LegacyBiosDxe/LegacyBootSupport.c  |  80 ++--
 .../Csm/LegacyBiosDxe/LegacyPci.c  |  72 ++-
 IntelFrameworkModulePkg/Csm/LegacyBiosDxe/Thunk.c  |  51 
 7 files changed, 135 insertions(+), 273 deletions(-)

diff --git a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBda.c 
b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBda.c
index c6670febee..9667dc2a0f 100644
--- a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBda.c
+++ b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBda.c
@@ -34,37 +34,36 @@ LegacyBiosInitBda (
   BDA_STRUC *Bda;
   UINT8 *Ebda;
 
-  DisableNullDetection ();
-
   Bda   = (BDA_STRUC *) ((UINTN) 0x400);
   Ebda  = (UINT8 *) ((UINTN) 0x9fc00);
 
-  ZeroMem (Bda, 0x100);
+  ACCESS_PAGE0_CODE ({
+ZeroMem (Bda, 0x100);
+//
+// 640k-1k for EBDA
+//
+Bda->MemSize= 0x27f;
+Bda->KeyHead= 0x1e;
+Bda->KeyTail= 0x1e;
+Bda->FloppyData = 0x00;
+Bda->FloppyTimeout  = 0xff;
+
+Bda->KeyStart   = 0x001E;
+Bda->KeyEnd = 0x003E;
+Bda->KeyboardStatus = 0x10;
+Bda->Ebda   = 0x9fc0;
+
+//
+// Move LPT time out here and zero out LPT4 since some SCSI OPROMS
+// use this as scratch pad (LPT4 is Reserved)
+//
+Bda->Lpt1_2Timeout  = 0x1414;
+Bda->Lpt3_4Timeout  = 0x1400;
+
+  });
+
   ZeroMem (Ebda, 0x400);
-  //
-  // 640k-1k for EBDA
-  //
-  Bda->MemSize= 0x27f;
-  Bda->KeyHead= 0x1e;
-  Bda->KeyTail= 0x1e;
-  Bda->FloppyData = 0x00;
-  Bda->FloppyTimeout  = 0xff;
-
-  Bda->KeyStart   = 0x001E;
-  Bda->KeyEnd = 0x003E;
-  Bda->KeyboardStatus = 0x10;
-  Bda->Ebda   = 0x9fc0;
-
-  //
-  // Move LPT time out here and zero out LPT4 since some SCSI OPROMS
-  // use this as scratch pad (LPT4 is Reserved)
-  //
-  Bda->Lpt1_2Timeout  = 0x1414;
-  Bda->Lpt3_4Timeout  = 0x1400;
-
-  *Ebda   = 0x01;
-
-  EnableNullDetection ();
+  *Ebda = 0x01;
 
   return EFI_SUCCESS;
 }
diff --git a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBios.c 
b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBios.c
index c6461f5547..d50c15eacb 100644
--- a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBios.c
+++ b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBios.c
@@ -786,115 +786,6 @@ ToggleEndOfDxeStatus (
   return;
 }
 
-//
-// Legacy BIOS needs to access memory between 0-4095, which will cause page
-// fault exception if NULL pointer detection mechanism is enabled. Following
-// functions can be used to disable/enable NULL pointer detection before/after
-// accessing those memory.
-//
-
-/**
-   Enable NULL pointer detection.
-**/
-VOID
-EnableNullDetection (
-  VOID
-  )
-{
-  EFI_STATUSStatus;
-  EFI_GCD_MEMORY_SPACE_DESCRIPTOR   Desc;
-
-  if (((PcdGet8 (PcdNullPointerDetectionPropertyMask) & BIT0) == 0)
-  ||
-  ((mEndOfDxe)  &&
-   ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & (BIT7|BIT0))
-== (BIT7|BIT0)))
- ) {
-return;
-  }
-
-  //
-  // Check current capabilities and attributes
-  //
-  Status = gDS->GetMemorySpaceDescriptor (0, );
-  ASSERT_EFI_ERROR (Status);
-
-  //
-  // Try to add EFI_MEMORY_RP support if necessary
-  //
-  if ((Desc.Capabilities & EFI_MEMORY_RP) == 0) {
-Desc.Capabilities |= EFI_MEMORY_RP;
-Status = gDS->SetMemorySpaceCapabilities (0, EFI_PAGES_TO_SIZE(1),
-  Desc.Capabilities);
-ASSERT_EFI_ERROR (Status);
-if (EFI_ERROR (Status)) {
-  return;
-}
-  }
-
-  //
-  // Don't bother if EFI_MEMORY_RP is already set.
-  //
-  if ((Desc.Attributes & EFI_MEMORY_RP) == 0) {
-Desc.Attributes |= EFI_MEMORY_RP;
-Status = gDS->SetMemorySpaceAttributes (0, EFI_PAGES_TO_SIZE(1),
-

[edk2] [PATCH 0/3] Remove dependency on PcdNullPointerDetectionPropertyMask

2017-12-05 Thread Jian J Wang

Due to the introduction of NULL pointer detection feature, page 0 will be
disabled if the feature is enabled, which will cause legacy code failed to
update legacy data in page 0.

To avoid page fault caused by above feature in legacy code, legacy related
drivers have to enable page 0 temporarily before accessing page 0 and disable
it afterwards.

Old GCD servie has a bug which paging related attributes are not awared and
managed by GCD service. The legacy code has to use PCD
PcdNullPointerDetectionPropertyMask to know if page 0 is disabled or not.
As a result, two methods

EnableNullDetection()
DisableNullDetection()

were introduced to do enable/disable job just mentioned.

But the newly added PcdNullPointerDetectionPropertyMask caused backward
compatibility issue in some packages having legcy drivers. Since the
attributes missing issue in GCD services has been fixed, it's now able to
eliminate the dependency on this PCD.

Jian J Wang (3):
  IntelFrameworkPkg/LegacyBios.h: Add a macro to guarantee page 0 access
  IntelFrameworkModulePkg/LegacyBiosDxe: Use macro to enable/disable
page 0
  IntelFrameworkModulePkg/KeyboardDxe: Use macro to enable/disable page
0

 .../Csm/BiosThunk/KeyboardDxe/BiosKeyboard.c   | 118 ++
 .../Csm/BiosThunk/KeyboardDxe/KeyboardDxe.inf  |   1 -
 .../Csm/LegacyBiosDxe/LegacyBda.c  |  53 
 .../Csm/LegacyBiosDxe/LegacyBios.c | 135 ++---
 .../Csm/LegacyBiosDxe/LegacyBiosDxe.inf|   1 -
 .../Csm/LegacyBiosDxe/LegacyBiosInterface.h|  16 ---
 .../Csm/LegacyBiosDxe/LegacyBootSupport.c  |  80 ++--
 .../Csm/LegacyBiosDxe/LegacyPci.c  |  72 ++-
 IntelFrameworkModulePkg/Csm/LegacyBiosDxe/Thunk.c  |  51 
 IntelFrameworkPkg/Include/Protocol/LegacyBios.h|  34 ++
 10 files changed, 179 insertions(+), 382 deletions(-)

-- 
2.15.1.windows.2

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

[edk2] [PATCH 1/3] IntelFrameworkPkg/LegacyBios.h: Add a macro to guarantee page 0 access

2017-12-05 Thread Jian J Wang

Due to the introduction of NULL pointer detection feature, page 0 will be
disabled if the feature is enabled, which will cause legacy code failed to
update legacy data in page 0. This macro is introduced to make sure the
page 0 is enabled before those code and restore the original status of it
afterwards.

Another reason to introduce this macro is to eliminate the dependency on
the PcdNullPointerDetectionPropertyMask. Because this is a new PCD, it
could cause some backward compatibility issue for some old packages.

This macro will simply check if the page 0 is disabled or not. If it's
disabled, it will enable it before code updating page 0 and disable it
afterwards. Otherwise, this macro will do nothing to page 0.

The usage of the macro will be look like (similar to DEBUG_CODE macro):

ACCESS_PAGE0_CODE(
  {
  
  }
);

Cc: Liming Gao 
Cc: Michael D Kinney 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang 
---
 IntelFrameworkPkg/Include/Protocol/LegacyBios.h | 34 +
 1 file changed, 34 insertions(+)

diff --git a/IntelFrameworkPkg/Include/Protocol/LegacyBios.h 
b/IntelFrameworkPkg/Include/Protocol/LegacyBios.h
index 641f101bce..f77c92ba21 100644
--- a/IntelFrameworkPkg/Include/Protocol/LegacyBios.h
+++ b/IntelFrameworkPkg/Include/Protocol/LegacyBios.h
@@ -1518,6 +1518,40 @@ struct _EFI_LEGACY_BIOS_PROTOCOL {
   EFI_LEGACY_BIOS_BOOT_UNCONVENTIONAL_DEVICE  BootUnconventionalDevice;
 };
 
+//
+// Legacy BIOS needs to access memory in page 0 (0-4095), which is disabled if
+// NULL pointer detection feature is enabled. Following macro can be used to
+// enable/disable page 0 before/after accessing it.
+//
+#define ACCESS_PAGE0_CODE(statements)   \
+  do {  \
+EFI_STATUSStatus_;  \
+EFI_GCD_MEMORY_SPACE_DESCRIPTOR   Desc_;\
+\
+Status_ = gDS->GetMemorySpaceDescriptor (0, _);\
+if (!EFI_ERROR (Status_)) { \
+  if ((Desc_.Attributes & EFI_MEMORY_RP) != 0) {\
+Status_ = gDS->SetMemorySpaceAttributes (   \
+0,  \
+EFI_PAGES_TO_SIZE(1),   \
+Desc_.Attributes &= ~EFI_MEMORY_RP  \
+);  \
+ASSERT_EFI_ERROR (Status_); \
+  } \
+\
+  statements;   \
+\
+  if ((Desc_.Attributes & EFI_MEMORY_RP) != 0) {\
+Status_ = gDS->SetMemorySpaceAttributes (   \
+0,  \
+EFI_PAGES_TO_SIZE(1),   \
+Desc_.Attributes\
+);  \
+ASSERT_EFI_ERROR (Status_); \
+  } \
+}   \
+  } while (FALSE)
+
 extern EFI_GUID gEfiLegacyBiosProtocolGuid;
 
 #endif
-- 
2.15.1.windows.2

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

Re: [edk2] [PATCH v3 0/2] Enable page table write protection

2017-12-05 Thread Yao, Jiewen

Thanks you. It looks good to me.
Reviewed-by: jiewen@intel.com

I suggest CPU owner can have double check the code before check in.

Thank you
Yao Jiewen


> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Jian J
> Wang
> Sent: Tuesday, December 5, 2017 4:16 PM
> To: edk2-devel@lists.01.org
> Subject: [edk2] [PATCH v3 0/2] Enable page table write protection
> 
> > v3 changes:
> >  a. According to code review comments, remove the public definitions of
> > page table pool. Now the DxeIpl and CpuDxe will have their own page
> > table pool but in the same mechanism. Related PCDs, GUDI and headers
> > are also removed.
> >  b. Apply protection to all page tables, including new ones added in
> > CpuDxe driver.
> >  c. Code/comments cleanup.
> 
> > v2 changes:
> >  a. Enable protection on any newly added page table after DxeIpl.
> >  b. Introduce page table pool concept to make page table allocation
> > and protection easier and error free.
> 
> Write Protect feature (CR0.WP) is always enabled in driver UefiCpuPkg/CpuDxe.
> But the memory pages used for page table are not set as read-only in the 
> driver
> DxeIplPeim, after the paging is setup. This might jeopardize the page table
> integrity if there's buffer overflow occured in other part of system.
> 
> This patch series will change this situation by clearing R/W bit in page 
> attribute
> of the pages used as page table.
> 
> Validation works include booting Windows (10/server 2016) and Linux
> (Fedora/Ubuntu)
> on OVMF and Intel real platform.
> 
> Jian J Wang (2):
>   MdeModulePkg/DxeIpl: Mark page table as read-only
>   UefiCpuPkg/CpuDxe: Enable protection for newly added page table
> 
>  MdeModulePkg/Core/DxeIplPeim/DxeIpl.h|  34 +++
>  MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  |   8 +-
>  MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 301
> ++-
>  MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h |  26 ++
>  UefiCpuPkg/CpuDxe/CpuDxe.c   |  17 +-
>  UefiCpuPkg/CpuDxe/CpuDxe.h   |   2 +
>  UefiCpuPkg/CpuDxe/CpuPageTable.c | 226
> -
>  UefiCpuPkg/CpuDxe/CpuPageTable.h |  34 +++
>  8 files changed, 635 insertions(+), 13 deletions(-)
> 
> --
> 2.15.1.windows.2
> 
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch v2] MdeModulePkg/NetLib: Add NetLibDetectMediaWaitTimeout() API to support EFI_NOT_READY media state detection

2017-12-05 Thread Wu, Jiaxin

Reviewed-by: Jiaxin Wu 



> -Original Message-
> From: Fu, Siyuan
> Sent: Tuesday, December 5, 2017 4:06 PM
> To: Wang, Fan ; edk2-devel@lists.01.org
> Cc: Ye, Ting ; Wu, Jiaxin 
> Subject: RE: [Patch v2] MdeModulePkg/NetLib: Add
> NetLibDetectMediaWaitTimeout() API to support EFI_NOT_READY media
> state detection
> 
> Reviewed-by: Fu Siyuan 
> 
> > -Original Message-
> > From: Wang, Fan
> > Sent: Monday, December 4, 2017 11:42 AM
> > To: edk2-devel@lists.01.org
> > Cc: Fu, Siyuan ; Ye, Ting ; Wu,
> > Jiaxin ; Wang, Fan 
> > Subject: [Patch v2] MdeModulePkg/NetLib: Add
> NetLibDetectMediaWaitTimeout()
> > API to support EFI_NOT_READY media state detection
> >
> > V2:
> >   * Return error status code directly when Aip protocol falied to detect
> > media rather than wait for another time's check.
> >   * Set media state default value to EFI_SUCCESS since some platforms may
> > not support retrieving media state from Aip protocol.
> >
> > Cc: Fu Siyuan 
> > Cc: Ye Ting 
> > Cc: Jiaxin Wu 
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Wang Fan 
> > ---
> >  MdeModulePkg/Include/Library/NetLib.h|  40 +++
> >  MdeModulePkg/Library/DxeNetLib/DxeNetLib.c   | 163
> > +++
> >  MdeModulePkg/Library/DxeNetLib/DxeNetLib.inf |   2 +
> >  3 files changed, 205 insertions(+)
> >
> > diff --git a/MdeModulePkg/Include/Library/NetLib.h
> > b/MdeModulePkg/Include/Library/NetLib.h
> > index b9df46c..7862df9 100644
> > --- a/MdeModulePkg/Include/Library/NetLib.h
> > +++ b/MdeModulePkg/Include/Library/NetLib.h
> > @@ -93,10 +93,16 @@ typedef UINT16  TCP_PORTNO;
> >  #define  DNS_CLASS_INET1
> >  #define  DNS_CLASS_CH  3
> >  #define  DNS_CLASS_HS  4
> >  #define  DNS_CLASS_ANY 255
> >
> > +//
> > +// Number of 100ns units time Interval for network media state detect
> > +//
> > +#define MEDIA_STATE_DETECT_TIME_INTERVAL  100U
> > +
> > +
> >  #pragma pack(1)
> >
> >  //
> >  // Ethernet head definition
> >  //
> > @@ -1246,10 +1252,44 @@ NetLibDetectMedia (
> >IN  EFI_HANDLEServiceHandle,
> >OUT BOOLEAN   *MediaPresent
> >);
> >
> >  /**
> > +
> > +  Detect media state for a network device. This routine will wait for a
> > period of time at
> > +  a specified checking interval when a certain network is under
> > connecting until connection
> > +  process finishes or timeout. If Aip protocol is supported by low layer
> > drivers, three kinds
> > +  of media states can be detected: EFI_SUCCESS, EFI_NOT_READY and
> > EFI_NO_MEDIA, represents
> > +  connected state, connecting state and no media state respectively.
> When
> > function detects
> > +  the current state is EFI_NOT_READY, it will loop to wait for next
> > time's check until state
> > +  turns to be EFI_SUCCESS or EFI_NO_MEDIA. If Aip protocol is not
> > supported, function will
> > +  call NetLibDetectMedia() and return state directly.
> > +
> > +  @param[in]   ServiceHandleThe handle where network service binding
> > protocols are
> > +installed on.
> > +  @param[in]   Timeout  The maximum number of 100ns units to wait
> > when network
> > +is connecting. Zero value means detect
> > once and return
> > +immediately.
> > +  @param[out]  MediaState   The pointer to the detected media state.
> > +
> > +  @retval EFI_SUCCESS   Media detection success.
> > +  @retval EFI_INVALID_PARAMETER ServiceHandle is not a valid network
> > device handle or
> > +MediaState pointer is NULL.
> > +  @retval EFI_DEVICE_ERROR  A device error occurred.
> > +  @retval EFI_TIMEOUT   Network is connecting but timeout.
> > +
> > +**/
> > +EFI_STATUS
> > +EFIAPI
> > +NetLibDetectMediaWaitTimeout (
> > +  IN  EFI_HANDLEServiceHandle,
> > +  IN  UINT64Timeout,
> > +  OUT EFI_STATUS*MediaState
> > +  );
> > +
> > +
> > +/**
> >Create an IPv4 device path node.
> >
> >The header type of IPv4 device path node is MESSAGING_DEVICE_PATH.
> >The header subtype of IPv4 device path node is MSG_IPv4_DP.
> >The length of the IPv4 device path node in bytes is 19.
> > diff --git a/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
> > b/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
> > index b8544b8..1bfa33d 100644
> > --- a/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
> > +++ b/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
> > @@ -17,10 +17,11 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF
> ANY KIND,
> > EITHER EXPRESS OR IMPLIED.
> >  #include 
> >
> >  #include 
> >  #include 
> >

Re: [edk2] [PATCH v3 1/2] MdeModulePkg: introduce SD/MMC override protocol

2017-12-05 Thread Ni, Ruiyu


On 12/6/2017 11:25 AM, Zeng, Star wrote:

It should be not needed for Capability, but may be needed for NotifyPhase.
Hao can explain more.


When the SdMmcPassthru protocol is installed by gBS->InstallProtocol(),
that's the indication of readiness of this protocol.

Before that, it's not guaranteed that every interface of SdMmcPassthru
is safe to call, e.g.: GetNextSlot or ResetDevice may not work because
the device enumeration depends on the SdMmcOverride.

Even for PassThru interface, is might be possible that some private
data needed by PassThru to work hasn't been initialized, or
future implementation might choose to defer some private data
initialization to just before protocol installation.

I think after Hao's evaluation, if PassThru is only needed and
must-use interface for some usage case, we could just pass the PassThru
function pointer to SdMmcOverride.




Thanks,
Star
-Original Message-
From: Ni, Ruiyu
Sent: Wednesday, December 6, 2017 11:22 AM
To: Ard Biesheuvel ; edk2-devel@lists.01.org
Cc: leif.lindh...@linaro.org; Kinney, Michael D ; Zeng, Star 
; Tian, Feng ; Wu, Hao A 

Subject: Re: [PATCH v3 1/2] MdeModulePkg: introduce SD/MMC override protocol

On 12/6/2017 2:01 AM, Ard Biesheuvel wrote:

Many ARM based SoCs have integrated SDHCI controllers, and often,
these implementations deviate in subtle ways from the pertinent
specifications. On the one hand, these deviations are quite easy to
work around, but on the other hand, having a collection of SoC
specific workarounds in the generic driver stack is undesirable.

So let's introduce an optional SD/MMC override protocol that we can
invoke at the appropriate moments in the device initialization.
That way, the workaround itself remains platform specific, but we can
still use the generic driver stack on such platforms.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel 
---
   MdeModulePkg/Include/Protocol/SdMmcOverride.h | 103 
   MdeModulePkg/MdeModulePkg.dec |   3 +
   2 files changed, 106 insertions(+)

diff --git a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
new file mode 100644
index ..af57988f5625
--- /dev/null
+++ b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
@@ -0,0 +1,103 @@
+/** @file
+  Protocol to describe overrides required to support non-standard
+SDHCI
+  implementations
+
+  Copyright (c) 2017, Linaro, Ltd. All rights reserved.
+
+  This program and the accompanying materials  are licensed and made
+ available under the terms and conditions of the BSD License  which
+ accompanies this distribution.  The full text of the license may be
+ found at  http://opensource.org/licenses/bsd-license.php
+
+  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
+ BASIS,  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
IMPLIED.
+
+**/
+
+#ifndef __SD_MMC_OVERRIDE_H__
+#define __SD_MMC_OVERRIDE_H__
+
+#include 
+
+#define EDKII_SD_MMC_OVERRIDE_PROTOCOL_GUID \
+  { 0xeaf9e3c1, 0xc9cd, 0x46db, { 0xa5, 0xe5, 0x5a, 0x12, 0x4c, 0x83,
+0x23, 0x23 } }
+
+#define EDKII_SD_MMC_OVERRIDE_PROTOCOL_VERSION0x1
+
+typedef struct _EDKII_SD_MMC_OVERRIDE EDKII_SD_MMC_OVERRIDE;
+
+typedef enum {
+  EdkiiSdMmcResetPre,
+  EdkiiSdMmcResetPost,
+  EdkiiSdMmcInitHostPre,
+  EdkiiSdMmcInitHostPost,
+} EDKII_SD_MMC_PHASE_TYPE;
+
+/**
+
+  Override function for SDHCI capability bits
+
+  @param[in]  PassThru  A pointer to the
+EFI_SD_MMC_PASS_THRU_PROTOCOL instance.
+  @param[in]  ControllerHandle  The EFI_HANDLE of the controller.
+  @param[in]  Slot  The 0 based slot index.
+  @param[in,out]  SdMmcHcSlotCapability The SDHCI capability structure.
+
+  @retval EFI_SUCCESS   The override function completed successfully.
+  @retval EFI_NOT_FOUND The specified controller or slot does not 
exist.
+  @retval EFI_INVALID_PARAMETER SdMmcHcSlotCapability is NULL
+
+**/
+typedef
+EFI_STATUS
+(EFIAPI * EDKII_SD_MMC_CAPABILITY) (
+  IN  EFI_SD_MMC_PASS_THRU_PROTOCOL   *PassThru,

By looking a bit deeper, I get confused about this parameter.
SdMmcOverride protocol is supposed to be used by SdMmcHostController driver to 
produce SdMmcPassthru protocol.
But how does SdMmcOverride uses the not-yet-produced SdMmcPassthru protocol? 
It's like a chicken-egg problem.


+  IN  EFI_HANDLE  ControllerHandle,
+  IN  UINT8   Slot,
+  IN  OUT UINT64  *SdMmcHcSlotCapability
+  );
+
+/**
+
+  Override function for SDHCI controller operations
+
+  @param[in]  PassThru  A pointer to the
+EFI_SD_MMC_PASS_THRU_PROTOCOL instance.
+  @param[in]

Re: [edk2] [PATCH v3 2/2] MdeModulePkg/SdMmcPciHcDxe: allow HC capabilities to be overridden

2017-12-05 Thread Ni, Ruiyu


Ard,
I should have provided some of them in the last version.
Sorry about that.

We just found an internal/private SdMmcPciHc implementation
developed by other teams. We are evaluating whether your
proposed SdMmcOverride can be used to retire that private
implementation.


On 12/6/2017 2:01 AM, Ard Biesheuvel wrote:

Invoke the newly introduced SD/MMC override protocol to override
the capabilities register after reading it from the device registers,
and to call the pre/post host init and reset hooks at the appropriate
times.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel 
---
  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c   | 134 
+++-
  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h   |   1 +
  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.inf |   2 +
  3 files changed, 133 insertions(+), 4 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c 
b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
index 0be8828abfcc..f1ea78de809e 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
@@ -17,6 +17,8 @@
  
  #include "SdMmcPciHcDxe.h"
  
+STATIC EDKII_SD_MMC_OVERRIDE   *mOverride;

+
  //
  // Driver Global Variables
  //
@@ -214,6 +216,104 @@ Done:
  }
  
  /**

+  Execute a SD/MMC host controller reset
+
+  @param[in] Private  A pointer to the SD_MMC_HC_PRIVATE_DATA instance.
+  @param[in] Slot The slot number of the host controller to reset.
+**/
+STATIC
+EFI_STATUS
+SdMmcPciHcResetHost (
+  IN  SD_MMC_HC_PRIVATE_DATA  *Private,
+  IN  UINT8   Slot
+  )

I checked the implementation of SdMmcHcReset(). It's quite simple.
So how about we do not introduce this new function SdMmcPciHcResetHost,
but just put the two NotifyPhase call inside SdMmcHcReset().

Because the names of the two functions (SdMmcPciHcResetHost and
SdMmcHcReset) are quite similar. After not a long period, maintainer
may get confused about which is which.

I agree parameters of SdMmcHcReset need to change.


+{
+  EFI_STATUSStatus;
+
+  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
+Status = mOverride->NotifyPhase (
+  >PassThru,
+  Private->ControllerHandle,
+  Slot,
+  EdkiiSdMmcResetPre);
+if (EFI_ERROR (Status)) {
+  DEBUG ((DEBUG_WARN,
+"%a: SD/MMC pre reset notifier callback failed - %r\n",
+__FUNCTION__, Status));
+  return Status;
+}
+  }
+
+  Status = SdMmcHcReset (Private->PciIo, Slot);
+  if (EFI_ERROR (Status)) {
+return Status;
+  }
+
+  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
+Status = mOverride->NotifyPhase (
+  >PassThru,
+  Private->ControllerHandle,
+  Slot,
+  EdkiiSdMmcResetPost);
+if (EFI_ERROR (Status)) {
+  DEBUG ((DEBUG_WARN,
+"%a: SD/MMC post reset notifier callback failed - %r\n",
+__FUNCTION__, Status));
+}
+  }
+  return Status;
+}
+
+/**
+  Initialize a SD/MMC host controller
+
+  @param[in] Private  A pointer to the SD_MMC_HC_PRIVATE_DATA instance.
+  @param[in] Slot The slot number of the host controller to initialize.
+**/
+STATIC
+EFI_STATUS
+SdMmcPciHcInitHost (
+  IN  SD_MMC_HC_PRIVATE_DATA  *Private,
+  IN  UINT8   Slot
+  )
+{
+  EFI_STATUSStatus;
+
+  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
+Status = mOverride->NotifyPhase (
+  >PassThru,
+  Private->ControllerHandle,
+  Slot,
+  EdkiiSdMmcInitHostPre);
+if (EFI_ERROR (Status)) {
+  DEBUG ((DEBUG_WARN,
+"%a: SD/MMC pre init notifier callback failed - %r\n",
+__FUNCTION__, Status));
+  return Status;
+}
+  }
+
+  Status = SdMmcHcInitHost (Private->PciIo, Slot, Private->Capability[Slot]);
+  if (EFI_ERROR (Status)) {
+return Status;
+  }
+
+  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
+Status = mOverride->NotifyPhase (
+  >PassThru,
+  Private->ControllerHandle,
+  Slot,
+  EdkiiSdMmcInitHostPost);
+if (EFI_ERROR (Status)) {
+  DEBUG ((DEBUG_WARN,
+"%a: SD/MMC post init notifier callback failed - %r\n",
+__FUNCTION__, Status));
+}
+  }
+  return Status;
+}
+
+/**
Sd removable device enumeration callback function when the timer event is 
signaled.
  
@param[in]  Event The Event this notify function registered to.

@@ -281,14 +381,14 @@ SdMmcPciHcEnumerateDevice (
  //
  // Reset the specified slot of the SD/MMC Pci Host Controller
  //
-Status

Re: [edk2] [PATCH v3 1/2] MdeModulePkg: introduce SD/MMC override protocol

2017-12-05 Thread Zeng, Star

It should be not needed for Capability, but may be needed for NotifyPhase.
Hao can explain more.


Thanks,
Star
-Original Message-
From: Ni, Ruiyu 
Sent: Wednesday, December 6, 2017 11:22 AM
To: Ard Biesheuvel ; edk2-devel@lists.01.org
Cc: leif.lindh...@linaro.org; Kinney, Michael D ; 
Zeng, Star ; Tian, Feng ; Wu, Hao A 

Subject: Re: [PATCH v3 1/2] MdeModulePkg: introduce SD/MMC override protocol

On 12/6/2017 2:01 AM, Ard Biesheuvel wrote:
> Many ARM based SoCs have integrated SDHCI controllers, and often, 
> these implementations deviate in subtle ways from the pertinent 
> specifications. On the one hand, these deviations are quite easy to 
> work around, but on the other hand, having a collection of SoC 
> specific workarounds in the generic driver stack is undesirable.
> 
> So let's introduce an optional SD/MMC override protocol that we can 
> invoke at the appropriate moments in the device initialization.
> That way, the workaround itself remains platform specific, but we can 
> still use the generic driver stack on such platforms.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel 
> ---
>   MdeModulePkg/Include/Protocol/SdMmcOverride.h | 103 
>   MdeModulePkg/MdeModulePkg.dec |   3 +
>   2 files changed, 106 insertions(+)
> 
> diff --git a/MdeModulePkg/Include/Protocol/SdMmcOverride.h 
> b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> new file mode 100644
> index ..af57988f5625
> --- /dev/null
> +++ b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> @@ -0,0 +1,103 @@
> +/** @file
> +  Protocol to describe overrides required to support non-standard 
> +SDHCI
> +  implementations
> +
> +  Copyright (c) 2017, Linaro, Ltd. All rights reserved.
> +
> +  This program and the accompanying materials  are licensed and made 
> + available under the terms and conditions of the BSD License  which 
> + accompanies this distribution.  The full text of the license may be 
> + found at  http://opensource.org/licenses/bsd-license.php
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" 
> + BASIS,  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS 
> OR IMPLIED.
> +
> +**/
> +
> +#ifndef __SD_MMC_OVERRIDE_H__
> +#define __SD_MMC_OVERRIDE_H__
> +
> +#include 
> +
> +#define EDKII_SD_MMC_OVERRIDE_PROTOCOL_GUID \
> +  { 0xeaf9e3c1, 0xc9cd, 0x46db, { 0xa5, 0xe5, 0x5a, 0x12, 0x4c, 0x83, 
> +0x23, 0x23 } }
> +
> +#define EDKII_SD_MMC_OVERRIDE_PROTOCOL_VERSION0x1
> +
> +typedef struct _EDKII_SD_MMC_OVERRIDE EDKII_SD_MMC_OVERRIDE;
> +
> +typedef enum {
> +  EdkiiSdMmcResetPre,
> +  EdkiiSdMmcResetPost,
> +  EdkiiSdMmcInitHostPre,
> +  EdkiiSdMmcInitHostPost,
> +} EDKII_SD_MMC_PHASE_TYPE;
> +
> +/**
> +
> +  Override function for SDHCI capability bits
> +
> +  @param[in]  PassThru  A pointer to the
> +EFI_SD_MMC_PASS_THRU_PROTOCOL 
> instance.
> +  @param[in]  ControllerHandle  The EFI_HANDLE of the controller.
> +  @param[in]  Slot  The 0 based slot index.
> +  @param[in,out]  SdMmcHcSlotCapability The SDHCI capability structure.
> +
> +  @retval EFI_SUCCESS   The override function completed successfully.
> +  @retval EFI_NOT_FOUND The specified controller or slot does not 
> exist.
> +  @retval EFI_INVALID_PARAMETER SdMmcHcSlotCapability is NULL
> +
> +**/
> +typedef
> +EFI_STATUS
> +(EFIAPI * EDKII_SD_MMC_CAPABILITY) (
> +  IN  EFI_SD_MMC_PASS_THRU_PROTOCOL   *PassThru,
By looking a bit deeper, I get confused about this parameter.
SdMmcOverride protocol is supposed to be used by SdMmcHostController driver to 
produce SdMmcPassthru protocol.
But how does SdMmcOverride uses the not-yet-produced SdMmcPassthru protocol? 
It's like a chicken-egg problem.

> +  IN  EFI_HANDLE  ControllerHandle,
> +  IN  UINT8   Slot,
> +  IN  OUT UINT64  *SdMmcHcSlotCapability
> +  );
> +
> +/**
> +
> +  Override function for SDHCI controller operations
> +
> +  @param[in]  PassThru  A pointer to the
> +EFI_SD_MMC_PASS_THRU_PROTOCOL 
> instance.
> +  @param[in]  ControllerHandle  The EFI_HANDLE of the controller.
> +  @param[in]  Slot  The 0 based slot index.
> +  @param[in,out]  PhaseType The type of operation and whether the
> +hook is invoked right before (pre) or
> +right after (post)
> +
> +  @retval EFI_SUCCESS   The override function completed successfully.
> +  @retval EFI_NOT_FOUND The specified controller or slot does not 
> exist.
> +  @retval EFI_INVALID_PARAMETER HookType is invalid
> +

Re: [edk2] [PATCH v3 1/2] MdeModulePkg: introduce SD/MMC override protocol

2017-12-05 Thread Ni, Ruiyu


On 12/6/2017 2:01 AM, Ard Biesheuvel wrote:

Many ARM based SoCs have integrated SDHCI controllers, and often,
these implementations deviate in subtle ways from the pertinent
specifications. On the one hand, these deviations are quite easy
to work around, but on the other hand, having a collection of SoC
specific workarounds in the generic driver stack is undesirable.

So let's introduce an optional SD/MMC override protocol that we
can invoke at the appropriate moments in the device initialization.
That way, the workaround itself remains platform specific, but we
can still use the generic driver stack on such platforms.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel 
---
  MdeModulePkg/Include/Protocol/SdMmcOverride.h | 103 
  MdeModulePkg/MdeModulePkg.dec |   3 +
  2 files changed, 106 insertions(+)

diff --git a/MdeModulePkg/Include/Protocol/SdMmcOverride.h 
b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
new file mode 100644
index ..af57988f5625
--- /dev/null
+++ b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
@@ -0,0 +1,103 @@
+/** @file
+  Protocol to describe overrides required to support non-standard SDHCI
+  implementations
+
+  Copyright (c) 2017, Linaro, Ltd. All rights reserved.
+
+  This program and the accompanying materials
+  are licensed and made available under the terms and conditions of the BSD 
License
+  which accompanies this distribution.  The full text of the license may be 
found at
+  http://opensource.org/licenses/bsd-license.php
+
+  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#ifndef __SD_MMC_OVERRIDE_H__
+#define __SD_MMC_OVERRIDE_H__
+
+#include 
+
+#define EDKII_SD_MMC_OVERRIDE_PROTOCOL_GUID \
+  { 0xeaf9e3c1, 0xc9cd, 0x46db, { 0xa5, 0xe5, 0x5a, 0x12, 0x4c, 0x83, 0x23, 
0x23 } }
+
+#define EDKII_SD_MMC_OVERRIDE_PROTOCOL_VERSION0x1
+
+typedef struct _EDKII_SD_MMC_OVERRIDE EDKII_SD_MMC_OVERRIDE;
+
+typedef enum {
+  EdkiiSdMmcResetPre,
+  EdkiiSdMmcResetPost,
+  EdkiiSdMmcInitHostPre,
+  EdkiiSdMmcInitHostPost,
+} EDKII_SD_MMC_PHASE_TYPE;
+
+/**
+
+  Override function for SDHCI capability bits
+
+  @param[in]  PassThru  A pointer to the
+EFI_SD_MMC_PASS_THRU_PROTOCOL instance.
+  @param[in]  ControllerHandle  The EFI_HANDLE of the controller.
+  @param[in]  Slot  The 0 based slot index.
+  @param[in,out]  SdMmcHcSlotCapability The SDHCI capability structure.
+
+  @retval EFI_SUCCESS   The override function completed successfully.
+  @retval EFI_NOT_FOUND The specified controller or slot does not 
exist.
+  @retval EFI_INVALID_PARAMETER SdMmcHcSlotCapability is NULL
+
+**/
+typedef
+EFI_STATUS
+(EFIAPI * EDKII_SD_MMC_CAPABILITY) (
+  IN  EFI_SD_MMC_PASS_THRU_PROTOCOL   *PassThru,

By looking a bit deeper, I get confused about this parameter.
SdMmcOverride protocol is supposed to be used by SdMmcHostController
driver to produce SdMmcPassthru protocol.
But how does SdMmcOverride uses the not-yet-produced SdMmcPassthru 
protocol? It's like a chicken-egg problem.



+  IN  EFI_HANDLE  ControllerHandle,
+  IN  UINT8   Slot,
+  IN  OUT UINT64  *SdMmcHcSlotCapability
+  );
+
+/**
+
+  Override function for SDHCI controller operations
+
+  @param[in]  PassThru  A pointer to the
+EFI_SD_MMC_PASS_THRU_PROTOCOL instance.
+  @param[in]  ControllerHandle  The EFI_HANDLE of the controller.
+  @param[in]  Slot  The 0 based slot index.
+  @param[in,out]  PhaseType The type of operation and whether the
+hook is invoked right before (pre) or
+right after (post)
+
+  @retval EFI_SUCCESS   The override function completed successfully.
+  @retval EFI_NOT_FOUND The specified controller or slot does not 
exist.
+  @retval EFI_INVALID_PARAMETER HookType is invalid
+
+**/
+typedef
+EFI_STATUS
+(EFIAPI * EDKII_SD_MMC_NOTIFY_PHASE) (
+  IN  EFI_SD_MMC_PASS_THRU_PROTOCOL   *PassThru,
+  IN  EFI_HANDLE  ControllerHandle,
+  IN  UINT8   Slot,
+  IN  EDKII_SD_MMC_PHASE_TYPE PhaseType
+  );
+
+struct _EDKII_SD_MMC_OVERRIDE {
+  //
+  // Protocol version of this implementation
+  //
+  UINTN Version;
+  //
+  // Callback to override SD/MMC host controller capability bits
+  //
+  EDKII_SD_MMC_CAPABILITY   Capability;
+  //
+  // Callback to invoke SD/MMC override hooks
+  //
+  EDKII_SD_MMC_NOTIFY_PHASE NotifyPhase;
+};
+
+extern EFI_GUID gEdkiiSdMmcOverrideProtocolGuid;
+
+#endif
diff --git

Re: [edk2] [Patch] BaseTools: Add object_files.lst as dependency of lib target

2017-12-05 Thread Gao, Liming

Reviewed-by: Liming Gao 

>-Original Message-
>From: Zhu, Yonghong
>Sent: Wednesday, December 06, 2017 9:52 AM
>To: edk2-devel@lists.01.org
>Cc: Gao, Liming 
>Subject: [Patch] BaseTools: Add object_files.lst as dependency of lib target
>
>Seems object_files.lst is not added as dependency of lib target, this
>patch update BaseTools to generate Makefile with this dependency.
>
>Cc: Liming Gao 
>Contributed-under: TianoCore Contribution Agreement 1.1
>Signed-off-by: Yonghong Zhu 
>---
> BaseTools/Source/Python/AutoGen/GenMake.py | 2 ++
> 1 file changed, 2 insertions(+)
>
>diff --git a/BaseTools/Source/Python/AutoGen/GenMake.py
>b/BaseTools/Source/Python/AutoGen/GenMake.py
>index 0f3ddd5..410ad59 100644
>--- a/BaseTools/Source/Python/AutoGen/GenMake.py
>+++ b/BaseTools/Source/Python/AutoGen/GenMake.py
>@@ -862,10 +862,12 @@ cleanlib:
> Deps.append(NewFile)
>
> # Use file list macro as dependency
> if T.GenFileListMacro:
> Deps.append("$(%s)" % T.FileListMacro)
>+if Type in [TAB_OBJECT_FILE, TAB_STATIC_LIBRARY]:
>+Deps.append("$(%s)" % T.ListFileMacro)
>
> TargetDict = {
> "target":   self.PlaceMacro(T.Target.Path, 
> self.Macros),
> "cmd"   :   "\n\t".join(T.Commands),
> "deps"  :   Deps
>--
>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 v3 0/2] Enable page table write protection

2017-12-05 Thread Wang, Jian J

Hi Laszlo,

Thanks for the test.

Jian
> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Wednesday, December 06, 2017 4:05 AM
> To: Wang, Jian J ; edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH v3 0/2] Enable page table write protection
> 
> On 12/05/17 09:16, Jian J Wang wrote:
> >> v3 changes:
> >>  a. According to code review comments, remove the public definitions of
> >> page table pool. Now the DxeIpl and CpuDxe will have their own page
> >> table pool but in the same mechanism. Related PCDs, GUDI and headers
> >> are also removed.
> >>  b. Apply protection to all page tables, including new ones added in
> >> CpuDxe driver.
> >>  c. Code/comments cleanup.
> >
> >> v2 changes:
> >>  a. Enable protection on any newly added page table after DxeIpl.
> >>  b. Introduce page table pool concept to make page table allocation
> >> and protection easier and error free.
> >
> > Write Protect feature (CR0.WP) is always enabled in driver
> UefiCpuPkg/CpuDxe.
> > But the memory pages used for page table are not set as read-only in the
> driver
> > DxeIplPeim, after the paging is setup. This might jeopardize the page table
> > integrity if there's buffer overflow occured in other part of system.
> >
> > This patch series will change this situation by clearing R/W bit in page 
> > attribute
> > of the pages used as page table.
> >
> > Validation works include booting Windows (10/server 2016) and Linux
> (Fedora/Ubuntu)
> > on OVMF and Intel real platform.
> >
> > Jian J Wang (2):
> >   MdeModulePkg/DxeIpl: Mark page table as read-only
> >   UefiCpuPkg/CpuDxe: Enable protection for newly added page table
> >
> >  MdeModulePkg/Core/DxeIplPeim/DxeIpl.h|  34 +++
> >  MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  |   8 +-
> >  MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 301
> ++-
> >  MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h |  26 ++
> >  UefiCpuPkg/CpuDxe/CpuDxe.c   |  17 +-
> >  UefiCpuPkg/CpuDxe/CpuDxe.h   |   2 +
> >  UefiCpuPkg/CpuDxe/CpuPageTable.c | 226 -
> >  UefiCpuPkg/CpuDxe/CpuPageTable.h |  34 +++
> >  8 files changed, 635 insertions(+), 13 deletions(-)
> >
> 
> series
> Regression-tested-by: Laszlo Ersek 
> 
> Thanks
> Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 02/19] ArmPlatformPkg: Tidy LcdGraphicsOutputDxe code: Added comments

2017-12-05 Thread Evan Lloyd



> -Original Message-
> From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
> Sent: 05 December 2017 19:59
> To: Evan Lloyd 
> Cc: edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH 02/19] ArmPlatformPkg: Tidy
> LcdGraphicsOutputDxe code: Added comments
>
> On Tue, Dec 05, 2017 at 06:55:25PM +, Evan Lloyd wrote:
> >
> >
> > > -Original Message-
> > > From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
> > > Sent: 12 October 2017 20:02
> > > To: Evan Lloyd 
> > > Cc: edk2-devel@lists.01.org
> > > Subject: Re: [edk2] [PATCH 02/19] ArmPlatformPkg: Tidy
> > > LcdGraphicsOutputDxe code: Added comments
> > >
> > > Given that all changes to the first file _remove_ comments, it may
> > > be better with a subject line saying "updating comments".
> > >
> > > On Tue, Sep 26, 2017 at 09:15:12PM +0100, evan.ll...@arm.com
> wrote:
> > > > From: Girish Pathak 
> > > >
> > > > There is no functional modification in this change As preparation
> > > > for a Change (Rejig of LcdGraphicsOutPutDxe), some comments are
> > > > modified and a few new comments are added.
> > > > This is to prevent mixing formatting changes with functional changes.
> > > >
> > > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > > Signed-off-by: Girish Pathak 
> > > > Signed-off-by: Evan Lloyd 
> > > > ---
> > ...
> > > >
> > > > -
> > > > +/** Platform related initialization function.
> > > > +  *
> > > > +  * @param IN Handle   Handle to the LCD device instance.
> > > > +  *
> > > > +  * @retval EFI_SUCCESSPlatform initialization success.
> > > > +  * @retval !(EFI_SUCCESS) Other errors.
> > > > +**/
> > >
> > > So ... 6.8 lists
> > > /**
> > >   text
> > > **/
> > > as the
> > >
> > > The format
> > > /**
> > >  * text
> > > **/
> > > is mentioned as "also legal because doxygen ignores the leading *".
> > >
> > > The format
> > > /**
> > >   *
> > > **/
> > > is never mentioned, although I guess "also legal" because * ignored.
> > >
> > > However, a quick skim in MdePkg suggests the former is the generally
> > > used variant. Can you please update to that format throughout (drop
> > > the leading '*' on lines not starting or ending the comment block)?
> >
> >  [[Evan Lloyd]] I'm not sure if Outlook has mangled something, or I'm
> > being obtuse, but I'm not sure I follow the distinction you are making
> there.
> > However, if your objection is to the leading '*' then we can remove
> > it.
>
> The objection is slightly with regards to the leading *, but moreso over it
> aligning with the second * of the opening /** rather than the first.

[[Evan Lloyd]] Just to be a barrack room lawyer - the CCS clearly states:
" 3.2.1 Formatting: General Rules
...
 All indentation (tabs) is two spaces. See "General Rules”."
" 5.1.2 ... All indentation is on two space boundaries. There are no exceptions 
to this rule!"

Of course you may quibble over this being an indentation, but the leading '*'s 
are certainly indented relative to the "/**" in either case.

>
> It is entirely possible that some form of email mangling is the cause
> (including perhaps you reading my reply in non-fixed width font).
[[Evan Lloyd]] Almost certainly true.
>
> > By the way - shouldn't it be:
> > /**  Brief description
> >
> >   Details
> > **/  (see Horor vacuii)
>
> That would be my preferred version. (I started typing that above, but seem
> to have lost my way after "as the".)
>
> It's just that
> /**
>  *
>  **/
> is common enough in the codebase that I wouldn't object to it.
>
> Whereas I haven't seen
> /**
>   *
>  **/
> anywhere else

 [[Evan Lloyd]] I refer my learned friend to  "CCS 5.1.2 ... All indentation is 
on two space boundaries. There are no exceptions to this rule!"
Also:
"1.1 Abstract
... All changes or additions from this point on shall conform to this 
specification. Pre-existing code does not need to be updated for the sole 
purpose of conforming to this specification. As conforming updates are made, 
the developer may update other content within the modified file to bring it 
within compliance with this specification."

Being serious: since we all seem to prefer no leading '*'s the above is not 
really relevant - we'll go with a match of the file header format (and 2 space 
indents for the body text).

>
> > I actually think the CCS is woefully inconsistent in its example
> > comment style, and that although leading '*'s are acceptable to
> > Doxygen, it would be better to stick to one style (that of the file
> > header comment, without leading '*'s) throughout.
>
> I won't argue about the consistency, and agree with your view on this.
>
> /
> Leif
>
> >
> > >
> > > No other comments (other than having these prototype
> documentations
> > > are a great improvement).
> > >
> > > /
> > > Leif
> > >
> > > >  EFI_STATUS
> > > >  LcdPlatformInitializeDisplay (
> > > >IN

Re: [edk2] [PATCH 19/19] ArmPlatformPkg: New DP500/DP550/DP650 GOP driver.

2017-12-05 Thread Ard Biesheuvel

On 5 December 2017 at 20:03, Evan Lloyd  wrote:
>
>
>> -Original Message-
>> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
>> Sent: 01 December 2017 17:19
>> To: Evan Lloyd 
>> Cc: edk2-devel@lists.01.org; Matteo Carlini ;
>> Leif Lindholm ; Girish Pathak
>> 
>> Subject: Re: [PATCH 19/19] ArmPlatformPkg: New DP500/DP550/DP650
>> GOP driver.
>>
>> On 1 December 2017 at 13:12, Evan Lloyd  wrote:
>> >
>> >
>> >> -Original Message-
>> >> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
>> >> Sent: 28 November 2017 18:18
>> >> To: Evan Lloyd 
>> >> Cc: edk2-devel@lists.01.org; Matteo Carlini ;
>> >> Leif Lindholm ; Girish Pathak
>> >> 
>> >> Subject: Re: [PATCH 19/19] ArmPlatformPkg: New DP500/DP550/DP650
>> GOP
>> >> driver.
>> >>
>> >> On 26 September 2017 at 21:15,   wrote:
>> >> > From: Girish Pathak 
>> >> >
>> >> > This change adds support for the ARM Mali DP500/DP500/DP650
>> display
>> >> > processors using the GOP protocol. It has been tested on FVP base
>> >> > models + DP550 support.
>> >> >
>> >> > This change does not modify functionality provided by PL111 or
>> >> > HDLCD. The driver should be suitable for those platforms that
>> >> > implement ARM Mali DP500/DP550/DP650 replacing PL111/HDLCD.
>> >> >
>> >> > Only "Layer Graphics" of the ARM Mali DP is configured for
>> >> > rendering the RGB/BGR format frame buffer to satisfy the UEFI GOP
>> >> > requirements Other layers e.g. video layers are not configured.
>> >> >
>> >> > NOTE: This change implements the Mali DP on models. Versions for
>> >> > actual hardware are liable to require extra handling for clock
>> >> > input changes, etc.
>> >> >
>> >> > Contributed-under: TianoCore Contribution Agreement 1.1
>> >> > Signed-off-by: Girish Pathak 
>> >> > Signed-off-by: Evan Lloyd 
>> >>
>> >> Hello Girish, Evan,
>> >>
>> >> (replying to 19/19 because I cannot find the cover letter in my
>> >> edk2-devel archive but this really applies to the whole series)
>> >>
>> >> I have been looking at these patches again now that I am trying to
>> >> clean up ArmPlatformPkg, which is currently a dumping ground for all
>> >> things vaguely ARM related, and is also structured quite differently
>> >> from other packages.
>> >>
>> >> Ideally, ArmPlatformPkg should only contain the following:
>> >> - library class interfaces under Include/Library; header files kept
>> >> here should only contain elements that define API
>> >> - driver specific include files Include/IndustryStandard but *only*
>> >> if they cannot be kept locally with the driver in question
>> >> - libraries under Library/
>> >> - drivers under Drivers/
>> >>
>> >> This aligns with the common arrangement adopted by most EDK2
>> packages.
>> >>
>> >> This series does many different things, and does not distinguish at
>> >> all between common code and code living under ArmVExpressPkg. Given
>> >
>> >  [[Evan Lloyd]] All of the commits in the series are in ArmPlatformPkg.
>> > You may be in the process of disentangling the VE specific bits, but
>> > hitherto that has not been a consideration.  (Note: I'm not arguing against
>> the disentangling, only pointing out that it was not a factor at the point we
>> submitted the patches) The reason there are so many commits is only that
>> we have been asked to break things up into "bite sized" chunks for the
>> convenience of maintainers.
>> > The aim was to make each a coherent item with a simple justification.
>> >
>> >> that I am trying to move ArmVExpressPkg out of EDK2 into
>> >> edk2-platforms (where it arguably belongs) having this series in
>> >> limbo for two months is basically blocking my work, and so I would
>> >> like to explore ways to proceed with this without interfering with
>> >> each other's work too much. At the same time, the way the code is
>> >> structured is a continuation of the pattern I am trying to get rid
>> >> of, so they will need some rework anyway in order to be upstreamable
>> IMHO.
>> >
>> >  [[Evan Lloyd]] Not being psychic, we had not made allowance for your
>> plans.
>> > However, if you take the trouble to look at the changes, they achieve
>> exactly the split you aim for.
>> > The display type code (PL011 and HDLCD) is unravelled from the VE code.
>> > All that remains would be to move the VE specific part into edk2-
>> platforms.
>> >
>> >>
>> >> So could we split it up please? Assuming the intention is the ability
>> >> to reuse the Mali code on non-VExpress platforms, I would like to see
>> >> that code proposed separately, without any mention of VExpressPkg.dec
>> >
>> >  [[Evan Lloyd]] Given that the original code was unfortunate, I'm not sure
>> that it makes much difference

Re: [edk2] [PATCH 03/19] ArmPlatformPkg: PL111 and HDLCD: add const qualifier

2017-12-05 Thread Ard Biesheuvel

On 5 December 2017 at 20:35, Evan Lloyd  wrote:
>
>
>> -Original Message-
>> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
>> Sent: 01 December 2017 17:32
>> To: Evan Lloyd 
>> Cc: edk2-devel@lists.01.org; ard.biesheu...@linaro.org@arm.com
>> <"ard.biesheu...@linaro.org"@arm.com>;
>> leif.lindh...@linaro.org@arm.com <"leif.lindh...@linaro.org"@arm.com>;
>> matteo.carl...@arm.com@arm.com
>> <"matteo.carl...@arm.com"@arm.com>; n...@arm.com@arm.com
>> <"n...@arm.com"@arm.com>
>> Subject: Re: [PATCH 03/19] ArmPlatformPkg: PL111 and HDLCD: add const
>> qualifier
>>
>> On 1 December 2017 at 16:17, Evan Lloyd  wrote:
>> > Hi Ard.
>> > Response inline below
>> >
>> >> -Original Message-
>> >> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
>> >> Sent: 12 October 2017 20:47
>> >> To: Evan Lloyd 
>> >> Cc: edk2-devel@lists.01.org; "ard.biesheu...@linaro.org"@arm.com;
>> >> "leif.lindh...@linaro.org"@arm.com;
>> >> "matteo.carl...@arm.com"@arm.com; "n...@arm.com"@arm.com
>> >> Subject: Re: [PATCH 03/19] ArmPlatformPkg: PL111 and HDLCD: add
>> const
>> >> qualifier
>> >>
>> >> On 26 September 2017 at 21:15,   wrote:
>> >> > From: Girish Pathak 
>> >> >
>> >> > This change adds some STATIC and CONST qualifiers (mainly to
>> >> > arguments of  functions) in PL111 and HdLcd modules.
>> >> >
>> >> > It doesn't add or modify any functionality.
>> >> >
>> >> > Contributed-under: TianoCore Contribution Agreement 1.1
>> >> > Signed-off-by: Girish Pathak 
>> >> > Signed-off-by: Evan Lloyd 
>> >> > ---
>> >> >
>> >>
>> ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdAr
>> >> mVExpress.c   | 34 ++--
>> >> >
>> >>
>> ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL11
>> >> 1LcdArmVExpress.c | 34 ++--
>> >> >  ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c
>> >> |  4 +--
>> >> >  ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c
>> >> |  4 +--
>> >> >  4 files changed, 38 insertions(+), 38 deletions(-)
>> >> >
>> >> > diff --git
>> >> >
>> >>
>> a/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcd
>> >> ArmVE
>> >> > xpress.c
>> >> >
>> >>
>> b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcd
>> >> ArmVE
>> >> > xpress.c index
>> >> >
>> >>
>> cfe3259d3c737de240350e8c3eab867b80c40948..b9859a56988f7e5be0ad
>> >> baa49048
>> >> > a683fe586bfe 100644
>> >> > ---
>> >> >
>> >>
>> a/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcd
>> >> ArmVE
>> >> > xpress.c
>> >> > +++
>> >>
>> b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcd
>> >> A
>> >> > +++ rmVExpress.c
>> >> > @@ -46,7 +46,7 @@ typedef struct {
>> >> >
>> >> >  /** The display modes supported by the platform.
>> >> >  **/
>> >> > -LCD_RESOLUTION mResolutions[] = {
>> >> > +STATIC CONST LCD_RESOLUTION mResolutions[] = {
>> >> >{ // Mode 0 : VGA : 640 x 480 x 24 bpp
>> >> >  VGA, VGA_H_RES_PIXELS, VGA_V_RES_PIXELS,
>> >> LCD_BITS_PER_PIXEL_24,
>> >> >  VGA_OSC_FREQUENCY,
>> >> > @@ -144,8 +144,8 @@ LcdPlatformInitializeDisplay (  **/  EFI_STATUS
>> >> > LcdPlatformGetVram (
>> >> > -  OUT EFI_PHYSICAL_ADDRESS*  VramBaseAddress,
>> >> > -  OUT UINTN* VramSize
>> >> > +  OUT EFI_PHYSICAL_ADDRESS * CONST  VramBaseAddress,
>> >> > +  OUT UINTN * CONST VramSize
>> >>
>> >> What is the point of this CONST (and all the other occurrences in
>> >> this patch)
>> >>
>> >> In all cases [AFAICT] the CONST applies to the argument itself, not
>> >> to the object it points to, which means the variable is CONST in the
>> >> scope of the function, but can still be dereferenced to assign the OUT
>> value.
>> >>
>> >> This means your change is technically correct, but it is extremely
>> >> unidiomatic for EDK2, so an explanation why this driver needs this
>> >> would be highly appreciated.
>> >>
>> > [[Evan Lloyd]] The style is explicitly sanctioned by the Edk2 CCS §
>> > 5.6.2.4.2 " Constant pointer to variable: UINTN * CONST ConstPointer;
>> > ConstPointer is a constant pointer to a variable UINTN."
>> >
>>
>> That paragraph is not about function prototypes, but about constant
>> pointers in general.
>>
>> > The real benefit is that it clearly identifies the pointer as not changed 
>> > in
>> the function.
>> > In this specific instance that also makes it obvious that the OUT
>> parameters are not array bases, just pointers to individual values.
>> >
>> > On a broader note - why would you ever not have a const where
>> something is not modified?
>> >
>> > As another view, the "unidiomatic for EDK2" argument implies you have a
>> very high opinion of the existing ArmPlatformPkg code quality.
>> > The  "we have always done it that way" argument does not encourage
>> quality improvements.
>> >
>>
>> This may all be true.

Re: [edk2] [PATCH 04/19] ArmPlatformPkg: LcdGraphicsOurputDxe: Add debug asserts

2017-12-05 Thread Evan Lloyd



> -Original Message-
> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> Sent: 01 December 2017 17:35
> To: Evan Lloyd 
> Cc: edk2-devel@lists.01.org; ard.biesheu...@linaro.org@arm.com
> <"ard.biesheu...@linaro.org"@arm.com>;
> leif.lindh...@linaro.org@arm.com <"leif.lindh...@linaro.org"@arm.com>;
> matteo.carl...@arm.com@arm.com
> <"matteo.carl...@arm.com"@arm.com>; n...@arm.com@arm.com
> <"n...@arm.com"@arm.com>
> Subject: Re: [PATCH 04/19] ArmPlatformPkg: LcdGraphicsOurputDxe: Add
> debug asserts
>
> On 1 December 2017 at 16:33, Evan Lloyd  wrote:
> > Responses inline:
> >
> >> -Original Message-
> >> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> >> Sent: 13 October 2017 08:33
> >> To: Evan Lloyd 
> >> Cc: edk2-devel@lists.01.org; "ard.biesheu...@linaro.org"@arm.com;
> >> "leif.lindh...@linaro.org"@arm.com;
> >> "matteo.carl...@arm.com"@arm.com; "n...@arm.com"@arm.com
> >> Subject: Re: [PATCH 04/19] ArmPlatformPkg: LcdGraphicsOurputDxe:
> Add
> >> debug asserts
> >>
> >> On 26 September 2017 at 21:15,   wrote:
> >> > From: Girish Pathak 
> >> >
> >> > This change adds some debug assertions e.g to catch NULL pointer
> >> > errors missing in PL11Lcd and HdLcd modules.
> >> >
> >> > This change also improves related error handling code.
> >> >
> >> > Contributed-under: TianoCore Contribution Agreement 1.1
> >> > Signed-off-by: Girish Pathak 
> >> > Signed-off-by: Evan Lloyd 
> >> > ---
> >> >
> >>
> ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdAr
> >> mVExpress.c   | 44 ++--
> >> >
> >>
> ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL11
> >> 1LcdArmVExpress.c | 43 ++-
> >> >  ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c
> >> |  8 ++--
> >> >  ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c
> >> |  8 ++--
> >> >  4 files changed, 90 insertions(+), 13 deletions(-)
> >> >
> >> > diff --git
> >> >
> >>
> a/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcd
> >> ArmVE
> >> > xpress.c
> >> >
> >>
> b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcd
> >> ArmVE
> >> > xpress.c index
> >> >
> >>
> b9859a56988f7e5be0adbaa49048a683fe586bfe..58dd9f0c77e1bc9af559a
> >> 71d0c7c
> >> > ce72d71c6da5 100644
> >> > ---
> >> >
> >>
> a/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcd
> >> ArmVE
> >> > xpress.c
> >> > +++
> >>
> b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcd
> >> A
> >> > +++ rmVExpress.c
> >> > @@ -140,6 +140,7 @@ LcdPlatformInitializeDisplay (
> >> >* buffer in bytes
> >> >*
> >> >* @retval EFI_SUCCESS Frame buffer memory allocation
> success.
> >> > +  * @retval EFI_INVALID_PARAMETER   VramBaseAddress or
> VramSize
> >> are NULL.
> >> >* @retval !(EFI_SUCCESS)  Other errors.
> >> >  **/
> >> >  EFI_STATUS
> >> > @@ -151,6 +152,13 @@ LcdPlatformGetVram (
> >> >EFI_STATUS  Status;
> >> >EFI_ALLOCATE_TYPE   AllocationType;
> >> >
> >> > +  // Check VramBaseAddress and VramSize are not NULL.
> >> > +  if (VramBaseAddress == NULL || VramSize == NULL) {
> >> > +ASSERT (VramBaseAddress != NULL);
> >> > +ASSERT (VramSize != NULL);
> >> > +return EFI_INVALID_PARAMETER;
> >> > +  }
> >> > +
> >> >// Set the vram size
> >> >*VramSize = LCD_VRAM_SIZE;
> >> >
> >> > @@ -169,6 +177,7 @@ LcdPlatformGetVram (
> >> >VramBaseAddress
> >> >);
> >> >if (EFI_ERROR (Status)) {
> >> > +ASSERT_EFI_ERROR (Status);
> >> >  return Status;
> >> >}
> >> >
> >> > @@ -179,8 +188,8 @@ LcdPlatformGetVram (
> >> >*VramSize,
> >> >EFI_MEMORY_WC
> >> >);
> >> > -  ASSERT_EFI_ERROR (Status);
> >> >if (EFI_ERROR (Status)) {
> >> > +ASSERT_EFI_ERROR (Status);
> >>
> >> What is the point of this change?
> > [[Evan Lloyd]] It is a minor efficiency improvement.  Since the ASSERT can
> only fire when the if condition is true, it removes a duplicated test from the
> main (non-error) code flow.  This is irrelevant on hardware, but actually
> significant when running debug builds on an emulator environment.
> >
>
> Fair enough. But I'd prefer to finally fix the DEBUG vs NOOPT build targets
> for ARM/AARCH64. DEBUG was never intended to be -O0 (and it is not on
> X86 either).
>
> Code is complex enough as it is, and how to move trivial tests like this
> around for 'performance' reasons is a dimension I would like to avoid.

 [[Evan Lloyd]] I have no objection to the DEBUG build being updated from -O0.
I strongly suspect that it was set to that because it reduced the incidence of 
the r18 problem that has now been fixed.
However, I can certainly envisage circumstances when we might need to do an

Re: [edk2] [PATCH 03/19] ArmPlatformPkg: PL111 and HDLCD: add const qualifier

2017-12-05 Thread Evan Lloyd



> -Original Message-
> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> Sent: 01 December 2017 17:32
> To: Evan Lloyd 
> Cc: edk2-devel@lists.01.org; ard.biesheu...@linaro.org@arm.com
> <"ard.biesheu...@linaro.org"@arm.com>;
> leif.lindh...@linaro.org@arm.com <"leif.lindh...@linaro.org"@arm.com>;
> matteo.carl...@arm.com@arm.com
> <"matteo.carl...@arm.com"@arm.com>; n...@arm.com@arm.com
> <"n...@arm.com"@arm.com>
> Subject: Re: [PATCH 03/19] ArmPlatformPkg: PL111 and HDLCD: add const
> qualifier
>
> On 1 December 2017 at 16:17, Evan Lloyd  wrote:
> > Hi Ard.
> > Response inline below
> >
> >> -Original Message-
> >> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> >> Sent: 12 October 2017 20:47
> >> To: Evan Lloyd 
> >> Cc: edk2-devel@lists.01.org; "ard.biesheu...@linaro.org"@arm.com;
> >> "leif.lindh...@linaro.org"@arm.com;
> >> "matteo.carl...@arm.com"@arm.com; "n...@arm.com"@arm.com
> >> Subject: Re: [PATCH 03/19] ArmPlatformPkg: PL111 and HDLCD: add
> const
> >> qualifier
> >>
> >> On 26 September 2017 at 21:15,   wrote:
> >> > From: Girish Pathak 
> >> >
> >> > This change adds some STATIC and CONST qualifiers (mainly to
> >> > arguments of  functions) in PL111 and HdLcd modules.
> >> >
> >> > It doesn't add or modify any functionality.
> >> >
> >> > Contributed-under: TianoCore Contribution Agreement 1.1
> >> > Signed-off-by: Girish Pathak 
> >> > Signed-off-by: Evan Lloyd 
> >> > ---
> >> >
> >>
> ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcdAr
> >> mVExpress.c   | 34 ++--
> >> >
> >>
> ArmPlatformPkg/ArmVExpressPkg/Library/PL111LcdArmVExpressLib/PL11
> >> 1LcdArmVExpress.c | 34 ++--
> >> >  ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/HdLcd.c
> >> |  4 +--
> >> >  ArmPlatformPkg/Drivers/LcdGraphicsOutputDxe/PL111Lcd.c
> >> |  4 +--
> >> >  4 files changed, 38 insertions(+), 38 deletions(-)
> >> >
> >> > diff --git
> >> >
> >>
> a/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcd
> >> ArmVE
> >> > xpress.c
> >> >
> >>
> b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcd
> >> ArmVE
> >> > xpress.c index
> >> >
> >>
> cfe3259d3c737de240350e8c3eab867b80c40948..b9859a56988f7e5be0ad
> >> baa49048
> >> > a683fe586bfe 100644
> >> > ---
> >> >
> >>
> a/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcd
> >> ArmVE
> >> > xpress.c
> >> > +++
> >>
> b/ArmPlatformPkg/ArmVExpressPkg/Library/HdLcdArmVExpressLib/HdLcd
> >> A
> >> > +++ rmVExpress.c
> >> > @@ -46,7 +46,7 @@ typedef struct {
> >> >
> >> >  /** The display modes supported by the platform.
> >> >  **/
> >> > -LCD_RESOLUTION mResolutions[] = {
> >> > +STATIC CONST LCD_RESOLUTION mResolutions[] = {
> >> >{ // Mode 0 : VGA : 640 x 480 x 24 bpp
> >> >  VGA, VGA_H_RES_PIXELS, VGA_V_RES_PIXELS,
> >> LCD_BITS_PER_PIXEL_24,
> >> >  VGA_OSC_FREQUENCY,
> >> > @@ -144,8 +144,8 @@ LcdPlatformInitializeDisplay (  **/  EFI_STATUS
> >> > LcdPlatformGetVram (
> >> > -  OUT EFI_PHYSICAL_ADDRESS*  VramBaseAddress,
> >> > -  OUT UINTN* VramSize
> >> > +  OUT EFI_PHYSICAL_ADDRESS * CONST  VramBaseAddress,
> >> > +  OUT UINTN * CONST VramSize
> >>
> >> What is the point of this CONST (and all the other occurrences in
> >> this patch)
> >>
> >> In all cases [AFAICT] the CONST applies to the argument itself, not
> >> to the object it points to, which means the variable is CONST in the
> >> scope of the function, but can still be dereferenced to assign the OUT
> value.
> >>
> >> This means your change is technically correct, but it is extremely
> >> unidiomatic for EDK2, so an explanation why this driver needs this
> >> would be highly appreciated.
> >>
> > [[Evan Lloyd]] The style is explicitly sanctioned by the Edk2 CCS §
> > 5.6.2.4.2 " Constant pointer to variable: UINTN * CONST ConstPointer;
> > ConstPointer is a constant pointer to a variable UINTN."
> >
>
> That paragraph is not about function prototypes, but about constant
> pointers in general.
>
> > The real benefit is that it clearly identifies the pointer as not changed in
> the function.
> > In this specific instance that also makes it obvious that the OUT
> parameters are not array bases, just pointers to individual values.
> >
> > On a broader note - why would you ever not have a const where
> something is not modified?
> >
> > As another view, the "unidiomatic for EDK2" argument implies you have a
> very high opinion of the existing ArmPlatformPkg code quality.
> > The  "we have always done it that way" argument does not encourage
> quality improvements.
> >
>
> This may all be true. But the fact remains that 99% of the EDK2 code does
> not constify its function parameters, and I was simply asking why we should
> deviate from that in this driver.
[[Evan Lloyd]] And the simple answer is

Re: [edk2] [PATCH v3 0/2] Enable page table write protection

2017-12-05 Thread Laszlo Ersek

On 12/05/17 09:16, Jian J Wang wrote:
>> v3 changes:
>>  a. According to code review comments, remove the public definitions of
>> page table pool. Now the DxeIpl and CpuDxe will have their own page
>> table pool but in the same mechanism. Related PCDs, GUDI and headers
>> are also removed.
>>  b. Apply protection to all page tables, including new ones added in
>> CpuDxe driver.
>>  c. Code/comments cleanup.
> 
>> v2 changes:
>>  a. Enable protection on any newly added page table after DxeIpl.
>>  b. Introduce page table pool concept to make page table allocation
>> and protection easier and error free.
> 
> Write Protect feature (CR0.WP) is always enabled in driver UefiCpuPkg/CpuDxe.
> But the memory pages used for page table are not set as read-only in the 
> driver
> DxeIplPeim, after the paging is setup. This might jeopardize the page table
> integrity if there's buffer overflow occured in other part of system.
> 
> This patch series will change this situation by clearing R/W bit in page 
> attribute
> of the pages used as page table.
> 
> Validation works include booting Windows (10/server 2016) and Linux 
> (Fedora/Ubuntu)
> on OVMF and Intel real platform.
> 
> Jian J Wang (2):
>   MdeModulePkg/DxeIpl: Mark page table as read-only
>   UefiCpuPkg/CpuDxe: Enable protection for newly added page table
> 
>  MdeModulePkg/Core/DxeIplPeim/DxeIpl.h|  34 +++
>  MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  |   8 +-
>  MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 301 
> ++-
>  MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h |  26 ++
>  UefiCpuPkg/CpuDxe/CpuDxe.c   |  17 +-
>  UefiCpuPkg/CpuDxe/CpuDxe.h   |   2 +
>  UefiCpuPkg/CpuDxe/CpuPageTable.c | 226 -
>  UefiCpuPkg/CpuDxe/CpuPageTable.h |  34 +++
>  8 files changed, 635 insertions(+), 13 deletions(-)
> 

series
Regression-tested-by: Laszlo Ersek 

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

Re: [edk2] [PATCH 19/19] ArmPlatformPkg: New DP500/DP550/DP650 GOP driver.

2017-12-05 Thread Evan Lloyd



> -Original Message-
> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> Sent: 01 December 2017 17:19
> To: Evan Lloyd 
> Cc: edk2-devel@lists.01.org; Matteo Carlini ;
> Leif Lindholm ; Girish Pathak
> 
> Subject: Re: [PATCH 19/19] ArmPlatformPkg: New DP500/DP550/DP650
> GOP driver.
>
> On 1 December 2017 at 13:12, Evan Lloyd  wrote:
> >
> >
> >> -Original Message-
> >> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> >> Sent: 28 November 2017 18:18
> >> To: Evan Lloyd 
> >> Cc: edk2-devel@lists.01.org; Matteo Carlini ;
> >> Leif Lindholm ; Girish Pathak
> >> 
> >> Subject: Re: [PATCH 19/19] ArmPlatformPkg: New DP500/DP550/DP650
> GOP
> >> driver.
> >>
> >> On 26 September 2017 at 21:15,   wrote:
> >> > From: Girish Pathak 
> >> >
> >> > This change adds support for the ARM Mali DP500/DP500/DP650
> display
> >> > processors using the GOP protocol. It has been tested on FVP base
> >> > models + DP550 support.
> >> >
> >> > This change does not modify functionality provided by PL111 or
> >> > HDLCD. The driver should be suitable for those platforms that
> >> > implement ARM Mali DP500/DP550/DP650 replacing PL111/HDLCD.
> >> >
> >> > Only "Layer Graphics" of the ARM Mali DP is configured for
> >> > rendering the RGB/BGR format frame buffer to satisfy the UEFI GOP
> >> > requirements Other layers e.g. video layers are not configured.
> >> >
> >> > NOTE: This change implements the Mali DP on models. Versions for
> >> > actual hardware are liable to require extra handling for clock
> >> > input changes, etc.
> >> >
> >> > Contributed-under: TianoCore Contribution Agreement 1.1
> >> > Signed-off-by: Girish Pathak 
> >> > Signed-off-by: Evan Lloyd 
> >>
> >> Hello Girish, Evan,
> >>
> >> (replying to 19/19 because I cannot find the cover letter in my
> >> edk2-devel archive but this really applies to the whole series)
> >>
> >> I have been looking at these patches again now that I am trying to
> >> clean up ArmPlatformPkg, which is currently a dumping ground for all
> >> things vaguely ARM related, and is also structured quite differently
> >> from other packages.
> >>
> >> Ideally, ArmPlatformPkg should only contain the following:
> >> - library class interfaces under Include/Library; header files kept
> >> here should only contain elements that define API
> >> - driver specific include files Include/IndustryStandard but *only*
> >> if they cannot be kept locally with the driver in question
> >> - libraries under Library/
> >> - drivers under Drivers/
> >>
> >> This aligns with the common arrangement adopted by most EDK2
> packages.
> >>
> >> This series does many different things, and does not distinguish at
> >> all between common code and code living under ArmVExpressPkg. Given
> >
> >  [[Evan Lloyd]] All of the commits in the series are in ArmPlatformPkg.
> > You may be in the process of disentangling the VE specific bits, but
> > hitherto that has not been a consideration.  (Note: I'm not arguing against
> the disentangling, only pointing out that it was not a factor at the point we
> submitted the patches) The reason there are so many commits is only that
> we have been asked to break things up into "bite sized" chunks for the
> convenience of maintainers.
> > The aim was to make each a coherent item with a simple justification.
> >
> >> that I am trying to move ArmVExpressPkg out of EDK2 into
> >> edk2-platforms (where it arguably belongs) having this series in
> >> limbo for two months is basically blocking my work, and so I would
> >> like to explore ways to proceed with this without interfering with
> >> each other's work too much. At the same time, the way the code is
> >> structured is a continuation of the pattern I am trying to get rid
> >> of, so they will need some rework anyway in order to be upstreamable
> IMHO.
> >
> >  [[Evan Lloyd]] Not being psychic, we had not made allowance for your
> plans.
> > However, if you take the trouble to look at the changes, they achieve
> exactly the split you aim for.
> > The display type code (PL011 and HDLCD) is unravelled from the VE code.
> > All that remains would be to move the VE specific part into edk2-
> platforms.
> >
> >>
> >> So could we split it up please? Assuming the intention is the ability
> >> to reuse the Mali code on non-VExpress platforms, I would like to see
> >> that code proposed separately, without any mention of VExpressPkg.dec
> >
> >  [[Evan Lloyd]] Given that the original code was unfortunate, I'm not sure
> that it makes much difference what order the changes get made.
> > Going West then North gets you to the same place as North then West
> (except near a pole).
> > If you accept that there was a logical

Re: [edk2] [PATCH 02/19] ArmPlatformPkg: Tidy LcdGraphicsOutputDxe code: Added comments

2017-12-05 Thread Leif Lindholm

On Tue, Dec 05, 2017 at 06:55:25PM +, Evan Lloyd wrote:
> 
> 
> > -Original Message-
> > From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
> > Sent: 12 October 2017 20:02
> > To: Evan Lloyd 
> > Cc: edk2-devel@lists.01.org
> > Subject: Re: [edk2] [PATCH 02/19] ArmPlatformPkg: Tidy
> > LcdGraphicsOutputDxe code: Added comments
> >
> > Given that all changes to the first file _remove_ comments, it may be better
> > with a subject line saying "updating comments".
> >
> > On Tue, Sep 26, 2017 at 09:15:12PM +0100, evan.ll...@arm.com wrote:
> > > From: Girish Pathak 
> > >
> > > There is no functional modification in this change As preparation for
> > > a Change (Rejig of LcdGraphicsOutPutDxe), some comments are modified
> > > and a few new comments are added.
> > > This is to prevent mixing formatting changes with functional changes.
> > >
> > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > Signed-off-by: Girish Pathak 
> > > Signed-off-by: Evan Lloyd 
> > > ---
> ...
> > >
> > > -
> > > +/** Platform related initialization function.
> > > +  *
> > > +  * @param IN Handle   Handle to the LCD device instance.
> > > +  *
> > > +  * @retval EFI_SUCCESSPlatform initialization success.
> > > +  * @retval !(EFI_SUCCESS) Other errors.
> > > +**/
> >
> > So ... 6.8 lists
> > /**
> >   text
> > **/
> > as the
> >
> > The format
> > /**
> >  * text
> > **/
> > is mentioned as "also legal because doxygen ignores the leading *".
> >
> > The format
> > /**
> >   *
> > **/
> > is never mentioned, although I guess "also legal" because * ignored.
> >
> > However, a quick skim in MdePkg suggests the former is the generally used
> > variant. Can you please update to that format throughout (drop the leading
> > '*' on lines not starting or ending the comment block)?
> 
>  [[Evan Lloyd]] I'm not sure if Outlook has mangled something, or I'm being 
> obtuse,
> but I'm not sure I follow the distinction you are making there.
> However, if your objection is to the leading '*' then we can remove
> it.

The objection is slightly with regards to the leading *, but moreso
over it aligning with the second * of the opening /** rather than the first.

It is entirely possible that some form of email mangling is the cause
(including perhaps you reading my reply in non-fixed width font).

> By the way - shouldn't it be:
> /**  Brief description
> 
>   Details
> **/  (see Horor vacuii)

That would be my preferred version. (I started typing that above, but
seem to have lost my way after "as the".)

It's just that
/**
 *
 **/
is common enough in the codebase that I wouldn't object to it.

Whereas I haven't seen
/**
  *
 **/
anywhere else

> I actually think the CCS is woefully inconsistent in its example
> comment style, and that although leading '*'s are acceptable to
> Doxygen, it would be better to stick to one style (that of the file
> header comment, without leading '*'s) throughout.

I won't argue about the consistency, and agree with your view on this.

/
Leif

> 
> >
> > No other comments (other than having these prototype documentations
> > are a great improvement).
> >
> > /
> > Leif
> >
> > >  EFI_STATUS
> > >  LcdPlatformInitializeDisplay (
> > >IN EFI_HANDLE   Handle
> > >);
> > >
> > > +/** Reserve VRAM memory in DRAM for the frame buffer
> > > +  * (unless it is reserved already).
> > > +  *
> > > +  * The allocated address can be used to set the frame buffer.
> > > +  * @param OUT VramBaseAddress  A pointer to the frame buffer
> > address.
> > > +  * @param OUT VramSize A pointer to the size of the frame
> > > +  * buffer in bytes
> > > +  *
> > > +  * @retval EFI_SUCCESS Frame buffer memory allocation 
> > > success.
> > > +  * @retval !(EFI_SUCCESS)  Other errors.
> > > +**/
> > >  EFI_STATUS
> > >  LcdPlatformGetVram (
> > >OUT EFI_PHYSICAL_ADDRESS* VramBaseAddress,
> > >OUT UINTN*VramSize
> > >);
> > >
> > > +/** Return total number of modes.
> > > +  *
> > > +  * @retval UINT32 Mode Number.
> > > +**/
> > >  UINT32
> > >  LcdPlatformGetMaxMode (
> > >VOID
> > >);
> > >
> > > +/** Set the requested display mode.
> > > +  *
> > > +  * @param IN ModeNumber Mode Number.
> > > +  * @retval   EFI_SUCCESSSet mode success.
> > > +  * @retval   EFI_INVALID_PARAMTER   Requested mode not found.
> > > +**/
> > >  EFI_STATUS
> > >  LcdPlatformSetMode (
> > >IN UINT32 ModeNumber
> > >);
> > >
> > > +/** Return information for the requested mode number.
> > > +  *
> > > +  * @param IN ModeNumberMode Number.
> > > +  * @param OUT Info Pointer for returned mode information
> > > +  * (on success).
> > > +  *
> >

Re: [edk2] [PATCH 02/19] ArmPlatformPkg: Tidy LcdGraphicsOutputDxe code: Added comments

2017-12-05 Thread Evan Lloyd



> -Original Message-
> From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
> Sent: 12 October 2017 20:02
> To: Evan Lloyd 
> Cc: edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH 02/19] ArmPlatformPkg: Tidy
> LcdGraphicsOutputDxe code: Added comments
>
> Given that all changes to the first file _remove_ comments, it may be better
> with a subject line saying "updating comments".
>
> On Tue, Sep 26, 2017 at 09:15:12PM +0100, evan.ll...@arm.com wrote:
> > From: Girish Pathak 
> >
> > There is no functional modification in this change As preparation for
> > a Change (Rejig of LcdGraphicsOutPutDxe), some comments are modified
> > and a few new comments are added.
> > This is to prevent mixing formatting changes with functional changes.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Girish Pathak 
> > Signed-off-by: Evan Lloyd 
> > ---
...
> >
> > -
> > +/** Platform related initialization function.
> > +  *
> > +  * @param IN Handle   Handle to the LCD device instance.
> > +  *
> > +  * @retval EFI_SUCCESSPlatform initialization success.
> > +  * @retval !(EFI_SUCCESS) Other errors.
> > +**/
>
> So ... 6.8 lists
> /**
>   text
> **/
> as the
>
> The format
> /**
>  * text
> **/
> is mentioned as "also legal because doxygen ignores the leading *".
>
> The format
> /**
>   *
> **/
> is never mentioned, although I guess "also legal" because * ignored.
>
> However, a quick skim in MdePkg suggests the former is the generally used
> variant. Can you please update to that format throughout (drop the leading
> '*' on lines not starting or ending the comment block)?

 [[Evan Lloyd]] I'm not sure if Outlook has mangled something, or I'm being 
obtuse,
but I'm not sure I follow the distinction you are making there.
However, if your objection is to the leading '*' then we can remove it.
By the way - shouldn't it be:
/**  Brief description

  Details
**/  (see Horor vacuii)

I actually think the CCS is woefully inconsistent in its example comment style, 
and that although leading '*'s are acceptable to Doxygen, it would be better to 
stick to one style (that of the file header comment, without leading '*'s) 
throughout.

>
> No other comments (other than having these prototype documentations
> are a great improvement).
>
> /
> Leif
>
> >  EFI_STATUS
> >  LcdPlatformInitializeDisplay (
> >IN EFI_HANDLE   Handle
> >);
> >
> > +/** Reserve VRAM memory in DRAM for the frame buffer
> > +  * (unless it is reserved already).
> > +  *
> > +  * The allocated address can be used to set the frame buffer.
> > +  * @param OUT VramBaseAddress  A pointer to the frame buffer
> address.
> > +  * @param OUT VramSize A pointer to the size of the frame
> > +  * buffer in bytes
> > +  *
> > +  * @retval EFI_SUCCESS Frame buffer memory allocation success.
> > +  * @retval !(EFI_SUCCESS)  Other errors.
> > +**/
> >  EFI_STATUS
> >  LcdPlatformGetVram (
> >OUT EFI_PHYSICAL_ADDRESS* VramBaseAddress,
> >OUT UINTN*VramSize
> >);
> >
> > +/** Return total number of modes.
> > +  *
> > +  * @retval UINT32 Mode Number.
> > +**/
> >  UINT32
> >  LcdPlatformGetMaxMode (
> >VOID
> >);
> >
> > +/** Set the requested display mode.
> > +  *
> > +  * @param IN ModeNumber Mode Number.
> > +  * @retval   EFI_SUCCESSSet mode success.
> > +  * @retval   EFI_INVALID_PARAMTER   Requested mode not found.
> > +**/
> >  EFI_STATUS
> >  LcdPlatformSetMode (
> >IN UINT32 ModeNumber
> >);
> >
> > +/** Return information for the requested mode number.
> > +  *
> > +  * @param IN ModeNumberMode Number.
> > +  * @param OUT Info Pointer for returned mode information
> > +  * (on success).
> > +  *
> > +  * @retval EFI_SUCCESS Success if the requested mode is found.
> > +  * @retval EFI_INVALID_PARAMETER   Requested mode not found.
> > +**/
> >  EFI_STATUS
> >  LcdPlatformQueryMode (
> >IN  UINT32ModeNumber,
> >OUT EFI_GRAPHICS_OUTPUT_MODE_INFORMATION  *Info
> >);
> >
> > +/** Returns the display timing information for the requested mode
> number.
> > +  *
> > +  * @param IN  ModeNumber   Mode Number.
> > +  * @param OUT HRes Pointer to horizontal resolution.
> > +  * @param OUT HSyncPointer to horizontal sync width.
> > +  * @param OUT HBackPorch   Pointer to horizontal back porch.
> > +  * @param OUT HFrontPorch  Pointer to horizontal front porch.
> > +  * @param OUT VRes Pointer to vertical resolution.
> > +  * @param OUT VSyncPointer to vertical sync width.
> > +  * @param OUT

Re: [edk2] [PATCH] BaseTools: improve C tools build time with GNU

2017-12-05 Thread Leif Lindholm

Hah, no ignore this.

I managed to not make it build the slow tools...

Back to the drawing board.

On Tue, Dec 05, 2017 at 06:08:14PM +, Leif Lindholm wrote:
> Commit 9e1131b70b4b
> ("BaseTools: Update BaseTools top GNUMakefile with the clear dependency")
> made parallel builds of BaseTools possible. However, the two utilities
> BrotliCompress and VfrCompile have substantially longer build (and
> especially link) time than the others - which leads to long waiting
> times. Build these two tools separately first, in order to reduce build
> time.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Leif Lindholm 
> ---
> 
> For comparison, this reduces the build time on my x86 machine (8 cores,
> 16 threads) down from >9s to <3s.
> On the Cortex-A53 based 24-core SynQuacer platform, it takes the build
> time down from 1m26s to 20s.
> 
>  BaseTools/Source/C/GNUmakefile | 9 ++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/BaseTools/Source/C/GNUmakefile b/BaseTools/Source/C/GNUmakefile
> index 0dc748267d..b7ddcb5022 100644
> --- a/BaseTools/Source/C/GNUmakefile
> +++ b/BaseTools/Source/C/GNUmakefile
> @@ -50,9 +50,11 @@ all: makerootdir subdirs
>  LIBRARIES = Common
>  VFRAUTOGEN = VfrCompile/VfrLexer.h
>  # NON_BUILDABLE_APPLICATIONS = GenBootSector BootSectImage
> -APPLICATIONS = \
> +SLOW_APPLICATIONS = \
>BrotliCompress \
> -  VfrCompile \
> +  VfrCompile
> +
> +APPLICATIONS = \
>GnuGenBootSector \
>BootSectImage \
>EfiLdrImage \
> @@ -72,7 +74,8 @@ APPLICATIONS = \
>  SUBDIRS := $(LIBRARIES) $(APPLICATIONS)
>  
>  $(LIBRARIES): $(MAKEROOT)/libs
> -$(APPLICATIONS): $(LIBRARIES) $(MAKEROOT)/bin $(VFRAUTOGEN)
> +$(SLOW_APPLICATIONS): $(LIBRARIES) $(MAKEROOT)/bin $(VFRAUTOGEN)
> +$(APPLICATIONS): $(SLOW_APPLICATIONS)
>  
>  .PHONY: outputdirs
>  makerootdir:
> -- 
> 2.11.0
> 
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] BaseTools: Fix GenSec GCC make failure

2017-12-05 Thread Laszlo Ersek

On 12/05/17 14:54, Yonghong Zhu wrote:
> It is a regression bug introduced by the patch b37b108, it cause GenSec
> make failure on GCC Env.
> 
> Cc: Liming Gao 
> Cc: Leif Lindholm 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Yonghong Zhu 
> ---
>  BaseTools/Source/C/GenSec/GenSec.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/BaseTools/Source/C/GenSec/GenSec.c 
> b/BaseTools/Source/C/GenSec/GenSec.c
> index 2b2def1..5545f12 100644
> --- a/BaseTools/Source/C/GenSec/GenSec.c
> +++ b/BaseTools/Source/C/GenSec/GenSec.c
> @@ -1324,11 +1324,11 @@ Returns:
>// Open file and read contents
>//
>DummyFile = fopen (LongFilePath (DummyFileName), "rb");
>if (DummyFile == NULL) {
>  Error (NULL, 0, 0001, "Error opening file", DummyFileName);
> -return EFI_ABORTED;
> +goto Finish;
>}
>  
>fseek (DummyFile, 0, SEEK_END);
>DummyFileSize = ftell (DummyFile);
>fseek (DummyFile, 0, SEEK_SET);
> @@ -1338,22 +1338,22 @@ Returns:
>DebugMsg (NULL, 0, 9, "Dummy files", "the dummy file name is %s and 
> the size is %u bytes", DummyFileName, (unsigned) DummyFileSize);
>  
>InFile = fopen(LongFilePath(InputFileName[0]), "rb");
>if (InFile == NULL) {
>  Error (NULL, 0, 0001, "Error opening file", InputFileName[0]);
> -return EFI_ABORTED;
> +goto Finish;
>}
>  
>fseek (InFile, 0, SEEK_END);
>InFileSize = ftell (InFile);
>fseek (InFile, 0, SEEK_SET);
>InFileBuffer = (UINT8 *) malloc (InFileSize);
>fread(InFileBuffer, 1, InFileSize, InFile);
>fclose(InFile);
>DebugMsg (NULL, 0, 9, "Input files", "the input file name is %s and 
> the size is %u bytes", InputFileName[0], (unsigned) InFileSize);
>if (InFileSize > DummyFileSize){
> -if (stricmp(DummyFileBuffer, InFileBuffer + (InFileSize - 
> DummyFileSize)) == 0){
> +if (stricmp((CHAR8 *)DummyFileBuffer, (CHAR8 *)(InFileBuffer + 
> (InFileSize - DummyFileSize))) == 0){
>SectGuidHeaderLength = InFileSize - DummyFileSize;
>  }
>}
>if (SectGuidHeaderLength == 0) {
>  SectGuidAttribute |= EFI_GUIDED_SECTION_PROCESSING_REQUIRED;
> 

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

[edk2] [PATCH] BaseTools: improve C tools build time with GNU

2017-12-05 Thread Leif Lindholm

Commit 9e1131b70b4b
("BaseTools: Update BaseTools top GNUMakefile with the clear dependency")
made parallel builds of BaseTools possible. However, the two utilities
BrotliCompress and VfrCompile have substantially longer build (and
especially link) time than the others - which leads to long waiting
times. Build these two tools separately first, in order to reduce build
time.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Leif Lindholm 
---

For comparison, this reduces the build time on my x86 machine (8 cores,
16 threads) down from >9s to <3s.
On the Cortex-A53 based 24-core SynQuacer platform, it takes the build
time down from 1m26s to 20s.

 BaseTools/Source/C/GNUmakefile | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/BaseTools/Source/C/GNUmakefile b/BaseTools/Source/C/GNUmakefile
index 0dc748267d..b7ddcb5022 100644
--- a/BaseTools/Source/C/GNUmakefile
+++ b/BaseTools/Source/C/GNUmakefile
@@ -50,9 +50,11 @@ all: makerootdir subdirs
 LIBRARIES = Common
 VFRAUTOGEN = VfrCompile/VfrLexer.h
 # NON_BUILDABLE_APPLICATIONS = GenBootSector BootSectImage
-APPLICATIONS = \
+SLOW_APPLICATIONS = \
   BrotliCompress \
-  VfrCompile \
+  VfrCompile
+
+APPLICATIONS = \
   GnuGenBootSector \
   BootSectImage \
   EfiLdrImage \
@@ -72,7 +74,8 @@ APPLICATIONS = \
 SUBDIRS := $(LIBRARIES) $(APPLICATIONS)
 
 $(LIBRARIES): $(MAKEROOT)/libs
-$(APPLICATIONS): $(LIBRARIES) $(MAKEROOT)/bin $(VFRAUTOGEN)
+$(SLOW_APPLICATIONS): $(LIBRARIES) $(MAKEROOT)/bin $(VFRAUTOGEN)
+$(APPLICATIONS): $(SLOW_APPLICATIONS)
 
 .PHONY: outputdirs
 makerootdir:
-- 
2.11.0

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

[edk2] [PATCH v3 2/2] MdeModulePkg/SdMmcPciHcDxe: allow HC capabilities to be overridden

2017-12-05 Thread Ard Biesheuvel

Invoke the newly introduced SD/MMC override protocol to override
the capabilities register after reading it from the device registers,
and to call the pre/post host init and reset hooks at the appropriate
times.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel 
---
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c   | 134 +++-
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h   |   1 +
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.inf |   2 +
 3 files changed, 133 insertions(+), 4 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c 
b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
index 0be8828abfcc..f1ea78de809e 100644
--- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
+++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
@@ -17,6 +17,8 @@
 
 #include "SdMmcPciHcDxe.h"
 
+STATIC EDKII_SD_MMC_OVERRIDE   *mOverride;
+
 //
 // Driver Global Variables
 //
@@ -214,6 +216,104 @@ Done:
 }
 
 /**
+  Execute a SD/MMC host controller reset
+
+  @param[in] Private  A pointer to the SD_MMC_HC_PRIVATE_DATA instance.
+  @param[in] Slot The slot number of the host controller to reset.
+**/
+STATIC
+EFI_STATUS
+SdMmcPciHcResetHost (
+  IN  SD_MMC_HC_PRIVATE_DATA  *Private,
+  IN  UINT8   Slot
+  )
+{
+  EFI_STATUSStatus;
+
+  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
+Status = mOverride->NotifyPhase (
+  >PassThru,
+  Private->ControllerHandle,
+  Slot,
+  EdkiiSdMmcResetPre);
+if (EFI_ERROR (Status)) {
+  DEBUG ((DEBUG_WARN,
+"%a: SD/MMC pre reset notifier callback failed - %r\n",
+__FUNCTION__, Status));
+  return Status;
+}
+  }
+
+  Status = SdMmcHcReset (Private->PciIo, Slot);
+  if (EFI_ERROR (Status)) {
+return Status;
+  }
+
+  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
+Status = mOverride->NotifyPhase (
+  >PassThru,
+  Private->ControllerHandle,
+  Slot,
+  EdkiiSdMmcResetPost);
+if (EFI_ERROR (Status)) {
+  DEBUG ((DEBUG_WARN,
+"%a: SD/MMC post reset notifier callback failed - %r\n",
+__FUNCTION__, Status));
+}
+  }
+  return Status;
+}
+
+/**
+  Initialize a SD/MMC host controller
+
+  @param[in] Private  A pointer to the SD_MMC_HC_PRIVATE_DATA instance.
+  @param[in] Slot The slot number of the host controller to initialize.
+**/
+STATIC
+EFI_STATUS
+SdMmcPciHcInitHost (
+  IN  SD_MMC_HC_PRIVATE_DATA  *Private,
+  IN  UINT8   Slot
+  )
+{
+  EFI_STATUSStatus;
+
+  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
+Status = mOverride->NotifyPhase (
+  >PassThru,
+  Private->ControllerHandle,
+  Slot,
+  EdkiiSdMmcInitHostPre);
+if (EFI_ERROR (Status)) {
+  DEBUG ((DEBUG_WARN,
+"%a: SD/MMC pre init notifier callback failed - %r\n",
+__FUNCTION__, Status));
+  return Status;
+}
+  }
+
+  Status = SdMmcHcInitHost (Private->PciIo, Slot, Private->Capability[Slot]);
+  if (EFI_ERROR (Status)) {
+return Status;
+  }
+
+  if (mOverride != NULL && mOverride->NotifyPhase != NULL) {
+Status = mOverride->NotifyPhase (
+  >PassThru,
+  Private->ControllerHandle,
+  Slot,
+  EdkiiSdMmcInitHostPost);
+if (EFI_ERROR (Status)) {
+  DEBUG ((DEBUG_WARN,
+"%a: SD/MMC post init notifier callback failed - %r\n",
+__FUNCTION__, Status));
+}
+  }
+  return Status;
+}
+
+/**
   Sd removable device enumeration callback function when the timer event is 
signaled.
 
   @param[in]  Event The Event this notify function registered to.
@@ -281,14 +381,14 @@ SdMmcPciHcEnumerateDevice (
 //
 // Reset the specified slot of the SD/MMC Pci Host Controller
 //
-Status = SdMmcHcReset (Private->PciIo, Slot);
+Status = SdMmcPciHcResetHost (Private, Slot);
 if (EFI_ERROR (Status)) {
   continue;
 }
 //
 // Reinitialize slot and restart identification process for the new 
attached device
 //
-Status = SdMmcHcInitHost (Private->PciIo, Slot, 
Private->Capability[Slot]);
+Status = SdMmcPciHcInitHost (Private, Slot);
 if (EFI_ERROR (Status)) {
   continue;
 }
@@ -601,6 +701,20 @@ SdMmcPciHcDriverBindingStart (
 goto Done;
   }
 
+  //
+  // Attempt to locate the singleton instance of the SD/MMC override protocol,
+  // which implements platform specific workarounds for non-standard SDHCI
+  // implementations.
+  //
+  if (mOverride

[edk2] [PATCH v3 0/2] quirks handling for SDHCI controllers

2017-12-05 Thread Ard Biesheuvel

Many SDHCI implementations exist that are almost spec complicant, and
could be driven by the generic SD/MMC host controller driver except
for some minimal necessary init time tweaks.

Adding such tweaks to the generic driver is undesirable. On the other
hand, forking the driver for every platform that has such a SDHCI
controller is problematic when it comes to upstreaming and ongoing
maintenance (which is arguably the point of upstreaming in the first
place).

So these patches propose a workaround that is minimally invasive on the
EDK2 side, but gives platforms a lot of leeway when it comes to applying
SDHCI quirks.

Changes since v2:
- use a singleton instance of the SD/MMC protocol rather than one per
  controller; this is needed to support 'reconnect -r', as pointed out
  by Ray
- use EDKII prefixes for all types defined by the protocol
- replace 'hook' with 'notify', and tweak some other identifiers
- add missing function comment headers for factored out functions

Changes since RFC/v1:
- add EFI_SD_MMC_PASS_THRU_PROTOCOL* member to override methods
- use UINT64* not VOID* to pass capability structure (which is always 64 bits
  in size)

Ard Biesheuvel (2):
  MdeModulePkg: introduce SD/MMC override protocol
  MdeModulePkg/SdMmcPciHcDxe: allow HC capabilities to be overridden

 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c   | 134 +++-
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h   |   1 +
 MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.inf |   2 +
 MdeModulePkg/Include/Protocol/SdMmcOverride.h| 103 +++
 MdeModulePkg/MdeModulePkg.dec|   3 +
 5 files changed, 239 insertions(+), 4 deletions(-)
 create mode 100644 MdeModulePkg/Include/Protocol/SdMmcOverride.h

-- 
2.11.0

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

[edk2] [PATCH v3 1/2] MdeModulePkg: introduce SD/MMC override protocol

2017-12-05 Thread Ard Biesheuvel

Many ARM based SoCs have integrated SDHCI controllers, and often,
these implementations deviate in subtle ways from the pertinent
specifications. On the one hand, these deviations are quite easy
to work around, but on the other hand, having a collection of SoC
specific workarounds in the generic driver stack is undesirable.

So let's introduce an optional SD/MMC override protocol that we
can invoke at the appropriate moments in the device initialization.
That way, the workaround itself remains platform specific, but we
can still use the generic driver stack on such platforms.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel 
---
 MdeModulePkg/Include/Protocol/SdMmcOverride.h | 103 
 MdeModulePkg/MdeModulePkg.dec |   3 +
 2 files changed, 106 insertions(+)

diff --git a/MdeModulePkg/Include/Protocol/SdMmcOverride.h 
b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
new file mode 100644
index ..af57988f5625
--- /dev/null
+++ b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
@@ -0,0 +1,103 @@
+/** @file
+  Protocol to describe overrides required to support non-standard SDHCI
+  implementations
+
+  Copyright (c) 2017, Linaro, Ltd. All rights reserved.
+
+  This program and the accompanying materials
+  are licensed and made available under the terms and conditions of the BSD 
License
+  which accompanies this distribution.  The full text of the license may be 
found at
+  http://opensource.org/licenses/bsd-license.php
+
+  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#ifndef __SD_MMC_OVERRIDE_H__
+#define __SD_MMC_OVERRIDE_H__
+
+#include 
+
+#define EDKII_SD_MMC_OVERRIDE_PROTOCOL_GUID \
+  { 0xeaf9e3c1, 0xc9cd, 0x46db, { 0xa5, 0xe5, 0x5a, 0x12, 0x4c, 0x83, 0x23, 
0x23 } }
+
+#define EDKII_SD_MMC_OVERRIDE_PROTOCOL_VERSION0x1
+
+typedef struct _EDKII_SD_MMC_OVERRIDE EDKII_SD_MMC_OVERRIDE;
+
+typedef enum {
+  EdkiiSdMmcResetPre,
+  EdkiiSdMmcResetPost,
+  EdkiiSdMmcInitHostPre,
+  EdkiiSdMmcInitHostPost,
+} EDKII_SD_MMC_PHASE_TYPE;
+
+/**
+
+  Override function for SDHCI capability bits
+
+  @param[in]  PassThru  A pointer to the
+EFI_SD_MMC_PASS_THRU_PROTOCOL instance.
+  @param[in]  ControllerHandle  The EFI_HANDLE of the controller.
+  @param[in]  Slot  The 0 based slot index.
+  @param[in,out]  SdMmcHcSlotCapability The SDHCI capability structure.
+
+  @retval EFI_SUCCESS   The override function completed successfully.
+  @retval EFI_NOT_FOUND The specified controller or slot does not 
exist.
+  @retval EFI_INVALID_PARAMETER SdMmcHcSlotCapability is NULL
+
+**/
+typedef
+EFI_STATUS
+(EFIAPI * EDKII_SD_MMC_CAPABILITY) (
+  IN  EFI_SD_MMC_PASS_THRU_PROTOCOL   *PassThru,
+  IN  EFI_HANDLE  ControllerHandle,
+  IN  UINT8   Slot,
+  IN  OUT UINT64  *SdMmcHcSlotCapability
+  );
+
+/**
+
+  Override function for SDHCI controller operations
+
+  @param[in]  PassThru  A pointer to the
+EFI_SD_MMC_PASS_THRU_PROTOCOL instance.
+  @param[in]  ControllerHandle  The EFI_HANDLE of the controller.
+  @param[in]  Slot  The 0 based slot index.
+  @param[in,out]  PhaseType The type of operation and whether the
+hook is invoked right before (pre) or
+right after (post)
+
+  @retval EFI_SUCCESS   The override function completed successfully.
+  @retval EFI_NOT_FOUND The specified controller or slot does not 
exist.
+  @retval EFI_INVALID_PARAMETER HookType is invalid
+
+**/
+typedef
+EFI_STATUS
+(EFIAPI * EDKII_SD_MMC_NOTIFY_PHASE) (
+  IN  EFI_SD_MMC_PASS_THRU_PROTOCOL   *PassThru,
+  IN  EFI_HANDLE  ControllerHandle,
+  IN  UINT8   Slot,
+  IN  EDKII_SD_MMC_PHASE_TYPE PhaseType
+  );
+
+struct _EDKII_SD_MMC_OVERRIDE {
+  //
+  // Protocol version of this implementation
+  //
+  UINTN Version;
+  //
+  // Callback to override SD/MMC host controller capability bits
+  //
+  EDKII_SD_MMC_CAPABILITY   Capability;
+  //
+  // Callback to invoke SD/MMC override hooks
+  //
+  EDKII_SD_MMC_NOTIFY_PHASE NotifyPhase;
+};
+
+extern EFI_GUID gEdkiiSdMmcOverrideProtocolGuid;
+
+#endif
diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index 856d67aceb21..64ceea029f94 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -562,6 +562,9 @@ [Protocols]
   ## Include/Protocol/SmmMemoryAttribute.h
   gEdkiiSmmMemoryAttributeProtocolGuid = { 0x69b792ea, 0x39ce, 0x402d, { 0xa2, 
0xa6, 0xf7, 0x21, 0xde,

Re: [edk2] [PATCH] MdeModulePkg/Core/Dxe: log informative memprotect msgs at DEBUG_INFO level

2017-12-05 Thread Laszlo Ersek

On 12/05/17 04:00, Zeng, Star wrote:
> Reviewed-by: Star Zeng 

Thank you both; commit a921228818b5.

Laszlo

> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com] 
> Sent: Tuesday, December 5, 2017 3:48 AM
> To: edk2-devel-01 
> Cc: Ard Biesheuvel ; Dong, Eric 
> ; Yao, Jiewen ; Gao, Liming 
> ; Zeng, Star 
> Subject: [PATCH] MdeModulePkg/Core/Dxe: log informative memprotect msgs at 
> DEBUG_INFO level
> 
> In commit 7eb927db3e25 ("MdeModulePkg/DxeCore: implement memory protection 
> policy", 2017-02-24), we added two informative messages with the
> InitializeDxeNxMemoryProtectionPolicy() function:
> 
>> InitializeDxeNxMemoryProtectionPolicy: applying strict permissions to 
>> active memory regions
> 
> and
> 
>> InitializeDxeNxMemoryProtectionPolicy: applying strict permissions to 
>> inactive memory regions
> 
> The messages don't report errors or warnings, thus downgrade their log masks 
> from DEBUG_ERROR to DEBUG_INFO.
> 
> Cc: Ard Biesheuvel 
> Cc: Eric Dong 
> Cc: Jiewen Yao 
> Cc: Liming Gao 
> Cc: Star Zeng 
> Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1520485
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek 
> ---
>  MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 13 +
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c 
> b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> index 21a52d0af55a..a74cfc137a22 100644
> --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> @@ -831,8 +831,11 @@ InitializeDxeNxMemoryProtectionPolicy (
>} while (Status == EFI_BUFFER_TOO_SMALL);
>ASSERT_EFI_ERROR (Status);
>  
> -  DEBUG((DEBUG_ERROR, "%a: applying strict permissions to active memory 
> regions\n",
> -__FUNCTION__));
> +  DEBUG ((
> +DEBUG_INFO,
> +"%a: applying strict permissions to active memory regions\n",
> +__FUNCTION__
> +));
>  
>MergeMemoryMapForProtectionPolicy (MemoryMap, , 
> DescriptorSize);
>  
> @@ -856,9 +859,11 @@ InitializeDxeNxMemoryProtectionPolicy (
>// accessible, but have not been added to the UEFI memory map (yet).
>//
>if (GetPermissionAttributeForMemoryType (EfiConventionalMemory) != 0) {
> -DEBUG((DEBUG_ERROR,
> +DEBUG ((
> +  DEBUG_INFO,
>"%a: applying strict permissions to inactive memory regions\n",
> -  __FUNCTION__));
> +  __FUNCTION__
> +  ));
>  
>  CoreAcquireGcdMemoryLock ();
>  
> --
> 2.14.1.3.gb7cf6e02401b
> 
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 

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

Re: [edk2] [PATCH v2 1/2] MdeModulePkg: introduce SD/MMC override protocol

2017-12-05 Thread Ard Biesheuvel

On 5 December 2017 at 13:09, Wu, Hao A  wrote:
>> -Original Message-
>> From: Ni, Ruiyu
>> Sent: Tuesday, December 05, 2017 6:35 PM
>> To: Ard Biesheuvel; Wu, Hao A
>> Cc: edk2-devel@lists.01.org; Leif Lindholm; Tian, Feng; Zeng, Star
>> Subject: Re: [edk2] [PATCH v2 1/2] MdeModulePkg: introduce SD/MMC override
>> protocol
>>
>> On 12/5/2017 6:24 PM, Ard Biesheuvel wrote:
>> > On 5 December 2017 at 10:12, Ni, Ruiyu  wrote:
>> >> Some comments re the detailed interfaces embedded in below.
>> >>
>> >> On 11/30/2017 6:11 PM, Ard Biesheuvel wrote:
>> >>>
>> >>> Many ARM based SoCs have integrated SDHCI controllers, and often,
>> >>> these implementations deviate in subtle ways from the pertinent
>> >>> specifications. On the one hand, these deviations are quite easy
>> >>> to work around, but on the other hand, having a collection of SoC
>> >>> specific workarounds in the generic driver stack is undesirable.
>> >>>
>> >>> So let's introduce an optional SD/MMC override protocol that we
>> >>> can invoke at the appropriate moments in the device initialization.
>> >>> That way, the workaround itself remains platform specific, but we
>> >>> can still use the generic driver stack on such platforms.
>> >>>
>> >>> Contributed-under: TianoCore Contribution Agreement 1.1
>> >>> Signed-off-by: Ard Biesheuvel 
>> >>> ---
>> >>>MdeModulePkg/Include/Protocol/SdMmcOverride.h | 103
>> 
>> >>>MdeModulePkg/MdeModulePkg.dec |   3 +
>> >>>2 files changed, 106 insertions(+)
>> >>>
>> >>> diff --git a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
>> >>> b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
>> >>> new file mode 100644
>> >>> index ..5a5c393896f4
>> >>> --- /dev/null
>> >>> +++ b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
>> >>> @@ -0,0 +1,103 @@
>> >>> +/** @file
>> >>> +  Protocol to describe overrides required to support non-standard SDHCI
>> >>> +  implementations
>>  >>> ...
>> >>> +  IN  EFI_SD_MMC_PASS_THRU_PROTOCOL   *PassThru,
>> >>> +  IN  EFI_HANDLE  ControllerHandle,
>> >>> +  IN  UINT8   Slot,
>> >>> +  IN  OUT UINT64  *SdMmcHcSlotCapability
>> >>> +  );
>> >>
>> >>
>> >> Is this API too specific?
>> >> Besides SlotCapability, is there any other attributes that may need
>> >> override as well?
>> >>
>> >
>> > The capability structure is the root data structure that describes the
>> > SD/MMC host controller. Which other data structures did you have in
>> > mind?
>> >
>>
>> I do not know either.
>> Hao, any comments?
>
> The service is overriding the 'Capability' field of the private data
> structure within the HC driver. It's the value read from the 'CAP'
> register of the SD/MMC HC.
>
> After a glance of the other fields in 'SD_MMC_HC_PRIVATE_DATA', maybe the
> 'MaxCurrent' field is another candidate can be overriden. However, I am
> not sure about this.
>

In my experience, trying to cover every imaginable use case upfront
usually fails. That is why I used enum based hooks, which are easily
expanded later if we need to. If we need to override maxcurrent later
on, we can just add it.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [PATCH v2] BaseTools: align ENCODE_ERROR macro with MdePkg version

2017-12-05 Thread Leif Lindholm

BaseTools' BaseTypes.h defined the ENCODE_ERROR macro as
 #define ENCODE_ERROR(a)  ((RETURN_STATUS)(MAX_BIT | (a)))
whereas MdePkg defines it as
 #define ENCODE_ERROR(StatusCode) ((RETURN_STATUS)(MAX_BIT | (StatusCode)))

When building with GCC 6.3 (at least) the former triggers
"error: overflow in implicit constant conversion [-Werror=overflow]"
Resolve this by aligning it with the latter one.

This also requires aligning the BaseTools typedef of RETURN_STATUS with
the MdePkg one: INTN -> UINTN.

Add an explicit initialization of *Alignment to 0 in GenFfs.c
GetAlignmentFromFile to get rid of a warning occuring with GCC after
this change (-Werror=maybe-uninitialized).

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Leif Lindholm 
---

Sending out a v2 because I got cought out by the leading # in the commit
message...

 BaseTools/Source/C/GenFfs/GenFfs.c| 1 +
 BaseTools/Source/C/Include/Common/BaseTypes.h | 4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/BaseTools/Source/C/GenFfs/GenFfs.c 
b/BaseTools/Source/C/GenFfs/GenFfs.c
index e2fb3e0d1e..3b4a9b7761 100644
--- a/BaseTools/Source/C/GenFfs/GenFfs.c
+++ b/BaseTools/Source/C/GenFfs/GenFfs.c
@@ -529,6 +529,7 @@ GetAlignmentFromFile(char *InFile, UINT32 *Alignment)
 
   InFileHandle= NULL;
   PeFileBuffer= NULL;
+  *Alignment  = 0;
 
   memset (, 0, sizeof (ImageContext));
 
diff --git a/BaseTools/Source/C/Include/Common/BaseTypes.h 
b/BaseTools/Source/C/Include/Common/BaseTypes.h
index 198647ab95..5fa4e560d8 100644
--- a/BaseTools/Source/C/Include/Common/BaseTypes.h
+++ b/BaseTools/Source/C/Include/Common/BaseTypes.h
@@ -170,12 +170,12 @@
 // EFI Error Codes common to all execution phases
 //
 
-typedef INTN RETURN_STATUS;
+typedef UINTN RETURN_STATUS;
 
 ///
 /// Set the upper bit to indicate EFI Error.
 ///
-#define ENCODE_ERROR(a)  (MAX_BIT | (a))
+#define ENCODE_ERROR(a)  ((RETURN_STATUS)(MAX_BIT | (a)))
 
 #define ENCODE_WARNING(a)(a)
 #define RETURN_ERROR(a)  ((a) < 0)
-- 
2.11.0

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

[edk2] [PATCH] BaseTools: align ENCODE_ERROR macro with MdePkg version

2017-12-05 Thread Leif Lindholm

BaseTools' BaseTypes.h defined the ENCODE_ERROR macro as
whereas MdePkg defines it as

When building with GCC 6.3 (at least) the former triggers
"error: overflow in implicit constant conversion [-Werror=overflow]"
Resolve this by aligning it with the latter one.

This also requires aligning the BaseTools typedef of RETURN_STATUS with
the MdePkg one: INTN -> UINTN.

Add an explicit initialization of *Alignment to 0 in GenFfs.c
GetAlignmentFromFile to get rid of a warning occuring with GCC after
this change (-Werror=maybe-uninitialized).

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Leif Lindholm 
---
 BaseTools/Source/C/GenFfs/GenFfs.c| 1 +
 BaseTools/Source/C/Include/Common/BaseTypes.h | 4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/BaseTools/Source/C/GenFfs/GenFfs.c 
b/BaseTools/Source/C/GenFfs/GenFfs.c
index e2fb3e0d1e..3b4a9b7761 100644
--- a/BaseTools/Source/C/GenFfs/GenFfs.c
+++ b/BaseTools/Source/C/GenFfs/GenFfs.c
@@ -529,6 +529,7 @@ GetAlignmentFromFile(char *InFile, UINT32 *Alignment)
 
   InFileHandle= NULL;
   PeFileBuffer= NULL;
+  *Alignment  = 0;
 
   memset (, 0, sizeof (ImageContext));
 
diff --git a/BaseTools/Source/C/Include/Common/BaseTypes.h 
b/BaseTools/Source/C/Include/Common/BaseTypes.h
index 198647ab95..5fa4e560d8 100644
--- a/BaseTools/Source/C/Include/Common/BaseTypes.h
+++ b/BaseTools/Source/C/Include/Common/BaseTypes.h
@@ -170,12 +170,12 @@
 // EFI Error Codes common to all execution phases
 //
 
-typedef INTN RETURN_STATUS;
+typedef UINTN RETURN_STATUS;
 
 ///
 /// Set the upper bit to indicate EFI Error.
 ///
-#define ENCODE_ERROR(a)  (MAX_BIT | (a))
+#define ENCODE_ERROR(a)  ((RETURN_STATUS)(MAX_BIT | (a)))
 
 #define ENCODE_WARNING(a)(a)
 #define RETURN_ERROR(a)  ((a) < 0)
-- 
2.11.0

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

Re: [edk2] [Patch] BaseTools: Fix GenSec GCC make failure

2017-12-05 Thread 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, December 5, 2017 9:55 PM
> To: edk2-devel@lists.01.org
> Cc: Leif Lindholm ; Gao, Liming 
> 
> Subject: [edk2] [Patch] BaseTools: Fix GenSec GCC make failure
> 
> It is a regression bug introduced by the patch b37b108, it cause GenSec
> make failure on GCC Env.
> 
> Cc: Liming Gao 
> Cc: Leif Lindholm 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Yonghong Zhu 
> ---
>  BaseTools/Source/C/GenSec/GenSec.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/BaseTools/Source/C/GenSec/GenSec.c 
> b/BaseTools/Source/C/GenSec/GenSec.c
> index 2b2def1..5545f12 100644
> --- a/BaseTools/Source/C/GenSec/GenSec.c
> +++ b/BaseTools/Source/C/GenSec/GenSec.c
> @@ -1324,11 +1324,11 @@ Returns:
>// Open file and read contents
>//
>DummyFile = fopen (LongFilePath (DummyFileName), "rb");
>if (DummyFile == NULL) {
>  Error (NULL, 0, 0001, "Error opening file", DummyFileName);
> -return EFI_ABORTED;
> +goto Finish;
>}
> 
>fseek (DummyFile, 0, SEEK_END);
>DummyFileSize = ftell (DummyFile);
>fseek (DummyFile, 0, SEEK_SET);
> @@ -1338,22 +1338,22 @@ Returns:
>DebugMsg (NULL, 0, 9, "Dummy files", "the dummy file name is %s and 
> the size is %u bytes", DummyFileName, (unsigned)
> DummyFileSize);
> 
>InFile = fopen(LongFilePath(InputFileName[0]), "rb");
>if (InFile == NULL) {
>  Error (NULL, 0, 0001, "Error opening file", InputFileName[0]);
> -return EFI_ABORTED;
> +goto Finish;
>}
> 
>fseek (InFile, 0, SEEK_END);
>InFileSize = ftell (InFile);
>fseek (InFile, 0, SEEK_SET);
>InFileBuffer = (UINT8 *) malloc (InFileSize);
>fread(InFileBuffer, 1, InFileSize, InFile);
>fclose(InFile);
>DebugMsg (NULL, 0, 9, "Input files", "the input file name is %s and 
> the size is %u bytes", InputFileName[0], (unsigned)
> InFileSize);
>if (InFileSize > DummyFileSize){
> -if (stricmp(DummyFileBuffer, InFileBuffer + (InFileSize - 
> DummyFileSize)) == 0){
> +if (stricmp((CHAR8 *)DummyFileBuffer, (CHAR8 *)(InFileBuffer + 
> (InFileSize - DummyFileSize))) == 0){
>SectGuidHeaderLength = InFileSize - DummyFileSize;
>  }
>}
>if (SectGuidHeaderLength == 0) {
>  SectGuidAttribute |= EFI_GUIDED_SECTION_PROCESSING_REQUIRED;
> --
> 2.6.1.windows.1
> 
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] BaseTools: Fix GenSec GCC make failure

2017-12-05 Thread Gao, Liming

Leif:
  I suggest to create another patch for it. 

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Leif 
> Lindholm
> Sent: Tuesday, December 5, 2017 10:51 PM
> To: Zhu, Yonghong 
> Cc: edk2-devel@lists.01.org; Gao, Liming 
> Subject: Re: [edk2] [Patch] BaseTools: Fix GenSec GCC make failure
> 
> On Tue, Dec 05, 2017 at 02:14:26PM +, Leif Lindholm wrote:
> > On Tue, Dec 05, 2017 at 09:54:59PM +0800, Yonghong Zhu wrote:
> > > It is a regression bug introduced by the patch b37b108, it cause GenSec
> > > make failure on GCC Env.
> > >
> > > Cc: Liming Gao 
> > > Cc: Leif Lindholm 
> > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > Signed-off-by: Yonghong Zhu 
> >
> > I can confirm this resolves my issue - pick either or both of:
> > Tested-by: Leif Lindholm 
> > Reviewed-by: Leif Lindholm 
> 
> Although thinking about it, it would still make sense for the
> ENCODE_ERROR macro in BaseTools/Source/C/Include/Common/BaseTypes.h to
> mimic the one in MdePkg/Include/Base.h:
> 
> diff --git a/BaseTools/Source/C/Include/Common/BaseTypes.h
> b/BaseTools/Source/C/Include/Common/BaseTypes.h
> index 198647ab95..5fa4e560d8 100644
> --- a/BaseTools/Source/C/Include/Common/BaseTypes.h
> +++ b/BaseTools/Source/C/Include/Common/BaseTypes.h
> @@ -170,12 +170,12 @@
>  // EFI Error Codes common to all execution phases
>   //
> 
> -typedef INTN RETURN_STATUS;
> +typedef UINTN RETURN_STATUS;
> 
>  ///
>  /// Set the upper bit to indicate EFI Error.
>  ///
> -#define ENCODE_ERROR(a)  (MAX_BIT | (a))
> +#define ENCODE_ERROR(a)  ((RETURN_STATUS)(MAX_BIT | (a)))
> 
>  #define ENCODE_WARNING(a)(a)
>  #define RETURN_ERROR(a)  ((a) < 0)
> 
> 
> Should I send this as a separate patch?
> 
> /
> Leif
> 
> > Thanks!
> >
> > > ---
> > >  BaseTools/Source/C/GenSec/GenSec.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/BaseTools/Source/C/GenSec/GenSec.c 
> > > b/BaseTools/Source/C/GenSec/GenSec.c
> > > index 2b2def1..5545f12 100644
> > > --- a/BaseTools/Source/C/GenSec/GenSec.c
> > > +++ b/BaseTools/Source/C/GenSec/GenSec.c
> > > @@ -1324,11 +1324,11 @@ Returns:
> > >// Open file and read contents
> > >//
> > >DummyFile = fopen (LongFilePath (DummyFileName), "rb");
> > >if (DummyFile == NULL) {
> > >  Error (NULL, 0, 0001, "Error opening file", DummyFileName);
> > > -return EFI_ABORTED;
> > > +goto Finish;
> > >}
> > >
> > >fseek (DummyFile, 0, SEEK_END);
> > >DummyFileSize = ftell (DummyFile);
> > >fseek (DummyFile, 0, SEEK_SET);
> > > @@ -1338,22 +1338,22 @@ Returns:
> > >DebugMsg (NULL, 0, 9, "Dummy files", "the dummy file name is %s 
> > > and the size is %u bytes", DummyFileName, (unsigned)
> DummyFileSize);
> > >
> > >InFile = fopen(LongFilePath(InputFileName[0]), "rb");
> > >if (InFile == NULL) {
> > >  Error (NULL, 0, 0001, "Error opening file", InputFileName[0]);
> > > -return EFI_ABORTED;
> > > +goto Finish;
> > >}
> > >
> > >fseek (InFile, 0, SEEK_END);
> > >InFileSize = ftell (InFile);
> > >fseek (InFile, 0, SEEK_SET);
> > >InFileBuffer = (UINT8 *) malloc (InFileSize);
> > >fread(InFileBuffer, 1, InFileSize, InFile);
> > >fclose(InFile);
> > >DebugMsg (NULL, 0, 9, "Input files", "the input file name is %s 
> > > and the size is %u bytes", InputFileName[0], (unsigned)
> InFileSize);
> > >if (InFileSize > DummyFileSize){
> > > -if (stricmp(DummyFileBuffer, InFileBuffer + (InFileSize - 
> > > DummyFileSize)) == 0){
> > > +if (stricmp((CHAR8 *)DummyFileBuffer, (CHAR8 *)(InFileBuffer + 
> > > (InFileSize - DummyFileSize))) == 0){
> > >SectGuidHeaderLength = InFileSize - DummyFileSize;
> > >  }
> > >}
> > >if (SectGuidHeaderLength == 0) {
> > >  SectGuidAttribute |= EFI_GUIDED_SECTION_PROCESSING_REQUIRED;
> > > --
> > > 2.6.1.windows.1
> > >
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] BaseTools: Fix GenSec GCC make failure

2017-12-05 Thread Leif Lindholm

On Tue, Dec 05, 2017 at 02:14:26PM +, Leif Lindholm wrote:
> On Tue, Dec 05, 2017 at 09:54:59PM +0800, Yonghong Zhu wrote:
> > It is a regression bug introduced by the patch b37b108, it cause GenSec
> > make failure on GCC Env.
> > 
> > Cc: Liming Gao 
> > Cc: Leif Lindholm 
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Yonghong Zhu 
> 
> I can confirm this resolves my issue - pick either or both of:
> Tested-by: Leif Lindholm 
> Reviewed-by: Leif Lindholm 

Although thinking about it, it would still make sense for the
ENCODE_ERROR macro in BaseTools/Source/C/Include/Common/BaseTypes.h to
mimic the one in MdePkg/Include/Base.h:

diff --git a/BaseTools/Source/C/Include/Common/BaseTypes.h
b/BaseTools/Source/C/Include/Common/BaseTypes.h
index 198647ab95..5fa4e560d8 100644
--- a/BaseTools/Source/C/Include/Common/BaseTypes.h
+++ b/BaseTools/Source/C/Include/Common/BaseTypes.h
@@ -170,12 +170,12 @@
 // EFI Error Codes common to all execution phases
  //

-typedef INTN RETURN_STATUS;
+typedef UINTN RETURN_STATUS;

 ///
 /// Set the upper bit to indicate EFI Error.
 ///
-#define ENCODE_ERROR(a)  (MAX_BIT | (a))
+#define ENCODE_ERROR(a)  ((RETURN_STATUS)(MAX_BIT | (a)))

 #define ENCODE_WARNING(a)(a)
 #define RETURN_ERROR(a)  ((a) < 0)
  

Should I send this as a separate patch?

/
Leif

> Thanks!
> 
> > ---
> >  BaseTools/Source/C/GenSec/GenSec.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/BaseTools/Source/C/GenSec/GenSec.c 
> > b/BaseTools/Source/C/GenSec/GenSec.c
> > index 2b2def1..5545f12 100644
> > --- a/BaseTools/Source/C/GenSec/GenSec.c
> > +++ b/BaseTools/Source/C/GenSec/GenSec.c
> > @@ -1324,11 +1324,11 @@ Returns:
> >// Open file and read contents
> >//
> >DummyFile = fopen (LongFilePath (DummyFileName), "rb");
> >if (DummyFile == NULL) {
> >  Error (NULL, 0, 0001, "Error opening file", DummyFileName);
> > -return EFI_ABORTED;
> > +goto Finish;
> >}
> >  
> >fseek (DummyFile, 0, SEEK_END);
> >DummyFileSize = ftell (DummyFile);
> >fseek (DummyFile, 0, SEEK_SET);
> > @@ -1338,22 +1338,22 @@ Returns:
> >DebugMsg (NULL, 0, 9, "Dummy files", "the dummy file name is %s and 
> > the size is %u bytes", DummyFileName, (unsigned) DummyFileSize);
> >  
> >InFile = fopen(LongFilePath(InputFileName[0]), "rb");
> >if (InFile == NULL) {
> >  Error (NULL, 0, 0001, "Error opening file", InputFileName[0]);
> > -return EFI_ABORTED;
> > +goto Finish;
> >}
> >  
> >fseek (InFile, 0, SEEK_END);
> >InFileSize = ftell (InFile);
> >fseek (InFile, 0, SEEK_SET);
> >InFileBuffer = (UINT8 *) malloc (InFileSize);
> >fread(InFileBuffer, 1, InFileSize, InFile);
> >fclose(InFile);
> >DebugMsg (NULL, 0, 9, "Input files", "the input file name is %s and 
> > the size is %u bytes", InputFileName[0], (unsigned) InFileSize);
> >if (InFileSize > DummyFileSize){
> > -if (stricmp(DummyFileBuffer, InFileBuffer + (InFileSize - 
> > DummyFileSize)) == 0){
> > +if (stricmp((CHAR8 *)DummyFileBuffer, (CHAR8 *)(InFileBuffer + 
> > (InFileSize - DummyFileSize))) == 0){
> >SectGuidHeaderLength = InFileSize - DummyFileSize;
> >  }
> >}
> >if (SectGuidHeaderLength == 0) {
> >  SectGuidAttribute |= EFI_GUIDED_SECTION_PROCESSING_REQUIRED;
> > -- 
> > 2.6.1.windows.1
> > 
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [platforms: PATCH v3 0/4] Armada 7k/8k - misc improvements pt.3

2017-12-05 Thread Marcin Wojtas

2017-12-05 15:10 GMT+01:00 Leif Lindholm :
>
> Thanks.
> For the series:
> Reviewed-by: Leif Lindholm 
> Pushed as 84c212b1b4..2348894694.
>

Thanks a lot!
Marcin

>
> On Tue, Dec 05, 2017 at 06:57:01AM +0100, Marcin Wojtas wrote:
> > Hi,
> >
> > The third version of the patchset brings the corrections
> > requested in the review. Details can be found in the changelog
> > below.
> >
> > The patches are available in the github:
> > https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/commits/misc-upstream-r20171205
> >
> > I'm looking forward to your comments or remarks.
> >
> > Best regards,
> > Marcin
> >
> > Changelog:
> > v2 -> v3
> > * 1/4
> > - remove hardcoded DEFINE from the dsc file and enable
> >   passing the argument in a command line during build time
> > - Add RB
> >
> > * 3/4
> > - Mention additional help information improvements in the
> >   commit log
> >
> > * 4/4
> > - Add RB
> >
> > v1 -> v2
> >
> > * 1/4
> > - Add tftp as a build-time selectable option
> > - Update commit message
> >
> > * 2/4
> > - Add RB
> >
> > * 3/4
> > - Leave initial file format and reword commit message
> >
> > * 4/4
> > - Leave PHY_AUTONEGOTIATE_TIMEOUT unmodified and
> >   remove double definition of the timeout.
> >
> > Marcin Wojtas (4):
> >   Marvell/Armada: Switch to dynamic tftp command
> >   Marvell/Armada: Fix watchdog control base
> >   Marvell/Applications: FirmwareUpdate: Fix usage information
> >   Marvell/Drivers: MvPhyDxe: Cleanup the header
> >
> >  Platform/Marvell/Applications/FirmwareUpdate/FUpdate.uni | Bin 5146 -> 
> > 5190 bytes
> >  Platform/Marvell/Armada/Armada.dsc.inc   |   6 +-
> >  Platform/Marvell/Armada/Armada70x0.fdf   |   3 +
> >  Platform/Marvell/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.c |   2 +-
> >  Platform/Marvell/Drivers/Net/Phy/MvPhyDxe/MvPhyDxe.h | 152 
> > 
> >  5 files changed, 39 insertions(+), 124 deletions(-)
> >
> > --
> > 2.7.4
> >
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [Patch] BaseTools: Fix GenSec GCC make failure

2017-12-05 Thread Leif Lindholm

On Tue, Dec 05, 2017 at 09:54:59PM +0800, Yonghong Zhu wrote:
> It is a regression bug introduced by the patch b37b108, it cause GenSec
> make failure on GCC Env.
> 
> Cc: Liming Gao 
> Cc: Leif Lindholm 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Yonghong Zhu 

I can confirm this resolves my issue - pick either or both of:
Tested-by: Leif Lindholm 
Reviewed-by: Leif Lindholm 

Thanks!

> ---
>  BaseTools/Source/C/GenSec/GenSec.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/BaseTools/Source/C/GenSec/GenSec.c 
> b/BaseTools/Source/C/GenSec/GenSec.c
> index 2b2def1..5545f12 100644
> --- a/BaseTools/Source/C/GenSec/GenSec.c
> +++ b/BaseTools/Source/C/GenSec/GenSec.c
> @@ -1324,11 +1324,11 @@ Returns:
>// Open file and read contents
>//
>DummyFile = fopen (LongFilePath (DummyFileName), "rb");
>if (DummyFile == NULL) {
>  Error (NULL, 0, 0001, "Error opening file", DummyFileName);
> -return EFI_ABORTED;
> +goto Finish;
>}
>  
>fseek (DummyFile, 0, SEEK_END);
>DummyFileSize = ftell (DummyFile);
>fseek (DummyFile, 0, SEEK_SET);
> @@ -1338,22 +1338,22 @@ Returns:
>DebugMsg (NULL, 0, 9, "Dummy files", "the dummy file name is %s and 
> the size is %u bytes", DummyFileName, (unsigned) DummyFileSize);
>  
>InFile = fopen(LongFilePath(InputFileName[0]), "rb");
>if (InFile == NULL) {
>  Error (NULL, 0, 0001, "Error opening file", InputFileName[0]);
> -return EFI_ABORTED;
> +goto Finish;
>}
>  
>fseek (InFile, 0, SEEK_END);
>InFileSize = ftell (InFile);
>fseek (InFile, 0, SEEK_SET);
>InFileBuffer = (UINT8 *) malloc (InFileSize);
>fread(InFileBuffer, 1, InFileSize, InFile);
>fclose(InFile);
>DebugMsg (NULL, 0, 9, "Input files", "the input file name is %s and 
> the size is %u bytes", InputFileName[0], (unsigned) InFileSize);
>if (InFileSize > DummyFileSize){
> -if (stricmp(DummyFileBuffer, InFileBuffer + (InFileSize - 
> DummyFileSize)) == 0){
> +if (stricmp((CHAR8 *)DummyFileBuffer, (CHAR8 *)(InFileBuffer + 
> (InFileSize - DummyFileSize))) == 0){
>SectGuidHeaderLength = InFileSize - DummyFileSize;
>  }
>}
>if (SectGuidHeaderLength == 0) {
>  SectGuidAttribute |= EFI_GUIDED_SECTION_PROCESSING_REQUIRED;
> -- 
> 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 3/4 V3] BaseTools: Update Gensec to set PROCESSING_REQUIRED value

2017-12-05 Thread Zhu, Yonghong

Hi Leif,

Thanks. I just sent out the patch to fix this bug. Could you help to try it?

Best Regards,
Zhu Yonghong


-Original Message-
From: Leif Lindholm [mailto:leif.lindh...@linaro.org] 
Sent: Tuesday, December 05, 2017 7:53 PM
To: Zhu, Yonghong 
Cc: edk2-devel@lists.01.org; Feng, YunhuaX ; Gao, 
Liming 
Subject: Re: [edk2] [Patch 3/4 V3] BaseTools: Update Gensec to set 
PROCESSING_REQUIRED value

This patch has broken BaseTools build with GCC on master:

make -C GenSec
make[2]: Entering directory '/work/git/edk2/BaseTools/Source/C/GenSec'
gcc  -c -MD -fshort-wchar -fno-strict-aliasing -Wall -Werror 
-Wno-deprecated-declarations -Wno-unused-result -nostdlib -c -g  -I .. -I 
../Include/Common -I ../Include/ -I ../Include/IndustryStandard -I ../Common/ 
-I .. -I . -I ../Include/X64/  -O2 GenSec.c -o GenSec.o In file included from 
../Include/Common/UefiBaseTypes.h:19:0,
 from GenSec.c:20:
GenSec.c: In function ‘main’:
../Include/Common/BaseTypes.h:178:38: error: overflow in implicit constant 
conversion [-Werror=overflow]
 #define ENCODE_ERROR(a)  (MAX_BIT | (a))
  ^
../Include/Common/BaseTypes.h:204:38: note: in expansion of macro ‘ENCODE_ERROR’
 #define RETURN_ABORTED   ENCODE_ERROR (21)
  ^~~~
../Include/Common/UefiBaseTypes.h:119:35: note: in expansion of macro 
‘RETURN_ABORTED’
 #define EFI_ABORTED   RETURN_ABORTED  
   ^~
GenSec.c:1329:16: note: in expansion of macro ‘EFI_ABORTED’
 return EFI_ABORTED;
^~~
../Include/Common/BaseTypes.h:178:38: error: overflow in implicit constant 
conversion [-Werror=overflow]
 #define ENCODE_ERROR(a)  (MAX_BIT | (a))
  ^
../Include/Common/BaseTypes.h:204:38: note: in expansion of macro ‘ENCODE_ERROR’
 #define RETURN_ABORTED   ENCODE_ERROR (21)
  ^~~~
../Include/Common/UefiBaseTypes.h:119:35: note: in expansion of macro 
‘RETURN_ABORTED’
 #define EFI_ABORTED   RETURN_ABORTED  
   ^~
GenSec.c:1343:16: note: in expansion of macro ‘EFI_ABORTED’
 return EFI_ABORTED;
^~~
GenSec.c:1354:21: error: pointer targets in passing argument 1 of ‘strcasecmp’ 
differ in signedness [-Werror=pointer-sign]
 if (stricmp(DummyFileBuffer, InFileBuffer + (InFileSize - 
DummyFileSize)) == 0){
 ^~~
In file included from GenSec.c:17:0:
/usr/include/string.h:529:12: note: expected ‘const char *’ but argument is of 
type ‘UINT8 * {aka unsigned char *}’
 extern int strcasecmp (const char *__s1, const char *__s2)
^~
GenSec.c:1354:38: error: pointer targets in passing argument 2 of ‘strcasecmp’ 
differ in signedness [-Werror=pointer-sign]
 if (stricmp(DummyFileBuffer, InFileBuffer + (InFileSize - 
DummyFileSize)) == 0){
  ^~~~ In file included from 
GenSec.c:17:0:
/usr/include/string.h:529:12: note: expected ‘const char *’ but argument is of 
type ‘UINT8 * {aka unsigned char *}’
 extern int strcasecmp (const char *__s1, const char *__s2)
^~
cc1: all warnings being treated as errors
../Makefiles/footer.makefile:27: recipe for target 'GenSec.o' failed
make[2]: *** [GenSec.o] Error 1
make[2]: Leaving directory '/work/git/edk2/BaseTools/Source/C/GenSec'
GNUmakefile:84: recipe for target 'GenSec' failed
make[1]: *** [GenSec] Error 2
make[1]: Leaving directory '/work/git/edk2/BaseTools/Source/C'
GNUmakefile:25: recipe for target 'Source/C' failed
make: *** [Source/C] Error 2
make: Leaving directory '/work/git/edk2/BaseTools'


On Wed, Nov 29, 2017 at 10:02:05PM +0800, Yonghong Zhu wrote:
> This patch add new option --dummy file, and we compare the dummpy file 
> with input file to decide whether we need to set PROCESSING_REQUIRED 
> value.
> 
> Cc: Liming Gao 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Yunhua Feng 
> ---
>  BaseTools/Source/C/GenSec/GenSec.c | 74 
> ++
>  1 file changed, 74 insertions(+)
> 
> diff --git a/BaseTools/Source/C/GenSec/GenSec.c 
> b/BaseTools/Source/C/GenSec/GenSec.c
> index d9cdc1f..904926c 100644
> --- a/BaseTools/Source/C/GenSec/GenSec.c
> +++ b/BaseTools/Source/C/GenSec/GenSec.c
> @@ -185,10 +185,13 @@ Returns:
>  used in Ver section.\n");
>fprintf (stdout, "  --sectionalign SectionAlign\n\
>  SectionAlign points to section alignment, which 
> support\n\
>  the alignment scope 1~16M. It is specified in same\n\
>  order that the section

[edk2] [Patch] BaseTools: Fix GenSec GCC make failure

2017-12-05 Thread Yonghong Zhu

It is a regression bug introduced by the patch b37b108, it cause GenSec
make failure on GCC Env.

Cc: Liming Gao 
Cc: Leif Lindholm 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Yonghong Zhu 
---
 BaseTools/Source/C/GenSec/GenSec.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/BaseTools/Source/C/GenSec/GenSec.c 
b/BaseTools/Source/C/GenSec/GenSec.c
index 2b2def1..5545f12 100644
--- a/BaseTools/Source/C/GenSec/GenSec.c
+++ b/BaseTools/Source/C/GenSec/GenSec.c
@@ -1324,11 +1324,11 @@ Returns:
   // Open file and read contents
   //
   DummyFile = fopen (LongFilePath (DummyFileName), "rb");
   if (DummyFile == NULL) {
 Error (NULL, 0, 0001, "Error opening file", DummyFileName);
-return EFI_ABORTED;
+goto Finish;
   }
 
   fseek (DummyFile, 0, SEEK_END);
   DummyFileSize = ftell (DummyFile);
   fseek (DummyFile, 0, SEEK_SET);
@@ -1338,22 +1338,22 @@ Returns:
   DebugMsg (NULL, 0, 9, "Dummy files", "the dummy file name is %s and the 
size is %u bytes", DummyFileName, (unsigned) DummyFileSize);
 
   InFile = fopen(LongFilePath(InputFileName[0]), "rb");
   if (InFile == NULL) {
 Error (NULL, 0, 0001, "Error opening file", InputFileName[0]);
-return EFI_ABORTED;
+goto Finish;
   }
 
   fseek (InFile, 0, SEEK_END);
   InFileSize = ftell (InFile);
   fseek (InFile, 0, SEEK_SET);
   InFileBuffer = (UINT8 *) malloc (InFileSize);
   fread(InFileBuffer, 1, InFileSize, InFile);
   fclose(InFile);
   DebugMsg (NULL, 0, 9, "Input files", "the input file name is %s and the 
size is %u bytes", InputFileName[0], (unsigned) InFileSize);
   if (InFileSize > DummyFileSize){
-if (stricmp(DummyFileBuffer, InFileBuffer + (InFileSize - 
DummyFileSize)) == 0){
+if (stricmp((CHAR8 *)DummyFileBuffer, (CHAR8 *)(InFileBuffer + 
(InFileSize - DummyFileSize))) == 0){
   SectGuidHeaderLength = InFileSize - DummyFileSize;
 }
   }
   if (SectGuidHeaderLength == 0) {
 SectGuidAttribute |= EFI_GUIDED_SECTION_PROCESSING_REQUIRED;
-- 
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 v2 1/2] MdeModulePkg: introduce SD/MMC override protocol

2017-12-05 Thread Wu, Hao A

> -Original Message-
> From: Ni, Ruiyu
> Sent: Tuesday, December 05, 2017 6:35 PM
> To: Ard Biesheuvel; Wu, Hao A
> Cc: edk2-devel@lists.01.org; Leif Lindholm; Tian, Feng; Zeng, Star
> Subject: Re: [edk2] [PATCH v2 1/2] MdeModulePkg: introduce SD/MMC override
> protocol
> 
> On 12/5/2017 6:24 PM, Ard Biesheuvel wrote:
> > On 5 December 2017 at 10:12, Ni, Ruiyu  wrote:
> >> Some comments re the detailed interfaces embedded in below.
> >>
> >> On 11/30/2017 6:11 PM, Ard Biesheuvel wrote:
> >>>
> >>> Many ARM based SoCs have integrated SDHCI controllers, and often,
> >>> these implementations deviate in subtle ways from the pertinent
> >>> specifications. On the one hand, these deviations are quite easy
> >>> to work around, but on the other hand, having a collection of SoC
> >>> specific workarounds in the generic driver stack is undesirable.
> >>>
> >>> So let's introduce an optional SD/MMC override protocol that we
> >>> can invoke at the appropriate moments in the device initialization.
> >>> That way, the workaround itself remains platform specific, but we
> >>> can still use the generic driver stack on such platforms.
> >>>
> >>> Contributed-under: TianoCore Contribution Agreement 1.1
> >>> Signed-off-by: Ard Biesheuvel 
> >>> ---
> >>>MdeModulePkg/Include/Protocol/SdMmcOverride.h | 103
> 
> >>>MdeModulePkg/MdeModulePkg.dec |   3 +
> >>>2 files changed, 106 insertions(+)
> >>>
> >>> diff --git a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> >>> b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> >>> new file mode 100644
> >>> index ..5a5c393896f4
> >>> --- /dev/null
> >>> +++ b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
> >>> @@ -0,0 +1,103 @@
> >>> +/** @file
> >>> +  Protocol to describe overrides required to support non-standard SDHCI
> >>> +  implementations
>  >>> ...
> >>> +  IN  EFI_SD_MMC_PASS_THRU_PROTOCOL   *PassThru,
> >>> +  IN  EFI_HANDLE  ControllerHandle,
> >>> +  IN  UINT8   Slot,
> >>> +  IN  OUT UINT64  *SdMmcHcSlotCapability
> >>> +  );
> >>
> >>
> >> Is this API too specific?
> >> Besides SlotCapability, is there any other attributes that may need
> >> override as well?
> >>
> >
> > The capability structure is the root data structure that describes the
> > SD/MMC host controller. Which other data structures did you have in
> > mind?
> >
> 
> I do not know either.
> Hao, any comments?

The service is overriding the 'Capability' field of the private data
structure within the HC driver. It's the value read from the 'CAP'
register of the SD/MMC HC.

After a glance of the other fields in 'SD_MMC_HC_PRIVATE_DATA', maybe the
'MaxCurrent' field is another candidate can be overriden. However, I am
not sure about this.

Best Regards,
Hao Wu

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

Re: [edk2] [PATCH v2 2/2] MdeModulePkg/SdMmcPciHcDxe: allow HC capabilities to be overridden

2017-12-05 Thread Wu, Hao A

One small comment below.

Best Regards,
Hao Wu


> -Original Message-
> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> Sent: Thursday, November 30, 2017 6:12 PM
> To: edk2-devel@lists.01.org
> Cc: leif.lindh...@linaro.org; Zeng, Star; Wu, Hao A; Tian, Feng; Ard 
> Biesheuvel
> Subject: [PATCH v2 2/2] MdeModulePkg/SdMmcPciHcDxe: allow HC capabilities
> to be overridden
> 
> Invoke the newly introduced SD/MMC override protocol to override
> the capabilities register after reading it from the device registers,
> and to call the pre/post host init and reset hooks at the appropriate
> times.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel 
> ---
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c   | 116
> +++-
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.h   |   6 +
>  MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.inf |   2 +
>  3 files changed, 119 insertions(+), 5 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> index 0be8828abfcc..61f64285807d 100644
> --- a/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> +++ b/MdeModulePkg/Bus/Pci/SdMmcPciHcDxe/SdMmcPciHcDxe.c
> @@ -60,7 +60,8 @@ SD_MMC_HC_PRIVATE_DATA gSdMmcPciHcTemplate = {
>{ // MaxCurrent
>  0,
>},
> -  0 // ControllerVersion
> +  0,// ControllerVersion
> +  NULL  // Override
>  };
> 
>  SD_DEVICE_PATHmSdDpTemplate = {
> @@ -213,6 +214,92 @@ Done:
>return;
>  }
> 

Please help to add the function description comments for function:
SdMmcPciHcResetHost() & SdMmcPciHcInitHost()

> +STATIC
> +EFI_STATUS
> +SdMmcPciHcResetHost (
> +  IN  SD_MMC_HC_PRIVATE_DATA  *Private,
> +  IN  UINT8   Slot
> +  )
> +{
> +  EFI_STATUSStatus;
> +
> +  if (Private->Override != NULL &&
> +  Private->Override->InvokeHook != NULL) {
> +Status = Private->Override->InvokeHook (
> +  >PassThru,
> +  Private->ControllerHandle,
> +  Slot,
> +  SD_MMC_OVERRIDE_RESET_PRE_HOOK);
> +if (EFI_ERROR (Status)) {
> +  DEBUG ((DEBUG_WARN, "%a: SD/MMC pre reset hook failed - %r\n",
> +__FUNCTION__, Status));
> +  return Status;
> +}
> +  }
> +
> +  Status = SdMmcHcReset (Private->PciIo, Slot);
> +  if (EFI_ERROR (Status)) {
> +return Status;
> +  }
> +
> +  if (Private->Override != NULL &&
> +  Private->Override->InvokeHook != NULL) {
> +Status = Private->Override->InvokeHook (
> +  >PassThru,
> +  Private->ControllerHandle,
> +  Slot,
> +  SD_MMC_OVERRIDE_RESET_POST_HOOK);
> +if (EFI_ERROR (Status)) {
> +  DEBUG ((DEBUG_WARN, "%a: SD/MMC post reset hook failed - %r\n",
> +__FUNCTION__, Status));
> +}
> +  }
> +  return Status;
> +}
> +
> +STATIC
> +EFI_STATUS
> +SdMmcPciHcInitHost (
> +  IN  SD_MMC_HC_PRIVATE_DATA  *Private,
> +  IN  UINT8   Slot
> +  )
> +{
> +  EFI_STATUSStatus;
> +
> +  if (Private->Override != NULL &&
> +  Private->Override->InvokeHook != NULL) {
> +Status = Private->Override->InvokeHook (
> +  >PassThru,
> +  Private->ControllerHandle,
> +  Slot,
> +  SD_MMC_OVERRIDE_INIT_HOST_PRE_HOOK);
> +if (EFI_ERROR (Status)) {
> +  DEBUG ((DEBUG_WARN, "%a: SD/MMC pre init hook failed - %r\n",
> +__FUNCTION__, Status));
> +  return Status;
> +}
> +  }
> +
> +  Status = SdMmcHcInitHost (Private->PciIo, Slot, Private->Capability[Slot]);
> +  if (EFI_ERROR (Status)) {
> +return Status;
> +  }
> +
> +  if (Private->Override != NULL &&
> +  Private->Override->InvokeHook != NULL) {
> +Status = Private->Override->InvokeHook (
> +  >PassThru,
> +  Private->ControllerHandle,
> +  Slot,
> +  SD_MMC_OVERRIDE_INIT_HOST_POST_HOOK);
> +if (EFI_ERROR (Status)) {
> +  DEBUG ((DEBUG_WARN, "%a: SD/MMC post init hook failed - %r\n",
> +__FUNCTION__, Status));
> +}
> +  }
> +  return Status;
> +}
> +
>  /**
>Sd removable device enumeration callback function when the timer event is
> signaled.
> 
> @@ -281,14 +368,14 @@ SdMmcPciHcEnumerateDevice (
>  //
>  // Reset the specified slot of the SD/MMC Pci Host Controller
>  //
> -Status = SdMmcHcReset (Private->PciIo, Slot);
> +Status = SdMmcPciHcResetHost

Re: [edk2] [Patch 3/4 V3] BaseTools: Update Gensec to set PROCESSING_REQUIRED value

2017-12-05 Thread Leif Lindholm

This patch has broken BaseTools build with GCC on master:

make -C GenSec
make[2]: Entering directory '/work/git/edk2/BaseTools/Source/C/GenSec'
gcc  -c -MD -fshort-wchar -fno-strict-aliasing -Wall -Werror 
-Wno-deprecated-declarations -Wno-unused-result -nostdlib -c -g  -I .. -I 
../Include/Common -I ../Include/ -I ../Include/IndustryStandard -I ../Common/ 
-I .. -I . -I ../Include/X64/  -O2 GenSec.c -o GenSec.o
In file included from ../Include/Common/UefiBaseTypes.h:19:0,
 from GenSec.c:20:
GenSec.c: In function ‘main’:
../Include/Common/BaseTypes.h:178:38: error: overflow in implicit constant 
conversion [-Werror=overflow]
 #define ENCODE_ERROR(a)  (MAX_BIT | (a))
  ^
../Include/Common/BaseTypes.h:204:38: note: in expansion of macro ‘ENCODE_ERROR’
 #define RETURN_ABORTED   ENCODE_ERROR (21)
  ^~~~
../Include/Common/UefiBaseTypes.h:119:35: note: in expansion of macro 
‘RETURN_ABORTED’
 #define EFI_ABORTED   RETURN_ABORTED  
   ^~
GenSec.c:1329:16: note: in expansion of macro ‘EFI_ABORTED’
 return EFI_ABORTED;
^~~
../Include/Common/BaseTypes.h:178:38: error: overflow in implicit constant 
conversion [-Werror=overflow]
 #define ENCODE_ERROR(a)  (MAX_BIT | (a))
  ^
../Include/Common/BaseTypes.h:204:38: note: in expansion of macro ‘ENCODE_ERROR’
 #define RETURN_ABORTED   ENCODE_ERROR (21)
  ^~~~
../Include/Common/UefiBaseTypes.h:119:35: note: in expansion of macro 
‘RETURN_ABORTED’
 #define EFI_ABORTED   RETURN_ABORTED  
   ^~
GenSec.c:1343:16: note: in expansion of macro ‘EFI_ABORTED’
 return EFI_ABORTED;
^~~
GenSec.c:1354:21: error: pointer targets in passing argument 1 of ‘strcasecmp’ 
differ in signedness [-Werror=pointer-sign]
 if (stricmp(DummyFileBuffer, InFileBuffer + (InFileSize - 
DummyFileSize)) == 0){
 ^~~
In file included from GenSec.c:17:0:
/usr/include/string.h:529:12: note: expected ‘const char *’ but argument is of 
type ‘UINT8 * {aka unsigned char *}’
 extern int strcasecmp (const char *__s1, const char *__s2)
^~
GenSec.c:1354:38: error: pointer targets in passing argument 2 of ‘strcasecmp’ 
differ in signedness [-Werror=pointer-sign]
 if (stricmp(DummyFileBuffer, InFileBuffer + (InFileSize - 
DummyFileSize)) == 0){
  ^~~~
In file included from GenSec.c:17:0:
/usr/include/string.h:529:12: note: expected ‘const char *’ but argument is of 
type ‘UINT8 * {aka unsigned char *}’
 extern int strcasecmp (const char *__s1, const char *__s2)
^~
cc1: all warnings being treated as errors
../Makefiles/footer.makefile:27: recipe for target 'GenSec.o' failed
make[2]: *** [GenSec.o] Error 1
make[2]: Leaving directory '/work/git/edk2/BaseTools/Source/C/GenSec'
GNUmakefile:84: recipe for target 'GenSec' failed
make[1]: *** [GenSec] Error 2
make[1]: Leaving directory '/work/git/edk2/BaseTools/Source/C'
GNUmakefile:25: recipe for target 'Source/C' failed
make: *** [Source/C] Error 2
make: Leaving directory '/work/git/edk2/BaseTools'


On Wed, Nov 29, 2017 at 10:02:05PM +0800, Yonghong Zhu wrote:
> This patch add new option --dummy file, and we compare the dummpy file
> with input file to decide whether we need to set PROCESSING_REQUIRED
> value.
> 
> Cc: Liming Gao 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Yunhua Feng 
> ---
>  BaseTools/Source/C/GenSec/GenSec.c | 74 
> ++
>  1 file changed, 74 insertions(+)
> 
> diff --git a/BaseTools/Source/C/GenSec/GenSec.c 
> b/BaseTools/Source/C/GenSec/GenSec.c
> index d9cdc1f..904926c 100644
> --- a/BaseTools/Source/C/GenSec/GenSec.c
> +++ b/BaseTools/Source/C/GenSec/GenSec.c
> @@ -185,10 +185,13 @@ Returns:
>  used in Ver section.\n");
>fprintf (stdout, "  --sectionalign SectionAlign\n\
>  SectionAlign points to section alignment, which 
> support\n\
>  the alignment scope 1~16M. It is specified in same\n\
>  order that the section file is input.\n");
> +  fprintf (stdout, "  --dummy dummyfile\n\
> +compare dummpyfile with input_file to decide 
> whether\n\
> +need to set PROCESSING_REQUIRED attribute.\n");
>fprintf (stdout, "  -v, --verbose Turn on verbose output with 
> informational messages.\n");
>fprintf (stdout, "  -q, --quiet   Disable all messages except key 
> message and fatal error\n");
>fprintf (stdout, "  -d, --debug level

Re: [edk2] [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP : Add support for Watchdog driver

2017-12-05 Thread Leif Lindholm

On Tue, Dec 05, 2017 at 05:07:00AM +, Udit Kumar wrote:
> >   I suggest return EFI_UNSUPPORTED for this case. The protocol 
> > implementation
> > could return its status besides spec defined status.
> 
> Thanks to help me , how core will treat this error  
> 1/  Wdt not available 
> 2/ ignoring this error 
> 3/ core is not registering handler  
> I guess 3 is valid, 

Looking at Core/Dxe/Misc/SetWatchdogTimer.c:
  //
  // Attempt to set the timeout
  //
  Status = gWatchdogTimer->SetTimerPeriod (gWatchdogTimer,
  MultU64x32 (Timeout, WATCHDOG_TIMER_CALIBRATE_PER_SECOND));

  //
  // Check for errors
  //
  if (EFI_ERROR (Status)) {
return EFI_DEVICE_ERROR;
  }

The SetWatchdogTimer() call would always return EFI_DEVICE_ERROR.

> On side track, looks wdt is not used by core services then do we
> really need this as part of arch protocol ?

Yes, that was ultimately what I was implying with my question
regarding whether this protocol is relevant for a watchdog that can
only ever reset the system on timeout.

The protocol looks to me to be designed to use a dedicated generic
timer as backing for a software watchdog.

Liming, Mike?

If that is the case, then I agree this driver should probably not
implement this protocol, but rather set up a timer event (or a
dedicated timer) to stroke the watchdog.

Regards,

Leif

> regards
> Udit 
> 
> > -Original Message-
> > From: Gao, Liming [mailto:liming@intel.com]
> > Sent: Monday, December 04, 2017 8:53 PM
> > To: Leif Lindholm ; Kinney, Michael D
> > 
> > Cc: Meenakshi Aggarwal ;
> > ard.biesheu...@linaro.org; edk2-devel@lists.01.org; Udit Kumar
> > ; Varun Sethi 
> > Subject: RE: [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP : Add 
> > support
> > for Watchdog driver
> > 
> > Leif:
> >   I suggest return EFI_UNSUPPORTED for this case. The protocol 
> > implementation
> > could return its status besides spec defined status.
> > 
> > Thanks
> > Liming
> > > -Original Message-
> > > From: Leif Lindholm [mailto:leif.lindh...@linaro.org]
> > > Sent: Monday, December 4, 2017 10:36 PM
> > > To: Kinney, Michael D ; Gao, Liming
> > > 
> > > Cc: Meenakshi Aggarwal ;
> > > ard.biesheu...@linaro.org; edk2-devel@lists.01.org;
> > > udit.ku...@nxp.com; v.se...@nxp.com
> > > Subject: Re: [PATCH edk2-platforms] [PATCH v3 2/9] Platform/NXP : Add
> > > support for Watchdog driver
> > >
> > > Mike, Liming, as MdePkg mainteiners - one question below:
> > >
> > > On Mon, Nov 27, 2017 at 04:21:50PM +0530, Meenakshi Aggarwal wrote:
> > > > diff --git a/Platform/NXP/Drivers/WatchDog/WatchDog.c
> > > > b/Platform/NXP/Drivers/WatchDog/WatchDog.c
> > > > new file mode 100644
> > > > index 000..a9c70ef
> > > > --- /dev/null
> > > > +++ b/Platform/NXP/Drivers/WatchDog/WatchDog.c
> > > > @@ -0,0 +1,421 @@
> > >
> > > ...
> > >
> > > > +/**
> > > > +  This function registers the handler NotifyFunction so it is
> > > > +called every time
> > > > +  the watchdog timer expires.  It also passes the amount of time
> > > > +since the last
> > > > +  handler call to the NotifyFunction.
> > > > +  If NotifyFunction is not NULL and a handler is not already
> > > > +registered,
> > > > +  then the new handler is registered and EFI_SUCCESS is returned.
> > > > +  If NotifyFunction is NULL, and a handler is already registered,
> > > > +  then that handler is unregistered.
> > > > +  If an attempt is made to register a handler when a handler is
> > > > +already registered,
> > > > +  then EFI_ALREADY_STARTED is returned.
> > > > +  If an attempt is made to unregister a handler when a handler is
> > > > +not registered,
> > > > +  then EFI_INVALID_PARAMETER is returned.
> > > > +
> > > > +  @param  This The EFI_TIMER_ARCH_PROTOCOL instance.
> > > > +  @param  NotifyFunction   The function to call when a timer interrupt 
> > > > fires.
> > This
> > > > +   function executes at TPL_HIGH_LEVEL. The 
> > > > DXE Core will
> > > > +   register a handler for the timer interrupt, 
> > > > so it can know
> > > > +   how much time has passed. This information 
> > > > is used to
> > > > +   signal timer based events. NULL will 
> > > > unregister the handler.
> > > > +
> > > > +  @retval EFI_SUCCESS   The watchdog timer handler was 
> > > > registered.
> > > > +  @retval EFI_ALREADY_STARTED   NotifyFunction is not NULL, and a
> > handler is already
> > > > +registered.
> > > > +  @retval EFI_INVALID_PARAMETER NotifyFunction is NULL, and a handler
> > was not
> > > > +previously registered.
> > > > +
> > > > +**/
> > > > +STATIC
> > > > +EFI_STATUS
> > > > +EFIAPI
> > > > +WdogRegisterHandler (
> > > > +  IN

Re: [edk2] [PATCH v2 1/2] MdeModulePkg: introduce SD/MMC override protocol

2017-12-05 Thread Ni, Ruiyu


On 12/5/2017 6:24 PM, Ard Biesheuvel wrote:

On 5 December 2017 at 10:12, Ni, Ruiyu  wrote:

Some comments re the detailed interfaces embedded in below.

On 11/30/2017 6:11 PM, Ard Biesheuvel wrote:


Many ARM based SoCs have integrated SDHCI controllers, and often,
these implementations deviate in subtle ways from the pertinent
specifications. On the one hand, these deviations are quite easy
to work around, but on the other hand, having a collection of SoC
specific workarounds in the generic driver stack is undesirable.

So let's introduce an optional SD/MMC override protocol that we
can invoke at the appropriate moments in the device initialization.
That way, the workaround itself remains platform specific, but we
can still use the generic driver stack on such platforms.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel 
---
   MdeModulePkg/Include/Protocol/SdMmcOverride.h | 103 
   MdeModulePkg/MdeModulePkg.dec |   3 +
   2 files changed, 106 insertions(+)

diff --git a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
new file mode 100644
index ..5a5c393896f4
--- /dev/null
+++ b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
@@ -0,0 +1,103 @@
+/** @file
+  Protocol to describe overrides required to support non-standard SDHCI
+  implementations

>>> ...

+  IN  EFI_SD_MMC_PASS_THRU_PROTOCOL   *PassThru,
+  IN  EFI_HANDLE  ControllerHandle,
+  IN  UINT8   Slot,
+  IN  OUT UINT64  *SdMmcHcSlotCapability
+  );



Is this API too specific?
Besides SlotCapability, is there any other attributes that may need
override as well?



The capability structure is the root data structure that describes the
SD/MMC host controller. Which other data structures did you have in
mind?



I do not know either.
Hao, any comments?

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

Re: [edk2] [PATCH v2 1/2] MdeModulePkg: introduce SD/MMC override protocol

2017-12-05 Thread Zeng, Star

BTW, I prefer all the typedef and structure/field names have EDKII_ prefix, for 
example MdeModulePkg\Include\Protocol\IoMmu.h and 
MdeModulePkg\Include\Ppi\SdMmcHostController.h.

Thanks,
Star
-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Ard 
Biesheuvel
Sent: Tuesday, December 5, 2017 6:25 PM
To: Ni, Ruiyu 
Cc: Wu, Hao A ; edk2-devel@lists.01.org; Leif Lindholm 
; Tian, Feng ; Zeng, Star 

Subject: Re: [edk2] [PATCH v2 1/2] MdeModulePkg: introduce SD/MMC override 
protocol

On 5 December 2017 at 10:12, Ni, Ruiyu  wrote:
> Some comments re the detailed interfaces embedded in below.
>
> On 11/30/2017 6:11 PM, Ard Biesheuvel wrote:
>>
>> Many ARM based SoCs have integrated SDHCI controllers, and often, 
>> these implementations deviate in subtle ways from the pertinent 
>> specifications. On the one hand, these deviations are quite easy to 
>> work around, but on the other hand, having a collection of SoC 
>> specific workarounds in the generic driver stack is undesirable.
>>
>> So let's introduce an optional SD/MMC override protocol that we can 
>> invoke at the appropriate moments in the device initialization.
>> That way, the workaround itself remains platform specific, but we can 
>> still use the generic driver stack on such platforms.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel 
>> ---
>>   MdeModulePkg/Include/Protocol/SdMmcOverride.h | 103 
>>   MdeModulePkg/MdeModulePkg.dec |   3 +
>>   2 files changed, 106 insertions(+)
>>
>> diff --git a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
>> b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
>> new file mode 100644
>> index ..5a5c393896f4
>> --- /dev/null
>> +++ b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
>> @@ -0,0 +1,103 @@
>> +/** @file
>> +  Protocol to describe overrides required to support non-standard 
>> +SDHCI
>> +  implementations
>> +
>> +  Copyright (c) 2017, Linaro, Ltd. All rights reserved.
>> +
>> +  This program and the accompanying materials  are licensed and made 
>> + available under the terms and conditions of the
>> BSD License
>> +  which accompanies this distribution.  The full text of the license 
>> + may
>> be found at
>> +  http://opensource.org/licenses/bsd-license.php
>> +
>> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" 
>> + BASIS,  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
>> + EXPRESS OR
>> IMPLIED.
>> +
>> +**/
>> +
>> +#ifndef __SD_MMC_OVERRIDE_H__
>> +#define __SD_MMC_OVERRIDE_H__
>> +
>> +#include 
>> +
>> +#define EDKII_SD_MMC_OVERRIDE_PROTOCOL_GUID \
>> +  { 0xeaf9e3c1, 0xc9cd, 0x46db, { 0xa5, 0xe5, 0x5a, 0x12, 0x4c, 
>> +0x83,
>> 0x23, 0x23 } }
>> +
>> +#define EDKII_SD_MMC_OVERRIDE_PROTOCOL_VERSION0x1
>> +
>> +typedef struct _SD_MMC_OVERRIDE SD_MMC_OVERRIDE;
>> +
>> +typedef enum {
>> +  SD_MMC_OVERRIDE_RESET_PRE_HOOK,
>> +  SD_MMC_OVERRIDE_RESET_POST_HOOK,
>> +  SD_MMC_OVERRIDE_INIT_HOST_PRE_HOOK,
>> +  SD_MMC_OVERRIDE_INIT_HOST_POST_HOOK,
>
>
> How about using name like "SdMmcResetPre"?

Sure

> Do "ResetPre"/"ResetPost" / "InitHostPre" / "InitHostPost" cover all 
> potential check points?
>

Perhaps not, but it is hard to predict the future :-) This is why I added the 
version field. (I should also update the second patch to reject the protocol if 
its version is higher than
EDKII_SD_MMC_OVERRIDE_PROTOCOL_VERSION)

>> +} SD_MMC_OVERRIDE_HOOK;
>
> How about use "SD_MMC_PHASE_TYPE"?
>

Sure.

>
>> +
>> +/**
>> +
>> +  Override function for SDHCI capability bits
>> +
>> +  @param[in]  PassThru  A pointer to the
>> +
>> + EFI_SD_MMC_PASS_THRU_PROTOCOL
>> instance.
>> +  @param[in]  ControllerHandle  The EFI_HANDLE of the controller.
>> +  @param[in]  Slot  The 0 based slot index.
>> +  @param[in,out]  SdMmcHcSlotCapability The SDHCI capability structure.
>> +
>> +  @retval EFI_SUCCESS   The override function completed
>> successfully.
>> +  @retval EFI_NOT_FOUND The specified controller or slot does not
>> exist.
>> +  @retval EFI_INVALID_PARAMETER SdMmcHcSlotCapability is NULL
>> +
>> +**/
>> +typedef
>> +EFI_STATUS
>> +(EFIAPI * SD_MMC_OVERRIDE_CAPABILITY) (
>
>
> How about "SD_MMC_CAPABILITY"?
>

Sure.

>> +  IN  EFI_SD_MMC_PASS_THRU_PROTOCOL   *PassThru,
>> +  IN  EFI_HANDLE  ControllerHandle,
>> +  IN  UINT8   Slot,
>> +  IN  OUT UINT64  *SdMmcHcSlotCapability
>> +  );
>
>
> Is this API too specific?
> Besides SlotCapability, is there any other attributes that may need 
> override as well?
>

The capability structure is the root data structure that describes the SD/MMC 
host controller.

[edk2] [PATCH 3/5] ArmPlatformPkg: add Null implementation of LcdPlatformlLib

2017-12-05 Thread Ard Biesheuvel

In order to be able to build ArmPlatformPkg components outside of
the context of a particular platform, add Null implementation of
LcdPlatformlLib.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel 
---
 ArmPlatformPkg/Library/LcdPlatformNullLib/LcdPlatformNullLib.c   | 92 

 ArmPlatformPkg/Library/LcdPlatformNullLib/LcdPlatformNullLib.inf | 28 ++
 2 files changed, 120 insertions(+)

diff --git a/ArmPlatformPkg/Library/LcdPlatformNullLib/LcdPlatformNullLib.c 
b/ArmPlatformPkg/Library/LcdPlatformNullLib/LcdPlatformNullLib.c
new file mode 100644
index ..071eb5ffd4be
--- /dev/null
+++ b/ArmPlatformPkg/Library/LcdPlatformNullLib/LcdPlatformNullLib.c
@@ -0,0 +1,92 @@
+/** @file
+
+  Copyright (c) 2017, Linaro, Ltd. All rights reserved.
+
+  This program and the accompanying materials
+  are licensed and made available under the terms and conditions of the BSD 
License
+  which accompanies this distribution.  The full text of the license may be 
found at
+  http://opensource.org/licenses/bsd-license.php
+
+  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#include 
+#include 
+#include 
+#include 
+
+EFI_STATUS
+LcdPlatformInitializeDisplay (
+  IN EFI_HANDLE   Handle
+  )
+{
+  ASSERT (FALSE);
+  return EFI_UNSUPPORTED;
+}
+
+EFI_STATUS
+LcdPlatformGetVram (
+  OUT EFI_PHYSICAL_ADDRESS* VramBaseAddress,
+  OUT UINTN*VramSize
+  )
+{
+  ASSERT (FALSE);
+  return EFI_UNSUPPORTED;
+}
+
+UINT32
+LcdPlatformGetMaxMode (
+  VOID
+  )
+{
+  ASSERT (FALSE);
+  return 0;
+}
+
+EFI_STATUS
+LcdPlatformSetMode (
+  IN UINT32 ModeNumber
+  )
+{
+  ASSERT (FALSE);
+  return EFI_UNSUPPORTED;
+}
+
+EFI_STATUS
+LcdPlatformQueryMode (
+  IN  UINT32ModeNumber,
+  OUT EFI_GRAPHICS_OUTPUT_MODE_INFORMATION  *Info
+  )
+{
+  ASSERT (FALSE);
+  return EFI_UNSUPPORTED;
+}
+
+EFI_STATUS
+LcdPlatformGetTimings (
+  IN  UINT32  ModeNumber,
+  OUT UINT32* HRes,
+  OUT UINT32* HSync,
+  OUT UINT32* HBackPorch,
+  OUT UINT32* HFrontPorch,
+  OUT UINT32* VRes,
+  OUT UINT32* VSync,
+  OUT UINT32* VBackPorch,
+  OUT UINT32* VFrontPorch
+  )
+{
+  ASSERT (FALSE);
+  return EFI_UNSUPPORTED;
+}
+
+EFI_STATUS
+LcdPlatformGetBpp (
+  IN  UINT32ModeNumber,
+  OUT LCD_BPP*  Bpp
+  )
+{
+  ASSERT (FALSE);
+  return EFI_UNSUPPORTED;
+}
diff --git a/ArmPlatformPkg/Library/LcdPlatformNullLib/LcdPlatformNullLib.inf 
b/ArmPlatformPkg/Library/LcdPlatformNullLib/LcdPlatformNullLib.inf
new file mode 100644
index ..41c1d9638812
--- /dev/null
+++ b/ArmPlatformPkg/Library/LcdPlatformNullLib/LcdPlatformNullLib.inf
@@ -0,0 +1,28 @@
+#/** @file
+#
+#  Copyright (c) 2017, Linaro, Ltd. All rights reserved.
+#
+#  This program and the accompanying materials
+#  are licensed and made available under the terms and conditions of the BSD 
License
+#  which accompanies this distribution.  The full text of the license may be 
found at
+#  http://opensource.org/licenses/bsd-license.php
+#
+#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
IMPLIED.
+#
+#**/
+
+[Defines]
+  INF_VERSION= 0x0001001A
+  BASE_NAME  = LcdPlatformNullLib
+  FILE_GUID  = b78d02bb-d0b5-4389-bc7f-b39ee846c784
+  MODULE_TYPE= BASE
+  VERSION_STRING = 1.0
+  LIBRARY_CLASS  = LcdPlatformNullLib
+
+[Sources]
+  LcdPlatformNullLib.c
+
+[Packages]
+  ArmPlatformPkg/ArmPlatformPkg.dec
+  MdePkg/MdePkg.dec
-- 
2.11.0

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

[edk2] [PATCH 2/5] ArmPlatformPkg/PrePeiCoreMPCore: use a unique GUID

2017-12-05 Thread Ard Biesheuvel

PrePeiCoreMPCore reuses the GUID of its unicore sibling, which is
usually fine, given that platforms never include both. However, it
prevents us from creating a package .DSC that does include both, so
update the GUID to a fresh one.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel 
---
 ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf 
b/ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf
index 8e0456f8dc2a..e3a31fa7c6f6 100644
--- a/ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf
+++ b/ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf
@@ -16,7 +16,7 @@
 [Defines]
   INF_VERSION= 0x00010005
   BASE_NAME  = ArmPlatformPrePeiCore
-  FILE_GUID  = 469fc080-aec1-11df-927c-0002a5d5c51b
+  FILE_GUID  = b78d02bb-d0b5-4389-bc7f-b39ee846c784
   MODULE_TYPE= SEC
   VERSION_STRING = 1.0
 
-- 
2.11.0

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

[edk2] [PATCH 4/5] ArmPlatformPkg: add Null implementation of NorFlashPlatformLib

2017-12-05 Thread Ard Biesheuvel

In order to be able to build ArmPlatformPkg components outside of
the context of a particular platform, add Null implementation of
NorFlashPlatformLib.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel 
---
 ArmPlatformPkg/Library/NorFlashPlatformNullLib/NorFlashPlatformNullLib.c   | 
34 
 ArmPlatformPkg/Library/NorFlashPlatformNullLib/NorFlashPlatformNullLib.inf | 
30 +
 2 files changed, 64 insertions(+)

diff --git 
a/ArmPlatformPkg/Library/NorFlashPlatformNullLib/NorFlashPlatformNullLib.c 
b/ArmPlatformPkg/Library/NorFlashPlatformNullLib/NorFlashPlatformNullLib.c
new file mode 100644
index ..264d18719763
--- /dev/null
+++ b/ArmPlatformPkg/Library/NorFlashPlatformNullLib/NorFlashPlatformNullLib.c
@@ -0,0 +1,34 @@
+/** @file
+
+ Copyright (c) 2014, Linaro Ltd. All rights reserved.
+
+ This program and the accompanying materials
+ are licensed and made available under the terms and conditions of the BSD 
License
+ which accompanies this distribution.  The full text of the license may be 
found at
+ http://opensource.org/licenses/bsd-license.php
+
+ THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+ **/
+
+#include 
+
+EFI_STATUS
+NorFlashPlatformInitialization (
+  VOID
+  )
+{
+  return EFI_SUCCESS;
+}
+
+EFI_STATUS
+NorFlashPlatformGetDevices (
+  OUT NOR_FLASH_DESCRIPTION   **NorFlashDescriptions,
+  OUT UINT32  *Count
+  )
+{
+  *NorFlashDescriptions = NULL;
+  *Count = 0;
+  return EFI_SUCCESS;
+}
diff --git 
a/ArmPlatformPkg/Library/NorFlashPlatformNullLib/NorFlashPlatformNullLib.inf 
b/ArmPlatformPkg/Library/NorFlashPlatformNullLib/NorFlashPlatformNullLib.inf
new file mode 100644
index ..777a629678e1
--- /dev/null
+++ b/ArmPlatformPkg/Library/NorFlashPlatformNullLib/NorFlashPlatformNullLib.inf
@@ -0,0 +1,30 @@
+#/** @file
+#
+#  Component description file for NorFlashPlatformNullLib module
+#
+#  Copyright (c) 2017, Linaro Ltd. All rights reserved.
+#
+#  This program and the accompanying materials
+#  are licensed and made available under the terms and conditions of the BSD 
License
+#  which accompanies this distribution.  The full text of the license may be 
found at
+#  http://opensource.org/licenses/bsd-license.php
+#
+#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
IMPLIED.
+#
+#**/
+
+[Defines]
+  INF_VERSION= 0x00010005
+  BASE_NAME  = NorFlashPlatformNullLib
+  FILE_GUID  = 29b733ad-d066-4df6-8a89-b9df1beb818a
+  MODULE_TYPE= DXE_DRIVER
+  VERSION_STRING = 1.0
+  LIBRARY_CLASS  = NorFlashPlatformLib
+
+[Sources.common]
+  NorFlashPlatformNullLib.c
+
+[Packages]
+  MdePkg/MdePkg.dec
+  ArmPlatformPkg/ArmPlatformPkg.dec
-- 
2.11.0

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

[edk2] [PATCH 5/5] ArmPlatformPkg: add package .DSC file

2017-12-05 Thread Ard Biesheuvel

Now that we have removed all the cruft from ArmPlatformPkg, add a .DSC
file that builds all the remaining components standalone, i.e., outside
of the context of any particular platform. This is primarily intended
for build time testing.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel 
---
 ArmPlatformPkg/ArmPlatformPkg.dsc | 121 
 1 file changed, 121 insertions(+)

diff --git a/ArmPlatformPkg/ArmPlatformPkg.dsc 
b/ArmPlatformPkg/ArmPlatformPkg.dsc
new file mode 100644
index ..032942e87891
--- /dev/null
+++ b/ArmPlatformPkg/ArmPlatformPkg.dsc
@@ -0,0 +1,121 @@
+#/** @file
+# ARM platform package.
+#
+# Copyright (c) 2009 - 2010, Apple Inc. All rights reserved.
+# Copyright (c) 2011 - 2015, ARM Ltd. All rights reserved.
+# Copyright (c) 2016 - 2017, Linaro Ltd. All rights reserved.
+#
+#This program and the accompanying materials
+#are licensed and made available under the terms and conditions of the BSD 
License
+#which accompanies this distribution. The full text of the license may be 
found at
+#http://opensource.org/licenses/bsd-license.php
+#
+#THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+#WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
IMPLIED.
+#
+#**/
+
+
+#
+# Defines Section - statements that will be processed to create a Makefile.
+#
+
+[Defines]
+  PLATFORM_NAME  = ArmPlatformPkg
+  PLATFORM_GUID  = 9ce08891-ac9c-476d-ab04-0c04d3a97544
+  PLATFORM_VERSION   = 0.1
+  DSC_SPECIFICATION  = 0x0001001A
+  OUTPUT_DIRECTORY   = Build/ArmPlatform
+  SUPPORTED_ARCHITECTURES= ARM|AARCH64
+  BUILD_TARGETS  = DEBUG|RELEASE
+  SKUID_IDENTIFIER   = DEFAULT
+
+[BuildOptions]
+  RELEASE_*_*_CC_FLAGS  = -DMDEPKG_NDEBUG
+  *_*_*_CC_FLAGS  = -DDISABLE_NEW_DEPRECATED_INTERFACES
+
+[PcdsFixedAtBuild]
+  gArmTokenSpaceGuid.PcdFdBaseAddress|0x0
+  gArmTokenSpaceGuid.PcdFdSize|0x1000
+
+[LibraryClasses.common]
+  ArmGicArchLib|ArmPkg/Library/ArmGicArchSecLib/ArmGicArchSecLib.inf
+  ArmGicLib|ArmPkg/Drivers/ArmGic/ArmGicLib.inf
+  ArmLib|ArmPkg/Library/ArmLib/ArmBaseLib.inf
+  
ArmPlatformLib|ArmPlatformPkg/Library/ArmPlatformLibNull/ArmPlatformLibNull.inf
+  
ArmPlatformStackLib|ArmPlatformPkg/Library/ArmPlatformStackLib/ArmPlatformStackLib.inf
+  ArmMmuLib|ArmPkg/Library/ArmMmuLib/ArmMmuBaseLib.inf
+  BaseLib|MdePkg/Library/BaseLib/BaseLib.inf
+  BaseMemoryLib|MdePkg/Library/BaseMemoryLib/BaseMemoryLib.inf
+  
CacheMaintenanceLib|ArmPkg/Library/ArmCacheMaintenanceLib/ArmCacheMaintenanceLib.inf
+  DebugAgentLib|MdeModulePkg/Library/DebugAgentLibNull/DebugAgentLibNull.inf
+  DebugLib|MdePkg/Library/BaseDebugLibNull/BaseDebugLibNull.inf
+  
DxeServicesTableLib|MdePkg/Library/DxeServicesTableLib/DxeServicesTableLib.inf
+  HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
+  IoLib|MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf
+  
LcdPlatformLib|ArmPlatformPkg/Library/LcdPlatformNullLib/LcdPlatformNullLib.inf
+  
LzmaDecompressLib|MdeModulePkg/Library/LzmaCustomDecompressLib/LzmaCustomDecompressLib.inf
+  
MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
+  MemoryInitPeiLib|ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.inf
+  
NorFlashPlatformLib|ArmPlatformPkg/Library/NorFlashPlatformNullLib/NorFlashPlatformNullLib.inf
+  PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
+  
PeCoffExtraActionLib|MdePkg/Library/BasePeCoffExtraActionLibNull/BasePeCoffExtraActionLibNull.inf
+  PeCoffLib|MdePkg/Library/BasePeCoffLib/BasePeCoffLib.inf
+  
PerformanceLib|MdePkg/Library/BasePerformanceLibNull/BasePerformanceLibNull.inf
+  PlatformPeiLib|ArmPlatformPkg/PlatformPei/PlatformPeiLib.inf
+  PrePiLib|EmbeddedPkg/Library/PrePiLib/PrePiLib.inf
+  PrintLib|MdePkg/Library/BasePrintLib/BasePrintLib.inf
+  SerialPortLib|MdePkg/Library/BaseSerialPortLibNull/BaseSerialPortLibNull.inf
+  TimerLib|MdePkg/Library/BaseTimerLibNullTemplate/BaseTimerLibNullTemplate.inf
+  
UefiBootServicesTableLib|MdePkg/Library/UefiBootServicesTableLib/UefiBootServicesTableLib.inf
+  
UefiDecompressLib|MdePkg/Library/BaseUefiDecompressLib/BaseUefiDecompressLib.inf
+  
UefiDriverEntryPoint|MdePkg/Library/UefiDriverEntryPoint/UefiDriverEntryPoint.inf
+  UefiLib|MdePkg/Library/UefiLib/UefiLib.inf
+  UefiRuntimeLib|MdePkg/Library/UefiRuntimeLib/UefiRuntimeLib.inf
+  DevicePathLib|MdePkg/Library/UefiDevicePathLib/UefiDevicePathLib.inf
+  
UefiRuntimeServicesTableLib|MdePkg/Library/UefiRuntimeServicesTableLib/UefiRuntimeServicesTableLib.inf
+
+  NULL|ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf
+  NULL|MdePkg/Library/BaseStackCheckLib/BaseStackCheckLib.inf
+

[edk2] [PATCH 0/5] ArmPlatformPkg: create standalone .dsc file

2017-12-05 Thread Ard Biesheuvel

Do some reshuffling and add some NULL library implementation so we can
start building ArmPlatformPkg from its own .DSC

Ard Biesheuvel (5):
  ArmPlatformPkg: remove unused ArmPlatformLibNullSec
  ArmPlatformPkg/PrePeiCoreMPCore: use a unique GUID
  ArmPlatformPkg: add Null implementation of LcdPlatformlLib
  ArmPlatformPkg: add Null implementation of NorFlashPlatformLib
  ArmPlatformPkg: add package .DSC file

 ArmPlatformPkg/ArmPlatformPkg.dsc  | 
121 
 ArmPlatformPkg/Library/ArmPlatformLibNull/ArmPlatformLibNullSec.inf|  
47 
 ArmPlatformPkg/Library/LcdPlatformNullLib/LcdPlatformNullLib.c |  
92 +++
 ArmPlatformPkg/Library/LcdPlatformNullLib/LcdPlatformNullLib.inf   |  
28 +
 ArmPlatformPkg/Library/NorFlashPlatformNullLib/NorFlashPlatformNullLib.c   |  
34 ++
 ArmPlatformPkg/Library/NorFlashPlatformNullLib/NorFlashPlatformNullLib.inf |  
30 +
 ArmPlatformPkg/PrePeiCore/PrePeiCoreMPCore.inf |   
2 +-
 7 files changed, 306 insertions(+), 48 deletions(-)
 create mode 100644 ArmPlatformPkg/ArmPlatformPkg.dsc
 delete mode 100644 
ArmPlatformPkg/Library/ArmPlatformLibNull/ArmPlatformLibNullSec.inf
 create mode 100644 
ArmPlatformPkg/Library/LcdPlatformNullLib/LcdPlatformNullLib.c
 create mode 100644 
ArmPlatformPkg/Library/LcdPlatformNullLib/LcdPlatformNullLib.inf
 create mode 100644 
ArmPlatformPkg/Library/NorFlashPlatformNullLib/NorFlashPlatformNullLib.c
 create mode 100644 
ArmPlatformPkg/Library/NorFlashPlatformNullLib/NorFlashPlatformNullLib.inf

-- 
2.11.0

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

[edk2] [PATCH 1/5] ArmPlatformPkg: remove unused ArmPlatformLibNullSec

2017-12-05 Thread Ard Biesheuvel

ArmPlatformLibNullSec is built from a secondary .inf that omits
ArmPlatformLibNullMem.c from the [Sources] section so the library
can be used in a SEC context. This is slightly dodgy, given that
the resulting library is incomplete. Let's just remove this version,
since it isn't used anywhere anyway.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel 
---
 ArmPlatformPkg/Library/ArmPlatformLibNull/ArmPlatformLibNullSec.inf | 47 

 1 file changed, 47 deletions(-)

diff --git 
a/ArmPlatformPkg/Library/ArmPlatformLibNull/ArmPlatformLibNullSec.inf 
b/ArmPlatformPkg/Library/ArmPlatformLibNull/ArmPlatformLibNullSec.inf
deleted file mode 100644
index 3cd5fd889ea2..
--- a/ArmPlatformPkg/Library/ArmPlatformLibNull/ArmPlatformLibNullSec.inf
+++ /dev/null
@@ -1,47 +0,0 @@
-#/* @file
-#  Copyright (c) 2011-2012, ARM Limited. All rights reserved.
-#
-#  This program and the accompanying materials
-#  are licensed and made available under the terms and conditions of the BSD 
License
-#  which accompanies this distribution.  The full text of the license may be 
found at
-#  http://opensource.org/licenses/bsd-license.php
-#
-#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
-#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
IMPLIED.
-#
-#*/
-
-[Defines]
-  INF_VERSION= 0x00010005
-  BASE_NAME  = ArmPlatformLibNull
-  FILE_GUID  = cb494bad-23ff-427e-8608-d7e138d3363b
-  MODULE_TYPE= BASE
-  VERSION_STRING = 1.0
-  LIBRARY_CLASS  = ArmPlatformLib
-
-[Packages]
-  MdePkg/MdePkg.dec
-  MdeModulePkg/MdeModulePkg.dec
-  ArmPkg/ArmPkg.dec
-  ArmPlatformPkg/ArmPlatformPkg.dec
-
-[LibraryClasses]
-  ArmLib
-  DebugLib
-
-[Sources.common]
-  ArmPlatformLibNull.c
-
-[Sources.Arm]
-  Arm/ArmPlatformHelper.S| GCC
-  Arm/ArmPlatformHelper.asm  | RVCT
-
-[Sources.AArch64]
-  AArch64/ArmPlatformHelper.S
-
-[FixedPcd]
-  gArmTokenSpaceGuid.PcdSystemMemoryBase
-  gArmTokenSpaceGuid.PcdSystemMemorySize
-
-  gArmTokenSpaceGuid.PcdArmPrimaryCoreMask
-  gArmTokenSpaceGuid.PcdArmPrimaryCore
-- 
2.11.0

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

Re: [edk2] [PATCH v2 1/2] MdeModulePkg: introduce SD/MMC override protocol

2017-12-05 Thread Ni, Ruiyu


Some comments re the detailed interfaces embedded in below.

On 11/30/2017 6:11 PM, Ard Biesheuvel wrote:

Many ARM based SoCs have integrated SDHCI controllers, and often,
these implementations deviate in subtle ways from the pertinent
specifications. On the one hand, these deviations are quite easy
to work around, but on the other hand, having a collection of SoC
specific workarounds in the generic driver stack is undesirable.

So let's introduce an optional SD/MMC override protocol that we
can invoke at the appropriate moments in the device initialization.
That way, the workaround itself remains platform specific, but we
can still use the generic driver stack on such platforms.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel 
---
  MdeModulePkg/Include/Protocol/SdMmcOverride.h | 103 
  MdeModulePkg/MdeModulePkg.dec |   3 +
  2 files changed, 106 insertions(+)

diff --git a/MdeModulePkg/Include/Protocol/SdMmcOverride.h 
b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
new file mode 100644
index ..5a5c393896f4
--- /dev/null
+++ b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
@@ -0,0 +1,103 @@
+/** @file
+  Protocol to describe overrides required to support non-standard SDHCI
+  implementations
+
+  Copyright (c) 2017, Linaro, Ltd. All rights reserved.
+
+  This program and the accompanying materials
+  are licensed and made available under the terms and conditions of the BSD 
License
+  which accompanies this distribution.  The full text of the license may be 
found at
+  http://opensource.org/licenses/bsd-license.php
+
+  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#ifndef __SD_MMC_OVERRIDE_H__
+#define __SD_MMC_OVERRIDE_H__
+
+#include 
+
+#define EDKII_SD_MMC_OVERRIDE_PROTOCOL_GUID \
+  { 0xeaf9e3c1, 0xc9cd, 0x46db, { 0xa5, 0xe5, 0x5a, 0x12, 0x4c, 0x83, 0x23, 
0x23 } }
+
+#define EDKII_SD_MMC_OVERRIDE_PROTOCOL_VERSION0x1
+
+typedef struct _SD_MMC_OVERRIDE SD_MMC_OVERRIDE;
+
+typedef enum {
+  SD_MMC_OVERRIDE_RESET_PRE_HOOK,
+  SD_MMC_OVERRIDE_RESET_POST_HOOK,
+  SD_MMC_OVERRIDE_INIT_HOST_PRE_HOOK,
+  SD_MMC_OVERRIDE_INIT_HOST_POST_HOOK,


How about using name like "SdMmcResetPre"?
Do "ResetPre"/"ResetPost" / "InitHostPre" / "InitHostPost" cover
all potential check points?


+} SD_MMC_OVERRIDE_HOOK;

How about use "SD_MMC_PHASE_TYPE"?



+
+/**
+
+  Override function for SDHCI capability bits
+
+  @param[in]  PassThru  A pointer to the
+EFI_SD_MMC_PASS_THRU_PROTOCOL instance.
+  @param[in]  ControllerHandle  The EFI_HANDLE of the controller.
+  @param[in]  Slot  The 0 based slot index.
+  @param[in,out]  SdMmcHcSlotCapability The SDHCI capability structure.
+
+  @retval EFI_SUCCESS   The override function completed successfully.
+  @retval EFI_NOT_FOUND The specified controller or slot does not 
exist.
+  @retval EFI_INVALID_PARAMETER SdMmcHcSlotCapability is NULL
+
+**/
+typedef
+EFI_STATUS
+(EFIAPI * SD_MMC_OVERRIDE_CAPABILITY) (


How about "SD_MMC_CAPABILITY"?


+  IN  EFI_SD_MMC_PASS_THRU_PROTOCOL   *PassThru,
+  IN  EFI_HANDLE  ControllerHandle,
+  IN  UINT8   Slot,
+  IN  OUT UINT64  *SdMmcHcSlotCapability
+  );


Is this API too specific?
Besides SlotCapability, is there any other attributes that may need
override as well?


+
+/**
+
+  Override function for SDHCI controller operations
+
+  @param[in]  PassThru  A pointer to the
+EFI_SD_MMC_PASS_THRU_PROTOCOL instance.
+  @param[in]  ControllerHandle  The EFI_HANDLE of the controller.
+  @param[in]  Slot  The 0 based slot index.
+  @param[in,out]  HookType  The type of operation and whether the
+hook is invoked right before (pre) or
+right after (post)
+
+  @retval EFI_SUCCESS   The override function completed successfully.
+  @retval EFI_NOT_FOUND The specified controller or slot does not 
exist.
+  @retval EFI_INVALID_PARAMETER HookType is invalid
+
+**/
+typedef
+EFI_STATUS
+(EFIAPI * SD_MMC_OVERRIDE_INVOKE_HOOK) (


How about "SD_MMC_NOTIFY_PHASE"?


+  IN  EFI_SD_MMC_PASS_THRU_PROTOCOL   *PassThru,
+  IN  EFI_HANDLE  ControllerHandle,
+  IN  UINT8   Slot,
+  IN  SD_MMC_OVERRIDE_HOOKHookType
+  );
+
+struct _SD_MMC_OVERRIDE {
+  //
+  // Protocol version of this implementation
+  //
+  UINTN   Version;
+  //
+  // Callback to override SD/MMC host controller capability bits
+  //
+  SD_MMC_OVERRIDE_CAPABILITY  OverrideCapability;How about

Re: [edk2] [PATCH v2 1/2] MdeModulePkg: introduce SD/MMC override protocol

2017-12-05 Thread Ard Biesheuvel

On 5 December 2017 at 08:50, Zeng, Star  wrote:
> Regardless the protocol name SdMmcOverride/SdMmcPlatform, so you mean the 
> protocol should be produced by a DXE driver, but not a UEFI driver, right? I 
> saw the example at 
> https://lists.01.org/pipermail/edk2-devel/2017-November/017672.html shared by 
> Ard installs the protocol in PlatformDxe that is a DXE driver.
>
> And you mean the SD/MMC host controller driver code should use 
> LocateProtocol, but not HandleProtocol to find out the protocol instance, 
> right?
>

I think this makes sense, yes. I will respin the second patch accordingly.

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

Re: [edk2] [PATCH v2 1/2] MdeModulePkg: introduce SD/MMC override protocol

2017-12-05 Thread Zeng, Star

Regardless the protocol name SdMmcOverride/SdMmcPlatform, so you mean the 
protocol should be produced by a DXE driver, but not a UEFI driver, right? I 
saw the example at 
https://lists.01.org/pipermail/edk2-devel/2017-November/017672.html shared by 
Ard installs the protocol in PlatformDxe that is a DXE driver.

And you mean the SD/MMC host controller driver code should use LocateProtocol, 
but not HandleProtocol to find out the protocol instance, right?


Thanks,
Star
-Original Message-
From: Ni, Ruiyu 
Sent: Tuesday, December 5, 2017 4:37 PM
To: Zeng, Star ; Ard Biesheuvel 
; edk2-devel@lists.01.org
Cc: Wu, Hao A ; Kinney, Michael D 
; Tian, Feng ; 
leif.lindh...@linaro.org
Subject: Re: [edk2] [PATCH v2 1/2] MdeModulePkg: introduce SD/MMC override 
protocol

On 12/5/2017 3:20 PM, Zeng, Star wrote:
> If making this protocol a platform level singleton instance, is it so hard to 
> define the interfaces and parameters since different controllers may need 
> different hook points and parameters?
> 
> Thanks,
> Star
> -Original Message-
> From: Ni, Ruiyu
> Sent: Tuesday, December 5, 2017 3:09 PM
> To: Ard Biesheuvel ; 
> edk2-devel@lists.01.org
> Cc: "hao.a...@intel.com; Kinney.d.michael"@intel.com; Tian, Feng 
> ; Zeng, Star ; 
> leif.lindh...@linaro.org
> Subject: Re: [edk2] [PATCH v2 1/2] MdeModulePkg: introduce SD/MMC 
> override protocol
> 
> On 11/30/2017 6:11 PM, Ard Biesheuvel wrote:
>> Many ARM based SoCs have integrated SDHCI controllers, and often, 
>> these implementations deviate in subtle ways from the pertinent 
>> specifications. On the one hand, these deviations are quite easy to 
>> work around, but on the other hand, having a collection of SoC 
>> specific workarounds in the generic driver stack is undesirable.
>>
>> So let's introduce an optional SD/MMC override protocol that we can 
>> invoke at the appropriate moments in the device initialization.
>> That way, the workaround itself remains platform specific, but we can 
>> still use the generic driver stack on such platforms.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel 
>> ---
>>MdeModulePkg/Include/Protocol/SdMmcOverride.h | 103 
>>MdeModulePkg/MdeModulePkg.dec |   3 +
>>2 files changed, 106 insertions(+)
>>
>> diff --git a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
>> b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
>> new file mode 100644
>> index ..5a5c393896f4
>> --- /dev/null
>> +++ b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
>> @@ -0,0 +1,103 @@
>> +/** @file
>> +  Protocol to describe overrides required to support non-standard 
>> +SDHCI
>> +  implementations
>> +
>> +  Copyright (c) 2017, Linaro, Ltd. All rights reserved.
>> +
>> +  This program and the accompanying materials  are licensed and made 
>> + available under the terms and conditions of the BSD License  which 
>> + accompanies this distribution.  The full text of the license may be 
>> + found at  http://opensource.org/licenses/bsd-license.php
>> +
>> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
>> + BASIS,  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS 
>> OR IMPLIED.
>> +
>> +**/
>> +
>> +#ifndef __SD_MMC_OVERRIDE_H__
>> +#define __SD_MMC_OVERRIDE_H__
>> +
>> +#include 
>> +
>> +#define EDKII_SD_MMC_OVERRIDE_PROTOCOL_GUID \
>> +  { 0xeaf9e3c1, 0xc9cd, 0x46db, { 0xa5, 0xe5, 0x5a, 0x12, 0x4c, 
>> +0x83, 0x23, 0x23 } }
>> +
>> +#define EDKII_SD_MMC_OVERRIDE_PROTOCOL_VERSION0x1
>> +
>> +typedef struct _SD_MMC_OVERRIDE SD_MMC_OVERRIDE;
>> +
>> +typedef enum {
>> +  SD_MMC_OVERRIDE_RESET_PRE_HOOK,
>> +  SD_MMC_OVERRIDE_RESET_POST_HOOK,
>> +  SD_MMC_OVERRIDE_INIT_HOST_PRE_HOOK,
>> +  SD_MMC_OVERRIDE_INIT_HOST_POST_HOOK,
>> +} SD_MMC_OVERRIDE_HOOK;
>> +
>> +/**
>> +
>> +  Override function for SDHCI capability bits
>> +
>> +  @param[in]  PassThru  A pointer to the
>> +EFI_SD_MMC_PASS_THRU_PROTOCOL 
>> instance.
>> +  @param[in]  ControllerHandle  The EFI_HANDLE of the controller.
>> +  @param[in]  Slot  The 0 based slot index.
>> +  @param[in,out]  SdMmcHcSlotCapability The SDHCI capability structure.
>> +
>> +  @retval EFI_SUCCESS   The override function completed 
>> successfully.
>> +  @retval EFI_NOT_FOUND The specified controller or slot does not 
>> exist.
>> +  @retval EFI_INVALID_PARAMETER SdMmcHcSlotCapability is NULL
>> +
>> +**/
>> +typedef
>> +EFI_STATUS
>> +(EFIAPI * SD_MMC_OVERRIDE_CAPABILITY) (
>> +  IN  EFI_SD_MMC_PASS_THRU_PROTOCOL   *PassThru,
>> +  IN  EFI_HANDLE  ControllerHandle,
>> +  IN  UINT8   Slot,
>> +  IN

Re: [edk2] [PATCH v2 1/2] MdeModulePkg: introduce SD/MMC override protocol

2017-12-05 Thread Ni, Ruiyu


On 12/5/2017 3:20 PM, Zeng, Star wrote:

If making this protocol a platform level singleton instance, is it so hard to 
define the interfaces and parameters since different controllers may need 
different hook points and parameters?

Thanks,
Star
-Original Message-
From: Ni, Ruiyu
Sent: Tuesday, December 5, 2017 3:09 PM
To: Ard Biesheuvel ; edk2-devel@lists.01.org
Cc: "hao.a...@intel.com; Kinney.d.michael"@intel.com; Tian, Feng 
; Zeng, Star ; leif.lindh...@linaro.org
Subject: Re: [edk2] [PATCH v2 1/2] MdeModulePkg: introduce SD/MMC override 
protocol

On 11/30/2017 6:11 PM, Ard Biesheuvel wrote:

Many ARM based SoCs have integrated SDHCI controllers, and often,
these implementations deviate in subtle ways from the pertinent
specifications. On the one hand, these deviations are quite easy to
work around, but on the other hand, having a collection of SoC
specific workarounds in the generic driver stack is undesirable.

So let's introduce an optional SD/MMC override protocol that we can
invoke at the appropriate moments in the device initialization.
That way, the workaround itself remains platform specific, but we can
still use the generic driver stack on such platforms.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel 
---
   MdeModulePkg/Include/Protocol/SdMmcOverride.h | 103 
   MdeModulePkg/MdeModulePkg.dec |   3 +
   2 files changed, 106 insertions(+)

diff --git a/MdeModulePkg/Include/Protocol/SdMmcOverride.h
b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
new file mode 100644
index ..5a5c393896f4
--- /dev/null
+++ b/MdeModulePkg/Include/Protocol/SdMmcOverride.h
@@ -0,0 +1,103 @@
+/** @file
+  Protocol to describe overrides required to support non-standard
+SDHCI
+  implementations
+
+  Copyright (c) 2017, Linaro, Ltd. All rights reserved.
+
+  This program and the accompanying materials  are licensed and made
+ available under the terms and conditions of the BSD License  which
+ accompanies this distribution.  The full text of the license may be
+ found at  http://opensource.org/licenses/bsd-license.php
+
+  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"
+ BASIS,  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
IMPLIED.
+
+**/
+
+#ifndef __SD_MMC_OVERRIDE_H__
+#define __SD_MMC_OVERRIDE_H__
+
+#include 
+
+#define EDKII_SD_MMC_OVERRIDE_PROTOCOL_GUID \
+  { 0xeaf9e3c1, 0xc9cd, 0x46db, { 0xa5, 0xe5, 0x5a, 0x12, 0x4c, 0x83,
+0x23, 0x23 } }
+
+#define EDKII_SD_MMC_OVERRIDE_PROTOCOL_VERSION0x1
+
+typedef struct _SD_MMC_OVERRIDE SD_MMC_OVERRIDE;
+
+typedef enum {
+  SD_MMC_OVERRIDE_RESET_PRE_HOOK,
+  SD_MMC_OVERRIDE_RESET_POST_HOOK,
+  SD_MMC_OVERRIDE_INIT_HOST_PRE_HOOK,
+  SD_MMC_OVERRIDE_INIT_HOST_POST_HOOK,
+} SD_MMC_OVERRIDE_HOOK;
+
+/**
+
+  Override function for SDHCI capability bits
+
+  @param[in]  PassThru  A pointer to the
+EFI_SD_MMC_PASS_THRU_PROTOCOL instance.
+  @param[in]  ControllerHandle  The EFI_HANDLE of the controller.
+  @param[in]  Slot  The 0 based slot index.
+  @param[in,out]  SdMmcHcSlotCapability The SDHCI capability structure.
+
+  @retval EFI_SUCCESS   The override function completed successfully.
+  @retval EFI_NOT_FOUND The specified controller or slot does not 
exist.
+  @retval EFI_INVALID_PARAMETER SdMmcHcSlotCapability is NULL
+
+**/
+typedef
+EFI_STATUS
+(EFIAPI * SD_MMC_OVERRIDE_CAPABILITY) (
+  IN  EFI_SD_MMC_PASS_THRU_PROTOCOL   *PassThru,
+  IN  EFI_HANDLE  ControllerHandle,
+  IN  UINT8   Slot,
+  IN  OUT UINT64  *SdMmcHcSlotCapability
+  );
+
+/**
+
+  Override function for SDHCI controller operations
+
+  @param[in]  PassThru  A pointer to the
+EFI_SD_MMC_PASS_THRU_PROTOCOL instance.
+  @param[in]  ControllerHandle  The EFI_HANDLE of the controller.
+  @param[in]  Slot  The 0 based slot index.
+  @param[in,out]  HookType  The type of operation and whether the
+hook is invoked right before (pre) or
+right after (post)
+
+  @retval EFI_SUCCESS   The override function completed successfully.
+  @retval EFI_NOT_FOUND The specified controller or slot does not 
exist.
+  @retval EFI_INVALID_PARAMETER HookType is invalid
+
+**/
+typedef
+EFI_STATUS
+(EFIAPI * SD_MMC_OVERRIDE_INVOKE_HOOK) (
+  IN  EFI_SD_MMC_PASS_THRU_PROTOCOL   *PassThru,
+  IN  EFI_HANDLE  ControllerHandle,
+  IN  UINT8   Slot,
+  IN  SD_MMC_OVERRIDE_HOOKHookType
+  );
+
+struct _SD_MMC_OVERRIDE {
+  //
+  // Protocol version of this

[edk2] [PATCH v3 1/2] MdeModulePkg/DxeIpl: Mark page table as read-only

2017-12-05 Thread Jian J Wang

> v3:
>Remove the public definition of PAGE_TABLE_POOL_HEADER but keep similar
>concept locally. CpuDxe has its own page table pool.

> v2:
>Introduce page table pool to ease the page table memory allocation and
>protection, which replaces the direct calling of AllocatePages().

This patch will set the memory pages used for page table as read-only
memory after the paging is setup. CR0.WP must set to let it take into
effect.

A simple page table memory management mechanism, page table pool concept,
is introduced to simplify the page table memory allocation and protection.
It will also help to reduce the potential recursive "split" action during
updating memory paging attributes.

The basic idea is to allocate a bunch of continuous pages of memory in
advance as one or more page table pools, and all future page tables
consumption will happen in those pool instead of system memory. If the page
pool is reserved at the boundary of 2MB page and with same size of 2MB page,
there's no page granularity "split" operation will be needed, because the
memory of new page tables (if needed) will be usually in the same page as
target page table you're working on.

And since we have centralized page tables (a few 2MB pages), it's easier
to protect them by changing their attributes to be read-only once and for
all. There's no need to apply the protection for new page tables any more
as long as the pool has free pages available.

Once current page table pool has been used up, one can allocate another 2MB
memory pool and just set this new 2MB memory block to be read-only instead of
setting the new page tables one page by one page.

Two new PCDs PcdPageTablePoolUnitSize and PcdPageTablePoolAlignment are used
to specify the size and alignment for page table pool. For IA32 processor
0x20 (2MB) is the only choice for both of them to meet the requirement of
page table pool.

Cc: Jiewen Yao 
Cc: Star Zeng 
Cc: Eric Dong 
Cc: Ruiyu Ni 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang 
---
 MdeModulePkg/Core/DxeIplPeim/DxeIpl.h|  34 +++
 MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  |   8 +-
 MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 301 ++-
 MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h |  26 ++
 4 files changed, 365 insertions(+), 4 deletions(-)

diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h 
b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h
index f3aabdb7e0..9dc80b1508 100644
--- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h
+++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.h
@@ -265,4 +265,38 @@ IsNullDetectionEnabled (
   VOID
   );
 
+/**
+  Prevent the memory pages used for page table from been overwritten.
+
+  @param[in] PageTableBaseBase address of page table (CR3).
+
+**/
+VOID
+EnablePageTableProtection (
+  IN  UINTN PageTableBase,
+  IN  BOOLEAN   Level4Paging
+  );
+
+/**
+  This API provides a way to allocate memory for page table.
+
+  This API can be called more than once to allocate memory for page tables.
+
+  Allocates the number of 4KB pages and returns a pointer to the allocated
+  buffer. The buffer returned is aligned on a 4KB boundary.
+
+  If Pages is 0, then NULL is returned.
+  If there is not enough memory remaining to satisfy the request, then NULL is
+  returned.
+
+  @param  Pages The number of 4 KB pages to allocate.
+
+  @return A pointer to the allocated buffer or NULL if allocation fails.
+
+**/
+VOID *
+AllocatePageTableMemory (
+  IN UINTN   Pages
+  );
+
 #endif
diff --git a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c 
b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
index 5649265367..13fff28e93 100644
--- a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
+++ b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
@@ -99,7 +99,7 @@ Create4GPageTablesIa32Pae (
   NumberOfPdpEntriesNeeded = (UINT32) LShiftU64 (1, (PhysicalAddressBits - 
30));
 
   TotalPagesNum = NumberOfPdpEntriesNeeded + 1;
-  PageAddress = (UINTN) AllocatePages (TotalPagesNum);
+  PageAddress = (UINTN) AllocatePageTableMemory (TotalPagesNum);
   ASSERT (PageAddress != 0);
 
   PageMap = (VOID *) PageAddress;
@@ -149,6 +149,12 @@ Create4GPageTablesIa32Pae (
   );
   }
 
+  //
+  // Protect the page table by marking the memory used for page table to be
+  // read-only.
+  //
+  EnablePageTableProtection ((UINTN)PageMap, FALSE);
+
   return (UINTN) PageMap;
 }
 
diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c 
b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
index 29b6205e88..4ef3521224 100644
--- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
+++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
@@ -31,6 +31,11 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
 #include "DxeIpl.h"
 #include "VirtualMemory.h"
 
+//
+// Global variable to keep track current

[edk2] [PATCH v3 0/2] Enable page table write protection

2017-12-05 Thread Jian J Wang

> v3 changes:
>  a. According to code review comments, remove the public definitions of
> page table pool. Now the DxeIpl and CpuDxe will have their own page
> table pool but in the same mechanism. Related PCDs, GUDI and headers
> are also removed.
>  b. Apply protection to all page tables, including new ones added in
> CpuDxe driver.
>  c. Code/comments cleanup.

> v2 changes:
>  a. Enable protection on any newly added page table after DxeIpl.
>  b. Introduce page table pool concept to make page table allocation
> and protection easier and error free.

Write Protect feature (CR0.WP) is always enabled in driver UefiCpuPkg/CpuDxe.
But the memory pages used for page table are not set as read-only in the driver
DxeIplPeim, after the paging is setup. This might jeopardize the page table
integrity if there's buffer overflow occured in other part of system.

This patch series will change this situation by clearing R/W bit in page 
attribute
of the pages used as page table.

Validation works include booting Windows (10/server 2016) and Linux 
(Fedora/Ubuntu)
on OVMF and Intel real platform.

Jian J Wang (2):
  MdeModulePkg/DxeIpl: Mark page table as read-only
  UefiCpuPkg/CpuDxe: Enable protection for newly added page table

 MdeModulePkg/Core/DxeIplPeim/DxeIpl.h|  34 +++
 MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  |   8 +-
 MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 301 ++-
 MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h |  26 ++
 UefiCpuPkg/CpuDxe/CpuDxe.c   |  17 +-
 UefiCpuPkg/CpuDxe/CpuDxe.h   |   2 +
 UefiCpuPkg/CpuDxe/CpuPageTable.c | 226 -
 UefiCpuPkg/CpuDxe/CpuPageTable.h |  34 +++
 8 files changed, 635 insertions(+), 13 deletions(-)

-- 
2.15.1.windows.2

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

[edk2] [PATCH v3 2/2] UefiCpuPkg/CpuDxe: Enable protection for newly added page table

2017-12-05 Thread Jian J Wang

> v3:
>Create driver's own page table pool instead of inheriting from DxeIpl.

> v2:
>Use the page table pool to allocate new page tables and save code
>to enable protection for them separately.

One of the functionalities of CpuDxe is to update memory paging attributes.
If page table protection is applied, it must be disabled temporarily before
any attributes update and enabled again afterwards.

This patch makes use of the same way as DxeIpl to allocate page table memory
from reserved memory pool, which helps to reduce potential "split" operation
and recursive calling of SetMemorySpaceAttributes().

Cc: Jiewen Yao 
Cc: Eric Dong 
Cc: Laszlo Ersek 
Cc: Ruiyu Ni 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang 
---
 UefiCpuPkg/CpuDxe/CpuDxe.c   |  17 ++-
 UefiCpuPkg/CpuDxe/CpuDxe.h   |   2 +
 UefiCpuPkg/CpuDxe/CpuPageTable.c | 226 +--
 UefiCpuPkg/CpuDxe/CpuPageTable.h |  34 ++
 4 files changed, 270 insertions(+), 9 deletions(-)

diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.c b/UefiCpuPkg/CpuDxe/CpuDxe.c
index 8ddebabd02..6ae2dcd1c7 100644
--- a/UefiCpuPkg/CpuDxe/CpuDxe.c
+++ b/UefiCpuPkg/CpuDxe/CpuDxe.c
@@ -25,6 +25,7 @@
 BOOLEAN   InterruptState = FALSE;
 EFI_HANDLEmCpuHandle = NULL;
 BOOLEAN   mIsFlushingGCD;
+BOOLEAN   mIsAllocatingPageTable = FALSE;
 UINT64mValidMtrrAddressMask;
 UINT64mValidMtrrBitsMask;
 UINT64mTimerPeriod = 0;
@@ -407,6 +408,20 @@ CpuSetMemoryAttributes (
 return EFI_SUCCESS;
   }
 
+  //
+  // During memory attributes updating, new pages may be allocated to setup
+  // smaller granularity of page table. Page allocation action might then cause
+  // another calling of CpuSetMemoryAttributes() recursively, due to memory
+  // protection policy configured (such as PcdDxeNxMemoryProtectionPolicy).
+  // Since this driver will always protect memory used as page table by itself,
+  // there's no need to apply protection policy requested from memory service.
+  // So it's safe to just return EFI_SUCCESS if this time of calling is caused
+  // by page table memory allocation.
+  //
+  if (mIsAllocatingPageTable) {
+DEBUG((DEBUG_VERBOSE, "  Allocating page table memory\n"));
+return EFI_SUCCESS;
+  }
 
   CacheAttributes = Attributes & CACHE_ATTRIBUTE_MASK;
   MemoryAttributes = Attributes & MEMORY_ATTRIBUTE_MASK;
@@ -487,7 +502,7 @@ CpuSetMemoryAttributes (
   //
   // Set memory attribute by page table
   //
-  return AssignMemoryPageAttributes (NULL, BaseAddress, Length, 
MemoryAttributes, AllocatePages);
+  return AssignMemoryPageAttributes (NULL, BaseAddress, Length, 
MemoryAttributes, NULL);
 }
 
 /**
diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.h b/UefiCpuPkg/CpuDxe/CpuDxe.h
index 9c0d22359d..540f5f2dbf 100644
--- a/UefiCpuPkg/CpuDxe/CpuDxe.h
+++ b/UefiCpuPkg/CpuDxe/CpuDxe.h
@@ -273,5 +273,7 @@ RefreshGcdMemoryAttributesFromPaging (
   VOID
   );
 
+extern BOOLEAN mIsAllocatingPageTable;
+
 #endif
 
diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c
index 9658ed74c5..57ec76378a 100644
--- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
+++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
@@ -87,6 +87,8 @@ PAGE_ATTRIBUTE_TABLE mPageAttributeTable[] = {
   {Page1G,  SIZE_1GB, PAGING_1G_ADDRESS_MASK_64},
 };
 
+PAGE_TABLE_POOL   *mPageTablePool = NULL;
+
 /**
   Enable write protection function for AP.
 
@@ -172,10 +174,6 @@ GetCurrentPagingContext (
   }
   if ((AsmReadCr0 () & BIT31) != 0) {
 PagingContext->ContextData.X64.PageTableBase = (AsmReadCr3 () & 
PAGING_4K_ADDRESS_MASK_64);
-if ((AsmReadCr0 () & BIT16) == 0) {
-  AsmWriteCr0 (AsmReadCr0 () | BIT16);
-  SyncMemoryPageAttributesAp (SyncCpuEnableWriteProtection);
-}
   } else {
 PagingContext->ContextData.X64.PageTableBase = 0;
   }
@@ -561,6 +559,59 @@ SplitPage (
   }
 }
 
+/**
+ Check the WP status in CR0 register. This bit is used to lock or unlock write
+ access to pages marked as read-only.
+
+  @retval TRUEWrite protection is enabled.
+  @retval FALSE   Write protection is disabled.
+**/
+BOOLEAN
+IsReadOnlyPageWriteProtected (
+  VOID
+  )
+{
+  return ((AsmReadCr0 () & BIT16) != 0);
+}
+
+/**
+  Disable write protection function for AP.
+
+  @param[in,out] Buffer  The pointer to private data buffer.
+**/
+VOID
+EFIAPI
+SyncCpuDisableWriteProtection (
+  IN OUT VOID *Buffer
+  )
+{
+  AsmWriteCr0 (AsmReadCr0() & ~BIT16);
+}
+
+/**
+ Disable Write Protect on pages marked as read-only.
+**/
+VOID
+DisableReadOnlyPageWriteProtect (
+  VOID
+  )
+{
+  AsmWriteCr0 (AsmReadCr0() & ~BIT16);
+  SyncMemoryPageAttributesAp (SyncCpuDisableWriteProtection);
+}
+
+/**
+ Enable Write Protect on pages marked as read-only.
+**/
+VOID
+EnableReadOnlyPageWriteProtect (
+  VOID
+  )
+{
+

Re: [edk2] [Patch 0/4] NetworkPkg/DnsDxe: Fix several issues in DnsDxe driver.

2017-12-05 Thread Fu, Siyuan

Series Reviewed-by: Fu Siyuan 



> -Original Message-
> From: Wu, Jiaxin
> Sent: Tuesday, December 5, 2017 2:59 PM
> To: edk2-devel@lists.01.org
> Cc: Ye, Ting ; Fu, Siyuan ; Wang,
> Fan ; Wu, Jiaxin 
> Subject: [Patch 0/4] NetworkPkg/DnsDxe: Fix several issues in DnsDxe
> driver.
> 
> Cc: Ye Ting 
> Cc: Fu Siyuan 
> Cc: Wang Fan 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Wu Jiaxin 
> 
> Jiaxin Wu (4):
>   NetworkPkg/DnsDxe: Remove the unnecessary if condition check in
> DNS.Config
>   NetworkPkg/DnsDxe: Update RetryCount/RetryInterval to comply with UEFI
> spec.
>   NetworkPkg/DnsDxe: Fix the potential memory leak issue.
>   NetworkPkg/DnsDxe: Avoid to access the freed memory buffer.
> 
>  NetworkPkg/DnsDxe/DnsDriver.h   |   4 +-
>  NetworkPkg/DnsDxe/DnsImpl.c | 135 ---
>  NetworkPkg/DnsDxe/DnsImpl.h |   5 +-
>  NetworkPkg/DnsDxe/DnsProtocol.c | 175 +++
> -
>  4 files changed, 229 insertions(+), 90 deletions(-)
> 
> --
> 1.9.5.msysgit.1

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

Re: [edk2] [Patch v2] MdeModulePkg/NetLib: Add NetLibDetectMediaWaitTimeout() API to support EFI_NOT_READY media state detection

2017-12-05 Thread Fu, Siyuan

Reviewed-by: Fu Siyuan 

> -Original Message-
> From: Wang, Fan
> Sent: Monday, December 4, 2017 11:42 AM
> To: edk2-devel@lists.01.org
> Cc: Fu, Siyuan ; Ye, Ting ; Wu,
> Jiaxin ; Wang, Fan 
> Subject: [Patch v2] MdeModulePkg/NetLib: Add NetLibDetectMediaWaitTimeout()
> API to support EFI_NOT_READY media state detection
> 
> V2:
>   * Return error status code directly when Aip protocol falied to detect
> media rather than wait for another time's check.
>   * Set media state default value to EFI_SUCCESS since some platforms may
> not support retrieving media state from Aip protocol.
> 
> Cc: Fu Siyuan 
> Cc: Ye Ting 
> Cc: Jiaxin Wu 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Wang Fan 
> ---
>  MdeModulePkg/Include/Library/NetLib.h|  40 +++
>  MdeModulePkg/Library/DxeNetLib/DxeNetLib.c   | 163
> +++
>  MdeModulePkg/Library/DxeNetLib/DxeNetLib.inf |   2 +
>  3 files changed, 205 insertions(+)
> 
> diff --git a/MdeModulePkg/Include/Library/NetLib.h
> b/MdeModulePkg/Include/Library/NetLib.h
> index b9df46c..7862df9 100644
> --- a/MdeModulePkg/Include/Library/NetLib.h
> +++ b/MdeModulePkg/Include/Library/NetLib.h
> @@ -93,10 +93,16 @@ typedef UINT16  TCP_PORTNO;
>  #define  DNS_CLASS_INET1
>  #define  DNS_CLASS_CH  3
>  #define  DNS_CLASS_HS  4
>  #define  DNS_CLASS_ANY 255
> 
> +//
> +// Number of 100ns units time Interval for network media state detect
> +//
> +#define MEDIA_STATE_DETECT_TIME_INTERVAL  100U
> +
> +
>  #pragma pack(1)
> 
>  //
>  // Ethernet head definition
>  //
> @@ -1246,10 +1252,44 @@ NetLibDetectMedia (
>IN  EFI_HANDLEServiceHandle,
>OUT BOOLEAN   *MediaPresent
>);
> 
>  /**
> +
> +  Detect media state for a network device. This routine will wait for a
> period of time at
> +  a specified checking interval when a certain network is under
> connecting until connection
> +  process finishes or timeout. If Aip protocol is supported by low layer
> drivers, three kinds
> +  of media states can be detected: EFI_SUCCESS, EFI_NOT_READY and
> EFI_NO_MEDIA, represents
> +  connected state, connecting state and no media state respectively. When
> function detects
> +  the current state is EFI_NOT_READY, it will loop to wait for next
> time's check until state
> +  turns to be EFI_SUCCESS or EFI_NO_MEDIA. If Aip protocol is not
> supported, function will
> +  call NetLibDetectMedia() and return state directly.
> +
> +  @param[in]   ServiceHandleThe handle where network service binding
> protocols are
> +installed on.
> +  @param[in]   Timeout  The maximum number of 100ns units to wait
> when network
> +is connecting. Zero value means detect
> once and return
> +immediately.
> +  @param[out]  MediaState   The pointer to the detected media state.
> +
> +  @retval EFI_SUCCESS   Media detection success.
> +  @retval EFI_INVALID_PARAMETER ServiceHandle is not a valid network
> device handle or
> +MediaState pointer is NULL.
> +  @retval EFI_DEVICE_ERROR  A device error occurred.
> +  @retval EFI_TIMEOUT   Network is connecting but timeout.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +NetLibDetectMediaWaitTimeout (
> +  IN  EFI_HANDLEServiceHandle,
> +  IN  UINT64Timeout,
> +  OUT EFI_STATUS*MediaState
> +  );
> +
> +
> +/**
>Create an IPv4 device path node.
> 
>The header type of IPv4 device path node is MESSAGING_DEVICE_PATH.
>The header subtype of IPv4 device path node is MSG_IPv4_DP.
>The length of the IPv4 device path node in bytes is 19.
> diff --git a/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
> b/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
> index b8544b8..1bfa33d 100644
> --- a/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
> +++ b/MdeModulePkg/Library/DxeNetLib/DxeNetLib.c
> @@ -17,10 +17,11 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND,
> EITHER EXPRESS OR IMPLIED.
>  #include 
> 
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
>  #include 
> 
> @@ -2501,10 +2502,172 @@ Exit:
> 
>return Status;
>  }
> 
>  /**
> +
> +  Detect media state for a network device. This routine will wait for a
> period of time at
> +  a specified checking interval when a certain network is under
> connecting until connection
> +  process finishs or timeout. If Aip protocol is supported by low layer
> drivers, three kinds
> +  of media states can be detected: EFI_SUCCESS, EFI_NOT_READY and
> EFI_NO_MEDIA, represents
> +  connected state, connecting state and no media state respectively. When
> function detects
> +  the

59 matches

Mail list logo