Skip to contentSkip to conversation

x86: svm: low CR3 bits are not MBZ

PATCH Applied Nadav Amit 4 participants 16 messages Jul 13, 2020 – Aug 29, 2020 KVM

lore
Share & tools
lore

Nadav Amit

author
#0 lore raw

PATCH 1/1 x86: svm: low CR3 bits are not MBZ

The low CR3 bits are reserved but not MBZ according to tha APM. The tests should therefore not check that they cause failed VM-entry. Tests on bare-metal show they do not.

Signed-off-by Nadav Amit <[email protected]>
---
 x86/svm.h       |  4 +---
 x86/svm_tests.c | 26 +-------------------------
 2 files changed, 2 insertions(+), 28 deletions(-)
x86/svm.h +1 -3
index f8e7429..15e0f18 100644
@@ -325,9 +325,7 @@ struct __attribute__ ((__packed__)) vmcb {
 #define SVM_CR0_SELECTIVE_MASK (X86_CR0_TS | X86_CR0_MP)
 
 #define	SVM_CR0_RESERVED_MASK			0xffffffff00000000U
-#define	SVM_CR3_LEGACY_RESERVED_MASK		0xfe7U
-#define	SVM_CR3_LEGACY_PAE_RESERVED_MASK	0x7U
-#define	SVM_CR3_LONG_RESERVED_MASK		0xfff0000000000fe7U
+#define	SVM_CR3_LONG_RESERVED_MASK		0xfff0000000000000U
 #define	SVM_CR4_LEGACY_RESERVED_MASK		0xff88f000U
 #define	SVM_CR4_RESERVED_MASK			0xffffffffff88f000U
 #define	SVM_DR6_RESERVED_MASK			0xffffffffffff1ff0U
x86/svm_tests.c +1 -25
index 3b0d019..1908c7c 100644
@@ -2007,38 +2007,14 @@ static void test_cr3(void)
 {
 	/*
 	 * CR3 MBZ bits based on different modes:
-	 *   [2:0]		    - legacy PAE
-	 *   [2:0], [11:5]	    - legacy non-PAE
-	 *   [2:0], [11:5], [63:52] - long mode
+	 *   [63:52] - long mode
 	 */
 	u64 cr3_saved = vmcb->save.cr3;
-	u64 cr4_saved = vmcb->save.cr4;
-	u64 cr4 = cr4_saved;
-	u64 efer_saved = vmcb->save.efer;
-	u64 efer = efer_saved;
 
-	efer &= ~EFER_LME;
-	vmcb->save.efer = efer;
-	cr4 |= X86_CR4_PAE;
-	vmcb->save.cr4 = cr4;
-	SVM_TEST_CR_RESERVED_BITS(0, 2, 1, 3, cr3_saved,
-	    SVM_CR3_LEGACY_PAE_RESERVED_MASK);
-
-	cr4 = cr4_saved & ~X86_CR4_PAE;
-	vmcb->save.cr4 = cr4;
-	SVM_TEST_CR_RESERVED_BITS(0, 11, 1, 3, cr3_saved,
-	    SVM_CR3_LEGACY_RESERVED_MASK);
-
-	cr4 |= X86_CR4_PAE;
-	vmcb->save.cr4 = cr4;
-	efer |= EFER_LME;
-	vmcb->save.efer = efer;
 	SVM_TEST_CR_RESERVED_BITS(0, 63, 1, 3, cr3_saved,
 	    SVM_CR3_LONG_RESERVED_MASK);
 
-	vmcb->save.cr4 = cr4_saved;
 	vmcb->save.cr3 = cr3_saved;
-	vmcb->save.efer = efer_saved;
 }
 
 static void test_cr4(void)
Signature · 1 line
2.25.1
#1 lore raw
Quoted 59 lines · Nadav Amit

The low CR3 bits are reserved but not MBZ according to tha APM. The tests should therefore not check that they cause failed VM-entry. Tests on bare-metal show they do not.

Signed-off-by Nadav Amit <[email protected]>
---
  x86/svm.h       |  4 +---
  x86/svm_tests.c | 26 +-------------------------
  2 files changed, 2 insertions(+), 28 deletions(-)
x86/svm.h +1 -3
index f8e7429..15e0f18 100644
@@ -325,9 +325,7 @@ struct __attribute__ ((__packed__)) vmcb {
  #define SVM_CR0_SELECTIVE_MASK (X86_CR0_TS | X86_CR0_MP)
 
  #define	SVM_CR0_RESERVED_MASK			0xffffffff00000000U
-#define	SVM_CR3_LEGACY_RESERVED_MASK		0xfe7U
-#define	SVM_CR3_LEGACY_PAE_RESERVED_MASK	0x7U
-#define	SVM_CR3_LONG_RESERVED_MASK		0xfff0000000000fe7U
+#define	SVM_CR3_LONG_RESERVED_MASK		0xfff0000000000000U
  #define	SVM_CR4_LEGACY_RESERVED_MASK		0xff88f000U
  #define	SVM_CR4_RESERVED_MASK			0xffffffffff88f000U
  #define	SVM_DR6_RESERVED_MASK			0xffffffffffff1ff0U
x86/svm_tests.c +1 -22
index 3b0d019..1908c7c 100644
@@ -2007,38 +2007,14 @@ static void test_cr3(void)
  {
  	/*
  	 * CR3 MBZ bits based on different modes:
-	 *   [2:0]		    - legacy PAE
-	 *   [2:0], [11:5]	    - legacy non-PAE
-	 *   [2:0], [11:5], [63:52] - long mode
+	 *   [63:52] - long mode
  	 */
  	u64 cr3_saved = vmcb->save.cr3;
-	u64 cr4_saved = vmcb->save.cr4;
-	u64 cr4 = cr4_saved;
-	u64 efer_saved = vmcb->save.efer;
-	u64 efer = efer_saved;
 
-	efer &= ~EFER_LME;
-	vmcb->save.efer = efer;
-	cr4 |= X86_CR4_PAE;
-	vmcb->save.cr4 = cr4;
-	SVM_TEST_CR_RESERVED_BITS(0, 2, 1, 3, cr3_saved,
-	    SVM_CR3_LEGACY_PAE_RESERVED_MASK);
-
-	cr4 = cr4_saved & ~X86_CR4_PAE;
-	vmcb->save.cr4 = cr4;
-	SVM_TEST_CR_RESERVED_BITS(0, 11, 1, 3, cr3_saved,
-	    SVM_CR3_LEGACY_RESERVED_MASK);
-
-	cr4 |= X86_CR4_PAE;
-	vmcb->save.cr4 = cr4;
-	efer |= EFER_LME;

x86/svm_tests.c static void test_cr3(void) · line 2020

Quoted 10 lines
x86/svm_tests.c +0 -3
-	vmcb->save.efer = efer;
  	SVM_TEST_CR_RESERVED_BITS(0, 63, 1, 3, cr3_saved,
  	    SVM_CR3_LONG_RESERVED_MASK);
 
-	vmcb->save.cr4 = cr4_saved;
  	vmcb->save.cr3 = cr3_saved;
-	vmcb->save.efer = efer_saved;
  }
 
  static void test_cr4(void)

APM says,

    "Reserved Bits. Reserved fields should be cleared to 0 by software when writing CR3."

If processor allows these bits to be left non-zero, "should be cleared to 0" means it's not mandatory then. I am wondering what this "should be" actually means :-) !

author
#2 lore raw
Quoted 72 lines · Krish Sadhukhan
Quoted 55 lines · Nadav Amit

The low CR3 bits are reserved but not MBZ according to tha APM. The tests should therefore not check that they cause failed VM-entry. Tests on bare-metal show they do not.

Signed-off-by Nadav Amit <[email protected]>
---
 x86/svm.h       |  4 +---
 x86/svm_tests.c | 26 +-------------------------
 2 files changed, 2 insertions(+), 28 deletions(-)
x86/svm.h +1 -3
index f8e7429..15e0f18 100644
@@ -325,9 +325,7 @@ struct __attribute__ ((__packed__)) vmcb {
 #define SVM_CR0_SELECTIVE_MASK (X86_CR0_TS | X86_CR0_MP)
   #define	SVM_CR0_RESERVED_MASK			0xffffffff00000000U
-#define	SVM_CR3_LEGACY_RESERVED_MASK		0xfe7U
-#define	SVM_CR3_LEGACY_PAE_RESERVED_MASK	0x7U
-#define	SVM_CR3_LONG_RESERVED_MASK		0xfff0000000000fe7U
+#define	SVM_CR3_LONG_RESERVED_MASK		0xfff0000000000000U
 #define	SVM_CR4_LEGACY_RESERVED_MASK		0xff88f000U
 #define	SVM_CR4_RESERVED_MASK			0xffffffffff88f000U
 #define	SVM_DR6_RESERVED_MASK			0xffffffffffff1ff0U
x86/svm_tests.c +1 -19
index 3b0d019..1908c7c 100644
@@ -2007,38 +2007,14 @@ static void test_cr3(void)
 {
 	/*
 	 * CR3 MBZ bits based on different modes:
-	 *   [2:0]		    - legacy PAE
-	 *   [2:0], [11:5]	    - legacy non-PAE
-	 *   [2:0], [11:5], [63:52] - long mode
+	 *   [63:52] - long mode
 	 */
 	u64 cr3_saved = vmcb->save.cr3;
-	u64 cr4_saved = vmcb->save.cr4;
-	u64 cr4 = cr4_saved;
-	u64 efer_saved = vmcb->save.efer;
-	u64 efer = efer_saved;
 -	efer &= ~EFER_LME;
-	vmcb->save.efer = efer;
-	cr4 |= X86_CR4_PAE;
-	vmcb->save.cr4 = cr4;
-	SVM_TEST_CR_RESERVED_BITS(0, 2, 1, 3, cr3_saved,
-	    SVM_CR3_LEGACY_PAE_RESERVED_MASK);
-
-	cr4 = cr4_saved & ~X86_CR4_PAE;
-	vmcb->save.cr4 = cr4;
-	SVM_TEST_CR_RESERVED_BITS(0, 11, 1, 3, cr3_saved,
-	    SVM_CR3_LEGACY_RESERVED_MASK);
-
-	cr4 |= X86_CR4_PAE;

x86/svm_tests.c static void test_cr3(void) · line 2019

Quoted 10 lines
x86/svm_tests.c +0 -4
-	vmcb->save.cr4 = cr4;
-	efer |= EFER_LME;
-	vmcb->save.efer = efer;
 	SVM_TEST_CR_RESERVED_BITS(0, 63, 1, 3, cr3_saved,
 	    SVM_CR3_LONG_RESERVED_MASK);
 -	vmcb->save.cr4 = cr4_saved;
 	vmcb->save.cr3 = cr3_saved;
-	vmcb->save.efer = efer_saved;
 }
   static void test_cr4(void)

APM says,

Quoted 3 lines

"Reserved Bits. Reserved fields should be cleared to 0 by software when writing CR3."

If processor allows these bits to be left non-zero, "should be cleared to 0" means it's not mandatory then. I am wondering what this "should be" actually means :-) !

I really tested it, so I guess we “should” not argue about it. ;-)

Anyhow, according to APM Figure 5-16 (“Control Register 3 (CR3)-Long Mode”), bits 52:63 are “reserved, MBZ” and others are just marked as “Reserved”. So it seems they are not the same.

#3 lore raw
Quoted 71 lines · Nadav Amit
Quoted 71 lines · Krish Sadhukhan
Quoted 55 lines · Nadav Amit

The low CR3 bits are reserved but not MBZ according to tha APM. The tests should therefore not check that they cause failed VM-entry. Tests on bare-metal show they do not.

Signed-off-by Nadav Amit <[email protected]>
---
  x86/svm.h       |  4 +---
  x86/svm_tests.c | 26 +-------------------------
  2 files changed, 2 insertions(+), 28 deletions(-)
x86/svm.h +1 -3
index f8e7429..15e0f18 100644
@@ -325,9 +325,7 @@ struct __attribute__ ((__packed__)) vmcb {
  #define SVM_CR0_SELECTIVE_MASK (X86_CR0_TS | X86_CR0_MP)
    #define	SVM_CR0_RESERVED_MASK			0xffffffff00000000U
-#define	SVM_CR3_LEGACY_RESERVED_MASK		0xfe7U
-#define	SVM_CR3_LEGACY_PAE_RESERVED_MASK	0x7U
-#define	SVM_CR3_LONG_RESERVED_MASK		0xfff0000000000fe7U
+#define	SVM_CR3_LONG_RESERVED_MASK		0xfff0000000000000U
  #define	SVM_CR4_LEGACY_RESERVED_MASK		0xff88f000U
  #define	SVM_CR4_RESERVED_MASK			0xffffffffff88f000U
  #define	SVM_DR6_RESERVED_MASK			0xffffffffffff1ff0U
x86/svm_tests.c +1 -19
index 3b0d019..1908c7c 100644
@@ -2007,38 +2007,14 @@ static void test_cr3(void)
  {
  	/*
  	 * CR3 MBZ bits based on different modes:
-	 *   [2:0]		    - legacy PAE
-	 *   [2:0], [11:5]	    - legacy non-PAE
-	 *   [2:0], [11:5], [63:52] - long mode
+	 *   [63:52] - long mode
  	 */
  	u64 cr3_saved = vmcb->save.cr3;
-	u64 cr4_saved = vmcb->save.cr4;
-	u64 cr4 = cr4_saved;
-	u64 efer_saved = vmcb->save.efer;
-	u64 efer = efer_saved;
  -	efer &= ~EFER_LME;
-	vmcb->save.efer = efer;
-	cr4 |= X86_CR4_PAE;
-	vmcb->save.cr4 = cr4;
-	SVM_TEST_CR_RESERVED_BITS(0, 2, 1, 3, cr3_saved,
-	    SVM_CR3_LEGACY_PAE_RESERVED_MASK);
-
-	cr4 = cr4_saved & ~X86_CR4_PAE;
-	vmcb->save.cr4 = cr4;
-	SVM_TEST_CR_RESERVED_BITS(0, 11, 1, 3, cr3_saved,
-	    SVM_CR3_LEGACY_RESERVED_MASK);
-
-	cr4 |= X86_CR4_PAE;

x86/svm_tests.c static void test_cr3(void) · line 2019

Quoted 10 lines
x86/svm_tests.c +0 -4
-	vmcb->save.cr4 = cr4;
-	efer |= EFER_LME;
-	vmcb->save.efer = efer;
  	SVM_TEST_CR_RESERVED_BITS(0, 63, 1, 3, cr3_saved,
  	    SVM_CR3_LONG_RESERVED_MASK);
  -	vmcb->save.cr4 = cr4_saved;
  	vmcb->save.cr3 = cr3_saved;
-	vmcb->save.efer = efer_saved;
  }
    static void test_cr4(void)

APM says,

Quoted 4 lines
Quoted 3 lines

"Reserved Bits. Reserved fields should be cleared to 0 by software when writing CR3."

If processor allows these bits to be left non-zero, "should be cleared to 0" means it's not mandatory then. I am wondering what this "should be" actually means :-) !

I really tested it, so I guess we “should” not argue about it. ;-)

No, I am not arguing over your test results. :-)

Quoted 5 lines

Anyhow, according to APM Figure 5-16 (“Control Register 3 (CR3)-Long Mode”), bits 52:63 are “reserved, MBZ” and others are just marked as “Reserved”. So it seems they are not the same.

I am just saying that the APM language "should be cleared to 0" is misleading if the processor doesn't enforce it.

author
#4 lore raw
Quoted 77 lines · Krish Sadhukhan
Quoted 73 lines · Nadav Amit
Quoted 73 lines · Krish Sadhukhan
Quoted 55 lines · Nadav Amit

The low CR3 bits are reserved but not MBZ according to tha APM. The tests should therefore not check that they cause failed VM-entry. Tests on bare-metal show they do not.

Signed-off-by Nadav Amit <[email protected]>
---
 x86/svm.h       |  4 +---
 x86/svm_tests.c | 26 +-------------------------
 2 files changed, 2 insertions(+), 28 deletions(-)
x86/svm.h +1 -3
index f8e7429..15e0f18 100644
@@ -325,9 +325,7 @@ struct __attribute__ ((__packed__)) vmcb {
 #define SVM_CR0_SELECTIVE_MASK (X86_CR0_TS | X86_CR0_MP)
   #define	SVM_CR0_RESERVED_MASK			0xffffffff00000000U
-#define	SVM_CR3_LEGACY_RESERVED_MASK		0xfe7U
-#define	SVM_CR3_LEGACY_PAE_RESERVED_MASK	0x7U
-#define	SVM_CR3_LONG_RESERVED_MASK		0xfff0000000000fe7U
+#define	SVM_CR3_LONG_RESERVED_MASK		0xfff0000000000000U
 #define	SVM_CR4_LEGACY_RESERVED_MASK		0xff88f000U
 #define	SVM_CR4_RESERVED_MASK			0xffffffffff88f000U
 #define	SVM_DR6_RESERVED_MASK			0xffffffffffff1ff0U
x86/svm_tests.c +1 -19
index 3b0d019..1908c7c 100644
@@ -2007,38 +2007,14 @@ static void test_cr3(void)
 {
 	/*
 	 * CR3 MBZ bits based on different modes:
-	 *   [2:0]		    - legacy PAE
-	 *   [2:0], [11:5]	    - legacy non-PAE
-	 *   [2:0], [11:5], [63:52] - long mode
+	 *   [63:52] - long mode
 	 */
 	u64 cr3_saved = vmcb->save.cr3;
-	u64 cr4_saved = vmcb->save.cr4;
-	u64 cr4 = cr4_saved;
-	u64 efer_saved = vmcb->save.efer;
-	u64 efer = efer_saved;
 -	efer &= ~EFER_LME;
-	vmcb->save.efer = efer;
-	cr4 |= X86_CR4_PAE;
-	vmcb->save.cr4 = cr4;
-	SVM_TEST_CR_RESERVED_BITS(0, 2, 1, 3, cr3_saved,
-	    SVM_CR3_LEGACY_PAE_RESERVED_MASK);
-
-	cr4 = cr4_saved & ~X86_CR4_PAE;
-	vmcb->save.cr4 = cr4;
-	SVM_TEST_CR_RESERVED_BITS(0, 11, 1, 3, cr3_saved,
-	    SVM_CR3_LEGACY_RESERVED_MASK);
-
-	cr4 |= X86_CR4_PAE;

x86/svm_tests.c static void test_cr3(void) · line 2019

Quoted 10 lines
x86/svm_tests.c +0 -4
-	vmcb->save.cr4 = cr4;
-	efer |= EFER_LME;
-	vmcb->save.efer = efer;
 	SVM_TEST_CR_RESERVED_BITS(0, 63, 1, 3, cr3_saved,
 	    SVM_CR3_LONG_RESERVED_MASK);
 -	vmcb->save.cr4 = cr4_saved;
 	vmcb->save.cr3 = cr3_saved;
-	vmcb->save.efer = efer_saved;
 }
   static void test_cr4(void)

APM says,

"Reserved Bits. Reserved fields should be cleared to 0 by software when writing CR3."

Quoted 7 lines
Quoted 2 lines
Quoted 1 line

If processor allows these bits to be left non-zero, "should be cleared to 0" means it's not mandatory then. I am wondering what this "should be" actually means :-) !

I really tested it, so I guess we “should” not argue about it. ;-)

No, I am not arguing over your test results. :-)

