[edk2-devel] [PATCH 08/13] MdePkg/BaseLib: add support for RMPADJUST instruction
Laszlo Ersek
lersek at redhat.com
Tue May 11 11:01:28 UTC 2021
On 05/07/21 22:38, Brijesh Singh wrote:
> From: Tom Lendacky <thomas.lendacky at 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 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: Tom Lendacky <thomas.lendacky at amd.com>
> Signed-off-by: Brijesh Singh <brijesh.singh at 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 at redhat.com>
Thanks
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#75000): https://edk2.groups.io/g/devel/message/75000
Mute This Topic: https://groups.io/mt/82665188/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