[PATCH RFC v2 05/28] MdePkg: Add AsmPvalidate() support


Brijesh Singh
 

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>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
MdePkg/Include/Library/BaseLib.h | 37 +++++++++++++++++
MdePkg/Library/BaseLib/BaseLib.inf | 1 +
MdePkg/Library/BaseLib/X64/Pvalidate.nasm | 43 ++++++++++++++++++++
3 files changed, 81 insertions(+)

diff --git a/MdePkg/Include/Library/BaseLib.h b/MdePkg/Include/Library/BaseLib.h
index 7253997a6f..92ce695e93 100644
--- a/MdePkg/Include/Library/BaseLib.h
+++ b/MdePkg/Include/Library/BaseLib.h
@@ -7518,5 +7518,42 @@ PatchInstructionX86 (
IN UINTN ValueSize
);

+/**
+ Execute a PVALIDATE instruction to validate or rescnids validation of a guest
+ page's RMP entry.
+
+ Upon completion, in addition to the return value the instruction also updates
+ the eFlags. A caller must check both the return code as well as eFlags to
+ determine if the RMP entry has been updated.
+
+ The function is available on x64.
+
+ @param[in] Address The guest virtual address to validate.
+ @param[in] PageSize The page size to use.
+ @param[i] Validate Validate or rescinds.
+ @param[out] Eflags The value of Eflags after PVALIDATE completion.
+
+ @retval PvalidateRetValue The return value from the PVALIDATE instruction.
+**/
+typedef enum {
+ PvalidatePageSize4K = 0,
+ PvalidatePageSize2MB,
+} PVALIDATE_PAGE_SIZE;
+
+typedef enum {
+ PvalidateRetSuccess = 0,
+ PvalidateRetFailInput = 1,
+ PvalidateRetFailSizemismatch = 6,
+} PVALIDATE_RET_VALUE;
+
+PVALIDATE_RET_VALUE
+EFIAPI
+AsmPvalidate (
+ IN PVALIDATE_PAGE_SIZE PageSize,
+ IN BOOLEAN Validate,
+ IN UINTN Address,
+ OUT IA32_EFLAGS32 *Eflags
+ );
+
#endif // defined (MDE_CPU_IA32) || defined (MDE_CPU_X64)
#endif // !defined (__BASE_LIB__)
diff --git a/MdePkg/Library/BaseLib/BaseLib.inf b/MdePkg/Library/BaseLib/BaseLib.inf
index b76f3af380..d33b4a8f7d 100644
--- a/MdePkg/Library/BaseLib/BaseLib.inf
+++ b/MdePkg/Library/BaseLib/BaseLib.inf
@@ -321,6 +321,7 @@
X64/XGetBv.nasm
X64/XSetBv.nasm
X64/VmgExit.nasm
+ X64/Pvalidate.nasm
ChkStkGcc.c | GCC

[Sources.EBC]
diff --git a/MdePkg/Library/BaseLib/X64/Pvalidate.nasm b/MdePkg/Library/BaseLib/X64/Pvalidate.nasm
new file mode 100644
index 0000000000..f2aba114ac
--- /dev/null
+++ b/MdePkg/Library/BaseLib/X64/Pvalidate.nasm
@@ -0,0 +1,43 @@
+;-----------------------------------------------------------------------------
+;
+; Copyright (c) 2020-2021, AMD. All rights reserved.<BR>
+; SPDX-License-Identifier: BSD-2-Clause-Patent
+;
+; Module Name:
+;
+; Pvalidate.Asm
+;
+; Abstract:
+;
+; AsmPvalidate function
+;
+; Notes:
+;
+;-----------------------------------------------------------------------------
+
+ SECTION .text
+
+;-----------------------------------------------------------------------------
+; PvalidateRetValue
+; EFIAPI
+; AsmPvalidate (
+; IN UINT32 RmpPageSize
+; IN UINT32 Validate,
+; IN UINTN Address,
+; OUT UINTN *Eflags,
+; )
+;-----------------------------------------------------------------------------
+global ASM_PFX(AsmPvalidate)
+ASM_PFX(AsmPvalidate):
+ mov rax, r8
+
+ ; PVALIDATE instruction opcode
+ DB 0xF2, 0x0F, 0x01, 0xFF
+
+ ; Read the Eflags
+ pushfq
+ pop r8
+ mov [r9], r8
+
+ ; The PVALIDATE instruction returns the status in rax register.
+ ret
--
2.17.1


Laszlo Ersek
 

On 04/30/21 13:51, 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>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
MdePkg/Include/Library/BaseLib.h | 37 +++++++++++++++++
MdePkg/Library/BaseLib/BaseLib.inf | 1 +
MdePkg/Library/BaseLib/X64/Pvalidate.nasm | 43 ++++++++++++++++++++
3 files changed, 81 insertions(+)

diff --git a/MdePkg/Include/Library/BaseLib.h b/MdePkg/Include/Library/BaseLib.h
index 7253997a6f..92ce695e93 100644
--- a/MdePkg/Include/Library/BaseLib.h
+++ b/MdePkg/Include/Library/BaseLib.h
@@ -7518,5 +7518,42 @@ PatchInstructionX86 (
IN UINTN ValueSize
);

+/**
+ Execute a PVALIDATE instruction to validate or rescnids validation of a guest
(1) typo: "rescnids"


+ page's RMP entry.
+
+ Upon completion, in addition to the return value the instruction also updates
+ the eFlags. A caller must check both the return code as well as eFlags to
+ determine if the RMP entry has been updated.
+
+ The function is available on x64.
(2) Please write "X64"; that's how the architecture is usually mentioned
in both the UEFI spec and in edk2.


+
+ @param[in] Address The guest virtual address to validate.
+ @param[in] PageSize The page size to use.
+ @param[i] Validate Validate or rescinds.
+ @param[out] Eflags The value of Eflags after PVALIDATE completion.
(3) Typo: "[i]" should be "[in]".


(4) The order of parameters listed in this comment block differs from
the actual parameter list.

The ECC plugin of the edk2 CI will catch this issue anyway. So, before
submitting the patch set to the list, please submit a personal PR on
github.com against the main repo, just to run CI on your patches.


+
+ @retval PvalidateRetValue The return value from the PVALIDATE instruction.
More on the return value / type later, below.

+**/
+typedef enum {
+ PvalidatePageSize4K = 0,
+ PvalidatePageSize2MB,
+} PVALIDATE_PAGE_SIZE;
+
+typedef enum {
+ PvalidateRetSuccess = 0,
+ PvalidateRetFailInput = 1,
+ PvalidateRetFailSizemismatch = 6,
+} PVALIDATE_RET_VALUE;
+
(5) These typedefs do not belong between the function leading comment
and the function declaration. Please hoist the typedefs just above the
leading comment, and add a separate comment for the typedefs -- using
the proper comment style for typedefs, too.