Quoted 3 lines

Anyhow, according to APM Figure 5-16 (“Control Register 3 (CR3)-Long Mode”), bits 52:63 are “reserved, MBZ” and others are just marked as “Reserved”. So it seems they are not the same.

I am just saying that the APM language "should be cleared to 0" is misleading if the processor doesn't enforce it.

Just to ensure I am clear - I am not blaming you in any way. I also found the phrasing confusing.

Having said that, if you (or anyone else) reintroduces “positive” tests, in which the VM CR3 is modified to ensure VM-entry succeeds when the reserved non-MBZ bits are set, please ensure the tests fails gracefully. The non-long-mode CR3 tests crashed since the VM page-tables were incompatible with the paging mode.

In other words, instead of setting a VMMCALL instruction in the VM to trap immediately after entry, consider clearing the present-bits in the high levels of the NPT; or injecting some exception that would trigger exit during vectoring or something like that.

P.S.: If it wasn’t clear, I am not going to fix KVM itself for some obvious reasons.

#5 lore raw
Quoted 93 lines · Nadav Amit
Quoted 77 lines · Krish Sadhukhan
Quoted 73 lines · Nadav Amit
Quoted 73 lines · Krish Sadhukhan
Quoted 55 lines · Nadav Amit

The low CR3 bits are reserved but not MBZ according to tha APM. The tests should therefore not check that they cause failed VM-entry. Tests on bare-metal show they do not.

