From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.groups.io with SMTP id smtpd.web08.10877.1620729003056570363 for ; Tue, 11 May 2021 03:30:03 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=WY8IdPwg; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1620729002; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=HyWQMSaIfXrDiPeKZ7fpsWyUDxerO4x8c9F6vAUus0E=; b=WY8IdPwgG6pDFfxYhORi8wmL8HgyZQpoILphaxtcvfgWlk4j0iAG4swe/i7kDw7usGH+/3 Iroz9j3j74b4DQ2oIp8XyMPTScBB743M642TLJM9hi/xldasbYg9ciR8igoBp8ThGLhb6w 3xzRPP5CnftpuR8Xj9AItfQ8heOruY4= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-11-5hQqsw0HN9yk08zFk1ntaA-1; Tue, 11 May 2021 06:29:56 -0400 X-MC-Unique: 5hQqsw0HN9yk08zFk1ntaA-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 21FFF6D581; Tue, 11 May 2021 10:29:54 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-233.ams2.redhat.com [10.36.112.233]) by smtp.corp.redhat.com (Postfix) with ESMTP id A076FE2DA; Tue, 11 May 2021 10:29:50 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 07/13] MdePkg/BaseLib: add support for PVALIDATE instruction To: devel@edk2.groups.io, brijesh.singh@amd.com Cc: James Bottomley , Min Xu , Jiewen Yao , Tom Lendacky , Jordan Justen , Ard Biesheuvel , Erdem Aktas , Michael D Kinney , Liming Gao , Zhiguang Liu References: <20210507203838.23706-1-brijesh.singh@amd.com> <20210507203838.23706-8-brijesh.singh@amd.com> From: "Laszlo Ersek" Message-ID: Date: Tue, 11 May 2021 12:29:49 +0200 MIME-Version: 1.0 In-Reply-To: <20210507203838.23706-8-brijesh.singh@amd.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 05/07/21 22:38, Brijesh Singh wrote: > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275 > > The PVALIDATE instruction validates or rescinds validation of a guest > page RMP entry. Upon completion, a return code is stored in EAX, rFLAGS > bits OF, ZF, AF, PF and SF are set based on this return code. If the > instruction completed succesfully, the rFLAGS bit CF indicates if the > contents of the RMP entry were changed or not. > > For more information about the instruction see AMD APM volume 3. > > Cc: James Bottomley > Cc: Min Xu > Cc: Jiewen Yao > Cc: Tom Lendacky > Cc: Jordan Justen > Cc: Ard Biesheuvel > Cc: Laszlo Ersek > Cc: Erdem Aktas > Cc: Michael D Kinney > Cc: Liming Gao > Cc: Zhiguang Liu > Signed-off-by: Brijesh Singh > --- > MdePkg/Library/BaseLib/BaseLib.inf | 1 + > MdePkg/Include/Library/BaseLib.h | 46 +++++++++++++++++++++++ > MdePkg/Include/X64/Nasm.inc | 8 ++++ > MdePkg/Library/BaseLib/X64/Pvalidate.nasm | 42 +++++++++++++++++++++ > 4 files changed, 97 insertions(+) > create mode 100644 MdePkg/Library/BaseLib/X64/Pvalidate.nasm > > diff --git a/MdePkg/Library/BaseLib/BaseLib.inf b/MdePkg/Library/BaseLib/BaseLib.inf > index b76f3af380ea..89a52f72c08a 100644 > --- a/MdePkg/Library/BaseLib/BaseLib.inf > +++ b/MdePkg/Library/BaseLib/BaseLib.inf > @@ -317,6 +317,7 @@ [Sources.X64] > X64/GccInlinePriv.c | GCC > X64/EnableDisableInterrupts.nasm > X64/DisablePaging64.nasm > + X64/Pvalidate.nasm > X64/RdRand.nasm > X64/XGetBv.nasm > X64/XSetBv.nasm > diff --git a/MdePkg/Include/Library/BaseLib.h b/MdePkg/Include/Library/BaseLib.h > index 7253997a6f8c..f177034af6a1 100644 > --- a/MdePkg/Include/Library/BaseLib.h > +++ b/MdePkg/Include/Library/BaseLib.h > @@ -4813,6 +4813,52 @@ SpeculationBarrier ( > VOID > ); > > +#if defined (MDE_CPU_X64) > +// > +// The page size for the PVALIDATE instruction > +// > +typedef enum { > + PvalidatePageSize4K = 0, > + PvalidatePageSize2MB, > +} PVALIDATE_PAGE_SIZE; > + > +// > +// PVALIDATE Return Code. > +// > +#define PVALIDATE_RET_SUCCESS 0 > +#define PVALIDATE_RET_FAIL_INPUT 1 > +#define PVALIDATE_RET_SIZE_MISMATCH 6 > + > +// > +// The PVALIDATE instruction did not made any changes to the RMP entry. (1) Typo: should be "did not make". > +// > +#define PVALIDATE_RET_NO_RMPUPDATE 255 > + > +/** > + Execute a PVALIDATE instruction to validate or rescinds validation of a guest (2) should be "to validate or to rescind validation" (infinitive form). > + page's RMP entry. > + > + The instruction is available only when CPUID Fn8000_001F_EAX[SNP]=1. > + > + The function is available on X64. > + > + @param[in] PageSize The page size to use. > + @param[in] Validate Validate or rescinds. (3) If you use the imperative for "validate", then "rescinds" (indicative) reads strangely. > + @param[in] Address The guest virtual address to validate. > + > + @retval The return value from the PVALIDATE instruction, and > + PVALIDATE_RET_NO_RMPUPDATE when there was no change in > + the RMP entry. (4) @retval is only usable with actual return values (constants). If you provide a natural language explanation, then @return is the proper doxygen directive. You can combine these BTW, for example: @retval PVALIDATE_RET_SUCCESS The PVALIDATE instruction succeeded, and updated the RMP entry. @retval PVALIDATE_RET_NO_RMPUPDATE The PVALIDATE instruction succeeded, but did not update the RMP entry. @return Failure codes from the PVALIDATE instruction. > +**/ > +UINTN > +EFIAPI > +AsmPvalidate ( > + IN PVALIDATE_PAGE_SIZE PageSize, > + IN BOOLEAN Validate, > + IN PHYSICAL_ADDRESS Address > + ); > +#endif > + > > #if defined (MDE_CPU_IA32) || defined (MDE_CPU_X64) > /// > diff --git a/MdePkg/Include/X64/Nasm.inc b/MdePkg/Include/X64/Nasm.inc > index 527f71e9eb4d..528bb3385609 100644 > --- a/MdePkg/Include/X64/Nasm.inc > +++ b/MdePkg/Include/X64/Nasm.inc > @@ -33,6 +33,14 @@ > DB 0xF3, 0x48, 0x0F, 0xAE, 0xE8 > %endmacro > > +; > +; Macro for the PVALIDATE instruction, defined in AMD APM volume 3. > +; NASM feature request URL: https://bugzilla.nasm.us/show_bug.cgi?id=3392753 > +; > +%macro PVALIDATE 0 > + DB 0xF2, 0x0F, 0x01, 0xFF > +%endmacro > + > ; NASM provides built-in macros STRUC and ENDSTRUC for structure definition. > ; For example, to define a structure called mytype containing a longword, > ; a word, a byte and a string of bytes, you might code Thanks for filing the NASM BZ! > diff --git a/MdePkg/Library/BaseLib/X64/Pvalidate.nasm b/MdePkg/Library/BaseLib/X64/Pvalidate.nasm > new file mode 100644 > index 000000000000..b20dac7e6831 > --- /dev/null > +++ b/MdePkg/Library/BaseLib/X64/Pvalidate.nasm > @@ -0,0 +1,42 @@ > +;----------------------------------------------------------------------------- > +; > +; Copyright (c) 2021, AMD. All rights reserved.
> +; SPDX-License-Identifier: BSD-2-Clause-Patent > +; > +;----------------------------------------------------------------------------- > + > +%include "Nasm.inc" > + > + SECTION .text > + > +;----------------------------------------------------------------------------- > +; UINTN > +; EFIAPI > +; AsmPvalidate ( > +; IN UINT32 RmpPageSize > +; IN UINT32 Validate, > +; IN PHYSICAL_ADDRESS Address > +; ) (5) This prototype does not match the one from the header file. I guess it's reasonable to replace the enum type and the BOOLEAN type with UINT32, in the assembly source code. But then I don't understand why PHYSICAL_ADDRESS is not replaced with UINT64 -- that would only be consistent with the other replacements. Furthermore, the parameter *names* PageSize and RmpPageSize do not match. > +;----------------------------------------------------------------------------- > +global ASM_PFX(AsmPvalidate) > +ASM_PFX(AsmPvalidate): > + mov rax, r8 > + > + PVALIDATE > + > + ; Save the carry flag. > + setb dl > + > + ; The PVALIDATE instruction returns the status in rax register. > + cmp rax, 0 > + jne PvalidateExit > + > + ; Check the carry flag to determine if RMP entry was updated. > + cmp dl, 0 > + jz PvalidateExit > + > + ; Return the PVALIDATE_RET_NO_RMPUPDATE. > + mov rax, 255 > + > +PvalidateExit: > + ret > This looks OK. I'm not very used to reading assembly, so "setc" (set if carry) instead of the equivalent "setb" (set if below) would have been easier to parse. Similarly, "je" (jump if equal) would be easier to read than the equivalent "jz" (jump if zero), especially after using "jne" (and not "jnz") with the previous "cmp". But, the assembly does look correct to me, so there's no need to change it. Thanks! Laszlo