[edk2-devel] [PATCH 07/13] MdePkg/BaseLib: add support for PVALIDATE instruction
Laszlo Ersek
lersek at redhat.com
Tue May 11 10:29:49 UTC 2021
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 <jejb at linux.ibm.com>
> Cc: Min Xu <min.m.xu at intel.com>
> Cc: Jiewen Yao <jiewen.yao at intel.com>
> Cc: Tom Lendacky <thomas.lendacky at amd.com>
> Cc: Jordan Justen <jordan.l.justen at intel.com>
> Cc: Ard Biesheuvel <ardb+tianocore at kernel.org>
> Cc: Laszlo Ersek <lersek at redhat.com>
> Cc: Erdem Aktas <erdemaktas at google.com>
> Cc: Michael D Kinney <michael.d.kinney at intel.com>
> Cc: Liming Gao <gaoliming at byosoft.com.cn>
> Cc: Zhiguang Liu <zhiguang.liu at intel.com>
> Signed-off-by: Brijesh Singh <brijesh.singh at amd.com>
> ---
> 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.<BR>
> +; 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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74999): https://edk2.groups.io/g/devel/message/74999
Mute This Topic: https://groups.io/mt/82665186/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list