Signed-off-by Nadav Amit <[email protected]>
---
  x86/svm.h       |  4 +---
  x86/svm_tests.c | 26 +-------------------------
  2 files changed, 2 insertions(+), 28 deletions(-)
x86/svm.h +1 -3
index f8e7429..15e0f18 100644
@@ -325,9 +325,7 @@ struct __attribute__ ((__packed__)) vmcb {
  #define SVM_CR0_SELECTIVE_MASK (X86_CR0_TS | X86_CR0_MP)
    #define	SVM_CR0_RESERVED_MASK			0xffffffff00000000U
-#define	SVM_CR3_LEGACY_RESERVED_MASK		0xfe7U
-#define	SVM_CR3_LEGACY_PAE_RESERVED_MASK	0x7U
-#define	SVM_CR3_LONG_RESERVED_MASK		0xfff0000000000fe7U
+#define	SVM_CR3_LONG_RESERVED_MASK		0xfff0000000000000U
  #define	SVM_CR4_LEGACY_RESERVED_MASK		0xff88f000U
  #define	SVM_CR4_RESERVED_MASK			0xffffffffff88f000U
  #define	SVM_DR6_RESERVED_MASK			0xffffffffffff1ff0U
x86/svm_tests.c +1 -19
index 3b0d019..1908c7c 100644
@@ -2007,38 +2007,14 @@ static void test_cr3(void)
  {
  	/*
  	 * CR3 MBZ bits based on different modes:
-	 *   [2:0]		    - legacy PAE
-	 *   [2:0], [11:5]	    - legacy non-PAE
-	 *   [2:0], [11:5], [63:52] - long mode
+	 *   [63:52] - long mode
  	 */
  	u64 cr3_saved = vmcb->save.cr3;
-	u64 cr4_saved = vmcb->save.cr4;
-	u64 cr4 = cr4_saved;
-	u64 efer_saved = vmcb->save.efer;
-	u64 efer = efer_saved;
  -	efer &= ~EFER_LME;
-	vmcb->save.efer = efer;
-	cr4 |= X86_CR4_PAE;
-	vmcb->save.cr4 = cr4;
-	SVM_TEST_CR_RESERVED_BITS(0, 2, 1, 3, cr3_saved,
-	    SVM_CR3_LEGACY_PAE_RESERVED_MASK);
-
-	cr4 = cr4_saved & ~X86_CR4_PAE;
-	vmcb->save.cr4 = cr4;
-	SVM_TEST_CR_RESERVED_BITS(0, 11, 1, 3, cr3_saved,
-	    SVM_CR3_LEGACY_RESERVED_MASK);
-
-	cr4 |= X86_CR4_PAE;

x86/svm_tests.c static void test_cr3(void) · line 2019

Quoted 10 lines
x86/svm_tests.c +0 -4
-	vmcb->save.cr4 = cr4;
-	efer |= EFER_LME;
-	vmcb->save.efer = efer;
  	SVM_TEST_CR_RESERVED_BITS(0, 63, 1, 3, cr3_saved,
  	    SVM_CR3_LONG_RESERVED_MASK);
  -	vmcb->save.cr4 = cr4_saved;
  	vmcb->save.cr3 = cr3_saved;
-	vmcb->save.efer = efer_saved;
  }
    static void test_cr4(void)

APM says,

"Reserved Bits. Reserved fields should be cleared to 0 by software when writing CR3."

Quoted 7 lines
Quoted 2 lines
Quoted 1 line

If processor allows these bits to be left non-zero, "should be cleared to 0" means it's not mandatory then. I am wondering what this "should be" actually means :-) !

I really tested it, so I guess we “should” not argue about it. ;-)