+PVALIDATE_RET_VALUE
(6) In my opinion, using an enum for an EFIAPI function's return type is
problematic. According to the UEFI spec (v2.9), "Table 2-3 Common UEFI
Data Types", <Enumerated Type> may correspond to INT32 or UINT32. I
don't like that ambiguity here. The spec also says that such types
should never be used at least as structure fields.

I'm perfectly fine with functions in standard (ISO) C programs returning
enums, but I think the situation is less clear in UEFI. I don't recall
standard interfaces (spec-level, or even edk2 / MdePkg interfaces) that
return enums.

I suggest the following instead. Drop the PVALIDATE_RET_VALUE enum
altogether. Specify EFI_STATUS as the return type, in the declaration of
the function.

In the UEFI spec, Appendix D specifies the numeric values of the status
codes. Furthermore, there are examples for NASM sources using EFI_*
status codes *numerically* in edk2; minimally:

- IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm
- IntelFsp2WrapperPkg/Library/SecFspWrapperPlatformSecLibSample/Ia32/SecEntry.nasm

Thus, please modify the assembly source code in this patch to return
EFI_SUCCESS (already value 0, conveniently) if the instruction succeeds.

Return EFI_INVALID_PARAMETER (0x8000_0002) in case the instruction fails
with error code 1 (FAIL_INPUT).

Return EFI_UNSUPPORTED (0x8000_0003), or even EFI_NO_MAPPING
(0x8000_0017), for value 6 (FAIL_SIZEMISMATCH).

The leading comment block of the function is supposed to explain these
associations:

@retval EFI_SUCCESS Successful completion (regardless of
whether the Validated bit changed state).
@retval INVALID_PARAMETER Invalid input parameters (FAIL_INPUT).
@retval EFI_UNSUPPORTED Page size mismatch between guest (2M) and
RMP entry (4K) (FAIL_SIZEMISMATCH).

(Passing in the PVALIDATE_PAGE_SIZE enum, as a parameter, should be
fine, BTW)


(7) According to the AMD APM, "Support for this instruction is indicated
by CPUID Fn8000_001F_EAX[SNP]=1".

Presumably, if the (physical, or emulated) hardware does not support
PVALIDATE, an #UD is raised. That condition should be explained in the
function's leading comment. (Mention the CPUID and the #UD, I guess.)


+EFIAPI
+AsmPvalidate (
+ IN PVALIDATE_PAGE_SIZE PageSize,
+ IN BOOLEAN Validate,
+ IN UINTN Address,
(8) This should be EFI_PHYSICAL_ADDRESS, not UINTN.


+ OUT IA32_EFLAGS32 *Eflags
+ );
+
#endif // defined (MDE_CPU_IA32) || defined (MDE_CPU_X64)
#endif // !defined (__BASE_LIB__)
(9) Unless you foresee particular uses for eflags *other than* CF, I
would suggest replacing the Eflags output parameter with

OUT BOOLEAN *RmpEntryUpdated

The function would still only have 4 parameters, which shouldn't be
difficult to handle in the assembly implementation (i.e. write to the
UINT8 (= BOOLEAN) object referenced by "RmpEntryUpdated"). EFIAPI means
that the first four params are passed in RCX, RDX, R8, R9.

Thus far, I can see only one AsmPvalidate() call: in IssuePvalidate(),
from patch #21 ("OvmfPkg/MemEncryptSevLib: Add support to validate
system RAM"). And there, CF looks sufficient.


(10) The instruction is X64 only, but you are providing the declaration
even if MDE_CPU_IA32 is #defined. That seems wrong; even the declaration
should be invisible in that case. Please declare the function for
MDE_CPU_X64 only.


diff --git a/MdePkg/Library/BaseLib/BaseLib.inf b/MdePkg/Library/BaseLib/BaseLib.inf
index b76f3af380..d33b4a8f7d 100644
--- a/MdePkg/Library/BaseLib/BaseLib.inf
+++ b/MdePkg/Library/BaseLib/BaseLib.inf
@@ -321,6 +321,7 @@
X64/XGetBv.nasm
X64/XSetBv.nasm
X64/VmgExit.nasm
+ X64/Pvalidate.nasm
ChkStkGcc.c | GCC

[Sources.EBC]
(11) This list of source files is already not sorted alphabetically,
unfortunately. But we can still do better than this: I suggest inserting
"X64/Pvalidate.nasm" just before "X64/RdRand.nasm".


(12) Your git setup seems less than ideal for formatting edk2 patches.
The @@ hunk header above does not show the INF file section being
modified. It should look something like this:

@@ -317,6 +317,7 @@ [Sources.X64]
^^^^^^^^^^^^^

Please run the "BaseTools/Scripts/SetupGit.py" script in your working
tree.

Alternatively, please see "xfuncname" at
<https://github.com/tianocore/tianocore.github.io/wiki/Laszlo%27s-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-05>.

This is of course not a bug in the patch, but fixing your setup will
help with the next round of review.


diff --git a/MdePkg/Library/BaseLib/X64/Pvalidate.nasm b/MdePkg/Library/BaseLib/X64/Pvalidate.nasm
new file mode 100644
index 0000000000..f2aba114ac
--- /dev/null
+++ b/MdePkg/Library/BaseLib/X64/Pvalidate.nasm
@@ -0,0 +1,43 @@
+;-----------------------------------------------------------------------------
+;
+; Copyright (c) 2020-2021, AMD. All rights reserved.<BR>
(13) I believe we don't introduce new files with copyright notices
referring to the past. IOW, I think you should only say "2021" here.


