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

[Michael S. Tsirkin]

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.

> > 
> > As it is you are adding APIs that in theory address
> > non PIIX issues but in practice don't implement for non
> > PIIX. So we never really know.
> 
> I once hacked q35 to make it work with device assignment. This really
> requires something like this.
 
Yes I'm aware of this motivation. This does not do much to
address the concerns though.

> It actually requires something generic,
> independent of PCI, but that's too much for this round.
> 
> Jan

And this just makes the concerns worse :(

-- 
MST