From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:60026) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SfCOP-0004yp-2U for qemu-devel@nongnu.org; Thu, 14 Jun 2012 11:55:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SfCOI-0003z8-Jo for qemu-devel@nongnu.org; Thu, 14 Jun 2012 11:55:28 -0400 Received: from goliath.siemens.de ([192.35.17.28]:21476) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SfCOI-0003yO-92 for qemu-devel@nongnu.org; Thu, 14 Jun 2012 11:55:22 -0400 Message-ID: <4FDA0966.4040606@siemens.com> Date: Thu, 14 Jun 2012 17:55:18 +0200 From: Jan Kiszka MIME-Version: 1.0 References: <20120614044751.11034.87619.stgit@bling.home> <20120614045118.11034.11599.stgit@bling.home> <20120614154950.GD18618@redhat.com> In-Reply-To: <20120614154950.GD18618@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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: "Michael S. Tsirkin" Cc: Alex Williamson , "qemu-devel@nongnu.org" 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 -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux