From: Tom Lendacky <thomas.lendacky@amd.com>
Use OFFSET_OF () and sizeof () to calculate the GHCB register field
offsets instead of hardcoding the values in the GHCB_REGISTER enum.
Rename GHCB_REGISTER to GHCB_QWORD_OFFSET to more appropriately describe
the enum. While redefing the values, only include (and add) fields that
are used per the GHCB specification.
Also, remove the DR7 field from the GHCB_SAVE_AREA structure since it is
not used/defined in the GHCB specification and then rename the reserved
fields as appropriate.
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Zhiguang Liu <zhiguang.liu@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
MdePkg/Include/Register/Amd/Ghcb.h | 48 ++++++++------------
OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c | 4 +-
2 files changed, 20 insertions(+), 32 deletions(-)
diff --git a/MdePkg/Include/Register/Amd/Ghcb.h b/MdePkg/Include/Register/Amd/Ghcb.h
index 54a80da0f6d7..33c7e8939a28 100644
--- a/MdePkg/Include/Register/Amd/Ghcb.h
+++ b/MdePkg/Include/Register/Amd/Ghcb.h
@@ -82,50 +82,25 @@
#define IOIO_SEG_DS (BIT11 | BIT10)
-typedef enum {
- GhcbCpl = 25,
- GhcbRflags = 46,
- GhcbRip,
- GhcbRsp = 59,
- GhcbRax = 63,
- GhcbRcx = 97,
- GhcbRdx,
- GhcbRbx,
- GhcbRbp = 101,
- GhcbRsi,
- GhcbRdi,
- GhcbR8,
- GhcbR9,
- GhcbR10,
- GhcbR11,
- GhcbR12,
- GhcbR13,
- GhcbR14,
- GhcbR15,
- GhcbXCr0 = 125,
-} GHCB_REGISTER;
-
typedef PACKED struct {
UINT8 Reserved1[203];
UINT8 Cpl;
- UINT8 Reserved2[148];
- UINT64 Dr7;
- UINT8 Reserved3[144];
+ UINT8 Reserved2[300];
UINT64 Rax;
- UINT8 Reserved4[264];
+ UINT8 Reserved3[264];
UINT64 Rcx;
UINT64 Rdx;
UINT64 Rbx;
- UINT8 Reserved5[112];
+ UINT8 Reserved4[112];
UINT64 SwExitCode;
UINT64 SwExitInfo1;
UINT64 SwExitInfo2;
UINT64 SwScratch;
- UINT8 Reserved6[56];
+ UINT8 Reserved5[56];
UINT64 XCr0;
UINT8 ValidBitmap[16];
UINT64 X87StateGpa;
- UINT8 Reserved7[1016];
+ UINT8 Reserved6[1016];
} GHCB_SAVE_AREA;
typedef PACKED struct {
@@ -136,6 +111,19 @@ typedef PACKED struct {
UINT32 GhcbUsage;
} GHCB;
+typedef enum {
+ GhcbCpl = OFFSET_OF (GHCB, SaveArea.Cpl) / sizeof (UINT64),
+ GhcbRax = OFFSET_OF (GHCB, SaveArea.Rax) / sizeof (UINT64),
+ GhcbRbx = OFFSET_OF (GHCB, SaveArea.Rbx) / sizeof (UINT64),
+ GhcbRcx = OFFSET_OF (GHCB, SaveArea.Rcx) / sizeof (UINT64),
+ GhcbRdx = OFFSET_OF (GHCB, SaveArea.Rdx) / sizeof (UINT64),
+ GhcbXCr0 = OFFSET_OF (GHCB, SaveArea.XCr0) / sizeof (UINT64),
+ GhcbSwExitCode = OFFSET_OF (GHCB, SaveArea.SwExitCode) / sizeof (UINT64),
+ GhcbSwExitInfo1 = OFFSET_OF (GHCB, SaveArea.SwExitInfo1) / sizeof (UINT64),
+ GhcbSwExitInfo2 = OFFSET_OF (GHCB, SaveArea.SwExitInfo2) / sizeof (UINT64),
+ GhcbSwScratch = OFFSET_OF (GHCB, SaveArea.SwScratch) / sizeof (UINT64),
+} GHCB_QWORD_OFFSET;
+
typedef union {
struct {
UINT32 Lower32Bits;
diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
index 8e42b305e83c..c5484a3f478c 100644
--- a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
+++ b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
@@ -153,7 +153,7 @@ STATIC
BOOLEAN
GhcbIsRegValid (
IN GHCB *Ghcb,
- IN GHCB_REGISTER Reg
+ IN GHCB_QWORD_OFFSET Reg
)
{
UINT32 RegIndex;
@@ -179,7 +179,7 @@ STATIC
VOID
GhcbSetRegValid (
IN OUT GHCB *Ghcb,
- IN GHCB_REGISTER Reg
+ IN GHCB_QWORD_OFFSET Reg
)
{
UINT32 RegIndex;
--
2.28.0
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#66100): https://edk2.groups.io/g/devel/message/66100
Mute This Topic: https://groups.io/mt/77425908/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Hi Tom,
On 10/10/20 18:06, Tom Lendacky wrote:
> From: Tom Lendacky <thomas.lendacky@amd.com>
>
> Use OFFSET_OF () and sizeof () to calculate the GHCB register field
> offsets instead of hardcoding the values in the GHCB_REGISTER enum.
> Rename GHCB_REGISTER to GHCB_QWORD_OFFSET to more appropriately describe
> the enum. While redefing the values, only include (and add) fields that
> are used per the GHCB specification.
>
> Also, remove the DR7 field from the GHCB_SAVE_AREA structure since it is
> not used/defined in the GHCB specification and then rename the reserved
> fields as appropriate.
>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
> MdePkg/Include/Register/Amd/Ghcb.h | 48 ++++++++------------
> OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c | 4 +-
> 2 files changed, 20 insertions(+), 32 deletions(-)
This patch modifies both MdePkg and OvmfPkg. I agree that, as an
exception, this is the right thing to do.
That's because we are not changing the identifiers of the enumeration
constants (such as GhcbCpl). Because those identifiers are declared at
file scope, having both GHCB_REGISTER and GHCB_QWORD_OFFSET declare
(e.g.) GhcbCpl would cause a compilation failure. Therefore we can't
implement this in multiple stages (first introduce GHCB_QWORD_OFFSET,
then remove GHCB_REGISTER separately).
(1) However, the subject line doesn't look correct. It should mention
both MdePkg and OvmfPkg. Also, we're not updating ValidBitmap settings
just yet.
I suggest:
MdePkg, OvmfPkg: clean up GHCB field offsets and save area
>
> diff --git a/MdePkg/Include/Register/Amd/Ghcb.h b/MdePkg/Include/Register/Amd/Ghcb.h
> index 54a80da0f6d7..33c7e8939a28 100644
> --- a/MdePkg/Include/Register/Amd/Ghcb.h
> +++ b/MdePkg/Include/Register/Amd/Ghcb.h
> @@ -82,50 +82,25 @@
> #define IOIO_SEG_DS (BIT11 | BIT10)
>
>
> -typedef enum {
> - GhcbCpl = 25,
> - GhcbRflags = 46,
> - GhcbRip,
> - GhcbRsp = 59,
> - GhcbRax = 63,
> - GhcbRcx = 97,
> - GhcbRdx,
> - GhcbRbx,
> - GhcbRbp = 101,
> - GhcbRsi,
> - GhcbRdi,
> - GhcbR8,
> - GhcbR9,
> - GhcbR10,
> - GhcbR11,
> - GhcbR12,
> - GhcbR13,
> - GhcbR14,
> - GhcbR15,
> - GhcbXCr0 = 125,
> -} GHCB_REGISTER;
> -
> typedef PACKED struct {
> UINT8 Reserved1[203];
> UINT8 Cpl;
> - UINT8 Reserved2[148];
> - UINT64 Dr7;
> - UINT8 Reserved3[144];
> + UINT8 Reserved2[300];
> UINT64 Rax;
> - UINT8 Reserved4[264];
> + UINT8 Reserved3[264];
> UINT64 Rcx;
> UINT64 Rdx;
> UINT64 Rbx;
> - UINT8 Reserved5[112];
> + UINT8 Reserved4[112];
> UINT64 SwExitCode;
> UINT64 SwExitInfo1;
> UINT64 SwExitInfo2;
> UINT64 SwScratch;
> - UINT8 Reserved6[56];
> + UINT8 Reserved5[56];
> UINT64 XCr0;
> UINT8 ValidBitmap[16];
> UINT64 X87StateGpa;
> - UINT8 Reserved7[1016];
> + UINT8 Reserved6[1016];
> } GHCB_SAVE_AREA;
(2) The meaning of a field called "ReservedN" must never change (it must
describe the same offset range in the structure, after introduction). If
we need to change the structure incompatibly, then please remove the old
ReservedN field name altogether. If a new reserved field has to be
introduced, in addition, then please find the largest N used with
Reserved fields in the structure currently, and use (N+1) in the name of
the new field.
This practice makes sure that any (out of tree) code that refers to a
ReservedN field in MdePkg will cleanly break during compilation after
this change. Changing the meaning of a ReservedN field could otherwise
cause misbehavior that could be harder to track down.
I realize this is somewhat unusual -- after all, if someone deliberately
accessed a field called "Reserved", any subsequent breakage is *their*
fault. However, the reason for this practice is that sometimes MdePkg
headers lag behind industry specs (meaning, non-UEFI specs), but drivers
or libs (outside of MdePkg or even edk2) already need to refer to
newly-specified fields. Even edk2 used to have code similar to the example
Reserved3[12] = 1;
Normally I don't advocate accommodating out-of-tree code, but following
this ReservedN scheme isn't hard.
>
> typedef PACKED struct {
> @@ -136,6 +111,19 @@ typedef PACKED struct {
> UINT32 GhcbUsage;
> } GHCB;
>
> +typedef enum {
> + GhcbCpl = OFFSET_OF (GHCB, SaveArea.Cpl) / sizeof (UINT64),
> + GhcbRax = OFFSET_OF (GHCB, SaveArea.Rax) / sizeof (UINT64),
> + GhcbRbx = OFFSET_OF (GHCB, SaveArea.Rbx) / sizeof (UINT64),
> + GhcbRcx = OFFSET_OF (GHCB, SaveArea.Rcx) / sizeof (UINT64),
> + GhcbRdx = OFFSET_OF (GHCB, SaveArea.Rdx) / sizeof (UINT64),
> + GhcbXCr0 = OFFSET_OF (GHCB, SaveArea.XCr0) / sizeof (UINT64),
> + GhcbSwExitCode = OFFSET_OF (GHCB, SaveArea.SwExitCode) / sizeof (UINT64),
> + GhcbSwExitInfo1 = OFFSET_OF (GHCB, SaveArea.SwExitInfo1) / sizeof (UINT64),
> + GhcbSwExitInfo2 = OFFSET_OF (GHCB, SaveArea.SwExitInfo2) / sizeof (UINT64),
> + GhcbSwScratch = OFFSET_OF (GHCB, SaveArea.SwScratch) / sizeof (UINT64),
> +} GHCB_QWORD_OFFSET;
> +
> typedef union {
> struct {
> UINT32 Lower32Bits;
> diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
> index 8e42b305e83c..c5484a3f478c 100644
> --- a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
> +++ b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
> @@ -153,7 +153,7 @@ STATIC
> BOOLEAN
> GhcbIsRegValid (
> IN GHCB *Ghcb,
> - IN GHCB_REGISTER Reg
> + IN GHCB_QWORD_OFFSET Reg
> )
> {
> UINT32 RegIndex;
> @@ -179,7 +179,7 @@ STATIC
> VOID
> GhcbSetRegValid (
> IN OUT GHCB *Ghcb,
> - IN GHCB_REGISTER Reg
> + IN GHCB_QWORD_OFFSET Reg
> )
> {
> UINT32 RegIndex;
>
Thanks,
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#66239): https://edk2.groups.io/g/devel/message/66239
Mute This Topic: https://groups.io/mt/77425908/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 10/15/20 3:40 AM, Laszlo Ersek wrote:
> Hi Tom,
Hi Laszlo,
>
> On 10/10/20 18:06, Tom Lendacky wrote:
>> From: Tom Lendacky <thomas.lendacky@amd.com>
>>
>> Use OFFSET_OF () and sizeof () to calculate the GHCB register field
>> offsets instead of hardcoding the values in the GHCB_REGISTER enum.
>> Rename GHCB_REGISTER to GHCB_QWORD_OFFSET to more appropriately describe
>> the enum. While redefing the values, only include (and add) fields that
>> are used per the GHCB specification.
>>
>> Also, remove the DR7 field from the GHCB_SAVE_AREA structure since it is
>> not used/defined in the GHCB specification and then rename the reserved
>> fields as appropriate.
>>
>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>> Cc: Liming Gao <gaoliming@byosoft.com.cn>
>> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>> Cc: Brijesh Singh <brijesh.singh@amd.com>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>> ---
>> MdePkg/Include/Register/Amd/Ghcb.h | 48 ++++++++------------
>> OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c | 4 +-
>> 2 files changed, 20 insertions(+), 32 deletions(-)
>
> This patch modifies both MdePkg and OvmfPkg. I agree that, as an
> exception, this is the right thing to do.
>
> That's because we are not changing the identifiers of the enumeration
> constants (such as GhcbCpl). Because those identifiers are declared at
> file scope, having both GHCB_REGISTER and GHCB_QWORD_OFFSET declare
> (e.g.) GhcbCpl would cause a compilation failure. Therefore we can't
> implement this in multiple stages (first introduce GHCB_QWORD_OFFSET,
> then remove GHCB_REGISTER separately).
>
> (1) However, the subject line doesn't look correct. It should mention
> both MdePkg and OvmfPkg. Also, we're not updating ValidBitmap settings
> just yet.
>
> I suggest:
>
> MdePkg, OvmfPkg: clean up GHCB field offsets and save area
Yup, I'll fix this up.
>
>>
>> diff --git a/MdePkg/Include/Register/Amd/Ghcb.h b/MdePkg/Include/Register/Amd/Ghcb.h
>> index 54a80da0f6d7..33c7e8939a28 100644
>> --- a/MdePkg/Include/Register/Amd/Ghcb.h
>> +++ b/MdePkg/Include/Register/Amd/Ghcb.h
>> @@ -82,50 +82,25 @@
>> #define IOIO_SEG_DS (BIT11 | BIT10)
>>
>>
>> -typedef enum {
>> - GhcbCpl = 25,
>> - GhcbRflags = 46,
>> - GhcbRip,
>> - GhcbRsp = 59,
>> - GhcbRax = 63,
>> - GhcbRcx = 97,
>> - GhcbRdx,
>> - GhcbRbx,
>> - GhcbRbp = 101,
>> - GhcbRsi,
>> - GhcbRdi,
>> - GhcbR8,
>> - GhcbR9,
>> - GhcbR10,
>> - GhcbR11,
>> - GhcbR12,
>> - GhcbR13,
>> - GhcbR14,
>> - GhcbR15,
>> - GhcbXCr0 = 125,
>> -} GHCB_REGISTER;
>> -
>> typedef PACKED struct {
>> UINT8 Reserved1[203];
>> UINT8 Cpl;
>> - UINT8 Reserved2[148];
>> - UINT64 Dr7;
>> - UINT8 Reserved3[144];
>> + UINT8 Reserved2[300];
>> UINT64 Rax;
>> - UINT8 Reserved4[264];
>> + UINT8 Reserved3[264];
>> UINT64 Rcx;
>> UINT64 Rdx;
>> UINT64 Rbx;
>> - UINT8 Reserved5[112];
>> + UINT8 Reserved4[112];
>> UINT64 SwExitCode;
>> UINT64 SwExitInfo1;
>> UINT64 SwExitInfo2;
>> UINT64 SwScratch;
>> - UINT8 Reserved6[56];
>> + UINT8 Reserved5[56];
>> UINT64 XCr0;
>> UINT8 ValidBitmap[16];
>> UINT64 X87StateGpa;
>> - UINT8 Reserved7[1016];
>> + UINT8 Reserved6[1016];
>> } GHCB_SAVE_AREA;
>
> (2) The meaning of a field called "ReservedN" must never change (it must
> describe the same offset range in the structure, after introduction). If
> we need to change the structure incompatibly, then please remove the old
> ReservedN field name altogether. If a new reserved field has to be
> introduced, in addition, then please find the largest N used with
> Reserved fields in the structure currently, and use (N+1) in the name of
> the new field.
>
> This practice makes sure that any (out of tree) code that refers to a
> ReservedN field in MdePkg will cleanly break during compilation after
> this change. Changing the meaning of a ReservedN field could otherwise
> cause misbehavior that could be harder to track down.
>
> I realize this is somewhat unusual -- after all, if someone deliberately
> accessed a field called "Reserved", any subsequent breakage is *their*
> fault. However, the reason for this practice is that sometimes MdePkg
> headers lag behind industry specs (meaning, non-UEFI specs), but drivers
> or libs (outside of MdePkg or even edk2) already need to refer to
> newly-specified fields. Even edk2 used to have code similar to the example
>
> Reserved3[12] = 1;
>
> Normally I don't advocate accommodating out-of-tree code, but following
> this ReservedN scheme isn't hard.
Ok, will do. I'll change the old Reserved2/Dr7/Reserved3 fields to a new
combined Reserved8 field.
Thanks,
Tom
>
>
>>
>> typedef PACKED struct {
>> @@ -136,6 +111,19 @@ typedef PACKED struct {
>> UINT32 GhcbUsage;
>> } GHCB;
>>
>> +typedef enum {
>> + GhcbCpl = OFFSET_OF (GHCB, SaveArea.Cpl) / sizeof (UINT64),
>> + GhcbRax = OFFSET_OF (GHCB, SaveArea.Rax) / sizeof (UINT64),
>> + GhcbRbx = OFFSET_OF (GHCB, SaveArea.Rbx) / sizeof (UINT64),
>> + GhcbRcx = OFFSET_OF (GHCB, SaveArea.Rcx) / sizeof (UINT64),
>> + GhcbRdx = OFFSET_OF (GHCB, SaveArea.Rdx) / sizeof (UINT64),
>> + GhcbXCr0 = OFFSET_OF (GHCB, SaveArea.XCr0) / sizeof (UINT64),
>> + GhcbSwExitCode = OFFSET_OF (GHCB, SaveArea.SwExitCode) / sizeof (UINT64),
>> + GhcbSwExitInfo1 = OFFSET_OF (GHCB, SaveArea.SwExitInfo1) / sizeof (UINT64),
>> + GhcbSwExitInfo2 = OFFSET_OF (GHCB, SaveArea.SwExitInfo2) / sizeof (UINT64),
>> + GhcbSwScratch = OFFSET_OF (GHCB, SaveArea.SwScratch) / sizeof (UINT64),
>> +} GHCB_QWORD_OFFSET;
>> +
>> typedef union {
>> struct {
>> UINT32 Lower32Bits;
>> diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
>> index 8e42b305e83c..c5484a3f478c 100644
>> --- a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
>> +++ b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
>> @@ -153,7 +153,7 @@ STATIC
>> BOOLEAN
>> GhcbIsRegValid (
>> IN GHCB *Ghcb,
>> - IN GHCB_REGISTER Reg
>> + IN GHCB_QWORD_OFFSET Reg
>> )
>> {
>> UINT32 RegIndex;
>> @@ -179,7 +179,7 @@ STATIC
>> VOID
>> GhcbSetRegValid (
>> IN OUT GHCB *Ghcb,
>> - IN GHCB_REGISTER Reg
>> + IN GHCB_QWORD_OFFSET Reg
>> )
>> {
>> UINT32 RegIndex;
>>
>
> Thanks,
> Laszlo
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#66256): https://edk2.groups.io/g/devel/message/66256
Mute This Topic: https://groups.io/mt/77425908/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2026 Red Hat, Inc.