No, I am not arguing over your test results. :-)

Quoted 3 lines

Anyhow, according to APM Figure 5-16 (“Control Register 3 (CR3)-Long Mode”), bits 52:63 are “reserved, MBZ” and others are just marked as “Reserved”. So it seems they are not the same.

I am just saying that the APM language "should be cleared to 0" is misleading if the processor doesn't enforce it.

Just to ensure I am clear - I am not blaming you in any way. I also found the phrasing confusing.

Having said that, if you (or anyone else) reintroduces “positive” tests, in which the VM CR3 is modified to ensure VM-entry succeeds when the reserved non-MBZ bits are set, please ensure the tests fails gracefully. The non-long-mode CR3 tests crashed since the VM page-tables were incompatible with the paging mode.

Quoted 8 lines

In other words, instead of setting a VMMCALL instruction in the VM to trap immediately after entry, consider clearing the present-bits in the high levels of the NPT; or injecting some exception that would trigger exit during vectoring or something like that.

P.S.: If it wasn’t clear, I am not going to fix KVM itself for some obvious reasons.

I think since the APM is not clear, re-adding any test that tests those bits, is like adding a test with "undefined behavior" to me.

Paolo, Should I send a KVM patch to remove checks for those non-MBZ reserved bits ?

author
#6 lore raw
Quoted 7 lines · Krish Sadhukhan
Quoted 3 lines · Nadav Amit
Quoted 3 lines · Krish Sadhukhan

