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

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

On Thursday 08 January 2009 00:18:39 Marcelo Tosatti wrote:
> Hi Sheng,
>

Thanks for the careful review! :)
> On Wed, Jan 07, 2009 at 06:42:37PM +0800, Sheng Yang wrote:
> > diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
> > index 5162a41..7460e7f 100644
> > --- a/virt/kvm/irq_comm.c
> > +++ b/virt/kvm/irq_comm.c
> > @@ -123,3 +123,73 @@ void kvm_fire_mask_notifiers(struct kvm *kvm, int
> > irq, bool mask) kimn->func(kimn, mask);
> >  }
> >
> > +int kvm_update_gsi_route(struct kvm *kvm, struct kvm_gsi_route_entry
> > *entry) +{
> > +	struct kvm_gsi_route_entry *found_entry, *new_entry;
> > +	int r, gsi;
> > +
> > +	mutex_lock(&kvm->lock);
> > +	/* Find whether we need a update or a new entry */
> > +	found_entry = kvm_find_gsi_route_entry(kvm, entry->gsi);
> > +	if (found_entry)
> > +		*found_entry = *entry;
> > +	else {
>
> Having a kvm_find_alloc_gsi_route_entry which either returns a present
> entry if found or returns a newly allocated one makes the code easier to
> read for me. Then just
>
>                 entry = kvm_find_alloc_gsi_route_entry
>                 *entry = *new_entry;
>
> Just style, feel free to ignore the comment.

A little different, for we have to use kvm_find_alloc_gsi_route_entry() to 
find correlated gsi for injecting interrupt, so allocate a new one is not that 
proper here. 
>
> > +		gsi = find_first_zero_bit(kvm->gsi_route_bitmap,
> > +					  KVM_NR_GSI_ROUTE_ENTRIES);
> > +		if (gsi >= KVM_NR_GSI_ROUTE_ENTRIES) {
> > +			r = -ENOSPC;
> > +			goto out;
> > +		}
> > +		__set_bit(gsi, kvm->gsi_route_bitmap);
> > +		entry->gsi = gsi | KVM_GSI_ROUTE_MASK;
> > +		new_entry = kzalloc(sizeof(*new_entry), GFP_KERNEL);
>
> Allocate first, then set the bit in the bitmap?
>
> > +		if (!new_entry) {
> > +			r = -ENOMEM;
> > +			goto out;
>
> Because here you don't clear the state on failure.
>
> </NITPICK MODE>

Oh, you are right.:)
>
> > +		}
> > +		*new_entry = *entry;
> > +		hlist_add_head(&new_entry->link, &kvm->gsi_route_list);
> > +	}
> > +	r = 0;
> > +out:
> > +	mutex_unlock(&kvm->lock);
> > +	return r;
> > +}
> > +
> >
> > +static int kvm_vm_ioctl_request_gsi_route(struct kvm *kvm,
> > +			struct kvm_gsi_route_guest *gsi_route,
> > +			struct kvm_gsi_route_entry_guest *guest_entries)
> > +{
> > +	struct kvm_gsi_route_entry entry;
> > +	int r, i;
> > +
> > +	for (i = 0; i < gsi_route->entries_nr; i++) {
> > +		memcpy(&entry, &guest_entries[i], sizeof(entry));
> > +		r = kvm_update_gsi_route(kvm, &entry);
> > +		if (r == 0)
> > +			guest_entries[i].gsi = entry.gsi;
> > +		else
> > +			break;
> > +	}
> > +	return r;
> > +}
> > +
> > +static int kvm_vm_ioctl_free_gsi_route(struct kvm *kvm,
> > +			struct kvm_gsi_route_guest *gsi_route,
> > +			struct kvm_gsi_route_entry_guest *guest_entries)
> > +{
> > +	struct kvm_gsi_route_entry *entry;
> > +	int r, i;
> > +
> > +	mutex_lock(&kvm->lock);
> > +	for (i = 0; i < gsi_route->entries_nr; i++) {
> > +		entry = kvm_find_gsi_route_entry(kvm, guest_entries[i].gsi);
> > +		if (!entry ||
> > +		    memcmp(entry, &guest_entries[i], sizeof(*entry)) != 0) {
> > +			printk(KERN_WARNING "kvm: illegal gsi mapping!");
>
> Don't think you need that printk?

After reconsidering, I am not sure if I should verify guest entry here, or 
just leave a gsi number to kernel for freeing. I am a little prefer only gsi 
here, for I think I can trust userspace to some degree... I would update the 
patch.(in fact, I didn't use this ioctl in userspace for now...)
>
> > +			r = -EINVAL;
> > +			goto out;
> > +		}
> > +		kvm_free_gsi_route(kvm, entry);
> > +	}
> > +out:
> > +	mutex_unlock(&kvm->lock);
> > +	return r;
> > +}
> > +
> >  static long kvm_vcpu_ioctl(struct file *filp,
> >  			   unsigned int ioctl, unsigned long arg)
> >  {
> > @@ -1803,6 +1846,7 @@ static long kvm_vm_ioctl(struct file *filp,
> >  {
> >  	struct kvm *kvm = filp->private_data;
> >  	void __user *argp = (void __user *)arg;
> > +	struct kvm_gsi_route_entry_guest *gsi_entries = NULL;
> >  	int r;
> >
> >  	if (kvm->mm != current->mm)
> > @@ -1887,10 +1931,72 @@ static long kvm_vm_ioctl(struct file *filp,
> >  		break;
> >  	}
> >  #endif
> > +	case KVM_REQUEST_GSI_ROUTE: {
> > +		struct kvm_gsi_route_guest gsi_route;
>
>                 r = -EFAULT;
>                 if (copy_from_user...)
>                         goto out;
>
>                 etc...
>
> To conform with the rest of the code in the function?

OK...
>
> > +		r = copy_from_user(&gsi_route, argp, sizeof gsi_route);
> > +		if (r)
> > +			goto out;
> > +		if (gsi_route.entries_nr == 0) {
> > +			r = -EFAULT;
> > +			goto out;
> > +		}
>
> Should check for a max of KVM_NR_GSI_ROUTE_ENTRIES ?

Yeah.
>
> > +		gsi_entries = kmalloc(gsi_route.entries_nr *
> > +				      sizeof(struct kvm_gsi_route_entry_guest),
> > +				      GFP_KERNEL);
>
> And use vmalloc/vfree instead.

Yeah... kmalloc is not necessary here...
>
> > +		if (!gsi_entries) {
> > +			r = -ENOMEM;
> > +			goto out;
> > +		}
> > +		r = copy_from_user(gsi_entries,
> > +				   (void __user *)gsi_route.entries,
> > +				   gsi_route.entries_nr *
> > +				   sizeof(struct kvm_gsi_route_entry_guest));
> > +		if (r)
> > +			goto out;
> > +		r = kvm_vm_ioctl_request_gsi_route(kvm, &gsi_route,
> > +						   gsi_entries);
> > +		if (r)
> > +			goto out;
> > +		r = copy_to_user((void __user *)gsi_route.entries,
> > +				gsi_entries,
> > +				gsi_route.entries_nr *
> > +				sizeof(struct kvm_gsi_route_entry_guest));
> > +		if (r)
> > +			goto out;
> > +		break;
> > +	}
> > +	case KVM_FREE_GSI_ROUTE: {
> > +		struct kvm_gsi_route_guest gsi_route;
> > +		r = copy_from_user(&gsi_route, argp, sizeof gsi_route);
> > +		if (r)
> > +			goto out;
> > +		if (gsi_route.entries_nr == 0) {
> > +			r = -EFAULT;
> > +			goto out;
> > +		}
>
> Check KVM_NR_GSI_ROUTE_ENTRIES and vmalloc.

Sure.

-- 
regards
Yang, Sheng