+; SPDX-License-Identifier: BSD-2-Clause-Patent
+;
+; Module Name:
+;
+; Pvalidate.Asm
+;
+; Abstract:
+;
+; AsmPvalidate function
+;
+; Notes:
+;
(14) I defer to the MdePkg maintainers on this, but "Module Name" is
plain wrong, and the Abstract is useless. Either fix those up please
("Abstract" could be a copy of the corrected leading comment block), or
just drop them both.


+;-----------------------------------------------------------------------------
+
+ SECTION .text
+
+;-----------------------------------------------------------------------------
+; PvalidateRetValue
+; EFIAPI
+; AsmPvalidate (
+; IN UINT32 RmpPageSize
+; IN UINT32 Validate,
+; IN UINTN Address,
+; OUT UINTN *Eflags,
+; )
+;-----------------------------------------------------------------------------
(15) Please update this accordingly to the corrected function
specification.


+global ASM_PFX(AsmPvalidate)
+ASM_PFX(AsmPvalidate):
+ mov rax, r8
+
+ ; PVALIDATE instruction opcode
+ DB 0xF2, 0x0F, 0x01, 0xFF
This is bad practice; we make every effort to avoid DB-encoded
instructions.

We have two PVALIDATE instances in the patch set (... that I can see
immediateyl); the first here, and the other in
"OvmfPkg/ResetVector/Ia32/PageTables64.asm" (from patch #17,
"OvmfPkg/ResetVector: Invalidate the GHCB page"). Therefore, hiding the
encoding of PVALIDATE behind a NASM macro definitely makes sense.

(16a) Please file a NASM feature request for PVALIDATE at
<https://bugzilla.nasm.us>.

(16b) In the present MdePkg patch, please extend the file

MdePkg/Include/X64/Nasm.inc

as follows:

diff --git a/MdePkg/Include/X64/Nasm.inc b/MdePkg/Include/X64/Nasm.inc
index 527f71e9eb4d..ff37f1e35707 100644
--- a/MdePkg/Include/X64/Nasm.inc
+++ b/MdePkg/Include/X64/Nasm.inc
@@ -33,6 +33,15 @@
DB 0xF3, 0x48, 0x0F, 0xAE, 0xE8
%endmacro

+;
+; Macro for the PVALIDATE instruction, defined in AMD publication #24594
+; revision 3.32. NASM feature request URL:
+; <https://bugzilla.nasm.us/show_bug.cgi?id=FIXME>.
+;
+%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
(16c) Please replace the FIXME placeholder above with the actual NASM BZ
number (from (16a)).

(16d) In the "MdePkg/Library/BaseLib/X64/Pvalidate.nasm" source file,
and also (later) in the "OvmfPkg/ResetVector/Ia32/PageTables64.asm"
source file, please use the PVALIDATE macro, in place of the naked DBs.


Back to your patch:

On 04/30/21 13:51, Brijesh Singh wrote:
+
+ ; Read the Eflags
+ pushfq
+ pop r8
+ mov [r9], r8
+
+ ; The PVALIDATE instruction returns the status in rax register.
+ ret
(17) The assembly code should be updated to match the new interface
contract (parameter order, parameter types, return values).


I'll continue reviewing the series later this week (hopefully tomorrow).

Thanks,
Laszlo


Laszlo Ersek
 

On 05/04/21 15:58, Laszlo Ersek wrote:

The leading comment block of the function is supposed to explain these
associations:

@retval EFI_SUCCESS Successful completion (regardless of
whether the Validated bit changed state).
@retval INVALID_PARAMETER Invalid input parameters (FAIL_INPUT).
@retval EFI_UNSUPPORTED Page size mismatch between guest (2M) and
RMP entry (4K) (FAIL_SIZEMISMATCH).
Apologies, that should have been "EFI_INVALID_PARAMETER", not just
"INVALID_PARAMETER".

Thanks
Laszlo


Brijesh Singh
 

On 5/4/21 8:58 AM, Laszlo Ersek wrote:
On 04/30/21 13:51, 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%7C6ceeec6c984d468bb87908d90f04b789%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637557335220626621%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=jF44vk%2FBUVukpuNg4vrcRqHnEa7nO%2FGPEe9ti720cnE%3D&amp;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>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
MdePkg/Include/Library/BaseLib.h | 37 +++++++++++++++++
MdePkg/Library/BaseLib/BaseLib.inf | 1 +
MdePkg/Library/BaseLib/X64/Pvalidate.nasm | 43 ++++++++++++++++++++
3 files changed, 81 insertions(+)

diff --git a/MdePkg/Include/Library/BaseLib.h b/MdePkg/Include/Library/BaseLib.h
index 7253997a6f..92ce695e93 100644
--- a/MdePkg/Include/Library/BaseLib.h
+++ b/MdePkg/Include/Library/BaseLib.h
@@ -7518,5 +7518,42 @@ PatchInstructionX86 (
IN UINTN ValueSize
);

+/**
+ Execute a PVALIDATE instruction to validate or rescnids validation of a guest
(1) typo: "rescnids"
Noted.




+ page's RMP entry.
+
+ Upon completion, in addition to the return value the instruction also updates
+ the eFlags. A caller must check both the return code as well as eFlags to
+ determine if the RMP entry has been updated.
+
+ The function is available on x64.
(2) Please write "X64"; that's how the architecture is usually mentioned
in both the UEFI spec and in edk2.
Noted.




+
+ @param[in] Address The guest virtual address to validate.
+ @param[in] PageSize The page size to use.
+ @param[i] Validate Validate or rescinds.
+ @param[out] Eflags The value of Eflags after PVALIDATE completion.
(3) Typo: "[i]" should be "[in]".
Noted.




(4) The order of parameters listed in this comment block differs from
the actual parameter list.

The ECC plugin of the edk2 CI will catch this issue anyway. So, before
submitting the patch set to the list, please submit a personal PR on
github.com against the main repo, just to run CI on your patches.
Interestingly I did ran the series with edk2 CI and everything passed
before I submitted for the review. But, I will go ahead and fix the order.



+
+ @retval PvalidateRetValue The return value from the PVALIDATE instruction.
More on the return value / type later, below.

+**/
+typedef enum {
+ PvalidatePageSize4K = 0,
+ PvalidatePageSize2MB,
+} PVALIDATE_PAGE_SIZE;
+
+typedef enum {
+ PvalidateRetSuccess = 0,
+ PvalidateRetFailInput = 1,
+ PvalidateRetFailSizemismatch = 6,
+} PVALIDATE_RET_VALUE;
+
(5) These typedefs do not belong between the function leading comment
and the function declaration. Please hoist the typedefs just above the
leading comment, and add a separate comment for the typedefs -- using
the proper comment style for typedefs, too.


