[edk2-devel] [PATCH 07/13] MdePkg/BaseLib: add support for PVALIDATE instruction
Brijesh Singh
brijesh.singh at amd.com
Tue May 11 17:18:50 UTC 2021
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 <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".
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/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