* [PATCH v2 0/3] Remove dependency on PcdNullPointerDetectionPropertyMask
@ 2017-12-07 5:40 Jian J Wang
2017-12-07 5:40 ` [PATCH v2 1/3] IntelFrameworkPkg/LegacyBios.h: Add a macro to guarantee page 0 access Jian J Wang
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Jian J Wang @ 2017-12-07 5:40 UTC (permalink / raw)
To: edk2-devel
> v2:
> a. Fix a typo in expression in the macro ACCESS_PAGE0_CODE
> b. Fix GCC49 build error
> c. Remove unnecessary braces enclosing code passed to ACCESS_PAGE0_CODE
Due to the introduction of NULL pointer detection feature, page 0 will be
disabled if the feature is enabled, which will cause legacy code failed to
update legacy data in page 0.
To avoid page fault caused by above feature in legacy code, legacy related
drivers have to enable page 0 temporarily before accessing page 0 and disable
it afterwards.
Old GCD servie has a bug which paging related attributes are not awared and
managed by GCD service. The legacy code has to use PCD
PcdNullPointerDetectionPropertyMask to know if page 0 is disabled or not.
As a result, two methods
EnableNullDetection()
DisableNullDetection()
were introduced to do enable/disable job just mentioned.
But the newly added PcdNullPointerDetectionPropertyMask caused backward
compatibility issue in some packages having legcy drivers. Since the
attributes missing issue in GCD services has been fixed, it's now able to
eliminate the dependency on this PCD.
Jian J Wang (3):
IntelFrameworkPkg/LegacyBios.h: Add a macro to guarantee page 0 access
IntelFrameworkModulePkg/LegacyBiosDxe: Use macro to enable/disable
page 0
IntelFrameworkModulePkg/KeyboardDxe: Use macro to enable/disable page
0
.../Csm/BiosThunk/KeyboardDxe/BiosKeyboard.c | 118 ++----------------
.../Csm/BiosThunk/KeyboardDxe/KeyboardDxe.inf | 1 -
.../Csm/LegacyBiosDxe/LegacyBda.c | 53 ++++----
.../Csm/LegacyBiosDxe/LegacyBios.c | 135 ++-------------------
.../Csm/LegacyBiosDxe/LegacyBiosDxe.inf | 1 -
.../Csm/LegacyBiosDxe/LegacyBiosInterface.h | 16 ---
.../Csm/LegacyBiosDxe/LegacyBootSupport.c | 80 ++++++------
.../Csm/LegacyBiosDxe/LegacyPci.c | 72 ++++++-----
IntelFrameworkModulePkg/Csm/LegacyBiosDxe/Thunk.c | 51 ++++----
IntelFrameworkPkg/Include/Protocol/LegacyBios.h | 34 ++++++
10 files changed, 179 insertions(+), 382 deletions(-)
--
2.15.1.windows.2
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2 1/3] IntelFrameworkPkg/LegacyBios.h: Add a macro to guarantee page 0 access
2017-12-07 5:40 [PATCH v2 0/3] Remove dependency on PcdNullPointerDetectionPropertyMask Jian J Wang
@ 2017-12-07 5:40 ` Jian J Wang
2017-12-07 7:00 ` Ni, Ruiyu
2017-12-07 5:40 ` [PATCH v2 2/3] IntelFrameworkModulePkg/LegacyBiosDxe: Use macro to enable/disable page 0 Jian J Wang
2017-12-07 5:40 ` [PATCH v2 3/3] IntelFrameworkModulePkg/KeyboardDxe: " Jian J Wang
2 siblings, 1 reply; 8+ messages in thread
From: Jian J Wang @ 2017-12-07 5:40 UTC (permalink / raw)
To: edk2-devel; +Cc: Liming Gao, Michael D Kinney, Ruiyu Ni
> v2:
> a. Fix a typo in expression in the macro ACCESS_PAGE0_CODE
> b. Fix GCC49 build error
ue to the introduction of NULL pointer detection feature, page 0 will be
disabled if the feature is enabled, which will cause legacy code failed to
update legacy data in page 0. This macro is introduced to make sure the
page 0 is enabled before those code and restore the original status of it
afterwards.
Another reason to introduce this macro is to eliminate the dependency on
the PcdNullPointerDetectionPropertyMask. Because this is a new PCD, it
could cause some backward compatibility issue for some old packages.
This macro will simply check if the page 0 is disabled or not. If it's
disabled, it will enable it before code updating page 0 and disable it
afterwards. Otherwise, this macro will do nothing to page 0.
The usage of the macro will be look like (similar to DEBUG_CODE macro):
ACCESS_PAGE0_CODE(
<code accessing page 0>
);
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>
---
IntelFrameworkPkg/Include/Protocol/LegacyBios.h | 34 +++++++++++++++++++++++++
1 file changed, 34 insertions(+)
diff --git a/IntelFrameworkPkg/Include/Protocol/LegacyBios.h b/IntelFrameworkPkg/Include/Protocol/LegacyBios.h
index 641f101bce..6a5f5464e7 100644
--- a/IntelFrameworkPkg/Include/Protocol/LegacyBios.h
+++ b/IntelFrameworkPkg/Include/Protocol/LegacyBios.h
@@ -1518,6 +1518,40 @@ struct _EFI_LEGACY_BIOS_PROTOCOL {
EFI_LEGACY_BIOS_BOOT_UNCONVENTIONAL_DEVICE BootUnconventionalDevice;
};
+//
+// Legacy BIOS needs to access memory in page 0 (0-4095), which is disabled if
+// NULL pointer detection feature is enabled. Following macro can be used to
+// enable/disable page 0 before/after accessing it.
+//
+#define ACCESS_PAGE0_CODE(statements) \
+ do { \
+ EFI_STATUS Status_; \
+ EFI_GCD_MEMORY_SPACE_DESCRIPTOR Desc_; \
+ \
+ Desc_.Attributes = 0; \
+ Status_ = gDS->GetMemorySpaceDescriptor (0, &Desc_); \
+ ASSERT_EFI_ERROR (Status_); \
+ if ((Desc_.Attributes & EFI_MEMORY_RP) != 0) { \
+ Status_ = gDS->SetMemorySpaceAttributes ( \
+ 0, \
+ EFI_PAGES_TO_SIZE(1), \
+ Desc_.Attributes & ~(UINT64)EFI_MEMORY_RP \
+ ); \
+ ASSERT_EFI_ERROR (Status_); \
+ } \
+ \
+ statements; \
+ \
+ if ((Desc_.Attributes & EFI_MEMORY_RP) != 0) { \
+ Status_ = gDS->SetMemorySpaceAttributes ( \
+ 0, \
+ EFI_PAGES_TO_SIZE(1), \
+ Desc_.Attributes \
+ ); \
+ ASSERT_EFI_ERROR (Status_); \
+ } \
+ } while (FALSE)
+
extern EFI_GUID gEfiLegacyBiosProtocolGuid;
#endif
--
2.15.1.windows.2
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v2 2/3] IntelFrameworkModulePkg/LegacyBiosDxe: Use macro to enable/disable page 0
2017-12-07 5:40 [PATCH v2 0/3] Remove dependency on PcdNullPointerDetectionPropertyMask Jian J Wang
2017-12-07 5:40 ` [PATCH v2 1/3] IntelFrameworkPkg/LegacyBios.h: Add a macro to guarantee page 0 access Jian J Wang
@ 2017-12-07 5:40 ` Jian J Wang
2017-12-07 7:01 ` Ni, Ruiyu
2017-12-07 5:40 ` [PATCH v2 3/3] IntelFrameworkModulePkg/KeyboardDxe: " Jian J Wang
2 siblings, 1 reply; 8+ messages in thread
From: Jian J Wang @ 2017-12-07 5:40 UTC (permalink / raw)
To: edk2-devel; +Cc: Liming Gao, Michael D Kinney, Ruiyu Ni
> v2:
> a. Remove unnecessary braces enclosing code passed to ACCESS_PAGE0_CODE
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/LegacyBiosDxe/LegacyBda.c | 53 ++++----
.../Csm/LegacyBiosDxe/LegacyBios.c | 135 ++-------------------
.../Csm/LegacyBiosDxe/LegacyBiosDxe.inf | 1 -
.../Csm/LegacyBiosDxe/LegacyBiosInterface.h | 16 ---
.../Csm/LegacyBiosDxe/LegacyBootSupport.c | 80 ++++++------
.../Csm/LegacyBiosDxe/LegacyPci.c | 72 ++++++-----
IntelFrameworkModulePkg/Csm/LegacyBiosDxe/Thunk.c | 51 ++++----
7 files changed, 135 insertions(+), 273 deletions(-)
diff --git a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBda.c b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBda.c
index c6670febee..a94cba435c 100644
--- a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBda.c
+++ b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBda.c
@@ -34,37 +34,36 @@ LegacyBiosInitBda (
BDA_STRUC *Bda;
UINT8 *Ebda;
- DisableNullDetection ();
-
Bda = (BDA_STRUC *) ((UINTN) 0x400);
Ebda = (UINT8 *) ((UINTN) 0x9fc00);
- ZeroMem (Bda, 0x100);
+ ACCESS_PAGE0_CODE (
+ ZeroMem (Bda, 0x100);
+ //
+ // 640k-1k for EBDA
+ //
+ Bda->MemSize = 0x27f;
+ Bda->KeyHead = 0x1e;
+ Bda->KeyTail = 0x1e;
+ Bda->FloppyData = 0x00;
+ Bda->FloppyTimeout = 0xff;
+
+ Bda->KeyStart = 0x001E;
+ Bda->KeyEnd = 0x003E;
+ Bda->KeyboardStatus = 0x10;
+ Bda->Ebda = 0x9fc0;
+
+ //
+ // Move LPT time out here and zero out LPT4 since some SCSI OPROMS
+ // use this as scratch pad (LPT4 is Reserved)
+ //
+ Bda->Lpt1_2Timeout = 0x1414;
+ Bda->Lpt3_4Timeout = 0x1400;
+
+ );
+
ZeroMem (Ebda, 0x400);
- //
- // 640k-1k for EBDA
- //
- Bda->MemSize = 0x27f;
- Bda->KeyHead = 0x1e;
- Bda->KeyTail = 0x1e;
- Bda->FloppyData = 0x00;
- Bda->FloppyTimeout = 0xff;
-
- Bda->KeyStart = 0x001E;
- Bda->KeyEnd = 0x003E;
- Bda->KeyboardStatus = 0x10;
- Bda->Ebda = 0x9fc0;
-
- //
- // Move LPT time out here and zero out LPT4 since some SCSI OPROMS
- // use this as scratch pad (LPT4 is Reserved)
- //
- Bda->Lpt1_2Timeout = 0x1414;
- Bda->Lpt3_4Timeout = 0x1400;
-
- *Ebda = 0x01;
-
- EnableNullDetection ();
+ *Ebda = 0x01;
return EFI_SUCCESS;
}
diff --git a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBios.c b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBios.c
index c6461f5547..fca08a8fa2 100644
--- a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBios.c
+++ b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBios.c
@@ -786,115 +786,6 @@ ToggleEndOfDxeStatus (
return;
}
-//
-// Legacy BIOS needs to access memory between 0-4095, which will cause page
-// fault exception if NULL pointer detection mechanism is enabled. Following
-// functions can be used to disable/enable NULL pointer detection before/after
-// accessing those memory.
-//
-
-/**
- Enable NULL pointer detection.
-**/
-VOID
-EnableNullDetection (
- VOID
- )
-{
- EFI_STATUS Status;
- EFI_GCD_MEMORY_SPACE_DESCRIPTOR Desc;
-
- if (((PcdGet8 (PcdNullPointerDetectionPropertyMask) & BIT0) == 0)
- ||
- ((mEndOfDxe) &&
- ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & (BIT7|BIT0))
- == (BIT7|BIT0)))
- ) {
- 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);
- }
-}
-
-/**
- Disable NULL pointer detection.
-**/
-VOID
-DisableNullDetection (
- VOID
- )
-{
- EFI_STATUS Status;
- EFI_GCD_MEMORY_SPACE_DESCRIPTOR Desc;
-
- if (((PcdGet8 (PcdNullPointerDetectionPropertyMask) & BIT0) == 0)
- ||
- ((mEndOfDxe) &&
- ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & (BIT7|BIT0))
- == (BIT7|BIT0)))
- ) {
- 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"));
- }
-}
-
/**
Install Driver to produce Legacy BIOS protocol.
@@ -1095,10 +986,10 @@ LegacyBiosInstall (
// Initialize region from 0x0000 to 4k. This initializes interrupt vector
// range.
//
- DisableNullDetection ();
- gBS->SetMem ((VOID *) ClearPtr, 0x400, INITIAL_VALUE_BELOW_1K);
- ZeroMem ((VOID *) ((UINTN)ClearPtr + 0x400), 0xC00);
- EnableNullDetection ();
+ ACCESS_PAGE0_CODE (
+ gBS->SetMem ((VOID *) ClearPtr, 0x400, INITIAL_VALUE_BELOW_1K);
+ ZeroMem ((VOID *) ((UINTN)ClearPtr + 0x400), 0xC00);
+ );
//
// Allocate pages for OPROM usage
@@ -1237,16 +1128,14 @@ LegacyBiosInstall (
//
// Save Unexpected interrupt vector so can restore it just prior to boot
//
- DisableNullDetection ();
-
- BaseVectorMaster = (UINT32 *) (sizeof (UINT32) * PROTECTED_MODE_BASE_VECTOR_MASTER);
- Private->BiosUnexpectedInt = BaseVectorMaster[0];
- IntRedirCode = (UINT32) (UINTN) Private->IntThunk->InterruptRedirectionCode;
- for (Index = 0; Index < 8; Index++) {
- BaseVectorMaster[Index] = (EFI_SEGMENT (IntRedirCode + Index * 4) << 16) | EFI_OFFSET (IntRedirCode + Index * 4);
- }
-
- EnableNullDetection ();
+ ACCESS_PAGE0_CODE (
+ BaseVectorMaster = (UINT32 *) (sizeof (UINT32) * PROTECTED_MODE_BASE_VECTOR_MASTER);
+ Private->BiosUnexpectedInt = BaseVectorMaster[0];
+ IntRedirCode = (UINT32) (UINTN) Private->IntThunk->InterruptRedirectionCode;
+ for (Index = 0; Index < 8; Index++) {
+ BaseVectorMaster[Index] = (EFI_SEGMENT (IntRedirCode + Index * 4) << 16) | EFI_OFFSET (IntRedirCode + Index * 4);
+ }
+ );
//
// Save EFI value
diff --git a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBiosDxe.inf b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBiosDxe.inf
index 6efc7f36ae..180c18e3fc 100644
--- a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBiosDxe.inf
+++ b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBiosDxe.inf
@@ -148,7 +148,6 @@
gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdHighPmmMemorySize ## CONSUMES
gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdOpromReservedMemoryBase ## CONSUMES
gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdOpromReservedMemorySize ## CONSUMES
- gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask ## CONSUMES
[Depex]
gEfiLegacyRegion2ProtocolGuid AND gEfiLegacyInterruptProtocolGuid AND gEfiLegacyBiosPlatformProtocolGuid AND gEfiLegacy8259ProtocolGuid AND gEfiGenericMemTestProtocolGuid AND gEfiCpuArchProtocolGuid AND gEfiTimerArchProtocolGuid AND gEfiVariableWriteArchProtocolGuid
diff --git a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBiosInterface.h b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBiosInterface.h
index 86a3b09080..cc2fc9fc13 100644
--- a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBiosInterface.h
+++ b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBiosInterface.h
@@ -1544,20 +1544,4 @@ LegacyBiosInstallVgaRom (
IN LEGACY_BIOS_INSTANCE *Private
);
-/**
- Enable NULL pointer detection.
-**/
-VOID
-EnableNullDetection (
- VOID
- );
-
-/**
- Disable NULL pointer detection.
-**/
-VOID
-DisableNullDetection (
- VOID
- );
-
#endif
diff --git a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBootSupport.c b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBootSupport.c
index c2ac69ce69..57ab78d648 100644
--- a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBootSupport.c
+++ b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBootSupport.c
@@ -1041,7 +1041,9 @@ GenericLegacyBoot (
//
// Setup BDA and EBDA standard areas before Legacy Boot
//
- LegacyBiosCompleteBdaBeforeBoot (Private);
+ ACCESS_PAGE0_CODE (
+ LegacyBiosCompleteBdaBeforeBoot (Private);
+ );
LegacyBiosCompleteStandardCmosBeforeBoot (Private);
//
@@ -1073,10 +1075,10 @@ GenericLegacyBoot (
// Use 182/10 to avoid floating point math.
//
LocalTime = (LocalTime * 182) / 10;
- DisableNullDetection ();
- BdaPtr = (UINT32 *) (UINTN)0x46C;
- *BdaPtr = LocalTime;
- EnableNullDetection ();
+ ACCESS_PAGE0_CODE (
+ BdaPtr = (UINT32 *) (UINTN)0x46C;
+ *BdaPtr = LocalTime;
+ );
//
// Shadow PCI ROMs. We must do this near the end since this will kick
@@ -1322,15 +1324,15 @@ GenericLegacyBoot (
// set of TIANO vectors) or takes it over.
//
//
- DisableNullDetection ();
- BaseVectorMaster = (UINT32 *) (sizeof (UINT32) * PROTECTED_MODE_BASE_VECTOR_MASTER);
- for (Index = 0; Index < 8; Index++) {
- Private->ThunkSavedInt[Index] = BaseVectorMaster[Index];
- if (Private->ThunkSeg == (UINT16) (BaseVectorMaster[Index] >> 16)) {
- BaseVectorMaster[Index] = (UINT32) (Private->BiosUnexpectedInt);
+ ACCESS_PAGE0_CODE (
+ BaseVectorMaster = (UINT32 *) (sizeof (UINT32) * PROTECTED_MODE_BASE_VECTOR_MASTER);
+ for (Index = 0; Index < 8; Index++) {
+ Private->ThunkSavedInt[Index] = BaseVectorMaster[Index];
+ if (Private->ThunkSeg == (UINT16) (BaseVectorMaster[Index] >> 16)) {
+ BaseVectorMaster[Index] = (UINT32) (Private->BiosUnexpectedInt);
+ }
}
- }
- EnableNullDetection ();
+ );
ZeroMem (&Regs, sizeof (EFI_IA32_REGISTER_SET));
Regs.X.AX = Legacy16Boot;
@@ -1344,12 +1346,12 @@ GenericLegacyBoot (
0
);
- DisableNullDetection ();
- BaseVectorMaster = (UINT32 *) (sizeof (UINT32) * PROTECTED_MODE_BASE_VECTOR_MASTER);
- for (Index = 0; Index < 8; Index++) {
- BaseVectorMaster[Index] = Private->ThunkSavedInt[Index];
- }
- EnableNullDetection ();
+ ACCESS_PAGE0_CODE (
+ BaseVectorMaster = (UINT32 *) (sizeof (UINT32) * PROTECTED_MODE_BASE_VECTOR_MASTER);
+ for (Index = 0; Index < 8; Index++) {
+ BaseVectorMaster[Index] = Private->ThunkSavedInt[Index];
+ }
+ );
}
Private->LegacyBootEntered = TRUE;
if ((mBootMode == BOOT_LEGACY_OS) || (mBootMode == BOOT_UNCONVENTIONAL_DEVICE)) {
@@ -1737,11 +1739,11 @@ LegacyBiosBuildE820 (
//
// First entry is 0 to (640k - EBDA)
//
- DisableNullDetection ();
- E820Table[0].BaseAddr = 0;
- E820Table[0].Length = (UINT64) ((*(UINT16 *) (UINTN)0x40E) << 4);
- E820Table[0].Type = EfiAcpiAddressRangeMemory;
- EnableNullDetection ();
+ ACCESS_PAGE0_CODE (
+ E820Table[0].BaseAddr = 0;
+ E820Table[0].Length = (UINT64) ((*(UINT16 *) (UINTN)0x40E) << 4);
+ E820Table[0].Type = EfiAcpiAddressRangeMemory;
+ );
//
// Second entry is (640k - EBDA) to 640k
@@ -1975,8 +1977,6 @@ LegacyBiosCompleteBdaBeforeBoot (
UINT16 MachineConfig;
DEVICE_PRODUCER_DATA_HEADER *SioPtr;
- DisableNullDetection ();
-
Bda = (BDA_STRUC *) ((UINTN) 0x400);
MachineConfig = 0;
@@ -2035,8 +2035,6 @@ LegacyBiosCompleteBdaBeforeBoot (
MachineConfig = (UINT16) (MachineConfig + 0x00 + 0x02 + (SioPtr->MousePresent * 0x04));
Bda->MachineConfig = MachineConfig;
- EnableNullDetection ();
-
return EFI_SUCCESS;
}
@@ -2063,17 +2061,15 @@ LegacyBiosUpdateKeyboardLedStatus (
Private = LEGACY_BIOS_INSTANCE_FROM_THIS (This);
- DisableNullDetection ();
-
- Bda = (BDA_STRUC *) ((UINTN) 0x400);
- LocalLeds = Leds;
- Bda->LedStatus = (UINT8) ((Bda->LedStatus &~0x07) | LocalLeds);
- LocalLeds = (UINT8) (LocalLeds << 4);
- Bda->ShiftStatus = (UINT8) ((Bda->ShiftStatus &~0x70) | LocalLeds);
- LocalLeds = (UINT8) (Leds & 0x20);
- Bda->KeyboardStatus = (UINT8) ((Bda->KeyboardStatus &~0x20) | LocalLeds);
-
- EnableNullDetection ();
+ ACCESS_PAGE0_CODE (
+ Bda = (BDA_STRUC *) ((UINTN) 0x400);
+ LocalLeds = Leds;
+ Bda->LedStatus = (UINT8) ((Bda->LedStatus &~0x07) | LocalLeds);
+ LocalLeds = (UINT8) (LocalLeds << 4);
+ Bda->ShiftStatus = (UINT8) ((Bda->ShiftStatus &~0x70) | LocalLeds);
+ LocalLeds = (UINT8) (Leds & 0x20);
+ Bda->KeyboardStatus = (UINT8) ((Bda->KeyboardStatus &~0x20) | LocalLeds);
+ );
//
// Call into Legacy16 code to allow it to do any processing
@@ -2119,9 +2115,9 @@ LegacyBiosCompleteStandardCmosBeforeBoot (
// to large capacity drives
// CMOS 14 = BDA 40:10 plus bit 3(display enabled)
//
- DisableNullDetection ();
- Bda = (UINT8)(*((UINT8 *)((UINTN)0x410)) | BIT3);
- EnableNullDetection ();
+ ACCESS_PAGE0_CODE (
+ Bda = (UINT8)(*((UINT8 *)((UINTN)0x410)) | BIT3);
+ );
//
// Force display enabled
diff --git a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyPci.c b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyPci.c
index d38cef3e33..b10a9dcef6 100644
--- a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyPci.c
+++ b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyPci.c
@@ -2403,35 +2403,33 @@ LegacyBiosInstallRom (
// 2. BBS compliants drives will not change 40:75 until boot time.
// 3. Onboard IDE controllers will change 40:75
//
- DisableNullDetection ();
-
- LocalDiskStart = (UINT8) ((*(UINT8 *) ((UINTN) 0x475)) + 0x80);
- if ((Private->Disk4075 + 0x80) < LocalDiskStart) {
- //
- // Update table since onboard IDE drives found
- //
- Private->LegacyEfiHddTable[Private->LegacyEfiHddTableIndex].PciSegment = 0xff;
- Private->LegacyEfiHddTable[Private->LegacyEfiHddTableIndex].PciBus = 0xff;
- Private->LegacyEfiHddTable[Private->LegacyEfiHddTableIndex].PciDevice = 0xff;
- Private->LegacyEfiHddTable[Private->LegacyEfiHddTableIndex].PciFunction = 0xff;
- Private->LegacyEfiHddTable[Private->LegacyEfiHddTableIndex].StartDriveNumber = (UINT8) (Private->Disk4075 + 0x80);
- Private->LegacyEfiHddTable[Private->LegacyEfiHddTableIndex].EndDriveNumber = LocalDiskStart;
- Private->LegacyEfiHddTableIndex ++;
- Private->Disk4075 = (UINT8) (LocalDiskStart & 0x7f);
- Private->DiskEnd = LocalDiskStart;
- }
-
- if (PciHandle != mVgaHandle) {
+ ACCESS_PAGE0_CODE (
+ LocalDiskStart = (UINT8) ((*(UINT8 *) ((UINTN) 0x475)) + 0x80);
+ if ((Private->Disk4075 + 0x80) < LocalDiskStart) {
+ //
+ // Update table since onboard IDE drives found
+ //
+ Private->LegacyEfiHddTable[Private->LegacyEfiHddTableIndex].PciSegment = 0xff;
+ Private->LegacyEfiHddTable[Private->LegacyEfiHddTableIndex].PciBus = 0xff;
+ Private->LegacyEfiHddTable[Private->LegacyEfiHddTableIndex].PciDevice = 0xff;
+ Private->LegacyEfiHddTable[Private->LegacyEfiHddTableIndex].PciFunction = 0xff;
+ Private->LegacyEfiHddTable[Private->LegacyEfiHddTableIndex].StartDriveNumber = (UINT8) (Private->Disk4075 + 0x80);
+ Private->LegacyEfiHddTable[Private->LegacyEfiHddTableIndex].EndDriveNumber = LocalDiskStart;
+ Private->LegacyEfiHddTableIndex ++;
+ Private->Disk4075 = (UINT8) (LocalDiskStart & 0x7f);
+ Private->DiskEnd = LocalDiskStart;
+ }
- EnablePs2Keyboard ();
+ if (PciHandle != mVgaHandle) {
- //
- // Store current mode settings since PrepareToScanRom may change mode.
- //
- VideoMode = *(UINT8 *) ((UINTN) (0x400 + BDA_VIDEO_MODE));
- }
+ EnablePs2Keyboard ();
- EnableNullDetection ();
+ //
+ // Store current mode settings since PrepareToScanRom may change mode.
+ //
+ VideoMode = *(UINT8 *) ((UINTN) (0x400 + BDA_VIDEO_MODE));
+ }
+ );
//
// Notify the platform that we are about to scan the ROM
@@ -2473,11 +2471,11 @@ LegacyBiosInstallRom (
// Multiply result by 18.2 for number of ticks since midnight.
// Use 182/10 to avoid floating point math.
//
- DisableNullDetection ();
- LocalTime = (LocalTime * 182) / 10;
- BdaPtr = (UINT32 *) ((UINTN) 0x46C);
- *BdaPtr = LocalTime;
- EnableNullDetection ();
+ ACCESS_PAGE0_CODE (
+ LocalTime = (LocalTime * 182) / 10;
+ BdaPtr = (UINT32 *) ((UINTN) 0x46C);
+ *BdaPtr = LocalTime;
+ );
//
// Pass in handoff data
@@ -2573,9 +2571,9 @@ LegacyBiosInstallRom (
//
// Set mode settings since PrepareToScanRom may change mode
//
- DisableNullDetection ();
- OldVideoMode = *(UINT8 *) ((UINTN) (0x400 + BDA_VIDEO_MODE));
- EnableNullDetection ();
+ ACCESS_PAGE0_CODE ({
+ OldVideoMode = *(UINT8 *) ((UINTN) (0x400 + BDA_VIDEO_MODE));
+ });
if (VideoMode != OldVideoMode) {
//
@@ -2617,9 +2615,9 @@ LegacyBiosInstallRom (
}
}
- DisableNullDetection ();
- LocalDiskEnd = (UINT8) ((*(UINT8 *) ((UINTN) 0x475)) + 0x80);
- EnableNullDetection ();
+ ACCESS_PAGE0_CODE (
+ LocalDiskEnd = (UINT8) ((*(UINT8 *) ((UINTN) 0x475)) + 0x80);
+ );
//
// Allow platform to perform any required actions after the
diff --git a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/Thunk.c b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/Thunk.c
index d249479c56..d330f4870b 100644
--- a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/Thunk.c
+++ b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/Thunk.c
@@ -73,10 +73,10 @@ LegacyBiosInt86 (
// The base address of legacy interrupt vector table is 0.
// We use this base address to get the legacy interrupt handler.
//
- DisableNullDetection ();
- Segment = (UINT16)(((UINT32 *)0)[BiosInt] >> 16);
- Offset = (UINT16)((UINT32 *)0)[BiosInt];
- EnableNullDetection ();
+ ACCESS_PAGE0_CODE (
+ Segment = (UINT16)(((UINT32 *)0)[BiosInt] >> 16);
+ Offset = (UINT16)((UINT32 *)0)[BiosInt];
+ );
return InternalLegacyBiosFarCall (
This,
@@ -286,29 +286,6 @@ InternalLegacyBiosFarCall (
AsmThunk16 (&mThunkContext);
- //
- // OPROM may allocate EBDA range by itself and change EBDA base and EBDA size.
- // Get the current EBDA base address, and compared with pre-allocate minimum
- // EBDA base address, if the current EBDA base address is smaller, it indicates
- // PcdEbdaReservedMemorySize should be adjusted to larger for more OPROMs.
- //
- DEBUG_CODE (
- {
- UINTN EbdaBaseAddress;
- UINTN ReservedEbdaBaseAddress;
-
- //
- // Skip this part of debug code if NULL pointer detection is enabled
- //
- if ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & BIT0) == 0) {
- EbdaBaseAddress = (*(UINT16 *) (UINTN) 0x40E) << 4;
- ReservedEbdaBaseAddress = CONVENTIONAL_MEMORY_TOP
- - PcdGet32 (PcdEbdaReservedMemorySize);
- ASSERT (ReservedEbdaBaseAddress <= EbdaBaseAddress);
- }
- }
- );
-
if (Stack != NULL && StackSize != 0) {
//
// Copy low memory stack to Stack
@@ -334,6 +311,26 @@ InternalLegacyBiosFarCall (
//
gBS->RestoreTPL (OriginalTpl);
+ //
+ // OPROM may allocate EBDA range by itself and change EBDA base and EBDA size.
+ // Get the current EBDA base address, and compared with pre-allocate minimum
+ // EBDA base address, if the current EBDA base address is smaller, it indicates
+ // PcdEbdaReservedMemorySize should be adjusted to larger for more OPROMs.
+ //
+ DEBUG_CODE (
+ {
+ UINTN EbdaBaseAddress;
+ UINTN ReservedEbdaBaseAddress;
+
+ ACCESS_PAGE0_CODE (
+ EbdaBaseAddress = (*(UINT16 *) (UINTN) 0x40E) << 4;
+ ReservedEbdaBaseAddress = CONVENTIONAL_MEMORY_TOP
+ - PcdGet32 (PcdEbdaReservedMemorySize);
+ ASSERT (ReservedEbdaBaseAddress <= EbdaBaseAddress);
+ );
+ }
+ );
+
//
// Restore interrupt of debug timer
//
--
2.15.1.windows.2
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v2 3/3] IntelFrameworkModulePkg/KeyboardDxe: Use macro to enable/disable page 0
2017-12-07 5:40 [PATCH v2 0/3] Remove dependency on PcdNullPointerDetectionPropertyMask Jian J Wang
2017-12-07 5:40 ` [PATCH v2 1/3] IntelFrameworkPkg/LegacyBios.h: Add a macro to guarantee page 0 access Jian J Wang
2017-12-07 5:40 ` [PATCH v2 2/3] IntelFrameworkModulePkg/LegacyBiosDxe: Use macro to enable/disable page 0 Jian J Wang
@ 2017-12-07 5:40 ` Jian J Wang
2017-12-07 7:01 ` Ni, Ruiyu
2 siblings, 1 reply; 8+ messages in thread
From: Jian J Wang @ 2017-12-07 5:40 UTC (permalink / raw)
To: edk2-devel; +Cc: Liming Gao, Michael D Kinney, Ruiyu Ni
> 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
--
2.15.1.windows.2
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/3] IntelFrameworkPkg/LegacyBios.h: Add a macro to guarantee page 0 access
2017-12-07 5:40 ` [PATCH v2 1/3] IntelFrameworkPkg/LegacyBios.h: Add a macro to guarantee page 0 access Jian J Wang
@ 2017-12-07 7:00 ` Ni, Ruiyu
2017-12-07 8:03 ` Wang, Jian J
0 siblings, 1 reply; 8+ messages in thread
From: Ni, Ruiyu @ 2017-12-07 7:00 UTC (permalink / raw)
To: Jian J Wang, edk2-devel; +Cc: Liming Gao, Michael D Kinney
On 12/7/2017 1:40 PM, Jian J Wang wrote:
>> v2:
>> a. Fix a typo in expression in the macro ACCESS_PAGE0_CODE
>> b. Fix GCC49 build error
>
> ue to the introduction of NULL pointer detection feature, page 0 will be
> disabled if the feature is enabled, which will cause legacy code failed to
> update legacy data in page 0. This macro is introduced to make sure the
> page 0 is enabled before those code and restore the original status of it
> afterwards.
>
> Another reason to introduce this macro is to eliminate the dependency on
> the PcdNullPointerDetectionPropertyMask. Because this is a new PCD, it
> could cause some backward compatibility issue for some old packages.
>
> This macro will simply check if the page 0 is disabled or not. If it's
> disabled, it will enable it before code updating page 0 and disable it
> afterwards. Otherwise, this macro will do nothing to page 0.
>
> The usage of the macro will be look like (similar to DEBUG_CODE macro):
>
> ACCESS_PAGE0_CODE(
> <code accessing page 0>
> );
>
> 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>
> ---
> IntelFrameworkPkg/Include/Protocol/LegacyBios.h | 34 +++++++++++++++++++++++++
> 1 file changed, 34 insertions(+)
>
> diff --git a/IntelFrameworkPkg/Include/Protocol/LegacyBios.h b/IntelFrameworkPkg/Include/Protocol/LegacyBios.h
> index 641f101bce..6a5f5464e7 100644
> --- a/IntelFrameworkPkg/Include/Protocol/LegacyBios.h
> +++ b/IntelFrameworkPkg/Include/Protocol/LegacyBios.h
> @@ -1518,6 +1518,40 @@ struct _EFI_LEGACY_BIOS_PROTOCOL {
> EFI_LEGACY_BIOS_BOOT_UNCONVENTIONAL_DEVICE BootUnconventionalDevice;
> };
>
> +//
> +// Legacy BIOS needs to access memory in page 0 (0-4095), which is disabled if
> +// NULL pointer detection feature is enabled. Following macro can be used to
> +// enable/disable page 0 before/after accessing it.
> +//
> +#define ACCESS_PAGE0_CODE(statements) \
> + do { \
> + EFI_STATUS Status_; \
> + EFI_GCD_MEMORY_SPACE_DESCRIPTOR Desc_; \
> + \
> + Desc_.Attributes = 0; \
> + Status_ = gDS->GetMemorySpaceDescriptor (0, &Desc_); \
> + ASSERT_EFI_ERROR (Status_); \
> + if ((Desc_.Attributes & EFI_MEMORY_RP) != 0) { \
> + Status_ = gDS->SetMemorySpaceAttributes ( \
> + 0, \
> + EFI_PAGES_TO_SIZE(1), \
> + Desc_.Attributes & ~(UINT64)EFI_MEMORY_RP \
> + ); \
> + ASSERT_EFI_ERROR (Status_); \
> + } \
> + \
> + statements; \
It's better to surrounded statements with {}.
So that when statements contains variable declaration, C compiler
doesn't complain.
> + \
> + if ((Desc_.Attributes & EFI_MEMORY_RP) != 0) { \
> + Status_ = gDS->SetMemorySpaceAttributes ( \
> + 0, \
> + EFI_PAGES_TO_SIZE(1), \
> + Desc_.Attributes \
> + ); \
> + ASSERT_EFI_ERROR (Status_); \
> + } \
> + } while (FALSE)
> +
> extern EFI_GUID gEfiLegacyBiosProtocolGuid;
>
> #endif
>
With the above suggested changes,
Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>
--
Thanks,
Ray
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/3] IntelFrameworkModulePkg/LegacyBiosDxe: Use macro to enable/disable page 0
2017-12-07 5:40 ` [PATCH v2 2/3] IntelFrameworkModulePkg/LegacyBiosDxe: Use macro to enable/disable page 0 Jian J Wang
@ 2017-12-07 7:01 ` Ni, Ruiyu
0 siblings, 0 replies; 8+ messages in thread
From: Ni, Ruiyu @ 2017-12-07 7:01 UTC (permalink / raw)
To: Jian J Wang, edk2-devel; +Cc: Liming Gao, Michael D Kinney
On 12/7/2017 1:40 PM, Jian J Wang wrote:
>> v2:
>> a. Remove unnecessary braces enclosing code passed to ACCESS_PAGE0_CODE
>
> 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/LegacyBiosDxe/LegacyBda.c | 53 ++++----
> .../Csm/LegacyBiosDxe/LegacyBios.c | 135 ++-------------------
> .../Csm/LegacyBiosDxe/LegacyBiosDxe.inf | 1 -
> .../Csm/LegacyBiosDxe/LegacyBiosInterface.h | 16 ---
> .../Csm/LegacyBiosDxe/LegacyBootSupport.c | 80 ++++++------
> .../Csm/LegacyBiosDxe/LegacyPci.c | 72 ++++++-----
> IntelFrameworkModulePkg/Csm/LegacyBiosDxe/Thunk.c | 51 ++++----
> 7 files changed, 135 insertions(+), 273 deletions(-)
>
> diff --git a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBda.c b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBda.c
> index c6670febee..a94cba435c 100644
> --- a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBda.c
> +++ b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBda.c
> @@ -34,37 +34,36 @@ LegacyBiosInitBda (
> BDA_STRUC *Bda;
> UINT8 *Ebda;
>
> - DisableNullDetection ();
> -
> Bda = (BDA_STRUC *) ((UINTN) 0x400);
> Ebda = (UINT8 *) ((UINTN) 0x9fc00);
>
> - ZeroMem (Bda, 0x100);
> + ACCESS_PAGE0_CODE (
> + ZeroMem (Bda, 0x100);
> + //
> + // 640k-1k for EBDA
> + //
> + Bda->MemSize = 0x27f;
> + Bda->KeyHead = 0x1e;
> + Bda->KeyTail = 0x1e;
> + Bda->FloppyData = 0x00;
> + Bda->FloppyTimeout = 0xff;
> +
> + Bda->KeyStart = 0x001E;
> + Bda->KeyEnd = 0x003E;
> + Bda->KeyboardStatus = 0x10;
> + Bda->Ebda = 0x9fc0;
> +
> + //
> + // Move LPT time out here and zero out LPT4 since some SCSI OPROMS
> + // use this as scratch pad (LPT4 is Reserved)
> + //
> + Bda->Lpt1_2Timeout = 0x1414;
> + Bda->Lpt3_4Timeout = 0x1400;
> +
> + );
> +
> ZeroMem (Ebda, 0x400);
> - //
> - // 640k-1k for EBDA
> - //
> - Bda->MemSize = 0x27f;
> - Bda->KeyHead = 0x1e;
> - Bda->KeyTail = 0x1e;
> - Bda->FloppyData = 0x00;
> - Bda->FloppyTimeout = 0xff;
> -
> - Bda->KeyStart = 0x001E;
> - Bda->KeyEnd = 0x003E;
> - Bda->KeyboardStatus = 0x10;
> - Bda->Ebda = 0x9fc0;
> -
> - //
> - // Move LPT time out here and zero out LPT4 since some SCSI OPROMS
> - // use this as scratch pad (LPT4 is Reserved)
> - //
> - Bda->Lpt1_2Timeout = 0x1414;
> - Bda->Lpt3_4Timeout = 0x1400;
> -
> - *Ebda = 0x01;
> -
> - EnableNullDetection ();
> + *Ebda = 0x01;
>
> return EFI_SUCCESS;
> }
> diff --git a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBios.c b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBios.c
> index c6461f5547..fca08a8fa2 100644
> --- a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBios.c
> +++ b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBios.c
> @@ -786,115 +786,6 @@ ToggleEndOfDxeStatus (
> return;
> }
>
> -//
> -// Legacy BIOS needs to access memory between 0-4095, which will cause page
> -// fault exception if NULL pointer detection mechanism is enabled. Following
> -// functions can be used to disable/enable NULL pointer detection before/after
> -// accessing those memory.
> -//
> -
> -/**
> - Enable NULL pointer detection.
> -**/
> -VOID
> -EnableNullDetection (
> - VOID
> - )
> -{
> - EFI_STATUS Status;
> - EFI_GCD_MEMORY_SPACE_DESCRIPTOR Desc;
> -
> - if (((PcdGet8 (PcdNullPointerDetectionPropertyMask) & BIT0) == 0)
> - ||
> - ((mEndOfDxe) &&
> - ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & (BIT7|BIT0))
> - == (BIT7|BIT0)))
> - ) {
> - 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);
> - }
> -}
> -
> -/**
> - Disable NULL pointer detection.
> -**/
> -VOID
> -DisableNullDetection (
> - VOID
> - )
> -{
> - EFI_STATUS Status;
> - EFI_GCD_MEMORY_SPACE_DESCRIPTOR Desc;
> -
> - if (((PcdGet8 (PcdNullPointerDetectionPropertyMask) & BIT0) == 0)
> - ||
> - ((mEndOfDxe) &&
> - ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & (BIT7|BIT0))
> - == (BIT7|BIT0)))
> - ) {
> - 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"));
> - }
> -}
> -
> /**
> Install Driver to produce Legacy BIOS protocol.
>
> @@ -1095,10 +986,10 @@ LegacyBiosInstall (
> // Initialize region from 0x0000 to 4k. This initializes interrupt vector
> // range.
> //
> - DisableNullDetection ();
> - gBS->SetMem ((VOID *) ClearPtr, 0x400, INITIAL_VALUE_BELOW_1K);
> - ZeroMem ((VOID *) ((UINTN)ClearPtr + 0x400), 0xC00);
> - EnableNullDetection ();
> + ACCESS_PAGE0_CODE (
> + gBS->SetMem ((VOID *) ClearPtr, 0x400, INITIAL_VALUE_BELOW_1K);
> + ZeroMem ((VOID *) ((UINTN)ClearPtr + 0x400), 0xC00);
> + );
>
> //
> // Allocate pages for OPROM usage
> @@ -1237,16 +1128,14 @@ LegacyBiosInstall (
> //
> // Save Unexpected interrupt vector so can restore it just prior to boot
> //
> - DisableNullDetection ();
> -
> - BaseVectorMaster = (UINT32 *) (sizeof (UINT32) * PROTECTED_MODE_BASE_VECTOR_MASTER);
> - Private->BiosUnexpectedInt = BaseVectorMaster[0];
> - IntRedirCode = (UINT32) (UINTN) Private->IntThunk->InterruptRedirectionCode;
> - for (Index = 0; Index < 8; Index++) {
> - BaseVectorMaster[Index] = (EFI_SEGMENT (IntRedirCode + Index * 4) << 16) | EFI_OFFSET (IntRedirCode + Index * 4);
> - }
> -
> - EnableNullDetection ();
> + ACCESS_PAGE0_CODE (
> + BaseVectorMaster = (UINT32 *) (sizeof (UINT32) * PROTECTED_MODE_BASE_VECTOR_MASTER);
> + Private->BiosUnexpectedInt = BaseVectorMaster[0];
> + IntRedirCode = (UINT32) (UINTN) Private->IntThunk->InterruptRedirectionCode;
> + for (Index = 0; Index < 8; Index++) {
> + BaseVectorMaster[Index] = (EFI_SEGMENT (IntRedirCode + Index * 4) << 16) | EFI_OFFSET (IntRedirCode + Index * 4);
> + }
> + );
>
> //
> // Save EFI value
> diff --git a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBiosDxe.inf b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBiosDxe.inf
> index 6efc7f36ae..180c18e3fc 100644
> --- a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBiosDxe.inf
> +++ b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBiosDxe.inf
> @@ -148,7 +148,6 @@
> gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdHighPmmMemorySize ## CONSUMES
> gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdOpromReservedMemoryBase ## CONSUMES
> gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdOpromReservedMemorySize ## CONSUMES
> - gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask ## CONSUMES
>
> [Depex]
> gEfiLegacyRegion2ProtocolGuid AND gEfiLegacyInterruptProtocolGuid AND gEfiLegacyBiosPlatformProtocolGuid AND gEfiLegacy8259ProtocolGuid AND gEfiGenericMemTestProtocolGuid AND gEfiCpuArchProtocolGuid AND gEfiTimerArchProtocolGuid AND gEfiVariableWriteArchProtocolGuid
> diff --git a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBiosInterface.h b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBiosInterface.h
> index 86a3b09080..cc2fc9fc13 100644
> --- a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBiosInterface.h
> +++ b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBiosInterface.h
> @@ -1544,20 +1544,4 @@ LegacyBiosInstallVgaRom (
> IN LEGACY_BIOS_INSTANCE *Private
> );
>
> -/**
> - Enable NULL pointer detection.
> -**/
> -VOID
> -EnableNullDetection (
> - VOID
> - );
> -
> -/**
> - Disable NULL pointer detection.
> -**/
> -VOID
> -DisableNullDetection (
> - VOID
> - );
> -
> #endif
> diff --git a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBootSupport.c b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBootSupport.c
> index c2ac69ce69..57ab78d648 100644
> --- a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBootSupport.c
> +++ b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyBootSupport.c
> @@ -1041,7 +1041,9 @@ GenericLegacyBoot (
> //
> // Setup BDA and EBDA standard areas before Legacy Boot
> //
> - LegacyBiosCompleteBdaBeforeBoot (Private);
> + ACCESS_PAGE0_CODE (
> + LegacyBiosCompleteBdaBeforeBoot (Private);
> + );
> LegacyBiosCompleteStandardCmosBeforeBoot (Private);
>
> //
> @@ -1073,10 +1075,10 @@ GenericLegacyBoot (
> // Use 182/10 to avoid floating point math.
> //
> LocalTime = (LocalTime * 182) / 10;
> - DisableNullDetection ();
> - BdaPtr = (UINT32 *) (UINTN)0x46C;
> - *BdaPtr = LocalTime;
> - EnableNullDetection ();
> + ACCESS_PAGE0_CODE (
> + BdaPtr = (UINT32 *) (UINTN)0x46C;
> + *BdaPtr = LocalTime;
> + );
>
> //
> // Shadow PCI ROMs. We must do this near the end since this will kick
> @@ -1322,15 +1324,15 @@ GenericLegacyBoot (
> // set of TIANO vectors) or takes it over.
> //
> //
> - DisableNullDetection ();
> - BaseVectorMaster = (UINT32 *) (sizeof (UINT32) * PROTECTED_MODE_BASE_VECTOR_MASTER);
> - for (Index = 0; Index < 8; Index++) {
> - Private->ThunkSavedInt[Index] = BaseVectorMaster[Index];
> - if (Private->ThunkSeg == (UINT16) (BaseVectorMaster[Index] >> 16)) {
> - BaseVectorMaster[Index] = (UINT32) (Private->BiosUnexpectedInt);
> + ACCESS_PAGE0_CODE (
> + BaseVectorMaster = (UINT32 *) (sizeof (UINT32) * PROTECTED_MODE_BASE_VECTOR_MASTER);
> + for (Index = 0; Index < 8; Index++) {
> + Private->ThunkSavedInt[Index] = BaseVectorMaster[Index];
> + if (Private->ThunkSeg == (UINT16) (BaseVectorMaster[Index] >> 16)) {
> + BaseVectorMaster[Index] = (UINT32) (Private->BiosUnexpectedInt);
> + }
> }
> - }
> - EnableNullDetection ();
> + );
>
> ZeroMem (&Regs, sizeof (EFI_IA32_REGISTER_SET));
> Regs.X.AX = Legacy16Boot;
> @@ -1344,12 +1346,12 @@ GenericLegacyBoot (
> 0
> );
>
> - DisableNullDetection ();
> - BaseVectorMaster = (UINT32 *) (sizeof (UINT32) * PROTECTED_MODE_BASE_VECTOR_MASTER);
> - for (Index = 0; Index < 8; Index++) {
> - BaseVectorMaster[Index] = Private->ThunkSavedInt[Index];
> - }
> - EnableNullDetection ();
> + ACCESS_PAGE0_CODE (
> + BaseVectorMaster = (UINT32 *) (sizeof (UINT32) * PROTECTED_MODE_BASE_VECTOR_MASTER);
> + for (Index = 0; Index < 8; Index++) {
> + BaseVectorMaster[Index] = Private->ThunkSavedInt[Index];
> + }
> + );
> }
> Private->LegacyBootEntered = TRUE;
> if ((mBootMode == BOOT_LEGACY_OS) || (mBootMode == BOOT_UNCONVENTIONAL_DEVICE)) {
> @@ -1737,11 +1739,11 @@ LegacyBiosBuildE820 (
> //
> // First entry is 0 to (640k - EBDA)
> //
> - DisableNullDetection ();
> - E820Table[0].BaseAddr = 0;
> - E820Table[0].Length = (UINT64) ((*(UINT16 *) (UINTN)0x40E) << 4);
> - E820Table[0].Type = EfiAcpiAddressRangeMemory;
> - EnableNullDetection ();
> + ACCESS_PAGE0_CODE (
> + E820Table[0].BaseAddr = 0;
> + E820Table[0].Length = (UINT64) ((*(UINT16 *) (UINTN)0x40E) << 4);
> + E820Table[0].Type = EfiAcpiAddressRangeMemory;
> + );
>
> //
> // Second entry is (640k - EBDA) to 640k
> @@ -1975,8 +1977,6 @@ LegacyBiosCompleteBdaBeforeBoot (
> UINT16 MachineConfig;
> DEVICE_PRODUCER_DATA_HEADER *SioPtr;
>
> - DisableNullDetection ();
> -
> Bda = (BDA_STRUC *) ((UINTN) 0x400);
> MachineConfig = 0;
>
> @@ -2035,8 +2035,6 @@ LegacyBiosCompleteBdaBeforeBoot (
> MachineConfig = (UINT16) (MachineConfig + 0x00 + 0x02 + (SioPtr->MousePresent * 0x04));
> Bda->MachineConfig = MachineConfig;
>
> - EnableNullDetection ();
> -
> return EFI_SUCCESS;
> }
>
> @@ -2063,17 +2061,15 @@ LegacyBiosUpdateKeyboardLedStatus (
>
> Private = LEGACY_BIOS_INSTANCE_FROM_THIS (This);
>
> - DisableNullDetection ();
> -
> - Bda = (BDA_STRUC *) ((UINTN) 0x400);
> - LocalLeds = Leds;
> - Bda->LedStatus = (UINT8) ((Bda->LedStatus &~0x07) | LocalLeds);
> - LocalLeds = (UINT8) (LocalLeds << 4);
> - Bda->ShiftStatus = (UINT8) ((Bda->ShiftStatus &~0x70) | LocalLeds);
> - LocalLeds = (UINT8) (Leds & 0x20);
> - Bda->KeyboardStatus = (UINT8) ((Bda->KeyboardStatus &~0x20) | LocalLeds);
> -
> - EnableNullDetection ();
> + ACCESS_PAGE0_CODE (
> + Bda = (BDA_STRUC *) ((UINTN) 0x400);
> + LocalLeds = Leds;
> + Bda->LedStatus = (UINT8) ((Bda->LedStatus &~0x07) | LocalLeds);
> + LocalLeds = (UINT8) (LocalLeds << 4);
> + Bda->ShiftStatus = (UINT8) ((Bda->ShiftStatus &~0x70) | LocalLeds);
> + LocalLeds = (UINT8) (Leds & 0x20);
> + Bda->KeyboardStatus = (UINT8) ((Bda->KeyboardStatus &~0x20) | LocalLeds);
> + );
>
> //
> // Call into Legacy16 code to allow it to do any processing
> @@ -2119,9 +2115,9 @@ LegacyBiosCompleteStandardCmosBeforeBoot (
> // to large capacity drives
> // CMOS 14 = BDA 40:10 plus bit 3(display enabled)
> //
> - DisableNullDetection ();
> - Bda = (UINT8)(*((UINT8 *)((UINTN)0x410)) | BIT3);
> - EnableNullDetection ();
> + ACCESS_PAGE0_CODE (
> + Bda = (UINT8)(*((UINT8 *)((UINTN)0x410)) | BIT3);
> + );
>
> //
> // Force display enabled
> diff --git a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyPci.c b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyPci.c
> index d38cef3e33..b10a9dcef6 100644
> --- a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyPci.c
> +++ b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/LegacyPci.c
> @@ -2403,35 +2403,33 @@ LegacyBiosInstallRom (
> // 2. BBS compliants drives will not change 40:75 until boot time.
> // 3. Onboard IDE controllers will change 40:75
> //
> - DisableNullDetection ();
> -
> - LocalDiskStart = (UINT8) ((*(UINT8 *) ((UINTN) 0x475)) + 0x80);
> - if ((Private->Disk4075 + 0x80) < LocalDiskStart) {
> - //
> - // Update table since onboard IDE drives found
> - //
> - Private->LegacyEfiHddTable[Private->LegacyEfiHddTableIndex].PciSegment = 0xff;
> - Private->LegacyEfiHddTable[Private->LegacyEfiHddTableIndex].PciBus = 0xff;
> - Private->LegacyEfiHddTable[Private->LegacyEfiHddTableIndex].PciDevice = 0xff;
> - Private->LegacyEfiHddTable[Private->LegacyEfiHddTableIndex].PciFunction = 0xff;
> - Private->LegacyEfiHddTable[Private->LegacyEfiHddTableIndex].StartDriveNumber = (UINT8) (Private->Disk4075 + 0x80);
> - Private->LegacyEfiHddTable[Private->LegacyEfiHddTableIndex].EndDriveNumber = LocalDiskStart;
> - Private->LegacyEfiHddTableIndex ++;
> - Private->Disk4075 = (UINT8) (LocalDiskStart & 0x7f);
> - Private->DiskEnd = LocalDiskStart;
> - }
> -
> - if (PciHandle != mVgaHandle) {
> + ACCESS_PAGE0_CODE (
> + LocalDiskStart = (UINT8) ((*(UINT8 *) ((UINTN) 0x475)) + 0x80);
> + if ((Private->Disk4075 + 0x80) < LocalDiskStart) {
> + //
> + // Update table since onboard IDE drives found
> + //
> + Private->LegacyEfiHddTable[Private->LegacyEfiHddTableIndex].PciSegment = 0xff;
> + Private->LegacyEfiHddTable[Private->LegacyEfiHddTableIndex].PciBus = 0xff;
> + Private->LegacyEfiHddTable[Private->LegacyEfiHddTableIndex].PciDevice = 0xff;
> + Private->LegacyEfiHddTable[Private->LegacyEfiHddTableIndex].PciFunction = 0xff;
> + Private->LegacyEfiHddTable[Private->LegacyEfiHddTableIndex].StartDriveNumber = (UINT8) (Private->Disk4075 + 0x80);
> + Private->LegacyEfiHddTable[Private->LegacyEfiHddTableIndex].EndDriveNumber = LocalDiskStart;
> + Private->LegacyEfiHddTableIndex ++;
> + Private->Disk4075 = (UINT8) (LocalDiskStart & 0x7f);
> + Private->DiskEnd = LocalDiskStart;
> + }
>
> - EnablePs2Keyboard ();
> + if (PciHandle != mVgaHandle) {
>
> - //
> - // Store current mode settings since PrepareToScanRom may change mode.
> - //
> - VideoMode = *(UINT8 *) ((UINTN) (0x400 + BDA_VIDEO_MODE));
> - }
> + EnablePs2Keyboard ();
>
> - EnableNullDetection ();
> + //
> + // Store current mode settings since PrepareToScanRom may change mode.
> + //
> + VideoMode = *(UINT8 *) ((UINTN) (0x400 + BDA_VIDEO_MODE));
> + }
> + );
>
> //
> // Notify the platform that we are about to scan the ROM
> @@ -2473,11 +2471,11 @@ LegacyBiosInstallRom (
> // Multiply result by 18.2 for number of ticks since midnight.
> // Use 182/10 to avoid floating point math.
> //
> - DisableNullDetection ();
> - LocalTime = (LocalTime * 182) / 10;
> - BdaPtr = (UINT32 *) ((UINTN) 0x46C);
> - *BdaPtr = LocalTime;
> - EnableNullDetection ();
> + ACCESS_PAGE0_CODE (
> + LocalTime = (LocalTime * 182) / 10;
> + BdaPtr = (UINT32 *) ((UINTN) 0x46C);
> + *BdaPtr = LocalTime;
> + );
>
> //
> // Pass in handoff data
> @@ -2573,9 +2571,9 @@ LegacyBiosInstallRom (
> //
> // Set mode settings since PrepareToScanRom may change mode
> //
> - DisableNullDetection ();
> - OldVideoMode = *(UINT8 *) ((UINTN) (0x400 + BDA_VIDEO_MODE));
> - EnableNullDetection ();
> + ACCESS_PAGE0_CODE ({
> + OldVideoMode = *(UINT8 *) ((UINTN) (0x400 + BDA_VIDEO_MODE));
> + });
>
> if (VideoMode != OldVideoMode) {
> //
> @@ -2617,9 +2615,9 @@ LegacyBiosInstallRom (
> }
> }
>
> - DisableNullDetection ();
> - LocalDiskEnd = (UINT8) ((*(UINT8 *) ((UINTN) 0x475)) + 0x80);
> - EnableNullDetection ();
> + ACCESS_PAGE0_CODE (
> + LocalDiskEnd = (UINT8) ((*(UINT8 *) ((UINTN) 0x475)) + 0x80);
> + );
>
> //
> // Allow platform to perform any required actions after the
> diff --git a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/Thunk.c b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/Thunk.c
> index d249479c56..d330f4870b 100644
> --- a/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/Thunk.c
> +++ b/IntelFrameworkModulePkg/Csm/LegacyBiosDxe/Thunk.c
> @@ -73,10 +73,10 @@ LegacyBiosInt86 (
> // The base address of legacy interrupt vector table is 0.
> // We use this base address to get the legacy interrupt handler.
> //
> - DisableNullDetection ();
> - Segment = (UINT16)(((UINT32 *)0)[BiosInt] >> 16);
> - Offset = (UINT16)((UINT32 *)0)[BiosInt];
> - EnableNullDetection ();
> + ACCESS_PAGE0_CODE (
> + Segment = (UINT16)(((UINT32 *)0)[BiosInt] >> 16);
> + Offset = (UINT16)((UINT32 *)0)[BiosInt];
> + );
>
> return InternalLegacyBiosFarCall (
> This,
> @@ -286,29 +286,6 @@ InternalLegacyBiosFarCall (
>
> AsmThunk16 (&mThunkContext);
>
> - //
> - // OPROM may allocate EBDA range by itself and change EBDA base and EBDA size.
> - // Get the current EBDA base address, and compared with pre-allocate minimum
> - // EBDA base address, if the current EBDA base address is smaller, it indicates
> - // PcdEbdaReservedMemorySize should be adjusted to larger for more OPROMs.
> - //
> - DEBUG_CODE (
> - {
> - UINTN EbdaBaseAddress;
> - UINTN ReservedEbdaBaseAddress;
> -
> - //
> - // Skip this part of debug code if NULL pointer detection is enabled
> - //
> - if ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & BIT0) == 0) {
> - EbdaBaseAddress = (*(UINT16 *) (UINTN) 0x40E) << 4;
> - ReservedEbdaBaseAddress = CONVENTIONAL_MEMORY_TOP
> - - PcdGet32 (PcdEbdaReservedMemorySize);
> - ASSERT (ReservedEbdaBaseAddress <= EbdaBaseAddress);
> - }
> - }
> - );
> -
> if (Stack != NULL && StackSize != 0) {
> //
> // Copy low memory stack to Stack
> @@ -334,6 +311,26 @@ InternalLegacyBiosFarCall (
> //
> gBS->RestoreTPL (OriginalTpl);
>
> + //
> + // OPROM may allocate EBDA range by itself and change EBDA base and EBDA size.
> + // Get the current EBDA base address, and compared with pre-allocate minimum
> + // EBDA base address, if the current EBDA base address is smaller, it indicates
> + // PcdEbdaReservedMemorySize should be adjusted to larger for more OPROMs.
> + //
> + DEBUG_CODE (
> + {
> + UINTN EbdaBaseAddress;
> + UINTN ReservedEbdaBaseAddress;
> +
> + ACCESS_PAGE0_CODE (
> + EbdaBaseAddress = (*(UINT16 *) (UINTN) 0x40E) << 4;
> + ReservedEbdaBaseAddress = CONVENTIONAL_MEMORY_TOP
> + - PcdGet32 (PcdEbdaReservedMemorySize);
> + ASSERT (ReservedEbdaBaseAddress <= EbdaBaseAddress);
> + );
> + }
> + );
> +
> //
> // Restore interrupt of debug timer
> //
>
Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>
--
Thanks,
Ray
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 3/3] IntelFrameworkModulePkg/KeyboardDxe: Use macro to enable/disable page 0
2017-12-07 5:40 ` [PATCH v2 3/3] IntelFrameworkModulePkg/KeyboardDxe: " Jian J Wang
@ 2017-12-07 7:01 ` Ni, Ruiyu
0 siblings, 0 replies; 8+ messages in thread
From: Ni, Ruiyu @ 2017-12-07 7:01 UTC (permalink / raw)
To: Jian J Wang, edk2-devel; +Cc: Liming Gao, Michael D Kinney
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/3] IntelFrameworkPkg/LegacyBios.h: Add a macro to guarantee page 0 access
2017-12-07 7:00 ` Ni, Ruiyu
@ 2017-12-07 8:03 ` Wang, Jian J
0 siblings, 0 replies; 8+ messages in thread
From: Wang, Jian J @ 2017-12-07 8:03 UTC (permalink / raw)
To: Ni, Ruiyu, edk2-devel@lists.01.org; +Cc: Gao, Liming, Kinney, Michael D
Thanks for the comment. The {} will be added.
Sincerely,
Jian
> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Thursday, December 07, 2017 3:00 PM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Gao, Liming <liming.gao@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Subject: Re: [PATCH v2 1/3] IntelFrameworkPkg/LegacyBios.h: Add a macro to
> guarantee page 0 access
>
> On 12/7/2017 1:40 PM, Jian J Wang wrote:
> >> v2:
> >> a. Fix a typo in expression in the macro ACCESS_PAGE0_CODE
> >> b. Fix GCC49 build error
> >
> > ue to the introduction of NULL pointer detection feature, page 0 will be
> > disabled if the feature is enabled, which will cause legacy code failed to
> > update legacy data in page 0. This macro is introduced to make sure the
> > page 0 is enabled before those code and restore the original status of it
> > afterwards.
> >
> > Another reason to introduce this macro is to eliminate the dependency on
> > the PcdNullPointerDetectionPropertyMask. Because this is a new PCD, it
> > could cause some backward compatibility issue for some old packages.
> >
> > This macro will simply check if the page 0 is disabled or not. If it's
> > disabled, it will enable it before code updating page 0 and disable it
> > afterwards. Otherwise, this macro will do nothing to page 0.
> >
> > The usage of the macro will be look like (similar to DEBUG_CODE macro):
> >
> > ACCESS_PAGE0_CODE(
> > <code accessing page 0>
> > );
> >
> > 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>
> > ---
> > IntelFrameworkPkg/Include/Protocol/LegacyBios.h | 34
> +++++++++++++++++++++++++
> > 1 file changed, 34 insertions(+)
> >
> > diff --git a/IntelFrameworkPkg/Include/Protocol/LegacyBios.h
> b/IntelFrameworkPkg/Include/Protocol/LegacyBios.h
> > index 641f101bce..6a5f5464e7 100644
> > --- a/IntelFrameworkPkg/Include/Protocol/LegacyBios.h
> > +++ b/IntelFrameworkPkg/Include/Protocol/LegacyBios.h
> > @@ -1518,6 +1518,40 @@ struct _EFI_LEGACY_BIOS_PROTOCOL {
> > EFI_LEGACY_BIOS_BOOT_UNCONVENTIONAL_DEVICE
> BootUnconventionalDevice;
> > };
> >
> > +//
> > +// Legacy BIOS needs to access memory in page 0 (0-4095), which is disabled
> if
> > +// NULL pointer detection feature is enabled. Following macro can be used to
> > +// enable/disable page 0 before/after accessing it.
> > +//
> > +#define ACCESS_PAGE0_CODE(statements) \
> > + do { \
> > + EFI_STATUS Status_; \
> > + EFI_GCD_MEMORY_SPACE_DESCRIPTOR Desc_; \
> > + \
> > + Desc_.Attributes = 0; \
> > + Status_ = gDS->GetMemorySpaceDescriptor (0, &Desc_); \
> > + ASSERT_EFI_ERROR (Status_); \
> > + if ((Desc_.Attributes & EFI_MEMORY_RP) != 0) { \
> > + Status_ = gDS->SetMemorySpaceAttributes ( \
> > + 0, \
> > + EFI_PAGES_TO_SIZE(1), \
> > + Desc_.Attributes & ~(UINT64)EFI_MEMORY_RP \
> > + ); \
> > + ASSERT_EFI_ERROR (Status_); \
> > + } \
> > + \
> > + statements; \
>
> It's better to surrounded statements with {}.
> So that when statements contains variable declaration, C compiler
> doesn't complain.
>
> > + \
> > + if ((Desc_.Attributes & EFI_MEMORY_RP) != 0) { \
> > + Status_ = gDS->SetMemorySpaceAttributes ( \
> > + 0, \
> > + EFI_PAGES_TO_SIZE(1), \
> > + Desc_.Attributes \
> > + ); \
> > + ASSERT_EFI_ERROR (Status_); \
> > + } \
> > + } while (FALSE)
> > +
> > extern EFI_GUID gEfiLegacyBiosProtocolGuid;
> >
> > #endif
> >
>
> With the above suggested changes,
> Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>
>
> --
> Thanks,
> Ray
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-12-07 7:58 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-12-07 5:40 [PATCH v2 0/3] Remove dependency on PcdNullPointerDetectionPropertyMask Jian J Wang
2017-12-07 5:40 ` [PATCH v2 1/3] IntelFrameworkPkg/LegacyBios.h: Add a macro to guarantee page 0 access Jian J Wang
2017-12-07 7:00 ` Ni, Ruiyu
2017-12-07 8:03 ` Wang, Jian J
2017-12-07 5:40 ` [PATCH v2 2/3] IntelFrameworkModulePkg/LegacyBiosDxe: Use macro to enable/disable page 0 Jian J Wang
2017-12-07 7:01 ` Ni, Ruiyu
2017-12-07 5:40 ` [PATCH v2 3/3] IntelFrameworkModulePkg/KeyboardDxe: " Jian J Wang
2017-12-07 7:01 ` Ni, Ruiyu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox