Re: [edk2] [PATCH] IntelSiliconPkg IntelVTdDxe: Signal AcpiNotificationFunc() initially

[Yao, Jiewen]

Reviewed-by: jiewen@intel.com

> -Original Message-
> From: Zeng, Star
> Sent: Thursday, January 4, 2018 11:31 AM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star ; Yao, Jiewen 
> Subject: [PATCH] IntelSiliconPkg IntelVTdDxe: Signal AcpiNotificationFunc()
> initially
> 
> Signal AcpiNotificationFunc() initially for the case that
> DMAR table has been installed when creating event.
> 
> Cc: Jiewen Yao 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Star Zeng 
> ---
>  IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.c | 16
> 
>  IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmarAcpiTable.c |  3 +++
>  2 files changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.c
> b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.c
> index 37b3b19bce90..648f64c20b77 100644
> --- a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.c
> +++ b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.c
> @@ -1,6 +1,6 @@
>  /** @file
> 
> -  Copyright (c) 2017, Intel Corporation. All rights reserved.
> +  Copyright (c) 2017 - 2018, Intel Corporation. All rights reserved.
>This program and the accompanying materials
>are licensed and made available under the terms and conditions of the BSD
> License
>which accompanies this distribution.  The full text of the license may be
> found at
> @@ -473,7 +473,8 @@ InitializeDmaProtection (
>EFI_STATUS  Status;
>EFI_EVENT   ExitBootServicesEvent;
>EFI_EVENT   LegacyBootEvent;
> -  EFI_EVENT   Event;
> +  EFI_EVENT   EventAcpi10;
> +  EFI_EVENT   EventAcpi20;
> 
>Status = gBS->CreateEventEx (
>EVT_NOTIFY_SIGNAL,
> @@ -481,7 +482,7 @@ InitializeDmaProtection (
>AcpiNotificationFunc,
>NULL,
>,
> -  
> +  
>);
>ASSERT_EFI_ERROR (Status);
> 
> @@ -491,10 +492,17 @@ InitializeDmaProtection (
>AcpiNotificationFunc,
>NULL,
>,
> -  
> +  
>);
>ASSERT_EFI_ERROR (Status);
> 
> +  //
> +  // Signal the events initially for the case
> +  // that DMAR table has been installed.
> +  //
> +  gBS->SignalEvent (EventAcpi20);
> +  gBS->SignalEvent (EventAcpi10);
> +
>Status = gBS->CreateEventEx (
>EVT_NOTIFY_SIGNAL,
>TPL_CALLBACK,
> diff --git a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmarAcpiTable.c
> b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmarAcpiTable.c
> index ce350bafbe3f..b16bd93d53f1 100644
> --- a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmarAcpiTable.c
> +++ b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmarAcpiTable.c
> @@ -1005,6 +1005,9 @@ GetDmarAcpiTable (
> 
> );
>}
> +  if (EFI_ERROR (Status)) {
> +return EFI_NOT_FOUND;
> +  }
>ASSERT (AcpiTable != NULL);
> 
>mAcpiDmarTable = FindAcpiPtr (
> --
> 2.7.0.windows.1

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

Re: [edk2] [PATCH] IntelSiliconPkg IntelVTdDxe: Use TPL to protect list/engine operation

[Zeng, Star]

Got it and agree.

I will keep TPL_CALLBACK for ExitBootServices/LegacyBoot.


Thanks,
Star
-Original Message-
From: Yao, Jiewen 
Sent: Thursday, January 4, 2018 2:17 PM
To: Yao, Jiewen ; Zeng, Star ; 
edk2-devel@lists.01.org
Subject: RE: [PATCH] IntelSiliconPkg IntelVTdDxe: Use TPL to protect 
list/engine operation

Correct typo.

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of 
> Yao, Jiewen
> Sent: Thursday, January 4, 2018 2:14 PM
> To: Zeng, Star ; edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH] IntelSiliconPkg IntelVTdDxe: Use TPL to 
> protect list/engine operation
> 
> It is good to have lock for linked list management.
> 
> However, I do not think we should update TPL for ExitBootServices/LegacyBoot.
> 
> I purposely use TPL_CALLBACK to make sure VTd is tear down later, so 
> that other driver can stop DMA before that.
> 
> Thank you
> Yao Jiewen
> 
> > -Original Message-
> > From: Zeng, Star
> > Sent: Thursday, January 4, 2018 11:32 AM
> > To: edk2-devel@lists.01.org
> > Cc: Zeng, Star ; Yao, Jiewen 
> > 
> > Subject: [PATCH] IntelSiliconPkg IntelVTdDxe: Use TPL to protect 
> > list/engine operation
> >
> > Cc: Jiewen Yao 
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Star Zeng 
> > ---
> >  IntelSiliconPkg/Feature/VTd/IntelVTdDxe/BmDma.c| 19 ++---
> >  .../Feature/VTd/IntelVTdDxe/DmaProtection.c|  8 +++---
> >  .../Feature/VTd/IntelVTdDxe/DmaProtection.h|  2 ++
> >  .../Feature/VTd/IntelVTdDxe/IntelVTdDxe.c  | 32
> > +-
> >  4 files changed, 28 insertions(+), 33 deletions(-)
> >
> > diff --git a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/BmDma.c
> > b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/BmDma.c
> > index e8685666e79a..57e086a64dbc 100644
> > --- a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/BmDma.c
> > +++ b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/BmDma.c
> > @@ -1,7 +1,7 @@
> >  /** @file
> >BmDma related function
> >
> > -  Copyright (c) 2017, Intel Corporation. All rights reserved.
> > +  Copyright (c) 2017 - 2018, Intel Corporation. All rights 
> > + reserved.
> >This program and the accompanying materials
> >are licensed and made available under the terms and conditions of 
> > the BSD License
> >which accompanies this distribution.  The full text of the 
> > license may be found at @@ -12,15 +12,7 @@
> >
> >  **/
> >
> > -#include 
> > -
> > -#include 
> > -
> > -#include 
> > -#include 
> > -#include 
> > -#include  -#include 
> > 
> > +#include "DmaProtection.h"
> >
> >  // TBD: May make it a policy
> >  #define DMA_MEMORY_TOP  MAX_UINTN
> > @@ -76,6 +68,7 @@ IoMmuMap (
> >MAP_INFO  *MapInfo;
> >EFI_PHYSICAL_ADDRESS
> > DmaMemoryTop;
> >BOOLEAN   NeedRemap;
> > +  EFI_TPL   OriginalTpl;
> >
> >if (NumberOfBytes == NULL || DeviceAddress == NULL ||
> >Mapping == NULL) {
> > @@ -198,7 +191,9 @@ IoMmuMap (
> >  MapInfo->DeviceAddress = MapInfo->HostAddress;
> >}
> >
> > +  OriginalTpl = gBS->RaiseTPL (VTD_TPL_LEVEL);
> >InsertTailList (, >Link);
> > +  gBS->RestoreTPL (OriginalTpl);
> >
> >//
> >// The DeviceAddress is the address of the maped buffer below 4GB 
> > @@ -233,6 +228,7 @@ IoMmuUnmap (  {
> >MAP_INFO *MapInfo;
> >LIST_ENTRY   *Link;
> > +  EFI_TPL  OriginalTpl;
> >
> >DEBUG ((DEBUG_VERBOSE, "IoMmuUnmap: 0x%08x\n", Mapping));
> >
> > @@ -241,6 +237,7 @@ IoMmuUnmap (
> >  return EFI_INVALID_PARAMETER;
> >}
> >
> > +  OriginalTpl = gBS->RaiseTPL (VTD_TPL_LEVEL);
> >MapInfo = NULL;
> >for (Link = GetFirstNode ()
> > ; !IsNull (, Link)
> > @@ -255,10 +252,12 @@ IoMmuUnmap (
> >// Mapping is not a valid value returned by Map()
> >//
> >if (MapInfo != Mapping) {
> > +gBS->RestoreTPL (OriginalTpl);
> >  DEBUG ((DEBUG_ERROR, "IoMmuUnmap: %r\n", 
> > EFI_INVALID_PARAMETER));
> >  return EFI_INVALID_PARAMETER;
> >}
> >RemoveEntryList (>Link);
> > +  gBS->RestoreTPL (OriginalTpl);
> >
> >if (MapInfo->DeviceAddress != MapInfo->HostAddress) {
> >  //
> > diff --git a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.c
> > b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.c
> > index 648f64c20b77..013823cc161f 100644
> > --- a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.c
> > +++ b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.c
> > @@ -478,7 +478,7 @@ InitializeDmaProtection (
> >
> >Status = gBS->CreateEventEx (
> >EVT_NOTIFY_SIGNAL,
> > -  TPL_CALLBACK,
> > +  VTD_TPL_LEVEL,
> >

Re: [edk2] [PATCH] IntelSiliconPkg IntelVTdDxe: Use TPL to protect list/engine operation

[Yao, Jiewen]

Correct typo.

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Yao,
> Jiewen
> Sent: Thursday, January 4, 2018 2:14 PM
> To: Zeng, Star ; edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH] IntelSiliconPkg IntelVTdDxe: Use TPL to protect
> list/engine operation
> 
> It is good to have lock for linked list management.
> 
> However, I do not think we should update TPL for ExitBootServices/LegacyBoot.
> 
> I purposely use TPL_CALLBACK to make sure VTd is tear down later, so that 
> other
> driver can stop DMA before that.
> 
> Thank you
> Yao Jiewen
> 
> > -Original Message-
> > From: Zeng, Star
> > Sent: Thursday, January 4, 2018 11:32 AM
> > To: edk2-devel@lists.01.org
> > Cc: Zeng, Star ; Yao, Jiewen 
> > Subject: [PATCH] IntelSiliconPkg IntelVTdDxe: Use TPL to protect list/engine
> > operation
> >
> > Cc: Jiewen Yao 
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Star Zeng 
> > ---
> >  IntelSiliconPkg/Feature/VTd/IntelVTdDxe/BmDma.c| 19 ++---
> >  .../Feature/VTd/IntelVTdDxe/DmaProtection.c|  8 +++---
> >  .../Feature/VTd/IntelVTdDxe/DmaProtection.h|  2 ++
> >  .../Feature/VTd/IntelVTdDxe/IntelVTdDxe.c  | 32
> > +-
> >  4 files changed, 28 insertions(+), 33 deletions(-)
> >
> > diff --git a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/BmDma.c
> > b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/BmDma.c
> > index e8685666e79a..57e086a64dbc 100644
> > --- a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/BmDma.c
> > +++ b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/BmDma.c
> > @@ -1,7 +1,7 @@
> >  /** @file
> >BmDma related function
> >
> > -  Copyright (c) 2017, Intel Corporation. All rights reserved.
> > +  Copyright (c) 2017 - 2018, Intel Corporation. All rights reserved.
> >This program and the accompanying materials
> >are licensed and made available under the terms and conditions of the BSD
> > License
> >which accompanies this distribution.  The full text of the license may be
> > found at
> > @@ -12,15 +12,7 @@
> >
> >  **/
> >
> > -#include 
> > -
> > -#include 
> > -
> > -#include 
> > -#include 
> > -#include 
> > -#include 
> > -#include 
> > +#include "DmaProtection.h"
> >
> >  // TBD: May make it a policy
> >  #define DMA_MEMORY_TOP  MAX_UINTN
> > @@ -76,6 +68,7 @@ IoMmuMap (
> >MAP_INFO  *MapInfo;
> >EFI_PHYSICAL_ADDRESS
> > DmaMemoryTop;
> >BOOLEAN   NeedRemap;
> > +  EFI_TPL   OriginalTpl;
> >
> >if (NumberOfBytes == NULL || DeviceAddress == NULL ||
> >Mapping == NULL) {
> > @@ -198,7 +191,9 @@ IoMmuMap (
> >  MapInfo->DeviceAddress = MapInfo->HostAddress;
> >}
> >
> > +  OriginalTpl = gBS->RaiseTPL (VTD_TPL_LEVEL);
> >InsertTailList (, >Link);
> > +  gBS->RestoreTPL (OriginalTpl);
> >
> >//
> >// The DeviceAddress is the address of the maped buffer below 4GB
> > @@ -233,6 +228,7 @@ IoMmuUnmap (
> >  {
> >MAP_INFO *MapInfo;
> >LIST_ENTRY   *Link;
> > +  EFI_TPL  OriginalTpl;
> >
> >DEBUG ((DEBUG_VERBOSE, "IoMmuUnmap: 0x%08x\n", Mapping));
> >
> > @@ -241,6 +237,7 @@ IoMmuUnmap (
> >  return EFI_INVALID_PARAMETER;
> >}
> >
> > +  OriginalTpl = gBS->RaiseTPL (VTD_TPL_LEVEL);
> >MapInfo = NULL;
> >for (Link = GetFirstNode ()
> > ; !IsNull (, Link)
> > @@ -255,10 +252,12 @@ IoMmuUnmap (
> >// Mapping is not a valid value returned by Map()
> >//
> >if (MapInfo != Mapping) {
> > +gBS->RestoreTPL (OriginalTpl);
> >  DEBUG ((DEBUG_ERROR, "IoMmuUnmap: %r\n",
> > EFI_INVALID_PARAMETER));
> >  return EFI_INVALID_PARAMETER;
> >}
> >RemoveEntryList (>Link);
> > +  gBS->RestoreTPL (OriginalTpl);
> >
> >if (MapInfo->DeviceAddress != MapInfo->HostAddress) {
> >  //
> > diff --git a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.c
> > b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.c
> > index 648f64c20b77..013823cc161f 100644
> > --- a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.c
> > +++ b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.c
> > @@ -478,7 +478,7 @@ InitializeDmaProtection (
> >
> >Status = gBS->CreateEventEx (
> >EVT_NOTIFY_SIGNAL,
> > -  TPL_CALLBACK,
> > +  VTD_TPL_LEVEL,
> >AcpiNotificationFunc,
> >NULL,
> >,
> > @@ -488,7 +488,7 @@ InitializeDmaProtection (
> >
> >Status = gBS->CreateEventEx (
> >EVT_NOTIFY_SIGNAL,
> > -  TPL_CALLBACK,
> > +  VTD_TPL_LEVEL,
> >AcpiNotificationFunc,
> >NULL,
> >,

Re: [edk2] [Patch] MdeModulePkg/DxeUdpIoLib: Did some code enhancement for UdpIoLib

[Fu, Siyuan]

Hi, Fan

+  If Udp version is not Udp4 or Udp6, then ASSERT().
It's better to update "Udp4" and "Udp6" to " UDP_IO_UDP4_VERSION" and " 
UDP_IO_UDP6_VERSION"

Other parts are good with me.


Reviewed-by: Fu Siyuan 


> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> fanwang2
> Sent: Thursday, January 4, 2018 2:02 PM
> To: edk2-devel@lists.01.org
> Cc: Ye, Ting ; Wang, Fan ; Fu,
> Siyuan ; Wu, Jiaxin 
> Subject: [edk2] [Patch] MdeModulePkg/DxeUdpIoLib: Did some code
> enhancement for UdpIoLib
> 
> From: Wang Fan 
> 
> * Added some ASSERT descriptions for library APIs.
> * Added "Optional" option for Context parameter in UdpIoCancelDgrams().
> * Added function return status check for UdpIoFreeIo().
> 
> Cc: Ye Ting 
> Cc: Fu Siyuan 
> Cc: Jiaxin Wu 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Wang Fan 
> ---
>  MdeModulePkg/Include/Library/UdpIoLib.h| 16 -
>  MdeModulePkg/Library/DxeUdpIoLib/DxeUdpIoLib.c | 90 ++---
> -
>  2 files changed, 78 insertions(+), 28 deletions(-)
> 
> diff --git a/MdeModulePkg/Include/Library/UdpIoLib.h
> b/MdeModulePkg/Include/Library/UdpIoLib.h
> index e0b44ce..86a11a9 100644
> --- a/MdeModulePkg/Include/Library/UdpIoLib.h
> +++ b/MdeModulePkg/Include/Library/UdpIoLib.h
> @@ -195,11 +195,13 @@ BOOLEAN
>IN VOID   *Context
>);
> 
>  /**
>Cancel all the sent datagram that pass the selection criteria of
> ToCancel.
> +
>If ToCancel is NULL, all the datagrams are cancelled.
> +  If Udp version is not Udp4 or Udp6, then ASSERT().
> 
>@param[in]  UdpIo The UDP_IO to cancel packet.
>@param[in]  IoStatus  The IoStatus to return to the packet
> owners.
>@param[in]  ToCancel  The select funtion to test whether to
> cancel this
>  packet or not.
> @@ -210,17 +212,20 @@ VOID
>  EFIAPI
>  UdpIoCancelDgrams (
>IN UDP_IO *UdpIo,
>IN EFI_STATUS IoStatus,
>IN UDP_IO_TO_CANCEL   ToCancel,OPTIONAL
> -  IN VOID   *Context
> +  IN VOID   *Context OPTIONAL
>);
> 
>  /**
>Creates a UDP_IO to access the UDP service. It creates and configures
>a UDP child.
> 
> +  If Configure is NULL, then ASSERT().
> +  If Udp version is not Udp4 or Udp6, then ASSERT().
> +
>It locates the UDP service binding prototype on the Controller
> parameter
>uses the UDP service binding prototype to create a UDP child (also
> known as
>a UDP instance) configures the UDP child by calling Configure function
> prototype.
>Any failures in creating or configuring the UDP child return NULL for
> failure.
> 
> @@ -245,15 +250,18 @@ UdpIoCreateIo (
>);
> 
>  /**
>Free the UDP_IO and all its related resources.
> 
> +  If Udp version is not Udp4 or Udp6, then ASSERT().
> +
>The function cancels all sent datagrams and receive requests.
> 
>@param[in]  UdpIo The UDP_IO to free.
> 
>@retval EFI_SUCCESS   The UDP_IO is freed.
> +  @retval OthersFailed to free UDP_IO.
> 
>  **/
>  EFI_STATUS
>  EFIAPI
>  UdpIoFreeIo (
> @@ -262,10 +270,12 @@ UdpIoFreeIo (
> 
>  /**
>Cleans up the UDP_IO without freeing it. Call this function
>if you intend to later re-use the UDP_IO.
> 
> +  If Udp version is not Udp4 or Udp6, then ASSERT().
> +
>This function releases all transmitted datagrams and receive requests
> and configures NULL for the UDP instance.
> 
>@param[in]  UdpIo The UDP_IO to clean up.
> 
>  **/
> @@ -276,10 +286,12 @@ UdpIoCleanIo (
>);
> 
>  /**
>Send a packet through the UDP_IO.
> 
> +  If Udp version is not Udp4 or Udp6, then ASSERT().
> +
>The packet will be wrapped in UDP_TX_TOKEN. Function Callback will be
> called
>when the packet is sent. The optional parameter EndPoint overrides the
> default
>address pair if specified.
> 
>@param[in]  UdpIo The UDP_IO to send the packet through.
> @@ -322,10 +334,12 @@ UdpIoCancelSentDatagram (
>);
> 
>  /**
>Issue a receive request to the UDP_IO.
> 
> +  If Udp version is not Udp4 or Udp6, then ASSERT().
> +
>This function is called when upper-layer needs packet from UDP for
> processing.
>Only one receive request is acceptable at a time. Therefore, one common
> usage model is
>to invoke this function inside its Callback function when the former
> packet
>is processed.
> 
> diff --git a/MdeModulePkg/Library/DxeUdpIoLib/DxeUdpIoLib.c
> b/MdeModulePkg/Library/DxeUdpIoLib/DxeUdpIoLib.c
> index 4861095..e2d3466 100644
> --- a/MdeModulePkg/Library/DxeUdpIoLib/DxeUdpIoLib.c
> +++ 

Re: [edk2] [PATCH] IntelSiliconPkg IntelVTdDxe: Support early SetAttributes()

[Yao, Jiewen]

Reviewed-by: jiewen@intel.com

> -Original Message-
> From: Zeng, Star
> Sent: Thursday, January 4, 2018 11:34 AM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star ; Yao, Jiewen 
> Subject: [PATCH] IntelSiliconPkg IntelVTdDxe: Support early SetAttributes()
> 
> Support early SetAttributes() before DMAR table is installed.
> 
> Cc: Jiewen Yao 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Star Zeng 
> ---
>  .../Feature/VTd/IntelVTdDxe/DmaProtection.c| 147
> +
>  .../Feature/VTd/IntelVTdDxe/DmaProtection.h|  46 ++-
>  .../Feature/VTd/IntelVTdDxe/DmarAcpiTable.c|   2 +-
>  .../Feature/VTd/IntelVTdDxe/IntelVTdDxe.c  |  10 +-
>  4 files changed, 202 insertions(+), 3 deletions(-)
> 
> diff --git a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.c
> b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.c
> index 013823cc161f..665afe71703d 100644
> --- a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.c
> +++ b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.c
> @@ -18,6 +18,151 @@ UINT64
> mAbove4GMemoryLimit;
> 
>  EDKII_PLATFORM_VTD_POLICY_PROTOCOL  *mPlatformVTdPolicy;
> 
> +VTD_ACCESS_REQUEST  *mAccessRequest = NULL;
> +UINTN   mAccessRequestCount = 0;
> +UINTN   mAccessRequestMaxCount = 0;
> +
> +/**
> +  Append VTd Access Request to global.
> +
> +  @param[in]  Segment   The Segment used to identify a VTd
> engine.
> +  @param[in]  SourceId  The SourceId used to identify a VTd
> engine and table entry.
> +  @param[in]  BaseAddress   The base of device memory address to be
> used as the DMA memory.
> +  @param[in]  LengthThe length of device memory address to
> be used as the DMA memory.
> +  @param[in]  IoMmuAccess   The IOMMU access.
> +
> +  @retval EFI_SUCCESS   The IoMmuAccess is set for the memory
> range specified by BaseAddress and Length.
> +  @retval EFI_INVALID_PARAMETER BaseAddress is not IoMmu Page size
> aligned.
> +  @retval EFI_INVALID_PARAMETER Length is not IoMmu Page size aligned.
> +  @retval EFI_INVALID_PARAMETER Length is 0.
> +  @retval EFI_INVALID_PARAMETER IoMmuAccess specified an illegal
> combination of access.
> +  @retval EFI_UNSUPPORTED   The bit mask of IoMmuAccess is not
> supported by the IOMMU.
> +  @retval EFI_UNSUPPORTED   The IOMMU does not support the
> memory range specified by BaseAddress and Length.
> +  @retval EFI_OUT_OF_RESOURCES  There are not enough resources
> available to modify the IOMMU access.
> +  @retval EFI_DEVICE_ERROR  The IOMMU device reported an error
> while attempting the operation.
> +
> +**/
> +EFI_STATUS
> +RequestAccessAttribute (
> +  IN UINT16 Segment,
> +  IN VTD_SOURCE_ID  SourceId,
> +  IN UINT64 BaseAddress,
> +  IN UINT64 Length,
> +  IN UINT64 IoMmuAccess
> +  )
> +{
> +  VTD_ACCESS_REQUEST*NewAccessRequest;
> +  UINTN Index;
> +
> +  //
> +  // Optimization for memory.
> +  //
> +  // If the last record is to IoMmuAccess=0,
> +  // Check previous records and remove the matched entry.
> +  //
> +  if (IoMmuAccess == 0) {
> +for (Index = 0; Index < mAccessRequestCount; Index++) {
> +  if ((mAccessRequest[Index].Segment == Segment) &&
> +  (mAccessRequest[Index].SourceId.Uint16 == SourceId.Uint16) &&
> +  (mAccessRequest[Index].BaseAddress == BaseAddress) &&
> +  (mAccessRequest[Index].Length == Length) &&
> +  (mAccessRequest[Index].IoMmuAccess != 0)) {
> +//
> +// Remove this record [Index].
> +// No need to add the new record.
> +//
> +if (Index != mAccessRequestCount - 1) {
> +  CopyMem (
> +[Index],
> +[Index + 1],
> +sizeof (VTD_ACCESS_REQUEST) * (mAccessRequestCount - 1 -
> Index)
> +);
> +}
> +ZeroMem ([mAccessRequestCount - 1],
> sizeof(VTD_ACCESS_REQUEST));
> +mAccessRequestCount--;
> +return EFI_SUCCESS;
> +  }
> +}
> +  }
> +
> +  if (mAccessRequestCount >= mAccessRequestMaxCount) {
> +NewAccessRequest = AllocateZeroPool (sizeof(*NewAccessRequest) *
> (mAccessRequestMaxCount + MAX_VTD_ACCESS_REQUEST));
> +if (NewAccessRequest == NULL) {
> +  return EFI_OUT_OF_RESOURCES;
> +}
> +mAccessRequestMaxCount += MAX_VTD_ACCESS_REQUEST;
> +if (mAccessRequest != NULL) {
> +  CopyMem (NewAccessRequest, mAccessRequest,
> sizeof(*NewAccessRequest) * mAccessRequestCount);
> +  FreePool (mAccessRequest);
> +  mAccessRequest = NewAccessRequest;
> +}
> +  }
> +
> +  ASSERT (mAccessRequestCount < mAccessRequestMaxCount);
> +
> +  mAccessRequest[mAccessRequestCount].Segment = Segment;
> +  

Re: [edk2] [PATCH] IntelSiliconPkg IntelVTdDxe: Use TPL to protect list/engine operation

[Yao, Jiewen]

It is good to have lock for linked list management.

However, I do not think we should update TPM for ExitBootServices/LegacyBoot.

I purposely use TPL_CALLBACK to make sure VTd is tear down later, so that other 
driver can stop DMA before that.

Thank you
Yao Jiewen

> -Original Message-
> From: Zeng, Star
> Sent: Thursday, January 4, 2018 11:32 AM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star ; Yao, Jiewen 
> Subject: [PATCH] IntelSiliconPkg IntelVTdDxe: Use TPL to protect list/engine
> operation
> 
> Cc: Jiewen Yao 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Star Zeng 
> ---
>  IntelSiliconPkg/Feature/VTd/IntelVTdDxe/BmDma.c| 19 ++---
>  .../Feature/VTd/IntelVTdDxe/DmaProtection.c|  8 +++---
>  .../Feature/VTd/IntelVTdDxe/DmaProtection.h|  2 ++
>  .../Feature/VTd/IntelVTdDxe/IntelVTdDxe.c  | 32
> +-
>  4 files changed, 28 insertions(+), 33 deletions(-)
> 
> diff --git a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/BmDma.c
> b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/BmDma.c
> index e8685666e79a..57e086a64dbc 100644
> --- a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/BmDma.c
> +++ b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/BmDma.c
> @@ -1,7 +1,7 @@
>  /** @file
>BmDma related function
> 
> -  Copyright (c) 2017, Intel Corporation. All rights reserved.
> +  Copyright (c) 2017 - 2018, Intel Corporation. All rights reserved.
>This program and the accompanying materials
>are licensed and made available under the terms and conditions of the BSD
> License
>which accompanies this distribution.  The full text of the license may be
> found at
> @@ -12,15 +12,7 @@
> 
>  **/
> 
> -#include 
> -
> -#include 
> -
> -#include 
> -#include 
> -#include 
> -#include 
> -#include 
> +#include "DmaProtection.h"
> 
>  // TBD: May make it a policy
>  #define DMA_MEMORY_TOP  MAX_UINTN
> @@ -76,6 +68,7 @@ IoMmuMap (
>MAP_INFO  *MapInfo;
>EFI_PHYSICAL_ADDRESS
> DmaMemoryTop;
>BOOLEAN   NeedRemap;
> +  EFI_TPL   OriginalTpl;
> 
>if (NumberOfBytes == NULL || DeviceAddress == NULL ||
>Mapping == NULL) {
> @@ -198,7 +191,9 @@ IoMmuMap (
>  MapInfo->DeviceAddress = MapInfo->HostAddress;
>}
> 
> +  OriginalTpl = gBS->RaiseTPL (VTD_TPL_LEVEL);
>InsertTailList (, >Link);
> +  gBS->RestoreTPL (OriginalTpl);
> 
>//
>// The DeviceAddress is the address of the maped buffer below 4GB
> @@ -233,6 +228,7 @@ IoMmuUnmap (
>  {
>MAP_INFO *MapInfo;
>LIST_ENTRY   *Link;
> +  EFI_TPL  OriginalTpl;
> 
>DEBUG ((DEBUG_VERBOSE, "IoMmuUnmap: 0x%08x\n", Mapping));
> 
> @@ -241,6 +237,7 @@ IoMmuUnmap (
>  return EFI_INVALID_PARAMETER;
>}
> 
> +  OriginalTpl = gBS->RaiseTPL (VTD_TPL_LEVEL);
>MapInfo = NULL;
>for (Link = GetFirstNode ()
> ; !IsNull (, Link)
> @@ -255,10 +252,12 @@ IoMmuUnmap (
>// Mapping is not a valid value returned by Map()
>//
>if (MapInfo != Mapping) {
> +gBS->RestoreTPL (OriginalTpl);
>  DEBUG ((DEBUG_ERROR, "IoMmuUnmap: %r\n",
> EFI_INVALID_PARAMETER));
>  return EFI_INVALID_PARAMETER;
>}
>RemoveEntryList (>Link);
> +  gBS->RestoreTPL (OriginalTpl);
> 
>if (MapInfo->DeviceAddress != MapInfo->HostAddress) {
>  //
> diff --git a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.c
> b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.c
> index 648f64c20b77..013823cc161f 100644
> --- a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.c
> +++ b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.c
> @@ -478,7 +478,7 @@ InitializeDmaProtection (
> 
>Status = gBS->CreateEventEx (
>EVT_NOTIFY_SIGNAL,
> -  TPL_CALLBACK,
> +  VTD_TPL_LEVEL,
>AcpiNotificationFunc,
>NULL,
>,
> @@ -488,7 +488,7 @@ InitializeDmaProtection (
> 
>Status = gBS->CreateEventEx (
>EVT_NOTIFY_SIGNAL,
> -  TPL_CALLBACK,
> +  VTD_TPL_LEVEL,
>AcpiNotificationFunc,
>NULL,
>,
> @@ -505,7 +505,7 @@ InitializeDmaProtection (
> 
>Status = gBS->CreateEventEx (
>EVT_NOTIFY_SIGNAL,
> -  TPL_CALLBACK,
> +  VTD_TPL_LEVEL,
>OnExitBootServices,
>NULL,
>,
> @@ -514,7 +514,7 @@ InitializeDmaProtection (
>ASSERT_EFI_ERROR (Status);
> 
>Status = EfiCreateEventLegacyBootEx (
> - TPL_CALLBACK,
> + VTD_TPL_LEVEL,
>   OnLegacyBoot,
>   NULL,
>   
> diff --git 

[edk2] [Patch] MdeModulePkg/DxeUdpIoLib: Did some code enhancement for UdpIoLib

[fanwang2]

From: Wang Fan 

* Added some ASSERT descriptions for library APIs.
* Added "Optional" option for Context parameter in UdpIoCancelDgrams().
* Added function return status check for UdpIoFreeIo().

Cc: Ye Ting 
Cc: Fu Siyuan 
Cc: Jiaxin Wu 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Wang Fan 
---
 MdeModulePkg/Include/Library/UdpIoLib.h| 16 -
 MdeModulePkg/Library/DxeUdpIoLib/DxeUdpIoLib.c | 90 ++
 2 files changed, 78 insertions(+), 28 deletions(-)

diff --git a/MdeModulePkg/Include/Library/UdpIoLib.h 
b/MdeModulePkg/Include/Library/UdpIoLib.h
index e0b44ce..86a11a9 100644
--- a/MdeModulePkg/Include/Library/UdpIoLib.h
+++ b/MdeModulePkg/Include/Library/UdpIoLib.h
@@ -195,11 +195,13 @@ BOOLEAN
   IN VOID   *Context
   );
 
 /**
   Cancel all the sent datagram that pass the selection criteria of ToCancel.
+
   If ToCancel is NULL, all the datagrams are cancelled.
+  If Udp version is not Udp4 or Udp6, then ASSERT().
 
   @param[in]  UdpIo The UDP_IO to cancel packet.
   @param[in]  IoStatus  The IoStatus to return to the packet 
owners.
   @param[in]  ToCancel  The select funtion to test whether to 
cancel this
 packet or not.
@@ -210,17 +212,20 @@ VOID
 EFIAPI
 UdpIoCancelDgrams (
   IN UDP_IO *UdpIo,
   IN EFI_STATUS IoStatus,
   IN UDP_IO_TO_CANCEL   ToCancel,OPTIONAL
-  IN VOID   *Context
+  IN VOID   *Context OPTIONAL
   );
 
 /**
   Creates a UDP_IO to access the UDP service. It creates and configures
   a UDP child.
 
+  If Configure is NULL, then ASSERT().
+  If Udp version is not Udp4 or Udp6, then ASSERT().
+
   It locates the UDP service binding prototype on the Controller parameter
   uses the UDP service binding prototype to create a UDP child (also known as
   a UDP instance) configures the UDP child by calling Configure function 
prototype.
   Any failures in creating or configuring the UDP child return NULL for 
failure.
 
@@ -245,15 +250,18 @@ UdpIoCreateIo (
   );
 
 /**
   Free the UDP_IO and all its related resources.
 
+  If Udp version is not Udp4 or Udp6, then ASSERT().
+
   The function cancels all sent datagrams and receive requests.
 
   @param[in]  UdpIo The UDP_IO to free.
 
   @retval EFI_SUCCESS   The UDP_IO is freed.
+  @retval OthersFailed to free UDP_IO.
 
 **/
 EFI_STATUS
 EFIAPI
 UdpIoFreeIo (
@@ -262,10 +270,12 @@ UdpIoFreeIo (
 
 /**
   Cleans up the UDP_IO without freeing it. Call this function
   if you intend to later re-use the UDP_IO.
 
+  If Udp version is not Udp4 or Udp6, then ASSERT().
+
   This function releases all transmitted datagrams and receive requests and 
configures NULL for the UDP instance.
 
   @param[in]  UdpIo The UDP_IO to clean up.
 
 **/
@@ -276,10 +286,12 @@ UdpIoCleanIo (
   );
 
 /**
   Send a packet through the UDP_IO.
 
+  If Udp version is not Udp4 or Udp6, then ASSERT().
+
   The packet will be wrapped in UDP_TX_TOKEN. Function Callback will be called
   when the packet is sent. The optional parameter EndPoint overrides the 
default
   address pair if specified.
 
   @param[in]  UdpIo The UDP_IO to send the packet through.
@@ -322,10 +334,12 @@ UdpIoCancelSentDatagram (
   );
 
 /**
   Issue a receive request to the UDP_IO.
 
+  If Udp version is not Udp4 or Udp6, then ASSERT().
+
   This function is called when upper-layer needs packet from UDP for 
processing.
   Only one receive request is acceptable at a time. Therefore, one common 
usage model is
   to invoke this function inside its Callback function when the former packet
   is processed.
 
diff --git a/MdeModulePkg/Library/DxeUdpIoLib/DxeUdpIoLib.c 
b/MdeModulePkg/Library/DxeUdpIoLib/DxeUdpIoLib.c
index 4861095..e2d3466 100644
--- a/MdeModulePkg/Library/DxeUdpIoLib/DxeUdpIoLib.c
+++ b/MdeModulePkg/Library/DxeUdpIoLib/DxeUdpIoLib.c
@@ -408,10 +408,13 @@ UdpIoCreateRxToken (
 }
 
 /**
   Wrap a transmit request into a new created UDP_TX_TOKEN.
 
+  If Packet is NULL, then ASSERT().
+  If Udp version is not Udp4 or Udp6, then ASSERT().
+
   @param[in]  UdpIo The UdpIo to send packet to.
   @param[in]  PacketThe user's packet.
   @param[in]  EndPoint  The local and remote access point.
   @param[in]  Gateway   The overrided next hop.
   @param[in]  CallBack  The function to call when transmission 
completed.
@@ -578,10 +581,13 @@ UdpIoCreateTxToken (
 
 /**
   Creates a UDP_IO to access the UDP service. It creates and configures
   a UDP child.
 
+  If Configure is NULL, then ASSERT().
+  If Udp version is not Udp4 or Udp6, then ASSERT().
+
   It locates the UDP service binding prototype on the Controller parameter
   

[edk2] [PATCH] IntelSiliconPkg IntelVTdDxe: Support early SetAttributes()

[Star Zeng]

Support early SetAttributes() before DMAR table is installed.

Cc: Jiewen Yao 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Star Zeng 
---
 .../Feature/VTd/IntelVTdDxe/DmaProtection.c| 147 +
 .../Feature/VTd/IntelVTdDxe/DmaProtection.h|  46 ++-
 .../Feature/VTd/IntelVTdDxe/DmarAcpiTable.c|   2 +-
 .../Feature/VTd/IntelVTdDxe/IntelVTdDxe.c  |  10 +-
 4 files changed, 202 insertions(+), 3 deletions(-)

diff --git a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.c 
b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.c
index 013823cc161f..665afe71703d 100644
--- a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.c
+++ b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.c
@@ -18,6 +18,151 @@ UINT64  mAbove4GMemoryLimit;
 
 EDKII_PLATFORM_VTD_POLICY_PROTOCOL  *mPlatformVTdPolicy;
 
+VTD_ACCESS_REQUEST  *mAccessRequest = NULL;
+UINTN   mAccessRequestCount = 0;
+UINTN   mAccessRequestMaxCount = 0;
+
+/**
+  Append VTd Access Request to global.
+
+  @param[in]  Segment   The Segment used to identify a VTd engine.
+  @param[in]  SourceId  The SourceId used to identify a VTd engine and 
table entry.
+  @param[in]  BaseAddress   The base of device memory address to be used 
as the DMA memory.
+  @param[in]  LengthThe length of device memory address to be used 
as the DMA memory.
+  @param[in]  IoMmuAccess   The IOMMU access.
+
+  @retval EFI_SUCCESS   The IoMmuAccess is set for the memory range 
specified by BaseAddress and Length.
+  @retval EFI_INVALID_PARAMETER BaseAddress is not IoMmu Page size aligned.
+  @retval EFI_INVALID_PARAMETER Length is not IoMmu Page size aligned.
+  @retval EFI_INVALID_PARAMETER Length is 0.
+  @retval EFI_INVALID_PARAMETER IoMmuAccess specified an illegal combination 
of access.
+  @retval EFI_UNSUPPORTED   The bit mask of IoMmuAccess is not supported 
by the IOMMU.
+  @retval EFI_UNSUPPORTED   The IOMMU does not support the memory range 
specified by BaseAddress and Length.
+  @retval EFI_OUT_OF_RESOURCES  There are not enough resources available to 
modify the IOMMU access.
+  @retval EFI_DEVICE_ERROR  The IOMMU device reported an error while 
attempting the operation.
+
+**/
+EFI_STATUS
+RequestAccessAttribute (
+  IN UINT16 Segment,
+  IN VTD_SOURCE_ID  SourceId,
+  IN UINT64 BaseAddress,
+  IN UINT64 Length,
+  IN UINT64 IoMmuAccess
+  )
+{
+  VTD_ACCESS_REQUEST*NewAccessRequest;
+  UINTN Index;
+
+  //
+  // Optimization for memory.
+  //
+  // If the last record is to IoMmuAccess=0,
+  // Check previous records and remove the matched entry.
+  //
+  if (IoMmuAccess == 0) {
+for (Index = 0; Index < mAccessRequestCount; Index++) {
+  if ((mAccessRequest[Index].Segment == Segment) &&
+  (mAccessRequest[Index].SourceId.Uint16 == SourceId.Uint16) &&
+  (mAccessRequest[Index].BaseAddress == BaseAddress) &&
+  (mAccessRequest[Index].Length == Length) &&
+  (mAccessRequest[Index].IoMmuAccess != 0)) {
+//
+// Remove this record [Index].
+// No need to add the new record.
+//
+if (Index != mAccessRequestCount - 1) {
+  CopyMem (
+[Index],
+[Index + 1],
+sizeof (VTD_ACCESS_REQUEST) * (mAccessRequestCount - 1 - Index)
+);
+}
+ZeroMem ([mAccessRequestCount - 1], 
sizeof(VTD_ACCESS_REQUEST));
+mAccessRequestCount--;
+return EFI_SUCCESS;
+  }
+}
+  }
+
+  if (mAccessRequestCount >= mAccessRequestMaxCount) {
+NewAccessRequest = AllocateZeroPool (sizeof(*NewAccessRequest) * 
(mAccessRequestMaxCount + MAX_VTD_ACCESS_REQUEST));
+if (NewAccessRequest == NULL) {
+  return EFI_OUT_OF_RESOURCES;
+}
+mAccessRequestMaxCount += MAX_VTD_ACCESS_REQUEST;
+if (mAccessRequest != NULL) {
+  CopyMem (NewAccessRequest, mAccessRequest, sizeof(*NewAccessRequest) * 
mAccessRequestCount);
+  FreePool (mAccessRequest);
+  mAccessRequest = NewAccessRequest;
+}
+  }
+
+  ASSERT (mAccessRequestCount < mAccessRequestMaxCount);
+
+  mAccessRequest[mAccessRequestCount].Segment = Segment;
+  mAccessRequest[mAccessRequestCount].SourceId = SourceId;
+  mAccessRequest[mAccessRequestCount].BaseAddress = BaseAddress;
+  mAccessRequest[mAccessRequestCount].Length = Length;
+  mAccessRequest[mAccessRequestCount].IoMmuAccess = IoMmuAccess;
+
+  mAccessRequestCount++;
+
+  return EFI_SUCCESS;
+}
+
+/**
+  Process Access Requests from before DMAR table is installed.
+
+**/
+VOID
+ProcessRequestedAccessAttribute (
+  VOID
+  )
+{
+  UINTN   Index;
+  EFI_STATUS  Status;
+
+  DEBUG ((DEBUG_INFO, 

[edk2] [PATCH] IntelSiliconPkg IntelVTdDxe: Use TPL to protect list/engine operation

[Star Zeng]

Cc: Jiewen Yao 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Star Zeng 
---
 IntelSiliconPkg/Feature/VTd/IntelVTdDxe/BmDma.c| 19 ++---
 .../Feature/VTd/IntelVTdDxe/DmaProtection.c|  8 +++---
 .../Feature/VTd/IntelVTdDxe/DmaProtection.h|  2 ++
 .../Feature/VTd/IntelVTdDxe/IntelVTdDxe.c  | 32 +-
 4 files changed, 28 insertions(+), 33 deletions(-)

diff --git a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/BmDma.c 
b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/BmDma.c
index e8685666e79a..57e086a64dbc 100644
--- a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/BmDma.c
+++ b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/BmDma.c
@@ -1,7 +1,7 @@
 /** @file
   BmDma related function
 
-  Copyright (c) 2017, Intel Corporation. All rights reserved.
+  Copyright (c) 2017 - 2018, Intel Corporation. All rights reserved.
   This program and the accompanying materials
   are licensed and made available under the terms and conditions of the BSD 
License
   which accompanies this distribution.  The full text of the license may be 
found at
@@ -12,15 +12,7 @@
 
 **/
 
-#include 
-
-#include 
-
-#include 
-#include 
-#include 
-#include 
-#include 
+#include "DmaProtection.h"
 
 // TBD: May make it a policy
 #define DMA_MEMORY_TOP  MAX_UINTN
@@ -76,6 +68,7 @@ IoMmuMap (
   MAP_INFO  *MapInfo;
   EFI_PHYSICAL_ADDRESS  DmaMemoryTop;
   BOOLEAN   NeedRemap;
+  EFI_TPL   OriginalTpl;
 
   if (NumberOfBytes == NULL || DeviceAddress == NULL ||
   Mapping == NULL) {
@@ -198,7 +191,9 @@ IoMmuMap (
 MapInfo->DeviceAddress = MapInfo->HostAddress;
   }
 
+  OriginalTpl = gBS->RaiseTPL (VTD_TPL_LEVEL);
   InsertTailList (, >Link);
+  gBS->RestoreTPL (OriginalTpl);
 
   //
   // The DeviceAddress is the address of the maped buffer below 4GB
@@ -233,6 +228,7 @@ IoMmuUnmap (
 {
   MAP_INFO *MapInfo;
   LIST_ENTRY   *Link;
+  EFI_TPL  OriginalTpl;
 
   DEBUG ((DEBUG_VERBOSE, "IoMmuUnmap: 0x%08x\n", Mapping));
 
@@ -241,6 +237,7 @@ IoMmuUnmap (
 return EFI_INVALID_PARAMETER;
   }
 
+  OriginalTpl = gBS->RaiseTPL (VTD_TPL_LEVEL);
   MapInfo = NULL;
   for (Link = GetFirstNode ()
; !IsNull (, Link)
@@ -255,10 +252,12 @@ IoMmuUnmap (
   // Mapping is not a valid value returned by Map()
   //
   if (MapInfo != Mapping) {
+gBS->RestoreTPL (OriginalTpl);
 DEBUG ((DEBUG_ERROR, "IoMmuUnmap: %r\n", EFI_INVALID_PARAMETER));
 return EFI_INVALID_PARAMETER;
   }
   RemoveEntryList (>Link);
+  gBS->RestoreTPL (OriginalTpl);
 
   if (MapInfo->DeviceAddress != MapInfo->HostAddress) {
 //
diff --git a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.c 
b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.c
index 648f64c20b77..013823cc161f 100644
--- a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.c
+++ b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.c
@@ -478,7 +478,7 @@ InitializeDmaProtection (
   
   Status = gBS->CreateEventEx (
   EVT_NOTIFY_SIGNAL,
-  TPL_CALLBACK,
+  VTD_TPL_LEVEL,
   AcpiNotificationFunc,
   NULL,
   ,
@@ -488,7 +488,7 @@ InitializeDmaProtection (
 
   Status = gBS->CreateEventEx (
   EVT_NOTIFY_SIGNAL,
-  TPL_CALLBACK,
+  VTD_TPL_LEVEL,
   AcpiNotificationFunc,
   NULL,
   ,
@@ -505,7 +505,7 @@ InitializeDmaProtection (
 
   Status = gBS->CreateEventEx (
   EVT_NOTIFY_SIGNAL,
-  TPL_CALLBACK,
+  VTD_TPL_LEVEL,
   OnExitBootServices,
   NULL,
   ,
@@ -514,7 +514,7 @@ InitializeDmaProtection (
   ASSERT_EFI_ERROR (Status);
 
   Status = EfiCreateEventLegacyBootEx (
- TPL_CALLBACK,
+ VTD_TPL_LEVEL,
  OnLegacyBoot,
  NULL,
  
diff --git a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.h 
b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.h
index 519a5ab00450..bc14ff9a6631 100644
--- a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.h
+++ b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.h
@@ -48,6 +48,8 @@
 #define ALIGN_VALUE_UP(Value, Alignment)  (((Value) + (Alignment) - 1) & 
(~((Alignment) - 1)))
 #define ALIGN_VALUE_LOW(Value, Alignment) ((Value) & (~((Alignment) - 1)))
 
+#define VTD_TPL_LEVEL TPL_NOTIFY
+
 //
 // This is the initial max PCI DATA number.
 // The number may be enlarged later.
diff --git a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/IntelVTdDxe.c 
b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/IntelVTdDxe.c
index 89d9bea3fc0f..570b47cf7364 100644
--- 

[edk2] [PATCH] IntelSiliconPkg IntelVTdDxe: Signal AcpiNotificationFunc() initially

[Star Zeng]

Signal AcpiNotificationFunc() initially for the case that
DMAR table has been installed when creating event.

Cc: Jiewen Yao 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Star Zeng 
---
 IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.c | 16 
 IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmarAcpiTable.c |  3 +++
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.c 
b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.c
index 37b3b19bce90..648f64c20b77 100644
--- a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.c
+++ b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.c
@@ -1,6 +1,6 @@
 /** @file
 
-  Copyright (c) 2017, Intel Corporation. All rights reserved.
+  Copyright (c) 2017 - 2018, Intel Corporation. All rights reserved.
   This program and the accompanying materials
   are licensed and made available under the terms and conditions of the BSD 
License
   which accompanies this distribution.  The full text of the license may be 
found at
@@ -473,7 +473,8 @@ InitializeDmaProtection (
   EFI_STATUS  Status;
   EFI_EVENT   ExitBootServicesEvent;
   EFI_EVENT   LegacyBootEvent;
-  EFI_EVENT   Event;
+  EFI_EVENT   EventAcpi10;
+  EFI_EVENT   EventAcpi20;
   
   Status = gBS->CreateEventEx (
   EVT_NOTIFY_SIGNAL,
@@ -481,7 +482,7 @@ InitializeDmaProtection (
   AcpiNotificationFunc,
   NULL,
   ,
-  
+  
   );
   ASSERT_EFI_ERROR (Status);
 
@@ -491,10 +492,17 @@ InitializeDmaProtection (
   AcpiNotificationFunc,
   NULL,
   ,
-  
+  
   );
   ASSERT_EFI_ERROR (Status);
 
+  //
+  // Signal the events initially for the case
+  // that DMAR table has been installed.
+  //
+  gBS->SignalEvent (EventAcpi20);
+  gBS->SignalEvent (EventAcpi10);
+
   Status = gBS->CreateEventEx (
   EVT_NOTIFY_SIGNAL,
   TPL_CALLBACK,
diff --git a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmarAcpiTable.c 
b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmarAcpiTable.c
index ce350bafbe3f..b16bd93d53f1 100644
--- a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmarAcpiTable.c
+++ b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmarAcpiTable.c
@@ -1005,6 +1005,9 @@ GetDmarAcpiTable (

);
   }
+  if (EFI_ERROR (Status)) {
+return EFI_NOT_FOUND;
+  }
   ASSERT (AcpiTable != NULL);
 
   mAcpiDmarTable = FindAcpiPtr (
-- 
2.7.0.windows.1

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

[edk2] [Patch 4/4] NetworkPkg: Add more parameter or return status check in UDP6 driver

[fanwang2]

From: Wang Fan 

In UDP6Dxe, there are several places that may be enhanced
to check input parameters and returned status. This patch
is to fix these issues.

Cc: Ye Ting 
Cc: Jiaxin Wu 
Cc: Fu Siyuan 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Wang Fan 
---
 NetworkPkg/Udp6Dxe/Udp6Driver.c | 48 +++--
 NetworkPkg/Udp6Dxe/Udp6Impl.c   | 17 +++
 NetworkPkg/Udp6Dxe/Udp6Main.c   |  2 +-
 3 files changed, 45 insertions(+), 22 deletions(-)

diff --git a/NetworkPkg/Udp6Dxe/Udp6Driver.c b/NetworkPkg/Udp6Dxe/Udp6Driver.c
index 6dde1fc..f9d528e 100644
--- a/NetworkPkg/Udp6Dxe/Udp6Driver.c
+++ b/NetworkPkg/Udp6Dxe/Udp6Driver.c
@@ -288,22 +288,19 @@ Udp6DriverBindingStop (
Udp6DestroyChildEntryInHandleBuffer,
,
NULL
);
   } else if (IsListEmpty (>ChildrenList)) {
-gBS->UninstallMultipleProtocolInterfaces (
-   NicHandle,
-   ,
-   >ServiceBinding,
-   NULL
-   );
+Status = gBS->UninstallMultipleProtocolInterfaces (
+   NicHandle,
+   ,
+   >ServiceBinding,
+   NULL
+   );
  
 Udp6CleanService (Udp6Service);
-
 FreePool (Udp6Service);
-
-Status = EFI_SUCCESS;
   }
 
   return Status;
 }
 
@@ -508,25 +505,34 @@ Udp6ServiceBindingDestroyChild (
   Instance->InDestroy = TRUE;
 
   //
   // Close the Ip6 protocol on the default IpIo.
   //
-  gBS->CloseProtocol (
- Udp6Service->IpIo->ChildHandle,
- ,
- gUdp6DriverBinding.DriverBindingHandle,
- Instance->ChildHandle
- );
+  Status = gBS->CloseProtocol (
+ Udp6Service->IpIo->ChildHandle,
+ ,
+ gUdp6DriverBinding.DriverBindingHandle,
+ Instance->ChildHandle
+ );
+  if (EFI_ERROR (Status)) {
+Instance->InDestroy = FALSE;
+return Status;
+  }
+
   //
   // Close the Ip6 protocol on this instance's IpInfo.
   //
-  gBS->CloseProtocol (
- Instance->IpInfo->ChildHandle,
- ,
- gUdp6DriverBinding.DriverBindingHandle,
- Instance->ChildHandle
- );
+  Status = gBS->CloseProtocol (
+ Instance->IpInfo->ChildHandle,
+ ,
+ gUdp6DriverBinding.DriverBindingHandle,
+ Instance->ChildHandle
+ );
+  if (EFI_ERROR (Status)) {
+Instance->InDestroy = FALSE;
+return Status;
+  }
 
   //
   // Uninstall the Udp6Protocol previously installed on the ChildHandle.
   //
   Status = gBS->UninstallMultipleProtocolInterfaces (
diff --git a/NetworkPkg/Udp6Dxe/Udp6Impl.c b/NetworkPkg/Udp6Dxe/Udp6Impl.c
index 458470c..d014e2d 100644
--- a/NetworkPkg/Udp6Dxe/Udp6Impl.c
+++ b/NetworkPkg/Udp6Dxe/Udp6Impl.c
@@ -55,10 +55,13 @@ Udp6FindInstanceByPort (
 /**
   This function is the packet transmitting notify function registered to the 
IpIo
   interface. It's called to signal the udp TxToken when the IpIo layer 
completes
   transmitting of the udp datagram.
 
+  If Context is NULL, then ASSERT().
+  If NotifyData is NULL, then ASSERT().
+
   @param[in]  StatusThe completion status of the output udp 
datagram.
   @param[in]  Context   Pointer to the context data.
   @param[in]  SenderSpecify a EFI_IP6_PROTOCOL for sending.
   @param[in]  NotifyDataPointer to the notify data.
 
@@ -73,10 +76,14 @@ Udp6DgramSent (
   );
 
 /**
   This function processes the received datagram passed up by the IpIo layer.
 
+  If NetSession is NULL, then ASSERT().
+  If Packet is NULL, then ASSERT().
+  If Context is NULL, then ASSERT().
+
   @param[in]  StatusThe status of this udp datagram.
   @param[in]  IcmpError The IcmpError code, only available when Status 
is
 EFI_ICMP_ERROR.
   @param[in]  NetSessionPointer to the EFI_NET_SESSION_DATA.
   @param[in]  PacketPointer to the NET_BUF containing the received 
udp
@@ -975,10 +982,13 @@ Udp6RemoveToken (
 /**
   This function is the packet transmitting notify function registered to the 
IpIo
   interface. It's called to signal the udp TxToken when IpIo layer completes 
the
   transmitting of the udp datagram.
 
+  If Context is NULL, then ASSERT().
+  If NotifyData is NULL, then ASSERT().
+
   @param[in]  StatusThe completion status of the output udp 
datagram.
   @param[in]  Context   Pointer to the context data.
   @param[in]  SenderSpecify a EFI_IP6_PROTOCOL for sending.
   @param[in]  NotifyDataPointer to the notify data.
 
@@ -993,10 +1003,12 @@ Udp6DgramSent (
   )
 {
   UDP6_INSTANCE_DATA *Instance;
   EFI_UDP6_COMPLETION_TOKEN  *Token;
 
+  ASSERT (Context != NULL && NotifyData != NULL);
+
   Instance = (UDP6_INSTANCE_DATA *) Context;
   Token= 

[edk2] [Patch 2/4] NetworkPkg: Fix a memory leak issue in UDP6 driver

[fanwang2]

From: Wang Fan 

In UDP6Dxe Udp6Groups(), the code return directly without free the
buffer allocated for McastIp when JoinFlag is TRUE. It is a memory
leak issue that needs to be fixed. This patch is to fix this issue.

Cc: Ye Ting 
Cc: Jiaxin Wu 
Cc: Fu Siyuan 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Wang Fan 
---
 NetworkPkg/Udp6Dxe/Udp6Main.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/NetworkPkg/Udp6Dxe/Udp6Main.c b/NetworkPkg/Udp6Dxe/Udp6Main.c
index 9105ef4..8495bc3 100644
--- a/NetworkPkg/Udp6Dxe/Udp6Main.c
+++ b/NetworkPkg/Udp6Dxe/Udp6Main.c
@@ -349,10 +349,13 @@ Udp6Groups (
 }
   }
 
   Instance = UDP6_INSTANCE_DATA_FROM_THIS (This);
   if (!Instance->Configured) {
+if (McastIp != NULL) {
+  FreePool (McastIp);
+}
 return EFI_NOT_STARTED;
   }
 
   Ip  = Instance->IpInfo->Ip.Ip6;
 
-- 
1.9.5.msysgit.1

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

[edk2] [Patch 3/4] NetworkPkg: Fix some coding style issues in UDP6 driver

[fanwang2]

From: Wang Fan 

In UDP6Dxe, there are some coding style issues, this patch
is to fix these issues.

Cc: Ye Ting 
Cc: Jiaxin Wu 
Cc: Fu Siyuan 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Wang Fan 
---
 NetworkPkg/Udp6Dxe/Udp6Driver.c | 20 ++--
 NetworkPkg/Udp6Dxe/Udp6Impl.c   | 10 ++
 NetworkPkg/Udp6Dxe/Udp6Main.c   |  2 +-
 3 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/NetworkPkg/Udp6Dxe/Udp6Driver.c b/NetworkPkg/Udp6Dxe/Udp6Driver.c
index a4b1104..6dde1fc 100644
--- a/NetworkPkg/Udp6Dxe/Udp6Driver.c
+++ b/NetworkPkg/Udp6Dxe/Udp6Driver.c
@@ -1,9 +1,9 @@
 /** @file
   Driver Binding functions and Service Binding functions for the Network 
driver module.
 
-  Copyright (c) 2009 - 2016, Intel Corporation. All rights reserved.
+  Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved.
 
   This program and the accompanying materials
   are licensed and made available under the terms and conditions of the BSD 
License
   which accompanies this distribution.  The full text of the license may be 
found at
   http://opensource.org/licenses/bsd-license.php.
@@ -162,11 +162,10 @@ Udp6DriverBindingStart (
   >ServiceBinding,
   NULL
   );
   if (EFI_ERROR (Status)) {
 Udp6CleanService (Udp6Service);
-goto EXIT;
   }
 
 EXIT:
   if (EFI_ERROR (Status)) {
 if (Udp6Service != NULL) {
@@ -180,12 +179,13 @@ EXIT:
   Callback function which provided by user to remove one node in 
NetDestroyLinkList process.
   
   @param[in]Entry   The entry to be removed.
   @param[in]Context Pointer to the callback context corresponds to 
the Context in NetDestroyLinkList.
 
-  @retval EFI_SUCCESS   The entry has been removed successfully.
-  @retval OthersFail to remove the entry.
+  @retval EFI_INVALID_PARAMETER  Entry is NULL or Context is NULL.
+  @retval EFI_SUCCESSThe entry has been removed successfully.
+  @retval Others Fail to remove the entry.
 
 **/
 EFI_STATUS
 EFIAPI
 Udp6DestroyChildEntryInHandleBuffer (
@@ -241,16 +241,16 @@ Udp6DriverBindingStop (
   IN  EFI_HANDLE   ControllerHandle,
   IN  UINTNNumberOfChildren,
   IN  EFI_HANDLE   *ChildHandleBuffer OPTIONAL
   )
 {
-  EFI_STATUSStatus;
-  EFI_HANDLENicHandle;
-  EFI_SERVICE_BINDING_PROTOCOL  *ServiceBinding;
-  UDP6_SERVICE_DATA *Udp6Service;
-  LIST_ENTRY*List;
-  UDP6_DESTROY_CHILD_IN_HANDLE_BUF_CONTEXT Context;
+  EFI_STATUSStatus;
+  EFI_HANDLENicHandle;
+  EFI_SERVICE_BINDING_PROTOCOL  *ServiceBinding;
+  UDP6_SERVICE_DATA *Udp6Service;
+  LIST_ENTRY*List;
+  UDP6_DESTROY_CHILD_IN_HANDLE_BUF_CONTEXT  Context;
 
   //
   // Find the NicHandle where UDP6 ServiceBinding Protocol is installed.
   //
   NicHandle = NetLibGetNicHandle (ControllerHandle, );
diff --git a/NetworkPkg/Udp6Dxe/Udp6Impl.c b/NetworkPkg/Udp6Dxe/Udp6Impl.c
index 25d4e6a..458470c 100644
--- a/NetworkPkg/Udp6Dxe/Udp6Impl.c
+++ b/NetworkPkg/Udp6Dxe/Udp6Impl.c
@@ -156,11 +156,12 @@ Udp6RecycleRxDataWrap (
   @param[in]  Packet Pointer to the buffer containing the received
  datagram.
   @param[in]  RxData Pointer to the EFI_UDP6_RECEIVE_DATA of this
  datagram.
 
-  @return Pointer to the structure wrapping the RxData and the Packet.
+  @return Pointer to the structure wrapping the RxData and the Packet. NULL 
will
+  be returned if any error occurs.
 
 **/
 UDP6_RXDATA_WRAP *
 Udp6WrapRxData (
   IN UDP6_INSTANCE_DATA *Instance,
@@ -1372,11 +1373,12 @@ Udp6RecycleRxDataWrap (
   @param[in]  Packet Pointer to the buffer containing the received
  datagram.
   @param[in]  RxData Pointer to the EFI_UDP6_RECEIVE_DATA of this
  datagram.
 
-  @return Pointer to the structure wrapping the RxData and the Packet.
+  @return Pointer to the structure wrapping the RxData and the Packet. NULL 
will
+  be returned if any error occurs.
 
 **/
 UDP6_RXDATA_WRAP *
 Udp6WrapRxData (
   IN UDP6_INSTANCE_DATA *Instance,
@@ -1596,11 +1598,11 @@ Udp6Demultiplex (
   UINT16 HeadSum;
   EFI_UDP6_RECEIVE_DATA  RxData;
   EFI_UDP6_SESSION_DATA  *Udp6Session;
   UINTN  Enqueued;
 
-  if (Packet->TotalSize < sizeof (EFI_UDP_HEADER)) {
+  if (Packet->TotalSize < UDP6_HEADER_SIZE) {
 NetbufFree (Packet);
 return;
   }
   
   //
@@ -1848,11 +1850,11 @@ Udp6IcmpHandler (
   EFI_UDP_HEADER *Udp6Header;
   

[edk2] [Patch 0/4] Fix some issues in Udp6Dxe

[fanwang2]

From: Wang Fan 

See descriptions in each patch.

Wang Fan (4):
  NetworkPkg: Add ASSERT error handling for UDP6 driver
  NetworkPkg: Fix a memory leak issue in UDP6 driver
  NetworkPkg: Fix some coding style issues in UDP6 driver
  NetworkPkg: Add more parameter or return status check in UDP6 driver

 NetworkPkg/Udp6Dxe/Udp6Driver.c | 68 ++---
 NetworkPkg/Udp6Dxe/Udp6Impl.c   | 43 +++---
 NetworkPkg/Udp6Dxe/Udp6Main.c   | 14 +++--
 3 files changed, 86 insertions(+), 39 deletions(-)

-- 
1.9.5.msysgit.1

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

[edk2] [Patch 1/4] NetworkPkg: Add ASSERT error handling for UDP6 driver

[fanwang2]

From: Wang Fan 

In Udp6Dxe, there are several places use ASSERT to check returned
value. But these errors should be handled if they occur, this patch
is to fix this issue.

Cc: Ye Ting 
Cc: Jiaxin Wu 
Cc: Fu Siyuan 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Wang Fan 
---
 NetworkPkg/Udp6Dxe/Udp6Impl.c | 16 +++-
 NetworkPkg/Udp6Dxe/Udp6Main.c |  7 ++-
 2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/NetworkPkg/Udp6Dxe/Udp6Impl.c b/NetworkPkg/Udp6Dxe/Udp6Impl.c
index edf2c23..25d4e6a 100644
--- a/NetworkPkg/Udp6Dxe/Udp6Impl.c
+++ b/NetworkPkg/Udp6Dxe/Udp6Impl.c
@@ -1,9 +1,9 @@
 /** @file
   Udp6 driver's whole implementation.
 
-  Copyright (c) 2009 - 2016, Intel Corporation. All rights reserved.
+  Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved.
 
   This program and the accompanying materials
   are licensed and made available under the terms and conditions of the BSD 
License
   which accompanies this distribution.  The full text of the license may be 
found at
   http://opensource.org/licenses/bsd-license.php.
@@ -1606,10 +1606,14 @@ Udp6Demultiplex (
   //
   // Get the datagram header from the packet buffer.
   //
   Udp6Header = (EFI_UDP_HEADER *) NetbufGetByte (Packet, 0, NULL);
   ASSERT (Udp6Header != NULL);
+  if (Udp6Header == NULL) {
+NetbufFree (Packet);
+return;
+  }
 
   if (Udp6Header->Checksum != 0) {
 //
 // check the checksum.
 //
@@ -1716,10 +1720,13 @@ Udp6SendPortUnreach (
   //
   // Get the Ipv6 Mode Data.
   //
   Ip6ModeData = AllocateZeroPool (sizeof (EFI_IP6_MODE_DATA));
   ASSERT (Ip6ModeData != NULL);
+  if (Ip6ModeData == NULL) {
+goto EXIT;
+  }
 
   //
   // If not finding the related IpSender use the default IpIo to send out
   // the port unreachable ICMP message.
   //
@@ -1764,10 +1771,13 @@ Udp6SendPortUnreach (
   //
   // Allocate space for the IP6_ICMP_ERROR_HEAD.
   //
   IcmpErrHdr = (IP6_ICMP_ERROR_HEAD *) NetbufAllocSpace (Packet, Len, FALSE);
   ASSERT (IcmpErrHdr != NULL);
+  if (IcmpErrHdr == NULL) {
+goto EXIT;
+  }
 
   //
   // Set the required fields for the icmp port unreachable message.
   //
   IcmpErrHdr->Head.Type = ICMP_V6_DEST_UNREACHABLE;
@@ -1845,10 +1855,14 @@ Udp6IcmpHandler (
 return;
   }
   
   Udp6Header = (EFI_UDP_HEADER *) NetbufGetByte (Packet, 0, NULL);
   ASSERT (Udp6Header != NULL);
+  if (Udp6Header == NULL) {
+NetbufFree (Packet);
+return;
+  }
 
   IP6_COPY_ADDRESS (, >Source);
   IP6_COPY_ADDRESS (, >Dest);
 
   Udp6Session.SourcePort  = NTOHS (Udp6Header->DstPort);
diff --git a/NetworkPkg/Udp6Dxe/Udp6Main.c b/NetworkPkg/Udp6Dxe/Udp6Main.c
index f3e9925..9105ef4 100644
--- a/NetworkPkg/Udp6Dxe/Udp6Main.c
+++ b/NetworkPkg/Udp6Dxe/Udp6Main.c
@@ -1,9 +1,9 @@
 /** @file
   Contains all EFI_UDP6_PROTOCOL interfaces.
 
-  Copyright (c) 2009 - 2014, Intel Corporation. All rights reserved.
+  Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved.
 
   This program and the accompanying materials
   are licensed and made available under the terms and conditions of the BSD 
License
   which accompanies this distribution.  The full text of the license may be 
found at
   http://opensource.org/licenses/bsd-license.php.
@@ -523,10 +523,15 @@ Udp6Transmit (
   Udp6Service= Instance->Udp6Service;
   *((UINTN *) >ProtoData[0]) = (UINTN) (Udp6Service->IpIo);
 
   Udp6Header = (EFI_UDP_HEADER *) NetbufAllocSpace (Packet, UDP6_HEADER_SIZE, 
TRUE);
   ASSERT (Udp6Header != NULL);
+  if (Udp6Header == NULL) {
+Status = EFI_OUT_OF_RESOURCES;
+goto ON_EXIT;
+  }
+  
   ConfigData = >ConfigData;
 
   //
   // Fill the udp header.
   //
-- 
1.9.5.msysgit.1

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

[edk2] [PATCH v2] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard

[Jian J Wang]

> v2 changes:
> a. Use each AP's ApTopOfStack to get the stack base address instead of
>cpu0's ApTopOfStack which is actually set incorrectly before.
> b. Fix cpu0's ApTopOfStack initialization.
> c. Fix wrong debug print format.

The reason is that DXE part initialization will reuse the stack allocated
at PEI phase, if MP was initialized before. Some code added to check this
situation and use stack base address saved in HOB passed from PEI.

Cc: Jiewen Yao 
Cc: Eric Dong 
Cc: Laszlo Ersek 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang 
---
 UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 19 ++-
 UefiCpuPkg/Library/MpInitLib/MpLib.c|  2 +-
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c 
b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
index 40c1bf407a..e832c16eca 100644
--- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
@@ -295,6 +295,7 @@ InitMpGlobalData (
   UINTN   Index;
   EFI_GCD_MEMORY_SPACE_DESCRIPTOR MemDesc;
   UINTN   StackBase;
+  CPU_INFO_IN_HOB *CpuInfoInHob;
 
   SaveCpuMpData (CpuMpData);
 
@@ -314,8 +315,21 @@ InitMpGlobalData (
   ASSERT (FALSE);
 }
 
+//
+// DXE will reuse stack allocated for APs at PEI phase if it's available.
+// Let's check it here.
+//
+// Note: BSP's stack guard is set at DxeIpl phase. But for the sake of
+// BSP/AP exchange, stack guard for ApTopOfStack of cpu 0 will still be
+// set here.
+//
+CpuInfoInHob = (CPU_INFO_IN_HOB *)(UINTN)CpuMpData->CpuInfoInHob;
 for (Index = 0; Index < CpuMpData->CpuCount; ++Index) {
-  StackBase = CpuMpData->Buffer + Index * CpuMpData->CpuApStackSize;
+  if (CpuInfoInHob != NULL && CpuInfoInHob[Index].ApTopOfStack != 0) {
+StackBase = CpuInfoInHob[Index].ApTopOfStack - 
CpuMpData->CpuApStackSize;
+  } else {
+StackBase = CpuMpData->Buffer + Index * CpuMpData->CpuApStackSize;
+  }
 
   Status = gDS->GetMemorySpaceDescriptor (StackBase, );
   ASSERT_EFI_ERROR (Status);
@@ -326,6 +340,9 @@ InitMpGlobalData (
   MemDesc.Attributes | EFI_MEMORY_RP
   );
   ASSERT_EFI_ERROR (Status);
+
+  DEBUG ((DEBUG_INFO, "Stack Guard set at %lx [cpu%lu]!\n",
+  (UINT64)StackBase, (UINT64)Index));
 }
   }
 
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c 
b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index 0c2058a7b0..1bfab8467b 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -1498,7 +1498,7 @@ MpInitLibInitialize (
   //
   // Set BSP basic information
   //
-  InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer);
+  InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer + ApStackSize);
   //
   // Save assembly code information
   //
-- 
2.15.1.windows.2

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

Re: [edk2] [RFC v4 4/6] UefiCpuPkg/CpuExceptionHandlerLib: Add helper to valid memory addresses

[Yao, Jiewen]

5) For CR4, please use meaning definition for BIT4/BIT5.
  if ((Cr4 & BIT4) != 0 && (*PageDirEntry & BIT7) != 0) {
if (((Cr4 & BIT5) != 0 && (UINT64)LinearAddress > 0xFULL) ||

6) For IA32 PAE/PSE calculation, same comment for 3 and 4.

7) Last but not least important, would you please share the information on how 
do you validate the 32bit PAE/PSE/normal 4K page table?

Thank you
Yao Jiewen

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Yao,
> Jiewen
> Sent: Thursday, January 4, 2018 9:36 AM
> To: Paulo Alcantara ; edk2-devel@lists.01.org
> Cc: Laszlo Ersek ; Dong, Eric 
> Subject: Re: [edk2] [RFC v4 4/6] UefiCpuPkg/CpuExceptionHandlerLib: Add
> helper to valid memory addresses
> 
> Some suggestion:
> 
> 1) I am not sure if it is proper to use ASSERT in an exception handler, 
> because we
> know something is wrong.
> 
>   ASSERT ((PhysicalAddress & (sizeof (*Pml4TableEntry) - 1)) == 0);
> 
> I suggest we just do the check, and return FALSE, if the prerequisite is not
> satisfied.
> 
> 2) Can we use meaningful definition for BIT0, BIT7?
> 
>   if ((*Pml4TableEntry & BIT0) == 0) {
>   if ((*PageDirPtrTableEntry & BIT7) != 0) {
> 
> 3) I am not sure if I understand below code.
> 
>   PhysicalAddress = (UINT64)Cr3 & (((1ULL << MaxPhyAddrBits) - 1) << 12);
>   PhysicalAddress = *Pml4TableEntry & (((1ULL << MaxPhyAddrBits) - 1) << 12);
>   PhysicalAddress = *PageDirPtrTableEntry & (((1ULL << MaxPhyAddrBits) - 1)
> << 12);
>   PhysicalAddress = *PageDirEntry & (((1ULL << MaxPhyAddrBits) - 1) << 12);
> 
> If MaxPhyAddrBits is 48, you will get "Cr3 & 0x0000". Is that what
> you want? I think we need "Cr3 & 0xF000"
> Should it be: PhysicalAddress = (UINT64)Cr3 & ((1ULL << MaxPhyAddrBits) - 1) &
> (~0xFFF);
> 
> 4) Can we use a more readable way to below? Personally, I do not suggest "<< 
> 3",
> which is just the index calculation.
> 
>   PhysicalAddress = (UINT64)Cr3 & (((1ULL << MaxPhyAddrBits) - 1) << 12);
>   PhysicalAddress |= (((UINT64)LinearAddress >> 39) & 0x1FF) << 3;
>   Pml4TableEntry = (UINT64 *)(UINTN)PhysicalAddress;
>   PhysicalAddress = *Pml4TableEntry & (((1ULL << MaxPhyAddrBits) - 1) << 12);
> 
> For example:
>   PhysicalAddress = (UINT64)Cr3 & ((1ULL << MaxPhyAddrBits) - 1) & (~0xFFF);
>   Pml4TableEntry = (UINT64 *)(UINTN)PhysicalAddress;
>   Index= (UINTN)(((UINT64)LinearAddress >> 39) & 0x1FF);
>   PhysicalAddress = Pml4TableEntry[Index] & ((1ULL << MaxPhyAddrBits) - 1) &
> (~0xFFF);
> 
> 
> 
> Thank you
> Yao Jiewen
> 
> 
> > -Original Message-
> > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Paulo
> > Alcantara
> > Sent: Friday, December 29, 2017 12:40 PM
> > To: edk2-devel@lists.01.org
> > Cc: Laszlo Ersek ; Dong, Eric 
> > Subject: [edk2] [RFC v4 4/6] UefiCpuPkg/CpuExceptionHandlerLib: Add helper
> to
> > valid memory addresses
> >
> > Introduce IsLinearAddressValid() function that will be used for
> > validating memory addresses that would get dereferenced during stack
> > traces in IA32 and X64 CPU exceptions.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Cc: Eric Dong 
> > Cc: Laszlo Ersek 
> > Requested-by: Brian Johnson 
> > Requested-by: Jiewen Yao 
> > Signed-off-by: Paulo Alcantara 
> > ---
> >  UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c | 382
> > 
> >  UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.h |
> 16 +
> >  2 files changed, 398 insertions(+)
> >
> > diff --git
> > a/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c
> > b/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c
> > index 867c5c01d6..52b3eb1463 100644
> > --- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c
> > +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c
> > @@ -14,6 +14,9 @@
> >
> >  #include "CpuExceptionCommon.h"
> >
> > +#include 
> > +#include 
> > +
> >  //
> >  // Error code flag indicating whether or not an error code will be
> >  // pushed on the stack if an exception occurs.
> > @@ -194,3 +197,382 @@ GetPdbFileName (
> >  }
> >}
> >  }
> > +
> > +/**
> > +  Check if a linear address is valid by walking the page tables in 4-level
> > +  paging mode.
> > +
> > +  @param[in]  Cr3 CR3 control register.
> > +  @param[in]  MaxPhyAddrBits  MAXPHYADDRBITS bits.
> > +  @param[in]  LinearAddress   Linear address to be checked.
> > +**/
> > +STATIC
> > +BOOLEAN
> > +Do4LevelPagingModeCheck (
> > +  IN UINTNCr3,
> > +  IN UINT8MaxPhyAddrBits,
> > +  IN UINTNLinearAddress
> > +  )
> > +{
> > +  UINT64 PhysicalAddress;
> > +  UINT64 *Pml4TableEntry;
> > +  UINT64 *PageDirPtrTableEntry;
> > +  UINT64 

Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard

[Wang, Jian J]

A correction: although BSP doesn't need it, we still need to initialize its 
ApTopOfStack
correctly because the MP service supports BSP/AP exchange. So I think the line 
1501
should be changed to

  InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer + 
CpuMpData->CpuApStackSize);

instead of

  InitializeApData (CpuMpData, 0, 0, NULL);

Regards,
Jian


> -Original Message-
> From: Wang, Jian J
> Sent: Thursday, January 04, 2018 9:09 AM
> To: Wang, Jian J ; Laszlo Ersek ;
> edk2-devel@lists.01.org
> Cc: Yao, Jiewen ; Dong, Eric 
> Subject: RE: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set
> as Stack Guard
> 
> Laszlo,
> 
> More explanations:
> 
> [UefiCpuPkg\Library\MpInitLib\MpLib.c]
> According to the code, the BSP's (CpuInfoInHob[0].ApTopOfStack) is initialized
> to
> the bottom of the stack (line 1501) but AP's ApTopOfStack is correctly 
> initialized
> (line 598). Although my calculation is correct, I think it'd be better to use 
> AP's
> ApTopOfStack directly. From this perspective, you're right.
> 
> Maybe it'd be better to pass a NULL pointer at line 1501 because BSP doesn't
> need
> it anyway.
> 
> Regards,
> Jian
> 
> 
> > -Original Message-
> > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Wang,
> > Jian J
> > Sent: Thursday, January 04, 2018 8:42 AM
> > To: Laszlo Ersek ; edk2-devel@lists.01.org
> > Cc: Yao, Jiewen ; Dong, Eric 
> > Subject: Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set
> > as Stack Guard
> >
> > Laszlo,
> >
> > I revisited code of MpInitLib. I found that CPU_INFO_IN_HOB.ApTopOfStack
> > was assigned to CpuMpData->Buffer in MpInitLibInitialize()
> >
> > (line1501)  InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer);
> >
> > but in
> >
> > (line598)  ApTopOfStack  = CpuMpData->Buffer + (ProcessorNumber + 1) *
> > CpuMpData->CpuApStackSize;
> > (line608)  InitializeApData (CpuMpData, ProcessorNumber, BistData,
> > ApTopOfStack);
> >
> > Since InitMpGlobalData() is called just after first situation, my patch is 
> > correct.
> >
> > I think the problem here is that ApTopOfStack initialized at line 1501 is 
> > not
> > correct.
> >
> >
> > Regards,
> > Jian
> >
> >
> > > -Original Message-
> > > From: Laszlo Ersek [mailto:ler...@redhat.com]
> > > Sent: Thursday, January 04, 2018 1:33 AM
> > > To: Wang, Jian J ; edk2-devel@lists.01.org
> > > Cc: Yao, Jiewen ; Dong, Eric ;
> > > Jeff Fan 
> > > Subject: Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address
> set
> > > as Stack Guard
> > >
> > > (CC Jeff)
> > >
> > > Sorry about the delay.
> > >
> > > I have some light comments below; I expect at least a few of them to be
> > > incorrect :)
> > >
> > > On 12/29/17 09:36, Jian J Wang wrote:
> > > > The reason is that DXE part initialization will reuse the stack 
> > > > allocated
> > > > at PEI phase, if MP was initialized before. Some code added to check 
> > > > this
> > > > situation and use stack base address saved in HOB passed from PEI.
> > > >
> > > > Cc: Jiewen Yao 
> > > > Cc: Eric Dong 
> > > > Cc: Laszlo Ersek 
> > > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > > Signed-off-by: Jian J Wang 
> > > > ---
> > > >  UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 17 +++--
> > > >  1 file changed, 15 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> > > b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> > > > index 40c1bf407a..05484c9ff3 100644
> > > > --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> > > > +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> > > > @@ -295,6 +295,7 @@ InitMpGlobalData (
> > > >UINTN   Index;
> > > >EFI_GCD_MEMORY_SPACE_DESCRIPTOR MemDesc;
> > > >UINTN   StackBase;
> > > > +  CPU_INFO_IN_HOB *CpuInfoInHob;
> > > >
> > > >SaveCpuMpData (CpuMpData);
> > > >
> > > > @@ -314,9 +315,18 @@ InitMpGlobalData (
> > > >ASSERT (FALSE);
> > > >  }
> > > >
> > > > -for (Index = 0; Index < CpuMpData->CpuCount; ++Index) {
> > > > -  StackBase = CpuMpData->Buffer + Index * CpuMpData-
> >CpuApStackSize;
> > > > +//
> > > > +// DXE will reuse stack allocated for APs at PEI phase if it's 
> > > > available.
> > > > +// Let's check it here.
> > > > +//
> > > > +CpuInfoInHob = (CPU_INFO_IN_HOB *)(UINTN)CpuMpData-
> > > >CpuInfoInHob;
> > > > +if (CpuInfoInHob != NULL && CpuInfoInHob->ApTopOfStack != 0) {
> > > > +  StackBase = CpuInfoInHob->ApTopOfStack;
> > > > +} else {
> > > > +  StackBase = CpuMpData->Buffer;
> > > > +}
> > >
> > 

Re: [edk2] [RFC v4 4/6] UefiCpuPkg/CpuExceptionHandlerLib: Add helper to valid memory addresses

[Yao, Jiewen]

Some suggestion:

1) I am not sure if it is proper to use ASSERT in an exception handler, because 
we know something is wrong.

  ASSERT ((PhysicalAddress & (sizeof (*Pml4TableEntry) - 1)) == 0);

I suggest we just do the check, and return FALSE, if the prerequisite is not 
satisfied.

2) Can we use meaningful definition for BIT0, BIT7?

  if ((*Pml4TableEntry & BIT0) == 0) {
  if ((*PageDirPtrTableEntry & BIT7) != 0) {

3) I am not sure if I understand below code.

  PhysicalAddress = (UINT64)Cr3 & (((1ULL << MaxPhyAddrBits) - 1) << 12);
  PhysicalAddress = *Pml4TableEntry & (((1ULL << MaxPhyAddrBits) - 1) << 12);
  PhysicalAddress = *PageDirPtrTableEntry & (((1ULL << MaxPhyAddrBits) - 1) << 
12);
  PhysicalAddress = *PageDirEntry & (((1ULL << MaxPhyAddrBits) - 1) << 12);

If MaxPhyAddrBits is 48, you will get "Cr3 & 0x0000". Is that what 
you want? I think we need "Cr3 & 0xF000"
Should it be: PhysicalAddress = (UINT64)Cr3 & ((1ULL << MaxPhyAddrBits) - 1) & 
(~0xFFF);

4) Can we use a more readable way to below? Personally, I do not suggest "<< 
3", which is just the index calculation.

  PhysicalAddress = (UINT64)Cr3 & (((1ULL << MaxPhyAddrBits) - 1) << 12);
  PhysicalAddress |= (((UINT64)LinearAddress >> 39) & 0x1FF) << 3;
  Pml4TableEntry = (UINT64 *)(UINTN)PhysicalAddress;
  PhysicalAddress = *Pml4TableEntry & (((1ULL << MaxPhyAddrBits) - 1) << 12);

For example:
  PhysicalAddress = (UINT64)Cr3 & ((1ULL << MaxPhyAddrBits) - 1) & (~0xFFF);
  Pml4TableEntry = (UINT64 *)(UINTN)PhysicalAddress;
  Index= (UINTN)(((UINT64)LinearAddress >> 39) & 0x1FF);
  PhysicalAddress = Pml4TableEntry[Index] & ((1ULL << MaxPhyAddrBits) - 1) & 
(~0xFFF);



Thank you
Yao Jiewen


> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Paulo
> Alcantara
> Sent: Friday, December 29, 2017 12:40 PM
> To: edk2-devel@lists.01.org
> Cc: Laszlo Ersek ; Dong, Eric 
> Subject: [edk2] [RFC v4 4/6] UefiCpuPkg/CpuExceptionHandlerLib: Add helper to
> valid memory addresses
> 
> Introduce IsLinearAddressValid() function that will be used for
> validating memory addresses that would get dereferenced during stack
> traces in IA32 and X64 CPU exceptions.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Cc: Eric Dong 
> Cc: Laszlo Ersek 
> Requested-by: Brian Johnson 
> Requested-by: Jiewen Yao 
> Signed-off-by: Paulo Alcantara 
> ---
>  UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c | 382
> 
>  UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.h |  16 +
>  2 files changed, 398 insertions(+)
> 
> diff --git
> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c
> index 867c5c01d6..52b3eb1463 100644
> --- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c
> +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c
> @@ -14,6 +14,9 @@
> 
>  #include "CpuExceptionCommon.h"
> 
> +#include 
> +#include 
> +
>  //
>  // Error code flag indicating whether or not an error code will be
>  // pushed on the stack if an exception occurs.
> @@ -194,3 +197,382 @@ GetPdbFileName (
>  }
>}
>  }
> +
> +/**
> +  Check if a linear address is valid by walking the page tables in 4-level
> +  paging mode.
> +
> +  @param[in]  Cr3 CR3 control register.
> +  @param[in]  MaxPhyAddrBits  MAXPHYADDRBITS bits.
> +  @param[in]  LinearAddress   Linear address to be checked.
> +**/
> +STATIC
> +BOOLEAN
> +Do4LevelPagingModeCheck (
> +  IN UINTNCr3,
> +  IN UINT8MaxPhyAddrBits,
> +  IN UINTNLinearAddress
> +  )
> +{
> +  UINT64 PhysicalAddress;
> +  UINT64 *Pml4TableEntry;
> +  UINT64 *PageDirPtrTableEntry;
> +  UINT64 *PageDirEntry;
> +  UINT64 *PageTableEntry;
> +
> +  //
> +  // In 4-level paging mode, linear addresses are 48 bits wide
> +  //
> +  if ((UINT64)LinearAddress > (1ULL << 48) - 1) {
> +return FALSE;
> +  }
> +
> +  //
> +  // Calculate physical address of PML4E
> +  //
> +  PhysicalAddress = (UINT64)Cr3 & (((1ULL << MaxPhyAddrBits) - 1) << 12);
> +  PhysicalAddress |= (((UINT64)LinearAddress >> 39) & 0x1FF) << 3;
> +
> +  ASSERT ((PhysicalAddress & (sizeof (*Pml4TableEntry) - 1)) == 0);
> +
> +  Pml4TableEntry = (UINT64 *)(UINTN)PhysicalAddress;
> +
> +  //
> +  // Check if a PDPTE is present
> +  //
> +  if ((*Pml4TableEntry & BIT0) == 0) {
> +return FALSE;
> +  }
> +
> +  //
> +  // Calculate physical address of PDPTE
> +  //
> +  PhysicalAddress = *Pml4TableEntry & (((1ULL << MaxPhyAddrBits) - 1) <<
> 12);
> +  PhysicalAddress |= (((UINT64)LinearAddress >> 30) & 0x1FF) << 3;
> +
> +  ASSERT ((PhysicalAddress & (sizeof (*PageDirPtrTableEntry) - 1)) == 0);
> +
> +  PageDirPtrTableEntry = (UINT64 *)(UINTN)PhysicalAddress;
> 

Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard

[Wang, Jian J]

Laszlo,

More explanations:

[UefiCpuPkg\Library\MpInitLib\MpLib.c]
According to the code, the BSP's (CpuInfoInHob[0].ApTopOfStack) is initialized 
to
the bottom of the stack (line 1501) but AP's ApTopOfStack is correctly 
initialized
(line 598). Although my calculation is correct, I think it'd be better to use 
AP's 
ApTopOfStack directly. From this perspective, you're right.

Maybe it'd be better to pass a NULL pointer at line 1501 because BSP doesn't 
need
it anyway.

Regards,
Jian


> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Wang,
> Jian J
> Sent: Thursday, January 04, 2018 8:42 AM
> To: Laszlo Ersek ; edk2-devel@lists.01.org
> Cc: Yao, Jiewen ; Dong, Eric 
> Subject: Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set
> as Stack Guard
> 
> Laszlo,
> 
> I revisited code of MpInitLib. I found that CPU_INFO_IN_HOB.ApTopOfStack
> was assigned to CpuMpData->Buffer in MpInitLibInitialize()
> 
> (line1501)  InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer);
> 
> but in
> 
> (line598)  ApTopOfStack  = CpuMpData->Buffer + (ProcessorNumber + 1) *
> CpuMpData->CpuApStackSize;
> (line608)  InitializeApData (CpuMpData, ProcessorNumber, BistData,
> ApTopOfStack);
> 
> Since InitMpGlobalData() is called just after first situation, my patch is 
> correct.
> 
> I think the problem here is that ApTopOfStack initialized at line 1501 is not
> correct.
> 
> 
> Regards,
> Jian
> 
> 
> > -Original Message-
> > From: Laszlo Ersek [mailto:ler...@redhat.com]
> > Sent: Thursday, January 04, 2018 1:33 AM
> > To: Wang, Jian J ; edk2-devel@lists.01.org
> > Cc: Yao, Jiewen ; Dong, Eric ;
> > Jeff Fan 
> > Subject: Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set
> > as Stack Guard
> >
> > (CC Jeff)
> >
> > Sorry about the delay.
> >
> > I have some light comments below; I expect at least a few of them to be
> > incorrect :)
> >
> > On 12/29/17 09:36, Jian J Wang wrote:
> > > The reason is that DXE part initialization will reuse the stack allocated
> > > at PEI phase, if MP was initialized before. Some code added to check this
> > > situation and use stack base address saved in HOB passed from PEI.
> > >
> > > Cc: Jiewen Yao 
> > > Cc: Eric Dong 
> > > Cc: Laszlo Ersek 
> > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > Signed-off-by: Jian J Wang 
> > > ---
> > >  UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 17 +++--
> > >  1 file changed, 15 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> > b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> > > index 40c1bf407a..05484c9ff3 100644
> > > --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> > > +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> > > @@ -295,6 +295,7 @@ InitMpGlobalData (
> > >UINTN   Index;
> > >EFI_GCD_MEMORY_SPACE_DESCRIPTOR MemDesc;
> > >UINTN   StackBase;
> > > +  CPU_INFO_IN_HOB *CpuInfoInHob;
> > >
> > >SaveCpuMpData (CpuMpData);
> > >
> > > @@ -314,9 +315,18 @@ InitMpGlobalData (
> > >ASSERT (FALSE);
> > >  }
> > >
> > > -for (Index = 0; Index < CpuMpData->CpuCount; ++Index) {
> > > -  StackBase = CpuMpData->Buffer + Index * CpuMpData->CpuApStackSize;
> > > +//
> > > +// DXE will reuse stack allocated for APs at PEI phase if it's 
> > > available.
> > > +// Let's check it here.
> > > +//
> > > +CpuInfoInHob = (CPU_INFO_IN_HOB *)(UINTN)CpuMpData-
> > >CpuInfoInHob;
> > > +if (CpuInfoInHob != NULL && CpuInfoInHob->ApTopOfStack != 0) {
> > > +  StackBase = CpuInfoInHob->ApTopOfStack;
> > > +} else {
> > > +  StackBase = CpuMpData->Buffer;
> > > +}
> >
> > So, if the HOB is not found, then StackBase is set okay.
> >
> > However, I'm unsure about the other case. The
> > CPU_INFO_IN_HOB.ApTopOfStack field identifies the *top* of the stack
> > (highest address, and the stack grows down); however the loop below
> > *increments* StackBase. Given the incrementing nature of the loop,
> > shouldn't we first calculate the actual base (= lowest address) from the
> > CPU_INFO_IN_HOB.ApTopOfStack field?
> >
> > Actually... I'm even more confused. The CpuMpData->CpuInfoInHob field
> > points to an *array* of CPU_INFO_IN_HOB structures. Therefore, for any
> > given processor #N, we should not calculate the stack base as
> >
> >   CpuMpData->CpuInfoInHob->ApTopOfStack + N * CpuMpData-
> > >CpuApStackSize
> >
> > instead we should calculate the stack base as something like:
> >
> >   CpuMpData->CpuInfoInHob[N].ApTopOfStack - CpuMpData->CpuApStackSize
> >
> > See
> > - the InitializeApData() function,
> > - and its call site in the 

Re: [edk2] [RFC v4 5/6] UefiCpuPkg/CpuExceptionHandlerLib: Ensure valid frame/stack pointers

[Yao, Jiewen]

Some suggestion:

1) Would you please use meaning definition for BIT2?

if ((SegmentSelector & BIT2) == 0) {

2) Can we just use (SegmentSelector & ~0x7) for below?

((SegmentSelector >> 3) * 8)

3) Below calculation seems wrong. Should it be: SegDescLimitInBytes = 
(UINTN)SegDescLimit * SIZE_4KB + (SIZE_4KB - 1) ?

  if (SegmentDescriptor->Bits.G == 1) {
SegDescLimitInBytes = (UINTN)SegDescLimit * SIZE_4KB;

Thank you
Yao Jiewen


> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Paulo
> Alcantara
> Sent: Friday, December 29, 2017 12:40 PM
> To: edk2-devel@lists.01.org
> Cc: Laszlo Ersek ; Dong, Eric 
> Subject: [edk2] [RFC v4 5/6] UefiCpuPkg/CpuExceptionHandlerLib: Ensure valid
> frame/stack pointers
> 
> Validate all possible memory dereferences during stack traces in IA32
> and X64 CPU exceptions.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Cc: Eric Dong 
> Cc: Laszlo Ersek 
> Requested-by: Brian Johnson 
> Requested-by: Jiewen Yao 
> Signed-off-by: Paulo Alcantara 
> ---
>  UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c |
> 143 +++-
>  UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c  |
> 75 +-
>  2 files changed, 210 insertions(+), 8 deletions(-)
> 
> diff --git
> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c
> index 25e02fbbc1..9b52d4f6d2 100644
> --- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c
> +++
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c
> @@ -398,6 +398,96 @@ DumpCpuContext (
>  );
>  }
> 
> +/**
> +  Check if a logical address is valid.
> +
> +  @param[in]  SystemContext  Pointer to EFI_SYSTEM_CONTEXT.
> +  @param[in]  SegmentSelectorSegment selector.
> +  @param[in]  Offset Offset or logical address.
> +**/
> +STATIC
> +BOOLEAN
> +IsLogicalAddressValid (
> +  IN  EFI_SYSTEM_CONTEXT   SystemContext,
> +  IN  UINT16   SegmentSelector,
> +  IN  UINTNOffset
> +  )
> +{
> +  IA32_SEGMENT_DESCRIPTOR  *SegmentDescriptor;
> +  UINT32   SegDescBase;
> +  UINT32   SegDescLimit;
> +  UINTNSegDescLimitInBytes;
> +
> +  //
> +  // Check for valid input parameters
> +  //
> +  if (SegmentSelector == 0 || Offset == 0) {
> +return FALSE;
> +  }
> +
> +  //
> +  // Check whether to look for a segment descriptor in GDT or LDT table
> +  //
> +  if ((SegmentSelector & BIT2) == 0) {
> +//
> +// Get segment descriptor from GDT table
> +//
> +SegmentDescriptor =
> +  (IA32_SEGMENT_DESCRIPTOR *)(
> +(UINTN)SystemContext.SystemContextIa32->Gdtr[0] +
> +((SegmentSelector >> 3) * 8)
> +);
> +  } else {
> +//
> +// Get segment descriptor from LDT table
> +//
> +SegmentDescriptor =
> +  (IA32_SEGMENT_DESCRIPTOR *)(
> +(UINTN)SystemContext.SystemContextIa32->Ldtr +
> +((SegmentSelector >> 3) * 8)
> +);
> +  }
> +
> +  //
> +  // Get segment descriptor's base address
> +  //
> +  SegDescBase = SegmentDescriptor->Bits.BaseLow |
> +(SegmentDescriptor->Bits.BaseMid << 16) |
> +(SegmentDescriptor->Bits.BaseHigh << 24);
> +
> +  //
> +  // Get segment descriptor's limit
> +  //
> +  SegDescLimit = SegmentDescriptor->Bits.LimitLow |
> +(SegmentDescriptor->Bits.LimitHigh << 16);
> +
> +  //
> +  // Calculate segment descriptor's limit in bytes
> +  //
> +  if (SegmentDescriptor->Bits.G == 1) {
> +SegDescLimitInBytes = (UINTN)SegDescLimit * SIZE_4KB;
> +  } else {
> +SegDescLimitInBytes = SegDescLimit;
> +  }
> +
> +  //
> +  // Make sure to not access beyond a segment limit boundary
> +  //
> +  if (Offset + SegDescBase > SegDescLimitInBytes) {
> +return FALSE;
> +  }
> +
> +  //
> +  // Check if the translated logical address (or linear address) is valid
> +  //
> +  return IsLinearAddressValid (
> +SystemContext.SystemContextIa32->Cr0,
> +SystemContext.SystemContextIa32->Cr3,
> +SystemContext.SystemContextIa32->Cr4,
> +Offset + SegDescBase
> +);
> +}
> +
>  /**
>Dump stack trace.
> 
> @@ -459,6 +549,20 @@ DumpStackTrace (
>InternalPrintMessage ("\nCall trace:\n");
> 
>for (;;) {
> +//
> +// Check for valid frame pointer
> +//
> +if (!IsLogicalAddressValid (SystemContext,
> +SystemContext.SystemContextIa32->Ss,
> +(UINTN)Ebp + 4) ||
> +!IsLogicalAddressValid (SystemContext,
> +SystemContext.SystemContextIa32->Ss,
> +(UINTN)Ebp)) {
> +  InternalPrintMessage ("%a: attempted to dereference an 

Re: [edk2] [Patch 0/5] Refine PXE driver code logic

[Fu, Siyuan]

Hi, Sriram

So far as I know we don't have plan to remove the MdeModulePkg copy, but we are 
taking priority to fix the NetworkPkg ones for these duplicate drivers. 

BestRegards
Fu Siyuan

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
> Subramanian, Sriram
> Sent: Tuesday, January 2, 2018 1:51 PM
> To: Fu, Siyuan ; edk2-devel@lists.01.org
> Subject: Re: [edk2] [Patch 0/5] Refine PXE driver code logic
> 
> Hi Siyuan,
> 
> Some of these problems also exist in the
> MdeModulePkg/Universal/Network/UefiPxeBcDxe/. Is there a plan to fix them
> there as well, or will the MdeModulePkg copy of the drivers be removed
> sometime in the near future?
> 
> Thanks,
> Sriram.
> 
> -Original Message-
> From: Subramanian, Sriram
> Sent: Tuesday, January 2, 2018 11:15 AM
> To: 'Fu Siyuan' ; edk2-devel@lists.01.org
> Subject: RE: [edk2] [Patch 0/5] Refine PXE driver code logic
> 
> Patch Series Reviewed-by: Sriram Subramanian 
> 
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Fu
> Siyuan
> Sent: Tuesday, January 2, 2018 10:58 AM
> To: edk2-devel@lists.01.org
> Subject: [edk2] [Patch 0/5] Refine PXE driver code logic
> 
> See each patch file for details.
> 
> Fu Siyuan (5):
>   NetworkPkg: Abort the PXE process if DHCP has been started by other
> instance.
>   NetworkPkg: Check allocated buffer pointer before use.
>   NetworkPkg: Fix memory leak problem in PXE driver.
>   NetworkPkg: Add assert for buffer pointer from DHCP driver.
>   NetworkPkg: Update PXE driver to check for NULL pointer before use it.
> 
>  NetworkPkg/UefiPxeBcDxe/PxeBcDhcp4.c  | 13 ++---
>  NetworkPkg/UefiPxeBcDxe/PxeBcDhcp6.c  | 32 +-
> --
>  NetworkPkg/UefiPxeBcDxe/PxeBcDriver.c |  5 -
>  NetworkPkg/UefiPxeBcDxe/PxeBcImpl.c   | 20 
>  4 files changed, 43 insertions(+), 27 deletions(-)
> 
> --
> 2.13.0.windows.1
> 
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [RFC v4 0/6] Stack trace support in X64 exception handling

[Yao, Jiewen]

Thanks Paulo.

I tried to apply the patch series to latest code, but fail with below message.

Have you rebased to latest code?

===
git.exe am --3way --ignore-space-change --keep-cr 
"C:\home\EdkIIGit\edk2\[edk2]-[RFC-v4-1-6]-UefiCpuPkg-CpuExceptionHandlerLib-X64-Add-stack-trace-support.patch"
Applying: UefiCpuPkg/CpuExceptionHandlerLib/X64: Add stack trace support
Using index info to reconstruct a base tree...
M   UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c
Falling back to patching base and 3-way merge...
Auto-merging 
UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c
CONFLICT (content): Merge conflict in 
UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c
Patch failed at 0001 UefiCpuPkg/CpuExceptionHandlerLib/X64: Add stack trace 
support
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

.git/rebase-apply/patch:13: trailing whitespace.
//
.git/rebase-apply/patch:14: trailing whitespace.
// Unknown PDB file name
.git/rebase-apply/patch:15: trailing whitespace.
//
.git/rebase-apply/patch:16: trailing whitespace.
GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8 *mUnknownPdbFileName = "";
.git/rebase-apply/patch:17: trailing whitespace.

warning: squelched 369 whitespace errors
warning: 374 lines add whitespace errors.
error: Failed to merge in the changes.

Fail
=

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Paulo
> Alcantara
> Sent: Friday, December 29, 2017 11:49 AM
> To: edk2-devel@lists.01.org
> Cc: Rick Bramley ; Dong, Eric
> ; Andrew Fish ; Yao, Jiewen
> ; Laszlo Ersek 
> Subject: [edk2] [RFC v4 0/6] Stack trace support in X64 exception handling
> 
> Hi,
> 
> This series adds stack trace support during IA32 and X64 CPU exceptions.
> 
> Informations like back trace, stack contents and image module names
> (that were part of the call stack) will be dumped out.
> 
> The current limitation is that it relies on available frame pointers
> (GCC only) in order to successfully unwind the stack.
> 
> (Sorry for the very long delay - I was very busy working on something
>  else and then went to vacations)
> 
> Jiewen,
> 
> I have tested it with VS2015x86 and the stack trace just hanged when
> printing out the first EIP (that is, no frame pointer at all).
> 
> Thanks!
> Paulo
> 
> Repo:   https://github.com/pcacjr/edk2.git
> Branch: stacktrace_v4
> 
> Cc: Rick Bramley 
> Cc: Andrew Fish 
> Cc: Eric Dong 
> Cc: Laszlo Ersek 
> Cc: Brian Johnson 
> Cc: Jeff Fan 
> Cc: Jiewen Yao 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Paulo Alcantara 
> ---
> 
> v1 -> v2:
>   * Add IA32 arch support (GCC toolchain only)
>   * Replace hard-coded stack alignment value (16) with
> CPU_STACK_ALIGNMENT.
>   * Check for proper stack and frame pointer alignments.
>   * Fix initialization of UnwoundStacksCount to 1.
>   * Move GetPdbFileName() to common code since it will be used by both
> IA32 and X64 implementations.
> 
> v2 -> v3:
>   * Fixed wrong assumption about "RIP < ImageBase" to start searching
> for another PE/COFF image. That is, RIP may point to lower and
> higher addresses for any other PE/COFF images. Both IA32 & X64.
> (Thanks Andrew & Jiewen)
>   * Fixed typo: unwond -> unwound. Both IA32 & X64. (Thanks Brian)
> 
> v3 -> v4:
>   * Validate all frame/stack pointer addresses before dereferencing them
> as requested by Brian & Jiewen.
>   * Correctly print out IP addresses during the stack traces (by Jeff)
> 
> Paulo Alcantara (6):
>   UefiCpuPkg/CpuExceptionHandlerLib/X64: Add stack trace support
>   UefiCpuPkg/CpuExceptionHandlerLib: Export GetPdbFileName()
>   UefiCpuPkg/CpuExceptionHandlerLib/Ia32: Add stack trace support
>   UefiCpuPkg/CpuExceptionHandlerLib: Add helper to valid memory
> addresses
>   UefiCpuPkg/CpuExceptionHandlerLib: Ensure valid frame/stack pointers
>   UefiCpuPkg/CpuExceptionHandlerLib: Correctly print IP addresses
> 
>  UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c
> | 484 ++--
>  UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.h
> |  41 +-
>  UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c |
> 445 +-
>  UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c  |
> 384 +++-
>  4 files changed, 1296 insertions(+), 58 deletions(-)
> 
> --
> 2.14.3
> 
> 

Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard

[Wang, Jian J]

Laszlo,

I revisited code of MpInitLib. I found that CPU_INFO_IN_HOB.ApTopOfStack
was assigned to CpuMpData->Buffer in MpInitLibInitialize()

(line1501)  InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer);

but in 

(line598)  ApTopOfStack  = CpuMpData->Buffer + (ProcessorNumber + 1) * 
CpuMpData->CpuApStackSize;
(line608)  InitializeApData (CpuMpData, ProcessorNumber, BistData, 
ApTopOfStack);

Since InitMpGlobalData() is called just after first situation, my patch is 
correct.

I think the problem here is that ApTopOfStack initialized at line 1501 is not 
correct.


Regards,
Jian


> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Thursday, January 04, 2018 1:33 AM
> To: Wang, Jian J ; edk2-devel@lists.01.org
> Cc: Yao, Jiewen ; Dong, Eric ;
> Jeff Fan 
> Subject: Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set
> as Stack Guard
> 
> (CC Jeff)
> 
> Sorry about the delay.
> 
> I have some light comments below; I expect at least a few of them to be
> incorrect :)
> 
> On 12/29/17 09:36, Jian J Wang wrote:
> > The reason is that DXE part initialization will reuse the stack allocated
> > at PEI phase, if MP was initialized before. Some code added to check this
> > situation and use stack base address saved in HOB passed from PEI.
> >
> > Cc: Jiewen Yao 
> > Cc: Eric Dong 
> > Cc: Laszlo Ersek 
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Jian J Wang 
> > ---
> >  UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 17 +++--
> >  1 file changed, 15 insertions(+), 2 deletions(-)
> >
> > diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> > index 40c1bf407a..05484c9ff3 100644
> > --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> > +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> > @@ -295,6 +295,7 @@ InitMpGlobalData (
> >UINTN   Index;
> >EFI_GCD_MEMORY_SPACE_DESCRIPTOR MemDesc;
> >UINTN   StackBase;
> > +  CPU_INFO_IN_HOB *CpuInfoInHob;
> >
> >SaveCpuMpData (CpuMpData);
> >
> > @@ -314,9 +315,18 @@ InitMpGlobalData (
> >ASSERT (FALSE);
> >  }
> >
> > -for (Index = 0; Index < CpuMpData->CpuCount; ++Index) {
> > -  StackBase = CpuMpData->Buffer + Index * CpuMpData->CpuApStackSize;
> > +//
> > +// DXE will reuse stack allocated for APs at PEI phase if it's 
> > available.
> > +// Let's check it here.
> > +//
> > +CpuInfoInHob = (CPU_INFO_IN_HOB *)(UINTN)CpuMpData-
> >CpuInfoInHob;
> > +if (CpuInfoInHob != NULL && CpuInfoInHob->ApTopOfStack != 0) {
> > +  StackBase = CpuInfoInHob->ApTopOfStack;
> > +} else {
> > +  StackBase = CpuMpData->Buffer;
> > +}
> 
> So, if the HOB is not found, then StackBase is set okay.
> 
> However, I'm unsure about the other case. The
> CPU_INFO_IN_HOB.ApTopOfStack field identifies the *top* of the stack
> (highest address, and the stack grows down); however the loop below
> *increments* StackBase. Given the incrementing nature of the loop,
> shouldn't we first calculate the actual base (= lowest address) from the
> CPU_INFO_IN_HOB.ApTopOfStack field?
> 
> Actually... I'm even more confused. The CpuMpData->CpuInfoInHob field
> points to an *array* of CPU_INFO_IN_HOB structures. Therefore, for any
> given processor #N, we should not calculate the stack base as
> 
>   CpuMpData->CpuInfoInHob->ApTopOfStack + N * CpuMpData-
> >CpuApStackSize
> 
> instead we should calculate the stack base as something like:
> 
>   CpuMpData->CpuInfoInHob[N].ApTopOfStack - CpuMpData->CpuApStackSize
> 
> See
> - the InitializeApData() function,
> - and its call site in the ApWakeupFunction() function.
> 
> (To my surprise, I personally modified InitializeApData() earlier, in
> commit dd3fa0cd72de ("UefiCpuPkg/MpInitLib: support 64-bit AP stack
> addresses", 2016-11-17) -- I've totally forgotten about that by now!)
> 
> What do you think?
> 
> >
> > +for (Index = 0; Index < CpuMpData->CpuCount; ++Index) {
> >Status = gDS->GetMemorySpaceDescriptor (StackBase, );
> >ASSERT_EFI_ERROR (Status);
> >
> > @@ -326,6 +336,9 @@ InitMpGlobalData (
> >MemDesc.Attributes | EFI_MEMORY_RP
> >);
> >ASSERT_EFI_ERROR (Status);
> > +
> > +  DEBUG ((DEBUG_VERBOSE, "Stack Guard set at %x [cpu%d]!\n",
> StackBase, Index));
> 
> StackBase has type UINTN, and so it should not be printed with %x. It
> should be cast to (UINT64), and then printed with %Lx.
> 
> Similarly, Index has type UINTN. It should not be printed with %d. It
> should be cast to (UINT64) and printed with %Lu.
> 
> 
> > +  StackBase += CpuMpData->CpuApStackSize;
> 
> Again, I don't think the simple increment applies when 

Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard

[Laszlo Ersek]

(CC Jeff)

Sorry about the delay.

I have some light comments below; I expect at least a few of them to be
incorrect :)

On 12/29/17 09:36, Jian J Wang wrote:
> The reason is that DXE part initialization will reuse the stack allocated
> at PEI phase, if MP was initialized before. Some code added to check this
> situation and use stack base address saved in HOB passed from PEI.
> 
> Cc: Jiewen Yao 
> Cc: Eric Dong 
> Cc: Laszlo Ersek 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang 
> ---
>  UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 17 +++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c 
> b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> index 40c1bf407a..05484c9ff3 100644
> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> @@ -295,6 +295,7 @@ InitMpGlobalData (
>UINTN   Index;
>EFI_GCD_MEMORY_SPACE_DESCRIPTOR MemDesc;
>UINTN   StackBase;
> +  CPU_INFO_IN_HOB *CpuInfoInHob;
>  
>SaveCpuMpData (CpuMpData);
>  
> @@ -314,9 +315,18 @@ InitMpGlobalData (
>ASSERT (FALSE);
>  }
>  
> -for (Index = 0; Index < CpuMpData->CpuCount; ++Index) {
> -  StackBase = CpuMpData->Buffer + Index * CpuMpData->CpuApStackSize;
> +//
> +// DXE will reuse stack allocated for APs at PEI phase if it's available.
> +// Let's check it here.
> +//
> +CpuInfoInHob = (CPU_INFO_IN_HOB *)(UINTN)CpuMpData->CpuInfoInHob;
> +if (CpuInfoInHob != NULL && CpuInfoInHob->ApTopOfStack != 0) {
> +  StackBase = CpuInfoInHob->ApTopOfStack;
> +} else {
> +  StackBase = CpuMpData->Buffer;
> +}

So, if the HOB is not found, then StackBase is set okay.

However, I'm unsure about the other case. The
CPU_INFO_IN_HOB.ApTopOfStack field identifies the *top* of the stack
(highest address, and the stack grows down); however the loop below
*increments* StackBase. Given the incrementing nature of the loop,
shouldn't we first calculate the actual base (= lowest address) from the
CPU_INFO_IN_HOB.ApTopOfStack field?

Actually... I'm even more confused. The CpuMpData->CpuInfoInHob field
points to an *array* of CPU_INFO_IN_HOB structures. Therefore, for any
given processor #N, we should not calculate the stack base as

  CpuMpData->CpuInfoInHob->ApTopOfStack + N * CpuMpData->CpuApStackSize

instead we should calculate the stack base as something like:

  CpuMpData->CpuInfoInHob[N].ApTopOfStack - CpuMpData->CpuApStackSize

See
- the InitializeApData() function,
- and its call site in the ApWakeupFunction() function.

(To my surprise, I personally modified InitializeApData() earlier, in
commit dd3fa0cd72de ("UefiCpuPkg/MpInitLib: support 64-bit AP stack
addresses", 2016-11-17) -- I've totally forgotten about that by now!)

What do you think?

>  
> +for (Index = 0; Index < CpuMpData->CpuCount; ++Index) {
>Status = gDS->GetMemorySpaceDescriptor (StackBase, );
>ASSERT_EFI_ERROR (Status);
>  
> @@ -326,6 +336,9 @@ InitMpGlobalData (
>MemDesc.Attributes | EFI_MEMORY_RP
>);
>ASSERT_EFI_ERROR (Status);
> +
> +  DEBUG ((DEBUG_VERBOSE, "Stack Guard set at %x [cpu%d]!\n", StackBase, 
> Index));

StackBase has type UINTN, and so it should not be printed with %x. It
should be cast to (UINT64), and then printed with %Lx.

Similarly, Index has type UINTN. It should not be printed with %d. It
should be cast to (UINT64) and printed with %Lu.


> +  StackBase += CpuMpData->CpuApStackSize;

Again, I don't think the simple increment applies when the
CpuMpData->CpuInfoInHob array exists.

>  }
>}
>  
> 

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

Re: [edk2] [edk2-CCodingStandardsSpecification PATCH 5/5] Fix Chapter 5 Typos

[Laszlo Ersek]

On 01/03/18 12:22, evan.ll...@arm.com wrote:
> From: Evan Lloyd 
> 
> 5.1.1 - Replace "less" with "fewer" (because columns is plural and
> countable)
> 5.1.5 - Correct tense.  (because the C specification still defines...)
> Insert full stop.
> Insert comma.
> 5.1.8 - Correct "provided" to "proven".
> 5.1.9 - remove hanging "This."
> 5.2.3.1 - replace "is comprised of" with "comprises" (comprise means
>   "consists of", so "comprised of" is a solecism.
>   Remove use of tab, as they are forbidden.
>   Remove -- before date in copyright header (None of the edk2
>   files have it).
> 5.4 - Add indent to comment text.
> 5.6.1.2 - Fix copy/paste text (from UEFI spec).
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Evan Lloyd 
> ---
>  5_source_files/52_spacing.md|  9 +
>  5_source_files/54_code_file_structure.md|  4 ++--
>  5_source_files/56_declarations_and_types.md |  4 ++--
>  5_source_files/README.md| 18 +-
>  4 files changed, 18 insertions(+), 17 deletions(-)
> 
> diff --git a/5_source_files/52_spacing.md b/5_source_files/52_spacing.md
> index 
> ddeabf7753a8713bf04e143d6e2e9bccf881f691..3c79f4e4ee91bcd4035d6cf7d8d32f1bb9c7756f
>  100644
> --- a/5_source_files/52_spacing.md
> +++ b/5_source_files/52_spacing.md
> @@ -249,7 +249,7 @@ And the comment will end with:
>  **/
>  ```
>  
> -The File Heading comment block is comprised of the following sections: File
> +The File Heading comment block comprises the following sections: File
>  Description, Copyright, License, and the optional Specification Reference and
>  Glossary sections.
>  
> @@ -266,8 +266,9 @@ Glossary sections.
>  **/
>  ```
>  
> -The following example begins each body line with a tab (two spaces). This is
> -the preferred indentation, but two tabs (four spaces) is also acceptable.
> +The following example begins each body line with an indent (two spaces).
> +This is the preferred indentation, but a double indent (four spaces) is also
> +acceptable.
>  
>   Example
>  
> @@ -278,7 +279,7 @@ the preferred indentation, but two tabs (four spaces) is 
> also acceptable.
>Detailed description of the file’s contents and other useful
>information for a person viewing the file for the first time.
>  
> -  Copyright (C) --20XX, Acme Corporation. All rights reserved.
> +  Copyright (C) 20XX, Acme Corporation. All rights reserved.
>This program and the accompanying materials
>are licensed and made available under the terms and conditions of
>the BSD License which accompanies this distribution. The full
> diff --git a/5_source_files/54_code_file_structure.md 
> b/5_source_files/54_code_file_structure.md
> index 
> 8cc9f4f61412b07f765d80d7b680c6dd38b838c1..ac999aae99ae9cfd8b6f97dc483e51bfbd7c7a0b
>  100644
> --- a/5_source_files/54_code_file_structure.md
> +++ b/5_source_files/54_code_file_structure.md
> @@ -68,8 +68,8 @@ these are C files with an extension of "`.c`".
>  
>  /* Function Definitions */
>  
> -/* If this is a protocol definition, the
> -protocol structure is defined and initialized here.
> +/* If this is a protocol definition, the protocol structure is defined and
> +  initialized here.
>  */
>  ```

So, I'm a bit hesitant about this. In edk2 we use /* comments */ (to my
knowledge, that is) only when they have to be embedded in replacement
texts of function-like macros, or in expressions that continue after the
comment on the same line. IOW, multi-line /* comments */ are mostly
unused, and I'm unsure if we should prepend a "*" to "initialized here".

On the other hand, the comments in this section look like they should be
fully replaced by actual code. In that sense the comment style we use
here does not matter.


>  
> diff --git a/5_source_files/56_declarations_and_types.md 
> b/5_source_files/56_declarations_and_types.md
> index 
> ec1803d980e1fa808b9dc515cdffbc4b47437435..5c57834fe1195b5487f0f59e8a0385e44d91aff4
>  100644
> --- a/5_source_files/56_declarations_and_types.md
> +++ b/5_source_files/56_declarations_and_types.md
> @@ -43,9 +43,9 @@ or from common EFI data types.
>  The corresponding EFI types must be used instead.
>  
>  "EFI Data Types" below contains the common data types that are referenced in
> -the interface definitions defined by this specification. Per the _UEFI
> -Specification_, version 2.3.1:
> +the interface definitions defined by the UEFI specification.
>  
> +Per the _UEFI Specification_, version 2.3.1:
>  "Unless otherwise specified, all data types are naturally aligned. Structures
>  are aligned on boundaries equal to the largest internal datum of the 
> structure,
>  and internal data is implicitly padded to achieve natural alignment."
> diff --git a/5_source_files/README.md b/5_source_files/README.md
> index 
> a93492db4f0f17e14d9c2c3c95e57cf0f6cc911e..a443148138f2abaf6b9131f4758858a4a5f45fd3
>  

Re: [edk2] [RFC v4 4/6] UefiCpuPkg/CpuExceptionHandlerLib: Add helper to valid memory addresses

[Brian J. Johnson]

On 12/28/2017 10:39 PM, Paulo Alcantara wrote:

+  //
+  // Check if paging is disabled
+  //
+  if ((Cr0 & BIT31) == 0) {
+//
+// If CR4.PAE bit is set, then the linear (or physical) address supports
+// only up to 36 bits.
+//
+if (((Cr4 & BIT5) != 0 && (UINT64)LinearAddress > 0xFULL) ||
+LinearAddress > 0x) {
+  return FALSE;
+}
+
+return TRUE;
+  }


Paulo,

The logic there doesn't look quite right:  if LinearAddress is between 
2^32 and 2^36-1, this code will always return FALSE, even if CR4.PAE is 
set.  Shouldn't it be:


   if ((UINT64)LinearAddress > 0xFULL ||
   ((Cr4 & BIT5) == 0 && LinearAddress > 0x)) {
 return FALSE;
   }

(I haven't examined all the code in detail, I just happened to notice 
this issue.)


This bug should get fixed before pushing this series.  I also have some 
more general design questions, which shouldn't hold up pushing the 
series, but I think merit some discussion:


This is great code for validating addresses in general, especially when 
guard pages are in use for NULL pointers, stack overflow, etc.  Thanks 
for adding it!  But for [er]sp and [er]bp validation, don't you really 
just want to know if the address is in the expected stack range?  Maybe 
the code which sets up the stack could communicate the valid range to 
CpuExceptionHandlerLib somehow.  It could use special debug register 
values like 
SourceLevelDebugPkg/Library/PeCoffExtraActionLibDebug/PeCoffExtraActionLib.c 
does.  Or perhaps it could use dynamic PCDs (although I don't know that 
it's a good idea to go looking up PCDs in an exception handler.)  Or 
maybe there's a more straightforward way  It would have to take AP 
stacks into account, and probably SetJump/LongJump as well.  That may or 
may not be simpler than the current code


More generally, I'd like to see some sort of platform-specific callout 
to further validate addresses.  Not all mapped addresses, or addresses 
up to the architectural limit, are safe to access.  For instance, reads 
to SMRAM outside of SMM will cause exceptions.  Also, we wouldn't want 
to go backtracing through MMIO or MMCFG space:  reads there could 
potentially have side effects on the hardware.


The rules can also vary at different points in boot.  For example, 
before memory is initialized, Intel Xeon processors generally execute 
32-bit code in cache-as-RAM mode, where the caches are jury-rigged to 
operate as temporary storage while the memory setup code is running.  In 
CAR mode, only a few address ranges can be accessed without causing 
machine checks:  the cache-as-RAM range containing the stack, heap, and 
HOB list, the architectural firmware range below 4G, and a few specific 
MMCFG and MMIO ranges.


So I'd like to suggest that you define an AddressValidationLib library 
class, which provides a routine which takes an address (or an address 
range?) and an indication of the intended use (memory read, memory 
write, execute/disassemble code, stack dump, IO, ...), and returns a 
value specifying if the access is:

- safe (IsLinearAddressValid() should return TRUE)
- unsafe (IsLinearAddressValid() should return FALSE)
- unknown (IsLinearAddressValid() should perform its other tests)

You can supply a NULL instance which always returns "unknown" for 
platforms which don't want to perform their own validation.


Thanks,
--
Brian J. Johnson
Enterprise X86 Lab

Hewlett Packard Enterprise
brian.john...@hpe.com

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

Re: [edk2] [edk2-CCodingStandardsSpecification PATCH 4/5] Fix Chapter 4 Typo

[Laszlo Ersek]

On 01/03/18 12:22, evan.ll...@arm.com wrote:
> From: Evan Lloyd 
> 
> 4.1.3.2 - remove "See" from 'as specified in See "File Heading"'
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Evan Lloyd 
> ---
>  4_naming_conventions/README.md | 418 ++--
>  1 file changed, 209 insertions(+), 209 deletions(-)

Looks like this file underwent a line terminator conversion as well.

If the file is currently not using CRLF, then I agree it should be
converted; however, the conversion should be please separated from the
typo fix.

... Hmm, the file already seems to use CRLF. So I think the conversion
was to LF, and must have been unintended.

Can you please resubmit without the line terminator changes?

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

Re: [edk2] [edk2-CCodingStandardsSpecification PATCH 3/5] Fix Chapter 3 Typos

[Laszlo Ersek]

On 01/03/18 12:22, evan.ll...@arm.com wrote:
> From: Evan Lloyd 
> 
> 3_quick_reference.md has some obviously invalid extra characters at the
> end of line 55 ( ",).  This patch removes them.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Evan Lloyd 
> ---
>  3_quick_reference.md | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/3_quick_reference.md b/3_quick_reference.md
> index 
> 13045d297b3be848d91f9be05380454b4cc2fe91..9367e422fb0337007409d6597913cb43da92e0c5
>  100644
> --- a/3_quick_reference.md
> +++ b/3_quick_reference.md
> @@ -55,7 +55,7 @@
>any file using the abbreviation or acronym. See "Abbreviation Usage" and
>"Glossary".
>  
> -* There is no limit to name lengths. A length of 10 to 30 characters is ",
> +* There is no limit to name lengths. A length of 10 to 30 characters is
>recommended. See "File Names" & "Identifiers that are always reserved".
>  
>  * File names must not start with numbers. See "File Names".
> 

Reviewed-by: Laszlo Ersek 

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

Re: [edk2] [edk2-CCodingStandardsSpecification PATCH 2/5] Fix Chapter 2 Typos

[Laszlo Ersek]

On 01/03/18 12:22, evan.ll...@arm.com wrote:
> From: Evan Lloyd 
> 
> 2.1 Accessibility - remove erroneous "as"
> 2.1 Confirmation - insert missing full stop
> 2.1 Forgiveness - excise superfluous "errors"
> 2.1 Standard techniques - remove redundant "be to"
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Evan Lloyd 
> ---
>  2_guiding_principles.md | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/2_guiding_principles.md b/2_guiding_principles.md
> index 
> a7759f27dcf71a948b903332c9bc14946e445cd8..5a51225b65dec2159a4fb94481920666c0d042ff
>  100644
> --- a/2_guiding_principles.md
> +++ b/2_guiding_principles.md
> @@ -47,7 +47,7 @@ The following is an alphabetical list of software design 
> principles:
>  **Accessibility**
>  
>  This entails designing objects and environments to be usable, with no
> -modification, by the greatest number of people as possible, including people
> +modification, by the greatest number of people possible, including people
>  with varying educational and social backgrounds, as well as those with motor 
> or
>  sensory challenges.
>  
> @@ -68,7 +68,7 @@ shortterm memory, as well as to accommodate its limits.
>  This is a technique used for critical actions, inputs, or commands.
>  Confirmations are primarily used to prevent unintended actions. Minimize 
> errors
>  in critical or irreversible operations with confirmations. If you overuse
> -confirmations, expect that they will be ignored Avoid overusing confirmations
> +confirmations, expect that they will be ignored. Avoid overusing 
> confirmations
>  to ensure that they remain unexpected and uncommon; otherwise, they may be
>  ignored. Use a two-step operation for hardware confirmations and dialogs for
>  software confirmations.
> @@ -97,7 +97,7 @@ about the assumptions you make.
>  **Forgiveness**
>  
>  Design to help users avoid errors and reduce the negative consequences of
> -errors any errors made. Recommended methods for achieving design forgiveness
> +any errors made. Recommended methods for achieving design forgiveness
>  include affordances, reversibility of actions, and safety nets. Effectively
>  designing for forgiveness results in a design needing minimal confirmations,
>  warnings, and help.
> @@ -171,7 +171,7 @@ classes of platforms from embedded systems to massively 
> parallel computers.
>  
>  Greater reliance on unique or exotic pieces makes a system harder to
>  understand, and more intimidating for someone trying to understand it the 
> first
> -time. Using standardized, common approaches should be to give the whole 
> system
> +time. Using standardized, common approaches should give the whole system
>  a familiar feeling. This standardization is one of the primary goals of this
>  document.
>  
> 


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

Re: [edk2] [edk2-CCodingStandardsSpecification PATCH 1/5] Fix Chapter 1 Typos

[Laszlo Ersek]

On 01/03/18 12:22, evan.ll...@arm.com wrote:
> From: Evan Lloyd 
> 
> 1.1 Abstract - replace "then" with "can" (to make meaningful)
> 1.2 Rationale - change
>   "commit to conforming to standards of this specification"
>   to   commit to conforming with the standards of this specification
> 1.3 Scope - add missing "to" in "an attempt make you aware of your
>   actions"
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Evan Lloyd 
> ---
>  1_introduction.md | 12 ++--
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/1_introduction.md b/1_introduction.md
> index 
> c22c04c9f76d2352e4f294c0e70f5ebf787f7054..5471a6bf5e94df6dd0c5fd0a4b5020c77d1f8ca0
>  100644
> --- a/1_introduction.md
> +++ b/1_introduction.md
> @@ -62,7 +62,7 @@ This specification addresses the chronic problem of 
> providing accurate
>  documentation of the code base by embedding the documentation within the 
> code.
>  While this does not guarantee that the documentation will be kept up to date,
>  it significantly increases the chances. A document generation system, 
> Doxygen,
> -then produce formatted documentation by extracting information from specially
> +can produce formatted documentation by extracting information from specially
>  formatted comment blocks and the syntactic elements of the code.
>  
>  This specification presents protocol standards that will ensure that the
> @@ -97,10 +97,10 @@ wide support. On the downside in that each developer's C 
> code could
>  lack of uniformity makes understanding and maintaining the code very 
> difficult.
>  
>  Uniformity is the key theme of these rules. You may disagree with some of our
> -decisions. Nevertheless, we ask that you commit to conforming to standards of
> -this specification. Also, there are pitfalls inherent in the C language that
> -this style guide may help you to avoid. The goal of this document is making
> -you, and those who follow you, more productive.
> +decisions. Nevertheless, we ask that you commit to conforming with the
> +standards of this specification. Also, there are pitfalls inherent in the C
> +language that this style guide may help you to avoid. The goal of this 
> document
> +is making you, and those who follow you, more productive.
>  
>  Some of the strict rules for protocol and driver construction may seem overly
>  onerous. Don't panic - there is a method to our madness - we intend to

OK, a risky observation from non-native speaker to native speaker:

To my knowledge, it's "conform to", not "conform with". Thus, an update
to this paragraph looks unnecessary to me, but I'm more than prepared to
be corrected :)

(Also, I'd replace "help you to avoid" with "help you avoid", i.e. drop
the "to". But, that's a separate thing, and the same caveat about being
a non-native speaker applies :) )

Looks fine to me otherwise.

Thanks!
Laszlo

> @@ -161,7 +161,7 @@ Topics covered in this coding standard include:
>  * Commenting rules
>  * Doxygen
>  
> -These guidelines represent an attempt make you aware of your actions, because
> +These guidelines represent an attempt to make you aware of your actions, 
> because
>  those actions affect the future readers and maintainers of the code you 
> produce.
>  
>  Pre-existing code ported to the EDK II environment does not have to conform 
> to
> 

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

Re: [edk2] [PATCH v2 01/13] ArmPlatformPkg: Tidy Lcd code: Coding standard

[Evan Lloyd]

Hi Ard

> -Original Message-
> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> Sent: 02 January 2018 15:21
> To: Evan Lloyd 
> Cc: edk2-devel@lists.01.org; Matteo Carlini ;
> eif.lindh...@linaro.org; nd 
> Subject: Re: [PATCH v2 01/13] ArmPlatformPkg: Tidy Lcd code: Coding
> standard
> 
> On 2 January 2018 at 15:11, Evan Lloyd  wrote:
> > Hi Ard.
> > One aim of these changes is to get those files we have to play with into a
> state where a beautifier like indent, astyle,  or clang-format can be used to
> help tidy our changes.  (NOTE, we do not have that fully working yet, but
> they do help.)  In a world where we have to play with several contradictory
> formatting standards (not just EDK2) then anything that can help is
> welcome.
> > Of the changes made:
> > Fixing the include guards: is a small improvement.  (Ideally
> patchcheck should reject these.)
> > Reducing lines to 80 columns: makes Leif (at least) happy, and 
> > aligns
> with formatter behaviour.
> > Correcting Doxygen format comments: prevents Doxygen generating
> gibberish.
> > Spaces before '(': Maintains consistency, and aligns with desired
> formatter behaviour.
> >
> 
> To be honest, this is an aspect I hadn't considered at all. It would be
> excellent if we could use tooling to fix our code wrt to coding style, and if
> changes such as these bring us closer to that goal, I am all for it.

 [[Evan Lloyd]] Excellent news - now you know why we were doing all those 
inexplicable things.  I really should have explained this more clearly before.

> 
> Would it be feasible to run that on entire packages, i.e., ArmPkg and
> ArmPlatformPkg?

 [[Evan Lloyd]]  As it stands, none of the tools is very happy with the EDK2 
style, and they tend to miss meeting it in different ways.
(As an aside - one common problem is that I have found no means of coercing 
them to force the ");" at the end of a function call onto its own line.  Your 
recent CCS change to make that optional helps a lot with that.)  However, I 
have so far failed to get any of the above tools to do exactly what we want.  
All that I can achieve is to generate a formatted view that can be compared 
against the original with a visual program like kdiff3 - highlighting some 
formatting improvements that can then be copied across, whilst the mismatches 
can be ignored.  This is not much, but it does help, especially with spacing 
and line length errors.

If you are interested, I am very happy to make those parameter files that I 
have available for consideration, as long as everyone understands how far from 
fully compliant they are, and that their limitations are manifold.  I suspect 
the path to a fully automated reformat will be a long and winding road.  The 
best bet might be to actually change one of the Open Source tools to suit us, 
but I certainly don't know of anyone with the leisure time available to take 
that on.

Regards,
Evan

> 
...
> >> > --
> >> > Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
> >> >
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH] BaseTools/DevicePath: fix GCC build error in print_mem(), and clean it up

[Laszlo Ersek]

On 01/03/18 09:37, Zhu, Yonghong wrote:
> I pushed this fix first since it blocked some GCC tool chain.
> 
> SHA-1: 9a6b445bc2e6e2db6f67ab3cc425d5831aa1b7c8 
> 
> Best Regards,
> Zhu Yonghong

Thank you!
Laszlo

> 
> 
> -Original Message-
> From: Zhu, Yonghong 
> Sent: Wednesday, January 3, 2018 9:27 AM
> To: Laszlo Ersek ; edk2-devel-01 
> Cc: Gao, Liming ; Zhu, Yonghong 
> Subject: RE: [PATCH] BaseTools/DevicePath: fix GCC build error in 
> print_mem(), and clean it up
> 
> Thanks.
> 
> Reviewed-by: Yonghong Zhu  
> 
> Best Regards,
> Zhu Yonghong
> 
> 
> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Wednesday, January 03, 2018 1:17 AM
> To: edk2-devel-01 
> Cc: Gao, Liming ; Zhu, Yonghong 
> Subject: [PATCH] BaseTools/DevicePath: fix GCC build error in print_mem(), 
> and clean it up
> 
> Currently "BaseTools/Source/C/DevicePath/DevicePath.c" fails to build with
> GCC48:
> 
>> DevicePath.c: In function 'print_mem':
>> DevicePath.c:109:5: error: 'for' loop initial declarations are only 
>> allowed in C99 mode
>>  for (size_t i=0; i>  ^
>> DevicePath.c:109:5: note: use option -std=c99 or -std=gnu99 to compile 
>> your code
> 
> In addition, the print_mem() function does not conform to the edk2 coding
> style:
> 
> - we use CamelCase and no underscores in identifiers,
> - the types and type qualifiers should follow the edk2 style,
> - initialization as part of definition is forbidden for local variables.
> 
> Clean these up.
> 
> While updating the print_mem()/PrintMem() call sites, also remove the 
> superfluous parentheses around the second argument.
> 
> Cc: Liming Gao 
> Cc: Yonghong Zhu 
> Fixes: 7dbc50bd244d95fdc1741b9cfc561f0bfd724de1
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek 
> ---
> 
> Notes:
> Build-tested only (I don't have a DSC file with a device path PCD).
> 
>  BaseTools/Source/C/DevicePath/DevicePath.c | 20 ++--
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/BaseTools/Source/C/DevicePath/DevicePath.c 
> b/BaseTools/Source/C/DevicePath/DevicePath.c
> index 4c87163209ab..76b8553b7145 100644
> --- a/BaseTools/Source/C/DevicePath/DevicePath.c
> +++ b/BaseTools/Source/C/DevicePath/DevicePath.c
> @@ -103,11 +103,19 @@ Returns:
>  }
>  
>  
> -void print_mem(void const *vp, size_t n)
> +STATIC
> +VOID
> +PrintMem (
> +  CONST VOID *Buffer,
> +  UINTN  Count
> +  )
>  {
> -unsigned char const *p = vp;
> -for (size_t i=0; i -printf("0x%02x ", p[i]);
> +  CONST UINT8 *Bytes;
> +  UINTN   Idx;
> +
> +  Bytes = Buffer;
> +  for (Idx = 0; Idx < Count; Idx++) {
> +printf("0x%02x ", Bytes[Idx]);
>}
>  }
>  
> @@ -177,10 +185,10 @@ int main(int argc, CHAR8 *argv[])
>DevicePath = UefiDevicePathLibConvertTextToDevicePath(Str16);
>while (!((DevicePath->Type == END_DEVICE_PATH_TYPE) && 
> (DevicePath->SubType == END_ENTIRE_DEVICE_PATH_SUBTYPE)) )
>{
> -print_mem(DevicePath, (DevicePath->Length[0] | DevicePath->Length[1] << 
> 8));
> +PrintMem (DevicePath, DevicePath->Length[0] | DevicePath->Length[1] 
> + << 8);
>  DevicePath = (EFI_DEVICE_PATH_PROTOCOL *)((UINT8 *)DevicePath + 
> (DevicePath->Length[0] | DevicePath->Length[1] << 8));
>}
> -  print_mem(DevicePath, (DevicePath->Length[0] | DevicePath->Length[1] << 
> 8));
> +  PrintMem (DevicePath, DevicePath->Length[0] | DevicePath->Length[1] 
> + << 8);
>putchar('\n');
>return STATUS_SUCCESS;
>  }
> --
> 2.14.1.3.gb7cf6e02401b
> 

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

Re: [edk2] 答复: [RFC v4 1/6] UefiCpuPkg/CpuExceptionHandlerLib/X64: Add stack trace support

[Paulo Alcantara]

Hi Jeff,

On 1/3/2018 6:53 AM, Fan Jeff wrote:

Paulo,

I did not receive your #0 patch. So, I reply #1 here.

Your serial of patches should work for IA32/X64 GCC and IA32 MSFT. I 
suggest you could push this serial of patches as soon as pass UefiCpuPkg 
owner/reviewers’ review and validation,


OK - That would be really appreciable if you could test it with IA32 
MSFT toolchain and see if the stacktrace is printed out correctly when 
passing the '/Oy-' compile option.




And then I could send my x64 MSFT support later.


Cool.

Thanks
Paulo



*From:* edk2-devel  on behalf of Paulo 
Alcantara 

*Sent:* Friday, December 29, 2017 12:39:34 PM
*To:* edk2-devel@lists.01.org
*Cc:* Laszlo Ersek; Eric Dong
*Subject:* [edk2] [RFC v4 1/6] UefiCpuPkg/CpuExceptionHandlerLib/X64: 
Add stack trace support

This patch adds stack trace support during a X64 CPU exception.

It will dump out back trace, stack contents as well as image module
names that were part of the call stack.

Contributed-under: TianoCore Contribution Agreement 1.1
Cc: Eric Dong 
Cc: Laszlo Ersek 
Signed-off-by: Paulo Alcantara 
---
  UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c | 
376 +++-

  1 file changed, 374 insertions(+), 2 deletions(-)

diff --git 
a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c 
b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c

index 1dcf4277de..19bfaa329a 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c
@@ -14,6 +14,11 @@

  #include "CpuExceptionCommon.h"

+//
+// Unknown PDB file name
+//
+GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8 *mUnknownPdbFileName = "";
+
  /**
    Return address map of exception handler template so that C code can 
generate

    exception tables.
@@ -398,6 +403,357 @@ DumpCpuContext (
  );
  }

+/**
+  Get absolute path and file name of PDB file in PE/COFF image.
+
+  @param[in]  ImageBase    Base address of PE/COFF image.
+  @param[out] PdbAbsoluteFilePath  Absolute path of PDB file.
+  @param[out] PdbFileName  File name of PDB file.
+**/
+STATIC
+VOID
+GetPdbFileName (
+  IN  UINTN    ImageBase,
+  OUT CHAR8    **PdbAbsoluteFilePath,
+  OUT CHAR8    **PdbFileName
+  )
+{
+  VOID   *PdbPointer;
+  CHAR8  *Str;
+
+  //
+  // Get PDB file name from PE/COFF image
+  //
+  PdbPointer = PeCoffLoaderGetPdbPointer ((VOID *)ImageBase);
+  if (PdbPointer == NULL) {
+    //
+    // No PDB file name found. Set it to an unknown file name.
+    //
+    *PdbFileName = (CHAR8 *)mUnknownPdbFileName;
+    if (PdbAbsoluteFilePath != NULL) {
+  *PdbAbsoluteFilePath = NULL;
+    }
+  } else {
+    //
+    // Get file name portion out of PDB file in PE/COFF image
+    //
+    Str = (CHAR8 *)((UINTN)PdbPointer +
+    AsciiStrLen ((CHAR8 *)PdbPointer) - sizeof *Str);
+    for (; *Str != '/' && *Str != '\\'; Str--) {
+  ;
+    }
+
+    //
+    // Set PDB file name (also skip trailing path separator: '/' or '\\')
+    //
+    *PdbFileName = Str + 1;
+
+    if (PdbAbsoluteFilePath != NULL) {
+  //
+  // Set absolute file path of PDB file
+  //
+  *PdbAbsoluteFilePath = PdbPointer;
+    }
+  }
+}
+
+/**
+  Dump stack contents.
+
+  @param[in]  CurrentRsp Current stack pointer address.
+  @param[in]  UnwoundStacksCount  Count of unwound stack frames.
+**/
+STATIC
+VOID
+DumpStackContents (
+  IN UINT64  CurrentRsp,
+  IN INTN    UnwoundStacksCount
+  )
+{
+  //
+  // Check for proper stack pointer alignment
+  //
+  if (((UINTN)CurrentRsp & (CPU_STACK_ALIGNMENT - 1)) != 0) {
+    InternalPrintMessage (" Unaligned stack pointer. \n");
+    return;
+  }
+
+  //
+  // Dump out stack contents
+  //
+  InternalPrintMessage ("\nStack dump:\n");
+  while (UnwoundStacksCount-- > 0) {
+    InternalPrintMessage (
+  "0x%016lx: %016lx %016lx\n",
+  CurrentRsp,
+  *(UINT64 *)CurrentRsp,
+  *(UINT64 *)((UINTN)CurrentRsp + 8)
+  );
+
+    //
+    // Point to next stack
+    //
+    CurrentRsp += CPU_STACK_ALIGNMENT;
+  }
+}
+
+/**
+  Dump all image module names from call stack.
+
+  @param[in]  SystemContext  Pointer to EFI_SYSTEM_CONTEXT.
+**/
+STATIC
+VOID
+DumpImageModuleNames (
+  IN EFI_SYSTEM_CONTEXT   SystemContext
+  )
+{
+  EFI_STATUS  Status;
+  UINT64  Rip;
+  UINTN   ImageBase;
+  VOID    *EntryPoint;
+  CHAR8   *PdbAbsoluteFilePath;
+  CHAR8   *PdbFileName;
+  UINT64  Rbp;
+  UINTN   LastImageBase;
+
+  //
+  // Set current RIP address
+  //
+  Rip = SystemContext.SystemContextX64->Rip;
+
+  //
+  // Set current frame pointer address
+  //
+  Rbp = SystemContext.SystemContextX64->Rbp;
+
+  //
+  // Check for proper frame pointer alignment

Re: [edk2] 答复: [RFC v4 5/6] UefiCpuPkg/CpuExceptionHandlerLib: Ensure valid frame/stack pointers

[Paulo Alcantara]

On 1/3/2018 6:45 AM, Fan Jeff wrote:

Paulo,

+    if (!IsLogicalAddressValid (SystemContext,
+    SystemContext.SystemContextIa32->Ss,
+    (UINTN)Ebp) ||
+    !IsLogicalAddressValid (SystemContext,
+    SystemContext.SystemContextIa32->Ss,
+    (UINTN)Ebp + 4)) {

I don’t understand why you check both ebp and ebp+4, I think it’s enough 
to only check EBP (saved stack pointer address)


Isn't it possible that EBP + 4 might potentially point to another page 
frame? If not, then I will drop it out in v5.


Thanks
Paulo



Jeff

*发件人: *Paulo Alcantara 
*发送时间: *2017年12月29日12:41
*收件人: *edk2-devel@lists.01.org 
*抄送: *Laszlo Ersek ; Eric Dong 

*主题: *[edk2] [RFC v4 5/6] UefiCpuPkg/CpuExceptionHandlerLib: Ensure 
valid frame/stack pointers


Validate all possible memory dereferences during stack traces in IA32
and X64 CPU exceptions.

Contributed-under: TianoCore Contribution Agreement 1.1
Cc: Eric Dong 
Cc: Laszlo Ersek 
Requested-by: Brian Johnson 
Requested-by: Jiewen Yao 
Signed-off-by: Paulo Alcantara 
---
  UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c 
| 143 +++-
  UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c  
|  75 +-

  2 files changed, 210 insertions(+), 8 deletions(-)

diff --git 
a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c 
b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c

index 25e02fbbc1..9b52d4f6d2 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c
@@ -398,6 +398,96 @@ DumpCpuContext (
  );
  }

+/**
+  Check if a logical address is valid.
+
+  @param[in]  SystemContext  Pointer to EFI_SYSTEM_CONTEXT.
+  @param[in]  SegmentSelector    Segment selector.
+  @param[in]  Offset Offset or logical address.
+**/
+STATIC
+BOOLEAN
+IsLogicalAddressValid (
+  IN  EFI_SYSTEM_CONTEXT   SystemContext,
+  IN  UINT16   SegmentSelector,
+  IN  UINTN    Offset
+  )
+{
+  IA32_SEGMENT_DESCRIPTOR  *SegmentDescriptor;
+  UINT32   SegDescBase;
+  UINT32   SegDescLimit;
+  UINTN    SegDescLimitInBytes;
+
+  //
+  // Check for valid input parameters
+  //
+  if (SegmentSelector == 0 || Offset == 0) {
+    return FALSE;
+  }
+
+  //
+  // Check whether to look for a segment descriptor in GDT or LDT table
+  //
+  if ((SegmentSelector & BIT2) == 0) {
+    //
+    // Get segment descriptor from GDT table
+    //
+    SegmentDescriptor =
+  (IA32_SEGMENT_DESCRIPTOR *)(
+    (UINTN)SystemContext.SystemContextIa32->Gdtr[0] +
+    ((SegmentSelector >> 3) * 8)
+    );
+  } else {
+    //
+    // Get segment descriptor from LDT table
+    //
+    SegmentDescriptor =
+  (IA32_SEGMENT_DESCRIPTOR *)(
+    (UINTN)SystemContext.SystemContextIa32->Ldtr +
+    ((SegmentSelector >> 3) * 8)
+    );
+  }
+
+  //
+  // Get segment descriptor's base address
+  //
+  SegDescBase = SegmentDescriptor->Bits.BaseLow |
+    (SegmentDescriptor->Bits.BaseMid << 16) |
+    (SegmentDescriptor->Bits.BaseHigh << 24);
+
+  //
+  // Get segment descriptor's limit
+  //
+  SegDescLimit = SegmentDescriptor->Bits.LimitLow |
+    (SegmentDescriptor->Bits.LimitHigh << 16);
+
+  //
+  // Calculate segment descriptor's limit in bytes
+  //
+  if (SegmentDescriptor->Bits.G == 1) {
+    SegDescLimitInBytes = (UINTN)SegDescLimit * SIZE_4KB;
+  } else {
+    SegDescLimitInBytes = SegDescLimit;
+  }
+
+  //
+  // Make sure to not access beyond a segment limit boundary
+  //
+  if (Offset + SegDescBase > SegDescLimitInBytes) {
+    return FALSE;
+  }
+
+  //
+  // Check if the translated logical address (or linear address) is valid
+  //
+  return IsLinearAddressValid (
+    SystemContext.SystemContextIa32->Cr0,
+    SystemContext.SystemContextIa32->Cr3,
+    SystemContext.SystemContextIa32->Cr4,
+    Offset + SegDescBase
+    );
+}
+
  /**
    Dump stack trace.

@@ -459,6 +549,20 @@ DumpStackTrace (
    InternalPrintMessage ("\nCall trace:\n");

    for (;;) {
+    //
+    // Check for valid frame pointer
+    //
+    if (!IsLogicalAddressValid (SystemContext,
+    SystemContext.SystemContextIa32->Ss,
+    (UINTN)Ebp + 4) ||
+    !IsLogicalAddressValid (SystemContext,
+    SystemContext.SystemContextIa32->Ss,
+    (UINTN)Ebp)) {
+  InternalPrintMessage ("%a: attempted to dereference an invalid 
frame "

+    "pointer at 0x%08x\n", __FUNCTION__, Ebp);
+

Re: [edk2] 答复: [RFC v4 4/6] UefiCpuPkg/CpuExceptionHandlerLib: Add helper to valid memory addresses

[Paulo Alcantara]

Hi Jeff,

On 1/3/2018 6:42 AM, Fan Jeff wrote:

Paul,

+  //
+  // Calculate physical address of PML4E
+  //
+  PhysicalAddress = (UINT64)Cr3 & (((1ULL << MaxPhyAddrBits) - 1) << 12);
+  PhysicalAddress |= (((UINT64)LinearAddress >> 39) & 0x1FF) << 3;
+

Should not pass VS build, instead you could use LShiftU64/RShiftU64 to 
do 64bit shift operation as below:


   PhysicalAddress = (UINT64)Cr3 & LShiftU64 (LShiftU64 (1, 
MaxPhyAddrBits) - 1, 12);


   PhysicalAddress |= LShiftU64 (RShiftU64((UINT64)LinearAddress, 39) & 
0x1FF), 3);


OK - I'll fix them up and then build-test it with IA32/X64 MSFT toolchain.

Thanks
Paulo



Jeff

*发件人: *Paulo Alcantara 
*发送时间: *2017年12月29日12:41
*收件人: *edk2-devel@lists.01.org 
*抄送: *Laszlo Ersek ; Eric Dong 

*主题: *[edk2] [RFC v4 4/6] UefiCpuPkg/CpuExceptionHandlerLib: Add 
helper to valid memory addresses


Introduce IsLinearAddressValid() function that will be used for
validating memory addresses that would get dereferenced during stack
traces in IA32 and X64 CPU exceptions.

Contributed-under: TianoCore Contribution Agreement 1.1
Cc: Eric Dong 
Cc: Laszlo Ersek 
Requested-by: Brian Johnson 
Requested-by: Jiewen Yao 
Signed-off-by: Paulo Alcantara 
---
  UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c | 382 


  UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.h |  16 +
  2 files changed, 398 insertions(+)

diff --git 
a/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c 
b/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c

index 867c5c01d6..52b3eb1463 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c
@@ -14,6 +14,9 @@

  #include "CpuExceptionCommon.h"

+#include 
+#include 
+
  //
  // Error code flag indicating whether or not an error code will be
  // pushed on the stack if an exception occurs.
@@ -194,3 +197,382 @@ GetPdbFileName (
  }
    }
  }
+
+/**
+  Check if a linear address is valid by walking the page tables in 4-level
+  paging mode.
+
+  @param[in]  Cr3 CR3 control register.
+  @param[in]  MaxPhyAddrBits  MAXPHYADDRBITS bits.
+  @param[in]  LinearAddress   Linear address to be checked.
+**/
+STATIC
+BOOLEAN
+Do4LevelPagingModeCheck (
+  IN UINTN    Cr3,
+  IN UINT8    MaxPhyAddrBits,
+  IN UINTN    LinearAddress
+  )
+{
+  UINT64 PhysicalAddress;
+  UINT64 *Pml4TableEntry;
+  UINT64 *PageDirPtrTableEntry;
+  UINT64 *PageDirEntry;
+  UINT64 *PageTableEntry;
+
+  //
+  // In 4-level paging mode, linear addresses are 48 bits wide
+  //
+  if ((UINT64)LinearAddress > (1ULL << 48) - 1) {
+    return FALSE;
+  }
+
+  //
+  // Calculate physical address of PML4E
+  //
+  PhysicalAddress = (UINT64)Cr3 & (((1ULL << MaxPhyAddrBits) - 1) << 12);
+  PhysicalAddress |= (((UINT64)LinearAddress >> 39) & 0x1FF) << 3;
+
+  ASSERT ((PhysicalAddress & (sizeof (*Pml4TableEntry) - 1)) == 0);
+
+  Pml4TableEntry = (UINT64 *)(UINTN)PhysicalAddress;
+
+  //
+  // Check if a PDPTE is present
+  //
+  if ((*Pml4TableEntry & BIT0) == 0) {
+    return FALSE;
+  }
+
+  //
+  // Calculate physical address of PDPTE
+  //
+  PhysicalAddress = *Pml4TableEntry & (((1ULL << MaxPhyAddrBits) - 1) 
<< 12);

+  PhysicalAddress |= (((UINT64)LinearAddress >> 30) & 0x1FF) << 3;
+
+  ASSERT ((PhysicalAddress & (sizeof (*PageDirPtrTableEntry) - 1)) == 0);
+
+  PageDirPtrTableEntry = (UINT64 *)(UINTN)PhysicalAddress;
+
+  //
+  // Check whether a PDPTE or 1GiB page entry is present
+  //
+  if ((*PageDirPtrTableEntry & BIT0) == 0) {
+    return FALSE;
+  }
+
+  //
+  // Check if PDPTE maps an 1GiB page
+  //
+  if ((*PageDirPtrTableEntry & BIT7) != 0) {
+    return TRUE;
+  }
+
+  //
+  // Calculate physical address of PDE
+  //
+  PhysicalAddress = *PageDirPtrTableEntry & (((1ULL << MaxPhyAddrBits) 
- 1) <<

+ 12);
+  PhysicalAddress |= (((UINT64)LinearAddress >> 21) & 0x1FF) << 3;
+
+  ASSERT ((PhysicalAddress & (sizeof (*PageDirEntry) - 1)) == 0);
+
+  PageDirEntry = (UINT64 *)(UINTN)PhysicalAddress;
+
+  //
+  // Check whether a PDE or a 2MiB page entry is present
+  //
+  if ((*PageDirEntry & BIT0) == 0) {
+    return FALSE;
+  }
+
+  //
+  // Check if PDE maps a 2MiB page
+  //
+  if ((*PageDirEntry & BIT7) != 0) {
+    return TRUE;
+  }
+
+  //
+  // Calculate physical address of PTE
+  //
+  PhysicalAddress = *PageDirEntry & (((1ULL << MaxPhyAddrBits) - 1) << 12);
+  PhysicalAddress |= (((UINT64)LinearAddress >> 12) & 0x1FF) << 3;
+
+  ASSERT ((PhysicalAddress & (sizeof (*PageTableEntry) - 1)) == 0);
+
+  PageTableEntry = (UINT64 *)(UINTN)PhysicalAddress;
+
+  //
+  // Check if PTE maps a 4KiB page
+  //
+  if ((*PageTableEntry & BIT0) == 

[edk2] [edk2-CCodingStandardsSpecification PATCH 4/5] Fix Chapter 4 Typo

[evan . lloyd]

From: Evan Lloyd 

4.1.3.2 - remove "See" from 'as specified in See "File Heading"'

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Evan Lloyd 
---
 4_naming_conventions/README.md | 418 ++--
 1 file changed, 209 insertions(+), 209 deletions(-)

diff --git a/4_naming_conventions/README.md b/4_naming_conventions/README.md
index 
bcf902875719f94c5c4c3e58af24e18772dae055..1a9cd008b5dbbf203148408f92ffb4881f499766
 100644
--- a/4_naming_conventions/README.md
+++ b/4_naming_conventions/README.md
@@ -1,209 +1,209 @@
-
-
-# 4 Naming Conventions
-
-## 4.1 General Naming Rules
-
-**Use good naming practice when declaring variable names.**
-
-Studies show that programs with names averaging 10 to 16 characters are the
-easiest to debug. The name length is just a guideline_; the most important
-thing is that the name conveys a clear meaning of what it represents_.
-
-**Do not overload commonly used terms.**
-
-For example, EFI has an event model, so don't call some abstraction that you
-define an Event. People will get confused. Make sure someone reading the code
-can tell what you are talking about.
-
-**Each word or concept must start with a capital letter and be followed by
-lower-case letters.**
-
-The intent is for names to be consistent and easily readable. Each word in a
-compound name should be visually distinct.
-
-### 4.1.1 Identifiers that are always reserved
-
-**Identifiers beginning with an underscore are always reserved**
-
-Define them only in the special ways allowed elsewhere in this document.
-
-**Identifiers that are defined in the Standard C and POSIX libraries are always
-reserved.**
-
-This includes macros, typedefs, variables, and functions, whether with external
-linkage or file scope. The only exception is with modules that are mutually
-exclusive with these libraries. These reserved identifiers are listed in
-"Reserved Identifiers" and reserved keywords are listed in "Reserved Keywords".
-
-### 4.1.2 Common Opposites in Variable Names
-
-Use the correct opposites when declaring names.
-
-## Table 1 Common Opposites
-
-|   | |  |
-| - | --- |  |
-| add / remove  | begin / end | create / destroy |
-| increment / decrement | first / last| get / release|
-| lock / unlock | put / get   | up / down|
-| old / new | min / max   | next / previous  |
-| source / destination  | open / close| show / hide  |
-|   | source / target | start / stop |
-
-### 4.1.3 Abbreviation Usage
-
- 4.1.3.1 The use of abbreviations shall be regulated.
-
-This document describes a common set of abbreviations that can be freely used.
-If you must make up abbreviations, remember the name is most important to the
-reader of the code, not the writer.
-
- 4.1.3.2 New abbreviations must be documented in the header of each using 
file.
-
-Any abbreviation used, which is not documented in this specification, or in the
-_UEFI Specification_ shall be placed into a Glossary section of the File header
-as specified in See "File Heading".
-
-Do not define a new abbreviation to replace an abbreviation that is already
-defined in this document. For example, do not define No to mean Number, because
-Num is the supported abbreviation.
-
-"EFI Supported Abbreviations" below lists the abbreviations that are
-standardized by this document and do not require a defining comment.
-
-## Table 2 EFI Supported Abbreviations
-
-| Abbreviation | Description |
-|  | --- |
-| Ptr  | Pointer |
-| Str  | Unicode string  |
-| Ascii| ASCII string|
-| Len  | Length  |
-| Arg  | Argument|
-| Max  | Maximum |
-| Min  | Minimum |
-| Char | Character   |
-| Num  | Number  |
-| Temp | Temporary   |
-| Src  | Source  |
-| Dst  | Destination |
-| BS   | EFI Boot Services Table |
-| RT   | EFI Runtime Table   |
-| ST   | EFI System Table|
-| Tpl  | EFI Task Priority Level |
-
- 4.1.3.3 Powers of 2 and 10
-
-You are encouraged to use the IEC international abbreviations for powers of 2
-(KiB for 2^10, MiB for 2^20, GiB for 2^30, etc.) rather than the old KB, MB,
-and GB, which IEC now reserves for powers of 10 (10^3, 10^6, 10^9). Given that
-many readers of the code may not have made the conversion to add the 'i', do
-not use KB, MB, and GB for powers of 10 Instead, use e.g. "2*10^6 bytes"
-instead of 2MB to avoid confusion. Note that GiB is derived from the G in
-'Giga', the 'i' in binary, and the B in 'Byte'.
-
-### 4.1.4 Acronym Usage
-

[edk2] [edk2-CCodingStandardsSpecification PATCH 5/5] Fix Chapter 5 Typos

[evan . lloyd]

From: Evan Lloyd 

5.1.1 - Replace "less" with "fewer" (because columns is plural and
countable)
5.1.5 - Correct tense.  (because the C specification still defines...)
Insert full stop.
Insert comma.
5.1.8 - Correct "provided" to "proven".
5.1.9 - remove hanging "This."
5.2.3.1 - replace "is comprised of" with "comprises" (comprise means
  "consists of", so "comprised of" is a solecism.
  Remove use of tab, as they are forbidden.
  Remove -- before date in copyright header (None of the edk2
  files have it).
5.4 - Add indent to comment text.
5.6.1.2 - Fix copy/paste text (from UEFI spec).

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Evan Lloyd 
---
 5_source_files/52_spacing.md|  9 +
 5_source_files/54_code_file_structure.md|  4 ++--
 5_source_files/56_declarations_and_types.md |  4 ++--
 5_source_files/README.md| 18 +-
 4 files changed, 18 insertions(+), 17 deletions(-)

diff --git a/5_source_files/52_spacing.md b/5_source_files/52_spacing.md
index 
ddeabf7753a8713bf04e143d6e2e9bccf881f691..3c79f4e4ee91bcd4035d6cf7d8d32f1bb9c7756f
 100644
--- a/5_source_files/52_spacing.md
+++ b/5_source_files/52_spacing.md
@@ -249,7 +249,7 @@ And the comment will end with:
 **/
 ```
 
-The File Heading comment block is comprised of the following sections: File
+The File Heading comment block comprises the following sections: File
 Description, Copyright, License, and the optional Specification Reference and
 Glossary sections.
 
@@ -266,8 +266,9 @@ Glossary sections.
 **/
 ```
 
-The following example begins each body line with a tab (two spaces). This is
-the preferred indentation, but two tabs (four spaces) is also acceptable.
+The following example begins each body line with an indent (two spaces).
+This is the preferred indentation, but a double indent (four spaces) is also
+acceptable.
 
  Example
 
@@ -278,7 +279,7 @@ the preferred indentation, but two tabs (four spaces) is 
also acceptable.
   Detailed description of the file’s contents and other useful
   information for a person viewing the file for the first time.
 
-  Copyright (C) --20XX, Acme Corporation. All rights reserved.
+  Copyright (C) 20XX, Acme Corporation. All rights reserved.
   This program and the accompanying materials
   are licensed and made available under the terms and conditions of
   the BSD License which accompanies this distribution. The full
diff --git a/5_source_files/54_code_file_structure.md 
b/5_source_files/54_code_file_structure.md
index 
8cc9f4f61412b07f765d80d7b680c6dd38b838c1..ac999aae99ae9cfd8b6f97dc483e51bfbd7c7a0b
 100644
--- a/5_source_files/54_code_file_structure.md
+++ b/5_source_files/54_code_file_structure.md
@@ -68,8 +68,8 @@ these are C files with an extension of "`.c`".
 
 /* Function Definitions */
 
-/* If this is a protocol definition, the
-protocol structure is defined and initialized here.
+/* If this is a protocol definition, the protocol structure is defined and
+  initialized here.
 */
 ```
 
diff --git a/5_source_files/56_declarations_and_types.md 
b/5_source_files/56_declarations_and_types.md
index 
ec1803d980e1fa808b9dc515cdffbc4b47437435..5c57834fe1195b5487f0f59e8a0385e44d91aff4
 100644
--- a/5_source_files/56_declarations_and_types.md
+++ b/5_source_files/56_declarations_and_types.md
@@ -43,9 +43,9 @@ or from common EFI data types.
 The corresponding EFI types must be used instead.
 
 "EFI Data Types" below contains the common data types that are referenced in
-the interface definitions defined by this specification. Per the _UEFI
-Specification_, version 2.3.1:
+the interface definitions defined by the UEFI specification.
 
+Per the _UEFI Specification_, version 2.3.1:
 "Unless otherwise specified, all data types are naturally aligned. Structures
 are aligned on boundaries equal to the largest internal datum of the structure,
 and internal data is implicitly padded to achieve natural alignment."
diff --git a/5_source_files/README.md b/5_source_files/README.md
index 
a93492db4f0f17e14d9c2c3c95e57cf0f6cc911e..a443148138f2abaf6b9131f4758858a4a5f45fd3
 100644
--- a/5_source_files/README.md
+++ b/5_source_files/README.md
@@ -33,9 +33,9 @@
 
 ## 5.1 General Rules
 
-### 5.1.1 Lines shall be 120 columns, or less
+### 5.1.1 Lines shall be 120 columns, or fewer
 
-Preferably, limit line lengths to 80 columns or less. When this doesn't leave
+Preferably, limit line lengths to 80 columns or fewer. When this doesn't leave
 sufficient space for a good postfix style comment, extend the line to a total
 of 120 columns. Having some level of uniformity in the expected width of the
 source is useful for viewing and printing the code.
@@ -79,9 +79,9 @@ Other than '\0', the only permissible escape sequences are:
 
 ### 5.1.5 Octal constants (Base 8) shall not be used.
 
-The C language specification has defined numbers whose first 

[edk2] [edk2-CCodingStandardsSpecification PATCH 1/5] Fix Chapter 1 Typos

[evan . lloyd]

From: Evan Lloyd 

1.1 Abstract - replace "then" with "can" (to make meaningful)
1.2 Rationale - change
  "commit to conforming to standards of this specification"
  to   commit to conforming with the standards of this specification
1.3 Scope - add missing "to" in "an attempt make you aware of your
  actions"

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Evan Lloyd 
---
 1_introduction.md | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/1_introduction.md b/1_introduction.md
index 
c22c04c9f76d2352e4f294c0e70f5ebf787f7054..5471a6bf5e94df6dd0c5fd0a4b5020c77d1f8ca0
 100644
--- a/1_introduction.md
+++ b/1_introduction.md
@@ -62,7 +62,7 @@ This specification addresses the chronic problem of providing 
accurate
 documentation of the code base by embedding the documentation within the code.
 While this does not guarantee that the documentation will be kept up to date,
 it significantly increases the chances. A document generation system, Doxygen,
-then produce formatted documentation by extracting information from specially
+can produce formatted documentation by extracting information from specially
 formatted comment blocks and the syntactic elements of the code.
 
 This specification presents protocol standards that will ensure that the
@@ -97,10 +97,10 @@ wide support. On the downside in that each developer's C 
code could
 lack of uniformity makes understanding and maintaining the code very difficult.
 
 Uniformity is the key theme of these rules. You may disagree with some of our
-decisions. Nevertheless, we ask that you commit to conforming to standards of
-this specification. Also, there are pitfalls inherent in the C language that
-this style guide may help you to avoid. The goal of this document is making
-you, and those who follow you, more productive.
+decisions. Nevertheless, we ask that you commit to conforming with the
+standards of this specification. Also, there are pitfalls inherent in the C
+language that this style guide may help you to avoid. The goal of this document
+is making you, and those who follow you, more productive.
 
 Some of the strict rules for protocol and driver construction may seem overly
 onerous. Don't panic - there is a method to our madness - we intend to
@@ -161,7 +161,7 @@ Topics covered in this coding standard include:
 * Commenting rules
 * Doxygen
 
-These guidelines represent an attempt make you aware of your actions, because
+These guidelines represent an attempt to make you aware of your actions, 
because
 those actions affect the future readers and maintainers of the code you 
produce.
 
 Pre-existing code ported to the EDK II environment does not have to conform to
-- 
Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")

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

[edk2] [edk2-CCodingStandardsSpecification PATCH 0/5] Typographic Corrections

[evan . lloyd]

From: Evan Lloyd 

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

This patch set contains a number of minor typographical corrections.
They have no specific theme, being only that set of things I happened to
notice during recent code review sessions where I was frequently
consulting the CCS.  Some are pedantic or may come under the heading of
"Typographic silliness", but they were enough to distract my attention
from the content, so fixing them may help others.

GitHub branch for review:
https://github.com/EvanLloyd/edk2-CCodingStandardsSpecification/tree/17Q4Typos_v1

GitHub word diff view of the patches in this series:
https://github.com/tianocore-docs/edk2-CCodingStandardsSpecification/compare/master...EvanLloyd:17Q4Typos_v1?w1


Evan Lloyd (5):
  Fix Chapter 1 Typos
  Fix Chapter 2 Typos
  Fix Chapter 3 Typos
  Fix Chapter 4 Typo
  Fix Chapter 5 Typos

 1_introduction.md   |  12 +-
 2_guiding_principles.md |   8 +-
 3_quick_reference.md|   2 +-
 4_naming_conventions/README.md  | 418 ++--
 5_source_files/52_spacing.md|   9 +-
 5_source_files/54_code_file_structure.md|   4 +-
 5_source_files/56_declarations_and_types.md |   4 +-
 5_source_files/README.md|  18 +-
 8 files changed, 238 insertions(+), 237 deletions(-)

-- 
Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")

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

[edk2] [edk2-CCodingStandardsSpecification PATCH 3/5] Fix Chapter 3 Typos

[evan . lloyd]

From: Evan Lloyd 

3_quick_reference.md has some obviously invalid extra characters at the
end of line 55 ( ",).  This patch removes them.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Evan Lloyd 
---
 3_quick_reference.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/3_quick_reference.md b/3_quick_reference.md
index 
13045d297b3be848d91f9be05380454b4cc2fe91..9367e422fb0337007409d6597913cb43da92e0c5
 100644
--- a/3_quick_reference.md
+++ b/3_quick_reference.md
@@ -55,7 +55,7 @@
   any file using the abbreviation or acronym. See "Abbreviation Usage" and
   "Glossary".
 
-* There is no limit to name lengths. A length of 10 to 30 characters is ",
+* There is no limit to name lengths. A length of 10 to 30 characters is
   recommended. See "File Names" & "Identifiers that are always reserved".
 
 * File names must not start with numbers. See "File Names".
-- 
Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")

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

[edk2] [edk2-CCodingStandardsSpecification PATCH 2/5] Fix Chapter 2 Typos

[evan . lloyd]

From: Evan Lloyd 

2.1 Accessibility - remove erroneous "as"
2.1 Confirmation - insert missing full stop
2.1 Forgiveness - excise superfluous "errors"
2.1 Standard techniques - remove redundant "be to"

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Evan Lloyd 
---
 2_guiding_principles.md | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/2_guiding_principles.md b/2_guiding_principles.md
index 
a7759f27dcf71a948b903332c9bc14946e445cd8..5a51225b65dec2159a4fb94481920666c0d042ff
 100644
--- a/2_guiding_principles.md
+++ b/2_guiding_principles.md
@@ -47,7 +47,7 @@ The following is an alphabetical list of software design 
principles:
 **Accessibility**
 
 This entails designing objects and environments to be usable, with no
-modification, by the greatest number of people as possible, including people
+modification, by the greatest number of people possible, including people
 with varying educational and social backgrounds, as well as those with motor or
 sensory challenges.
 
@@ -68,7 +68,7 @@ shortterm memory, as well as to accommodate its limits.
 This is a technique used for critical actions, inputs, or commands.
 Confirmations are primarily used to prevent unintended actions. Minimize errors
 in critical or irreversible operations with confirmations. If you overuse
-confirmations, expect that they will be ignored Avoid overusing confirmations
+confirmations, expect that they will be ignored. Avoid overusing confirmations
 to ensure that they remain unexpected and uncommon; otherwise, they may be
 ignored. Use a two-step operation for hardware confirmations and dialogs for
 software confirmations.
@@ -97,7 +97,7 @@ about the assumptions you make.
 **Forgiveness**
 
 Design to help users avoid errors and reduce the negative consequences of
-errors any errors made. Recommended methods for achieving design forgiveness
+any errors made. Recommended methods for achieving design forgiveness
 include affordances, reversibility of actions, and safety nets. Effectively
 designing for forgiveness results in a design needing minimal confirmations,
 warnings, and help.
@@ -171,7 +171,7 @@ classes of platforms from embedded systems to massively 
parallel computers.
 
 Greater reliance on unique or exotic pieces makes a system harder to
 understand, and more intimidating for someone trying to understand it the first
-time. Using standardized, common approaches should be to give the whole system
+time. Using standardized, common approaches should give the whole system
 a familiar feeling. This standardization is one of the primary goals of this
 document.
 
-- 
Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")

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

Re: [edk2] [PATCH edk2-platforms v2 14/18] ARM/VExpressPkg: Reserving framebuffer at build

[Evan Lloyd]

> -Original Message-
> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> Sent: 23 December 2017 16:03
> To: Evan Lloyd 
> Cc: edk2-devel@lists.01.org; Arvind Chauhan ;
> Daniil Egranov ; Thomas Abraham
> ; "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 edk2-platforms v2 14/18] ARM/VExpressPkg:
> Reserving framebuffer at build
>
> On 22 December 2017 at 19:08,   wrote:
> > From: Girish Pathak 
> >
> > This change uses two PCDs, PcdArmLcdFrameBufferBase and
> > PcdArmLcdFrameBufferSize introduced in correspondiong EDK2 patch to
> > reserve framebuffer in DRAM if these values are defined in platform
> > specific DSC file, avoiding the need to allocate dynamically.
> > This allows the framebuffer to appear as "I/O memory" outside of the
> > normal RAM map, which is similar to the "VRAM" case.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
...
> > +  VirtualMemoryTable[Index].Attributes =
> > +ARM_MEMORY_REGION_ATTRIBUTE_UNCACHED_UNBUFFERED;
> > +#endif
> > +
>
> Whose responsibility is it to ensure that this region is removed from
> SystemMemoryBase/SystemMemorySize? If it is up to the .DSC author,
> could you please add an ASSERT() here that they don't overlap?

 [[Evan Lloyd]] That is an excellent suggestion, thank you - we'll add that.

>
> >// Map sparse memory region if present
> >if (HasSparseMemory) {
> >  VirtualMemoryTable[++Index].PhysicalBase = SparseMemoryBase;
> > --
> > Guid("CE165669-3EF3-493F-B85D-6190EE5B9759")
> >
IMPORTANT NOTICE: The contents of this email and any attachments are 
confidential and may also be privileged. If you are not the intended recipient, 
please notify the sender immediately and do not disclose the contents to any 
other person, use it for any purpose, or store or copy the information in any 
medium. Thank you.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] 答复: [RFC v4 1/6] UefiCpuPkg/CpuExceptionHandlerLib/X64: Add stack trace support

[Fan Jeff]

Paulo,



I did not receive your #0 patch. So, I reply #1 here.



Your serial of patches should work for IA32/X64 GCC and IA32 MSFT. I suggest 
you could push this serial of patches as soon as pass UefiCpuPkg 
owner/reviewers’ review and validation,



And then I could send my x64 MSFT support later.



Thanks!

Jeff


From: edk2-devel  on behalf of Paulo Alcantara 

Sent: Friday, December 29, 2017 12:39:34 PM
To: edk2-devel@lists.01.org
Cc: Laszlo Ersek; Eric Dong
Subject: [edk2] [RFC v4 1/6] UefiCpuPkg/CpuExceptionHandlerLib/X64: Add stack 
trace support

This patch adds stack trace support during a X64 CPU exception.

It will dump out back trace, stack contents as well as image module
names that were part of the call stack.

Contributed-under: TianoCore Contribution Agreement 1.1
Cc: Eric Dong 
Cc: Laszlo Ersek 
Signed-off-by: Paulo Alcantara 
---
 UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c | 376 
+++-
 1 file changed, 374 insertions(+), 2 deletions(-)

diff --git 
a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c 
b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c
index 1dcf4277de..19bfaa329a 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c
@@ -14,6 +14,11 @@

 #include "CpuExceptionCommon.h"

+//
+// Unknown PDB file name
+//
+GLOBAL_REMOVE_IF_UNREFERENCED CONST CHAR8 *mUnknownPdbFileName = "";
+
 /**
   Return address map of exception handler template so that C code can generate
   exception tables.
@@ -398,6 +403,357 @@ DumpCpuContext (
 );
 }

+/**
+  Get absolute path and file name of PDB file in PE/COFF image.
+
+  @param[in]  ImageBaseBase address of PE/COFF image.
+  @param[out] PdbAbsoluteFilePath  Absolute path of PDB file.
+  @param[out] PdbFileName  File name of PDB file.
+**/
+STATIC
+VOID
+GetPdbFileName (
+  IN  UINTNImageBase,
+  OUT CHAR8**PdbAbsoluteFilePath,
+  OUT CHAR8**PdbFileName
+  )
+{
+  VOID   *PdbPointer;
+  CHAR8  *Str;
+
+  //
+  // Get PDB file name from PE/COFF image
+  //
+  PdbPointer = PeCoffLoaderGetPdbPointer ((VOID *)ImageBase);
+  if (PdbPointer == NULL) {
+//
+// No PDB file name found. Set it to an unknown file name.
+//
+*PdbFileName = (CHAR8 *)mUnknownPdbFileName;
+if (PdbAbsoluteFilePath != NULL) {
+  *PdbAbsoluteFilePath = NULL;
+}
+  } else {
+//
+// Get file name portion out of PDB file in PE/COFF image
+//
+Str = (CHAR8 *)((UINTN)PdbPointer +
+AsciiStrLen ((CHAR8 *)PdbPointer) - sizeof *Str);
+for (; *Str != '/' && *Str != '\\'; Str--) {
+  ;
+}
+
+//
+// Set PDB file name (also skip trailing path separator: '/' or '\\')
+//
+*PdbFileName = Str + 1;
+
+if (PdbAbsoluteFilePath != NULL) {
+  //
+  // Set absolute file path of PDB file
+  //
+  *PdbAbsoluteFilePath = PdbPointer;
+}
+  }
+}
+
+/**
+  Dump stack contents.
+
+  @param[in]  CurrentRsp Current stack pointer address.
+  @param[in]  UnwoundStacksCount  Count of unwound stack frames.
+**/
+STATIC
+VOID
+DumpStackContents (
+  IN UINT64  CurrentRsp,
+  IN INTNUnwoundStacksCount
+  )
+{
+  //
+  // Check for proper stack pointer alignment
+  //
+  if (((UINTN)CurrentRsp & (CPU_STACK_ALIGNMENT - 1)) != 0) {
+InternalPrintMessage (" Unaligned stack pointer. \n");
+return;
+  }
+
+  //
+  // Dump out stack contents
+  //
+  InternalPrintMessage ("\nStack dump:\n");
+  while (UnwoundStacksCount-- > 0) {
+InternalPrintMessage (
+  "0x%016lx: %016lx %016lx\n",
+  CurrentRsp,
+  *(UINT64 *)CurrentRsp,
+  *(UINT64 *)((UINTN)CurrentRsp + 8)
+  );
+
+//
+// Point to next stack
+//
+CurrentRsp += CPU_STACK_ALIGNMENT;
+  }
+}
+
+/**
+  Dump all image module names from call stack.
+
+  @param[in]  SystemContext  Pointer to EFI_SYSTEM_CONTEXT.
+**/
+STATIC
+VOID
+DumpImageModuleNames (
+  IN EFI_SYSTEM_CONTEXT   SystemContext
+  )
+{
+  EFI_STATUS  Status;
+  UINT64  Rip;
+  UINTN   ImageBase;
+  VOID*EntryPoint;
+  CHAR8   *PdbAbsoluteFilePath;
+  CHAR8   *PdbFileName;
+  UINT64  Rbp;
+  UINTN   LastImageBase;
+
+  //
+  // Set current RIP address
+  //
+  Rip = SystemContext.SystemContextX64->Rip;
+
+  //
+  // Set current frame pointer address
+  //
+  Rbp = SystemContext.SystemContextX64->Rbp;
+
+  //
+  // Check for proper frame pointer alignment
+  //
+  if (((UINTN)Rbp & (CPU_STACK_ALIGNMENT - 1)) != 0) {
+InternalPrintMessage (" Unaligned frame pointer. \n");
+return;
+  }
+
+  //
+  // Get initial PE/COFF image base address from current RIP
+  //
+  ImageBase = PeCoffSearchImageBase (Rip);
+  if (ImageBase == 0) {
+ 

[edk2] 答复: [RFC v4 6/6] UefiCpuPkg/CpuExceptionHandlerLib: Correctly print IP addresses

[Fan Jeff]

Paulo,



Thanks.



Reviewed-by:  Jeff Fan 



Jeff




From: edk2-devel  on behalf of Paulo Alcantara 

Sent: Friday, December 29, 2017 12:39:39 PM
To: edk2-devel@lists.01.org
Cc: Laszlo Ersek; Eric Dong
Subject: [edk2] [RFC v4 6/6] UefiCpuPkg/CpuExceptionHandlerLib: Correctly print 
IP addresses

Remove the supurious '- 1' when calculating the IP addresses during the
stack traces.

Contributed-under: TianoCore Contribution Agreement 1.1
Cc: Eric Dong 
Cc: Laszlo Ersek 
Requested-by: Jeff Fan 
Signed-off-by: Paulo Alcantara 
---
 UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c | 2 +-
 UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git 
a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c 
b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c
index 9b52d4f6d2..e2a3341286 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c
@@ -573,7 +573,7 @@ DumpStackTrace (
   *UnwoundStacksCount - 1,
   Eip,
   ImageBase,
-  Eip - ImageBase - 1,
+  Eip - ImageBase,
   Ebp,
   PdbFileName
   );
diff --git 
a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c 
b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c
index 8067c34122..5ae3aa4e73 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c
@@ -717,7 +717,7 @@ DumpStackTrace (
   *UnwoundStacksCount - 1,
   Rip,
   ImageBase,
-  Rip - ImageBase - 1,
+  Rip - ImageBase,
   Rbp,
   PdbFileName
   );
--
2.14.3

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

[edk2] 答复: [RFC v4 5/6] UefiCpuPkg/CpuExceptionHandlerLib: Ensure valid frame/stack pointers

[Fan Jeff]

Paulo,

+if (!IsLogicalAddressValid (SystemContext,
+SystemContext.SystemContextIa32->Ss,
+(UINTN)Ebp) ||
+!IsLogicalAddressValid (SystemContext,
+SystemContext.SystemContextIa32->Ss,
+(UINTN)Ebp + 4)) {

I don’t understand why you check both ebp and ebp+4, I think it’s enough to 
only check EBP (saved stack pointer address)

Jeff

发件人: Paulo Alcantara
发送时间: 2017年12月29日 12:41
收件人: edk2-devel@lists.01.org
抄送: Laszlo Ersek; Eric 
Dong
主题: [edk2] [RFC v4 5/6] UefiCpuPkg/CpuExceptionHandlerLib: Ensure valid 
frame/stack pointers

Validate all possible memory dereferences during stack traces in IA32
and X64 CPU exceptions.

Contributed-under: TianoCore Contribution Agreement 1.1
Cc: Eric Dong 
Cc: Laszlo Ersek 
Requested-by: Brian Johnson 
Requested-by: Jiewen Yao 
Signed-off-by: Paulo Alcantara 
---
 UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c | 143 
+++-
 UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c  |  75 
+-
 2 files changed, 210 insertions(+), 8 deletions(-)

diff --git 
a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c 
b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c
index 25e02fbbc1..9b52d4f6d2 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c
@@ -398,6 +398,96 @@ DumpCpuContext (
 );
 }

+/**
+  Check if a logical address is valid.
+
+  @param[in]  SystemContext  Pointer to EFI_SYSTEM_CONTEXT.
+  @param[in]  SegmentSelectorSegment selector.
+  @param[in]  Offset Offset or logical address.
+**/
+STATIC
+BOOLEAN
+IsLogicalAddressValid (
+  IN  EFI_SYSTEM_CONTEXT   SystemContext,
+  IN  UINT16   SegmentSelector,
+  IN  UINTNOffset
+  )
+{
+  IA32_SEGMENT_DESCRIPTOR  *SegmentDescriptor;
+  UINT32   SegDescBase;
+  UINT32   SegDescLimit;
+  UINTNSegDescLimitInBytes;
+
+  //
+  // Check for valid input parameters
+  //
+  if (SegmentSelector == 0 || Offset == 0) {
+return FALSE;
+  }
+
+  //
+  // Check whether to look for a segment descriptor in GDT or LDT table
+  //
+  if ((SegmentSelector & BIT2) == 0) {
+//
+// Get segment descriptor from GDT table
+//
+SegmentDescriptor =
+  (IA32_SEGMENT_DESCRIPTOR *)(
+(UINTN)SystemContext.SystemContextIa32->Gdtr[0] +
+((SegmentSelector >> 3) * 8)
+);
+  } else {
+//
+// Get segment descriptor from LDT table
+//
+SegmentDescriptor =
+  (IA32_SEGMENT_DESCRIPTOR *)(
+(UINTN)SystemContext.SystemContextIa32->Ldtr +
+((SegmentSelector >> 3) * 8)
+);
+  }
+
+  //
+  // Get segment descriptor's base address
+  //
+  SegDescBase = SegmentDescriptor->Bits.BaseLow |
+(SegmentDescriptor->Bits.BaseMid << 16) |
+(SegmentDescriptor->Bits.BaseHigh << 24);
+
+  //
+  // Get segment descriptor's limit
+  //
+  SegDescLimit = SegmentDescriptor->Bits.LimitLow |
+(SegmentDescriptor->Bits.LimitHigh << 16);
+
+  //
+  // Calculate segment descriptor's limit in bytes
+  //
+  if (SegmentDescriptor->Bits.G == 1) {
+SegDescLimitInBytes = (UINTN)SegDescLimit * SIZE_4KB;
+  } else {
+SegDescLimitInBytes = SegDescLimit;
+  }
+
+  //
+  // Make sure to not access beyond a segment limit boundary
+  //
+  if (Offset + SegDescBase > SegDescLimitInBytes) {
+return FALSE;
+  }
+
+  //
+  // Check if the translated logical address (or linear address) is valid
+  //
+  return IsLinearAddressValid (
+SystemContext.SystemContextIa32->Cr0,
+SystemContext.SystemContextIa32->Cr3,
+SystemContext.SystemContextIa32->Cr4,
+Offset + SegDescBase
+);
+}
+
 /**
   Dump stack trace.

@@ -459,6 +549,20 @@ DumpStackTrace (
   InternalPrintMessage ("\nCall trace:\n");

   for (;;) {
+//
+// Check for valid frame pointer
+//
+if (!IsLogicalAddressValid (SystemContext,
+SystemContext.SystemContextIa32->Ss,
+(UINTN)Ebp + 4) ||
+!IsLogicalAddressValid (SystemContext,
+SystemContext.SystemContextIa32->Ss,
+(UINTN)Ebp)) {
+  InternalPrintMessage ("%a: attempted to dereference an invalid frame "
+"pointer at 0x%08x\n", __FUNCTION__, Ebp);
+  break;
+}
+
 //
 // Print stack frame in the following format:
 //
@@ -588,6 +692,16 @@ DumpImageModuleNames (
   // Walk through call stack and find next module names
   //
   for 

[edk2] 答复: [RFC v4 4/6] UefiCpuPkg/CpuExceptionHandlerLib: Add helper to valid memory addresses

[Fan Jeff]

Paul,

+  //
+  // Calculate physical address of PML4E
+  //
+  PhysicalAddress = (UINT64)Cr3 & (((1ULL << MaxPhyAddrBits) - 1) << 12);
+  PhysicalAddress |= (((UINT64)LinearAddress >> 39) & 0x1FF) << 3;
+

Should not pass VS build, instead you could use LShiftU64/RShiftU64 to do 64bit 
shift operation as below:

  PhysicalAddress = (UINT64)Cr3 & LShiftU64 (LShiftU64 (1, MaxPhyAddrBits) - 1, 
12);
  PhysicalAddress |= LShiftU64 (RShiftU64((UINT64)LinearAddress, 39) & 0x1FF), 
3);

Jeff

发件人: Paulo Alcantara
发送时间: 2017年12月29日 12:41
收件人: edk2-devel@lists.01.org
抄送: Laszlo Ersek; Eric 
Dong
主题: [edk2] [RFC v4 4/6] UefiCpuPkg/CpuExceptionHandlerLib: Add helper to valid 
memory addresses

Introduce IsLinearAddressValid() function that will be used for
validating memory addresses that would get dereferenced during stack
traces in IA32 and X64 CPU exceptions.

Contributed-under: TianoCore Contribution Agreement 1.1
Cc: Eric Dong 
Cc: Laszlo Ersek 
Requested-by: Brian Johnson 
Requested-by: Jiewen Yao 
Signed-off-by: Paulo Alcantara 
---
 UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c | 382 

 UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.h |  16 +
 2 files changed, 398 insertions(+)

diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c 
b/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c
index 867c5c01d6..52b3eb1463 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.c
@@ -14,6 +14,9 @@

 #include "CpuExceptionCommon.h"

+#include 
+#include 
+
 //
 // Error code flag indicating whether or not an error code will be
 // pushed on the stack if an exception occurs.
@@ -194,3 +197,382 @@ GetPdbFileName (
 }
   }
 }
+
+/**
+  Check if a linear address is valid by walking the page tables in 4-level
+  paging mode.
+
+  @param[in]  Cr3 CR3 control register.
+  @param[in]  MaxPhyAddrBits  MAXPHYADDRBITS bits.
+  @param[in]  LinearAddress   Linear address to be checked.
+**/
+STATIC
+BOOLEAN
+Do4LevelPagingModeCheck (
+  IN UINTNCr3,
+  IN UINT8MaxPhyAddrBits,
+  IN UINTNLinearAddress
+  )
+{
+  UINT64 PhysicalAddress;
+  UINT64 *Pml4TableEntry;
+  UINT64 *PageDirPtrTableEntry;
+  UINT64 *PageDirEntry;
+  UINT64 *PageTableEntry;
+
+  //
+  // In 4-level paging mode, linear addresses are 48 bits wide
+  //
+  if ((UINT64)LinearAddress > (1ULL << 48) - 1) {
+return FALSE;
+  }
+
+  //
+  // Calculate physical address of PML4E
+  //
+  PhysicalAddress = (UINT64)Cr3 & (((1ULL << MaxPhyAddrBits) - 1) << 12);
+  PhysicalAddress |= (((UINT64)LinearAddress >> 39) & 0x1FF) << 3;
+
+  ASSERT ((PhysicalAddress & (sizeof (*Pml4TableEntry) - 1)) == 0);
+
+  Pml4TableEntry = (UINT64 *)(UINTN)PhysicalAddress;
+
+  //
+  // Check if a PDPTE is present
+  //
+  if ((*Pml4TableEntry & BIT0) == 0) {
+return FALSE;
+  }
+
+  //
+  // Calculate physical address of PDPTE
+  //
+  PhysicalAddress = *Pml4TableEntry & (((1ULL << MaxPhyAddrBits) - 1) << 12);
+  PhysicalAddress |= (((UINT64)LinearAddress >> 30) & 0x1FF) << 3;
+
+  ASSERT ((PhysicalAddress & (sizeof (*PageDirPtrTableEntry) - 1)) == 0);
+
+  PageDirPtrTableEntry = (UINT64 *)(UINTN)PhysicalAddress;
+
+  //
+  // Check whether a PDPTE or 1GiB page entry is present
+  //
+  if ((*PageDirPtrTableEntry & BIT0) == 0) {
+return FALSE;
+  }
+
+  //
+  // Check if PDPTE maps an 1GiB page
+  //
+  if ((*PageDirPtrTableEntry & BIT7) != 0) {
+return TRUE;
+  }
+
+  //
+  // Calculate physical address of PDE
+  //
+  PhysicalAddress = *PageDirPtrTableEntry & (((1ULL << MaxPhyAddrBits) - 1) <<
+ 12);
+  PhysicalAddress |= (((UINT64)LinearAddress >> 21) & 0x1FF) << 3;
+
+  ASSERT ((PhysicalAddress & (sizeof (*PageDirEntry) - 1)) == 0);
+
+  PageDirEntry = (UINT64 *)(UINTN)PhysicalAddress;
+
+  //
+  // Check whether a PDE or a 2MiB page entry is present
+  //
+  if ((*PageDirEntry & BIT0) == 0) {
+return FALSE;
+  }
+
+  //
+  // Check if PDE maps a 2MiB page
+  //
+  if ((*PageDirEntry & BIT7) != 0) {
+return TRUE;
+  }
+
+  //
+  // Calculate physical address of PTE
+  //
+  PhysicalAddress = *PageDirEntry & (((1ULL << MaxPhyAddrBits) - 1) << 12);
+  PhysicalAddress |= (((UINT64)LinearAddress >> 12) & 0x1FF) << 3;
+
+  ASSERT ((PhysicalAddress & (sizeof (*PageTableEntry) - 1)) == 0);
+
+  PageTableEntry = (UINT64 *)(UINTN)PhysicalAddress;
+
+  //
+  // Check if PTE maps a 4KiB page
+  //
+  if ((*PageTableEntry & BIT0) == 0) {
+return FALSE;
+  }
+
+  return TRUE;
+}
+
+/**
+  Check if a linear address is valid by walking the page tables in 32-bit 
paging
+  mode.
+
+  @param[in]  Cr3  

Re: [edk2] [PATCH] BaseTools/DevicePath: fix GCC build error in print_mem(), and clean it up

[Zhu, Yonghong]

I pushed this fix first since it blocked some GCC tool chain.

SHA-1: 9a6b445bc2e6e2db6f67ab3cc425d5831aa1b7c8 

Best Regards,
Zhu Yonghong


-Original Message-
From: Zhu, Yonghong 
Sent: Wednesday, January 3, 2018 9:27 AM
To: Laszlo Ersek ; edk2-devel-01 
Cc: Gao, Liming ; Zhu, Yonghong 
Subject: RE: [PATCH] BaseTools/DevicePath: fix GCC build error in print_mem(), 
and clean it up

Thanks.

Reviewed-by: Yonghong Zhu  

Best Regards,
Zhu Yonghong


-Original Message-
From: Laszlo Ersek [mailto:ler...@redhat.com]
Sent: Wednesday, January 03, 2018 1:17 AM
To: edk2-devel-01 
Cc: Gao, Liming ; Zhu, Yonghong 
Subject: [PATCH] BaseTools/DevicePath: fix GCC build error in print_mem(), and 
clean it up

Currently "BaseTools/Source/C/DevicePath/DevicePath.c" fails to build with
GCC48:

> DevicePath.c: In function 'print_mem':
> DevicePath.c:109:5: error: 'for' loop initial declarations are only 
> allowed in C99 mode
>  for (size_t i=0; i  ^
> DevicePath.c:109:5: note: use option -std=c99 or -std=gnu99 to compile 
> your code

In addition, the print_mem() function does not conform to the edk2 coding
style:

- we use CamelCase and no underscores in identifiers,
- the types and type qualifiers should follow the edk2 style,
- initialization as part of definition is forbidden for local variables.

Clean these up.

While updating the print_mem()/PrintMem() call sites, also remove the 
superfluous parentheses around the second argument.

Cc: Liming Gao 
Cc: Yonghong Zhu 
Fixes: 7dbc50bd244d95fdc1741b9cfc561f0bfd724de1
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek 
---

Notes:
Build-tested only (I don't have a DSC file with a device path PCD).

 BaseTools/Source/C/DevicePath/DevicePath.c | 20 ++--
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/BaseTools/Source/C/DevicePath/DevicePath.c 
b/BaseTools/Source/C/DevicePath/DevicePath.c
index 4c87163209ab..76b8553b7145 100644
--- a/BaseTools/Source/C/DevicePath/DevicePath.c
+++ b/BaseTools/Source/C/DevicePath/DevicePath.c
@@ -103,11 +103,19 @@ Returns:
 }
 
 
-void print_mem(void const *vp, size_t n)
+STATIC
+VOID
+PrintMem (
+  CONST VOID *Buffer,
+  UINTN  Count
+  )
 {
-unsigned char const *p = vp;
-for (size_t i=0; iType == END_DEVICE_PATH_TYPE) && (DevicePath->SubType 
== END_ENTIRE_DEVICE_PATH_SUBTYPE)) )
   {
-print_mem(DevicePath, (DevicePath->Length[0] | DevicePath->Length[1] << 
8));
+PrintMem (DevicePath, DevicePath->Length[0] | DevicePath->Length[1] 
+ << 8);
 DevicePath = (EFI_DEVICE_PATH_PROTOCOL *)((UINT8 *)DevicePath + 
(DevicePath->Length[0] | DevicePath->Length[1] << 8));
   }
-  print_mem(DevicePath, (DevicePath->Length[0] | DevicePath->Length[1] << 8));
+  PrintMem (DevicePath, DevicePath->Length[0] | DevicePath->Length[1] 
+ << 8);
   putchar('\n');
   return STATUS_SUCCESS;
 }
--
2.14.1.3.gb7cf6e02401b

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