From patchwork Tue Jun 7 18:46:13 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Alex Williamson X-Patchwork-Id: 859062 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by demeter2.kernel.org (8.14.4/8.14.3) with ESMTP id p57IkIQN005406 for ; Tue, 7 Jun 2011 18:46:18 GMT Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755276Ab1FGSqP (ORCPT ); Tue, 7 Jun 2011 14:46:15 -0400 Received: from mx1.redhat.com ([209.132.183.28]:48396 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754875Ab1FGSqO (ORCPT ); Tue, 7 Jun 2011 14:46:14 -0400 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p57IkEc2005326 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Tue, 7 Jun 2011 14:46:14 -0400 Received: from [10.3.113.86] (ovpn-113-86.phx2.redhat.com [10.3.113.86]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id p57IkDBG019984; Tue, 7 Jun 2011 14:46:13 -0400 Subject: Re: [PATCH] pci-assign: Do not reset the device unless the kernel supports it From: Alex Williamson To: Jan Kiszka Cc: Avi Kivity , Marcelo Tosatti , kvm Date: Tue, 07 Jun 2011 12:46:13 -0600 In-Reply-To: <4DEDDDDE.4010209@web.de> References: <4DED470F.4020203@web.de> <1307396894.5901.5.camel@x201> <4DED4EDA.80803@web.de> <4DEDDC1D.7000905@redhat.com> <4DEDDDDE.4010209@web.de> Message-ID: <1307472373.5901.33.camel@x201> Mime-Version: 1.0 X-Scanned-By: MIMEDefang 2.67 on 10.5.11.12 Sender: kvm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org X-Greylist: IP, sender and recipient auto-whitelisted, not delayed by milter-greylist-4.2.6 (demeter2.kernel.org [140.211.167.43]); Tue, 07 Jun 2011 18:46:19 +0000 (UTC) On Tue, 2011-06-07 at 10:14 +0200, Jan Kiszka wrote: > On 2011-06-07 10:06, Avi Kivity wrote: > > On 06/07/2011 01:04 AM, Jan Kiszka wrote: > >> On 2011-06-06 23:48, Alex Williamson wrote: > >> > On Mon, 2011-06-06 at 23:30 +0200, Jan Kiszka wrote: > >> >> From: Jan Kiszka > >> >> > >> >> At least kernels 2.6.38 and 2.6.39 do not properly support issuing a > >> >> reset on an assigned device and corrupt its config space. Prevent > >> >> this by checking for a host kernel with the required support, > >> tagged by > >> >> the to-be-introduced KVM_CAP_DEVICE_RESET. > >> > > >> > Wouldn't it be easier just to revert ed78661f in 2.6.39 stable? I > >> guess > >> > we don't have an option to do that for .38 since stable is done there, > >> > but there are also some intel-iommu breakages that won't make > >> stable for > >> > that release. It seems like the userspace invoked reset resolves > >> known, > >> > demonstrable issues of devices continuing to DMA into guest memory > >> while > >> > ed78661f is mostly a theoretical change. > >> > >> Easier would be this patch. But I don't mind reverting the problematic > >> commit in 39, whatever is preferred. We should just resolve the issue > >> finally. > > > > Kernel problems should be solved in the kernel (with exceptions of > > course, but don't see the need here). > > Then please file a revert for stable ASAP. How's this? For stable only or course. Thanks, Alex Revert "KVM: Save/restore state of assigned PCI device" From: Alex Williamson This reverts ed78661f2614d3c9f69c23e280db3bafdabdf5bb as it assumes the saved PCI state will remain valid for the entire length of time that it is attached to a guest. This fails when userspace makes use of the pci-sysfs reset interface, which invalidates the saved device state, leaving nothing to be restored after the device is reset on de-assignment. This leaves the device in an unusable state. 3.0.0 will add an interface for KVM to save the PCI state in a buffer unaffected by other callers of pci_reset_function(), but the most appropriate stable fix seems to be reverting this change since the original assumption about the device saved state persisting is incorrect. Signed-off-by: Alex Williamson Acked-by: Jan Kiszka --- virt/kvm/assigned-dev.c | 5 +---- 1 files changed, 1 insertions(+), 4 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c index ae72ae6..e3f1235 100644 --- a/virt/kvm/assigned-dev.c +++ b/virt/kvm/assigned-dev.c @@ -197,8 +197,7 @@ static void kvm_free_assigned_device(struct kvm *kvm, { kvm_free_assigned_irq(kvm, assigned_dev); - __pci_reset_function(assigned_dev->dev); - pci_restore_state(assigned_dev->dev); + pci_reset_function(assigned_dev->dev); pci_release_regions(assigned_dev->dev); pci_disable_device(assigned_dev->dev); @@ -515,7 +514,6 @@ static int kvm_vm_ioctl_assign_device(struct kvm *kvm, } pci_reset_function(dev); - pci_save_state(dev); match->assigned_dev_id = assigned_dev->assigned_dev_id; match->host_segnr = assigned_dev->segnr; @@ -546,7 +544,6 @@ out: mutex_unlock(&kvm->lock); return r; out_list_del: - pci_restore_state(dev); list_del(&match->list); pci_release_regions(dev); out_disable: