Re: [PATCH 02/18] xen: hook io_apic read/write operations

[Jeremy Fitzhardinge <jeremy@goop.org>]

Ingo Molnar wrote:
>> Hm, every time you see this code, you always have this 
>> quasi-Pavlovian response.
>>     
>
> Yep, my reaction to ugly code is pretty predictable, and 
> (hopefully!) repeatable. So calling it Pavlovian is an implicit 
> (albeit, i suspect, unintended ;-) compliment.
>   

My frustration is that you've generally not replied, and so we haven't 
been able to discuss it.  Could you be more specific about what's 
triggering the reaction?  Is it that the change is happening at the 
io-apic level, or that its some explicit Xen code pasted in here rather 
than via an io_apic_ops?

>> You say "use an irqchip".  I say:
>>
>>    * We already use irqchip
>>    * but most of the interesting IO apic accesses (routing) are not
>>      done via the irqchip interface
>>    * so irqchip doesn't help
>>     
>
> I dont see the problem. All APIs within Linux are kept minimalistic 
> and are extended on the fly, on an on-demand basis.
>   

Sure, where it makes sense.  One wouldn't start extending an API in a 
completely different direction.  irq_chip currently only deals with 
"irqs"; what those irqs mean and connect to are not its business because 
that has all been set up elsewhere.  If you start adding interrupt 
routing, then it starts needing to know about devices, busses, etc.  I 
can't see how that makes much sense at all, particularly for an 
arch-independent interface.

>
> Well, my main task at this stage is to point out ugly code. I might 
> be able to do research for you and come up with a plan for you, but 
> that's really a courtesy in general and is not always possible for 
> maintainers. You might argue "of all possible solutions this is the 
> cleanest" but i havent seen you make that point.

I'd love to, but Plato isn't taking my calls so I can't check.

But I do think hooking the io-apic operations makes more sense than any 
other solution because we explicitly want all the other code above it.  
The *only* difference between a Xen io-apic and a native io-apic is that 
we need to access the registers with hypercalls rather than mmio.  Same 
registers, same meanings, same settings made at the same time.  So the 
io-apic accessors are at precisely the right level of abstraction for 
our needs; introducing something higher-level would be an abstraction 
impedance mismatch, and would be no better for it.

An io_apic_ops makes sense to me, if adding it would stop triggering 
your ugly-code detector.  But that's specifically what HPA objected to 
in this series...

    J