[PATCH 0/2] MdePkg, OvmfPkg: fix IoWriteFifo8(), and print debug messages with it

[PATCH 0/2] MdePkg, OvmfPkg: fix IoWriteFifo8(), and print debug messages with it

[Laszlo Ersek]

Repo:   https://github.com/lersek/edk2.git
Branch: debugprint_iofifo

The first patch (for MdePkg) fixes an assembly language operand ordering
oversight in the unrolled / SEV implementations of IoWriteFifoXX().

The second patch makes OvmfPkg use IoWriteFifo8() for printing DEBUG
messages and ASSERT() failures.

Thanks,
Laszlo

Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>

Laszlo Ersek (2):
  MdePkg/BaseIoLibIntrinsic: fix SEV (=unrolled) variants of
    IoWriteFifoXX()
  OvmfPkg/PlatformDebugLibIoPort: write messages with IoWriteFifo8()

 OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c     | 20 ++++++++------------
 MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifoSev.nasm |  6 +++---
 MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifoSev.nasm  |  6 +++---
 3 files changed, 14 insertions(+), 18 deletions(-)

-- 
2.14.1.3.gb7cf6e02401b

[PATCH 1/2] MdePkg/BaseIoLibIntrinsic: fix SEV (=unrolled) variants of IoWriteFifoXX()

[Laszlo Ersek]