[snip]

Quoted 15 lines
Quoted 15 lines
Quoted 1 line

I am just saying that the APM language "should be cleared to 0" is misleading if the processor doesn't enforce it.

Just to ensure I am clear - I am not blaming you in any way. I also found the phrasing confusing.

Having said that, if you (or anyone else) reintroduces “positive” tests, in which the VM CR3 is modified to ensure VM-entry succeeds when the reserved non-MBZ bits are set, please ensure the tests fails gracefully. The non-long-mode CR3 tests crashed since the VM page-tables were incompatible with the paging mode.

In other words, instead of setting a VMMCALL instruction in the VM to trap immediately after entry, consider clearing the present-bits in the high levels of the NPT; or injecting some exception that would trigger exit during vectoring or something like that.

Quoted 6 lines
Quoted 2 lines

P.S.: If it wasn’t clear, I am not going to fix KVM itself for some obvious reasons.

I think since the APM is not clear, re-adding any test that tests those bits, is like adding a test with "undefined behavior" to me.

Paolo, Should I send a KVM patch to remove checks for those non-MBZ reserved bits ?

Which non-MBZ reserved bits (other than those that I addressed) do you refer to?

#7 lore raw
Quoted 24 lines · Nadav Amit
Quoted 7 lines · Krish Sadhukhan
Quoted 3 lines · Nadav Amit
Quoted 3 lines · Krish Sadhukhan

[snip]

Quoted 15 lines
Quoted 15 lines
Quoted 1 line

I am just saying that the APM language "should be cleared to 0" is misleading if the processor doesn't enforce it.

Just to ensure I am clear - I am not blaming you in any way. I also found the phrasing confusing.

Having said that, if you (or anyone else) reintroduces “positive” tests, in which the VM CR3 is modified to ensure VM-entry succeeds when the reserved non-MBZ bits are set, please ensure the tests fails gracefully. The non-long-mode CR3 tests crashed since the VM page-tables were incompatible with the paging mode.

In other words, instead of setting a VMMCALL instruction in the VM to trap immediately after entry, consider clearing the present-bits in the high levels of the NPT; or injecting some exception that would trigger exit during vectoring or something like that.

Quoted 9 lines
Quoted 6 lines
Quoted 2 lines

P.S.: If it wasn’t clear, I am not going to fix KVM itself for some obvious reasons.

I think since the APM is not clear, re-adding any test that tests those bits, is like adding a test with "undefined behavior" to me.

Paolo, Should I send a KVM patch to remove checks for those non-MBZ reserved bits ?

Which non-MBZ reserved bits (other than those that I addressed) do you refer to?

