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

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 3473 bytes --]

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.

So, what can I do to make this API more acceptable for you?

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]