Re: [PATCH v2 4/4] kvm: i386: Add classic PCI device assignment

["Michael S. Tsirkin" <mst@redhat.com>]

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 <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?
> 
> 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