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

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

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. 

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
> >>>
> >>>
> >>> 
> >>
> >