Re: [PATCH 4/8] KVM: Add a route layer to convert MSI message to GSI

[Sheng Yang <sheng@linux.intel.com>]

On Wednesday 24 December 2008 10:31:00 Sheng Yang wrote:
> On Wednesday 24 December 2008 01:55:42 Marcelo Tosatti wrote:
> > 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.
>
> OK.
>
> > > +++ 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?
>
> No... It's not platform specific data, at least, we want it to use with
> x86, IA64 and virtio, that's why I make it so generic...
>
> > >  }
> > > +
> > > +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?
>
> OK. (Though I am confusing with all kinds of ERR all the time, ENOSPC show
> "No space left in the device"... And last time somebody told me "ENOTTY"
> means something is not available...)
>
> > > +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...
>
Oh, we preserved the user-passed GSI in case of userspace want to update one 
entry. :)

-- 
regards
Yang, Sheng