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

[Alexey Kardashevskiy]

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