Re: [edk2-devel] [PATCH v11 06/46] MdePkg/BaseLib: Add support for the XGETBV instruction

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

On 7/23/20 9:59 AM, Gao, Liming wrote:
> Tom:
> 
>> -----Original Message-----
>> From: Tom Lendacky <thomas.lendacky@amd.com>
>> Sent: Thursday, July 23, 2020 10:19 PM
>> To: Gao, Liming <liming.gao@intel.com>; Laszlo Ersek <lersek@redhat.com>; Liu, Zhiguang <zhiguang.liu@intel.com>;
>> devel@edk2.groups.io
>> Cc: Brijesh Singh <brijesh.singh@amd.com>; Ard Biesheuvel <ard.biesheuvel@arm.com>; Dong, Eric <eric.dong@intel.com>; Justen,
>> Jordan L <jordan.l.justen@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Ni, Ray <ray.ni@intel.com>
>> Subject: Re: [edk2-devel] [PATCH v11 06/46] MdePkg/BaseLib: Add support for the XGETBV instruction
>>
>> On 7/22/20 8:16 PM, Gao, Liming wrote:
>>> Laszlo:
>>>
>>> -----Original Message-----
>>> From: Laszlo Ersek <lersek@redhat.com>
>>> Sent: 2020年7月23日 4:28
>>> To: Liu, Zhiguang <zhiguang.liu@intel.com>; devel@edk2.groups.io; thomas.lendacky@amd.com
>>> Cc: Brijesh Singh <brijesh.singh@amd.com>; Ard Biesheuvel <ard.biesheuvel@arm.com>; Dong, Eric <eric.dong@intel.com>; Justen,
>> Jordan L <jordan.l.justen@intel.com>; Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Ni, Ray
>> <ray.ni@intel.com>
>>> Subject: Re: [edk2-devel] [PATCH v11 06/46] MdePkg/BaseLib: Add support for the XGETBV instruction
>>>
>>> On 07/22/20 02:55, Liu, Zhiguang wrote:
>>>> Hi Tom,
>>>> Nasm is a cross-OS assembly code and can be used in Linux.
>>>> So I think we don't need implement the same function in GccInline.c, we can just use the nasm file in Linux.
>>>
>>> I could agree, but this would create an inconsistency with the existent functions (where both gcc inline assembly and NASM exists).
>>> [Liming] Yes. This is clean up task to make the existing ones be consistent. The new one X86 assembly function (IA32 and X64)
>> should follow nasm style.
>>
>> I was following what I thought was convention. If you'd like, I can remove
>> the "| MSFT" from the .nasm entries in the .inf file and delete (not add)
>> the functions in the GccInline.c file.
>>
>> Let me know what you would like done.
> 
> Yes. This is my suggestion. Only add nasm style, don't touch GccInline.c file.

Ok, I'll  update the patches and send out a new version soon. I'll wait a 
day or two to see if there is any further feedback before sending.

Thanks,
Tom

