Re: [Qemu-devel] [PATCH 2/2] vfio-powerpc: added VFIO support

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Wed, 2012-07-11 at 12:54 +1000, Alexey Kardashevskiy wrote:
> > Why do you need this, aren't the extension checks sufficient for this to
> > be a nop for you?
> 
> 
> It uses ioapic_remove_gsi_eoi_notifier() so it needs some #ifdef anyway. And as we do not support
> kvm_irqchip_in_kernel(), there is no point in fixing it and I disabled it all.
> When we make eoi notifiers a platform independent, then yes, it will be nop.

In fact we have an internal experimental patch to move our
PIC emulation into the kernel but so far I have not managed
to make it fit in the existing irqchip stuff.

That irqchip interface is nasty. It's completely x86 centric,
and have tendrils all over the place, into the msi code, into
devices (virtio-pci.c) etc... in ways that are essentially unusable for
anything that looks a bit different.

IE. In urgent need of refactoring.

> >> -
> >> +#ifndef TARGET_PPC64
> >>      vdev->msi_vectors[vector].virq = kvm_irqchip_add_msi_route(kvm_state, msg);
> >>      if (vdev->msi_vectors[vector].virq < 0 || 
> >>          kvm_irqchip_add_irqfd(kvm_state, fd,
> >> @@ -551,7 +575,11 @@ static int vfio_msix_vector_use(PCIDevice *pdev,
> >>          qemu_set_fd_handler(fd, vfio_msi_interrupt, NULL,
> >>                              &vdev->msi_vectors[vector]);
> >>      }
> >> -
> >> +#else
> >> +    vdev->msi_vectors[vector].virq = -1;
> >> +    qemu_set_fd_handler(fd, vfio_msi_interrupt, NULL,
> >> +                        &vdev->msi_vectors[vector]);
> >> +#endif
> > 
> > This shouldn't be necessary once the abort is removed from
> > kvm_irqchip_add_msi_route.  It'll be merged next time the kvm uq tree
> > merges into qemu.

It must also return an irq number, what to chose in that case ? Ie. the
whole irqchip API is a trainwreck if you ask me :-) Very poorly thought
out.

> True, I just did not pick up your very last changes. Updating is always painful, and now it is even
> worse then usual as pci_get_irq has been renamed to something else :) Will do though.

 .../...

> Well I probably can add MemoryListener for the DMA window and move all power-specific map/unmap code
> to VFIO but it does not look much better. I would rather prefer separating IOMMU code from vfio_pci
> somehow (more or less as it is now for powerpc). While doing it, we could think of the API to get
> this fd which we need anyway in order to setup the DMA window which is per group (which QEMU does
> not understand) but not per device.

Right. What we really want is our own private iommu interface based on
the iommu type. Each iommu will have it's own "quirks" in that regard
anyways.

Cheers,
Ben.

> 
> 
> > Alex
> > 
> >> diff --git a/hw/vfio_pci.h b/hw/vfio_pci.h
> >> index 226607c..0d13341 100644
> >> --- a/hw/vfio_pci.h
> >> +++ b/hw/vfio_pci.h
> >> @@ -105,4 +105,6 @@ typedef struct VFIOGroup {
> >>  #define VFIO_FLAG_IOMMU_SHARED_BIT 0
> >>  #define VFIO_FLAG_IOMMU_SHARED (1U << VFIO_FLAG_UIOMMU_SHARED_BIT)
> >>  
> >> +int vfio_get_container_fd(struct PCIBus *pbus);
> >> +
> >>  #endif /* __VFIO_H__ */
> > 
> > 
> > 
> 
> 
> -- 
> Alexey
> 
> 
>