I am referring to,

    "[PATCH 2/3 v4] KVM: nSVM: Check that MBZ bits in CR3 and CR4 are not set on vmrun of nested guests"

in which I added the following:

+#define MSR_CR3_LEGACY_RESERVED_MASK        0xfe7U +#define MSR_CR3_LEGACY_PAE_RESERVED_MASK    0x7U +#define MSR_CR3_LONG_RESERVED_MASK        0xfff0000000000fe7U

author
#8 lore raw
Quoted 41 lines · Krish Sadhukhan
Quoted 22 lines · Nadav Amit
Quoted 5 lines · Krish Sadhukhan
Quoted 1 line · Nadav Amit
Quoted 1 line · Krish Sadhukhan

[snip]

Quoted 15 lines
Quoted 15 lines
Quoted 1 line

I am just saying that the APM language "should be cleared to 0" is misleading if the processor doesn't enforce it.

Just to ensure I am clear - I am not blaming you in any way. I also found the phrasing confusing.

Having said that, if you (or anyone else) reintroduces “positive” tests, in which the VM CR3 is modified to ensure VM-entry succeeds when the reserved non-MBZ bits are set, please ensure the tests fails gracefully. The non-long-mode CR3 tests crashed since the VM page-tables were incompatible with the paging mode.

In other words, instead of setting a VMMCALL instruction in the VM to trap immediately after entry, consider clearing the present-bits in the high levels of the NPT; or injecting some exception that would trigger exit during vectoring or something like that.

Quoted 8 lines
Quoted 6 lines
Quoted 2 lines

P.S.: If it wasn’t clear, I am not going to fix KVM itself for some obvious reasons.

I think since the APM is not clear, re-adding any test that tests those bits, is like adding a test with "undefined behavior" to me.

Paolo, Should I send a KVM patch to remove checks for those non-MBZ reserved bits ?

Which non-MBZ reserved bits (other than those that I addressed) do you refer to?

I am referring to,

"[PATCH 2/3 v4] KVM: nSVM: Check that MBZ bits in CR3 and CR4 are not set on vmrun of nested guests"

in which I added the following:

Review

Quoted 3 lines
(quoted diff) +3 -0
+#define MSR_CR3_LEGACY_RESERVED_MASK        0xfe7U
+#define MSR_CR3_LEGACY_PAE_RESERVED_MASK    0x7U
+#define MSR_CR3_LONG_RESERVED_MASK        0xfff0000000000fe7U

Oh, you refer to KVM, not kvm-unit-tests...

That’s out of my scope ;-)

#9 lore raw
In reply to Krish Sadhukhan
Quoted 44 lines · Krish Sadhukhan
Quoted 24 lines · Nadav Amit
Quoted 7 lines · Krish Sadhukhan
Quoted 3 lines · Nadav Amit
Quoted 3 lines · Krish Sadhukhan

[snip]

Quoted 15 lines
Quoted 15 lines
Quoted 1 line

I am just saying that the APM language "should be cleared to 0" is misleading if the processor doesn't enforce it.

Just to ensure I am clear - I am not blaming you in any way. I also found the phrasing confusing.

Having said that, if you (or anyone else) reintroduces “positive” tests, in which the VM CR3 is modified to ensure VM-entry succeeds when the reserved non-MBZ bits are set, please ensure the tests fails gracefully. The non-long-mode CR3 tests crashed since the VM page-tables were incompatible with the paging mode.

In other words, instead of setting a VMMCALL instruction in the VM to trap immediately after entry, consider clearing the present-bits in the high levels of the NPT; or injecting some exception that would trigger exit during vectoring or something like that.

Quoted 9 lines
Quoted 6 lines
Quoted 2 lines

P.S.: If it wasn’t clear, I am not going to fix KVM itself for some obvious reasons.

I think since the APM is not clear, re-adding any test that tests those bits, is like adding a test with "undefined behavior" to me.

Paolo, Should I send a KVM patch to remove checks for those non-MBZ reserved bits ?

Which non-MBZ reserved bits (other than those that I addressed) do you refer to?

I am referring to,

"[PATCH 2/3 v4] KVM: nSVM: Check that MBZ bits in CR3 and CR4 are
not set on vmrun of nested guests"

in which I added the following:

Review

Quoted 3 lines
(quoted diff) +3 -0
+#define MSR_CR3_LEGACY_RESERVED_MASK        0xfe7U
+#define MSR_CR3_LEGACY_PAE_RESERVED_MASK    0x7U
+#define MSR_CR3_LONG_RESERVED_MASK        0xfff0000000000fe7U

In my experience, the APM generally distinguishes between "reserved" and "reserved, MBZ." The low bits you have indicated for CR3 are marked only as "reserved" in Figures 3-4, 3-5, and 3-6 of the APM, volume 2. Only bits 63:52 are marked as "reserved, MBZ." (In fact, Figure 3-6 of the May 2020 version of the APM, revision 3.35, also calls out bits 11:0 as the PCID when CR4.PCIDE is set.)

Of course, you could always test the behavior. :-)

#10 lore raw
Quoted 55 lines · Jim Mattson
Quoted 43 lines · Krish Sadhukhan
Quoted 24 lines · Nadav Amit
Quoted 7 lines · Krish Sadhukhan
Quoted 3 lines · Nadav Amit
Quoted 3 lines · Krish Sadhukhan

[snip]

Quoted 15 lines
Quoted 15 lines
Quoted 1 line

I am just saying that the APM language "should be cleared to 0" is misleading if the processor doesn't enforce it.

Just to ensure I am clear - I am not blaming you in any way. I also found the phrasing confusing.

Having said that, if you (or anyone else) reintroduces “positive” tests, in which the VM CR3 is modified to ensure VM-entry succeeds when the reserved non-MBZ bits are set, please ensure the tests fails gracefully. The non-long-mode CR3 tests crashed since the VM page-tables were incompatible with the paging mode.

