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

[Michael S. Tsirkin]

On Wed, Jun 13, 2012 at 10:48:31AM -0600, Alex Williamson wrote:
> 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.

The nice thing overlapping regions work fine now.
So we can have both a region in device and a region in mmio.
For currectness it's enough to have msix_mmio subregion,
probably split to msix_mmio_vectors and msix_mmio_pba.

But was all talking about msix_init. I'm fine with wrappers
that simplify things.

> > 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

Fair enough.

-- 
MST