From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:33241) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Semfn-00007f-CI for qemu-devel@nongnu.org; Wed, 13 Jun 2012 08:27:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Semff-00015v-UT for qemu-devel@nongnu.org; Wed, 13 Jun 2012 08:27:42 -0400 Received: from mx1.redhat.com ([209.132.183.28]:40716) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Semff-00015c-ME for qemu-devel@nongnu.org; Wed, 13 Jun 2012 08:27:35 -0400 Message-ID: <1339590450.24037.90.camel@bling.home> From: Alex Williamson Date: Wed, 13 Jun 2012 06:27:30 -0600 In-Reply-To: <4FD86EF1.9000305@siemens.com> References: <20120612200243.2994.18161.stgit@bling.home> <4FD86EF1.9000305@siemens.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 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: Jan Kiszka Cc: "qemu-devel@nongnu.org" , "mst@redhat.com" On Wed, 2012-06-13 at 12:44 +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. If it's embedded on PCIDevice, then there are questions around whether drivers using msix_init should use that MemoryRegion for their BAR, then if we have the MemoryRegion for one BAR embedded in PCIDevice, why not all of them? It seems imperfect either way. > 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. Sure, I can move the name into msix_init_bar(). Thanks, Alex