Re: [PATCH v5 2/2] kvm: x86: hyperv: guest->host event signaling via eventfd

[David Hildenbrand <david@xxxxxxxxxx>]

>  
> +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, &param, 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?

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 :) )


> +		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.


> +	if (conn_id & ~KVM_HYPERV_CONN_ID_MASK)
> +		return HV_STATUS_INVALID_CONNECTION_ID;
> +
> +	idx = srcu_read_lock(&vcpu->kvm->srcu);
> +	eventfd = idr_find(&vcpu->kvm->arch.hyperv.conn_to_evt, conn_id);
> +	if (eventfd)
> +		eventfd_signal(eventfd, 1);
> +	srcu_read_unlock(&vcpu->kvm->srcu, idx);
> +
> +	return eventfd ? HV_STATUS_SUCCESS : HV_STATUS_INVALID_CONNECTION_ID;
> +}
> +


-- 

Thanks,

David / dhildenb