In commit b6d11d7c4678 ("MdePkg: BaseIoLibIntrinsic (IoLib class)
library", 2017-04-12), the MOV instructions in the write loops were
probably copied from the read loops. However, the operand order was not
adjusted.

As a result, the IoWriteFifoXX() routines, when invoked in SEV guests, now
overwrite the source buffer with value 0x01 / 0x0001 / 0x00000001 -- the
SevNoRepIo() function returns value 1 in EAX, in SEV guests --, and write
the same value to the target IO port.

Fix this by putting the target operand (AL / AX / EAX) first, and the
source operand (BYTE / WORD / DWORD [ESI/RSI]) second.

Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Fixes: b6d11d7c467810ea7f2e2eda46ef0bdc57bf1475
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifoSev.nasm | 6 +++---
 MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifoSev.nasm  | 6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifoSev.nasm b/MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifoSev.nasm
index 3e80c17d04a3..4b2af807cff8 100644
--- a/MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifoSev.nasm
+++ b/MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifoSev.nasm
@@ -212,7 +212,7 @@ ASM_PFX(IoWriteFifo8):
     jecxz   @IoWriteFifo8_Done
 
 @IoWriteFifo8_Loop:
-    mov     byte [esi], al
+    mov     al, byte [esi]
     out     dx, al
     inc     esi
     loop    @IoWriteFifo8_Loop
@@ -250,7 +250,7 @@ ASM_PFX(IoWriteFifo16):
     jecxz   @IoWriteFifo16_Done
 
 @IoWriteFifo16_Loop:
-    mov     word [esi], ax
+    mov     ax, word [esi]
     out     dx, ax
     add     esi, 2
     loop    @IoWriteFifo16_Loop
@@ -288,7 +288,7 @@ ASM_PFX(IoWriteFifo32):
     jecxz   @IoWriteFifo32_Done
 
 @IoWriteFifo32_Loop:
-    mov     dword [esi], eax
+    mov     eax, dword [esi]
     out     dx, eax
     add     esi, 4
     loop    @IoWriteFifo32_Loop
diff --git a/MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifoSev.nasm b/MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifoSev.nasm
index 26e016625b72..4d86a6cd5330 100644
--- a/MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifoSev.nasm
+++ b/MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifoSev.nasm
@@ -205,7 +205,7 @@ ASM_PFX(IoWriteFifo8):
     jrcxz   @IoWriteFifo8_Done
 
 @IoWriteFifo8_Loop:
-    mov     byte [rsi], al
+    mov     al, byte [rsi]
     out     dx, al
     inc     rsi
     loop    @IoWriteFifo8_Loop
@@ -241,7 +241,7 @@ ASM_PFX(IoWriteFifo16):
     jrcxz   @IoWriteFifo16_Done
 
 @IoWriteFifo16_Loop:
-    mov     word [rsi], ax
+    mov     ax, word [rsi]
     out     dx, ax
     add     rsi, 2
     loop    @IoWriteFifo16_Loop
@@ -277,7 +277,7 @@ ASM_PFX(IoWriteFifo32):
     jrcxz   @IoWriteFifo32_Done
 
 @IoWriteFifo32_Loop:
-    mov     dword [rsi], eax
+    mov     eax, dword [rsi]
     out     dx, eax
     add     rsi, 4
     loop    @IoWriteFifo32_Loop
-- 
2.14.1.3.gb7cf6e02401b

Re: [PATCH 1/2] MdePkg/BaseIoLibIntrinsic: fix SEV (=unrolled) variants of IoWriteFifoXX()

[Gao, Liming]

Reviewed-by: Liming Gao <liming.gao@intel.com>

>-----Original Message-----
>From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>Laszlo Ersek
>Sent: Monday, September 04, 2017 11:57 PM
>To: edk2-devel-01 <edk2-devel@lists.01.org>
>Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Justen, Jordan L
><jordan.l.justen@intel.com>; Gao, Liming <liming.gao@intel.com>
>Subject: [edk2] [PATCH 1/2] MdePkg/BaseIoLibIntrinsic: fix SEV (=unrolled)
>variants of IoWriteFifoXX()
>
>In commit b6d11d7c4678 ("MdePkg: BaseIoLibIntrinsic (IoLib class)
>library", 2017-04-12), the MOV instructions in the write loops were
>probably copied from the read loops. However, the operand order was not
>adjusted.
>
>As a result, the IoWriteFifoXX() routines, when invoked in SEV guests, now
>overwrite the source buffer with value 0x01 / 0x0001 / 0x00000001 -- the
>SevNoRepIo() function returns value 1 in EAX, in SEV guests --, and write
>the same value to the target IO port.
>
>Fix this by putting the target operand (AL / AX / EAX) first, and the
>source operand (BYTE / WORD / DWORD [ESI/RSI]) second.
>
>Cc: Brijesh Singh <brijesh.singh@amd.com>
>Cc: Jordan Justen <jordan.l.justen@intel.com>
>Cc: Liming Gao <liming.gao@intel.com>
>Cc: Michael D Kinney <michael.d.kinney@intel.com>
>Fixes: b6d11d7c467810ea7f2e2eda46ef0bdc57bf1475
>Contributed-under: TianoCore Contribution Agreement 1.1
>Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>---
> MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifoSev.nasm | 6 +++---
> MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifoSev.nasm  | 6 +++---
> 2 files changed, 6 insertions(+), 6 deletions(-)
>
>diff --git a/MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifoSev.nasm
>b/MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifoSev.nasm
>index 3e80c17d04a3..4b2af807cff8 100644
>--- a/MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifoSev.nasm
>+++ b/MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifoSev.nasm
>@@ -212,7 +212,7 @@ ASM_PFX(IoWriteFifo8):
>     jecxz   @IoWriteFifo8_Done
>
> @IoWriteFifo8_Loop:
>-    mov     byte [esi], al
>+    mov     al, byte [esi]
>     out     dx, al
>     inc     esi
>     loop    @IoWriteFifo8_Loop
>@@ -250,7 +250,7 @@ ASM_PFX(IoWriteFifo16):
>     jecxz   @IoWriteFifo16_Done
>
> @IoWriteFifo16_Loop:
>-    mov     word [esi], ax
>+    mov     ax, word [esi]
>     out     dx, ax
>     add     esi, 2
>     loop    @IoWriteFifo16_Loop
>@@ -288,7 +288,7 @@ ASM_PFX(IoWriteFifo32):
>     jecxz   @IoWriteFifo32_Done
>
> @IoWriteFifo32_Loop:
>-    mov     dword [esi], eax
>+    mov     eax, dword [esi]
>     out     dx, eax
>     add     esi, 4
>     loop    @IoWriteFifo32_Loop
>diff --git a/MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifoSev.nasm
>b/MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifoSev.nasm
>index 26e016625b72..4d86a6cd5330 100644
>--- a/MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifoSev.nasm
>+++ b/MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifoSev.nasm
>@@ -205,7 +205,7 @@ ASM_PFX(IoWriteFifo8):
>     jrcxz   @IoWriteFifo8_Done
>
> @IoWriteFifo8_Loop:
>-    mov     byte [rsi], al
>+    mov     al, byte [rsi]
>     out     dx, al
>     inc     rsi
>     loop    @IoWriteFifo8_Loop
>@@ -241,7 +241,7 @@ ASM_PFX(IoWriteFifo16):
>     jrcxz   @IoWriteFifo16_Done
>
> @IoWriteFifo16_Loop:
>-    mov     word [rsi], ax
>+    mov     ax, word [rsi]
>     out     dx, ax
>     add     rsi, 2
>     loop    @IoWriteFifo16_Loop
>@@ -277,7 +277,7 @@ ASM_PFX(IoWriteFifo32):
>     jrcxz   @IoWriteFifo32_Done
>
> @IoWriteFifo32_Loop:
>-    mov     dword [rsi], eax
>+    mov     eax, dword [rsi]
>     out     dx, eax
>     add     rsi, 4
>     loop    @IoWriteFifo32_Loop
>--
>2.14.1.3.gb7cf6e02401b
>
>
>_______________________________________________
>edk2-devel mailing list
>edk2-devel@lists.01.org
>https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 1/2] MdePkg/BaseIoLibIntrinsic: fix SEV (=unrolled) variants of IoWriteFifoXX()

[Brijesh Singh]

On 09/04/2017 10:57 AM, Laszlo Ersek wrote:
> In commit b6d11d7c4678 ("MdePkg: BaseIoLibIntrinsic (IoLib class)
> library", 2017-04-12), the MOV instructions in the write loops were
> probably copied from the read loops. However, the operand order was not
> adjusted.
> 
> As a result, the IoWriteFifoXX() routines, when invoked in SEV guests, now
> overwrite the source buffer with value 0x01 / 0x0001 / 0x00000001 -- the
> SevNoRepIo() function returns value 1 in EAX, in SEV guests --, and write
> the same value to the target IO port.
> 
> Fix this by putting the target operand (AL / AX / EAX) first, and the
> source operand (BYTE / WORD / DWORD [ESI/RSI]) second.
> 
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Fixes: b6d11d7c467810ea7f2e2eda46ef0bdc57bf1475
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>


Reviewed-by: Brijesh Singh <brijesh.singh@amd.com>

[PATCH 2/2] OvmfPkg/PlatformDebugLibIoPort: write messages with IoWriteFifo8()

[Laszlo Ersek]

Since commit 19c6d9feaaf8 ("MdePkg: Expand BaseIoLibIntrinsic (IoLib
class) library", 2017-01-14), IoWriteFifo8() has been widely available to
modules. Use it to print debug messages and assertion failures to the QEMU
debug port, rather than open-coded loops.

In the general case this speeds up logging, because debug messages will
now trap to QEMU once per message (as opposed to once per character), due
to "REP OUTSB" in "MdePkg/Library/BaseIoLibIntrinsic/*/IoFifoSev.nasm".

In SEV guests, there is no speedup (SEV doesn't support the REP prefix).
SEV is detected internally to BaseIoLibIntrinsic.

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/PlatformDebugLibIoPort/DebugLib.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c
index 44850a9dbad0..5435767c1c69 100644
--- a/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c
+++ b/OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c
@@ -69,7 +69,7 @@ DebugPrint (
 {
   CHAR8    Buffer[MAX_DEBUG_MESSAGE_LENGTH];
   VA_LIST  Marker;
-  UINT8    *Ptr;
+  UINTN    Length;
 
   //
   // If Format is NULL, then ASSERT().
@@ -87,15 +87,13 @@ DebugPrint (
   // Convert the DEBUG() message to an ASCII String
   //
   VA_START (Marker, Format);
-  AsciiVSPrint (Buffer, sizeof (Buffer), Format, Marker);
+  Length = AsciiVSPrint (Buffer, sizeof (Buffer), Format, Marker);
   VA_END (Marker);
 
   //
   // Send the print string to the debug I/O port
   //
-  for (Ptr = (UINT8 *) Buffer; *Ptr; Ptr++) {
-    IoWrite8 (PcdGet16(PcdDebugIoPort), *Ptr);
-  }
+  IoWriteFifo8 (PcdGet16 (PcdDebugIoPort), Length, Buffer);
 }
 
 
@@ -129,20 +127,18 @@ DebugAssert (
   )
 {
   CHAR8  Buffer[MAX_DEBUG_MESSAGE_LENGTH];
-  UINT8 *Ptr;
+  UINTN  Length;
 
   //
   // Generate the ASSERT() message in Ascii format
   //
-  AsciiSPrint (Buffer, sizeof Buffer, "ASSERT %a(%Lu): %a\n", FileName,
-    (UINT64)LineNumber, Description);
+  Length = AsciiSPrint (Buffer, sizeof Buffer, "ASSERT %a(%Lu): %a\n",
+             FileName, (UINT64)LineNumber, Description);
 
   //
-  // Send the print string to the Console Output device
+  // Send the print string to the debug I/O port
   //
-  for (Ptr = (UINT8 *) Buffer; *Ptr; Ptr++) {
-    IoWrite8 (PcdGet16(PcdDebugIoPort), *Ptr);
-  }
+  IoWriteFifo8 (PcdGet16 (PcdDebugIoPort), Length, Buffer);
 
   //
   // Generate a Breakpoint, DeadLoop, or NOP based on PCD settings
-- 
2.14.1.3.gb7cf6e02401b

Re: [PATCH 2/2] OvmfPkg/PlatformDebugLibIoPort: write messages with IoWriteFifo8()

[Brijesh Singh]

On 09/04/2017 10:57 AM, Laszlo Ersek wrote:
> Since commit 19c6d9feaaf8 ("MdePkg: Expand BaseIoLibIntrinsic (IoLib
> class) library", 2017-01-14), IoWriteFifo8() has been widely available to
> modules. Use it to print debug messages and assertion failures to the QEMU
> debug port, rather than open-coded loops.
> 
> In the general case this speeds up logging, because debug messages will
> now trap to QEMU once per message (as opposed to once per character), due
> to "REP OUTSB" in "MdePkg/Library/BaseIoLibIntrinsic/*/IoFifoSev.nasm".
> 
> In SEV guests, there is no speedup (SEV doesn't support the REP prefix).
> SEV is detected internally to BaseIoLibIntrinsic.
> 
> 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>

Acked-by: Brijesh Singh <brijesh.singh@amd.com>

Re: [PATCH 0/2] MdePkg, OvmfPkg: fix IoWriteFifo8(), and print debug messages with it

[Laszlo Ersek]

On 09/04/17 17:57, Laszlo Ersek wrote:
> Repo:   https://github.com/lersek/edk2.git
> Branch: debugprint_iofifo
> 
> The first patch (for MdePkg) fixes an assembly language operand ordering
> oversight in the unrolled / SEV implementations of IoWriteFifoXX().
> 
> The second patch makes OvmfPkg use IoWriteFifo8() for printing DEBUG
> messages and ASSERT() failures.
> 
> Thanks,
> Laszlo
> 
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> 
> Laszlo Ersek (2):
>   MdePkg/BaseIoLibIntrinsic: fix SEV (=unrolled) variants of
>     IoWriteFifoXX()
>   OvmfPkg/PlatformDebugLibIoPort: write messages with IoWriteFifo8()
> 
>  OvmfPkg/Library/PlatformDebugLibIoPort/DebugLib.c     | 20 ++++++++------------
>  MdePkg/Library/BaseIoLibIntrinsic/Ia32/IoFifoSev.nasm |  6 +++---
>  MdePkg/Library/BaseIoLibIntrinsic/X64/IoFifoSev.nasm  |  6 +++---
>  3 files changed, 14 insertions(+), 18 deletions(-)
> 

Thanks for the feedback, pushed as aa9aa47e06ac..80886a695377.

Laszlo