On Thu, Jun 1, 2023 at 12:52 PM Oliver Upton <oliver.upton@xxxxxxxxx> wrote: > > So the other angle of my concern w.r.t. NOWAIT exits is the fact that > userspace gets to decide whether or not we annotate such an exit. We all > agree that a NOWAIT exit w/o context isn't actionable, right? Yup > Sean is suggesting that we abuse the fact that kvm_run already contains > junk for EFAULT exits and populate kvm_run::memory_fault unconditionally > [*]. I agree with him, and it eliminates the odd quirk of 'bare' NOWAIT > exits too. Old userspace will still see 'garbage' in kvm_run struct, > but one man's trash is another man's treasure after all :) > > So, based on that, could you: > > - Unconditionally prepare MEMORY_FAULT exits everywhere you're > converting here > > - Redefine KVM_CAP_MEMORY_FAULT_INFO as an informational cap, and do > not accept an attempt to enable it. Instead, have calls to > KVM_CHECK_EXTENSION return a set of flags describing the supported > feature set. Sure. I've been collecting feedback as it comes in, so I can send up a v4 with everything up to now soon. The major thing left to resolve is that the exact set of annotations is still waiting on feedback: I've already gone ahead and dropped everything I wasn't sure of in [1], so the next version will be quite a bit smaller. If it turns out that I've dropped too much, then I can add things back in based on the feedback. [1] https://lore.kernel.org/kvm/20230412213510.1220557-1-amoorthy@xxxxxxxxxx/T/#mfe28e6a5015b7cd8c5ea1c351b0ca194aeb33daf > Eventually, you can stuff a bit in there to advertise that all > EFAULTs are reliable. I don't think this is an objective: the idea is to annotate efaults tracing back to user accesses (see [2]). Although the idea of annotating with some "unrecoverable" flag set for other efaults has been tossed around, so we may end up with that. [2] https://lore.kernel.org/kvm/20230412213510.1220557-1-amoorthy@xxxxxxxxxx/T/#m5715f3a14a6a9ff9a4188918ec105592f0bfc69a > [*] https://lore.kernel.org/kvmarm/ZHjqkdEOVUiazj5d@xxxxxxxxxx/ > > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > > index cf7d3de6f3689..f3effc93cbef3 100644 > > --- a/virt/kvm/kvm_main.c > > +++ b/virt/kvm/kvm_main.c > > @@ -1142,6 +1142,7 @@ static struct kvm *kvm_create_vm(unsigned long type, const char *fdname) > > spin_lock_init(&kvm->mn_invalidate_lock); > > rcuwait_init(&kvm->mn_memslots_update_rcuwait); > > xa_init(&kvm->vcpu_array); > > + kvm->fill_efault_info = false; > > > > INIT_LIST_HEAD(&kvm->gpc_list); > > spin_lock_init(&kvm->gpc_lock); > > @@ -4096,6 +4097,8 @@ static long kvm_vcpu_ioctl(struct file *filp, > > put_pid(oldpid); > > } > > r = kvm_arch_vcpu_ioctl_run(vcpu); > > + WARN_ON_ONCE(r == -EFAULT && > > + vcpu->run->exit_reason != KVM_EXIT_MEMORY_FAULT); > > This might be a bit overkill, as it will definitely fire on unsupported > architectures. Instead you may want to condition this on an architecture > actually selecting support for MEMORY_FAULT_INFO. Ah, that's embarrassing. Thanks for the catch.
