[PATCH RFC v2 04/28] MdePkg: Define the Page State Change VMGEXIT structures


Brijesh Singh
 

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275

The Page State Change NAE exit will be used by the SEV-SNP guest to
request a page state change using the GHCB protocol. See the GHCB
spec section 4.1.6 and 2.3.1 for more detail on the structure
definitions.

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/Register/Amd/Fam17Msr.h | 15 ++++++++++
MdePkg/Include/Register/Amd/Ghcb.h | 29 ++++++++++++++++++++
2 files changed, 44 insertions(+)

diff --git a/MdePkg/Include/Register/Amd/Fam17Msr.h b/MdePkg/Include/Register/Amd/Fam17Msr.h
index e19bd04b6c..432cee2feb 100644
--- a/MdePkg/Include/Register/Amd/Fam17Msr.h
+++ b/MdePkg/Include/Register/Amd/Fam17Msr.h
@@ -58,6 +58,19 @@ typedef union {
UINT64 GuestFrameNumber:52;
} GhcbGpaRegister;

+ struct {
+ UINT64 Function:12;
+ UINT64 GuestFrameNumber:40;
+ UINT64 Operation:4;
+ UINT64 Reserved:8;
+ } SnpPageStateChangeRequest;
+
+ struct {
+ UINT32 Function:12;
+ UINT32 Reserved:20;
+ UINT32 ErrorCode;
+ } SnpPageStateChangeResponse;
+
VOID *Ghcb;