+PVALIDATE_RET_VALUE
(6) In my opinion, using an enum for an EFIAPI function's return type is
problematic. According to the UEFI spec (v2.9), "Table 2-3 Common UEFI
Data Types", <Enumerated Type> may correspond to INT32 or UINT32. I
don't like that ambiguity here. The spec also says that such types
should never be used at least as structure fields.

I'm perfectly fine with functions in standard (ISO) C programs returning
enums, but I think the situation is less clear in UEFI. I don't recall
standard interfaces (spec-level, or even edk2 / MdePkg interfaces) that
return enums.

I suggest the following instead. Drop the PVALIDATE_RET_VALUE enum
altogether. Specify EFI_STATUS as the return type, in the declaration of
the function.
Works with me.



In the UEFI spec, Appendix D specifies the numeric values of the status
codes. Furthermore, there are examples for NASM sources using EFI_*
status codes *numerically* in edk2; minimally:

- IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm
- IntelFsp2WrapperPkg/Library/SecFspWrapperPlatformSecLibSample/Ia32/SecEntry.nasm

Thus, please modify the assembly source code in this patch to return
EFI_SUCCESS (already value 0, conveniently) if the instruction succeeds.
Works with me.


Return EFI_INVALID_PARAMETER (0x8000_0002) in case the instruction fails
with error code 1 (FAIL_INPUT).
Works with me.



Return EFI_UNSUPPORTED (0x8000_0003), or even EFI_NO_MAPPING
(0x8000_0017), for value 6 (FAIL_SIZEMISMATCH).
I am not sure if we really want to do this. You will see later in the
patches that in some cases the PVALIDATE will return a failure and we
will need to know the failure code to determine the next steps.
Especially this particular error code is used later. This error happens
when the page size of the backing pages does not match with the
pvalidated size. In those cases we need to retry the PVALIDATE with
lower page size so that a validation succeed. One such a example is:

- Guest ask hypervisor to add the page as 2M in RMP table.

- Hypervisor added the page as 512 4K pages - because it was not able to
find a large backing pages.

- Guest attempts to pvalidate the page as a 2M. The pvalidate will
return a failure saying its a size mismatch between the requested
pvalidated and RMP table. The recommendation is that guest should try
with a smaller page size.

I would prefer to pass the pvalidate error as-is to caller so that it
can make the correct decision.



The leading comment block of the function is supposed to explain these
associations:

@retval EFI_SUCCESS Successful completion (regardless of
whether the Validated bit changed state).
@retval INVALID_PARAMETER Invalid input parameters (FAIL_INPUT).
@retval EFI_UNSUPPORTED Page size mismatch between guest (2M) and
RMP entry (4K) (FAIL_SIZEMISMATCH).

(Passing in the PVALIDATE_PAGE_SIZE enum, as a parameter, should be
fine, BTW)


(7) According to the AMD APM, "Support for this instruction is indicated
by CPUID Fn8000_001F_EAX[SNP]=1".

Presumably, if the (physical, or emulated) hardware does not support
PVALIDATE, an #UD is raised. That condition should be explained in the
function's leading comment. (Mention the CPUID and the #UD, I guess.)
Noted.

+EFIAPI
+AsmPvalidate (
+ IN PVALIDATE_PAGE_SIZE PageSize,
+ IN BOOLEAN Validate,
+ IN UINTN Address,
(8) This should be EFI_PHYSICAL_ADDRESS, not UINTN.
Noted.



+ OUT IA32_EFLAGS32 *Eflags
+ );
+
#endif // defined (MDE_CPU_IA32) || defined (MDE_CPU_X64)
#endif // !defined (__BASE_LIB__)
(9) Unless you foresee particular uses for eflags *other than* CF, I
would suggest replacing the Eflags output parameter with

OUT BOOLEAN *RmpEntryUpdated

The function would still only have 4 parameters, which shouldn't be
difficult to handle in the assembly implementation (i.e. write to the
UINT8 (= BOOLEAN) object referenced by "RmpEntryUpdated"). EFIAPI means
that the first four params are passed in RCX, RDX, R8, R9.
Works with me, thats what I ended up doing for the guest kernel patches
and will do the same for OVMF as well.

Thus far, I can see only one AsmPvalidate() call: in IssuePvalidate(),
from patch #21 ("OvmfPkg/MemEncryptSevLib: Add support to validate
system RAM"). And there, CF looks sufficient.


(10) The instruction is X64 only, but you are providing the declaration
even if MDE_CPU_IA32 is #defined. That seems wrong; even the declaration
should be invisible in that case. Please declare the function for
MDE_CPU_X64 only.
Noted.


diff --git a/MdePkg/Library/BaseLib/BaseLib.inf b/MdePkg/Library/BaseLib/BaseLib.inf
index b76f3af380..d33b4a8f7d 100644
--- a/MdePkg/Library/BaseLib/BaseLib.inf
+++ b/MdePkg/Library/BaseLib/BaseLib.inf
@@ -321,6 +321,7 @@
X64/XGetBv.nasm
X64/XSetBv.nasm
X64/VmgExit.nasm
+ X64/Pvalidate.nasm
ChkStkGcc.c | GCC

[Sources.EBC]
(11) This list of source files is already not sorted alphabetically,
unfortunately. But we can still do better than this: I suggest inserting
"X64/Pvalidate.nasm" just before "X64/RdRand.nasm".
Noted.



(12) Your git setup seems less than ideal for formatting edk2 patches.
The @@ hunk header above does not show the INF file section being
modified. It should look something like this:

@@ -317,6 +317,7 @@ [Sources.X64]
^^^^^^^^^^^^^

Please run the "BaseTools/Scripts/SetupGit.py" script in your working
tree.
Will do.



Alternatively, please see "xfuncname" at
<https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftianocore%2Ftianocore.github.io%2Fwiki%2FLaszlo%2527s-unkempt-git-guide-for-edk2-contributors-and-maintainers%23contrib-05&;data=04%7C01%7Cbrijesh.singh%40amd.com%7C6ceeec6c984d468bb87908d90f04b789%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637557335220626621%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=W16GsCzqxFZlH7DBOIFY0EJy%2FS6DTSCt0qEVOMLjjRs%3D&amp;reserved=0>.

This is of course not a bug in the patch, but fixing your setup will
help with the next round of review.


diff --git a/MdePkg/Library/BaseLib/X64/Pvalidate.nasm b/MdePkg/Library/BaseLib/X64/Pvalidate.nasm
new file mode 100644
index 0000000000..f2aba114ac
--- /dev/null
+++ b/MdePkg/Library/BaseLib/X64/Pvalidate.nasm
@@ -0,0 +1,43 @@
+;-----------------------------------------------------------------------------
+;
+; Copyright (c) 2020-2021, AMD. All rights reserved.<BR>
(13) I believe we don't introduce new files with copyright notices
referring to the past. IOW, I think you should only say "2021" here.
Noted.


+; SPDX-License-Identifier: BSD-2-Clause-Patent
+;
+; Module Name:
+;
+; Pvalidate.Asm
+;
+; Abstract:
+;
+; AsmPvalidate function
+;
+; Notes:
+;
(14) I defer to the MdePkg maintainers on this, but "Module Name" is
plain wrong, and the Abstract is useless. Either fix those up please
("Abstract" could be a copy of the corrected leading comment block), or
just drop them both.


+;-----------------------------------------------------------------------------
+
+ SECTION .text
+
+;-----------------------------------------------------------------------------
+; PvalidateRetValue
+; EFIAPI
+; AsmPvalidate (
+; IN UINT32 RmpPageSize
+; IN UINT32 Validate,
+; IN UINTN Address,
+; OUT UINTN *Eflags,
+; )
+;-----------------------------------------------------------------------------
(15) Please update this accordingly to the corrected function
specification.
Noted.



+global ASM_PFX(AsmPvalidate)
+ASM_PFX(AsmPvalidate):
+ mov rax, r8
+
+ ; PVALIDATE instruction opcode
+ DB 0xF2, 0x0F, 0x01, 0xFF
This is bad practice; we make every effort to avoid DB-encoded
instructions.

We have two PVALIDATE instances in the patch set (... that I can see
immediateyl); the first here, and the other in
"OvmfPkg/ResetVector/Ia32/PageTables64.asm" (from patch #17,
"OvmfPkg/ResetVector: Invalidate the GHCB page"). Therefore, hiding the
encoding of PVALIDATE behind a NASM macro definitely makes sense.

(16a) Please file a NASM feature request for PVALIDATE at
<https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.nasm.us%2F&;data=04%7C01%7Cbrijesh.singh%40amd.com%7C6ceeec6c984d468bb87908d90f04b789%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637557335220636620%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=VtpkUnBunOdo3n86u3dBhoJZPxLN8M95ClyYUtC7v9o%3D&amp;reserved=0>.
Will do.



(16b) In the present MdePkg patch, please extend the file

MdePkg/Include/X64/Nasm.inc

as follows:

diff --git a/MdePkg/Include/X64/Nasm.inc b/MdePkg/Include/X64/Nasm.inc
index 527f71e9eb4d..ff37f1e35707 100644
--- a/MdePkg/Include/X64/Nasm.inc
+++ b/MdePkg/Include/X64/Nasm.inc
@@ -33,6 +33,15 @@
DB 0xF3, 0x48, 0x0F, 0xAE, 0xE8
%endmacro

+;
+; Macro for the PVALIDATE instruction, defined in AMD publication #24594
+; revision 3.32. NASM feature request URL:
+; <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.nasm.us%2Fshow_bug.cgi%3Fid%3DFIXME&;data=04%7C01%7Cbrijesh.singh%40amd.com%7C6ceeec6c984d468bb87908d90f04b789%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637557335220636620%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=WeN3YmLN6OEpY8azwBt0ixogffkCCSk70P2Y%2Bff0Hlo%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
Sure, I will add the new macro and use it.


(16c) Please replace the FIXME placeholder above with the actual NASM BZ
number (from (16a)).

(16d) In the "MdePkg/Library/BaseLib/X64/Pvalidate.nasm" source file,
and also (later) in the "OvmfPkg/ResetVector/Ia32/PageTables64.asm"
source file, please use the PVALIDATE macro, in place of the naked DBs.


Back to your patch:

On 04/30/21 13:51, Brijesh Singh wrote:
+
+ ; Read the Eflags
+ pushfq
+ pop r8
+ mov [r9], r8
+
+ ; The PVALIDATE instruction returns the status in rax register.
+ ret
(17) The assembly code should be updated to match the new interface
contract (parameter order, parameter types, return values).
Sure, I will update based on your feedback.



I'll continue reviewing the series later this week (hopefully tomorrow).

thanks for all your review feedback.


Brijesh Singh
 

On 5/4/21 2:07 PM, Brijesh Singh via groups.io wrote:
Return EFI_UNSUPPORTED (0x8000_0003), or even EFI_NO_MAPPING
(0x8000_0017), for value 6 (FAIL_SIZEMISMATCH).
I am not sure if we really want to do this. You will see later in the
patches that in some cases the PVALIDATE will return a failure and we
will need to know the failure code to determine the next steps.
Especially this particular error code is used later. This error happens
when the page size of the backing pages does not match with the
pvalidated size. In those cases we need to retry the PVALIDATE with
lower page size so that a validation succeed. One such a example is:

- Guest ask hypervisor to add the page as 2M in RMP table.

- Hypervisor added the page as 512 4K pages - because it was not able to
find a large backing pages.

- Guest attempts to pvalidate the page as a 2M. The pvalidate will
return a failure saying its a size mismatch between the requested
pvalidated and RMP table. The recommendation is that guest should try
with a smaller page size.

I would prefer to pass the pvalidate error as-is to caller so that it
can make the correct decision.
I am perfectly fine if the function return UINTN and then use #define
instead of the enum to define the PVALIDATE return code. So that caller
can check the error code. Let me know your thought on #define instead of
the enum.

-Brijesh


Brijesh Singh
 

On 5/4/21 2:55 PM, Brijesh Singh via groups.io wrote:
On 5/4/21 2:07 PM, Brijesh Singh via groups.io wrote:
Return EFI_UNSUPPORTED (0x8000_0003), or even EFI_NO_MAPPING
(0x8000_0017), for value 6 (FAIL_SIZEMISMATCH).
I am not sure if we really want to do this. You will see later in the
patches that in some cases the PVALIDATE will return a failure and we
will need to know the failure code to determine the next steps.
Especially this particular error code is used later. This error happens
when the page size of the backing pages does not match with the
pvalidated size. In those cases we need to retry the PVALIDATE with
lower page size so that a validation succeed. One such a example is:

- Guest ask hypervisor to add the page as 2M in RMP table.

- Hypervisor added the page as 512 4K pages - because it was not able to
find a large backing pages.

- Guest attempts to pvalidate the page as a 2M. The pvalidate will
return a failure saying its a size mismatch between the requested
pvalidated and RMP table. The recommendation is that guest should try
with a smaller page size.

I would prefer to pass the pvalidate error as-is to caller so that it
can make the correct decision.
I am perfectly fine if the function return UINTN and then use #define
instead of the enum to define the PVALIDATE return code. So that caller
can check the error code. Let me know your thought on #define instead of
the enum.
Apologies, I missed the fact that you said document the mapping between
the PVALIDATE return value and EFI_STATUS. So a caller is responsible to
look at the EFI document to know what the error code means. The
unsupported here does not mean that PVALIDATE is not support on
platform. I am good with it. I will go ahead with it.

-Brijesh


Brijesh Singh
 

On 5/4/21 3:28 PM, Brijesh Singh wrote:
On 5/4/21 2:55 PM, Brijesh Singh via groups.io wrote:
On 5/4/21 2:07 PM, Brijesh Singh via groups.io wrote:
Return EFI_UNSUPPORTED (0x8000_0003), or even EFI_NO_MAPPING
(0x8000_0017), for value 6 (FAIL_SIZEMISMATCH).
I am not sure if we really want to do this. You will see later in the
patches that in some cases the PVALIDATE will return a failure and we
will need to know the failure code to determine the next steps.
Especially this particular error code is used later. This error happens
when the page size of the backing pages does not match with the
pvalidated size. In those cases we need to retry the PVALIDATE with
lower page size so that a validation succeed. One such a example is:

- Guest ask hypervisor to add the page as 2M in RMP table.

- Hypervisor added the page as 512 4K pages - because it was not able to
find a large backing pages.

- Guest attempts to pvalidate the page as a 2M. The pvalidate will
return a failure saying its a size mismatch between the requested
pvalidated and RMP table. The recommendation is that guest should try
with a smaller page size.

I would prefer to pass the pvalidate error as-is to caller so that it
can make the correct decision.
I am perfectly fine if the function return UINTN and then use #define
instead of the enum to define the PVALIDATE return code. So that caller
can check the error code. Let me know your thought on #define instead of
the enum.
Apologies, I missed the fact that you said document the mapping between
the PVALIDATE return value and EFI_STATUS. So a caller is responsible to
look at the EFI document to know what the error code means. The
unsupported here does not mean that PVALIDATE is not support on
platform. I am good with it. I will go ahead with it.
While coding it I am coming to realizing that mapping from PVALIDATE
return value to EFI will add more code in assemblym and wanted to make
sure that we all are okay with it. The proposed mapping looks like this:

// No mapping required, both the values are zero

PVALIDATE_RET_SUCCESS -> EFI_SUCCESS   

// Pvalidate.nasm need to map from PVALIDATE fail input (1) to
EFI_INVALID_PRAMS (2)

PVALIATE_RET_FAIL_INPUT -> EFI_INVALID_PARAMS

// Pvalidate.nasm need to map from PVALIDATE SIZE_MISMATCH (6) to
EFI_UNSUPPORT(3)

PVALIATE_RET_SIZEMISMATCH -> EFI_UNSUPPORTED

May I recommend to use UINTN and simply propagate the PVALIDATE return
value  or use the below mapping to simplify the pvalidate.nasm
implementation:

PVALIDATE_RET_SUCCESS(0) -> EFI_SUCCESS(0)

PVALIDATE_RET_FAIL_INPUT(1) -> EFI_LOAD_ERROR(1)

PVALIDATE_RET_SIZE_MISMATCH(6) -> EFI_NOT_READY(6)

Thanks


Laszlo Ersek
 

On 05/04/21 21:07, Brijesh Singh wrote:

On 5/4/21 8:58 AM, Laszlo Ersek wrote:
(4) The order of parameters listed in this comment block differs from
the actual parameter list.

The ECC plugin of the edk2 CI will catch this issue anyway. So,
before submitting the patch set to the list, please submit a personal
PR on github.com against the main repo, just to run CI on your
patches.
Interestingly I did ran the series with edk2 CI and everything passed
before I submitted for the review. But, I will go ahead and fix the
order.
Thank you for being careful up-front -- and then I don't understand this
result. The "EccCheck" plugin is not disabled in
"MdePkg/MdePkg.ci.yaml", and I distinctly remember ECC being incredibly
pedantic about any function leading comment matching the function's
declaration.

Anyway, thanks for updating this, in the next version.


