On Wed, Dec 13, 2017 at 12:14:48PM +0100, David Hildenbrand wrote:
> On 13.12.2017 12:04, Roman Kagan wrote:
> > On Wed, Dec 13, 2017 at 10:51:06AM +0100, David Hildenbrand wrote:
> >>> +static u16 kvm_hvcall_signal_event(struct kvm_vcpu *vcpu, bool fast, u64 param)
> >>> +{
> >>> + u16 ret;
> >>> + u32 conn_id, flag_no;
> >>> + int idx;
> >>> + struct eventfd_ctx *eventfd;
> >>> +
> >>> + if (unlikely(!fast)) {
> >>> + gpa_t gpa = param;
> >>> +
> >>> + if ((gpa & (__alignof__(param) - 1)) ||
> >>> + offset_in_page(gpa) + sizeof(param) > PAGE_SIZE)
> >>> + return HV_STATUS_INVALID_ALIGNMENT;
> >>> +
> >>> + idx = srcu_read_lock(&vcpu->kvm->srcu);
> >>> + ret = kvm_vcpu_read_guest(vcpu, gpa, ¶m, sizeof(param));
> >>> + srcu_read_unlock(&vcpu->kvm->srcu, idx);
> >>> +
> >>> + if (ret < 0)
> >>> + return HV_STATUS_INSUFFICIENT_MEMORY;
> >>> + }
> >>> +
> >>> + /*
> >>> + * the signaled event number is made up of a 24bit "connection id" and
> >>> + * a 16bit "flag number"; on the hypervisor side it's only their sum
> >>> + * that matters
> >>> + */
> >>> + conn_id = param;
> >>> + flag_no = param >> 32;
> >>> + if ((conn_id & ~KVM_HYPERV_CONN_ID_MASK) || (flag_no & 0xffff0000))
> >>
> >> 1. You seem to check for RsvdZ here (flag_no & 0xffff0000),
> >> "HV_STATUS_INVALID_CONNECTION_ID: The specified connection ID is
> >> invalid."
> >> -> Shouldn't the reserved field be simply ignored?
> >
> > It's "reserved zero", so I think it's correct to reject non-zero values.
>
> If it's not documented, guess it is ignored?
It is (sort of) documented, per
"1.2 Reserved Values
[...]
Zero value (documented as RsvdZ in diagrams and ReservedZ in code
segments) – For maximum forward compatibility, clients should zero the
value within this field."
> At least "HV_STATUS_INVALID_CONNECTION_ID" is wrong, as this is not
> the connection id.
I (ab)used this return code here to fall through to userspace handling
of this hypercall. But it may indeed make more sense to return
HV_STATUS_INVALID_HYPERCALL_INPUT, as in "A reserved bit in the
specified hypercall input value is non-zero."
> >>> + return HV_STATUS_INVALID_CONNECTION_ID;
> >>> + conn_id += flag_no;
> >>
> >> "FlagNumber specifies the relative index of the event flag that the
> >> caller wants to set within the target"
> >>
> >> I am not sure if we should simply change the connection id here. This
> >> seems to be an additional parameter to be passed to the one connection id.
> >
> > Right. I think I misinterpreted this part back when I implemented it in
> > QEMU (first submission was this summer). We lived happily with that
> > code, because the FlagNuber was always zero, and then I copied that over
> > to this KVM patch.
> >
> > I think requiring it to be zero is a better choice; I've prepared v7 to
> > that end.
>
> But which return value to use? HV_STATUS_INVALID_PARAMETER?
I think we're better off resorting to the userspace for handling it (at
least to report the situation). With the current patch this will be
HV_STATUS_INVALID_CONNECTION_ID, but I can use something else or define
a special code for that if desired.
Roman.
