Re: [PATCH v3 3/6] MdeModulePkg/Core/Dxe: Add EndOfDxe workaround

["Wang, Jian J" <jian.j.wang@intel.com>]

Thanks for the feedback. Please see my comments below.

> -----Original Message-----
> From: Zeng, Star
> Sent: Thursday, September 28, 2017 11:35 AM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>; Yao,
> Jiewen <jiewen.yao@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>;
> Wolman, Ayellet <ayellet.wolman@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: RE: [PATCH v3 3/6] MdeModulePkg/Core/Dxe: Add EndOfDxe
> workaround
> 
> Some comments to this patch.
> 
> 1. How about using lower TPL TPL_CALLBACK instead of TPL_NOTIFY for the
> notification?

I think it's safe to use TPL_CALLBACK.

> 2. Should GCD SetMemorySpaceCapabilities + SetMemorySpaceAttributes be
> used instead of gCpu->SetMemoryAttributes()?

Yes. Since the GCG out-of-sync issue has been fixed, GCD service
should be used instead.

> 
> Thanks,
> Star
> -----Original Message-----
> From: Wang, Jian J
> Sent: Thursday, September 28, 2017 9:04 AM
> To: edk2-devel@lists.01.org
> Cc: Zeng, Star <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>; Laszlo
> Ersek <lersek@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; Kinney,
> Michael D <michael.d.kinney@intel.com>; Justen, Jordan L
> <jordan.l.justen@intel.com>; Wolman, Ayellet <ayellet.wolman@intel.com>
> Subject: [PATCH v3 3/6] MdeModulePkg/Core/Dxe: Add EndOfDxe workaround
> 
> One of issue caused by enabling NULL pointer detection is that some PCI device
> OptionROM, binary drivers and binary OS boot loaders may have NULL pointer
> access bugs, which will prevent BIOS from booting and is almost impossible to
> fix. BIT7 of PCD PcdNullPointerDetectionPropertyMask is used as a workaround
> to indicate BIOS to disable NULL pointer detection right after event
> gEfiEndOfDxeEventGroupGuid, and then let boot continue.
> 
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Michael Kinney <michael.d.kinney@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Ayellet Wolman <ayellet.wolman@intel.com>
> Suggested-by: Ayellet Wolman <ayellet.wolman@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
>  MdeModulePkg/Core/Dxe/DxeMain.inf             |  1 +
>  MdeModulePkg/Core/Dxe/Mem/Page.c              |  4 ++-
>  MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 48
> +++++++++++++++++++++++++++
>  3 files changed, 52 insertions(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf
> b/MdeModulePkg/Core/Dxe/DxeMain.inf
> index 30d5984f7c..0a161ffd71 100644
> --- a/MdeModulePkg/Core/Dxe/DxeMain.inf
> +++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
> @@ -192,6 +192,7 @@
>    gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable                   ##
> CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy                   ##
> CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy
> ## CONSUMES
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask
> ## CONSUMES
> 
>  # [Hob]
>  # RESOURCE_DESCRIPTOR   ## CONSUMES
> diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c
> b/MdeModulePkg/Core/Dxe/Mem/Page.c
> index a142c79ee2..0468df3171 100644
> --- a/MdeModulePkg/Core/Dxe/Mem/Page.c
> +++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
> @@ -188,7 +188,9 @@ CoreAddRange (
>    // used for other purposes.
>    //
>    if (Type == EfiConventionalMemory && Start == 0 && (End >= EFI_PAGE_SIZE -
> 1)) {
> -    SetMem ((VOID *)(UINTN)Start, EFI_PAGE_SIZE, 0);
> +    if ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & BIT0) == 0) {
> +      SetMem ((VOID *)(UINTN)Start, EFI_PAGE_SIZE, 0);
> +    }
>    }
> 
>    //
> diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> index a73c4ccd64..73e3b269f3 100644
> --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> @@ -995,6 +995,36 @@ MemoryProtectionExitBootServicesCallback (
>    }
>  }
> 
> +/**
> +  Disable NULL pointer detection after EndOfDxe. This is a workaround
> +resort in
> +  order to skip unfixable NULL pointer access issues detected in
> +OptionROM or
> +  boot loaders.
> +
> +  @param[in]  Event     The Event this notify function registered to.
> +  @param[in]  Context   Pointer to the context data registered to the Event.
> +**/
> +VOID
> +EFIAPI
> +DisableNullDetectionAtTheEndOfDxe (
> +  EFI_EVENT                               Event,
> +  VOID                                    *Context
> +  )
> +{
> +  EFI_STATUS                        Status;
> +
> +  DEBUG ((DEBUG_INFO, "DisableNullDetectionAtTheEndOfDxe():
> + start\r\n"));  //  // Disable NULL pointer detection by enabling first
> + 4K page  //  Status = gCpu->SetMemoryAttributes (gCpu, 0,
> + EFI_PAGE_SIZE, 0);  ASSERT_EFI_ERROR (Status);
> +
> +  CoreCloseEvent (Event);
> +  DEBUG ((DEBUG_INFO, "DisableNullDetectionAtTheEndOfDxe(): end\r\n"));
> +
> +  return;
> +}
> +
>  /**
>    Initialize Memory Protection support.
>  **/
> @@ -1006,6 +1036,7 @@ CoreInitializeMemoryProtection (  {
>    EFI_STATUS  Status;
>    EFI_EVENT   Event;
> +  EFI_EVENT   EndOfDxeEvent;
>    VOID        *Registration;
> 
>    mImageProtectionPolicy = PcdGet32(PcdImageProtectionPolicy);
> @@ -1044,6 +1075,23 @@ CoreInitializeMemoryProtection (
>                 );
>      ASSERT_EFI_ERROR(Status);
>    }
> +
> +  //
> +  // Register a callback to disable NULL pointer detection at EndOfDxe
> + //  if ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & (BIT0|BIT7))
> +       == (BIT0|BIT7)) {
> +    Status = CoreCreateEventEx (
> +                    EVT_NOTIFY_SIGNAL,
> +                    TPL_NOTIFY,
> +                    DisableNullDetectionAtTheEndOfDxe,
> +                    NULL,
> +                    &gEfiEndOfDxeEventGroupGuid,
> +                    &EndOfDxeEvent
> +                    );
> +    ASSERT_EFI_ERROR (Status);
> +  }
> +
>    return ;
>  }
> 
> --
> 2.14.1.windows.1