From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60150) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bFD1M-0005O7-3N for qemu-devel@nongnu.org; Tue, 21 Jun 2016 00:10:41 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bFD1F-0000MR-Sp for qemu-devel@nongnu.org; Tue, 21 Jun 2016 00:10:38 -0400 Received: from mx1.redhat.com ([209.132.183.28]:46670) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bFD1F-0000M1-Fu for qemu-devel@nongnu.org; Tue, 21 Jun 2016 00:10:33 -0400 Date: Tue, 21 Jun 2016 07:10:27 +0300 From: "Michael S. Tsirkin" Message-ID: <20160621064823-mutt-send-email-mst@redhat.com> References: <1466282525-3018-1-git-send-email-ido@wizery.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1466282525-3018-1-git-send-email-ido@wizery.com> Subject: Re: [Qemu-devel] [PATCH] i386: pci-assign: Fix MSI-X table size List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Ido Yariv Cc: qemu-devel@nongnu.org, Paolo Bonzini , Richard Henderson , Eduardo Habkost On Sat, Jun 18, 2016 at 04:42:05PM -0400, Ido Yariv wrote: > The current code creates a whole page mmio region for the MSI-X table > size. > > However, the page containing the MSI-X table may contain other registers > not related to MSI-X. Creating an mmio region for the whole page masks > such registers and may break drivers in the guest OS. > > Since maximal number of entries is known, use that instead to deduce the > table size when setting up the mmio region. > > Signed-off-by: Ido Yariv I personally don't want to spend cycles maintaining the old pci-assign but if someone does I don't have an issue with that. I remember why I coded up MSIX_PAGE_SIZE - this was before the new memory API which made it very tricky to support sub-page mappings. The PCI spec stringly recommends against sharing a page between page table and other registers but I guess if a device violates that rule it's a better idea to make it work slowly than fail. Patch looks good to me so: Reviewed-by: Michael S. Tsirkin > --- > hw/i386/kvm/pci-assign.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c > index f9c9014..98997d1 100644 > --- a/hw/i386/kvm/pci-assign.c > +++ b/hw/i386/kvm/pci-assign.c > @@ -36,8 +36,6 @@ > #include "kvm_i386.h" > #include "hw/pci/pci-assign.h" > > -#define MSIX_PAGE_SIZE 0x1000 > - > /* From linux/ioport.h */ > #define IORESOURCE_IO 0x00000100 /* Resource type */ > #define IORESOURCE_MEM 0x00000200 > @@ -122,6 +120,7 @@ typedef struct AssignedDevice { > int *msi_virq; > MSIXTableEntry *msix_table; > hwaddr msix_table_addr; > + uint16_t msix_table_size; > uint16_t msix_max; > MemoryRegion mmio; > char *configfd_name; > @@ -1310,6 +1309,7 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev, Error **errp) > bar_nr = msix_table_entry & PCI_MSIX_FLAGS_BIRMASK; > msix_table_entry &= ~PCI_MSIX_FLAGS_BIRMASK; > dev->msix_table_addr = pci_region[bar_nr].base_addr + msix_table_entry; > + dev->msix_table_size = msix_max * sizeof(MSIXTableEntry); > dev->msix_max = msix_max; > } > > @@ -1633,7 +1633,7 @@ static void assigned_dev_msix_reset(AssignedDevice *dev) > return; > } > > - memset(dev->msix_table, 0, MSIX_PAGE_SIZE); > + memset(dev->msix_table, 0, dev->msix_table_size); > > for (i = 0, entry = dev->msix_table; i < dev->msix_max; i++, entry++) { > entry->ctrl = cpu_to_le32(0x1); /* Masked */ > @@ -1642,8 +1642,8 @@ static void assigned_dev_msix_reset(AssignedDevice *dev) > > static void assigned_dev_register_msix_mmio(AssignedDevice *dev, Error **errp) > { > - dev->msix_table = mmap(NULL, MSIX_PAGE_SIZE, PROT_READ|PROT_WRITE, > - MAP_ANONYMOUS|MAP_PRIVATE, 0, 0); > + dev->msix_table = mmap(NULL, dev->msix_table_size, PROT_READ | PROT_WRITE, > + MAP_ANONYMOUS | MAP_PRIVATE, 0, 0); > if (dev->msix_table == MAP_FAILED) { > error_setg_errno(errp, errno, "failed to allocate msix_table"); > dev->msix_table = NULL; > @@ -1653,7 +1653,7 @@ static void assigned_dev_register_msix_mmio(AssignedDevice *dev, Error **errp) > assigned_dev_msix_reset(dev); > > memory_region_init_io(&dev->mmio, OBJECT(dev), &assigned_dev_msix_mmio_ops, > - dev, "assigned-dev-msix", MSIX_PAGE_SIZE); > + dev, "assigned-dev-msix", dev->msix_table_size); > } > > static void assigned_dev_unregister_msix_mmio(AssignedDevice *dev) > @@ -1662,7 +1662,7 @@ static void assigned_dev_unregister_msix_mmio(AssignedDevice *dev) > return; > } > > - if (munmap(dev->msix_table, MSIX_PAGE_SIZE) == -1) { > + if (munmap(dev->msix_table, dev->msix_table_size) == -1) { > error_report("error unmapping msix_table! %s", strerror(errno)); > } > dev->msix_table = NULL; > -- > 2.5.5