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

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

On 05/07/21 22:38, Brijesh Singh wrote:
> From: Tom Lendacky <thomas.lendacky@amd.com>
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275
> 
> The RMPADJUST instruction will be used by the SEV-SNP guest to modify the
> RMP permissions for a guest page. See AMD APM volume 3 for further
> details.
> 
> 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: Tom Lendacky <thomas.lendacky@amd.com>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  MdePkg/Library/BaseLib/BaseLib.inf        |  1 +
>  MdePkg/Include/Library/BaseLib.h          | 36 +++++++++++++++++++-
>  MdePkg/Include/X64/Nasm.inc               |  8 +++++
>  MdePkg/Library/BaseLib/X64/RmpAdjust.nasm | 40 +++++++++++++++++++++++
>  4 files changed, 84 insertions(+), 1 deletion(-)
>  create mode 100644 MdePkg/Library/BaseLib/X64/RmpAdjust.nasm
> 
> diff --git a/MdePkg/Library/BaseLib/BaseLib.inf b/MdePkg/Library/BaseLib/BaseLib.inf
> index 89a52f72c08a..6ccb8997b7e8 100644
> --- a/MdePkg/Library/BaseLib/BaseLib.inf
> +++ b/MdePkg/Library/BaseLib/BaseLib.inf
> @@ -319,6 +319,7 @@ [Sources.X64]
>    X64/DisablePaging64.nasm
>    X64/Pvalidate.nasm
>    X64/RdRand.nasm
> +  X64/RmpAdjust.nasm
>    X64/XGetBv.nasm
>    X64/XSetBv.nasm
>    X64/VmgExit.nasm
> diff --git a/MdePkg/Include/Library/BaseLib.h b/MdePkg/Include/Library/BaseLib.h
> index f177034af6a1..04e58f995b9a 100644
> --- a/MdePkg/Include/Library/BaseLib.h
> +++ b/MdePkg/Include/Library/BaseLib.h
> @@ -4857,9 +4857,43 @@ AsmPvalidate (
>    IN   BOOLEAN                 Validate,
>    IN   PHYSICAL_ADDRESS        Address
>    );
> +
> +//
> +// RDX settings for RMPADJUST
> +//
> +#define RMPADJUST_VMPL_MAX               3
> +#define RMPADJUST_VMPL_MASK              0xFF
> +#define RMPADJUST_VMPL_SHIFT             0
> +#define RMPADJUST_PERMISSION_MASK_MASK   0xFF
> +#define RMPADJUST_PERMISSION_MASK_SHIFT  8
> +#define RMPADJUST_VMSA_PAGE_BIT          BIT16
> +
> +/**
> +  Adjusts the permissions of an SEV-SNP guest page.
> +
> +  Executes a RMPADJUST instruction with the register state specified by Rax,
> +  Rcx and Rdx. Returns Eax. This function is only available x64.

(1) trivial typo: IMO it should be "on X64" (preposition missing, and
X64 should be upper case).

> +
> +  The instruction is available only when CPUID Fn8000_001F_EAX[SNP]=1.
> +
> +  @param[in]  Rax   The value to load into RAX before executing the RMPADJUST
> +                    instruction.
> +  @param[in]  Rcx   The value to load into RCX before executing the RMPADJUST
> +                    instruction.
> +  @param[in]  Rdx   The value to load into RDX before executing the RMPADJUST
> +                    instruction.
> +
> +  @return     Eax
> +**/
> +UINTN

(2) Not a "hard requirement", just something I thought I'd raise: both
the spec, and the leading comment (twice), say that the return code is
in EAX (not RAX). Would it make sense to use UINT32 for the return type
of the function?

(3) Since we are talking return codes... For PVALIDATE, the previous
patch introduces macros for the return codes. I haven't looked at
RMPADJUST before, but now it seems like SEV-ES-related machine
instructions come with a "global" status code table: 0 for SUCCESS, 1
for FAIL_INPUT, 6 for FAIL_SIZEMISMATCH (<-- all of these are shared by
PVALIDATE and RMPADJUST), and now FAIL_PERMISSION (2) for RMPADJUST only.

So now I wonder if these macros belong in an AMD-specific header file...
Anyway, I definitely defer to Liming and Mike on this MdePkg content.

> +EFIAPI
> +AsmRmpAdjust (
> +  IN      UINTN                     Rax,
> +  IN      UINTN                     Rcx,
> +  IN      UINTN                     Rdx
> +  );

(4) Given that we really call these R*x, shouldn't we make them
explicitly UINT64? I don't think there's an interpretation for RAX that
is *not* 64-bit.

>  #endif
>  
> -

(5) Indeed, this newline is superfluous, I just didn't want to obsess
about it under patch #7. If you agree it's unneeded, then please drop it
from patch#7.

>  #if defined (MDE_CPU_IA32) || defined (MDE_CPU_X64)
>  ///
>  /// IA32 and x64 Specific Functions.
> diff --git a/MdePkg/Include/X64/Nasm.inc b/MdePkg/Include/X64/Nasm.inc
> index 528bb3385609..cfb14edc9449 100644
> --- a/MdePkg/Include/X64/Nasm.inc
> +++ b/MdePkg/Include/X64/Nasm.inc
> @@ -41,6 +41,14 @@
>      DB 0xF2, 0x0F, 0x01, 0xFF
>  %endmacro
>  
> +;
> +; Macro for the RMPADJUST instruction, defined in AMD APM volume 3.
> +; NASM feature request URL: https://bugzilla.nasm.us/show_bug.cgi?id=3392754
> +;
> +%macro RMPADJUST       0
> +    DB 0xF3, 0x0F, 0x01, 0xFE
> +%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
> diff --git a/MdePkg/Library/BaseLib/X64/RmpAdjust.nasm b/MdePkg/Library/BaseLib/X64/RmpAdjust.nasm
> new file mode 100644
> index 000000000000..f2c295b67c9c
> --- /dev/null
> +++ b/MdePkg/Library/BaseLib/X64/RmpAdjust.nasm
> @@ -0,0 +1,40 @@
> +;-----------------------------------------------------------------------------
> +;
> +; Copyright (c) 2021, Advanced Micro Devices, Inc. All rights reserved.<BR>
> +; SPDX-License-Identifier: BSD-2-Clause-Patent
> +;
> +; Module Name:
> +;
> +;   RmpAdjust.Asm
> +;
> +; Abstract:
> +;
> +;   AsmRmpAdjust function
> +;
> +; Notes:
> +;
> +;-----------------------------------------------------------------------------
> +
> +%include "Nasm.inc"
> +
> +    SECTION .text
> +
> +;-----------------------------------------------------------------------------
> +;  UINTN
> +;  EFIAPI
> +;  AsmRmpAdjust (
> +;    IN  UINTN  Rax,
> +;    IN  UINTN  Rcx,
> +;    IN  UINTN  Rdx
> +;    )

(6) If you agree with my points (2) and (4), then please don't forget to
sync this part.

> +;-----------------------------------------------------------------------------
> +global ASM_PFX(AsmRmpAdjust)
> +ASM_PFX(AsmRmpAdjust):
> +  mov     rax, rcx       ; Input Rax is in RCX by calling convention
> +  mov     rcx, rdx       ; Input Rcx is in RDX by calling convention
> +  mov     rdx, r8        ; Input Rdx is in R8  by calling convention
> +
> +  RMPADJUST
> +
> +  ; RMPADJUST returns the status in the EAX register.
> +  ret
> 

I only made trivial comments on this patch; even if you disagreed with
everything I said, I'd be OK.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo