From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Michael S. Tsirkin" <mst@redhat.com>
Subject: Re: [PATCH v2 4/4] kvm: i386: Add classic PCI device
	assignment
Date: Wed, 29 Aug 2012 00:49:12 +0300
Message-ID: <20120828214912.GE5817@redhat.com>
References: <cover.1346048917.git.jan.kiszka@web.de>
	<825e653c9cfe9d8e26185917cbe1f1dd7ae299e2.1346048917.git.jan.kiszka@web.de>
	<503CA542.7030707@siemens.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: kvm@vger.kernel.org, Marcelo Tosatti <mtosatti@redhat.com>,
	qemu-devel@nongnu.org, Blue Swirl <blauwirbel@gmail.com>,
	Alex Williamson <alex.williamson@redhat.com>, Avi Kivity <avi@redhat.com>,
	Andreas =?iso-8859-1?Q?F=E4rber?= <afaerber@suse.de>
To: Jan Kiszka <jan.kiszka@siemens.com>
Return-path: <qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org>
Content-Disposition: inline
In-Reply-To: <503CA542.7030707@siemens.com>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
Errors-To: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org
Sender: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org
List-Id: kvm.vger.kernel.org

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 <jan.kiszka@siemens.com>
> ---
> 
> Changes in v2:
>  - Addressed review comments of Andreas and most of Blue (see [1] for
>    unchanged aspects)

any chance you can address mine? Specifically I suggested
listing commit id that you started from. 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.

-- 
MST