* [PATCH 01/11] OvmfPkg/BaseMemEncryptSevLib: unify encrypt/decrypt DEBUG messages
2017-08-30 15:53 [PATCH 00/11] OvmfPkg: improve SEV-related DEBUG messages Laszlo Ersek
@ 2017-08-30 15:53 ` Laszlo Ersek
2017-08-30 15:53 ` [PATCH 02/11] OvmfPkg/BaseMemEncryptSevLib: break DEBUG calls to multiple lines Laszlo Ersek
` (10 subsequent siblings)
11 siblings, 0 replies; 14+ messages in thread
From: Laszlo Ersek @ 2017-08-30 15:53 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Brijesh Singh, Jordan Justen
Unify the debug messages between InternalMemEncryptSevSetMemoryEncrypted()
and InternalMemEncryptSevSetMemoryDecrypted().
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c | 20 ++++++++++++--------
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c
index 7cbbf915f443..dbaad7766dbe 100644
--- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c
+++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c
@@ -239,6 +239,18 @@ SetMemoryEncDec (
UINT64 PgTableMask;
UINT64 AddressEncMask;
+ DEBUG ((
+ DEBUG_VERBOSE,
+ "%a:%a: Cr3Base=0x%Lx Physical=0x%Lx Length=0x%Lx Mode=%a CacheFlush=%u\n",
+ gEfiCallerBaseName,
+ __FUNCTION__,
+ Cr3BaseAddress,
+ PhysicalAddress,
+ (UINT64)Length,
+ (Mode == SetCBit) ? "Encrypt" : "Decrypt",
+ (UINT32)CacheFlush
+ ));
+
//
// Check if we have a valid memory encryption mask
//
@@ -399,10 +411,6 @@ InternalMemEncryptSevSetMemoryDecrypted (
)
{
- DEBUG ((DEBUG_VERBOSE,
- "%a:%a Clear C-bit Cr3 %Lx Base %Lx Length %Lx flush %d\n",
- gEfiCallerBaseName, __FUNCTION__, Cr3BaseAddress, PhysicalAddress, Length,
- Flush));
return SetMemoryEncDec (Cr3BaseAddress, PhysicalAddress, Length, ClearCBit, Flush);
}
@@ -431,9 +439,5 @@ InternalMemEncryptSevSetMemoryEncrypted (
IN BOOLEAN Flush
)
{
- DEBUG ((DEBUG_VERBOSE,
- "%a:%a Set C-bit Cr3 %Lx Base %Lx Length %Lx flush %d\n",
- gEfiCallerBaseName, __FUNCTION__, Cr3BaseAddress, PhysicalAddress, Length,
- Flush));
return SetMemoryEncDec (Cr3BaseAddress, PhysicalAddress, Length, SetCBit, Flush);
}
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH 02/11] OvmfPkg/BaseMemEncryptSevLib: break DEBUG calls to multiple lines
2017-08-30 15:53 [PATCH 00/11] OvmfPkg: improve SEV-related DEBUG messages Laszlo Ersek
2017-08-30 15:53 ` [PATCH 01/11] OvmfPkg/BaseMemEncryptSevLib: unify encrypt/decrypt " Laszlo Ersek
@ 2017-08-30 15:53 ` Laszlo Ersek
2017-08-30 15:53 ` [PATCH 03/11] OvmfPkg/BaseMemEncryptSevLib: clean up DEBUG prefixes Laszlo Ersek
` (9 subsequent siblings)
11 siblings, 0 replies; 14+ messages in thread
From: Laszlo Ersek @ 2017-08-30 15:53 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Brijesh Singh, Jordan Justen
None of the DEBUG macro invocations in SetMemoryEncDec() fit on a single
line. Break them to multiple lines, for (a) conforming to the coding style
spec, (b) easier modification in later patches.
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c | 68 ++++++++++++++------
1 file changed, 48 insertions(+), 20 deletions(-)
diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c
index dbaad7766dbe..83e3d8992173 100644
--- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c
+++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c
@@ -286,18 +286,26 @@ SetMemoryEncDec (
PageMapLevel4Entry = (VOID*) (Cr3BaseAddress & ~PgTableMask);
PageMapLevel4Entry += PML4_OFFSET(PhysicalAddress);
if (!PageMapLevel4Entry->Bits.Present) {
- DEBUG ((DEBUG_WARN,
- "%a:%a ERROR bad PML4 for %lx\n", gEfiCallerBaseName, __FUNCTION__,
- PhysicalAddress));
+ DEBUG ((
+ DEBUG_WARN,
+ "%a:%a ERROR bad PML4 for %lx\n",
+ gEfiCallerBaseName,
+ __FUNCTION__,
+ PhysicalAddress
+ ));
return RETURN_NO_MAPPING;
}
PageDirectory1GEntry = (VOID*) ((PageMapLevel4Entry->Bits.PageTableBaseAddress<<12) & ~PgTableMask);
PageDirectory1GEntry += PDP_OFFSET(PhysicalAddress);
if (!PageDirectory1GEntry->Bits.Present) {
- DEBUG ((DEBUG_WARN,
- "%a:%a ERROR bad PDPE for %lx\n", gEfiCallerBaseName,
- __FUNCTION__, PhysicalAddress));
+ DEBUG ((
+ DEBUG_WARN,
+ "%a:%a ERROR bad PDPE for %lx\n",
+ gEfiCallerBaseName,
+ __FUNCTION__,
+ PhysicalAddress
+ ));
return RETURN_NO_MAPPING;
}
@@ -311,17 +319,25 @@ SetMemoryEncDec (
//
if (!(PhysicalAddress & (BIT30 - 1)) && Length >= BIT30) {
SetOrClearCBit(&PageDirectory1GEntry->Uint64, Mode);
- DEBUG ((DEBUG_VERBOSE,
- "%a:%a Updated 1GB entry for %lx\n", gEfiCallerBaseName,
- __FUNCTION__, PhysicalAddress));
+ DEBUG ((
+ DEBUG_VERBOSE,
+ "%a:%a Updated 1GB entry for %lx\n",
+ gEfiCallerBaseName,
+ __FUNCTION__,
+ PhysicalAddress
+ ));
PhysicalAddress += BIT30;
Length -= BIT30;
} else {
//
// We must split the page
//
- DEBUG ((DEBUG_VERBOSE,
- "%a:%a Spliting 1GB page\n", gEfiCallerBaseName, __FUNCTION__));
+ DEBUG ((
+ DEBUG_VERBOSE,
+ "%a:%a Spliting 1GB page\n",
+ gEfiCallerBaseName,
+ __FUNCTION__
+ ));
Split1GPageTo2M(((UINT64)PageDirectory1GEntry->Bits.PageTableBaseAddress)<<30, (UINT64*) PageDirectory1GEntry, 0, 0);
continue;
}
@@ -333,9 +349,13 @@ SetMemoryEncDec (
PageDirectory2MEntry = (VOID*) ((PageUpperDirectoryPointerEntry->Bits.PageTableBaseAddress<<12) & ~PgTableMask);
PageDirectory2MEntry += PDE_OFFSET(PhysicalAddress);
if (!PageDirectory2MEntry->Bits.Present) {
- DEBUG ((DEBUG_WARN,
- "%a:%a ERROR bad PDE for %lx\n", gEfiCallerBaseName, __FUNCTION__,
- PhysicalAddress));
+ DEBUG ((
+ DEBUG_WARN,
+ "%a:%a ERROR bad PDE for %lx\n",
+ gEfiCallerBaseName,
+ __FUNCTION__,
+ PhysicalAddress
+ ));
return RETURN_NO_MAPPING;
}
//
@@ -354,9 +374,13 @@ SetMemoryEncDec (
//
// We must split up this page into 4K pages
//
- DEBUG ((DEBUG_VERBOSE,
- "%a:%a Spliting 2MB page at %lx\n", gEfiCallerBaseName,__FUNCTION__,
- PhysicalAddress));
+ DEBUG ((
+ DEBUG_VERBOSE,
+ "%a:%a Spliting 2MB page at %lx\n",
+ gEfiCallerBaseName,
+ __FUNCTION__,
+ PhysicalAddress
+ ));
Split2MPageTo4K (((UINT64)PageDirectory2MEntry->Bits.PageTableBaseAddress) << 21, (UINT64*) PageDirectory2MEntry, 0, 0);
continue;
}
@@ -365,9 +389,13 @@ SetMemoryEncDec (
PageTableEntry = (VOID*) (PageDirectoryPointerEntry->Bits.PageTableBaseAddress<<12 & ~PgTableMask);
PageTableEntry += PTE_OFFSET(PhysicalAddress);
if (!PageTableEntry->Bits.Present) {
- DEBUG ((DEBUG_WARN,
- "%a:%a ERROR bad PTE for %lx\n", gEfiCallerBaseName,
- __FUNCTION__, PhysicalAddress));
+ DEBUG ((
+ DEBUG_WARN,
+ "%a:%a ERROR bad PTE for %lx\n",
+ gEfiCallerBaseName,
+ __FUNCTION__,
+ PhysicalAddress
+ ));
return RETURN_NO_MAPPING;
}
SetOrClearCBit (&PageTableEntry->Uint64, Mode);
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH 03/11] OvmfPkg/BaseMemEncryptSevLib: clean up DEBUG prefixes
2017-08-30 15:53 [PATCH 00/11] OvmfPkg: improve SEV-related DEBUG messages Laszlo Ersek
2017-08-30 15:53 ` [PATCH 01/11] OvmfPkg/BaseMemEncryptSevLib: unify encrypt/decrypt " Laszlo Ersek
2017-08-30 15:53 ` [PATCH 02/11] OvmfPkg/BaseMemEncryptSevLib: break DEBUG calls to multiple lines Laszlo Ersek
@ 2017-08-30 15:53 ` Laszlo Ersek
2017-08-30 15:53 ` [PATCH 04/11] OvmfPkg/BaseMemEncryptSevLib: clean up debug logging of PhysicalAddress Laszlo Ersek
` (8 subsequent siblings)
11 siblings, 0 replies; 14+ messages in thread
From: Laszlo Ersek @ 2017-08-30 15:53 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Brijesh Singh, Jordan Justen
The prefix for the SetMemoryEncDec() DEBUG messages should be
"ModuleName:FunctionName: "
not
"ModuleName:FunctionName "
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c
index 83e3d8992173..5e8c9b4c439b 100644
--- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c
+++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c
@@ -288,7 +288,7 @@ SetMemoryEncDec (
if (!PageMapLevel4Entry->Bits.Present) {
DEBUG ((
DEBUG_WARN,
- "%a:%a ERROR bad PML4 for %lx\n",
+ "%a:%a: ERROR bad PML4 for %lx\n",
gEfiCallerBaseName,
__FUNCTION__,
PhysicalAddress
@@ -301,7 +301,7 @@ SetMemoryEncDec (
if (!PageDirectory1GEntry->Bits.Present) {
DEBUG ((
DEBUG_WARN,
- "%a:%a ERROR bad PDPE for %lx\n",
+ "%a:%a: ERROR bad PDPE for %lx\n",
gEfiCallerBaseName,
__FUNCTION__,
PhysicalAddress
@@ -321,7 +321,7 @@ SetMemoryEncDec (
SetOrClearCBit(&PageDirectory1GEntry->Uint64, Mode);
DEBUG ((
DEBUG_VERBOSE,
- "%a:%a Updated 1GB entry for %lx\n",
+ "%a:%a: Updated 1GB entry for %lx\n",
gEfiCallerBaseName,
__FUNCTION__,
PhysicalAddress
@@ -334,7 +334,7 @@ SetMemoryEncDec (
//
DEBUG ((
DEBUG_VERBOSE,
- "%a:%a Spliting 1GB page\n",
+ "%a:%a: Spliting 1GB page\n",
gEfiCallerBaseName,
__FUNCTION__
));
@@ -351,7 +351,7 @@ SetMemoryEncDec (
if (!PageDirectory2MEntry->Bits.Present) {
DEBUG ((
DEBUG_WARN,
- "%a:%a ERROR bad PDE for %lx\n",
+ "%a:%a: ERROR bad PDE for %lx\n",
gEfiCallerBaseName,
__FUNCTION__,
PhysicalAddress
@@ -376,7 +376,7 @@ SetMemoryEncDec (
//
DEBUG ((
DEBUG_VERBOSE,
- "%a:%a Spliting 2MB page at %lx\n",
+ "%a:%a: Spliting 2MB page at %lx\n",
gEfiCallerBaseName,
__FUNCTION__,
PhysicalAddress
@@ -391,7 +391,7 @@ SetMemoryEncDec (
if (!PageTableEntry->Bits.Present) {
DEBUG ((
DEBUG_WARN,
- "%a:%a ERROR bad PTE for %lx\n",
+ "%a:%a: ERROR bad PTE for %lx\n",
gEfiCallerBaseName,
__FUNCTION__,
PhysicalAddress
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH 04/11] OvmfPkg/BaseMemEncryptSevLib: clean up debug logging of PhysicalAddress
2017-08-30 15:53 [PATCH 00/11] OvmfPkg: improve SEV-related DEBUG messages Laszlo Ersek
` (2 preceding siblings ...)
2017-08-30 15:53 ` [PATCH 03/11] OvmfPkg/BaseMemEncryptSevLib: clean up DEBUG prefixes Laszlo Ersek
@ 2017-08-30 15:53 ` Laszlo Ersek
2017-08-30 15:53 ` [PATCH 05/11] OvmfPkg/BaseMemEncryptSevLib: promote DEBUG_WARN levels to DEBUG_ERROR Laszlo Ersek
` (7 subsequent siblings)
11 siblings, 0 replies; 14+ messages in thread
From: Laszlo Ersek @ 2017-08-30 15:53 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Brijesh Singh, Jordan Justen
In the SetMemoryEncDec() function, the way we currently report
PhysicalAddress is not uniform:
- mostly we say "for %lx",
- in one spot we say "at %lx" (even though the 2MB page being split does
not live *at* PhysicalAddress, instead it maps PhysicalAddress),
- in another spot we don't log PhysicalAddress at all (when splitting a
1GB page).
Unify this, using the format string "for Physical=0x%Lx".
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c
index 5e8c9b4c439b..58c793dae6bf 100644
--- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c
+++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c
@@ -288,7 +288,7 @@ SetMemoryEncDec (
if (!PageMapLevel4Entry->Bits.Present) {
DEBUG ((
DEBUG_WARN,
- "%a:%a: ERROR bad PML4 for %lx\n",
+ "%a:%a: ERROR bad PML4 for Physical=0x%Lx\n",
gEfiCallerBaseName,
__FUNCTION__,
PhysicalAddress
@@ -301,7 +301,7 @@ SetMemoryEncDec (
if (!PageDirectory1GEntry->Bits.Present) {
DEBUG ((
DEBUG_WARN,
- "%a:%a: ERROR bad PDPE for %lx\n",
+ "%a:%a: ERROR bad PDPE for Physical=0x%Lx\n",
gEfiCallerBaseName,
__FUNCTION__,
PhysicalAddress
@@ -321,7 +321,7 @@ SetMemoryEncDec (
SetOrClearCBit(&PageDirectory1GEntry->Uint64, Mode);
DEBUG ((
DEBUG_VERBOSE,
- "%a:%a: Updated 1GB entry for %lx\n",
+ "%a:%a: Updated 1GB entry for Physical=0x%Lx\n",
gEfiCallerBaseName,
__FUNCTION__,
PhysicalAddress
@@ -334,9 +334,10 @@ SetMemoryEncDec (
//
DEBUG ((
DEBUG_VERBOSE,
- "%a:%a: Spliting 1GB page\n",
+ "%a:%a: Spliting 1GB page for Physical=0x%Lx\n",
gEfiCallerBaseName,
- __FUNCTION__
+ __FUNCTION__,
+ PhysicalAddress
));
Split1GPageTo2M(((UINT64)PageDirectory1GEntry->Bits.PageTableBaseAddress)<<30, (UINT64*) PageDirectory1GEntry, 0, 0);
continue;
@@ -351,7 +352,7 @@ SetMemoryEncDec (
if (!PageDirectory2MEntry->Bits.Present) {
DEBUG ((
DEBUG_WARN,
- "%a:%a: ERROR bad PDE for %lx\n",
+ "%a:%a: ERROR bad PDE for Physical=0x%Lx\n",
gEfiCallerBaseName,
__FUNCTION__,
PhysicalAddress
@@ -376,7 +377,7 @@ SetMemoryEncDec (
//
DEBUG ((
DEBUG_VERBOSE,
- "%a:%a: Spliting 2MB page at %lx\n",
+ "%a:%a: Spliting 2MB page for Physical=0x%Lx\n",
gEfiCallerBaseName,
__FUNCTION__,
PhysicalAddress
@@ -391,7 +392,7 @@ SetMemoryEncDec (
if (!PageTableEntry->Bits.Present) {
DEBUG ((
DEBUG_WARN,
- "%a:%a: ERROR bad PTE for %lx\n",
+ "%a:%a: ERROR bad PTE for Physical=0x%Lx\n",
gEfiCallerBaseName,
__FUNCTION__,
PhysicalAddress
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH 05/11] OvmfPkg/BaseMemEncryptSevLib: promote DEBUG_WARN levels to DEBUG_ERROR
2017-08-30 15:53 [PATCH 00/11] OvmfPkg: improve SEV-related DEBUG messages Laszlo Ersek
` (3 preceding siblings ...)
2017-08-30 15:53 ` [PATCH 04/11] OvmfPkg/BaseMemEncryptSevLib: clean up debug logging of PhysicalAddress Laszlo Ersek
@ 2017-08-30 15:53 ` Laszlo Ersek
2017-08-30 15:53 ` [PATCH 06/11] OvmfPkg/BaseMemEncryptSevLib: clean up upper-case / lower-case in DEBUGs Laszlo Ersek
` (6 subsequent siblings)
11 siblings, 0 replies; 14+ messages in thread
From: Laszlo Ersek @ 2017-08-30 15:53 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Brijesh Singh, Jordan Justen
In SetMemoryEncDec(), we have four locations where we (a) log a message on
the DEBUG_WARN level that says "ERROR", (b) return the status code
RETURN_NO_MAPPING right after.
These messages clearly describe actual errors (bad PML4, PDPE, PDE, PTE).
Promote their debug levels to DEBUG_ERROR, and remove the word "ERROR"
from the messages.
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c
index 58c793dae6bf..c37d799bc1f4 100644
--- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c
+++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c
@@ -287,8 +287,8 @@ SetMemoryEncDec (
PageMapLevel4Entry += PML4_OFFSET(PhysicalAddress);
if (!PageMapLevel4Entry->Bits.Present) {
DEBUG ((
- DEBUG_WARN,
- "%a:%a: ERROR bad PML4 for Physical=0x%Lx\n",
+ DEBUG_ERROR,
+ "%a:%a: bad PML4 for Physical=0x%Lx\n",
gEfiCallerBaseName,
__FUNCTION__,
PhysicalAddress
@@ -300,8 +300,8 @@ SetMemoryEncDec (
PageDirectory1GEntry += PDP_OFFSET(PhysicalAddress);
if (!PageDirectory1GEntry->Bits.Present) {
DEBUG ((
- DEBUG_WARN,
- "%a:%a: ERROR bad PDPE for Physical=0x%Lx\n",
+ DEBUG_ERROR,
+ "%a:%a: bad PDPE for Physical=0x%Lx\n",
gEfiCallerBaseName,
__FUNCTION__,
PhysicalAddress
@@ -351,8 +351,8 @@ SetMemoryEncDec (
PageDirectory2MEntry += PDE_OFFSET(PhysicalAddress);
if (!PageDirectory2MEntry->Bits.Present) {
DEBUG ((
- DEBUG_WARN,
- "%a:%a: ERROR bad PDE for Physical=0x%Lx\n",
+ DEBUG_ERROR,
+ "%a:%a: bad PDE for Physical=0x%Lx\n",
gEfiCallerBaseName,
__FUNCTION__,
PhysicalAddress
@@ -391,8 +391,8 @@ SetMemoryEncDec (
PageTableEntry += PTE_OFFSET(PhysicalAddress);
if (!PageTableEntry->Bits.Present) {
DEBUG ((
- DEBUG_WARN,
- "%a:%a: ERROR bad PTE for Physical=0x%Lx\n",
+ DEBUG_ERROR,
+ "%a:%a: bad PTE for Physical=0x%Lx\n",
gEfiCallerBaseName,
__FUNCTION__,
PhysicalAddress
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH 06/11] OvmfPkg/BaseMemEncryptSevLib: clean up upper-case / lower-case in DEBUGs
2017-08-30 15:53 [PATCH 00/11] OvmfPkg: improve SEV-related DEBUG messages Laszlo Ersek
` (4 preceding siblings ...)
2017-08-30 15:53 ` [PATCH 05/11] OvmfPkg/BaseMemEncryptSevLib: promote DEBUG_WARN levels to DEBUG_ERROR Laszlo Ersek
@ 2017-08-30 15:53 ` Laszlo Ersek
2017-08-30 15:53 ` [PATCH 07/11] OvmfPkg/BaseMemEncryptSevLib: fix typos in DEBUG messages Laszlo Ersek
` (5 subsequent siblings)
11 siblings, 0 replies; 14+ messages in thread
From: Laszlo Ersek @ 2017-08-30 15:53 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Brijesh Singh, Jordan Justen
Debug messages that start as natural (English) language phrases (after the
debug prefix) should uniformly begin with lower-case or upper-case. In
SetMemoryEncDec() we have a mixture now. Stick with lower-case.
(Upper-case is better for full sentences that also end with punctuation.)
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c
index c37d799bc1f4..96969617e0a3 100644
--- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c
+++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c
@@ -321,7 +321,7 @@ SetMemoryEncDec (
SetOrClearCBit(&PageDirectory1GEntry->Uint64, Mode);
DEBUG ((
DEBUG_VERBOSE,
- "%a:%a: Updated 1GB entry for Physical=0x%Lx\n",
+ "%a:%a: updated 1GB entry for Physical=0x%Lx\n",
gEfiCallerBaseName,
__FUNCTION__,
PhysicalAddress
@@ -334,7 +334,7 @@ SetMemoryEncDec (
//
DEBUG ((
DEBUG_VERBOSE,
- "%a:%a: Spliting 1GB page for Physical=0x%Lx\n",
+ "%a:%a: spliting 1GB page for Physical=0x%Lx\n",
gEfiCallerBaseName,
__FUNCTION__,
PhysicalAddress
@@ -377,7 +377,7 @@ SetMemoryEncDec (
//
DEBUG ((
DEBUG_VERBOSE,
- "%a:%a: Spliting 2MB page for Physical=0x%Lx\n",
+ "%a:%a: spliting 2MB page for Physical=0x%Lx\n",
gEfiCallerBaseName,
__FUNCTION__,
PhysicalAddress
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH 07/11] OvmfPkg/BaseMemEncryptSevLib: fix typos in DEBUG messages
2017-08-30 15:53 [PATCH 00/11] OvmfPkg: improve SEV-related DEBUG messages Laszlo Ersek
` (5 preceding siblings ...)
2017-08-30 15:53 ` [PATCH 06/11] OvmfPkg/BaseMemEncryptSevLib: clean up upper-case / lower-case in DEBUGs Laszlo Ersek
@ 2017-08-30 15:53 ` Laszlo Ersek
2017-08-30 15:53 ` [PATCH 08/11] OvmfPkg/IoMmuDxe: IoMmuMap(): log nicer and more informative DEBUG msgs Laszlo Ersek
` (4 subsequent siblings)
11 siblings, 0 replies; 14+ messages in thread
From: Laszlo Ersek @ 2017-08-30 15:53 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Brijesh Singh, Jordan Justen
Replace "spliting" with "splitting".
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c
index 96969617e0a3..e1e705c34626 100644
--- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c
+++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c
@@ -334,7 +334,7 @@ SetMemoryEncDec (
//
DEBUG ((
DEBUG_VERBOSE,
- "%a:%a: spliting 1GB page for Physical=0x%Lx\n",
+ "%a:%a: splitting 1GB page for Physical=0x%Lx\n",
gEfiCallerBaseName,
__FUNCTION__,
PhysicalAddress
@@ -377,7 +377,7 @@ SetMemoryEncDec (
//
DEBUG ((
DEBUG_VERBOSE,
- "%a:%a: spliting 2MB page for Physical=0x%Lx\n",
+ "%a:%a: splitting 2MB page for Physical=0x%Lx\n",
gEfiCallerBaseName,
__FUNCTION__,
PhysicalAddress
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH 08/11] OvmfPkg/IoMmuDxe: IoMmuMap(): log nicer and more informative DEBUG msgs
2017-08-30 15:53 [PATCH 00/11] OvmfPkg: improve SEV-related DEBUG messages Laszlo Ersek
` (6 preceding siblings ...)
2017-08-30 15:53 ` [PATCH 07/11] OvmfPkg/BaseMemEncryptSevLib: fix typos in DEBUG messages Laszlo Ersek
@ 2017-08-30 15:53 ` Laszlo Ersek
2017-08-30 15:53 ` [PATCH 09/11] OvmfPkg/IoMmuDxe: IoMmuUnmap(): clean up DEBUG message Laszlo Ersek
` (3 subsequent siblings)
11 siblings, 0 replies; 14+ messages in thread
From: Laszlo Ersek @ 2017-08-30 15:53 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Brijesh Singh, Jordan Justen
Log all relevant IN and IN OUT parameters on entry.
When exiting with success, log all relevant OUT and IN OUT parameters.
Don't log OUT and IN OUT parameters that are never set or changed after
entering the function (i.e., *NumberOfBytes).
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/IoMmuDxe/AmdSevIoMmu.c | 31 ++++++++++++++++++--
1 file changed, 28 insertions(+), 3 deletions(-)
diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
index ec625166f459..59cee95c0e21 100644
--- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
+++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
@@ -44,6 +44,19 @@ STATIC LIST_ENTRY mRecycledMapInfos = INITIALIZE_LIST_HEAD_VARIABLE (
#define COMMON_BUFFER_SIG SIGNATURE_64 ('C', 'M', 'N', 'B', 'U', 'F', 'F', 'R')
+//
+// ASCII names for EDKII_IOMMU_OPERATION constants, for debug logging.
+//
+STATIC CONST CHAR8 * CONST
+mBusMasterOperationName[EdkiiIoMmuOperationMaximum] = {
+ "Read",
+ "Write",
+ "CommonBuffer",
+ "Read64",
+ "Write64",
+ "CommonBuffer64"
+};
+
//
// The following structure enables Map() and Unmap() to perform in-place
// decryption and encryption, respectively, for BusMasterCommonBuffer[64]
@@ -116,6 +129,18 @@ IoMmuMap (
COMMON_BUFFER_HEADER *CommonBufferHeader;
VOID *DecryptionSource;
+ DEBUG ((
+ DEBUG_VERBOSE,
+ "%a: Operation=%a Host=0x%p Bytes=0x%Lx\n",
+ __FUNCTION__,
+ ((Operation >= 0 &&
+ Operation < ARRAY_SIZE (mBusMasterOperationName)) ?
+ mBusMasterOperationName[Operation] :
+ "Invalid"),
+ HostAddress,
+ (UINT64)((NumberOfBytes == NULL) ? 0 : *NumberOfBytes)
+ ));
+
if (HostAddress == NULL || NumberOfBytes == NULL || DeviceAddress == NULL ||
Mapping == NULL) {
return EFI_INVALID_PARAMETER;
@@ -281,12 +306,12 @@ IoMmuMap (
DEBUG ((
DEBUG_VERBOSE,
- "%a PlainText 0x%Lx Crypted 0x%Lx Pages 0x%Lx Bytes 0x%Lx\n",
+ "%a: Mapping=0x%p Device(PlainText)=0x%Lx Crypted=0x%Lx Pages=0x%Lx\n",
__FUNCTION__,
+ MapInfo,
MapInfo->PlainTextAddress,
MapInfo->CryptedAddress,
- (UINT64)MapInfo->NumberOfPages,
- (UINT64)MapInfo->NumberOfBytes
+ (UINT64)MapInfo->NumberOfPages
));
return EFI_SUCCESS;
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH 09/11] OvmfPkg/IoMmuDxe: IoMmuUnmap(): clean up DEBUG message
2017-08-30 15:53 [PATCH 00/11] OvmfPkg: improve SEV-related DEBUG messages Laszlo Ersek
` (7 preceding siblings ...)
2017-08-30 15:53 ` [PATCH 08/11] OvmfPkg/IoMmuDxe: IoMmuMap(): log nicer and more informative DEBUG msgs Laszlo Ersek
@ 2017-08-30 15:53 ` Laszlo Ersek
2017-08-30 15:53 ` [PATCH 10/11] OvmfPkg/IoMmuDxe: IoMmuAllocateBuffer(): nicer and more informative DEBUGs Laszlo Ersek
` (2 subsequent siblings)
11 siblings, 0 replies; 14+ messages in thread
From: Laszlo Ersek @ 2017-08-30 15:53 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Brijesh Singh, Jordan Justen
The only important external information for this function, and for the
human looking at the log, is the Mapping input parameter. Log it on entry.
Stop logging the contents of the MAP_INFO structure pointed-to by Mapping.
Thanks to the previous patch, we can now associate IoMmuUnmap() messages
with IoMmuMap() messages -- and thereby with MAP_INFO contents -- purely
via Mapping.
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/IoMmuDxe/AmdSevIoMmu.c | 12 ++----------
1 file changed, 2 insertions(+), 10 deletions(-)
diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
index 59cee95c0e21..a153d250d545 100644
--- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
+++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
@@ -348,6 +348,8 @@ IoMmuUnmap (
COMMON_BUFFER_HEADER *CommonBufferHeader;
VOID *EncryptionTarget;
+ DEBUG ((DEBUG_VERBOSE, "%a: Mapping=0x%p\n", __FUNCTION__, Mapping));
+
if (Mapping == NULL) {
return EFI_INVALID_PARAMETER;
}
@@ -399,16 +401,6 @@ IoMmuUnmap (
break;
}
- DEBUG ((
- DEBUG_VERBOSE,
- "%a PlainText 0x%Lx Crypted 0x%Lx Pages 0x%Lx Bytes 0x%Lx\n",
- __FUNCTION__,
- MapInfo->PlainTextAddress,
- MapInfo->CryptedAddress,
- (UINT64)MapInfo->NumberOfPages,
- (UINT64)MapInfo->NumberOfBytes
- ));
-
//
// Restore the memory encryption mask on the area we used to hold the
// plaintext.
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH 10/11] OvmfPkg/IoMmuDxe: IoMmuAllocateBuffer(): nicer and more informative DEBUGs
2017-08-30 15:53 [PATCH 00/11] OvmfPkg: improve SEV-related DEBUG messages Laszlo Ersek
` (8 preceding siblings ...)
2017-08-30 15:53 ` [PATCH 09/11] OvmfPkg/IoMmuDxe: IoMmuUnmap(): clean up DEBUG message Laszlo Ersek
@ 2017-08-30 15:53 ` Laszlo Ersek
2017-08-30 15:53 ` [PATCH 11/11] OvmfPkg/IoMmuDxe: IoMmuFreeBuffer(): clean up DEBUG message Laszlo Ersek
2017-08-31 15:14 ` [PATCH 00/11] OvmfPkg: improve SEV-related DEBUG messages Brijesh Singh
11 siblings, 0 replies; 14+ messages in thread
From: Laszlo Ersek @ 2017-08-30 15:53 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Brijesh Singh, Jordan Justen
Log all relevant IN and IN OUT parameters on entry.
(Note that the HostAddress parameter is IN OUT rather than OUT due to
historical reasons. The "IN EFI_ALLOCATE_TYPE Type" parameter is now to be
ignored, but historically it could be set to AllocateMaxAddress for
example, and for that HostAddress had to be IN OUT.)
When exiting with success, log all relevant OUT parameters (i.e.,
HostAddress). Also log the new (internal) StashBuffer address, on which
IoMmuMap() and IoMmuUnmap() rely on, for BusMasterCommonBuffer operations
(in-place decryption and encryption, respectively).
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/IoMmuDxe/AmdSevIoMmu.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
index a153d250d545..0ab7043498bd 100644
--- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
+++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
@@ -490,6 +490,15 @@ IoMmuAllocateBuffer (
UINTN CommonBufferPages;
COMMON_BUFFER_HEADER *CommonBufferHeader;
+ DEBUG ((
+ DEBUG_VERBOSE,
+ "%a: MemoryType=%u Pages=0x%Lx Attributes=0x%Lx\n",
+ __FUNCTION__,
+ (UINT32)MemoryType,
+ (UINT64)Pages,
+ Attributes
+ ));
+
//
// Validate Attributes
//
@@ -566,10 +575,10 @@ IoMmuAllocateBuffer (
DEBUG ((
DEBUG_VERBOSE,
- "%a Address 0x%Lx Pages 0x%Lx\n",
+ "%a: Host=0x%Lx Stash=0x%p\n",
__FUNCTION__,
PhysicalAddress,
- (UINT64)Pages
+ StashBuffer
));
return EFI_SUCCESS;
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH 11/11] OvmfPkg/IoMmuDxe: IoMmuFreeBuffer(): clean up DEBUG message
2017-08-30 15:53 [PATCH 00/11] OvmfPkg: improve SEV-related DEBUG messages Laszlo Ersek
` (9 preceding siblings ...)
2017-08-30 15:53 ` [PATCH 10/11] OvmfPkg/IoMmuDxe: IoMmuAllocateBuffer(): nicer and more informative DEBUGs Laszlo Ersek
@ 2017-08-30 15:53 ` Laszlo Ersek
2017-08-31 15:14 ` [PATCH 00/11] OvmfPkg: improve SEV-related DEBUG messages Brijesh Singh
11 siblings, 0 replies; 14+ messages in thread
From: Laszlo Ersek @ 2017-08-30 15:53 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Brijesh Singh, Jordan Justen
Log all relevant IN parameters on entry. (There are only IN parameters.)
Beautify the format string.
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/IoMmuDxe/AmdSevIoMmu.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
index 0ab7043498bd..bc57de5b572b 100644
--- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
+++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
@@ -611,6 +611,14 @@ IoMmuFreeBuffer (
UINTN CommonBufferPages;
COMMON_BUFFER_HEADER *CommonBufferHeader;
+ DEBUG ((
+ DEBUG_VERBOSE,
+ "%a: Host=0x%p Pages=0x%Lx\n",
+ __FUNCTION__,
+ HostAddress,
+ (UINT64)Pages
+ ));
+
CommonBufferPages = Pages + 1;
CommonBufferHeader = (COMMON_BUFFER_HEADER *)(
(UINTN)HostAddress - EFI_PAGE_SIZE
@@ -630,14 +638,6 @@ IoMmuFreeBuffer (
//
FreePages (CommonBufferHeader->StashBuffer, Pages);
- DEBUG ((
- DEBUG_VERBOSE,
- "%a Address 0x%Lx Pages 0x%Lx\n",
- __FUNCTION__,
- (UINT64)(UINTN)HostAddress,
- (UINT64)Pages
- ));
-
//
// Release the common buffer itself. Unmap() has re-encrypted it in-place, so
// no need to zero it.
--
2.14.1.3.gb7cf6e02401b
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH 00/11] OvmfPkg: improve SEV-related DEBUG messages
2017-08-30 15:53 [PATCH 00/11] OvmfPkg: improve SEV-related DEBUG messages Laszlo Ersek
` (10 preceding siblings ...)
2017-08-30 15:53 ` [PATCH 11/11] OvmfPkg/IoMmuDxe: IoMmuFreeBuffer(): clean up DEBUG message Laszlo Ersek
@ 2017-08-31 15:14 ` Brijesh Singh
2017-09-01 12:23 ` Laszlo Ersek
11 siblings, 1 reply; 14+ messages in thread
From: Brijesh Singh @ 2017-08-31 15:14 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-01; +Cc: brijesh.singh, Jordan Justen
On 08/30/2017 10:53 AM, Laszlo Ersek wrote:
> Repo: https://github.com/lersek/edk2.git
> Branch: sev_debug_messages
>
> I've now looked at quite a bit of SEV-related DEBUG messages, and I find
> them really hard to read, and to process with command line utilities
> like grep, sort, uniq.
>
> For example, shell pipelines should be possible to construct with these
> utilities to collect decrypt/encrypt operations, in chronological order,
> grouped by address ever decrypted.
>
> The log should also be human-readable as much as possible -- it
> shouldn't be redundant, but all information for forward- and
> back-referencing should be available.
>
> I propose this series for the above improvements.
>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
>
Very nice improvement in debug message, thank you !
Reviewed-by: Brijesh Singh <brijesh.singh@amd.com>
Tested-by: Brijesh Singh <brijesh.singh@amd.com>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 00/11] OvmfPkg: improve SEV-related DEBUG messages
2017-08-31 15:14 ` [PATCH 00/11] OvmfPkg: improve SEV-related DEBUG messages Brijesh Singh
@ 2017-09-01 12:23 ` Laszlo Ersek
0 siblings, 0 replies; 14+ messages in thread
From: Laszlo Ersek @ 2017-09-01 12:23 UTC (permalink / raw)
To: Brijesh Singh, edk2-devel-01; +Cc: Jordan Justen
On 08/31/17 17:14, Brijesh Singh wrote:
>
>
> On 08/30/2017 10:53 AM, Laszlo Ersek wrote:
>> Repo: https://github.com/lersek/edk2.git
>> Branch: sev_debug_messages
>>
>> I've now looked at quite a bit of SEV-related DEBUG messages, and I find
>> them really hard to read, and to process with command line utilities
>> like grep, sort, uniq.
>>
>> For example, shell pipelines should be possible to construct with these
>> utilities to collect decrypt/encrypt operations, in chronological order,
>> grouped by address ever decrypted.
>>
>> The log should also be human-readable as much as possible -- it
>> shouldn't be redundant, but all information for forward- and
>> back-referencing should be available.
>>
>> I propose this series for the above improvements.
>>
>> Cc: Brijesh Singh <brijesh.singh@amd.com>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>
>
> Very nice improvement in debug message, thank you !
>
> Reviewed-by: Brijesh Singh <brijesh.singh@amd.com>
> Tested-by: Brijesh Singh <brijesh.singh@amd.com>
Thank you, pushed as commit range 63ed4d2757eb..1afbb85f8736.
Laszlo
^ permalink raw reply [flat|nested] 14+ messages in thread