Re: [PATCH 03/28] x86/irq: Use irq_remap specific print_IO_APICpaths only on Intel

From: Ingo Molnar
Date: Fri Jul 06 2012 - 10:00:39 EST

* Joerg Roedel <joerg.roedel@xxxxxxx> wrote:

> On Fri, Jul 06, 2012 at 10:50:36AM +0200, Ingo Molnar wrote:
> >
> > * Joerg Roedel <joerg.roedel@xxxxxxx> wrote:
> > > extern int irq_remapping_enabled;
> > > +extern int intel_irq_remap_debug;
> > Instead of yet another set of global flags thrown around the
> > kernel please properly factor out this code, its data structures
> > and methods: introduce a single descriptor structure that
> > describes this piece of hardware, with debugging flags part of
> > this structure - with operations function pointer structure and
> > such.
> Not sure I understand what you mean. So, simplified, from a
> hardware point of view we have IO-APICs and MSIs. This doesn't
> change with IOMMU-based interrupt remapping. The IO-APICs and
> MSIs are properly abstraced through 'struct irq_chip'.

I wouldn't call the IO-APIC code 'properly abstracted' - it's
basically minimally abstracted to make genirq work, but
otherwise it's still stock full of global data and methods,
remnants of the old monolithic IO-APIC code.

( A proper abstraction would stick it all into some sort PCI
driver alike structure, including enumeration, initialization,
debugging and other non-core details. )

> When an IOMMU comes into play the IO-APICs and MSIs need to be
> programmed differently so that they send the IRQ messages in a
> way the IOMMU can remap. This is done by using a different
> 'struct irq_chip' when interrupt remapping is enabled.

So the way this could work in a cleaner fashion is to
encapsulate the logic even more. Today we have a per irq_desc
irq_cfg data descriptor, but there's still global knowledge in
actual vector allocation such as create_irq() or
msi_compose_msg(). Patterns like:

if (irq_remapped(cfg)) {
compose_remapped_msi_msg(pdev, irq, dest, msg, hpet_id);
return err;

if (x2apic_enabled())
msg->address_hi = MSI_ADDR_BASE_HI |
msg->address_hi = MSI_ADDR_BASE_HI;

all all signs of insufficient abstraction.

Methods like the ->set_affinity() variants are sufficiently
abstracted out. Others, including the bits I commented on, not
so much.

> For IRQ remapping there are two (not so much) different
> implementations which are abstracted through 'struct
> irq_remap_ops' made accessible via functions.
> So what I _think_ you mean is to add another call-back to the
> irq_remap_ops to print out debugging information and use that
> call-back when IRQ remapping is enabled instead of the routine
> in io_apic.c. Is that right?

This would be part of it, yes - and doing that alone would make
this patch more palatable.

I'd also suggest other reductions of complexity - for example
CONFIG_IRQ_REMAP should probably be an unconditional feature -
it's not a huge amount of code.

More importantly, all the silly open-coded if
(irq_remapping_enabled) checks should be eliminated from core
x86 code. IRQ remapping should be either be an irq_chip detail
or should live in a separate layer.

So before extending all this please get this into shape.


To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at
Please read the FAQ at