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

[Alexey Kardashevskiy]

Jan, Alex, ping?


On 12/07/12 15:47, Alexey Kardashevskiy wrote:
> 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