Re: [Qemu-devel] [PATCH 05/13] pci: Add pci_device_route_intx_to_irq

[Alex Williamson <alex.williamson@redhat.com>]

On Sun, 2012-06-10 at 17:43 +0300, Michael S. Tsirkin wrote:
> On Sun, Jun 10, 2012 at 08:19:28AM -0600, Alex Williamson wrote:
> > On Sun, 2012-06-10 at 12:49 +0200, Jan Kiszka wrote:
> > > On 2012-06-10 12:41, Michael S. Tsirkin wrote:
> > > > On Sun, Jun 10, 2012 at 12:08:23PM +0200, Jan Kiszka wrote:
> > > >> On 2012-06-10 11:55, Michael S. Tsirkin wrote:
> > > >>> On Thu, Jun 07, 2012 at 06:46:38PM +0200, Jan Kiszka wrote:
> > > >>>> On 2012-06-07 18:28, Michael S. Tsirkin wrote:
> > > >>>>> On Thu, Jun 07, 2012 at 05:10:17PM +0200, Jan Kiszka wrote:
> > > >>>>>> On 2012-06-07 16:32, Michael S. Tsirkin wrote:
> > > >>>>>>> On Mon, Jun 04, 2012 at 10:52:13AM +0200, Jan Kiszka wrote:
> > > >>>>>>>> @@ -1089,6 +1093,14 @@ static void pci_set_irq(void *opaque, int irq_num, int level)
> > > >>>>>>>>      pci_change_irq_level(pci_dev, irq_num, change);
> > > >>>>>>>>  }
> > > >>>>>>>>  
> > > >>>>>>>> +PCIINTxRoute pci_device_route_intx_to_irq(PCIDevice *dev, int pin)
> > > >>>>>>>> +{
> > > >>>>>>>> +    PCIBus *bus = dev->host_bus;
> > > >>>>>>>> +
> > > >>>>>>>> +    assert(bus->route_intx_to_irq);
> > > >>>>>>>> +    return bus->route_intx_to_irq(bus->irq_opaque, dev->host_intx_pin[pin]);
> > > >>>>>>>> +}
> > > >>>>>>>> +
> > > >>>>>>>>  /***********************************************************/
> > > >>>>>>>>  /* monitor info on PCI */
> > > >>>>>>>>  
> > > >>>>>>>
> > > >>>>>>> Just an idea: can devices cache this result, bypassing the
> > > >>>>>>> intx to irq lookup on data path?
> > > >>>>>>
> > > >>>>>> That lookup is part of set_irq which we don't bypass so far and where
> > > >>>>>> this is generally trivial. If we want to cache the effects of set_irq as
> > > >>>>>> well, I guess things would become pretty complex (e.g. due to vmstate
> > > >>>>>> compatibility), and I'm unsure if it would buy us much.
> > > >>>>>
> > > >>>>> This is less for performance but more for making
> > > >>>>> everyone use the same infrastructure rather than
> > > >>>>> assigned devices being the weird case.
> > > >>>>
> > > >>>> Device assignment is weird. It bypasses all state updates as it does not
> > > >>>> have to bother about migratability.
> > > >>>>
> > > >>>> Well, of course we could cache the host bridge routing result as well,
> > > >>>> for every device. It would have to be in addition to host_intx_pin. But
> > > >>>> the result would look pretty strange to me.
> > > >>>>
> > > >>>> In any case, I would prefer to do this, if at all, on top of this
> > > >>>> series, specifically as it will require to touch all host bridges.
> > > >>>
> > > >>> I'd like to ponder this a bit more then.
> > > >>>
> > > >>> If the claim is that device assignment is only needed for
> > > >>> piix anyway, then why not make it depend on piix *explicitly*?
> > > >>> Yes ugly but this will make it very easy to find and
> > > >>> address any missing pieces.
> > > >>
> > > >> Because it is conceptually independent of the PIIX, we will need it for
> > > >> successors of that x86 chipset as well, and I won't add the ugly hack of
> > > >> qemu-kvm upstream
> > > > 
> > > > So you look at an API and see it requires a route
> > > > callback. And you ask "why doesn't chipset X implement it"?
> > > > And the answer is "because it's only used by device assignment".
> > > > Which you will only know if you read this thread. So it's
> > > > a hack. And I'd rather have the hacks in device-assignment.c
> > > > than in pci.c even if the former are nastier.
> > > 
> > > I don't share your view on this. It is surely _not_ a hack, specifically
> > > when compared to what we have so far and what could be done otherwise,
> > > e.g. hacking device-assignment and PIIX to make them cooperate (sorry, I
> > > would vote against such an attempt). This is just a partially used
> > > generic API. Any chipset not providing the required routing function
> > > will cause an assert once some tries to make use of it.
> > 
> > I agree, and frankly it sounds like a rather biased attitude against
> > device assignment.
> > piix and q35 may be the only initial chipsets to
> > implement this, but VFIO is designed to be architecture neutral.  It's
> > already working on POWER.
> 
> Not with this patch it doesn't.

Alexey, how are you handling INTx routing and EOI on POWER?

> >  Interrupt routing is one of the most
> > intrusive parts because we cannot know from inside the device assignment
> > code how the chipset exposes intx out to an irq.  Jan's interface makes
> > it easy for a chipset to add this, versus hacks in device assignment
> > code, even if they were possible.  If it needs to be tweaked for other
> > chipsets, we'll do that when they come.  Please don't roadblock device
> > assignment or VFIO support by not allowing us a well architected,
> > generic interface to track interrupts.  Thanks,
> > 
> > Alex
> 
> Problems are:
> 1. This sticks NULL in all chipsets except one. This means it's
>    hard to find and fill out.

I don't buy that.  Look for all the callers, find the one that's not
NULL, look at what it does...  How's that a problem?  This can be solved
by documentation.

> 2. Adds a function, in every chipset, that is only used by assignment.
>    This means many maintainers have no way to test it.

It's effectively optional, so they don't have to implement it.  They can
wait to care about it until they want device assignment.  This can also
be solved by documentation so the maintainer knows when and how this is
used and can choose to implement it or not.

> Ways to handle this that came up so far, in order of my preference:
> 1. Cache all of route in devices.

How do we get the route to cache it?

> 2. Some ugly hack in device assignment.

We still have the problem of how do we get the route to start with?

> 3. Merge as is and fix it when it gets broken.

Isn't that how open source works? ;^)

> So if you expect me to merge this work, then either Jan does (1), or
> gives up and does (2), or I find some time and do (1), or I fail to do
> (1) and get convinced that we need to do (3). Or someone else gets
> involved.

I still have trouble seeing how (3) is a problem.  The translation of an
INTx to an irq is chipset specific.  We need a chipset function to
perform that transform.  We don't know how to do this for every chipset
in the tree, nor do many of those chipset care at the moment about
device assignment.  Jan has created an arrangement where chipsets can
easily opt-in to this support.  Aside from asking us to go spend a month
digging up specs for every chipset in tree and trying to implement this
ourselves, what kind of enhancement to the interface are you looking
for?  Thanks,

Alex