Sorry, attached wrong patch in previous email, my original purpose is to flush the mapping
for the execution function.
On Tue, Apr 13, 2021 at 11:13:51AM +0800, Yang Weijiang wrote:
> On Mon, Apr 12, 2021 at 06:30:58PM +0000, Sean Christopherson wrote:
> > On Mon, Apr 12, 2021, Yang Weijiang wrote:
> > > On Fri, Apr 09, 2021 at 06:59:45PM +0000, Sean Christopherson wrote:
> > > > On Fri, Apr 09, 2021, Paolo Bonzini wrote:
> > > > > On 09/04/21 09:55, Yang Weijiang wrote:
> > > > > > During kvm-unit-test, below failure pattern is observed, this is due to testing thread
> > > > > > migration + cache "lazy" flush during test, so forcely flush the cache to avoid the issue.
> > > > > > Pin the test app to certain physical CPU can fix the issue as well. The error report is
> > > > > > misleading, pke is the victim of the issue.
> > > > > >
> > > > > > test user cr4.pke: FAIL: error code 5 expected 4
> > > > > > Dump mapping: address: 0x123400000000
> > > > > > ------L4: 21ea007
> > > > > > ------L3: 21eb007
> > > > > > ------L2: 21ec000
> > > > > > ------L1: 2000000
> > > > > >
> > > > > > Signed-off-by: Yang Weijiang <weijiang.yang@xxxxxxxxx>
> > > > > > ---
> > > > > > x86/access.c | 2 ++
> > > > > > 1 file changed, 2 insertions(+)
> > > > > >
> > > > > > diff --git a/x86/access.c b/x86/access.c
> > > > > > index 7dc9eb6..379d533 100644
> > > > > > --- a/x86/access.c
> > > > > > +++ b/x86/access.c
> > > > > > @@ -211,6 +211,8 @@ static unsigned set_cr4_smep(int smep)
> > > > > > ptl2[2] |= PT_USER_MASK;
> > > > > > if (!r)
> > > > > > shadow_cr4 = cr4;
> > > > > > +
> > > > > > + invlpg((void *)(ptl2[2] & ~PAGE_SIZE));
> > > > > > return r;
> > > > > > }
> > > > > >
> > > > >
> > > > > Applied, thanks.
> > > >
> > > > Egad, I can't keep up with this new Paolo :-D
> > > >
> > > >
> > > > Would it also work to move the existing invlpg() into ac_test_do_access()?
> > > >
> > > Hi, Sean,
> > > You patch works for the app on my side, but one thing makes my confused, my patch
> > > invalidates the mapping for test code(ac_test_do_access), but your patch invlidates
> > > at->virt, they're not mapped to the same page. Why it works?
> >
> > I don't know why your patch works. Best guess is that INVLPG on the PMD is
> > causing the CPU to flush the entire TLB, i.e. the problematic entry is collateral
> > damage.
> Hi, Sean,
> Maybe you've figured out the root cause, just attached the patch to let you know what I did
> for comparison, toggling two different invlpg() got significant different results.
>
> >
> > > I simplified the test by only executing two patterns as below:
> > >
> > > printf("\n############# start test ############\n\n");
> > > at.flags = 0x8000000;
> > > ac_test_exec(&at, &pool);
> > > at.flags = 0x200000; /* or 0x10200000 */
> > > ac_test_exec(&at, &pool);
> > > printf("############# end test ############\n\n");
> > >
> > > with your patch I still got error code 5 while getting error code 4 with my patch.
> > > What makes it different?
> >
> > Now I'm really confused. This:
> >
> > at.flags = 0x8000000;
> > ac_test_exec(&at, &pool);
> >
> > runs the test with a not-present PTE. I don't understand how you are getting
> > error code '5' (USER + PRESENT) when running the user test; there shouldn't be
> > anything for at->virt in the TLB.
> error code '5' is (USER + PG protection violation) :-)
>
> >
> > Are there tests being run before this point?
> What I changed is included in the patch.
>
> > Even then, explicitly flushing
> > at->virt should work. The fact that set_cr4_smep() modifies a PMD and not the
> > leaf PTE should be irrelevant. Per the SDM:
> >
> > INVLPG also invalidates all entries in all paging-structure caches associated
> > with the current PCID, regardless of the linear addresses to which they correspond.
> Even PCID is not enabled in CR4? If it's the case, it cannot tell why two
> different flavor of invalidation makes different results.
>
> Thanks for the comments!
> diff --git a/x86/access.c b/x86/access.c
> index 7dc9eb6..c7bb009 100644
> --- a/x86/access.c
> +++ b/x86/access.c
> @@ -638,6 +638,7 @@ static int ac_test_do_access(ac_test_t *at)
> unsigned long rsp;
> _Bool success = true;
> int flags = at->flags;
> + extern u64 ptl2[];
>
> ++unique;
> if (!(unique & 65535)) {
> @@ -670,6 +671,8 @@ static int ac_test_do_access(ac_test_t *at)
> fault = 0;
> }
>
> + //invlpg((void *)(ptl2[2] & ~PAGE_SIZE));
> + invlpg(at->virt);
> asm volatile ("mov $fixed1, %%rsi \n\t"
> "mov %%rsp, %%rdx \n\t"
> "cmp $0, %[user] \n\t"
> @@ -740,6 +743,8 @@ static int ac_test_do_access(ac_test_t *at)
> printf("PASS\n");
> }
> }
> + printf("cr0 = 0x%lx, cr3 = 0x%lx, cr4 = 0x%lx\n", read_cr0(), read_cr3(), read_cr4());
> + printf("at->flags = 0x%x, error = 0x%x, fault = 0x%x\n\n", at->flags, e, fault);
> return success;
> }
>
> @@ -955,7 +960,7 @@ static int ac_test_run(void)
> {
> ac_test_t at;
> ac_pool_t pool;
> - int i, tests, successes;
> + int tests, successes;
>
> printf("run\n");
> tests = successes = 0;
> @@ -1018,7 +1023,17 @@ static int ac_test_run(void)
>
> ac_env_int(&pool);
> ac_test_init(&at, (void *)(0x123400000000 + 16 * smp_id()));
> - do {
> +//////////////////////////////////////////////////////////////
> + printf("\n############# start test ############\n\n");
> + at.flags = 0x8000000 - 1;
> + ac_test_bump(&at);
> + ac_test_exec(&at, &pool);
> + at.flags = 0x200000 - 1;
> + ac_test_bump(&at);
> + ac_test_exec(&at, &pool);
> + printf("\n############# end test ############\n\n");
> +//////////////////////////////////////////////////////////////
> + /*do {
> ++tests;
> successes += ac_test_exec(&at, &pool);
> } while (ac_test_bump(&at));
> @@ -1026,7 +1041,7 @@ static int ac_test_run(void)
> for (i = 0; i < ARRAY_SIZE(ac_test_cases); i++) {
> ++tests;
> successes += ac_test_cases[i](&pool);
> - }
> + }*/
>
> printf("\n%d tests, %d failures\n", tests, tests - successes);
>
diff --git a/x86/access.c b/x86/access.c
index 7dc9eb6..87985fe 100644
--- a/x86/access.c
+++ b/x86/access.c
@@ -670,6 +670,8 @@ static int ac_test_do_access(ac_test_t *at)
fault = 0;
}
+ invlpg(ac_test_do_access);
+ //invlpg(at->virt);
asm volatile ("mov $fixed1, %%rsi \n\t"
"mov %%rsp, %%rdx \n\t"
"cmp $0, %[user] \n\t"
@@ -740,6 +742,8 @@ static int ac_test_do_access(ac_test_t *at)
printf("PASS\n");
}
}
+ printf("cr0 = 0x%lx, cr3 = 0x%lx, cr4 = 0x%lx\n", read_cr0(), read_cr3(), read_cr4());
+ printf("ac_test_do_access = %p, at->flags = 0x%x, error = 0x%x, fault = 0x%x\n\n", ac_test_do_access, at->flags, e, fault);
return success;
}
@@ -955,7 +959,7 @@ static int ac_test_run(void)
{
ac_test_t at;
ac_pool_t pool;
- int i, tests, successes;
+ int tests, successes;
printf("run\n");
tests = successes = 0;
@@ -1018,7 +1022,17 @@ static int ac_test_run(void)
ac_env_int(&pool);
ac_test_init(&at, (void *)(0x123400000000 + 16 * smp_id()));
- do {
+//////////////////////////////////////////////////////////////
+ printf("\n############# start test ############\n\n");
+ at.flags = 0x8000000 - 1;
+ ac_test_bump(&at);
+ ac_test_exec(&at, &pool);
+ at.flags = 0x200000 - 1;
+ ac_test_bump(&at);
+ ac_test_exec(&at, &pool);
+ printf("\n############# end test ############\n\n");
+//////////////////////////////////////////////////////////////
+ /*do {
++tests;
successes += ac_test_exec(&at, &pool);
} while (ac_test_bump(&at));
@@ -1026,7 +1040,7 @@ static int ac_test_run(void)
for (i = 0; i < ARRAY_SIZE(ac_test_cases); i++) {
++tests;
successes += ac_test_cases[i](&pool);
- }
+ }*/
printf("\n%d tests, %d failures\n", tests, tests - successes);
