[kvm-unit-tests PATCH 0/3] x86/vmx: fix vmx_EPT_AD_* tests

[kvm-unit-tests PATCH 0/3] x86/vmx: fix vmx_EPT_AD_* tests

[Radim Krčmář]

This series turns

  FAIL vmx_EPT_AD_enabled (12 tests, 2 unexpected failures)
  FAIL vmx_EPT_AD_disabled (14 tests, 4 unexpected failures)

into

  PASS vmx_EPT_AD_enabled (13 tests)
  PASS vmx_EPT_AD_disabled (15 tests)

as I think there are no KVM bugs involved.

Other ept tests are still failing and I didn't see a mistake that would
cause

  FAIL vmx_ept_access_test_paddr_read_only_ad_disabled (timeout; duration=90s)
  FAIL vmx_ept_access_test_paddr_read_execute_ad_disabled (timeout; duration=90s)

so a KVM bug there seems likely.


Radim Krčmář (3):
  x86/vmx: fix EPT - MMIO access
  x86/vmx: fix detection of unmapped PTE
  x86/vmx: get EPT at the last level

 x86/vmx.c       | 33 +++++++++++++++++++--------------
 x86/vmx.h       |  4 ++--
 x86/vmx_tests.c | 23 ++++++++++++-----------
 3 files changed, 33 insertions(+), 27 deletions(-)

-- 
2.13.2

[kvm-unit-tests PATCH 1/3] x86/vmx: fix EPT - MMIO access

[Radim Krčmář]

Reading the memory mapped page with x2apic is a bug.  Use the generic reader
instead.  An alternative would be to disable x2apic.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 x86/vmx_tests.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index 1f439522cad8..7be016ce4fbc 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -1031,7 +1031,7 @@ static int ept_init_common(bool have_ad)
 	install_ept(pml4, (unsigned long)data_page1, (unsigned long)data_page2,
 			EPT_RA | EPT_WA | EPT_EA);
 
-	apic_version = *((u32 *)0xfee00030UL);
+	apic_version = apic_read(APIC_LVR);
 	return VMX_TEST_START;
 }
 
-- 
2.13.2

[kvm-unit-tests PATCH 2/3] x86/vmx: fix detection of unmapped PTE

[Radim Krčmář]

check_ept_ad() was testing get_ept_pte() for 0, which meant that the PTE
lookup failed.  dff740c00197 ("x86: ept assertions") changed the return
value in that case to -1, which broke the test.

Returning 0 and -1 is ambiguous, so let's return false instead and get
the PTE through a pointer argument.  This skips a test that was failing
before, because it was looking at invalid type (the meta -1) instead of
the pte.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 I'm not sure if the case was supposed to be tested more, rather than
 called as "ok".
---
 x86/vmx.c       | 33 +++++++++++++++++++--------------
 x86/vmx.h       |  4 ++--
 x86/vmx_tests.c | 20 ++++++++++----------
 3 files changed, 31 insertions(+), 26 deletions(-)

