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

From: Jeremy Fitzhardinge
Date: Mon May 11 2009 - 14:25:34 EST


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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/