Re: [Qemu-devel] [PATCH] msix: Support specifying offsets, BARs, and capability location

[Alex Williamson]

On Wed, 2012-06-13 at 18:28 +0300, Michael S. Tsirkin wrote:
> 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 <address@hidden>
> > > >>> ---
> > > >>>
> > > >>>  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?

That's easy enough, but do we still have a question of who initializes
the BAR?  From you previous msg:

On Wed, 2012-06-13 at 17:14 +0300, Michael S. Tsirkin wrote:
> One thing to consider is that things like bar size
> can not change across versions without breaking e.g.
> migration. Keeping them in one place makes it easier
> to keep them consistent.

Yes, that's accounted for in msix_init_bar(); maintaining a 4k BAR split
between vector table and PBA.  This seems to be an argument for keeping
the MemoryRegion allocation in msix_init_bar() as it then all lives in a
single place and devices can't break migration because of it.

> A good API will let device query things like required
> MSIX bar size, but then let the device use that.

It seems like we have two users: 1) those that know everything about
MSIX and know exactly where they want it 2) those that don't really want
to know anything about it and just want a simple interface.  Who are the
users of this query interface?  It makes me uncomfortable to have
something in between?  Either the caller can tell us precisely what they
want or we should handle the whole thing.  Thanks,

Alex