[PATCH kvm-unit-tests 0/2] vmx: fixes to v2 tests

[PATCH kvm-unit-tests 0/2] vmx: fixes to v2 tests

[Paolo Bonzini]

These patches fix the new EPT access tests by Peter Feiner and David Matlack,
so that (with a couple KVM bugs fixed) everything passes with both
eptad=0 and eptad=1.

I've pushed these fixes into the "next" branch to kvm-unit-tests.git;
I've also squashed these into the original patches and pushed the
result to branch "next-fixed", which I intend to merge into master.
Reviews of course are always welcome!

Paolo

Paolo Bonzini (2):
  vmx: always perform access with modified page table for PADDR tests
  vmx: fix expected results of new EPT tests

 x86/vmx_tests.c | 71 ++++++++++++++++++++++++++++++---------------------------
 1 file changed, 38 insertions(+), 33 deletions(-)

-- 
1.8.3.1

[PATCH kvm-unit-tests 1/2] vmx: always perform access with modified page table for PADDR tests

[Paolo Bonzini]

Even if we do not expect a violation, we should try and perform the
access while the EPT page tables have been modified.  Without this
change, ept_access_paddr is basically a no-op with expect_violation
equal to false.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 x86/vmx_tests.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index 734d785..e1f92d4 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -2340,12 +2340,15 @@ static void ept_access_paddr(unsigned long ept_access, unsigned long pte_ad,
 	orig_epte = ept_twiddle(gpa, /*mkhuge=*/0, /*level=*/1,
 				/*clear=*/EPT_PRESENT, /*set=*/ept_access);
 
-	if (expect_violation)
+	if (expect_violation) {
 		do_ept_violation(/*leaf=*/true, op,
 				 expected_qual | EPT_VLT_LADDR_VLD, gpa);
-
-	ept_untwiddle(gpa, /*level=*/1, orig_epte);
-	do_ept_access_op(op);
+		ept_untwiddle(gpa, /*level=*/1, orig_epte);
+		do_ept_access_op(op);
+	} else {
+		do_ept_access_op(op);
+		ept_untwiddle(gpa, /*level=*/1, orig_epte);
+	}
 
 	TEST_ASSERT(*ptep & PT_ACCESSED_MASK);
 	if ((pte_ad & PT_DIRTY_MASK) || op == OP_WRITE)
-- 
1.8.3.1

[PATCH kvm-unit-tests 2/2] vmx: fix expected results of new EPT tests

[Paolo Bonzini]

Remove RD/EX exchange hack which we can fix in KVM; mark page table
accesses as read/write when EPT A/D is enabled, and expect them to
be handled as read/write even with disabled EPT A/D bits (even though
the exit qualification says otherwise).

With these changes, and the corresponding KVM patch I'm going to send
out, all v2 tests pass with both eptad=0 and eptad=1.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 x86/vmx_tests.c | 60 +++++++++++++++++++++++++++++----------------------------
 1 file changed, 31 insertions(+), 29 deletions(-)

diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
index e1f92d4..03e4ad4 100644
--- a/x86/vmx_tests.c
+++ b/x86/vmx_tests.c
@@ -2206,12 +2206,6 @@ static void do_ept_violation(bool leaf, enum ept_access_op op,
 
 	qual = vmcs_read(EXI_QUALIFICATION);
 
-	/* Hack now so important tests can pass. */
-	if (!leaf && (expected_qual & EPT_VLT_PERM_RD)
-	    && !(expected_qual & EPT_VLT_PERM_EX))
-		expected_qual = (expected_qual & ~EPT_VLT_PERM_RD) |
-				EPT_VLT_PERM_EX;
-
 	diagnose_ept_violation_qual(expected_qual, qual);
 	TEST_EXPECT_EQ(expected_qual, qual);
 
@@ -2777,33 +2771,38 @@ 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;
+	/*
+	 * When EPT AD bits are disabled, all accesses to guest paging
+	 * structures are reported as reads as far as EPT translation
+	 * is concerned, but any write of A/D bits still fails (with an
+	 * EPT violation and exit qualification 010'001'001).
+	 */
+	u64 qual = EPT_VLT_RD | EPT_VLT_PERM_RD;
 
 	ept_access_test_setup();
 	ept_disable_ad_bits();
 
-	/* Can't update A bit, so all accesses fail. */
 	ept_access_violation_paddr(EPT_RA, 0, OP_READ, qual);
 	ept_access_violation_paddr(EPT_RA, 0, OP_WRITE, qual);
 	ept_access_violation_paddr(EPT_RA, 0, OP_EXEC, qual);
-	/* AD bits disabled, so only writes try to update the D bit. */
-	ept_access_allowed_paddr(EPT_RA, PT_ACCESSED_MASK, OP_READ);
+	ept_access_violation_paddr(EPT_RA, PT_ACCESSED_MASK, OP_READ, qual);
 	ept_access_violation_paddr(EPT_RA, PT_ACCESSED_MASK, OP_WRITE, qual);
-	ept_access_allowed_paddr(EPT_RA, PT_ACCESSED_MASK, OP_EXEC);
-	/* Both A and D already set, so read-only is OK. */
-	ept_access_allowed_paddr(EPT_RA, PT_AD_MASK, OP_READ);
-	ept_access_allowed_paddr(EPT_RA, PT_AD_MASK, OP_WRITE);
-	ept_access_allowed_paddr(EPT_RA, PT_AD_MASK, OP_EXEC);
+	ept_access_violation_paddr(EPT_RA, PT_ACCESSED_MASK, OP_EXEC, qual);
+	ept_access_violation_paddr(EPT_RA, PT_AD_MASK, OP_READ, qual);
+	ept_access_violation_paddr(EPT_RA, PT_AD_MASK, OP_WRITE, qual);
+	ept_access_violation_paddr(EPT_RA, PT_AD_MASK, OP_EXEC, qual);
 }
 
 static void ept_access_test_paddr_read_only_ad_enabled(void)
@@ -2813,7 +2812,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();
@@ -2849,23 +2848,26 @@ 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;
+	/*
+	 * When EPT AD bits are disabled, all accesses to guest paging
+	 * structures are reported as reads as far as EPT translation
+	 * is concerned, but any write of A/D bits still fails (with an
+	 * EPT violation and exit qualification 010'101'001).
+	 */
+	u64 qual = EPT_VLT_RD | EPT_VLT_PERM_RD | EPT_VLT_PERM_EX;
 
 	ept_access_test_setup();
 	ept_disable_ad_bits();
 
-	/* Can't update A bit, so all accesses fail. */
 	ept_access_violation_paddr(EPT_RA | EPT_EA, 0, OP_READ, qual);
 	ept_access_violation_paddr(EPT_RA | EPT_EA, 0, OP_WRITE, qual);
 	ept_access_violation_paddr(EPT_RA | EPT_EA, 0, OP_EXEC, qual);
-	/* AD bits disabled, so only writes try to update the D bit. */
-	ept_access_allowed_paddr(EPT_RA | EPT_EA, PT_ACCESSED_MASK, OP_READ);
+	ept_access_violation_paddr(EPT_RA | EPT_EA, PT_ACCESSED_MASK, OP_READ, qual);
 	ept_access_violation_paddr(EPT_RA | EPT_EA, PT_ACCESSED_MASK, OP_WRITE, qual);
-	ept_access_allowed_paddr(EPT_RA | EPT_EA, PT_ACCESSED_MASK, OP_EXEC);
-	/* Both A and D already set, so read-only is OK. */
-	ept_access_allowed_paddr(EPT_RA | EPT_EA, PT_AD_MASK, OP_READ);
-	ept_access_allowed_paddr(EPT_RA | EPT_EA, PT_AD_MASK, OP_WRITE);
-	ept_access_allowed_paddr(EPT_RA | EPT_EA, PT_AD_MASK, OP_EXEC);
+	ept_access_violation_paddr(EPT_RA | EPT_EA, PT_ACCESSED_MASK, OP_EXEC, qual);
+	ept_access_violation_paddr(EPT_RA | EPT_EA, PT_AD_MASK, OP_READ, qual);
+	ept_access_violation_paddr(EPT_RA | EPT_EA, PT_AD_MASK, OP_WRITE, qual);
+	ept_access_violation_paddr(EPT_RA | EPT_EA, PT_AD_MASK, OP_EXEC, qual);
 }
 
 static void ept_access_test_paddr_read_execute_ad_enabled(void)
@@ -2875,7 +2877,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();
-- 
1.8.3.1

Re: [PATCH kvm-unit-tests 2/2] vmx: fix expected results of new EPT tests

[Peter Feiner]

On Thu, May 11, 2017 at 4:23 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Remove RD/EX exchange hack which we can fix in KVM; mark page table
> accesses as read/write when EPT A/D is enabled, and expect them to
> be handled as read/write even with disabled EPT A/D bits (even though
> the exit qualification says otherwise).

I assume this is a stopgap change. I mean, you're asserting for the
wrong behavior just so the tests pass. Correct?

To fix this properly, you've got to disable EPT A/D in VMCS02 when
it's disabled in VMCS12. We've been running with such a patch for the
last year but haven't sent it upstream yet :-(   (Shame on us for
upstreaming the test without the fix!)

>
> With these changes, and the corresponding KVM patch I'm going to send
> out, all v2 tests pass with both eptad=0 and eptad=1.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  x86/vmx_tests.c | 60 +++++++++++++++++++++++++++++----------------------------
>  1 file changed, 31 insertions(+), 29 deletions(-)
>
> diff --git a/x86/vmx_tests.c b/x86/vmx_tests.c
> index e1f92d4..03e4ad4 100644
> --- a/x86/vmx_tests.c
> +++ b/x86/vmx_tests.c
> @@ -2206,12 +2206,6 @@ static void do_ept_violation(bool leaf, enum ept_access_op op,
>
>         qual = vmcs_read(EXI_QUALIFICATION);
>
> -       /* Hack now so important tests can pass. */
> -       if (!leaf && (expected_qual & EPT_VLT_PERM_RD)
> -           && !(expected_qual & EPT_VLT_PERM_EX))
> -               expected_qual = (expected_qual & ~EPT_VLT_PERM_RD) |
> -                               EPT_VLT_PERM_EX;
> -
>         diagnose_ept_violation_qual(expected_qual, qual);
>         TEST_EXPECT_EQ(expected_qual, qual);
>
> @@ -2777,33 +2771,38 @@ 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;
> +       /*
> +        * When EPT AD bits are disabled, all accesses to guest paging
> +        * structures are reported as reads as far as EPT translation
> +        * is concerned, but any write of A/D bits still fails (with an
> +        * EPT violation and exit qualification 010'001'001).
> +        */
> +       u64 qual = EPT_VLT_RD | EPT_VLT_PERM_RD;
>
>         ept_access_test_setup();
>         ept_disable_ad_bits();
>
> -       /* Can't update A bit, so all accesses fail. */
>         ept_access_violation_paddr(EPT_RA, 0, OP_READ, qual);
>         ept_access_violation_paddr(EPT_RA, 0, OP_WRITE, qual);
>         ept_access_violation_paddr(EPT_RA, 0, OP_EXEC, qual);
> -       /* AD bits disabled, so only writes try to update the D bit. */
> -       ept_access_allowed_paddr(EPT_RA, PT_ACCESSED_MASK, OP_READ);
> +       ept_access_violation_paddr(EPT_RA, PT_ACCESSED_MASK, OP_READ, qual);
>         ept_access_violation_paddr(EPT_RA, PT_ACCESSED_MASK, OP_WRITE, qual);
> -       ept_access_allowed_paddr(EPT_RA, PT_ACCESSED_MASK, OP_EXEC);
> -       /* Both A and D already set, so read-only is OK. */
> -       ept_access_allowed_paddr(EPT_RA, PT_AD_MASK, OP_READ);
> -       ept_access_allowed_paddr(EPT_RA, PT_AD_MASK, OP_WRITE);
> -       ept_access_allowed_paddr(EPT_RA, PT_AD_MASK, OP_EXEC);
> +       ept_access_violation_paddr(EPT_RA, PT_ACCESSED_MASK, OP_EXEC, qual);
> +       ept_access_violation_paddr(EPT_RA, PT_AD_MASK, OP_READ, qual);
> +       ept_access_violation_paddr(EPT_RA, PT_AD_MASK, OP_WRITE, qual);
> +       ept_access_violation_paddr(EPT_RA, PT_AD_MASK, OP_EXEC, qual);
>  }
>
>  static void ept_access_test_paddr_read_only_ad_enabled(void)
> @@ -2813,7 +2812,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();
> @@ -2849,23 +2848,26 @@ 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;
> +       /*
> +        * When EPT AD bits are disabled, all accesses to guest paging
> +        * structures are reported as reads as far as EPT translation
> +        * is concerned, but any write of A/D bits still fails (with an
> +        * EPT violation and exit qualification 010'101'001).
> +        */
> +       u64 qual = EPT_VLT_RD | EPT_VLT_PERM_RD | EPT_VLT_PERM_EX;
>
>         ept_access_test_setup();
>         ept_disable_ad_bits();
>
> -       /* Can't update A bit, so all accesses fail. */
>         ept_access_violation_paddr(EPT_RA | EPT_EA, 0, OP_READ, qual);
>         ept_access_violation_paddr(EPT_RA | EPT_EA, 0, OP_WRITE, qual);
>         ept_access_violation_paddr(EPT_RA | EPT_EA, 0, OP_EXEC, qual);
> -       /* AD bits disabled, so only writes try to update the D bit. */
> -       ept_access_allowed_paddr(EPT_RA | EPT_EA, PT_ACCESSED_MASK, OP_READ);
> +       ept_access_violation_paddr(EPT_RA | EPT_EA, PT_ACCESSED_MASK, OP_READ, qual);
>         ept_access_violation_paddr(EPT_RA | EPT_EA, PT_ACCESSED_MASK, OP_WRITE, qual);
> -       ept_access_allowed_paddr(EPT_RA | EPT_EA, PT_ACCESSED_MASK, OP_EXEC);
> -       /* Both A and D already set, so read-only is OK. */
> -       ept_access_allowed_paddr(EPT_RA | EPT_EA, PT_AD_MASK, OP_READ);
> -       ept_access_allowed_paddr(EPT_RA | EPT_EA, PT_AD_MASK, OP_WRITE);
> -       ept_access_allowed_paddr(EPT_RA | EPT_EA, PT_AD_MASK, OP_EXEC);
> +       ept_access_violation_paddr(EPT_RA | EPT_EA, PT_ACCESSED_MASK, OP_EXEC, qual);
> +       ept_access_violation_paddr(EPT_RA | EPT_EA, PT_AD_MASK, OP_READ, qual);
> +       ept_access_violation_paddr(EPT_RA | EPT_EA, PT_AD_MASK, OP_WRITE, qual);
> +       ept_access_violation_paddr(EPT_RA | EPT_EA, PT_AD_MASK, OP_EXEC, qual);
>  }
>
>  static void ept_access_test_paddr_read_execute_ad_enabled(void)
> @@ -2875,7 +2877,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();
> --
> 1.8.3.1
>

