From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:55945) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SelfB-0002Ro-Rh for qemu-devel@nongnu.org; Wed, 13 Jun 2012 07:23:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SelfA-00013L-4X for qemu-devel@nongnu.org; Wed, 13 Jun 2012 07:23:01 -0400 Received: from david.siemens.de ([192.35.17.14]:16127) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Self9-000131-RC for qemu-devel@nongnu.org; Wed, 13 Jun 2012 07:23:00 -0400 Message-ID: <4FD87810.6080707@siemens.com> Date: Wed, 13 Jun 2012 13:22:56 +0200 From: Jan Kiszka MIME-Version: 1.0 References: <20120612200243.2994.18161.stgit@bling.home> <4FD86EF1.9000305@siemens.com> <20120613112131.GE18001@redhat.com> In-Reply-To: <20120613112131.GE18001@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] msix: Support specifying offsets, BARs, and capability location 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-13 13:21, Michael S. Tsirkin wrote: > On Wed, Jun 13, 2012 at 12:44:01PM +0200, Jan Kiszka wrote: >> On 2012-06-12 22:03, Alex Williamson wrote: >>> msix_init has very little configurability as to how it lays out MSIX >>> for a device. It claims to resize BARs, but doesn't actually do this >>> anymore. This patch allows MSIX to be fully specified, which is >>> necessary both for emulated devices trying to match the physical >>> layout of a hardware device as well as for any kind of device >>> assignment. >>> >>> New functions msix_init_bar & msix_uninit_bar provide wrappers around >>> the more detailed functions for drivers that just want a simple MSIX >>> setup. >>> >>> Signed-off-by: Alex Williamson >>> --- >>> >>> hw/ivshmem.c | 9 +- >>> hw/msix.c | 299 +++++++++++++++++++++++++++++++------------------------ >>> hw/msix.h | 11 +- >>> hw/pci.h | 12 ++ >>> hw/virtio-pci.c | 15 +-- >>> 5 files changed, 192 insertions(+), 154 deletions(-) >>> >>> diff --git a/hw/ivshmem.c b/hw/ivshmem.c >>> index 05559b6..71c84a6 100644 >>> --- a/hw/ivshmem.c >>> +++ b/hw/ivshmem.c >>> @@ -563,16 +563,13 @@ static uint64_t ivshmem_get_size(IVShmemState * s) { >>> >>> static void ivshmem_setup_msi(IVShmemState * s) >>> { >>> - memory_region_init(&s->msix_bar, "ivshmem-msix", 4096); >>> - if (!msix_init(&s->dev, s->vectors, &s->msix_bar, 1, 0)) { >>> - pci_register_bar(&s->dev, 1, PCI_BASE_ADDRESS_SPACE_MEMORY, >>> - &s->msix_bar); >>> - IVSHMEM_DPRINTF("msix initialized (%d vectors)\n", s->vectors); >>> - } else { >>> + if (msix_init_bar(&s->dev, s->vectors, &s->msix_bar, 1, "ivshmem-msix")) { >> >> I don't think the callers of msix_init_bar should have to provide the >> memory region for that bar. That can be embedded into PCIDevice, just >> like you did for the table and PBA. That was my idea with msix_init_simple. >> >> Back then, I only included a generic memory region name. That can be >> improved, but without bothering the caller. Just derive it from >> PCIDevice::name. >> >> Jan > > I think callers must initialize the BAR regions. > This is because BAR can include other stuff besides MSI-X. > MSI-X adds its own subregion. That's the non-common case handled by msix_init. I don't see this as typical for emulated devices. Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux