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.
> 2. KVM_HYPERV_CONN_ID_MASK. "ConnectionId (4 bytes)".
>
> Why should it only be 3bytes?
>
> (I am pretty sure I am missing something in the document here :) )
"11.10.7 Connections
Connections are identified by 32-bit IDs. The high 8 bits are reserved
and must be 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.
Thanks,
Roman.
