[PATCH v2 00/11] SEV-ES guest support fixes and cleanup

[PATCH v2 00/11] 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:

- Update the calculation of the qword offset of fields within the GHCB
  by removing the hardcoding of the offsets and using the OFFSET_OF ()
  and sizeof () functions to calculate the values. Remove unused values
  and add values that will be used in later patches.

- Set the SwExitCode, SwExitInfo1, SwExitInfo2 and SwScratch valid bits
  in the GHCB ValidBitmap area when these fields are for a VMGEXIT. This
  is done by adding two new interfaces to the VmgExitLib library to set
  and test the bits of the GHCB ValidBitmap. This reduces code duplication
  and keeps access to the ValidBitmap field within the VmgExitLib library.

- Update the Qemu flash drive services support to add SEV-ES support for
  erasing blocks.

- Disable interrupts when using the GHCB.

- Use the processor number for setting the AP stack pointer instead of the
  APIC ID by calling GetProcessorNumber().

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3008

---

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>

Changes since v1:
- For the GHCB savearea changes, create a new reserved area name instead
  of "renumbering" the reserved areas.
- Rework the ValidBitmap set/test support to be part of the VmgExitLib
  library. Create two new interfaces for setting and testing bits in the
  GHCB ValidBitmap field and adjust all existing code and the new code in
  this series to use these interfaces for the ValidBitmap updates/checks.
- Don't disable interrupts for just the Qemu flash services support, but
  rather, cover all users of the GHCB by disabling interrupts in VmgInit()
  and restoring them in VmgDone(). This requires changes to those
  interaces.

Tom Lendacky (11):
  MdePkg, OvmfPkg: Clean up GHCB field offsets and save area
  UefiCpuPkg/VmgExitLib: Add interfaces to set/read GHCB ValidBitmap
    bits
  OvmfPkg/VmgExitLib: Implement new VmgExitLib interfaces
  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
  UefiCpuPkg, OvmfPkg: Disable interrupts when using the GHCB
  UefiCpuPkg/MpInitLib: For SEV-ES guest, set stack based on processor
    number

 MdePkg/Include/Register/Amd/Ghcb.h                    |  40 +++---
 UefiCpuPkg/Include/Library/VmgExitLib.h               |  51 +++++++-
 OvmfPkg/Library/VmgExitLib/VmgExitLib.c               |  84 ++++++++++++-
 OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c         | 129 ++++++--------------
 OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c    |   4 +-
 OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c |   6 +-
 UefiCpuPkg/Library/MpInitLib/DxeMpLib.c               |   5 +-
 UefiCpuPkg/Library/MpInitLib/MpLib.c                  |  14 ++-
 UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.c    |  60 +++++++--
 UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm         |   6 +
 10 files changed, 258 insertions(+), 141 deletions(-)

-- 
2.28.0

[PATCH v2 01/11] MdePkg, OvmfPkg: Clean up GHCB field offsets and save area

[Lendacky, Thomas]

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

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3008

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 redefining 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            | 40 +++++++-------------
 OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c |  4 +-
 2 files changed, 16 insertions(+), 28 deletions(-)

diff --git a/MdePkg/Include/Register/Amd/Ghcb.h b/MdePkg/Include/Register/Amd/Ghcb.h
index 54a80da0f6d7..93fb6e3cb0fc 100644
--- a/MdePkg/Include/Register/Amd/Ghcb.h
+++ b/MdePkg/Include/Register/Amd/Ghcb.h
@@ -82,35 +82,10 @@
 #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                  Reserved8[300];
   UINT64                 Rax;
   UINT8                  Reserved4[264];
   UINT64                 Rcx;
@@ -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 v2 01/11] MdePkg, OvmfPkg: Clean up GHCB field offsets and save area

[gaoliming]

Tom:

> -----邮件原件-----
> 发件人: Tom Lendacky <thomas.lendacky@amd.com>
> 发送时间: 2020年10月17日 0:09
> 收件人: devel@edk2.groups.io
> 抄送: Brijesh Singh <brijesh.singh@amd.com>; Michael D Kinney
> <michael.d.kinney@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>;
> Zhiguang Liu <zhiguang.liu@intel.com>; Jordan Justen
> <jordan.l.justen@intel.com>; Laszlo Ersek <lersek@redhat.com>; Ard
> Biesheuvel <ard.biesheuvel@arm.com>
> 主题: [PATCH v2 01/11] MdePkg, OvmfPkg: Clean up GHCB field offsets and
> save area
> 
> From: Tom Lendacky <thomas.lendacky@amd.com>
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3008
> 
> 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 redefining 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            | 40
> +++++++-------------
>  OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c |  4 +-
>  2 files changed, 16 insertions(+), 28 deletions(-)
> 

Please separate the patch for the change in OvmfPkg. 
One commit can't cross the different packages. 
I understand this is the incompatible change. But, we still need to follow
this rule. 

> diff --git a/MdePkg/Include/Register/Amd/Ghcb.h
> b/MdePkg/Include/Register/Amd/Ghcb.h
> index 54a80da0f6d7..93fb6e3cb0fc 100644
> --- a/MdePkg/Include/Register/Amd/Ghcb.h
> +++ b/MdePkg/Include/Register/Amd/Ghcb.h
> @@ -82,35 +82,10 @@
>  #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                  Reserved8[300];
>    UINT64                 Rax;
>    UINT8                  Reserved4[264];
>    UINT64                 Rcx;
> @@ -136,6 +111,19 @@ typedef PACKED struct {
>    UINT32                 GhcbUsage;
>  } GHCB;
> 
> +typedef enum {
> +  GhcbCpl          = OFFSET_OF (GHCB, SaveArea.Cpl) / sizeof (UINT64),

OFFSET_OF (GHCB, SaveArea.Cpl) is 204. But, it can't fully divide 8
(sizeof(UINT64)). Is it correct?

Thanks
Liming
> +  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

Re: 回复: [PATCH v2 01/11] MdePkg, OvmfPkg: Clean up GHCB field offsets and save area

[Lendacky, Thomas]

On 10/18/20 8:41 PM, gaoliming wrote:
> Tom:
> 
>> -----邮件原件-----
>> 发件人: Tom Lendacky <thomas.lendacky@amd.com>
>> 发送时间: 2020年10月17日 0:09
>> 收件人: devel@edk2.groups.io
>> 抄送: Brijesh Singh <brijesh.singh@amd.com>; Michael D Kinney
>> <michael.d.kinney@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>;
>> Zhiguang Liu <zhiguang.liu@intel.com>; Jordan Justen
>> <jordan.l.justen@intel.com>; Laszlo Ersek <lersek@redhat.com>; Ard
>> Biesheuvel <ard.biesheuvel@arm.com>
>> 主题: [PATCH v2 01/11] MdePkg, OvmfPkg: Clean up GHCB field offsets and
>> save area
>>
>> From: Tom Lendacky <thomas.lendacky@amd.com>
>>
>> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3008&amp;data=04%7C01%7Cthomas.lendacky%40amd.com%7C4fd17a68a915477d9d1508d873d027b1%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637386685165902990%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=NBPdQ98RyMUlma0qkEvhBjUdykQ24p7pTK9I61ttX7Q%3D&amp;reserved=0
>>
>> 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 redefining 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            | 40
>> +++++++-------------
>>  OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c |  4 +-
>>  2 files changed, 16 insertions(+), 28 deletions(-)
>>
> 
> Please separate the patch for the change in OvmfPkg. 
> One commit can't cross the different packages. 
> I understand this is the incompatible change. But, we still need to follow
> this rule. 

Ok, then I'll resubmit without the name change. To me, it's not worth
doing 3 commits just to accomplish the rename from GHCB_REGISTER to
GHCB_QWORD_OFFSET.

Thanks,
Tom

> 
>> diff --git a/MdePkg/Include/Register/Amd/Ghcb.h
>> b/MdePkg/Include/Register/Amd/Ghcb.h
>> index 54a80da0f6d7..93fb6e3cb0fc 100644
>> --- a/MdePkg/Include/Register/Amd/Ghcb.h
>> +++ b/MdePkg/Include/Register/Amd/Ghcb.h
>> @@ -82,35 +82,10 @@
>>  #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                  Reserved8[300];
>>    UINT64                 Rax;
>>    UINT8                  Reserved4[264];
>>    UINT64                 Rcx;
>> @@ -136,6 +111,19 @@ typedef PACKED struct {
>>    UINT32                 GhcbUsage;
>>  } GHCB;
>>
>> +typedef enum {
>> +  GhcbCpl          = OFFSET_OF (GHCB, SaveArea.Cpl) / sizeof (UINT64),
> 
> OFFSET_OF (GHCB, SaveArea.Cpl) is 204. But, it can't fully divide 8
> (sizeof(UINT64)). Is it correct?
> 
> Thanks
> Liming
>> +  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
> 
> 
> 

Re: 回复: [PATCH v2 01/11] MdePkg, OvmfPkg: Clean up GHCB field offsets and save area

[Laszlo Ersek]

Hi Tom,

On 10/19/20 14:50, Tom Lendacky wrote:
> On 10/18/20 8:41 PM, gaoliming wrote:

>>> +typedef enum {
>>> +  GhcbCpl          = OFFSET_OF (GHCB, SaveArea.Cpl) / sizeof (UINT64),
>>
>> OFFSET_OF (GHCB, SaveArea.Cpl) is 204. But, it can't fully divide 8
>> (sizeof(UINT64)). Is it correct?


I didn't notice this before, and from your response to Liming, I think
you may have missed Liming pointing it out as well.

Thanks
Laszlo

Re: 回复: [PATCH v2 01/11] MdePkg, OvmfPkg: Clean up GHCB field offsets and save area

[Lendacky, Thomas]

On 10/19/20 3:17 PM, Laszlo Ersek wrote:
> Hi Tom,
> 
> On 10/19/20 14:50, Tom Lendacky wrote:
>> On 10/18/20 8:41 PM, gaoliming wrote:
> 
>>>> +typedef enum {
>>>> +  GhcbCpl          = OFFSET_OF (GHCB, SaveArea.Cpl) / sizeof (UINT64),
>>>
>>> OFFSET_OF (GHCB, SaveArea.Cpl) is 204. But, it can't fully divide 8
>>> (sizeof(UINT64)). Is it correct?

Yes, this is correct. Even though the Cpl offset is not on an 8 byte
boundary and its size is not a full 8 bytes, any change in that qword
range needs to be marked. The calculation will result in a value of 25,
which is what the original hardcoded value was.

> 
> 
> I didn't notice this before, and from your response to Liming, I think
> you may have missed Liming pointing it out as well.

Yes I did miss it... thanks for spotting that, Laszlo!

Thanks,
Tom

> 
> Thanks
> Laszlo
> 

Re: 回复: [PATCH v2 01/11] MdePkg, OvmfPkg: Clean up GHCB field offsets and save area

[Laszlo Ersek]

On 10/19/20 14:50, Tom Lendacky wrote:
> On 10/18/20 8:41 PM, gaoliming wrote:

>> Please separate the patch for the change in OvmfPkg. 
>> One commit can't cross the different packages. 
>> I understand this is the incompatible change. But, we still need to follow
>> this rule. 

I disagree.

We should do whatever we can for avoiding cross-package patches, but in
some cases, they are unavoidable. This is one of those cases.

> Ok, then I'll resubmit without the name change. To me, it's not worth
> doing 3 commits just to accomplish the rename from GHCB_REGISTER to
> GHCB_QWORD_OFFSET.

When reviewing v1, I immediately thought of doing the rename in 3
commits (introduce new type, rebase client sites, remove old type). I
didn't suggest it because it wouldn't work.

I wrote:

    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).

In other words, if we attempted to do this in 3 stages, then the 2nd
patch (introducing the new type, in addition to the old type) would not
compile. The new type could not reuse the old type's enum constants
(their identifiers). So we'd either have to change the enum constant
names of the old type (and then we'd be back to square 1 -- the client
sites would have to be migrated in the same patch), or introduce the new
type with new enum constant names as well. But then, the client sites
would have to be migrated to the new enum constant names as well, not
just the new enum type name.

This is why I said that, IMO, in this case a cross-package patch was
acceptable. Because otherwise we could never rename an enum type without
also renaming the enum constants.

Thanks
Laszlo

回复: 回复: [PATCH v2 01/11] MdePkg, OvmfPkg: Clean up GHCB field offsets and save area

[gaoliming]

Laszlo and Tom:

> -----邮件原件-----
> 发件人: Laszlo Ersek <lersek@redhat.com>
> 发送时间: 2020年10月20日 4:42
> 收件人: Tom Lendacky <thomas.lendacky@amd.com>; gaoliming
> <gaoliming@byosoft.com.cn>; devel@edk2.groups.io
> 抄送: 'Brijesh Singh' <brijesh.singh@amd.com>; 'Michael D Kinney'
> <michael.d.kinney@intel.com>; 'Zhiguang Liu' <zhiguang.liu@intel.com>;
> 'Jordan Justen' <jordan.l.justen@intel.com>; 'Ard Biesheuvel'
> <ard.biesheuvel@arm.com>
> 主题: Re: 回复: [PATCH v2 01/11] MdePkg, OvmfPkg: Clean up GHCB field
> offsets and save area
> 
> On 10/19/20 14:50, Tom Lendacky wrote:
> > On 10/18/20 8:41 PM, gaoliming wrote:
> 
> >> Please separate the patch for the change in OvmfPkg.
> >> One commit can't cross the different packages.
> >> I understand this is the incompatible change. But, we still need to follow
> >> this rule.
> 
> I disagree.
> 
> We should do whatever we can for avoiding cross-package patches, but in
> some cases, they are unavoidable. This is one of those cases.

I suggest to define enum GHCB_QWORD_OFFSET, then use typedef GHCB_QWORD_OFFSET GHCB_REGISTER; in Ghcb.h
The comments can be added here to describe it is for compatibility. The old one is not recommend.

Then, the change in MdePkg is compatible. Next patch is to update OvmfPkg VmgExit to consume GHCB_QWORD_OFFSET. 

Thanks
Liming
> 
> > Ok, then I'll resubmit without the name change. To me, it's not worth
> > doing 3 commits just to accomplish the rename from GHCB_REGISTER to
> > GHCB_QWORD_OFFSET.
> 
> When reviewing v1, I immediately thought of doing the rename in 3
> commits (introduce new type, rebase client sites, remove old type). I
> didn't suggest it because it wouldn't work.
> 
> I wrote:
> 
>     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).
> 
> In other words, if we attempted to do this in 3 stages, then the 2nd
> patch (introducing the new type, in addition to the old type) would not
> compile. The new type could not reuse the old type's enum constants
> (their identifiers). So we'd either have to change the enum constant
> names of the old type (and then we'd be back to square 1 -- the client
> sites would have to be migrated in the same patch), or introduce the new
> type with new enum constant names as well. But then, the client sites
> would have to be migrated to the new enum constant names as well, not
> just the new enum type name.
> 
> This is why I said that, IMO, in this case a cross-package patch was
> acceptable. Because otherwise we could never rename an enum type without
> also renaming the enum constants.
> 
> Thanks
> Laszlo

Re: 回复: 回复: [PATCH v2 01/11] MdePkg, OvmfPkg: Clean up GHCB field offsets and save area

[Laszlo Ersek]

On 10/20/20 03:08, gaoliming wrote:
> Laszlo and Tom:
> 
>> -----邮件原件-----
>> 发件人: Laszlo Ersek <lersek@redhat.com>
>> 发送时间: 2020年10月20日 4:42
>> 收件人: Tom Lendacky <thomas.lendacky@amd.com>; gaoliming
>> <gaoliming@byosoft.com.cn>; devel@edk2.groups.io
>> 抄送: 'Brijesh Singh' <brijesh.singh@amd.com>; 'Michael D Kinney'
>> <michael.d.kinney@intel.com>; 'Zhiguang Liu' <zhiguang.liu@intel.com>;
>> 'Jordan Justen' <jordan.l.justen@intel.com>; 'Ard Biesheuvel'
>> <ard.biesheuvel@arm.com>
>> 主题: Re: 回复: [PATCH v2 01/11] MdePkg, OvmfPkg: Clean up GHCB field
>> offsets and save area
>>
>> On 10/19/20 14:50, Tom Lendacky wrote:
>>> On 10/18/20 8:41 PM, gaoliming wrote:
>>
>>>> Please separate the patch for the change in OvmfPkg.
>>>> One commit can't cross the different packages.
>>>> I understand this is the incompatible change. But, we still need to follow
>>>> this rule.
>>
>> I disagree.
>>
>> We should do whatever we can for avoiding cross-package patches, but in
>> some cases, they are unavoidable. This is one of those cases.
> 
> I suggest to define enum GHCB_QWORD_OFFSET, then use typedef GHCB_QWORD_OFFSET GHCB_REGISTER; in Ghcb.h
> The comments can be added here to describe it is for compatibility. The old one is not recommend.
> 
> Then, the change in MdePkg is compatible. Next patch is to update OvmfPkg VmgExit to consume GHCB_QWORD_OFFSET. 

Ah, I totally missed that we could use typedef to bridge the gap. That
indeed allows us to do the rename in three steps (only for the type
name, the enum constant identifiers can stay the same). After the
rename, the enum constant values can be cleaned up in a separate (4th)
patch.

Thank you!
Laszlo

> 
> Thanks
> Liming
>>
>>> Ok, then I'll resubmit without the name change. To me, it's not worth
>>> doing 3 commits just to accomplish the rename from GHCB_REGISTER to
>>> GHCB_QWORD_OFFSET.
>>
>> When reviewing v1, I immediately thought of doing the rename in 3
>> commits (introduce new type, rebase client sites, remove old type). I
>> didn't suggest it because it wouldn't work.
>>
>> I wrote:
>>
>>     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).
>>
>> In other words, if we attempted to do this in 3 stages, then the 2nd
>> patch (introducing the new type, in addition to the old type) would not
>> compile. The new type could not reuse the old type's enum constants
>> (their identifiers). So we'd either have to change the enum constant
>> names of the old type (and then we'd be back to square 1 -- the client
>> sites would have to be migrated in the same patch), or introduce the new
>> type with new enum constant names as well. But then, the client sites
>> would have to be migrated to the new enum constant names as well, not
>> just the new enum type name.
>>
>> This is why I said that, IMO, in this case a cross-package patch was
>> acceptable. Because otherwise we could never rename an enum type without
>> also renaming the enum constants.
>>
>> Thanks
>> Laszlo
> 
> 
> 

Re: 回复: 回复: [PATCH v2 01/11] MdePkg, OvmfPkg: Clean up GHCB field offsets and save area

[Lendacky, Thomas]

On 10/20/20 3:31 AM, Laszlo Ersek wrote:
> On 10/20/20 03:08, gaoliming wrote:
>> Laszlo and Tom:
>>
>>> -----邮件原件-----
>>> 发件人: Laszlo Ersek <lersek@redhat.com>
>>> 发送时间: 2020年10月20日 4:42
>>> 收件人: Tom Lendacky <thomas.lendacky@amd.com>; gaoliming
>>> <gaoliming@byosoft.com.cn>; devel@edk2.groups.io
>>> 抄送: 'Brijesh Singh' <brijesh.singh@amd.com>; 'Michael D Kinney'
>>> <michael.d.kinney@intel.com>; 'Zhiguang Liu' <zhiguang.liu@intel.com>;
>>> 'Jordan Justen' <jordan.l.justen@intel.com>; 'Ard Biesheuvel'
>>> <ard.biesheuvel@arm.com>
>>> 主题: Re: 回复: [PATCH v2 01/11] MdePkg, OvmfPkg: Clean up GHCB field
>>> offsets and save area
>>>
>>> On 10/19/20 14:50, Tom Lendacky wrote:
>>>> On 10/18/20 8:41 PM, gaoliming wrote:
>>>
>>>>> Please separate the patch for the change in OvmfPkg.
>>>>> One commit can't cross the different packages.
>>>>> I understand this is the incompatible change. But, we still need to follow
>>>>> this rule.
>>>
>>> I disagree.
>>>
>>> We should do whatever we can for avoiding cross-package patches, but in
>>> some cases, they are unavoidable. This is one of those cases.
>>
>> I suggest to define enum GHCB_QWORD_OFFSET, then use typedef GHCB_QWORD_OFFSET GHCB_REGISTER; in Ghcb.h
>> The comments can be added here to describe it is for compatibility. The old one is not recommend.
>>
>> Then, the change in MdePkg is compatible. Next patch is to update OvmfPkg VmgExit to consume GHCB_QWORD_OFFSET. 
> 
> Ah, I totally missed that we could use typedef to bridge the gap. That
> indeed allows us to do the rename in three steps (only for the type
> name, the enum constant identifiers can stay the same). After the
> rename, the enum constant values can be cleaned up in a separate (4th)
> patch.

It seems like a lot of churn for just renaming. If there are no
objections, I'll keep the GHCB_REGISTER name. The important piece is the
change from hardcoding the offset values to using a calculated value.

Thanks,
Tom

> 
> Thank you!
> Laszlo
> 
>>
>> Thanks
>> Liming
>>>
>>>> Ok, then I'll resubmit without the name change. To me, it's not worth
>>>> doing 3 commits just to accomplish the rename from GHCB_REGISTER to
>>>> GHCB_QWORD_OFFSET.
>>>
>>> When reviewing v1, I immediately thought of doing the rename in 3
>>> commits (introduce new type, rebase client sites, remove old type). I
>>> didn't suggest it because it wouldn't work.
>>>
>>> I wrote:
>>>
>>>     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).
>>>
>>> In other words, if we attempted to do this in 3 stages, then the 2nd
>>> patch (introducing the new type, in addition to the old type) would not
>>> compile. The new type could not reuse the old type's enum constants
>>> (their identifiers). So we'd either have to change the enum constant
>>> names of the old type (and then we'd be back to square 1 -- the client
>>> sites would have to be migrated in the same patch), or introduce the new
>>> type with new enum constant names as well. But then, the client sites
>>> would have to be migrated to the new enum constant names as well, not
>>> just the new enum type name.
>>>
>>> This is why I said that, IMO, in this case a cross-package patch was
>>> acceptable. Because otherwise we could never rename an enum type without
>>> also renaming the enum constants.
>>>
>>> Thanks
>>> Laszlo
>>
>>
>>
> 

回复: 回复: 回复: [PATCH v2 01/11] MdePkg, OvmfPkg: Clean up GHCB field offsets and save area

[gaoliming]

Tom:

> -----邮件原件-----
> 发件人: Tom Lendacky <thomas.lendacky@amd.com>
> 发送时间: 2020年10月20日 21:10
> 收件人: Laszlo Ersek <lersek@redhat.com>; gaoliming
> <gaoliming@byosoft.com.cn>; devel@edk2.groups.io
> 抄送: 'Brijesh Singh' <brijesh.singh@amd.com>; 'Michael D Kinney'
> <michael.d.kinney@intel.com>; 'Zhiguang Liu' <zhiguang.liu@intel.com>;
> 'Jordan Justen' <jordan.l.justen@intel.com>; 'Ard Biesheuvel'
> <ard.biesheuvel@arm.com>
> 主题: Re: 回复: 回复: [PATCH v2 01/11] MdePkg, OvmfPkg: Clean up GHCB
> field offsets and save area
> 
> On 10/20/20 3:31 AM, Laszlo Ersek wrote:
> > On 10/20/20 03:08, gaoliming wrote:
> >> Laszlo and Tom:
> >>
> >>> -----邮件原件-----
> >>> 发件人: Laszlo Ersek <lersek@redhat.com>
> >>> 发送时间: 2020年10月20日 4:42
> >>> 收件人: Tom Lendacky <thomas.lendacky@amd.com>; gaoliming
> >>> <gaoliming@byosoft.com.cn>; devel@edk2.groups.io
> >>> 抄送: 'Brijesh Singh' <brijesh.singh@amd.com>; 'Michael D Kinney'
> >>> <michael.d.kinney@intel.com>; 'Zhiguang Liu' <zhiguang.liu@intel.com>;
> >>> 'Jordan Justen' <jordan.l.justen@intel.com>; 'Ard Biesheuvel'
> >>> <ard.biesheuvel@arm.com>
> >>> 主题: Re: 回复: [PATCH v2 01/11] MdePkg, OvmfPkg: Clean up GHCB
> field
> >>> offsets and save area
> >>>
> >>> On 10/19/20 14:50, Tom Lendacky wrote:
> >>>> On 10/18/20 8:41 PM, gaoliming wrote:
> >>>
> >>>>> Please separate the patch for the change in OvmfPkg.
> >>>>> One commit can't cross the different packages.
> >>>>> I understand this is the incompatible change. But, we still need to
> follow
> >>>>> this rule.
> >>>
> >>> I disagree.
> >>>
> >>> We should do whatever we can for avoiding cross-package patches, but in
> >>> some cases, they are unavoidable. This is one of those cases.
> >>
> >> I suggest to define enum GHCB_QWORD_OFFSET, then use typedef
> GHCB_QWORD_OFFSET GHCB_REGISTER; in Ghcb.h
> >> The comments can be added here to describe it is for compatibility. The old
> one is not recommend.
> >>
> >> Then, the change in MdePkg is compatible. Next patch is to update
> OvmfPkg VmgExit to consume GHCB_QWORD_OFFSET.
> >
> > Ah, I totally missed that we could use typedef to bridge the gap. That
> > indeed allows us to do the rename in three steps (only for the type
> > name, the enum constant identifiers can stay the same). After the
> > rename, the enum constant values can be cleaned up in a separate (4th)
> > patch.
> 
> It seems like a lot of churn for just renaming. If there are no
> objections, I'll keep the GHCB_REGISTER name. The important piece is the
> change from hardcoding the offset values to using a calculated value.
> 
I am fine to keep current GHCB_REGISTER name. 

Thanks
Liming

> Thanks,
> Tom
> 
> >
> > Thank you!
> > Laszlo
> >
> >>
> >> Thanks
> >> Liming
> >>>
> >>>> Ok, then I'll resubmit without the name change. To me, it's not worth
> >>>> doing 3 commits just to accomplish the rename from GHCB_REGISTER to
> >>>> GHCB_QWORD_OFFSET.
> >>>
> >>> When reviewing v1, I immediately thought of doing the rename in 3
> >>> commits (introduce new type, rebase client sites, remove old type). I
> >>> didn't suggest it because it wouldn't work.
> >>>
> >>> I wrote:
> >>>
> >>>     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).
> >>>
> >>> In other words, if we attempted to do this in 3 stages, then the 2nd
> >>> patch (introducing the new type, in addition to the old type) would not
> >>> compile. The new type could not reuse the old type's enum constants
> >>> (their identifiers). So we'd either have to change the enum constant
> >>> names of the old type (and then we'd be back to square 1 -- the client
> >>> sites would have to be migrated in the same patch), or introduce the new
> >>> type with new enum constant names as well. But then, the client sites
> >>> would have to be migrated to the new enum constant names as well, not
> >>> just the new enum type name.
> >>>
> >>> This is why I said that, IMO, in this case a cross-package patch was
> >>> acceptable. Because otherwise we could never rename an enum type
> without
> >>> also renaming the enum constants.
> >>>
> >>> Thanks
> >>> Laszlo
> >>
> >>
> >>
> >

Re: 回复: 回复: 回复: [PATCH v2 01/11] MdePkg, OvmfPkg: Clean up GHCB field offsets and save area

[Lendacky, Thomas]

On 10/20/20 7:54 PM, gaoliming wrote:
> Tom:

Hi Liming,

> 
>> -----邮件原件-----
>> 发件人: Tom Lendacky <thomas.lendacky@amd.com>
>> 发送时间: 2020年10月20日 21:10
>> 收件人: Laszlo Ersek <lersek@redhat.com>; gaoliming
>> <gaoliming@byosoft.com.cn>; devel@edk2.groups.io
>> 抄送: 'Brijesh Singh' <brijesh.singh@amd.com>; 'Michael D Kinney'
>> <michael.d.kinney@intel.com>; 'Zhiguang Liu' <zhiguang.liu@intel.com>;
>> 'Jordan Justen' <jordan.l.justen@intel.com>; 'Ard Biesheuvel'
>> <ard.biesheuvel@arm.com>
>> 主题: Re: 回复: 回复: [PATCH v2 01/11] MdePkg, OvmfPkg: Clean up GHCB
>> field offsets and save area
>>
>> On 10/20/20 3:31 AM, Laszlo Ersek wrote:
>>> On 10/20/20 03:08, gaoliming wrote:
>>>> Laszlo and Tom:
>>>>
>>>>> -----邮件原件-----
>>>>> 发件人: Laszlo Ersek <lersek@redhat.com>
>>>>> 发送时间: 2020年10月20日 4:42
>>>>> 收件人: Tom Lendacky <thomas.lendacky@amd.com>; gaoliming
>>>>> <gaoliming@byosoft.com.cn>; devel@edk2.groups.io
>>>>> 抄送: 'Brijesh Singh' <brijesh.singh@amd.com>; 'Michael D Kinney'
>>>>> <michael.d.kinney@intel.com>; 'Zhiguang Liu' <zhiguang.liu@intel.com>;
>>>>> 'Jordan Justen' <jordan.l.justen@intel.com>; 'Ard Biesheuvel'
>>>>> <ard.biesheuvel@arm.com>
>>>>> 主题: Re: 回复: [PATCH v2 01/11] MdePkg, OvmfPkg: Clean up GHCB
>> field
>>>>> offsets and save area
>>>>>
>>>>> On 10/19/20 14:50, Tom Lendacky wrote:
>>>>>> On 10/18/20 8:41 PM, gaoliming wrote:
>>>>>
>>>>>>> Please separate the patch for the change in OvmfPkg.
>>>>>>> One commit can't cross the different packages.
>>>>>>> I understand this is the incompatible change. But, we still need to
>> follow
>>>>>>> this rule.
>>>>>
>>>>> I disagree.
>>>>>
>>>>> We should do whatever we can for avoiding cross-package patches, but in
>>>>> some cases, they are unavoidable. This is one of those cases.
>>>>
>>>> I suggest to define enum GHCB_QWORD_OFFSET, then use typedef
>> GHCB_QWORD_OFFSET GHCB_REGISTER; in Ghcb.h
>>>> The comments can be added here to describe it is for compatibility. The old
>> one is not recommend.
>>>>
>>>> Then, the change in MdePkg is compatible. Next patch is to update
>> OvmfPkg VmgExit to consume GHCB_QWORD_OFFSET.
>>>
>>> Ah, I totally missed that we could use typedef to bridge the gap. That
>>> indeed allows us to do the rename in three steps (only for the type
>>> name, the enum constant identifiers can stay the same). After the
>>> rename, the enum constant values can be cleaned up in a separate (4th)
>>> patch.
>>
>> It seems like a lot of churn for just renaming. If there are no
>> objections, I'll keep the GHCB_REGISTER name. The important piece is the
>> change from hardcoding the offset values to using a calculated value.
>>
> I am fine to keep current GHCB_REGISTER name. 

This raises a question about patch 10 of the series. Since that patch
changes interfaces, I included both the UefiCpuPkg and OvmfPkg changes in
that one patch. Will that be acceptable?

Thanks,
Tom

