Re: [edk2-devel] [PATCH v9 08/46] UefiCpuPkg: Implement library support for VMGEXIT

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

On 7/7/20 12:11 PM, Tom Lendacky wrote:
> On 7/7/20 10:50 AM, Tom Lendacky wrote:
>> On 7/7/20 10:36 AM, Laszlo Ersek wrote:
>>> On 07/06/20 22:03, Tom Lendacky wrote:
>>>> On 7/2/20 2:04 AM, Dong, Eric wrote:
>>>>> Hi Tom,
>>>>
>>>> Hi Eric,
>>>>
>>>>>
>>>>> We have root cause this Mac file format issue. The patch mail from your side include extra two "=0D=0D" , and our test tool convert them to "\r\r". This is Mac file line ending format. So this issue been reported. We have updated our tool to handle this special case.
>>>>
>>>> Good to know, thanks!
>>>>
>>>>>
>>>>> With that change, now I met below error when use VS2015 tool chain. Can you help to fix it?
>>>>>
>>>>> Building ... g:\edk2-open-source\edk2\MdePkg\Library\PeiCoreEntryPoint\PeiCoreEntryPoint.inf [X64]
>>>>> PeCoffLoaderEx.c
>>>>> g:\edk2-open-source\edk2\OvmfPkg\Library\VmgExitLib\VmgExitVcHandler.c(386): warning C4334: '<<': result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?)
>>>>> NMAKE : fatal error U1077: '"C:\Program Files (x86)\Microsoft Visual Studio 14.0\Vc\bin\x86_amd64\cl.exe"' : return code '0x2'
>>>
>>> This is for the line
>>>
>>>       Displacement *= (1 << Ext->Sib.Scale);
>>>
>>> from
>>>
>>>   [edk2-devel] [PATCH v9 17/46]
>>>   OvmfPkg/VmgExitLib: Add support for NPF NAE events (MMIO)
>>>
>>>>
>>>> Yup, looks like that needs to be a "1ULL <<" instead of "1 <<".
>>>> I have verified that fixes the issue.
>>>
>>> I disagree.
>>>
>>> At that point, Displacement is of type INT64, and it may well be a
>>> negative value. We definitely want to multiply it by a signed int
>>> (values 1, 2, 4, 8).
>>>
>>> I commented on this before. Please see:
>>>
>>> (i) my comment block (10) here -- especially comment (10c):
>>>
>>>   https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2Fmessage%2F60144&amp;data=02%7C01%7Cthomas.lendacky%40amd.com%7Cec0cb2ad96694b66d8ff08d8228b7c8e%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637297329772337705&amp;sdata=g%2BGooY1Sv0G7ydr11Jh%2BTXxo4Wy6ZWcT5Mq9VmWddi8%3D&amp;reserved=0
>>>
>>> (alternative link:
>>> <https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fmid.mail-archive.com%2F169e44cb-2c1c-6d9a-342a-2a1f618e3753%40redhat.com&amp;data=02%7C01%7Cthomas.lendacky%40amd.com%7Cec0cb2ad96694b66d8ff08d8228b7c8e%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637297329772337705&amp;sdata=6p91db%2F6oz%2FHc65Sq4fvH%2FcPmiAfdS8MImsaznaoaXA%3D&amp;reserved=0>)
>>>
>>> (ii) and my comment here:
>>>
>>>   https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2Fmessage%2F60146&amp;data=02%7C01%7Cthomas.lendacky%40amd.com%7Cec0cb2ad96694b66d8ff08d8228b7c8e%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637297329772337705&amp;sdata=iNIBJCIlfEEsY37cdwUbH27tx5HvXVs3PZiOQfaGeLQ%3D&amp;reserved=0
>>>
>>> (alternative link:
>>> <https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fmid.mail-archive.com%2F139ce789-b938-c8b9-030e-c1b6c67e47ea%40redhat.com&amp;data=02%7C01%7Cthomas.lendacky%40amd.com%7Cec0cb2ad96694b66d8ff08d8228b7c8e%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637297329772337705&amp;sdata=mWCAHqTOpp7B9nWUJjTRJ9VZ74iwdElRTOoNhEpFs%2Bc%3D&amp;reserved=0>).
>>>
>>>
>>> The compiler warning is well-meaning, but unnecessary. A 64-bit shift is
>>> *NOT* intended. We want to end up with one of the signed int (aka INT32)
>>> values 1, 2, 4 or 8. And then multiply the INT64 Displacement with that
>>> value. For the multiplication, the INT32 value 1, 2, 4 or 8 will be
>>> implicitly converted to INT64. That's entirely intentional.
>>>
>>> If we want to suppress the warning, while keeping the logic intact, we
>>> should employ an explicit cast:
>>>
>>>   Displacement *= (INT64)(1 << Ext->Sib.Scale);
>>
>> Ok, that makes sense. I'll use the explicit cast.
>>
>>>
>>>>
>>>> One thing I noticed is that the 32-bit builds
>>>> (PlatformCI_OvmfPkg_Windows_VS2019_PR, Platform_CI OVMF_IA32_NOOPT and
>>>> Platform_CI OVMF_IA32X64_NOOPT) encounter an error:
>>>>
>>>> ERROR - Linker #2001 from SecMain.lib(SecMain.obj) :   unresolved external symbol __allshl
>>>> ERROR - Linker #1120 from d:\a\1\s\Build\Ovmf3264\NOOPT_VS2019\IA32\OvmfPkg\Sec\SecMain\DEBUG\SecMain.dll : fatal   1 unresolved externals
>>>> ERROR - Compiler #1077 from NMAKE : fatal   '"C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC\Tools\MSVC\14.26.28801\bin\Hostx86\x86\link.exe"' : return code '0x460'
>>>>
>>>> Any idea what is causing this error?
>>>
>>> A left-shift operator (<<) applied to a 64-bit operand is somehow
>>> finding its way into the 32-bit SEC build.
>>>
>>> That is indeed wrong (for such cases, we're supposed to use LShiftU64()
>>> from BaseLib).
>>>
>>> What I don't understand however is that all of the "<<" operator uses,
>>> on 64-bit operands, should already be limited to code that is *only*
>>> built for X64!
>>>
>>> For example, with this series applied, SecMain in OVMF consumes
>>> "UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf".
>>> And the latter consumes VmgExitLib.
>>>
>>> But VmgExitLib is resolved to
>>> "UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.inf", in the IA32 and
>>> IA32X64 DSC files. This Null instance contains no left-shifts.
>>>
>>> Therefore any << operators, applied to 64-bit operands, present in
>>> "OvmfPkg/Library/VmgExitLib", should never be compiled for IA32 and IA32X64.
>>>
>>> So I don't know where the problematic "<<" comes from. It does not come
>>> from VmgExitLib, as far as I can tell.
>>
>> Yes, I don't think it's coming from VmgExitLib, either.
>>
>> I wonder if it somehow might be coming from the MSR_SEV_ES_GHCB_REGISTER
>> struct and the bit fields that are used within it? That code, while not
>> executed in non-X64 builds because SEV-ES is not active, is still built
>> and maybe the bit fields result in implicit shifts occurring, specifically
>> in SevEsProtocolFailure()?
>>
>> I'll experiment with some things and see if that is the issue.
> 
> I commented out the setting of the GhcbTerminate fields in the
> SevEsProtocolFailure() routine of OvmfPkg/Sec/SecMain.c and the error
> disappeared. I'll see if changing from using UINT64 to multiple UINT32
> entries fixes the problem, but I wouldn't think that the bit fields
> would/should cause an issue here with 32-bit builds.

Changing the bit fields from UINT64 to UINT32 fixed the error and SEV-ES
support continues to function properly. Since the architecture is little
endian, there was no need to pad out to the full UINT64 size for the
structs (the union takes care of that in general). The change looks like
this:

diff --git a/MdePkg/Include/Register/Amd/Fam17Msr.h b/MdePkg/Include/Register/Amd/Fam17Msr.h
index 466a3143599c..3cbe593868d4 100644
--- a/MdePkg/Include/Register/Amd/Fam17Msr.h
+++ b/MdePkg/Include/Register/Amd/Fam17Msr.h
@@ -28,7 +28,7 @@
 **/
 typedef union {
   struct {
-    UINT64  Function:12;
+    UINT32  Function:12;
   } GhcbInfo;
 
   struct {
@@ -39,9 +39,9 @@ typedef union {
   } GhcbProtocol;
 
   struct {
-    UINT64  Function:12;
-    UINT64  ReasonCodeSet:4;
-    UINT64  ReasonCode:8;
+    UINT32  Function:12;
+    UINT32  ReasonCodeSet:4;
+    UINT32  ReasonCode:8;
   } GhcbTerminate;
 
   VOID    *Ghcb;

Unless there are any concerns, I'll incorporate this change.

Thanks,
Tom

> 
> Thanks,
> Tom
> 
>>
>> Thanks,
>> Tom
>>
>>>
>>> Thanks,
>>> Laszlo
>>>