Re: [WIP Patch v2 04/14] KVM: x86: Add KVM_CAP_X86_MEMORY_FAULT_EXIT and associated kvm_run field

[Sean Christopherson <seanjc@google.com>]

On Fri, Mar 17, 2023, Anish Moorthy wrote:
> On Thu, Mar 16, 2023 at 5:02 PM Isaku Yamahata <isaku.yamahata@gmail.com> 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...