Re: [PATCH kvm-unit-tests 2/2] vmx: fix expected results of new EPT tests

[Paolo Bonzini]

----- Original Message -----
> From: "Peter Feiner" <pfeiner@google.com>
> To: "Paolo Bonzini" <pbonzini@redhat.com>
> Cc: kvm@vger.kernel.org, "David Matlack" <dmatlack@google.com>
> Sent: Thursday, May 11, 2017 5:58:49 PM
> Subject: Re: [PATCH kvm-unit-tests 2/2] vmx: fix expected results of new EPT tests
> 
> On Thu, May 11, 2017 at 4:23 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> > Remove RD/EX exchange hack which we can fix in KVM; mark page table
> > accesses as read/write when EPT A/D is enabled, and expect them to
> > be handled as read/write even with disabled EPT A/D bits (even though
> > the exit qualification says otherwise).
> 
> I assume this is a stopgap change. I mean, you're asserting for the
> wrong behavior just so the tests pass. Correct?

No, I've tried the tests on upstream Linux with eptad=0 (so that EPT A/D
is not used by KVM on the host) and they also hang with an infinite stream
of EPT violations.  See the KVM patch I sent which also explains the hang
in the comments ("[PATCH 2/2] KVM: nVMX: fix nEPT handling of guest page
table accesses").

So it seems to me that this is the expected behavior of the processor
even when A/D bits are disabled.  I haven't tested on a processor with
EPT but without A/D bits though.

Paolo

Re: [PATCH kvm-unit-tests 2/2] vmx: fix expected results of new EPT tests

[David Matlack]

On Thu, May 11, 2017 at 10:30 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> ----- Original Message -----
>> From: "Peter Feiner" <pfeiner@google.com>
>> To: "Paolo Bonzini" <pbonzini@redhat.com>
>> Cc: kvm@vger.kernel.org, "David Matlack" <dmatlack@google.com>
>> Sent: Thursday, May 11, 2017 5:58:49 PM
>> Subject: Re: [PATCH kvm-unit-tests 2/2] vmx: fix expected results of new EPT tests
>>
>> On Thu, May 11, 2017 at 4:23 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> > Remove RD/EX exchange hack which we can fix in KVM; mark page table
>> > accesses as read/write when EPT A/D is enabled, and expect them to
>> > be handled as read/write even with disabled EPT A/D bits (even though
>> > the exit qualification says otherwise).
>>
>> I assume this is a stopgap change. I mean, you're asserting for the
>> wrong behavior just so the tests pass. Correct?
>
> No, I've tried the tests on upstream Linux with eptad=0 (so that EPT A/D
> is not used by KVM on the host) and they also hang with an infinite stream
> of EPT violations.

