Re: [Qemu-devel] [PATCH 1/2] pci: Add pci_device_get_host_irq

[Jan Kiszka <jan.kiszka@siemens.com>]

On 2012-06-01 15:27, Michael S. Tsirkin wrote:
> On Fri, Jun 01, 2012 at 02:52:56PM +0200, Jan Kiszka wrote:
>> On 2012-05-30 22:31, Michael S. Tsirkin wrote:
>>>>> So we'll just have PIIX_NUM_PIC_IRQS entries there and use
>>>>> irq_count instead of the pic_levels bitmap.
>>>>
>>>> Just that this affects generic PCI code, not only PIIX-specific things.
>>>
>>> Yes but it's not a problem - pci_bus_irqs sets the map function and nirqs.
>>>
>>>> And that we need to save/restore some irq_count field according to the
>>>> old semantics.
>>>
>>> Well, it's a bug: this is redundant info we should not have exposed it.
>>>
>>> Anyway, let's make the rest work properly and cleanly first, add a FIXME
>>> for now, then we'll find a hack making it work for migration.
>>
>> It remains non-trivial: I got your patch working (a minor init issue),
>> but yet without changing the number of IRQs for PIIX3, so keeping the
>> irq_count semantics for this host bridge.
>>
>> Now I'm facing three possibilities of how to proceed:
> 
> They all look OK I think :) Some comments below.
> 
>> 1. Give up on the (currently broken) feature to write a vmstate for
>>    older QEMU versions.
>>
>>    This will allow to declare the irq_count field in vmstate_pcibus
>>    unused, and we would have to restore it on vmload step-wise via the
>>    PCI devices. It would also allow to change its semantics for PIIX3,
>>    mapping directly to PIC IRQs.
> 
> I think that's okay too simply because these things are usually
> easy to fix after the fact when the rest of the issues are addressed.

Don't get what you mean with "fixed". If we fix the vmstate generation
in making it backward-compatible again, we enter option 2. Option 1 is
explicitly about giving this up.

> 
>> 2. Keep writing a legacy irq_count field.
>>
>>    This will require quite a few new APIs so that host bridges that
>>    want to change their nirq can still generate a compatible irq_count
>>    vmstate field. Namely:
>>     - A function to set up vmstate_irq_count and define a callback that
>>       the core will invoke to prepare the vmstate_irq_count before
>>       vmsave.
>>     - A function to obtain the IRQ mapping /without/ the final host
>>       bridge step. This is required so that the callback above can
>>       calculate the old state like in the PIIX3 case.
> 
> Does this really need to be so complex? It seems that we just need
> pci_get_irq_count(bus, irq) which can use the existing map_irq API, no?
> Then invoke that before save.

No, because the new map_irq of the PIIX3 bridge will also include the
host bridge routing (or masking) according to the PIRQx routoing
registers of the PIIX3. Moreover, the fixup of the written legacy
irq_count state has to happen in the PCI layer, which therefore has to
query the host bridge for fixup information, not the other way around
(because the PCI bus vmstate is separate from the PIIX3 host bridge).

> 
>> 3. Keep irq_count and nirq as is, introduce additional map_host_irq.
>>
>>    This is simpler than 2 and more compatible than 1. It would also
>>    allow to introduce the polarity and masking information more
>>    smoothly as we won't have to add it to existing map_irq callbacks
>>    then.
> 
> So what does it map, and to what?

PCI bus IRQ to *host* IRQ. It is supposed to explore the routing of the
host bridge between the root bus and the host's interrupt controller
(i.e. the step that is currently missing the cached chain).

> Maybe we can make the name imply that somehow.

Better suggestions for this handler and maybe also the existing map_irq
are welcome to make the difference clearer.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux