* [PATCH v3 01/11] MdePkg: Clean up GHCB field offsets and save area
2020-10-29 14:17 [PATCH v3 00/11] SEV-ES guest support fixes and cleanup Lendacky, Thomas
@ 2020-10-29 14:17 ` Lendacky, Thomas
2020-10-30 1:03 ` 回复: " gaoliming
2020-10-29 14:17 ` [PATCH v3 02/11] UefiCpuPkg/VmgExitLib: Add interfaces to set/read GHCB ValidBitmap bits Lendacky, Thomas
` (11 subsequent siblings)
12 siblings, 1 reply; 25+ messages in thread
From: Lendacky, Thomas @ 2020-10-29 14:17 UTC (permalink / raw)
To: devel; +Cc: Brijesh Singh, Michael D Kinney, Liming Gao, Zhiguang Liu
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.
Define only fields that are used per the GHCB specification, which will
result in removing some fields and adding others.
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>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
MdePkg/Include/Register/Amd/Ghcb.h | 40 +++++++-------------
1 file changed, 14 insertions(+), 26 deletions(-)
diff --git a/MdePkg/Include/Register/Amd/Ghcb.h b/MdePkg/Include/Register/Amd/Ghcb.h
index 54a80da0f6d7..1fce64d5b7cd 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_REGISTER;
+
typedef union {
struct {
UINT32 Lower32Bits;
--
2.28.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* 回复: [PATCH v3 01/11] MdePkg: Clean up GHCB field offsets and save area
2020-10-29 14:17 ` [PATCH v3 01/11] MdePkg: Clean up GHCB field offsets and save area Lendacky, Thomas
@ 2020-10-30 1:03 ` gaoliming
0 siblings, 0 replies; 25+ messages in thread
From: gaoliming @ 2020-10-30 1:03 UTC (permalink / raw)
To: 'Tom Lendacky', devel
Cc: 'Brijesh Singh', 'Michael D Kinney',
'Zhiguang Liu'
Reviewed-by: Liming Gao <gaoliming@byosoft.com.cn>
> -----邮件原件-----
> 发件人: Tom Lendacky <thomas.lendacky@amd.com>
> 发送时间: 2020年10月29日 22:18
> 收件人: 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>
> 主题: [PATCH v3 01/11] MdePkg: 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.
> Define only fields that are used per the GHCB specification, which will
> result in removing some fields and adding others.
>
> 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>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
> MdePkg/Include/Register/Amd/Ghcb.h | 40 +++++++-------------
> 1 file changed, 14 insertions(+), 26 deletions(-)
>
> diff --git a/MdePkg/Include/Register/Amd/Ghcb.h
> b/MdePkg/Include/Register/Amd/Ghcb.h
> index 54a80da0f6d7..1fce64d5b7cd 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_REGISTER;
> +
> typedef union {
> struct {
> UINT32 Lower32Bits;
> --
> 2.28.0
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v3 02/11] UefiCpuPkg/VmgExitLib: Add interfaces to set/read GHCB ValidBitmap bits
2020-10-29 14:17 [PATCH v3 00/11] SEV-ES guest support fixes and cleanup Lendacky, Thomas
2020-10-29 14:17 ` [PATCH v3 01/11] MdePkg: Clean up GHCB field offsets and save area Lendacky, Thomas
@ 2020-10-29 14:17 ` Lendacky, Thomas
2020-11-03 16:22 ` [edk2-devel] " Laszlo Ersek
` (2 more replies)
2020-10-29 14:17 ` [PATCH v3 03/11] OvmfPkg/VmgExitLib: Implement new VmgExitLib interfaces Lendacky, Thomas
` (10 subsequent siblings)
12 siblings, 3 replies; 25+ messages in thread
From: Lendacky, Thomas @ 2020-10-29 14:17 UTC (permalink / raw)
To: devel; +Cc: Brijesh Singh, Eric Dong, Ray Ni, Laszlo Ersek, Rahul Kumar
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>
Reviewed-by: Laszlo Ersek <lersek@redhat.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..07e8af6450b9 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_REGISTER 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 valid in the GHCB
+ @retval FALSE Offset is not marked valid in the GHCB
+
+**/
+BOOLEAN
+EFIAPI
+VmgIsOffsetValid (
+ IN GHCB *Ghcb,
+ IN GHCB_REGISTER Offset
+ );
+
/**
Handle a #VC exception.
diff --git a/UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.c b/UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.c
index bb265e1700d2..b47e282aff82 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_REGISTER 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 valid in the GHCB
+ @retval FALSE Offset is not marked valid in the GHCB
+
+**/
+BOOLEAN
+EFIAPI
+VmgIsOffsetValid (
+ IN GHCB *Ghcb,
+ IN GHCB_REGISTER Offset
+ )
+{
+ return FALSE;
+}
+
/**
Handle a #VC exception.
--
2.28.0
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [edk2-devel] [PATCH v3 02/11] UefiCpuPkg/VmgExitLib: Add interfaces to set/read GHCB ValidBitmap bits
2020-10-29 14:17 ` [PATCH v3 02/11] UefiCpuPkg/VmgExitLib: Add interfaces to set/read GHCB ValidBitmap bits Lendacky, Thomas
@ 2020-11-03 16:22 ` Laszlo Ersek
2020-11-03 16:38 ` Laszlo Ersek
2020-11-04 8:19 ` Dong, Eric
2 siblings, 0 replies; 25+ messages in thread
From: Laszlo Ersek @ 2020-11-03 16:22 UTC (permalink / raw)
To: devel, thomas.lendacky; +Cc: Brijesh Singh, Eric Dong, Ray Ni, Rahul Kumar
On 10/29/20 15:17, 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>
> Reviewed-by: Laszlo Ersek <lersek@redhat.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(+)
Thanks for the updates.
Laszlo
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [edk2-devel] [PATCH v3 02/11] UefiCpuPkg/VmgExitLib: Add interfaces to set/read GHCB ValidBitmap bits
2020-10-29 14:17 ` [PATCH v3 02/11] UefiCpuPkg/VmgExitLib: Add interfaces to set/read GHCB ValidBitmap bits Lendacky, Thomas
2020-11-03 16:22 ` [edk2-devel] " Laszlo Ersek
@ 2020-11-03 16:38 ` Laszlo Ersek
2020-11-04 8:19 ` Dong, Eric
2 siblings, 0 replies; 25+ messages in thread
From: Laszlo Ersek @ 2020-11-03 16:38 UTC (permalink / raw)
To: Eric Dong, Ray Ni; +Cc: devel, thomas.lendacky, Brijesh Singh, Rahul Kumar
Ray, Eric, can one of you please ACK this patch? I've reviewed it
already, but your ACK would be appreciated, process-wise. I intend to
merge the series on Thursday at the latest (Nov 5th).
Thank you.
Laszlo
On 10/29/20 15:17, 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>
> Reviewed-by: Laszlo Ersek <lersek@redhat.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..07e8af6450b9 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_REGISTER 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 valid in the GHCB
> + @retval FALSE Offset is not marked valid in the GHCB
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +VmgIsOffsetValid (
> + IN GHCB *Ghcb,
> + IN GHCB_REGISTER Offset
> + );
> +
> /**
> Handle a #VC exception.
>
> diff --git a/UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.c b/UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.c
> index bb265e1700d2..b47e282aff82 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_REGISTER 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 valid in the GHCB
> + @retval FALSE Offset is not marked valid in the GHCB
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +VmgIsOffsetValid (
> + IN GHCB *Ghcb,
> + IN GHCB_REGISTER Offset
> + )
> +{
> + return FALSE;
> +}
> +
> /**
> Handle a #VC exception.
>
>
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH v3 02/11] UefiCpuPkg/VmgExitLib: Add interfaces to set/read GHCB ValidBitmap bits
2020-10-29 14:17 ` [PATCH v3 02/11] UefiCpuPkg/VmgExitLib: Add interfaces to set/read GHCB ValidBitmap bits Lendacky, Thomas
2020-11-03 16:22 ` [edk2-devel] " Laszlo Ersek
2020-11-03 16:38 ` Laszlo Ersek
@ 2020-11-04 8:19 ` Dong, Eric
2 siblings, 0 replies; 25+ messages in thread
From: Dong, Eric @ 2020-11-04 8:19 UTC (permalink / raw)
To: Tom Lendacky, devel@edk2.groups.io
Cc: Brijesh Singh, Ni, Ray, Laszlo Ersek, Kumar, Rahul1
Acked-by: Eric Dong <eric.dong@intel.com>
-----Original Message-----
From: Tom Lendacky <thomas.lendacky@amd.com>
Sent: Thursday, October 29, 2020 10:18 PM
To: devel@edk2.groups.io
Cc: Brijesh Singh <brijesh.singh@amd.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>
Subject: [PATCH v3 02/11] UefiCpuPkg/VmgExitLib: Add interfaces to set/read GHCB ValidBitmap bits
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>
Reviewed-by: Laszlo Ersek <lersek@redhat.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..07e8af6450b9 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_REGISTER 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 valid in the GHCB
+ @retval FALSE Offset is not marked valid in the GHCB
+
+**/
+BOOLEAN
+EFIAPI
+VmgIsOffsetValid (
+ IN GHCB *Ghcb,
+ IN GHCB_REGISTER Offset
+ );
+
/**
Handle a #VC exception.
diff --git a/UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.c b/UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.c
index bb265e1700d2..b47e282aff82 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_REGISTER 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 valid in the GHCB
+ @retval FALSE Offset is not marked valid in the GHCB
+
+**/
+BOOLEAN
+EFIAPI
+VmgIsOffsetValid (
+ IN GHCB *Ghcb,
+ IN GHCB_REGISTER Offset
+ )
+{
+ return FALSE;
+}
+
/**
Handle a #VC exception.
--
2.28.0
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v3 03/11] OvmfPkg/VmgExitLib: Implement new VmgExitLib interfaces
2020-10-29 14:17 [PATCH v3 00/11] SEV-ES guest support fixes and cleanup Lendacky, Thomas
2020-10-29 14:17 ` [PATCH v3 01/11] MdePkg: Clean up GHCB field offsets and save area Lendacky, Thomas
2020-10-29 14:17 ` [PATCH v3 02/11] UefiCpuPkg/VmgExitLib: Add interfaces to set/read GHCB ValidBitmap bits Lendacky, Thomas
@ 2020-10-29 14:17 ` Lendacky, Thomas
2020-11-03 16:26 ` [edk2-devel] " Laszlo Ersek
2020-10-29 14:17 ` [PATCH v3 04/11] OvmfPkg/VmgExitLib: Set the SW exit fields when performing VMGEXIT Lendacky, Thomas
` (9 subsequent siblings)
12 siblings, 1 reply; 25+ messages in thread
From: Lendacky, Thomas @ 2020-10-29 14:17 UTC (permalink / raw)
To: devel; +Cc: Brijesh Singh, Jordan Justen, Laszlo Ersek, Ard Biesheuvel
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>
Reviewed-by: Laszlo Ersek <lersek@redhat.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..21f68b19c94e 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_REGISTER 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 valid in the GHCB
+ @retval FALSE Offset is not marked valid in the GHCB
+
+**/
+BOOLEAN
+EFIAPI
+VmgIsOffsetValid (
+ IN GHCB *Ghcb,
+ IN GHCB_REGISTER 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 8e42b305e83c..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_REGISTER 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_REGISTER 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
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [edk2-devel] [PATCH v3 03/11] OvmfPkg/VmgExitLib: Implement new VmgExitLib interfaces
2020-10-29 14:17 ` [PATCH v3 03/11] OvmfPkg/VmgExitLib: Implement new VmgExitLib interfaces Lendacky, Thomas
@ 2020-11-03 16:26 ` Laszlo Ersek
0 siblings, 0 replies; 25+ messages in thread
From: Laszlo Ersek @ 2020-11-03 16:26 UTC (permalink / raw)
To: devel, thomas.lendacky; +Cc: Brijesh Singh, Jordan Justen, Ard Biesheuvel
On 10/29/20 15:17, 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>
> Reviewed-by: Laszlo Ersek <lersek@redhat.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(-)
Looks OK.
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v3 04/11] OvmfPkg/VmgExitLib: Set the SW exit fields when performing VMGEXIT
2020-10-29 14:17 [PATCH v3 00/11] SEV-ES guest support fixes and cleanup Lendacky, Thomas
` (2 preceding siblings ...)
2020-10-29 14:17 ` [PATCH v3 03/11] OvmfPkg/VmgExitLib: Implement new VmgExitLib interfaces Lendacky, Thomas
@ 2020-10-29 14:17 ` Lendacky, Thomas
2020-10-29 14:17 ` [PATCH v3 05/11] OvmfPkg/VmgExitLib: Set the SwScratch valid bit for IOIO events Lendacky, Thomas
` (8 subsequent siblings)
12 siblings, 0 replies; 25+ messages in thread
From: Lendacky, Thomas @ 2020-10-29 14:17 UTC (permalink / raw)
To: devel; +Cc: Brijesh Singh, Jordan Justen, Laszlo Ersek, Ard Biesheuvel
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>
Reviewed-by: Laszlo Ersek <lersek@redhat.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 21f68b19c94e..0540df8a04d4 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
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH v3 05/11] OvmfPkg/VmgExitLib: Set the SwScratch valid bit for IOIO events
2020-10-29 14:17 [PATCH v3 00/11] SEV-ES guest support fixes and cleanup Lendacky, Thomas
` (3 preceding siblings ...)
2020-10-29 14:17 ` [PATCH v3 04/11] OvmfPkg/VmgExitLib: Set the SW exit fields when performing VMGEXIT Lendacky, Thomas
@ 2020-10-29 14:17 ` Lendacky, Thomas
2020-10-29 14:17 ` [PATCH v3 06/11] OvmfPkg/VmgExitLib: Set the SwScratch valid bit for MMIO events Lendacky, Thomas
` (7 subsequent siblings)
12 siblings, 0 replies; 25+ messages in thread
From: Lendacky, Thomas @ 2020-10-29 14:17 UTC (permalink / raw)
To: devel; +Cc: Brijesh Singh, Jordan Justen, Laszlo Ersek, Ard Biesheuvel
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>
Reviewed-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
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH v3 06/11] OvmfPkg/VmgExitLib: Set the SwScratch valid bit for MMIO events
2020-10-29 14:17 [PATCH v3 00/11] SEV-ES guest support fixes and cleanup Lendacky, Thomas
` (4 preceding siblings ...)
2020-10-29 14:17 ` [PATCH v3 05/11] OvmfPkg/VmgExitLib: Set the SwScratch valid bit for IOIO events Lendacky, Thomas
@ 2020-10-29 14:17 ` Lendacky, Thomas
2020-10-29 14:17 ` [PATCH v3 07/11] UefiCpuPkg/MpInitLib: Set the SW exit fields when performing VMGEXIT Lendacky, Thomas
` (6 subsequent siblings)
12 siblings, 0 replies; 25+ messages in thread
From: Lendacky, Thomas @ 2020-10-29 14:17 UTC (permalink / raw)
To: devel; +Cc: Brijesh Singh, Jordan Justen, Laszlo Ersek, Ard Biesheuvel
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
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH v3 07/11] UefiCpuPkg/MpInitLib: Set the SW exit fields when performing VMGEXIT
2020-10-29 14:17 [PATCH v3 00/11] SEV-ES guest support fixes and cleanup Lendacky, Thomas
` (5 preceding siblings ...)
2020-10-29 14:17 ` [PATCH v3 06/11] OvmfPkg/VmgExitLib: Set the SwScratch valid bit for MMIO events Lendacky, Thomas
@ 2020-10-29 14:17 ` Lendacky, Thomas
2020-10-29 14:17 ` [PATCH v3 08/11] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Set the SwScratch valid bit Lendacky, Thomas
` (5 subsequent siblings)
12 siblings, 0 replies; 25+ messages in thread
From: Lendacky, Thomas @ 2020-10-29 14:17 UTC (permalink / raw)
To: devel; +Cc: Brijesh Singh, Eric Dong, Ray Ni, Laszlo Ersek, Rahul Kumar
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
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH v3 08/11] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Set the SwScratch valid bit
2020-10-29 14:17 [PATCH v3 00/11] SEV-ES guest support fixes and cleanup Lendacky, Thomas
` (6 preceding siblings ...)
2020-10-29 14:17 ` [PATCH v3 07/11] UefiCpuPkg/MpInitLib: Set the SW exit fields when performing VMGEXIT Lendacky, Thomas
@ 2020-10-29 14:17 ` Lendacky, Thomas
2020-10-29 14:18 ` [PATCH v3 09/11] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Fix erase blocks for SEV-ES Lendacky, Thomas
` (4 subsequent siblings)
12 siblings, 0 replies; 25+ messages in thread
From: Lendacky, Thomas @ 2020-10-29 14:17 UTC (permalink / raw)
To: devel; +Cc: Brijesh Singh, Jordan Justen, Laszlo Ersek, Ard Biesheuvel
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>
Reviewed-by: Laszlo Ersek <lersek@redhat.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
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH v3 09/11] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Fix erase blocks for SEV-ES
2020-10-29 14:17 [PATCH v3 00/11] SEV-ES guest support fixes and cleanup Lendacky, Thomas
` (7 preceding siblings ...)
2020-10-29 14:17 ` [PATCH v3 08/11] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Set the SwScratch valid bit Lendacky, Thomas
@ 2020-10-29 14:18 ` Lendacky, Thomas
2020-10-29 14:18 ` [PATCH v3 10/11] UefiCpuPkg, OvmfPkg: Disable interrupts when using the GHCB Lendacky, Thomas
` (3 subsequent siblings)
12 siblings, 0 replies; 25+ messages in thread
From: Lendacky, Thomas @ 2020-10-29 14:18 UTC (permalink / raw)
To: devel; +Cc: Brijesh Singh, Jordan Justen, Laszlo Ersek, Ard Biesheuvel
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
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH v3 10/11] UefiCpuPkg, OvmfPkg: Disable interrupts when using the GHCB
2020-10-29 14:17 [PATCH v3 00/11] SEV-ES guest support fixes and cleanup Lendacky, Thomas
` (8 preceding siblings ...)
2020-10-29 14:18 ` [PATCH v3 09/11] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Fix erase blocks for SEV-ES Lendacky, Thomas
@ 2020-10-29 14:18 ` Lendacky, Thomas
2020-11-03 16:38 ` [edk2-devel] " Laszlo Ersek
2020-11-04 8:20 ` Dong, Eric
2020-10-29 14:18 ` [PATCH v3 11/11] UefiCpuPkg/MpInitLib: For SEV-ES guest, set stack based on processor number Lendacky, Thomas
` (2 subsequent siblings)
12 siblings, 2 replies; 25+ messages in thread
From: Lendacky, Thomas @ 2020-10-29 14:18 UTC (permalink / raw)
To: devel
Cc: Brijesh Singh, Eric Dong, Ray Ni, Laszlo Ersek, Rahul Kumar,
Jordan Justen, Ard Biesheuvel
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>
Reviewed-by: Laszlo Ersek <lersek@redhat.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 07e8af6450b9..061948cf840d 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 0540df8a04d4..bc5cd61d751f 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 6d977d45bcdd..1f47ff3f73b5 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 b47e282aff82..89b065cb3ff3 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
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [edk2-devel] [PATCH v3 10/11] UefiCpuPkg, OvmfPkg: Disable interrupts when using the GHCB
2020-10-29 14:18 ` [PATCH v3 10/11] UefiCpuPkg, OvmfPkg: Disable interrupts when using the GHCB Lendacky, Thomas
@ 2020-11-03 16:38 ` Laszlo Ersek
2020-11-04 8:20 ` Dong, Eric
1 sibling, 0 replies; 25+ messages in thread
From: Laszlo Ersek @ 2020-11-03 16:38 UTC (permalink / raw)
To: Eric Dong, Ray Ni
Cc: devel, thomas.lendacky, Brijesh Singh, Rahul Kumar, Jordan Justen,
Ard Biesheuvel
Ray, Eric, can one of you please ACK this patch as well, for the
UefiCpuPkg parts? I've reviewed the patch already (in full), but your
ACK would be appreciated, process-wise. I intend to merge the series on
Thursday at the latest (Nov 5th).
(Eric stated earlier that he was OK with modifying both UefiCpuPkg and
OvmfPkg: <https://edk2.groups.io/g/devel/message/66642>.)
Thank you.
Laszlo
On 10/29/20 15:18, 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>
> Reviewed-by: Laszlo Ersek <lersek@redhat.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 07e8af6450b9..061948cf840d 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 0540df8a04d4..bc5cd61d751f 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 6d977d45bcdd..1f47ff3f73b5 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 b47e282aff82..89b065cb3ff3 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
> )
> {
> }
>
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [edk2-devel] [PATCH v3 10/11] UefiCpuPkg, OvmfPkg: Disable interrupts when using the GHCB
2020-10-29 14:18 ` [PATCH v3 10/11] UefiCpuPkg, OvmfPkg: Disable interrupts when using the GHCB Lendacky, Thomas
2020-11-03 16:38 ` [edk2-devel] " Laszlo Ersek
@ 2020-11-04 8:20 ` Dong, Eric
1 sibling, 0 replies; 25+ messages in thread
From: Dong, Eric @ 2020-11-04 8:20 UTC (permalink / raw)
To: devel@edk2.groups.io, thomas.lendacky@amd.com
Cc: Brijesh Singh, Ni, Ray, Laszlo Ersek, Kumar, Rahul1,
Justen, Jordan L, Ard Biesheuvel
Acked-by: Eric Dong <eric.dong@intel.com>
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Lendacky, Thomas
Sent: Thursday, October 29, 2020 10:18 PM
To: devel@edk2.groups.io
Cc: Brijesh Singh <brijesh.singh@amd.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Ard Biesheuvel <ard.biesheuvel@arm.com>
Subject: [edk2-devel] [PATCH v3 10/11] UefiCpuPkg, OvmfPkg: Disable interrupts when using the GHCB
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>
Reviewed-by: Laszlo Ersek <lersek@redhat.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 07e8af6450b9..061948cf840d 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 0540df8a04d4..bc5cd61d751f 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 6d977d45bcdd..1f47ff3f73b5 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 b47e282aff82..89b065cb3ff3 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
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v3 11/11] UefiCpuPkg/MpInitLib: For SEV-ES guest, set stack based on processor number
2020-10-29 14:17 [PATCH v3 00/11] SEV-ES guest support fixes and cleanup Lendacky, Thomas
` (9 preceding siblings ...)
2020-10-29 14:18 ` [PATCH v3 10/11] UefiCpuPkg, OvmfPkg: Disable interrupts when using the GHCB Lendacky, Thomas
@ 2020-10-29 14:18 ` Lendacky, Thomas
2020-11-02 16:46 ` [PATCH v3 00/11] SEV-ES guest support fixes and cleanup Laszlo Ersek
2020-11-05 3:29 ` [edk2-devel] " Laszlo Ersek
12 siblings, 0 replies; 25+ messages in thread
From: Lendacky, Thomas @ 2020-10-29 14:18 UTC (permalink / raw)
To: devel; +Cc: Brijesh Singh, Eric Dong, Ray Ni, Laszlo Ersek, Rahul Kumar
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 1f47ff3f73b5..681fa79b4cff 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
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH v3 00/11] SEV-ES guest support fixes and cleanup
2020-10-29 14:17 [PATCH v3 00/11] SEV-ES guest support fixes and cleanup Lendacky, Thomas
` (10 preceding siblings ...)
2020-10-29 14:18 ` [PATCH v3 11/11] UefiCpuPkg/MpInitLib: For SEV-ES guest, set stack based on processor number Lendacky, Thomas
@ 2020-11-02 16:46 ` Laszlo Ersek
2020-11-05 3:29 ` [edk2-devel] " Laszlo Ersek
12 siblings, 0 replies; 25+ messages in thread
From: Laszlo Ersek @ 2020-11-02 16:46 UTC (permalink / raw)
To: Tom Lendacky, devel
Cc: Brijesh Singh, Ard Biesheuvel, Eric Dong, Liming Gao,
Jordan Justen, Michael D Kinney, Rahul Kumar, Zhiguang Liu,
Ray Ni
On 10/29/20 15:17, Tom Lendacky wrote:
> From: Tom Lendacky <thomas.lendacky@amd.com>
>
> This patch series provides some fixes, updates and cleanup to the SEV-ES
> guest support:
>
> - 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:
> 6ad819c1abe3 ("FmpDevicePkg/FmpDeviceLib: Add Last Attempt Status to Check/Set API")
>
> 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 v2:
> - Don't rename the GHCB_REGISTER enum type.
I've got this queued for review. I'll need some time for getting to it,
as I've just returned after some absence, and everything seems to have
collapsed on my head (as usual).
Laszlo
>
> 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: 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(-)
>
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [edk2-devel] [PATCH v3 00/11] SEV-ES guest support fixes and cleanup
2020-10-29 14:17 [PATCH v3 00/11] SEV-ES guest support fixes and cleanup Lendacky, Thomas
` (11 preceding siblings ...)
2020-11-02 16:46 ` [PATCH v3 00/11] SEV-ES guest support fixes and cleanup Laszlo Ersek
@ 2020-11-05 3:29 ` Laszlo Ersek
2020-11-05 14:34 ` Lendacky, Thomas
12 siblings, 1 reply; 25+ messages in thread
From: Laszlo Ersek @ 2020-11-05 3:29 UTC (permalink / raw)
To: devel, thomas.lendacky
Cc: Brijesh Singh, Ard Biesheuvel, Eric Dong, Liming Gao,
Jordan Justen, Michael D Kinney, Rahul Kumar, Zhiguang Liu,
Ray Ni
On 10/29/20 15:17, Lendacky, Thomas wrote:
> 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:
> 6ad819c1abe3 ("FmpDevicePkg/FmpDeviceLib: Add Last Attempt Status to Check/Set API")
>
> 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 v2:
> - Don't rename the GHCB_REGISTER enum type.
>
> 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: 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(-)
>
I've submitted PR#1086 <https://github.com/tianocore/edk2/pull/1086>,
but CI seems slower than usual today, and I really need some sleep, so I
won't wait for CI. Tom, if the PR succeeds, please close TianoCore#3008,
noting the commit range, and please also follow up in this thread with
the commit range.
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [edk2-devel] [PATCH v3 00/11] SEV-ES guest support fixes and cleanup
2020-11-05 3:29 ` [edk2-devel] " Laszlo Ersek
@ 2020-11-05 14:34 ` Lendacky, Thomas
2020-11-06 6:29 ` Lendacky, Thomas
0 siblings, 1 reply; 25+ messages in thread
From: Lendacky, Thomas @ 2020-11-05 14:34 UTC (permalink / raw)
To: Laszlo Ersek, devel
Cc: Brijesh Singh, Ard Biesheuvel, Eric Dong, Liming Gao,
Jordan Justen, Michael D Kinney, Rahul Kumar, Zhiguang Liu,
Ray Ni
On 11/4/20 9:29 PM, Laszlo Ersek wrote:
> On 10/29/20 15:17, Lendacky, Thomas wrote:
>> 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3008&data=04%7C01%7Cthomas.lendacky%40amd.com%7C0697eb8b721342b8a39508d8813b1f3c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637401438219622636%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=7Ox%2FhGWT9GKg9v5gIX2xRjIvKLiGBkeBzT7Via20cZk%3D&reserved=0
>>
>> ---
>>
>> These patches are based on commit:
>> 6ad819c1abe3 ("FmpDevicePkg/FmpDeviceLib: Add Last Attempt Status to Check/Set API")
>>
>> 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 v2:
>> - Don't rename the GHCB_REGISTER enum type.
>>
>> 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: 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(-)
>>
>
> I've submitted PR#1086 <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftianocore%2Fedk2%2Fpull%2F1086&data=04%7C01%7Cthomas.lendacky%40amd.com%7C0697eb8b721342b8a39508d8813b1f3c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637401438219622636%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=NH2IfxbTqaHvVijHOGExWFU%2FECzxOVwgJyVajPZjaCw%3D&reserved=0>,
> but CI seems slower than usual today, and I really need some sleep, so I
> won't wait for CI. Tom, if the PR succeeds, please close TianoCore#3008,
> noting the commit range, and please also follow up in this thread with
> the commit range.
Thanks, Laszlo!
It looks like it failed because it doesn't like the use of the "sizeof
(UINT64)". I suppose I can change that to just hard code a value of 8. Let
me know what you think.
Thanks,
Tom
>
> Thanks!
> Laszlo
>
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [edk2-devel] [PATCH v3 00/11] SEV-ES guest support fixes and cleanup
2020-11-05 14:34 ` Lendacky, Thomas
@ 2020-11-06 6:29 ` Lendacky, Thomas
2020-11-06 16:46 ` Laszlo Ersek
0 siblings, 1 reply; 25+ messages in thread
From: Lendacky, Thomas @ 2020-11-06 6:29 UTC (permalink / raw)
To: Laszlo Ersek, devel
Cc: Brijesh Singh, Ard Biesheuvel, Eric Dong, Liming Gao,
Jordan Justen, Michael D Kinney, Rahul Kumar, Zhiguang Liu,
Ray Ni
On 11/5/20 8:34 AM, Tom Lendacky wrote:
> On 11/4/20 9:29 PM, Laszlo Ersek wrote:
>> On 10/29/20 15:17, Lendacky, Thomas wrote:
>>> 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3008&data=04%7C01%7Cthomas.lendacky%40amd.com%7C0697eb8b721342b8a39508d8813b1f3c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637401438219622636%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=7Ox%2FhGWT9GKg9v5gIX2xRjIvKLiGBkeBzT7Via20cZk%3D&reserved=0
>>>
>>>
>>> ---
>>>
>>> These patches are based on commit:
>>> 6ad819c1abe3 ("FmpDevicePkg/FmpDeviceLib: Add Last Attempt Status to
>>> Check/Set API")
>>>
>>> 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 v2:
>>> - Don't rename the GHCB_REGISTER enum type.
>>>
>>> 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: 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(-)
>>>
>>
>> I've submitted PR#1086
>> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftianocore%2Fedk2%2Fpull%2F1086&data=04%7C01%7Cthomas.lendacky%40amd.com%7C0697eb8b721342b8a39508d8813b1f3c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637401438219622636%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=NH2IfxbTqaHvVijHOGExWFU%2FECzxOVwgJyVajPZjaCw%3D&reserved=0>,
>>
>> but CI seems slower than usual today, and I really need some sleep, so I
>> won't wait for CI. Tom, if the PR succeeds, please close TianoCore#3008,
>> noting the commit range, and please also follow up in this thread with
>> the commit range.
>
> Thanks, Laszlo!
>
> It looks like it failed because it doesn't like the use of the "sizeof
> (UINT64)". I suppose I can change that to just hard code a value of 8. Let
> me know what you think.
I did verify that changing the "sizeof (UINT64)" to "8" in the first patch
results in all CI tests passing. I can re-submit with that, if you feel
that is the best course of action.
Thanks,
Tom
>
> Thanks,
> Tom
>
>>
>> Thanks!
>> Laszlo
>>
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [edk2-devel] [PATCH v3 00/11] SEV-ES guest support fixes and cleanup
2020-11-06 6:29 ` Lendacky, Thomas
@ 2020-11-06 16:46 ` Laszlo Ersek
2020-11-06 17:48 ` Lendacky, Thomas
0 siblings, 1 reply; 25+ messages in thread
From: Laszlo Ersek @ 2020-11-06 16:46 UTC (permalink / raw)
To: Tom Lendacky, devel
Cc: Brijesh Singh, Ard Biesheuvel, Eric Dong, Liming Gao,
Jordan Justen, Michael D Kinney, Rahul Kumar, Zhiguang Liu,
Ray Ni
On 11/06/20 07:29, Tom Lendacky wrote:
> On 11/5/20 8:34 AM, Tom Lendacky wrote:
>> On 11/4/20 9:29 PM, Laszlo Ersek wrote:
>>> I've submitted PR#1086 [...] but CI seems slower than usual today,
>>> and I really need some sleep, so I won't wait for CI. Tom, if the PR
>>> succeeds, please close TianoCore#3008, noting the commit range, and
>>> please also follow up in this thread with the commit range.
>>
>> Thanks, Laszlo!
>>
>> It looks like it failed because it doesn't like the use of the
>> "sizeof (UINT64)". I suppose I can change that to just hard code a
>> value of 8. Let me know what you think.
>
> I did verify that changing the "sizeof (UINT64)" to "8" in the first
> patch results in all CI tests passing. I can re-submit with that, if
> you feel that is the best course of action.
So this is the error (from ECC), if I understand correctly:
> 2020-11-05T03:25:05.2175761Z ERROR - EFI coding style error
> 2020-11-05T03:25:05.2176262Z ERROR - *Error code: 8005
> 2020-11-05T03:25:05.2177026Z ERROR - *Variable name does not follow the rules: 1. First character should be upper case 2. Must contain lower case characters 3. No white space characters 4. Global variable name must start with a 'g'
> 2020-11-05T03:25:05.2177773Z ERROR - *file: //home/vsts/work/1/s/MdePkg/Include/Register/Amd/Ghcb.h
> 2020-11-05T03:25:05.2180810Z ERROR - *Line number: 114
> 2020-11-05T03:25:05.2181426Z ERROR - *Member variable [GHCB_REGISTER.(UINT64)] NOT follow naming convention.
for such lines as:
> GhcbCpl = OFFSET_OF (GHCB, SaveArea.Cpl) / sizeof (UINT64),
I think this is a bug in CheckMemberVariableFormat(), in
"BaseTools/Source/Python/Ecc/c.py".
I don't exactly understand how that ECC method functions. However,
squashing the following update into the first (MdePkg) patch of this
series seems to work:
> diff --git a/MdePkg/Include/Register/Amd/Ghcb.h b/MdePkg/Include/Register/Amd/Ghcb.h
> index 1fce64d5b7cd..3814a79e0619 100644
> --- a/MdePkg/Include/Register/Amd/Ghcb.h
> +++ b/MdePkg/Include/Register/Amd/Ghcb.h
> @@ -111,17 +111,20 @@ typedef PACKED struct {
> UINT32 GhcbUsage;
> } GHCB;
>
> +#define GHCB_SAVE_AREA_WORD_OFFSET(RegisterField) \
> + (OFFSET_OF (GHCB, SaveArea.RegisterField) / sizeof (UINT64))
> +
> 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),
> + GhcbCpl = GHCB_SAVE_AREA_WORD_OFFSET (Cpl),
> + GhcbRax = GHCB_SAVE_AREA_WORD_OFFSET (Rax),
> + GhcbRbx = GHCB_SAVE_AREA_WORD_OFFSET (Rbx),
> + GhcbRcx = GHCB_SAVE_AREA_WORD_OFFSET (Rcx),
> + GhcbRdx = GHCB_SAVE_AREA_WORD_OFFSET (Rdx),
> + GhcbXCr0 = GHCB_SAVE_AREA_WORD_OFFSET (XCr0),
> + GhcbSwExitCode = GHCB_SAVE_AREA_WORD_OFFSET (SwExitCode),
> + GhcbSwExitInfo1 = GHCB_SAVE_AREA_WORD_OFFSET (SwExitInfo1),
> + GhcbSwExitInfo2 = GHCB_SAVE_AREA_WORD_OFFSET (SwExitInfo2),
> + GhcbSwScratch = GHCB_SAVE_AREA_WORD_OFFSET (SwScratch),
> } GHCB_REGISTER;
>
> typedef union {
I build-tested this update with gcc, and also verified that it passes
ECC, by running CI locally. (I reproduced the bogus ECC error message
locally at first, of course, to see if the update makes a difference.)
> PROGRESS - --Running MdePkg: EccCheck Test NO-TARGET --
> PROGRESS - --->Test Success: EccCheck Test NO-TARGET
If it works for you as well (check with a personal build PR perhaps),
then I suggest posting v4 -- Liming should please re-check the udpated
MdePkg patch, and then we can merge the series.
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [edk2-devel] [PATCH v3 00/11] SEV-ES guest support fixes and cleanup
2020-11-06 16:46 ` Laszlo Ersek
@ 2020-11-06 17:48 ` Lendacky, Thomas
0 siblings, 0 replies; 25+ messages in thread
From: Lendacky, Thomas @ 2020-11-06 17:48 UTC (permalink / raw)
To: Laszlo Ersek, devel
Cc: Brijesh Singh, Ard Biesheuvel, Eric Dong, Liming Gao,
Jordan Justen, Michael D Kinney, Rahul Kumar, Zhiguang Liu,
Ray Ni
On 11/6/20 10:46 AM, Laszlo Ersek wrote:
> On 11/06/20 07:29, Tom Lendacky wrote:
>> On 11/5/20 8:34 AM, Tom Lendacky wrote:
>>> On 11/4/20 9:29 PM, Laszlo Ersek wrote:
>
>>>> I've submitted PR#1086 [...] but CI seems slower than usual today,
>>>> and I really need some sleep, so I won't wait for CI. Tom, if the PR
>>>> succeeds, please close TianoCore#3008, noting the commit range, and
>>>> please also follow up in this thread with the commit range.
>>>
>>> Thanks, Laszlo!
>>>
>>> It looks like it failed because it doesn't like the use of the
>>> "sizeof (UINT64)". I suppose I can change that to just hard code a
>>> value of 8. Let me know what you think.
>>
>> I did verify that changing the "sizeof (UINT64)" to "8" in the first
>> patch results in all CI tests passing. I can re-submit with that, if
>> you feel that is the best course of action.
>
>
> So this is the error (from ECC), if I understand correctly:
>
>> 2020-11-05T03:25:05.2175761Z ERROR - EFI coding style error
>> 2020-11-05T03:25:05.2176262Z ERROR - *Error code: 8005
>> 2020-11-05T03:25:05.2177026Z ERROR - *Variable name does not follow the rules: 1. First character should be upper case 2. Must contain lower case characters 3. No white space characters 4. Global variable name must start with a 'g'
>> 2020-11-05T03:25:05.2177773Z ERROR - *file: //home/vsts/work/1/s/MdePkg/Include/Register/Amd/Ghcb.h
>> 2020-11-05T03:25:05.2180810Z ERROR - *Line number: 114
>> 2020-11-05T03:25:05.2181426Z ERROR - *Member variable [GHCB_REGISTER.(UINT64)] NOT follow naming convention.
>
> for such lines as:
>
>> GhcbCpl = OFFSET_OF (GHCB, SaveArea.Cpl) / sizeof (UINT64),
>
> I think this is a bug in CheckMemberVariableFormat(), in
> "BaseTools/Source/Python/Ecc/c.py".
>
> I don't exactly understand how that ECC method functions. However,
> squashing the following update into the first (MdePkg) patch of this
> series seems to work:
>
>> diff --git a/MdePkg/Include/Register/Amd/Ghcb.h b/MdePkg/Include/Register/Amd/Ghcb.h
>> index 1fce64d5b7cd..3814a79e0619 100644
>> --- a/MdePkg/Include/Register/Amd/Ghcb.h
>> +++ b/MdePkg/Include/Register/Amd/Ghcb.h
>> @@ -111,17 +111,20 @@ typedef PACKED struct {
>> UINT32 GhcbUsage;
>> } GHCB;
>>
>> +#define GHCB_SAVE_AREA_WORD_OFFSET(RegisterField) \
I'll make one minor change here, s/WORD/QWORD/.
>> + (OFFSET_OF (GHCB, SaveArea.RegisterField) / sizeof (UINT64))
>> +
>> 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),
>> + GhcbCpl = GHCB_SAVE_AREA_WORD_OFFSET (Cpl),
>> + GhcbRax = GHCB_SAVE_AREA_WORD_OFFSET (Rax),
>> + GhcbRbx = GHCB_SAVE_AREA_WORD_OFFSET (Rbx),
>> + GhcbRcx = GHCB_SAVE_AREA_WORD_OFFSET (Rcx),
>> + GhcbRdx = GHCB_SAVE_AREA_WORD_OFFSET (Rdx),
>> + GhcbXCr0 = GHCB_SAVE_AREA_WORD_OFFSET (XCr0),
>> + GhcbSwExitCode = GHCB_SAVE_AREA_WORD_OFFSET (SwExitCode),
>> + GhcbSwExitInfo1 = GHCB_SAVE_AREA_WORD_OFFSET (SwExitInfo1),
>> + GhcbSwExitInfo2 = GHCB_SAVE_AREA_WORD_OFFSET (SwExitInfo2),
>> + GhcbSwScratch = GHCB_SAVE_AREA_WORD_OFFSET (SwScratch),
>> } GHCB_REGISTER;
>>
>> typedef union {
>
> I build-tested this update with gcc, and also verified that it passes
> ECC, by running CI locally. (I reproduced the bogus ECC error message
> locally at first, of course, to see if the update makes a difference.)
>
>> PROGRESS - --Running MdePkg: EccCheck Test NO-TARGET --
>> PROGRESS - --->Test Success: EccCheck Test NO-TARGET
>
> If it works for you as well (check with a personal build PR perhaps),
Yup, that works, all tests passed.
> then I suggest posting v4 -- Liming should please re-check the udpated
> MdePkg patch, and then we can merge the series.
Ok, I'll submit a v4 and without Liming's Reviewed-by on the first patch.
Thanks,
Tom
>
> Thanks!
> Laszlo
>
^ permalink raw reply [flat|nested] 25+ messages in thread