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

[Michael S. Tsirkin]

On Wed, Jun 13, 2012 at 06:38:50AM -0600, Alex Williamson wrote:
> On Wed, 2012-06-13 at 15:25 +0300, Michael S. Tsirkin wrote:
> > On Wed, Jun 13, 2012 at 01:22:56PM +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.
> > > 
> > > Jan
> > 
> > Devices create other bars, easier to let them create msix bar as well,
> > and easier to figure out what's going on.
> 
> It's easier for a device that doesn't know anything about MSIX to know
> to create a 4k BAR and pass it in? It's easier to know what's going on
> when we give a device more opportunities to break it?  If they create it
> then we have to worry about the size and have to consider what if they
> try to use part of it for something themselves.  IMHO, that's an uglier
> API and leads to a more complicated "simple" init.  Neither virtio-pci
> nor ivshmem care about these details, and with this, they just pick a
> BAR, provide a MemoryRegion and everything else is done.  Thanks,
> 
> Alex

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.

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

-- 
MST