> 
> Thanks
> Liming
>>
>> Thanks,
>> Tom
>>
>>>
>>> For example, consider AsmReadEflags():
>>> - inline assembly for MSFT IA32 ("Ia32/ReadEflags.c")
>>> - NASM for MSFT X64 ("X64/ReadEflags.nasm")
>>> - inline assembly for GCC IA32 ("Ia32/GccInline.c")
>>> - inline assembly for GCC X64 ("X64/GccInline.c")
>>>
>>> The source file "X64/ReadEflags.nasm" could be used with GCC X64 too, not just with MSFT X64.
>>>
>>> So why do we have the gcc inline implementation for AsmReadEflags() in "X64/GccInline.c", in the first place?
>>> [Liming] This is the history. Nasm migration replaces .S and .asm. But, the remaining one in C source is not replaced.
>>>
>>> Thanks
>>> Liming
>>> The pattern that a contributor is supposed to follow is not clear to me.
>>>
>>> Thanks,
>>> Laszlo
>>>
>>>>> -----Original Message-----
>>>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
>>>>> Lendacky, Thomas
>>>>> Sent: Wednesday, July 22, 2020 5:19 AM
>>>>> To: devel@edk2.groups.io
>>>>> Cc: Brijesh Singh <brijesh.singh@amd.com>; Ard Biesheuvel
>>>>> <ard.biesheuvel@arm.com>; Dong, Eric <eric.dong@intel.com>; Justen,
>>>>> Jordan L <jordan.l.justen@intel.com>; Laszlo Ersek
>>>>> <lersek@redhat.com>; Gao, Liming <liming.gao@intel.com>; Kinney,
>>>>> Michael D <michael.d.kinney@intel.com>; Ni, Ray <ray.ni@intel.com>
>>>>> Subject: [edk2-devel] [PATCH v11 06/46] MdePkg/BaseLib: Add support
>>>>> for the XGETBV instruction
>>>>>
>>>>> 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%3D2198&amp;d
>> ata=02%7C01%7Cthomas.lendacky%40amd.com%7C7cbbd5ae7ded4963ad1e08d82ea5ff27%7C3dd8961fe4884e608e11a82d994e183d%
>> 7C0%7C0%7C637310637778599307&amp;sdata=GPutAlSGucRGFnjU4rxWXfeiy4fJhZHHZe5YJ8hhPSQ%3D&amp;reserved=0
>>>>>
>>>>> Under SEV-ES, a CPUID instruction requires the current value of the
>>>>> XCR0 register. In order to retrieve that value, the XGETBV
>>>>> instruction needs to be executed.
>>>>>
>>>>> Provide the necessary support to execute the XGETBV instruction.
>>>>>
>>>>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>>>>> Cc: Liming Gao <liming.gao@intel.com>
>>>>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>>>>> ---
>>>>>    MdePkg/Library/BaseLib/BaseLib.inf      |  2 ++
>>>>>    MdePkg/Include/Library/BaseLib.h        | 17 +++++++++++++
>>>>>    MdePkg/Library/BaseLib/Ia32/GccInline.c | 28 ++++++++++++++++++++
>>>>> MdePkg/Library/BaseLib/X64/GccInline.c  | 30 ++++++++++++++++++++++
>>>>> MdePkg/Library/BaseLib/Ia32/XGetBv.nasm | 31
>>>>> ++++++++++++++++++++++
>>>>>    MdePkg/Library/BaseLib/X64/XGetBv.nasm  | 34
>>>>> +++++++++++++++++++++++++
>>>>>    6 files changed, 142 insertions(+)
>>>>>    create mode 100644 MdePkg/Library/BaseLib/Ia32/XGetBv.nasm
>>>>>    create mode 100644 MdePkg/Library/BaseLib/X64/XGetBv.nasm
>>>>>
>>>>> diff --git a/MdePkg/Library/BaseLib/BaseLib.inf
>>>>> b/MdePkg/Library/BaseLib/BaseLib.inf
>>>>> index c740a819cacf..e26c0d8cb0ac 100644
>>>>> --- a/MdePkg/Library/BaseLib/BaseLib.inf
>>>>> +++ b/MdePkg/Library/BaseLib/BaseLib.inf
>>>>> @@ -153,6 +153,7 @@ [Sources.Ia32]
>>>>>      Ia32/ARShiftU64.c | MSFT
>>>>>      Ia32/EnableCache.c | MSFT
>>>>>      Ia32/DisableCache.c | MSFT
>>>>> +  Ia32/XGetBv.nasm | MSFT
>>>>>
>>>>>
>>>>>      Ia32/GccInline.c | GCC
>>>>> @@ -288,6 +289,7 @@ [Sources.X64]
>>>>>      X64/ReadCr2.nasm| MSFT
>>>>>      X64/ReadCr0.nasm| MSFT
>>>>>      X64/ReadEflags.nasm| MSFT
>>>>> +  X64/XGetBv.nasm | MSFT
>>>>>
>>>>>
>>>>>      X64/Non-existing.c
>>>>> diff --git a/MdePkg/Include/Library/BaseLib.h
>>>>> b/MdePkg/Include/Library/BaseLib.h
>>>>> index 8e7b87cbda4e..7edf0051a0a0 100644
>>>>> --- a/MdePkg/Include/Library/BaseLib.h
>>>>> +++ b/MdePkg/Include/Library/BaseLib.h
>>>>> @@ -7831,6 +7831,23 @@ AsmLfence (
>>>>>      VOID
>>>>>      );
>>>>>
>>>>> +/**
>>>>> +  Executes a XGETBV instruction
>>>>> +
>>>>> +  Executes a XGETBV instruction. This function is only available on
>>>>> + IA-32 and  x64.
>>>>> +
>>>>> +  @param[in] Index        Extended control register index
>>>>> +
>>>>> +  @return                 The current value of the extended control register
>>>>> +**/
>>>>> +UINT64
>>>>> +EFIAPI
>>>>> +AsmXGetBv (
>>>>> +  IN UINT32  Index
>>>>> +  );
>>>>> +
>>>>> +
>>>>>    /**
>>>>>      Patch the immediate operand of an IA32 or X64 instruction such
>>>>> that the byte,
>>>>>      word, dword or qword operand is encoded at the end of the
>>>>> instruction's diff --git a/MdePkg/Library/BaseLib/Ia32/GccInline.c
>>>>> b/MdePkg/Library/BaseLib/Ia32/GccInline.c
>>>>> index 6ed938187a08..c2565ab9a183 100644
>>>>> --- a/MdePkg/Library/BaseLib/Ia32/GccInline.c
>>>>> +++ b/MdePkg/Library/BaseLib/Ia32/GccInline.c
>>>>> @@ -584,3 +584,31 @@ AsmReadTsc (
>>>>>
>>>>>      return Data;
>>>>>    }
>>>>> +
>>>>> +
>>>>> +/**
>>>>> +  Executes a XGETBV instruction
>>>>> +
>>>>> +  Executes a XGETBV instruction. This function is only available on
>>>>> + IA-32 and  x64.
>>>>> +
>>>>> +  @param[in] Index        Extended control register index
>>>>> +
>>>>> +  @return                 The current value of the extended control register
>>>>> +**/
>>>>> +UINT64
>>>>> +EFIAPI
>>>>> +AsmXGetBv (
>>>>> +  IN UINT32 Index
>>>>> +  )
>>>>> +{
>>>>> +  UINT64 Data;
>>>>> +
>>>>> +  __asm__ __volatile__ (
>>>>> +    "xgetbv"
>>>>> +    : "=A" (Data)
>>>>> +    : "c"  (Index)
>>>>> +    );
>>>>> +
>>>>> +  return Data;
>>>>> +}
>>>>> diff --git a/MdePkg/Library/BaseLib/X64/GccInline.c
>>>>> b/MdePkg/Library/BaseLib/X64/GccInline.c
>>>>> index 40a208f1985f..65f864e35922 100644
>>>>> --- a/MdePkg/Library/BaseLib/X64/GccInline.c
>>>>> +++ b/MdePkg/Library/BaseLib/X64/GccInline.c
>>>>> @@ -560,3 +560,33 @@ AsmReadTsc (
>>>>>
>>>>>      return (((UINT64)HiData) << 32) | LowData;  }
>>>>> +
>>>>> +
>>>>> +/**
>>>>> +  Executes a XGETBV instruction
>>>>> +
>>>>> +  Executes a XGETBV instruction. This function is only available on
>>>>> + IA-32 and  x64.
>>>>> +
>>>>> +  @param[in] Index        Extended control register index
>>>>> +
>>>>> +  @return                 The current value of the extended control register
>>>>> +**/
>>>>> +UINT64
>>>>> +EFIAPI
>>>>> +AsmXGetBv (
>>>>> +  IN UINT32 Index
>>>>> +  )
>>>>> +{
>>>>> +  UINT32 LowData;
>>>>> +  UINT32 HighData;
>>>>> +
>>>>> +  __asm__ __volatile__ (
>>>>> +    "xgetbv"
>>>>> +    : "=a" (LowData),
>>>>> +      "=d" (HighData)
>>>>> +    : "c"  (Index)
>>>>> +    );
>>>>> +
>>>>> +  return (((UINT64)HighData) << 32) | LowData; }
>>>>> diff --git a/MdePkg/Library/BaseLib/Ia32/XGetBv.nasm
>>>>> b/MdePkg/Library/BaseLib/Ia32/XGetBv.nasm
>>>>> new file mode 100644
>>>>> index 000000000000..9f7b03bbff35
>>>>> --- /dev/null
>>>>> +++ b/MdePkg/Library/BaseLib/Ia32/XGetBv.nasm
>>>>> @@ -0,0 +1,31 @@
>>>>> +;-------------------------------------------------------------------
>>>>> +-----------
>>>>> +;
>>>>> +; Copyright (C) 2020, Advanced Micro Devices, Inc. All rights
>>>>> +reserved.<BR> ; SPDX-License-Identifier: BSD-2-Clause-Patent ; ;
>>>>> +Module Name:
>>>>> +;
>>>>> +;   XGetBv.Asm
>>>>> +;
>>>>> +; Abstract:
>>>>> +;
>>>>> +;   AsmXgetBv function
>>>>> +;
>>>>> +; Notes:
>>>>> +;
>>>>> +;-------------------------------------------------------------------
>>>>> +-----------
>>>>> +
>>>>> +    SECTION .text
>>>>> +
>>>>> +;-------------------------------------------------------------------
>>>>> +-----------
>>>>> +; UINT64
>>>>> +; EFIAPI
>>>>> +; AsmXGetBv (
>>>>> +;   IN UINT32  Index
>>>>> +;   );
>>>>> +;-------------------------------------------------------------------
>>>>> +-----------
>>>>> +global ASM_PFX(AsmXGetBv)
>>>>> +ASM_PFX(AsmXGetBv):
>>>>> +    mov     ecx, [esp + 4]
>>>>> +    xgetbv
>>>>> +    ret
>>>>> diff --git a/MdePkg/Library/BaseLib/X64/XGetBv.nasm
>>>>> b/MdePkg/Library/BaseLib/X64/XGetBv.nasm
>>>>> new file mode 100644
>>>>> index 000000000000..09f3be8ae0a8
>>>>> --- /dev/null
>>>>> +++ b/MdePkg/Library/BaseLib/X64/XGetBv.nasm
>>>>> @@ -0,0 +1,34 @@
>>>>> +;-------------------------------------------------------------------
>>>>> +-----------
>>>>> +;
>>>>> +; Copyright (C) 2020, Advanced Micro Devices, Inc. All rights
>>>>> +reserved.<BR> ; SPDX-License-Identifier: BSD-2-Clause-Patent ; ;
>>>>> +Module Name:
>>>>> +;
>>>>> +;   XGetBv.Asm
>>>>> +;
>>>>> +; Abstract:
>>>>> +;
>>>>> +;   AsmXgetBv function
>>>>> +;
>>>>> +; Notes:
>>>>> +;
>>>>> +;-------------------------------------------------------------------
>>>>> +-----------
>>>>> +
>>>>> +    DEFAULT REL
>>>>> +    SECTION .text
>>>>> +
>>>>> +;-------------------------------------------------------------------
>>>>> +-----------
>>>>> +; UINT64
>>>>> +; EFIAPI
>>>>> +; AsmXGetBv (
>>>>> +;   IN UINT32  Index
>>>>> +;   );
>>>>> +;-------------------------------------------------------------------
>>>>> +-----------
>>>>> +global ASM_PFX(AsmXGetBv)
>>>>> +ASM_PFX(AsmXGetBv):
>>>>> +    xgetbv
>>>>> +    shl     rdx, 32
>>>>> +    or      rax, rdx
>>>>> +    ret
>>>>> +
>>>>> --
>>>>> 2.27.0
>>>>>
>>>>>
>>>>> 
>>>>
>>>