[edk2] [PATCH v3 6/6] MdeModulePkg/Core: add freed-memory guard feature
> v3 changes:
> a. Merge from #4 and #5 of v2 patch
> b. Coding style cleanup
Freed-memory guard is used to detect UAF (Use-After-Free) memory issue
which is illegal access to memory which has been freed. The principle
behind is similar to heap guard feature, that is we'll turn all pool
memory allocation to page allocation and mark them to be not-present
once they are freed. The freed memory block will not be added back into
memory pool.
This means that, once a page is allocated and freed, it cannot be
re-allocated. This will bring an issue, which is that there's
risk that memory space will be used out. To address it, the memory
service add logic to put part (at most 64 pages a time) of freed pages
back into page pool, so that the memory service can still have memory
to allocate, when all memory space have been allocated once. This is
called memory promotion. The promoted pages are always from the eldest
pages which haven been freed.
This feature brings another problem is that memory map descriptors will
be increased enormously (200+ -> 2000+). One of change in this patch
is to update MergeMemoryMap() in file PropertiesTable.c to allow merge
freed pages back into the memory map. Now the number can stay at around
510.
Cc: Star Zeng
Cc: Michael D Kinney
Cc: Jiewen Yao
Cc: Ruiyu Ni
Cc: Laszlo Ersek
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang
---
MdeModulePkg/Core/Dxe/Mem/HeapGuard.c | 409 +-
MdeModulePkg/Core/Dxe/Mem/HeapGuard.h | 65 +++-
MdeModulePkg/Core/Dxe/Mem/Page.c | 41 ++-
MdeModulePkg/Core/Dxe/Mem/Pool.c | 23 +-
MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 2 +-
MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c | 18 +-
6 files changed, 524 insertions(+), 34 deletions(-)
diff --git a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
index 663f969c0d..449a022658 100644
--- a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
+++ b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
@@ -44,6 +44,11 @@ GLOBAL_REMOVE_IF_UNREFERENCED UINTN
mLevelShift[GUARDED_HEAP_MAP_TABLE_DEPTH]
GLOBAL_REMOVE_IF_UNREFERENCED UINTN mLevelMask[GUARDED_HEAP_MAP_TABLE_DEPTH]
= GUARDED_HEAP_MAP_TABLE_DEPTH_MASKS;
+//
+// Used for promoting freed but not used pages.
+//
+GLOBAL_REMOVE_IF_UNREFERENCED EFI_PHYSICAL_ADDRESS mLastPromotedPage =
BASE_4GB;
+
/**
Set corresponding bits in bitmap table to 1 according to the address.
@@ -379,7 +384,7 @@ ClearGuardedMemoryBits (
@return An integer containing the guarded memory bitmap.
**/
-UINTN
+UINT64
GetGuardedMemoryBits (
IN EFI_PHYSICAL_ADDRESSAddress,
IN UINTN NumberOfPages
@@ -387,7 +392,7 @@ GetGuardedMemoryBits (
{
UINT64*BitMap;
UINTN Bits;
- UINTN Result;
+ UINT64Result;
UINTN Shift;
UINTN BitsToUnitEnd;
@@ -660,15 +665,16 @@ IsPageTypeToGuard (
/**
Check to see if the heap guard is enabled for page and/or pool allocation.
+ @param[in] GuardType Specify the sub-type(s) of Heap Guard.
+
@return TRUE/FALSE.
**/
BOOLEAN
IsHeapGuardEnabled (
- VOID
+ UINT8 GuardType
)
{
- return IsMemoryTypeToGuard (EfiMaxMemoryType, AllocateAnyPages,
- GUARD_HEAP_TYPE_POOL|GUARD_HEAP_TYPE_PAGE);
+ return IsMemoryTypeToGuard (EfiMaxMemoryType, AllocateAnyPages, GuardType);
}
/**
@@ -1203,6 +1209,380 @@ SetAllGuardPages (
}
}
+/**
+ Find the address of top-most guarded free page.
+
+ @param[out] AddressStart address of top-most guarded free page.
+
+ @return VOID.
+**/
+VOID
+GetLastGuardedFreePageAddress (
+ OUT EFI_PHYSICAL_ADDRESS *Address
+ )
+{
+ EFI_PHYSICAL_ADDRESSAddressGranularity;
+ EFI_PHYSICAL_ADDRESSBaseAddress;
+ UINTN Level;
+ UINT64 Map;
+ INTNIndex;
+
+ ASSERT (mMapLevel >= 1);
+
+ BaseAddress = 0;
+ Map = mGuardedMemoryMap;
+ for (Level = GUARDED_HEAP_MAP_TABLE_DEPTH - mMapLevel;
+ Level < GUARDED_HEAP_MAP_TABLE_DEPTH;
+ ++Level) {
+AddressGranularity = LShiftU64 (1, mLevelShift[Level]);
+
+//
+// Find the non-NULL entry at largest index.
+//
+for (Index = (INTN)mLevelMask[Level]; Index >= 0 ; --Index) {
+ if (((UINT64 *)(UINTN)Map)[Index] != 0) {
+BaseAddress += MultU64x32 (AddressGranularity, (UINT32)Index);
+Map = ((UINT64 *)(UINTN)Map)[Index];
+break;
+ }
+}
+ }
+
+ //
+ // Find the non-zero MSB then get the page address.
+ //
+ while (Map != 0) {
+Map = RShiftU64 (Map, 1);
+BaseAddress += EFI_PAGES_TO_SIZE (1);
+ }
+
+ *Address = BaseAddress;
+}
+
+/**
+ Record freed pages.
+
+ @param[in] BaseAddress Base address of just freed pages.
+ @param[in] Pages Number of freed pages.
+
+ @return VOID.
+**/
+VOID
+MarkFreedPages (
+
[edk2] [PATCH v3 2/6] MdeModulePkg: introduce UEFI freed-memory guard bit in HeapGuard PCD
> v3 changes: > a. split from v2 #1 patch file. > b. refine the commit message and title. UAF (Use-After-Free) memory issue is kind of illegal access to memory which has been freed. It can be detected by a new freed-memory guard enforced onto freed memory. BIT4 of following PCD is used to enable the freed-memory guard feature. gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask Please note this feature is for debug purpose and should not be enabled in product BIOS, and cannot be enabled with pool/page heap guard at the same time. It's disabled by default. Cc: Star Zeng Cc: Michael D Kinney Cc: Jiewen Yao Cc: Ruiyu Ni Cc: Laszlo Ersek Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Jian J Wang --- MdeModulePkg/MdeModulePkg.dec | 6 ++ MdeModulePkg/MdeModulePkg.uni | 4 +++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec index 2009dbc5fd..255b92ea67 100644 --- a/MdeModulePkg/MdeModulePkg.dec +++ b/MdeModulePkg/MdeModulePkg.dec @@ -1011,14 +1011,20 @@ gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPoolType|0x0|UINT64|0x30001053 ## This mask is to control Heap Guard behavior. + # # Note that due to the limit of pool memory implementation and the alignment # requirement of UEFI spec, BIT7 is a try-best setting which cannot guarantee # that the returned pool is exactly adjacent to head guard page or tail guard # page. + # + # Note that UEFI freed-memory guard and pool/page guard cannot be enabled + # at the same time. + # # BIT0 - Enable UEFI page guard. # BIT1 - Enable UEFI pool guard. # BIT2 - Enable SMM page guard. # BIT3 - Enable SMM pool guard. + # BIT4 - Enable UEFI freed-memory guard (Use-After-Free memory detection). # BIT6 - Enable non-stop mode. # BIT7 - The direction of Guard Page for Pool Guard. # 0 - The returned pool is near the tail guard page. diff --git a/MdeModulePkg/MdeModulePkg.uni b/MdeModulePkg/MdeModulePkg.uni index 9d2e473fa9..e72b893509 100644 --- a/MdeModulePkg/MdeModulePkg.uni +++ b/MdeModulePkg/MdeModulePkg.uni @@ -1227,11 +1227,13 @@ "Note that due to the limit of pool memory implementation and the alignment\n" "requirement of UEFI spec, BIT7 is a try-best setting which cannot guarantee\n" "that the returned pool is exactly adjacent to head guard page or tail guard\n" - "page.\n" + "page.\n\n" + "Note that UEFI freed-memory guard and pool/page guard cannot be enabled at the same time.\n\n" " BIT0 - Enable UEFI page guard.\n" " BIT1 - Enable UEFI pool guard.\n" " BIT2 - Enable SMM page guard.\n" " BIT3 - Enable SMM pool guard.\n" + " BIT4 - Enable UEFI freed-memory guard (Use-After-Free memory detection).\n" " BIT7 - The direction of Guard Page for Pool Guard.\n" " 0 - The returned pool is near the tail guard page.\n" " 1 - The returned pool is near the head guard page." -- 2.16.2.windows.1 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH v3 5/6] MdeModulePkg/Core: prevent re-acquire GCD memory lock
> v3 changes:
> a. drop the changes to CoreGetIoSpaceMap() because it won't cause
>problem
> b. refine the logic in changes of CoreGetMemorySpaceMap()
>and add more comments
This issue is hidden in current code but exposed by introduction
of freed-memory guard feature due to the fact that the feature
will turn all pool allocation to page allocation.
The solution is move the memory allocation in CoreGetMemorySpaceMap()
and CoreGetIoSpaceMap() to be out of the GCD memory map lock.
Although it's rare, a while-loop is added to make sure that the count
of descriptor of memory map is the same after memory allocation, because
it's executed outside the GCD memory lock.
CoreDumpGcdMemorySpaceMap()
=> CoreGetMemorySpaceMap()
=> CoreAcquireGcdMemoryLock () *
AllocatePool()
=> InternalAllocatePool()
=> CoreAllocatePool()
=> CoreAllocatePoolI()
=> CoreAllocatePoolPagesI()
=> CoreAllocatePoolPages()
=> FindFreePages()
=> PromoteMemoryResource()
=> CoreAcquireGcdMemoryLock() **
Cc: Star Zeng
Cc: Michael D Kinney
Cc: Jiewen Yao
Cc: Ruiyu Ni
Cc: Laszlo Ersek
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang
---
MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 80 +++--
1 file changed, 54 insertions(+), 26 deletions(-)
diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
index d9c65a8045..bc02945721 100644
--- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
+++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
@@ -1691,10 +1691,10 @@ CoreGetMemorySpaceMap (
OUT EFI_GCD_MEMORY_SPACE_DESCRIPTOR **MemorySpaceMap
)
{
- EFI_STATUS Status;
LIST_ENTRY *Link;
EFI_GCD_MAP_ENTRY*Entry;
EFI_GCD_MEMORY_SPACE_DESCRIPTOR *Descriptor;
+ UINTNDescriptorCount;
//
// Make sure parameters are valid
@@ -1706,38 +1706,66 @@ CoreGetMemorySpaceMap (
return EFI_INVALID_PARAMETER;
}
+ *NumberOfDescriptors = 0;
+ *MemorySpaceMap = NULL;
+
CoreAcquireGcdMemoryLock ();
- //
- // Count the number of descriptors
- //
- *NumberOfDescriptors = CoreCountGcdMapEntry ();
+ while (TRUE) {
+//
+// Count the number of descriptors. It might be done more than once because
+// there's code which has to be running outside the GCD lock.
+//
+DescriptorCount = CoreCountGcdMapEntry ();
+if (DescriptorCount == *NumberOfDescriptors) {
+ //
+ // Fill in the MemorySpaceMap if no memory space map change.
+ //
+ Descriptor = *MemorySpaceMap;
+ Link = mGcdMemorySpaceMap.ForwardLink;
+ while (Link != ) {
+Entry = CR (Link, EFI_GCD_MAP_ENTRY, Link, EFI_GCD_MAP_SIGNATURE);
+BuildMemoryDescriptor (Descriptor, Entry);
+Descriptor++;
+Link = Link->ForwardLink;
+ }
- //
- // Allocate the MemorySpaceMap
- //
- *MemorySpaceMap = AllocatePool (*NumberOfDescriptors * sizeof
(EFI_GCD_MEMORY_SPACE_DESCRIPTOR));
- if (*MemorySpaceMap == NULL) {
-Status = EFI_OUT_OF_RESOURCES;
-goto Done;
- }
+ break;
+}
- //
- // Fill in the MemorySpaceMap
- //
- Descriptor = *MemorySpaceMap;
- Link = mGcdMemorySpaceMap.ForwardLink;
- while (Link != ) {
-Entry = CR (Link, EFI_GCD_MAP_ENTRY, Link, EFI_GCD_MAP_SIGNATURE);
-BuildMemoryDescriptor (Descriptor, Entry);
-Descriptor++;
-Link = Link->ForwardLink;
+//
+// Release the lock before memory allocation, because it might cause
+// GCD lock conflict in one of calling path in AllocatPool().
+//
+CoreReleaseGcdMemoryLock ();
+
+//
+// Allocate memory to store the MemorySpaceMap. Note it might be already
+// allocated if there's map descriptor change during memory allocation at
+// last time.
+//
+if (*MemorySpaceMap != NULL) {
+ FreePool (*MemorySpaceMap);
+}
+
+*MemorySpaceMap = AllocatePool (DescriptorCount *
+sizeof (EFI_GCD_MEMORY_SPACE_DESCRIPTOR));
+if (*MemorySpaceMap == NULL) {
+ *NumberOfDescriptors = 0;
+ return EFI_OUT_OF_RESOURCES;
+}
+
+//
+// Save the descriptor number got before for another round of check to make
+// sure we won't miss any, since we have code running outside the GCD lock.
+//
+*NumberOfDescriptors = DescriptorCount;
+CoreAcquireGcdMemoryLock ();
}
- Status = EFI_SUCCESS;
-Done:
CoreReleaseGcdMemoryLock ();
- return Status;
+
+ return EFI_SUCCESS;
}
--
2.16.2.windows.1
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH v3 0/6] Introduce freed-memory guard feature
> v3 changes: > Updated per comments from Laszlo. Please refer to individual patch > file for details Freed-memory guard is a new feauture used to detect UAF (Use-After-Free) memory issue. Tests: a. Feature basic unit/functionality test b. OVMF regression test Jian J Wang (6): MdeModulePkg: cleanup Heap Guard pool/page type PCD documentation MdeModulePkg: introduce UEFI freed-memory guard bit in HeapGuard PCD UefiCpuPkg/CpuDxe: consider freed-memory guard in non-stop mode UefiCpuPkg/CpuDxe: prevent recursive calling of InitializePageTablePool MdeModulePkg/Core: prevent re-acquire GCD memory lock MdeModulePkg/Core: add freed-memory guard feature MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 80 +++-- MdeModulePkg/Core/Dxe/Mem/HeapGuard.c | 409 +- MdeModulePkg/Core/Dxe/Mem/HeapGuard.h | 65 +++- MdeModulePkg/Core/Dxe/Mem/Page.c | 41 ++- MdeModulePkg/Core/Dxe/Mem/Pool.c | 23 +- MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 2 +- MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c | 18 +- MdeModulePkg/MdeModulePkg.dec | 10 + MdeModulePkg/MdeModulePkg.uni | 6 +- UefiCpuPkg/CpuDxe/CpuDxe.h| 2 +- UefiCpuPkg/CpuDxe/CpuPageTable.c | 23 +- 11 files changed, 615 insertions(+), 64 deletions(-) -- 2.16.2.windows.1 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH v3 3/6] UefiCpuPkg/CpuDxe: consider freed-memory guard in non-stop mode
> v3 changes:
> a. split from #2 patch of v2
> b. refine the commit message
> c. refine the title
Non-stop mode was introduced / explained in commit 8f2613628acf
("MdeModulePkg/MdeModulePkg.dec: add new settings for PCDs",
2018-08-30).
The macro HEAP_GUARD_NONSTOP_MODE was added to CpuDxe in commit
dcc026217fdc ("UefiCpuPkg/CpuDxe: implement non-stop mode for uefi",
2018-08-30).
Another instance of the macro HEAP_GUARD_NONSTOP_MODE was added to
PiSmmCpuDxeSmm -- with BIT1|BIT0 replaced with BIT3|BIT2 -- in commit
09afd9a42a7f ("UefiCpuPkg/PiSmmCpuDxeSmm: implement non-stop mode for
SMM", 2018-08-30)
Since the freed-memory guard is for UEFI-only. This patch only updates
HEAP_GUARD_NONSTOP_MODE in "UefiCpuPkg/CpuDxe/CpuDxe.h" (add BIT4).
Cc: Laszlo Ersek
Cc: Star Zeng
Cc: Michael D Kinney
Cc: Jiewen Yao
Cc: Ruiyu Ni
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang
---
UefiCpuPkg/CpuDxe/CpuDxe.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.h b/UefiCpuPkg/CpuDxe/CpuDxe.h
index 064ea05bba..3183a3f7f4 100644
--- a/UefiCpuPkg/CpuDxe/CpuDxe.h
+++ b/UefiCpuPkg/CpuDxe/CpuDxe.h
@@ -58,7 +58,7 @@
)
#define HEAP_GUARD_NONSTOP_MODE \
-((PcdGet8 (PcdHeapGuardPropertyMask) & (BIT6|BIT1|BIT0)) > BIT6)
+((PcdGet8 (PcdHeapGuardPropertyMask) & (BIT6|BIT4|BIT1|BIT0)) > BIT6)
#define NULL_DETECTION_NONSTOP_MODE \
((PcdGet8 (PcdNullPointerDetectionPropertyMask) & (BIT6|BIT0)) > BIT6)
--
2.16.2.windows.1
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH v3 1/6] MdeModulePkg: cleanup Heap Guard pool/page type PCD documentation
> v3 changes: > a. split from #1 patch of v2 > b. update title This cleanup is meant for avoiding misuse of newly introduced BIT4 (UAF detection) of PCD PcdHeapGuardPropertyMask, because it applies to all types of physical memory. In another words, PcdHeapGuardPoolType and PcdHeapGuardPageType don't have effect to the BIT4 of PcdHeapGuardPropertyMask. Cc: Star Zeng Cc: Michael D Kinney Cc: Jiewen Yao Cc: Ruiyu Ni Cc: Laszlo Ersek Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Jian J Wang --- MdeModulePkg/MdeModulePkg.dec | 4 MdeModulePkg/MdeModulePkg.uni | 2 ++ 2 files changed, 6 insertions(+) diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec index 6037504fa7..2009dbc5fd 100644 --- a/MdeModulePkg/MdeModulePkg.dec +++ b/MdeModulePkg/MdeModulePkg.dec @@ -955,6 +955,8 @@ # free pages for all of them. The page allocation for the type related to # cleared bits keeps the same as ususal. # + # This PCD is only valid if BIT0 and/or BIT2 are set in PcdHeapGuardPropertyMask. + # # Below is bit mask for this PCD: (Order is same as UEFI spec) # EfiReservedMemoryType 0x0001 # EfiLoaderCode 0x0002 @@ -984,6 +986,8 @@ # if there's enough free memory for all of them. The pool allocation for the # type related to cleared bits keeps the same as ususal. # + # This PCD is only valid if BIT1 and/or BIT3 are set in PcdHeapGuardPropertyMask. + # # Below is bit mask for this PCD: (Order is same as UEFI spec) # EfiReservedMemoryType 0x0001 # EfiLoaderCode 0x0002 diff --git a/MdeModulePkg/MdeModulePkg.uni b/MdeModulePkg/MdeModulePkg.uni index a6bcb627cf..9d2e473fa9 100644 --- a/MdeModulePkg/MdeModulePkg.uni +++ b/MdeModulePkg/MdeModulePkg.uni @@ -1171,6 +1171,7 @@ " before and after corresponding type of pages allocated if there's enough\n" " free pages for all of them. The page allocation for the type related to\n" " cleared bits keeps the same as ususal.\n\n" + " This PCD is only valid if BIT0 and/or BIT2 are set in PcdHeapGuardPropertyMask.\n\n" " Below is bit mask for this PCD: (Order is same as UEFI spec)\n" " EfiReservedMemoryType 0x0001\n" " EfiLoaderCode 0x0002\n" @@ -1198,6 +1199,7 @@ " before and after corresponding type of pages which the allocated pool occupies,\n" " if there's enough free memory for all of them. The pool allocation for the\n" " type related to cleared bits keeps the same as ususal.\n\n" + " This PCD is only valid if BIT1 and/or BIT3 are set in PcdHeapGuardPropertyMask.\n\n" " Below is bit mask for this PCD: (Order is same as UEFI spec)\n" " EfiReservedMemoryType 0x0001\n" " EfiLoaderCode 0x0002\n" -- 2.16.2.windows.1 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH v3 4/6] UefiCpuPkg/CpuDxe: prevent recursive calling of InitializePageTablePool
> v3 changes:
> a. split from #2 patch
> b. refine the commit message and title
> c. remove "else" branch
The freed-memory guard feature will cause an recursive calling
of InitializePageTablePool(). This is due to a fact that
AllocateAlignedPages() is used to allocate page table pool memory.
This function will most likely call gBS->FreePages to free unaligned
pages and then cause another round of page attributes change, like
below (freed pages will be always marked not-present if freed-memory
guard is enabled)
FreePages() <===|
=> CpuSetMemoryAttributes()|
=> |
=> InitializePageTablePool() |
=> AllocateAlignedPages() |
=> FreePages() |
The solution is add a global variable as a lock in page table pool
allocation function and fail any other requests if it has not been
done.
Since this issue will only happen if free-memory guard is enabled,
it won't affect CpuSetMemoryAttributes() in default build of a BIOS.
If free-memory guard is enabled, it only affect the pages
(failed to mark them as not-present) freed in AllocateAlignedPages().
Since those freed pages haven't been used yet (their addresses not
yet exposed to code outside AllocateAlignedPages), it won't compromise
the freed-memory guard feature.
This change will just fail the CpuSetMemoryAttributes() called from
FreePages() but it won't fail the FreePages(). So the error status
won't be propagated to upper layer of code.
Cc: Laszlo Ersek
Cc: Star Zeng
Cc: Michael D Kinney
Cc: Jiewen Yao
Cc: Ruiyu Ni
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang
---
UefiCpuPkg/CpuDxe/CpuPageTable.c | 23 +--
1 file changed, 21 insertions(+), 2 deletions(-)
diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c
index 33e8ee2d2c..4bee8c7772 100644
--- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
+++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
@@ -100,6 +100,7 @@ PAGE_ATTRIBUTE_TABLE mPageAttributeTable[] = {
};
PAGE_TABLE_POOL *mPageTablePool = NULL;
+BOOLEAN mPageTablePoolLock = FALSE;
PAGE_TABLE_LIB_PAGING_CONTEXT mPagingContext;
EFI_SMM_BASE2_PROTOCOL*mSmmBase2 = NULL;
@@ -1046,6 +1047,16 @@ InitializePageTablePool (
VOID *Buffer;
BOOLEAN IsModified;
+ //
+ // Do not allow re-entrance.
+ //
+ if (mPageTablePoolLock) {
+return FALSE;
+ }
+
+ mPageTablePoolLock = TRUE;
+ IsModified = FALSE;
+
//
// Always reserve at least PAGE_TABLE_POOL_UNIT_PAGES, including one page for
// header.
@@ -1056,9 +1067,15 @@ InitializePageTablePool (
Buffer = AllocateAlignedPages (PoolPages, PAGE_TABLE_POOL_ALIGNMENT);
if (Buffer == NULL) {
DEBUG ((DEBUG_ERROR, "ERROR: Out of aligned pages\r\n"));
-return FALSE;
+goto Done;
}
+ DEBUG ((
+DEBUG_INFO,
+"Paging: added %lu pages to page table pool\r\n",
+(UINT64)PoolPages
+));
+
//
// Link all pools into a list for easier track later.
//
@@ -1092,7 +1109,9 @@ InitializePageTablePool (
);
ASSERT (IsModified == TRUE);
- return TRUE;
+Done:
+ mPageTablePoolLock = FALSE;
+ return IsModified;
}
/**
--
2.16.2.windows.1
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] FatBinPkg: Remove FatBinPkg and modify document
Thank you all. Push at 8a2794f6f3a42bcc878a30565e1db9ac96fdc7cd Thanks Liming >-Original Message- >From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Leif >Lindholm >Sent: Thursday, October 18, 2018 10:15 AM >To: Ard Biesheuvel >Cc: Ni, Ruiyu ; Laszlo Ersek ; edk2- >de...@lists.01.org; Andrew Fish >Subject: Re: [edk2] [PATCH] FatBinPkg: Remove FatBinPkg and modify >document > >On Thu, Oct 18, 2018 at 09:08:04AM +0800, Ard Biesheuvel wrote: >> On 17 October 2018 at 19:18, Laszlo Ersek wrote: >> > On 10/16/18 09:11, shenglei wrote: >> >> Remove FatBinPkg and modify Maintainers.txt. >> >> https://bugzilla.tianocore.org/show_bug.cgi?id=1105 >> >> >> >> Cc: Ruiyu Ni >> >> Contributed-under: TianoCore Contribution Agreement 1.1 >> >> Signed-off-by: Shenglei Zhang >> >> --- >> >> FatBinPkg/EnhancedFatDxe/AArch64/Fat.efi | Bin 26752 -> 0 bytes >> >> FatBinPkg/EnhancedFatDxe/Arm/Fat.efi | Bin 17152 -> 0 bytes >> >> FatBinPkg/EnhancedFatDxe/Ebc/Fat.efi | Bin 79136 -> 0 bytes >> >> FatBinPkg/EnhancedFatDxe/Fat.inf | 48 --- >> >> FatBinPkg/EnhancedFatDxe/Ia32/Fat.efi| Bin 21088 -> 0 bytes >> >> FatBinPkg/EnhancedFatDxe/Ipf/Fat.efi | Bin 154400 -> 0 bytes >> >> FatBinPkg/EnhancedFatDxe/X64/Fat.efi | Bin 28576 -> 0 bytes >> >> FatBinPkg/FatBinPkg.dec | 20 -- >> >> FatBinPkg/ReadMe.txt | 9 - >> >> Maintainers.txt | 2 +- >> >> 10 files changed, 1 insertion(+), 78 deletions(-) >> >> delete mode 100644 FatBinPkg/EnhancedFatDxe/AArch64/Fat.efi >> >> delete mode 100755 FatBinPkg/EnhancedFatDxe/Arm/Fat.efi >> >> delete mode 100644 FatBinPkg/EnhancedFatDxe/Ebc/Fat.efi >> >> delete mode 100644 FatBinPkg/EnhancedFatDxe/Fat.inf >> >> delete mode 100644 FatBinPkg/EnhancedFatDxe/Ia32/Fat.efi >> >> delete mode 100644 FatBinPkg/EnhancedFatDxe/Ipf/Fat.efi >> >> delete mode 100644 FatBinPkg/EnhancedFatDxe/X64/Fat.efi >> >> delete mode 100644 FatBinPkg/FatBinPkg.dec >> >> delete mode 100644 FatBinPkg/ReadMe.txt >> > >> > I'm happy about this patch. >> > >> > Reviewed-by: Laszlo Ersek >> >> Reviewed-by: Ard Biesheuvel > >Reviewed-by: Leif Lindholm > >/ >Leif >___ >edk2-devel mailing list >edk2-devel@lists.01.org >https://lists.01.org/mailman/listinfo/edk2-devel ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] MdePkg-BaseLib: Fix PathCleanUpDirectories() issue with "\\..\\.."
Reviewed-by: Liming Gao
>-Original Message-
>From: Ni, Ruiyu
>Sent: Wednesday, October 24, 2018 12:43 PM
>To: jim.dai...@dell.com; edk2-devel@lists.01.org
>Cc: Kinney, Michael D ; Gao, Liming
>
>Subject: RE: [edk2] [PATCH] MdePkg-BaseLib: Fix PathCleanUpDirectories()
>issue with "\\..\\.."
>
>Reviewed-by: Ruiyu Ni
>
>Thanks/Ray
>
>> -Original Message-
>> From: edk2-devel On Behalf Of
>> jim.dai...@dell.com
>> Sent: Saturday, October 6, 2018 3:15 AM
>> To: edk2-devel@lists.01.org
>> Cc: Kinney, Michael D ; Gao, Liming
>>
>> Subject: [edk2] [PATCH] MdePkg-BaseLib: Fix PathCleanUpDirectories()
>issue
>> with "\\..\\.."
>>
>> Replace multiple, consecutive "\" characters prior to other processing
>> involving "\" characters. This fixes an issue where "\\..\\..", "//..//..",
>> and
>> similar input paths are not cleaned properly.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Jim Dailey
>> ---
>> MdePkg/Library/BaseLib/FilePaths.c | 15 ---
>> 1 file changed, 8 insertions(+), 7 deletions(-)
>>
>> diff --git a/MdePkg/Library/BaseLib/FilePaths.c
>> b/MdePkg/Library/BaseLib/FilePaths.c
>> index d6f3758ecb..c5ca0a3b77 100644
>> --- a/MdePkg/Library/BaseLib/FilePaths.c
>> +++ b/MdePkg/Library/BaseLib/FilePaths.c
>> @@ -2,6 +2,7 @@
>>Defines file-path manipulation functions.
>>
>>Copyright (c) 2011 - 2017, Intel Corporation. All rights reserved.
>> + Copyright (c) 2018, Dell Technologies. All rights reserved.
>>This program and the accompanying materials
>>are licensed and made available under the terms and conditions of the BSD
>> License
>>which accompanies this distribution. The full text of the license may be
>> found at @@ -85,6 +86,13 @@ PathCleanUpDirectories(
>> }
>>}
>>
>> + //
>> + // Replace the "\\" with "\"
>> + //
>> + while ((TempString = StrStr (Path, L"")) != NULL) {
>> +CopyMem (TempString, TempString + 1, StrSize (TempString + 1)); }
>> +
>>//
>>// Remove all the "\.". E.g.: fs0:\abc\.\def\.
>>//
>> @@ -106,13 +114,6 @@ PathCleanUpDirectories(
>> CopyMem (Path + StrLen (Path), TempString + 3, StrSize (TempString +
>> 3));
>>}
>>
>> - //
>> - // Replace the "\\" with "\"
>> - //
>> - while ((TempString = StrStr (Path, L"")) != NULL) {
>> -CopyMem (TempString, TempString + 1, StrSize (TempString + 1));
>> - }
>> -
>>return Path;
>> }
>>
>> --
>> 2.17.0.windows.1
>>
>> ___
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] MdePkg-BaseLib: Fix PathCleanUpDirectories() issue with "\\..\\.."
Reviewed-by: Ruiyu Ni
Thanks/Ray
> -Original Message-
> From: edk2-devel On Behalf Of
> jim.dai...@dell.com
> Sent: Saturday, October 6, 2018 3:15 AM
> To: edk2-devel@lists.01.org
> Cc: Kinney, Michael D ; Gao, Liming
>
> Subject: [edk2] [PATCH] MdePkg-BaseLib: Fix PathCleanUpDirectories() issue
> with "\\..\\.."
>
> Replace multiple, consecutive "\" characters prior to other processing
> involving "\" characters. This fixes an issue where "\\..\\..", "//..//..",
> and
> similar input paths are not cleaned properly.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jim Dailey
> ---
> MdePkg/Library/BaseLib/FilePaths.c | 15 ---
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/MdePkg/Library/BaseLib/FilePaths.c
> b/MdePkg/Library/BaseLib/FilePaths.c
> index d6f3758ecb..c5ca0a3b77 100644
> --- a/MdePkg/Library/BaseLib/FilePaths.c
> +++ b/MdePkg/Library/BaseLib/FilePaths.c
> @@ -2,6 +2,7 @@
>Defines file-path manipulation functions.
>
>Copyright (c) 2011 - 2017, Intel Corporation. All rights reserved.
> + Copyright (c) 2018, Dell Technologies. All rights reserved.
>This program and the accompanying materials
>are licensed and made available under the terms and conditions of the BSD
> License
>which accompanies this distribution. The full text of the license may be
> found at @@ -85,6 +86,13 @@ PathCleanUpDirectories(
> }
>}
>
> + //
> + // Replace the "\\" with "\"
> + //
> + while ((TempString = StrStr (Path, L"")) != NULL) {
> +CopyMem (TempString, TempString + 1, StrSize (TempString + 1)); }
> +
>//
>// Remove all the "\.". E.g.: fs0:\abc\.\def\.
>//
> @@ -106,13 +114,6 @@ PathCleanUpDirectories(
> CopyMem (Path + StrLen (Path), TempString + 3, StrSize (TempString + 3));
>}
>
> - //
> - // Replace the "\\" with "\"
> - //
> - while ((TempString = StrStr (Path, L"")) != NULL) {
> -CopyMem (TempString, TempString + 1, StrSize (TempString + 1));
> - }
> -
>return Path;
> }
>
> --
> 2.17.0.windows.1
>
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 3/3] MdeModulePkg/NvmExpressDxe: Refine PassThru IO queue creation behavior
Reviewed-by: Ruiyu Ni
Thanks/Ray
> -Original Message-
> From: Wu, Hao A
> Sent: Wednesday, October 24, 2018 9:20 AM
> To: edk2-devel@lists.01.org
> Cc: Wu, Hao A ; Yao, Jiewen ;
> Ni, Ruiyu ; Zeng, Star
> Subject: [PATCH v2 3/3] MdeModulePkg/NvmExpressDxe: Refine PassThru
> IO queue creation behavior
>
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1260
>
> For the PassThru() service of NVM Express Pass Through Protocol, the
> current implementation (function NvmExpressPassThru()) will only use the
> IO Completion/Submission queues created internally by this driver during the
> controller initialization process. Any other IO queues created will not be
> consumed.
>
> So the value is little to accept external IO Completion/Submission queue
> creation request. This commit will refine the behavior of function
> NvmExpressPassThru(), it will only accept driver internal IO queue creation
> commands and will return "EFI_UNSUPPORTED" for external ones.
>
> Cc: Jiewen Yao
> Cc: Ruiyu Ni
> Cc: Star Zeng
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Hao Wu
> ---
> MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.h | 7 +-
> MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressHci.c | 6 +
> MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c | 25
> +---
> 3 files changed, 29 insertions(+), 9 deletions(-)
>
> diff --git a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.h
> b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.h
> index ad0d9b8966..fe7d37c118 100644
> --- a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.h
> +++ b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.h
> @@ -3,7 +3,7 @@
>NVM Express specification.
>
>(C) Copyright 2016 Hewlett Packard Enterprise Development LP
> - Copyright (c) 2013 - 2017, Intel Corporation. All rights reserved.
> + Copyright (c) 2013 - 2018, Intel Corporation. All rights
> + reserved.
>This program and the accompanying materials
>are licensed and made available under the terms and conditions of the BSD
> License
>which accompanies this distribution. The full text of the license may be
> found at @@ -147,6 +147,11 @@ struct
> _NVME_CONTROLLER_PRIVATE_DATA {
>NVME_CQHDBL CqHdbl[NVME_MAX_QUEUES];
>UINT16 AsyncSqHead;
>
> + //
> + // Flag to indicate internal IO queue creation.
> + //
> + BOOLEAN CreateIoQueue;
> +
>UINT8 Pt[NVME_MAX_QUEUES];
>UINT16 Cid[NVME_MAX_QUEUES];
>
> diff --git a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressHci.c
> b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressHci.c
> index 421561f16d..4a070f3f13 100644
> --- a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressHci.c
> +++ b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressHci.c
> @@ -584,6 +584,7 @@ NvmeCreateIoCompletionQueue (
>UINT16 QueueSize;
>
>Status = EFI_SUCCESS;
> + Private->CreateIoQueue = TRUE;
>
>for (Index = 1; Index < NVME_MAX_QUEUES; Index++) {
> ZeroMem (,
> sizeof(EFI_NVM_EXPRESS_PASS_THRU_COMMAND_PACKET));
> @@ -627,6 +628,8 @@ NvmeCreateIoCompletionQueue (
> }
>}
>
> + Private->CreateIoQueue = FALSE;
> +
>return Status;
> }
>
> @@ -653,6 +656,7 @@ NvmeCreateIoSubmissionQueue (
>UINT16 QueueSize;
>
>Status = EFI_SUCCESS;
> + Private->CreateIoQueue = TRUE;
>
>for (Index = 1; Index < NVME_MAX_QUEUES; Index++) {
> ZeroMem (,
> sizeof(EFI_NVM_EXPRESS_PASS_THRU_COMMAND_PACKET));
> @@ -698,6 +702,8 @@ NvmeCreateIoSubmissionQueue (
> }
>}
>
> + Private->CreateIoQueue = FALSE;
> +
>return Status;
> }
>
> diff --git a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> index c52e960771..78464ff422 100644
> --- a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> +++ b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> @@ -587,14 +587,23 @@ NvmExpressPassThru (
>}
>
>Sq->Prp[0] = (UINT64)(UINTN)Packet->TransferBuffer;
> - //
> - // If the NVMe cmd has data in or out, then mapping the user buffer to the
> PCI controller specific addresses.
> - // Note here we don't handle data buffer for CreateIOSubmitionQueue
> and CreateIOCompletionQueue cmds because
> - // these two cmds are special which requires their data buffer must
> support simultaneous access by both the
> - // processor and a PCI Bus Master. It's caller's responsbility to ensure
> this.
> - //
> - if (((Sq->Opc & (BIT0 | BIT1)) != 0) &&
> - !((Packet->QueueType == NVME_ADMIN_QUEUE) && ((Sq->Opc ==
> NVME_ADMIN_CRIOCQ_CMD) || (Sq->Opc ==
> NVME_ADMIN_CRIOSQ_CMD {
> + if ((Packet->QueueType == NVME_ADMIN_QUEUE) &&
> + ((Sq->Opc == NVME_ADMIN_CRIOCQ_CMD) || (Sq->Opc ==
> NVME_ADMIN_CRIOSQ_CMD))) {
> +//
> +// Currently, we
Re: [edk2] [patch 0/3] MdePkg/UefiDevicePathLib: Enahncement for Sata/Usbxx/AcpiExp device path
On 10/12/2018 10:18 AM, Dandan Bi wrote: https://bugzilla.tianocore.org/show_bug.cgi?id=1243 This pacth series is mainly to do the enahncement for Sata/Usbxx/AcpiExp device path to handle the cases when some optional paremeter is not specified in the text device path. Cc: Ruiyu Ni Cc: Michael D Kinney Cc: Liming Gao Dandan Bi (3): MdePkg: Handle Sata device path when optional para is not specified MdePkg: Handle USBxxx device path when optional para is not specified MdePkg: Handle AcpiExp device path when optional para is not specified .../UefiDevicePathLib/DevicePathFromText.c| 51 --- .../UefiDevicePathLib/DevicePathToText.c | 23 ++--- 2 files changed, 60 insertions(+), 14 deletions(-) Reviewed-by: Ruiyu Ni -- Thanks, Ray ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH] IntelSiliconPkg VTdDxe: Report status code for VTd error
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1273
Current code only uses DEBUG() for VTd error.
This patch updates to also report status code for VTd error.
Test done:
Created case that has VTd error and confirmed the error
status code could be reported as expected.
Cc: Jiewen Yao
Cc: Rangasai V Chaganty
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Star Zeng
---
IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.h | 1 +
IntelSiliconPkg/Feature/VTd/IntelVTdDxe/IntelVTdDxe.inf | 2 ++
IntelSiliconPkg/Feature/VTd/IntelVTdDxe/VtdReg.c| 1 +
IntelSiliconPkg/IntelSiliconPkg.dec | 6 ++
4 files changed, 10 insertions(+)
diff --git a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.h
b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.h
index 2ec92fe523c3..baa092f3ac0c 100644
--- a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.h
+++ b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.h
@@ -28,6 +28,7 @@
#include
#include
#include
+#include
#include
#include
diff --git a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/IntelVTdDxe.inf
b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/IntelVTdDxe.inf
index 60bb335da946..ca1f2d709215 100644
--- a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/IntelVTdDxe.inf
+++ b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/IntelVTdDxe.inf
@@ -60,6 +60,7 @@ [LibraryClasses]
CacheMaintenanceLib
PerformanceLib
PrintLib
+ ReportStatusCodeLib
[Guids]
gEfiEventExitBootServicesGuid ## CONSUMES ## Event
@@ -78,6 +79,7 @@ [Protocols]
[Pcd]
gIntelSiliconPkgTokenSpaceGuid.PcdVTdPolicyPropertyMask ## CONSUMES
+ gIntelSiliconPkgTokenSpaceGuid.PcdErrorCodeVTdError ## CONSUMES
[Depex]
gEfiPciRootBridgeIoProtocolGuid
diff --git a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/VtdReg.c
b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/VtdReg.c
index e564d373c756..45285510a500 100644
--- a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/VtdReg.c
+++ b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/VtdReg.c
@@ -545,6 +545,7 @@ DumpVtdIfError (
}
if (HasError) {
+ REPORT_STATUS_CODE (EFI_ERROR_CODE, PcdGet32 (PcdErrorCodeVTdError));
DEBUG((DEBUG_INFO, "\n ERROR \n"));
DumpVtdRegs (Num);
DEBUG((DEBUG_INFO, " ERROR \n\n"));
diff --git a/IntelSiliconPkg/IntelSiliconPkg.dec
b/IntelSiliconPkg/IntelSiliconPkg.dec
index 2f5bef6089f9..900e8f63c64d 100644
--- a/IntelSiliconPkg/IntelSiliconPkg.dec
+++ b/IntelSiliconPkg/IntelSiliconPkg.dec
@@ -47,6 +47,12 @@ [Ppis]
[Protocols]
gEdkiiPlatformVTdPolicyProtocolGuid = { 0x3d17e448, 0x466, 0x4e20, { 0x99,
0x9f, 0xb2, 0xe1, 0x34, 0x88, 0xee, 0x22 }}
+[PcdsFixedAtBuild, PcdsPatchableInModule]
+ ## Error code for VTd error.
+ # EDKII_ERROR_CODE_VTD_ERROR = (EFI_IO_BUS_UNSPECIFIED | (EFI_OEM_SPECIFIC
| 0x)) = 0x02008000
+ # @Prompt Error code for VTd error.
+
gIntelSiliconPkgTokenSpaceGuid.PcdErrorCodeVTdError|0x02008000|UINT32|0x0005
+
[PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
## This is the GUID of the FFS which contains the Graphics Video BIOS Table
(VBT)
# The VBT content is stored as a RAW section which is consumed by GOP
PEI/UEFI driver.
--
2.7.0.windows.1
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH] IntelSiliconPkg VTdDxe: Option to force no early access attr request
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1272
To have high confidence in usage for platform, add option (BIT2 of
PcdVTdPolicyPropertyMask) to force no IOMMU access attribute request
recording before DMAR table is installed.
Check PcdVTdPolicyPropertyMask BIT2 before RequestAccessAttribute()
and ProcessRequestedAccessAttribute(), then RequestAccessAttribute(),
ProcessRequestedAccessAttribute() and mAccessRequestXXX variables
could be optimized by compiler when PcdVTdPolicyPropertyMask BIT2 = 1.
Test done:
1: Created case that has IOMMU access attribute request before DMAR
table is installed, ASSERT was triggered after setting
PcdVTdPolicyPropertyMask BIT2 to 1.
2. Confirmed RequestAccessAttribute(), ProcessRequestedAccessAttribute()
and mAccessRequestXXX variables were optimized by compiler after
setting PcdVTdPolicyPropertyMask BIT2 to 1.
Cc: Jiewen Yao
Cc: Rangasai V Chaganty
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Star Zeng
---
IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.c | 8 +++-
IntelSiliconPkg/Feature/VTd/IntelVTdDxe/IntelVTdDxe.c | 7 +++
IntelSiliconPkg/IntelSiliconPkg.dec | 1 +
3 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.c
b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.c
index 86d50eb6f288..7784545631b3 100644
--- a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.c
+++ b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/DmaProtection.c
@@ -515,7 +515,13 @@ SetupVtd (
ParseDmarAcpiTableRmrr ();
- ProcessRequestedAccessAttribute ();
+ if ((PcdGet8 (PcdVTdPolicyPropertyMask) & BIT2) == 0) {
+//
+// Support IOMMU access attribute request recording before DMAR table is
installed.
+// Here is to process the requests.
+//
+ProcessRequestedAccessAttribute ();
+ }
for (Index = 0; Index < mVtdUnitNumber; Index++) {
DEBUG ((DEBUG_INFO,"VTD Unit %d (Segment: %04x)\n", Index,
mVtdUnitInformation[Index].Segment));
diff --git a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/IntelVTdDxe.c
b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/IntelVTdDxe.c
index 25d7c80af1d4..09948ce50e94 100644
--- a/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/IntelVTdDxe.c
+++ b/IntelSiliconPkg/Feature/VTd/IntelVTdDxe/IntelVTdDxe.c
@@ -254,6 +254,13 @@ VTdSetAttribute (
// Record the entry to driver global variable.
// As such once VTd is activated, the setting can be adopted.
//
+if ((PcdGet8 (PcdVTdPolicyPropertyMask) & BIT2) != 0) {
+ //
+ // Force no IOMMU access attribute request recording before DMAR table
is installed.
+ //
+ ASSERT_EFI_ERROR (EFI_NOT_READY);
+ return EFI_NOT_READY;
+}
Status = RequestAccessAttribute (Segment, SourceId, DeviceAddress, Length,
IoMmuAccess);
} else {
PERF_CODE (
diff --git a/IntelSiliconPkg/IntelSiliconPkg.dec
b/IntelSiliconPkg/IntelSiliconPkg.dec
index b9646d773b95..900e8f63c64d 100644
--- a/IntelSiliconPkg/IntelSiliconPkg.dec
+++ b/IntelSiliconPkg/IntelSiliconPkg.dec
@@ -64,6 +64,7 @@ [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic,
PcdsDynamicEx]
## The mask is used to control VTd behavior.
# BIT0: Enable IOMMU during boot (If DMAR table is installed in DXE. If
VTD_INFO_PPI is installed in PEI.)
# BIT1: Enable IOMMU when transfer control to OS (ExitBootService in normal
boot. EndOfPEI in S3)
+ # BIT2: Force no IOMMU access attribute request recording before DMAR table
is installed.
# @Prompt The policy for VTd driver behavior.
gIntelSiliconPkgTokenSpaceGuid.PcdVTdPolicyPropertyMask|1|UINT8|0x0002
--
2.7.0.windows.1
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] OvmfPkg: initialize bochs when initializing vmsvga
On 2018-10-23 22:02, Laszlo Ersek wrote:
On 10/23/18 15:42, Gerd Hoffmann wrote:
Hi,
Please help me see the situation better. Here's my current
understanding.
(1) QemuVideoDxe doesn't set up the VMW SVGA FIFO, and does not store
1
to the SVGA_REG_CONFIG_DONE register. And this is not a "small
missing step" -- the FIFO setup can be considered a separate
feature.
(2) We don't intend to implement the FIFO setup feature. (In
particular
because we don't intend to track changed rectangles and send
updates
via the FIFO.)
(3) The intent of the original VMW SVGA enablement patch for
QemuVideoDxe, namely commit c137d9508169, was to enable booting
some
UEFI operating systems on OVMF that had guest drivers only for
VMW
SVGA.
(4) The QEMU device model now -- since commit 104bd1dc70 -- falls
back
to stdvga (that is, Bochs) in response to QemuVideoDxe's actions.
(5) Your proposal is to set up the Bochs interface in QemuVideoDxe,
*in
addition* to the -- now dysfunctional! -- VMW SVGA interface.
Is my understanding correct?
I understand things the same way.
So, what do you think of the following approach, instead of your
currently proposed patch:
- revert commit c137d9508169 ("OvmfPkg/QemuVideoDxe: VMWare SVGA
device
support", 2017-04-07)
- revert commit 05a537945872 ("OvmfPkg/QemuVideoDxe: Helper functions
for unaligned port I/O.", 2017-04-07)
- given that QEMU provides the Bochs interface anyway, with the VMW
SVGA
device model, simply recognize the "QEMU VMWare SVGA" card, in the
"gQemuVideoCardList" array, as the QEMU_VIDEO_BOCHS_MMIO variant.
Makes sense to me.
Great, thank you.
Alternatively write a full-blown vmsvga driver which works simliar to
the virtio-gpu driver. I have my doubts this is worth the effort
though.
Right, please let us *not* do this.
Before QEMU commit 104bd1dc70, was it really *required* to use the
(now
dead) QemuVideoDxe code for VMW SVGA, or would it always have been
possible to simply use the Bochs interface, to drive the VMW SVGA
device?
I think bochs interface works works with all vmsvga version (in qemu).
Great!
Didn't test though.
That's fine; I think I haven't tested VMW SVGA, as in "ever". That's up
to people that actually use the device model. As I wrote previously,
it's quite telling that the consequences of QEMU commit 104bd1dc70,
from
release v2.10.0, are reported only now, more than a year after the
release -- VMW SVGA must not be a very popular device model.
Yuchenlin, can you then please investigate this approach, including
testing the reverts (the Bochs-only implementation) against QEMU
v2.9.1?
Sure!
Thanks,
yuchenlin
(FWIW, upstream QEMU doesn't seem to support earlier releases than
v2.10.2 any longer, according to
[edk2] [PATCH v2 3/3] MdeModulePkg/NvmExpressDxe: Refine PassThru IO queue creation behavior
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1260
For the PassThru() service of NVM Express Pass Through Protocol, the
current implementation (function NvmExpressPassThru()) will only use the
IO Completion/Submission queues created internally by this driver during
the controller initialization process. Any other IO queues created will
not be consumed.
So the value is little to accept external IO Completion/Submission queue
creation request. This commit will refine the behavior of function
NvmExpressPassThru(), it will only accept driver internal IO queue
creation commands and will return "EFI_UNSUPPORTED" for external ones.
Cc: Jiewen Yao
Cc: Ruiyu Ni
Cc: Star Zeng
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu
---
MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.h | 7 +-
MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressHci.c | 6 +
MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c | 25
+---
3 files changed, 29 insertions(+), 9 deletions(-)
diff --git a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.h
b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.h
index ad0d9b8966..fe7d37c118 100644
--- a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.h
+++ b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.h
@@ -3,7 +3,7 @@
NVM Express specification.
(C) Copyright 2016 Hewlett Packard Enterprise Development LP
- Copyright (c) 2013 - 2017, Intel Corporation. All rights reserved.
+ Copyright (c) 2013 - 2018, Intel Corporation. All rights reserved.
This program and the accompanying materials
are licensed and made available under the terms and conditions of the BSD
License
which accompanies this distribution. The full text of the license may be
found at
@@ -147,6 +147,11 @@ struct _NVME_CONTROLLER_PRIVATE_DATA {
NVME_CQHDBL CqHdbl[NVME_MAX_QUEUES];
UINT16 AsyncSqHead;
+ //
+ // Flag to indicate internal IO queue creation.
+ //
+ BOOLEAN CreateIoQueue;
+
UINT8 Pt[NVME_MAX_QUEUES];
UINT16 Cid[NVME_MAX_QUEUES];
diff --git a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressHci.c
b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressHci.c
index 421561f16d..4a070f3f13 100644
--- a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressHci.c
+++ b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressHci.c
@@ -584,6 +584,7 @@ NvmeCreateIoCompletionQueue (
UINT16 QueueSize;
Status = EFI_SUCCESS;
+ Private->CreateIoQueue = TRUE;
for (Index = 1; Index < NVME_MAX_QUEUES; Index++) {
ZeroMem (, sizeof(EFI_NVM_EXPRESS_PASS_THRU_COMMAND_PACKET));
@@ -627,6 +628,8 @@ NvmeCreateIoCompletionQueue (
}
}
+ Private->CreateIoQueue = FALSE;
+
return Status;
}
@@ -653,6 +656,7 @@ NvmeCreateIoSubmissionQueue (
UINT16 QueueSize;
Status = EFI_SUCCESS;
+ Private->CreateIoQueue = TRUE;
for (Index = 1; Index < NVME_MAX_QUEUES; Index++) {
ZeroMem (, sizeof(EFI_NVM_EXPRESS_PASS_THRU_COMMAND_PACKET));
@@ -698,6 +702,8 @@ NvmeCreateIoSubmissionQueue (
}
}
+ Private->CreateIoQueue = FALSE;
+
return Status;
}
diff --git a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
index c52e960771..78464ff422 100644
--- a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
+++ b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
@@ -587,14 +587,23 @@ NvmExpressPassThru (
}
Sq->Prp[0] = (UINT64)(UINTN)Packet->TransferBuffer;
- //
- // If the NVMe cmd has data in or out, then mapping the user buffer to the
PCI controller specific addresses.
- // Note here we don't handle data buffer for CreateIOSubmitionQueue and
CreateIOCompletionQueue cmds because
- // these two cmds are special which requires their data buffer must support
simultaneous access by both the
- // processor and a PCI Bus Master. It's caller's responsbility to ensure
this.
- //
- if (((Sq->Opc & (BIT0 | BIT1)) != 0) &&
- !((Packet->QueueType == NVME_ADMIN_QUEUE) && ((Sq->Opc ==
NVME_ADMIN_CRIOCQ_CMD) || (Sq->Opc == NVME_ADMIN_CRIOSQ_CMD {
+ if ((Packet->QueueType == NVME_ADMIN_QUEUE) &&
+ ((Sq->Opc == NVME_ADMIN_CRIOCQ_CMD) || (Sq->Opc ==
NVME_ADMIN_CRIOSQ_CMD))) {
+//
+// Currently, we only use the IO Completion/Submission queues created
internally
+// by this driver during controller initialization. Any other IO queues
created
+// will not be consumed here. The value is little to accept external IO
queue
+// creation requests, so here we will return EFI_UNSUPPORTED for external
IO
+// queue creation request.
+//
+if (!Private->CreateIoQueue) {
+ DEBUG ((DEBUG_ERROR, "NvmExpressPassThru: Does not support external IO
queues creation request.\n"));
+ return EFI_UNSUPPORTED;
+}
+
[edk2] [PATCH v2 2/3] MdeModulePkg/NvmExpressDxe: Always copy CQ entry to PassThru packet
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1259
According to the the NVM Express spec Revision 1.1, for some commands,
command-related information will be stored in the Dword 0 of the
completion queue entry.
One case is for the Get Features Command (Section 5.9.2 of the spec),
Dword 0 of the completion queue entry may contain feature information.
Hence, this commit will always copy the content of completion queue entry
to the PassThru packet regardless of the execution result of the command.
Cc: Liangcheng Tang
Cc: Star Zeng
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu
Reviewed-by: Ruiyu Ni
---
MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c | 9 -
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
index bfcd349794..c52e960771 100644
--- a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
+++ b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
@@ -781,17 +781,16 @@ NvmExpressPassThru (
} else {
Status = EFI_DEVICE_ERROR;
//
- // Copy the Respose Queue entry for this command to the callers response
buffer
- //
- CopyMem(Packet->NvmeCompletion, Cq, sizeof(EFI_NVM_EXPRESS_COMPLETION));
-
- //
// Dump every completion entry status for debugging.
//
DEBUG_CODE_BEGIN();
NvmeDumpStatus(Cq);
DEBUG_CODE_END();
}
+//
+// Copy the Respose Queue entry for this command to the callers response
buffer
+//
+CopyMem(Packet->NvmeCompletion, Cq, sizeof(EFI_NVM_EXPRESS_COMPLETION));
} else {
//
// Timeout occurs for an NVMe command. Reset the controller to abort the
--
2.12.0.windows.1
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH v2 1/3] MdeModulePkg/NvmExpressDxe: Refine data buffer & len check in PassThru
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1142
According to the the NVM Express spec Revision 1.1, for some commands
(like Get/Set Feature Command, Figure 89 & 90 of the spec), the Memory
Buffer maybe optional although the command opcode indicates there is a
data transfer between host & controller (Get/Set Feature Command, Figure
38 of the spec).
Hence, this commit refine the checks for the 'TransferLength' and
'TransferBuffer' field of the EFI_NVM_EXPRESS_PASS_THRU_COMMAND_PACKET
structure to address this issue.
Cc: Liangcheng Tang
Cc: Star Zeng
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu
Reviewed-by: Ruiyu Ni
---
MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c | 33
+++-
1 file changed, 18 insertions(+), 15 deletions(-)
diff --git a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
index 2468871322..bfcd349794 100644
--- a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
+++ b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
@@ -595,7 +595,8 @@ NvmExpressPassThru (
//
if (((Sq->Opc & (BIT0 | BIT1)) != 0) &&
!((Packet->QueueType == NVME_ADMIN_QUEUE) && ((Sq->Opc ==
NVME_ADMIN_CRIOCQ_CMD) || (Sq->Opc == NVME_ADMIN_CRIOSQ_CMD {
-if ((Packet->TransferLength == 0) || (Packet->TransferBuffer == NULL)) {
+if (((Packet->TransferLength != 0) && (Packet->TransferBuffer == NULL)) ||
+((Packet->TransferLength == 0) && (Packet->TransferBuffer != NULL))) {
return EFI_INVALID_PARAMETER;
}
@@ -605,21 +606,23 @@ NvmExpressPassThru (
Flag = EfiPciIoOperationBusMasterWrite;
}
-MapLength = Packet->TransferLength;
-Status = PciIo->Map (
- PciIo,
- Flag,
- Packet->TransferBuffer,
- ,
- ,
-
- );
-if (EFI_ERROR (Status) || (Packet->TransferLength != MapLength)) {
- return EFI_OUT_OF_RESOURCES;
-}
+if ((Packet->TransferLength != 0) && (Packet->TransferBuffer != NULL)) {
+ MapLength = Packet->TransferLength;
+ Status = PciIo->Map (
+PciIo,
+Flag,
+Packet->TransferBuffer,
+,
+,
+
+);
+ if (EFI_ERROR (Status) || (Packet->TransferLength != MapLength)) {
+return EFI_OUT_OF_RESOURCES;
+ }
-Sq->Prp[0] = PhyAddr;
-Sq->Prp[1] = 0;
+ Sq->Prp[0] = PhyAddr;
+ Sq->Prp[1] = 0;
+}
if((Packet->MetadataLength != 0) && (Packet->MetadataBuffer != NULL)) {
MapLength = Packet->MetadataLength;
--
2.12.0.windows.1
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH v2 0/3] NvmExpressDxe: Bug fixes within NvmExpressPassThru()
V2 changes: A. For patch 3/3, introduces a BOOLEAN flag in the controller private data structure. The flag will indicate internal IO queues creation and it will simplifies the check logic in the NvmExpressPassThru() function. V1 history: The series will address a couple of bugs within the NvmExpressPassThru() function. Please refer to the log messages of each commit for more details. Cc: Liangcheng Tang Cc: Jiewen Yao Cc: Ruiyu Ni Cc: Star Zeng Hao Wu (3): MdeModulePkg/NvmExpressDxe: Refine data buffer & len check in PassThru MdeModulePkg/NvmExpressDxe: Always copy CQ entry to PassThru packet MdeModulePkg/NvmExpressDxe: Refine PassThru IO queue creation behavior MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpress.h | 7 +- MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressHci.c | 6 ++ MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c | 67 3 files changed, 51 insertions(+), 29 deletions(-) -- 2.12.0.windows.1 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v1 3/3] MdeModulePkg/NvmExpressDxe: Refine PassThru IO queue creation behavior
> -Original Message-
> From: Ni, Ruiyu
> Sent: Tuesday, October 23, 2018 4:54 PM
> To: Wu, Hao A; edk2-devel@lists.01.org
> Cc: Yao, Jiewen; Zeng, Star
> Subject: Re: [PATCH v1 3/3] MdeModulePkg/NvmExpressDxe: Refine PassThru
> IO queue creation behavior
>
> On 10/18/2018 2:42 PM, Hao Wu wrote:
> > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1260
> >
> > For the PassThru() service of NVM Express Pass Through Protocol, the
> > current implementation (function NvmExpressPassThru()) will only use the
> > IO Completion/Submission queues created internally by this driver during
> > the controller initialization process. Any other IO queues created will
> > not be consumed.
> >
> > So the value is little to accept external IO Completion/Submission queue
> > creation request. This commit will refine the behavior of function
> > NvmExpressPassThru(), it will only accept driver internal IO queue
> > creation commands and will return "EFI_UNSUPPORTED" for external ones.
> >
> > Cc: Jiewen Yao
> > Cc: Ruiyu Ni
> > Cc: Star Zeng
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Hao Wu
> > ---
> > MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c | 42
>
> > 1 file changed, 34 insertions(+), 8 deletions(-)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> > index c52e960771..0c550bd52c 100644
> > --- a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> > +++ b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
> > @@ -476,6 +476,7 @@ NvmExpressPassThru (
> > UINT32 Data;
> > NVME_PASS_THRU_ASYNC_REQ *AsyncRequest;
> > EFI_TPLOldTpl;
> > + UINT32 CrIoQid;
> >
> > //
> > // check the data fields in Packet parameter.
> > @@ -587,14 +588,39 @@ NvmExpressPassThru (
> > }
> >
> > Sq->Prp[0] = (UINT64)(UINTN)Packet->TransferBuffer;
> > - //
> > - // If the NVMe cmd has data in or out, then mapping the user buffer to
> the PCI controller specific addresses.
> > - // Note here we don't handle data buffer for CreateIOSubmitionQueue
> and CreateIOCompletionQueue cmds because
> > - // these two cmds are special which requires their data buffer must
> support simultaneous access by both the
> > - // processor and a PCI Bus Master. It's caller's responsbility to ensure
> > this.
> > - //
> > - if (((Sq->Opc & (BIT0 | BIT1)) != 0) &&
> > - !((Packet->QueueType == NVME_ADMIN_QUEUE) && ((Sq->Opc ==
> NVME_ADMIN_CRIOCQ_CMD) || (Sq->Opc == NVME_ADMIN_CRIOSQ_CMD
> {
> > + if ((Packet->QueueType == NVME_ADMIN_QUEUE) &&
> > + ((Sq->Opc == NVME_ADMIN_CRIOCQ_CMD) || (Sq->Opc ==
> NVME_ADMIN_CRIOSQ_CMD))) {
> > +//
> > +// Command Dword 10 should be valid for CreateIOCompletionQueue
> and
> > +// CreateIOSubmissionQueue commands.
> > +//
> > +if (!(Packet->NvmeCmd->Flags & CDW10_VALID)) {
> > + return EFI_INVALID_PARAMETER;
> > +}
> > +
> > +//
> > +// Bits 15:0 of Command Dword 10 is the Queue Identifier (QID) for
> > +// CreateIOCompletionQueue and CreateIOSubmissionQueue commands.
> > +//
> > +CrIoQid = Packet->NvmeCmd->Cdw10 & 0x;
> > +
> > +//
> > +// Currently, we only use the IO Completion/Submission queues created
> internally
> > +// by this driver during controller initialization. Any other IO queues
> created
> > +// will not be consumed here. The value is little to accept external IO
> queue
> > +// creation requests, so here we will return EFI_UNSUPPORTED for
> external IO
> > +// queue creation request.
> > +//
> > +if ((CrIoQid >= NVME_MAX_QUEUES) ||
> > +((Sq->Opc == NVME_ADMIN_CRIOCQ_CMD) && (Packet-
> >TransferBuffer != Private->CqBufferPciAddr[CrIoQid])) ||
> > +((Sq->Opc == NVME_ADMIN_CRIOSQ_CMD) && (Packet-
> >TransferBuffer != Private->SqBufferPciAddr[CrIoQid]))) {
> > + DEBUG ((DEBUG_ERROR, "NvmExpressPassThru: Does not support
> external IO queues creation request.\n"));
> > + return EFI_UNSUPPORTED;
> > +}
>
> Does the above check is to make sure only accept IO queues creation
> request from NvmeCreateIoCompletionQueue() and
> NvmeCreateIoSubmissionQueue()?
Yes. Actually, the above codes (including the "Packet->NvmeCmd->Flags") are to
ensure that the IO queues creation request is internal.
>
> The check is very complex and unnecessary IMO. If we introduce a boolean
> field like CreateQueue in the Private structure and set it to TRUE when
> calling PassThru() from the above two internal functions, the above
> check can be replaced with only one line check:
>if (Private->CreateQueue)
>
> It does introduce a "dirty" flag in Private structure, but the above
> complex check can be avoided.
> What's your opinion?
Yes, I think for this way the code is more simple.
I will propose a V2 of the series.
Re: [edk2] [PATCH v2 1/5] MdeModulePkg/MdeModulePkg.dec: update PCD description for new feature
Laszlo, Thank you very much for the comments. I went through all of them in other patch emails and I think I have no objection. So to save all of us time I'm not going to respond them separately. I'll send out v3 patch soon. Thanks again. Regards, Jian > -Original Message- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Wednesday, October 24, 2018 12:09 AM > To: Wang, Jian J ; edk2-devel@lists.01.org > Cc: Kinney, Michael D ; Ni, Ruiyu > ; Yao, Jiewen ; Zeng, Star > > Subject: Re: [edk2] [PATCH v2 1/5] MdeModulePkg/MdeModulePkg.dec: update > PCD description for new feature > > On 10/23/18 16:53, Jian J Wang wrote: > >> v2 changes: > >> a. Drop PCD PcdUseAfterFreeDetectionPropertyMask. Use BIT4 of > >>PcdHeapGuardPropertyMask instead. Update related descriptions in both > >>dec and uni files. > > > > Freed-memory guard is used to detect UAF (Use-After-Free) memory issue > > which is illegal access to memory which has been freed. The principle > > behind is similar to heap guard feature, that is we'll turn all pool > > memory allocation to page allocation and mark them to be not-present > > once they are freed. > > > > This also implies that, once a page is allocated and freed, it cannot > > be re-allocated. This will bring another issue, which is that there's > > risk that memory space will be used out. To address it, this patch > > series add logic put part (at most 64 pages a time) of freed pages > > back into page pool, so that the memory service can still have memory > > to allocate, when all memory space have been allocated once. This is > > called memory promotion. The promoted pages are always from the eldest > > pages freed. > > > > BIT4 of following PCD is used to enable the freed-memory guard feature. > > > > gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask > > > > Please note this feature cannot be enabled with heap guard at the same > > time. > > > > Cc: Star Zeng > > Cc: Michael D Kinney > > Cc: Jiewen Yao > > Cc: Ruiyu Ni > > Cc: Laszlo Ersek > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Jian J Wang > > --- > > MdeModulePkg/MdeModulePkg.dec | 10 ++ > > MdeModulePkg/MdeModulePkg.uni | 6 +- > > 2 files changed, 15 insertions(+), 1 deletion(-) > > > > diff --git a/MdeModulePkg/MdeModulePkg.dec > b/MdeModulePkg/MdeModulePkg.dec > > index 6037504fa7..255b92ea67 100644 > > --- a/MdeModulePkg/MdeModulePkg.dec > > +++ b/MdeModulePkg/MdeModulePkg.dec > > @@ -955,6 +955,8 @@ > ># free pages for all of them. The page allocation for the type related to > ># cleared bits keeps the same as ususal. > ># > > + # This PCD is only valid if BIT0 and/or BIT2 are set in > PcdHeapGuardPropertyMask. > > + # > ># Below is bit mask for this PCD: (Order is same as UEFI spec) > ># EfiReservedMemoryType 0x0001 > ># EfiLoaderCode 0x0002 > > @@ -984,6 +986,8 @@ > ># if there's enough free memory for all of them. The pool allocation for > > the > ># type related to cleared bits keeps the same as ususal. > ># > > + # This PCD is only valid if BIT1 and/or BIT3 are set in > PcdHeapGuardPropertyMask. > > + # > ># Below is bit mask for this PCD: (Order is same as UEFI spec) > ># EfiReservedMemoryType 0x0001 > ># EfiLoaderCode 0x0002 > > @@ -1007,14 +1011,20 @@ > > > gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPoolType|0x0|UINT64|0x30 > 001053 > > > >## This mask is to control Heap Guard behavior. > > + # > ># Note that due to the limit of pool memory implementation and the > alignment > ># requirement of UEFI spec, BIT7 is a try-best setting which cannot > guarantee > ># that the returned pool is exactly adjacent to head guard page or tail > > guard > ># page. > > (1) The above changes are not related to the use-after-free feature; > they should go into a separate cleanup patch. (Which is very welcome > otherwise.) The cleanup patch should be patch #1 in the series. > > The subject should be, for example: > > MdeModulePkg: clean up Heap Guard PageType / PoolType PCD > documentation > > (71 characters) > > > + # > > + # Note that UEFI freed-memory guard and pool/page guard cannot be > enabled > > + # at the same time. > > + # > ># BIT0 - Enable UEFI page guard. > ># BIT1 - Enable UEFI pool guard. > ># BIT2 - Enable SMM page guard. > ># BIT3 - Enable SMM pool guard. > > + # BIT4 - Enable UEFI freed-memory guard (Use-After-Free memory > detection). > ># BIT6 - Enable non-stop mode. > ># BIT7 - The direction of Guard Page for Pool Guard. > ># 0 - The returned pool is near the tail guard page. > > (2) This should go into patch #2 in the series. However, the current > subject line is useless: > > MdeModulePkg/MdeModulePkg.dec: update PCD description for new feature > > Instead, I suggest:
Re: [edk2] [PATCH v2] IntelFsp2Pkg: FSP should not override IDT
Hello,
Please see my reply below inline.
Thanks!
Chasel
-Original Message-
From: af...@apple.com [mailto:af...@apple.com]
Sent: Tuesday, October 23, 2018 6:29 PM
To: Chiu, Chasel
Cc: edk2-devel@lists.01.org; Yao, Jiewen
Subject: Re: [edk2] [PATCH v2] IntelFsp2Pkg: FSP should not override IDT
> On Oct 23, 2018, at 2:33 AM, Chasel, Chiu wrote:
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1265
>
> FSP should not override IDT table when it is initialized by boot
> loader. IDT should be re-initialized in FSP only when it is invalid.
> To mitigate temporary memory usage a PCD PcdFspMaxInterruptSupported
> created for platform to decide how many interrupts the FSP IDT table
> can support.
>
> Test: Verified on internal platform and boots successfully.
>
> Cc: Jiewen Yao
> Cc: Desimone Nathaniel L
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Chasel Chiu
> ---
> IntelFsp2Pkg/FspSecCore/FspSecCoreM.inf | 1 +
> IntelFsp2Pkg/FspSecCore/SecMain.c | 24 +++-
> IntelFsp2Pkg/FspSecCore/SecMain.h | 6 ++
> IntelFsp2Pkg/IntelFsp2Pkg.dec | 4
> 4 files changed, 26 insertions(+), 9 deletions(-)
>
> diff --git a/IntelFsp2Pkg/FspSecCore/FspSecCoreM.inf
> b/IntelFsp2Pkg/FspSecCore/FspSecCoreM.inf
> index c61af10b8a..dafe6f5993 100644
> --- a/IntelFsp2Pkg/FspSecCore/FspSecCoreM.inf
> +++ b/IntelFsp2Pkg/FspSecCore/FspSecCoreM.inf
> @@ -62,6 +62,7 @@
> gIntelFsp2PkgTokenSpaceGuid.PcdTemporaryRamSize ## CONSUMES
> gIntelFsp2PkgTokenSpaceGuid.PcdFspTemporaryRamSize ## CONSUMES
> gIntelFsp2PkgTokenSpaceGuid.PcdFspHeapSizePercentage ## CONSUMES
> + gIntelFsp2PkgTokenSpaceGuid.PcdFspMaxInterruptSupported ## CONSUMES
>
> [Ppis]
> gEfiTemporaryRamSupportPpiGuid ## PRODUCES
> diff --git a/IntelFsp2Pkg/FspSecCore/SecMain.c
> b/IntelFsp2Pkg/FspSecCore/SecMain.c
> index 37fd4dfdeb..ddbfc4fcdf 100644
> --- a/IntelFsp2Pkg/FspSecCore/SecMain.c
> +++ b/IntelFsp2Pkg/FspSecCore/SecMain.c
> @@ -70,6 +70,7 @@ SecStartup (
> UINT32 Index;
> FSP_GLOBAL_DATA PeiFspData;
> UINT64 ExceptionHandler;
> + UINTN IdtSize;
>
> //
> // Process all libraries constructor function linked to SecCore.
> @@ -98,13 +99,26 @@ SecStartup (
> // | |
> // |---|> TempRamBase
> IdtTableInStack.PeiService = NULL;
> - ExceptionHandler = FspGetExceptionHandler(mIdtEntryTemplate);
> - for (Index = 0; Index < SEC_IDT_ENTRY_COUNT; Index ++) {
> -CopyMem ((VOID*)[Index],
> (VOID*), sizeof (UINT64));
> + AsmReadIdtr ();
> + if ((IdtDescriptor.Base == 0) && (IdtDescriptor.Limit == 0x)) {
Are these architectural value at reset?
Thanks,
Andrew Fish
Chasel: Yes, these are default values from reset.
> +ExceptionHandler = FspGetExceptionHandler(mIdtEntryTemplate);
> +for (Index = 0; Index < FixedPcdGet8(PcdFspMaxInterruptSupported); Index
> ++) {
> + CopyMem ((VOID*)[Index],
> (VOID*), sizeof (UINT64));
> +}
> +IdtSize = sizeof (IdtTableInStack.IdtTable); } else {
> +if (IdtDescriptor.Limit + 1 > sizeof (IdtTableInStack.IdtTable)) {
> + //
> + // ERROR: IDT table size from boot loader is larger than FSP can
> support, DeadLoop here!
> + //
> + CpuDeadLoop();
> +} else {
> + IdtSize = IdtDescriptor.Limit + 1;
> +}
> +CopyMem ((VOID *) (UINTN) , (VOID *)
> + IdtDescriptor.Base, IdtSize);
> }
> -
> IdtDescriptor.Base = (UINTN)
> - IdtDescriptor.Limit = (UINT16)(sizeof (IdtTableInStack.IdtTable) -
> 1);
> + IdtDescriptor.Limit = (UINT16)(IdtSize - 1);
>
> AsmWriteIdtr ();
>
> diff --git a/IntelFsp2Pkg/FspSecCore/SecMain.h
> b/IntelFsp2Pkg/FspSecCore/SecMain.h
> index 291bc5ca5c..19ac2fbfc1 100644
> --- a/IntelFsp2Pkg/FspSecCore/SecMain.h
> +++ b/IntelFsp2Pkg/FspSecCore/SecMain.h
> @@ -1,6 +1,6 @@
> /** @file
>
> - Copyright (c) 2014 - 2016, Intel Corporation. All rights
> reserved.
> + Copyright (c) 2014 - 2018, Intel Corporation. All rights
> + reserved.
> This program and the accompanying materials
> are licensed and made available under the terms and conditions of the BSD
> License
> which accompanies this distribution. The full text of the license
> may be found at @@ -29,8 +29,6 @@ #include
> #include
>
> -#define SEC_IDT_ENTRY_COUNT34
> -
> typedef VOID (*PEI_CORE_ENTRY) ( \
> IN CONST EFI_SEC_PEI_HAND_OFF*SecCoreData, \
> IN CONST EFI_PEI_PPI_DESCRIPTOR *PpiList \ @@ -38,7 +36,7 @@
> typedef VOID (*PEI_CORE_ENTRY) ( \
>
> typedef struct _SEC_IDT_TABLE {
> EFI_PEI_SERVICES *PeiService;
> - UINT64IdtTable[SEC_IDT_ENTRY_COUNT];
> + UINT64IdtTable[FixedPcdGet8 (PcdFspMaxInterruptSupported)];
> } SEC_IDT_TABLE;
>
> /**
> diff --git a/IntelFsp2Pkg/IntelFsp2Pkg.dec
> b/IntelFsp2Pkg/IntelFsp2Pkg.dec
Re: [edk2] OpenProtocolInformation
The same protocol can be installed on many handles in the handle database. For example, if there are multiple mass storage devices in a platform, there will be multiple Block I/O Protocols in the handle database. Mike > -Original Message- > From: edk2-devel [mailto:edk2-devel- > boun...@lists.01.org] On Behalf Of TVKR > Sent: Tuesday, October 23, 2018 4:06 PM > To: edk2-devel@lists.01.org > Subject: [edk2] OpenProtocolInformation > > Hi, > > If the main purpose of the OpenProtocolInformation > service is to provide > the complete list of agents currently using a specific > protocol interface > then what is the need for providing the Handle argument > too as an input? > Why does it matter what Handle was this queried protocol > installed on? > > Thanks > ___ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] OpenProtocolInformation
Hi, If the main purpose of the OpenProtocolInformation service is to provide the complete list of agents currently using a specific protocol interface then what is the need for providing the Handle argument too as an input? Why does it matter what Handle was this queried protocol installed on? Thanks ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 4/5] MdeModulePkg/Core: add freed-memory guard feature
On 10/23/18 16:53, Jian J Wang wrote: >> v2 changes: >> a. Change prototype and implementation of IsHeapGuardEnabled() >>to allow it to check freed-memory guard feature. >> b. Drop IsUafEnabled() because of a. >> c. Move the sanity check of freed-memory guard and heap guard >>into HeapGuardCpuArchProtocolNotify() >> d. Add GuardFreedPagesChecked() to avoid duplicate feature check >> e. Coding style cleanup > > Freed-memory guard is used to detect UAF (Use-After-Free) memory issue > which is illegal access to memory which has been freed. The principle > behind is similar to heap guard feature, that is we'll turn all pool > memory allocation to page allocation and mark them to be not-present > once they are freed. > > This also implies that, once a page is allocated and freed, it cannot > be re-allocated. This will bring another issue, which is that there's > risk that memory space will be used out. To address it, the memory > service add logic to put part (at most 64 pages a time) of freed pages > back into page pool, so that the memory service can still have memory > to allocate, when all memory space have been allocated once. This is > called memory promotion. The promoted pages are always from the eldest > pages which haven been freed. > > Cc: Star Zeng > Cc: Michael D Kinney > Cc: Jiewen Yao > Cc: Ruiyu Ni > Cc: Laszlo Ersek > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Jian J Wang > --- > MdeModulePkg/Core/Dxe/Mem/HeapGuard.c | 409 > +- > MdeModulePkg/Core/Dxe/Mem/HeapGuard.h | 63 +- > MdeModulePkg/Core/Dxe/Mem/Page.c | 41 +++- > MdeModulePkg/Core/Dxe/Mem/Pool.c | 21 +- > 4 files changed, 513 insertions(+), 21 deletions(-) I don't know when I will find the time to review this patch. Please make sure that with BIT4 clear in the PCD, the changes are a no-op. I'd prefer if you could regression-test the changes on OVMF as well, not just on physical platforms. Other than that, until I find the time, please proceed with the normal review workflow -- feel free to submit further versions, according to the MdeModulePkg maintainers' comments, and/or even push the final version, should I prove unable to comment on this patch in time. Thanks! Laszlo ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [Patch v3 6/6] BaseTools/GenFds: create and use new variable in FdfParser
replace lots of '}' and "}" with a shared new consistent variable.
Cc: Bob Feng
Cc: Yonghong Zhu
Cc: Liming Gao
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jaben Carsey
---
BaseTools/Source/Python/GenFds/FdfParser.py | 45 ++--
1 file changed, 23 insertions(+), 22 deletions(-)
diff --git a/BaseTools/Source/Python/GenFds/FdfParser.py
b/BaseTools/Source/Python/GenFds/FdfParser.py
index d954c0b40b3b..bf6e0bd2286c 100644
--- a/BaseTools/Source/Python/GenFds/FdfParser.py
+++ b/BaseTools/Source/Python/GenFds/FdfParser.py
@@ -65,8 +65,9 @@ T_CHAR_TAB = '\t'
T_CHAR_DOUBLE_QUOTE = '\"'
T_CHAR_SINGLE_QUOTE = '\''
T_CHAR_STAR = '*'
+T_CHAR_BRACE_R = '}'
-SEPARATORS = {TAB_EQUAL_SPLIT, TAB_VALUE_SPLIT, TAB_COMMA_SPLIT, '{', '}'}
+SEPARATORS = {TAB_EQUAL_SPLIT, TAB_VALUE_SPLIT, TAB_COMMA_SPLIT, '{',
T_CHAR_BRACE_R}
ALIGNMENTS = {"Auto", "8", "16", "32", "64", "128", "512", "1K", "4K", "32K",
"64K", "128K",
"256K", "512K", "1M", "2M", "4M", "8M",
"16M"}
ALIGNMENT_NOAUTO = ALIGNMENTS - {"Auto"}
@@ -2021,7 +2022,7 @@ class FdfParser:
DataString += self._Token
DataString += TAB_COMMA_SPLIT
-if not self._IsToken("}"):
+if not self._IsToken(T_CHAR_BRACE_R):
raise Warning.ExpectedCurlyClose(self.FileName,
self.CurrentLineNumber)
DataString = DataString.rstrip(TAB_COMMA_SPLIT)
@@ -2061,7 +2062,7 @@ class FdfParser:
DataString += self._Token
DataString += TAB_COMMA_SPLIT
-if not self._IsToken("}"):
+if not self._IsToken(T_CHAR_BRACE_R):
raise Warning.ExpectedCurlyClose(self.FileName,
self.CurrentLineNumber)
DataString = DataString.rstrip(TAB_COMMA_SPLIT)
@@ -2330,10 +2331,10 @@ class FdfParser:
DataString += self._Token
DataString += TAB_COMMA_SPLIT
-if not self._IsToken("}"):
+if not self._IsToken(T_CHAR_BRACE_R):
raise Warning.ExpectedCurlyClose(self.FileName,
self.CurrentLineNumber)
-if not self._IsToken("}"):
+if not self._IsToken(T_CHAR_BRACE_R):
raise Warning.ExpectedCurlyClose(self.FileName,
self.CurrentLineNumber)
DataString = DataString.rstrip(TAB_COMMA_SPLIT)
@@ -2348,7 +2349,7 @@ class FdfParser:
FvObj.FvExtEntryData.append(self._Token)
-if not self._IsToken("}"):
+if not self._IsToken(T_CHAR_BRACE_R):
raise Warning.ExpectedCurlyClose(self.FileName,
self.CurrentLineNumber)
return True
@@ -2384,7 +2385,7 @@ class FdfParser:
if not IsInf and not IsFile:
break
-if not self._IsToken("}"):
+if not self._IsToken(T_CHAR_BRACE_R):
raise Warning.ExpectedCurlyClose(self.FileName,
self.CurrentLineNumber)
FvObj.AprioriSectionList.append(AprSectionObj)
@@ -2659,7 +2660,7 @@ class FdfParser:
FfsFileObj.FileName = self._Token.replace('$(SPACE)', ' ')
self._VerifyFile(FfsFileObj.FileName)
-if not self._IsToken("}"):
+if not self._IsToken(T_CHAR_BRACE_R):
raise Warning.ExpectedCurlyClose(self.FileName,
self.CurrentLineNumber)
## _GetRAWData() method
@@ -2684,7 +2685,7 @@ class FdfParser:
raise Warning.Expected("Filename value", self.FileName,
self.CurrentLineNumber)
FileName = self._Token.replace('$(SPACE)', ' ')
-if FileName == '}':
+if FileName == T_CHAR_BRACE_R:
self._UndoToken()
raise Warning.Expected("Filename value", self.FileName,
self.CurrentLineNumber)
@@ -2693,7 +2694,7 @@ class FdfParser:
FfsFileObj.FileName.append(File.Path)
FfsFileObj.SubAlignment.append(AlignValue)
-if self._IsToken("}"):
+if self._IsToken(T_CHAR_BRACE_R):
self._UndoToken()
break
@@ -2865,7 +2866,7 @@ class FdfParser:
if not IsInf and not IsFile:
break
-if not self._IsToken("}"):
+if not self._IsToken(T_CHAR_BRACE_R):
raise Warning.ExpectedCurlyClose(self.FileName,
self.CurrentLineNumber)
FvImageSectionObj = FvImageSection()
@@ -2890,10 +2891,10 @@ class FdfParser:
raise Warning.ExpectedEquals(self.FileName,
self.CurrentLineNumber)
if not self._IsToken("{"):
raise Warning.ExpectedCurlyOpen(self.FileName,
self.CurrentLineNumber)
-if not self._SkipToToken("}"):
+if not self._SkipToToken(T_CHAR_BRACE_R):
raise Warning.Expected("Depex expression ending '}'",
self.FileName, self.CurrentLineNumber)
-DepexSectionObj.Expression =
[edk2] [Patch v3 5/6] BaseTools/GenFds: refactor FdfParser warnings
make functions for common error messages
refactor to use these functions
Cc: Yonghong Zhu
Cc: Liming Gao
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jaben Carsey
---
BaseTools/Source/Python/GenFds/FdfParser.py | 603 ++--
1 file changed, 303 insertions(+), 300 deletions(-)
diff --git a/BaseTools/Source/Python/GenFds/FdfParser.py
b/BaseTools/Source/Python/GenFds/FdfParser.py
index f8f559aecffd..d954c0b40b3b 100644
--- a/BaseTools/Source/Python/GenFds/FdfParser.py
+++ b/BaseTools/Source/Python/GenFds/FdfParser.py
@@ -129,6 +129,24 @@ class Warning (Exception):
def __str__(self):
return self.Message
+# helper functions to facilitate consistency in warnings
+# each function is for a different common warning
+@staticmethod
+def Expected(Str, File, Line):
+return Warning("expected {}".format(Str), File, Line)
+@staticmethod
+def ExpectedEquals(File, Line):
+return Warning.Expected("'='", File, Line)
+@staticmethod
+def ExpectedCurlyOpen(File, Line):
+return Warning.Expected("'{'", File, Line)
+@staticmethod
+def ExpectedCurlyClose(File, Line):
+return Warning.Expected("'}'", File, Line)
+@staticmethod
+def ExpectedBracketClose(File, Line):
+return Warning.Expected("']'", File, Line)
+
## The Include file content class that used to record file data when parsing
include file
#
# May raise Exception when opening file.
@@ -184,8 +202,6 @@ class IncludeFileProfile:
return (self.FileName, Line - InsertedLines + 1)
-
-
## The FDF content class that used to record file data when parsing FDF
#
# May raise Exception when opening file.
@@ -563,10 +579,10 @@ class FdfParser:
if self._Token == TAB_DEFINE:
if not self._GetNextToken():
-raise Warning("expected Macro name", self.FileName,
self.CurrentLineNumber)
+raise Warning.Expected("Macro name", self.FileName,
self.CurrentLineNumber)
Macro = self._Token
if not self._IsToken(TAB_EQUAL_SPLIT):
-raise Warning("expected '='", self.FileName,
self.CurrentLineNumber)
+raise Warning.ExpectedEquals(self.FileName,
self.CurrentLineNumber)
Value = self._GetExpression()
MacroDict[Macro] = Value
@@ -575,7 +591,7 @@ class FdfParser:
IncludeLine = self.CurrentLineNumber
IncludeOffset = self.CurrentOffsetWithinLine - len(TAB_INCLUDE)
if not self._GetNextToken():
-raise Warning("expected include file name", self.FileName,
self.CurrentLineNumber)
+raise Warning.Expected("include file name", self.FileName,
self.CurrentLineNumber)
IncFileName = self._Token
PreIndex = 0
StartPos = IncFileName.find('$(', PreIndex)
@@ -731,10 +747,10 @@ class FdfParser:
DefineLine = self.CurrentLineNumber - 1
DefineOffset = self.CurrentOffsetWithinLine -
len(TAB_DEFINE)
if not self._GetNextToken():
-raise Warning("expected Macro name", self.FileName,
self.CurrentLineNumber)
+raise Warning.Expected("Macro name", self.FileName,
self.CurrentLineNumber)
Macro = self._Token
if not self._IsToken(TAB_EQUAL_SPLIT):
-raise Warning("expected '='", self.FileName,
self.CurrentLineNumber)
+raise Warning.ExpectedEquals(self.FileName,
self.CurrentLineNumber)
Value = self._GetExpression()
self._SetMacroValue(Macro, Value)
@@ -747,7 +763,7 @@ class FdfParser:
PcdPair = self._GetNextPcdSettings()
PcdName = "%s.%s" % (PcdPair[1], PcdPair[0])
if not self._IsToken(TAB_EQUAL_SPLIT):
-raise Warning("expected '='", self.FileName,
self.CurrentLineNumber)
+raise Warning.ExpectedEquals(self.FileName,
self.CurrentLineNumber)
Value = self._GetExpression()
Value = self._EvaluateConditional(Value,
self.CurrentLineNumber, 'eval', True)
@@ -1179,20 +1195,20 @@ class FdfParser:
def _GetNextPcdSettings(self):
if not self._GetNextWord():
-raise Warning("expected format of
.", self.FileName, self.CurrentLineNumber)
+raise Warning.Expected("", self.FileName,
self.CurrentLineNumber)
pcdTokenSpaceCName = self._Token
if not self._IsToken(TAB_SPLIT):
-raise Warning("expected format of
.", self.FileName, self.CurrentLineNumber)
+raise Warning.Expected(".", self.FileName, self.CurrentLineNumber)
if not self._GetNextWord():
-raise Warning("expected format of
.", self.FileName,
[edk2] [Patch v3 3/6] Basetools/GenFds: refactor class FV
1) initialize UiFvName via __init__ parameter. No change to default behavior.
2) initialize 3 empty lists in __init__. Curently not guarenteed initialized.
Cc: Yonghong Zhu
Cc: Liming Gao
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jaben Carsey
---
BaseTools/Source/Python/GenFds/FdfParser.py | 6 +-
BaseTools/Source/Python/GenFds/Fv.py| 9 +
2 files changed, 6 insertions(+), 9 deletions(-)
diff --git a/BaseTools/Source/Python/GenFds/FdfParser.py
b/BaseTools/Source/Python/GenFds/FdfParser.py
index 8b4d5e7fd863..9e806de0d294 100644
--- a/BaseTools/Source/Python/GenFds/FdfParser.py
+++ b/BaseTools/Source/Python/GenFds/FdfParser.py
@@ -2090,8 +2090,7 @@ class FdfParser:
if not self._IsToken(TAB_SECTION_END):
raise Warning("expected ']'", self.FileName,
self.CurrentLineNumber)
-FvObj = FV()
-FvObj.UiFvName = self.CurrentFvName
+FvObj = FV(Name=self.CurrentFvName)
self.Profile.FvDict[self.CurrentFvName] = FvObj
Status = self._GetCreateFile(FvObj)
@@ -2102,9 +2101,6 @@ class FdfParser:
self._GetAddressStatements(FvObj)
-FvObj.FvExtEntryTypeValue = []
-FvObj.FvExtEntryType = []
-FvObj.FvExtEntryData = []
while True:
self._GetSetStatements(FvObj)
diff --git a/BaseTools/Source/Python/GenFds/Fv.py
b/BaseTools/Source/Python/GenFds/Fv.py
index 30525bd11fcc..d89e7bfbdd6e 100644
--- a/BaseTools/Source/Python/GenFds/Fv.py
+++ b/BaseTools/Source/Python/GenFds/Fv.py
@@ -37,8 +37,8 @@ class FV (object):
#
# @param selfThe object pointer
#
-def __init__(self):
-self.UiFvName = None
+def __init__(self, Name=None):
+self.UiFvName = Name
self.CreateFileName = None
self.BlockSizeList = []
self.DefineVarDict = {}
@@ -61,7 +61,9 @@ class FV (object):
self.FvForceRebase = None
self.FvRegionInFD = None
self.UsedSizeEnable = False
-
+self.FvExtEntryTypeValue = []
+self.FvExtEntryType = []
+self.FvExtEntryData = []
## AddToBuffer()
#
# Generate Fv and add it to the Buffer
@@ -77,7 +79,6 @@ class FV (object):
# @retval string Generated FV file path
#
def AddToBuffer (self, Buffer, BaseAddress=None, BlockSize= None,
BlockNum=None, ErasePloarity='1', VtfDict=None, MacroDict = {}, Flag=False):
-
if BaseAddress is None and self.UiFvName.upper() + 'fv' in
GenFdsGlobalVariable.ImageBinDict:
return GenFdsGlobalVariable.ImageBinDict[self.UiFvName.upper() +
'fv']
--
2.16.2.windows.1
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [Patch v3 0/6] BaseTools/GenFds: cleanup GenFds
Cleanup to many files for GenFds. No command line visible changes are included. 1) refactor imports to reduce namespace clutter. 2) refactor to use existing sharable objects (and create a few new) 3) eliminate shadowing of names 4) remove double underscored private methods for PEP8 5) eliminate unused code/parameters/variables 6) add standard warnings and use them for common code changes from v1: 1) do not shadow CapsuleFV. 2) rebase on master changes from v2: 1) do not delete duplicate function calls. 2) add ".lower()" for GUID string comparison. 3) rebase on master Jaben Carsey (6): BaseTools/GenFds: cleanup GenFds BaseTools/GenFds: change objects to sets Basetools/GenFds: refactor class FV BaseTools/GenFds: remove MacroDict parameter BaseTools/GenFds: refactor FdfParser warnings BaseTools/GenFds: create and use new variable in FdfParser BaseTools/Source/Python/CommonDataClass/FdfClass.py| 73 - BaseTools/Source/Python/Eot/EotMain.py | 372 +- BaseTools/Source/Python/GenFds/AprioriSection.py | 45 +- BaseTools/Source/Python/GenFds/Capsule.py | 26 +- BaseTools/Source/Python/GenFds/CapsuleData.py |1 - BaseTools/Source/Python/GenFds/CompressSection.py |4 +- BaseTools/Source/Python/GenFds/DataSection.py |4 +- BaseTools/Source/Python/GenFds/DepexSection.py |5 +- BaseTools/Source/Python/GenFds/EfiSection.py | 16 +- BaseTools/Source/Python/GenFds/FdfParser.py| 3780 ++-- BaseTools/Source/Python/GenFds/Ffs.py | 82 +- BaseTools/Source/Python/GenFds/FfsFileStatement.py | 37 +- BaseTools/Source/Python/GenFds/FfsInfStatement.py | 10 +- BaseTools/Source/Python/GenFds/Fv.py | 54 +- BaseTools/Source/Python/GenFds/FvImageSection.py |6 +- BaseTools/Source/Python/GenFds/GenFds.py | 160 +- BaseTools/Source/Python/GenFds/GenFdsGlobalVariable.py | 208 +- BaseTools/Source/Python/GenFds/GuidSection.py |4 +- BaseTools/Source/Python/GenFds/OptionRom.py|6 +- BaseTools/Source/Python/GenFds/Region.py | 12 +- BaseTools/Source/Python/GenFds/UiSection.py|4 +- BaseTools/Source/Python/GenFds/VerSection.py | 16 +- BaseTools/Source/Python/GenFds/Vtf.py | 48 +- BaseTools/Source/Python/build/BuildReport.py |5 +- 24 files changed, 2576 insertions(+), 2402 deletions(-) -- 2.16.2.windows.1 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [Patch v3 2/6] BaseTools/GenFds: change objects to sets
Change lists and tuples used solely for "in" testing to sets.
These operations are not order dependent.
fixed some line length for PEP8 compliance on some.
Cc: Liming Gao
Cc: Yonghong Zhu
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jaben Carsey
---
BaseTools/Source/Python/GenFds/FdfParser.py | 222 +++-
1 file changed, 123 insertions(+), 99 deletions(-)
diff --git a/BaseTools/Source/Python/GenFds/FdfParser.py
b/BaseTools/Source/Python/GenFds/FdfParser.py
index 62bf9f14012a..8b4d5e7fd863 100644
--- a/BaseTools/Source/Python/GenFds/FdfParser.py
+++ b/BaseTools/Source/Python/GenFds/FdfParser.py
@@ -70,6 +70,7 @@ SEPARATORS = {TAB_EQUAL_SPLIT, TAB_VALUE_SPLIT,
TAB_COMMA_SPLIT, '{', '}'}
ALIGNMENTS = {"Auto", "8", "16", "32", "64", "128", "512", "1K", "4K", "32K",
"64K", "128K",
"256K", "512K", "1M", "2M", "4M", "8M",
"16M"}
ALIGNMENT_NOAUTO = ALIGNMENTS - {"Auto"}
+CR_LB_SET = {T_CHAR_CR, TAB_LINE_BREAK}
RegionSizePattern =
compile("\s*(?P(?:0x|0X)?[a-fA-F0-9]+)\s*\|\s*(?P(?:0x|0X)?[a-fA-F0-9]+)\s*")
RegionSizeGuidPattern =
compile("\s*(?P\w+\.\w+[\.\w\[\]]*)\s*\|\s*(?P\w+\.\w+[\.\w\[\]]*)\s*")
@@ -266,7 +267,7 @@ class FdfParser:
#
def _SkipWhiteSpace(self):
while not self._EndOfFile():
-if self._CurrentChar() in (TAB_PRINTCHAR_NUL, T_CHAR_CR,
TAB_LINE_BREAK, TAB_SPACE_SPLIT, T_CHAR_TAB):
+if self._CurrentChar() in {TAB_PRINTCHAR_NUL, T_CHAR_CR,
TAB_LINE_BREAK, TAB_SPACE_SPLIT, T_CHAR_TAB}:
self._SkippedChars += str(self._CurrentChar())
self._GetOneChar()
else:
@@ -406,14 +407,14 @@ class FdfParser:
return
Offset = StartPos[1]
-while self.Profile.FileLinesList[StartPos[0]][Offset] not in
(T_CHAR_CR, TAB_LINE_BREAK):
+while self.Profile.FileLinesList[StartPos[0]][Offset] not in CR_LB_SET:
self.Profile.FileLinesList[StartPos[0]][Offset] = Value
Offset += 1
Line = StartPos[0]
while Line < EndPos[0]:
Offset = 0
-while self.Profile.FileLinesList[Line][Offset] not in (T_CHAR_CR,
TAB_LINE_BREAK):
+while self.Profile.FileLinesList[Line][Offset] not in CR_LB_SET:
self.Profile.FileLinesList[Line][Offset] = Value
Offset += 1
Line += 1
@@ -707,7 +708,7 @@ class FdfParser:
PreIndex = 0
StartPos = CurLine.find('$(', PreIndex)
EndPos = CurLine.find(')', StartPos+2)
-while StartPos != -1 and EndPos != -1 and self._Token not
in [TAB_IF_DEF, TAB_IF_N_DEF, TAB_IF, TAB_ELSE_IF]:
+while StartPos != -1 and EndPos != -1 and self._Token not
in {TAB_IF_DEF, TAB_IF_N_DEF, TAB_IF, TAB_ELSE_IF}:
MacroName = CurLine[StartPos+2: EndPos]
MacorValue = self._GetMacroValue(MacroName)
if MacorValue is not None:
@@ -759,7 +760,7 @@ class FdfParser:
self.Profile.PcdFileLineDict[PcdPair] = FileLineTuple
self._WipeOffArea.append(((SetLine, SetOffset),
(self.CurrentLineNumber - 1, self.CurrentOffsetWithinLine - 1)))
-elif self._Token in (TAB_IF_DEF, TAB_IF_N_DEF, TAB_IF):
+elif self._Token in {TAB_IF_DEF, TAB_IF_N_DEF, TAB_IF}:
IfStartPos = (self.CurrentLineNumber - 1,
self.CurrentOffsetWithinLine - len(self._Token))
IfList.append([IfStartPos, None, None])
@@ -777,7 +778,7 @@ class FdfParser:
IfList[-1] = [IfList[-1][0], ConditionSatisfied,
BranchDetermined]
if ConditionSatisfied:
self._WipeOffArea.append((IfList[-1][0],
(self.CurrentLineNumber - 1, self.CurrentOffsetWithinLine - 1)))
-elif self._Token in (TAB_ELSE_IF, '!else'):
+elif self._Token in {TAB_ELSE_IF, TAB_ELSE}:
ElseStartPos = (self.CurrentLineNumber - 1,
self.CurrentOffsetWithinLine - len(self._Token))
if len(IfList) <= 0:
raise Warning("Missing !if statement", self.FileName,
self.CurrentLineNumber)
@@ -860,13 +861,12 @@ class FdfParser:
MacroDict.update(GlobalData.gGlobalDefines)
MacroDict.update(GlobalData.gCommandLineDefines)
-if GlobalData.BuildOptionPcd:
-for Item in GlobalData.BuildOptionPcd:
-if isinstance(Item, tuple):
-continue
-PcdName, TmpValue = Item.split(TAB_EQUAL_SPLIT)
-TmpValue = BuildOptionValue(TmpValue, {})
-MacroDict[PcdName.strip()] = TmpValue
+for Item in GlobalData.BuildOptionPcd:
+if isinstance(Item, tuple):
+continue
+PcdName, TmpValue = Item.split(TAB_EQUAL_SPLIT)
+TmpValue = BuildOptionValue(TmpValue, {})
+
[edk2] [Patch v3 4/6] BaseTools/GenFds: remove MacroDict parameter
The MacroDict parameter goes around in circles through 4 functions without use.
1. GetSectionData calls into GetLeafSection, otherwise doesn’t use MacroDict
2. GetLeafSection calls into GetFileStatement, otherwise doesn’t use MacroDict
3. GetFileStatement calls into GetFilePart, otherwise doesn’t use MacroDict
4. GetFilePart calls into GetSectionData, otherwise doesn’t use MacroDict
Go to 1 and repeat forever.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jaben Carsey
---
BaseTools/Source/Python/GenFds/FdfParser.py | 48 +++-
1 file changed, 16 insertions(+), 32 deletions(-)
diff --git a/BaseTools/Source/Python/GenFds/FdfParser.py
b/BaseTools/Source/Python/GenFds/FdfParser.py
index 9e806de0d294..f8f559aecffd 100644
--- a/BaseTools/Source/Python/GenFds/FdfParser.py
+++ b/BaseTools/Source/Python/GenFds/FdfParser.py
@@ -2113,12 +2113,12 @@ class FdfParser:
if FvObj.FvNameString == 'TRUE' and not FvObj.FvNameGuid:
raise Warning("FvNameString found but FvNameGuid was not found",
self.FileName, self.CurrentLineNumber)
-self._GetAprioriSection(FvObj, FvObj.DefineVarDict.copy())
-self._GetAprioriSection(FvObj, FvObj.DefineVarDict.copy())
+self._GetAprioriSection(FvObj)
+self._GetAprioriSection(FvObj)
while True:
isInf = self._GetInfStatement(FvObj)
-isFile = self._GetFileStatement(FvObj, MacroDict =
FvObj.DefineVarDict.copy())
+isFile = self._GetFileStatement(FvObj)
if not isInf and not isFile:
break
@@ -2353,11 +2353,10 @@ class FdfParser:
#
# @param selfThe object pointer
# @param FvObj for whom apriori is got
-# @param MacroDict dictionary used to replace macro
# @retval TrueSuccessfully find apriori statement
# @retval False Not able to find apriori statement
#
-def _GetAprioriSection(self, FvObj, MacroDict = {}):
+def _GetAprioriSection(self, FvObj):
if not self._IsKeyword("APRIORI"):
return False
@@ -2372,7 +2371,6 @@ class FdfParser:
AprSectionObj.AprioriType = AprType
self._GetDefineStatements(AprSectionObj)
-MacroDict.update(AprSectionObj.DefineVarDict)
while True:
IsInf = self._GetInfStatement(AprSectionObj)
@@ -2526,11 +2524,10 @@ class FdfParser:
#
# @param selfThe object pointer
# @param Obj for whom FILE statement is got
-# @param MacroDict dictionary used to replace macro
# @retval TrueSuccessfully find FILE statement
# @retval False Not able to find FILE statement
#
-def _GetFileStatement(self, Obj, ForCapsule = False, MacroDict = {}):
+def _GetFileStatement(self, Obj, ForCapsule = False):
if not self._IsKeyword("FILE"):
return False
@@ -2561,7 +2558,7 @@ class FdfParser:
FfsFileObj.NameGuid = self._Token
-self._GetFilePart(FfsFileObj, MacroDict.copy())
+self._GetFilePart(FfsFileObj)
if ForCapsule:
capsuleFfs = CapsuleFfs()
@@ -2608,9 +2605,8 @@ class FdfParser:
#
# @param selfThe object pointer
# @param FfsFileObj for whom component is got
-# @param MacroDict dictionary used to replace macro
#
-def _GetFilePart(self, FfsFileObj, MacroDict = {}):
+def _GetFilePart(self, FfsFileObj):
self._GetFileOpts(FfsFileObj)
if not self._IsToken("{"):
@@ -2645,11 +2641,11 @@ class FdfParser:
elif self._Token in {TAB_DEFINE, "APRIORI", "SECTION"}:
self._UndoToken()
-self._GetSectionData(FfsFileObj, MacroDict)
+self._GetSectionData(FfsFileObj)
elif hasattr(FfsFileObj, 'FvFileType') and FfsFileObj.FvFileType ==
'RAW':
self._UndoToken()
-self._GetRAWData(FfsFileObj, MacroDict)
+self._GetRAWData(FfsFileObj)
else:
FfsFileObj.CurrentLineNum = self.CurrentLineNumber
@@ -2666,9 +2662,8 @@ class FdfParser:
#
# @param self The object pointer
# @param FfsFileObj for whom section is got
-# @param MacroDictdictionary used to replace macro
#
-def _GetRAWData(self, FfsFileObj, MacroDict = {}):
+def _GetRAWData(self, FfsFileObj):
FfsFileObj.FileName = []
FfsFileObj.SubAlignment = []
while True:
@@ -2756,26 +2751,18 @@ class FdfParser:
return False
-## _GetFilePart() method
+## _GetSectionData() method
#
# Get section data for FILE statement
#
# @param selfThe object pointer
# @param FfsFileObj for whom section is got
-# @param MacroDict dictionary used to replace macro
#
-def _GetSectionData(self, FfsFileObj, MacroDict = {}):
-Dict = {}
-
Re: [edk2] [PATCH v2 5/5] MdeModulePkg/Core: fix-up for changes introduced by freed-memory guard
On 10/23/18 16:53, Jian J Wang wrote:
>> v2 changes:
>> a. Update IsHeapGuardEnabled() calling because its prototype change
>
> Two changes are fixed up:
> a. Prototype change of function IsHeapGuardEnabled()
This kind of separation, between the patches, is wrong then. If someone
bisects the edk2 git history, and checks out the edk2 tree at patch #4
in this series, they will get a build failure.
> b. More memory map descriptors are introduced by new feature.
>MergeMemoryMap() is updated to merge freed-pages into adjacent memory
>descriptor to reduce the overall descriptor number.
In such cases, the usual way to structure the patch series is as follows:
- First the patch is added that makes the dependent / consumer code more
generic, so that it can later cope with the new feature. Right after
this "prep" patch, the new code paths in the consumer code are not
exercised in practice. Importantly however, there is neither a
compilation failure, nor a functionality error. It's just that the new
additions are not active yet; they work as NOPs.
- Second, the patch with the new feature is added; it basically "snaps
in place", and activates the code paths that were prepared earlier.
In large patch sets, it's not uncommon to see 5+ "prep" patches, each
equipping a separate aspect (or consumer site) for the new feature,
gradually. And then the final feature patch is plugged in.
Thanks
Laszlo
>
> Cc: Star Zeng
> Cc: Michael D Kinney
> Cc: Jiewen Yao
> Cc: Ruiyu Ni
> Cc: Laszlo Ersek
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang
> ---
> MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 2 +-
> MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c | 16 +++-
> 2 files changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> index fa8f8fe91a..6298b67db1 100644
> --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> @@ -1250,7 +1250,7 @@ ApplyMemoryProtectionPolicy (
>// Don't overwrite Guard pages, which should be the first and/or last page,
>// if any.
>//
> - if (IsHeapGuardEnabled ()) {
> + if (IsHeapGuardEnabled (GUARD_HEAP_TYPE_PAGE|GUARD_HEAP_TYPE_POOL)) {
> if (IsGuardPage (Memory)) {
>Memory += EFI_PAGE_SIZE;
>Length -= EFI_PAGE_SIZE;
> diff --git a/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c
> b/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c
> index 05eb4f422b..f6595c90ed 100644
> --- a/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c
> +++ b/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c
> @@ -32,6 +32,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
> EXPRESS OR IMPLIED.
> #include
>
> #include "DxeMain.h"
> +#include "HeapGuard.h"
>
> #define PREVIOUS_MEMORY_DESCRIPTOR(MemoryDescriptor, Size) \
>((EFI_MEMORY_DESCRIPTOR *)((UINT8 *)(MemoryDescriptor) - (Size)))
> @@ -205,16 +206,13 @@ MergeMemoryMap (
> NextMemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry,
> DescriptorSize);
>
> do {
> - MemoryBlockLength = (UINT64) (EfiPagesToSize
> (MemoryMapEntry->NumberOfPages));
> + MergeGuardPages (NewMemoryMapEntry, NextMemoryMapEntry->PhysicalStart);
> + MemoryBlockLength = (UINT64) (EfiPagesToSize
> (NewMemoryMapEntry->NumberOfPages));
>if (((UINTN)NextMemoryMapEntry < (UINTN)MemoryMapEnd) &&
> - (MemoryMapEntry->Type == NextMemoryMapEntry->Type) &&
> - (MemoryMapEntry->Attribute == NextMemoryMapEntry->Attribute) &&
> - ((MemoryMapEntry->PhysicalStart + MemoryBlockLength) ==
> NextMemoryMapEntry->PhysicalStart)) {
> -MemoryMapEntry->NumberOfPages += NextMemoryMapEntry->NumberOfPages;
> -if (NewMemoryMapEntry != MemoryMapEntry) {
> - NewMemoryMapEntry->NumberOfPages +=
> NextMemoryMapEntry->NumberOfPages;
> -}
> -
> + (NewMemoryMapEntry->Type == NextMemoryMapEntry->Type) &&
> + (NewMemoryMapEntry->Attribute == NextMemoryMapEntry->Attribute) &&
> + ((NewMemoryMapEntry->PhysicalStart + MemoryBlockLength) ==
> NextMemoryMapEntry->PhysicalStart)) {
> +NewMemoryMapEntry->NumberOfPages +=
> NextMemoryMapEntry->NumberOfPages;
> NextMemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (NextMemoryMapEntry,
> DescriptorSize);
> continue;
>} else {
>
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2 2/5] UefiCpuPkg/CpuDxe: fix an infinite loop issue
On 10/23/18 16:53, Jian J Wang wrote:
>> v2 changes:
>> a. Change the type of mPageTablePoolLock to be BOOLEAN. Related code
>>logic is also updated and refined.
>> b. Add non-stop mode for freed-memory guard feature
>
> The freed-memory guard feature will cause an infinite calling
> of InitializePageTablePool().
(1) An important statement is missing from the commit message. Namely,
under the UAF guard feature, FreePages() will modify page attributes.
Similarly, the subject line is mostly useless. First, we're not "fixing"
an issue: the issue doesn't exist yet. We're preventing the issue before
the next patches could introduce it. Second, "infinite loop" is way too
generic. Here's a suggestion:
UefiCpuPkg/CpuDxe: prevent inf. recursion from FreePages->SetMemoryAttributes
(77 characters. A bit longer than I'd like (which is 74), but given the
long function names, it's hard to compress down the subject better.
Also, I don't recommend "inf. loop", we have recursion here, not a plain
loop.)
> This is due to a fact that
> AllocateAlignedPages() is used to allocate page table pool memory.
> This function will most likely call gBS->FreePages to free unaligned
> pages and then cause another round of page attributes change, like
> below
>
>FreePages() <===|
> => SetMemoryAttributes() |
> => |
> => InitializePageTablePool() |
> => AllocateAlignedPages() |
> => FreePages() |
>
> The solution is add a global variable as a lock in page table pool
> allocation function and fail any other requests if it has not been
> done.
(2) Please document what the practical consequences are when the global
variable prevents the infinite recursion. As far as I can see, we are
going to propagate various error codes as far as possible outwards.
Questions:
- Will this error reach the caller (for example, a 3rd party UEFI
driver) if it checks the return status of gBS->FreePages()?
- What is the consequence for the UAF guard? Is it only that the freed
pages will not be marked inaccessible, or something more serious (like
some inconsistency in internal platform firmware structures)?
> This patch also add non-stop mode for freed-memory guard.
(3) Please isolate the update to the non-stop macro
(HEAP_GUARD_NONSTOP_MODE) to a separate patch.
- Non-stop mode was introduced / explained in commit 8f2613628acf
("MdeModulePkg/MdeModulePkg.dec: add new settings for PCDs",
2018-08-30).
- The macro HEAP_GUARD_NONSTOP_MODE was added to CpuDxe in commit
dcc026217fdc ("UefiCpuPkg/CpuDxe: implement non-stop mode for uefi",
2018-08-30).
- Another instance of the macro HEAP_GUARD_NONSTOP_MODE was added to
PiSmmCpuDxeSmm -- with BIT1|BIT0 replaced with BIT3|BIT2 -- in commit
09afd9a42a7f ("UefiCpuPkg/PiSmmCpuDxeSmm: implement non-stop mode for
SMM", 2018-08-30)
- According to the first patch in the present series, the UAF guard is
UEFI-only. (Well, "DXE-only" would be more precise; the point is, it
does not cover SMM.)
Thus, the fact that we don't modify the macro definition in
PiSmmCpuDxeSmm, and also *why* we don't do that, should be mentioned
explicitly in the commit message of the patch that modifies
HEAP_GUARD_NONSTOP_MODE in "UefiCpuPkg/CpuDxe/CpuDxe.h":
UefiCpuPkg/CpuDxe: consider Use After Free guard in non-stop mode
(65 characters)
> Cc: Laszlo Ersek
> Cc: Star Zeng
> Cc: Michael D Kinney
> Cc: Jiewen Yao
> Cc: Ruiyu Ni
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang
> ---
> UefiCpuPkg/CpuDxe/CpuDxe.h | 2 +-
> UefiCpuPkg/CpuDxe/CpuPageTable.c | 19 +--
> 2 files changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.h b/UefiCpuPkg/CpuDxe/CpuDxe.h
> index 064ea05bba..3183a3f7f4 100644
> --- a/UefiCpuPkg/CpuDxe/CpuDxe.h
> +++ b/UefiCpuPkg/CpuDxe/CpuDxe.h
> @@ -58,7 +58,7 @@
> )
>
> #define HEAP_GUARD_NONSTOP_MODE \
> -((PcdGet8 (PcdHeapGuardPropertyMask) & (BIT6|BIT1|BIT0)) > BIT6)
> +((PcdGet8 (PcdHeapGuardPropertyMask) & (BIT6|BIT4|BIT1|BIT0)) > BIT6)
>
> #define NULL_DETECTION_NONSTOP_MODE \
> ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & (BIT6|BIT0)) >
> BIT6)
> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> index 33e8ee2d2c..b7beaf935b 100644
> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> @@ -100,6 +100,7 @@ PAGE_ATTRIBUTE_TABLE mPageAttributeTable[] = {
> };
>
> PAGE_TABLE_POOL *mPageTablePool = NULL;
> +BOOLEAN mPageTablePoolLock = FALSE;
> PAGE_TABLE_LIB_PAGING_CONTEXT mPagingContext;
> EFI_SMM_BASE2_PROTOCOL*mSmmBase2 = NULL;
>
> @@ -1046,6 +1047,16 @@ InitializePageTablePool (
>VOID *Buffer;
>BOOLEAN IsModified;
>
> + //
> + // Do not allow re-entrance.
> + //
> + if (mPageTablePoolLock) {
> +
Re: [edk2] [PATCH v2 1/5] MdeModulePkg/MdeModulePkg.dec: update PCD description for new feature
On 10/23/18 16:53, Jian J Wang wrote: >> v2 changes: >> a. Drop PCD PcdUseAfterFreeDetectionPropertyMask. Use BIT4 of >>PcdHeapGuardPropertyMask instead. Update related descriptions in both >>dec and uni files. > > Freed-memory guard is used to detect UAF (Use-After-Free) memory issue > which is illegal access to memory which has been freed. The principle > behind is similar to heap guard feature, that is we'll turn all pool > memory allocation to page allocation and mark them to be not-present > once they are freed. > > This also implies that, once a page is allocated and freed, it cannot > be re-allocated. This will bring another issue, which is that there's > risk that memory space will be used out. To address it, this patch > series add logic put part (at most 64 pages a time) of freed pages > back into page pool, so that the memory service can still have memory > to allocate, when all memory space have been allocated once. This is > called memory promotion. The promoted pages are always from the eldest > pages freed. > > BIT4 of following PCD is used to enable the freed-memory guard feature. > > gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask > > Please note this feature cannot be enabled with heap guard at the same > time. > > Cc: Star Zeng > Cc: Michael D Kinney > Cc: Jiewen Yao > Cc: Ruiyu Ni > Cc: Laszlo Ersek > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Jian J Wang > --- > MdeModulePkg/MdeModulePkg.dec | 10 ++ > MdeModulePkg/MdeModulePkg.uni | 6 +- > 2 files changed, 15 insertions(+), 1 deletion(-) > > diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec > index 6037504fa7..255b92ea67 100644 > --- a/MdeModulePkg/MdeModulePkg.dec > +++ b/MdeModulePkg/MdeModulePkg.dec > @@ -955,6 +955,8 @@ ># free pages for all of them. The page allocation for the type related to ># cleared bits keeps the same as ususal. ># > + # This PCD is only valid if BIT0 and/or BIT2 are set in > PcdHeapGuardPropertyMask. > + # ># Below is bit mask for this PCD: (Order is same as UEFI spec) ># EfiReservedMemoryType 0x0001 ># EfiLoaderCode 0x0002 > @@ -984,6 +986,8 @@ ># if there's enough free memory for all of them. The pool allocation for > the ># type related to cleared bits keeps the same as ususal. ># > + # This PCD is only valid if BIT1 and/or BIT3 are set in > PcdHeapGuardPropertyMask. > + # ># Below is bit mask for this PCD: (Order is same as UEFI spec) ># EfiReservedMemoryType 0x0001 ># EfiLoaderCode 0x0002 > @@ -1007,14 +1011,20 @@ >gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPoolType|0x0|UINT64|0x30001053 > >## This mask is to control Heap Guard behavior. > + # ># Note that due to the limit of pool memory implementation and the > alignment ># requirement of UEFI spec, BIT7 is a try-best setting which cannot > guarantee ># that the returned pool is exactly adjacent to head guard page or tail > guard ># page. (1) The above changes are not related to the use-after-free feature; they should go into a separate cleanup patch. (Which is very welcome otherwise.) The cleanup patch should be patch #1 in the series. The subject should be, for example: MdeModulePkg: clean up Heap Guard PageType / PoolType PCD documentation (71 characters) > + # > + # Note that UEFI freed-memory guard and pool/page guard cannot be enabled > + # at the same time. > + # ># BIT0 - Enable UEFI page guard. ># BIT1 - Enable UEFI pool guard. ># BIT2 - Enable SMM page guard. ># BIT3 - Enable SMM pool guard. > + # BIT4 - Enable UEFI freed-memory guard (Use-After-Free memory > detection). ># BIT6 - Enable non-stop mode. ># BIT7 - The direction of Guard Page for Pool Guard. ># 0 - The returned pool is near the tail guard page. (2) This should go into patch #2 in the series. However, the current subject line is useless: MdeModulePkg/MdeModulePkg.dec: update PCD description for new feature Instead, I suggest: MdeModulePkg: introduce UEFI Use After Free feature bit in Heap Guard PCD (73 characters). > diff --git a/MdeModulePkg/MdeModulePkg.uni b/MdeModulePkg/MdeModulePkg.uni > index a6bcb627cf..e72b893509 100644 > --- a/MdeModulePkg/MdeModulePkg.uni > +++ b/MdeModulePkg/MdeModulePkg.uni > @@ -1171,6 +1171,7 @@ > >" before and after corresponding type of pages allocated if > there's enough\n" > >" free pages for all of them. The page allocation for the type > related to\n" > >" cleared bits keeps the same as ususal.\n\n" > +
[edk2] [PATCH v2 5/5] MdeModulePkg/Core: fix-up for changes introduced by freed-memory guard
> v2 changes:
> a. Update IsHeapGuardEnabled() calling because its prototype change
Two changes are fixed up:
a. Prototype change of function IsHeapGuardEnabled()
b. More memory map descriptors are introduced by new feature.
MergeMemoryMap() is updated to merge freed-pages into adjacent memory
descriptor to reduce the overall descriptor number.
Cc: Star Zeng
Cc: Michael D Kinney
Cc: Jiewen Yao
Cc: Ruiyu Ni
Cc: Laszlo Ersek
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang
---
MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 2 +-
MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c | 16 +++-
2 files changed, 8 insertions(+), 10 deletions(-)
diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
index fa8f8fe91a..6298b67db1 100644
--- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
+++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
@@ -1250,7 +1250,7 @@ ApplyMemoryProtectionPolicy (
// Don't overwrite Guard pages, which should be the first and/or last page,
// if any.
//
- if (IsHeapGuardEnabled ()) {
+ if (IsHeapGuardEnabled (GUARD_HEAP_TYPE_PAGE|GUARD_HEAP_TYPE_POOL)) {
if (IsGuardPage (Memory)) {
Memory += EFI_PAGE_SIZE;
Length -= EFI_PAGE_SIZE;
diff --git a/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c
b/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c
index 05eb4f422b..f6595c90ed 100644
--- a/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c
+++ b/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c
@@ -32,6 +32,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
EXPRESS OR IMPLIED.
#include
#include "DxeMain.h"
+#include "HeapGuard.h"
#define PREVIOUS_MEMORY_DESCRIPTOR(MemoryDescriptor, Size) \
((EFI_MEMORY_DESCRIPTOR *)((UINT8 *)(MemoryDescriptor) - (Size)))
@@ -205,16 +206,13 @@ MergeMemoryMap (
NextMemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry,
DescriptorSize);
do {
- MemoryBlockLength = (UINT64) (EfiPagesToSize
(MemoryMapEntry->NumberOfPages));
+ MergeGuardPages (NewMemoryMapEntry, NextMemoryMapEntry->PhysicalStart);
+ MemoryBlockLength = (UINT64) (EfiPagesToSize
(NewMemoryMapEntry->NumberOfPages));
if (((UINTN)NextMemoryMapEntry < (UINTN)MemoryMapEnd) &&
- (MemoryMapEntry->Type == NextMemoryMapEntry->Type) &&
- (MemoryMapEntry->Attribute == NextMemoryMapEntry->Attribute) &&
- ((MemoryMapEntry->PhysicalStart + MemoryBlockLength) ==
NextMemoryMapEntry->PhysicalStart)) {
-MemoryMapEntry->NumberOfPages += NextMemoryMapEntry->NumberOfPages;
-if (NewMemoryMapEntry != MemoryMapEntry) {
- NewMemoryMapEntry->NumberOfPages +=
NextMemoryMapEntry->NumberOfPages;
-}
-
+ (NewMemoryMapEntry->Type == NextMemoryMapEntry->Type) &&
+ (NewMemoryMapEntry->Attribute == NextMemoryMapEntry->Attribute) &&
+ ((NewMemoryMapEntry->PhysicalStart + MemoryBlockLength) ==
NextMemoryMapEntry->PhysicalStart)) {
+NewMemoryMapEntry->NumberOfPages += NextMemoryMapEntry->NumberOfPages;
NextMemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (NextMemoryMapEntry,
DescriptorSize);
continue;
} else {
--
2.16.2.windows.1
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH v2 4/5] MdeModulePkg/Core: add freed-memory guard feature
> v2 changes:
> a. Change prototype and implementation of IsHeapGuardEnabled()
>to allow it to check freed-memory guard feature.
> b. Drop IsUafEnabled() because of a.
> c. Move the sanity check of freed-memory guard and heap guard
>into HeapGuardCpuArchProtocolNotify()
> d. Add GuardFreedPagesChecked() to avoid duplicate feature check
> e. Coding style cleanup
Freed-memory guard is used to detect UAF (Use-After-Free) memory issue
which is illegal access to memory which has been freed. The principle
behind is similar to heap guard feature, that is we'll turn all pool
memory allocation to page allocation and mark them to be not-present
once they are freed.
This also implies that, once a page is allocated and freed, it cannot
be re-allocated. This will bring another issue, which is that there's
risk that memory space will be used out. To address it, the memory
service add logic to put part (at most 64 pages a time) of freed pages
back into page pool, so that the memory service can still have memory
to allocate, when all memory space have been allocated once. This is
called memory promotion. The promoted pages are always from the eldest
pages which haven been freed.
Cc: Star Zeng
Cc: Michael D Kinney
Cc: Jiewen Yao
Cc: Ruiyu Ni
Cc: Laszlo Ersek
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang
---
MdeModulePkg/Core/Dxe/Mem/HeapGuard.c | 409 +-
MdeModulePkg/Core/Dxe/Mem/HeapGuard.h | 63 +-
MdeModulePkg/Core/Dxe/Mem/Page.c | 41 +++-
MdeModulePkg/Core/Dxe/Mem/Pool.c | 21 +-
4 files changed, 513 insertions(+), 21 deletions(-)
diff --git a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
index 663f969c0d..5666420a6d 100644
--- a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
+++ b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
@@ -44,6 +44,11 @@ GLOBAL_REMOVE_IF_UNREFERENCED UINTN
mLevelShift[GUARDED_HEAP_MAP_TABLE_DEPTH]
GLOBAL_REMOVE_IF_UNREFERENCED UINTN mLevelMask[GUARDED_HEAP_MAP_TABLE_DEPTH]
= GUARDED_HEAP_MAP_TABLE_DEPTH_MASKS;
+//
+// Used for promoting freed but not used pages.
+//
+GLOBAL_REMOVE_IF_UNREFERENCED EFI_PHYSICAL_ADDRESS mLastPromotedPage =
BASE_4GB;
+
/**
Set corresponding bits in bitmap table to 1 according to the address.
@@ -379,7 +384,7 @@ ClearGuardedMemoryBits (
@return An integer containing the guarded memory bitmap.
**/
-UINTN
+UINT64
GetGuardedMemoryBits (
IN EFI_PHYSICAL_ADDRESSAddress,
IN UINTN NumberOfPages
@@ -387,7 +392,7 @@ GetGuardedMemoryBits (
{
UINT64*BitMap;
UINTN Bits;
- UINTN Result;
+ UINT64Result;
UINTN Shift;
UINTN BitsToUnitEnd;
@@ -660,15 +665,16 @@ IsPageTypeToGuard (
/**
Check to see if the heap guard is enabled for page and/or pool allocation.
+ @param[in] GuardType Specify the sub-type(s) of Heap Guard.
+
@return TRUE/FALSE.
**/
BOOLEAN
IsHeapGuardEnabled (
- VOID
+ UINT8 GuardType
)
{
- return IsMemoryTypeToGuard (EfiMaxMemoryType, AllocateAnyPages,
- GUARD_HEAP_TYPE_POOL|GUARD_HEAP_TYPE_PAGE);
+ return IsMemoryTypeToGuard (EfiMaxMemoryType, AllocateAnyPages, GuardType);
}
/**
@@ -1203,6 +1209,380 @@ SetAllGuardPages (
}
}
+/**
+ Find the address of top-most guarded free page.
+
+ @param[out] AddressStart address of top-most guarded free page.
+
+ @return VOID.
+**/
+VOID
+GetLastGuardedFreePageAddress (
+ OUT EFI_PHYSICAL_ADDRESS *Address
+ )
+{
+ EFI_PHYSICAL_ADDRESSAddressGranularity;
+ EFI_PHYSICAL_ADDRESSBaseAddress;
+ UINTN Level;
+ UINT64 Map;
+ INTNIndex;
+
+ ASSERT (mMapLevel >= 1);
+
+ BaseAddress = 0;
+ Map = mGuardedMemoryMap;
+ for (Level = GUARDED_HEAP_MAP_TABLE_DEPTH - mMapLevel;
+ Level < GUARDED_HEAP_MAP_TABLE_DEPTH;
+ ++Level) {
+AddressGranularity = LShiftU64 (1, mLevelShift[Level]);
+
+//
+// Find the non-NULL entry at largest index.
+//
+for (Index = (INTN)mLevelMask[Level]; Index >= 0 ; --Index) {
+ if (((UINT64 *)(UINTN)Map)[Index] != 0) {
+BaseAddress += MultU64x32 (AddressGranularity, (UINT32)Index);
+Map = ((UINT64 *)(UINTN)Map)[Index];
+break;
+ }
+}
+ }
+
+ //
+ // Find the non-zero MSB then get the page address.
+ //
+ while (Map != 0) {
+Map = RShiftU64 (Map, 1);
+BaseAddress += EFI_PAGES_TO_SIZE (1);
+ }
+
+ *Address = BaseAddress;
+}
+
+/**
+ Record freed pages.
+
+ @param[in] BaseAddress Base address of just freed pages.
+ @param[in] Pages Number of freed pages.
+
+ @return VOID.
+**/
+VOID
+MarkFreedPages (
+ IN EFI_PHYSICAL_ADDRESS BaseAddress,
+ IN UINTNPages
+ )
+{
+ SetGuardedMemoryBits (BaseAddress, Pages);
+}
+
+/**
+ Record freed pages as well as mark
[edk2] [PATCH v2 2/5] UefiCpuPkg/CpuDxe: fix an infinite loop issue
> v2 changes:
> a. Change the type of mPageTablePoolLock to be BOOLEAN. Related code
>logic is also updated and refined.
> b. Add non-stop mode for freed-memory guard feature
The freed-memory guard feature will cause an infinite calling
of InitializePageTablePool(). This is due to a fact that
AllocateAlignedPages() is used to allocate page table pool memory.
This function will most likely call gBS->FreePages to free unaligned
pages and then cause another round of page attributes change, like
below
FreePages() <===|
=> SetMemoryAttributes() |
=> |
=> InitializePageTablePool() |
=> AllocateAlignedPages() |
=> FreePages() |
The solution is add a global variable as a lock in page table pool
allocation function and fail any other requests if it has not been
done.
This patch also add non-stop mode for freed-memory guard.
Cc: Laszlo Ersek
Cc: Star Zeng
Cc: Michael D Kinney
Cc: Jiewen Yao
Cc: Ruiyu Ni
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang
---
UefiCpuPkg/CpuDxe/CpuDxe.h | 2 +-
UefiCpuPkg/CpuDxe/CpuPageTable.c | 19 +--
2 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.h b/UefiCpuPkg/CpuDxe/CpuDxe.h
index 064ea05bba..3183a3f7f4 100644
--- a/UefiCpuPkg/CpuDxe/CpuDxe.h
+++ b/UefiCpuPkg/CpuDxe/CpuDxe.h
@@ -58,7 +58,7 @@
)
#define HEAP_GUARD_NONSTOP_MODE \
-((PcdGet8 (PcdHeapGuardPropertyMask) & (BIT6|BIT1|BIT0)) > BIT6)
+((PcdGet8 (PcdHeapGuardPropertyMask) & (BIT6|BIT4|BIT1|BIT0)) > BIT6)
#define NULL_DETECTION_NONSTOP_MODE \
((PcdGet8 (PcdNullPointerDetectionPropertyMask) & (BIT6|BIT0)) > BIT6)
diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c
index 33e8ee2d2c..b7beaf935b 100644
--- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
+++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
@@ -100,6 +100,7 @@ PAGE_ATTRIBUTE_TABLE mPageAttributeTable[] = {
};
PAGE_TABLE_POOL *mPageTablePool = NULL;
+BOOLEAN mPageTablePoolLock = FALSE;
PAGE_TABLE_LIB_PAGING_CONTEXT mPagingContext;
EFI_SMM_BASE2_PROTOCOL*mSmmBase2 = NULL;
@@ -1046,6 +1047,16 @@ InitializePageTablePool (
VOID *Buffer;
BOOLEAN IsModified;
+ //
+ // Do not allow re-entrance.
+ //
+ if (mPageTablePoolLock) {
+return FALSE;
+ }
+
+ mPageTablePoolLock = TRUE;
+ IsModified = FALSE;
+
//
// Always reserve at least PAGE_TABLE_POOL_UNIT_PAGES, including one page for
// header.
@@ -1056,7 +1067,9 @@ InitializePageTablePool (
Buffer = AllocateAlignedPages (PoolPages, PAGE_TABLE_POOL_ALIGNMENT);
if (Buffer == NULL) {
DEBUG ((DEBUG_ERROR, "ERROR: Out of aligned pages\r\n"));
-return FALSE;
+goto Done;
+ } else {
+DEBUG ((DEBUG_INFO, "Paging: added %ld pages to page table pool\r\n",
(UINT64)PoolPages));
}
//
@@ -1092,7 +1105,9 @@ InitializePageTablePool (
);
ASSERT (IsModified == TRUE);
- return TRUE;
+Done:
+ mPageTablePoolLock = FALSE;
+ return IsModified;
}
/**
--
2.16.2.windows.1
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH v2 3/5] MdeModulePkg/Core: fix a lock issue in GCD memory map dump
> v2 changes:
> a. Update implementation of CoreGetMemorySpaceMap() and
>CoreGetIoSpaceMap() to avoid lock failure. Drop the code to
>detect debug print level used to achieve the same effect.
This issue is hidden in current code but exposed by introduction
of freed-memory guard feature due to the fact that the feature
will turn all pool allocation to page allocation.
The solution is move the memory allocation in CoreGetMemorySpaceMap()
and CoreGetIoSpaceMap() to be out of the GCD memory map lock.
CoreDumpGcdMemorySpaceMap()
=> CoreGetMemorySpaceMap()
=> CoreAcquireGcdMemoryLock () *
AllocatePool()
=> InternalAllocatePool()
=> CoreAllocatePool()
=> CoreAllocatePoolI()
=> CoreAllocatePoolPagesI()
=> CoreAllocatePoolPages()
=> FindFreePages()
=> PromoteMemoryResource()
=> CoreAcquireGcdMemoryLock() **
Cc: Star Zeng
Cc: Michael D Kinney
Cc: Jiewen Yao
Cc: Ruiyu Ni
Cc: Laszlo Ersek
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang
---
MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 140
1 file changed, 86 insertions(+), 54 deletions(-)
diff --git a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
index d9c65a8045..133c3dcd87 100644
--- a/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
+++ b/MdeModulePkg/Core/Dxe/Gcd/Gcd.c
@@ -1691,10 +1691,10 @@ CoreGetMemorySpaceMap (
OUT EFI_GCD_MEMORY_SPACE_DESCRIPTOR **MemorySpaceMap
)
{
- EFI_STATUS Status;
LIST_ENTRY *Link;
EFI_GCD_MAP_ENTRY*Entry;
EFI_GCD_MEMORY_SPACE_DESCRIPTOR *Descriptor;
+ UINTNNumber;
//
// Make sure parameters are valid
@@ -1706,38 +1706,54 @@ CoreGetMemorySpaceMap (
return EFI_INVALID_PARAMETER;
}
- CoreAcquireGcdMemoryLock ();
-
//
// Count the number of descriptors
//
- *NumberOfDescriptors = CoreCountGcdMapEntry ();
+ Number = 0;
+ *NumberOfDescriptors = 0;
+ *MemorySpaceMap = NULL;
+ while (TRUE) {
+//
+// Allocate the MemorySpaceMap
+//
+if (Number != 0) {
+ if (*MemorySpaceMap != NULL) {
+FreePool (*MemorySpaceMap);
+ }
- //
- // Allocate the MemorySpaceMap
- //
- *MemorySpaceMap = AllocatePool (*NumberOfDescriptors * sizeof
(EFI_GCD_MEMORY_SPACE_DESCRIPTOR));
- if (*MemorySpaceMap == NULL) {
-Status = EFI_OUT_OF_RESOURCES;
-goto Done;
- }
+ *MemorySpaceMap = AllocatePool (Number *
sizeof(EFI_GCD_MEMORY_SPACE_DESCRIPTOR));
+ if (*MemorySpaceMap == NULL) {
+return EFI_OUT_OF_RESOURCES;
+ }
- //
- // Fill in the MemorySpaceMap
- //
- Descriptor = *MemorySpaceMap;
- Link = mGcdMemorySpaceMap.ForwardLink;
- while (Link != ) {
-Entry = CR (Link, EFI_GCD_MAP_ENTRY, Link, EFI_GCD_MAP_SIGNATURE);
-BuildMemoryDescriptor (Descriptor, Entry);
-Descriptor++;
-Link = Link->ForwardLink;
+ *NumberOfDescriptors = Number;
+}
+
+CoreAcquireGcdMemoryLock ();
+
+Number = CoreCountGcdMapEntry ();
+if (Number == *NumberOfDescriptors) {
+ //
+ // Fill in the MemorySpaceMap
+ //
+ Descriptor = *MemorySpaceMap;
+ Link = mGcdMemorySpaceMap.ForwardLink;
+ while (Link != && Number != 0) {
+Entry = CR (Link, EFI_GCD_MAP_ENTRY, Link, EFI_GCD_MAP_SIGNATURE);
+BuildMemoryDescriptor (Descriptor, Entry);
+Descriptor++;
+Number--;
+Link = Link->ForwardLink;
+ }
+
+ CoreReleaseGcdMemoryLock ();
+ break;
+}
+
+CoreReleaseGcdMemoryLock ();
}
- Status = EFI_SUCCESS;
-Done:
- CoreReleaseGcdMemoryLock ();
- return Status;
+ return EFI_SUCCESS;
}
@@ -1964,10 +1980,10 @@ CoreGetIoSpaceMap (
OUT EFI_GCD_IO_SPACE_DESCRIPTOR **IoSpaceMap
)
{
- EFI_STATUS Status;
LIST_ENTRY *Link;
EFI_GCD_MAP_ENTRY*Entry;
EFI_GCD_IO_SPACE_DESCRIPTOR *Descriptor;
+ UINTNNumber;
//
// Make sure parameters are valid
@@ -1979,38 +1995,54 @@ CoreGetIoSpaceMap (
return EFI_INVALID_PARAMETER;
}
- CoreAcquireGcdIoLock ();
+ Number = 0;
+ *NumberOfDescriptors = 0;
+ *IoSpaceMap = NULL;
+ while (TRUE) {
+//
+// Allocate the IoSpaceMap
+//
+if (Number != 0) {
+ if (*IoSpaceMap != NULL) {
+FreePool (*IoSpaceMap);
+ }
- //
- // Count the number of descriptors
- //
- *NumberOfDescriptors = CoreCountGcdMapEntry ();
+ *IoSpaceMap = AllocatePool (Number *
sizeof(EFI_GCD_IO_SPACE_DESCRIPTOR));
+ if (*IoSpaceMap == NULL) {
+return EFI_OUT_OF_RESOURCES;
+ }
- //
- // Allocate the IoSpaceMap
- //
- *IoSpaceMap = AllocatePool (*NumberOfDescriptors * sizeof
(EFI_GCD_IO_SPACE_DESCRIPTOR));
- if (*IoSpaceMap == NULL) {
-Status = EFI_OUT_OF_RESOURCES;
-goto Done;
- }
+ *NumberOfDescriptors = Number;
+}
- //
- // Fill in the IoSpaceMap
- //
- Descriptor = *IoSpaceMap;
-
[edk2] [PATCH v2 1/5] MdeModulePkg/MdeModulePkg.dec: update PCD description for new feature
> v2 changes: > a. Drop PCD PcdUseAfterFreeDetectionPropertyMask. Use BIT4 of >PcdHeapGuardPropertyMask instead. Update related descriptions in both >dec and uni files. Freed-memory guard is used to detect UAF (Use-After-Free) memory issue which is illegal access to memory which has been freed. The principle behind is similar to heap guard feature, that is we'll turn all pool memory allocation to page allocation and mark them to be not-present once they are freed. This also implies that, once a page is allocated and freed, it cannot be re-allocated. This will bring another issue, which is that there's risk that memory space will be used out. To address it, this patch series add logic put part (at most 64 pages a time) of freed pages back into page pool, so that the memory service can still have memory to allocate, when all memory space have been allocated once. This is called memory promotion. The promoted pages are always from the eldest pages freed. BIT4 of following PCD is used to enable the freed-memory guard feature. gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask Please note this feature cannot be enabled with heap guard at the same time. Cc: Star Zeng Cc: Michael D Kinney Cc: Jiewen Yao Cc: Ruiyu Ni Cc: Laszlo Ersek Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Jian J Wang --- MdeModulePkg/MdeModulePkg.dec | 10 ++ MdeModulePkg/MdeModulePkg.uni | 6 +- 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec index 6037504fa7..255b92ea67 100644 --- a/MdeModulePkg/MdeModulePkg.dec +++ b/MdeModulePkg/MdeModulePkg.dec @@ -955,6 +955,8 @@ # free pages for all of them. The page allocation for the type related to # cleared bits keeps the same as ususal. # + # This PCD is only valid if BIT0 and/or BIT2 are set in PcdHeapGuardPropertyMask. + # # Below is bit mask for this PCD: (Order is same as UEFI spec) # EfiReservedMemoryType 0x0001 # EfiLoaderCode 0x0002 @@ -984,6 +986,8 @@ # if there's enough free memory for all of them. The pool allocation for the # type related to cleared bits keeps the same as ususal. # + # This PCD is only valid if BIT1 and/or BIT3 are set in PcdHeapGuardPropertyMask. + # # Below is bit mask for this PCD: (Order is same as UEFI spec) # EfiReservedMemoryType 0x0001 # EfiLoaderCode 0x0002 @@ -1007,14 +1011,20 @@ gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPoolType|0x0|UINT64|0x30001053 ## This mask is to control Heap Guard behavior. + # # Note that due to the limit of pool memory implementation and the alignment # requirement of UEFI spec, BIT7 is a try-best setting which cannot guarantee # that the returned pool is exactly adjacent to head guard page or tail guard # page. + # + # Note that UEFI freed-memory guard and pool/page guard cannot be enabled + # at the same time. + # # BIT0 - Enable UEFI page guard. # BIT1 - Enable UEFI pool guard. # BIT2 - Enable SMM page guard. # BIT3 - Enable SMM pool guard. + # BIT4 - Enable UEFI freed-memory guard (Use-After-Free memory detection). # BIT6 - Enable non-stop mode. # BIT7 - The direction of Guard Page for Pool Guard. # 0 - The returned pool is near the tail guard page. diff --git a/MdeModulePkg/MdeModulePkg.uni b/MdeModulePkg/MdeModulePkg.uni index a6bcb627cf..e72b893509 100644 --- a/MdeModulePkg/MdeModulePkg.uni +++ b/MdeModulePkg/MdeModulePkg.uni @@ -1171,6 +1171,7 @@ " before and after corresponding type of pages allocated if there's enough\n" " free pages for all of them. The page allocation for the type related to\n" " cleared bits keeps the same as ususal.\n\n" + " This PCD is only valid if BIT0 and/or BIT2 are set in PcdHeapGuardPropertyMask.\n\n" " Below is bit mask for this PCD: (Order is same as UEFI spec)\n" " EfiReservedMemoryType 0x0001\n" " EfiLoaderCode 0x0002\n" @@ -1198,6 +1199,7 @@ " before and after corresponding type of pages which the allocated pool occupies,\n"
[edk2] [PATCH v2 0/5] Add freed-memory guard feature
> v2 changes: > a. Drop PCD PcdUseAfterFreeDetectionPropertyMask. Use BIT4 of >PcdHeapGuardPropertyMask instead. Add more descriptions about >the new usage in dec/uni file as well. > b. Use global of BOOLEAN other than EFI_LOCK to avoid reentrance >of calling InitializePageTablePool() > c. Update implementation of CoreGetMemorySpaceMap() and >CoreGetIoSpaceMap() to avoid lock failure. Drop the code to >detect debug print level used to achieve the same effect. > d. Change prototype and implementation of IsHeapGuardEnabled() >to allow it to check freed-memory guard feature. > e. Move the sanity check of freed-memory guard and heap guard >into HeapGuardCpuArchProtocolNotify() > f. Add GuardFreedPagesChecked() to avoid duplicate feature check > g. Split patch series into smaller patch files Freed-memory guard is a new feauture used to detect UAF (Use-After-Free) memory issue. Jian J Wang (5): MdeModulePkg/MdeModulePkg.dec: update PCD description for new feature UefiCpuPkg/CpuDxe: fix an infinite loop issue MdeModulePkg/Core: fix a lock issue in GCD memory map dump MdeModulePkg/Core: add freed-memory guard feature MdeModulePkg/Core: fix-up for changes introduced by freed-memory guard MdeModulePkg/Core/Dxe/Gcd/Gcd.c | 140 + MdeModulePkg/Core/Dxe/Mem/HeapGuard.c | 409 +- MdeModulePkg/Core/Dxe/Mem/HeapGuard.h | 63 +++- MdeModulePkg/Core/Dxe/Mem/Page.c | 41 ++- MdeModulePkg/Core/Dxe/Mem/Pool.c | 21 +- MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 2 +- MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c | 16 +- MdeModulePkg/MdeModulePkg.dec | 10 + MdeModulePkg/MdeModulePkg.uni | 6 +- UefiCpuPkg/CpuDxe/CpuDxe.h| 2 +- UefiCpuPkg/CpuDxe/CpuPageTable.c | 19 +- 11 files changed, 640 insertions(+), 89 deletions(-) -- 2.16.2.windows.1 ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] OvmfPkg: initialize bochs when initializing vmsvga
On 10/23/18 15:42, Gerd Hoffmann wrote:
> Hi,
>
>> Please help me see the situation better. Here's my current
>> understanding.
>>
>> (1) QemuVideoDxe doesn't set up the VMW SVGA FIFO, and does not store 1
>> to the SVGA_REG_CONFIG_DONE register. And this is not a "small
>> missing step" -- the FIFO setup can be considered a separate
>> feature.
>>
>> (2) We don't intend to implement the FIFO setup feature. (In particular
>> because we don't intend to track changed rectangles and send updates
>> via the FIFO.)
>>
>> (3) The intent of the original VMW SVGA enablement patch for
>> QemuVideoDxe, namely commit c137d9508169, was to enable booting some
>> UEFI operating systems on OVMF that had guest drivers only for VMW
>> SVGA.
>>
>> (4) The QEMU device model now -- since commit 104bd1dc70 -- falls back
>> to stdvga (that is, Bochs) in response to QemuVideoDxe's actions.
>>
>> (5) Your proposal is to set up the Bochs interface in QemuVideoDxe, *in
>> addition* to the -- now dysfunctional! -- VMW SVGA interface.
>>
>> Is my understanding correct?
>
> I understand things the same way.
>
>> So, what do you think of the following approach, instead of your
>> currently proposed patch:
>>
>> - revert commit c137d9508169 ("OvmfPkg/QemuVideoDxe: VMWare SVGA device
>> support", 2017-04-07)
>>
>> - revert commit 05a537945872 ("OvmfPkg/QemuVideoDxe: Helper functions
>> for unaligned port I/O.", 2017-04-07)
>>
>> - given that QEMU provides the Bochs interface anyway, with the VMW SVGA
>> device model, simply recognize the "QEMU VMWare SVGA" card, in the
>> "gQemuVideoCardList" array, as the QEMU_VIDEO_BOCHS_MMIO variant.
>
> Makes sense to me.
Great, thank you.
> Alternatively write a full-blown vmsvga driver which works simliar to
> the virtio-gpu driver. I have my doubts this is worth the effort
> though.
Right, please let us *not* do this.
>
>> Before QEMU commit 104bd1dc70, was it really *required* to use the (now
>> dead) QemuVideoDxe code for VMW SVGA, or would it always have been
>> possible to simply use the Bochs interface, to drive the VMW SVGA
>> device?
>
> I think bochs interface works works with all vmsvga version (in qemu).
Great!
> Didn't test though.
That's fine; I think I haven't tested VMW SVGA, as in "ever". That's up
to people that actually use the device model. As I wrote previously,
it's quite telling that the consequences of QEMU commit 104bd1dc70, from
release v2.10.0, are reported only now, more than a year after the
release -- VMW SVGA must not be a very popular device model.
Yuchenlin, can you then please investigate this approach, including
testing the reverts (the Bochs-only implementation) against QEMU v2.9.1?
(FWIW, upstream QEMU doesn't seem to support earlier releases than
v2.10.2 any longer, according to
Re: [edk2] [PATCH v2 0/3] Add more checker for Tianocompress and Ueficompress(CVE FIX)
Thank you. I will update title when push this change. > -Original Message- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Tuesday, October 23, 2018 6:32 PM > To: Gao, Liming ; edk2-devel@lists.01.org > Subject: Re: [edk2] [PATCH v2 0/3] Add more checker for Tianocompress and > Ueficompress(CVE FIX) > > Hi Liming, > > On 10/22/18 17:18, Liming Gao wrote: > > In V2, update commit message with fixed CVE number. > > > > Fix CVE-2017-5731,CVE-2017-5732,CVE-2017-5733,CVE-2017-5734,CVE-2017-5735 > > https://bugzilla.tianocore.org/show_bug.cgi?id=686 > > > > Liming Gao (3): > > MdePkg: Add more checker in UefiDecompressLib to access the valid > > buffer only(CVE FIX) > > IntelFrameworkModulePkg: Add more checker in > > UefiTianoDecompressLib(CVE FIX) > > BaseTools: Add more checker in Decompress algorithm to access the > > valid buffer(CVE FIX) > > > > BaseTools/Source/C/Common/Decompress.c | 23 +-- > > BaseTools/Source/C/TianoCompress/TianoCompress.c | 26 > > +- > > .../BaseUefiTianoCustomDecompressLib.c | 16 +++-- > > .../BaseUefiDecompressLib/BaseUefiDecompressLib.c | 17 -- > > 4 files changed, 75 insertions(+), 7 deletions(-) > > > > in the subject lines, please add a space character before the string > "(CVE FIX)". This can be done before pushing, of course. > > I haven't reviewed the patches for correctness, but formally, they look > OK to me. I'm ACKing the set to confirm that. Thanks for the commit > message updates. > > Acked-by: Laszlo Ersek > > Laszlo ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] OvmfPkg: initialize bochs when initializing vmsvga
Hi,
> Please help me see the situation better. Here's my current
> understanding.
>
> (1) QemuVideoDxe doesn't set up the VMW SVGA FIFO, and does not store 1
> to the SVGA_REG_CONFIG_DONE register. And this is not a "small
> missing step" -- the FIFO setup can be considered a separate
> feature.
>
> (2) We don't intend to implement the FIFO setup feature. (In particular
> because we don't intend to track changed rectangles and send updates
> via the FIFO.)
>
> (3) The intent of the original VMW SVGA enablement patch for
> QemuVideoDxe, namely commit c137d9508169, was to enable booting some
> UEFI operating systems on OVMF that had guest drivers only for VMW
> SVGA.
>
> (4) The QEMU device model now -- since commit 104bd1dc70 -- falls back
> to stdvga (that is, Bochs) in response to QemuVideoDxe's actions.
>
> (5) Your proposal is to set up the Bochs interface in QemuVideoDxe, *in
> addition* to the -- now dysfunctional! -- VMW SVGA interface.
>
> Is my understanding correct?
I understand things the same way.
> So, what do you think of the following approach, instead of your
> currently proposed patch:
>
> - revert commit c137d9508169 ("OvmfPkg/QemuVideoDxe: VMWare SVGA device
> support", 2017-04-07)
>
> - revert commit 05a537945872 ("OvmfPkg/QemuVideoDxe: Helper functions
> for unaligned port I/O.", 2017-04-07)
>
> - given that QEMU provides the Bochs interface anyway, with the VMW SVGA
> device model, simply recognize the "QEMU VMWare SVGA" card, in the
> "gQemuVideoCardList" array, as the QEMU_VIDEO_BOCHS_MMIO variant.
Makes sense to me.
Alternatively write a full-blown vmsvga driver which works simliar to
the virtio-gpu driver. I have my doubts this is worth the effort
though.
> Before QEMU commit 104bd1dc70, was it really *required* to use the (now
> dead) QemuVideoDxe code for VMW SVGA, or would it always have been
> possible to simply use the Bochs interface, to drive the VMW SVGA
> device?
I think bochs interface works works with all vmsvga version (in qemu).
Didn't test though.
cheers,
Gerd
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] OvmfPkg: initialize bochs when initializing vmsvga
On 10/23/18 13:12, yuchenlin wrote:
> Hi, Laszlo
>
> On 2018-10-23 18:18, Laszlo Ersek wrote:
>> (1) Adding Gerd (because he maintains video in QEMU), and Phil
>> Dennis-Jordan (for authoring commit c137d9508169,
>> "OvmfPkg/QemuVideoDxe: VMWare SVGA device support", 2017-04-07).
>>
>>
>> On 10/23/18 04:40, yuchen...@synology.com wrote:
>>> From: yuchenlin
>>>
>>> When driver doesn't set fifo config, the vmsvga will fall back
>>> to std vga. However, we don't initialize vbe related port. It
>>> causes blank screen in qemu console.
>>
>> (2) The words "when driver doesn't set fifo config" tell me nothing.
>> The QemuVideoDxe directory has zero instances of the word "fifo". The
>> same applies to "OvmfPkg/Include/IndustryStandard/VmwareSvga.h".
>>
>> In addition, the "vmsvga will fall back to std vga" statement is also
>> unclear. Is that a statement about the QEMU device model's behavior?
>>
>
> About FIFO, you can refer to
> https://github.com/prepare/vmware-svga/blob/master/doc/svga_interface.txt#L34,
>
>
> "The main advantage of VMware's SVGA device over alternatives like VBE
> is that the virtual machine can explicitly indicate which ares of the
> screen have changed by sending update rectangles through the device's
> command FIFO."
>
> According to
> https://github.com/prepare/vmware-svga/blob/master/doc/svga_interface.txt#L533,
>
> to use vmsvga's FIFO. Driver should setup FIFO and set
> SVGA_REG_CONFIG_DONE to 1. However, OVMF doesn't do it. In this case,
> vmsvga will fall back to vga. It is QEMU device model's behavior after
> commit 104bd1dc70 (vmsvga: fix vmsvga_update_display).
>
>> I vaguely suspect that your intent might be to say, "QemuVideoDxe
>> does not perform a necessary configuration step, and therefore it
>> cannot drive QEMU's VMW SVGA device". However, if that is indeed your
>> intent, then I believe something must have changed recently in QEMU,
>> because QemuVideoDxe *definitely* worked when Phil contributed the
>> VMW SVGA driver logic, in commit c137d9508169.
>>
>> Are we talking about a QEMU regression, or a driver-side
>> configuration step that QemuVideoDxe has always missed (and we're
>> being punished for it only now)?
>>
>
> This is QEMU behavior change in commit 104bd1dc70 (vmsvga: fix
> vmsvga_update_display). In my opinion, it is a correct change.
>
>>
>>> This patch will fix "Guest has not initialized the display (yet)"
>>> when using qemu -device vmware-svga with ovmf.
>>
>> Right, as I write above, this definitely worked earlier. I suggest
>> bisecting QEMU (and/or testing older QEMU machine types) to identify
>> the QEMU side change. Once we know that, we can decide whether this
>> is a QEMU regression, or just exposing a long-standing OVMF bug.
>
> In my opinion, it is a long-standing OVMF bug, which is based on the
> wrong behavior of QEMU's vmsvga. It relied on dirty memory region to
> call dpy_gfx_update for updating display.
Thank you for the explanation!
Please help me see the situation better. Here's my current
understanding.
(1) QemuVideoDxe doesn't set up the VMW SVGA FIFO, and does not store 1
to the SVGA_REG_CONFIG_DONE register. And this is not a "small
missing step" -- the FIFO setup can be considered a separate
feature.
(2) We don't intend to implement the FIFO setup feature. (In particular
because we don't intend to track changed rectangles and send updates
via the FIFO.)
(3) The intent of the original VMW SVGA enablement patch for
QemuVideoDxe, namely commit c137d9508169, was to enable booting some
UEFI operating systems on OVMF that had guest drivers only for VMW
SVGA.
(4) The QEMU device model now -- since commit 104bd1dc70 -- falls back
to stdvga (that is, Bochs) in response to QemuVideoDxe's actions.
(5) Your proposal is to set up the Bochs interface in QemuVideoDxe, *in
addition* to the -- now dysfunctional! -- VMW SVGA interface.
Is my understanding correct?
Because, here's what I think: the VMW SVGA guest driver logic in
QemuVideoDxe is basically dead code now, since QEMU commit 104bd1dc70.
As I understand it, we don't rely on it *at all*. It does nothing for
us.
(And that has been the case since QEMU v2.10.0, released on 2017-Aug-30.
It's quite telling that the first issue report comes in now, after more
than one year...)
So, what do you think of the following approach, instead of your
currently proposed patch:
- revert commit c137d9508169 ("OvmfPkg/QemuVideoDxe: VMWare SVGA device
support", 2017-04-07)
- revert commit 05a537945872 ("OvmfPkg/QemuVideoDxe: Helper functions
for unaligned port I/O.", 2017-04-07)
- given that QEMU provides the Bochs interface anyway, with the VMW SVGA
device model, simply recognize the "QEMU VMWare SVGA" card, in the
"gQemuVideoCardList" array, as the QEMU_VIDEO_BOCHS_MMIO variant.
Would that work? Because, if our current VMW SVGA driver code is
basically dead, I prefer a solution that simplifies QemuVideoDxe,
instead of complicating it further. And,
Re: [edk2] [PATCH] BaseTools/Ecc: Update a checkpoint criteria.
Reviewed-by: Yonghong Zhu
Best Regards,
Zhu Yonghong
-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Yonghong
Zhu
Sent: Tuesday, October 16, 2018 4:25 PM
To: edk2-devel@lists.01.org
Cc: Chen, Hesheng
Subject: [edk2] [PATCH] BaseTools/Ecc: Update a checkpoint criteria.
From: Hess Chen
Change the criteria of the checkpoint of "#ifndef" to remove the requirement of
prefix '_'.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Hess Chen
---
BaseTools/Source/Python/Ecc/Check.py| 2 +-
BaseTools/Source/Python/Ecc/EccToolError.py | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/BaseTools/Source/Python/Ecc/Check.py
b/BaseTools/Source/Python/Ecc/Check.py
index fc86ad96f2..3bf86b42cd 100644
--- a/BaseTools/Source/Python/Ecc/Check.py
+++ b/BaseTools/Source/Python/Ecc/Check.py
@@ -1374,7 +1374,7 @@ class Check(object):
RecordSet = EccGlobalData.gDb.TblFile.Exec(SqlCommand)
for Record in RecordSet:
Name = Record[1].replace('#ifndef', '').strip()
-if Name[0] != '_' or Name[-1] != '_':
+if Name[-1] != '_':
if not
EccGlobalData.gException.IsException(ERROR_NAMING_CONVENTION_CHECK_IFNDEF_STATEMENT,
Name):
EccGlobalData.gDb.TblReport.Insert(ERROR_NAMING_CONVENTION_CHECK_IFNDEF_STATEMENT,
OtherMsg="The #ifndef name [%s] does not follow the rules" % (Name),
BelongsToTable=FileTable, BelongsToItem=Record[0])
diff --git a/BaseTools/Source/Python/Ecc/EccToolError.py
b/BaseTools/Source/Python/Ecc/EccToolError.py
index ae0a31af8a..e327a7888d 100644
--- a/BaseTools/Source/Python/Ecc/EccToolError.py
+++ b/BaseTools/Source/Python/Ecc/EccToolError.py
@@ -167,7 +167,7 @@ gEccErrorMessage = {
ERROR_NAMING_CONVENTION_CHECK_ALL : "",
ERROR_NAMING_CONVENTION_CHECK_DEFINE_STATEMENT : "Only capital letters are
allowed to be used for #define declarations",
ERROR_NAMING_CONVENTION_CHECK_TYPEDEF_STATEMENT : "Only capital letters
are allowed to be used for typedef declarations",
-ERROR_NAMING_CONVENTION_CHECK_IFNDEF_STATEMENT : "The #ifndef at the start
of an include file should use both prefix and postfix underscore characters,
'_'",
+ERROR_NAMING_CONVENTION_CHECK_IFNDEF_STATEMENT : "The #ifndef at the start
of an include file should use a postfix underscore characters, '_'",
ERROR_NAMING_CONVENTION_CHECK_PATH_NAME : """Path name does not follow the
rules: 1. First character should be upper case 2. Must contain lower case
characters 3. No white space characters""",
ERROR_NAMING_CONVENTION_CHECK_VARIABLE_NAME : """Variable name does not
follow the rules: 1. First character should be upper case 2. Must contain lower
case characters 3. No white space characters 4. Global variable name must start
with a 'g'""",
ERROR_NAMING_CONVENTION_CHECK_FUNCTION_NAME : """Function name does not
follow the rules: 1. First character should be upper case 2. Must contain lower
case characters 3. No white space characters""",
--
2.14.2.windows.2
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] BaseTools/ECC: Fix an identification issue of typedef function.
Reviewed-by: Yonghong Zhu
Best Regards,
Zhu Yonghong
-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Yonghong
Zhu
Sent: Tuesday, October 16, 2018 4:24 PM
To: edk2-devel@lists.01.org
Cc: Chen, Hesheng
Subject: [edk2] [PATCH] BaseTools/ECC: Fix an identification issue of typedef
function.
From: Hess Chen
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Hess Chen
---
BaseTools/Source/Python/Ecc/Check.py | 12 +++-
BaseTools/Source/Python/Ecc/c.py | 8 ++--
2 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/BaseTools/Source/Python/Ecc/Check.py
b/BaseTools/Source/Python/Ecc/Check.py
index 3bf86b42cd..eb086362bd 100644
--- a/BaseTools/Source/Python/Ecc/Check.py
+++ b/BaseTools/Source/Python/Ecc/Check.py
@@ -586,13 +586,23 @@ class Check(object):
if EccGlobalData.gConfig.IncludeFileCheckData == '1' or
EccGlobalData.gConfig.IncludeFileCheckAll == '1' or
EccGlobalData.gConfig.CheckAll == '1':
EdkLogger.quiet("Checking header file data ...")
+# Get all typedef functions
+gAllTypedefFun = []
+for IdentifierTable in EccGlobalData.gIdentifierTableList:
+SqlCommand = """select Name from %s
+where Model = %s """ % (IdentifierTable,
MODEL_IDENTIFIER_TYPEDEF)
+RecordSet = EccGlobalData.gDb.TblFile.Exec(SqlCommand)
+for Record in RecordSet:
+if Record[0].startswith('('):
+gAllTypedefFun.append(Record[0])
+
#for Dirpath, Dirnames, Filenames in self.WalkTree():
#for F in Filenames:
#if os.path.splitext(F)[1] in ('.h'):
#FullName = os.path.join(Dirpath, F)
#MsgList = c.CheckHeaderFileData(FullName)
for FullName in EccGlobalData.gHFileList:
-MsgList = c.CheckHeaderFileData(FullName)
+MsgList = c.CheckHeaderFileData(FullName,
+ gAllTypedefFun)
# Doxygen document checking
def DoxygenCheck(self):
diff --git a/BaseTools/Source/Python/Ecc/c.py b/BaseTools/Source/Python/Ecc/c.py
index 953f1630b6..b8d6adde16 100644
--- a/BaseTools/Source/Python/Ecc/c.py
+++ b/BaseTools/Source/Python/Ecc/c.py
@@ -2144,7 +2144,7 @@ def CheckBooleanValueComparison(FullFileName):
PrintErrorMsg(ERROR_PREDICATE_EXPRESSION_CHECK_BOOLEAN_VALUE, 'Predicate
Expression: %s' % Exp, FileTable, Str[2])
-def CheckHeaderFileData(FullFileName):
+def CheckHeaderFileData(FullFileName, AllTypedefFun=[]):
ErrorMsgList = []
FileID = GetTableID(FullFileName, ErrorMsgList) @@ -2160,7 +2160,11 @@ def
CheckHeaderFileData(FullFileName):
ResultSet = Db.TblFile.Exec(SqlStatement)
for Result in ResultSet:
if not Result[1].startswith('extern'):
-PrintErrorMsg(ERROR_INCLUDE_FILE_CHECK_DATA, 'Variable definition
appears in header file', FileTable, Result[0])
+for Item in AllTypedefFun:
+if '(%s)' % Result[1] in Item:
+break
+else:
+PrintErrorMsg(ERROR_INCLUDE_FILE_CHECK_DATA, 'Variable
+ definition appears in header file', FileTable, Result[0])
SqlStatement = """ select ID
from Function
--
2.14.2.windows.2
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] BaseTools/UPT: Fix an issue of UNI string checking.
Reviewed-by: Yonghong Zhu
Best Regards,
Zhu Yonghong
-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Yonghong
Zhu
Sent: Tuesday, October 16, 2018 4:29 PM
To: edk2-devel@lists.01.org
Cc: Chen, Hesheng
Subject: [edk2] [PATCH] BaseTools/UPT: Fix an issue of UNI string checking.
From: Hess Chen
The tool now can detect the error that the content between double quotes
contains another double quotes or enter key.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Hess Chen
---
.../Source/Python/UPT/Library/UniClassObject.py| 23 ++
1 file changed, 23 insertions(+)
diff --git a/BaseTools/Source/Python/UPT/Library/UniClassObject.py
b/BaseTools/Source/Python/UPT/Library/UniClassObject.py
index 670cf3b4ee..cd575d5a34 100644
--- a/BaseTools/Source/Python/UPT/Library/UniClassObject.py
+++ b/BaseTools/Source/Python/UPT/Library/UniClassObject.py
@@ -566,6 +566,22 @@ class UniFileClassObject(object):
if Line.startswith(u'#language') and len(Line.split()) == 2:
MultiLineFeedExits = True
+#
+# Check the situation that there only has one '"' for the language
entry
+#
+if Line.startswith(u'#string') and Line.find(u'#language') > 0 and
Line.count(u'"') == 1:
+EdkLogger.Error("Unicode File Parser",
ToolError.FORMAT_INVALID,
+ExtraData='''The line %s misses '"' at the end
of it in file %s'''
+% (LineCount, File.Path))
+
+#
+# Check the situation that there has more than 2 '"' for the
language entry
+#
+if Line.startswith(u'#string') and Line.find(u'#language') > 0 and
Line.replace(u'\\"', '').count(u'"') > 2:
+EdkLogger.Error("Unicode File Parser",
ToolError.FORMAT_INVALID,
+ExtraData='''The line %s has more than 2 '"'
for language entry in file %s'''
+% (LineCount, File.Path))
+
#
# Between two String entry, can not contain line feed
#
@@ -727,6 +743,13 @@ class UniFileClassObject(object):
else:
EdkLogger.Error("Unicode File Parser",
ToolError.FORMAT_INVALID, ExtraData=File.Path)
elif Line.startswith(u'"'):
+#
+# Check the situation that there has more than 2 '"' for the
language entry
+#
+if Line.replace(u'\\"', '').count(u'"') > 2:
+EdkLogger.Error("Unicode File Parser",
ToolError.FORMAT_INVALID,
+ExtraData='''The line %s has more than 2
'"' for language entry in file %s'''
+% (LineCount, File.Path))
if u'#string' in Line or u'#language' in Line:
EdkLogger.Error("Unicode File Parser",
ToolError.FORMAT_INVALID, ExtraData=File.Path)
NewLines.append(Line)
--
2.14.2.windows.2
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH V2] BaseTools/ECC: Add a checkpoint to check no usage for deprecated functions.
Reviewed-by: Yonghong Zhu
Best Regards,
Zhu Yonghong
-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Yonghong
Zhu
Sent: Thursday, October 18, 2018 9:20 AM
To: edk2-devel@lists.01.org
Cc: Chen, Hesheng
Subject: [edk2] [PATCH V2] BaseTools/ECC: Add a checkpoint to check no usage
for deprecated functions.
From: Hess Chen
V2: change list to set
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Hess Chen
---
BaseTools/Source/Python/Ecc/Check.py | 60
BaseTools/Source/Python/Ecc/Configuration.py | 3 ++
BaseTools/Source/Python/Ecc/EccToolError.py | 2 +
BaseTools/Source/Python/Ecc/config.ini | 2 +
4 files changed, 67 insertions(+)
diff --git a/BaseTools/Source/Python/Ecc/Check.py
b/BaseTools/Source/Python/Ecc/Check.py
index eb086362bd..d2192d10cb 100644
--- a/BaseTools/Source/Python/Ecc/Check.py
+++ b/BaseTools/Source/Python/Ecc/Check.py
@@ -270,6 +270,66 @@ class Check(object):
self.FunctionLayoutCheckPrototype()
self.FunctionLayoutCheckBody()
self.FunctionLayoutCheckLocalVariable()
+self.FunctionLayoutCheckDeprecated()
+
+# To check if the deprecated functions are used
+def FunctionLayoutCheckDeprecated(self):
+if EccGlobalData.gConfig.CFunctionLayoutCheckNoDeprecated == '1' or
EccGlobalData.gConfig.CFunctionLayoutCheckAll == '1' or
EccGlobalData.gConfig.CheckAll == '1':
+EdkLogger.quiet("Checking function no deprecated one being
+ used ...")
+
+DeprecatedFunctionSet = ('UnicodeValueToString',
+ 'AsciiValueToString',
+ 'StrCpy',
+ 'StrnCpy',
+ 'StrCat',
+ 'StrnCat',
+ 'UnicodeStrToAsciiStr',
+ 'AsciiStrCpy',
+ 'AsciiStrnCpy',
+ 'AsciiStrCat',
+ 'AsciiStrnCat',
+ 'AsciiStrToUnicodeStr',
+ 'PcdSet8',
+ 'PcdSet16',
+ 'PcdSet32',
+ 'PcdSet64',
+ 'PcdSetPtr',
+ 'PcdSetBool',
+ 'PcdSetEx8',
+ 'PcdSetEx16',
+ 'PcdSetEx32',
+ 'PcdSetEx64',
+ 'PcdSetExPtr',
+ 'PcdSetExBool',
+ 'LibPcdSet8',
+ 'LibPcdSet16',
+ 'LibPcdSet32',
+ 'LibPcdSet64',
+ 'LibPcdSetPtr',
+ 'LibPcdSetBool',
+ 'LibPcdSetEx8',
+ 'LibPcdSetEx16',
+ 'LibPcdSetEx32',
+ 'LibPcdSetEx64',
+ 'LibPcdSetExPtr',
+ 'LibPcdSetExBool',
+ 'GetVariable',
+ 'GetEfiGlobalVariable',
+ )
+
+for IdentifierTable in EccGlobalData.gIdentifierTableList:
+SqlCommand = """select ID, Name, BelongsToFile from %s
+where Model = %s """ % (IdentifierTable,
MODEL_IDENTIFIER_FUNCTION_CALLING)
+RecordSet = EccGlobalData.gDb.TblFile.Exec(SqlCommand)
+for Record in RecordSet:
+for Key in DeprecatedFunctionSet:
+if Key == Record[1]:
+if not
EccGlobalData.gException.IsException(ERROR_C_FUNCTION_LAYOUT_CHECK_NO_DEPRECATE,
Key):
+OtherMsg = 'The function [%s] is deprecated
which should NOT be used' % Key
+
EccGlobalData.gDb.TblReport.Insert(ERROR_C_FUNCTION_LAYOUT_CHECK_NO_DEPRECATE,
+
OtherMsg=OtherMsg,
+
BelongsToTable=IdentifierTable,
+
+ BelongsToItem=Record[0])
def WalkTree(self):
IgnoredPattern = c.GetIgnoredDirListPattern() diff --git
a/BaseTools/Source/Python/Ecc/Configuration.py
b/BaseTools/Source/Python/Ecc/Configuration.py
index c19a3990c7..8f6886169c 100644
---
Re: [edk2] [PATCH v1 00/10] UDF: Bugfixes
Paulo and Star, Thanks a lot for the review. Series pushed at 4df8f5bfa2.. 68099b52b0. Best Regards, Hao Wu > -Original Message- > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Wu, > Hao A > Sent: Tuesday, October 23, 2018 2:10 PM > To: Zeng, Star; Paulo Alcantara; edk2-devel@lists.01.org > Cc: Ni, Ruiyu; Yao, Jiewen > Subject: Re: [edk2] [PATCH v1 00/10] UDF: Bugfixes > > > -Original Message- > > From: Zeng, Star > > Sent: Tuesday, October 23, 2018 1:46 PM > > To: Paulo Alcantara; Wu, Hao A; edk2-devel@lists.01.org > > Cc: Ni, Ruiyu; Yao, Jiewen; Zeng, Star > > Subject: Re: [edk2] [PATCH v1 00/10] UDF: Bugfixes > > > > On 2018/10/22 22:39, Paulo Alcantara wrote: > > > Hao Wu writes: > > > > > >> The series will address a couple of bugs within the UDF related codes. > > >> > > >> Please refer to the log message of each commit for more details. > > >> > > >> Cc: Paulo Alcantara > > >> Cc: Ruiyu Ni > > >> Cc: Jiewen Yao > > >> Cc: Star Zeng > > >> > > >> Hao Wu (10): > > >>MdeModulePkg/PartitionDxe: Add check for underlying device block > size > > >>MdeModulePkg/UdfDxe: Refine boundary checks for file/path name > > string > > >>MdeModulePkg/UdfDxe: Add boundary check the read of FE/EFE > > >>MdeModulePkg/UdfDxe: Add boundary check for ComponentIdentifier > > decode > > >>MdeModulePkg/UdfDxe: Add boundary check for getting volume (free) > > size > > >>MdeModulePkg/UdfDxe: Correct behavior for UdfSetPosition() > > >>MdeModulePkg/UdfDxe: Fix a typo within SetFileInfo() > > >>MdeModulePkg/UdfDxe: Update GetInfo() for FS VolumeLabel info > > request > > >>MdeModulePkg/UdfDxe: Add more check when getting PD from LongAd > > >>MdeModulePkg/UdfDxe: Avoid possible use of already-freed data > > >> > > >> MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c| 28 +++ > > >> MdeModulePkg/Universal/Disk/UdfDxe/File.c | 96 > > >> > > >> MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 253 > > ++-- > > >> MdeModulePkg/Universal/Disk/UdfDxe/Udf.h | 63 - > > >> MdeModulePkg/Universal/Disk/UdfDxe/UdfDxe.inf | 1 + > > >> 5 files changed, 362 insertions(+), 79 deletions(-) > > > > > > For the series: > > > > > > Reviewed-by: Paulo Alcantara > > > > I could not review the in detail. > > Thanks Paulo's Reviewed-by. > > > > I have two minor feedback. > > 1. I saw some place using 'basic validations' and some place using > > 'basic validation', should they be aligned? > > 2. I think you can add "Copyright (c) 2018, Intel Corporation. All > > rights reserved." for every file you changed. > > > > Acked-by: Star Zeng > > Thanks. I will address the above 2 points before push. > > Best Regards, > Hao Wu > > > > > Thanks, > > Star > > > > > > > > Thanks! > > > Paulo > > > > > ___ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] OvmfPkg: initialize bochs when initializing vmsvga
Hi, Laszlo On 2018-10-23 18:18, Laszlo Ersek wrote: (1) Adding Gerd (because he maintains video in QEMU), and Phil Dennis-Jordan (for authoring commit c137d9508169, "OvmfPkg/QemuVideoDxe: VMWare SVGA device support", 2017-04-07). On 10/23/18 04:40, yuchen...@synology.com wrote: From: yuchenlin When driver doesn't set fifo config, the vmsvga will fall back to std vga. However, we don't initialize vbe related port. It causes blank screen in qemu console. (2) The words "when driver doesn't set fifo config" tell me nothing. The QemuVideoDxe directory has zero instances of the word "fifo". The same applies to "OvmfPkg/Include/IndustryStandard/VmwareSvga.h". In addition, the "vmsvga will fall back to std vga" statement is also unclear. Is that a statement about the QEMU device model's behavior? About FIFO, you can refer to https://github.com/prepare/vmware-svga/blob/master/doc/svga_interface.txt#L34, "The main advantage of VMware's SVGA device over alternatives like VBE is that the virtual machine can explicitly indicate which ares of the screen have changed by sending update rectangles through the device's command FIFO." According to https://github.com/prepare/vmware-svga/blob/master/doc/svga_interface.txt#L533, to use vmsvga's FIFO. Driver should setup FIFO and set SVGA_REG_CONFIG_DONE to 1. However, OVMF doesn't do it. In this case, vmsvga will fall back to vga. It is QEMU device model's behavior after commit 104bd1dc70 (vmsvga: fix vmsvga_update_display). I vaguely suspect that your intent might be to say, "QemuVideoDxe does not perform a necessary configuration step, and therefore it cannot drive QEMU's VMW SVGA device". However, if that is indeed your intent, then I believe something must have changed recently in QEMU, because QemuVideoDxe *definitely* worked when Phil contributed the VMW SVGA driver logic, in commit c137d9508169. Are we talking about a QEMU regression, or a driver-side configuration step that QemuVideoDxe has always missed (and we're being punished for it only now)? This is QEMU behavior change in commit 104bd1dc70 (vmsvga: fix vmsvga_update_display). In my opinion, it is a correct change. This patch will fix "Guest has not initialized the display (yet)" when using qemu -device vmware-svga with ovmf. Right, as I write above, this definitely worked earlier. I suggest bisecting QEMU (and/or testing older QEMU machine types) to identify the QEMU side change. Once we know that, we can decide whether this is a QEMU regression, or just exposing a long-standing OVMF bug. In my opinion, it is a long-standing OVMF bug, which is based on the wrong behavior of QEMU's vmsvga. It relied on dirty memory region to call dpy_gfx_update for updating display. Comments about the code below: Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: yuchenlin --- OvmfPkg/QemuVideoDxe/Driver.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c index 0dce80e59..255c01881 100644 --- a/OvmfPkg/QemuVideoDxe/Driver.c +++ b/OvmfPkg/QemuVideoDxe/Driver.c @@ -1067,8 +1067,7 @@ InitializeVmwareSvgaGraphicsMode ( VmwareSvgaWrite (Private, VmwareSvgaRegEnable, 1); - SetDefaultPalette (Private); - ClearScreen (Private); + InitializeBochsGraphicsMode (Private, ModeData); } EFI_STATUS (3) Calling InitializeBochsGraphicsMode() from within InitializeVmwareSvgaGraphicsMode() seems wrong, considering the current structure of the driver. We only have InitializeXxxGraphicsMode() calls in QemuVideoGraphicsOutputSetMode(). In order to determine which variant to call, we check the "Private->Variant" field, in a "switch" statement. Therefore: (3a) If a general fallback from "VmwareSvga" to "Bochs" is necessary, then the fallback logic should be added to earlier code that sets the Variant field. You can see an example for that in the QemuVideoControllerDriverStart() function, near the debug message "QemuVideo: No mmio bar, fallback to port io". There the Variant field is degraded from the originally detected QEMU_VIDEO_BOCHS_MMIO value, to QEMU_VIDEO_BOCHS. (3b) Or else, if calling InitializeVmwareSvgaGraphicsMode() is fine as a basis, but we also need the actions of InitializeBochsGraphicsMode() *in addition*, then please: - extract the common actions from InitializeBochsGraphicsMode() to a new helper function, and call the helper from both InitializeBochsGraphicsMode() and InitializeVmwareSvgaGraphicsMode(), - explain, in InitializeVmwareSvgaGraphicsMode(), *why* the Bochs config actions are necessary, in addition to the VmwareSvga actions. Thanks, Laszlo Thanks, yuchenlin ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [Patch] BaseTools: Fix VPD PCD Sub-section display bug
original we get the VPD PCD items from the VPDGuid.map file in the FV
output folder. but this logic doesn't work when 1) there only have
single non Default SKU, 2) there have multiple SKU with same value.
Now we change it to get the really VPD Pcd items that already display
in the PCD section of the report.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Yonghong Zhu
---
BaseTools/Source/Python/build/BuildReport.py | 46 ++--
1 file changed, 17 insertions(+), 29 deletions(-)
diff --git a/BaseTools/Source/Python/build/BuildReport.py
b/BaseTools/Source/Python/build/BuildReport.py
index de8f0fb..ea98ef7 100644
--- a/BaseTools/Source/Python/build/BuildReport.py
+++ b/BaseTools/Source/Python/build/BuildReport.py
@@ -124,10 +124,13 @@ gDriverTypeMap = {
}
## The look up table of the supported opcode in the dependency expression
binaries
gOpCodeList = ["BEFORE", "AFTER", "PUSH", "AND", "OR", "NOT", "TRUE", "FALSE",
"END", "SOR"]
+## Save VPD Pcd
+VPDPcdList = []
+
##
# Writes a string to the file object.
#
# This function writes a string to the file object and a new line is appended
# afterwards. It may optionally wraps the string for better readability.
@@ -1399,10 +1402,13 @@ class PcdReport(object):
FileWrite(File, ' %-*s : %6s %10s = %s' %
(self.MaxLen, ' ', TypeName, '(' + Pcd.DatumType + ')', Value))
else:
FileWrite(File, ' %-*s : %6s %10s %10s = %s'
% (self.MaxLen, ' ', TypeName, '(' + Pcd.DatumType + ')', '(' + SkuIdName +
')', Value))
if TypeName in ('DYNVPD', 'DEXVPD'):
FileWrite(File, '%*s' % (self.MaxLen + 4,
SkuInfo.VpdOffset))
+VPDPcdItem = (Pcd.TokenSpaceGuidCName + '.' +
PcdTokenCName, SkuIdName, SkuInfo.VpdOffset, Pcd.MaxDatumSize,
SkuInfo.DefaultValue)
+if VPDPcdItem not in VPDPcdList:
+VPDPcdList.append(VPDPcdItem)
if IsStructure:
FiledOverrideFlag = False
OverrideValues = Pcd.SkuOverrideValues[Sku]
if OverrideValues:
Keys = OverrideValues.keys()
@@ -2015,39 +2021,18 @@ class FdReport(object):
self.FdName = Fd.FdUiName
self.BaseAddress = Fd.BaseAddress
self.Size = Fd.Size
self.FdRegionList = [FdRegionReport(FdRegion, Wa) for FdRegion in
Fd.RegionList]
self.FvPath = os.path.join(Wa.BuildDir, TAB_FV_DIRECTORY)
-self.VpdFilePath = os.path.join(self.FvPath, "%s.map" %
Wa.Platform.VpdToolGuid)
self.VPDBaseAddress = 0
self.VPDSize = 0
-self.VPDInfoList = []
for index, FdRegion in enumerate(Fd.RegionList):
if str(FdRegion.RegionType) is 'FILE' and Wa.Platform.VpdToolGuid
in str(FdRegion.RegionDataList):
self.VPDBaseAddress = self.FdRegionList[index].BaseAddress
self.VPDSize = self.FdRegionList[index].Size
break
-if os.path.isfile(self.VpdFilePath):
-fd = open(self.VpdFilePath, "r")
-Lines = fd.readlines()
-for Line in Lines:
-Line = Line.strip()
-if len(Line) == 0 or Line.startswith("#"):
-continue
-try:
-PcdName, SkuId, Offset, Size, Value =
Line.split("#")[0].split("|")
-PcdName, SkuId, Offset, Size, Value = PcdName.strip(),
SkuId.strip(), Offset.strip(), Size.strip(), Value.strip()
-if Offset.lower().startswith('0x'):
-Offset = '0x%08X' % (int(Offset, 16) +
self.VPDBaseAddress)
-else:
-Offset = '0x%08X' % (int(Offset, 10) +
self.VPDBaseAddress)
-self.VPDInfoList.append("%s | %s | %s | %s | %s" %
(PcdName, SkuId, Offset, Size, Value))
-except:
-EdkLogger.error("BuildReport", CODE_ERROR, "Fail to parse
VPD information file %s" % self.VpdFilePath)
-fd.close()
-
##
# Generate report for the firmware device.
#
# This function generates report for the firmware device.
#
@@ -2063,27 +2048,30 @@ class FdReport(object):
if len(self.FdRegionList) > 0:
FileWrite(File, gSectionSep)
for FdRegionItem in self.FdRegionList:
FdRegionItem.GenerateReport(File)
-if len(self.VPDInfoList) > 0:
+if VPDPcdList:
+VPDPcdList.sort(key=lambda x: int(x[2], 0))
FileWrite(File, gSubSectionStart)
FileWrite(File, "FD VPD Region")
FileWrite(File, "Base Address: 0x%X" % self.VPDBaseAddress)
FileWrite(File, "Size: 0x%X (%.0fK)" %
(self.VPDSize, self.VPDSize / 1024.0))
Re: [edk2] [PATCH v2 0/3] Add more checker for Tianocompress and Ueficompress(CVE FIX)
Hi Liming, On 10/22/18 17:18, Liming Gao wrote: > In V2, update commit message with fixed CVE number. > > Fix CVE-2017-5731,CVE-2017-5732,CVE-2017-5733,CVE-2017-5734,CVE-2017-5735 > https://bugzilla.tianocore.org/show_bug.cgi?id=686 > > Liming Gao (3): > MdePkg: Add more checker in UefiDecompressLib to access the valid > buffer only(CVE FIX) > IntelFrameworkModulePkg: Add more checker in > UefiTianoDecompressLib(CVE FIX) > BaseTools: Add more checker in Decompress algorithm to access the > valid buffer(CVE FIX) > > BaseTools/Source/C/Common/Decompress.c | 23 +-- > BaseTools/Source/C/TianoCompress/TianoCompress.c | 26 > +- > .../BaseUefiTianoCustomDecompressLib.c | 16 +++-- > .../BaseUefiDecompressLib/BaseUefiDecompressLib.c | 17 -- > 4 files changed, 75 insertions(+), 7 deletions(-) > in the subject lines, please add a space character before the string "(CVE FIX)". This can be done before pushing, of course. I haven't reviewed the patches for correctness, but formally, they look OK to me. I'm ACKing the set to confirm that. Thanks for the commit message updates. Acked-by: Laszlo Ersek Laszlo ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v2] IntelFsp2Pkg: FSP should not override IDT
> On Oct 23, 2018, at 2:33 AM, Chasel, Chiu wrote:
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1265
>
> FSP should not override IDT table when it is initialized
> by boot loader. IDT should be re-initialized in FSP only
> when it is invalid.
> To mitigate temporary memory usage a PCD
> PcdFspMaxInterruptSupported created for platform to decide
> how many interrupts the FSP IDT table can support.
>
> Test: Verified on internal platform and boots successfully.
>
> Cc: Jiewen Yao
> Cc: Desimone Nathaniel L
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Chasel Chiu
> ---
> IntelFsp2Pkg/FspSecCore/FspSecCoreM.inf | 1 +
> IntelFsp2Pkg/FspSecCore/SecMain.c | 24 +++-
> IntelFsp2Pkg/FspSecCore/SecMain.h | 6 ++
> IntelFsp2Pkg/IntelFsp2Pkg.dec | 4
> 4 files changed, 26 insertions(+), 9 deletions(-)
>
> diff --git a/IntelFsp2Pkg/FspSecCore/FspSecCoreM.inf
> b/IntelFsp2Pkg/FspSecCore/FspSecCoreM.inf
> index c61af10b8a..dafe6f5993 100644
> --- a/IntelFsp2Pkg/FspSecCore/FspSecCoreM.inf
> +++ b/IntelFsp2Pkg/FspSecCore/FspSecCoreM.inf
> @@ -62,6 +62,7 @@
> gIntelFsp2PkgTokenSpaceGuid.PcdTemporaryRamSize ## CONSUMES
> gIntelFsp2PkgTokenSpaceGuid.PcdFspTemporaryRamSize ## CONSUMES
> gIntelFsp2PkgTokenSpaceGuid.PcdFspHeapSizePercentage ## CONSUMES
> + gIntelFsp2PkgTokenSpaceGuid.PcdFspMaxInterruptSupported ## CONSUMES
>
> [Ppis]
> gEfiTemporaryRamSupportPpiGuid ## PRODUCES
> diff --git a/IntelFsp2Pkg/FspSecCore/SecMain.c
> b/IntelFsp2Pkg/FspSecCore/SecMain.c
> index 37fd4dfdeb..ddbfc4fcdf 100644
> --- a/IntelFsp2Pkg/FspSecCore/SecMain.c
> +++ b/IntelFsp2Pkg/FspSecCore/SecMain.c
> @@ -70,6 +70,7 @@ SecStartup (
> UINT32 Index;
> FSP_GLOBAL_DATA PeiFspData;
> UINT64 ExceptionHandler;
> + UINTN IdtSize;
>
> //
> // Process all libraries constructor function linked to SecCore.
> @@ -98,13 +99,26 @@ SecStartup (
> // | |
> // |---|> TempRamBase
> IdtTableInStack.PeiService = NULL;
> - ExceptionHandler = FspGetExceptionHandler(mIdtEntryTemplate);
> - for (Index = 0; Index < SEC_IDT_ENTRY_COUNT; Index ++) {
> -CopyMem ((VOID*)[Index],
> (VOID*), sizeof (UINT64));
> + AsmReadIdtr ();
> + if ((IdtDescriptor.Base == 0) && (IdtDescriptor.Limit == 0x)) {
Are these architectural value at reset?
Thanks,
Andrew Fish
> +ExceptionHandler = FspGetExceptionHandler(mIdtEntryTemplate);
> +for (Index = 0; Index < FixedPcdGet8(PcdFspMaxInterruptSupported); Index
> ++) {
> + CopyMem ((VOID*)[Index],
> (VOID*), sizeof (UINT64));
> +}
> +IdtSize = sizeof (IdtTableInStack.IdtTable);
> + } else {
> +if (IdtDescriptor.Limit + 1 > sizeof (IdtTableInStack.IdtTable)) {
> + //
> + // ERROR: IDT table size from boot loader is larger than FSP can
> support, DeadLoop here!
> + //
> + CpuDeadLoop();
> +} else {
> + IdtSize = IdtDescriptor.Limit + 1;
> +}
> +CopyMem ((VOID *) (UINTN) , (VOID *)
> IdtDescriptor.Base, IdtSize);
> }
> -
> IdtDescriptor.Base = (UINTN)
> - IdtDescriptor.Limit = (UINT16)(sizeof (IdtTableInStack.IdtTable) - 1);
> + IdtDescriptor.Limit = (UINT16)(IdtSize - 1);
>
> AsmWriteIdtr ();
>
> diff --git a/IntelFsp2Pkg/FspSecCore/SecMain.h
> b/IntelFsp2Pkg/FspSecCore/SecMain.h
> index 291bc5ca5c..19ac2fbfc1 100644
> --- a/IntelFsp2Pkg/FspSecCore/SecMain.h
> +++ b/IntelFsp2Pkg/FspSecCore/SecMain.h
> @@ -1,6 +1,6 @@
> /** @file
>
> - Copyright (c) 2014 - 2016, Intel Corporation. All rights reserved.
> + Copyright (c) 2014 - 2018, Intel Corporation. All rights reserved.
> This program and the accompanying materials
> are licensed and made available under the terms and conditions of the BSD
> License
> which accompanies this distribution. The full text of the license may be
> found at
> @@ -29,8 +29,6 @@
> #include
> #include
>
> -#define SEC_IDT_ENTRY_COUNT34
> -
> typedef VOID (*PEI_CORE_ENTRY) ( \
> IN CONST EFI_SEC_PEI_HAND_OFF*SecCoreData, \
> IN CONST EFI_PEI_PPI_DESCRIPTOR *PpiList \
> @@ -38,7 +36,7 @@ typedef VOID (*PEI_CORE_ENTRY) ( \
>
> typedef struct _SEC_IDT_TABLE {
> EFI_PEI_SERVICES *PeiService;
> - UINT64IdtTable[SEC_IDT_ENTRY_COUNT];
> + UINT64IdtTable[FixedPcdGet8 (PcdFspMaxInterruptSupported)];
> } SEC_IDT_TABLE;
>
> /**
> diff --git a/IntelFsp2Pkg/IntelFsp2Pkg.dec b/IntelFsp2Pkg/IntelFsp2Pkg.dec
> index 5b037d65e2..50496241da 100644
> --- a/IntelFsp2Pkg/IntelFsp2Pkg.dec
> +++ b/IntelFsp2Pkg/IntelFsp2Pkg.dec
> @@ -86,6 +86,10 @@
> # x % of FSP temporary memory will be used for heap
> # (100 - x) % of FSP temporary memory will be used for stack
> gIntelFsp2PkgTokenSpaceGuid.PcdFspHeapSizePercentage|50|
> UINT8|0x1004
> + #
> + # Maximal
Re: [edk2] [PATCH] OvmfPkg: initialize bochs when initializing vmsvga
(1) Adding Gerd (because he maintains video in QEMU), and Phil Dennis-Jordan (for authoring commit c137d9508169, "OvmfPkg/QemuVideoDxe: VMWare SVGA device support", 2017-04-07). On 10/23/18 04:40, yuchen...@synology.com wrote: > From: yuchenlin > > When driver doesn't set fifo config, the vmsvga will fall back > to std vga. However, we don't initialize vbe related port. It > causes blank screen in qemu console. (2) The words "when driver doesn't set fifo config" tell me nothing. The QemuVideoDxe directory has zero instances of the word "fifo". The same applies to "OvmfPkg/Include/IndustryStandard/VmwareSvga.h". In addition, the "vmsvga will fall back to std vga" statement is also unclear. Is that a statement about the QEMU device model's behavior? I vaguely suspect that your intent might be to say, "QemuVideoDxe does not perform a necessary configuration step, and therefore it cannot drive QEMU's VMW SVGA device". However, if that is indeed your intent, then I believe something must have changed recently in QEMU, because QemuVideoDxe *definitely* worked when Phil contributed the VMW SVGA driver logic, in commit c137d9508169. Are we talking about a QEMU regression, or a driver-side configuration step that QemuVideoDxe has always missed (and we're being punished for it only now)? > This patch will fix "Guest has not initialized the display (yet)" > when using qemu -device vmware-svga with ovmf. Right, as I write above, this definitely worked earlier. I suggest bisecting QEMU (and/or testing older QEMU machine types) to identify the QEMU side change. Once we know that, we can decide whether this is a QEMU regression, or just exposing a long-standing OVMF bug. Comments about the code below: > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: yuchenlin > --- > OvmfPkg/QemuVideoDxe/Driver.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/OvmfPkg/QemuVideoDxe/Driver.c b/OvmfPkg/QemuVideoDxe/Driver.c > index 0dce80e59..255c01881 100644 > --- a/OvmfPkg/QemuVideoDxe/Driver.c > +++ b/OvmfPkg/QemuVideoDxe/Driver.c > @@ -1067,8 +1067,7 @@ InitializeVmwareSvgaGraphicsMode ( > >VmwareSvgaWrite (Private, VmwareSvgaRegEnable, 1); > > - SetDefaultPalette (Private); > - ClearScreen (Private); > + InitializeBochsGraphicsMode (Private, ModeData); > } > > EFI_STATUS > (3) Calling InitializeBochsGraphicsMode() from within InitializeVmwareSvgaGraphicsMode() seems wrong, considering the current structure of the driver. We only have InitializeXxxGraphicsMode() calls in QemuVideoGraphicsOutputSetMode(). In order to determine which variant to call, we check the "Private->Variant" field, in a "switch" statement. Therefore: (3a) If a general fallback from "VmwareSvga" to "Bochs" is necessary, then the fallback logic should be added to earlier code that sets the Variant field. You can see an example for that in the QemuVideoControllerDriverStart() function, near the debug message "QemuVideo: No mmio bar, fallback to port io". There the Variant field is degraded from the originally detected QEMU_VIDEO_BOCHS_MMIO value, to QEMU_VIDEO_BOCHS. (3b) Or else, if calling InitializeVmwareSvgaGraphicsMode() is fine as a basis, but we also need the actions of InitializeBochsGraphicsMode() *in addition*, then please: - extract the common actions from InitializeBochsGraphicsMode() to a new helper function, and call the helper from both InitializeBochsGraphicsMode() and InitializeVmwareSvgaGraphicsMode(), - explain, in InitializeVmwareSvgaGraphicsMode(), *why* the Bochs config actions are necessary, in addition to the VmwareSvga actions. Thanks, Laszlo ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] MdeModulePkg/PiSmmIpl: Do not reset SMRAM to UC when CPU driver runs
On 10/23/18 11:36, Ni, Ruiyu wrote: > On 10/23/2018 11:12 AM, Lohr, Paul A wrote: >> Hello, >> >> Code to remove SMRAM = UC (line 650-ish) looks good. I would suggest >> adding some debug comments in the area it was removed. Thanks. > > debug message or comments? > I guess you'd like to have some comments to say "SMRR is enabled by CPU > SMM driver so no need to reset the SMRAM to UC in MTRR". Correct? Such a comment sounds great to me, just please include: "by calling SmmCpuFeaturesInitializeProcessor from SmmCpuFeaturesLib". [...] Thanks! Laszlo ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] MdeModulePkg/PiSmmIpl: Do not reset SMRAM to UC when CPU driver runs
On 10/23/2018 11:12 AM, Lohr, Paul A wrote: Hello, Code to remove SMRAM = UC (line 650-ish) looks good. I would suggest adding some debug comments in the area it was removed. Thanks. debug message or comments? I guess you'd like to have some comments to say "SMRR is enabled by CPU SMM driver so no need to reset the SMRAM to UC in MTRR". Correct? Per #4, I also "think" the second SMRAM = UC should be removed, in addition to the SMRAM = WB in the same area. But this is under investigation now. Expect an update in 24-48 hours. I did some search in close source platform code and found out that the SMRAM is set to WB by default. So it turns out setting SMRAM to WB is unnecessary here. But this introduces a platform assumption. If platform doesn't set SMRAM to WB, removing the WB set here will cause system boot performance downgrade. Above is my understanding. Please post update here once you finish the investigation. Per #5, I too do not like the "CPU AP" comment, as I initially thought it referred to Application Processor Yes, please clean that AP comment up! Sure I will do that. Paul A. Lohr – Server Firmware Enabling 512.239.9073 (cell) 512.794.5044 (work) ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
[edk2] [PATCH v2] IntelFsp2Pkg: FSP should not override IDT
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1265
FSP should not override IDT table when it is initialized
by boot loader. IDT should be re-initialized in FSP only
when it is invalid.
To mitigate temporary memory usage a PCD
PcdFspMaxInterruptSupported created for platform to decide
how many interrupts the FSP IDT table can support.
Test: Verified on internal platform and boots successfully.
Cc: Jiewen Yao
Cc: Desimone Nathaniel L
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Chasel Chiu
---
IntelFsp2Pkg/FspSecCore/FspSecCoreM.inf | 1 +
IntelFsp2Pkg/FspSecCore/SecMain.c | 24 +++-
IntelFsp2Pkg/FspSecCore/SecMain.h | 6 ++
IntelFsp2Pkg/IntelFsp2Pkg.dec | 4
4 files changed, 26 insertions(+), 9 deletions(-)
diff --git a/IntelFsp2Pkg/FspSecCore/FspSecCoreM.inf
b/IntelFsp2Pkg/FspSecCore/FspSecCoreM.inf
index c61af10b8a..dafe6f5993 100644
--- a/IntelFsp2Pkg/FspSecCore/FspSecCoreM.inf
+++ b/IntelFsp2Pkg/FspSecCore/FspSecCoreM.inf
@@ -62,6 +62,7 @@
gIntelFsp2PkgTokenSpaceGuid.PcdTemporaryRamSize ## CONSUMES
gIntelFsp2PkgTokenSpaceGuid.PcdFspTemporaryRamSize ## CONSUMES
gIntelFsp2PkgTokenSpaceGuid.PcdFspHeapSizePercentage ## CONSUMES
+ gIntelFsp2PkgTokenSpaceGuid.PcdFspMaxInterruptSupported ## CONSUMES
[Ppis]
gEfiTemporaryRamSupportPpiGuid ## PRODUCES
diff --git a/IntelFsp2Pkg/FspSecCore/SecMain.c
b/IntelFsp2Pkg/FspSecCore/SecMain.c
index 37fd4dfdeb..ddbfc4fcdf 100644
--- a/IntelFsp2Pkg/FspSecCore/SecMain.c
+++ b/IntelFsp2Pkg/FspSecCore/SecMain.c
@@ -70,6 +70,7 @@ SecStartup (
UINT32 Index;
FSP_GLOBAL_DATA PeiFspData;
UINT64 ExceptionHandler;
+ UINTN IdtSize;
//
// Process all libraries constructor function linked to SecCore.
@@ -98,13 +99,26 @@ SecStartup (
// | |
// |---|> TempRamBase
IdtTableInStack.PeiService = NULL;
- ExceptionHandler = FspGetExceptionHandler(mIdtEntryTemplate);
- for (Index = 0; Index < SEC_IDT_ENTRY_COUNT; Index ++) {
-CopyMem ((VOID*)[Index],
(VOID*), sizeof (UINT64));
+ AsmReadIdtr ();
+ if ((IdtDescriptor.Base == 0) && (IdtDescriptor.Limit == 0x)) {
+ExceptionHandler = FspGetExceptionHandler(mIdtEntryTemplate);
+for (Index = 0; Index < FixedPcdGet8(PcdFspMaxInterruptSupported); Index
++) {
+ CopyMem ((VOID*)[Index],
(VOID*), sizeof (UINT64));
+}
+IdtSize = sizeof (IdtTableInStack.IdtTable);
+ } else {
+if (IdtDescriptor.Limit + 1 > sizeof (IdtTableInStack.IdtTable)) {
+ //
+ // ERROR: IDT table size from boot loader is larger than FSP can
support, DeadLoop here!
+ //
+ CpuDeadLoop();
+} else {
+ IdtSize = IdtDescriptor.Limit + 1;
+}
+CopyMem ((VOID *) (UINTN) , (VOID *)
IdtDescriptor.Base, IdtSize);
}
-
IdtDescriptor.Base = (UINTN)
- IdtDescriptor.Limit = (UINT16)(sizeof (IdtTableInStack.IdtTable) - 1);
+ IdtDescriptor.Limit = (UINT16)(IdtSize - 1);
AsmWriteIdtr ();
diff --git a/IntelFsp2Pkg/FspSecCore/SecMain.h
b/IntelFsp2Pkg/FspSecCore/SecMain.h
index 291bc5ca5c..19ac2fbfc1 100644
--- a/IntelFsp2Pkg/FspSecCore/SecMain.h
+++ b/IntelFsp2Pkg/FspSecCore/SecMain.h
@@ -1,6 +1,6 @@
/** @file
- Copyright (c) 2014 - 2016, Intel Corporation. All rights reserved.
+ Copyright (c) 2014 - 2018, Intel Corporation. All rights reserved.
This program and the accompanying materials
are licensed and made available under the terms and conditions of the BSD
License
which accompanies this distribution. The full text of the license may be
found at
@@ -29,8 +29,6 @@
#include
#include
-#define SEC_IDT_ENTRY_COUNT34
-
typedef VOID (*PEI_CORE_ENTRY) ( \
IN CONST EFI_SEC_PEI_HAND_OFF*SecCoreData, \
IN CONST EFI_PEI_PPI_DESCRIPTOR *PpiList \
@@ -38,7 +36,7 @@ typedef VOID (*PEI_CORE_ENTRY) ( \
typedef struct _SEC_IDT_TABLE {
EFI_PEI_SERVICES *PeiService;
- UINT64IdtTable[SEC_IDT_ENTRY_COUNT];
+ UINT64IdtTable[FixedPcdGet8 (PcdFspMaxInterruptSupported)];
} SEC_IDT_TABLE;
/**
diff --git a/IntelFsp2Pkg/IntelFsp2Pkg.dec b/IntelFsp2Pkg/IntelFsp2Pkg.dec
index 5b037d65e2..50496241da 100644
--- a/IntelFsp2Pkg/IntelFsp2Pkg.dec
+++ b/IntelFsp2Pkg/IntelFsp2Pkg.dec
@@ -86,6 +86,10 @@
# x % of FSP temporary memory will be used for heap
# (100 - x) % of FSP temporary memory will be used for stack
gIntelFsp2PkgTokenSpaceGuid.PcdFspHeapSizePercentage|50|
UINT8|0x1004
+ #
+ # Maximal Interrupt supported in IDT table.
+ #
+ gIntelFsp2PkgTokenSpaceGuid.PcdFspMaxInterruptSupported |34|
UINT8|0x1005
[PcdsFixedAtBuild,PcdsDynamic,PcdsDynamicEx]
gIntelFsp2PkgTokenSpaceGuid.PcdFspReservedMemoryLength
|0x0010|UINT32|0x4653
--
2.13.3.windows.1
___
Re: [edk2] [PATCH] MdeModulePkg/PiSmmIpl: Do not reset SMRAM to UC when CPU driver runs
On 10/22/2018 10:30 PM, Laszlo Ersek wrote:
On 10/22/18 11:03, Ruiyu Ni wrote:
Today's PiSmmIpl implementation initially sets SMRAM to WB to speed
up the SMM core/modules loading before SMM CPU driver runs.
When SMM CPU driver runs, PiSmmIpl resets the SMRAM to UC. It's done
in SmmIplDxeDispatchEventNotify(). COMM_BUFFER_SMM_DISPATCH_RESTART
is returned from SMM core that SMM CPU driver is just dispatched.
Since now the SMRR is widely used to control the SMRAM cache setting.
It's not needed to reset the SMRAM to UC anymore.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni
Cc: Jiewen Yao
Cc: Michael Kinney
---
MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c | 13 -
1 file changed, 13 deletions(-)
diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
index f8cbe1704b..dc0d9a70b0 100644
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
@@ -672,19 +672,6 @@ SmmIplDxeDispatchEventNotify (
return;
}
-//
-// Attempt to reset SMRAM cacheability to UC
-// Assume CPU AP is available at this time
-//
-Status = gDS->SetMemorySpaceAttributes(
-mSmramCacheBase,
-mSmramCacheSize,
-EFI_MEMORY_UC
-);
-if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_WARN, "SMM IPL failed to reset SMRAM window to
EFI_MEMORY_UC\n"));
-}
-
//
// Close all SMRAM ranges to protect SMRAM
//
I vaguely remember this code from commit b07ea4c198a4 ("MdeModulePkg:
SmmIplEntry(): don't suppress SMM core startup failure", 2015-05-12). So
here I'm asking just out of curiosity -- because SMRR is not necessary
to handle on QEMU/KVM.
I assume that the original UC setting was part of the SMRAM protection
that starts at the next line -- the comment is visible in the context,
saying "Close all SMRAM ranges to protect SMRAM". The commit message
suggests that explicit SMRR management has now taken that role.
(1) Can you add more details to the commit message where that explicit
management happens? I.e., what modules call what library interfaces? I
believe the relevant APIs are from SmmCpuFeaturesLib.
Yes. SmmCpuFeaturesInitializeProcessor() from SmmCpuFeaturesLib enables
SMRR
(2) Also, can you elaborate on "widely"? Does that mean
in most edk2 modules for which SMRAM caching is relevant
or
in most edk2-based platform firmware that is shipped in production
?
Most (ALL maybe) edk2-based platform firmware that is shipped in production.
(3) You forgot to mention the bugzilla in the commit message:
https://bugzilla.tianocore.org/show_bug.cgi?id=1268
(4) From commit b07ea4c198a4 that I mention above, it seems that we have
another instance of this UC setting in the SMM IPL runtime DXE driver;
namely in the SmmIplEntry() function. Should we remove that too?
Now, I understand that the situation at that site is different, because
in that case, we are resetting the cacheability after *failure* to
launch the SMM Core, so there may have been no chance for any module to
manage SMRR. And so we have to roll back our own initial WB setting. Do
I understand that right?
Yes. I don't want to change that piece of code.
(5) I like that this patch removes a "CPU AP" comment, because the
comment is misleading. In this context, "CPU AP" means "CPU
Architectural Protocol" (which underlies
gDS->SetMemorySpaceAttributes()), and not "Application Processor". It's
good that the patch removes such a confusing comment.
However, other potential abuses of the shorthand "CPU AP" remain in the
code. Would you consider submitting a separate patch that replaces all
remaining instances of "CPU AP" with "CPU Arch Protocol", in
"MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c"?
Sure the "AP" also confused me in the beginning. I will post a separate
patch to change it.
... I originally intended to test this patch at once, but given my
question (4), you might want to modify the code as well, not just the
commit message, and so I figured I'd postpone the testing until your answer.
Let me check Paul's mail then decide what to do next.
Thanks!
Laszlo
--
Thanks,
Ray
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v1 1/3] MdeModulePkg/NvmExpressDxe: Refine data buffer & len check in PassThru
On 10/18/2018 2:41 PM, Hao Wu wrote:
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1142
According to the the NVM Express spec Revision 1.1, for some commands
(like Get/Set Feature Command, Figure 89 & 90 of the spec), the Memory
Buffer maybe optional although the command opcode indicates there is a
data transfer between host & controller (Get/Set Feature Command, Figure
38 of the spec).
Hence, this commit refine the checks for the 'TransferLength' and
'TransferBuffer' field of the EFI_NVM_EXPRESS_PASS_THRU_COMMAND_PACKET
structure to address this issue.
Cc: Liangcheng Tang
Cc: Ruiyu Ni
Cc: Star Zeng
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu
---
MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c | 33
+++-
1 file changed, 18 insertions(+), 15 deletions(-)
diff --git a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
index 2468871322..bfcd349794 100644
--- a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
+++ b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
@@ -595,7 +595,8 @@ NvmExpressPassThru (
//
if (((Sq->Opc & (BIT0 | BIT1)) != 0) &&
!((Packet->QueueType == NVME_ADMIN_QUEUE) && ((Sq->Opc ==
NVME_ADMIN_CRIOCQ_CMD) || (Sq->Opc == NVME_ADMIN_CRIOSQ_CMD {
-if ((Packet->TransferLength == 0) || (Packet->TransferBuffer == NULL)) {
+if (((Packet->TransferLength != 0) && (Packet->TransferBuffer == NULL)) ||
+((Packet->TransferLength == 0) && (Packet->TransferBuffer != NULL))) {
return EFI_INVALID_PARAMETER;
}
@@ -605,21 +606,23 @@ NvmExpressPassThru (
Flag = EfiPciIoOperationBusMasterWrite;
}
-MapLength = Packet->TransferLength;
-Status = PciIo->Map (
- PciIo,
- Flag,
- Packet->TransferBuffer,
- ,
- ,
-
- );
-if (EFI_ERROR (Status) || (Packet->TransferLength != MapLength)) {
- return EFI_OUT_OF_RESOURCES;
-}
+if ((Packet->TransferLength != 0) && (Packet->TransferBuffer != NULL)) {
+ MapLength = Packet->TransferLength;
+ Status = PciIo->Map (
+PciIo,
+Flag,
+Packet->TransferBuffer,
+,
+,
+
+);
+ if (EFI_ERROR (Status) || (Packet->TransferLength != MapLength)) {
+return EFI_OUT_OF_RESOURCES;
+ }
-Sq->Prp[0] = PhyAddr;
-Sq->Prp[1] = 0;
+ Sq->Prp[0] = PhyAddr;
+ Sq->Prp[1] = 0;
+}
if((Packet->MetadataLength != 0) && (Packet->MetadataBuffer != NULL)) {
MapLength = Packet->MetadataLength;
Reviewed-by: Ruiyu Ni
--
Thanks,
Ray
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v1 2/3] MdeModulePkg/NvmExpressDxe: Always copy CQ entry to PassThru packet
On 10/18/2018 2:41 PM, Hao Wu wrote:
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1259
According to the the NVM Express spec Revision 1.1, for some commands,
command-related information will be stored in the Dword 0 of the
completion queue entry.
One case is for the Get Features Command (Section 5.9.2 of the spec),
Dword 0 of the completion queue entry may contain feature information.
Hence, this commit will always copy the content of completion queue entry
to the PassThru packet regardless of the execution result of the command.
Cc: Liangcheng Tang
Cc: Ruiyu Ni
Cc: Star Zeng
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu
---
MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c | 9 -
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
index bfcd349794..c52e960771 100644
--- a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
+++ b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
@@ -781,17 +781,16 @@ NvmExpressPassThru (
} else {
Status = EFI_DEVICE_ERROR;
//
- // Copy the Respose Queue entry for this command to the callers response
buffer
- //
- CopyMem(Packet->NvmeCompletion, Cq, sizeof(EFI_NVM_EXPRESS_COMPLETION));
-
- //
// Dump every completion entry status for debugging.
//
DEBUG_CODE_BEGIN();
NvmeDumpStatus(Cq);
DEBUG_CODE_END();
}
+//
+// Copy the Respose Queue entry for this command to the callers response
buffer
+//
+CopyMem(Packet->NvmeCompletion, Cq, sizeof(EFI_NVM_EXPRESS_COMPLETION));
} else {
//
// Timeout occurs for an NVMe command. Reset the controller to abort the
Reviewed-by: Ruiyu Ni
--
Thanks,
Ray
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v1 3/3] MdeModulePkg/NvmExpressDxe: Refine PassThru IO queue creation behavior
On 10/18/2018 2:42 PM, Hao Wu wrote:
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=1260
For the PassThru() service of NVM Express Pass Through Protocol, the
current implementation (function NvmExpressPassThru()) will only use the
IO Completion/Submission queues created internally by this driver during
the controller initialization process. Any other IO queues created will
not be consumed.
So the value is little to accept external IO Completion/Submission queue
creation request. This commit will refine the behavior of function
NvmExpressPassThru(), it will only accept driver internal IO queue
creation commands and will return "EFI_UNSUPPORTED" for external ones.
Cc: Jiewen Yao
Cc: Ruiyu Ni
Cc: Star Zeng
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu
---
MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c | 42
1 file changed, 34 insertions(+), 8 deletions(-)
diff --git a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
index c52e960771..0c550bd52c 100644
--- a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
+++ b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressPassthru.c
@@ -476,6 +476,7 @@ NvmExpressPassThru (
UINT32 Data;
NVME_PASS_THRU_ASYNC_REQ *AsyncRequest;
EFI_TPLOldTpl;
+ UINT32 CrIoQid;
//
// check the data fields in Packet parameter.
@@ -587,14 +588,39 @@ NvmExpressPassThru (
}
Sq->Prp[0] = (UINT64)(UINTN)Packet->TransferBuffer;
- //
- // If the NVMe cmd has data in or out, then mapping the user buffer to the
PCI controller specific addresses.
- // Note here we don't handle data buffer for CreateIOSubmitionQueue and
CreateIOCompletionQueue cmds because
- // these two cmds are special which requires their data buffer must support
simultaneous access by both the
- // processor and a PCI Bus Master. It's caller's responsbility to ensure
this.
- //
- if (((Sq->Opc & (BIT0 | BIT1)) != 0) &&
- !((Packet->QueueType == NVME_ADMIN_QUEUE) && ((Sq->Opc ==
NVME_ADMIN_CRIOCQ_CMD) || (Sq->Opc == NVME_ADMIN_CRIOSQ_CMD {
+ if ((Packet->QueueType == NVME_ADMIN_QUEUE) &&
+ ((Sq->Opc == NVME_ADMIN_CRIOCQ_CMD) || (Sq->Opc ==
NVME_ADMIN_CRIOSQ_CMD))) {
+//
+// Command Dword 10 should be valid for CreateIOCompletionQueue and
+// CreateIOSubmissionQueue commands.
+//
+if (!(Packet->NvmeCmd->Flags & CDW10_VALID)) {
+ return EFI_INVALID_PARAMETER;
+}
+
+//
+// Bits 15:0 of Command Dword 10 is the Queue Identifier (QID) for
+// CreateIOCompletionQueue and CreateIOSubmissionQueue commands.
+//
+CrIoQid = Packet->NvmeCmd->Cdw10 & 0x;
+
+//
+// Currently, we only use the IO Completion/Submission queues created
internally
+// by this driver during controller initialization. Any other IO queues
created
+// will not be consumed here. The value is little to accept external IO
queue
+// creation requests, so here we will return EFI_UNSUPPORTED for external
IO
+// queue creation request.
+//
+if ((CrIoQid >= NVME_MAX_QUEUES) ||
+((Sq->Opc == NVME_ADMIN_CRIOCQ_CMD) && (Packet->TransferBuffer !=
Private->CqBufferPciAddr[CrIoQid])) ||
+((Sq->Opc == NVME_ADMIN_CRIOSQ_CMD) && (Packet->TransferBuffer !=
Private->SqBufferPciAddr[CrIoQid]))) {
+ DEBUG ((DEBUG_ERROR, "NvmExpressPassThru: Does not support external IO queues
creation request.\n"));
+ return EFI_UNSUPPORTED;
+}
Does the above check is to make sure only accept IO queues creation
request from NvmeCreateIoCompletionQueue() and
NvmeCreateIoSubmissionQueue()?
The check is very complex and unnecessary IMO. If we introduce a boolean
field like CreateQueue in the Private structure and set it to TRUE when
calling PassThru() from the above two internal functions, the above
check can be replaced with only one line check:
if (Private->CreateQueue)
It does introduce a "dirty" flag in Private structure, but the above
complex check can be avoided.
What's your opinion?
+ } else if ((Sq->Opc & (BIT0 | BIT1)) != 0) {
+//
+// If the NVMe cmd has data in or out, then mapping the user buffer to the
PCI controller specific addresses.
+//
if (((Packet->TransferLength != 0) && (Packet->TransferBuffer == NULL)) ||
((Packet->TransferLength == 0) && (Packet->TransferBuffer != NULL))) {
return EFI_INVALID_PARAMETER;
--
Thanks,
Ray
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v1 00/10] UDF: Bugfixes
> -Original Message- > From: Zeng, Star > Sent: Tuesday, October 23, 2018 1:46 PM > To: Paulo Alcantara; Wu, Hao A; edk2-devel@lists.01.org > Cc: Ni, Ruiyu; Yao, Jiewen; Zeng, Star > Subject: Re: [edk2] [PATCH v1 00/10] UDF: Bugfixes > > On 2018/10/22 22:39, Paulo Alcantara wrote: > > Hao Wu writes: > > > >> The series will address a couple of bugs within the UDF related codes. > >> > >> Please refer to the log message of each commit for more details. > >> > >> Cc: Paulo Alcantara > >> Cc: Ruiyu Ni > >> Cc: Jiewen Yao > >> Cc: Star Zeng > >> > >> Hao Wu (10): > >>MdeModulePkg/PartitionDxe: Add check for underlying device block size > >>MdeModulePkg/UdfDxe: Refine boundary checks for file/path name > string > >>MdeModulePkg/UdfDxe: Add boundary check the read of FE/EFE > >>MdeModulePkg/UdfDxe: Add boundary check for ComponentIdentifier > decode > >>MdeModulePkg/UdfDxe: Add boundary check for getting volume (free) > size > >>MdeModulePkg/UdfDxe: Correct behavior for UdfSetPosition() > >>MdeModulePkg/UdfDxe: Fix a typo within SetFileInfo() > >>MdeModulePkg/UdfDxe: Update GetInfo() for FS VolumeLabel info > request > >>MdeModulePkg/UdfDxe: Add more check when getting PD from LongAd > >>MdeModulePkg/UdfDxe: Avoid possible use of already-freed data > >> > >> MdeModulePkg/Universal/Disk/PartitionDxe/Udf.c| 28 +++ > >> MdeModulePkg/Universal/Disk/UdfDxe/File.c | 96 > >> MdeModulePkg/Universal/Disk/UdfDxe/FileSystemOperations.c | 253 > ++-- > >> MdeModulePkg/Universal/Disk/UdfDxe/Udf.h | 63 - > >> MdeModulePkg/Universal/Disk/UdfDxe/UdfDxe.inf | 1 + > >> 5 files changed, 362 insertions(+), 79 deletions(-) > > > > For the series: > > > > Reviewed-by: Paulo Alcantara > > I could not review the in detail. > Thanks Paulo's Reviewed-by. > > I have two minor feedback. > 1. I saw some place using 'basic validations' and some place using > 'basic validation', should they be aligned? > 2. I think you can add "Copyright (c) 2018, Intel Corporation. All > rights reserved." for every file you changed. > > Acked-by: Star Zeng Thanks. I will address the above 2 points before push. Best Regards, Hao Wu > > Thanks, > Star > > > > > Thanks! > > Paulo > > ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH] IntelFsp2Pkg: FSP should not override IDT
Hi Chasel
Good idea to reserve bootloader IDT.
I have a little concern on the code below. It unconditionally limits the boot
loader IDT to our IdtTableInStack.
#define SEC_IDT_ENTRY_COUNT34
typedef struct _SEC_IDT_TABLE {
EFI_PEI_SERVICES *PeiService;
UINT64IdtTable[SEC_IDT_ENTRY_COUNT];
} SEC_IDT_TABLE;
Is that right assumption that 34 entries is enough?
Can we enlarge the IdtTableInStack to hold or bootloader IDT?
Or can we enlarge IdtTableInStack to hold all 0x100 entries, and use ASSERT for
that?
Silence failure seems not the best choice.
> +// Get minimum size
> +if (IdtDescriptor.Limit + 1 > sizeof (IdtTableInStack.IdtTable)) {
> + IdtSize = sizeof (IdtTableInStack.IdtTable);
> +} else {
Thank you
Yao Jiewen
> -Original Message-
> From: Chiu, Chasel
> Sent: Friday, October 19, 2018 5:43 PM
> To: edk2-devel@lists.01.org
> Cc: Yao, Jiewen ; Desimone, Nathaniel L
> ; Chiu, Chasel
> Subject: [PATCH] IntelFsp2Pkg: FSP should not override IDT
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1265
>
> FSP should not override IDT table when it is initialized
> by boot loader. IDT should be re-initialized in FSP only
> when it is invalid.
>
> Test: Verified on internal platform and boots successfully.
>
> Cc: Jiewen Yao
> Cc: Desimone Nathaniel L
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Chasel Chiu
> ---
> IntelFsp2Pkg/FspSecCore/SecMain.c | 22 +-
> 1 file changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/IntelFsp2Pkg/FspSecCore/SecMain.c
> b/IntelFsp2Pkg/FspSecCore/SecMain.c
> index 37fd4dfdeb..5912221204 100644
> --- a/IntelFsp2Pkg/FspSecCore/SecMain.c
> +++ b/IntelFsp2Pkg/FspSecCore/SecMain.c
> @@ -70,6 +70,7 @@ SecStartup (
>UINT32 Index;
>FSP_GLOBAL_DATA PeiFspData;
>UINT64 ExceptionHandler;
> + UINTN IdtSize;
>
>//
>// Process all libraries constructor function linked to SecCore.
> @@ -98,13 +99,24 @@ SecStartup (
>// | |
>// |---|> TempRamBase
>IdtTableInStack.PeiService = NULL;
> - ExceptionHandler = FspGetExceptionHandler(mIdtEntryTemplate);
> - for (Index = 0; Index < SEC_IDT_ENTRY_COUNT; Index ++) {
> -CopyMem ((VOID*)[Index],
> (VOID*), sizeof (UINT64));
> + AsmReadIdtr ();
> + if ((IdtDescriptor.Base == 0) && (IdtDescriptor.Limit == 0x)) {
> +ExceptionHandler = FspGetExceptionHandler(mIdtEntryTemplate);
> +for (Index = 0; Index < SEC_IDT_ENTRY_COUNT; Index ++) {
> + CopyMem ((VOID*)[Index],
> (VOID*), sizeof (UINT64));
> +}
> +IdtSize = sizeof (IdtTableInStack.IdtTable);
> + } else {
> +// Get minimum size
> +if (IdtDescriptor.Limit + 1 > sizeof (IdtTableInStack.IdtTable)) {
> + IdtSize = sizeof (IdtTableInStack.IdtTable);
> +} else {
> + IdtSize = IdtDescriptor.Limit + 1;
> +}
> +CopyMem ((VOID *) (UINTN) , (VOID *)
> IdtDescriptor.Base, IdtSize);
>}
> -
>IdtDescriptor.Base = (UINTN)
> - IdtDescriptor.Limit = (UINT16)(sizeof (IdtTableInStack.IdtTable) - 1);
> + IdtDescriptor.Limit = (UINT16)(IdtSize - 1);
>
>AsmWriteIdtr ();
>
> --
> 2.13.3.windows.1
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