I think the failures are caused by this code in handle_ept_violation,
which clears the ACC_WRITE bit of the exit qualification before
handling the fault, when EPT A/D is disabled:

if (is_guest_mode(vcpu)
   && !(exit_qualification & EPT_VIOLATION_GVA_TRANSLATED)) {
        /*
         * Fix up exit_qualification according to whether guest
         * page table accesses are reads or writes.
         */
        u64 eptp = nested_ept_get_cr3(vcpu);
        if (!(eptp & VMX_EPT_AD_ENABLE_BIT))
                exit_qualification &= ~EPT_VIOLATION_ACC_WRITE;
}

Per 28.2.3.2 EPT Violations: "Writes by the logical processor to guest
paging structures to update accessed and dirty flags are considered to
be data writes." In other words, it's valid for
EPT_VIOLATION_GVA_TRANSLATED and EPT_VIOLATION_ACC_WRITE to both be
set in the exit qual when EPT A/D is disabled.

> See the KVM patch I sent which also explains the hang
> in the comments ("[PATCH 2/2] KVM: nVMX: fix nEPT handling of guest page
> table accesses").
>
> So it seems to me that this is the expected behavior of the processor
> even when A/D bits are disabled.  I haven't tested on a processor with
> EPT but without A/D bits though.
>
> Paolo

Re: [PATCH kvm-unit-tests 2/2] vmx: fix expected results of new EPT tests

[Paolo Bonzini]

On 11/05/2017 21:51, David Matlack wrote:
>> No, I've tried the tests on upstream Linux with eptad=0 (so that EPT A/D
>> is not used by KVM on the host) and they also hang with an infinite stream
>> of EPT violations.
> 
> I think the failures are caused by this code in handle_ept_violation,
> which clears the ACC_WRITE bit of the exit qualification before
> handling the fault, when EPT A/D is disabled:
> 
> if (is_guest_mode(vcpu)
>    && !(exit_qualification & EPT_VIOLATION_GVA_TRANSLATED)) {
>         /*
>          * Fix up exit_qualification according to whether guest
>          * page table accesses are reads or writes.
>          */
>         u64 eptp = nested_ept_get_cr3(vcpu);
>         if (!(eptp & VMX_EPT_AD_ENABLE_BIT))
>                 exit_qualification &= ~EPT_VIOLATION_ACC_WRITE;
> }

