[PATCH 00/11] OvmfPkg: improve SEV-related DEBUG messages

[PATCH 00/11] OvmfPkg: improve SEV-related DEBUG messages

[Laszlo Ersek]

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>

Thanks,
Laszlo

Laszlo Ersek (11):
  OvmfPkg/BaseMemEncryptSevLib: unify encrypt/decrypt DEBUG messages
  OvmfPkg/BaseMemEncryptSevLib: break DEBUG calls to multiple lines
  OvmfPkg/BaseMemEncryptSevLib: clean up DEBUG prefixes
  OvmfPkg/BaseMemEncryptSevLib: clean up debug logging of
    PhysicalAddress
  OvmfPkg/BaseMemEncryptSevLib: promote DEBUG_WARN levels to DEBUG_ERROR
  OvmfPkg/BaseMemEncryptSevLib: clean up upper-case / lower-case in
    DEBUGs
  OvmfPkg/BaseMemEncryptSevLib: fix typos in DEBUG messages
  OvmfPkg/IoMmuDxe: IoMmuMap(): log nicer and more informative DEBUG
    msgs
  OvmfPkg/IoMmuDxe: IoMmuUnmap(): clean up DEBUG message
  OvmfPkg/IoMmuDxe: IoMmuAllocateBuffer(): nicer and more informative
    DEBUGs
  OvmfPkg/IoMmuDxe: IoMmuFreeBuffer(): clean up DEBUG message

 OvmfPkg/IoMmuDxe/AmdSevIoMmu.c                           | 72 +++++++++++-----
 OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c | 89 ++++++++++++++------
 2 files changed, 110 insertions(+), 51 deletions(-)

-- 
2.14.1.3.gb7cf6e02401b

[PATCH 01/11] OvmfPkg/BaseMemEncryptSevLib: unify encrypt/decrypt DEBUG messages

[Laszlo Ersek]

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

[PATCH 02/11] OvmfPkg/BaseMemEncryptSevLib: break DEBUG calls to multiple lines

[Laszlo Ersek]

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

[PATCH 03/11] OvmfPkg/BaseMemEncryptSevLib: clean up DEBUG prefixes

[Laszlo Ersek]

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

[PATCH 04/11] OvmfPkg/BaseMemEncryptSevLib: clean up debug logging of PhysicalAddress

[Laszlo Ersek]

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

[PATCH 05/11] OvmfPkg/BaseMemEncryptSevLib: promote DEBUG_WARN levels to DEBUG_ERROR

[Laszlo Ersek]

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

[PATCH 06/11] OvmfPkg/BaseMemEncryptSevLib: clean up upper-case / lower-case in DEBUGs

[Laszlo Ersek]

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

[PATCH 07/11] OvmfPkg/BaseMemEncryptSevLib: fix typos in DEBUG messages

[Laszlo Ersek]

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

[PATCH 08/11] OvmfPkg/IoMmuDxe: IoMmuMap(): log nicer and more informative DEBUG msgs

[Laszlo Ersek]

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

[PATCH 09/11] OvmfPkg/IoMmuDxe: IoMmuUnmap(): clean up DEBUG message

[Laszlo Ersek]

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

[PATCH 10/11] OvmfPkg/IoMmuDxe: IoMmuAllocateBuffer(): nicer and more informative DEBUGs

[Laszlo Ersek]

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

[PATCH 11/11] OvmfPkg/IoMmuDxe: IoMmuFreeBuffer(): clean up DEBUG message

[Laszlo Ersek]

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

Re: [PATCH 00/11] OvmfPkg: improve SEV-related DEBUG messages

[Brijesh Singh]

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>

Re: [PATCH 00/11] OvmfPkg: improve SEV-related DEBUG messages

[Laszlo Ersek]

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