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

[Michael S. Tsirkin]

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.


> -- 
> Siemens AG, Corporate Technology, CT T DE IT 1
> Corporate Competence Center Embedded Linux