I thought so as well; that's why my hypervisor patch moved 
trace_kvm_page_fault before munging exit_qualification.  Then I searched
for 0x82 or 0x8a in the traces, didn't find it, and got terribly confused.

What actually happens doesn't match what the SDM says.  You correctly cite this:

> Per 28.2.3.2 EPT Violations: "Writes by the logical processor to guest
> paging structures to update accessed and dirty flags are considered to
> be data writes." 

though here you mean EPT_VIOLATION_GVA_TRANSLATED=0, EPT_VIOLATION_ACC_WRITE=1:

> In other words, it's valid for
> EPT_VIOLATION_GVA_TRANSLATED and EPT_VIOLATION_ACC_WRITE to both be
> set in the exit qual when EPT A/D is disabled.

What really happens is that the processor first reads the page tables,
then translates the resulting GPA, and then finally does _another_ EPT
translation for the page tables, this time with EPT_VIOLATION_ACC_READ=1
and EPT_VIOLATION_ACC_WRITE=1:

        qemu-kvm-3683  [042]  2364.196479: kvm_entry:            vcpu 0
        qemu-kvm-3683  [042]  2364.196479: kvm_exit:             reason EPT_VIOLATION rip 0x40519e info 8b 0
        qemu-kvm-3683  [042]  2364.196479: kvm_nested_vmexit:    rip: 0x000000000040519e reason: EPT_VIOLATION ext_inf1: 0x000000000000008b ext_inf2: 0x0000000000000000 ext_int: 0x00000000 ext_int_err: 0x00000000
        qemu-kvm-3683  [042]  2364.196480: kvm_page_fault:       address 479000 error_code 8b
        qemu-kvm-3683  [042]  2364.196480: kvm_mmu_pagetable_walk: addr 479000 pferr 7 P|W|U
        qemu-kvm-3683  [042]  2364.196480: kvm_mmu_paging_element: pte 472007 level 4
        qemu-kvm-3683  [042]  2364.196480: kvm_mmu_paging_element: pte 473007 level 3
        qemu-kvm-3683  [042]  2364.196480: kvm_mmu_paging_element: pte 67c007 level 2
        qemu-kvm-3683  [042]  2364.196480: kvm_mmu_paging_element: pte 479001 level 1