Return EFI_UNSUPPORTED (0x8000_0003), or even EFI_NO_MAPPING
(0x8000_0017), for value 6 (FAIL_SIZEMISMATCH).
I am not sure if we really want to do this. You will see later in the
patches that in some cases the PVALIDATE will return a failure and we
will need to know the failure code to determine the next steps.
Especially this particular error code is used later. This error
happens when the page size of the backing pages does not match with
the pvalidated size. In those cases we need to retry the PVALIDATE
with lower page size so that a validation succeed. One such a example
is:

- Guest ask hypervisor to add the page as 2M in RMP table.

- Hypervisor added the page as 512 4K pages - because it was not able
to find a large backing pages.

- Guest attempts to pvalidate the page as a 2M. The pvalidate will
return a failure saying its a size mismatch between the requested
pvalidated and RMP table. The recommendation is that guest should try
with a smaller page size.

I would prefer to pass the pvalidate error as-is to caller so that it
can make the correct decision.
My intent is certainly not to mask, or collapse, distinct outputs from
PVALIDATE.

The EFI_STATUS codes that I suggested for AsmPvalidate() keep the
distinct PVALIDATE results distinct. The AMD APM (vol3) describes three
result codes (SUCCESS, FAIL_INPUT, FAIL_SIZEMISMATCH), and I suggested a
*bijective* EFI_STATUS mapping for those -- EFI_SUCCESS,
EFI_INVALID_PARAMETER, and EFI_UNSUPPORTED; in that order.

EFI_NO_MAPPING was only an alternative for EFI_UNSUPPORTED -- in case
you preferred EFI_NO_MAPPING to EFI_UNSUPPORTED, for representing
FAIL_SIZEMISMATCH.

Each one of the EFI_STATUS codes I suggest corresponds uniquely to a
direct PVALIDATE result code (injectivity), and each direct PVALIDATE
result code is covered (surjectivity). So it's a bijection.

CF is only meaningful on output if PVALIDATE succeeds, according to the
AMD APM:

If the instruction completed successfully, the rFLAGS bit CF
indicates if the contents of the RMP entry were changed or not.

Therefore CF should only be checked (and it *can* be checked, via the
BOOLEAN "RmpEntryUpdated" output parameter that I also recommended) if
EFI_SUCCESS is returned by AsmPvalidate().


Furthermore, I *did* check the (sole) AsmPvalidate() call site in the
series, before posting my earlier comment. It goes like this, in patch
#21 ("OvmfPkg/MemEncryptSevLib: Add support to validate system RAM"):

Ret = AsmPvalidate (RmpPageSize, Validate, Address, &EFlags);

//
// Check the rFlags.CF to verify that PVALIDATE updated the RMP entry.
// If there was a no change in the RMP entry then we are either double
// validating or invalidating the memory. This can lead to a security compromise.
//
if (EFlags.Bits.CF) {
DEBUG ((
DEBUG_ERROR, "%a:%a: Double %a detected for address 0x%Lx\n",
gEfiCallerBaseName,
__FUNCTION__,
Validate ? "Validate" : "Invalidate",
Address
));
SnpPageStateFailureTerminate ();
}

return Ret;
This logic is incorrect, in my opinion. CF should not be checked before
ensuring that Ret equal SUCCESS.

The correct logic for IssuePvalidate() would be:

Status = AsmPvalidate (RmpPageSize, Validate, Address, &RmpEntryUpdated);
if (EFI_ERROR (Status)) {
return Status;
}
if (!RmpEntryUpdated) {
SnpPageStateFailureTerminate ();
}

And then the caller of IssuePvalidate() -- PvalidateRange() -- can rely
on the status codes, returned by IssuePvalidate(), with the following
meanings:

- EFI_SUCCESS: PVALIDATE completed *and* it successfully updated the RMP
entry

- EFI_INVALID_PARAMETER: PVALIDATE failed with FAIL_INPUT (and CF was
meaningless on output)

- EFI_UNSUPPORTED (or EFI_NO_MAPPING, if you like): PVALIDATE failed
with FAIL_SIZEMISMATCH (and CF was meaningless on output); PVALIDATE
should be retried with the small (4K) page size.


I entirely agree with your high-level goal here. My point is that the
mapping that I'm proposing loses *no* information, it is more idiomatic
for edk2, and it's not difficult to implement in assembly.

If you absolutely insist on returning status codes 0, 1, and 6, from
AsmPvalidate(), I can live with that too (although I do think it's a lot
less idiomatic for edk2); in that case however, please make the return
type of AsmPvalidate() UINT32.

(And to repeat: my other point is that the CF checking logic in
IssuePvalidate() is currently wrong, as it relies on a value (CF) which
may be indeterminate in some cases.)

Thanks!
Laszlo


Laszlo Ersek
 

On 05/04/21 21:55, Brijesh Singh wrote:

On 5/4/21 2:07 PM, Brijesh Singh via groups.io wrote:
Return EFI_UNSUPPORTED (0x8000_0003), or even EFI_NO_MAPPING
(0x8000_0017), for value 6 (FAIL_SIZEMISMATCH).
I am not sure if we really want to do this. You will see later in the
patches that in some cases the PVALIDATE will return a failure and we
will need to know the failure code to determine the next steps.
Especially this particular error code is used later. This error happens
when the page size of the backing pages does not match with the
pvalidated size. In those cases we need to retry the PVALIDATE with
lower page size so that a validation succeed. One such a example is:

- Guest ask hypervisor to add the page as 2M in RMP table.

- Hypervisor added the page as 512 4K pages - because it was not able to
find a large backing pages.

- Guest attempts to pvalidate the page as a 2M. The pvalidate will
return a failure saying its a size mismatch between the requested
pvalidated and RMP table. The recommendation is that guest should try
with a smaller page size.

I would prefer to pass the pvalidate error as-is to caller so that it
can make the correct decision.
I am perfectly fine if the function return UINTN and then use #define
instead of the enum to define the PVALIDATE return code. So that caller
can check the error code. Let me know your thought on #define instead of
the enum.
(1) If the funcion returns UINTN rather than an enum type, that resolves
my request, technically speaking. I think it's inferior to the
EFI_STATUS mapping that I proposed, but it certainly resolves my request!

