Re: [PATCH v12 07/46] MdePkg/BaseLib: Add support for the VMGEXIT instruction

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

Laszlo:
 Yes. This solution makes sense. 

Thanks
Liming
-----Original Message-----
From: Laszlo Ersek <lersek@redhat.com> 
Sent: 2020年7月28日 23:19
To: Tom Lendacky <thomas.lendacky@amd.com>; Gao, Liming <liming.gao@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: [PATCH v12 07/46] MdePkg/BaseLib: Add support for the VMGEXIT instruction

On 07/28/20 16:13, Tom Lendacky wrote:

> IIUC, create a VmgExit.c file in MdePkg/Library/BaseLib/Ia32/ that 
> doesn't actually encode the VMGEXIT instruction, just calls 
> CpuBreakpoint(), e.g.:
>
>   VOID
>   EFIAPI
>   AsmVmgExit (
>     VOID
>     )
>   {
>     CpuBreakpoint();
>   }
>

Yes -- either that, or even just open-code (copy) what we have in

  MdePkg/Library/BaseLib/Ia32/CpuBreakpoint.nasm

now:

global ASM_PFX(AsmVmgExit)
ASM_PFX(AsmVmgExit):
;
; VMGEXIT makes no sense on IA32, and NASM versions before 2.12 cannot ; translate the 64-bit "rep vmmcall" instruction into elf32 format. So ; provide a stub implementation that is identical to CpuBreakpoint(). In ; practice, AsmVmgExit() should never be called on IA32.
;
    int  3
    ret

Because this assembly-language implementation might be more "true" to the name "AsmVmgExit".

Liming, would you be OK with this approach? In other words, the set of files changed/introduced in this patch would not change, just the implementation of IA32 AsmVmgExit().

> The other alternative is to use a DB-encoded instruction, though I 
> know that isn't the most popular approach.

Right, I've been quite on a quest to eliminate DBs that encode instructions.

> The BITS 64 method to generate the instruction bytes is also used in 
> OvmfPkg/ResetVector/Ia32/PageTables64.asm, but that file is only 
> included when ARCH_X64 is defined, so there shouldn't be an issue 
> there, plus the nasm file format is bin (-f bin).

I confirm that; the following commands all work on RHEL7, with this series applied:

$ build -b NOOPT -t GCC48 -m OvmfPkg/ResetVector/ResetVector.inf -a IA32        -p OvmfPkg/OvmfPkgIa32.dsc
$ build -b NOOPT -t GCC48 -m OvmfPkg/ResetVector/ResetVector.inf -a IA32 -a X64 -p OvmfPkg/OvmfPkgIa32X64.dsc
$ build -b NOOPT -t GCC48 -m OvmfPkg/ResetVector/ResetVector.inf         -a X64 -p OvmfPkg/OvmfPkgX64.dsc

Thanks!
Laszlo