On Fri, Nov 01, 2024, Binbin Wu wrote:
> On 10/31/2024 10:54 PM, Sean Christopherson wrote:
> > My other idea was have an out-param to separate the return code intended for KVM
> > from the return code intended for the guest. I generally dislike out-params, but
> > trying to juggle a return value that multiplexes guest and host values seems like
> > an even worse idea.
> >
> > Also completely untested...
...
> > case KVM_HC_MAP_GPA_RANGE: {
> > u64 gpa = a0, npages = a1, attrs = a2;
> > - ret = -KVM_ENOSYS;
> > + *ret = -KVM_ENOSYS;
> > if (!user_exit_on_hypercall(vcpu->kvm, KVM_HC_MAP_GPA_RANGE))
> > break;
> > if (!PAGE_ALIGNED(gpa) || !npages ||
> > gpa_to_gfn(gpa) + npages <= gpa_to_gfn(gpa)) {
> > - ret = -KVM_EINVAL;
> > + *ret = -KVM_EINVAL;
> > break;
> > }
>
> *ret needs to be set to 0 for this case before returning 0 to caller?
No, because the caller should consume *ret if and only if the function return value
is '1', i.e. iff KVM should resume the guest. And I think we actually want to
intentionally not touch *ret, because a sufficient smart compiler (or static
analysis tool) should be able to detect that incorrect usage of *ret is consuming
uninitialized data.
> > @@ -10080,13 +10078,13 @@ unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
> > return 0;
> > }
> > default:
> > - ret = -KVM_ENOSYS;
> > + *ret = -KVM_ENOSYS;
> > break;
> > }
> > out:
> > ++vcpu->stat.hypercalls;
> > - return ret;
> > + return 1;
> > }
> > EXPORT_SYMBOL_GPL(__kvm_emulate_hypercall);
> > @@ -10094,7 +10092,7 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
> > {
> > unsigned long nr, a0, a1, a2, a3, ret;
> > int op_64_bit;
> > - int cpl;
> > + int cpl, r;
> > if (kvm_xen_hypercall_enabled(vcpu->kvm))
> > return kvm_xen_hypercall(vcpu);
> > @@ -10110,10 +10108,9 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
> > op_64_bit = is_64_bit_hypercall(vcpu);
> > cpl = kvm_x86_call(get_cpl)(vcpu);
> > - ret = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl);
> > - if (nr == KVM_HC_MAP_GPA_RANGE && !ret)
> > - /* MAP_GPA tosses the request to the user space. */
> > - return 0;
> > + r = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl, &ret);
> > + if (r <= r)
> A typo here.
> I guess it meant to be "if (r <= ret)" ?
No, "if (r <= 0)", i.e. exit to userspace on 0 or -errno.
> So the combinations will be
> ----------------------------------------------------------------------------
> | r | ret | r <= ret |
> ---|-----|-----------|----------|-------------------------------------------
> 1 | 0 | 0 | true | return r, which is 0, exit to userspace
> ---|-----|-----------|----------|-------------------------------------------
> 2 | 1 | 0 | false | set vcpu's RAX and return back to guest
> ---|-----|-----------|----------|-------------------------------------------
> 3 | 1 | -KVM_Exxx | false | set vcpu's RAX and return back to guest
> ---|-----|-----------|----------|-------------------------------------------
> 4 | 1 | Positive | true | return r, which is 1,
> | | N | | back to guest without setting vcpu's RAX
> ----------------------------------------------------------------------------
>
> KVM_HC_SEND_IPI, which calls kvm_pv_send_ipi() can hit case 4, which will
> return back to guest without setting RAX. It is different from the current behavior.
>
> r can be 0 only if there is no other error detected during pre-checks.
> I think it can just check whether r is 0 or not.
Yeah, I just fat fingered the code (and didn't even compile test).
