On Fri, Mar 17, 2023, Anish Moorthy wrote:
> On Thu, Mar 16, 2023 at 5:02 PM Isaku Yamahata <isaku.yamahata@xxxxxxxxx> wrote:
> > > +inline int kvm_memfault_exit_or_efault(
> > > + struct kvm_vcpu *vcpu, uint64_t gpa, uint64_t len, uint64_t exit_flags)
> > > +{
> > > + if (!(vcpu->kvm->memfault_exit_reasons & exit_flags))
> > > + return -EFAULT;
> > > + vcpu->run->exit_reason = KVM_EXIT_MEMORY_FAULT;
> > > + vcpu->run->memory_fault.gpa = gpa;
> > > + vcpu->run->memory_fault.len = len;
> > > + vcpu->run->memory_fault.flags = exit_flags;
> > > + return -1;
> >
> > Why -1? 0? Anyway enum exit_fastpath_completion is x86 kvm mmu internal
> > convention. As WIP, it's okay for now, though.
>
> The -1 isn't to indicate a failure in this function itself, but to
> allow callers to substitute this for "return -EFAULT." A return code
> of zero would mask errors and cause KVM to proceed in ways that it
> shouldn't. For instance, "setup_vmgexit_scratch" uses it like this
>
> if (kvm_read_guest(svm->vcpu.kvm, scratch_gpa_beg, scratch_va, len)) {
> ...
> - return -EFAULT;
> + return kvm_memfault_exit_or_efault(...);
> }
>
> and looking at one of its callers (sev_handle_vmgexit) shows how a
> return code of zero would cause a different control flow
>
> case SVM_VMGEXIT_MMIO_READ:
> ret = setup_vmgexit_scratch(svm, true, control->exit_info_2);
> if (ret)
> break;
>
> ret = ret = kvm_sev_es_mmio_read(vcpu,
Hmm, I generally agree with Isaku, the helper should really return 0. Returning
-1 might work, but it'll likely confuse userspace, and will definitely confuse
KVM developers.
The "0 means exit to userspace" behavior is definitely a pain though, and is likely
going to make this all extremely fragile.
I wonder if we can get away with returning -EFAULT, but still filling vcpu->run
with KVM_EXIT_MEMORY_FAULT and all the other metadata. That would likely simplify
the implementation greatly, and would let KVM fill vcpu->run unconditonally. KVM
would still need a capability to advertise support to userspace, but userspace
wouldn't need to opt in. I think this may have been my very original though, and
I just never actually wrote it down...