(2) How you provide the symbolic names for the values 0, 1, 6, does not
matter. If you use #define's (= macros), that's fine. If you use an enum
type, with enum constants, that's also fine. This aspect is orthogonal
to my request.

Thanks!
Laszlo


Laszlo Ersek
 

On 05/04/21 22:28, Brijesh Singh wrote:

On 5/4/21 2:55 PM, Brijesh Singh via groups.io wrote:
On 5/4/21 2:07 PM, Brijesh Singh via groups.io wrote:
Return EFI_UNSUPPORTED (0x8000_0003), or even EFI_NO_MAPPING
(0x8000_0017), for value 6 (FAIL_SIZEMISMATCH).
I am not sure if we really want to do this. You will see later in the
patches that in some cases the PVALIDATE will return a failure and we
will need to know the failure code to determine the next steps.
Especially this particular error code is used later. This error happens
when the page size of the backing pages does not match with the
pvalidated size. In those cases we need to retry the PVALIDATE with
lower page size so that a validation succeed. One such a example is:

- Guest ask hypervisor to add the page as 2M in RMP table.

- Hypervisor added the page as 512 4K pages - because it was not able to
find a large backing pages.

- Guest attempts to pvalidate the page as a 2M. The pvalidate will
return a failure saying its a size mismatch between the requested
pvalidated and RMP table. The recommendation is that guest should try
with a smaller page size.

I would prefer to pass the pvalidate error as-is to caller so that it
can make the correct decision.
I am perfectly fine if the function return UINTN and then use #define
instead of the enum to define the PVALIDATE return code. So that caller
can check the error code. Let me know your thought on #define instead of
the enum.
I'm sorry that I'm responding out-of-order to this email of yours -- for
some reason, your email is not correctly threaded in my list folder. It
is not connected to the 05/28 sub-thread, but to the blurb.

Apologies, I missed the fact that you said document the mapping between
the PVALIDATE return value and EFI_STATUS. So a caller is responsible to
look at the EFI document to know what the error code means. The
unsupported here does not mean that PVALIDATE is not support on
platform.
That's right. First, you are only providing an X64 function definition.
Second, I requested that even the *declaration* of the function be
restricted to X64. Third, I requested that we document in the leading
comment that, unless CPUID reports support for PVALIDATE, the function
will trigger an #UD; it's the caller's responsibility *not* to call the
function, then. With those in mind, EFI_UNSUPPORTED *need not* stand for
any particular "platform characteristic", it can stand for whatever we
want it to -- such as mismatched page size. Still, in case you disliked
this ambiguity of EFI_UNSUPPORTED, I offered EFI_NO_MAPPING, for the
page size mismatch case.

I am good with it. I will go ahead with it.
Thanks -- if you still prefer the UINTN approach, I can live with that
too (although EFI_STATUS would certainly make me happier).

Thank you!
Laszlo


Laszlo Ersek
 

On 05/05/21 01:03, Brijesh Singh wrote:

On 5/4/21 3:28 PM, Brijesh Singh wrote:
On 5/4/21 2:55 PM, Brijesh Singh via groups.io wrote:
On 5/4/21 2:07 PM, Brijesh Singh via groups.io wrote:
Return EFI_UNSUPPORTED (0x8000_0003), or even EFI_NO_MAPPING
(0x8000_0017), for value 6 (FAIL_SIZEMISMATCH).
I am not sure if we really want to do this. You will see later in the
patches that in some cases the PVALIDATE will return a failure and we
will need to know the failure code to determine the next steps.
Especially this particular error code is used later. This error happens
when the page size of the backing pages does not match with the
pvalidated size. In those cases we need to retry the PVALIDATE with
lower page size so that a validation succeed. One such a example is:

- Guest ask hypervisor to add the page as 2M in RMP table.

- Hypervisor added the page as 512 4K pages - because it was not able to
find a large backing pages.

- Guest attempts to pvalidate the page as a 2M. The pvalidate will
return a failure saying its a size mismatch between the requested
pvalidated and RMP table. The recommendation is that guest should try
with a smaller page size.

I would prefer to pass the pvalidate error as-is to caller so that it
can make the correct decision.
I am perfectly fine if the function return UINTN and then use #define
instead of the enum to define the PVALIDATE return code. So that caller
can check the error code. Let me know your thought on #define instead of
the enum.
Apologies, I missed the fact that you said document the mapping between
the PVALIDATE return value and EFI_STATUS. So a caller is responsible to
look at the EFI document to know what the error code means. The
unsupported here does not mean that PVALIDATE is not support on
platform. I am good with it. I will go ahead with it.
While coding it I am coming to realizing that mapping from PVALIDATE
return value to EFI will add more code in assemblym and wanted to make
sure that we all are okay with it. The proposed mapping looks like this:

// No mapping required, both the values are zero

PVALIDATE_RET_SUCCESS -> EFI_SUCCESS   

// Pvalidate.nasm need to map from PVALIDATE fail input (1) to
EFI_INVALID_PRAMS (2)

PVALIATE_RET_FAIL_INPUT -> EFI_INVALID_PARAMS

// Pvalidate.nasm need to map from PVALIDATE SIZE_MISMATCH (6) to
EFI_UNSUPPORT(3)

PVALIATE_RET_SIZEMISMATCH -> EFI_UNSUPPORTED

May I recommend to use UINTN and simply propagate the PVALIDATE return
value  or use the below mapping to simplify the pvalidate.nasm
implementation:

PVALIDATE_RET_SUCCESS(0) -> EFI_SUCCESS(0)

PVALIDATE_RET_FAIL_INPUT(1) -> EFI_LOAD_ERROR(1)

PVALIDATE_RET_SIZE_MISMATCH(6) -> EFI_NOT_READY(6)
The additional complexity in the assembly source code is not lost on me.
I'm receptive to that.

I think EFI_LOAD_ERROR and EFI_NOT_READY would do more damage than good;
those error codes are meaningless in this context.

Therefore, please go ahead with the UINTN return type. And, whether you
pick #defines, or enum constants, for the symbolic names (for the
comparisons at the call sites), that's up to you.

Thanks!
Laszlo