> 
> Thanks
> Liming
> 
>> Thanks,
>> Tom
>>
>>>
>>> Thank you!
>>> Laszlo
>>>
>>>>
>>>> Thanks
>>>> Liming
>>>>>
>>>>>> Ok, then I'll resubmit without the name change. To me, it's not worth
>>>>>> doing 3 commits just to accomplish the rename from GHCB_REGISTER to
>>>>>> GHCB_QWORD_OFFSET.
>>>>>
>>>>> When reviewing v1, I immediately thought of doing the rename in 3
>>>>> commits (introduce new type, rebase client sites, remove old type). I
>>>>> didn't suggest it because it wouldn't work.
>>>>>
>>>>> I wrote:
>>>>>
>>>>>     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).
>>>>>
>>>>> In other words, if we attempted to do this in 3 stages, then the 2nd
>>>>> patch (introducing the new type, in addition to the old type) would not
>>>>> compile. The new type could not reuse the old type's enum constants
>>>>> (their identifiers). So we'd either have to change the enum constant
>>>>> names of the old type (and then we'd be back to square 1 -- the client
>>>>> sites would have to be migrated in the same patch), or introduce the new
>>>>> type with new enum constant names as well. But then, the client sites
>>>>> would have to be migrated to the new enum constant names as well, not
>>>>> just the new enum type name.
>>>>>
>>>>> This is why I said that, IMO, in this case a cross-package patch was
>>>>> acceptable. Because otherwise we could never rename an enum type
>> without
>>>>> also renaming the enum constants.
>>>>>
>>>>> Thanks
>>>>> Laszlo
>>>>
>>>>
>>>>
>>>
> 
> 

回复: [edk2-devel] 回复: 回复: 回复: [PATCH v2 01/11] MdePkg, OvmfPkg: Clean up GHCB field offsets and save area

[gaoliming]

Tom:
  For the patch in UefiCpuPkg, this changes the library interface. So, the consumer and producer code is required to be changed together. There is no good compatible way to do it. I still prefer to separate them for the different package. Although the first commit breaks the tree, your patch is the patch serial, they will be merged together. Its impact should be small. 

Thanks
Liming
> -----邮件原件-----
> 发件人: bounce+27952+66512+4905953+8761045@groups.io
> <bounce+27952+66512+4905953+8761045@groups.io> 代表 Lendacky,
> Thomas
> 发送时间: 2020年10月22日 5:54
> 收件人: gaoliming <gaoliming@byosoft.com.cn>; 'Laszlo Ersek'
> <lersek@redhat.com>; devel@edk2.groups.io
> 抄送: 'Brijesh Singh' <brijesh.singh@amd.com>; 'Michael D Kinney'
> <michael.d.kinney@intel.com>; 'Zhiguang Liu' <zhiguang.liu@intel.com>;
> 'Jordan Justen' <jordan.l.justen@intel.com>; 'Ard Biesheuvel'
> <ard.biesheuvel@arm.com>
> 主题: Re: [edk2-devel] 回复: 回复: 回复: [PATCH v2 01/11] MdePkg,
> OvmfPkg: Clean up GHCB field offsets and save area
> 
> On 10/20/20 7:54 PM, gaoliming wrote:
> > Tom:
> 
> Hi Liming,
> 
> >
> >> -----邮件原件-----
> >> 发件人: Tom Lendacky <thomas.lendacky@amd.com>
> >> 发送时间: 2020年10月20日 21:10
> >> 收件人: Laszlo Ersek <lersek@redhat.com>; gaoliming
> >> <gaoliming@byosoft.com.cn>; devel@edk2.groups.io
> >> 抄送: 'Brijesh Singh' <brijesh.singh@amd.com>; 'Michael D Kinney'
> >> <michael.d.kinney@intel.com>; 'Zhiguang Liu' <zhiguang.liu@intel.com>;
> >> 'Jordan Justen' <jordan.l.justen@intel.com>; 'Ard Biesheuvel'
> >> <ard.biesheuvel@arm.com>
> >> 主题: Re: 回复: 回复: [PATCH v2 01/11] MdePkg, OvmfPkg: Clean up
> GHCB
> >> field offsets and save area
> >>
> >> On 10/20/20 3:31 AM, Laszlo Ersek wrote:
> >>> On 10/20/20 03:08, gaoliming wrote:
> >>>> Laszlo and Tom:
> >>>>
> >>>>> -----邮件原件-----
> >>>>> 发件人: Laszlo Ersek <lersek@redhat.com>
> >>>>> 发送时间: 2020年10月20日 4:42
> >>>>> 收件人: Tom Lendacky <thomas.lendacky@amd.com>; gaoliming
> >>>>> <gaoliming@byosoft.com.cn>; devel@edk2.groups.io
> >>>>> 抄送: 'Brijesh Singh' <brijesh.singh@amd.com>; 'Michael D Kinney'
> >>>>> <michael.d.kinney@intel.com>; 'Zhiguang Liu'
> <zhiguang.liu@intel.com>;
> >>>>> 'Jordan Justen' <jordan.l.justen@intel.com>; 'Ard Biesheuvel'
> >>>>> <ard.biesheuvel@arm.com>
> >>>>> 主题: Re: 回复: [PATCH v2 01/11] MdePkg, OvmfPkg: Clean up GHCB
> >> field
> >>>>> offsets and save area
> >>>>>
> >>>>> On 10/19/20 14:50, Tom Lendacky wrote:
> >>>>>> On 10/18/20 8:41 PM, gaoliming wrote:
> >>>>>
> >>>>>>> Please separate the patch for the change in OvmfPkg.
> >>>>>>> One commit can't cross the different packages.
> >>>>>>> I understand this is the incompatible change. But, we still need to
> >> follow
> >>>>>>> this rule.
> >>>>>
> >>>>> I disagree.
> >>>>>
> >>>>> We should do whatever we can for avoiding cross-package patches, but
> in
> >>>>> some cases, they are unavoidable. This is one of those cases.
> >>>>
> >>>> I suggest to define enum GHCB_QWORD_OFFSET, then use typedef
> >> GHCB_QWORD_OFFSET GHCB_REGISTER; in Ghcb.h
> >>>> The comments can be added here to describe it is for compatibility. The
> old
> >> one is not recommend.
> >>>>
> >>>> Then, the change in MdePkg is compatible. Next patch is to update
> >> OvmfPkg VmgExit to consume GHCB_QWORD_OFFSET.
> >>>
> >>> Ah, I totally missed that we could use typedef to bridge the gap. That
> >>> indeed allows us to do the rename in three steps (only for the type
> >>> name, the enum constant identifiers can stay the same). After the
> >>> rename, the enum constant values can be cleaned up in a separate (4th)
> >>> patch.
> >>
> >> It seems like a lot of churn for just renaming. If there are no
> >> objections, I'll keep the GHCB_REGISTER name. The important piece is the
> >> change from hardcoding the offset values to using a calculated value.
> >>
> > I am fine to keep current GHCB_REGISTER name.
> 
> This raises a question about patch 10 of the series. Since that patch
> changes interfaces, I included both the UefiCpuPkg and OvmfPkg changes in
> that one patch. Will that be acceptable?
> 
> Thanks,
> Tom
> 
> >
> > Thanks
> > Liming
> >
> >> Thanks,
> >> Tom
> >>
> >>>
> >>> Thank you!
> >>> Laszlo
> >>>
> >>>>
> >>>> Thanks
> >>>> Liming
> >>>>>
> >>>>>> Ok, then I'll resubmit without the name change. To me, it's not worth
> >>>>>> doing 3 commits just to accomplish the rename from GHCB_REGISTER
> to
> >>>>>> GHCB_QWORD_OFFSET.
> >>>>>
> >>>>> When reviewing v1, I immediately thought of doing the rename in 3
> >>>>> commits (introduce new type, rebase client sites, remove old type). I
> >>>>> didn't suggest it because it wouldn't work.
> >>>>>
> >>>>> I wrote:
> >>>>>
> >>>>>     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).
> >>>>>
> >>>>> In other words, if we attempted to do this in 3 stages, then the 2nd
> >>>>> patch (introducing the new type, in addition to the old type) would not
> >>>>> compile. The new type could not reuse the old type's enum constants
> >>>>> (their identifiers). So we'd either have to change the enum constant
> >>>>> names of the old type (and then we'd be back to square 1 -- the client
> >>>>> sites would have to be migrated in the same patch), or introduce the
> new
> >>>>> type with new enum constant names as well. But then, the client sites
> >>>>> would have to be migrated to the new enum constant names as well,
> not
> >>>>> just the new enum type name.
> >>>>>
> >>>>> This is why I said that, IMO, in this case a cross-package patch was
> >>>>> acceptable. Because otherwise we could never rename an enum type
> >> without
> >>>>> also renaming the enum constants.
> >>>>>
> >>>>> Thanks
> >>>>> Laszlo
> >>>>
> >>>>
> >>>>
> >>>
> >
> >
> 
> 
> 
> 

Re: 回复: [edk2-devel] 回复: 回复: 回复: [PATCH v2 01/11] MdePkg, OvmfPkg: Clean up GHCB field offsets and save area

[Laszlo Ersek]

On 10/22/20 03:25, gaoliming wrote:
> Tom:
>   For the patch in UefiCpuPkg, this changes the library interface. So, the consumer and producer code is required to be changed together. There is no good compatible way to do it. I still prefer to separate them for the different package. Although the first commit breaks the tree, your patch is the patch serial, they will be merged together. Its impact should be small. 

I really disagree here. Bisectability is way more important, in my
opinion, than avoiding a multi-package patch.

I agree we should avoid multi-package patches as much as we can -- but
not more than we can. Bisectability (= keeping the tree buildable at
every commit) is very important, in my opinion.

Thanks,
Laszlo

回复: 回复: [edk2-devel] 回复: 回复: 回复: [PATCH v2 01/11] MdePkg, OvmfPkg: Clean up GHCB field offsets and save area

[gaoliming]

Laszlo:
 I agree Bisectability is important. I want to consider the impact and balance the change. Now, this change will update UefiCpuPkg. I also want to collect the feedback from UefiCpuPkg maintainer: Ray and Eric. 

Thanks
Liming
> -----邮件原件-----
> 发件人: bounce+27952+66536+4905953+8761045@groups.io
> <bounce+27952+66536+4905953+8761045@groups.io> 代表 Laszlo Ersek
> 发送时间: 2020年10月22日 21:13
> 收件人: gaoliming <gaoliming@byosoft.com.cn>; devel@edk2.groups.io;
> thomas.lendacky@amd.com
> 抄送: 'Brijesh Singh' <brijesh.singh@amd.com>; 'Michael D Kinney'
> <michael.d.kinney@intel.com>; 'Zhiguang Liu' <zhiguang.liu@intel.com>;
> 'Jordan Justen' <jordan.l.justen@intel.com>; 'Ard Biesheuvel'
> <ard.biesheuvel@arm.com>
> 主题: Re: 回复: [edk2-devel] 回复: 回复: 回复: [PATCH v2 01/11] MdePkg,
> OvmfPkg: Clean up GHCB field offsets and save area
> 
> On 10/22/20 03:25, gaoliming wrote:
> > Tom:
> >   For the patch in UefiCpuPkg, this changes the library interface. So, the
> consumer and producer code is required to be changed together. There is no
> good compatible way to do it. I still prefer to separate them for the different
> package. Although the first commit breaks the tree, your patch is the patch
> serial, they will be merged together. Its impact should be small.
> 
> I really disagree here. Bisectability is way more important, in my
> opinion, than avoiding a multi-package patch.
> 
> I agree we should avoid multi-package patches as much as we can -- but
> not more than we can. Bisectability (= keeping the tree buildable at
> every commit) is very important, in my opinion.
> 
> Thanks,
> Laszlo
> 
> 
> 
> 
> 

Re: 回复: 回复: [edk2-devel] 回复: 回复: 回复: [PATCH v2 01/11] MdePkg, OvmfPkg: Clean up GHCB field offsets and save area

[Lendacky, Thomas]

On 10/22/20 9:57 PM, gaoliming wrote:
> Laszlo:
>  I agree Bisectability is important. I want to consider the impact and balance the change. Now, this change will update UefiCpuPkg. I also want to collect the feedback from UefiCpuPkg maintainer: Ray and Eric. 

Ray/Eric,

Any feedback on this question (specifically for patch #10 of this series,
not this patch)? I'd also prefer to maintain bisectability and therefore
submit the changes to the UefiCpuPkg and OvmfPkg in the same patch.

Thanks,
Tom

> 
> Thanks
> Liming
>> -----邮件原件-----
>> 发件人: bounce+27952+66536+4905953+8761045@groups.io
>> <bounce+27952+66536+4905953+8761045@groups.io> 代表 Laszlo Ersek
>> 发送时间: 2020年10月22日 21:13
>> 收件人: gaoliming <gaoliming@byosoft.com.cn>; devel@edk2.groups.io;
>> thomas.lendacky@amd.com
>> 抄送: 'Brijesh Singh' <brijesh.singh@amd.com>; 'Michael D Kinney'
>> <michael.d.kinney@intel.com>; 'Zhiguang Liu' <zhiguang.liu@intel.com>;
>> 'Jordan Justen' <jordan.l.justen@intel.com>; 'Ard Biesheuvel'
>> <ard.biesheuvel@arm.com>
>> 主题: Re: 回复: [edk2-devel] 回复: 回复: 回复: [PATCH v2 01/11] MdePkg,
>> OvmfPkg: Clean up GHCB field offsets and save area
>>
>> On 10/22/20 03:25, gaoliming wrote:
>>> Tom:
>>>   For the patch in UefiCpuPkg, this changes the library interface. So, the
>> consumer and producer code is required to be changed together. There is no
>> good compatible way to do it. I still prefer to separate them for the different
>> package. Although the first commit breaks the tree, your patch is the patch
>> serial, they will be merged together. Its impact should be small.
>>
>> I really disagree here. Bisectability is way more important, in my
>> opinion, than avoiding a multi-package patch.
>>
>> I agree we should avoid multi-package patches as much as we can -- but
>> not more than we can. Bisectability (= keeping the tree buildable at
>> every commit) is very important, in my opinion.
>>
>> Thanks,
>> Laszlo
>>
>>
>>
>> 
>>
> 
> 
> 

Re: 回复: 回复: [edk2-devel] 回复: 回复: 回复: [PATCH v2 01/11] MdePkg, OvmfPkg: Clean up GHCB field offsets and save area

[Dong, Eric]

I agree with Laszlo, we need to care Bisectability first.

Thanks,
Eric
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Lendacky, Thomas
Sent: Tuesday, October 27, 2020 12:34 AM
To: gaoliming <gaoliming@byosoft.com.cn>; devel@edk2.groups.io; lersek@redhat.com; Ni, Ray <ray.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
Cc: 'Brijesh Singh' <brijesh.singh@amd.com>; Liu, Zhiguang <zhiguang.liu@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; 'Ard Biesheuvel' <ard.biesheuvel@arm.com>
Subject: Re: 回复: 回复: [edk2-devel] 回复: 回复: 回复: [PATCH v2 01/11] MdePkg, OvmfPkg: Clean up GHCB field offsets and save area

On 10/22/20 9:57 PM, gaoliming wrote:
> Laszlo:
>  I agree Bisectability is important. I want to consider the impact and balance the change. Now, this change will update UefiCpuPkg. I also want to collect the feedback from UefiCpuPkg maintainer: Ray and Eric. 

Ray/Eric,