(This trace comes from vanilla Linux 4.11.0).  This matches your kvm-unit-tests
patch ("x86: fix ept_access_test_paddr exit qualifications").  Aargh.

So Peter is right; the only way to get this behavior is to run with
EPT A/D bits disabled when L1 disables them.  I'll wait for you guys to send
a patch for this---we have plenty of time to include it in 4.12.0.  In the
meanwhile Radim shall drop patch 2 from the KVM series.

Thanks,

Paolo

Re: [PATCH kvm-unit-tests 2/2] vmx: fix expected results of new EPT tests

[David Matlack]

On Fri, May 12, 2017 at 1:45 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 11/05/2017 21:51, David Matlack wrote:
>>> No, I've tried the tests on upstream Linux with eptad=0 (so that EPT A/D
>>> is not used by KVM on the host) and they also hang with an infinite stream
>>> of EPT violations.
>>
>> I think the failures are caused by this code in handle_ept_violation,
>> which clears the ACC_WRITE bit of the exit qualification before
>> handling the fault, when EPT A/D is disabled:
>>
>> if (is_guest_mode(vcpu)
>>    && !(exit_qualification & EPT_VIOLATION_GVA_TRANSLATED)) {
>>         /*
>>          * Fix up exit_qualification according to whether guest
>>          * page table accesses are reads or writes.
>>          */
>>         u64 eptp = nested_ept_get_cr3(vcpu);
>>         if (!(eptp & VMX_EPT_AD_ENABLE_BIT))
>>                 exit_qualification &= ~EPT_VIOLATION_ACC_WRITE;
>> }
>
> I thought so as well; that's why my hypervisor patch moved
> trace_kvm_page_fault before munging exit_qualification.  Then I searched
> for 0x82 or 0x8a in the traces, didn't find it, and got terribly confused.
>
> What actually happens doesn't match what the SDM says.  You correctly cite this:
>
>> Per 28.2.3.2 EPT Violations: "Writes by the logical processor to guest
>> paging structures to update accessed and dirty flags are considered to
>> be data writes."
>
> though here you mean EPT_VIOLATION_GVA_TRANSLATED=0, EPT_VIOLATION_ACC_WRITE=1:
>
>> In other words, it's valid for
>> EPT_VIOLATION_GVA_TRANSLATED and EPT_VIOLATION_ACC_WRITE to both be
>> set in the exit qual when EPT A/D is disabled.
>
> What really happens is that the processor first reads the page tables,
> then translates the resulting GPA, and then finally does _another_ EPT
> translation for the page tables, this time with EPT_VIOLATION_ACC_READ=1
> and EPT_VIOLATION_ACC_WRITE=1:
>
>         qemu-kvm-3683  [042]  2364.196479: kvm_entry:            vcpu 0
>         qemu-kvm-3683  [042]  2364.196479: kvm_exit:             reason EPT_VIOLATION rip 0x40519e info 8b 0
>         qemu-kvm-3683  [042]  2364.196479: kvm_nested_vmexit:    rip: 0x000000000040519e reason: EPT_VIOLATION ext_inf1: 0x000000000000008b ext_inf2: 0x0000000000000000 ext_int: 0x00000000 ext_int_err: 0x00000000
>         qemu-kvm-3683  [042]  2364.196480: kvm_page_fault:       address 479000 error_code 8b
>         qemu-kvm-3683  [042]  2364.196480: kvm_mmu_pagetable_walk: addr 479000 pferr 7 P|W|U
>         qemu-kvm-3683  [042]  2364.196480: kvm_mmu_paging_element: pte 472007 level 4
>         qemu-kvm-3683  [042]  2364.196480: kvm_mmu_paging_element: pte 473007 level 3
>         qemu-kvm-3683  [042]  2364.196480: kvm_mmu_paging_element: pte 67c007 level 2
>         qemu-kvm-3683  [042]  2364.196480: kvm_mmu_paging_element: pte 479001 level 1
>
> (This trace comes from vanilla Linux 4.11.0).  This matches your kvm-unit-tests
> patch ("x86: fix ept_access_test_paddr exit qualifications").  Aargh.
>
> So Peter is right; the only way to get this behavior is to run with
> EPT A/D bits disabled when L1 disables them.  I'll wait for you guys to send
> a patch for this---we have plenty of time to include it in 4.12.0.  In the
> meanwhile Radim shall drop patch 2 from the KVM series.

