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

[Michael S. Tsirkin]

On Fri, Jun 01, 2012 at 03:57:01PM +0200, Jan Kiszka wrote:
> 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.

What I really mean is I think I see how 2 can be added without much
pain. So let's focus on 1 for now and worst case we break migration.

> > 
> >> 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

So I won't object to adding a new API but if we do
it properly this won't help compatibility :(

Let's formulate what these do exactly, this will
also help us come up with sensible names.

1. The difference is that pci bridges route interrupt pins. So it gets
interrupt pin on device and returns interrupt pin on connector. All
attributes are standard PCI.  We should remove all mentions of "irq"
really.


2. The pci root (yes it's a host bridge but let's
not use the term host if we can) routes
an interrupt pin on device to a host irq. It can also
do more things like invert polarity.

So yes we can add 2 to piix but we really should
remove 1 from it.

Wrt names - do you object to long names?
How about route_interrupt_pin for 1
and route_interrupt_pin_to_irq for 2?


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