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

[Jan Kiszka]

On 2012-05-30 19:41, Michael S. Tsirkin wrote:
> 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 :)

One final output. Userspace originated IRQs can be merged in userspace
before flipping KVM's [IOA]PIC input line.

> 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.

Sorry, your patch doesn't help me in any way.

Jan

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