[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] i386: pci-assign: Fix MSI-X table size
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [PATCH] i386: pci-assign: Fix MSI-X table size |
Date: |
Tue, 21 Jun 2016 07:10:27 +0300 |
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 <address@hidden>
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 <address@hidden>
> ---
> 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