[PATCH 0/9] SEV-ES guest support fixes and cleanup

[PATCH 0/9] SEV-ES guest support fixes and cleanup

[Lendacky, Thomas]

From: Tom Lendacky <thomas.lendacky@amd.com>

This patch series provides some fixes, updates and cleanup to the SEV-ES
guest support:

The first patch updates the calculation of the qword offset of fields
within the GHCB. Specifically, it removes the hardcoding of the offsets
and uses the OFFSET_OF () and sizeof () functions to calculate the
values, removes unused values and add values that will be used in later
patches.

The next five patches set the SwExitCode/SwExitInfo1/SwExitInfo2/SwScratch
valid bits in the GHCB ValidBitmap area when these fields are set at
VMGEXIT.

The next two patches update the Qemu flash drive services support to
add SEV-ES support to erasing blocks and to disable interrupts when using
the GHCB.

Finally, the last patch uses the processor number for setting the AP stack
pointer instead of the APIC ID (using GetProcessorNumber()).

---

These patches are based on commit:
ae511331e0fb ("BaseTools Build_Rule: Add the missing ASM16_FLAGS for ASM16 source file")

Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>

Tom Lendacky (9):
  OvmfPkg/VmgExitLib: Update ValidBitmap settings
  OvmfPkg/VmgExitLib: Set the SW exit fields when performing VMGEXIT
  OvmfPkg/VmgExitLib: Set the SwScratch valid bit for IOIO events
  OvmfPkg/VmgExitLib: Set the SwScratch valid bit for MMIO events
  UefiCpuPkg/MpInitLib: Set the SW exit fields when performing VMGEXIT
  OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Set the SwScratch valid bit
  OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Fix erase blocks for SEV-ES
  OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Disable interrupts when using
    GHCB
  UefiCpuPkg/MpInitLib: For SEV-ES guest set stack based on processor
    number

 MdePkg/Include/Register/Amd/Ghcb.h                    | 48 ++++++++------------
 OvmfPkg/Library/VmgExitLib/VmgExitLib.c               | 30 ++++++++++++
 OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c         | 10 +++-
 OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c    |  4 +-
 OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c | 21 +++++++++
 UefiCpuPkg/Library/MpInitLib/MpLib.c                  |  7 ++-
 UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm         |  6 +++
 7 files changed, 91 insertions(+), 35 deletions(-)

-- 
2.28.0

[PATCH 1/9] OvmfPkg/VmgExitLib: Update ValidBitmap settings

[Lendacky, Thomas]

From: Tom Lendacky <thomas.lendacky@amd.com>

Use OFFSET_OF () and sizeof () to calculate the GHCB register field
offsets instead of hardcoding the values in the GHCB_REGISTER enum.
Rename GHCB_REGISTER to GHCB_QWORD_OFFSET to more appropriately describe
the enum. While redefing the values, only include (and add) fields that
are used per the GHCB specification.

Also, remove the DR7 field from the GHCB_SAVE_AREA structure since it is
not used/defined in the GHCB specification and then rename the reserved
fields as appropriate.

Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
 MdePkg/Include/Register/Amd/Ghcb.h            | 48 ++++++++------------
 OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c |  4 +-
 2 files changed, 20 insertions(+), 32 deletions(-)

diff --git a/MdePkg/Include/Register/Amd/Ghcb.h b/MdePkg/Include/Register/Amd/Ghcb.h
index 54a80da0f6d7..33c7e8939a28 100644
--- a/MdePkg/Include/Register/Amd/Ghcb.h
+++ b/MdePkg/Include/Register/Amd/Ghcb.h
@@ -82,50 +82,25 @@
 #define IOIO_SEG_DS         (BIT11 | BIT10)
 
 
