From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sheng Yang Subject: Re: [PATCH 01/10] KVM: Add a route layer to convert MSI message to GSI Date: Thu, 8 Jan 2009 15:30:33 +0800 Message-ID: <200901081530.34420.sheng@linux.intel.com> References: <1231324966-22286-1-git-send-email-sheng@linux.intel.com> <1231324966-22286-2-git-send-email-sheng@linux.intel.com> <20090107161839.GD5442@amt.cnet> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Cc: Avi Kivity , kvm@vger.kernel.org To: Marcelo Tosatti Return-path: Received: from mga05.intel.com ([192.55.52.89]:35416 "EHLO fmsmga101.fm.intel.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752775AbZAHHah (ORCPT ); Thu, 8 Jan 2009 02:30:37 -0500 In-Reply-To: <20090107161839.GD5442@amt.cnet> Content-Disposition: inline Sender: kvm-owner@vger.kernel.org List-ID: 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. > > 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