On Thu, Feb 23, 2023, Anish Moorthy wrote: > On Thu, Feb 23, 2023 at 12:55 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote: > > > (3) switched over to a memslot flag: KVM_CAP_MEMORY_FAULT_EXIT simply > > > indicates whether this flag is supported. > > > > The new memslot flag should depend on KVM_CAP_MEMORY_FAULT_EXIT, but > > KVM_CAP_MEMORY_FAULT_EXIT should be a standalone thing, i.e. should convert "all" > > guest-memory -EFAULTS to KVM_CAP_MEMORY_FAULT_EXIT. All in quotes because I would > > likely be ok with a partial conversion for the initial implementation if there > > are paths that would require an absurd amount of work to convert. > > I'm actually not sure where all of the sources of guest-memory -EFAULTs are > or how I'd go about finding them. Heh, if anyone can says they can list _all_ sources of KVM accesses to guest memory of the top of their head, they're lying. Finding the sources is a bit tedious, but shouldn't be too hard. The scope is is -EFAULT in the context of KVM_RUN that gets returned to userspace. There are 150+ references to -EFAULT to wade through, but the vast majority are obviously not in scope, e.g. are for uaccess failures in other ioctls(). > Is the standalone behavior of KVM_CAP_MEMORY_FAULT_EXIT something you're > suggesting because that new name is too broad for what it does right now? No, I want a standalone thing because I want to start driving toward KVM never returning -EFAULT to host userspace for accesses to guest memory in the context of ioctl(KVM_RUN). E.g. I want to replace the "return -EFAULT" in kvm_handle_error_pfn() with something like "return kvm_handle_memory_fault_exit(...)". My hope/goal is that return useful information will allow userspace to do more interesting things with guest memory without needing invasive, one-off changes to KVM. At the very least, it should get us to the point where a memory fault from KVM_RUN exits to userspace with sane, helpful information, not a generic "-EFAULT, have fun debugging!". > If so then I'd rather just rename it again: but if that functionality should > be included with this series, then I'll take a look at the required work > given a couple of pointers :) > > I will say, a partial implementation seems worse than no > implementation: isn't there a risk that someone ends up depending on > the incomplete behavior? In this case, I don't think so. We definitely would need to document that KVM may still return -EFAULT in certain scenarios, but we have at least one known use case (this one) where catching the common cases is sufficient. And if/when use cases come along that need 100% accuracy, we can hunt down and fix the KVM remaining "bugs".
