Re: [PATCH] [kvm-unit-tests PATCH] x86/access: Fix intermittent test failure

[Sean Christopherson <seanjc@google.com>]

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@intel.com>
> > > > ---
> > > >   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.

> 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.

Are there tests being run before this point?  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.