On 12/07/12 15:29, Alex Williamson wrote: > On Thu, 2012-07-12 at 14:58 +1000, Alexey Kardashevskiy wrote: >> On 12/07/12 14:43, Alex Williamson wrote: >>> 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. >> >> There are 2 level of intrusion. >> >> 1. Fix PCIINTxRoute to return the GSI's qemu_irq as well. > > Slightly confusing because pdev->irq[] is a qemu_irq, but you want the > actual ioapic/pic/xics qemu_irq w/o walking through the various devices, > correct?
Yes. The qemu_irq which corresponds to the GSI which pci_get_irq is returning. > I'm not sure what we do once we have it though. Do we get to > call something like the function outlined above on these "special" > qemu_irqs? They are not special but just "global". This is what hw/pc_piix.c allocates with qemu_allocate_irqs(). Assuming we have properly initialized add_eoi_notifier() callback in the qemu_irq struct, we can easily add a notifier via this callback. Or I did not get the whole idea. > >> 2. Add add_eoi_notifier to all levels including PCI. As a part of this, we >> will have to add this >> callback to all pci_register_bus() calls to reach global interrupts via >> platform-specific PCI bus. > > Just like the PCI INTx route callback, most of these can just be > passthrough. We just need to get to the end qemu_irq that registered a > real add notifier. That might make it possible to do it w/o interfering > too much with other callers, I hope. Yes. This is why I propose to extend the PCIINTxRoute struct. Actually even adding a callback into QEMUMachine is not that bad idea. If a pointer to the struct QEMUMachine was passed into QEMUMachineInitFunc(), it would be the right place to init such callback, one per machine but not per every qemu_irq as it is the same for the whole machine and will not change. >> I would stay with 1). Is that bad? > > It still seems to present a rather large incongruity, but if we're > planning to cache the qemu_irq there anyway, maybe it's a secondary use. Cannot see how it is different from having pci_get_irq() or pci_route_irq_fn() though. -- Alexey