From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:37642) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SfCXi-0005U4-Ja for qemu-devel@nongnu.org; Thu, 14 Jun 2012 12:05:12 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SfCXa-0007V6-G6 for qemu-devel@nongnu.org; Thu, 14 Jun 2012 12:05:06 -0400 Received: from mx1.redhat.com ([209.132.183.28]:28224) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SfCXa-0007Uw-8P for qemu-devel@nongnu.org; Thu, 14 Jun 2012 12:04:58 -0400 Date: Thu, 14 Jun 2012 19:05:31 +0300 From: "Michael S. Tsirkin" Message-ID: <20120614160531.GA19270@redhat.com> References: <20120614044751.11034.87619.stgit@bling.home> <20120614045118.11034.11599.stgit@bling.home> <20120614154950.GD18618@redhat.com> <4FDA0966.4040606@siemens.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4FDA0966.4040606@siemens.com> Subject: Re: [Qemu-devel] [PATCH v2 1/6] msix: Add simple BAR allocation MSIX setup functions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jan Kiszka Cc: Alex Williamson , "qemu-devel@nongnu.org" On Thu, Jun 14, 2012 at 05:55:18PM +0200, Jan Kiszka wrote: > On 2012-06-14 17:49, Michael S. Tsirkin wrote: > > On Wed, Jun 13, 2012 at 10:51:19PM -0600, Alex Williamson wrote: > >> msi_init() takes over a BAR without really specifying or allowing > >> specification of how it does so. Instead, let's split it into > >> two interfaces, one fully specified, and one trivially easy. This > >> implements the latter. msix_init_exclusive_bar() takes over > >> allocating and filling a PCI BAR _exclusively_ for the use of MSIX. > >> When used, the matching msi_uninit_exclusive_bar() should be used > >> to tear it down. > >> > >> Signed-off-by: Alex Williamson > >> --- > >> > >> hw/msix.c | 43 +++++++++++++++++++++++++++++++++++++++++++ > >> hw/msix.h | 3 +++ > >> hw/pci.h | 2 ++ > >> 3 files changed, 48 insertions(+) > >> > >> diff --git a/hw/msix.c b/hw/msix.c > >> index b64f109..a4cdfb0 100644 > >> --- a/hw/msix.c > >> +++ b/hw/msix.c > >> @@ -299,6 +299,41 @@ err_config: > >> return ret; > >> } > >> > >> +int msix_init_exclusive_bar(PCIDevice *dev, unsigned short nentries, > >> + uint8_t bar_nr) > >> +{ > >> + int ret; > >> + char *name; > >> + > >> + /* > >> + * Migration compatibility dictates that this remains a 4k > >> + * BAR with the vector table in the lower half and PBA in > >> + * the upper half. > >> + */ > >> + if (nentries * PCI_MSIX_ENTRY_SIZE > 2048) { > >> + return -EINVAL; > >> + } > >> + > >> + if (asprintf(&name, "%s MSIX BAR", dev->name) == -1) { > >> + return -ENOMEM; > >> + } > >> + > >> + memory_region_init(&dev->msix_exclusive_bar, name, 4096); > >> + > >> + free(name); > >> + > >> + ret = msix_init(dev, nentries, &dev->msix_exclusive_bar, bar_nr, 4096); > >> + if (ret) { > >> + memory_region_destroy(&dev->msix_exclusive_bar); > >> + return ret; > >> + } > >> + > >> + pci_register_bar(dev, bar_nr, PCI_BASE_ADDRESS_SPACE_MEMORY, > >> + &dev->msix_exclusive_bar); > >> + > >> + return 0; > >> +} > >> + > >> static void msix_free_irq_entries(PCIDevice *dev) > >> { > >> int vector; > >> @@ -329,6 +364,14 @@ int msix_uninit(PCIDevice *dev, MemoryRegion *bar) > >> 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 e5a488d..bed6bfb 100644 > >> --- a/hw/msix.h > >> +++ b/hw/msix.h > >> @@ -7,11 +7,14 @@ > >> int msix_init(PCIDevice *pdev, unsigned short nentries, > >> MemoryRegion *bar, > >> unsigned bar_nr, unsigned bar_size); > >> +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_exclusive_bar(PCIDevice *dev); > >> > >> unsigned int msix_nr_vectors_allocated(const PCIDevice *dev); > >> > >> diff --git a/hw/pci.h b/hw/pci.h > >> index 4c96268..d517a54 100644 > >> --- a/hw/pci.h > >> +++ b/hw/pci.h > >> @@ -226,6 +226,8 @@ struct PCIDevice { > >> > >> /* Space to store MSIX table */ > >> uint8_t *msix_table_page; > >> + /* MemoryRegion container for msix exclusive BAR setup */ > >> + MemoryRegion msix_exclusive_bar; > > > > Wrappers are good but this doesn't belong in PCIDevice IMO. > > E.g. when I debug and look at data structure I don't want to dig into > > code and go "oh this device uses an exclusive bar to it's here, that one > > uses a non exlcusive so the field is invalid, the real bar is over > > there. > > > > Keep the region in devices where it was, pass to msix_init_exclusive_bar > > by parameter. > > I disagree. There are plenty of fields in PCIDevice that are only used > under certain circumstances. This is just +1, and it makes the interface > usage much easier. > > Jan *Much* easier? It's one parameter less. > -- > Siemens AG, Corporate Technology, CT T DE IT 1 > Corporate Competence Center Embedded Linux