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

[Alex Williamson]

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