Re: [edk2-devel] [PATCH v3 00/11] SEV-ES guest support fixes and cleanup

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

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
>