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

["Lendacky, Thomas" <thomas.lendacky@amd.com>]

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
>