On Thu, May 11, 2017 at 4:23 AM, Paolo Bonzini <pbonzini@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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
>