In other words, instead of setting a VMMCALL instruction in the VM to trap immediately after entry, consider clearing the present-bits in the high levels of the NPT; or injecting some exception that would trigger exit during vectoring or something like that.

Quoted 9 lines
Quoted 6 lines
Quoted 2 lines

P.S.: If it wasn’t clear, I am not going to fix KVM itself for some obvious reasons.

I think since the APM is not clear, re-adding any test that tests those bits, is like adding a test with "undefined behavior" to me.

Paolo, Should I send a KVM patch to remove checks for those non-MBZ reserved bits ?

Which non-MBZ reserved bits (other than those that I addressed) do you refer to?

I am referring to,

"[PATCH 2/3 v4] KVM: nSVM: Check that MBZ bits in CR3 and CR4 are
not set on vmrun of nested guests"

in which I added the following:

Review

Quoted 3 lines
(quoted diff) +3 -0
+#define MSR_CR3_LEGACY_RESERVED_MASK        0xfe7U
+#define MSR_CR3_LEGACY_PAE_RESERVED_MASK    0x7U
+#define MSR_CR3_LONG_RESERVED_MASK        0xfff0000000000fe7U

In my experience, the APM generally distinguishes between "reserved" and "reserved, MBZ." The low bits you have indicated for CR3 are marked only as "reserved" in Figures 3-4, 3-5, and 3-6 of the APM, volume 2. Only bits 63:52 are marked as "reserved, MBZ." (In fact, Figure 3-6 of the May 2020 version of the APM, revision 3.35, also calls out bits 11:0 as the PCID when CR4.PCIDE is set.)

Quoted 1 line

Of course, you could always test the behavior. :-)

I did some experiments on the processor behavior on an Epyc 2 system via KVM:

   1. MBZ bits:  VMRUN passes even if these bits are set to 1 and guest is exiting with exit code of            SVM_EXIT_VMMCALL. According to the APM, this settting should constitute an invalid guest state and hence I should get and exit code of SVM_EXIT_ERR. There's no KVM check in place for these CR3 bits, so the check is all done in hardware.

   2. non-MBZ reserved bits:  Based on Nadav Amit's suggestion, I set the 'not present' bit in an upper level NPT in order to trigger an NPF and I did get an exit code of SVM_EXIT_NPF when I set any of these bits. I am hoping that the processor has done the consistency check before it tripped on NPF and not the other way around, so that our test is useful :

    In PAE-legacy and non-PAE-legacy modes, the guest doesn't exit with SVM_EXIT_VMMCALL when these bits are set to 0. I am not sure if I am missing any special setting for the PAE-legacy and non-PAE-legacy modes. In long-mode, however, the processor seems to behave as per APM, i.e., guest exits with SVM_EXIT_VMMCALL when these bits are set to 0.

maintainer
#11 lore raw
Quoted 18 lines · Krish Sadhukhan
Quoted 1 line

I did some experiments on the processor behavior on an Epyc 2 system via KVM:

   1. MBZ bits:  VMRUN passes even if these bits are set to 1 and guest is exiting with exit code of            SVM_EXIT_VMMCALL. According to the APM, this settting should constitute an invalid guest state and hence I should get and exit code of SVM_EXIT_ERR. There's no KVM check in place for these CR3 bits, so the check is all done in hardware.

   2. non-MBZ reserved bits:  Based on Nadav Amit's suggestion, I set the 'not present' bit in an upper level NPT in order to trigger an NPF and I did get an exit code of SVM_EXIT_NPF when I set any of these bits. I am hoping that the processor has done the consistency check before it tripped on NPF and not the other way around, so that our test is useful :

Quoted 5 lines

    In PAE-legacy and non-PAE-legacy modes, the guest doesn't exit with SVM_EXIT_VMMCALL when these bits are set to 0. I am not sure if I am missing any special setting for the PAE-legacy and non-PAE-legacy modes. In long-mode, however, the processor seems to behave as per APM, i.e., guest exits with SVM_EXIT_VMMCALL when these bits are set to 0.

Are you going to send patches for this?

Thanks,

Paolo

#12 lore raw
Quoted 17 lines · Paolo Bonzini
Quoted 16 lines · Krish Sadhukhan

I did some experiments on the processor behavior on an Epyc 2 system via KVM:

   1. MBZ bits:  VMRUN passes even if these bits are set to 1 and guest is exiting with exit code of            SVM_EXIT_VMMCALL. According to the APM, this settting should constitute an invalid guest state and hence I should get and exit code of SVM_EXIT_ERR. There's no KVM check in place for these CR3 bits, so the check is all done in hardware.

   2. non-MBZ reserved bits:  Based on Nadav Amit's suggestion, I set the 'not present' bit in an upper level NPT in order to trigger an NPF and I did get an exit code of SVM_EXIT_NPF when I set any of these bits. I am hoping that the processor has done the consistency check before it tripped on NPF and not the other way around, so that our test is useful :

Quoted 6 lines
Quoted 5 lines

    In PAE-legacy and non-PAE-legacy modes, the guest doesn't exit with SVM_EXIT_VMMCALL when these bits are set to 0. I am not sure if I am missing any special setting for the PAE-legacy and non-PAE-legacy modes. In long-mode, however, the processor seems to behave as per APM, i.e., guest exits with SVM_EXIT_VMMCALL when these bits are set to 0.

Are you going to send patches for this?

Yes, I am working on it. I need to complete some more investigation.

Quoted 5 lines

Thanks,

Paolo

#13 lore raw
Quoted 24 lines · Krish Sadhukhan
Quoted 22 lines · Paolo Bonzini
Quoted 21 lines · Krish Sadhukhan

