From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:47538) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SfCds-00080J-UB for qemu-devel@nongnu.org; Thu, 14 Jun 2012 12:11:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SfCdm-0001PU-9X for qemu-devel@nongnu.org; Thu, 14 Jun 2012 12:11:28 -0400 Received: from thoth.sbs.de ([192.35.17.2]:19577) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SfCdl-0001Or-Vp for qemu-devel@nongnu.org; Thu, 14 Jun 2012 12:11:22 -0400 Message-ID: <4FDA0D26.5010000@siemens.com> Date: Thu, 14 Jun 2012 18:11: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> <4FDA0966.4040606@siemens.com> <20120614160531.GA19270@redhat.com> In-Reply-To: <20120614160531.GA19270@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 18:05, Michael S. Tsirkin wrote: > 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. ...and one field to add to every user of msix_init_exclusive_bar while those users have no business with it. The container is a pure internal field, so it belongs to the infrastructure. Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux