On 5/11/21 5:29 AM, Laszlo Ersek wrote: > On 05/07/21 22:38, Brijesh Singh wrote: >> BZ: >> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3275&data=04%7C01%7Cbrijesh.singh%40amd.com%7Cc5e03d8f093c430d01b608d91467bc91%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637563258060234952%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=QFATwHj5Jtoughp2bLTtFChUuKCWzQBSOGcZb2I%2B534%3D&reserved=0 >> >> 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 <j...@linux.ibm.com> >> Cc: Min Xu <min.m...@intel.com> >> Cc: Jiewen Yao <jiewen....@intel.com> >> Cc: Tom Lendacky <thomas.lenda...@amd.com> >> Cc: Jordan Justen <jordan.l.jus...@intel.com> >> Cc: Ard Biesheuvel <ardb+tianoc...@kernel.org> >> Cc: Laszlo Ersek <ler...@redhat.com> >> Cc: Erdem Aktas <erdemak...@google.com> >> Cc: Michael D Kinney <michael.d.kin...@intel.com> >> Cc: Liming Gao <gaolim...@byosoft.com.cn> >> Cc: Zhiguang Liu <zhiguang....@intel.com> >> Signed-off-by: Brijesh Singh <brijesh.si...@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". Noted. > >> +// >> +#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). Noted. > >> + 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. I will use validate or invalidate in future patches. > >> + @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.
Will do. >> +**/ >> +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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.nasm.us%2Fshow_bug.cgi%3Fid%3D3392753&data=04%7C01%7Cbrijesh.singh%40amd.com%7Cc5e03d8f093c430d01b608d91467bc91%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637563258060234952%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=Sk1vgmvOHjp2VVlONWIqDMZnb5nfV5%2Fl9pegv9AQgmg%3D&reserved=0 >> +; >> +%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. > Will fix in next rev. >> +;----------------------------------------------------------------------------- >> +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 (#75022): https://edk2.groups.io/g/devel/message/75022 Mute This Topic: https://groups.io/mt/82665186/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
