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

[Michael S. Tsirkin]

On Wed, May 30, 2012 at 07:29:52PM +0200, Jan Kiszka wrote:
> On 2012-05-30 19:20, Michael S. Tsirkin wrote:
> > On Wed, May 30, 2012 at 06:46:03PM +0200, Jan Kiszka wrote:
> >> On 2012-05-30 18:17, Michael S. Tsirkin wrote:
> >>> On Wed, May 30, 2012 at 05:47:45PM +0200, Jan Kiszka wrote:
> >>>> On 2012-05-30 16:59, Michael S. Tsirkin wrote:
> >>>>> On Wed, May 30, 2012 at 04:46:14PM +0200, Jan Kiszka wrote:
> >>>>>> On 2012-05-30 16:42, Michael S. Tsirkin wrote:
> >>>>>>> On Wed, May 30, 2012 at 04:38:25PM +0200, Jan Kiszka wrote:
> >>>>>>>> On 2012-05-30 15:34, Michael S. Tsirkin wrote:
> >>>>>>>>> Below's as far as I got - hopefully this
> >>>>>>>>> explains what I had in mind: for deep hierarchies
> >>>>>>>>> this will save a bus scan so I think this makes sense
> >>>>>>>>> even in the absense of kvm irqchip.
> >>>>>>>>>
> >>>>>>>>> Warning: completely untested and known to be incomplete.
> >>>>>>>>> Please judge whether this is going in a direction that
> >>>>>>>>> is helpful for your efforts. If you respond in the positive,
> >>>>>>>>> I hope to be able to get back to this next week.
> >>>>>>>>
> >>>>>>>> We need to
> >>>>>>>>  - account for polarity changes along the route
> >>>>>>>>  - get rid of irq_count as it is no longer updated in the fast path,
> >>>>>>>>    thus useless on routing updates
> >>>>>>>
> >>>>>>> I'll need to consider this more to understand what you mean here.
> >>>>>>
> >>>>>> If, e.g., the host bridge is configured to flip the polarity of some
> >>>>>> interrupt on delivery, the fast path must be able to explore this in
> >>>>>> order to do the same.
> >>>>>
> >>>>> This can be solved incrementally I think - handle polarity
> >>>>> change like mapping change, no?
> >>>>
> >>>> Both belong together if we want to do it generically, IMHO.
> >>>
> >>> So irq_polarity callback like we have for map_irq ATM?  But at least pci
> >>> bridges never do this flip.  Maybe this is the property of the interrupt
> >>> controller after all?
> >>
> >> It is a property of some host bridges apparently (e.g. bonito).
> > 
> > So I'm not sure it's worth it to abstract that but I don't mind either.
> 
> It is must for a generic solution. We cannot modeling after a single
> host bridge called PIIX3.
> 
> > 
> >> In
> >> theory, someone could also add a PCI-PCI bridge that does this (or does
> >> the spec disallow it?).
> > 
> > It seems to disallow it.
> > 
> >>>
> >>>>>
> >>>>>> Then you may want to have a look at how irq_count is maintained and 
> >>>>>> when
> >>>>>> it is used.
> >>>>>
> >>>>> In my patch it simply there to OR irq levels of all devices
> >>>>> connected to a specific pin.
> >>>>
> >>>> I think what we want is to avoid any walks and intermediate state
> >>>> updates for all IRQ deliveries.
> >>>
> >>> Well the bus is not an intermediate state ATM as piix
> >>> only has a bit per IRQ so it can't OR devices together.
> >>> So you want to move the counter over to piix?
> >>
> >> irq_count can't be moved logically as it is part of the vmstate. But it
> >> should only be generated for saving the state by polling all devices
> >> (and bridges).
> >>
> >> For that we need is an optional callback for devices via which we can
> >> ask them to update PCIDevice::irq_state in case they don't trigger
> >> pci_set_irq regularly.
> > 
> > Let's worry about migration compatibility separately.
> > 
> >> And pci_set_irq could simply change the input
> >> line of the IRQ controller according to the cached route and polarity
> >> mapping.
> > 
> > But the line is shared between multiple devices.
> > You need to perform a logical OR function between them all,
> > this is what the counter does.
> 
> Needs to be solved at a different level (at the final output of the fast
> path, surely not per PCI bus).
> 
> Jan

Well the final output is in kvm :) What happens with assigned devices is
that kvm performs the logical OR using source id.  But the number of
available source IDs is limited (up to BIT_PER_LONG currently).

So it's not reasonable to allocate one per emulated device.

So assigned devices will have to be special-cased somehow.
Given that, is there really a point in moving these bits
around?

Well, I'll stop ranting here.  The patch that I sent is not intrusive
and simply gives you a simple way to implement pci_device_get_host_irq,
also optimizing emulated devices somewhat.  So if you think you need
pci_device_get_host_irq I think this is a reasonable way to support
that.  But if you changed your mind, I don't mind.

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