Re: [Qemu-devel] [PATCH 0/2] RFC: powerpc-vfio: adding support

[Alex Williamson <alex.williamson@redhat.com>]

On Thu, 2012-07-12 at 14:38 +1000, Alexey Kardashevskiy wrote:
> On 12/07/12 14:31, Alex Williamson wrote:
> > On Thu, 2012-07-12 at 14:16 +1000, Alexey Kardashevskiy wrote:
> >> On 12/07/12 12:54, Alex Williamson wrote:
> >>> On Wed, 2012-07-11 at 12:25 +1000, Alexey Kardashevskiy wrote:
> >>>> On 11/07/12 02:57, Alex Williamson wrote:
> >>>>> On Tue, 2012-07-10 at 15:51 +1000, Alexey Kardashevskiy wrote:
> >>>>>> The two patches in this set are supposed to add VFIO support for POWER.
> >>>>>>
> >>>>>> The first one adds one more step in the initalizaion sequence which I am not
> >>>>>> sure is correct.
> >>>>>>
> >>>>>> The second patch adds actual VFIO support. It is not ready to submit but
> >>>>>> ready to discuss. I would like to get rid of all #ifdef TARGET_PPC64 in patch #2
> >>>>>> and I wonder if there is any plan to implement some generic EOI support code, etc.
> >>>>>
> >>>>> A generic EOI notifier is on my todo list, but I have no idea what it's
> >>>>> going to look like.  As you know, I've got an ioapic specific notifier
> >>>>> in my tree, you add a spapr specific one.  I welcome ideas on how to
> >>>>> create something generic that has a chance of being accepted.  Thanks,
> >>>>
> >>>>
> >>>> So far the only platform specific call is xxxx_add_gsi_eoi_notifier. The
> >>>> xxxx_remove_gsi_eoi_notifier only calls notifier_remove, you've got to fix yours
> >>>> ioapic_remove_gsi_eoi_notifier() as it does too much :)
> >>>>
> >>>>
> >>>> The only place for placing "add_eoi" callback I can see right now is QEMUMachine as there is no
> >>>> unified machine interrupt controller - IOAPIC has its own type TYPE_IOAPIC_COMMON and XICS is not
> >>>> even a SysBusDevice. And the callback is not specific for any kind of bus so it cannot go to PCIBus.
> >>>>
> >>>> Does it sound reasonable?
> >>>
> >>> I suspect we'd need to somehow tie it into qemu_irq where both handlers
> >>> and notifiers are allocated so we don't really care the underlying
> >>> implementation.  Something like qemu_add_irq_eoi_notifier(qemu_irq
> >>> irq, ...).  It's another mess like adding the PCIBus interrupt line to
> >>> gsi effort though.  Thanks,
> >>
> >>
> >> Tried. Added add_eoi_notifier() callback to qemu_irq, new IRQ allocator:
> >> qemu_irq *qemu_allocate_irqs2(qemu_irq_handler handler, void *opaque, int n,
> >>                               qemu_eoi_add_notifier add_notifier);
> >> and called it from the XICS initialization code.
> >>
> >> It could work out if pci_get_irq() or pci_route_irq_fn() returned qemu_irq but no, they just return
> >> a global IRQ number (pure or embedded in a struct) and there is no common way to resolve qemu_irq
> >> (and then add_eoi_notifier()) from that number within vfio_pci.
> > 
> > Well GSI and qemu_irq are different address spaces.  We still need GSI
> > for any kind of qemu bypass case.
> 
> No, that is ok, we also need GSI because XICS and IOAPIC need it in the end.
> 
> >> May be we could add the callback pointer into PCIINTxRoute?
> > 
> > Maybe, but why is this PCI specific?  Can't we call it as
> > qemu_add_irq_eoi_notifier(pdev->irq[0], Notifier)?  That would work much
> > like qemu_set_irq, extracting the irq number from the IRQState and
> > passing it through to the add_notifier callback for IRQState until it
> > got to the ioapic/pic/xics.
> > 
> > int qemu_add_irq_eoi_notifier(qemu_irq *irq, Notifier notifier)
> > {
> >     if (!irq || !irq->add_eoi_notifier)
> >         return -1;
> > 
> >    return irq->add_eoi_notifier(irq->opaque, irq->n, notifier);
> > }
> > 
> 
> Then we will have to entirely replace qemu_allocate_irqs() with qemu_allocate_irqs2() and pass some
> non-zero add_eoi_notifier() on every level, at least for PCI for now. I would like to avoid that if
> possible - hard to get accepted :)

Yep, that's why I said it was the same kind of mess as the PCIBus intx
routing.  It's intrusive, but qemu_irq is the common interrupt model so
we need to make use of it.  A callback on QEMUMachine seems completely
random.  Thanks,

Alex