-typedef enum {
-  GhcbCpl              = 25,
-  GhcbRflags           = 46,
-  GhcbRip,
-  GhcbRsp              = 59,
-  GhcbRax              = 63,
-  GhcbRcx              = 97,
-  GhcbRdx,
-  GhcbRbx,
-  GhcbRbp              = 101,
-  GhcbRsi,
-  GhcbRdi,
-  GhcbR8,
-  GhcbR9,
-  GhcbR10,
-  GhcbR11,
-  GhcbR12,
-  GhcbR13,
-  GhcbR14,
-  GhcbR15,
-  GhcbXCr0             = 125,
-} GHCB_REGISTER;
-
 typedef PACKED struct {
   UINT8                  Reserved1[203];
   UINT8                  Cpl;
-  UINT8                  Reserved2[148];
-  UINT64                 Dr7;
-  UINT8                  Reserved3[144];
+  UINT8                  Reserved2[300];
   UINT64                 Rax;
-  UINT8                  Reserved4[264];
+  UINT8                  Reserved3[264];
   UINT64                 Rcx;
   UINT64                 Rdx;
   UINT64                 Rbx;
-  UINT8                  Reserved5[112];
+  UINT8                  Reserved4[112];
   UINT64                 SwExitCode;
   UINT64                 SwExitInfo1;
   UINT64                 SwExitInfo2;
   UINT64                 SwScratch;
-  UINT8                  Reserved6[56];
+  UINT8                  Reserved5[56];
   UINT64                 XCr0;
   UINT8                  ValidBitmap[16];
   UINT64                 X87StateGpa;
-  UINT8                  Reserved7[1016];
+  UINT8                  Reserved6[1016];
 } GHCB_SAVE_AREA;
 
 typedef PACKED struct {
@@ -136,6 +111,19 @@ typedef PACKED struct {
   UINT32                 GhcbUsage;
 } GHCB;
 
+typedef enum {
+  GhcbCpl          = OFFSET_OF (GHCB, SaveArea.Cpl) / sizeof (UINT64),
+  GhcbRax          = OFFSET_OF (GHCB, SaveArea.Rax) / sizeof (UINT64),
+  GhcbRbx          = OFFSET_OF (GHCB, SaveArea.Rbx) / sizeof (UINT64),
+  GhcbRcx          = OFFSET_OF (GHCB, SaveArea.Rcx) / sizeof (UINT64),
+  GhcbRdx          = OFFSET_OF (GHCB, SaveArea.Rdx) / sizeof (UINT64),
+  GhcbXCr0         = OFFSET_OF (GHCB, SaveArea.XCr0) / sizeof (UINT64),
+  GhcbSwExitCode   = OFFSET_OF (GHCB, SaveArea.SwExitCode) / sizeof (UINT64),
+  GhcbSwExitInfo1  = OFFSET_OF (GHCB, SaveArea.SwExitInfo1) / sizeof (UINT64),
+  GhcbSwExitInfo2  = OFFSET_OF (GHCB, SaveArea.SwExitInfo2) / sizeof (UINT64),
+  GhcbSwScratch    = OFFSET_OF (GHCB, SaveArea.SwScratch) / sizeof (UINT64),
+} GHCB_QWORD_OFFSET;
+
 typedef union {
   struct {
     UINT32  Lower32Bits;
diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
index 8e42b305e83c..c5484a3f478c 100644
--- a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
+++ b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
@@ -153,7 +153,7 @@ STATIC
 BOOLEAN
 GhcbIsRegValid (
   IN GHCB                *Ghcb,
-  IN GHCB_REGISTER       Reg
+  IN GHCB_QWORD_OFFSET   Reg
   )
 {
   UINT32  RegIndex;
@@ -179,7 +179,7 @@ STATIC
 VOID
 GhcbSetRegValid (
   IN OUT GHCB                *Ghcb,
-  IN     GHCB_REGISTER       Reg
+  IN     GHCB_QWORD_OFFSET   Reg
   )
 {
   UINT32  RegIndex;
-- 
2.28.0

[PATCH 2/9] OvmfPkg/VmgExitLib: Set the SW exit fields when performing VMGEXIT

[Lendacky, Thomas]

From: Tom Lendacky <thomas.lendacky@amd.com>

All fields that are set in the GHCB should have their associated bit in
the GHCB ValidBitmap field set. Add support to set the bits for the
software exit information fields when performing a VMGEXIT (SwExitCode,
SwExitInfo1, SwExitInfo2).

Fixes: 61bacc0fa16f ("OvmfPkg/VmgExitLib: Implement library support for VmgExitLib in OVMF")
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
 OvmfPkg/Library/VmgExitLib/VmgExitLib.c | 30 ++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitLib.c b/OvmfPkg/Library/VmgExitLib/VmgExitLib.c
index 53040cc6f649..6cf649c6101b 100644
--- a/OvmfPkg/Library/VmgExitLib/VmgExitLib.c
+++ b/OvmfPkg/Library/VmgExitLib/VmgExitLib.c
@@ -78,6 +78,32 @@ VmgExitErrorCheck (
   return Status;
 }
 
+/**
+  Marks a field at the specified offset as valid in the GHCB.
+
+  The ValidBitmap area represents the areas of the GHCB that have been marked
+  valid. Set the area of the GHCB at the specified offset as valid.
+
+  @param[in, out] Ghcb    Pointer to the Guest-Hypervisor Communication Block
+  @param[in] Offset       Offset in the GHCB to mark valid
+
+**/
+STATIC
+VOID
+GhcbSetOffsetValid (
+  IN OUT GHCB               *Ghcb,
+  IN     GHCB_QWORD_OFFSET  Offset
+  )
+{
+  UINT32  OffsetIndex;
+  UINT32  OffsetBit;
+
+  OffsetIndex = Offset / 8;
+  OffsetBit   = Offset & 0x07;
+
+  Ghcb->SaveArea.ValidBitmap[OffsetIndex] |= (1 << OffsetBit);
+}
+
 /**
   Perform VMGEXIT.
 
@@ -110,6 +136,10 @@ VmgExit (
   Ghcb->SaveArea.SwExitInfo1 = ExitInfo1;
   Ghcb->SaveArea.SwExitInfo2 = ExitInfo2;
 
+  GhcbSetOffsetValid (Ghcb, GhcbSwExitCode);
+  GhcbSetOffsetValid (Ghcb, GhcbSwExitInfo1);
+  GhcbSetOffsetValid (Ghcb, GhcbSwExitInfo2);
+
   //
   // Guest memory is used for the guest-hypervisor communication, so fence
   // the invocation of the VMGEXIT instruction to ensure GHCB accesses are
-- 
2.28.0

[PATCH 3/9] OvmfPkg/VmgExitLib: Set the SwScratch valid bit for IOIO events

[Lendacky, Thomas]

From: Tom Lendacky <thomas.lendacky@amd.com>

All fields that are set in the GHCB should have their associated bit in
the GHCB ValidBitmap field set. Add support to set the bit for the scratch
area field (SwScratch).

Fixes: 0020157a9825 ("OvmfPkg/VmgExitLib: Support string IO for IOIO_PROT NAE events")
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
 OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
index c5484a3f478c..40120e29af18 100644
--- a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
+++ b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
@@ -1289,6 +1289,7 @@ IoioExit (
       }
 
       Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
+      GhcbSetRegValid (Ghcb, GhcbSwScratch);
       Status = VmgExit (Ghcb, SVM_EXIT_IOIO_PROT, ExitInfo1, ExitInfo2);
       if (Status != 0) {
         return Status;
-- 
2.28.0

[PATCH 4/9] OvmfPkg/VmgExitLib: Set the SwScratch valid bit for MMIO events

[Lendacky, Thomas]

From: Tom Lendacky <thomas.lendacky@amd.com>

All fields that are set in the GHCB should have their associated bit in
the GHCB ValidBitmap field set. Add support to set the bit for the scratch
area field (SwScratch).

Fixes: c45f678a1ea2 ("OvmfPkg/VmgExitLib: Add support for NPF NAE events (MMIO)")
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
 OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
index 40120e29af18..6d6fe6800031 100644
--- a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
+++ b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
@@ -720,6 +720,7 @@ MmioExit (
     CopyMem (Ghcb->SharedBuffer, &InstructionData->Ext.RegData, Bytes);
 
     Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
+    GhcbSetRegValid (Ghcb, GhcbSwScratch);
     Status = VmgExit (Ghcb, SVM_EXIT_MMIO_WRITE, ExitInfo1, ExitInfo2);
     if (Status != 0) {
       return Status;
@@ -749,6 +750,7 @@ MmioExit (
     CopyMem (Ghcb->SharedBuffer, InstructionData->Immediate, Bytes);
 
     Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
+    GhcbSetRegValid (Ghcb, GhcbSwScratch);
     Status = VmgExit (Ghcb, SVM_EXIT_MMIO_WRITE, ExitInfo1, ExitInfo2);
     if (Status != 0) {
       return Status;
@@ -781,6 +783,7 @@ MmioExit (
     ExitInfo2 = Bytes;
 
     Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
+    GhcbSetRegValid (Ghcb, GhcbSwScratch);
     Status = VmgExit (Ghcb, SVM_EXIT_MMIO_READ, ExitInfo1, ExitInfo2);
     if (Status != 0) {
       return Status;
@@ -811,6 +814,7 @@ MmioExit (
     ExitInfo2 = Bytes;
 
     Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
+    GhcbSetRegValid (Ghcb, GhcbSwScratch);
     Status = VmgExit (Ghcb, SVM_EXIT_MMIO_READ, ExitInfo1, ExitInfo2);
     if (Status != 0) {
       return Status;
@@ -836,6 +840,7 @@ MmioExit (
     ExitInfo2 = Bytes;
 
     Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
+    GhcbSetRegValid (Ghcb, GhcbSwScratch);
     Status = VmgExit (Ghcb, SVM_EXIT_MMIO_READ, ExitInfo1, ExitInfo2);
     if (Status != 0) {
       return Status;
-- 
2.28.0

[PATCH 5/9] UefiCpuPkg/MpInitLib: Set the SW exit fields when performing VMGEXIT

[Lendacky, Thomas]

From: Tom Lendacky <thomas.lendacky@amd.com>

All fields that are set in the GHCB should have their associated bit in
the GHCB ValidBitmap field set. Add support to set the bits for the
software exit information fields when performing a VMGEXIT (SwExitCode,
SwExitInfo1, SwExitInfo2).

Fixes: 20da7ca42a33 ("UefiCpuPkg/MpInitLib: Prepare SEV-ES guest APs for OS use")
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
 UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
index 5d30f35b201c..5532a1d391bc 100644
--- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
+++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
@@ -533,6 +533,12 @@ BITS 64
 
     mov        rax, 0x80000004   ; VMGEXIT AP_RESET_HOLD
     mov        [rdx + 0x390], rax
+    mov        rax, 114          ; Set SwExitCode valid bit
+    bts        [rdx + 0x3f0], rax
+    inc        rax               ; Set SwExitInfo1 valid bit
+    bts        [rdx + 0x3f0], rax
+    inc        rax               ; Set SwExitInfo2 valid bit
+    bts        [rdx + 0x3f0], rax
 
     pop        rdx
     pop        rcx
-- 
2.28.0

[PATCH 6/9] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Set the SwScratch valid bit

[Lendacky, Thomas]

From: Tom Lendacky <thomas.lendacky@amd.com>

All fields that are set in the GHCB should have their associated bit in
the GHCB ValidBitmap field set. Add support to set the bit for the scratch
area field (SwScratch).

Fixes: 437eb3f7a8db ("OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Bypass flash detection with SEV-ES")
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
 OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
index 565383ee26d2..5d5a117c48e0 100644
--- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
+++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
@@ -52,10 +52,15 @@ QemuFlashPtrWrite (
   if (MemEncryptSevEsIsEnabled ()) {
     MSR_SEV_ES_GHCB_REGISTER  Msr;
     GHCB                      *Ghcb;
+    UINT32                    ScratchIndex;
+    UINT32                    ScratchBit;
 
     Msr.GhcbPhysicalAddress = AsmReadMsr64 (MSR_SEV_ES_GHCB);
     Ghcb = Msr.Ghcb;
 
+    ScratchIndex = GhcbSwScratch / 8;
+    ScratchBit   = GhcbSwScratch & 0x07;
+
     //
     // Writing to flash is emulated by the hypervisor through the use of write
     // protection. This won't work for an SEV-ES guest because the write won't
@@ -66,6 +71,7 @@ QemuFlashPtrWrite (
     VmgInit (Ghcb);
     Ghcb->SharedBuffer[0] = Value;
     Ghcb->SaveArea.SwScratch = (UINT64) (UINTN) Ghcb->SharedBuffer;
+    Ghcb->SaveArea.ValidBitmap[ScratchIndex] |= (1 << ScratchBit);
     VmgExit (Ghcb, SVM_EXIT_MMIO_WRITE, (UINT64) (UINTN) Ptr, 1);
     VmgDone (Ghcb);
   } else {
-- 
2.28.0

[PATCH 7/9] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Fix erase blocks for SEV-ES

[Lendacky, Thomas]

From: Tom Lendacky <thomas.lendacky@amd.com>

The original SEV-ES support missed updating the QemuFlashEraseBlock()
function to successfully erase blocks. Update QemuFlashEraseBlock() to
call the QemuFlashPtrWrite() to be able to successfully perform the
commands under SEV-ES.

Fixes: 437eb3f7a8db ("OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Bypass flash detection with SEV-ES")
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
 OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c
index 0d29bf701aca..d19997032ec9 100644
--- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c
+++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c
@@ -232,8 +232,8 @@ QemuFlashEraseBlock (
   }
 
   Ptr = QemuFlashPtr (Lba, 0);
-  *Ptr = BLOCK_ERASE_CMD;
-  *Ptr = BLOCK_ERASE_CONFIRM_CMD;
+  QemuFlashPtrWrite (Ptr, BLOCK_ERASE_CMD);
+  QemuFlashPtrWrite (Ptr, BLOCK_ERASE_CONFIRM_CMD);
   return EFI_SUCCESS;
 }
 
-- 
2.28.0

[PATCH 8/9] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Disable interrupts when using GHCB

[Lendacky, Thomas]

From: Tom Lendacky <thomas.lendacky@amd.com>

The QemuFlashPtrWrite() flash services runtime uses the GHCB and VmgExit()
directly to perform the flash write when running as an SEV-ES guest. If an
interrupt arrives between VmgInit() and VmgDone(), the Dr7 read in the
interrupt handler will generate a #VC, which can overwrite information in
the GHCB that QemuFlashPtrWrite() has set. Prevent this from occurring by
disabling interrupts around the usage of the GHCB.

Fixes: 437eb3f7a8db ("OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Bypass flash detection with SEV-ES")
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
 OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
index 5d5a117c48e0..872e58db7cc0 100644
--- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
+++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
@@ -9,6 +9,7 @@
 
 **/
 
+#include <Library/BaseLib.h>
 #include <Library/UefiRuntimeLib.h>
 #include <Library/MemEncryptSevLib.h>
 #include <Library/VmgExitLib.h>
@@ -54,6 +55,7 @@ QemuFlashPtrWrite (
     GHCB                      *Ghcb;
     UINT32                    ScratchIndex;
     UINT32                    ScratchBit;
+    BOOLEAN                   InterruptsEnabled;
 
     Msr.GhcbPhysicalAddress = AsmReadMsr64 (MSR_SEV_ES_GHCB);
     Ghcb = Msr.Ghcb;
@@ -61,6 +63,15 @@ QemuFlashPtrWrite (
     ScratchIndex = GhcbSwScratch / 8;
     ScratchBit   = GhcbSwScratch & 0x07;
 
+    //
+    // Be sure that an interrupt can't cause a #VC while the GHCB is
+    // being used.
+    //
+    InterruptsEnabled = GetInterruptState ();
+    if (InterruptsEnabled) {
+      DisableInterrupts ();
+    }
+
     //
     // Writing to flash is emulated by the hypervisor through the use of write
     // protection. This won't work for an SEV-ES guest because the write won't
@@ -74,6 +85,10 @@ QemuFlashPtrWrite (
     Ghcb->SaveArea.ValidBitmap[ScratchIndex] |= (1 << ScratchBit);
     VmgExit (Ghcb, SVM_EXIT_MMIO_WRITE, (UINT64) (UINTN) Ptr, 1);
     VmgDone (Ghcb);
+
+    if (InterruptsEnabled) {
+      EnableInterrupts ();
+    }
   } else {
     *Ptr = Value;
   }
-- 
2.28.0

[PATCH 9/9] UefiCpuPkg/MpInitLib: For SEV-ES guest set stack based on processor number

[Lendacky, Thomas]

From: Tom Lendacky <thomas.lendacky@amd.com>

Set the SEV-ES reset stack address for an AP based on the processor number
instead of the APIC ID in case the APIC IDs are not zero-based and densely
packed/enumerated. This will ensure an AP reset stack address does not get
set outside of the AP reset stack memory allocation.

Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
 UefiCpuPkg/Library/MpInitLib/MpLib.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index 07426274f639..71922141b70b 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -680,11 +680,16 @@ MpInitLibSevEsAPReset (
   IN CPU_MP_DATA                  *CpuMpData
   )
 {
+  EFI_STATUS       Status;
+  UINTN            ProcessorNumber;
   UINT16           Code16, Code32;
   AP_RESET         *APResetFn;
   UINTN            BufferStart;
   UINTN            StackStart;
 
+  Status = GetProcessorNumber (CpuMpData, &ProcessorNumber);
+  ASSERT_EFI_ERROR (Status);
+
   Code16 = GetProtectedMode16CS ();
   Code32 = GetProtectedMode32CS ();
 
@@ -696,7 +701,7 @@ MpInitLibSevEsAPReset (
 
   BufferStart = CpuMpData->MpCpuExchangeInfo->BufferStart;
   StackStart = CpuMpData->SevEsAPResetStackStart -
-                 (AP_RESET_STACK_SIZE * GetApicId ());
+                 (AP_RESET_STACK_SIZE * ProcessorNumber);
 
   //
   // This call never returns.
-- 
2.28.0

Re: [PATCH 9/9] UefiCpuPkg/MpInitLib: For SEV-ES guest set stack based on processor number

[Ni, Ray]

Acked-by: Ray Ni <ray.ni@intel.com>

> -----Original Message-----
> From: Tom Lendacky <thomas.lendacky@amd.com>
> Sent: Sunday, October 11, 2020 12:07 AM
> To: devel@edk2.groups.io
> Cc: Brijesh Singh <brijesh.singh@amd.com>; Dong, Eric <eric.dong@intel.com>;
> Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; Kumar, Rahul1
> <rahul1.kumar@intel.com>
> Subject: [PATCH 9/9] UefiCpuPkg/MpInitLib: For SEV-ES guest set stack based
> on processor number
> 
> From: Tom Lendacky <thomas.lendacky@amd.com>
> 
> Set the SEV-ES reset stack address for an AP based on the processor number
> instead of the APIC ID in case the APIC IDs are not zero-based and densely
> packed/enumerated. This will ensure an AP reset stack address does not get
> set outside of the AP reset stack memory allocation.
> 
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/MpLib.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 07426274f639..71922141b70b 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -680,11 +680,16 @@ MpInitLibSevEsAPReset (
>    IN CPU_MP_DATA                  *CpuMpData
>    )
>  {
> +  EFI_STATUS       Status;
> +  UINTN            ProcessorNumber;
>    UINT16           Code16, Code32;
>    AP_RESET         *APResetFn;
>    UINTN            BufferStart;
>    UINTN            StackStart;
> 
> +  Status = GetProcessorNumber (CpuMpData, &ProcessorNumber);
> +  ASSERT_EFI_ERROR (Status);
> +
>    Code16 = GetProtectedMode16CS ();
>    Code32 = GetProtectedMode32CS ();
> 
> @@ -696,7 +701,7 @@ MpInitLibSevEsAPReset (
> 
>    BufferStart = CpuMpData->MpCpuExchangeInfo->BufferStart;
>    StackStart = CpuMpData->SevEsAPResetStackStart -
> -                 (AP_RESET_STACK_SIZE * GetApicId ());
> +                 (AP_RESET_STACK_SIZE * ProcessorNumber);
> 
>    //
>    // This call never returns.
> --
> 2.28.0

Re: [PATCH 5/9] UefiCpuPkg/MpInitLib: Set the SW exit fields when performing VMGEXIT

[Ni, Ray]

Acked-by: Ray Ni <ray.ni@intel.com>

> -----Original Message-----
> From: Tom Lendacky <thomas.lendacky@amd.com>
> Sent: Sunday, October 11, 2020 12:07 AM
> To: devel@edk2.groups.io
> Cc: Brijesh Singh <brijesh.singh@amd.com>; Dong, Eric <eric.dong@intel.com>;
> Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; Kumar, Rahul1
> <rahul1.kumar@intel.com>
> Subject: [PATCH 5/9] UefiCpuPkg/MpInitLib: Set the SW exit fields when
> performing VMGEXIT
> 
> From: Tom Lendacky <thomas.lendacky@amd.com>
> 
> All fields that are set in the GHCB should have their associated bit in
> the GHCB ValidBitmap field set. Add support to set the bits for the
> software exit information fields when performing a VMGEXIT (SwExitCode,
> SwExitInfo1, SwExitInfo2).
> 
> Fixes: 20da7ca42a33 ("UefiCpuPkg/MpInitLib: Prepare SEV-ES guest APs for OS
> use")
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> index 5d30f35b201c..5532a1d391bc 100644
> --- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> +++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> @@ -533,6 +533,12 @@ BITS 64
> 
>      mov        rax, 0x80000004   ; VMGEXIT AP_RESET_HOLD
>      mov        [rdx + 0x390], rax
> +    mov        rax, 114          ; Set SwExitCode valid bit
> +    bts        [rdx + 0x3f0], rax
> +    inc        rax               ; Set SwExitInfo1 valid bit
> +    bts        [rdx + 0x3f0], rax
> +    inc        rax               ; Set SwExitInfo2 valid bit
> +    bts        [rdx + 0x3f0], rax
> 
>      pop        rdx
>      pop        rcx
> --
> 2.28.0

Re: [PATCH 0/9] SEV-ES guest support fixes and cleanup

[Laszlo Ersek]

Hi Tom,

On 10/10/20 18:06, Tom Lendacky wrote:
> From: Tom Lendacky <thomas.lendacky@amd.com>
> 
> This patch series provides some fixes, updates and cleanup to the SEV-ES
> guest support:
> 
> The first patch updates the calculation of the qword offset of fields
> within the GHCB. Specifically, it removes the hardcoding of the offsets
> and uses the OFFSET_OF () and sizeof () functions to calculate the
> values, removes unused values and add values that will be used in later
> patches.
> 
> The next five patches set the SwExitCode/SwExitInfo1/SwExitInfo2/SwScratch
> valid bits in the GHCB ValidBitmap area when these fields are set at
> VMGEXIT.
> 
> The next two patches update the Qemu flash drive services support to
> add SEV-ES support to erasing blocks and to disable interrupts when using
> the GHCB.
> 
> Finally, the last patch uses the processor number for setting the AP stack
> pointer instead of the APIC ID (using GetProcessorNumber()).

please file a TianoCore BZ for this series, assign it to yourself, link
the v1 posting in a comment on the BZ, and update the commit messages to
reference that BZ.

I find this relevant because edk2-stable202008 resolved TianoCore#2198.
If (in your opinion) downstreams that aim at supporting SEV-ES should
also have these patches (for example, if they should backport them on
top of edk2-stable202008), then having a TianoCore Bugzilla would be
quite helpful to them, for tracking purposes.

Thanks,
Laszlo

> 
> ---
> 
> These patches are based on commit:
> ae511331e0fb ("BaseTools Build_Rule: Add the missing ASM16_FLAGS for ASM16 source file")
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> 
> Tom Lendacky (9):
>   OvmfPkg/VmgExitLib: Update ValidBitmap settings
>   OvmfPkg/VmgExitLib: Set the SW exit fields when performing VMGEXIT
>   OvmfPkg/VmgExitLib: Set the SwScratch valid bit for IOIO events
>   OvmfPkg/VmgExitLib: Set the SwScratch valid bit for MMIO events
>   UefiCpuPkg/MpInitLib: Set the SW exit fields when performing VMGEXIT
>   OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Set the SwScratch valid bit
>   OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Fix erase blocks for SEV-ES
>   OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Disable interrupts when using
>     GHCB
>   UefiCpuPkg/MpInitLib: For SEV-ES guest set stack based on processor
>     number
> 
>  MdePkg/Include/Register/Amd/Ghcb.h                    | 48 ++++++++------------
>  OvmfPkg/Library/VmgExitLib/VmgExitLib.c               | 30 ++++++++++++
>  OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c         | 10 +++-
>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c    |  4 +-
>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c | 21 +++++++++
>  UefiCpuPkg/Library/MpInitLib/MpLib.c                  |  7 ++-
>  UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm         |  6 +++
>  7 files changed, 91 insertions(+), 35 deletions(-)
> 

Re: [PATCH 1/9] OvmfPkg/VmgExitLib: Update ValidBitmap settings

[Laszlo Ersek]

Hi Tom,

On 10/10/20 18:06, Tom Lendacky wrote:
> From: Tom Lendacky <thomas.lendacky@amd.com>
> 
> Use OFFSET_OF () and sizeof () to calculate the GHCB register field
> offsets instead of hardcoding the values in the GHCB_REGISTER enum.
> Rename GHCB_REGISTER to GHCB_QWORD_OFFSET to more appropriately describe
> the enum. While redefing the values, only include (and add) fields that
> are used per the GHCB specification.
> 
> Also, remove the DR7 field from the GHCB_SAVE_AREA structure since it is
> not used/defined in the GHCB specification and then rename the reserved
> fields as appropriate.
> 
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>  MdePkg/Include/Register/Amd/Ghcb.h            | 48 ++++++++------------
>  OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c |  4 +-
>  2 files changed, 20 insertions(+), 32 deletions(-)

This patch modifies both MdePkg and OvmfPkg. I agree that, as an
exception, this is the right thing to do.

That's because we are not changing the identifiers of the enumeration
constants (such as GhcbCpl). Because those identifiers are declared at
file scope, having both GHCB_REGISTER and GHCB_QWORD_OFFSET declare
(e.g.) GhcbCpl would cause a compilation failure. Therefore we can't
implement this in multiple stages (first introduce GHCB_QWORD_OFFSET,
then remove GHCB_REGISTER separately).

(1) However, the subject line doesn't look correct. It should mention
both MdePkg and OvmfPkg. Also, we're not updating ValidBitmap settings
just yet.

I suggest:

  MdePkg, OvmfPkg: clean up GHCB field offsets and save area

> 
> diff --git a/MdePkg/Include/Register/Amd/Ghcb.h b/MdePkg/Include/Register/Amd/Ghcb.h
> index 54a80da0f6d7..33c7e8939a28 100644
> --- a/MdePkg/Include/Register/Amd/Ghcb.h
> +++ b/MdePkg/Include/Register/Amd/Ghcb.h
> @@ -82,50 +82,25 @@
>  #define IOIO_SEG_DS         (BIT11 | BIT10)
>  
>  
> -typedef enum {
> -  GhcbCpl              = 25,
> -  GhcbRflags           = 46,
> -  GhcbRip,
> -  GhcbRsp              = 59,
> -  GhcbRax              = 63,
> -  GhcbRcx              = 97,
> -  GhcbRdx,
> -  GhcbRbx,
> -  GhcbRbp              = 101,
> -  GhcbRsi,
> -  GhcbRdi,
> -  GhcbR8,
> -  GhcbR9,
> -  GhcbR10,
> -  GhcbR11,
> -  GhcbR12,
> -  GhcbR13,
> -  GhcbR14,
> -  GhcbR15,
> -  GhcbXCr0             = 125,
> -} GHCB_REGISTER;
> -
>  typedef PACKED struct {
>    UINT8                  Reserved1[203];
>    UINT8                  Cpl;
> -  UINT8                  Reserved2[148];
> -  UINT64                 Dr7;
> -  UINT8                  Reserved3[144];
> +  UINT8                  Reserved2[300];
>    UINT64                 Rax;
> -  UINT8                  Reserved4[264];
> +  UINT8                  Reserved3[264];
>    UINT64                 Rcx;
>    UINT64                 Rdx;
>    UINT64                 Rbx;
> -  UINT8                  Reserved5[112];
> +  UINT8                  Reserved4[112];
>    UINT64                 SwExitCode;
>    UINT64                 SwExitInfo1;
>    UINT64                 SwExitInfo2;
>    UINT64                 SwScratch;
> -  UINT8                  Reserved6[56];
> +  UINT8                  Reserved5[56];
>    UINT64                 XCr0;
>    UINT8                  ValidBitmap[16];
>    UINT64                 X87StateGpa;
> -  UINT8                  Reserved7[1016];
> +  UINT8                  Reserved6[1016];
>  } GHCB_SAVE_AREA;

(2) The meaning of a field called "ReservedN" must never change (it must
describe the same offset range in the structure, after introduction). If
we need to change the structure incompatibly, then please remove the old
ReservedN field name altogether. If a new reserved field has to be
introduced, in addition, then please find the largest N used with
Reserved fields in the structure currently, and use (N+1) in the name of
the new field.

This practice makes sure that any (out of tree) code that refers to a
ReservedN field in MdePkg will cleanly break during compilation after
this change. Changing the meaning of a ReservedN field could otherwise
cause misbehavior that could be harder to track down.

I realize this is somewhat unusual -- after all, if someone deliberately
accessed a field called "Reserved", any subsequent breakage is *their*
fault. However, the reason for this practice is that sometimes MdePkg
headers lag behind industry specs (meaning, non-UEFI specs), but drivers
or libs (outside of MdePkg or even edk2) already need to refer to
newly-specified fields. Even edk2 used to have code similar to the example

  Reserved3[12] = 1;

Normally I don't advocate accommodating out-of-tree code, but following
this ReservedN scheme isn't hard.


>  
>  typedef PACKED struct {
> @@ -136,6 +111,19 @@ typedef PACKED struct {
>    UINT32                 GhcbUsage;
>  } GHCB;
>  
> +typedef enum {
> +  GhcbCpl          = OFFSET_OF (GHCB, SaveArea.Cpl) / sizeof (UINT64),
> +  GhcbRax          = OFFSET_OF (GHCB, SaveArea.Rax) / sizeof (UINT64),
> +  GhcbRbx          = OFFSET_OF (GHCB, SaveArea.Rbx) / sizeof (UINT64),
> +  GhcbRcx          = OFFSET_OF (GHCB, SaveArea.Rcx) / sizeof (UINT64),
> +  GhcbRdx          = OFFSET_OF (GHCB, SaveArea.Rdx) / sizeof (UINT64),
> +  GhcbXCr0         = OFFSET_OF (GHCB, SaveArea.XCr0) / sizeof (UINT64),
> +  GhcbSwExitCode   = OFFSET_OF (GHCB, SaveArea.SwExitCode) / sizeof (UINT64),
> +  GhcbSwExitInfo1  = OFFSET_OF (GHCB, SaveArea.SwExitInfo1) / sizeof (UINT64),
> +  GhcbSwExitInfo2  = OFFSET_OF (GHCB, SaveArea.SwExitInfo2) / sizeof (UINT64),
> +  GhcbSwScratch    = OFFSET_OF (GHCB, SaveArea.SwScratch) / sizeof (UINT64),
> +} GHCB_QWORD_OFFSET;
> +
>  typedef union {
>    struct {
>      UINT32  Lower32Bits;
> diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
> index 8e42b305e83c..c5484a3f478c 100644
> --- a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
> +++ b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
> @@ -153,7 +153,7 @@ STATIC
>  BOOLEAN
>  GhcbIsRegValid (
>    IN GHCB                *Ghcb,
> -  IN GHCB_REGISTER       Reg
> +  IN GHCB_QWORD_OFFSET   Reg
>    )
>  {
>    UINT32  RegIndex;
> @@ -179,7 +179,7 @@ STATIC
>  VOID
>  GhcbSetRegValid (
>    IN OUT GHCB                *Ghcb,
> -  IN     GHCB_REGISTER       Reg
> +  IN     GHCB_QWORD_OFFSET   Reg
>    )
>  {
>    UINT32  RegIndex;
> 

Thanks,
Laszlo

Re: [PATCH 2/9] OvmfPkg/VmgExitLib: Set the SW exit fields when performing VMGEXIT

[Laszlo Ersek]

On 10/10/20 18:07, Tom Lendacky wrote:
> From: Tom Lendacky <thomas.lendacky@amd.com>
> 
> All fields that are set in the GHCB should have their associated bit in
> the GHCB ValidBitmap field set. Add support to set the bits for the
> software exit information fields when performing a VMGEXIT (SwExitCode,
> SwExitInfo1, SwExitInfo2).
> 
> Fixes: 61bacc0fa16f ("OvmfPkg/VmgExitLib: Implement library support for VmgExitLib in OVMF")
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>  OvmfPkg/Library/VmgExitLib/VmgExitLib.c | 30 ++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitLib.c b/OvmfPkg/Library/VmgExitLib/VmgExitLib.c
> index 53040cc6f649..6cf649c6101b 100644
> --- a/OvmfPkg/Library/VmgExitLib/VmgExitLib.c
> +++ b/OvmfPkg/Library/VmgExitLib/VmgExitLib.c
> @@ -78,6 +78,32 @@ VmgExitErrorCheck (
>    return Status;
>  }
>  
> +/**
> +  Marks a field at the specified offset as valid in the GHCB.
> +
> +  The ValidBitmap area represents the areas of the GHCB that have been marked
> +  valid. Set the area of the GHCB at the specified offset as valid.
> +
> +  @param[in, out] Ghcb    Pointer to the Guest-Hypervisor Communication Block
> +  @param[in] Offset       Offset in the GHCB to mark valid
> +
> +**/
> +STATIC
> +VOID
> +GhcbSetOffsetValid (
> +  IN OUT GHCB               *Ghcb,
> +  IN     GHCB_QWORD_OFFSET  Offset
> +  )
> +{
> +  UINT32  OffsetIndex;
> +  UINT32  OffsetBit;
> +
> +  OffsetIndex = Offset / 8;
> +  OffsetBit   = Offset & 0x07;

(1) I suggest improving the consistency of operators -- please either
use division and remainder ("Offset / 8" and "Offset % 8"), or bit shift
and masking ("Offset >> 3" and "Offset & 0x7")

With that:

Acked-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo

> +
> +  Ghcb->SaveArea.ValidBitmap[OffsetIndex] |= (1 << OffsetBit);
> +}
> +
>  /**
>    Perform VMGEXIT.
>  
> @@ -110,6 +136,10 @@ VmgExit (
>    Ghcb->SaveArea.SwExitInfo1 = ExitInfo1;
>    Ghcb->SaveArea.SwExitInfo2 = ExitInfo2;
>  
> +  GhcbSetOffsetValid (Ghcb, GhcbSwExitCode);
> +  GhcbSetOffsetValid (Ghcb, GhcbSwExitInfo1);
> +  GhcbSetOffsetValid (Ghcb, GhcbSwExitInfo2);
> +
>    //
>    // Guest memory is used for the guest-hypervisor communication, so fence
>    // the invocation of the VMGEXIT instruction to ensure GHCB accesses are
> 

Re: [PATCH 3/9] OvmfPkg/VmgExitLib: Set the SwScratch valid bit for IOIO events

[Laszlo Ersek]

On 10/10/20 18:07, Tom Lendacky wrote:
> From: Tom Lendacky <thomas.lendacky@amd.com>
> 
> All fields that are set in the GHCB should have their associated bit in
> the GHCB ValidBitmap field set. Add support to set the bit for the scratch
> area field (SwScratch).
> 
> Fixes: 0020157a9825 ("OvmfPkg/VmgExitLib: Support string IO for IOIO_PROT NAE events")
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>  OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
> index c5484a3f478c..40120e29af18 100644
> --- a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
> +++ b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
> @@ -1289,6 +1289,7 @@ IoioExit (
>        }
>  
>        Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
> +      GhcbSetRegValid (Ghcb, GhcbSwScratch);
>        Status = VmgExit (Ghcb, SVM_EXIT_IOIO_PROT, ExitInfo1, ExitInfo2);
>        if (Status != 0) {
>          return Status;
> 

Acked-by: Laszlo Ersek <lersek@redhat.com>

Re: [PATCH 2/9] OvmfPkg/VmgExitLib: Set the SW exit fields when performing VMGEXIT

[Laszlo Ersek]

On 10/15/20 10:47, Laszlo Ersek wrote:
> On 10/10/20 18:07, Tom Lendacky wrote:
>> From: Tom Lendacky <thomas.lendacky@amd.com>
>>
>> All fields that are set in the GHCB should have their associated bit in
>> the GHCB ValidBitmap field set. Add support to set the bits for the
>> software exit information fields when performing a VMGEXIT (SwExitCode,
>> SwExitInfo1, SwExitInfo2).
>>
>> Fixes: 61bacc0fa16f ("OvmfPkg/VmgExitLib: Implement library support for VmgExitLib in OVMF")
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>> Cc: Brijesh Singh <brijesh.singh@amd.com>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>> ---
>>  OvmfPkg/Library/VmgExitLib/VmgExitLib.c | 30 ++++++++++++++++++++
>>  1 file changed, 30 insertions(+)
>>
>> diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitLib.c b/OvmfPkg/Library/VmgExitLib/VmgExitLib.c
>> index 53040cc6f649..6cf649c6101b 100644
>> --- a/OvmfPkg/Library/VmgExitLib/VmgExitLib.c
>> +++ b/OvmfPkg/Library/VmgExitLib/VmgExitLib.c
>> @@ -78,6 +78,32 @@ VmgExitErrorCheck (
>>    return Status;
>>  }
>>  
>> +/**
>> +  Marks a field at the specified offset as valid in the GHCB.
>> +
>> +  The ValidBitmap area represents the areas of the GHCB that have been marked
>> +  valid. Set the area of the GHCB at the specified offset as valid.
>> +
>> +  @param[in, out] Ghcb    Pointer to the Guest-Hypervisor Communication Block
>> +  @param[in] Offset       Offset in the GHCB to mark valid
>> +
>> +**/
>> +STATIC
>> +VOID
>> +GhcbSetOffsetValid (
>> +  IN OUT GHCB               *Ghcb,
>> +  IN     GHCB_QWORD_OFFSET  Offset
>> +  )
>> +{
>> +  UINT32  OffsetIndex;
>> +  UINT32  OffsetBit;
>> +
>> +  OffsetIndex = Offset / 8;
>> +  OffsetBit   = Offset & 0x07;
> 
> (1) I suggest improving the consistency of operators -- please either
> use division and remainder ("Offset / 8" and "Offset % 8"), or bit shift
> and masking ("Offset >> 3" and "Offset & 0x7")
> 
> With that:
> 
> Acked-by: Laszlo Ersek <lersek@redhat.com>

... I realize I didn't make the same comment for GhcbIsRegValid(), so
I'm taking back the above -- consistency with GhcbIsRegValid() is more
important. No change needed here.

Acked-by: Laszlo Ersek <lersek@redhat.com>

Thanks,
Laszlo

> 
> Thanks
> Laszlo
> 
>> +
>> +  Ghcb->SaveArea.ValidBitmap[OffsetIndex] |= (1 << OffsetBit);
>> +}
>> +
>>  /**
>>    Perform VMGEXIT.
>>  
>> @@ -110,6 +136,10 @@ VmgExit (
>>    Ghcb->SaveArea.SwExitInfo1 = ExitInfo1;
>>    Ghcb->SaveArea.SwExitInfo2 = ExitInfo2;
>>  
>> +  GhcbSetOffsetValid (Ghcb, GhcbSwExitCode);
>> +  GhcbSetOffsetValid (Ghcb, GhcbSwExitInfo1);
>> +  GhcbSetOffsetValid (Ghcb, GhcbSwExitInfo2);
>> +
>>    //
>>    // Guest memory is used for the guest-hypervisor communication, so fence
>>    // the invocation of the VMGEXIT instruction to ensure GHCB accesses are
>>
> 

Re: [PATCH 4/9] OvmfPkg/VmgExitLib: Set the SwScratch valid bit for MMIO events

[Laszlo Ersek]

On 10/10/20 18:07, Tom Lendacky wrote:
> From: Tom Lendacky <thomas.lendacky@amd.com>
> 
> All fields that are set in the GHCB should have their associated bit in
> the GHCB ValidBitmap field set. Add support to set the bit for the scratch
> area field (SwScratch).
> 
> Fixes: c45f678a1ea2 ("OvmfPkg/VmgExitLib: Add support for NPF NAE events (MMIO)")
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>  OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
> index 40120e29af18..6d6fe6800031 100644
> --- a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
> +++ b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
> @@ -720,6 +720,7 @@ MmioExit (
>      CopyMem (Ghcb->SharedBuffer, &InstructionData->Ext.RegData, Bytes);
>  
>      Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
> +    GhcbSetRegValid (Ghcb, GhcbSwScratch);
>      Status = VmgExit (Ghcb, SVM_EXIT_MMIO_WRITE, ExitInfo1, ExitInfo2);
>      if (Status != 0) {
>        return Status;
> @@ -749,6 +750,7 @@ MmioExit (
>      CopyMem (Ghcb->SharedBuffer, InstructionData->Immediate, Bytes);
>  
>      Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
> +    GhcbSetRegValid (Ghcb, GhcbSwScratch);
>      Status = VmgExit (Ghcb, SVM_EXIT_MMIO_WRITE, ExitInfo1, ExitInfo2);
>      if (Status != 0) {
>        return Status;
> @@ -781,6 +783,7 @@ MmioExit (
>      ExitInfo2 = Bytes;
>  
>      Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
> +    GhcbSetRegValid (Ghcb, GhcbSwScratch);
>      Status = VmgExit (Ghcb, SVM_EXIT_MMIO_READ, ExitInfo1, ExitInfo2);
>      if (Status != 0) {
>        return Status;
> @@ -811,6 +814,7 @@ MmioExit (
>      ExitInfo2 = Bytes;
>  
>      Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
> +    GhcbSetRegValid (Ghcb, GhcbSwScratch);
>      Status = VmgExit (Ghcb, SVM_EXIT_MMIO_READ, ExitInfo1, ExitInfo2);
>      if (Status != 0) {
>        return Status;
> @@ -836,6 +840,7 @@ MmioExit (
>      ExitInfo2 = Bytes;
>  
>      Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
> +    GhcbSetRegValid (Ghcb, GhcbSwScratch);
>      Status = VmgExit (Ghcb, SVM_EXIT_MMIO_READ, ExitInfo1, ExitInfo2);
>      if (Status != 0) {
>        return Status;
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Re: [PATCH 2/9] OvmfPkg/VmgExitLib: Set the SW exit fields when performing VMGEXIT

[Laszlo Ersek]

On 10/15/20 10:50, Laszlo Ersek wrote:
> On 10/15/20 10:47, Laszlo Ersek wrote:
>> On 10/10/20 18:07, Tom Lendacky wrote:
>>> From: Tom Lendacky <thomas.lendacky@amd.com>
>>>
>>> All fields that are set in the GHCB should have their associated bit in
>>> the GHCB ValidBitmap field set. Add support to set the bits for the
>>> software exit information fields when performing a VMGEXIT (SwExitCode,
>>> SwExitInfo1, SwExitInfo2).
>>>
>>> Fixes: 61bacc0fa16f ("OvmfPkg/VmgExitLib: Implement library support for VmgExitLib in OVMF")
>>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
>>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>>> Cc: Brijesh Singh <brijesh.singh@amd.com>
>>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>>> ---
>>>  OvmfPkg/Library/VmgExitLib/VmgExitLib.c | 30 ++++++++++++++++++++
>>>  1 file changed, 30 insertions(+)
>>>
>>> diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitLib.c b/OvmfPkg/Library/VmgExitLib/VmgExitLib.c
>>> index 53040cc6f649..6cf649c6101b 100644
>>> --- a/OvmfPkg/Library/VmgExitLib/VmgExitLib.c
>>> +++ b/OvmfPkg/Library/VmgExitLib/VmgExitLib.c
>>> @@ -78,6 +78,32 @@ VmgExitErrorCheck (
>>>    return Status;
>>>  }
>>>  
>>> +/**
>>> +  Marks a field at the specified offset as valid in the GHCB.
>>> +
>>> +  The ValidBitmap area represents the areas of the GHCB that have been marked
>>> +  valid. Set the area of the GHCB at the specified offset as valid.
>>> +
>>> +  @param[in, out] Ghcb    Pointer to the Guest-Hypervisor Communication Block
>>> +  @param[in] Offset       Offset in the GHCB to mark valid
>>> +
>>> +**/
>>> +STATIC
>>> +VOID
>>> +GhcbSetOffsetValid (
>>> +  IN OUT GHCB               *Ghcb,
>>> +  IN     GHCB_QWORD_OFFSET  Offset
>>> +  )
>>> +{
>>> +  UINT32  OffsetIndex;
>>> +  UINT32  OffsetBit;
>>> +
>>> +  OffsetIndex = Offset / 8;
>>> +  OffsetBit   = Offset & 0x07;
>>
>> (1) I suggest improving the consistency of operators -- please either
>> use division and remainder ("Offset / 8" and "Offset % 8"), or bit shift
>> and masking ("Offset >> 3" and "Offset & 0x7")
>>
>> With that:
>>
>> Acked-by: Laszlo Ersek <lersek@redhat.com>
> 
> ... I realize I didn't make the same comment for GhcbIsRegValid(), so
> I'm taking back the above -- consistency with GhcbIsRegValid() is more
> important. No change needed here.
> 
> Acked-by: Laszlo Ersek <lersek@redhat.com>

Wow, I'm needing *a lot* of time for getting back into this. Sorry about
that. :/

So, as I'm slowly grasping the idea behind this series (--> wherever we
make a VmgExit() call, having populated some fields in the GHCB, make
sure the "valid bitmap" is set correctly too), it's becoming clear that
we need a new "VmgExitLib.h" API.

Because, (a) VmgExit() is declared in that lib class header anyway, and
the new helper function effectively helps us set up the (thick)
parameters for that call, (b) at the end of this v1 series, we have the
"valid bitmap" setting logic triplicated (not counting the assembly code
logic in patch #5):

- GhcbSetRegValid() -- existent function in
"OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c"

- GhcbSetOffsetValid() -- function basically identical to
GhcbSetRegValid(), added in this patch to
"OvmfPkg/Library/VmgExitLib/VmgExitLib.c". (This means that the same
library instance will have two copies of the same logic.)

- QemuFlashPtrWrite() -- from patch #6

So I think we should first replace the GhcbSetRegValid() function with a
public (UefiCpuPkg VmgExitLib) API called VmgSetOffsetValid(). This
would likely take two patches -- first patch, introduce the API and add
the VmgExitLibNull implementation under UefiCpuPkg; second patch, add
the real implementation, replacing GhcbSetRegValid(), under OvmfPkg.
Then, in the rest of the series, call VmgSetOffsetValid() wherever
needed (in C code, of course; in assembly, the open-coded stuff is OK I
guess).

Thanks,
Laszlo

Re: [PATCH 6/9] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Set the SwScratch valid bit

[Laszlo Ersek]

On 10/10/20 18:07, Tom Lendacky wrote:
> From: Tom Lendacky <thomas.lendacky@amd.com>
> 
> All fields that are set in the GHCB should have their associated bit in
> the GHCB ValidBitmap field set. Add support to set the bit for the scratch
> area field (SwScratch).
> 
> Fixes: 437eb3f7a8db ("OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Bypass flash detection with SEV-ES")
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
> index 565383ee26d2..5d5a117c48e0 100644
> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
> @@ -52,10 +52,15 @@ QemuFlashPtrWrite (
>    if (MemEncryptSevEsIsEnabled ()) {
>      MSR_SEV_ES_GHCB_REGISTER  Msr;
>      GHCB                      *Ghcb;
> +    UINT32                    ScratchIndex;
> +    UINT32                    ScratchBit;
>  
>      Msr.GhcbPhysicalAddress = AsmReadMsr64 (MSR_SEV_ES_GHCB);
>      Ghcb = Msr.Ghcb;
>  
> +    ScratchIndex = GhcbSwScratch / 8;
> +    ScratchBit   = GhcbSwScratch & 0x07;
> +
>      //
>      // Writing to flash is emulated by the hypervisor through the use of write
>      // protection. This won't work for an SEV-ES guest because the write won't
> @@ -66,6 +71,7 @@ QemuFlashPtrWrite (
>      VmgInit (Ghcb);
>      Ghcb->SharedBuffer[0] = Value;
>      Ghcb->SaveArea.SwScratch = (UINT64) (UINTN) Ghcb->SharedBuffer;
> +    Ghcb->SaveArea.ValidBitmap[ScratchIndex] |= (1 << ScratchBit);
>      VmgExit (Ghcb, SVM_EXIT_MMIO_WRITE, (UINT64) (UINTN) Ptr, 1);
>      VmgDone (Ghcb);
>    } else {
> 

Makes sense, but please reimplement this with the new (proposed)
VmgSetOffsetValid() library function.

Thanks
Laszlo

Re: [PATCH 7/9] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Fix erase blocks for SEV-ES

[Laszlo Ersek]

On 10/10/20 18:07, Tom Lendacky wrote:
> From: Tom Lendacky <thomas.lendacky@amd.com>
> 
> The original SEV-ES support missed updating the QemuFlashEraseBlock()
> function to successfully erase blocks. Update QemuFlashEraseBlock() to
> call the QemuFlashPtrWrite() to be able to successfully perform the
> commands under SEV-ES.
> 
> Fixes: 437eb3f7a8db ("OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Bypass flash detection with SEV-ES")
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c
> index 0d29bf701aca..d19997032ec9 100644
> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c
> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c
> @@ -232,8 +232,8 @@ QemuFlashEraseBlock (
>    }
>  
>    Ptr = QemuFlashPtr (Lba, 0);
> -  *Ptr = BLOCK_ERASE_CMD;
> -  *Ptr = BLOCK_ERASE_CONFIRM_CMD;
> +  QemuFlashPtrWrite (Ptr, BLOCK_ERASE_CMD);
> +  QemuFlashPtrWrite (Ptr, BLOCK_ERASE_CONFIRM_CMD);
>    return EFI_SUCCESS;
>  }
>  
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Sorry for not noticing this during review.

Thanks
Laszlo

Re: [PATCH 8/9] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Disable interrupts when using GHCB

[Laszlo Ersek]

On 10/10/20 18:07, Tom Lendacky wrote:
> From: Tom Lendacky <thomas.lendacky@amd.com>
> 
> The QemuFlashPtrWrite() flash services runtime uses the GHCB and VmgExit()
> directly to perform the flash write when running as an SEV-ES guest. If an
> interrupt arrives

(1) please clarify what kind of interrupt you've seen in practice (my
guess: timer interrupt)

> between VmgInit() and VmgDone(),

(2) VmgDone() is currently an empty function (both library instances) --
did you mean VmgExit()?


> the Dr7 read in the
> interrupt handler

(3) please clarify what interrupt handler you have in mind (function
name and file with full path would be helpful)

> will generate a #VC, which can overwrite information in
> the GHCB that QemuFlashPtrWrite() has set. Prevent this from occurring by
> disabling interrupts around the usage of the GHCB.

(4) I like the last sentence, because it seems to support my suspicion
that the problem is generic. Should we push the interrupt disablement /
re-enablement logic into VmgInit() and VmgDone()?

For that, the pre-VmgInit() interrupt state would have to be saved (for
restoration) somewhere.

(5) I wonder if raising the TPL to TPL_HIGH_LEVEL, rather than messing
with interrupts explicitly, works too. (Search the UEFI spec for
"TPL_HIGH_LEVEL".) Managing the TPL feels cleaner.

... Either way, VmgInit() would have to output either the "old TPL", or
the "old interrupt state", for VmgDone() to restore.

... Hm wait, VmgInit() is called from ApWakeupFunction()
[UefiCpuPkg/Library/MpInitLib/MpLib.c], which is built for PEI too --
there is no "TPL" concept in PEI.

So I guess we should indeed manipulate the interrupts briefly, but I
believe we should still have that logic occur every time we are setting
up a VmgExit(). And so VmgInit() / VmgDone() look like the perfect
bracket for that. What's your take?

Thanks!
Laszlo

> 
> Fixes: 437eb3f7a8db ("OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Bypass flash detection with SEV-ES")
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
> index 5d5a117c48e0..872e58db7cc0 100644
> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
> @@ -9,6 +9,7 @@
>  
>  **/
>  
> +#include <Library/BaseLib.h>
>  #include <Library/UefiRuntimeLib.h>
>  #include <Library/MemEncryptSevLib.h>
>  #include <Library/VmgExitLib.h>
> @@ -54,6 +55,7 @@ QemuFlashPtrWrite (
>      GHCB                      *Ghcb;
>      UINT32                    ScratchIndex;
>      UINT32                    ScratchBit;
> +    BOOLEAN                   InterruptsEnabled;
>  
>      Msr.GhcbPhysicalAddress = AsmReadMsr64 (MSR_SEV_ES_GHCB);
>      Ghcb = Msr.Ghcb;
> @@ -61,6 +63,15 @@ QemuFlashPtrWrite (
>      ScratchIndex = GhcbSwScratch / 8;
>      ScratchBit   = GhcbSwScratch & 0x07;
>  
> +    //
> +    // Be sure that an interrupt can't cause a #VC while the GHCB is
> +    // being used.
> +    //
> +    InterruptsEnabled = GetInterruptState ();
> +    if (InterruptsEnabled) {
> +      DisableInterrupts ();
> +    }
> +
>      //
>      // Writing to flash is emulated by the hypervisor through the use of write
>      // protection. This won't work for an SEV-ES guest because the write won't
> @@ -74,6 +85,10 @@ QemuFlashPtrWrite (
>      Ghcb->SaveArea.ValidBitmap[ScratchIndex] |= (1 << ScratchBit);
>      VmgExit (Ghcb, SVM_EXIT_MMIO_WRITE, (UINT64) (UINTN) Ptr, 1);
>      VmgDone (Ghcb);
> +
> +    if (InterruptsEnabled) {
> +      EnableInterrupts ();
> +    }
>    } else {
>      *Ptr = Value;
>    }
> 

Re: [PATCH 9/9] UefiCpuPkg/MpInitLib: For SEV-ES guest set stack based on processor number

[Laszlo Ersek]

On 10/10/20 18:07, Tom Lendacky wrote:
> From: Tom Lendacky <thomas.lendacky@amd.com>
> 
> Set the SEV-ES reset stack address for an AP based on the processor number
> instead of the APIC ID in case the APIC IDs are not zero-based and densely
> packed/enumerated. This will ensure an AP reset stack address does not get
> set outside of the AP reset stack memory allocation.
> 
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/MpLib.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 07426274f639..71922141b70b 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -680,11 +680,16 @@ MpInitLibSevEsAPReset (
>    IN CPU_MP_DATA                  *CpuMpData
>    )
>  {
> +  EFI_STATUS       Status;
> +  UINTN            ProcessorNumber;
>    UINT16           Code16, Code32;
>    AP_RESET         *APResetFn;
>    UINTN            BufferStart;
>    UINTN            StackStart;
>  
> +  Status = GetProcessorNumber (CpuMpData, &ProcessorNumber);
> +  ASSERT_EFI_ERROR (Status);
> +
>    Code16 = GetProtectedMode16CS ();
>    Code32 = GetProtectedMode32CS ();
>  
> @@ -696,7 +701,7 @@ MpInitLibSevEsAPReset (
>  
>    BufferStart = CpuMpData->MpCpuExchangeInfo->BufferStart;
>    StackStart = CpuMpData->SevEsAPResetStackStart -
> -                 (AP_RESET_STACK_SIZE * GetApicId ());
> +                 (AP_RESET_STACK_SIZE * ProcessorNumber);
>  
>    //
>    // This call never returns.
> 

trivial request: please insert a comma in the subject line, after "For
SEV-ES guest". (The new subject length should still be accepted by
"PatchCheck.py".)

Thanks
Laszlo

Re: [PATCH 0/9] SEV-ES guest support fixes and cleanup

[Lendacky, Thomas]

On 10/15/20 2:43 AM, Laszlo Ersek wrote:
> Hi Tom,
> 
> On 10/10/20 18:06, Tom Lendacky wrote:
>> From: Tom Lendacky <thomas.lendacky@amd.com>
>>
>> This patch series provides some fixes, updates and cleanup to the SEV-ES
>> guest support:
>>
>> The first patch updates the calculation of the qword offset of fields
>> within the GHCB. Specifically, it removes the hardcoding of the offsets
>> and uses the OFFSET_OF () and sizeof () functions to calculate the
>> values, removes unused values and add values that will be used in later
>> patches.
>>
>> The next five patches set the SwExitCode/SwExitInfo1/SwExitInfo2/SwScratch
>> valid bits in the GHCB ValidBitmap area when these fields are set at
>> VMGEXIT.
>>
>> The next two patches update the Qemu flash drive services support to
>> add SEV-ES support to erasing blocks and to disable interrupts when using
>> the GHCB.
>>
>> Finally, the last patch uses the processor number for setting the AP stack
>> pointer instead of the APIC ID (using GetProcessorNumber()).
> 
> please file a TianoCore BZ for this series, assign it to yourself, link
> the v1 posting in a comment on the BZ, and update the commit messages to
> reference that BZ.
> 
> I find this relevant because edk2-stable202008 resolved TianoCore#2198.
> If (in your opinion) downstreams that aim at supporting SEV-ES should
> also have these patches (for example, if they should backport them on
> top of edk2-stable202008), then having a TianoCore Bugzilla would be
> quite helpful to them, for tracking purposes.

Ok, done (https://bugzilla.tianocore.org/show_bug.cgi?id=3008).

One thing I noticed in the bugzilla is that there is a way to specify the 
releases the issue was observed in and must be fixed in, but the 
edk2-stable202008 release isn't listed in it (yet).

Thanks,
Tom

> 
> Thanks,
> Laszlo
> 
>>
>> ---
>>
>> These patches are based on commit:
>> ae511331e0fb ("BaseTools Build_Rule: Add the missing ASM16_FLAGS for ASM16 source file")
>>
>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Liming Gao <gaoliming@byosoft.com.cn>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>> Cc: Rahul Kumar <rahul1.kumar@intel.com>
>> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
>> Cc: Ray Ni <ray.ni@intel.com>
>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>> Cc: Brijesh Singh <brijesh.singh@amd.com>
>>
>> Tom Lendacky (9):
>>    OvmfPkg/VmgExitLib: Update ValidBitmap settings
>>    OvmfPkg/VmgExitLib: Set the SW exit fields when performing VMGEXIT
>>    OvmfPkg/VmgExitLib: Set the SwScratch valid bit for IOIO events
>>    OvmfPkg/VmgExitLib: Set the SwScratch valid bit for MMIO events
>>    UefiCpuPkg/MpInitLib: Set the SW exit fields when performing VMGEXIT
>>    OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Set the SwScratch valid bit
>>    OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Fix erase blocks for SEV-ES
>>    OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Disable interrupts when using
>>      GHCB
>>    UefiCpuPkg/MpInitLib: For SEV-ES guest set stack based on processor
>>      number
>>
>>   MdePkg/Include/Register/Amd/Ghcb.h                    | 48 ++++++++------------
>>   OvmfPkg/Library/VmgExitLib/VmgExitLib.c               | 30 ++++++++++++
>>   OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c         | 10 +++-
>>   OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c    |  4 +-
>>   OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c | 21 +++++++++
>>   UefiCpuPkg/Library/MpInitLib/MpLib.c                  |  7 ++-
>>   UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm         |  6 +++
>>   7 files changed, 91 insertions(+), 35 deletions(-)
>>
> 

Re: [PATCH 1/9] OvmfPkg/VmgExitLib: Update ValidBitmap settings

[Lendacky, Thomas]

On 10/15/20 3:40 AM, Laszlo Ersek wrote:
> Hi Tom,

Hi Laszlo,

> 
> On 10/10/20 18:06, Tom Lendacky wrote:
>> From: Tom Lendacky <thomas.lendacky@amd.com>
>>
>> Use OFFSET_OF () and sizeof () to calculate the GHCB register field
>> offsets instead of hardcoding the values in the GHCB_REGISTER enum.
>> Rename GHCB_REGISTER to GHCB_QWORD_OFFSET to more appropriately describe
>> the enum. While redefing the values, only include (and add) fields that
>> are used per the GHCB specification.
>>
>> Also, remove the DR7 field from the GHCB_SAVE_AREA structure since it is
>> not used/defined in the GHCB specification and then rename the reserved
>> fields as appropriate.
>>
>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>> Cc: Liming Gao <gaoliming@byosoft.com.cn>
>> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>> Cc: Brijesh Singh <brijesh.singh@amd.com>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>> ---
>>   MdePkg/Include/Register/Amd/Ghcb.h            | 48 ++++++++------------
>>   OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c |  4 +-
>>   2 files changed, 20 insertions(+), 32 deletions(-)
> 
> This patch modifies both MdePkg and OvmfPkg. I agree that, as an
> exception, this is the right thing to do.
> 
> That's because we are not changing the identifiers of the enumeration
> constants (such as GhcbCpl). Because those identifiers are declared at
> file scope, having both GHCB_REGISTER and GHCB_QWORD_OFFSET declare
> (e.g.) GhcbCpl would cause a compilation failure. Therefore we can't
> implement this in multiple stages (first introduce GHCB_QWORD_OFFSET,
> then remove GHCB_REGISTER separately).
> 
> (1) However, the subject line doesn't look correct. It should mention
> both MdePkg and OvmfPkg. Also, we're not updating ValidBitmap settings
> just yet.
> 
> I suggest:
> 
>    MdePkg, OvmfPkg: clean up GHCB field offsets and save area

Yup, I'll fix this up.

> 
>>
>> diff --git a/MdePkg/Include/Register/Amd/Ghcb.h b/MdePkg/Include/Register/Amd/Ghcb.h
>> index 54a80da0f6d7..33c7e8939a28 100644
>> --- a/MdePkg/Include/Register/Amd/Ghcb.h
>> +++ b/MdePkg/Include/Register/Amd/Ghcb.h
>> @@ -82,50 +82,25 @@
>>   #define IOIO_SEG_DS         (BIT11 | BIT10)
>>   
>>   
>> -typedef enum {
>> -  GhcbCpl              = 25,
>> -  GhcbRflags           = 46,
>> -  GhcbRip,
>> -  GhcbRsp              = 59,
>> -  GhcbRax              = 63,
>> -  GhcbRcx              = 97,
>> -  GhcbRdx,
>> -  GhcbRbx,
>> -  GhcbRbp              = 101,
>> -  GhcbRsi,
>> -  GhcbRdi,
>> -  GhcbR8,
>> -  GhcbR9,
>> -  GhcbR10,
>> -  GhcbR11,
>> -  GhcbR12,
>> -  GhcbR13,
>> -  GhcbR14,
>> -  GhcbR15,
>> -  GhcbXCr0             = 125,
>> -} GHCB_REGISTER;
>> -
>>   typedef PACKED struct {
>>     UINT8                  Reserved1[203];
>>     UINT8                  Cpl;
>> -  UINT8                  Reserved2[148];
>> -  UINT64                 Dr7;
>> -  UINT8                  Reserved3[144];
>> +  UINT8                  Reserved2[300];
>>     UINT64                 Rax;
>> -  UINT8                  Reserved4[264];
>> +  UINT8                  Reserved3[264];
>>     UINT64                 Rcx;
>>     UINT64                 Rdx;
>>     UINT64                 Rbx;
>> -  UINT8                  Reserved5[112];
>> +  UINT8                  Reserved4[112];
>>     UINT64                 SwExitCode;
>>     UINT64                 SwExitInfo1;
>>     UINT64                 SwExitInfo2;
>>     UINT64                 SwScratch;
>> -  UINT8                  Reserved6[56];
>> +  UINT8                  Reserved5[56];
>>     UINT64                 XCr0;
>>     UINT8                  ValidBitmap[16];
>>     UINT64                 X87StateGpa;
>> -  UINT8                  Reserved7[1016];
>> +  UINT8                  Reserved6[1016];
>>   } GHCB_SAVE_AREA;
> 
> (2) The meaning of a field called "ReservedN" must never change (it must
> describe the same offset range in the structure, after introduction). If
> we need to change the structure incompatibly, then please remove the old
> ReservedN field name altogether. If a new reserved field has to be
> introduced, in addition, then please find the largest N used with
> Reserved fields in the structure currently, and use (N+1) in the name of
> the new field.
> 
> This practice makes sure that any (out of tree) code that refers to a
> ReservedN field in MdePkg will cleanly break during compilation after
> this change. Changing the meaning of a ReservedN field could otherwise
> cause misbehavior that could be harder to track down.
> 
> I realize this is somewhat unusual -- after all, if someone deliberately
> accessed a field called "Reserved", any subsequent breakage is *their*
> fault. However, the reason for this practice is that sometimes MdePkg
> headers lag behind industry specs (meaning, non-UEFI specs), but drivers
> or libs (outside of MdePkg or even edk2) already need to refer to
> newly-specified fields. Even edk2 used to have code similar to the example
> 
>    Reserved3[12] = 1;
> 
> Normally I don't advocate accommodating out-of-tree code, but following
> this ReservedN scheme isn't hard.

Ok, will do. I'll change the old Reserved2/Dr7/Reserved3 fields to a new 
combined Reserved8 field.

Thanks,
Tom

> 
> 
>>   
>>   typedef PACKED struct {
>> @@ -136,6 +111,19 @@ typedef PACKED struct {
>>     UINT32                 GhcbUsage;
>>   } GHCB;
>>   
>> +typedef enum {
>> +  GhcbCpl          = OFFSET_OF (GHCB, SaveArea.Cpl) / sizeof (UINT64),
>> +  GhcbRax          = OFFSET_OF (GHCB, SaveArea.Rax) / sizeof (UINT64),
>> +  GhcbRbx          = OFFSET_OF (GHCB, SaveArea.Rbx) / sizeof (UINT64),
>> +  GhcbRcx          = OFFSET_OF (GHCB, SaveArea.Rcx) / sizeof (UINT64),
>> +  GhcbRdx          = OFFSET_OF (GHCB, SaveArea.Rdx) / sizeof (UINT64),
>> +  GhcbXCr0         = OFFSET_OF (GHCB, SaveArea.XCr0) / sizeof (UINT64),
>> +  GhcbSwExitCode   = OFFSET_OF (GHCB, SaveArea.SwExitCode) / sizeof (UINT64),
>> +  GhcbSwExitInfo1  = OFFSET_OF (GHCB, SaveArea.SwExitInfo1) / sizeof (UINT64),
>> +  GhcbSwExitInfo2  = OFFSET_OF (GHCB, SaveArea.SwExitInfo2) / sizeof (UINT64),
>> +  GhcbSwScratch    = OFFSET_OF (GHCB, SaveArea.SwScratch) / sizeof (UINT64),
>> +} GHCB_QWORD_OFFSET;
>> +
>>   typedef union {
>>     struct {
>>       UINT32  Lower32Bits;
>> diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
>> index 8e42b305e83c..c5484a3f478c 100644
>> --- a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
>> +++ b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
>> @@ -153,7 +153,7 @@ STATIC
>>   BOOLEAN
>>   GhcbIsRegValid (
>>     IN GHCB                *Ghcb,
>> -  IN GHCB_REGISTER       Reg
>> +  IN GHCB_QWORD_OFFSET   Reg
>>     )
>>   {
>>     UINT32  RegIndex;
>> @@ -179,7 +179,7 @@ STATIC
>>   VOID
>>   GhcbSetRegValid (
>>     IN OUT GHCB                *Ghcb,
>> -  IN     GHCB_REGISTER       Reg
>> +  IN     GHCB_QWORD_OFFSET   Reg
>>     )
>>   {
>>     UINT32  RegIndex;
>>
> 
> Thanks,
> Laszlo
> 

Re: [PATCH 2/9] OvmfPkg/VmgExitLib: Set the SW exit fields when performing VMGEXIT

[Lendacky, Thomas]

On 10/15/20 4:19 AM, Laszlo Ersek wrote:
> On 10/15/20 10:50, Laszlo Ersek wrote:
>> On 10/15/20 10:47, Laszlo Ersek wrote:
>>> On 10/10/20 18:07, Tom Lendacky wrote:
>>>> From: Tom Lendacky <thomas.lendacky@amd.com>
>>>>
>>>> All fields that are set in the GHCB should have their associated bit in
>>>> the GHCB ValidBitmap field set. Add support to set the bits for the
>>>> software exit information fields when performing a VMGEXIT (SwExitCode,
>>>> SwExitInfo1, SwExitInfo2).
>>>>
>>>> Fixes: 61bacc0fa16f ("OvmfPkg/VmgExitLib: Implement library support for VmgExitLib in OVMF")
>>>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
>>>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>>>> Cc: Brijesh Singh <brijesh.singh@amd.com>
>>>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>>>> ---
>>>>   OvmfPkg/Library/VmgExitLib/VmgExitLib.c | 30 ++++++++++++++++++++
>>>>   1 file changed, 30 insertions(+)
>>>>
>>>> diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitLib.c b/OvmfPkg/Library/VmgExitLib/VmgExitLib.c
>>>> index 53040cc6f649..6cf649c6101b 100644
>>>> --- a/OvmfPkg/Library/VmgExitLib/VmgExitLib.c
>>>> +++ b/OvmfPkg/Library/VmgExitLib/VmgExitLib.c
>>>> @@ -78,6 +78,32 @@ VmgExitErrorCheck (
>>>>     return Status;
>>>>   }
>>>>   
>>>> +/**
>>>> +  Marks a field at the specified offset as valid in the GHCB.
>>>> +
>>>> +  The ValidBitmap area represents the areas of the GHCB that have been marked
>>>> +  valid. Set the area of the GHCB at the specified offset as valid.
>>>> +
>>>> +  @param[in, out] Ghcb    Pointer to the Guest-Hypervisor Communication Block
>>>> +  @param[in] Offset       Offset in the GHCB to mark valid
>>>> +
>>>> +**/
>>>> +STATIC
>>>> +VOID
>>>> +GhcbSetOffsetValid (
>>>> +  IN OUT GHCB               *Ghcb,
>>>> +  IN     GHCB_QWORD_OFFSET  Offset
>>>> +  )
>>>> +{
>>>> +  UINT32  OffsetIndex;
>>>> +  UINT32  OffsetBit;
>>>> +
>>>> +  OffsetIndex = Offset / 8;
>>>> +  OffsetBit   = Offset & 0x07;
>>>
>>> (1) I suggest improving the consistency of operators -- please either
>>> use division and remainder ("Offset / 8" and "Offset % 8"), or bit shift
>>> and masking ("Offset >> 3" and "Offset & 0x7")
>>>
>>> With that:
>>>
>>> Acked-by: Laszlo Ersek <lersek@redhat.com>
>>
>> ... I realize I didn't make the same comment for GhcbIsRegValid(), so
>> I'm taking back the above -- consistency with GhcbIsRegValid() is more
>> important. No change needed here.
>>
>> Acked-by: Laszlo Ersek <lersek@redhat.com>
> 
> Wow, I'm needing *a lot* of time for getting back into this. Sorry about
> that. :/
> 
> So, as I'm slowly grasping the idea behind this series (--> wherever we
> make a VmgExit() call, having populated some fields in the GHCB, make
> sure the "valid bitmap" is set correctly too), it's becoming clear that
> we need a new "VmgExitLib.h" API.
> 
> Because, (a) VmgExit() is declared in that lib class header anyway, and
> the new helper function effectively helps us set up the (thick)
> parameters for that call, (b) at the end of this v1 series, we have the
> "valid bitmap" setting logic triplicated (not counting the assembly code
> logic in patch #5):
> 
> - GhcbSetRegValid() -- existent function in
> "OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c"
> 
> - GhcbSetOffsetValid() -- function basically identical to
> GhcbSetRegValid(), added in this patch to
> "OvmfPkg/Library/VmgExitLib/VmgExitLib.c". (This means that the same
> library instance will have two copies of the same logic.)
> 
> - QemuFlashPtrWrite() -- from patch #6
> 
> So I think we should first replace the GhcbSetRegValid() function with a
> public (UefiCpuPkg VmgExitLib) API called VmgSetOffsetValid(). This
> would likely take two patches -- first patch, introduce the API and add
> the VmgExitLibNull implementation under UefiCpuPkg; second patch, add
> the real implementation, replacing GhcbSetRegValid(), under OvmfPkg.
> Then, in the rest of the series, call VmgSetOffsetValid() wherever
> needed (in C code, of course; in assembly, the open-coded stuff is OK I
> guess).

Yup, I was toying with the idea of adding a new function to the library, 
too. I'll do that, using the approach you outlined.

Thanks,
Tom

> 
> Thanks,
> Laszlo
> 

Re: [PATCH 2/9] OvmfPkg/VmgExitLib: Set the SW exit fields when performing VMGEXIT

[Lendacky, Thomas]

On 10/15/20 9:10 AM, Tom Lendacky wrote:
> On 10/15/20 4:19 AM, Laszlo Ersek wrote:
>> On 10/15/20 10:50, Laszlo Ersek wrote:
>>> On 10/15/20 10:47, Laszlo Ersek wrote:
>>>> On 10/10/20 18:07, Tom Lendacky wrote:
>>>>> From: Tom Lendacky <thomas.lendacky@amd.com>
>>>>>
>>>>> All fields that are set in the GHCB should have their associated bit in
>>>>> the GHCB ValidBitmap field set. Add support to set the bits for the
>>>>> software exit information fields when performing a VMGEXIT (SwExitCode,
>>>>> SwExitInfo1, SwExitInfo2).
>>>>>
>>>>> Fixes: 61bacc0fa16f ("OvmfPkg/VmgExitLib: Implement library support 
>>>>> for VmgExitLib in OVMF")
>>>>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>>>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
>>>>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>>>>> Cc: Brijesh Singh <brijesh.singh@amd.com>
>>>>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>>>>> ---
>>>>>   OvmfPkg/Library/VmgExitLib/VmgExitLib.c | 30 ++++++++++++++++++++
>>>>>   1 file changed, 30 insertions(+)
>>>>>
>>>>> diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitLib.c 
>>>>> b/OvmfPkg/Library/VmgExitLib/VmgExitLib.c
>>>>> index 53040cc6f649..6cf649c6101b 100644
>>>>> --- a/OvmfPkg/Library/VmgExitLib/VmgExitLib.c
>>>>> +++ b/OvmfPkg/Library/VmgExitLib/VmgExitLib.c
>>>>> @@ -78,6 +78,32 @@ VmgExitErrorCheck (
>>>>>     return Status;
>>>>>   }
>>>>> +/**
>>>>> +  Marks a field at the specified offset as valid in the GHCB.
>>>>> +
>>>>> +  The ValidBitmap area represents the areas of the GHCB that have 
>>>>> been marked
>>>>> +  valid. Set the area of the GHCB at the specified offset as valid.
>>>>> +
>>>>> +  @param[in, out] Ghcb    Pointer to the Guest-Hypervisor 
>>>>> Communication Block
>>>>> +  @param[in] Offset       Offset in the GHCB to mark valid
>>>>> +
>>>>> +**/
>>>>> +STATIC
>>>>> +VOID
>>>>> +GhcbSetOffsetValid (
>>>>> +  IN OUT GHCB               *Ghcb,
>>>>> +  IN     GHCB_QWORD_OFFSET  Offset
>>>>> +  )
>>>>> +{
>>>>> +  UINT32  OffsetIndex;
>>>>> +  UINT32  OffsetBit;
>>>>> +
>>>>> +  OffsetIndex = Offset / 8;
>>>>> +  OffsetBit   = Offset & 0x07;
>>>>
>>>> (1) I suggest improving the consistency of operators -- please either
>>>> use division and remainder ("Offset / 8" and "Offset % 8"), or bit shift
>>>> and masking ("Offset >> 3" and "Offset & 0x7")
>>>>
>>>> With that:
>>>>
>>>> Acked-by: Laszlo Ersek <lersek@redhat.com>
>>>
>>> ... I realize I didn't make the same comment for GhcbIsRegValid(), so
>>> I'm taking back the above -- consistency with GhcbIsRegValid() is more
>>> important. No change needed here.
>>>
>>> Acked-by: Laszlo Ersek <lersek@redhat.com>
>>
>> Wow, I'm needing *a lot* of time for getting back into this. Sorry about
>> that. :/
>>
>> So, as I'm slowly grasping the idea behind this series (--> wherever we
>> make a VmgExit() call, having populated some fields in the GHCB, make
>> sure the "valid bitmap" is set correctly too), it's becoming clear that
>> we need a new "VmgExitLib.h" API.
>>
>> Because, (a) VmgExit() is declared in that lib class header anyway, and
>> the new helper function effectively helps us set up the (thick)
>> parameters for that call, (b) at the end of this v1 series, we have the
>> "valid bitmap" setting logic triplicated (not counting the assembly code
>> logic in patch #5):
>>
>> - GhcbSetRegValid() -- existent function in
>> "OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c"
>>
>> - GhcbSetOffsetValid() -- function basically identical to
>> GhcbSetRegValid(), added in this patch to
>> "OvmfPkg/Library/VmgExitLib/VmgExitLib.c". (This means that the same
>> library instance will have two copies of the same logic.)
>>
>> - QemuFlashPtrWrite() -- from patch #6
>>
>> So I think we should first replace the GhcbSetRegValid() function with a
>> public (UefiCpuPkg VmgExitLib) API called VmgSetOffsetValid(). This
>> would likely take two patches -- first patch, introduce the API and add
>> the VmgExitLibNull implementation under UefiCpuPkg; second patch, add
>> the real implementation, replacing GhcbSetRegValid(), under OvmfPkg.
>> Then, in the rest of the series, call VmgSetOffsetValid() wherever
>> needed (in C code, of course; in assembly, the open-coded stuff is OK I
>> guess).
> 
> Yup, I was toying with the idea of adding a new function to the library, 
> too. I'll do that, using the approach you outlined.

Also, I'll add an interface, VmgIsOffsetValid(), so that all the 
ValidBitmap manipulation and examination is contained in the library. I'll 
replace the GhcbIsRegValid() implementation with calls to this interface.

Thanks,
Tom

> 
> Thanks,
> Tom
> 
>>
>> Thanks,
>> Laszlo
>>

Re: [PATCH 2/9] OvmfPkg/VmgExitLib: Set the SW exit fields when performing VMGEXIT

[Lendacky, Thomas]

On 10/15/20 3:47 AM, Laszlo Ersek wrote:
> On 10/10/20 18:07, Tom Lendacky wrote:
>> From: Tom Lendacky <thomas.lendacky@amd.com>
>>
>> All fields that are set in the GHCB should have their associated bit in
>> the GHCB ValidBitmap field set. Add support to set the bits for the
>> software exit information fields when performing a VMGEXIT (SwExitCode,
>> SwExitInfo1, SwExitInfo2).
>>
>> Fixes: 61bacc0fa16f ("OvmfPkg/VmgExitLib: Implement library support for VmgExitLib in OVMF")

Btw, what is the preferred method for a Fixes: tag in edk2? I've seen it
three different ways - the above way and also:

  Fixes: 61bacc0fa16fd6f595a2c4222425cb6286e19977

and

  Fixes: 61bacc0fa16f ("OvmfPkg/VmgExitLib: Implement library support ...")

Thanks,
Tom

>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>> Cc: Brijesh Singh <brijesh.singh@amd.com>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>> ---
>>   OvmfPkg/Library/VmgExitLib/VmgExitLib.c | 30 ++++++++++++++++++++
>>   1 file changed, 30 insertions(+)
>>
>> diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitLib.c b/OvmfPkg/Library/VmgExitLib/VmgExitLib.c
>> index 53040cc6f649..6cf649c6101b 100644
>> --- a/OvmfPkg/Library/VmgExitLib/VmgExitLib.c
>> +++ b/OvmfPkg/Library/VmgExitLib/VmgExitLib.c
>> @@ -78,6 +78,32 @@ VmgExitErrorCheck (
>>     return Status;
>>   }
>>   
>> +/**
>> +  Marks a field at the specified offset as valid in the GHCB.
>> +
>> +  The ValidBitmap area represents the areas of the GHCB that have been marked
>> +  valid. Set the area of the GHCB at the specified offset as valid.
>> +
>> +  @param[in, out] Ghcb    Pointer to the Guest-Hypervisor Communication Block
>> +  @param[in] Offset       Offset in the GHCB to mark valid
>> +
>> +**/
>> +STATIC
>> +VOID
>> +GhcbSetOffsetValid (
>> +  IN OUT GHCB               *Ghcb,
>> +  IN     GHCB_QWORD_OFFSET  Offset
>> +  )
>> +{
>> +  UINT32  OffsetIndex;
>> +  UINT32  OffsetBit;
>> +
>> +  OffsetIndex = Offset / 8;
>> +  OffsetBit   = Offset & 0x07;
> 
> (1) I suggest improving the consistency of operators -- please either
> use division and remainder ("Offset / 8" and "Offset % 8"), or bit shift
> and masking ("Offset >> 3" and "Offset & 0x7")
> 
> With that:
> 
> Acked-by: Laszlo Ersek <lersek@redhat.com>
> 
> Thanks
> Laszlo
> 
>> +
>> +  Ghcb->SaveArea.ValidBitmap[OffsetIndex] |= (1 << OffsetBit);
>> +}
>> +
>>   /**
>>     Perform VMGEXIT.
>>   
>> @@ -110,6 +136,10 @@ VmgExit (
>>     Ghcb->SaveArea.SwExitInfo1 = ExitInfo1;
>>     Ghcb->SaveArea.SwExitInfo2 = ExitInfo2;
>>   
>> +  GhcbSetOffsetValid (Ghcb, GhcbSwExitCode);
>> +  GhcbSetOffsetValid (Ghcb, GhcbSwExitInfo1);
>> +  GhcbSetOffsetValid (Ghcb, GhcbSwExitInfo2);
>> +
>>     //
>>     // Guest memory is used for the guest-hypervisor communication, so fence
>>     // the invocation of the VMGEXIT instruction to ensure GHCB accesses are
>>
> 

Re: [PATCH 0/9] SEV-ES guest support fixes and cleanup

[Laszlo Ersek]

Mike,

(top posting on purpose)

can you please update the bugzilla installation so that it show the
edk2-stable202008 tag in the "list of affected releases" and "list of
releases needing fixes" widgets?

Thanks!
Laszlo

On 10/15/20 15:26, Tom Lendacky wrote:
> On 10/15/20 2:43 AM, Laszlo Ersek wrote:
>> Hi Tom,
>>
>> On 10/10/20 18:06, Tom Lendacky wrote:
>>> From: Tom Lendacky <thomas.lendacky@amd.com>
>>>
>>> This patch series provides some fixes, updates and cleanup to the SEV-ES
>>> guest support:
>>>
>>> The first patch updates the calculation of the qword offset of fields
>>> within the GHCB. Specifically, it removes the hardcoding of the offsets
>>> and uses the OFFSET_OF () and sizeof () functions to calculate the
>>> values, removes unused values and add values that will be used in later
>>> patches.
>>>
>>> The next five patches set the
>>> SwExitCode/SwExitInfo1/SwExitInfo2/SwScratch
>>> valid bits in the GHCB ValidBitmap area when these fields are set at
>>> VMGEXIT.
>>>
>>> The next two patches update the Qemu flash drive services support to
>>> add SEV-ES support to erasing blocks and to disable interrupts when
>>> using
>>> the GHCB.
>>>
>>> Finally, the last patch uses the processor number for setting the AP
>>> stack
>>> pointer instead of the APIC ID (using GetProcessorNumber()).
>>
>> please file a TianoCore BZ for this series, assign it to yourself, link
>> the v1 posting in a comment on the BZ, and update the commit messages to
>> reference that BZ.
>>
>> I find this relevant because edk2-stable202008 resolved TianoCore#2198.
>> If (in your opinion) downstreams that aim at supporting SEV-ES should
>> also have these patches (for example, if they should backport them on
>> top of edk2-stable202008), then having a TianoCore Bugzilla would be
>> quite helpful to them, for tracking purposes.
> 
> Ok, done (https://bugzilla.tianocore.org/show_bug.cgi?id=3008).
> 
> One thing I noticed in the bugzilla is that there is a way to specify
> the releases the issue was observed in and must be fixed in, but the
> edk2-stable202008 release isn't listed in it (yet).
> 
> Thanks,
> Tom
> 
>>
>> Thanks,
>> Laszlo
>>
>>>
>>> ---
>>>
>>> These patches are based on commit:
>>> ae511331e0fb ("BaseTools Build_Rule: Add the missing ASM16_FLAGS for
>>> ASM16 source file")
>>>
>>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
>>> Cc: Eric Dong <eric.dong@intel.com>
>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>> Cc: Liming Gao <gaoliming@byosoft.com.cn>
>>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>>> Cc: Rahul Kumar <rahul1.kumar@intel.com>
>>> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
>>> Cc: Ray Ni <ray.ni@intel.com>
>>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>>> Cc: Brijesh Singh <brijesh.singh@amd.com>
>>>
>>> Tom Lendacky (9):
>>>    OvmfPkg/VmgExitLib: Update ValidBitmap settings
>>>    OvmfPkg/VmgExitLib: Set the SW exit fields when performing VMGEXIT
>>>    OvmfPkg/VmgExitLib: Set the SwScratch valid bit for IOIO events
>>>    OvmfPkg/VmgExitLib: Set the SwScratch valid bit for MMIO events
>>>    UefiCpuPkg/MpInitLib: Set the SW exit fields when performing VMGEXIT
>>>    OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Set the SwScratch valid bit
>>>    OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Fix erase blocks for SEV-ES
>>>    OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Disable interrupts when using
>>>      GHCB
>>>    UefiCpuPkg/MpInitLib: For SEV-ES guest set stack based on processor
>>>      number
>>>
>>>   MdePkg/Include/Register/Amd/Ghcb.h                    | 48
>>> ++++++++------------
>>>   OvmfPkg/Library/VmgExitLib/VmgExitLib.c               | 30
>>> ++++++++++++
>>>   OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c         | 10 +++-
>>>   OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c    |  4 +-
>>>   OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c | 21 +++++++++
>>>   UefiCpuPkg/Library/MpInitLib/MpLib.c                  |  7 ++-
>>>   UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm         |  6 +++
>>>   7 files changed, 91 insertions(+), 35 deletions(-)
>>>
>>
> 

Re: [PATCH 2/9] OvmfPkg/VmgExitLib: Set the SW exit fields when performing VMGEXIT

[Laszlo Ersek]

On 10/15/20 16:33, Tom Lendacky wrote:
> On 10/15/20 9:10 AM, Tom Lendacky wrote:
>> On 10/15/20 4:19 AM, Laszlo Ersek wrote:
>>> On 10/15/20 10:50, Laszlo Ersek wrote:
>>>> On 10/15/20 10:47, Laszlo Ersek wrote:
>>>>> On 10/10/20 18:07, Tom Lendacky wrote:
>>>>>> From: Tom Lendacky <thomas.lendacky@amd.com>
>>>>>>
>>>>>> All fields that are set in the GHCB should have their associated
>>>>>> bit in
>>>>>> the GHCB ValidBitmap field set. Add support to set the bits for the
>>>>>> software exit information fields when performing a VMGEXIT
>>>>>> (SwExitCode,
>>>>>> SwExitInfo1, SwExitInfo2).
>>>>>>
>>>>>> Fixes: 61bacc0fa16f ("OvmfPkg/VmgExitLib: Implement library
>>>>>> support for VmgExitLib in OVMF")
>>>>>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>>>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>>>>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
>>>>>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>>>>>> Cc: Brijesh Singh <brijesh.singh@amd.com>
>>>>>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>>>>>> ---
>>>>>>   OvmfPkg/Library/VmgExitLib/VmgExitLib.c | 30 ++++++++++++++++++++
>>>>>>   1 file changed, 30 insertions(+)
>>>>>>
>>>>>> diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitLib.c
>>>>>> b/OvmfPkg/Library/VmgExitLib/VmgExitLib.c
>>>>>> index 53040cc6f649..6cf649c6101b 100644
>>>>>> --- a/OvmfPkg/Library/VmgExitLib/VmgExitLib.c
>>>>>> +++ b/OvmfPkg/Library/VmgExitLib/VmgExitLib.c
>>>>>> @@ -78,6 +78,32 @@ VmgExitErrorCheck (
>>>>>>     return Status;
>>>>>>   }
>>>>>> +/**
>>>>>> +  Marks a field at the specified offset as valid in the GHCB.
>>>>>> +
>>>>>> +  The ValidBitmap area represents the areas of the GHCB that have
>>>>>> been marked
>>>>>> +  valid. Set the area of the GHCB at the specified offset as valid.
>>>>>> +
>>>>>> +  @param[in, out] Ghcb    Pointer to the Guest-Hypervisor
>>>>>> Communication Block
>>>>>> +  @param[in] Offset       Offset in the GHCB to mark valid
>>>>>> +
>>>>>> +**/
>>>>>> +STATIC
>>>>>> +VOID
>>>>>> +GhcbSetOffsetValid (
>>>>>> +  IN OUT GHCB               *Ghcb,
>>>>>> +  IN     GHCB_QWORD_OFFSET  Offset
>>>>>> +  )
>>>>>> +{
>>>>>> +  UINT32  OffsetIndex;
>>>>>> +  UINT32  OffsetBit;
>>>>>> +
>>>>>> +  OffsetIndex = Offset / 8;
>>>>>> +  OffsetBit   = Offset & 0x07;
>>>>>
>>>>> (1) I suggest improving the consistency of operators -- please either
>>>>> use division and remainder ("Offset / 8" and "Offset % 8"), or bit
>>>>> shift
>>>>> and masking ("Offset >> 3" and "Offset & 0x7")
>>>>>
>>>>> With that:
>>>>>
>>>>> Acked-by: Laszlo Ersek <lersek@redhat.com>
>>>>
>>>> ... I realize I didn't make the same comment for GhcbIsRegValid(), so
>>>> I'm taking back the above -- consistency with GhcbIsRegValid() is more
>>>> important. No change needed here.
>>>>
>>>> Acked-by: Laszlo Ersek <lersek@redhat.com>
>>>
>>> Wow, I'm needing *a lot* of time for getting back into this. Sorry about
>>> that. :/
>>>
>>> So, as I'm slowly grasping the idea behind this series (--> wherever we
>>> make a VmgExit() call, having populated some fields in the GHCB, make
>>> sure the "valid bitmap" is set correctly too), it's becoming clear that
>>> we need a new "VmgExitLib.h" API.
>>>
>>> Because, (a) VmgExit() is declared in that lib class header anyway, and
>>> the new helper function effectively helps us set up the (thick)
>>> parameters for that call, (b) at the end of this v1 series, we have the
>>> "valid bitmap" setting logic triplicated (not counting the assembly code
>>> logic in patch #5):
>>>
>>> - GhcbSetRegValid() -- existent function in
>>> "OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c"
>>>
>>> - GhcbSetOffsetValid() -- function basically identical to
>>> GhcbSetRegValid(), added in this patch to
>>> "OvmfPkg/Library/VmgExitLib/VmgExitLib.c". (This means that the same
>>> library instance will have two copies of the same logic.)
>>>
>>> - QemuFlashPtrWrite() -- from patch #6
>>>
>>> So I think we should first replace the GhcbSetRegValid() function with a
>>> public (UefiCpuPkg VmgExitLib) API called VmgSetOffsetValid(). This
>>> would likely take two patches -- first patch, introduce the API and add
>>> the VmgExitLibNull implementation under UefiCpuPkg; second patch, add
>>> the real implementation, replacing GhcbSetRegValid(), under OvmfPkg.
>>> Then, in the rest of the series, call VmgSetOffsetValid() wherever
>>> needed (in C code, of course; in assembly, the open-coded stuff is OK I
>>> guess).
>>
>> Yup, I was toying with the idea of adding a new function to the
>> library, too. I'll do that, using the approach you outlined.
> 
> Also, I'll add an interface, VmgIsOffsetValid(), so that all the
> ValidBitmap manipulation and examination is contained in the library.
> I'll replace the GhcbIsRegValid() implementation with calls to this
> interface.

Sounds good!

Thanks
Laszlo

Re: [PATCH 2/9] OvmfPkg/VmgExitLib: Set the SW exit fields when performing VMGEXIT

[Laszlo Ersek]

On 10/15/20 17:27, Tom Lendacky wrote:
> On 10/15/20 3:47 AM, Laszlo Ersek wrote:
>> On 10/10/20 18:07, Tom Lendacky wrote:
>>> From: Tom Lendacky <thomas.lendacky@amd.com>
>>>
>>> All fields that are set in the GHCB should have their associated bit in
>>> the GHCB ValidBitmap field set. Add support to set the bits for the
>>> software exit information fields when performing a VMGEXIT (SwExitCode,
>>> SwExitInfo1, SwExitInfo2).
>>>
>>> Fixes: 61bacc0fa16f ("OvmfPkg/VmgExitLib: Implement library support for VmgExitLib in OVMF")
> 
> Btw, what is the preferred method for a Fixes: tag in edk2? I've seen it
> three different ways - the above way and also:
> 
>   Fixes: 61bacc0fa16fd6f595a2c4222425cb6286e19977
> 
> and
> 
>   Fixes: 61bacc0fa16f ("OvmfPkg/VmgExitLib: Implement library support ...")

I don't think we've been consistent :)

Personally I slightly prefer the

  Fixes: 61bacc0fa16fd6f595a2c4222425cb6286e19977

format, as a "tag" on the commit message. The natural text in the commit
message may still refer to

  This patch fixes commit 61bacc0fa16f ("OvmfPkg/VmgExitLib: Implement
  library support ..."), which did so and so...

But, it's not a big deal, either format from the two you cite should be OK.

Thanks
Laszlo

Re: [PATCH 8/9] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Disable interrupts when using GHCB

[Lendacky, Thomas]

On 10/15/20 4:45 AM, Laszlo Ersek wrote:
> On 10/10/20 18:07, Tom Lendacky wrote:
>> From: Tom Lendacky <thomas.lendacky@amd.com>
>>
>> The QemuFlashPtrWrite() flash services runtime uses the GHCB and VmgExit()
>> directly to perform the flash write when running as an SEV-ES guest. If an
>> interrupt arrives
> 
> (1) please clarify what kind of interrupt you've seen in practice (my
> guess: timer interrupt)
> 
>> between VmgInit() and VmgDone(),
> 
> (2) VmgDone() is currently an empty function (both library instances) --
> did you mean VmgExit()?
> 
> 
>> the Dr7 read in the
>> interrupt handler
> 
> (3) please clarify what interrupt handler you have in mind (function
> name and file with full path would be helpful)
> 
>> will generate a #VC, which can overwrite information in
>> the GHCB that QemuFlashPtrWrite() has set. Prevent this from occurring by
>> disabling interrupts around the usage of the GHCB.
> 
> (4) I like the last sentence, because it seems to support my suspicion
> that the problem is generic. Should we push the interrupt disablement /
> re-enablement logic into VmgInit() and VmgDone()?
> 
> For that, the pre-VmgInit() interrupt state would have to be saved (for
> restoration) somewhere.
> 
> (5) I wonder if raising the TPL to TPL_HIGH_LEVEL, rather than messing
> with interrupts explicitly, works too. (Search the UEFI spec for
> "TPL_HIGH_LEVEL".) Managing the TPL feels cleaner.
> 
> ... Either way, VmgInit() would have to output either the "old TPL", or
> the "old interrupt state", for VmgDone() to restore.
> 
> ... Hm wait, VmgInit() is called from ApWakeupFunction()
> [UefiCpuPkg/Library/MpInitLib/MpLib.c], which is built for PEI too --
> there is no "TPL" concept in PEI.
> 
> So I guess we should indeed manipulate the interrupts briefly, but I
> believe we should still have that logic occur every time we are setting
> up a VmgExit(). And so VmgInit() / VmgDone() look like the perfect
> bracket for that. What's your take?

Yes, I believe that will work nicely. I can add a second parameter to 
VmgInit() that takes a pointer to a BOOLEAN to return the interrupt state 
and then pass that to VmgDone(). To keep builds from breaking, I'll need 
to do the UefiCpuPkg and OvmfPkg changes in the same patch.

Thanks,
Tom

> 
> Thanks!
> Laszlo
> 
>>
>> Fixes: 437eb3f7a8db ("OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Bypass flash detection with SEV-ES")
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>> ---
>>   OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c | 15 +++++++++++++++
>>   1 file changed, 15 insertions(+)
>>
>> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
>> index 5d5a117c48e0..872e58db7cc0 100644
>> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
>> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
>> @@ -9,6 +9,7 @@
>>   
>>   **/
>>   
>> +#include <Library/BaseLib.h>
>>   #include <Library/UefiRuntimeLib.h>
>>   #include <Library/MemEncryptSevLib.h>
>>   #include <Library/VmgExitLib.h>
>> @@ -54,6 +55,7 @@ QemuFlashPtrWrite (
>>       GHCB                      *Ghcb;
>>       UINT32                    ScratchIndex;
>>       UINT32                    ScratchBit;
>> +    BOOLEAN                   InterruptsEnabled;
>>   
>>       Msr.GhcbPhysicalAddress = AsmReadMsr64 (MSR_SEV_ES_GHCB);
>>       Ghcb = Msr.Ghcb;
>> @@ -61,6 +63,15 @@ QemuFlashPtrWrite (
>>       ScratchIndex = GhcbSwScratch / 8;
>>       ScratchBit   = GhcbSwScratch & 0x07;
>>   
>> +    //
>> +    // Be sure that an interrupt can't cause a #VC while the GHCB is
>> +    // being used.
>> +    //
>> +    InterruptsEnabled = GetInterruptState ();
>> +    if (InterruptsEnabled) {
>> +      DisableInterrupts ();
>> +    }
>> +
>>       //
>>       // Writing to flash is emulated by the hypervisor through the use of write
>>       // protection. This won't work for an SEV-ES guest because the write won't
>> @@ -74,6 +85,10 @@ QemuFlashPtrWrite (
>>       Ghcb->SaveArea.ValidBitmap[ScratchIndex] |= (1 << ScratchBit);
>>       VmgExit (Ghcb, SVM_EXIT_MMIO_WRITE, (UINT64) (UINTN) Ptr, 1);
>>       VmgDone (Ghcb);
>> +
>> +    if (InterruptsEnabled) {
>> +      EnableInterrupts ();
>> +    }
>>     } else {
>>       *Ptr = Value;
>>     }
>>
>