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

[Michael S. Tsirkin]

On Fri, Jun 01, 2012 at 05:34:14PM +0300, Michael S. Tsirkin wrote:
> 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?

A small clarification.
What I tried to say above is that pci bus does not
route irqs. pci bridge routes irqs: either host or
pci to pci bridge.

But as long as we thought the functionality is roughly
the same it made sense to say that bus has this
function simply because both bridge types have a bus.

But if not then the clean thing is a callback in p2p bridge
and another one in a host bridge. Having both these
and one in the bus looks weird.

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