diff mbox

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

Message ID 20170629172647.22188-3-rkrcmar@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Radim Krčmář June 29, 2017, 5:26 p.m. UTC 
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(-)

Comments

Peter Feiner June 29, 2017, 5:38 p.m. UTC | #1 
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!
Paolo Bonzini June 30, 2017, 10:33 a.m. UTC | #2 
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;
>
Paolo Bonzini July 3, 2017, 10:34 a.m. UTC | #3 
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
Radim Krčmář July 3, 2017, 4:42 p.m. UTC | #4 
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.
diff mbox

Patch

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;