diff --git a/x86/vmx.c b/x86/vmx.c
index 9189a66759ec..5e3832727f05 100644
--- a/x86/vmx.c
+++ b/x86/vmx.c
@@ -809,29 +809,30 @@ void setup_ept_range(unsigned long *pml4, unsigned long start,
 
 /* get_ept_pte : Get the PTE of a given level in EPT,
     @level == 1 means get the latest level*/
-unsigned long get_ept_pte(unsigned long *pml4,
-		unsigned long guest_addr, int level)
+bool get_ept_pte(unsigned long *pml4, unsigned long guest_addr, int level,
+		unsigned long *pte)
 {
 	int l;
-	unsigned long *pt = pml4, pte;
+	unsigned long *pt = pml4, iter_pte;
 	unsigned offset;
 
 	assert(level >= 1 && level <= 4);
 
 	for (l = EPT_PAGE_LEVEL; ; --l) {
 		offset = (guest_addr >> EPT_LEVEL_SHIFT(l)) & EPT_PGDIR_MASK;
-		pte = pt[offset];
-		if (!(pte & (EPT_PRESENT)))
-			return -1;
+		iter_pte = pt[offset];
+		if (!(iter_pte & (EPT_PRESENT)))
+			return false;
 		if (l == level)
 			break;
-		if (l < 4 && (pte & EPT_LARGE_PAGE))
-			return -1;
-		pt = (unsigned long *)(pte & EPT_ADDR_MASK);
+		if (l < 4 && (iter_pte & EPT_LARGE_PAGE))
+			return false;
+		pt = (unsigned long *)(iter_pte & EPT_ADDR_MASK);
 	}
 	offset = (guest_addr >> EPT_LEVEL_SHIFT(l)) & EPT_PGDIR_MASK;
-	pte = pt[offset];
-	return pte;
+	if (pte)
+		*pte = pt[offset];
+	return true;
 }
 
 static void clear_ept_ad_pte(unsigned long *pml4, unsigned long guest_addr)
@@ -894,9 +895,10 @@ void check_ept_ad(unsigned long *pml4, u64 guest_cr3,
 	for (l = EPT_PAGE_LEVEL; ; --l) {
 		offset = (guest_addr >> EPT_LEVEL_SHIFT(l)) & EPT_PGDIR_MASK;
 
-		ept_pte = get_ept_pte(pml4, (u64) &pt[offset], 1);
-		if (ept_pte == 0)
+		if (!get_ept_pte(pml4, (u64) &pt[offset], 1, &ept_pte)) {
+			printf("EPT - guest level %d page table is not mapped.\n", l);
 			return;
+		}
 
 		if (!bad_pt_ad) {
 			bad_pt_ad |= (ept_pte & (EPT_ACCESS_FLAG|EPT_DIRTY_FLAG)) != expected_pt_ad;
@@ -925,7 +927,10 @@ void check_ept_ad(unsigned long *pml4, u64 guest_cr3,
 	offset_in_page = guest_addr & ((1 << EPT_LEVEL_SHIFT(l)) - 1);
 	gpa = (pt[offset] & PT_ADDR_MASK) | (guest_addr & offset_in_page);
 
-	ept_pte = get_ept_pte(pml4, gpa, 1);
+	if (!get_ept_pte(pml4, gpa, 1, &ept_pte)) {
+		report("EPT - guest physical address is not mapped", false);
+		return;
+	}
 	report("EPT - guest physical address A=%d/D=%d",
 	       (ept_pte & (EPT_ACCESS_FLAG|EPT_DIRTY_FLAG)) == expected_gpa_ad,
 	       !!(expected_gpa_ad & EPT_ACCESS_FLAG),
diff --git a/x86/vmx.h b/x86/vmx.h
index 787466653f8b..efbb320f44c7 100644
--- a/x86/vmx.h
+++ b/x86/vmx.h
@@ -697,8 +697,8 @@ void install_ept(unsigned long *pml4, unsigned long phys,
 		unsigned long guest_addr, u64 perm);
 void setup_ept_range(unsigned long *pml4, unsigned long start,
 		     unsigned long len, int map_1g, int map_2m, u64 perm);
-unsigned long get_ept_pte(unsigned long *pml4,
-		unsigned long guest_addr, int level);
+bool get_ept_pte(unsigned long *pml4, unsigned long guest_addr, int level,
+		unsigned long *pte);
 void set_ept_pte(unsigned long *pml4, unsigned long guest_addr,
 		int level, u64 pte_val);
 void check_ept_ad(unsigned long *pml4, u64 guest_cr3,
diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index 7be016ce4fbc..181c3c73cb60 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -1212,17 +1212,18 @@ static int ept_exit_handler_common(bool have_ad)
 			break;
 		case 3:
 			clear_ept_ad(pml4, guest_cr3, (unsigned long)data_page1);
-			data_page1_pte = get_ept_pte(pml4,
-				(unsigned long)data_page1, 1);
+			TEST_ASSERT(get_ept_pte(pml4, (unsigned long)data_page1,
+						1, &data_page1_pte));
 			set_ept_pte(pml4, (unsigned long)data_page1, 
 				1, data_page1_pte & ~EPT_PRESENT);
 			ept_sync(INVEPT_SINGLE, eptp);
 			break;
 		case 4:
-			data_page1_pte = get_ept_pte(pml4,
-				(unsigned long)data_page1, 2);
+			TEST_ASSERT(get_ept_pte(pml4, (unsigned long)data_page1,
+						2, &data_page1_pte));
 			data_page1_pte &= PAGE_MASK;
-			data_page1_pte_pte = get_ept_pte(pml4, data_page1_pte, 2);
+			TEST_ASSERT(get_ept_pte(pml4, data_page1_pte,
+						2, &data_page1_pte_pte));
 			set_ept_pte(pml4, data_page1_pte, 2,
 				data_page1_pte_pte & ~EPT_PRESENT);
 			ept_sync(INVEPT_SINGLE, eptp);
@@ -1267,7 +1268,7 @@ static int ept_exit_handler_common(bool have_ad)
 			if (exit_qual == (EPT_VLT_WR | EPT_VLT_LADDR_VLD |
 					EPT_VLT_PADDR))
 				vmx_inc_test_stage();
-			set_ept_pte(pml4, (unsigned long)data_page1, 
+			set_ept_pte(pml4, (unsigned long)data_page1,
 				1, data_page1_pte | (EPT_PRESENT));
 			ept_sync(INVEPT_SINGLE, eptp);
 			break;
@@ -2175,8 +2176,7 @@ static unsigned long ept_twiddle(unsigned long gpa, bool mkhuge, int level,
 	unsigned long pte;
 
 	/* Screw with the mapping at the requested level. */
-	orig_pte = get_ept_pte(pml4, gpa, level);
-	TEST_ASSERT(orig_pte != -1);
+	TEST_ASSERT(get_ept_pte(pml4, gpa, level, &orig_pte));
 	pte = orig_pte;
 	if (mkhuge)
 		pte = (orig_pte & ~EPT_ADDR_MASK) | data->hpa | EPT_LARGE_PAGE;
@@ -2603,8 +2603,8 @@ static void ept_access_test_setup(void)
 	 * Make sure nothing's mapped here so the tests that screw with the
 	 * pml4 entry don't inadvertently break something.
 	 */
-	TEST_ASSERT_EQ(get_ept_pte(pml4, data->gpa, 4), -1);
-	TEST_ASSERT_EQ(get_ept_pte(pml4, data->gpa + size - 1, 4), -1);
+	TEST_ASSERT(!get_ept_pte(pml4, data->gpa, 4, NULL));
+	TEST_ASSERT(!get_ept_pte(pml4, data->gpa + size - 1, 4, NULL));
 	install_ept(pml4, data->hpa, data->gpa, EPT_PRESENT);
 
 	data->hva[0] = MAGIC_VAL_1;
-- 
2.13.2

[kvm-unit-tests PATCH 3/3] x86/vmx: get EPT at the last level

[Radim Krčmář]

vmx_EPT_AD_* tests mark the last level as non-present, but that doesn't
mean we cannot look at A/D bits of that last level.
This fixes "EPT - guest physical address is not mapped" in case 3.

Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
---
 x86/vmx.c       | 4 ++--
 x86/vmx_tests.c | 5 +++--
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/x86/vmx.c b/x86/vmx.c
index 5e3832727f05..56c2c079ebc5 100644
--- a/x86/vmx.c
+++ b/x86/vmx.c
@@ -821,12 +821,12 @@ bool get_ept_pte(unsigned long *pml4, unsigned long guest_addr, int level,
 	for (l = EPT_PAGE_LEVEL; ; --l) {
 		offset = (guest_addr >> EPT_LEVEL_SHIFT(l)) & EPT_PGDIR_MASK;
 		iter_pte = pt[offset];
-		if (!(iter_pte & (EPT_PRESENT)))
-			return false;
 		if (l == level)
 			break;
 		if (l < 4 && (iter_pte & EPT_LARGE_PAGE))
 			return false;
+		if (!(iter_pte & (EPT_PRESENT)))
+			return false;
 		pt = (unsigned long *)(iter_pte & EPT_ADDR_MASK);
 	}
 	offset = (guest_addr >> EPT_LEVEL_SHIFT(l)) & EPT_PGDIR_MASK;
diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index 181c3c73cb60..567f7143b427 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -2582,6 +2582,7 @@ static void ept_access_test_setup(void)
 	unsigned long npages = 1ul << PAGE_1G_ORDER;
 	unsigned long size = npages * PAGE_SIZE;
 	unsigned long *page_table = current_page_table();
+	unsigned long pte;
 
 	if (setup_ept(false))
 		test_skip("EPT not supported");
@@ -2603,8 +2604,8 @@ static void ept_access_test_setup(void)
 	 * Make sure nothing's mapped here so the tests that screw with the
 	 * pml4 entry don't inadvertently break something.
 	 */
-	TEST_ASSERT(!get_ept_pte(pml4, data->gpa, 4, NULL));
-	TEST_ASSERT(!get_ept_pte(pml4, data->gpa + size - 1, 4, NULL));
+	TEST_ASSERT(get_ept_pte(pml4, data->gpa, 4, &pte) && pte == 0);
+	TEST_ASSERT(get_ept_pte(pml4, data->gpa + size - 1, 4, &pte) && pte == 0);
 	install_ept(pml4, data->hpa, data->gpa, EPT_PRESENT);
 
 	data->hva[0] = MAGIC_VAL_1;
-- 
2.13.2

Re: [kvm-unit-tests PATCH 1/3] x86/vmx: fix EPT - MMIO access

[Peter Feiner]

On Thu, Jun 29, 2017 at 10:26 AM, Radim Krčmář <rkrcmar@redhat.com> wrote:
>
> Reading the memory mapped page with x2apic is a bug.  Use the generic reader
> instead.  An alternative would be to disable x2apic.
>
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
Reviewed-by: Peter Feiner <pfeiner@google.com>

> ---
>  x86/vmx_tests.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
> index 1f439522cad8..7be016ce4fbc 100644
> --- a/x86/vmx_tests.c
> +++ b/x86/vmx_tests.c
> @@ -1031,7 +1031,7 @@ static int ept_init_common(bool have_ad)
>         install_ept(pml4, (unsigned long)data_page1, (unsigned long)data_page2,
>                         EPT_RA | EPT_WA | EPT_EA);
>
> -       apic_version = *((u32 *)0xfee00030UL);
> +       apic_version = apic_read(APIC_LVR);
>         return VMX_TEST_START;
>  }
>
> --
> 2.13.2
>

Looks good!

Re: [kvm-unit-tests PATCH 2/3] x86/vmx: fix detection of unmapped PTE

[Peter Feiner]

On Thu, Jun 29, 2017 at 10:26 AM, Radim Krčmář <rkrcmar@redhat.com> wrote:
> check_ept_ad() was testing get_ept_pte() for 0, which meant that the PTE
> lookup failed.  dff740c00197 ("x86: ept assertions") changed the return
> value in that case to -1, which broke the test.
>
> Returning 0 and -1 is ambiguous, so let's return false instead and get
> the PTE through a pointer argument.  This skips a test that was failing
> before, because it was looking at invalid type (the meta -1) instead of
> the pte.
>
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
Reviewed-by: Peter Feiner <pfeiner@google.com>

> ---
>  I'm not sure if the case was supposed to be tested more, rather than
>  called as "ok".
> ---
>  x86/vmx.c       | 33 +++++++++++++++++++--------------
>  x86/vmx.h       |  4 ++--
>  x86/vmx_tests.c | 20 ++++++++++----------
>  3 files changed, 31 insertions(+), 26 deletions(-)
>
> diff --git a/x86/vmx.c b/x86/vmx.c
> index 9189a66759ec..5e3832727f05 100644
> --- a/x86/vmx.c
> +++ b/x86/vmx.c
> @@ -809,29 +809,30 @@ void setup_ept_range(unsigned long *pml4, unsigned long start,
>
>  /* get_ept_pte : Get the PTE of a given level in EPT,
>      @level == 1 means get the latest level*/
> -unsigned long get_ept_pte(unsigned long *pml4,
> -               unsigned long guest_addr, int level)
> +bool get_ept_pte(unsigned long *pml4, unsigned long guest_addr, int level,
> +               unsigned long *pte)
>  {
>         int l;
> -       unsigned long *pt = pml4, pte;
> +       unsigned long *pt = pml4, iter_pte;
>         unsigned offset;
>
>         assert(level >= 1 && level <= 4);
>
>         for (l = EPT_PAGE_LEVEL; ; --l) {
>                 offset = (guest_addr >> EPT_LEVEL_SHIFT(l)) & EPT_PGDIR_MASK;
> -               pte = pt[offset];
> -               if (!(pte & (EPT_PRESENT)))
> -                       return -1;
> +               iter_pte = pt[offset];
> +               if (!(iter_pte & (EPT_PRESENT)))
> +                       return false;
>                 if (l == level)
>                         break;
> -               if (l < 4 && (pte & EPT_LARGE_PAGE))
> -                       return -1;
> -               pt = (unsigned long *)(pte & EPT_ADDR_MASK);
> +               if (l < 4 && (iter_pte & EPT_LARGE_PAGE))
> +                       return false;
> +               pt = (unsigned long *)(iter_pte & EPT_ADDR_MASK);
>         }
>         offset = (guest_addr >> EPT_LEVEL_SHIFT(l)) & EPT_PGDIR_MASK;
> -       pte = pt[offset];
> -       return pte;
> +       if (pte)
> +               *pte = pt[offset];
> +       return true;
>  }
>
>  static void clear_ept_ad_pte(unsigned long *pml4, unsigned long guest_addr)
> @@ -894,9 +895,10 @@ void check_ept_ad(unsigned long *pml4, u64 guest_cr3,
>         for (l = EPT_PAGE_LEVEL; ; --l) {
>                 offset = (guest_addr >> EPT_LEVEL_SHIFT(l)) & EPT_PGDIR_MASK;
>
> -               ept_pte = get_ept_pte(pml4, (u64) &pt[offset], 1);
> -               if (ept_pte == 0)
> +               if (!get_ept_pte(pml4, (u64) &pt[offset], 1, &ept_pte)) {
> +                       printf("EPT - guest level %d page table is not mapped.\n", l);
>                         return;
> +               }
>
>                 if (!bad_pt_ad) {
>                         bad_pt_ad |= (ept_pte & (EPT_ACCESS_FLAG|EPT_DIRTY_FLAG)) != expected_pt_ad;
> @@ -925,7 +927,10 @@ void check_ept_ad(unsigned long *pml4, u64 guest_cr3,
>         offset_in_page = guest_addr & ((1 << EPT_LEVEL_SHIFT(l)) - 1);
>         gpa = (pt[offset] & PT_ADDR_MASK) | (guest_addr & offset_in_page);
>
> -       ept_pte = get_ept_pte(pml4, gpa, 1);
> +       if (!get_ept_pte(pml4, gpa, 1, &ept_pte)) {
> +               report("EPT - guest physical address is not mapped", false);
> +               return;
> +       }
>         report("EPT - guest physical address A=%d/D=%d",
>                (ept_pte & (EPT_ACCESS_FLAG|EPT_DIRTY_FLAG)) == expected_gpa_ad,
>                !!(expected_gpa_ad & EPT_ACCESS_FLAG),
> diff --git a/x86/vmx.h b/x86/vmx.h
> index 787466653f8b..efbb320f44c7 100644
> --- a/x86/vmx.h
> +++ b/x86/vmx.h
> @@ -697,8 +697,8 @@ void install_ept(unsigned long *pml4, unsigned long phys,
>                 unsigned long guest_addr, u64 perm);
>  void setup_ept_range(unsigned long *pml4, unsigned long start,
>                      unsigned long len, int map_1g, int map_2m, u64 perm);
> -unsigned long get_ept_pte(unsigned long *pml4,
> -               unsigned long guest_addr, int level);
> +bool get_ept_pte(unsigned long *pml4, unsigned long guest_addr, int level,
> +               unsigned long *pte);
>  void set_ept_pte(unsigned long *pml4, unsigned long guest_addr,
>                 int level, u64 pte_val);
>  void check_ept_ad(unsigned long *pml4, u64 guest_cr3,
> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
> index 7be016ce4fbc..181c3c73cb60 100644
> --- a/x86/vmx_tests.c
> +++ b/x86/vmx_tests.c
> @@ -1212,17 +1212,18 @@ static int ept_exit_handler_common(bool have_ad)
>                         break;
>                 case 3:
>                         clear_ept_ad(pml4, guest_cr3, (unsigned long)data_page1);
> -                       data_page1_pte = get_ept_pte(pml4,
> -                               (unsigned long)data_page1, 1);
> +                       TEST_ASSERT(get_ept_pte(pml4, (unsigned long)data_page1,
> +                                               1, &data_page1_pte));
>                         set_ept_pte(pml4, (unsigned long)data_page1,
>                                 1, data_page1_pte & ~EPT_PRESENT);
>                         ept_sync(INVEPT_SINGLE, eptp);
>                         break;
>                 case 4:
> -                       data_page1_pte = get_ept_pte(pml4,
> -                               (unsigned long)data_page1, 2);
> +                       TEST_ASSERT(get_ept_pte(pml4, (unsigned long)data_page1,
> +                                               2, &data_page1_pte));
>                         data_page1_pte &= PAGE_MASK;
> -                       data_page1_pte_pte = get_ept_pte(pml4, data_page1_pte, 2);
> +                       TEST_ASSERT(get_ept_pte(pml4, data_page1_pte,
> +                                               2, &data_page1_pte_pte));
>                         set_ept_pte(pml4, data_page1_pte, 2,
>                                 data_page1_pte_pte & ~EPT_PRESENT);
>                         ept_sync(INVEPT_SINGLE, eptp);
> @@ -1267,7 +1268,7 @@ static int ept_exit_handler_common(bool have_ad)
>                         if (exit_qual == (EPT_VLT_WR | EPT_VLT_LADDR_VLD |
>                                         EPT_VLT_PADDR))
>                                 vmx_inc_test_stage();
> -                       set_ept_pte(pml4, (unsigned long)data_page1,
> +                       set_ept_pte(pml4, (unsigned long)data_page1,
>                                 1, data_page1_pte | (EPT_PRESENT));
>                         ept_sync(INVEPT_SINGLE, eptp);
>                         break;
> @@ -2175,8 +2176,7 @@ static unsigned long ept_twiddle(unsigned long gpa, bool mkhuge, int level,
>         unsigned long pte;
>
>         /* Screw with the mapping at the requested level. */
> -       orig_pte = get_ept_pte(pml4, gpa, level);
> -       TEST_ASSERT(orig_pte != -1);
> +       TEST_ASSERT(get_ept_pte(pml4, gpa, level, &orig_pte));
>         pte = orig_pte;
>         if (mkhuge)
>                 pte = (orig_pte & ~EPT_ADDR_MASK) | data->hpa | EPT_LARGE_PAGE;
> @@ -2603,8 +2603,8 @@ static void ept_access_test_setup(void)
>          * Make sure nothing's mapped here so the tests that screw with the
>          * pml4 entry don't inadvertently break something.
>          */
> -       TEST_ASSERT_EQ(get_ept_pte(pml4, data->gpa, 4), -1);
> -       TEST_ASSERT_EQ(get_ept_pte(pml4, data->gpa + size - 1, 4), -1);
> +       TEST_ASSERT(!get_ept_pte(pml4, data->gpa, 4, NULL));
> +       TEST_ASSERT(!get_ept_pte(pml4, data->gpa + size - 1, 4, NULL));
>         install_ept(pml4, data->hpa, data->gpa, EPT_PRESENT);
>
>         data->hva[0] = MAGIC_VAL_1;
> --
> 2.13.2
>

Very nice fix and cleanup overall. Thanks!

Re: [kvm-unit-tests PATCH 3/3] x86/vmx: get EPT at the last level

[Peter Feiner]

On Thu, Jun 29, 2017 at 10:26 AM, Radim Krčmář <rkrcmar@redhat.com> wrote:
> vmx_EPT_AD_* tests mark the last level as non-present, but that doesn't
> mean we cannot look at A/D bits of that last level.
> This fixes "EPT - guest physical address is not mapped" in case 3.
>
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  x86/vmx.c       | 4 ++--
>  x86/vmx_tests.c | 5 +++--
>  2 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/x86/vmx.c b/x86/vmx.c
> index 5e3832727f05..56c2c079ebc5 100644
> --- a/x86/vmx.c
> +++ b/x86/vmx.c
> @@ -821,12 +821,12 @@ bool get_ept_pte(unsigned long *pml4, unsigned long guest_addr, int level,
>         for (l = EPT_PAGE_LEVEL; ; --l) {
>                 offset = (guest_addr >> EPT_LEVEL_SHIFT(l)) & EPT_PGDIR_MASK;
>                 iter_pte = pt[offset];
> -               if (!(iter_pte & (EPT_PRESENT)))
> -                       return false;
>                 if (l == level)
>                         break;
>                 if (l < 4 && (iter_pte & EPT_LARGE_PAGE))
>                         return false;
> +               if (!(iter_pte & (EPT_PRESENT)))
> +                       return false;
>                 pt = (unsigned long *)(iter_pte & EPT_ADDR_MASK);
>         }
>         offset = (guest_addr >> EPT_LEVEL_SHIFT(l)) & EPT_PGDIR_MASK;
> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
> index 181c3c73cb60..567f7143b427 100644
> --- a/x86/vmx_tests.c
> +++ b/x86/vmx_tests.c
> @@ -2582,6 +2582,7 @@ static void ept_access_test_setup(void)
>         unsigned long npages = 1ul << PAGE_1G_ORDER;
>         unsigned long size = npages * PAGE_SIZE;
>         unsigned long *page_table = current_page_table();
> +       unsigned long pte;
>
>         if (setup_ept(false))
>                 test_skip("EPT not supported");
> @@ -2603,8 +2604,8 @@ static void ept_access_test_setup(void)
>          * Make sure nothing's mapped here so the tests that screw with the
>          * pml4 entry don't inadvertently break something.
>          */
> -       TEST_ASSERT(!get_ept_pte(pml4, data->gpa, 4, NULL));
> -       TEST_ASSERT(!get_ept_pte(pml4, data->gpa + size - 1, 4, NULL));
> +       TEST_ASSERT(get_ept_pte(pml4, data->gpa, 4, &pte) && pte == 0);
> +       TEST_ASSERT(get_ept_pte(pml4, data->gpa + size - 1, 4, &pte) && pte == 0);

This isn't right. The PML4 for 1 TiB shouldn't be present ("Make sure
nothing's mapped
here so the tests that screw with the pml4 entry don't inadvertently
break something."),
so the walk definitely shouldn't get to the leaf entry.  I'd actually expect
get_ept_pte(pml4, data->gpa, 2, &pte) to return false.

>         install_ept(pml4, data->hpa, data->gpa, EPT_PRESENT);
>
>         data->hva[0] = MAGIC_VAL_1;
> --
> 2.13.2
>

Re: [kvm-unit-tests PATCH 3/3] x86/vmx: get EPT at the last level

[Radim Krčmář]

2017-06-29 10:51-0700, Peter Feiner:
> On Thu, Jun 29, 2017 at 10:26 AM, Radim Krčmář <rkrcmar@redhat.com> wrote:
> > vmx_EPT_AD_* tests mark the last level as non-present, but that doesn't
> > mean we cannot look at A/D bits of that last level.
> > This fixes "EPT - guest physical address is not mapped" in case 3.
> >
> > Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> > ---
> > diff --git a/x86/vmx.c b/x86/vmx.c
> > @@ -821,12 +821,12 @@ bool get_ept_pte(unsigned long *pml4, unsigned long guest_addr, int level,
> >         for (l = EPT_PAGE_LEVEL; ; --l) {
> >                 offset = (guest_addr >> EPT_LEVEL_SHIFT(l)) & EPT_PGDIR_MASK;
> >                 iter_pte = pt[offset];
> > -               if (!(iter_pte & (EPT_PRESENT)))
> > -                       return false;
> >                 if (l == level)
> >                         break;
> >                 if (l < 4 && (iter_pte & EPT_LARGE_PAGE))
> >                         return false;
> > +               if (!(iter_pte & (EPT_PRESENT)))
> > +                       return false;
> >                 pt = (unsigned long *)(iter_pte & EPT_ADDR_MASK);
> >         }
> >         offset = (guest_addr >> EPT_LEVEL_SHIFT(l)) & EPT_PGDIR_MASK;
> > diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
> > @@ -2603,8 +2604,8 @@ static void ept_access_test_setup(void)
> >          * Make sure nothing's mapped here so the tests that screw with the
> >          * pml4 entry don't inadvertently break something.
> >          */
> > -       TEST_ASSERT(!get_ept_pte(pml4, data->gpa, 4, NULL));
> > -       TEST_ASSERT(!get_ept_pte(pml4, data->gpa + size - 1, 4, NULL));
> > +       TEST_ASSERT(get_ept_pte(pml4, data->gpa, 4, &pte) && pte == 0);
> > +       TEST_ASSERT(get_ept_pte(pml4, data->gpa + size - 1, 4, &pte) && pte == 0);
> 
> This isn't right. The PML4 for 1 TiB shouldn't be present ("Make sure
> nothing's mapped
> here so the tests that screw with the pml4 entry don't inadvertently
> break something."),
> so the walk definitely shouldn't get to the leaf entry.  I'd actually expect
> get_ept_pte(pml4, data->gpa, 2, &pte) to return false.

The assert asks for 'level 4', which is the topmost level at the moment.

"get_ept_pte(pml4, data->gpa, 2, &pte)" would return false here, even 3,
as level 4 is not present, but 4 returns true and gives pte of that
level, because the pte is actually accessible without any walk ...

The patch adds a check for 'pte == 0' afterwards to ensure that nothing
is actually mapped there (0 implies unset EPT_PRESENT).

Any idea how to improve it?

Thanks.

Re: [kvm-unit-tests PATCH 3/3] x86/vmx: get EPT at the last level

[Peter Feiner]

On Thu, Jun 29, 2017 at 11:08 AM, Radim Krčmář <rkrcmar@redhat.com> wrote:
> 2017-06-29 10:51-0700, Peter Feiner:
>> On Thu, Jun 29, 2017 at 10:26 AM, Radim Krčmář <rkrcmar@redhat.com> wrote:
>> > vmx_EPT_AD_* tests mark the last level as non-present, but that doesn't
>> > mean we cannot look at A/D bits of that last level.
>> > This fixes "EPT - guest physical address is not mapped" in case 3.
>> >
>> > Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
>> > ---
>> > diff --git a/x86/vmx.c b/x86/vmx.c
>> > @@ -821,12 +821,12 @@ bool get_ept_pte(unsigned long *pml4, unsigned long guest_addr, int level,
>> >         for (l = EPT_PAGE_LEVEL; ; --l) {
>> >                 offset = (guest_addr >> EPT_LEVEL_SHIFT(l)) & EPT_PGDIR_MASK;
>> >                 iter_pte = pt[offset];
>> > -               if (!(iter_pte & (EPT_PRESENT)))
>> > -                       return false;
>> >                 if (l == level)
>> >                         break;
>> >                 if (l < 4 && (iter_pte & EPT_LARGE_PAGE))
>> >                         return false;
>> > +               if (!(iter_pte & (EPT_PRESENT)))
>> > +                       return false;
>> >                 pt = (unsigned long *)(iter_pte & EPT_ADDR_MASK);
>> >         }
>> >         offset = (guest_addr >> EPT_LEVEL_SHIFT(l)) & EPT_PGDIR_MASK;
>> > diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
>> > @@ -2603,8 +2604,8 @@ static void ept_access_test_setup(void)
>> >          * Make sure nothing's mapped here so the tests that screw with the
>> >          * pml4 entry don't inadvertently break something.
>> >          */
>> > -       TEST_ASSERT(!get_ept_pte(pml4, data->gpa, 4, NULL));
>> > -       TEST_ASSERT(!get_ept_pte(pml4, data->gpa + size - 1, 4, NULL));
>> > +       TEST_ASSERT(get_ept_pte(pml4, data->gpa, 4, &pte) && pte == 0);
>> > +       TEST_ASSERT(get_ept_pte(pml4, data->gpa + size - 1, 4, &pte) && pte == 0);
>>
>> This isn't right. The PML4 for 1 TiB shouldn't be present ("Make sure
>> nothing's mapped
>> here so the tests that screw with the pml4 entry don't inadvertently
>> break something."),
>> so the walk definitely shouldn't get to the leaf entry.  I'd actually expect
>> get_ept_pte(pml4, data->gpa, 2, &pte) to return false.
>
> The assert asks for 'level 4', which is the topmost level at the moment.
>
> "get_ept_pte(pml4, data->gpa, 2, &pte)" would return false here, even 3,
> as level 4 is not present, but 4 returns true and gives pte of that
> level, because the pte is actually accessible without any walk ...
>
> The patch adds a check for 'pte == 0' afterwards to ensure that nothing
> is actually mapped there (0 implies unset EPT_PRESENT).
>
> Any idea how to improve it?
>
> Thanks.

Sorry, I'm an idiot. I was thinking 4 levels deep, which is pretty
dumb because we use pml4 to indicate the 4th from the bottom :-)

Patch looks good!

Re: [kvm-unit-tests PATCH 1/3] x86/vmx: fix EPT - MMIO access

[Paolo Bonzini]

On 29/06/2017 19:34, Peter Feiner wrote:
> Reading the memory mapped page with x2apic is a bug.  Use the generic reader
> instead.  An alternative would be to disable x2apic.

Disabling x2apic would test what the test is supposed to test. :)

Paolo

Re: [kvm-unit-tests PATCH 2/3] x86/vmx: fix detection of unmapped PTE

[Paolo Bonzini]

On 29/06/2017 19:26, Radim Krčmář wrote:
> check_ept_ad() was testing get_ept_pte() for 0, which meant that the PTE
> lookup failed.  dff740c00197 ("x86: ept assertions") changed the return
> value in that case to -1, which broke the test.
> 
> Returning 0 and -1 is ambiguous, so let's return false instead and get
> the PTE through a pointer argument.  This skips a test that was failing
> before, because it was looking at invalid type (the meta -1) instead of
> the pte.
> 
> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com>
> ---
>  I'm not sure if the case was supposed to be tested more, rather than
>  called as "ok".
> ---
>  x86/vmx.c       | 33 +++++++++++++++++++--------------
>  x86/vmx.h       |  4 ++--
>  x86/vmx_tests.c | 20 ++++++++++----------
>  3 files changed, 31 insertions(+), 26 deletions(-)

This breaks the vmx PML test:

Test suite: PML
PASS: PML - Dirty GPA Logging
ERROR - unexpected stage, 2.
VMEXIT info:
	vmexit reason = 18
	exit qualification = 0
	Bit 31 of reason = 0
	guest_rip = 0x4049b3
	RAX=0x149a000    RBX=0x3f8    RCX=0x184    RDX=0x3fd
	RSP=0x46efcf    RBP=0x46efdf    RSI=0x41a8a3    RDI=0x41a8a3
	R8 =0xa    R9 =0x3f8    R10=0    R11=0
	R12=0    R13=0    R14=0    R15=0


(should have been "PASS: PML Full Event").  I didn't investigate why.

Paolo

> diff --git a/x86/vmx.c b/x86/vmx.c
> index 9189a66759ec..5e3832727f05 100644
> --- a/x86/vmx.c
> +++ b/x86/vmx.c
> @@ -809,29 +809,30 @@ void setup_ept_range(unsigned long *pml4, unsigned long start,
>  
>  /* get_ept_pte : Get the PTE of a given level in EPT,
>      @level == 1 means get the latest level*/
> -unsigned long get_ept_pte(unsigned long *pml4,
> -		unsigned long guest_addr, int level)
> +bool get_ept_pte(unsigned long *pml4, unsigned long guest_addr, int level,
> +		unsigned long *pte)
>  {
>  	int l;
> -	unsigned long *pt = pml4, pte;
> +	unsigned long *pt = pml4, iter_pte;
>  	unsigned offset;
>  
>  	assert(level >= 1 && level <= 4);
>  
>  	for (l = EPT_PAGE_LEVEL; ; --l) {
>  		offset = (guest_addr >> EPT_LEVEL_SHIFT(l)) & EPT_PGDIR_MASK;
> -		pte = pt[offset];
> -		if (!(pte & (EPT_PRESENT)))
> -			return -1;
> +		iter_pte = pt[offset];
> +		if (!(iter_pte & (EPT_PRESENT)))
> +			return false;
>  		if (l == level)
>  			break;
> -		if (l < 4 && (pte & EPT_LARGE_PAGE))
> -			return -1;
> -		pt = (unsigned long *)(pte & EPT_ADDR_MASK);
> +		if (l < 4 && (iter_pte & EPT_LARGE_PAGE))
> +			return false;
> +		pt = (unsigned long *)(iter_pte & EPT_ADDR_MASK);
>  	}
>  	offset = (guest_addr >> EPT_LEVEL_SHIFT(l)) & EPT_PGDIR_MASK;
> -	pte = pt[offset];
> -	return pte;
> +	if (pte)
> +		*pte = pt[offset];
> +	return true;
>  }
>  
>  static void clear_ept_ad_pte(unsigned long *pml4, unsigned long guest_addr)
> @@ -894,9 +895,10 @@ void check_ept_ad(unsigned long *pml4, u64 guest_cr3,
>  	for (l = EPT_PAGE_LEVEL; ; --l) {
>  		offset = (guest_addr >> EPT_LEVEL_SHIFT(l)) & EPT_PGDIR_MASK;
>  
> -		ept_pte = get_ept_pte(pml4, (u64) &pt[offset], 1);
> -		if (ept_pte == 0)
> +		if (!get_ept_pte(pml4, (u64) &pt[offset], 1, &ept_pte)) {
> +			printf("EPT - guest level %d page table is not mapped.\n", l);
>  			return;
> +		}
>  
>  		if (!bad_pt_ad) {
>  			bad_pt_ad |= (ept_pte & (EPT_ACCESS_FLAG|EPT_DIRTY_FLAG)) != expected_pt_ad;
> @@ -925,7 +927,10 @@ void check_ept_ad(unsigned long *pml4, u64 guest_cr3,
>  	offset_in_page = guest_addr & ((1 << EPT_LEVEL_SHIFT(l)) - 1);
>  	gpa = (pt[offset] & PT_ADDR_MASK) | (guest_addr & offset_in_page);
>  
> -	ept_pte = get_ept_pte(pml4, gpa, 1);
> +	if (!get_ept_pte(pml4, gpa, 1, &ept_pte)) {
> +		report("EPT - guest physical address is not mapped", false);
> +		return;
> +	}
>  	report("EPT - guest physical address A=%d/D=%d",
>  	       (ept_pte & (EPT_ACCESS_FLAG|EPT_DIRTY_FLAG)) == expected_gpa_ad,
>  	       !!(expected_gpa_ad & EPT_ACCESS_FLAG),
> diff --git a/x86/vmx.h b/x86/vmx.h
> index 787466653f8b..efbb320f44c7 100644
> --- a/x86/vmx.h
> +++ b/x86/vmx.h
> @@ -697,8 +697,8 @@ void install_ept(unsigned long *pml4, unsigned long phys,
>  		unsigned long guest_addr, u64 perm);
>  void setup_ept_range(unsigned long *pml4, unsigned long start,
>  		     unsigned long len, int map_1g, int map_2m, u64 perm);
> -unsigned long get_ept_pte(unsigned long *pml4,
> -		unsigned long guest_addr, int level);
> +bool get_ept_pte(unsigned long *pml4, unsigned long guest_addr, int level,
> +		unsigned long *pte);
>  void set_ept_pte(unsigned long *pml4, unsigned long guest_addr,
>  		int level, u64 pte_val);
>  void check_ept_ad(unsigned long *pml4, u64 guest_cr3,
> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
> index 7be016ce4fbc..181c3c73cb60 100644
> --- a/x86/vmx_tests.c
> +++ b/x86/vmx_tests.c
> @@ -1212,17 +1212,18 @@ static int ept_exit_handler_common(bool have_ad)
>  			break;
>  		case 3:
>  			clear_ept_ad(pml4, guest_cr3, (unsigned long)data_page1);
> -			data_page1_pte = get_ept_pte(pml4,
> -				(unsigned long)data_page1, 1);
> +			TEST_ASSERT(get_ept_pte(pml4, (unsigned long)data_page1,
> +						1, &data_page1_pte));
>  			set_ept_pte(pml4, (unsigned long)data_page1, 
>  				1, data_page1_pte & ~EPT_PRESENT);
>  			ept_sync(INVEPT_SINGLE, eptp);
>  			break;
>  		case 4:
> -			data_page1_pte = get_ept_pte(pml4,
> -				(unsigned long)data_page1, 2);
> +			TEST_ASSERT(get_ept_pte(pml4, (unsigned long)data_page1,
> +						2, &data_page1_pte));
>  			data_page1_pte &= PAGE_MASK;
> -			data_page1_pte_pte = get_ept_pte(pml4, data_page1_pte, 2);
> +			TEST_ASSERT(get_ept_pte(pml4, data_page1_pte,
> +						2, &data_page1_pte_pte));
>  			set_ept_pte(pml4, data_page1_pte, 2,
>  				data_page1_pte_pte & ~EPT_PRESENT);
>  			ept_sync(INVEPT_SINGLE, eptp);
> @@ -1267,7 +1268,7 @@ static int ept_exit_handler_common(bool have_ad)
>  			if (exit_qual == (EPT_VLT_WR | EPT_VLT_LADDR_VLD |
>  					EPT_VLT_PADDR))
>  				vmx_inc_test_stage();
> -			set_ept_pte(pml4, (unsigned long)data_page1, 
> +			set_ept_pte(pml4, (unsigned long)data_page1,
>  				1, data_page1_pte | (EPT_PRESENT));
>  			ept_sync(INVEPT_SINGLE, eptp);
>  			break;
> @@ -2175,8 +2176,7 @@ static unsigned long ept_twiddle(unsigned long gpa, bool mkhuge, int level,
>  	unsigned long pte;
>  
>  	/* Screw with the mapping at the requested level. */
> -	orig_pte = get_ept_pte(pml4, gpa, level);
> -	TEST_ASSERT(orig_pte != -1);
> +	TEST_ASSERT(get_ept_pte(pml4, gpa, level, &orig_pte));
>  	pte = orig_pte;
>  	if (mkhuge)
>  		pte = (orig_pte & ~EPT_ADDR_MASK) | data->hpa | EPT_LARGE_PAGE;
> @@ -2603,8 +2603,8 @@ static void ept_access_test_setup(void)
>  	 * Make sure nothing's mapped here so the tests that screw with the
>  	 * pml4 entry don't inadvertently break something.
>  	 */
> -	TEST_ASSERT_EQ(get_ept_pte(pml4, data->gpa, 4), -1);
> -	TEST_ASSERT_EQ(get_ept_pte(pml4, data->gpa + size - 1, 4), -1);
> +	TEST_ASSERT(!get_ept_pte(pml4, data->gpa, 4, NULL));
> +	TEST_ASSERT(!get_ept_pte(pml4, data->gpa + size - 1, 4, NULL));
>  	install_ept(pml4, data->hpa, data->gpa, EPT_PRESENT);
>  
>  	data->hva[0] = MAGIC_VAL_1;
> 

Re: [kvm-unit-tests PATCH 2/3] x86/vmx: fix detection of unmapped PTE

[Paolo Bonzini]

On 30/06/2017 12:33, Paolo Bonzini wrote:
> This breaks the vmx PML test:
> 
> Test suite: PML
> PASS: PML - Dirty GPA Logging
> ERROR - unexpected stage, 2.
> VMEXIT info:
> 	vmexit reason = 18
> 	exit qualification = 0
> 	Bit 31 of reason = 0
> 	guest_rip = 0x4049b3
> 	RAX=0x149a000    RBX=0x3f8    RCX=0x184    RDX=0x3fd
> 	RSP=0x46efcf    RBP=0x46efdf    RSI=0x41a8a3    RDI=0x41a8a3
> 	R8 =0xa    R9 =0x3f8    R10=0    R11=0
> 	R12=0    R13=0    R14=0    R15=0
> 
> 
> (should have been "PASS: PML Full Event").  I didn't investigate why.

Hmm, actually it seems to come and go.  I've applied patches 2 and 3 for
now.

Paolo

Re: [kvm-unit-tests PATCH 2/3] x86/vmx: fix detection of unmapped PTE

[Radim Krčmář]

2017-07-03 12:34+0200, Paolo Bonzini:
> On 30/06/2017 12:33, Paolo Bonzini wrote:
> > This breaks the vmx PML test:
> > 
> > Test suite: PML
> > PASS: PML - Dirty GPA Logging
> > ERROR - unexpected stage, 2.
> > VMEXIT info:
> > 	vmexit reason = 18
> > 	exit qualification = 0
> > 	Bit 31 of reason = 0
> > 	guest_rip = 0x4049b3
> > 	RAX=0x149a000    RBX=0x3f8    RCX=0x184    RDX=0x3fd
> > 	RSP=0x46efcf    RBP=0x46efdf    RSI=0x41a8a3    RDI=0x41a8a3
> > 	R8 =0xa    R9 =0x3f8    R10=0    R11=0
> > 	R12=0    R13=0    R14=0    R15=0
> > 
> > 
> > (should have been "PASS: PML Full Event").  I didn't investigate why.
> 
> Hmm, actually it seems to come and go.  I've applied patches 2 and 3 for
> now.

I had to use a different machine to reproduce ... with the patches, the
guest report() (probably) accesses 1 more page, which adds one entry to
PML and changes the progress of this pml_main loop:

	while (vmx_get_test_stage() == 1) {
		*((u32 *)data_page2) = 0x1;
		if (count++ > PML_INDEX)
			break;
		vmcall();
	}

Each cycle adds 4 entries to the PML and the extra entry makes the log
fill just before the vmcall.  I don't think the one extra access is
something we need to tend to, so I'll send a fix for the PML test.

Re: [kvm-unit-tests PATCH 1/3] x86/vmx: fix EPT - MMIO access

[Radim Krčmář]

2017-06-30 12:22+0200, Paolo Bonzini:
> On 29/06/2017 19:34, Peter Feiner wrote:
> > Reading the memory mapped page with x2apic is a bug.  Use the generic reader
> > instead.  An alternative would be to disable x2apic.
> 
> Disabling x2apic would test what the test is supposed to test. :)

We should still be doing the same --  we're interested in testing the
MMIO access from the guest, which is being done in ept_main().

ept_init_common() is used just to access the reference value and I think
that allowing x2APIC there gives us a bit more leeway,

thanks.

Re: [kvm-unit-tests PATCH 1/3] x86/vmx: fix EPT - MMIO access

[Paolo Bonzini]

----- Original Message -----
> From: "Radim Krčmář" <rkrcmar@redhat.com>
> To: "Paolo Bonzini" <pbonzini@redhat.com>
> Cc: "Peter Feiner" <pfeiner@google.com>, kvm@vger.kernel.org, "David Matlack" <dmatlack@google.com>
> Sent: Monday, July 3, 2017 7:13:44 PM
> Subject: Re: [kvm-unit-tests PATCH 1/3] x86/vmx: fix EPT - MMIO access
> 
> 2017-06-30 12:22+0200, Paolo Bonzini:
> > On 29/06/2017 19:34, Peter Feiner wrote:
> > > Reading the memory mapped page with x2apic is a bug.  Use the generic
> > > reader
> > > instead.  An alternative would be to disable x2apic.
> > 
> > Disabling x2apic would test what the test is supposed to test. :)
> 
> We should still be doing the same --  we're interested in testing the
> MMIO access from the guest, which is being done in ept_main().
> 
> ept_init_common() is used just to access the reference value and I think
> that allowing x2APIC there gives us a bit more leeway,

Thanks for the explanation.  You're obviously right, will apply tomorrow
with an improved commit message.

Paolo