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? At least
"HV_STATUS_INVALID_CONNECTION_ID" is wrong, as this is not the
connection id.
>
>> 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.[...]"
This makes then sense indeed!
>
>>> + 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?
>
> Thanks,
> Roman.
>
--
Thanks,
David / dhildenb
