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

[Anish Moorthy <amoorthy@google.com>]

On Thu, Mar 16, 2023 at 5:02 PM Isaku Yamahata <isaku.yamahata@gmail.com> wrote:

> > +7.34 KVM_CAP_X86_MEMORY_FAULT_EXIT
> > +----------------------------------
> > +
> > +:Architectures: x86
>
> Why x86 specific?

Sean was the only one to bring this functionality up and originally
did so in the context of some x86-specific functions, so I assumed
that x86 was the only ask and that maybe the other architectures had
alternative solutions. Admittedly I also wanted to avoid wading
through another big set of -EFAULT references :/

Those are the only reasons though. Marc, Oliver, should I bring this
capability to Arm as well?

> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > index e38ddda05b261..00aec43860ff1 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->memfault_exit_reasons = 0;
> >
> >       INIT_LIST_HEAD(&kvm->gpc_list);
> >       spin_lock_init(&kvm->gpc_lock);
> > @@ -4671,6 +4672,14 @@ static int kvm_vm_ioctl_enable_cap_generic(struct kvm *kvm,
> >
> >               return r;
> >       }
> > +     case KVM_CAP_X86_MEMORY_FAULT_EXIT: {
> > +             if (!kvm_vm_ioctl_check_extension(kvm, KVM_CAP_X86_MEMORY_FAULT_EXIT))
> > +                     return -EINVAL;
> > +             else if (!kvm_memfault_exit_flags_valid(cap->args[0]))
> > +                     return -EINVAL;
> > +             kvm->memfault_exit_reasons = cap->args[0];
> > +             return 0;
> > +     }
>
> Is KVM_CAP_X86_MEMORY_FAULT_EXIT really specific to x86?
> If so, this should go to kvm_vm_ioctl_enable_cap() in arch/x86/kvm/x86.c.
> (Or make it non-arch specific.)

Ah, thanks for the catch: I renamed my old non-x86 specific
capability, and forgot to move this block.

> > +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,