Any feedback on this question (specifically for patch #10 of this series, not this patch)? I'd also prefer to maintain bisectability and therefore submit the changes to the UefiCpuPkg and OvmfPkg in the same patch.

Thanks,
Tom

> 
> Thanks
> Liming
>> -----邮件原件-----
>> 发件人: bounce+27952+66536+4905953+8761045@groups.io
>> <bounce+27952+66536+4905953+8761045@groups.io> 代表 Laszlo Ersek
>> 发送时间: 2020年10月22日 21:13
>> 收件人: gaoliming <gaoliming@byosoft.com.cn>; devel@edk2.groups.io; 
>> thomas.lendacky@amd.com
>> 抄送: 'Brijesh Singh' <brijesh.singh@amd.com>; 'Michael D Kinney'
>> <michael.d.kinney@intel.com>; 'Zhiguang Liu' 
>> <zhiguang.liu@intel.com>; 'Jordan Justen' <jordan.l.justen@intel.com>; 'Ard Biesheuvel'
>> <ard.biesheuvel@arm.com>
>> 主题: Re: 回复: [edk2-devel] 回复: 回复: 回复: [PATCH v2 01/11] MdePkg,
>> OvmfPkg: Clean up GHCB field offsets and save area
>>
>> On 10/22/20 03:25, gaoliming wrote:
>>> Tom:
>>>   For the patch in UefiCpuPkg, this changes the library interface. 
>>> So, the
>> consumer and producer code is required to be changed together. There 
>> is no good compatible way to do it. I still prefer to separate them 
>> for the different package. Although the first commit breaks the tree, 
>> your patch is the patch serial, they will be merged together. Its impact should be small.
>>
>> I really disagree here. Bisectability is way more important, in my 
>> opinion, than avoiding a multi-package patch.
>>
>> I agree we should avoid multi-package patches as much as we can -- 
>> but not more than we can. Bisectability (= keeping the tree buildable 
>> at every commit) is very important, in my opinion.
>>
>> Thanks,
>> Laszlo
>>
>>
>>
>> 
>>
> 
> 
> 

Re: 回复: 回复: [edk2-devel] 回复: 回复: 回复: [PATCH v2 01/11] MdePkg, OvmfPkg: Clean up GHCB field offsets and save area

[Lendacky, Thomas]

On 10/27/20 2:57 AM, Dong, Eric wrote:
> I agree with Laszlo, we need to care Bisectability first.

Thanks for responding, Eric. I'll submit v3 as previously submitted.

Thanks,
Tom

> 
> Thanks,
> Eric
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Lendacky, Thomas
> Sent: Tuesday, October 27, 2020 12:34 AM
> To: gaoliming <gaoliming@byosoft.com.cn>; devel@edk2.groups.io; lersek@redhat.com; Ni, Ray <ray.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: 'Brijesh Singh' <brijesh.singh@amd.com>; Liu, Zhiguang <zhiguang.liu@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; 'Ard Biesheuvel' <ard.biesheuvel@arm.com>
> Subject: Re: 回复: 回复: [edk2-devel] 回复: 回复: 回复: [PATCH v2 01/11] MdePkg, OvmfPkg: Clean up GHCB field offsets and save area
> 
> On 10/22/20 9:57 PM, gaoliming wrote:
>> Laszlo:
>>  I agree Bisectability is important. I want to consider the impact and balance the change. Now, this change will update UefiCpuPkg. I also want to collect the feedback from UefiCpuPkg maintainer: Ray and Eric. 
> 
> Ray/Eric,
> 
> Any feedback on this question (specifically for patch #10 of this series, not this patch)? I'd also prefer to maintain bisectability and therefore submit the changes to the UefiCpuPkg and OvmfPkg in the same patch.
> 
> Thanks,
> Tom
> 
>>
>> Thanks
>> Liming
>>> -----邮件原件-----
>>> 发件人: bounce+27952+66536+4905953+8761045@groups.io
>>> <bounce+27952+66536+4905953+8761045@groups.io> 代表 Laszlo Ersek
>>> 发送时间: 2020年10月22日 21:13
>>> 收件人: gaoliming <gaoliming@byosoft.com.cn>; devel@edk2.groups.io; 
>>> thomas.lendacky@amd.com
>>> 抄送: 'Brijesh Singh' <brijesh.singh@amd.com>; 'Michael D Kinney'
>>> <michael.d.kinney@intel.com>; 'Zhiguang Liu' 
>>> <zhiguang.liu@intel.com>; 'Jordan Justen' <jordan.l.justen@intel.com>; 'Ard Biesheuvel'
>>> <ard.biesheuvel@arm.com>
>>> 主题: Re: 回复: [edk2-devel] 回复: 回复: 回复: [PATCH v2 01/11] MdePkg,
>>> OvmfPkg: Clean up GHCB field offsets and save area
>>>
>>> On 10/22/20 03:25, gaoliming wrote:
>>>> Tom:
>>>>   For the patch in UefiCpuPkg, this changes the library interface. 
>>>> So, the
>>> consumer and producer code is required to be changed together. There 
>>> is no good compatible way to do it. I still prefer to separate them 
>>> for the different package. Although the first commit breaks the tree, 
>>> your patch is the patch serial, they will be merged together. Its impact should be small.
>>>
>>> I really disagree here. Bisectability is way more important, in my 
>>> opinion, than avoiding a multi-package patch.
>>>
>>> I agree we should avoid multi-package patches as much as we can -- 
>>> but not more than we can. Bisectability (= keeping the tree buildable 
>>> at every commit) is very important, in my opinion.
>>>
>>> Thanks,
>>> Laszlo
>>>
>>>
>>>
>>>
>>>
>>
>>
>>
> 
> 
> 
> 
> 

Re: 回复: 回复: [PATCH v2 01/11] MdePkg, OvmfPkg: Clean up GHCB field offsets and save area

[Laszlo Ersek]

On 10/20/20 15:10, Tom Lendacky wrote:
> On 10/20/20 3:31 AM, Laszlo Ersek wrote:
>> On 10/20/20 03:08, gaoliming wrote:
>>> Laszlo and Tom:
>>>
>>>> -----邮件原件-----
>>>> 发件人: Laszlo Ersek <lersek@redhat.com>
>>>> 发送时间: 2020年10月20日 4:42
>>>> 收件人: Tom Lendacky <thomas.lendacky@amd.com>; gaoliming
>>>> <gaoliming@byosoft.com.cn>; devel@edk2.groups.io
>>>> 抄送: 'Brijesh Singh' <brijesh.singh@amd.com>; 'Michael D Kinney'
>>>> <michael.d.kinney@intel.com>; 'Zhiguang Liu' <zhiguang.liu@intel.com>;
>>>> 'Jordan Justen' <jordan.l.justen@intel.com>; 'Ard Biesheuvel'
>>>> <ard.biesheuvel@arm.com>
>>>> 主题: Re: 回复: [PATCH v2 01/11] MdePkg, OvmfPkg: Clean up GHCB field
>>>> offsets and save area
>>>>
>>>> On 10/19/20 14:50, Tom Lendacky wrote:
>>>>> On 10/18/20 8:41 PM, gaoliming wrote:
>>>>
>>>>>> Please separate the patch for the change in OvmfPkg.
>>>>>> One commit can't cross the different packages.
>>>>>> I understand this is the incompatible change. But, we still need to follow
>>>>>> this rule.
>>>>
>>>> I disagree.
>>>>
>>>> We should do whatever we can for avoiding cross-package patches, but in
>>>> some cases, they are unavoidable. This is one of those cases.
>>>
>>> I suggest to define enum GHCB_QWORD_OFFSET, then use typedef GHCB_QWORD_OFFSET GHCB_REGISTER; in Ghcb.h
>>> The comments can be added here to describe it is for compatibility. The old one is not recommend.
>>>
>>> Then, the change in MdePkg is compatible. Next patch is to update OvmfPkg VmgExit to consume GHCB_QWORD_OFFSET. 
>>
>> Ah, I totally missed that we could use typedef to bridge the gap. That
>> indeed allows us to do the rename in three steps (only for the type
>> name, the enum constant identifiers can stay the same). After the
>> rename, the enum constant values can be cleaned up in a separate (4th)
>> patch.
> 
> It seems like a lot of churn for just renaming.

Yes, it is. (I guess it helps downstreams that only want to cherry-pick
patches from specific packages.)

> If there are no
> objections, I'll keep the GHCB_REGISTER name. The important piece is the
> change from hardcoding the offset values to using a calculated value.

If the GHCB_REGISTER type name works for you, it certainly works for me :)

Thanks
Laszlo

[PATCH v2 02/11] UefiCpuPkg/VmgExitLib: Add interfaces to set/read GHCB ValidBitmap bits

[Lendacky, Thomas]

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

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3008

In upcoming patches, the setting of the bits in the GHCB ValidBitmap will
be performed in multiple places. In order to reduce code duplication, add
an interface, VmgSetOffsetValid(), to VmgExitLib library to perform this
function. Also, to keep management of the ValidBitmap within the library,
add an inteface, VmgIsOffsetValid(), to return whether the bit in the
ValidBitmap is set for a specified offset.

The new VmgSetOffsetValid() function is a VOID function and will be an
empty function in the VmgExitLibNull implementation of the VmgExitLib
library.

The new VmgIsOffsetValid() function returns a BOOLEAN to indicate if the
offset is valid. This will always return FALSE in the VmgExitLibNull
implementation of the VmgExitLib library.

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/Include/Library/VmgExitLib.h            | 37 +++++++++++++++++
 UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.c | 42 ++++++++++++++++++++
 2 files changed, 79 insertions(+)

diff --git a/UefiCpuPkg/Include/Library/VmgExitLib.h b/UefiCpuPkg/Include/Library/VmgExitLib.h
index 45fc27d35e29..ba5ea024839e 100644
--- a/UefiCpuPkg/Include/Library/VmgExitLib.h
+++ b/UefiCpuPkg/Include/Library/VmgExitLib.h
@@ -74,6 +74,43 @@ VmgDone (
   IN OUT GHCB                *Ghcb
   );
 
+/**
+  Marks a specified offset as valid in the GHCB.
+
+  The ValidBitmap area represents the areas of the GHCB that have been marked
+  valid. Set the bit in ValidBitmap for the input offset.
+
+  @param[in, out]  Ghcb       A pointer to the GHCB
+  @param[in]       Offset     Qword offset in the GHCB to mark valid
+
+**/
+VOID
+EFIAPI
+VmgSetOffsetValid (
+  IN OUT GHCB                *Ghcb,
+  IN     GHCB_QWORD_OFFSET   Offset
+  );
+
+/**
+  Checks if a specified offset is valid in the GHCB.
+
+  The ValidBitmap area represents the areas of the GHCB that have been marked
+  valid. Return whether the bit in the ValidBitmap is set for the input offset.
+
+  @param[in]  Ghcb            A pointer to the GHCB
+  @param[in]  Offset          Qword offset in the GHCB to mark valid
+
+  @retval TRUE                Offset is marked vald in the GHCB
+  @retval FALSE               Offset is not marked valid in the GHCB
+
+**/
+BOOLEAN
+EFIAPI
+VmgIsOffsetValid (
+  IN GHCB                    *Ghcb,
+  IN GHCB_QWORD_OFFSET       Offset
+  );
+
 /**
   Handle a #VC exception.
 
diff --git a/UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.c b/UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.c
index bb265e1700d2..b000232c472e 100644
--- a/UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.c
+++ b/UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.c
@@ -89,6 +89,48 @@ VmgDone (
 {
 }
 
+/**
+  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 bit in ValidBitmap for the input offset.
+
+  @param[in, out] Ghcb    Pointer to the Guest-Hypervisor Communication Block
+  @param[in] Offset       Qword offset in the GHCB to mark valid
+
+**/
+VOID
+EFIAPI
+VmgSetOffsetValid (
+  IN OUT GHCB                *Ghcb,
+  IN     GHCB_QWORD_OFFSET   Offset
+  )
+{
+}
+
+/**
+  Checks if a specified offset is valid in the GHCB.
+
+  The ValidBitmap area represents the areas of the GHCB that have been marked
+  valid. Return whether the bit in the ValidBitmap is set for the input offset.
+
+  @param[in]  Ghcb            A pointer to the GHCB
+  @param[in]  Offset          Qword offset in the GHCB to mark valid
+
+  @retval TRUE                Offset is marked vald in the GHCB
+  @retval FALSE               Offset is not marked valid in the GHCB
+
+**/
+BOOLEAN
+EFIAPI
+VmgIsOffsetValid (
+  IN GHCB                    *Ghcb,
+  IN GHCB_QWORD_OFFSET       Offset
+  )
+{
+  return FALSE;
+}
+
 /**
   Handle a #VC exception.
 
-- 
2.28.0

Re: [edk2-devel] [PATCH v2 02/11] UefiCpuPkg/VmgExitLib: Add interfaces to set/read GHCB ValidBitmap bits

[Laszlo Ersek]

On 10/16/20 18:09, Lendacky, Thomas wrote:
> From: Tom Lendacky <thomas.lendacky@amd.com>
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3008
> 
> In upcoming patches, the setting of the bits in the GHCB ValidBitmap will
> be performed in multiple places. In order to reduce code duplication, add
> an interface, VmgSetOffsetValid(), to VmgExitLib library to perform this
> function. Also, to keep management of the ValidBitmap within the library,
> add an inteface, VmgIsOffsetValid(), to return whether the bit in the
> ValidBitmap is set for a specified offset.
> 
> The new VmgSetOffsetValid() function is a VOID function and will be an
> empty function in the VmgExitLibNull implementation of the VmgExitLib
> library.
> 
> The new VmgIsOffsetValid() function returns a BOOLEAN to indicate if the
> offset is valid. This will always return FALSE in the VmgExitLibNull
> implementation of the VmgExitLib library.
> 
> 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/Include/Library/VmgExitLib.h            | 37 +++++++++++++++++
>  UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.c | 42 ++++++++++++++++++++
>  2 files changed, 79 insertions(+)
> 
> diff --git a/UefiCpuPkg/Include/Library/VmgExitLib.h b/UefiCpuPkg/Include/Library/VmgExitLib.h
> index 45fc27d35e29..ba5ea024839e 100644
> --- a/UefiCpuPkg/Include/Library/VmgExitLib.h
> +++ b/UefiCpuPkg/Include/Library/VmgExitLib.h
> @@ -74,6 +74,43 @@ VmgDone (
>    IN OUT GHCB                *Ghcb
>    );
>  
> +/**
> +  Marks a specified offset as valid in the GHCB.
> +
> +  The ValidBitmap area represents the areas of the GHCB that have been marked
> +  valid. Set the bit in ValidBitmap for the input offset.
> +
> +  @param[in, out]  Ghcb       A pointer to the GHCB
> +  @param[in]       Offset     Qword offset in the GHCB to mark valid
> +
> +**/
> +VOID
> +EFIAPI
> +VmgSetOffsetValid (
> +  IN OUT GHCB                *Ghcb,
> +  IN     GHCB_QWORD_OFFSET   Offset
> +  );
> +
> +/**
> +  Checks if a specified offset is valid in the GHCB.
> +
> +  The ValidBitmap area represents the areas of the GHCB that have been marked
> +  valid. Return whether the bit in the ValidBitmap is set for the input offset.
> +
> +  @param[in]  Ghcb            A pointer to the GHCB
> +  @param[in]  Offset          Qword offset in the GHCB to mark valid
> +
> +  @retval TRUE                Offset is marked vald in the GHCB
> +  @retval FALSE               Offset is not marked valid in the GHCB
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +VmgIsOffsetValid (
> +  IN GHCB                    *Ghcb,
> +  IN GHCB_QWORD_OFFSET       Offset
> +  );
> +
>  /**
>    Handle a #VC exception.
>  
> diff --git a/UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.c b/UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.c
> index bb265e1700d2..b000232c472e 100644
> --- a/UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.c
> +++ b/UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.c
> @@ -89,6 +89,48 @@ VmgDone (
>  {
>  }
>  
> +/**
> +  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 bit in ValidBitmap for the input offset.
> +
> +  @param[in, out] Ghcb    Pointer to the Guest-Hypervisor Communication Block
> +  @param[in] Offset       Qword offset in the GHCB to mark valid
> +
> +**/
> +VOID
> +EFIAPI
> +VmgSetOffsetValid (
> +  IN OUT GHCB                *Ghcb,
> +  IN     GHCB_QWORD_OFFSET   Offset
> +  )
> +{
> +}
> +
> +/**
> +  Checks if a specified offset is valid in the GHCB.
> +
> +  The ValidBitmap area represents the areas of the GHCB that have been marked
> +  valid. Return whether the bit in the ValidBitmap is set for the input offset.
> +
> +  @param[in]  Ghcb            A pointer to the GHCB
> +  @param[in]  Offset          Qword offset in the GHCB to mark valid
> +
> +  @retval TRUE                Offset is marked vald in the GHCB
> +  @retval FALSE               Offset is not marked valid in the GHCB
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +VmgIsOffsetValid (
> +  IN GHCB                    *Ghcb,
> +  IN GHCB_QWORD_OFFSET       Offset
> +  )
> +{
> +  return FALSE;
> +}
> +
>  /**
>    Handle a #VC exception.
>  
> 

s/vald/valid/, twice

With that fixed:

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

Thanks
Laszlo

[PATCH v2 03/11] OvmfPkg/VmgExitLib: Implement new VmgExitLib interfaces

[Lendacky, Thomas]

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

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3008

The VmgExitLib library added two new interfaces, VmgSetOffsetValid() and
VmgIsOffsetValid(), that must now be implemented in the OvmfPkg version
of the library.

Implement VmgSetOffsetValid() and VmgIsOffsetValid() and update existing
code, that is directly accessing ValidBitmap, to use the new interfaces.

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       |  54 +++++++++
 OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c | 118 +++++---------------
 2 files changed, 85 insertions(+), 87 deletions(-)

diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitLib.c b/OvmfPkg/Library/VmgExitLib/VmgExitLib.c
index 53040cc6f649..3072c2265df7 100644
--- a/OvmfPkg/Library/VmgExitLib/VmgExitLib.c
+++ b/OvmfPkg/Library/VmgExitLib/VmgExitLib.c
@@ -157,3 +157,57 @@ VmgDone (
 {
 }
 
+/**
+  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 bit in ValidBitmap for the input offset.
+
+  @param[in, out] Ghcb    Pointer to the Guest-Hypervisor Communication Block
+  @param[in]      Offset  Qword offset in the GHCB to mark valid
+
+**/
+VOID
+EFIAPI
+VmgSetOffsetValid (
+  IN OUT GHCB                *Ghcb,
+  IN     GHCB_QWORD_OFFSET   Offset
+  )
+{
+  UINT32  OffsetIndex;
+  UINT32  OffsetBit;
+
+  OffsetIndex = Offset / 8;
+  OffsetBit   = Offset % 8;
+
+  Ghcb->SaveArea.ValidBitmap[OffsetIndex] |= (1 << OffsetBit);
+}
+
+/**
+  Checks if a specified offset is valid in the GHCB.
+
+  The ValidBitmap area represents the areas of the GHCB that have been marked
+  valid. Return whether the bit in the ValidBitmap is set for the input offset.
+
+  @param[in]  Ghcb            A pointer to the GHCB
+  @param[in]  Offset          Qword offset in the GHCB to mark valid
+
+  @retval TRUE                Offset is marked vald in the GHCB
+  @retval FALSE               Offset is not marked valid in the GHCB
+
+**/
+BOOLEAN
+EFIAPI
+VmgIsOffsetValid (
+  IN GHCB                    *Ghcb,
+  IN GHCB_QWORD_OFFSET       Offset
+  )
+{
+  UINT32  OffsetIndex;
+  UINT32  OffsetBit;
+
+  OffsetIndex = Offset / 8;
+  OffsetBit   = Offset % 8;
+
+  return ((Ghcb->SaveArea.ValidBitmap[OffsetIndex] & (1 << OffsetBit)) != 0);
+}
diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
index c5484a3f478c..7d14341d592b 100644
--- a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
+++ b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
@@ -135,62 +135,6 @@ typedef struct {
 } SEV_ES_PER_CPU_DATA;
 
 
-/**
-  Checks the GHCB to determine if the specified register has been marked valid.
-
-  The ValidBitmap area represents the areas of the GHCB that have been marked
-  valid. Return an indication of whether the area of the GHCB that holds the
-  specified register has been marked valid.
-
-  @param[in] Ghcb    Pointer to the Guest-Hypervisor Communication Block
-  @param[in] Reg     Offset in the GHCB of the register to check
-
-  @retval TRUE       Register has been marked vald in the GHCB
-  @retval FALSE      Register has not been marked valid in the GHCB
-
-**/
-STATIC
-BOOLEAN
-GhcbIsRegValid (
-  IN GHCB                *Ghcb,
-  IN GHCB_QWORD_OFFSET   Reg
-  )
-{
-  UINT32  RegIndex;
-  UINT32  RegBit;
-
-  RegIndex = Reg / 8;
-  RegBit   = Reg & 0x07;
-
-  return ((Ghcb->SaveArea.ValidBitmap[RegIndex] & (1 << RegBit)) != 0);
-}
-
-/**
-  Marks a register 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 that holds the specified register as valid.
-
-  @param[in, out] Ghcb    Pointer to the Guest-Hypervisor Communication Block
-  @param[in] Reg          Offset in the GHCB of the register to mark valid
-
-**/
-STATIC
-VOID
-GhcbSetRegValid (
-  IN OUT GHCB                *Ghcb,
-  IN     GHCB_QWORD_OFFSET   Reg
-  )
-{
-  UINT32  RegIndex;
-  UINT32  RegBit;
-
-  RegIndex = Reg / 8;
-  RegBit   = Reg & 0x07;
-
-  Ghcb->SaveArea.ValidBitmap[RegIndex] |= (1 << RegBit);
-}
-
 /**
   Return a pointer to the contents of the specified register.
 
@@ -891,9 +835,9 @@ MwaitExit (
   DecodeModRm (Regs, InstructionData);
 
   Ghcb->SaveArea.Rax = Regs->Rax;
-  GhcbSetRegValid (Ghcb, GhcbRax);
+  VmgSetOffsetValid (Ghcb, GhcbRax);
   Ghcb->SaveArea.Rcx = Regs->Rcx;
-  GhcbSetRegValid (Ghcb, GhcbRcx);
+  VmgSetOffsetValid (Ghcb, GhcbRcx);
 
   return VmgExit (Ghcb, SVM_EXIT_MWAIT, 0, 0);
 }
@@ -923,11 +867,11 @@ MonitorExit (
   DecodeModRm (Regs, InstructionData);
 
   Ghcb->SaveArea.Rax = Regs->Rax;  // Identity mapped, so VA = PA
-  GhcbSetRegValid (Ghcb, GhcbRax);
+  VmgSetOffsetValid (Ghcb, GhcbRax);
   Ghcb->SaveArea.Rcx = Regs->Rcx;
-  GhcbSetRegValid (Ghcb, GhcbRcx);
+  VmgSetOffsetValid (Ghcb, GhcbRcx);
   Ghcb->SaveArea.Rdx = Regs->Rdx;
-  GhcbSetRegValid (Ghcb, GhcbRdx);
+  VmgSetOffsetValid (Ghcb, GhcbRdx);
 
   return VmgExit (Ghcb, SVM_EXIT_MONITOR, 0, 0);
 }
@@ -988,9 +932,9 @@ RdtscpExit (
     return Status;
   }
 
-  if (!GhcbIsRegValid (Ghcb, GhcbRax) ||
-      !GhcbIsRegValid (Ghcb, GhcbRcx) ||
-      !GhcbIsRegValid (Ghcb, GhcbRdx)) {
+  if (!VmgIsOffsetValid (Ghcb, GhcbRax) ||
+      !VmgIsOffsetValid (Ghcb, GhcbRcx) ||
+      !VmgIsOffsetValid (Ghcb, GhcbRdx)) {
     return UnsupportedExit (Ghcb, Regs, InstructionData);
   }
   Regs->Rax = Ghcb->SaveArea.Rax;
@@ -1027,16 +971,16 @@ VmmCallExit (
   DecodeModRm (Regs, InstructionData);
 
   Ghcb->SaveArea.Rax = Regs->Rax;
-  GhcbSetRegValid (Ghcb, GhcbRax);
+  VmgSetOffsetValid (Ghcb, GhcbRax);
   Ghcb->SaveArea.Cpl = (UINT8) (Regs->Cs & 0x3);
-  GhcbSetRegValid (Ghcb, GhcbCpl);
+  VmgSetOffsetValid (Ghcb, GhcbCpl);
 
   Status = VmgExit (Ghcb, SVM_EXIT_VMMCALL, 0, 0);
   if (Status != 0) {
     return Status;
   }
 
-  if (!GhcbIsRegValid (Ghcb, GhcbRax)) {
+  if (!VmgIsOffsetValid (Ghcb, GhcbRax)) {
     return UnsupportedExit (Ghcb, Regs, InstructionData);
   }
   Regs->Rax = Ghcb->SaveArea.Rax;
@@ -1074,15 +1018,15 @@ MsrExit (
   case 0x30: // WRMSR
     ExitInfo1 = 1;
     Ghcb->SaveArea.Rax = Regs->Rax;
-    GhcbSetRegValid (Ghcb, GhcbRax);
+    VmgSetOffsetValid (Ghcb, GhcbRax);
     Ghcb->SaveArea.Rdx = Regs->Rdx;
-    GhcbSetRegValid (Ghcb, GhcbRdx);
+    VmgSetOffsetValid (Ghcb, GhcbRdx);
     //
     // fall through
     //
   case 0x32: // RDMSR
     Ghcb->SaveArea.Rcx = Regs->Rcx;
-    GhcbSetRegValid (Ghcb, GhcbRcx);
+    VmgSetOffsetValid (Ghcb, GhcbRcx);
     break;
   default:
     return UnsupportedExit (Ghcb, Regs, InstructionData);
@@ -1094,8 +1038,8 @@ MsrExit (
   }
 
   if (ExitInfo1 == 0) {
-    if (!GhcbIsRegValid (Ghcb, GhcbRax) ||
-        !GhcbIsRegValid (Ghcb, GhcbRdx)) {
+    if (!VmgIsOffsetValid (Ghcb, GhcbRax) ||
+        !VmgIsOffsetValid (Ghcb, GhcbRdx)) {
       return UnsupportedExit (Ghcb, Regs, InstructionData);
     }
     Regs->Rax = Ghcb->SaveArea.Rax;
@@ -1311,7 +1255,7 @@ IoioExit (
     } else {
       CopyMem (&Ghcb->SaveArea.Rax, &Regs->Rax, IOIO_DATA_BYTES (ExitInfo1));
     }
-    GhcbSetRegValid (Ghcb, GhcbRax);
+    VmgSetOffsetValid (Ghcb, GhcbRax);
 
     Status = VmgExit (Ghcb, SVM_EXIT_IOIO_PROT, ExitInfo1, 0);
     if (Status != 0) {
@@ -1319,7 +1263,7 @@ IoioExit (
     }
 
     if ((ExitInfo1 & IOIO_TYPE_IN) != 0) {
-      if (!GhcbIsRegValid (Ghcb, GhcbRax)) {
+      if (!VmgIsOffsetValid (Ghcb, GhcbRax)) {
         return UnsupportedExit (Ghcb, Regs, InstructionData);
       }
       CopyMem (&Regs->Rax, &Ghcb->SaveArea.Rax, IOIO_DATA_BYTES (ExitInfo1));
@@ -1379,15 +1323,15 @@ CpuidExit (
   UINT64  Status;
 
   Ghcb->SaveArea.Rax = Regs->Rax;
-  GhcbSetRegValid (Ghcb, GhcbRax);
+  VmgSetOffsetValid (Ghcb, GhcbRax);
   Ghcb->SaveArea.Rcx = Regs->Rcx;
-  GhcbSetRegValid (Ghcb, GhcbRcx);
+  VmgSetOffsetValid (Ghcb, GhcbRcx);
   if (Regs->Rax == CPUID_EXTENDED_STATE) {
     IA32_CR4  Cr4;
 
     Cr4.UintN = AsmReadCr4 ();
     Ghcb->SaveArea.XCr0 = (Cr4.Bits.OSXSAVE == 1) ? AsmXGetBv (0) : 1;
-    GhcbSetRegValid (Ghcb, GhcbXCr0);
+    VmgSetOffsetValid (Ghcb, GhcbXCr0);
   }
 
   Status = VmgExit (Ghcb, SVM_EXIT_CPUID, 0, 0);
@@ -1395,10 +1339,10 @@ CpuidExit (
     return Status;
   }
 
-  if (!GhcbIsRegValid (Ghcb, GhcbRax) ||
-      !GhcbIsRegValid (Ghcb, GhcbRbx) ||
-      !GhcbIsRegValid (Ghcb, GhcbRcx) ||
-      !GhcbIsRegValid (Ghcb, GhcbRdx)) {
+  if (!VmgIsOffsetValid (Ghcb, GhcbRax) ||
+      !VmgIsOffsetValid (Ghcb, GhcbRbx) ||
+      !VmgIsOffsetValid (Ghcb, GhcbRcx) ||
+      !VmgIsOffsetValid (Ghcb, GhcbRdx)) {
     return UnsupportedExit (Ghcb, Regs, InstructionData);
   }
   Regs->Rax = Ghcb->SaveArea.Rax;
@@ -1434,15 +1378,15 @@ RdpmcExit (
   UINT64  Status;
 
   Ghcb->SaveArea.Rcx = Regs->Rcx;
-  GhcbSetRegValid (Ghcb, GhcbRcx);
+  VmgSetOffsetValid (Ghcb, GhcbRcx);
 
   Status = VmgExit (Ghcb, SVM_EXIT_RDPMC, 0, 0);
   if (Status != 0) {
     return Status;
   }
 
-  if (!GhcbIsRegValid (Ghcb, GhcbRax) ||
-      !GhcbIsRegValid (Ghcb, GhcbRdx)) {
+  if (!VmgIsOffsetValid (Ghcb, GhcbRax) ||
+      !VmgIsOffsetValid (Ghcb, GhcbRdx)) {
     return UnsupportedExit (Ghcb, Regs, InstructionData);
   }
   Regs->Rax = Ghcb->SaveArea.Rax;
@@ -1480,8 +1424,8 @@ RdtscExit (
     return Status;
   }
 
-  if (!GhcbIsRegValid (Ghcb, GhcbRax) ||
-      !GhcbIsRegValid (Ghcb, GhcbRdx)) {
+  if (!VmgIsOffsetValid (Ghcb, GhcbRax) ||
+      !VmgIsOffsetValid (Ghcb, GhcbRdx)) {
     return UnsupportedExit (Ghcb, Regs, InstructionData);
   }
   Regs->Rax = Ghcb->SaveArea.Rax;
@@ -1531,7 +1475,7 @@ Dr7WriteExit (
   // Using a value of 0 for ExitInfo1 means RAX holds the value
   //
   Ghcb->SaveArea.Rax = *Register;
-  GhcbSetRegValid (Ghcb, GhcbRax);
+  VmgSetOffsetValid (Ghcb, GhcbRax);
 
   Status = VmgExit (Ghcb, SVM_EXIT_DR7_WRITE, 0, 0);
   if (Status != 0) {
-- 
2.28.0

Re: [edk2-devel] [PATCH v2 03/11] OvmfPkg/VmgExitLib: Implement new VmgExitLib interfaces

[Laszlo Ersek]

On 10/16/20 18:09, Lendacky, Thomas wrote:
> From: Tom Lendacky <thomas.lendacky@amd.com>
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3008
> 
> The VmgExitLib library added two new interfaces, VmgSetOffsetValid() and
> VmgIsOffsetValid(), that must now be implemented in the OvmfPkg version
> of the library.
> 
> Implement VmgSetOffsetValid() and VmgIsOffsetValid() and update existing
> code, that is directly accessing ValidBitmap, to use the new interfaces.
> 
> 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       |  54 +++++++++
>  OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c | 118 +++++---------------
>  2 files changed, 85 insertions(+), 87 deletions(-)
> 
> diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitLib.c b/OvmfPkg/Library/VmgExitLib/VmgExitLib.c
> index 53040cc6f649..3072c2265df7 100644
> --- a/OvmfPkg/Library/VmgExitLib/VmgExitLib.c
> +++ b/OvmfPkg/Library/VmgExitLib/VmgExitLib.c
> @@ -157,3 +157,57 @@ VmgDone (
>  {
>  }
>  
> +/**
> +  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 bit in ValidBitmap for the input offset.
> +
> +  @param[in, out] Ghcb    Pointer to the Guest-Hypervisor Communication Block
> +  @param[in]      Offset  Qword offset in the GHCB to mark valid
> +
> +**/
> +VOID
> +EFIAPI
> +VmgSetOffsetValid (
> +  IN OUT GHCB                *Ghcb,
> +  IN     GHCB_QWORD_OFFSET   Offset
> +  )
> +{
> +  UINT32  OffsetIndex;
> +  UINT32  OffsetBit;
> +
> +  OffsetIndex = Offset / 8;
> +  OffsetBit   = Offset % 8;
> +
> +  Ghcb->SaveArea.ValidBitmap[OffsetIndex] |= (1 << OffsetBit);
> +}
> +
> +/**
> +  Checks if a specified offset is valid in the GHCB.
> +
> +  The ValidBitmap area represents the areas of the GHCB that have been marked
> +  valid. Return whether the bit in the ValidBitmap is set for the input offset.
> +
> +  @param[in]  Ghcb            A pointer to the GHCB
> +  @param[in]  Offset          Qword offset in the GHCB to mark valid
> +
> +  @retval TRUE                Offset is marked vald in the GHCB

s/vald/valid/

with that:

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

Thanks
laszlo

> +  @retval FALSE               Offset is not marked valid in the GHCB
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +VmgIsOffsetValid (
> +  IN GHCB                    *Ghcb,
> +  IN GHCB_QWORD_OFFSET       Offset
> +  )
> +{
> +  UINT32  OffsetIndex;
> +  UINT32  OffsetBit;
> +
> +  OffsetIndex = Offset / 8;
> +  OffsetBit   = Offset % 8;
> +
> +  return ((Ghcb->SaveArea.ValidBitmap[OffsetIndex] & (1 << OffsetBit)) != 0);
> +}
> diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
> index c5484a3f478c..7d14341d592b 100644
> --- a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
> +++ b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
> @@ -135,62 +135,6 @@ typedef struct {
>  } SEV_ES_PER_CPU_DATA;
>  
>  
> -/**
> -  Checks the GHCB to determine if the specified register has been marked valid.
> -
> -  The ValidBitmap area represents the areas of the GHCB that have been marked
> -  valid. Return an indication of whether the area of the GHCB that holds the
> -  specified register has been marked valid.
> -
> -  @param[in] Ghcb    Pointer to the Guest-Hypervisor Communication Block
> -  @param[in] Reg     Offset in the GHCB of the register to check
> -
> -  @retval TRUE       Register has been marked vald in the GHCB
> -  @retval FALSE      Register has not been marked valid in the GHCB
> -
> -**/
> -STATIC
> -BOOLEAN
> -GhcbIsRegValid (
> -  IN GHCB                *Ghcb,
> -  IN GHCB_QWORD_OFFSET   Reg
> -  )
> -{
> -  UINT32  RegIndex;
> -  UINT32  RegBit;
> -
> -  RegIndex = Reg / 8;
> -  RegBit   = Reg & 0x07;
> -
> -  return ((Ghcb->SaveArea.ValidBitmap[RegIndex] & (1 << RegBit)) != 0);
> -}
> -
> -/**
> -  Marks a register 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 that holds the specified register as valid.
> -
> -  @param[in, out] Ghcb    Pointer to the Guest-Hypervisor Communication Block
> -  @param[in] Reg          Offset in the GHCB of the register to mark valid
> -
> -**/
> -STATIC
> -VOID
> -GhcbSetRegValid (
> -  IN OUT GHCB                *Ghcb,
> -  IN     GHCB_QWORD_OFFSET   Reg
> -  )
> -{
> -  UINT32  RegIndex;
> -  UINT32  RegBit;
> -
> -  RegIndex = Reg / 8;
> -  RegBit   = Reg & 0x07;
> -
> -  Ghcb->SaveArea.ValidBitmap[RegIndex] |= (1 << RegBit);
> -}
> -
>  /**
>    Return a pointer to the contents of the specified register.
>  
> @@ -891,9 +835,9 @@ MwaitExit (
>    DecodeModRm (Regs, InstructionData);
>  
>    Ghcb->SaveArea.Rax = Regs->Rax;
> -  GhcbSetRegValid (Ghcb, GhcbRax);
> +  VmgSetOffsetValid (Ghcb, GhcbRax);
>    Ghcb->SaveArea.Rcx = Regs->Rcx;
> -  GhcbSetRegValid (Ghcb, GhcbRcx);
> +  VmgSetOffsetValid (Ghcb, GhcbRcx);
>  
>    return VmgExit (Ghcb, SVM_EXIT_MWAIT, 0, 0);
>  }
> @@ -923,11 +867,11 @@ MonitorExit (
>    DecodeModRm (Regs, InstructionData);
>  
>    Ghcb->SaveArea.Rax = Regs->Rax;  // Identity mapped, so VA = PA
> -  GhcbSetRegValid (Ghcb, GhcbRax);
> +  VmgSetOffsetValid (Ghcb, GhcbRax);
>    Ghcb->SaveArea.Rcx = Regs->Rcx;
> -  GhcbSetRegValid (Ghcb, GhcbRcx);
> +  VmgSetOffsetValid (Ghcb, GhcbRcx);
>    Ghcb->SaveArea.Rdx = Regs->Rdx;
> -  GhcbSetRegValid (Ghcb, GhcbRdx);
> +  VmgSetOffsetValid (Ghcb, GhcbRdx);
>  
>    return VmgExit (Ghcb, SVM_EXIT_MONITOR, 0, 0);
>  }
> @@ -988,9 +932,9 @@ RdtscpExit (
>      return Status;
>    }
>  
> -  if (!GhcbIsRegValid (Ghcb, GhcbRax) ||
> -      !GhcbIsRegValid (Ghcb, GhcbRcx) ||
> -      !GhcbIsRegValid (Ghcb, GhcbRdx)) {
> +  if (!VmgIsOffsetValid (Ghcb, GhcbRax) ||
> +      !VmgIsOffsetValid (Ghcb, GhcbRcx) ||
> +      !VmgIsOffsetValid (Ghcb, GhcbRdx)) {
>      return UnsupportedExit (Ghcb, Regs, InstructionData);
>    }
>    Regs->Rax = Ghcb->SaveArea.Rax;
> @@ -1027,16 +971,16 @@ VmmCallExit (
>    DecodeModRm (Regs, InstructionData);
>  
>    Ghcb->SaveArea.Rax = Regs->Rax;
> -  GhcbSetRegValid (Ghcb, GhcbRax);
> +  VmgSetOffsetValid (Ghcb, GhcbRax);
>    Ghcb->SaveArea.Cpl = (UINT8) (Regs->Cs & 0x3);
> -  GhcbSetRegValid (Ghcb, GhcbCpl);
> +  VmgSetOffsetValid (Ghcb, GhcbCpl);
>  
>    Status = VmgExit (Ghcb, SVM_EXIT_VMMCALL, 0, 0);
>    if (Status != 0) {
>      return Status;
>    }
>  
> -  if (!GhcbIsRegValid (Ghcb, GhcbRax)) {
> +  if (!VmgIsOffsetValid (Ghcb, GhcbRax)) {
>      return UnsupportedExit (Ghcb, Regs, InstructionData);
>    }
>    Regs->Rax = Ghcb->SaveArea.Rax;
> @@ -1074,15 +1018,15 @@ MsrExit (
>    case 0x30: // WRMSR
>      ExitInfo1 = 1;
>      Ghcb->SaveArea.Rax = Regs->Rax;
> -    GhcbSetRegValid (Ghcb, GhcbRax);
> +    VmgSetOffsetValid (Ghcb, GhcbRax);
>      Ghcb->SaveArea.Rdx = Regs->Rdx;
> -    GhcbSetRegValid (Ghcb, GhcbRdx);
> +    VmgSetOffsetValid (Ghcb, GhcbRdx);
>      //
>      // fall through
>      //
>    case 0x32: // RDMSR
>      Ghcb->SaveArea.Rcx = Regs->Rcx;
> -    GhcbSetRegValid (Ghcb, GhcbRcx);
> +    VmgSetOffsetValid (Ghcb, GhcbRcx);
>      break;
>    default:
>      return UnsupportedExit (Ghcb, Regs, InstructionData);
> @@ -1094,8 +1038,8 @@ MsrExit (
>    }
>  
>    if (ExitInfo1 == 0) {
> -    if (!GhcbIsRegValid (Ghcb, GhcbRax) ||
> -        !GhcbIsRegValid (Ghcb, GhcbRdx)) {
> +    if (!VmgIsOffsetValid (Ghcb, GhcbRax) ||
> +        !VmgIsOffsetValid (Ghcb, GhcbRdx)) {
>        return UnsupportedExit (Ghcb, Regs, InstructionData);
>      }
>      Regs->Rax = Ghcb->SaveArea.Rax;
> @@ -1311,7 +1255,7 @@ IoioExit (
>      } else {
>        CopyMem (&Ghcb->SaveArea.Rax, &Regs->Rax, IOIO_DATA_BYTES (ExitInfo1));
>      }
> -    GhcbSetRegValid (Ghcb, GhcbRax);
> +    VmgSetOffsetValid (Ghcb, GhcbRax);
>  
>      Status = VmgExit (Ghcb, SVM_EXIT_IOIO_PROT, ExitInfo1, 0);
>      if (Status != 0) {
> @@ -1319,7 +1263,7 @@ IoioExit (
>      }
>  
>      if ((ExitInfo1 & IOIO_TYPE_IN) != 0) {
> -      if (!GhcbIsRegValid (Ghcb, GhcbRax)) {
> +      if (!VmgIsOffsetValid (Ghcb, GhcbRax)) {
>          return UnsupportedExit (Ghcb, Regs, InstructionData);
>        }
>        CopyMem (&Regs->Rax, &Ghcb->SaveArea.Rax, IOIO_DATA_BYTES (ExitInfo1));
> @@ -1379,15 +1323,15 @@ CpuidExit (
>    UINT64  Status;
>  
>    Ghcb->SaveArea.Rax = Regs->Rax;
> -  GhcbSetRegValid (Ghcb, GhcbRax);
> +  VmgSetOffsetValid (Ghcb, GhcbRax);
>    Ghcb->SaveArea.Rcx = Regs->Rcx;
> -  GhcbSetRegValid (Ghcb, GhcbRcx);
> +  VmgSetOffsetValid (Ghcb, GhcbRcx);
>    if (Regs->Rax == CPUID_EXTENDED_STATE) {
>      IA32_CR4  Cr4;
>  
>      Cr4.UintN = AsmReadCr4 ();
>      Ghcb->SaveArea.XCr0 = (Cr4.Bits.OSXSAVE == 1) ? AsmXGetBv (0) : 1;
> -    GhcbSetRegValid (Ghcb, GhcbXCr0);
> +    VmgSetOffsetValid (Ghcb, GhcbXCr0);
>    }
>  
>    Status = VmgExit (Ghcb, SVM_EXIT_CPUID, 0, 0);
> @@ -1395,10 +1339,10 @@ CpuidExit (
>      return Status;
>    }
>  
> -  if (!GhcbIsRegValid (Ghcb, GhcbRax) ||
> -      !GhcbIsRegValid (Ghcb, GhcbRbx) ||
> -      !GhcbIsRegValid (Ghcb, GhcbRcx) ||
> -      !GhcbIsRegValid (Ghcb, GhcbRdx)) {
> +  if (!VmgIsOffsetValid (Ghcb, GhcbRax) ||
> +      !VmgIsOffsetValid (Ghcb, GhcbRbx) ||
> +      !VmgIsOffsetValid (Ghcb, GhcbRcx) ||
> +      !VmgIsOffsetValid (Ghcb, GhcbRdx)) {
>      return UnsupportedExit (Ghcb, Regs, InstructionData);
>    }
>    Regs->Rax = Ghcb->SaveArea.Rax;
> @@ -1434,15 +1378,15 @@ RdpmcExit (
>    UINT64  Status;
>  
>    Ghcb->SaveArea.Rcx = Regs->Rcx;
> -  GhcbSetRegValid (Ghcb, GhcbRcx);
> +  VmgSetOffsetValid (Ghcb, GhcbRcx);
>  
>    Status = VmgExit (Ghcb, SVM_EXIT_RDPMC, 0, 0);
>    if (Status != 0) {
>      return Status;
>    }
>  
> -  if (!GhcbIsRegValid (Ghcb, GhcbRax) ||
> -      !GhcbIsRegValid (Ghcb, GhcbRdx)) {
> +  if (!VmgIsOffsetValid (Ghcb, GhcbRax) ||
> +      !VmgIsOffsetValid (Ghcb, GhcbRdx)) {
>      return UnsupportedExit (Ghcb, Regs, InstructionData);
>    }
>    Regs->Rax = Ghcb->SaveArea.Rax;
> @@ -1480,8 +1424,8 @@ RdtscExit (
>      return Status;
>    }
>  
> -  if (!GhcbIsRegValid (Ghcb, GhcbRax) ||
> -      !GhcbIsRegValid (Ghcb, GhcbRdx)) {
> +  if (!VmgIsOffsetValid (Ghcb, GhcbRax) ||
> +      !VmgIsOffsetValid (Ghcb, GhcbRdx)) {
>      return UnsupportedExit (Ghcb, Regs, InstructionData);
>    }
>    Regs->Rax = Ghcb->SaveArea.Rax;
> @@ -1531,7 +1475,7 @@ Dr7WriteExit (
>    // Using a value of 0 for ExitInfo1 means RAX holds the value
>    //
>    Ghcb->SaveArea.Rax = *Register;
> -  GhcbSetRegValid (Ghcb, GhcbRax);
> +  VmgSetOffsetValid (Ghcb, GhcbRax);
>  
>    Status = VmgExit (Ghcb, SVM_EXIT_DR7_WRITE, 0, 0);
>    if (Status != 0) {
> 

[PATCH v2 04/11] OvmfPkg/VmgExitLib: Set the SW exit fields when performing VMGEXIT

[Lendacky, Thomas]

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

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3008

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: 61bacc0fa16fd6f595a2c4222425cb6286e19977
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 | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitLib.c b/OvmfPkg/Library/VmgExitLib/VmgExitLib.c
index 3072c2265df7..ae86d850ba61 100644
--- a/OvmfPkg/Library/VmgExitLib/VmgExitLib.c
+++ b/OvmfPkg/Library/VmgExitLib/VmgExitLib.c
@@ -110,6 +110,10 @@ VmgExit (
   Ghcb->SaveArea.SwExitInfo1 = ExitInfo1;
   Ghcb->SaveArea.SwExitInfo2 = ExitInfo2;
 
+  VmgSetOffsetValid (Ghcb, GhcbSwExitCode);
+  VmgSetOffsetValid (Ghcb, GhcbSwExitInfo1);
+  VmgSetOffsetValid (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

Re: [edk2-devel] [PATCH v2 04/11] OvmfPkg/VmgExitLib: Set the SW exit fields when performing VMGEXIT

[Laszlo Ersek]

On 10/16/20 18:09, Lendacky, Thomas wrote:
> From: Tom Lendacky <thomas.lendacky@amd.com>
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3008
> 
> 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: 61bacc0fa16fd6f595a2c4222425cb6286e19977
> 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 | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitLib.c b/OvmfPkg/Library/VmgExitLib/VmgExitLib.c
> index 3072c2265df7..ae86d850ba61 100644
> --- a/OvmfPkg/Library/VmgExitLib/VmgExitLib.c
> +++ b/OvmfPkg/Library/VmgExitLib/VmgExitLib.c
> @@ -110,6 +110,10 @@ VmgExit (
>    Ghcb->SaveArea.SwExitInfo1 = ExitInfo1;
>    Ghcb->SaveArea.SwExitInfo2 = ExitInfo2;
>  
> +  VmgSetOffsetValid (Ghcb, GhcbSwExitCode);
> +  VmgSetOffsetValid (Ghcb, GhcbSwExitInfo1);
> +  VmgSetOffsetValid (Ghcb, GhcbSwExitInfo2);
> +
>    //
>    // Guest memory is used for the guest-hypervisor communication, so fence
>    // the invocation of the VMGEXIT instruction to ensure GHCB accesses are
> 

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

[PATCH v2 05/11] OvmfPkg/VmgExitLib: Set the SwScratch valid bit for IOIO events

[Lendacky, Thomas]

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

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3008

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: 0020157a9825e5f5784ff014044f11c0558c92fe
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>
Acked-by: Laszlo Ersek <lersek@redhat.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 7d14341d592b..e5f14035b06f 100644
--- a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
+++ b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
@@ -1233,6 +1233,7 @@ IoioExit (
       }
 
       Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
+      VmgSetOffsetValid (Ghcb, GhcbSwScratch);
       Status = VmgExit (Ghcb, SVM_EXIT_IOIO_PROT, ExitInfo1, ExitInfo2);
       if (Status != 0) {
         return Status;
-- 
2.28.0

Re: [edk2-devel] [PATCH v2 05/11] OvmfPkg/VmgExitLib: Set the SwScratch valid bit for IOIO events

[Laszlo Ersek]

On 10/16/20 18:09, Lendacky, Thomas wrote:
> From: Tom Lendacky <thomas.lendacky@amd.com>
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3008
> 
> 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: 0020157a9825e5f5784ff014044f11c0558c92fe
> 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>
> Acked-by: Laszlo Ersek <lersek@redhat.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 7d14341d592b..e5f14035b06f 100644
> --- a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
> +++ b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
> @@ -1233,6 +1233,7 @@ IoioExit (
>        }
>  
>        Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
> +      VmgSetOffsetValid (Ghcb, GhcbSwScratch);
>        Status = VmgExit (Ghcb, SVM_EXIT_IOIO_PROT, ExitInfo1, ExitInfo2);
>        if (Status != 0) {
>          return Status;
> 

let's make that a

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

[PATCH v2 06/11] OvmfPkg/VmgExitLib: Set the SwScratch valid bit for MMIO events

[Lendacky, Thomas]

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

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3008

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: c45f678a1ea2080344e125dc55b14e4b9f98483d
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>
Reviewed-by: Laszlo Ersek <lersek@redhat.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 e5f14035b06f..9bf9d160179c 100644
--- a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
+++ b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
@@ -664,6 +664,7 @@ MmioExit (
     CopyMem (Ghcb->SharedBuffer, &InstructionData->Ext.RegData, Bytes);
 
     Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
+    VmgSetOffsetValid (Ghcb, GhcbSwScratch);
     Status = VmgExit (Ghcb, SVM_EXIT_MMIO_WRITE, ExitInfo1, ExitInfo2);
     if (Status != 0) {
       return Status;
@@ -693,6 +694,7 @@ MmioExit (
     CopyMem (Ghcb->SharedBuffer, InstructionData->Immediate, Bytes);
 
     Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
+    VmgSetOffsetValid (Ghcb, GhcbSwScratch);
     Status = VmgExit (Ghcb, SVM_EXIT_MMIO_WRITE, ExitInfo1, ExitInfo2);
     if (Status != 0) {
       return Status;
@@ -725,6 +727,7 @@ MmioExit (
     ExitInfo2 = Bytes;
 
     Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
+    VmgSetOffsetValid (Ghcb, GhcbSwScratch);
     Status = VmgExit (Ghcb, SVM_EXIT_MMIO_READ, ExitInfo1, ExitInfo2);
     if (Status != 0) {
       return Status;
@@ -755,6 +758,7 @@ MmioExit (
     ExitInfo2 = Bytes;
 
     Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
+    VmgSetOffsetValid (Ghcb, GhcbSwScratch);
     Status = VmgExit (Ghcb, SVM_EXIT_MMIO_READ, ExitInfo1, ExitInfo2);
     if (Status != 0) {
       return Status;
@@ -780,6 +784,7 @@ MmioExit (
     ExitInfo2 = Bytes;
 
     Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
+    VmgSetOffsetValid (Ghcb, GhcbSwScratch);
     Status = VmgExit (Ghcb, SVM_EXIT_MMIO_READ, ExitInfo1, ExitInfo2);
     if (Status != 0) {
       return Status;
-- 
2.28.0

[PATCH v2 07/11] UefiCpuPkg/MpInitLib: Set the SW exit fields when performing VMGEXIT

[Lendacky, Thomas]

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

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3008

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: 20da7ca42a33d3ef767ce4129f11496af7f67c9f
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>
Acked-by: Ray Ni <ray.ni@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 v2 08/11] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Set the SwScratch valid bit

[Lendacky, Thomas]

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

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3008

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: 437eb3f7a8db7681afe0e6064d3a8edb12abb766
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 | 1 +
 1 file changed, 1 insertion(+)

diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
index 565383ee26d2..f9b21b54137d 100644
--- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
+++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
@@ -66,6 +66,7 @@ QemuFlashPtrWrite (
     VmgInit (Ghcb);
     Ghcb->SharedBuffer[0] = Value;
     Ghcb->SaveArea.SwScratch = (UINT64) (UINTN) Ghcb->SharedBuffer;
+    VmgSetOffsetValid (Ghcb, GhcbSwScratch);
     VmgExit (Ghcb, SVM_EXIT_MMIO_WRITE, (UINT64) (UINTN) Ptr, 1);
     VmgDone (Ghcb);
   } else {
-- 
2.28.0

Re: [edk2-devel] [PATCH v2 08/11] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Set the SwScratch valid bit

[Laszlo Ersek]

On 10/16/20 18:09, Lendacky, Thomas wrote:
> From: Tom Lendacky <thomas.lendacky@amd.com>
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3008
> 
> 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: 437eb3f7a8db7681afe0e6064d3a8edb12abb766
> 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 | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
> index 565383ee26d2..f9b21b54137d 100644
> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
> @@ -66,6 +66,7 @@ QemuFlashPtrWrite (
>      VmgInit (Ghcb);
>      Ghcb->SharedBuffer[0] = Value;
>      Ghcb->SaveArea.SwScratch = (UINT64) (UINTN) Ghcb->SharedBuffer;
> +    VmgSetOffsetValid (Ghcb, GhcbSwScratch);
>      VmgExit (Ghcb, SVM_EXIT_MMIO_WRITE, (UINT64) (UINTN) Ptr, 1);
>      VmgDone (Ghcb);
>    } else {
> 

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

[PATCH v2 09/11] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Fix erase blocks for SEV-ES

[Lendacky, Thomas]

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

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3008

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: 437eb3f7a8db7681afe0e6064d3a8edb12abb766
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.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 v2 10/11] UefiCpuPkg, OvmfPkg: Disable interrupts when using the GHCB

[Lendacky, Thomas]

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

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3008

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 VmgExit(), the Dr7 read in the
interrupt handler will generate a #VC, which can overwrite information in
the GHCB that QemuFlashPtrWrite() has set. This has been seen with the
timer interrupt firing and the CpuExceptionHandlerLib library code,
UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/
  Xcode5ExceptionHandlerAsm.nasm and
  ExceptionHandlerAsm.nasm
reading the Dr7 register while QemuFlashPtrWrite() is using the GHCB. In
general, it is necessary to protect the GHCB whenever it is used, not just
in QemuFlashPtrWrite().

Disable interrupts around the usage of the GHCB by modifying the VmgInit()
and VmgDone() interfaces:
- VmgInit() will take an extra parameter that is a pointer to a BOOLEAN
  that will hold the interrupt state at the time of invocation. VmgInit()
  will get and save this interrupt state before updating the GHCB.
- VmgDone() will take an extra parameter that is used to indicate whether
  interrupts are to be (re)enabled. Before exiting, VmgDone() will enable
  interrupts if that is requested.

Fixes: 437eb3f7a8db7681afe0e6064d3a8edb12abb766
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>
Cc: Jordan Justen <jordan.l.justen@intel.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>
---
 UefiCpuPkg/Include/Library/VmgExitLib.h               | 14 ++++++++---
 OvmfPkg/Library/VmgExitLib/VmgExitLib.c               | 26 +++++++++++++++++---
 OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c         |  5 ++--
 OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c |  5 ++--
 UefiCpuPkg/Library/MpInitLib/DxeMpLib.c               |  5 ++--
 UefiCpuPkg/Library/MpInitLib/MpLib.c                  |  7 +++---
 UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.c    | 18 ++++++++------
 7 files changed, 55 insertions(+), 25 deletions(-)

diff --git a/UefiCpuPkg/Include/Library/VmgExitLib.h b/UefiCpuPkg/Include/Library/VmgExitLib.h
index ba5ea024839e..617b6cf8d2e7 100644
--- a/UefiCpuPkg/Include/Library/VmgExitLib.h
+++ b/UefiCpuPkg/Include/Library/VmgExitLib.h
@@ -50,13 +50,16 @@ VmgExit (
   Performs the necessary steps in preparation for invoking VMGEXIT. Must be
   called before setting any fields within the GHCB.
 
-  @param[in, out]  Ghcb       A pointer to the GHCB
+  @param[in, out]  Ghcb            A pointer to the GHCB
+  @param[in, out]  InterruptState  A pointer to hold the current interrupt
+                                   state, used for restoring in VmgDone ()
 
 **/
 VOID
 EFIAPI
 VmgInit (
-  IN OUT GHCB                *Ghcb
+  IN OUT GHCB                *Ghcb,
+  IN OUT BOOLEAN             *InterruptState
   );
 
 /**
@@ -65,13 +68,16 @@ VmgInit (
   Performs the necessary steps to cleanup after invoking VMGEXIT. Must be
   called after obtaining needed fields within the GHCB.
 
-  @param[in, out]  Ghcb       A pointer to the GHCB
+  @param[in, out]  Ghcb            A pointer to the GHCB
+  @param[in]       InterruptState  An indicator to conditionally (re)enable
+                                   interrupts
 
 **/
 VOID
 EFIAPI
 VmgDone (
-  IN OUT GHCB                *Ghcb
+  IN OUT GHCB                *Ghcb,
+  IN     BOOLEAN             InterruptState
   );
 
 /**
diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitLib.c b/OvmfPkg/Library/VmgExitLib/VmgExitLib.c
index ae86d850ba61..18b102df5f9a 100644
--- a/OvmfPkg/Library/VmgExitLib/VmgExitLib.c
+++ b/OvmfPkg/Library/VmgExitLib/VmgExitLib.c
@@ -132,15 +132,27 @@ VmgExit (
   Performs the necessary steps in preparation for invoking VMGEXIT. Must be
   called before setting any fields within the GHCB.
 
-  @param[in, out]  Ghcb       A pointer to the GHCB
+  @param[in, out]  Ghcb            A pointer to the GHCB
+  @param[in, out]  InterruptState  A pointer to hold the current interrupt
+                                   state, used for restoring in VmgDone ()
 
 **/
 VOID
 EFIAPI
 VmgInit (
-  IN OUT GHCB                *Ghcb
+  IN OUT GHCB                *Ghcb,
+  IN OUT BOOLEAN             *InterruptState
   )
 {
+  //
+  // Be sure that an interrupt can't cause a #VC while the GHCB is
+  // being used.
+  //
+  *InterruptState = GetInterruptState ();
+  if (*InterruptState) {
+    DisableInterrupts ();
+  }
+
   SetMem (&Ghcb->SaveArea, sizeof (Ghcb->SaveArea), 0);
 }
 
@@ -150,15 +162,21 @@ VmgInit (
   Performs the necessary steps to cleanup after invoking VMGEXIT. Must be
   called after obtaining needed fields within the GHCB.
 
-  @param[in, out]  Ghcb       A pointer to the GHCB
+  @param[in, out]  Ghcb            A pointer to the GHCB
+  @param[in]       InterruptState  An indicator to conditionally (re)enable
+                                   interrupts
 
 **/
 VOID
 EFIAPI
 VmgDone (
-  IN OUT GHCB                *Ghcb
+  IN OUT GHCB                *Ghcb,
+  IN     BOOLEAN             InterruptState
   )
 {
+  if (InterruptState) {
+    EnableInterrupts ();
+  }
 }
 
 /**
diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
index 9bf9d160179c..1671db3a01b1 100644
--- a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
+++ b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
@@ -1568,6 +1568,7 @@ VmgExitHandleVc (
   SEV_ES_INSTRUCTION_DATA   InstructionData;
   UINT64                    ExitCode, Status;
   EFI_STATUS                VcRet;
+  BOOLEAN                   InterruptState;
 
   VcRet = EFI_SUCCESS;
 
@@ -1578,7 +1579,7 @@ VmgExitHandleVc (
   Regs = SystemContext.SystemContextX64;
   Ghcb = Msr.Ghcb;
 
-  VmgInit (Ghcb);
+  VmgInit (Ghcb, &InterruptState);
 
   ExitCode = Regs->ExceptionData;
   switch (ExitCode) {
@@ -1662,7 +1663,7 @@ VmgExitHandleVc (
     VcRet = EFI_PROTOCOL_ERROR;
   }
 
-  VmgDone (Ghcb);
+  VmgDone (Ghcb, InterruptState);
 
   return VcRet;
 }
diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
index f9b21b54137d..1b0742967f71 100644
--- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
+++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
@@ -52,6 +52,7 @@ QemuFlashPtrWrite (
   if (MemEncryptSevEsIsEnabled ()) {
     MSR_SEV_ES_GHCB_REGISTER  Msr;
     GHCB                      *Ghcb;
+    BOOLEAN                   InterruptState;
 
     Msr.GhcbPhysicalAddress = AsmReadMsr64 (MSR_SEV_ES_GHCB);
     Ghcb = Msr.Ghcb;
@@ -63,12 +64,12 @@ QemuFlashPtrWrite (
     // #VC exception. Instead, use the the VMGEXIT MMIO write support directly
     // to perform the update.
     //
-    VmgInit (Ghcb);
+    VmgInit (Ghcb, &InterruptState);
     Ghcb->SharedBuffer[0] = Value;
     Ghcb->SaveArea.SwScratch = (UINT64) (UINTN) Ghcb->SharedBuffer;
     VmgSetOffsetValid (Ghcb, GhcbSwScratch);
     VmgExit (Ghcb, SVM_EXIT_MMIO_WRITE, (UINT64) (UINTN) Ptr, 1);
-    VmgDone (Ghcb);
+    VmgDone (Ghcb, InterruptState);
   } else {
     *Ptr = Value;
   }
diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
index 2c00d72ddefe..7839c249760e 100644
--- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
@@ -171,6 +171,7 @@ GetSevEsAPMemory (
   EFI_PHYSICAL_ADDRESS      StartAddress;
   MSR_SEV_ES_GHCB_REGISTER  Msr;
   GHCB                      *Ghcb;
+  BOOLEAN                   InterruptState;
 
   //
   // Allocate 1 page for AP jump table page
@@ -192,9 +193,9 @@ GetSevEsAPMemory (
   Msr.GhcbPhysicalAddress = AsmReadMsr64 (MSR_SEV_ES_GHCB);
   Ghcb = Msr.Ghcb;
 
-  VmgInit (Ghcb);
+  VmgInit (Ghcb, &InterruptState);
   VmgExit (Ghcb, SVM_EXIT_AP_JUMP_TABLE, 0, (UINT64) (UINTN) StartAddress);
-  VmgDone (Ghcb);
+  VmgDone (Ghcb, InterruptState);
 
   return (UINTN) StartAddress;
 }
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index 07426274f639..4f4b26a7c196 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -884,6 +884,7 @@ ApWakeupFunction (
           GHCB                      *Ghcb;
           UINT64                    Status;
           BOOLEAN                   DoDecrement;
+          BOOLEAN                   InterruptState;
 
           DoDecrement = (BOOLEAN) (CpuMpData->InitFlag == ApInitConfig);
 
@@ -891,7 +892,7 @@ ApWakeupFunction (
             Msr.GhcbPhysicalAddress = AsmReadMsr64 (MSR_SEV_ES_GHCB);
             Ghcb = Msr.Ghcb;
 
-            VmgInit (Ghcb);
+            VmgInit (Ghcb, &InterruptState);
 
             if (DoDecrement) {
               DoDecrement = FALSE;
@@ -905,11 +906,11 @@ ApWakeupFunction (
 
             Status = VmgExit (Ghcb, SVM_EXIT_AP_RESET_HOLD, 0, 0);
             if ((Status == 0) && (Ghcb->SaveArea.SwExitInfo2 != 0)) {
-              VmgDone (Ghcb);
+              VmgDone (Ghcb, InterruptState);
               break;
             }
 
-            VmgDone (Ghcb);
+            VmgDone (Ghcb, InterruptState);
           }
 
           //
diff --git a/UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.c b/UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.c
index b000232c472e..24defd624c63 100644
--- a/UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.c
+++ b/UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.c
@@ -57,15 +57,16 @@ VmgExit (
   Performs the necessary steps in preparation for invoking VMGEXIT. Must be
   called before setting any fields within the GHCB.
 
-  The base library function does nothing.
-
-  @param[in, out]  Ghcb       A pointer to the GHCB
+  @param[in, out]  Ghcb            A pointer to the GHCB
+  @param[in, out]  InterruptState  A pointer to hold the current interrupt
+                                   state, used for restoring in VmgDone ()
 
 **/
 VOID
 EFIAPI
 VmgInit (
-  IN OUT GHCB                *Ghcb
+  IN OUT GHCB                *Ghcb,
+  IN OUT BOOLEAN             *InterruptState
   )
 {
 }
@@ -76,15 +77,16 @@ VmgInit (
   Performs the necessary steps to cleanup after invoking VMGEXIT. Must be
   called after obtaining needed fields within the GHCB.
 
-  The base library function does nothing.
-
-  @param[in, out]  Ghcb       A pointer to the GHCB
+  @param[in, out]  Ghcb            A pointer to the GHCB
+  @param[in]       InterruptState  An indicator to conditionally (re)enable
+                                   interrupts
 
 **/
 VOID
 EFIAPI
 VmgDone (
-  IN OUT GHCB                *Ghcb
+  IN OUT GHCB                *Ghcb,
+  IN     BOOLEAN             InterruptState
   )
 {
 }
-- 
2.28.0

Re: [edk2-devel] [PATCH v2 10/11] UefiCpuPkg, OvmfPkg: Disable interrupts when using the GHCB

[Laszlo Ersek]

On 10/16/20 18:09, Lendacky, Thomas wrote:
> From: Tom Lendacky <thomas.lendacky@amd.com>
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3008
> 
> 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 VmgExit(), the Dr7 read in the
> interrupt handler will generate a #VC, which can overwrite information in
> the GHCB that QemuFlashPtrWrite() has set. This has been seen with the
> timer interrupt firing and the CpuExceptionHandlerLib library code,
> UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/
>   Xcode5ExceptionHandlerAsm.nasm and
>   ExceptionHandlerAsm.nasm
> reading the Dr7 register while QemuFlashPtrWrite() is using the GHCB. In
> general, it is necessary to protect the GHCB whenever it is used, not just
> in QemuFlashPtrWrite().
> 
> Disable interrupts around the usage of the GHCB by modifying the VmgInit()
> and VmgDone() interfaces:
> - VmgInit() will take an extra parameter that is a pointer to a BOOLEAN
>   that will hold the interrupt state at the time of invocation. VmgInit()
>   will get and save this interrupt state before updating the GHCB.
> - VmgDone() will take an extra parameter that is used to indicate whether
>   interrupts are to be (re)enabled. Before exiting, VmgDone() will enable
>   interrupts if that is requested.
> 
> Fixes: 437eb3f7a8db7681afe0e6064d3a8edb12abb766
> 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>
> Cc: Jordan Justen <jordan.l.justen@intel.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>
> ---
>  UefiCpuPkg/Include/Library/VmgExitLib.h               | 14 ++++++++---
>  OvmfPkg/Library/VmgExitLib/VmgExitLib.c               | 26 +++++++++++++++++---
>  OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c         |  5 ++--
>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c |  5 ++--
>  UefiCpuPkg/Library/MpInitLib/DxeMpLib.c               |  5 ++--
>  UefiCpuPkg/Library/MpInitLib/MpLib.c                  |  7 +++---
>  UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.c    | 18 ++++++++------
>  7 files changed, 55 insertions(+), 25 deletions(-)
> 
> diff --git a/UefiCpuPkg/Include/Library/VmgExitLib.h b/UefiCpuPkg/Include/Library/VmgExitLib.h
> index ba5ea024839e..617b6cf8d2e7 100644
> --- a/UefiCpuPkg/Include/Library/VmgExitLib.h
> +++ b/UefiCpuPkg/Include/Library/VmgExitLib.h
> @@ -50,13 +50,16 @@ VmgExit (
>    Performs the necessary steps in preparation for invoking VMGEXIT. Must be
>    called before setting any fields within the GHCB.
>  
> -  @param[in, out]  Ghcb       A pointer to the GHCB
> +  @param[in, out]  Ghcb            A pointer to the GHCB
> +  @param[in, out]  InterruptState  A pointer to hold the current interrupt
> +                                   state, used for restoring in VmgDone ()
>  
>  **/
>  VOID
>  EFIAPI
>  VmgInit (
> -  IN OUT GHCB                *Ghcb
> +  IN OUT GHCB                *Ghcb,
> +  IN OUT BOOLEAN             *InterruptState
>    );
>  
>  /**
> @@ -65,13 +68,16 @@ VmgInit (
>    Performs the necessary steps to cleanup after invoking VMGEXIT. Must be
>    called after obtaining needed fields within the GHCB.
>  
> -  @param[in, out]  Ghcb       A pointer to the GHCB
> +  @param[in, out]  Ghcb            A pointer to the GHCB
> +  @param[in]       InterruptState  An indicator to conditionally (re)enable
> +                                   interrupts
>  
>  **/
>  VOID
>  EFIAPI
>  VmgDone (
> -  IN OUT GHCB                *Ghcb
> +  IN OUT GHCB                *Ghcb,
> +  IN     BOOLEAN             InterruptState
>    );
>  
>  /**
> diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitLib.c b/OvmfPkg/Library/VmgExitLib/VmgExitLib.c
> index ae86d850ba61..18b102df5f9a 100644
> --- a/OvmfPkg/Library/VmgExitLib/VmgExitLib.c
> +++ b/OvmfPkg/Library/VmgExitLib/VmgExitLib.c
> @@ -132,15 +132,27 @@ VmgExit (
>    Performs the necessary steps in preparation for invoking VMGEXIT. Must be
>    called before setting any fields within the GHCB.
>  
> -  @param[in, out]  Ghcb       A pointer to the GHCB
> +  @param[in, out]  Ghcb            A pointer to the GHCB
> +  @param[in, out]  InterruptState  A pointer to hold the current interrupt
> +                                   state, used for restoring in VmgDone ()
>  
>  **/
>  VOID
>  EFIAPI
>  VmgInit (
> -  IN OUT GHCB                *Ghcb
> +  IN OUT GHCB                *Ghcb,
> +  IN OUT BOOLEAN             *InterruptState
>    )
>  {
> +  //
> +  // Be sure that an interrupt can't cause a #VC while the GHCB is
> +  // being used.
> +  //
> +  *InterruptState = GetInterruptState ();
> +  if (*InterruptState) {
> +    DisableInterrupts ();
> +  }
> +
>    SetMem (&Ghcb->SaveArea, sizeof (Ghcb->SaveArea), 0);
>  }
>  
> @@ -150,15 +162,21 @@ VmgInit (
>    Performs the necessary steps to cleanup after invoking VMGEXIT. Must be
>    called after obtaining needed fields within the GHCB.
>  
> -  @param[in, out]  Ghcb       A pointer to the GHCB
> +  @param[in, out]  Ghcb            A pointer to the GHCB
> +  @param[in]       InterruptState  An indicator to conditionally (re)enable
> +                                   interrupts
>  
>  **/
>  VOID
>  EFIAPI
>  VmgDone (
> -  IN OUT GHCB                *Ghcb
> +  IN OUT GHCB                *Ghcb,
> +  IN     BOOLEAN             InterruptState
>    )
>  {
> +  if (InterruptState) {
> +    EnableInterrupts ();
> +  }
>  }
>  
>  /**
> diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
> index 9bf9d160179c..1671db3a01b1 100644
> --- a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
> +++ b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
> @@ -1568,6 +1568,7 @@ VmgExitHandleVc (
>    SEV_ES_INSTRUCTION_DATA   InstructionData;
>    UINT64                    ExitCode, Status;
>    EFI_STATUS                VcRet;
> +  BOOLEAN                   InterruptState;
>  
>    VcRet = EFI_SUCCESS;
>  
> @@ -1578,7 +1579,7 @@ VmgExitHandleVc (
>    Regs = SystemContext.SystemContextX64;
>    Ghcb = Msr.Ghcb;
>  
> -  VmgInit (Ghcb);
> +  VmgInit (Ghcb, &InterruptState);
>  
>    ExitCode = Regs->ExceptionData;
>    switch (ExitCode) {
> @@ -1662,7 +1663,7 @@ VmgExitHandleVc (
>      VcRet = EFI_PROTOCOL_ERROR;
>    }
>  
> -  VmgDone (Ghcb);
> +  VmgDone (Ghcb, InterruptState);
>  
>    return VcRet;
>  }
> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
> index f9b21b54137d..1b0742967f71 100644
> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlashDxe.c
> @@ -52,6 +52,7 @@ QemuFlashPtrWrite (
>    if (MemEncryptSevEsIsEnabled ()) {
>      MSR_SEV_ES_GHCB_REGISTER  Msr;
>      GHCB                      *Ghcb;
> +    BOOLEAN                   InterruptState;
>  
>      Msr.GhcbPhysicalAddress = AsmReadMsr64 (MSR_SEV_ES_GHCB);
>      Ghcb = Msr.Ghcb;
> @@ -63,12 +64,12 @@ QemuFlashPtrWrite (
>      // #VC exception. Instead, use the the VMGEXIT MMIO write support directly
>      // to perform the update.
>      //
> -    VmgInit (Ghcb);
> +    VmgInit (Ghcb, &InterruptState);
>      Ghcb->SharedBuffer[0] = Value;
>      Ghcb->SaveArea.SwScratch = (UINT64) (UINTN) Ghcb->SharedBuffer;
>      VmgSetOffsetValid (Ghcb, GhcbSwScratch);
>      VmgExit (Ghcb, SVM_EXIT_MMIO_WRITE, (UINT64) (UINTN) Ptr, 1);
> -    VmgDone (Ghcb);
> +    VmgDone (Ghcb, InterruptState);
>    } else {
>      *Ptr = Value;
>    }
> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> index 2c00d72ddefe..7839c249760e 100644
> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> @@ -171,6 +171,7 @@ GetSevEsAPMemory (
>    EFI_PHYSICAL_ADDRESS      StartAddress;
>    MSR_SEV_ES_GHCB_REGISTER  Msr;
>    GHCB                      *Ghcb;
> +  BOOLEAN                   InterruptState;
>  
>    //
>    // Allocate 1 page for AP jump table page
> @@ -192,9 +193,9 @@ GetSevEsAPMemory (
>    Msr.GhcbPhysicalAddress = AsmReadMsr64 (MSR_SEV_ES_GHCB);
>    Ghcb = Msr.Ghcb;
>  
> -  VmgInit (Ghcb);
> +  VmgInit (Ghcb, &InterruptState);
>    VmgExit (Ghcb, SVM_EXIT_AP_JUMP_TABLE, 0, (UINT64) (UINTN) StartAddress);
> -  VmgDone (Ghcb);
> +  VmgDone (Ghcb, InterruptState);
>  
>    return (UINTN) StartAddress;
>  }
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 07426274f639..4f4b26a7c196 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -884,6 +884,7 @@ ApWakeupFunction (
>            GHCB                      *Ghcb;
>            UINT64                    Status;
>            BOOLEAN                   DoDecrement;
> +          BOOLEAN                   InterruptState;
>  
>            DoDecrement = (BOOLEAN) (CpuMpData->InitFlag == ApInitConfig);
>  
> @@ -891,7 +892,7 @@ ApWakeupFunction (
>              Msr.GhcbPhysicalAddress = AsmReadMsr64 (MSR_SEV_ES_GHCB);
>              Ghcb = Msr.Ghcb;
>  
> -            VmgInit (Ghcb);
> +            VmgInit (Ghcb, &InterruptState);
>  
>              if (DoDecrement) {
>                DoDecrement = FALSE;
> @@ -905,11 +906,11 @@ ApWakeupFunction (
>  
>              Status = VmgExit (Ghcb, SVM_EXIT_AP_RESET_HOLD, 0, 0);
>              if ((Status == 0) && (Ghcb->SaveArea.SwExitInfo2 != 0)) {
> -              VmgDone (Ghcb);
> +              VmgDone (Ghcb, InterruptState);
>                break;
>              }
>  
> -            VmgDone (Ghcb);
> +            VmgDone (Ghcb, InterruptState);
>            }
>  
>            //
> diff --git a/UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.c b/UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.c
> index b000232c472e..24defd624c63 100644
> --- a/UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.c
> +++ b/UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.c
> @@ -57,15 +57,16 @@ VmgExit (
>    Performs the necessary steps in preparation for invoking VMGEXIT. Must be
>    called before setting any fields within the GHCB.
>  
> -  The base library function does nothing.
> -
> -  @param[in, out]  Ghcb       A pointer to the GHCB
> +  @param[in, out]  Ghcb            A pointer to the GHCB
> +  @param[in, out]  InterruptState  A pointer to hold the current interrupt
> +                                   state, used for restoring in VmgDone ()
>  
>  **/
>  VOID
>  EFIAPI
>  VmgInit (
> -  IN OUT GHCB                *Ghcb
> +  IN OUT GHCB                *Ghcb,
> +  IN OUT BOOLEAN             *InterruptState
>    )
>  {
>  }
> @@ -76,15 +77,16 @@ VmgInit (
>    Performs the necessary steps to cleanup after invoking VMGEXIT. Must be
>    called after obtaining needed fields within the GHCB.
>  
> -  The base library function does nothing.
> -
> -  @param[in, out]  Ghcb       A pointer to the GHCB
> +  @param[in, out]  Ghcb            A pointer to the GHCB
> +  @param[in]       InterruptState  An indicator to conditionally (re)enable
> +                                   interrupts
>  
>  **/
>  VOID
>  EFIAPI
>  VmgDone (
> -  IN OUT GHCB                *Ghcb
> +  IN OUT GHCB                *Ghcb,
> +  IN     BOOLEAN             InterruptState
>    )
>  {
>  }
> 

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

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

[Lendacky, Thomas]

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

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3008

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>
Acked-by: Ray Ni <ray.ni@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 4f4b26a7c196..c9bb1d25c616 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