Sounds good, we'll get you that patch. When does 4.12.0 close?

>
> Thanks,
>
> Paolo

Re: [PATCH kvm-unit-tests 2/2] vmx: fix expected results of new EPT tests

[Paolo Bonzini]

> On Fri, May 12, 2017 at 1:45 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> > On 11/05/2017 21:51, David Matlack wrote:
> >>> No, I've tried the tests on upstream Linux with eptad=0 (so that EPT A/D
> >>> is not used by KVM on the host) and they also hang with an infinite
> >>> stream
> >>> of EPT violations.
> >>
> >> I think the failures are caused by this code in handle_ept_violation,
> >> which clears the ACC_WRITE bit of the exit qualification before
> >> handling the fault, when EPT A/D is disabled:
> >>
> >> if (is_guest_mode(vcpu)
> >>    && !(exit_qualification & EPT_VIOLATION_GVA_TRANSLATED)) {
> >>         /*
> >>          * Fix up exit_qualification according to whether guest
> >>          * page table accesses are reads or writes.
> >>          */
> >>         u64 eptp = nested_ept_get_cr3(vcpu);
> >>         if (!(eptp & VMX_EPT_AD_ENABLE_BIT))
> >>                 exit_qualification &= ~EPT_VIOLATION_ACC_WRITE;
> >> }
> >
> > I thought so as well; that's why my hypervisor patch moved
> > trace_kvm_page_fault before munging exit_qualification.  Then I searched
> > for 0x82 or 0x8a in the traces, didn't find it, and got terribly confused.
> >
> > What actually happens doesn't match what the SDM says.  You correctly cite
> > this:
> >
> >> Per 28.2.3.2 EPT Violations: "Writes by the logical processor to guest
> >> paging structures to update accessed and dirty flags are considered to
> >> be data writes."
> >
> > though here you mean EPT_VIOLATION_GVA_TRANSLATED=0,
> > EPT_VIOLATION_ACC_WRITE=1:
> >
> >> In other words, it's valid for
> >> EPT_VIOLATION_GVA_TRANSLATED and EPT_VIOLATION_ACC_WRITE to both be
> >> set in the exit qual when EPT A/D is disabled.
> >
> > What really happens is that the processor first reads the page tables,
> > then translates the resulting GPA, and then finally does _another_ EPT
> > translation for the page tables, this time with EPT_VIOLATION_ACC_READ=1
> > and EPT_VIOLATION_ACC_WRITE=1:
> >
> >         qemu-kvm-3683  [042]  2364.196479: kvm_entry:            vcpu 0
> >         qemu-kvm-3683  [042]  2364.196479: kvm_exit:             reason
> >         EPT_VIOLATION rip 0x40519e info 8b 0
> >         qemu-kvm-3683  [042]  2364.196479: kvm_nested_vmexit:    rip:
> >         0x000000000040519e reason: EPT_VIOLATION ext_inf1:
> >         0x000000000000008b ext_inf2: 0x0000000000000000 ext_int:
> >         0x00000000 ext_int_err: 0x00000000
> >         qemu-kvm-3683  [042]  2364.196480: kvm_page_fault:       address
> >         479000 error_code 8b
> >         qemu-kvm-3683  [042]  2364.196480: kvm_mmu_pagetable_walk: addr
> >         479000 pferr 7 P|W|U
> >         qemu-kvm-3683  [042]  2364.196480: kvm_mmu_paging_element: pte
> >         472007 level 4
> >         qemu-kvm-3683  [042]  2364.196480: kvm_mmu_paging_element: pte
> >         473007 level 3
> >         qemu-kvm-3683  [042]  2364.196480: kvm_mmu_paging_element: pte
> >         67c007 level 2
> >         qemu-kvm-3683  [042]  2364.196480: kvm_mmu_paging_element: pte
> >         479001 level 1
> >
> > (This trace comes from vanilla Linux 4.11.0).  This matches your
> > kvm-unit-tests
> > patch ("x86: fix ept_access_test_paddr exit qualifications").  Aargh.
> >
> > So Peter is right; the only way to get this behavior is to run with
> > EPT A/D bits disabled when L1 disables them.  I'll wait for you guys to
> > send
> > a patch for this---we have plenty of time to include it in 4.12.0.  In the
> > meanwhile Radim shall drop patch 2 from the KVM series.
> 
> Sounds good, we'll get you that patch. When does 4.12.0 close?

Sending it within the next 3-4 weeks should be fine, even if it needs a
couple iterations to get everything in shape.

Paolo