From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50535) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YYDSL-0001sz-PU for qemu-devel@nongnu.org; Wed, 18 Mar 2015 08:52:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YYDSI-0000AB-Hj for qemu-devel@nongnu.org; Wed, 18 Mar 2015 08:52:17 -0400 Received: from mx1.redhat.com ([209.132.183.28]:42524) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YYDSI-0000A1-9q for qemu-devel@nongnu.org; Wed, 18 Mar 2015 08:52:14 -0400 Date: Wed, 18 Mar 2015 13:52:06 +0100 From: "Michael S. Tsirkin" Message-ID: <20150318134611-mutt-send-email-mst@redhat.com> References: <1426671309-13645-1-git-send-email-jasowang@redhat.com> <1426671309-13645-19-git-send-email-jasowang@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1426671309-13645-19-git-send-email-jasowang@redhat.com> Subject: Re: [Qemu-devel] [PATCH V4 18/19] pci: remove hard-coded bar size in msix_init_exclusive_bar() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jason Wang Cc: cornelia.huck@de.ibm.com, Keith Busch , Stefan Hajnoczi , qemu-devel@nongnu.org, Kevin Wolf On Wed, Mar 18, 2015 at 05:35:08PM +0800, Jason Wang wrote: > This patch let msix_init_exclusive_bar() can accept bar_size parameter > other than a hard-coded limit 4096. Then caller can specify a bar_size > depends on msix entries and can use up to 2048 msix entries as PCI > spec allows. To keep migration compatibility, 4096 is used for all > callers and pba were start from half of bar size. > > Cc: Keith Busch > Cc: Kevin Wolf > Cc: Stefan Hajnoczi > Cc: Michael S. Tsirkin > Signed-off-by: Jason Wang Hmm this API looks strange. Caller will then have to have msi specific knowledge. Can't we keep the size iternal to msix.c? > --- > hw/block/nvme.c | 2 +- > hw/misc/ivshmem.c | 2 +- > hw/pci/msix.c | 18 +++++++----------- > hw/virtio/virtio-pci.c | 2 +- > include/hw/pci/msix.h | 2 +- > 5 files changed, 11 insertions(+), 15 deletions(-) > > diff --git a/hw/block/nvme.c b/hw/block/nvme.c > index 0f3dfb9..09d7884 100644 > --- a/hw/block/nvme.c > +++ b/hw/block/nvme.c > @@ -787,7 +787,7 @@ static int nvme_init(PCIDevice *pci_dev) > pci_register_bar(&n->parent_obj, 0, > PCI_BASE_ADDRESS_SPACE_MEMORY | PCI_BASE_ADDRESS_MEM_TYPE_64, > &n->iomem); > - msix_init_exclusive_bar(&n->parent_obj, n->num_queues, 4); > + msix_init_exclusive_bar(&n->parent_obj, n->num_queues, 4, 4096); > > id->vid = cpu_to_le16(pci_get_word(pci_conf + PCI_VENDOR_ID)); > id->ssvid = cpu_to_le16(pci_get_word(pci_conf + PCI_SUBSYSTEM_VENDOR_ID)); > diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c > index 5d272c8..3e2a2d4 100644 > --- a/hw/misc/ivshmem.c > +++ b/hw/misc/ivshmem.c > @@ -631,7 +631,7 @@ static uint64_t ivshmem_get_size(IVShmemState * s) { > > static void ivshmem_setup_msi(IVShmemState * s) > { > - if (msix_init_exclusive_bar(PCI_DEVICE(s), s->vectors, 1)) { > + if (msix_init_exclusive_bar(PCI_DEVICE(s), s->vectors, 1, 4096)) { > IVSHMEM_DPRINTF("msix initialization failed\n"); > exit(1); > } > diff --git a/hw/pci/msix.c b/hw/pci/msix.c > index 24de260..9a1894f 100644 > --- a/hw/pci/msix.c > +++ b/hw/pci/msix.c > @@ -291,33 +291,29 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries, > } > > int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries, > - uint8_t bar_nr) > + uint8_t bar_nr, uint32_t bar_size) > { > int ret; > char *name; > + uint32_t bar_pba_offset = bar_size / 2; Spec says we must give BIR 4k of it's own, but for larger BARs, using up half the BAR just for BIR seems wasteful. > > /* > * Migration compatibility dictates that this remains a 4k > * BAR with the vector table in the lower half and PBA in > * the upper half. Do not use these elsewhere! > */ You've left comment here where it no longer makes any sense. > -#define MSIX_EXCLUSIVE_BAR_SIZE 4096 > -#define MSIX_EXCLUSIVE_BAR_TABLE_OFFSET 0 > -#define MSIX_EXCLUSIVE_BAR_PBA_OFFSET (MSIX_EXCLUSIVE_BAR_SIZE / 2) > -#define MSIX_EXCLUSIVE_CAP_OFFSET 0 > - > - if (nentries * PCI_MSIX_ENTRY_SIZE > MSIX_EXCLUSIVE_BAR_PBA_OFFSET) { > + if (nentries * PCI_MSIX_ENTRY_SIZE > bar_pba_offset) { > return -EINVAL; > } > > name = g_strdup_printf("%s-msix", dev->name); > - memory_region_init(&dev->msix_exclusive_bar, OBJECT(dev), name, MSIX_EXCLUSIVE_BAR_SIZE); > + memory_region_init(&dev->msix_exclusive_bar, OBJECT(dev), name, bar_size); > g_free(name); > > ret = msix_init(dev, nentries, &dev->msix_exclusive_bar, bar_nr, > - MSIX_EXCLUSIVE_BAR_TABLE_OFFSET, &dev->msix_exclusive_bar, > - bar_nr, MSIX_EXCLUSIVE_BAR_PBA_OFFSET, > - MSIX_EXCLUSIVE_CAP_OFFSET); > + 0, &dev->msix_exclusive_bar, > + bar_nr, bar_pba_offset, > + 0); > if (ret) { > return ret; > } > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > index 02e3ce8..4a5febb 100644 > --- a/hw/virtio/virtio-pci.c > +++ b/hw/virtio/virtio-pci.c > @@ -937,7 +937,7 @@ static void virtio_pci_device_plugged(DeviceState *d) > config[PCI_INTERRUPT_PIN] = 1; > > if (proxy->nvectors && > - msix_init_exclusive_bar(&proxy->pci_dev, proxy->nvectors, 1)) { > + msix_init_exclusive_bar(&proxy->pci_dev, proxy->nvectors, 1, 4096)) { > error_report("unable to init msix vectors to %" PRIu32, > proxy->nvectors); > proxy->nvectors = 0; > diff --git a/include/hw/pci/msix.h b/include/hw/pci/msix.h > index 954d82b..43edebc 100644 > --- a/include/hw/pci/msix.h > +++ b/include/hw/pci/msix.h > @@ -11,7 +11,7 @@ int msix_init(PCIDevice *dev, unsigned short nentries, > unsigned table_offset, MemoryRegion *pba_bar, > uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos); > int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries, > - uint8_t bar_nr); > + uint8_t bar_nr, uint32_t bar_size); > > void msix_write_config(PCIDevice *dev, uint32_t address, uint32_t val, int len); > > -- > 2.1.0