Re: [edk2-devel] [PATCH 06/13] MdePkg/Register/Amd: define GHCB macros for SNP AP creation

["Laszlo Ersek" <lersek@redhat.com>]

On 05/11/21 17:43, Tom Lendacky wrote:
> On 5/11/21 4:59 AM, Laszlo Ersek wrote:
>> On 05/07/21 22:38, Brijesh Singh wrote:
>>> From: Tom Lendacky <thomas.lendacky@amd.com>
>>>
>>> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3275&amp;data=04%7C01%7Cthomas.lendacky%40amd.com%7C92c1323bd1e84a2a38e208d914636ddf%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637563239563579592%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=DMDhcseilROSsL6EISUoT9p0pI%2BmXtEC3rLHIQS4NmI%3D&amp;reserved=0
>>>
>>> Version 2 of GHCB introduces NAE for creating AP when SEV-SNP is
>>> enabled in the guest VM. See the GHCB spec section for additional
>>> details.
>>
>> (1) The actual section reference is missing. I'll fix it up: from where
>> the spec introduces exit code 0x8000_0013, the sections appear to be
>> 4.1.9 and 4.3.2. Also, Table 5. "List of Supported Non-Automatic Events"
>> is relevant for the SVM_VMGEXIT_SNP_AP_* macros.
> 
> There are some needed changes to this patch, so I can fix that up. I just
> avoided putting actual section numbers in there because it is possible
> that they can change in future versions.

As long as AMD keeps older revisions of the spec available for download,
I think it's fine to include precise references (covering the spec
revision number too).

>>> +#define SEV_ES_RESET_CS_ATTRIBUTES    (BIT7 | BIT4 | BIT3 | BIT1)
>>> +#define SEV_ES_RESET_DS_ATTRIBUTES    (BIT7 | BIT4 | BIT1)
>>> +#define SEV_ES_RESET_ES_ATTRIBUTES    SEV_ES_RESET_DS_ATTRIBUTES
>>> +#define SEV_ES_RESET_FS_ATTRIBUTES    SEV_ES_RESET_DS_ATTRIBUTES
>>> +#define SEV_ES_RESET_GS_ATTRIBUTES    SEV_ES_RESET_DS_ATTRIBUTES
>>> +#define SEV_ES_RESET_SS_ATTRIBUTES    SEV_ES_RESET_DS_ATTRIBUTES
>>> +
>>> +#define SEV_ES_RESET_GDTR_ATTRIBUTES  0
>>> +#define SEV_ES_RESET_LDTR_ATTRIBUTES  (BIT7 | 2)
>>> +#define SEV_ES_RESET_IDTR_ATTRIBUTES  0
>>> +#define SEV_ES_RESET_TR_ATTRIBUTES    (BIT7 | 3)
>>
>> (4) ... I guess I can't go ahead merging this myself, after all (Liming
>> may of course still merge the MdePkg patches, if he wants to).
>>
>> My problem here is that the bit positions are cryptic.
>>
>> I've found the *normal* (not SEV-ES) segment descriptor attributes in
>> the AMD APM (publication #24593, revision 3.37, date March 2021, volume
>> 2, sections sections 4.7 and 4.8).
>>
>> However, the bit positions SEV-ES descriptors are surely different. For
>> the "normal" segment descriptors, we already have the
>> IA32_SEGMENT_DESCRIPTOR type in edk2, with the nicely broken-out
>> attribute bits. The bit meanings within
>> "SEV_ES_SEGMENT_REGISTER.Attributes" remain unclear to me.
>>
>> Please at least provide a *specific* documentation reference in the
>> commit message where I can verify (or at least "decode") the attribute bits.
> 
> Yeah, it is a strange format. The format is documented in sections 15.5
> (VMRUN Instruction) and 10 (System-Management Mode).
> 
> I can try to further document the bit assignments, e.g.
> 
> #define SEV_ES_SEGMENT_ATTRIBUTE_PRESENT	BIT7
> #define SEV_ES_SEGMENT_ATTRIBUTE_USER		BIT4
> etc.

If it's not a big burden, can you please do both? I.e., (a) include the
spec reference(s) in the commit message, and (b) introduce either
bit-fields, or symbolic names (macros), for the relevant bits?

Thanks!
Laszlo