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 <alex.william...@redhat.com>
> > >>> ---
> > >>>
> > >>> 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?
--
MST
