diff mbox

[kvm-unit-tests] x86: fix ept_access_test_paddr exit qualifications

Message ID 20170505205723.39601-1-dmatlack@google.com (mailing list archive)
State New, archived
Headers show

Commit Message

David Matlack May 5, 2017, 8:57 p.m. UTC
SDM Volume 3, Section 28.2.3.2 EPT Violations:
Reads by the logical processor of guest paging structures to translate a
linear address are considered to be data reads.

SDM Volume 3, Table 27-7, Footnote 1:
If A/D flags for EPT are enabled, accesses to guest paging-structure
entries are treated as writes with regard to EPT Violations. If such an
access causes an EPT violation, the processor sets both bit 0 and bit 1
of the exit qualification.

Signed-off-by: David Matlack <dmatlack@google.com>
---
 x86/vmx_tests.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

This patch fixes the following test failures:

FAIL vmx_ept_access_test_paddr_not_present_ad_enabled (65 tests, 3 unexpected failures)
FAIL vmx_ept_access_test_paddr_read_only_ad_enabled (175 tests, 9 unexpected failures)
FAIL vmx_ept_access_test_paddr_read_execute_ad_enabled (175 tests, 9 unexpected failures)

Comments

Paolo Bonzini May 6, 2017, 8:54 a.m. UTC | #1
On 05/05/2017 22:57, David Matlack wrote:
> SDM Volume 3, Section 28.2.3.2 EPT Violations:
> Reads by the logical processor of guest paging structures to translate a
> linear address are considered to be data reads.

This is for A/D bits disabled.

> SDM Volume 3, Table 27-7, Footnote 1:
> If A/D flags for EPT are enabled, accesses to guest paging-structure
> entries are treated as writes with regard to EPT Violations. If such an
> access causes an EPT violation, the processor sets both bit 0 and bit 1
> of the exit qualification.

This is for A/D enabled.

>  static void ept_access_test_paddr_read_only_ad_disabled(void)
>  {
> -	u64 qual = EPT_VLT_WR | EPT_VLT_PERM_RD;
> +	u64 qual = EPT_VLT_WR | EPT_VLT_RD | EPT_VLT_PERM_RD;
>  
>  	ept_access_test_setup();
>  	ept_disable_ad_bits();
> @@ -2754,7 +2756,7 @@ static void ept_access_test_paddr_read_write_execute(void)
>  
>  static void ept_access_test_paddr_read_execute_ad_disabled(void)
>  {
> -	u64 qual = EPT_VLT_WR | EPT_VLT_PERM_RD | EPT_VLT_PERM_EX;
> +	u64 qual = EPT_VLT_WR | EPT_VLT_RD | EPT_VLT_PERM_RD | EPT_VLT_PERM_EX;
>  
>  	ept_access_test_setup();
>  	ept_disable_ad_bits();

So I think these should use EPT_VLT_RD only.

Paolo
David Matlack May 11, 2017, 6:41 p.m. UTC | #2
On Sat, May 6, 2017 at 1:54 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
>
> On 05/05/2017 22:57, David Matlack wrote:
> > SDM Volume 3, Section 28.2.3.2 EPT Violations:
> > Reads by the logical processor of guest paging structures to translate a
> > linear address are considered to be data reads.
>
> This is for A/D bits disabled.
>
> > SDM Volume 3, Table 27-7, Footnote 1:
> > If A/D flags for EPT are enabled, accesses to guest paging-structure
> > entries are treated as writes with regard to EPT Violations. If such an
> > access causes an EPT violation, the processor sets both bit 0 and bit 1
> > of the exit qualification.
>
> This is for A/D enabled.
>
> >  static void ept_access_test_paddr_read_only_ad_disabled(void)
> >  {
> > -     u64 qual = EPT_VLT_WR | EPT_VLT_PERM_RD;
> > +     u64 qual = EPT_VLT_WR | EPT_VLT_RD | EPT_VLT_PERM_RD;
> >
> >       ept_access_test_setup();
> >       ept_disable_ad_bits();
> > @@ -2754,7 +2756,7 @@ static void ept_access_test_paddr_read_write_execute(void)
> >
> >  static void ept_access_test_paddr_read_execute_ad_disabled(void)
> >  {
> > -     u64 qual = EPT_VLT_WR | EPT_VLT_PERM_RD | EPT_VLT_PERM_EX;
> > +     u64 qual = EPT_VLT_WR | EPT_VLT_RD | EPT_VLT_PERM_RD | EPT_VLT_PERM_EX;
> >
> >       ept_access_test_setup();
> >       ept_disable_ad_bits();
>
> So I think these should use EPT_VLT_RD only.

IIUC, EPT_VLT_WR is set in qual because the processor tries to update
the A and/or D bit on the x86 page table entry, but the guest page
containing the page table entry is mapped Read-Execute in the EPT.

>
> Paolo
Paolo Bonzini May 12, 2017, 8:43 a.m. UTC | #3
On 05/05/2017 22:57, David Matlack wrote:
>  static void ept_access_test_paddr_read_only_ad_disabled(void)
>  {
> -	u64 qual = EPT_VLT_WR | EPT_VLT_PERM_RD;
> +	u64 qual = EPT_VLT_WR | EPT_VLT_RD | EPT_VLT_PERM_RD;

So this patch *was* correct after all.  Sorry. :)

I've added a comment here and in
ept_access_test_paddr_read_execute_ad_disabled:

	/*
	 * When EPT AD bits are disabled, all accesses to guest paging
	 * structures are reported separately as a read and (after
	 * translation of the GPA to host physical address) a read+write
	 * if the A/D bits have to be set.
	 */

then squashed everything and pushed to kvm-unit-tests.git master branch.

Thanks again!

Paolo
diff mbox

Patch

diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index 1766af3fa708..1749c8a572e8 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -2682,17 +2682,19 @@  static void ept_access_test_paddr_not_present_ad_disabled(void)
 
 static void ept_access_test_paddr_not_present_ad_enabled(void)
 {
+	u64 qual = EPT_VLT_RD | EPT_VLT_WR;
+
 	ept_access_test_setup();
 	ept_enable_ad_bits_or_skip_test();
 
-	ept_access_violation_paddr(0, PT_AD_MASK, OP_READ, EPT_VLT_WR);
-	ept_access_violation_paddr(0, PT_AD_MASK, OP_WRITE, EPT_VLT_WR);
-	ept_access_violation_paddr(0, PT_AD_MASK, OP_EXEC, EPT_VLT_WR);
+	ept_access_violation_paddr(0, PT_AD_MASK, OP_READ, qual);
+	ept_access_violation_paddr(0, PT_AD_MASK, OP_WRITE, qual);
+	ept_access_violation_paddr(0, PT_AD_MASK, OP_EXEC, qual);
 }
 
 static void ept_access_test_paddr_read_only_ad_disabled(void)
 {
-	u64 qual = EPT_VLT_WR | EPT_VLT_PERM_RD;
+	u64 qual = EPT_VLT_WR | EPT_VLT_RD | EPT_VLT_PERM_RD;
 
 	ept_access_test_setup();
 	ept_disable_ad_bits();
@@ -2718,7 +2720,7 @@  static void ept_access_test_paddr_read_only_ad_enabled(void)
 	 * structures are considered writes as far as EPT translation
 	 * is concerned.
 	 */
-	u64 qual = EPT_VLT_WR | EPT_VLT_PERM_RD;
+	u64 qual = EPT_VLT_WR | EPT_VLT_RD | EPT_VLT_PERM_RD;
 
 	ept_access_test_setup();
 	ept_enable_ad_bits_or_skip_test();
@@ -2754,7 +2756,7 @@  static void ept_access_test_paddr_read_write_execute(void)
 
 static void ept_access_test_paddr_read_execute_ad_disabled(void)
 {
-	u64 qual = EPT_VLT_WR | EPT_VLT_PERM_RD | EPT_VLT_PERM_EX;
+	u64 qual = EPT_VLT_WR | EPT_VLT_RD | EPT_VLT_PERM_RD | EPT_VLT_PERM_EX;
 
 	ept_access_test_setup();
 	ept_disable_ad_bits();
@@ -2780,7 +2782,7 @@  static void ept_access_test_paddr_read_execute_ad_enabled(void)
 	 * structures are considered writes as far as EPT translation
 	 * is concerned.
 	 */
-	u64 qual = EPT_VLT_WR | EPT_VLT_PERM_RD | EPT_VLT_PERM_EX;
+	u64 qual = EPT_VLT_WR | EPT_VLT_RD | EPT_VLT_PERM_RD | EPT_VLT_PERM_EX;
 
 	ept_access_test_setup();
 	ept_enable_ad_bits_or_skip_test();