From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH 3/5] KVM: pci device assignment Date: Tue, 29 Jul 2008 17:02:26 +0300 Message-ID: <488F22F2.7030709@qumranet.com> References: <1217262388-7309-1-git-send-email-benami@il.ibm.com> <1217262388-7309-3-git-send-email-benami@il.ibm.com> <1217262388-7309-4-git-send-email-benami@il.ibm.com> <200807291449.35914.amit.shah@qumranet.com> <1217324307.24756.5.camel@cluwyn.haifa.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: Amit Shah , kvm@vger.kernel.org, Muli Ben-Yehuda , weidong.han@intel.com, anthony@codemonkey.ws To: Ben-Ami Yassour Return-path: Received: from il.qumranet.com ([212.179.150.194]:32516 "EHLO il.qumranet.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755428AbYG2OCa (ORCPT ); Tue, 29 Jul 2008 10:02:30 -0400 In-Reply-To: <1217324307.24756.5.camel@cluwyn.haifa.ibm.com> Sender: kvm-owner@vger.kernel.org List-ID: Ben-Ami Yassour wrote: > On Tue, 2008-07-29 at 14:49 +0530, Amit Shah wrote: > >> * On Monday 28 Jul 2008 21:56:26 Ben-Ami Yassour wrote: >> >> >>> +static int kvm_vm_ioctl_assign_device(struct kvm *kvm, >>> + struct kvm_assigned_pci_dev *assigned_dev) >>> +{ >>> >> >>> + if (pci_enable_device(dev)) { >>> + printk(KERN_INFO "%s: Could not enable PCI device\n", __func__); >>> + r = -EBUSY; >>> + goto out_put; >>> + } >>> + r = pci_request_regions(dev, "kvm_assigned_device"); >>> + if (r) { >>> + printk(KERN_INFO "%s: Could not get access to device regions\n", >>> + __func__); >>> + goto out_disable; >>> >> Shouldn't disable here unconditionally (see my comment earlier to the previous >> patch). >> > Why? the device should not be used by the host at the same time. > What is the condition that you were thinking of? > > pci_enable_device() can succeed even if the device was already enabled, so Amit was probably wishing to avoid an assignment failure disabling a device under a driver's feet. But I see that pci_disable_device() will pair with pci_enable_device() correctly (doing reference counts), so I think the code is correct as is. -- error compiling committee.c: too many arguments to function