| Message ID | 20240911204158.2034295-14-seanjc@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | KVM: selftests: Morph max_guest_mem to mmu_stress | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/vmtest-fixes-PR | fail | merge-conflict |
On Wed, Sep 11, 2024 at 1:42 PM Sean Christopherson <seanjc@google.com> wrote: > > Add two phases to mmu_stress_test to verify that KVM correctly handles > guest memory that was writable, and then made read-only in the primary MMU, > and then made writable again. > > Add bonus coverage for x86 and arm64 to verify that all of guest memory was > marked read-only. Making forward progress (without making memory writable) > requires arch specific code to skip over the faulting instruction, but the > test can at least verify each vCPU's starting page was made read-only for > other architectures. > > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > tools/testing/selftests/kvm/mmu_stress_test.c | 104 +++++++++++++++++- > 1 file changed, 101 insertions(+), 3 deletions(-) > > diff --git a/tools/testing/selftests/kvm/mmu_stress_test.c b/tools/testing/selftests/kvm/mmu_stress_test.c > index 50c3a17418c4..c07c15d7cc9a 100644 > --- a/tools/testing/selftests/kvm/mmu_stress_test.c > +++ b/tools/testing/selftests/kvm/mmu_stress_test.c > @@ -16,6 +16,8 @@ > #include "guest_modes.h" > #include "processor.h" > > +static bool mprotect_ro_done; > + > static void guest_code(uint64_t start_gpa, uint64_t end_gpa, uint64_t stride) > { > uint64_t gpa; > @@ -31,6 +33,42 @@ static void guest_code(uint64_t start_gpa, uint64_t end_gpa, uint64_t stride) > *((volatile uint64_t *)gpa); > GUEST_SYNC(2); > > + /* > + * 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. > + * > + * For architectures that support skipping the faulting instruction, > + * generate the store via inline assembly to ensure the exact length > + * of the instruction is known and stable (vcpu_arch_put_guest() on > + * fixed-length architectures should work, but the cost of paranoia > + * is low in this case). For x86, hand-code the exact opcode so that > + * there is no room for variability in the generated instruction. > + */ > + do { > + for (gpa = start_gpa; gpa < end_gpa; gpa += stride) > +#ifdef __x86_64__ > + asm volatile(".byte 0x48,0x89,0x00" :: "a"(gpa) : "memory"); /* mov %rax, (%rax) */ I'm curious what you think about using labels (in asm, but perhaps also in C) and *setting* the PC instead of incrementing the PC. Diff attached (tested on x86). It might even be safe/okay to always use vcpu_arch_put_guest(), just set the PC to a label immediately following it. I don't feel strongly, so feel free to ignore. > +#elif defined(__aarch64__) > + asm volatile("str %0, [%0]" :: "r" (gpa) : "memory"); > +#else > + vcpu_arch_put_guest(*((volatile uint64_t *)gpa), gpa); > +#endif > + } while (!READ_ONCE(mprotect_ro_done)); > + > + /* > + * Only architectures that write the entire range can explicitly sync, > + * as other architectures will be stuck on the write fault. > + */ > +#if defined(__x86_64__) || defined(__aarch64__) > + GUEST_SYNC(3); > +#endif
On Wed, Sep 11, 2024, James Houghton wrote: > On Wed, Sep 11, 2024 at 1:42 PM Sean Christopherson <seanjc@google.com> wrote: > > @@ -31,6 +33,42 @@ static void guest_code(uint64_t start_gpa, uint64_t end_gpa, uint64_t stride) > > *((volatile uint64_t *)gpa); > > GUEST_SYNC(2); > > > > + /* > > + * 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. > > + * > > + * For architectures that support skipping the faulting instruction, > > + * generate the store via inline assembly to ensure the exact length > > + * of the instruction is known and stable (vcpu_arch_put_guest() on > > + * fixed-length architectures should work, but the cost of paranoia > > + * is low in this case). For x86, hand-code the exact opcode so that > > + * there is no room for variability in the generated instruction. > > + */ > > + do { > > + for (gpa = start_gpa; gpa < end_gpa; gpa += stride) > > +#ifdef __x86_64__ > > + asm volatile(".byte 0x48,0x89,0x00" :: "a"(gpa) : "memory"); /* mov %rax, (%rax) */ > > I'm curious what you think about using labels (in asm, but perhaps > also in C) and *setting* the PC instead of incrementing the PC. I have nothing against asm labels, but generally speaking I don't like using _global_ labels to skip instructions. E.g. __KVM_ASM_SAFE() uses labels to compute the instruction size, but those labels are local and never directly used outside of the macro. The biggest problem with global labels is that they don't scale. E.g. if we extend this test in the future with another testcase that needs to skip a gpa, then we'll end up with skip_page1 and skip_page2, and the code starts to become even harder to follow. Don't get me wrong, skipping a fixed instruction size is awful too, but in my experience they are less painful to maintain over the long haul. > Diff attached (tested on x86). Nit, in the future, just copy+pase the diff for small things like this (and even for large diffs in many cases) so that readers don't need to open an attachment (depending on their mail client), and so that it's easier to comment on the proposed changed. `git am --scissors` (a.k.a. `git am -c`) can be used to essentially extract and apply such a diff from the mail. > It might even be safe/okay to always use vcpu_arch_put_guest(), just set the > PC to a label immediately following it. That would not be safe/feasible. Labels in C code are scoped to the function. And AFAIK, labels for use with goto are also not visible symbols, they are statements. The "standard" way to expose a label from a function is to use inline asm, at which point there are zero guarantees that nothing necessary is generated between vcpu_arch_put_guest() and the next asm() block. E.g. ignoring the inline asm for the moment, the compiler could generate multiple paths for a loop, e.g. an unrolled version for a small number of iterations, and an actual loop for a larger number of iterations. Trying to define a label as a singular symbol for that is nonsensical.
diff --git a/tools/testing/selftests/kvm/mmu_stress_test.c b/tools/testing/selftests/kvm/mmu_stress_test.c index 50c3a17418c4..c07c15d7cc9a 100644 --- a/tools/testing/selftests/kvm/mmu_stress_test.c +++ b/tools/testing/selftests/kvm/mmu_stress_test.c @@ -16,6 +16,8 @@ #include "guest_modes.h" #include "processor.h" +static bool mprotect_ro_done; + static void guest_code(uint64_t start_gpa, uint64_t end_gpa, uint64_t stride) { uint64_t gpa; @@ -31,6 +33,42 @@ static void guest_code(uint64_t start_gpa, uint64_t end_gpa, uint64_t stride) *((volatile uint64_t *)gpa); GUEST_SYNC(2); + /* + * 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. + * + * For architectures that support skipping the faulting instruction, + * generate the store via inline assembly to ensure the exact length + * of the instruction is known and stable (vcpu_arch_put_guest() on + * fixed-length architectures should work, but the cost of paranoia + * is low in this case). For x86, hand-code the exact opcode so that + * there is no room for variability in the generated instruction. + */ + do { + for (gpa = start_gpa; gpa < end_gpa; gpa += stride) +#ifdef __x86_64__ + asm volatile(".byte 0x48,0x89,0x00" :: "a"(gpa) : "memory"); /* mov %rax, (%rax) */ +#elif defined(__aarch64__) + asm volatile("str %0, [%0]" :: "r" (gpa) : "memory"); +#else + vcpu_arch_put_guest(*((volatile uint64_t *)gpa), gpa); +#endif + } while (!READ_ONCE(mprotect_ro_done)); + + /* + * Only architectures that write the entire range can explicitly sync, + * as other architectures will be stuck on the write fault. + */ +#if defined(__x86_64__) || defined(__aarch64__) + GUEST_SYNC(3); +#endif + + for (gpa = start_gpa; gpa < end_gpa; gpa += stride) + vcpu_arch_put_guest(*((volatile uint64_t *)gpa), gpa); + GUEST_SYNC(4); + GUEST_ASSERT(0); } @@ -78,6 +116,7 @@ static void *vcpu_worker(void *data) struct vcpu_info *info = data; struct kvm_vcpu *vcpu = info->vcpu; struct kvm_vm *vm = vcpu->vm; + int r; vcpu_args_set(vcpu, 3, info->start_gpa, info->end_gpa, vm->page_size); @@ -100,6 +139,57 @@ static void *vcpu_worker(void *data) /* Stage 2, read all of guest memory, which is now read-only. */ run_vcpu(vcpu, 2); + + /* + * Stage 3, write guest memory and verify KVM returns -EFAULT for once + * the mprotect(PROT_READ) lands. Only architectures that support + * validating *all* of guest memory sync for this stage, as vCPUs will + * be stuck on the faulting instruction for other architectures. Go to + * stage 3 without a rendezvous + */ + do { + r = _vcpu_run(vcpu); + } while (!r); + TEST_ASSERT(r == -1 && errno == EFAULT, + "Expected EFAULT on write to RO memory, got r = %d, errno = %d", r, errno); + +#if defined(__x86_64__) || defined(__aarch64__) + /* + * Verify *all* writes from the guest hit EFAULT due to the VMA now + * being read-only. x86 and arm64 only at this time as skipping the + * instruction that hits the EFAULT requires advancing the program + * counter, which is arch specific and relies on inline assembly. + */ +#ifdef __x86_64__ + vcpu->run->kvm_valid_regs = KVM_SYNC_X86_REGS; +#endif + for (;;) { + r = _vcpu_run(vcpu); + if (!r) + break; + TEST_ASSERT_EQ(errno, EFAULT); +#if defined(__x86_64__) + WRITE_ONCE(vcpu->run->kvm_dirty_regs, KVM_SYNC_X86_REGS); + vcpu->run->s.regs.regs.rip += 3; +#elif defined(__aarch64__) + vcpu_set_reg(vcpu, ARM64_CORE_REG(regs.pc), + vcpu_get_reg(vcpu, ARM64_CORE_REG(regs.pc)) + 4); +#endif + + } + assert_sync_stage(vcpu, 3); +#endif /* __x86_64__ || __aarch64__ */ + rendezvous_with_boss(); + + /* + * Stage 4. Run to completion, waiting for mprotect(PROT_WRITE) to + * make the memory writable again. + */ + do { + r = _vcpu_run(vcpu); + } while (r && errno == EFAULT); + TEST_ASSERT_EQ(r, 0); + assert_sync_stage(vcpu, 4); rendezvous_with_boss(); return NULL; @@ -182,7 +272,7 @@ int main(int argc, char *argv[]) const uint64_t start_gpa = SZ_4G; const int first_slot = 1; - struct timespec time_start, time_run1, time_reset, time_run2, time_ro; + struct timespec time_start, time_run1, time_reset, time_run2, time_ro, time_rw; uint64_t max_gpa, gpa, slot_size, max_mem, i; int max_slots, slot, opt, fd; bool hugepages = false; @@ -287,19 +377,27 @@ 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); + rendezvous_with_vcpus(&time_ro, "mprotect RO"); + mprotect(mem, slot_size, PROT_READ | PROT_WRITE); + rendezvous_with_vcpus(&time_rw, "mprotect RW"); + time_rw = timespec_sub(time_rw, time_ro); time_ro = timespec_sub(time_ro, time_run2); time_run2 = timespec_sub(time_run2, time_reset); time_reset = timespec_sub(time_reset, time_run1); time_run1 = timespec_sub(time_run1, time_start); pr_info("run1 = %ld.%.9lds, reset = %ld.%.9lds, run2 = %ld.%.9lds, " - "ro = %ld.%.9lds\n", + "ro = %ld.%.9lds, rw = %ld.%.9lds\n", time_run1.tv_sec, time_run1.tv_nsec, time_reset.tv_sec, time_reset.tv_nsec, time_run2.tv_sec, time_run2.tv_nsec, - time_ro.tv_sec, time_ro.tv_nsec); + time_ro.tv_sec, time_ro.tv_nsec, + time_rw.tv_sec, time_rw.tv_nsec); /* * Delete even numbered slots (arbitrary) and unmap the first half of
Add two phases to mmu_stress_test to verify that KVM correctly handles guest memory that was writable, and then made read-only in the primary MMU, and then made writable again. Add bonus coverage for x86 and arm64 to verify that all of guest memory was marked read-only. Making forward progress (without making memory writable) requires arch specific code to skip over the faulting instruction, but the test can at least verify each vCPU's starting page was made read-only for other architectures. Signed-off-by: Sean Christopherson <seanjc@google.com> --- tools/testing/selftests/kvm/mmu_stress_test.c | 104 +++++++++++++++++- 1 file changed, 101 insertions(+), 3 deletions(-)