From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH 2/2] KVM: Use pci_store/load_saved_state() around VM device usage Date: Wed, 20 Apr 2011 10:19:49 +0300 Message-ID: <4DAE8915.8020804@redhat.com> References: <20110415194220.2838.10750.stgit@s20.home> <20110415195439.2838.58928.stgit@s20.home> <4DAAB221.4080506@redhat.com> <1303155820.3110.91.camel@x201> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Cc: linux-pci@vger.kernel.org, kvm@vger.kernel.org, jan.kiszka@siemens.com, jbarnes@virtuousgeek.org To: Alex Williamson Return-path: In-Reply-To: <1303155820.3110.91.camel@x201> Sender: linux-pci-owner@vger.kernel.org List-Id: kvm.vger.kernel.org On 04/18/2011 10:43 PM, Alex Williamson wrote: > On Sun, 2011-04-17 at 12:25 +0300, Avi Kivity wrote: > > On 04/15/2011 10:54 PM, Alex Williamson wrote: > > > Store the device saved state so that we can reload the device back > > > to the original state when it's unassigned. This has the benefit > > > that the state survives across pci_reset_function() calls via > > > the PCI sysfs reset interface while the VM is using the device. > > > > > @@ -516,7 +518,7 @@ static int kvm_vm_ioctl_assign_device(struct kvm *kvm, > > > > > > pci_reset_function(dev); > > > pci_save_state(dev); > > > - > > > + match->pci_saved_state = pci_store_saved_state(dev); > > > match->assigned_dev_id = assigned_dev->assigned_dev_id; > > > > Error check? > > > > It might be better to give up the opacity of the data structure and make > > pci_saved_state the full struct, not a pointer. > > pci_store_saved_state() returns NULL on error, which is correctly > handled if we pass NULL to pci_load_saved_state() or a pointer to NULL > to pci_load_and_free_saved_state(). But we silently swallow an error, this isn't good. > This is also why I changed the > __pci_reset_function() back to a normal pci_reset_function(), so we're > never left with an uninitialized device like we are now. > > We could be more verbose or return an error here, but we've gone for a > long time not even doing this save/restore across VM usage, so I don't > think it's worthy of preventing the device attachment if it fails. At least a log? Note avoiding the pointer would have removed the problem altogether. -- error compiling committee.c: too many arguments to function