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

[Jan Kiszka]

On 2012-05-21 23:03, Michael S. Tsirkin wrote:
> On Mon, May 21, 2012 at 05:35:34PM -0300, Jan Kiszka wrote:
>> On 2012-05-21 16:05, Michael S. Tsirkin wrote:
>>> On Mon, May 21, 2012 at 10:13:47AM -0300, Jan Kiszka wrote:
>>>> @@ -386,6 +387,14 @@ static void piix3_set_irq(void *opaque, int pirq, int 
>>>> level)
>>>>      piix3_set_irq_level(piix3, pirq, level);
>>>>  }
>>>>  
>>>> +static int piix3_map_host_irq(void *opaque, int pci_intx)
>>>> +{
>>>> +    PIIX3State *piix3 = opaque;
>>>> +    int host_irq = piix3->dev.config[PIIX_PIRQC + pci_intx];
>>>> +
>>>> +    return host_irq < PIIX_NUM_PIC_IRQS ? host_irq : -1;
>>>> +}
>>>> +
>>>>  /* irq routing is changed. so rebuild bitmap */
>>>>  static void piix3_update_irq_levels(PIIX3State *piix3)
>>>>  {
>>>
>>>
>>> So, instead of special API just for assignment,
>>> I would like to see map_irq in piix being reworked
>>> to take dev config into account.
>>> I think piix is almost unique in this but need to check,
>>
>> Maybe it is the only host bridge with routing that we have in QEMU right
>> now, but it is surely not unique (e.g. all later Intel chipsets support
>> this).
> 
> Yes. APIs for this should be in place. Just saying
> instead of failing we can easily just make it work
> if there are no remappings.
> 
>>> if not fix other host buses that are programmable.
>>> PCI bridges are all fixed routing.
>>>
>>> Then we can drop set_irq callback.
>>
>> set_irq may do more than IRQ routing. It may also flip polarity (see
>> bonito.c).
> 
> In that case host_map_irq is not a good API?
> Need to investigate.
> 
>> I guess we need to analyze the requirements of all in-tree
>> host bridges first.
> 
> Yes.
> 
>>>
>>> Finally all devices can cache the irq#,
>>> and piix would scan devices behind it
>>> and update the irq.
>>>
>>> Assignment then just needs a notifier, since
>>> it owns the device just a pointer in device is
>>> enough.
>>
>> I cannot completely follow your ideas here yet. Do you want to pass the
>> mapping information along the notifier, or why "just ... a notifier"?
> 
> 
> Each device would resolve IRQs that it uses.
> 
> 
>>>
>>> Could you look at doing this please?
>>> If no I can give it a stub.
>>>
>>
>> Unless we can converge over a final API quickly, I would suggest to base
>> these refactorings on top of what I propose here. We will have to touch
>> all host bridges more invasively for this anyway. If you can explain to
>> me how simple the refactoring can be (but I'm a bit skeptical ;) ), I
>> could do this, otherwise I would prefer to focus on the device
>> assignment topic which provide some more nuts.
>>
>> Jan
> 
> Yes it's simple. I'll post in a couple of days (lots of
> meetings tomorrow). I think you'll
> see it's easier and cleaner.

I looked into this again and it appears to me that, in fact, more work
is required to bypass the current interrupt routing on delivery:

The PIIX2 tracks the interrupt level of each device on its bus with the
help of PCIBus::irq_count. On routing changes via its config space,
those levels are currently used to generate the new host IRQ states.
But, once we bypass that level state tracking, we need to do something
else, and that better for _all_ devices: clear all outputs and let the
devices issue an update. The assigned device could provide this based on
the INTx status bit, for all others we need to track the level generically.

Did you start working on this topic already?

Jan

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