From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Permerror (SPF Permanent Error: More than 10 MX records returned) identity=mailfrom; client-ip=134.134.136.100; helo=mga07.intel.com; envelope-from=ruiyu.ni@intel.com; receiver=edk2-devel@lists.01.org Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 2C0B621B02824 for ; Wed, 6 Dec 2017 22:57:17 -0800 (PST) Received: from orsmga007.jf.intel.com ([10.7.209.58]) by orsmga105.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 06 Dec 2017 23:01:49 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.45,371,1508828400"; d="scan'208";a="752018" Received: from ray-dev.ccr.corp.intel.com (HELO [10.239.9.14]) ([10.239.9.14]) by orsmga007.jf.intel.com with ESMTP; 06 Dec 2017 23:01:48 -0800 To: Jian J Wang , edk2-devel@lists.01.org Cc: Liming Gao , Michael D Kinney References: <20171207054049.18140-1-jian.j.wang@intel.com> <20171207054049.18140-4-jian.j.wang@intel.com> From: "Ni, Ruiyu" Message-ID: <83eb8462-cbe9-c066-7886-a5bd3a91ed46@Intel.com> Date: Thu, 7 Dec 2017 15:01:48 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.5.0 MIME-Version: 1.0 In-Reply-To: <20171207054049.18140-4-jian.j.wang@intel.com> Subject: Re: [PATCH v2 3/3] IntelFrameworkModulePkg/KeyboardDxe: Use macro to enable/disable page 0 X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 07 Dec 2017 06:57:17 -0000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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( > > ); > > to replace above methods to do the same job, which also makes code more > readability. > > Cc: Liming Gao > Cc: Michael D Kinney > Cc: Ruiyu Ni > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Jian J Wang > --- > .../Csm/BiosThunk/KeyboardDxe/BiosKeyboard.c | 118 ++------------------- > .../Csm/BiosThunk/KeyboardDxe/KeyboardDxe.inf | 1 - > 2 files changed, 10 insertions(+), 109 deletions(-) > > diff --git a/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/BiosKeyboard.c b/IntelFrameworkModulePkg/Csm/BiosThunk/KeyboardDxe/BiosKeyboard.c > index ebf03d30c1..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 -- Thanks, Ray