I did some experiments on the processor behavior on an Epyc 2 system via KVM:

   1. MBZ bits:  VMRUN passes even if these bits are set to 1 and guest is exiting with exit code of           SVM_EXIT_VMMCALL. According to the APM, this settting should constitute an invalid guest state and hence I should get and exit code of SVM_EXIT_ERR. There's no KVM check in place for these CR3 bits, so the check is all done in hardware.

   2. non-MBZ reserved bits:  Based on Nadav Amit's suggestion, I set the 'not present' bit in an upper level NPT in order to trigger an NPF and I did get an exit code of SVM_EXIT_NPF when I set any of these bits. I am hoping that the processor has done the consistency check before it tripped on NPF and not the other way around, so that our test is useful :

    In PAE-legacy and non-PAE-legacy modes, the guest doesn't exit with SVM_EXIT_VMMCALL when these bits are set to 0. I am not sure if I am missing any special setting for the PAE-legacy and non-PAE-legacy modes. In long-mode, however, the processor seems to behave as per APM, i.e., guest exits with SVM_EXIT_VMMCALL when these bits are set to 0.

Quoted 4 lines
Quoted 1 line

Are you going to send patches for this?

Yes, I am working on it. I need to complete some more investigation.

I have sent out a patch for testing the non-MBZ reserved bits in long mode.

I haven't been able to find a reliable way to test the non-MBZ reserved bits in legacy (PAE and non-PAE) modes.  In long mode if I set any MBZ bit and an in valid NPT entry, I get VMEXIT_INVALID before VMEXIT_NPF.  But I am not sure if this same method of testing is working when a non-MBZ reserved bit is set. It seems that consistency checking is not enforced on these low-order reserved bits. My goal is to get past the consistency checking phase and then trigger a VMEXIT_NPF during translation of guest pages in NPT.  I created a 3-level page table for legacy PAE mode (as per APM) and tried VMRUN with a  non-MBZ reserved bit set, I am getting VMEXIT_NPF but the EXITINFO1 field contains the nested guest's CR3. So I am not entirely sure if I have gotten past the consistency checking phase.

If there's a better way to test these bits, please let me know.

Quoted 5 lines
Quoted 5 lines

Thanks,

Paolo

maintainer Applied
#15 lore raw
In reply to Nadav Amit
Quoted 60 lines · Nadav Amit

The low CR3 bits are reserved but not MBZ according to tha APM. The tests should therefore not check that they cause failed VM-entry. Tests on bare-metal show they do not.

Signed-off-by Nadav Amit <[email protected]>
---
 x86/svm.h       |  4 +---
 x86/svm_tests.c | 26 +-------------------------
 2 files changed, 2 insertions(+), 28 deletions(-)
x86/svm.h +1 -3
index f8e7429..15e0f18 100644
@@ -325,9 +325,7 @@ struct __attribute__ ((__packed__)) vmcb {
 #define SVM_CR0_SELECTIVE_MASK (X86_CR0_TS | X86_CR0_MP)
 
 #define	SVM_CR0_RESERVED_MASK			0xffffffff00000000U
-#define	SVM_CR3_LEGACY_RESERVED_MASK		0xfe7U
-#define	SVM_CR3_LEGACY_PAE_RESERVED_MASK	0x7U
-#define	SVM_CR3_LONG_RESERVED_MASK		0xfff0000000000fe7U
+#define	SVM_CR3_LONG_RESERVED_MASK		0xfff0000000000000U
 #define	SVM_CR4_LEGACY_RESERVED_MASK		0xff88f000U
 #define	SVM_CR4_RESERVED_MASK			0xffffffffff88f000U
 #define	SVM_DR6_RESERVED_MASK			0xffffffffffff1ff0U
x86/svm_tests.c +1 -22
index 3b0d019..1908c7c 100644
@@ -2007,38 +2007,14 @@ static void test_cr3(void)
 {
 	/*
 	 * CR3 MBZ bits based on different modes:
-	 *   [2:0]		    - legacy PAE
-	 *   [2:0], [11:5]	    - legacy non-PAE
-	 *   [2:0], [11:5], [63:52] - long mode
+	 *   [63:52] - long mode
 	 */
 	u64 cr3_saved = vmcb->save.cr3;
-	u64 cr4_saved = vmcb->save.cr4;
-	u64 cr4 = cr4_saved;
-	u64 efer_saved = vmcb->save.efer;
-	u64 efer = efer_saved;
 
-	efer &= ~EFER_LME;
-	vmcb->save.efer = efer;
-	cr4 |= X86_CR4_PAE;
-	vmcb->save.cr4 = cr4;
-	SVM_TEST_CR_RESERVED_BITS(0, 2, 1, 3, cr3_saved,
-	    SVM_CR3_LEGACY_PAE_RESERVED_MASK);
-
-	cr4 = cr4_saved & ~X86_CR4_PAE;
-	vmcb->save.cr4 = cr4;
-	SVM_TEST_CR_RESERVED_BITS(0, 11, 1, 3, cr3_saved,
-	    SVM_CR3_LEGACY_RESERVED_MASK);
-
-	cr4 |= X86_CR4_PAE;
-	vmcb->save.cr4 = cr4;
-	efer |= EFER_LME;

x86/svm_tests.c static void test_cr3(void) · line 2020

Quoted 10 lines
x86/svm_tests.c +0 -3
-	vmcb->save.efer = efer;
 	SVM_TEST_CR_RESERVED_BITS(0, 63, 1, 3, cr3_saved,
 	    SVM_CR3_LONG_RESERVED_MASK);
 
-	vmcb->save.cr4 = cr4_saved;
 	vmcb->save.cr3 = cr3_saved;
-	vmcb->save.efer = efer_saved;
 }
 
 static void test_cr4(void)

Queued, thanks.

Paolo