From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:52437) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SfAnJ-0005MP-4h for qemu-devel@nongnu.org; Thu, 14 Jun 2012 10:13:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SfAnD-00072v-St for qemu-devel@nongnu.org; Thu, 14 Jun 2012 10:13:04 -0400 Received: from mx1.redhat.com ([209.132.183.28]:16082) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SfAnD-00072i-K4 for qemu-devel@nongnu.org; Thu, 14 Jun 2012 10:12:59 -0400 Message-ID: <1339683177.24818.19.camel@ul30vt> From: Alex Williamson Date: Thu, 14 Jun 2012 08:12:57 -0600 In-Reply-To: <4FD981F2.5010905@siemens.com> References: <20120614044751.11034.87619.stgit@bling.home> <20120614045155.11034.91392.stgit@bling.home> <4FD981F2.5010905@siemens.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Mime-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH v2 5/6] msix: Allow full specification of MSIX layout List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jan Kiszka Cc: "qemu-devel@nongnu.org" , "mst@redhat.com" On Thu, 2012-06-14 at 08:17 +0200, Jan Kiszka wrote: > On 2012-06-14 06:51, Alex Williamson wrote: > > > -/* Initialize the MSI-X structures. Note: if MSI-X is supported, BAR size is > > - * modified, it should be retrieved with msix_bar_size. */ > > +/* Add MSI-X capability to the config space for the device. */ > > +static int msix_add_config(struct PCIDevice *dev, unsigned short nentries, > > + uint8_t table_bar, unsigned table_offset, > > + uint8_t pba_bar, unsigned pba_offset, uint8_t pos) > > Why not fold msix_add_config into msix_init and move sanity checks to > the beginning, i.e. before resource allocations? Then you also do not > need to call msix_uninit from msix_init. Likely a matter of taste, so > not a must-have. Yep, that works nicely. fixed. > > +{ > > + int config_offset; > > + uint8_t *config; > > + > > + if (nentries < 1 || nentries > PCI_MSIX_FLAGS_QSIZE + 1) { > > + return -EINVAL; > > + } > > + > > + config_offset = pci_add_capability(dev, PCI_CAP_ID_MSIX, > > + pos, MSIX_CAP_LENGTH); > > + if (config_offset < 0) { > > + return config_offset; > > + } > > + > > + config = dev->config + config_offset; > > + > > + pci_set_word(config + PCI_MSIX_FLAGS, nentries - 1); > > + pci_set_long(config + PCI_MSIX_TABLE, table_offset | table_bar); > > + pci_set_long(config + PCI_MSIX_PBA, pba_offset | pba_bar); > > + > > + dev->msix_cap = config_offset; > > + > > + /* Make flags bit writable. */ > > + dev->wmask[config_offset + MSIX_CONTROL_OFFSET] |= MSIX_ENABLE_MASK | > > + MSIX_MASKALL_MASK; > > + > > + dev->msix_function_masked = true; > > + > > + return 0; > > +} > > + > > +/* Clean up resources for the device. */ > > +void msix_uninit(PCIDevice *dev, MemoryRegion *table_bar, MemoryRegion *pba_bar) > > +{ > > + if (!(dev->cap_present & QEMU_PCI_CAP_MSIX)) { > > msix_present() Fixed. > > + return; > > + } > > + > > + pci_del_capability(dev, PCI_CAP_ID_MSIX, MSIX_CAP_LENGTH); > > + dev->msix_cap = 0; > > + dev->msix_entries_nr = 0; > > + > > + memory_region_del_subregion(pba_bar, &dev->msix_pba_mmio); > > + memory_region_destroy(&dev->msix_pba_mmio); > > + g_free(dev->msix_pba); > > + dev->msix_pba = NULL; > > + > > + memory_region_del_subregion(table_bar, &dev->msix_table_mmio); > > + memory_region_destroy(&dev->msix_table_mmio); > > + g_free(dev->msix_table); > > + dev->msix_table = NULL; > > + > > + g_free(dev->msix_entry_used); > > + dev->msix_entry_used = NULL; > > + dev->cap_present &= ~QEMU_PCI_CAP_MSIX; > > +} > > + > > +/* Initialize the MSI-X structures */ > > int msix_init(struct PCIDevice *dev, unsigned short nentries, > > - MemoryRegion *bar, > > - unsigned bar_nr, unsigned bar_size) > > + MemoryRegion *table_bar, uint8_t table_bar_nr, > > + unsigned table_offset, MemoryRegion *pba_bar, > > + uint8_t pba_bar_nr, unsigned pba_offset, uint8_t cap_pos) > > { > > int ret; > > unsigned table_size, pba_size; > > @@ -281,43 +279,41 @@ int msix_init(struct PCIDevice *dev, unsigned short nentries, > > if (!msi_supported) { > > return -ENOTSUP; > > } > > - if (nentries > MSIX_MAX_ENTRIES) > > - return -EINVAL; > > > > table_size = nentries * PCI_MSIX_ENTRY_SIZE; > > pba_size = QEMU_ALIGN_UP(nentries, 64) / 8; > > > > - dev->msix_entry_used = g_malloc0(MSIX_MAX_ENTRIES * > > - sizeof *dev->msix_entry_used); > > + /* Sanity test: table & pba don't overlap, fit within BARs, min aligned */ > > + if ((table_bar_nr == pba_bar_nr && > > + ranges_overlap(table_offset, table_size, pba_offset, pba_size)) || > > + table_offset + table_size > memory_region_size(table_bar) || > > + pba_offset + pba_size > memory_region_size(pba_bar) || > > + (table_offset | pba_offset) & PCI_MSIX_FLAGS_BIRMASK) { > > + return -EINVAL; > > + } > > > > dev->msix_table = g_malloc0(table_size); > > dev->msix_pba = g_malloc0(pba_size); > > + dev->msix_entry_used = g_malloc0(nentries * sizeof *dev->msix_entry_used); > > + dev->msix_entries_nr = nentries; > > + dev->cap_present |= QEMU_PCI_CAP_MSIX; > > + > > msix_mask_all(dev, nentries); > > > > memory_region_init_io(&dev->msix_table_mmio, &msix_table_mmio_ops, dev, > > "msix", table_size); > > + memory_region_add_subregion(table_bar, table_offset, &dev->msix_table_mmio); > > + > > memory_region_init_io(&dev->msix_pba_mmio, &msix_pba_mmio_ops, dev, > > "msix-pba", pba_size); > > + memory_region_add_subregion(pba_bar, pba_offset, &dev->msix_pba_mmio); > > > > - dev->msix_entries_nr = nentries; > > - ret = msix_add_config(dev, nentries, bar_nr, bar_size); > > - if (ret) > > - goto err_config; > > - > > - dev->cap_present |= QEMU_PCI_CAP_MSIX; > > - msix_mmio_setup(dev, bar); > > - return 0; > > + ret = msix_add_config(dev, nentries, table_bar_nr, table_offset, > > + pba_bar_nr, pba_offset, cap_pos); > > + if (ret) { > > + msix_uninit(dev, table_bar, pba_bar); > > + } > > > > -err_config: > > - dev->msix_entries_nr = 0; > > - memory_region_destroy(&dev->msix_pba_mmio); > > - g_free(dev->msix_pba); > > - dev->msix_pba = NULL; > > - memory_region_destroy(&dev->msix_table_mmio); > > - g_free(dev->msix_table); > > - dev->msix_table = NULL; > > - g_free(dev->msix_entry_used); > > - dev->msix_entry_used = NULL; > > return ret; > > } > > > > @@ -344,7 +340,8 @@ int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries, > > > > free(name); > > > > - ret = msix_init(dev, nentries, &dev->msix_exclusive_bar, bar_nr, 4096); > > + ret = msix_init(dev, nentries, &dev->msix_exclusive_bar, bar_nr, 0, > > + &dev->msix_exclusive_bar, bar_nr, 2048, 0); > > Doesn't this have to be msix_table_mmi and msix_pba_mmio? Nope, msix_exclusive_bar is the container, msix_table_mmio and msix_pba_mmio are the subregions for their respective types. > > if (ret) { > > memory_region_destroy(&dev->msix_exclusive_bar); > > return ret; > > @@ -356,6 +353,14 @@ int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries, > > return 0; > > } > > > > +void msix_uninit_exclusive_bar(PCIDevice *dev) > > +{ > > + if (msix_present(dev)) { > > + msix_uninit(dev, &dev->msix_exclusive_bar, &dev->msix_exclusive_bar); > > Same here. Same as above. > > + memory_region_destroy(&dev->msix_exclusive_bar); > > + } > > +} > > + > > static void msix_free_irq_entries(PCIDevice *dev) > > { > > int vector; > > @@ -366,38 +371,6 @@ static void msix_free_irq_entries(PCIDevice *dev) > > } > > } > > > > -/* Clean up resources for the device. */ > > -int msix_uninit(PCIDevice *dev, MemoryRegion *bar) > > -{ > > - if (!msix_present(dev)) { > > - return 0; > > - } > > - pci_del_capability(dev, PCI_CAP_ID_MSIX, MSIX_CAP_LENGTH); > > - dev->msix_cap = 0; > > - msix_free_irq_entries(dev); > > - dev->msix_entries_nr = 0; > > - memory_region_del_subregion(bar, &dev->msix_pba_mmio); > > - memory_region_destroy(&dev->msix_pba_mmio); > > - g_free(dev->msix_pba); > > - dev->msix_pba = NULL; > > - memory_region_del_subregion(bar, &dev->msix_table_mmio); > > - memory_region_destroy(&dev->msix_table_mmio); > > - g_free(dev->msix_table); > > - dev->msix_table = NULL; > > - g_free(dev->msix_entry_used); > > - dev->msix_entry_used = NULL; > > - dev->cap_present &= ~QEMU_PCI_CAP_MSIX; > > - return 0; > > -} > > - > > -void msix_uninit_exclusive_bar(PCIDevice *dev) > > -{ > > - if (msix_present(dev)) { > > - msix_uninit(dev, &dev->msix_exclusive_bar); > > - memory_region_destroy(&dev->msix_exclusive_bar); > > - } > > -} > > - > > void msix_save(PCIDevice *dev, QEMUFile *f) > > { > > unsigned n = dev->msix_entries_nr; > > diff --git a/hw/msix.h b/hw/msix.h > > index bed6bfb..14b1a2e 100644 > > --- a/hw/msix.h > > +++ b/hw/msix.h > > @@ -4,16 +4,18 @@ > > #include "qemu-common.h" > > #include "pci.h" > > > > -int msix_init(PCIDevice *pdev, unsigned short nentries, > > - MemoryRegion *bar, > > - unsigned bar_nr, unsigned bar_size); > > +int msix_init(PCIDevice *dev, unsigned short nentries, > > + MemoryRegion *table_bar, uint8_t table_bar_nr, > > + 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); > > > > void msix_write_config(PCIDevice *pci_dev, uint32_t address, > > uint32_t val, int len); > > > > -int msix_uninit(PCIDevice *d, MemoryRegion *bar); > > +void msix_uninit(PCIDevice *dev, MemoryRegion *table_bar, > > + MemoryRegion *pba_bar); > > void msix_uninit_exclusive_bar(PCIDevice *dev); > > > > unsigned int msix_nr_vectors_allocated(const PCIDevice *dev); > > > > Interfaces look good to me, the logic as well - except for the few remarks. Thanks! Alex