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

["Ni, Ruiyu" <ruiyu.ni@Intel.com>]

On 12/7/2017 1:40 PM, Jian J Wang wrote:
>> v2:
>> a. Remove unnecessary braces enclosing code passed to ACCESS_PAGE0_CODE
> 
> Current implementation uses following two methods
> 
>      EnableNullDetection()
>      DisableNullDetection()
> 
> to enable/disable page 0. These two methods will check PCD
> PcdNullPointerDetectionPropertyMask to know if the page 0 is disabled or not.
> This is due to the fact that old GCD service doesn't provide paging related
> attributes of memory block. Since this issue has been fixed, GCD services
> can be used to determine the paging status of page 0. This is also make it
> possible to just use a new macro
> 
>      ACCESS_PAGE0_CODE(
>        <code accessing page 0>
>      );
> 
> to replace above methods to do the same job, which also makes code more
> readability.
> 
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
>   .../Csm/BiosThunk/KeyboardDxe/BiosKeyboard.c       | 118 ++-------------------
>   .../Csm/BiosThunk/KeyboardDxe/KeyboardDxe.inf      |   1 -
>   2 files changed, 10 insertions(+), 109 deletions(-)
> 
> diff --git a/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/BiosKeyboard.c b/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/BiosKeyboard.c
> index ebf03d30c1..ec525891dc 100644
> --- a/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/BiosKeyboard.c
> +++ b/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/BiosKeyboard.c
> @@ -1732,98 +1732,6 @@ CheckKeyboardConnect (
>     }
>   }
>   
> -/**
> -  Disable NULL pointer detection.
> -**/
> -VOID
> -DisableNullDetection (
> -  VOID
> -  )
> -{
> -  EFI_STATUS                            Status;
> -  EFI_GCD_MEMORY_SPACE_DESCRIPTOR       Desc;
> -
> -  if ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & BIT0) == 0) {
> -    return;
> -  }
> -
> -  //
> -  // Check current capabilities and attributes
> -  //
> -  Status = gDS->GetMemorySpaceDescriptor (0, &Desc);
> -  ASSERT_EFI_ERROR (Status);
> -
> -  //
> -  // Try to add EFI_MEMORY_RP support if necessary
> -  //
> -  if ((Desc.Capabilities & EFI_MEMORY_RP) == 0) {
> -    Desc.Capabilities |= EFI_MEMORY_RP;
> -    Status = gDS->SetMemorySpaceCapabilities (0, EFI_PAGES_TO_SIZE(1),
> -                                              Desc.Capabilities);
> -    ASSERT_EFI_ERROR (Status);
> -    if (EFI_ERROR (Status)) {
> -      return;
> -    }
> -  }
> -
> -  //
> -  // Don't bother if EFI_MEMORY_RP is already cleared.
> -  //
> -  if ((Desc.Attributes & EFI_MEMORY_RP) != 0) {
> -    Desc.Attributes &= ~EFI_MEMORY_RP;
> -    Status = gDS->SetMemorySpaceAttributes (0, EFI_PAGES_TO_SIZE(1),
> -                                            Desc.Attributes);
> -    ASSERT_EFI_ERROR (Status);
> -  } else {
> -    DEBUG ((DEBUG_WARN, "!!! Page 0 is supposed to be disabled !!!\r\n"));
> -  }
> -}
> -
> -/**
> -   Enable NULL pointer detection.
> -**/
> -VOID
> -EnableNullDetection (
> -  VOID
> -  )
> -{
> -  EFI_STATUS                            Status;
> -  EFI_GCD_MEMORY_SPACE_DESCRIPTOR       Desc;
> -
> -  if ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & BIT0) == 0) {
> -    return;
> -  }
> -
> -  //
> -  // Check current capabilities and attributes
> -  //
> -  Status = gDS->GetMemorySpaceDescriptor (0, &Desc);
> -  ASSERT_EFI_ERROR (Status);
> -
> -  //
> -  // Try to add EFI_MEMORY_RP support if necessary
> -  //
> -  if ((Desc.Capabilities & EFI_MEMORY_RP) == 0) {
> -    Desc.Capabilities |= EFI_MEMORY_RP;
> -    Status = gDS->SetMemorySpaceCapabilities (0, EFI_PAGES_TO_SIZE(1),
> -                                              Desc.Capabilities);
> -    ASSERT_EFI_ERROR (Status);
> -    if (EFI_ERROR (Status)) {
> -      return;
> -    }
> -  }
> -
> -  //
> -  // Don't bother if EFI_MEMORY_RP is already set.
> -  //
> -  if ((Desc.Attributes & EFI_MEMORY_RP) == 0) {
> -    Desc.Attributes |= EFI_MEMORY_RP;
> -    Status = gDS->SetMemorySpaceAttributes (0, EFI_PAGES_TO_SIZE(1),
> -                                            Desc.Attributes);
> -    ASSERT_EFI_ERROR (Status);
> -  }
> -}
> -
>   /**
>     Timer event handler: read a series of key stroke from 8042
>     and put them into memory key buffer.
> @@ -1931,16 +1839,13 @@ BiosKeyboardTimerHandler (
>     //   0 Right Shift pressed
>   
>   
> -  //
> -  // Disable NULL pointer detection temporarily
> -  //
> -  DisableNullDetection ();
> -
>     //
>     // Clear the CTRL and ALT BDA flag
>     //
> -  KbFlag1 = *((UINT8 *) (UINTN) 0x417);  // read the STATUS FLAGS 1
> -  KbFlag2 = *((UINT8 *) (UINTN) 0x418); // read STATUS FLAGS 2
> +  ACCESS_PAGE0_CODE (
> +    KbFlag1 = *((UINT8 *) (UINTN) 0x417); // read the STATUS FLAGS 1
> +    KbFlag2 = *((UINT8 *) (UINTN) 0x418); // read STATUS FLAGS 2
> +  );
>   
>     DEBUG_CODE (
>       {
> @@ -2008,15 +1913,12 @@ BiosKeyboardTimerHandler (
>     //
>     // Clear left alt and left ctrl BDA flag
>     //
> -  KbFlag2 &= ~(KB_LEFT_ALT_PRESSED | KB_LEFT_CTRL_PRESSED);
> -  *((UINT8 *) (UINTN) 0x418) = KbFlag2;
> -  KbFlag1 &= ~0x0C;
> -  *((UINT8 *) (UINTN) 0x417) = KbFlag1;
> -
> -  //
> -  // Restore NULL pointer detection
> -  //
> -  EnableNullDetection ();
> +  ACCESS_PAGE0_CODE (
> +    KbFlag2 &= ~(KB_LEFT_ALT_PRESSED | KB_LEFT_CTRL_PRESSED);
> +    *((UINT8 *) (UINTN) 0x418) = KbFlag2;
> +    KbFlag1 &= ~0x0C;
> +    *((UINT8 *) (UINTN) 0x417) = KbFlag1;
> +  );
>     
>     //
>     // Output EFI input key and shift/toggle state
> diff --git a/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/KeyboardDxe.inf b/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/KeyboardDxe.inf
> index 596f4ced44..eaaedbfa9c 100644
> --- a/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/KeyboardDxe.inf
> +++ b/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/KeyboardDxe.inf
> @@ -74,7 +74,6 @@
>   
>   [Pcd]
>     gEfiMdeModulePkgTokenSpaceGuid.PcdFastPS2Detection                  ## SOMETIMES_CONSUMES
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask  ## CONSUMES
>   
>   [UserExtensions.TianoCore."ExtraFiles"]
>     KeyboardDxeExtra.uni
> 
Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>

-- 
Thanks,
Ray