Re: [edk2-devel] [PATCH 07/13] MdePkg/BaseLib: add support for PVALIDATE instruction

["Brijesh Singh" <brijesh.singh@amd.com>]

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@linux.ibm.com>
>> Cc: Min Xu <min.m.xu@intel.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Erdem Aktas <erdemaktas@google.com>
>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>> Cc: Liming Gao <gaoliming@byosoft.com.cn>
>> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
>> Signed-off-by: Brijesh Singh <brijesh.singh@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&amp;data=04%7C01%7Cbrijesh.singh%40amd.com%7Cc5e03d8f093c430d01b608d91467bc91%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637563258060234952%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=Sk1vgmvOHjp2VVlONWIqDMZnb5nfV5%2Fl9pegv9AQgmg%3D&amp;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
>