From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:42978) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SepUQ-0006zA-BP for qemu-devel@nongnu.org; Wed, 13 Jun 2012 11:28:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SepUO-0003cl-1E for qemu-devel@nongnu.org; Wed, 13 Jun 2012 11:28:09 -0400 Received: from mx1.redhat.com ([209.132.183.28]:9148) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SepUN-0003ca-PJ for qemu-devel@nongnu.org; Wed, 13 Jun 2012 11:28:07 -0400 Date: Wed, 13 Jun 2012 18:28:39 +0300 From: "Michael S. Tsirkin" Message-ID: <20120613152839.GA21274@redhat.com> References: <20120612200243.2994.18161.stgit@bling.home> <4FD86EF1.9000305@siemens.com> <20120613112131.GE18001@redhat.com> <4FD87810.6080707@siemens.com> <1339590626.24037.92.camel@bling.home> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1339590626.24037.92.camel@bling.home> 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: Alex Williamson Cc: Jan Kiszka , "qemu-devel@nongnu.org" On Wed, Jun 13, 2012 at 06:30:26AM -0600, Alex Williamson wrote: > On Wed, 2012-06-13 at 13:22 +0200, Jan Kiszka wrote: > > 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. > > Exactly, if the caller wants a more complicated layout, msix_init > handles that. msix_init_bar simplifies exactly how most drivers use it > today. Thanks, > > Alex I think I did not explain myself well. I'm fine with wrappers: _bar, _simple, etc. And I like it that you have provided a symmetrical _uninit. Only one thing that worries me is that it is bundled in one patch with extending functionality. Would be better to 1. add _bar wrapper 2-n. switch users one by one n+1. change api of msi_init. Hmm? -- MST