From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Michael S. Tsirkin" Subject: Re: [PATCH v2 4/4] kvm: i386: Add classic PCI device assignment Date: Wed, 29 Aug 2012 13:35:53 +0300 Message-ID: <20120829103553.GF5225@redhat.com> References: <825e653c9cfe9d8e26185917cbe1f1dd7ae299e2.1346048917.git.jan.kiszka@web.de> <503CA542.7030707@siemens.com> <20120828214912.GE5817@redhat.com> <503DD676.80907@siemens.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Avi Kivity , Marcelo Tosatti , "kvm@vger.kernel.org" , "qemu-devel@nongnu.org" , Alex Williamson , Blue Swirl , Andreas =?iso-8859-1?Q?F=E4rber?= To: Jan Kiszka Return-path: Received: from mx1.redhat.com ([209.132.183.28]:8497 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751751Ab2H2Kem (ORCPT ); Wed, 29 Aug 2012 06:34:42 -0400 Content-Disposition: inline In-Reply-To: <503DD676.80907@siemens.com> Sender: kvm-owner@vger.kernel.org List-ID: On Wed, Aug 29, 2012 at 10:44:38AM +0200, Jan Kiszka wrote: > On 2012-08-28 23:49, Michael S. Tsirkin wrote: > > On Tue, Aug 28, 2012 at 01:02:26PM +0200, Jan Kiszka wrote: > >> This adds PCI device assignment for i386 targets using the classic KVM > >> interfaces. This version is 100% identical to what is being maintained > >> in qemu-kvm for several years and is supported by libvirt as well. It is > >> expected to remain relevant for another couple of years until kernels > >> without full-features and performance-wise equivalent VFIO support are > >> obsolete. > >> > >> A refactoring to-do that should be done in-tree is to model MSI and > >> MSI-X support via the generic PCI layer, similar to what VFIO is already > >> doing for MSI-X. This should improve the correctness and clean up the > >> code from duplicate logic. > >> > >> Signed-off-by: Jan Kiszka > >> --- > >> > >> Changes in v2: > >> - Addressed review comments of Andreas and most of Blue (see [1] for > >> unchanged aspects) > > > > any chance you can address mine? > > Sorry, must have missed that mail. Can you point me to it? Hmm I dont see it on list but do see in my outgoing mailbox. Not sure why, I resent it. But below where the only two issues that would be better to fix before merging. Others can be fixed upstream. > > Specifically I suggested > > listing commit id that you started from. > > OK. > > > Also: > > > >> +static void msix_reset(AssignedDevice *dev) > >> +{ > >> + MSIXTableEntry *entry; > >> + int i; > >> + > >> + if (!dev->msix_table) { > >> + return; > >> + } > >> + > >> + memset(dev->msix_table, 0, MSIX_PAGE_SIZE); > >> + > >> + for (i = 0, entry = dev->msix_table; i < dev->msix_max; i++, entry++) { > >> + entry->ctrl = cpu_to_le32(0x1); /* Masked */ > >> + } > >> +} > > > > Is a bad name. Ideally file scope names should have a prefix > > assigned_dev_ or pci_assign_ or something like that > > but it's not a hard requirement. In this case > > file includes msix.h so prefix msix_ is confusing. > > True, will fix. > > Jan > > -- > Siemens AG, Corporate Technology, CT RTC ITP SDP-DE > Corporate Competence Center Embedded Linux From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:37423) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T6fbh-0005Xd-OH for qemu-devel@nongnu.org; Wed, 29 Aug 2012 06:34:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1T6fba-0006N4-CP for qemu-devel@nongnu.org; Wed, 29 Aug 2012 06:34:45 -0400 Received: from mx1.redhat.com ([209.132.183.28]:57522) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T6fba-0006Mz-3G for qemu-devel@nongnu.org; Wed, 29 Aug 2012 06:34:38 -0400 Date: Wed, 29 Aug 2012 13:35:53 +0300 From: "Michael S. Tsirkin" Message-ID: <20120829103553.GF5225@redhat.com> References: <825e653c9cfe9d8e26185917cbe1f1dd7ae299e2.1346048917.git.jan.kiszka@web.de> <503CA542.7030707@siemens.com> <20120828214912.GE5817@redhat.com> <503DD676.80907@siemens.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <503DD676.80907@siemens.com> Subject: Re: [Qemu-devel] [PATCH v2 4/4] kvm: i386: Add classic PCI device assignment List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jan Kiszka Cc: "kvm@vger.kernel.org" , Marcelo Tosatti , "qemu-devel@nongnu.org" , Blue Swirl , Alex Williamson , Avi Kivity , Andreas =?iso-8859-1?Q?F=E4rber?= On Wed, Aug 29, 2012 at 10:44:38AM +0200, Jan Kiszka wrote: > On 2012-08-28 23:49, Michael S. Tsirkin wrote: > > On Tue, Aug 28, 2012 at 01:02:26PM +0200, Jan Kiszka wrote: > >> This adds PCI device assignment for i386 targets using the classic KVM > >> interfaces. This version is 100% identical to what is being maintained > >> in qemu-kvm for several years and is supported by libvirt as well. It is > >> expected to remain relevant for another couple of years until kernels > >> without full-features and performance-wise equivalent VFIO support are > >> obsolete. > >> > >> A refactoring to-do that should be done in-tree is to model MSI and > >> MSI-X support via the generic PCI layer, similar to what VFIO is already > >> doing for MSI-X. This should improve the correctness and clean up the > >> code from duplicate logic. > >> > >> Signed-off-by: Jan Kiszka > >> --- > >> > >> Changes in v2: > >> - Addressed review comments of Andreas and most of Blue (see [1] for > >> unchanged aspects) > > > > any chance you can address mine? > > Sorry, must have missed that mail. Can you point me to it? Hmm I dont see it on list but do see in my outgoing mailbox. Not sure why, I resent it. But below where the only two issues that would be better to fix before merging. Others can be fixed upstream. > > Specifically I suggested > > listing commit id that you started from. > > OK. > > > Also: > > > >> +static void msix_reset(AssignedDevice *dev) > >> +{ > >> + MSIXTableEntry *entry; > >> + int i; > >> + > >> + if (!dev->msix_table) { > >> + return; > >> + } > >> + > >> + memset(dev->msix_table, 0, MSIX_PAGE_SIZE); > >> + > >> + for (i = 0, entry = dev->msix_table; i < dev->msix_max; i++, entry++) { > >> + entry->ctrl = cpu_to_le32(0x1); /* Masked */ > >> + } > >> +} > > > > Is a bad name. Ideally file scope names should have a prefix > > assigned_dev_ or pci_assign_ or something like that > > but it's not a hard requirement. In this case > > file includes msix.h so prefix msix_ is confusing. > > True, will fix. > > Jan > > -- > Siemens AG, Corporate Technology, CT RTC ITP SDP-DE > Corporate Competence Center Embedded Linux