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.w...@intel.com>; edk2-devel@lists.01.org
> Cc: Dong, Eric <eric.d...@intel.com>; Laszlo Ersek <ler...@redhat.com>; Yao,
> Jiewen <jiewen....@intel.com>; Kinney, Michael D
> <michael.d.kin...@intel.com>; Justen, Jordan L <jordan.l.jus...@intel.com>;
> Wolman, Ayellet <ayellet.wol...@intel.com>; Zeng, Star <star.z...@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.z...@intel.com>; Dong, Eric <eric.d...@intel.com>; Laszlo
> Ersek <ler...@redhat.com>; Yao, Jiewen <jiewen....@intel.com>; Kinney,
> Michael D <michael.d.kin...@intel.com>; Justen, Jordan L
> <jordan.l.jus...@intel.com>; Wolman, Ayellet <ayellet.wol...@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.z...@intel.com>
> Cc: Eric Dong <eric.d...@intel.com>
> Cc: Laszlo Ersek <ler...@redhat.com>
> Cc: Jiewen Yao <jiewen....@intel.com>
> Cc: Michael Kinney <michael.d.kin...@intel.com>
> Cc: Jordan Justen <jordan.l.jus...@intel.com>
> Cc: Ayellet Wolman <ayellet.wol...@intel.com>
> Suggested-by: Ayellet Wolman <ayellet.wol...@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.w...@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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