UINT64 GhcbPhysicalAddress;
@@ -69,6 +82,8 @@ typedef union {
#define GHCB_INFO_CPUID_RESPONSE 5
#define GHCB_INFO_GHCB_GPA_REGISTER_REQUEST 18
#define GHCB_INFO_GHCB_GPA_REGISTER_RESPONSE 19
+#define GHCB_INFO_SNP_PAGE_STATE_CHANGE_REQUEST 20
+#define GHCB_INFO_SNP_PAGE_STATE_CHANGE_RESPONSE 21
#define GHCB_HYPERVISOR_FEATURES_REQUEST 128
#define GHCB_HYPERVISOR_FEATURES_RESPONSE 129
#define GHCB_INFO_TERMINATE_REQUEST 256
diff --git a/MdePkg/Include/Register/Amd/Ghcb.h b/MdePkg/Include/Register/Amd/Ghcb.h
index 2d64a4c28f..1e7c0daed3 100644
--- a/MdePkg/Include/Register/Amd/Ghcb.h
+++ b/MdePkg/Include/Register/Amd/Ghcb.h
@@ -54,6 +54,7 @@
#define SVM_EXIT_NMI_COMPLETE 0x80000003ULL
#define SVM_EXIT_AP_RESET_HOLD 0x80000004ULL
#define SVM_EXIT_AP_JUMP_TABLE 0x80000005ULL
+#define SVM_EXIT_SNP_PAGE_STATE_CHANGE 0x80000010ULL
#define SVM_EXIT_HYPERVISOR_FEATURES 0x8000FFFDULL
#define SVM_EXIT_UNSUPPORTED 0x8000FFFFULL

@@ -160,4 +161,32 @@ typedef union {
#define GHCB_HV_FEATURES_SNP_AP_CREATE (GHCB_HV_FEATURES_SNP | BIT1)
#define GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION (GHCB_HV_FEATURES_SNP_AP_CREATE | BIT2)
#define GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION_TIMER (GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION | BIT3)
+
+// SNP Page State Change
+#define SNP_PAGE_STATE_MAX_NPAGES 4095
+#define SNP_PAGE_STATE_MAX_ENTRY 253
+#define SNP_PAGE_STATE_PRIVATE 1
+#define SNP_PAGE_STATE_SHARED 2
+#define SNP_PAGE_STATE_PSMASH 3
+#define SNP_PAGE_STATE_UNSMASH 4
+
+typedef PACKED struct {
+ UINT64 CurrentPage:12;
+ UINT64 GuestFrameNumber:40;
+ UINT64 Op:4;
+ UINT64 PageSize:1;
+ UINT64 Rsvd: 7;
+} SNP_PAGE_STATE_ENTRY;
+
+typedef PACKED struct {
+ UINT16 CurrentEntry;
+ UINT16 EndEntry;
+ UINT32 Rsvd;
+} SNP_PAGE_STATE_HEADER;
+
+typedef struct {
+ SNP_PAGE_STATE_HEADER Header;
+ SNP_PAGE_STATE_ENTRY Entry[SNP_PAGE_STATE_MAX_ENTRY];
+} SNP_PAGE_STATE_CHANGE_INFO;
+
#endif
--
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 Page State Change NAE exit will be used by the SEV-SNP guest to
request a page state change using the GHCB protocol. See the GHCB
spec section 4.1.6 and 2.3.1 for more detail on the structure
definitions.

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/Register/Amd/Fam17Msr.h | 15 ++++++++++
MdePkg/Include/Register/Amd/Ghcb.h | 29 ++++++++++++++++++++
2 files changed, 44 insertions(+)

diff --git a/MdePkg/Include/Register/Amd/Fam17Msr.h b/MdePkg/Include/Register/Amd/Fam17Msr.h
index e19bd04b6c..432cee2feb 100644
--- a/MdePkg/Include/Register/Amd/Fam17Msr.h
+++ b/MdePkg/Include/Register/Amd/Fam17Msr.h
@@ -58,6 +58,19 @@ typedef union {
UINT64 GuestFrameNumber:52;
} GhcbGpaRegister;

+ struct {
+ UINT64 Function:12;
+ UINT64 GuestFrameNumber:40;
+ UINT64 Operation:4;
+ UINT64 Reserved:8;
+ } SnpPageStateChangeRequest;
+
+ struct {
+ UINT32 Function:12;
+ UINT32 Reserved:20;
+ UINT32 ErrorCode;
+ } SnpPageStateChangeResponse;
+
VOID *Ghcb;
This matches section 2.3.1 in rev 2.00.

UINT64 GhcbPhysicalAddress;
@@ -69,6 +82,8 @@ typedef union {
#define GHCB_INFO_CPUID_RESPONSE 5
#define GHCB_INFO_GHCB_GPA_REGISTER_REQUEST 18
#define GHCB_INFO_GHCB_GPA_REGISTER_RESPONSE 19
+#define GHCB_INFO_SNP_PAGE_STATE_CHANGE_REQUEST 20
+#define GHCB_INFO_SNP_PAGE_STATE_CHANGE_RESPONSE 21
#define GHCB_HYPERVISOR_FEATURES_REQUEST 128
#define GHCB_HYPERVISOR_FEATURES_RESPONSE 129
#define GHCB_INFO_TERMINATE_REQUEST 256
Matches section 2.3.1.

diff --git a/MdePkg/Include/Register/Amd/Ghcb.h b/MdePkg/Include/Register/Amd/Ghcb.h
index 2d64a4c28f..1e7c0daed3 100644
--- a/MdePkg/Include/Register/Amd/Ghcb.h
+++ b/MdePkg/Include/Register/Amd/Ghcb.h
@@ -54,6 +54,7 @@
#define SVM_EXIT_NMI_COMPLETE 0x80000003ULL
#define SVM_EXIT_AP_RESET_HOLD 0x80000004ULL
#define SVM_EXIT_AP_JUMP_TABLE 0x80000005ULL
+#define SVM_EXIT_SNP_PAGE_STATE_CHANGE 0x80000010ULL
#define SVM_EXIT_HYPERVISOR_FEATURES 0x8000FFFDULL
#define SVM_EXIT_UNSUPPORTED 0x8000FFFFULL
Matches "Table 5. List of Supported Non-Automatic Events".

@@ -160,4 +161,32 @@ typedef union {
#define GHCB_HV_FEATURES_SNP_AP_CREATE (GHCB_HV_FEATURES_SNP | BIT1)
#define GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION (GHCB_HV_FEATURES_SNP_AP_CREATE | BIT2)
#define GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION_TIMER (GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION | BIT3)
+
+// SNP Page State Change
(1) Comment style.

+#define SNP_PAGE_STATE_MAX_NPAGES 4095
+#define SNP_PAGE_STATE_MAX_ENTRY 253
+#define SNP_PAGE_STATE_PRIVATE 1
+#define SNP_PAGE_STATE_SHARED 2
+#define SNP_PAGE_STATE_PSMASH 3
+#define SNP_PAGE_STATE_UNSMASH 4
(2) The PSMASH and UNSMASH operations are not documented in the rev 2.00
spec, in the GHCB MSR protocol. That's probably because PSMASH and
UNSMASH can only be defined in terms of 2MB pages, and
GHCB_INFO_SNP_PAGE_STATE_CHANGE_REQUEST is suitable only for individual,
4KB pages. I think it would be useful to point out somehow here that
PSMASH and UNSMASH are restricted to the GHCB shared area protocol
(perhaps extend the leading comment on this block of macros).

(3) I don't understand what "MAX_NPAGES" stands for (4095). The rest of
the series never uses the macro, and I can't associate it with anything
from the spec. If the macro is supposed to relate to the 4KB / 2MB page
smashing / splitting, then its replacement text should be 512. Unless
the macro corresponds to a definition in the spec, I think we should
drop it.

+
+typedef PACKED struct {
+ UINT64 CurrentPage:12;
+ UINT64 GuestFrameNumber:40;
+ UINT64 Op:4;
+ UINT64 PageSize:1;
+ UINT64 Rsvd: 7;
+} SNP_PAGE_STATE_ENTRY;
+
+typedef PACKED struct {
+ UINT16 CurrentEntry;
+ UINT16 EndEntry;
+ UINT32 Rsvd;
+} SNP_PAGE_STATE_HEADER;
(4) We tend to write

#pragma pack (1)
...
#pragma pack ()

rather than PACKED -- but anyway, is packing really necessary? "Natural
alignment" is required in edk2. I'm OK with packing, but I think the
pragma is the preferred form.

(5) Please spell out both "Rsvd" fields above as "Reserved".

(6) Stray space character in "Rsvd: 7".

(7) The field name "Op" is inconsistent with the other field name
"Operation".

(8) I think there is a bug (typo) in the rev 2.00 spec, in 4.1.6 "SNP
Page State Change": it says

... calculated from the supplied guest physical frame number (GFN) for
the requested page size (GPA = GFN << 12).

But, if you can choose 2MB page size in the request, then the (GPA = GFN
<< 12) formula is not g

(9) If my understanding of the spec is correct, "EndEntry" has
*inclusive* meaning. That's unusual. Any particular reason for not
making "EndEntry" exclusive (in the spec)?

+
+typedef struct {
+ SNP_PAGE_STATE_HEADER Header;
+ SNP_PAGE_STATE_ENTRY Entry[SNP_PAGE_STATE_MAX_ENTRY];
+} SNP_PAGE_STATE_CHANGE_INFO;
+
#endif
Yes, this looks OK. Size is 2+2+4+253*8 = 2032 bytes, which matches the
size of GHCB.SharedBuffer.

(10) However, *if* you decide to declare SNP_PAGE_STATE_ENTRY and
SNP_PAGE_STATE_HEADER explicitly as packed, then you should do the same
for SNP_PAGE_STATE_CHANGE_INFO.

(11) Like I mentioned earlier, it's probably helpful if you start the
subject line with

MdePkg/Register/Amd: ...

on all of these MdePkg patches. If that becomes too tight, for some of
the MdePkg patches, then I suggest "MdePkg/Amd: ..." (i.e., drop
"Register").

Thanks
Laszlo


Laszlo Ersek
 

On 05/04/21 14:33, Laszlo Ersek wrote:
On 04/30/21 13:51, Brijesh Singh wrote:
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275

The Page State Change NAE exit will be used by the SEV-SNP guest to
request a page state change using the GHCB protocol. See the GHCB
spec section 4.1.6 and 2.3.1 for more detail on the structure
definitions.

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/Register/Amd/Fam17Msr.h | 15 ++++++++++
MdePkg/Include/Register/Amd/Ghcb.h | 29 ++++++++++++++++++++
2 files changed, 44 insertions(+)

diff --git a/MdePkg/Include/Register/Amd/Fam17Msr.h b/MdePkg/Include/Register/Amd/Fam17Msr.h
index e19bd04b6c..432cee2feb 100644
--- a/MdePkg/Include/Register/Amd/Fam17Msr.h
+++ b/MdePkg/Include/Register/Amd/Fam17Msr.h
@@ -58,6 +58,19 @@ typedef union {
UINT64 GuestFrameNumber:52;
} GhcbGpaRegister;

+ struct {
+ UINT64 Function:12;
+ UINT64 GuestFrameNumber:40;
+ UINT64 Operation:4;
+ UINT64 Reserved:8;
+ } SnpPageStateChangeRequest;
+
+ struct {
+ UINT32 Function:12;
+ UINT32 Reserved:20;
+ UINT32 ErrorCode;
+ } SnpPageStateChangeResponse;
+
VOID *Ghcb;
This matches section 2.3.1 in rev 2.00.

UINT64 GhcbPhysicalAddress;
@@ -69,6 +82,8 @@ typedef union {
#define GHCB_INFO_CPUID_RESPONSE 5
#define GHCB_INFO_GHCB_GPA_REGISTER_REQUEST 18
#define GHCB_INFO_GHCB_GPA_REGISTER_RESPONSE 19
+#define GHCB_INFO_SNP_PAGE_STATE_CHANGE_REQUEST 20
+#define GHCB_INFO_SNP_PAGE_STATE_CHANGE_RESPONSE 21
#define GHCB_HYPERVISOR_FEATURES_REQUEST 128
#define GHCB_HYPERVISOR_FEATURES_RESPONSE 129
#define GHCB_INFO_TERMINATE_REQUEST 256
Matches section 2.3.1.

diff --git a/MdePkg/Include/Register/Amd/Ghcb.h b/MdePkg/Include/Register/Amd/Ghcb.h
index 2d64a4c28f..1e7c0daed3 100644
--- a/MdePkg/Include/Register/Amd/Ghcb.h
+++ b/MdePkg/Include/Register/Amd/Ghcb.h
@@ -54,6 +54,7 @@
#define SVM_EXIT_NMI_COMPLETE 0x80000003ULL
#define SVM_EXIT_AP_RESET_HOLD 0x80000004ULL
#define SVM_EXIT_AP_JUMP_TABLE 0x80000005ULL
+#define SVM_EXIT_SNP_PAGE_STATE_CHANGE 0x80000010ULL
#define SVM_EXIT_HYPERVISOR_FEATURES 0x8000FFFDULL
#define SVM_EXIT_UNSUPPORTED 0x8000FFFFULL
Matches "Table 5. List of Supported Non-Automatic Events".

@@ -160,4 +161,32 @@ typedef union {
#define GHCB_HV_FEATURES_SNP_AP_CREATE (GHCB_HV_FEATURES_SNP | BIT1)
#define GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION (GHCB_HV_FEATURES_SNP_AP_CREATE | BIT2)
#define GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION_TIMER (GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION | BIT3)
+
+// SNP Page State Change
(1) Comment style.

+#define SNP_PAGE_STATE_MAX_NPAGES 4095
+#define SNP_PAGE_STATE_MAX_ENTRY 253
+#define SNP_PAGE_STATE_PRIVATE 1
+#define SNP_PAGE_STATE_SHARED 2
+#define SNP_PAGE_STATE_PSMASH 3
+#define SNP_PAGE_STATE_UNSMASH 4
(2) The PSMASH and UNSMASH operations are not documented in the rev 2.00
spec, in the GHCB MSR protocol. That's probably because PSMASH and
UNSMASH can only be defined in terms of 2MB pages, and
GHCB_INFO_SNP_PAGE_STATE_CHANGE_REQUEST is suitable only for individual,
4KB pages. I think it would be useful to point out somehow here that
PSMASH and UNSMASH are restricted to the GHCB shared area protocol
(perhaps extend the leading comment on this block of macros).

(3) I don't understand what "MAX_NPAGES" stands for (4095). The rest of
the series never uses the macro, and I can't associate it with anything
from the spec. If the macro is supposed to relate to the 4KB / 2MB page
smashing / splitting, then its replacement text should be 512. Unless
the macro corresponds to a definition in the spec, I think we should
drop it.

+
+typedef PACKED struct {
+ UINT64 CurrentPage:12;
+ UINT64 GuestFrameNumber:40;
+ UINT64 Op:4;
+ UINT64 PageSize:1;
+ UINT64 Rsvd: 7;
+} SNP_PAGE_STATE_ENTRY;
+
+typedef PACKED struct {
+ UINT16 CurrentEntry;
+ UINT16 EndEntry;
+ UINT32 Rsvd;
+} SNP_PAGE_STATE_HEADER;
(4) We tend to write

#pragma pack (1)
...
#pragma pack ()

rather than PACKED -- but anyway, is packing really necessary? "Natural
alignment" is required in edk2. I'm OK with packing, but I think the
pragma is the preferred form.

(5) Please spell out both "Rsvd" fields above as "Reserved".

(6) Stray space character in "Rsvd: 7".

(7) The field name "Op" is inconsistent with the other field name
"Operation".

(8) I think there is a bug (typo) in the rev 2.00 spec, in 4.1.6 "SNP
Page State Change": it says

... calculated from the supplied guest physical frame number (GFN) for
the requested page size (GPA = GFN << 12).

But, if you can choose 2MB page size in the request, then the (GPA = GFN
<< 12) formula is not g
Sorry, unfinished sentence: I meant that the formula was not generally
correct.

Thanks
Laszlo


(9) If my understanding of the spec is correct, "EndEntry" has
*inclusive* meaning. That's unusual. Any particular reason for not
making "EndEntry" exclusive (in the spec)?

+
+typedef struct {
+ SNP_PAGE_STATE_HEADER Header;
+ SNP_PAGE_STATE_ENTRY Entry[SNP_PAGE_STATE_MAX_ENTRY];
+} SNP_PAGE_STATE_CHANGE_INFO;
+
#endif
Yes, this looks OK. Size is 2+2+4+253*8 = 2032 bytes, which matches the
size of GHCB.SharedBuffer.

(10) However, *if* you decide to declare SNP_PAGE_STATE_ENTRY and
SNP_PAGE_STATE_HEADER explicitly as packed, then you should do the same
for SNP_PAGE_STATE_CHANGE_INFO.

(11) Like I mentioned earlier, it's probably helpful if you start the
subject line with

MdePkg/Register/Amd: ...

on all of these MdePkg patches. If that becomes too tight, for some of
the MdePkg patches, then I suggest "MdePkg/Amd: ..." (i.e., drop
"Register").

Thanks
Laszlo


Lendacky, Thomas
 

On 5/4/21 8:59 AM, Laszlo Ersek wrote:
On 05/04/21 14:33, 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%7Cthomas.lendacky%40amd.com%7Cf400bca14b6f4f138a1908d90f05090f%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637557336582189771%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=bjXNVWGpurRGUZkjemvDQR%2FYnEQRG9ENN22jUjtkNP0%3D&amp;reserved=0

The Page State Change NAE exit will be used by the SEV-SNP guest to
request a page state change using the GHCB protocol. See the GHCB
spec section 4.1.6 and 2.3.1 for more detail on the structure
definitions.

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/Register/Amd/Fam17Msr.h | 15 ++++++++++
MdePkg/Include/Register/Amd/Ghcb.h | 29 ++++++++++++++++++++
2 files changed, 44 insertions(+)

diff --git a/MdePkg/Include/Register/Amd/Fam17Msr.h b/MdePkg/Include/Register/Amd/Fam17Msr.h
index e19bd04b6c..432cee2feb 100644
--- a/MdePkg/Include/Register/Amd/Fam17Msr.h
+++ b/MdePkg/Include/Register/Amd/Fam17Msr.h
@@ -58,6 +58,19 @@ typedef union {
UINT64 GuestFrameNumber:52;
} GhcbGpaRegister;

+ struct {
+ UINT64 Function:12;
+ UINT64 GuestFrameNumber:40;
+ UINT64 Operation:4;
+ UINT64 Reserved:8;
+ } SnpPageStateChangeRequest;
+
+ struct {
+ UINT32 Function:12;
+ UINT32 Reserved:20;
+ UINT32 ErrorCode;
+ } SnpPageStateChangeResponse;
+
VOID *Ghcb;
This matches section 2.3.1 in rev 2.00.

UINT64 GhcbPhysicalAddress;
@@ -69,6 +82,8 @@ typedef union {
#define GHCB_INFO_CPUID_RESPONSE 5
#define GHCB_INFO_GHCB_GPA_REGISTER_REQUEST 18
#define GHCB_INFO_GHCB_GPA_REGISTER_RESPONSE 19
+#define GHCB_INFO_SNP_PAGE_STATE_CHANGE_REQUEST 20
+#define GHCB_INFO_SNP_PAGE_STATE_CHANGE_RESPONSE 21
#define GHCB_HYPERVISOR_FEATURES_REQUEST 128
#define GHCB_HYPERVISOR_FEATURES_RESPONSE 129
#define GHCB_INFO_TERMINATE_REQUEST 256
Matches section 2.3.1.

diff --git a/MdePkg/Include/Register/Amd/Ghcb.h b/MdePkg/Include/Register/Amd/Ghcb.h
index 2d64a4c28f..1e7c0daed3 100644
--- a/MdePkg/Include/Register/Amd/Ghcb.h
+++ b/MdePkg/Include/Register/Amd/Ghcb.h
@@ -54,6 +54,7 @@
#define SVM_EXIT_NMI_COMPLETE 0x80000003ULL
#define SVM_EXIT_AP_RESET_HOLD 0x80000004ULL
#define SVM_EXIT_AP_JUMP_TABLE 0x80000005ULL
+#define SVM_EXIT_SNP_PAGE_STATE_CHANGE 0x80000010ULL
#define SVM_EXIT_HYPERVISOR_FEATURES 0x8000FFFDULL
#define SVM_EXIT_UNSUPPORTED 0x8000FFFFULL
Matches "Table 5. List of Supported Non-Automatic Events".

@@ -160,4 +161,32 @@ typedef union {
#define GHCB_HV_FEATURES_SNP_AP_CREATE (GHCB_HV_FEATURES_SNP | BIT1)
#define GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION (GHCB_HV_FEATURES_SNP_AP_CREATE | BIT2)
#define GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION_TIMER (GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION | BIT3)
+
+// SNP Page State Change
(1) Comment style.

+#define SNP_PAGE_STATE_MAX_NPAGES 4095
+#define SNP_PAGE_STATE_MAX_ENTRY 253
+#define SNP_PAGE_STATE_PRIVATE 1
+#define SNP_PAGE_STATE_SHARED 2
+#define SNP_PAGE_STATE_PSMASH 3
+#define SNP_PAGE_STATE_UNSMASH 4
(2) The PSMASH and UNSMASH operations are not documented in the rev 2.00
spec, in the GHCB MSR protocol. That's probably because PSMASH and
UNSMASH can only be defined in terms of 2MB pages, and
GHCB_INFO_SNP_PAGE_STATE_CHANGE_REQUEST is suitable only for individual,
4KB pages. I think it would be useful to point out somehow here that
PSMASH and UNSMASH are restricted to the GHCB shared area protocol
(perhaps extend the leading comment on this block of macros).

(3) I don't understand what "MAX_NPAGES" stands for (4095). The rest of
the series never uses the macro, and I can't associate it with anything
from the spec. If the macro is supposed to relate to the 4KB / 2MB page
smashing / splitting, then its replacement text should be 512. Unless
the macro corresponds to a definition in the spec, I think we should
drop it.

+
+typedef PACKED struct {
+ UINT64 CurrentPage:12;
+ UINT64 GuestFrameNumber:40;
+ UINT64 Op:4;
+ UINT64 PageSize:1;
+ UINT64 Rsvd: 7;
+} SNP_PAGE_STATE_ENTRY;
+
+typedef PACKED struct {
+ UINT16 CurrentEntry;
+ UINT16 EndEntry;
+ UINT32 Rsvd;
+} SNP_PAGE_STATE_HEADER;
(4) We tend to write

#pragma pack (1)
...
#pragma pack ()

rather than PACKED -- but anyway, is packing really necessary? "Natural
alignment" is required in edk2. I'm OK with packing, but I think the
pragma is the preferred form.

(5) Please spell out both "Rsvd" fields above as "Reserved".

(6) Stray space character in "Rsvd: 7".

(7) The field name "Op" is inconsistent with the other field name
"Operation".

(8) I think there is a bug (typo) in the rev 2.00 spec, in 4.1.6 "SNP
Page State Change": it says

... calculated from the supplied guest physical frame number (GFN) for
the requested page size (GPA = GFN << 12).

But, if you can choose 2MB page size in the request, then the (GPA = GFN
<< 12) formula is not g
Sorry, unfinished sentence: I meant that the formula was not generally
correct.
Actually, for any page size, the GPA for any GFN is GFN << 12.

For the SNP Page State Change NAE event, it is up to the hypervisor to
ensure that the GFN/GPA supplied is aligned appropriately for the
requested page size (see 4.1.6 of the GHCB spec where the page operations
are defined). A GFN is naturally 4K aligned, so only a 2MB page size needs
GFN/GPA alignment validation.

For the SNP Page State Change MSR protocol, only a 4K page size is supported.

Thanks,
Tom


Thanks
Laszlo


(9) If my understanding of the spec is correct, "EndEntry" has
*inclusive* meaning. That's unusual. Any particular reason for not
making "EndEntry" exclusive (in the spec)?

+
+typedef struct {
+ SNP_PAGE_STATE_HEADER Header;
+ SNP_PAGE_STATE_ENTRY Entry[SNP_PAGE_STATE_MAX_ENTRY];
+} SNP_PAGE_STATE_CHANGE_INFO;
+
#endif
Yes, this looks OK. Size is 2+2+4+253*8 = 2032 bytes, which matches the
size of GHCB.SharedBuffer.

(10) However, *if* you decide to declare SNP_PAGE_STATE_ENTRY and
SNP_PAGE_STATE_HEADER explicitly as packed, then you should do the same
for SNP_PAGE_STATE_CHANGE_INFO.

(11) Like I mentioned earlier, it's probably helpful if you start the
subject line with

MdePkg/Register/Amd: ...

on all of these MdePkg patches. If that becomes too tight, for some of
the MdePkg patches, then I suggest "MdePkg/Amd: ..." (i.e., drop
"Register").

Thanks
Laszlo


Laszlo Ersek
 

On 05/04/21 16:48, Tom Lendacky wrote:


On 5/4/21 8:59 AM, Laszlo Ersek wrote:
On 05/04/21 14:33, 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%7Cthomas.lendacky%40amd.com%7Cf400bca14b6f4f138a1908d90f05090f%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637557336582189771%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=bjXNVWGpurRGUZkjemvDQR%2FYnEQRG9ENN22jUjtkNP0%3D&amp;reserved=0

The Page State Change NAE exit will be used by the SEV-SNP guest to
request a page state change using the GHCB protocol. See the GHCB
spec section 4.1.6 and 2.3.1 for more detail on the structure
definitions.

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/Register/Amd/Fam17Msr.h | 15 ++++++++++
MdePkg/Include/Register/Amd/Ghcb.h | 29 ++++++++++++++++++++
2 files changed, 44 insertions(+)

diff --git a/MdePkg/Include/Register/Amd/Fam17Msr.h b/MdePkg/Include/Register/Amd/Fam17Msr.h
index e19bd04b6c..432cee2feb 100644
--- a/MdePkg/Include/Register/Amd/Fam17Msr.h
+++ b/MdePkg/Include/Register/Amd/Fam17Msr.h
@@ -58,6 +58,19 @@ typedef union {
UINT64 GuestFrameNumber:52;
} GhcbGpaRegister;

+ struct {
+ UINT64 Function:12;
+ UINT64 GuestFrameNumber:40;
+ UINT64 Operation:4;
+ UINT64 Reserved:8;
+ } SnpPageStateChangeRequest;
+
+ struct {
+ UINT32 Function:12;
+ UINT32 Reserved:20;
+ UINT32 ErrorCode;
+ } SnpPageStateChangeResponse;
+
VOID *Ghcb;
This matches section 2.3.1 in rev 2.00.

UINT64 GhcbPhysicalAddress;
@@ -69,6 +82,8 @@ typedef union {
#define GHCB_INFO_CPUID_RESPONSE 5
#define GHCB_INFO_GHCB_GPA_REGISTER_REQUEST 18
#define GHCB_INFO_GHCB_GPA_REGISTER_RESPONSE 19
+#define GHCB_INFO_SNP_PAGE_STATE_CHANGE_REQUEST 20
+#define GHCB_INFO_SNP_PAGE_STATE_CHANGE_RESPONSE 21
#define GHCB_HYPERVISOR_FEATURES_REQUEST 128
#define GHCB_HYPERVISOR_FEATURES_RESPONSE 129
#define GHCB_INFO_TERMINATE_REQUEST 256
Matches section 2.3.1.

diff --git a/MdePkg/Include/Register/Amd/Ghcb.h b/MdePkg/Include/Register/Amd/Ghcb.h
index 2d64a4c28f..1e7c0daed3 100644
--- a/MdePkg/Include/Register/Amd/Ghcb.h
+++ b/MdePkg/Include/Register/Amd/Ghcb.h
@@ -54,6 +54,7 @@
#define SVM_EXIT_NMI_COMPLETE 0x80000003ULL
#define SVM_EXIT_AP_RESET_HOLD 0x80000004ULL
#define SVM_EXIT_AP_JUMP_TABLE 0x80000005ULL
+#define SVM_EXIT_SNP_PAGE_STATE_CHANGE 0x80000010ULL
#define SVM_EXIT_HYPERVISOR_FEATURES 0x8000FFFDULL
#define SVM_EXIT_UNSUPPORTED 0x8000FFFFULL
Matches "Table 5. List of Supported Non-Automatic Events".

@@ -160,4 +161,32 @@ typedef union {
#define GHCB_HV_FEATURES_SNP_AP_CREATE (GHCB_HV_FEATURES_SNP | BIT1)
#define GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION (GHCB_HV_FEATURES_SNP_AP_CREATE | BIT2)
#define GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION_TIMER (GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION | BIT3)
+
+// SNP Page State Change
(1) Comment style.

+#define SNP_PAGE_STATE_MAX_NPAGES 4095
+#define SNP_PAGE_STATE_MAX_ENTRY 253
+#define SNP_PAGE_STATE_PRIVATE 1
+#define SNP_PAGE_STATE_SHARED 2
+#define SNP_PAGE_STATE_PSMASH 3
+#define SNP_PAGE_STATE_UNSMASH 4
(2) The PSMASH and UNSMASH operations are not documented in the rev 2.00
spec, in the GHCB MSR protocol. That's probably because PSMASH and
UNSMASH can only be defined in terms of 2MB pages, and
GHCB_INFO_SNP_PAGE_STATE_CHANGE_REQUEST is suitable only for individual,
4KB pages. I think it would be useful to point out somehow here that
PSMASH and UNSMASH are restricted to the GHCB shared area protocol
(perhaps extend the leading comment on this block of macros).

(3) I don't understand what "MAX_NPAGES" stands for (4095). The rest of
the series never uses the macro, and I can't associate it with anything
from the spec. If the macro is supposed to relate to the 4KB / 2MB page
smashing / splitting, then its replacement text should be 512. Unless
the macro corresponds to a definition in the spec, I think we should
drop it.

+
+typedef PACKED struct {
+ UINT64 CurrentPage:12;
+ UINT64 GuestFrameNumber:40;
+ UINT64 Op:4;
+ UINT64 PageSize:1;
+ UINT64 Rsvd: 7;
+} SNP_PAGE_STATE_ENTRY;
+
+typedef PACKED struct {
+ UINT16 CurrentEntry;
+ UINT16 EndEntry;
+ UINT32 Rsvd;
+} SNP_PAGE_STATE_HEADER;
(4) We tend to write

#pragma pack (1)
...
#pragma pack ()

rather than PACKED -- but anyway, is packing really necessary? "Natural
alignment" is required in edk2. I'm OK with packing, but I think the
pragma is the preferred form.

(5) Please spell out both "Rsvd" fields above as "Reserved".

(6) Stray space character in "Rsvd: 7".

(7) The field name "Op" is inconsistent with the other field name
"Operation".

(8) I think there is a bug (typo) in the rev 2.00 spec, in 4.1.6 "SNP
Page State Change": it says

... calculated from the supplied guest physical frame number (GFN) for
the requested page size (GPA = GFN << 12).

But, if you can choose 2MB page size in the request, then the (GPA = GFN
<< 12) formula is not g
Sorry, unfinished sentence: I meant that the formula was not generally
correct.
Actually, for any page size, the GPA for any GFN is GFN << 12.

For the SNP Page State Change NAE event, it is up to the hypervisor to
ensure that the GFN/GPA supplied is aligned appropriately for the
requested page size (see 4.1.6 of the GHCB spec where the page operations
are defined). A GFN is naturally 4K aligned, so only a 2MB page size needs
GFN/GPA alignment validation.
Thanks. I guess I was confused by the term "for the requested page size".

For the SNP Page State Change MSR protocol, only a 4K page size is supported.
Yes, that's clear.

Thanks!
Laszlo


Brijesh Singh
 

On 5/4/21 7:33 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%7Cbb2eaad10f574a464cb008d90ef8e5b4%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637557284470753463%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=fpm8afTb4UwW13njwD9WzHWIZZhjUCdmm3Vkt9GJ2SI%3D&amp;reserved=0

The Page State Change NAE exit will be used by the SEV-SNP guest to
request a page state change using the GHCB protocol. See the GHCB
spec section 4.1.6 and 2.3.1 for more detail on the structure
definitions.

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/Register/Amd/Fam17Msr.h | 15 ++++++++++
MdePkg/Include/Register/Amd/Ghcb.h | 29 ++++++++++++++++++++
2 files changed, 44 insertions(+)

diff --git a/MdePkg/Include/Register/Amd/Fam17Msr.h b/MdePkg/Include/Register/Amd/Fam17Msr.h
index e19bd04b6c..432cee2feb 100644
--- a/MdePkg/Include/Register/Amd/Fam17Msr.h
+++ b/MdePkg/Include/Register/Amd/Fam17Msr.h
@@ -58,6 +58,19 @@ typedef union {
UINT64 GuestFrameNumber:52;
} GhcbGpaRegister;

+ struct {
+ UINT64 Function:12;
+ UINT64 GuestFrameNumber:40;
+ UINT64 Operation:4;
+ UINT64 Reserved:8;
+ } SnpPageStateChangeRequest;
+
+ struct {
+ UINT32 Function:12;
+ UINT32 Reserved:20;
+ UINT32 ErrorCode;
+ } SnpPageStateChangeResponse;
+
VOID *Ghcb;
This matches section 2.3.1 in rev 2.00.

UINT64 GhcbPhysicalAddress;
@@ -69,6 +82,8 @@ typedef union {
#define GHCB_INFO_CPUID_RESPONSE 5
#define GHCB_INFO_GHCB_GPA_REGISTER_REQUEST 18
#define GHCB_INFO_GHCB_GPA_REGISTER_RESPONSE 19
+#define GHCB_INFO_SNP_PAGE_STATE_CHANGE_REQUEST 20
+#define GHCB_INFO_SNP_PAGE_STATE_CHANGE_RESPONSE 21
#define GHCB_HYPERVISOR_FEATURES_REQUEST 128
#define GHCB_HYPERVISOR_FEATURES_RESPONSE 129
#define GHCB_INFO_TERMINATE_REQUEST 256
Matches section 2.3.1.

diff --git a/MdePkg/Include/Register/Amd/Ghcb.h b/MdePkg/Include/Register/Amd/Ghcb.h
index 2d64a4c28f..1e7c0daed3 100644
--- a/MdePkg/Include/Register/Amd/Ghcb.h
+++ b/MdePkg/Include/Register/Amd/Ghcb.h
@@ -54,6 +54,7 @@
#define SVM_EXIT_NMI_COMPLETE 0x80000003ULL
#define SVM_EXIT_AP_RESET_HOLD 0x80000004ULL
#define SVM_EXIT_AP_JUMP_TABLE 0x80000005ULL
+#define SVM_EXIT_SNP_PAGE_STATE_CHANGE 0x80000010ULL
#define SVM_EXIT_HYPERVISOR_FEATURES 0x8000FFFDULL
#define SVM_EXIT_UNSUPPORTED 0x8000FFFFULL
Matches "Table 5. List of Supported Non-Automatic Events".

@@ -160,4 +161,32 @@ typedef union {
#define GHCB_HV_FEATURES_SNP_AP_CREATE (GHCB_HV_FEATURES_SNP | BIT1)
#define GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION (GHCB_HV_FEATURES_SNP_AP_CREATE | BIT2)
#define GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION_TIMER (GHCB_HV_FEATURES_SNP_RESTRICTED_INJECTION | BIT3)
+
+// SNP Page State Change
(1) Comment style.
Noted.



+#define SNP_PAGE_STATE_MAX_NPAGES 4095
+#define SNP_PAGE_STATE_MAX_ENTRY 253
+#define SNP_PAGE_STATE_PRIVATE 1
+#define SNP_PAGE_STATE_SHARED 2
+#define SNP_PAGE_STATE_PSMASH 3
+#define SNP_PAGE_STATE_UNSMASH 4
(2) The PSMASH and UNSMASH operations are not documented in the rev 2.00
spec, in the GHCB MSR protocol. That's probably because PSMASH and
UNSMASH can only be defined in terms of 2MB pages, and
GHCB_INFO_SNP_PAGE_STATE_CHANGE_REQUEST is suitable only for individual,
4KB pages. I think it would be useful to point out somehow here that
PSMASH and UNSMASH are restricted to the GHCB shared area protocol
(perhaps extend the leading comment on this block of macros).
I will add something in comment to clarify that UNMASH and PSMASH are
not available in MSR protocol.



(3) I don't understand what "MAX_NPAGES" stands for (4095). The rest of
the series never uses the macro, and I can't associate it with anything
from the spec. If the macro is supposed to relate to the 4KB / 2MB page
smashing / splitting, then its replacement text should be 512. Unless
the macro corresponds to a definition in the spec, I think we should
drop it.
That macro is replaced is no longer used, I ended up creating a more
meaningful macro (SNP_PAGES_STATE_MAX_ENTRY). I will remove it MAX_NPAGES.

+
+typedef PACKED struct {
+ UINT64 CurrentPage:12;
+ UINT64 GuestFrameNumber:40;
+ UINT64 Op:4;
+ UINT64 PageSize:1;
+ UINT64 Rsvd: 7;
+} SNP_PAGE_STATE_ENTRY;
+
+typedef PACKED struct {
+ UINT16 CurrentEntry;
+ UINT16 EndEntry;
+ UINT32 Rsvd;
+} SNP_PAGE_STATE_HEADER;
(4) We tend to write

#pragma pack (1)
...
#pragma pack ()

rather than PACKED -- but anyway, is packing really necessary? "Natural
alignment" is required in edk2. I'm OK with packing, but I think the
pragma is the preferred form.
Noted.


(5) Please spell out both "Rsvd" fields above as "Reserved".
Noted.



(6) Stray space character in "Rsvd: 7".
Noted.



(7) The field name "Op" is inconsistent with the other field name
"Operation".
Noted.



(8) I think there is a bug (typo) in the rev 2.00 spec, in 4.1.6 "SNP
Page State Change": it says

... calculated from the supplied guest physical frame number (GFN) for
the requested page size (GPA = GFN << 12).

But, if you can choose 2MB page size in the request, then the (GPA = GFN
<< 12) formula is not g
I think Tom already clarified it on his latest response.


(9) If my understanding of the spec is correct, "EndEntry" has
*inclusive* meaning. That's unusual. Any particular reason for not
making "EndEntry" exclusive (in the spec)?
Sometimes guest may need to fill only few entries. The "EndEntry" will
give hint to hypervisor that it should stop processing after it reached
to the EndEntry.


+
+typedef struct {
+ SNP_PAGE_STATE_HEADER Header;
+ SNP_PAGE_STATE_ENTRY Entry[SNP_PAGE_STATE_MAX_ENTRY];
+} SNP_PAGE_STATE_CHANGE_INFO;
+
#endif
Yes, this looks OK. Size is 2+2+4+253*8 = 2032 bytes, which matches the
size of GHCB.SharedBuffer.

(10) However, *if* you decide to declare SNP_PAGE_STATE_ENTRY and
SNP_PAGE_STATE_HEADER explicitly as packed, then you should do the same
for SNP_PAGE_STATE_CHANGE_INFO.
Noted.



(11) Like I mentioned earlier, it's probably helpful if you start the
subject line with

MdePkg/Register/Amd: ...

on all of these MdePkg patches. If that becomes too tight, for some of
the MdePkg patches, then I suggest "MdePkg/Amd: ..." (i.e., drop
"Register").
Noted.

Thanks

Brijesh


Laszlo Ersek
 

On 05/04/21 20:53, Brijesh Singh wrote:

On 5/4/21 7:33 AM, Laszlo Ersek wrote:
On 04/30/21 13:51, Brijesh Singh wrote:
+typedef PACKED struct {
+ UINT16 CurrentEntry;
+ UINT16 EndEntry;
+ UINT32 Rsvd;
+} SNP_PAGE_STATE_HEADER;
(9) If my understanding of the spec is correct, "EndEntry" has
*inclusive* meaning. That's unusual. Any particular reason for not
making "EndEntry" exclusive (in the spec)?
Sometimes guest may need to fill only few entries. The "EndEntry" will
give hint to hypervisor that it should stop processing after it reached
to the EndEntry.
I understood the purpose of EndEntry.

My question is why the entry identified by EndEntry *itself* needs to be
processed.

Put differently, I'm asking why your loop controlling expression is

Info->Header.CurrentEntry <= Info->Header.EndEntry [1]

and why not

Info->Header.CurrentEntry < Info->Header.EndEntry [2]

in patch #21 ("OvmfPkg/MemEncryptSevLib: Add support to validate system
RAM").

In case [1], EndEntry is an inclusive limit.

In case [2], EndEntry would be an exclusive limit.

My question is why the spec defines EndEntry with inclusive rather than
exclusive meaning.

Using inclusive low bounds with exclusive high bounds has mental advantages:

https://www.cs.utexas.edu/users/EWD/transcriptions/EWD08xx/EWD831.html

Is there a specific reason for which the spec chose the inclusive high
bound?

Thanks
Laszlo


Brijesh Singh
 

On 5/5/21 1:24 PM, Laszlo Ersek wrote:
On 05/04/21 20:53, Brijesh Singh wrote:
On 5/4/21 7:33 AM, Laszlo Ersek wrote:
On 04/30/21 13:51, Brijesh Singh wrote:
+typedef PACKED struct {
+ UINT16 CurrentEntry;
+ UINT16 EndEntry;
+ UINT32 Rsvd;
+} SNP_PAGE_STATE_HEADER;
(9) If my understanding of the spec is correct, "EndEntry" has
*inclusive* meaning. That's unusual. Any particular reason for not
making "EndEntry" exclusive (in the spec)?
Sometimes guest may need to fill only few entries. The "EndEntry" will
give hint to hypervisor that it should stop processing after it reached
to the EndEntry.
I understood the purpose of EndEntry.

My question is why the entry identified by EndEntry *itself* needs to be
processed.

Put differently, I'm asking why your loop controlling expression is

Info->Header.CurrentEntry <= Info->Header.EndEntry [1]

and why not

Info->Header.CurrentEntry < Info->Header.EndEntry [2]

in patch #21 ("OvmfPkg/MemEncryptSevLib: Add support to validate system
RAM").

In case [1], EndEntry is an inclusive limit.

In case [2], EndEntry would be an exclusive limit.

My question is why the spec defines EndEntry with inclusive rather than
exclusive meaning.

Using inclusive low bounds with exclusive high bounds has mental advantages:

https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.cs.utexas.edu%2Fusers%2FEWD%2Ftranscriptions%2FEWD08xx%2FEWD831.html&;data=04%7C01%7Cbrijesh.singh%40amd.com%7Ce837bb9a71bd47a8337b08d90ff303ae%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637558358685096067%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=uhtfVW6Q%2FIpHwI%2BfNM3DVur92b2KFB5pjaL1dgZtLC8%3D&amp;reserved=0

Is there a specific reason for which the spec chose the inclusive high
bound?
Tom and I talked to see if one of us remembers the reason for choosing
the EndEntry inclusive. Unfortunately, we were not able to recall or
trace from where this request came in, but now it is in the spec, and we
have to live with it :(

-Brijesh