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

["Liming Gao" <liming.gao@intel.com>]

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.

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://bugzilla.tianocore.org/show_bug.cgi?id=2198
>>
>> 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
>>
>>
>> 
>