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

[Michael S. Tsirkin]

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.

Yes that's fine.

> > 
> >>>
> >>>> diff --git a/hw/pci.h b/hw/pci.h
> >>>> index 5b54e2d..bbba01e 100644
> >>>> --- a/hw/pci.h
> >>>> +++ b/hw/pci.h
> >>>> @@ -141,6 +141,15 @@ enum {
> >>>>  #define PCI_DEVICE_GET_CLASS(obj) \
> >>>>       OBJECT_GET_CLASS(PCIDeviceClass, (obj), TYPE_PCI_DEVICE)
> >>>>  
> >>>> +typedef struct PCIINTxRoute {
> >>>> +    enum {
> >>>> +        PCI_INTX_ENABLED,
> >>>> +        PCI_INTX_INVERTED,
> >>>> +        PCI_INTX_DISABLED,
> >>>> +    } mode;
> >>>> +    int irq;
> >>>> +} PCIINTxRoute;
> >>>
> >>> Is this INTX route or IRQ route?
> >>> Is the INTX enabled/disabled/inverted or the IRQ?
> >>>
> >>> I have the impression it's the IRQ, in the apic.
> >>> PCI INTX are never inverted they are always active low.
> >>
> >> This should be considered as "the route *of* an INTx", not "to some
> >> IRQ". I could call it PCIINTxToIRQRoute if you prefer, but it's a bit
> >> lengthy.
> >>
> >> Jan
> > 
> > Yes but the polarity is in apic? Or is it in host bridge?
> > 
> 
> Nope (then we would not have to bother). At least one host bridge
> (bonito) is apparently able to invert the polarity.
> 
> Jan
>