On Wed, Feb 26, 2025 at 11:30:15AM -0800, Sean Christopherson wrote:
> On Wed, Feb 26, 2025, Yan Zhao wrote:
> > On Tue, Feb 25, 2025 at 05:48:39PM -0800, Sean Christopherson wrote:
> > > On Sat, Feb 08, 2025, Yan Zhao wrote:
> > > > In the read-only mprotect() phase of mmu_stress_test, ensure that
> > > > mprotect(PROT_READ) has completed before the guest starts writing to the
> > > > read-only mprotect() memory.
> > > >
> > > > Without waiting for mprotect_ro_done before the guest starts writing in
> > > > stage 3 (the stage for read-only mprotect()), the host's assertion of stage
> > > > 3 could fail if mprotect_ro_done is set to true in the window between the
> > > > guest finishing writes to all GPAs and executing GUEST_SYNC(3).
> > > >
> > > > This scenario is easy to occur especially when there are hundred of vCPUs.
> > > >
> > > > CPU 0 CPU 1 guest CPU 1 host
> > > > enter stage 3's 1st loop
> > > > //in stage 3
> > > > write all GPAs
> > > > @rip 0x4025f0
> > > >
> > > > mprotect(PROT_READ)
> > > > mprotect_ro_done=true
> > > > GUEST_SYNC(3)
> > > > r=0, continue stage 3's 1st loop
> > > >
> > > > //in stage 4
> > > > write GPA
> > > > @rip 0x402635
> > > >
> > > > -EFAULT, jump out stage 3's 1st loop
> > > > enter stage 3's 2nd loop
> > > > write GPA
> > > > @rip 0x402635
> > > > -EFAULT, continue stage 3's 2nd loop
> > > > guest rip += 3
> > > >
> > > > The test then fails and reports "Unhandled exception '0xe' at guest RIP
> > > > '0x402638'", since the next valid guest rip address is 0x402639, i.e. the
> > > > "(mem) = val" in vcpu_arch_put_guest() is compiled into a mov instruction
> > > > of length 4.
> > >
> > > This shouldn't happen. On x86, stage 3 is a hand-coded "mov %rax, (%rax)", not
> > > vcpu_arch_put_guest(). Either something else is going on, or __x86_64__ isn't
> > > defined?
> > stage 3 is hand-coded "mov %rax, (%rax)", but stage 4 is with
> > vcpu_arch_put_guest().
> >
> > The original code expects that "mov %rax, (%rax)" in stage 3 can produce
> > -EFAULT, so that in the host thread can jump out of stage 3's 1st vcpu_run()
> > loop.
>
> Ugh, I forgot that there are two loops in stage-3. I tried to prevent this race,
> but violated my own rule of not using arbitrary delays to avoid races.
>
> Completely untested, but I think this should address the problem (I'll test
> later today; you already did the hard work of debugging). The only thing I'm
> not positive is correct is making the first _vcpu_run() a one-off instead of a
> loop.
Right, making the first _vcpu_run() a one-off could produce below error:
"Expected EFAULT on write to RO memory, got r = 0, errno = 4".
>
> diff --git a/tools/testing/selftests/kvm/mmu_stress_test.c b/tools/testing/selftests/kvm/mmu_stress_test.c
> index d9c76b4c0d88..9ac1800bb770 100644
> --- a/tools/testing/selftests/kvm/mmu_stress_test.c
> +++ b/tools/testing/selftests/kvm/mmu_stress_test.c
> @@ -18,6 +18,7 @@
> #include "ucall_common.h"
>
> static bool mprotect_ro_done;
> +static bool vcpu_hit_ro_fault;
>
> static void guest_code(uint64_t start_gpa, uint64_t end_gpa, uint64_t stride)
> {
> @@ -36,9 +37,9 @@ static void guest_code(uint64_t start_gpa, uint64_t end_gpa, uint64_t stride)
>
> /*
> * Write to the region while mprotect(PROT_READ) is underway. Keep
> - * looping until the memory is guaranteed to be read-only, otherwise
> - * vCPUs may complete their writes and advance to the next stage
> - * prematurely.
> + * looping until the memory is guaranteed to be read-only and a fault
> + * has occured, otherwise vCPUs may complete their writes and advance
> + * to the next stage prematurely.
> *
> * For architectures that support skipping the faulting instruction,
> * generate the store via inline assembly to ensure the exact length
> @@ -56,7 +57,7 @@ static void guest_code(uint64_t start_gpa, uint64_t end_gpa, uint64_t stride)
> #else
> vcpu_arch_put_guest(*((volatile uint64_t *)gpa), gpa);
> #endif
> - } while (!READ_ONCE(mprotect_ro_done));
> + } while (!READ_ONCE(mprotect_ro_done) && !READ_ONCE(vcpu_hit_ro_fault));
>
> /*
> * Only architectures that write the entire range can explicitly sync,
> @@ -148,12 +149,13 @@ static void *vcpu_worker(void *data)
> * be stuck on the faulting instruction for other architectures. Go to
> * stage 3 without a rendezvous
> */
> - do {
> - r = _vcpu_run(vcpu);
> - } while (!r);
> + r = _vcpu_run(vcpu);
> TEST_ASSERT(r == -1 && errno == EFAULT,
> "Expected EFAULT on write to RO memory, got r = %d, errno = %d", r, errno);
>
> + /* Tell the vCPU it hit a RO fault. */
> + WRITE_ONCE(vcpu_hit_ro_fault, true);
> +
> #if defined(__x86_64__) || defined(__aarch64__)
> /*
> * Verify *all* writes from the guest hit EFAULT due to the VMA now
> @@ -378,7 +380,6 @@ int main(int argc, char *argv[])
> rendezvous_with_vcpus(&time_run2, "run 2");
>
> mprotect(mem, slot_size, PROT_READ);
> - usleep(10);
> mprotect_ro_done = true;
> sync_global_to_guest(vm, mprotect_ro_done);
>
>
