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

["Laszlo Ersek" <lersek@redhat.com>]

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@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".

> +//
> +#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