From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marcelo Tosatti <mtosatti@redhat.com>
Subject: Re: [PATCH 4/8] KVM: Add a route layer to convert MSI message to
	GSI
Date: Tue, 23 Dec 2008 15:55:42 -0200
Message-ID: <20081223175542.GB5449@amt.cnet>
References: <1230019231-16543-1-git-send-email-sheng@linux.intel.com> <1230019231-16543-5-git-send-email-sheng@linux.intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: Avi Kivity <avi@redhat.com>, kvm@vger.kernel.org
To: Sheng Yang <sheng@linux.intel.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx2.redhat.com ([66.187.237.31]:58594 "EHLO mx2.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1751016AbYLWRzv (ORCPT <rfc822;kvm@vger.kernel.org>);
	Tue, 23 Dec 2008 12:55:51 -0500
Content-Disposition: inline
In-Reply-To: <1230019231-16543-5-git-send-email-sheng@linux.intel.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

On Tue, Dec 23, 2008 at 04:00:27PM +0800, Sheng Yang wrote:
> Avi's purpose, to use single kvm_set_irq() to deal with all interrupt, including
> MSI. So here is it.
> 
> struct gsi_msg is a mapping from a special gsi(with KVM_GSI_MSG_MASK) to
> MSI/MSI-X message address/data.
> 
> Now we support up to 256 gsi_msg mapping, and gsi_msg is allocated by kernel and
> provide two ioctls to userspace, which is more flexiable.
> 
> Signed-off-by: Sheng Yang <sheng@linux.intel.com>
> ---
>  include/linux/kvm.h      |   12 ++++++++
>  include/linux/kvm_host.h |   16 ++++++++++
>  virt/kvm/irq_comm.c      |   70 ++++++++++++++++++++++++++++++++++++++++++++++
>  virt/kvm/kvm_main.c      |   63 +++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 161 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/kvm.h b/include/linux/kvm.h
> index 5b965f6..b091a86 100644
> --- a/include/linux/kvm.h
> +++ b/include/linux/kvm.h
> @@ -394,6 +394,7 @@ struct kvm_trace_rec {
>  #define KVM_CAP_USER_NMI 22
>  #endif
>  #define KVM_CAP_SET_GUEST_DEBUG 23
> +#define KVM_CAP_GSI_MSG 24
>  
>  /*
>   * ioctls for VM fds
> @@ -427,6 +428,8 @@ struct kvm_trace_rec {
>  				   struct kvm_assigned_pci_dev)
>  #define KVM_ASSIGN_IRQ _IOR(KVMIO, 0x70, \
>  			    struct kvm_assigned_irq)
> +#define KVM_REQUEST_GSI_MSG _IOWR(KVMIO, 0x71, struct kvm_assigned_gsi_msg)
> +#define KVM_FREE_GSI_MSG _IOR(KVMIO, 0x72, __u32)

Wrap the __u32 into a struct. Could use kvm_assigned_gsi_msg itself.

> +++ b/include/linux/kvm_host.h
> @@ -132,6 +132,10 @@ struct kvm {
>  	unsigned long mmu_notifier_seq;
>  	long mmu_notifier_count;
>  #endif
> +	struct hlist_head gsi_msg_list;
> +	struct mutex gsi_msg_lock;
> +#define KVM_NR_GSI_MSG	    256
> +	DECLARE_BITMAP(gsi_msg_bitmap, KVM_NR_GSI_MSG);
>  };

This is platform specific data. Can't it live in kvm_arch?

>  }
> +
> +int kvm_update_gsi_msg(struct kvm *kvm, struct kvm_gsi_msg *gsi_msg)
> +{
> +	struct kvm_gsi_msg *found_msg, *new_gsi_msg;
> +	int r, gsi;
> +
> +	mutex_lock(&kvm->gsi_msg_lock);
> +	/* Find whether we need a update or a new entry */
> +	found_msg = kvm_find_gsi_msg(kvm, gsi_msg->gsi);
> +	if (found_msg)
> +		*found_msg = *gsi_msg;
> +	else {
> +		gsi = find_first_zero_bit(kvm->gsi_msg_bitmap, KVM_NR_GSI_MSG);
> +		if (gsi >= KVM_NR_GSI_MSG) {
> +			r = -EFAULT;

ENOSPC?

> +static int kvm_vm_ioctl_request_gsi_msg(struct kvm *kvm,
> +					struct kvm_assigned_gsi_msg *agsi_msg)
> +{
> +	struct kvm_gsi_msg gsi_msg;
> +	int r;
> +
> +	gsi_msg.gsi = agsi_msg->gsi;
> +	gsi_msg.msg.address_lo = agsi_msg->msg.addr_lo;
> +	gsi_msg.msg.address_hi = agsi_msg->msg.addr_hi;
> +	gsi_msg.msg.data = agsi_msg->msg.data;
> +
> +	r = kvm_update_gsi_msg(kvm, &gsi_msg);
> +	if (r == 0)
> +		agsi_msg->gsi = gsi_msg.gsi;
> +	return r;
> +}

Can't see the purpose of this function. Why preserve the user-passed GSI
value in case of failure? It will return an error anyway...