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

Yeah, for the MSI(due to we emulated the configuration space in the userspace) 
and virtio.

Why it would return a error? I missed something?

-- 
regards
Yang, Sheng