On Thu, Mar 16, 2023 at 5:02 PM Isaku Yamahata <isaku.yamahata@xxxxxxxxx> 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,
