[PATCH 00/19 v2] Improve IRQ remapping abstraction in x86 core code

[PATCH 00/19 v2] Improve IRQ remapping abstraction in x86 core code

[Joerg Roedel]

Hi,

here is the second version of the patch-set to improve the abstraction
of interrupt remapping in the x86 core code. A more detailed description
can be found in the original post at:

	https://lkml.org/lkml/2012/8/7/317

Changes from v1->v2:

	* Fixed a couple of compile issues on i386 (reported by
	  Fengguang Wu, thanks a lot)
	* Rebased to Linux v3.6-rc2

Please review.

Thanks,

	Joerg

Diffstat:

 arch/x86/include/asm/hpet.h          |    5 +-
 arch/x86/include/asm/hw_irq.h        |   13 +-
 arch/x86/include/asm/io_apic.h       |   31 +++
 arch/x86/include/asm/irq_remapping.h |   40 ++--
 arch/x86/include/asm/pci.h           |    2 +
 arch/x86/include/asm/x86_init.h      |   30 ++-
 arch/x86/kernel/apic/apic.c          |   28 ++-
 arch/x86/kernel/apic/io_apic.c       |  377 +++++++++++++---------------------
 arch/x86/kernel/hpet.c               |    2 +-
 arch/x86/kernel/x86_init.c           |   25 ++-
 drivers/iommu/dmar.c                 |    2 +
 drivers/iommu/intel-iommu.c          |    2 +
 drivers/iommu/intel_irq_remapping.c  |    8 +
 drivers/iommu/irq_remapping.c        |  186 ++++++++++++++++-
 drivers/iommu/irq_remapping.h        |    3 +
 15 files changed, 454 insertions(+), 300 deletions(-)

[PATCH 01/19] x86, apic: Move irq_remapping_enabled checks into IRQ-remapping code

[Joerg Roedel]

Move the three easy to move checks in the x86' apic.c file
into the IRQ-remapping code.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/x86/kernel/apic/apic.c   |    9 +++------
 drivers/iommu/irq_remapping.c |   11 +++++++++--
 2 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index 24deb30..41681b3 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -1444,8 +1444,7 @@ void __init bsp_end_local_APIC_setup(void)
 	 * Now that local APIC setup is completed for BP, configure the fault
 	 * handling for interrupt remapping.
 	 */
-	if (irq_remapping_enabled)
-		irq_remap_enable_fault_handling();
+	irq_remap_enable_fault_handling();
 
 }
 
@@ -2218,8 +2217,7 @@ static int lapic_suspend(void)
 	local_irq_save(flags);
 	disable_local_APIC();
 
-	if (irq_remapping_enabled)
-		irq_remapping_disable();
+	irq_remapping_disable();
 
 	local_irq_restore(flags);
 	return 0;
@@ -2287,8 +2285,7 @@ static void lapic_resume(void)
 	apic_write(APIC_ESR, 0);
 	apic_read(APIC_ESR);
 
-	if (irq_remapping_enabled)
-		irq_remapping_reenable(x2apic_mode);
+	irq_remapping_reenable(x2apic_mode);
 
 	local_irq_restore(flags);
 }
diff --git a/drivers/iommu/irq_remapping.c b/drivers/iommu/irq_remapping.c
index 151690d..283a43f 100644
--- a/drivers/iommu/irq_remapping.c
+++ b/drivers/iommu/irq_remapping.c
@@ -82,7 +82,9 @@ int __init irq_remapping_enable(void)
 
 void irq_remapping_disable(void)
 {
-	if (!remap_ops || !remap_ops->disable)
+	if (!irq_remapping_enabled ||
+	    !remap_ops ||
+	    !remap_ops->disable)
 		return;
 
 	remap_ops->disable();
@@ -90,7 +92,9 @@ void irq_remapping_disable(void)
 
 int irq_remapping_reenable(int mode)
 {
-	if (!remap_ops || !remap_ops->reenable)
+	if (!irq_remapping_enabled ||
+	    !remap_ops ||
+	    !remap_ops->reenable)
 		return 0;
 
 	return remap_ops->reenable(mode);
@@ -98,6 +102,9 @@ int irq_remapping_reenable(int mode)
 
 int __init irq_remap_enable_fault_handling(void)
 {
+	if (!irq_remapping_enabled)
+		return 0;
+
 	if (!remap_ops || !remap_ops->enable_faulting)
 		return -ENODEV;
 
-- 
1.7.9.5

[PATCH 02/19] [RFC] x86, apic: Mask IO-APIC and PIC unconditionally on LAPIC resume

[Joerg Roedel]

IO-APIC and PIC use the same resume routines when IRQ
remapping is enabled or disabled. So it should be safe to
mask the other APICs for the IRQ-remapping-disabled case
too.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/x86/kernel/apic/apic.c |   19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index 41681b3..109380a 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -2233,16 +2233,15 @@ static void lapic_resume(void)
 		return;
 
 	local_irq_save(flags);
-	if (irq_remapping_enabled) {
-		/*
-		 * IO-APIC and PIC have their own resume routines.
-		 * We just mask them here to make sure the interrupt
-		 * subsystem is completely quiet while we enable x2apic
-		 * and interrupt-remapping.
-		 */
-		mask_ioapic_entries();
-		legacy_pic->mask_all();
-	}
+
+	/*
+	 * IO-APIC and PIC have their own resume routines.
+	 * We just mask them here to make sure the interrupt
+	 * subsystem is completely quiet while we enable x2apic
+	 * and interrupt-remapping.
+	 */
+	mask_ioapic_entries();
+	legacy_pic->mask_all();
 
 	if (x2apic_mode)
 		enable_x2apic();
-- 
1.7.9.5

Re: [PATCH 02/19] [RFC] x86, apic: Mask IO-APIC and PIC unconditionally on LAPIC resume

[Sebastian Andrzej Siewior]

On Mon, Aug 20, 2012 at 03:55:48PM +0200, Joerg Roedel wrote:
> IO-APIC and PIC use the same resume routines when IRQ
> remapping is enabled or disabled. So it should be safe to
> mask the other APICs for the IRQ-remapping-disabled case
> too.

So your point is that it works with irq-remapping so it should work without as
well.
The lapic code registers its opcodes as the first syscore device so this code
is always executed before the lapic's and pic resume code. PIC masks its sources
during init so it does not matter if they are masked earlier. And ioapic, well
it restores its entries as well.
So yes, I think this safe to do here.

Sebastian

[PATCH 03/19] x86, io_apic: Introduce x86_io_apic_ops.disable()

[Joerg Roedel]

This function pointer is used to call a system-specific
function for disabling the IO-APIC. Currently this is used
for IRQ remapping which has its own disable routine.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/x86/include/asm/io_apic.h  |    2 ++
 arch/x86/include/asm/x86_init.h |    9 +++++----
 arch/x86/kernel/apic/io_apic.c  |   41 ++++++++++++++++++---------------------
 arch/x86/kernel/x86_init.c      |    9 +++++----
 drivers/iommu/irq_remapping.c   |   31 ++++++++++++++++++++++++++++-
 5 files changed, 61 insertions(+), 31 deletions(-)

diff --git a/arch/x86/include/asm/io_apic.h b/arch/x86/include/asm/io_apic.h
index 73d8c53..d59e172 100644
--- a/arch/x86/include/asm/io_apic.h
+++ b/arch/x86/include/asm/io_apic.h
@@ -179,6 +179,7 @@ extern void __init native_io_apic_init_mappings(void);
 extern unsigned int native_io_apic_read(unsigned int apic, unsigned int reg);
 extern void native_io_apic_write(unsigned int apic, unsigned int reg, unsigned int val);
 extern void native_io_apic_modify(unsigned int apic, unsigned int reg, unsigned int val);
+extern void native_disable_io_apic(void);
 
 static inline unsigned int io_apic_read(unsigned int apic, unsigned int reg)
 {
@@ -223,6 +224,7 @@ static inline void disable_ioapic_support(void) { }
 #define native_io_apic_read		NULL
 #define native_io_apic_write		NULL
 #define native_io_apic_modify		NULL
+#define native_disable_io_apic		NULL
 #endif
 
 #endif /* _ASM_X86_IO_APIC_H */
diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
index 38155f6..8e1b44c 100644
--- a/arch/x86/include/asm/x86_init.h
+++ b/arch/x86/include/asm/x86_init.h
@@ -189,10 +189,11 @@ struct x86_msi_ops {
 };
 
 struct x86_io_apic_ops {
-	void		(*init)  (void);
-	unsigned int	(*read)  (unsigned int apic, unsigned int reg);
-	void		(*write) (unsigned int apic, unsigned int reg, unsigned int value);
-	void		(*modify)(unsigned int apic, unsigned int reg, unsigned int value);
+	void		(*init)   (void);
+	unsigned int	(*read)   (unsigned int apic, unsigned int reg);
+	void		(*write)  (unsigned int apic, unsigned int reg, unsigned int value);
+	void		(*modify) (unsigned int apic, unsigned int reg, unsigned int value);
+	void		(*disable)(void);
 };
 
 extern struct x86_init_ops x86_init;
diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index a6c64aa..b919f1e 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -1925,30 +1925,14 @@ void __init enable_IO_APIC(void)
 	clear_IO_APIC();
 }
 
-/*
- * Not an __init, needed by the reboot code
- */
-void disable_IO_APIC(void)
+void native_disable_io_apic(void)
 {
 	/*
-	 * Clear the IO-APIC before rebooting:
-	 */
-	clear_IO_APIC();
-
-	if (!legacy_pic->nr_legacy_irqs)
-		return;
-
-	/*
 	 * If the i8259 is routed through an IOAPIC
 	 * Put that IOAPIC in virtual wire mode
 	 * so legacy interrupts can be delivered.
-	 *
-	 * With interrupt-remapping, for now we will use virtual wire A mode,
-	 * as virtual wire B is little complex (need to configure both
-	 * IOAPIC RTE as well as interrupt-remapping table entry).
-	 * As this gets called during crash dump, keep this simple for now.
 	 */
-	if (ioapic_i8259.pin != -1 && !irq_remapping_enabled) {
+	if (ioapic_i8259.pin != -1) {
 		struct IO_APIC_route_entry entry;
 
 		memset(&entry, 0, sizeof(entry));
@@ -1968,12 +1952,25 @@ void disable_IO_APIC(void)
 		ioapic_write_entry(ioapic_i8259.apic, ioapic_i8259.pin, entry);
 	}
 
+	if (cpu_has_apic || apic_from_smp_config())
+		disconnect_bsp_APIC(ioapic_i8259.pin != -1);
+
+}
+
+/*
+ * Not an __init, needed by the reboot code
+ */
+void disable_IO_APIC(void)
+{
 	/*
-	 * Use virtual wire A mode when interrupt remapping is enabled.
+	 * Clear the IO-APIC before rebooting:
 	 */
-	if (cpu_has_apic || apic_from_smp_config())
-		disconnect_bsp_APIC(!irq_remapping_enabled &&
-				ioapic_i8259.pin != -1);
+	clear_IO_APIC();
+
+	if (!legacy_pic->nr_legacy_irqs)
+		return;
+
+	x86_io_apic_ops.disable();
 }
 
 #ifdef CONFIG_X86_32
diff --git a/arch/x86/kernel/x86_init.c b/arch/x86/kernel/x86_init.c
index 9f3167e..3ea56c2 100644
--- a/arch/x86/kernel/x86_init.c
+++ b/arch/x86/kernel/x86_init.c
@@ -120,8 +120,9 @@ struct x86_msi_ops x86_msi = {
 };
 
 struct x86_io_apic_ops x86_io_apic_ops = {
-	.init	= native_io_apic_init_mappings,
-	.read	= native_io_apic_read,
-	.write	= native_io_apic_write,
-	.modify	= native_io_apic_modify,
+	.init			= native_io_apic_init_mappings,
+	.read			= native_io_apic_read,
+	.write			= native_io_apic_write,
+	.modify			= native_io_apic_modify,
+	.disable		= native_disable_io_apic,
 };
diff --git a/drivers/iommu/irq_remapping.c b/drivers/iommu/irq_remapping.c
index 283a43f..f72a5e8 100644
--- a/drivers/iommu/irq_remapping.c
+++ b/drivers/iommu/irq_remapping.c
@@ -1,3 +1,4 @@
+#include <linux/cpumask.h>
 #include <linux/kernel.h>
 #include <linux/string.h>
 #include <linux/cpumask.h>
@@ -6,6 +7,9 @@
 
 #include <asm/hw_irq.h>
 #include <asm/irq_remapping.h>
+#include <asm/processor.h>
+#include <asm/x86_init.h>
+#include <asm/apic.h>
 
 #include "irq_remapping.h"
 
@@ -17,6 +21,24 @@ int no_x2apic_optout;
 
 static struct irq_remap_ops *remap_ops;
 
+static void irq_remapping_disable_io_apic(void)
+{
+	/*
+	 * With interrupt-remapping, for now we will use virtual wire A
+	 * mode, as virtual wire B is little complex (need to configure
+	 * both IOAPIC RTE as well as interrupt-remapping table entry).
+	 * As this gets called during crash dump, keep this simple for
+	 * now.
+	 */
+	if (cpu_has_apic || apic_from_smp_config())
+		disconnect_bsp_APIC(0);
+}
+
+static void __init irq_remapping_modify_x86_ops(void)
+{
+	x86_io_apic_ops.disable	=	irq_remapping_disable_io_apic;
+}
+
 static __init int setup_nointremap(char *str)
 {
 	disable_irq_remap = 1;
@@ -74,10 +96,17 @@ int __init irq_remapping_prepare(void)
 
 int __init irq_remapping_enable(void)
 {
+	int ret;
+
 	if (!remap_ops || !remap_ops->enable)
 		return -ENODEV;
 
-	return remap_ops->enable();
+	ret = remap_ops->enable();
+
+	if (irq_remapping_enabled)
+		irq_remapping_modify_x86_ops();
+
+	return ret;
 }
 
 void irq_remapping_disable(void)
-- 
1.7.9.5

Re: [PATCH 03/19] x86, io_apic: Introduce x86_io_apic_ops.disable()

[Sebastian Andrzej Siewior]

On Mon, Aug 20, 2012 at 03:55:49PM +0200, Joerg Roedel wrote:
> This function pointer is used to call a system-specific
> function for disabling the IO-APIC. Currently this is used
> for IRQ remapping which has its own disable routine.

What I miss here is the fact that you extract irq-mapping specific bits from
disable_IO_APIC() and put them in a separate function named
irq_remapping_disable_io_apic(). And having a big if in this function probably
didn't look that sexy so you decided to use function op. Nice move, no
question but not abvious from the description here and it took a while to figure
it out based on the code. Well, maybe it is late…

> diff --git a/drivers/iommu/irq_remapping.c b/drivers/iommu/irq_remapping.c
> index 283a43f..f72a5e8 100644
> --- a/drivers/iommu/irq_remapping.c
> +++ b/drivers/iommu/irq_remapping.c
> @@ -17,6 +21,24 @@ int no_x2apic_optout;
<snip>
> +static void __init irq_remapping_modify_x86_ops(void)

I don't want sound to picky here but the term 'modify' is bad I think. Would
'overwrite' fit better here? Is 'modify' used someplace else in x86 so you
follow a common pattern here? Maybe it is just me.

> +{
> +	x86_io_apic_ops.disable	=	irq_remapping_disable_io_apic;
> +}
> +

Sebastian

Re: [PATCH 03/19] x86, io_apic: Introduce x86_io_apic_ops.disable()

[Joerg Roedel]

On Thu, Aug 23, 2012 at 11:11:40PM +0200, Sebastian Andrzej Siewior wrote:
> On Mon, Aug 20, 2012 at 03:55:49PM +0200, Joerg Roedel wrote:
> What I miss here is the fact that you extract irq-mapping specific bits from
> disable_IO_APIC() and put them in a separate function named
> irq_remapping_disable_io_apic(). And having a big if in this function probably
> didn't look that sexy so you decided to use function op. Nice move, no
> question but not abvious from the description here and it took a while to figure
> it out based on the code. Well, maybe it is lateâ?¦

Well, yeah, the confusing thing with this patch is, that it does not only
introduce the x86_io_apic_ops.disable call-back but also adds the
infrastructure to change this op (and others). So the infrastructure
change should probably be in a seperate patch. But on the other side the
infrastructure change is small and the patch-set is already really
large.
Is it worth it to split that out?

> I don't want sound to picky here but the term 'modify' is bad I think. Would
> 'overwrite' fit better here? Is 'modify' used someplace else in x86 so you
> follow a common pattern here? Maybe it is just me.

Probably it's just me, but overwrite sounds like 'replace' (== overwrite
all ops with own values). But the code changes only a few of the ops
which need different behavior for interrupt remapping. I think 'modify'
describes that better, no?


	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

Re: [PATCH 03/19] x86, io_apic: Introduce x86_io_apic_ops.disable()

[Sebastian Andrzej Siewior]

On Fri, Aug 24, 2012 at 02:22:26PM +0200, Joerg Roedel wrote:
> Well, yeah, the confusing thing with this patch is, that it does not only
> introduce the x86_io_apic_ops.disable call-back but also adds the
> infrastructure to change this op (and others). So the infrastructure
> change should probably be in a seperate patch. But on the other side the
> infrastructure change is small and the patch-set is already really
> large.
> Is it worth it to split that out?

I don't insist to redo the patch because of this. Please redo the patch
description to better describe what you do. You pull the "irq remap" code out
and stuff it in another function.

> > I don't want sound to picky here but the term 'modify' is bad I think. Would
> > 'overwrite' fit better here? Is 'modify' used someplace else in x86 so you
> > follow a common pattern here? Maybe it is just me.
> 
> Probably it's just me, but overwrite sounds like 'replace' (== overwrite
> all ops with own values). But the code changes only a few of the ops
> which need different behavior for interrupt remapping. I think 'modify'
> describes that better, no?

Leave it then :)

> 
> 
> 	Joerg

Sebastian

Re: [PATCH 03/19] x86, io_apic: Introduce x86_io_apic_ops.disable()

[Joerg Roedel]

On Sat, Aug 25, 2012 at 07:17:05PM +0200, Sebastian Andrzej Siewior wrote:
> On Fri, Aug 24, 2012 at 02:22:26PM +0200, Joerg Roedel wrote:

> > Is it worth it to split that out?
> 
> I don't insist to redo the patch because of this. Please redo the patch
> description to better describe what you do. You pull the "irq remap" code out
> and stuff it in another function.

Okay, I will update the commit message :)


	Joerg

[PATCH 04/19] x86, io_apic: Introduce x86_io_apic_ops.print_entries for debugging

[Joerg Roedel]

This call-back is used to dump IO-APIC entries for debugging
purposes into the kernel log. VT-d needs a special routine
for this and will overwrite the default.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/x86/include/asm/io_apic.h      |    3 +
 arch/x86/include/asm/x86_init.h     |    1 +
 arch/x86/kernel/apic/io_apic.c      |  109 ++++++++++++++++++-----------------
 arch/x86/kernel/x86_init.c          |    1 +
 drivers/iommu/intel_irq_remapping.c |    4 ++
 5 files changed, 65 insertions(+), 53 deletions(-)

diff --git a/arch/x86/include/asm/io_apic.h b/arch/x86/include/asm/io_apic.h
index d59e172..21aa81e 100644
--- a/arch/x86/include/asm/io_apic.h
+++ b/arch/x86/include/asm/io_apic.h
@@ -180,6 +180,8 @@ extern unsigned int native_io_apic_read(unsigned int apic, unsigned int reg);
 extern void native_io_apic_write(unsigned int apic, unsigned int reg, unsigned int val);
 extern void native_io_apic_modify(unsigned int apic, unsigned int reg, unsigned int val);
 extern void native_disable_io_apic(void);
+extern void native_io_apic_print_entries(unsigned int apic, unsigned int nr_entries);
+extern void intel_ir_io_apic_print_entries(unsigned int apic, unsigned int nr_entries);
 
 static inline unsigned int io_apic_read(unsigned int apic, unsigned int reg)
 {
@@ -225,6 +227,7 @@ static inline void disable_ioapic_support(void) { }
 #define native_io_apic_write		NULL
 #define native_io_apic_modify		NULL
 #define native_disable_io_apic		NULL
+#define native_io_apic_print_entries	NULL
 #endif
 
 #endif /* _ASM_X86_IO_APIC_H */
diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
index 8e1b44c..be8e9a6 100644
--- a/arch/x86/include/asm/x86_init.h
+++ b/arch/x86/include/asm/x86_init.h
@@ -194,6 +194,7 @@ struct x86_io_apic_ops {
 	void		(*write)  (unsigned int apic, unsigned int reg, unsigned int value);
 	void		(*modify) (unsigned int apic, unsigned int reg, unsigned int value);
 	void		(*disable)(void);
+	void		(*print_entries)(unsigned int apic, unsigned int nr_entries);
 };
 
 extern struct x86_init_ops x86_init;
diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index b919f1e..1ac89a6 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -1517,9 +1517,63 @@ static void __init setup_timer_IRQ0_pin(unsigned int ioapic_idx,
 	ioapic_write_entry(ioapic_idx, pin, entry);
 }
 
-__apicdebuginit(void) print_IO_APIC(int ioapic_idx)
+void native_io_apic_print_entries(unsigned int apic, unsigned int nr_entries)
+{
+	int i;
+
+	pr_debug(" NR Dst Mask Trig IRR Pol Stat Dmod Deli Vect:\n");
+
+	for (i = 0; i <= nr_entries; i++) {
+		struct IO_APIC_route_entry entry;
+
+		entry = ioapic_read_entry(apic, i);
+
+		pr_debug(" %02x %02X  ", i, entry.dest);
+		pr_cont("%1d    %1d    %1d   %1d   %1d    "
+			"%1d    %1d    %02X\n",
+			entry.mask,
+			entry.trigger,
+			entry.irr,
+			entry.polarity,
+			entry.delivery_status,
+			entry.dest_mode,
+			entry.delivery_mode,
+			entry.vector);
+	}
+}
+
+void intel_ir_io_apic_print_entries(unsigned int apic,
+				    unsigned int nr_entries)
 {
 	int i;
+
+	pr_debug(" NR Indx Fmt Mask Trig IRR Pol Stat Indx2 Zero Vect:\n");
+
+	for (i = 0; i <= nr_entries; i++) {
+		struct IR_IO_APIC_route_entry *ir_entry;
+		struct IO_APIC_route_entry entry;
+
+		entry = ioapic_read_entry(apic, i);
+
+		ir_entry = (struct IR_IO_APIC_route_entry *)&entry;
+
+		pr_debug(" %02x %04X ", i, ir_entry->index);
+		pr_cont("%1d   %1d    %1d    %1d   %1d   "
+			"%1d    %1d     %X    %02X\n",
+			ir_entry->format,
+			ir_entry->mask,
+			ir_entry->trigger,
+			ir_entry->irr,
+			ir_entry->polarity,
+			ir_entry->delivery_status,
+			ir_entry->index2,
+			ir_entry->zero,
+			ir_entry->vector);
+	}
+}
+
+__apicdebuginit(void) print_IO_APIC(int ioapic_idx)
+{
 	union IO_APIC_reg_00 reg_00;
 	union IO_APIC_reg_01 reg_01;
 	union IO_APIC_reg_02 reg_02;
@@ -1572,58 +1626,7 @@ __apicdebuginit(void) print_IO_APIC(int ioapic_idx)
 
 	printk(KERN_DEBUG ".... IRQ redirection table:\n");
 
-	if (irq_remapping_enabled) {
-		printk(KERN_DEBUG " NR Indx Fmt Mask Trig IRR"
-			" Pol Stat Indx2 Zero Vect:\n");
-	} else {
-		printk(KERN_DEBUG " NR Dst Mask Trig IRR Pol"
-			" Stat Dmod Deli Vect:\n");
-	}
-
-	for (i = 0; i <= reg_01.bits.entries; i++) {
-		if (irq_remapping_enabled) {
-			struct IO_APIC_route_entry entry;
-			struct IR_IO_APIC_route_entry *ir_entry;
-
-			entry = ioapic_read_entry(ioapic_idx, i);
-			ir_entry = (struct IR_IO_APIC_route_entry *) &entry;
-			printk(KERN_DEBUG " %02x %04X ",
-				i,
-				ir_entry->index
-			);
-			pr_cont("%1d   %1d    %1d    %1d   %1d   "
-				"%1d    %1d     %X    %02X\n",
-				ir_entry->format,
-				ir_entry->mask,
-				ir_entry->trigger,
-				ir_entry->irr,
-				ir_entry->polarity,
-				ir_entry->delivery_status,
-				ir_entry->index2,
-				ir_entry->zero,
-				ir_entry->vector
-			);
-		} else {
-			struct IO_APIC_route_entry entry;
-
-			entry = ioapic_read_entry(ioapic_idx, i);
-			printk(KERN_DEBUG " %02x %02X  ",
-				i,
-				entry.dest
-			);
-			pr_cont("%1d    %1d    %1d   %1d   %1d    "
-				"%1d    %1d    %02X\n",
-				entry.mask,
-				entry.trigger,
-				entry.irr,
-				entry.polarity,
-				entry.delivery_status,
-				entry.dest_mode,
-				entry.delivery_mode,
-				entry.vector
-			);
-		}
-	}
+	x86_io_apic_ops.print_entries(ioapic_idx, reg_01.bits.entries);
 }
 
 __apicdebuginit(void) print_IO_APICs(void)
diff --git a/arch/x86/kernel/x86_init.c b/arch/x86/kernel/x86_init.c
index 3ea56c2..dc19c50 100644
--- a/arch/x86/kernel/x86_init.c
+++ b/arch/x86/kernel/x86_init.c
@@ -125,4 +125,5 @@ struct x86_io_apic_ops x86_io_apic_ops = {
 	.write			= native_io_apic_write,
 	.modify			= native_io_apic_modify,
 	.disable		= native_disable_io_apic,
+	.print_entries		= native_io_apic_print_entries,
 };
diff --git a/drivers/iommu/intel_irq_remapping.c b/drivers/iommu/intel_irq_remapping.c
index e0b18f3..321db1a 100644
--- a/drivers/iommu/intel_irq_remapping.c
+++ b/drivers/iommu/intel_irq_remapping.c
@@ -617,6 +617,10 @@ static int __init intel_enable_irq_remapping(void)
 		goto error;
 
 	irq_remapping_enabled = 1;
+
+	/* VT-d needs a special routine to print IO-APIC entries */
+	x86_io_apic_ops.print_entries = intel_ir_io_apic_print_entries;
+
 	pr_info("Enabled IRQ remapping in %s mode\n", eim ? "x2apic" : "xapic");
 
 	return eim ? IRQ_REMAP_X2APIC_MODE : IRQ_REMAP_XAPIC_MODE;
-- 
1.7.9.5

Re: [PATCH 04/19] x86, io_apic: Introduce x86_io_apic_ops.print_entries for debugging

[Sebastian Andrzej Siewior]

On Mon, Aug 20, 2012 at 03:55:50PM +0200, Joerg Roedel wrote:
> This call-back is used to dump IO-APIC entries for debugging
> purposes into the kernel log. VT-d needs a special routine
> for this and will overwrite the default.

Okay, again: code basically does what it did, it has been shifted around and
is now called directly.

> Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
> diff --git a/drivers/iommu/intel_irq_remapping.c b/drivers/iommu/intel_irq_remapping.c
> index e0b18f3..321db1a 100644
> --- a/drivers/iommu/intel_irq_remapping.c
> +++ b/drivers/iommu/intel_irq_remapping.c
> @@ -617,6 +617,10 @@ static int __init intel_enable_irq_remapping(void)
>  		goto error;
>  
>  	irq_remapping_enabled = 1;
> +
> +	/* VT-d needs a special routine to print IO-APIC entries */
Seriously? That that's why you overwrite it with a different function :)

> +	x86_io_apic_ops.print_entries = intel_ir_io_apic_print_entries;
> +
>  	pr_info("Enabled IRQ remapping in %s mode\n", eim ? "x2apic" : "xapic");
>  
>  	return eim ? IRQ_REMAP_X2APIC_MODE : IRQ_REMAP_XAPIC_MODE;

Sebastian

[PATCH 05/19] x86, hpet: Introduce x86_msi_ops.setup_hpet_msi

[Joerg Roedel]

This function pointer can be overwritten by the IRQ
remapping code. The irq_remapping_enabled check can be
removed from default_setup_hpet_msi.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/x86/include/asm/hpet.h     |    5 +++--
 arch/x86/include/asm/x86_init.h |    1 +
 arch/x86/kernel/apic/io_apic.c  |    7 +------
 arch/x86/kernel/hpet.c          |    2 +-
 arch/x86/kernel/x86_init.c      |   10 ++++++----
 drivers/iommu/irq_remapping.c   |    3 ++-
 6 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/arch/x86/include/asm/hpet.h b/arch/x86/include/asm/hpet.h
index 2c392d6..ad2dae0 100644
--- a/arch/x86/include/asm/hpet.h
+++ b/arch/x86/include/asm/hpet.h
@@ -82,9 +82,9 @@ extern void hpet_msi_write(struct hpet_dev *hdev, struct msi_msg *msg);
 extern void hpet_msi_read(struct hpet_dev *hdev, struct msi_msg *msg);
 
 #ifdef CONFIG_PCI_MSI
-extern int arch_setup_hpet_msi(unsigned int irq, unsigned int id);
+extern int default_setup_hpet_msi(unsigned int irq, unsigned int id);
 #else
-static inline int arch_setup_hpet_msi(unsigned int irq, unsigned int id)
+static inline int default_setup_hpet_msi(unsigned int irq, unsigned int id)
 {
 	return -EINVAL;
 }
@@ -113,6 +113,7 @@ extern void hpet_unregister_irq_handler(rtc_irq_handler handler);
 static inline int hpet_enable(void) { return 0; }
 static inline int is_hpet_enabled(void) { return 0; }
 #define hpet_readl(a) 0
+#define default_setup_hpet_msi	NULL
 
 #endif
 #endif /* _ASM_X86_HPET_H */
diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
index be8e9a6..1f49c8a 100644
--- a/arch/x86/include/asm/x86_init.h
+++ b/arch/x86/include/asm/x86_init.h
@@ -186,6 +186,7 @@ struct x86_msi_ops {
 	void (*teardown_msi_irq)(unsigned int irq);
 	void (*teardown_msi_irqs)(struct pci_dev *dev);
 	void (*restore_msi_irqs)(struct pci_dev *dev, int irq);
+	int  (*setup_hpet_msi)(unsigned int irq, unsigned int id);
 };
 
 struct x86_io_apic_ops {
diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index 1ac89a6..3d58775 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -3297,17 +3297,12 @@ static struct irq_chip hpet_msi_type = {
 	.irq_retrigger = ioapic_retrigger_irq,
 };
 
-int arch_setup_hpet_msi(unsigned int irq, unsigned int id)
+int default_setup_hpet_msi(unsigned int irq, unsigned int id)
 {
 	struct irq_chip *chip = &hpet_msi_type;
 	struct msi_msg msg;
 	int ret;
 
-	if (irq_remapping_enabled) {
-		if (!setup_hpet_msi_remapped(irq, id))
-			return -1;
-	}
-
 	ret = msi_compose_msg(NULL, irq, &msg, id);
 	if (ret < 0)
 		return ret;
diff --git a/arch/x86/kernel/hpet.c b/arch/x86/kernel/hpet.c
index 1460a5d..22d4fe5 100644
--- a/arch/x86/kernel/hpet.c
+++ b/arch/x86/kernel/hpet.c
@@ -478,7 +478,7 @@ static int hpet_msi_next_event(unsigned long delta,
 
 static int hpet_setup_msi_irq(unsigned int irq)
 {
-	if (arch_setup_hpet_msi(irq, hpet_blockid)) {
+	if (x86_msi.setup_hpet_msi(irq, hpet_blockid)) {
 		destroy_irq(irq);
 		return -EINVAL;
 	}
diff --git a/arch/x86/kernel/x86_init.c b/arch/x86/kernel/x86_init.c
index dc19c50..339e108 100644
--- a/arch/x86/kernel/x86_init.c
+++ b/arch/x86/kernel/x86_init.c
@@ -19,6 +19,7 @@
 #include <asm/time.h>
 #include <asm/irq.h>
 #include <asm/io_apic.h>
+#include <asm/hpet.h>
 #include <asm/pat.h>
 #include <asm/tsc.h>
 #include <asm/iommu.h>
@@ -113,10 +114,11 @@ struct x86_platform_ops x86_platform = {
 
 EXPORT_SYMBOL_GPL(x86_platform);
 struct x86_msi_ops x86_msi = {
-	.setup_msi_irqs = native_setup_msi_irqs,
-	.teardown_msi_irq = native_teardown_msi_irq,
-	.teardown_msi_irqs = default_teardown_msi_irqs,
-	.restore_msi_irqs = default_restore_msi_irqs,
+	.setup_msi_irqs		= native_setup_msi_irqs,
+	.teardown_msi_irq	= native_teardown_msi_irq,
+	.teardown_msi_irqs	= default_teardown_msi_irqs,
+	.restore_msi_irqs	= default_restore_msi_irqs,
+	.setup_hpet_msi		= default_setup_hpet_msi,
 };
 
 struct x86_io_apic_ops x86_io_apic_ops = {
diff --git a/drivers/iommu/irq_remapping.c b/drivers/iommu/irq_remapping.c
index f72a5e8..f439151 100644
--- a/drivers/iommu/irq_remapping.c
+++ b/drivers/iommu/irq_remapping.c
@@ -36,7 +36,8 @@ static void irq_remapping_disable_io_apic(void)
 
 static void __init irq_remapping_modify_x86_ops(void)
 {
-	x86_io_apic_ops.disable	=	irq_remapping_disable_io_apic;
+	x86_io_apic_ops.disable		= irq_remapping_disable_io_apic;
+	x86_msi.setup_hpet_msi		= setup_hpet_msi_remapped;
 }
 
 static __init int setup_nointremap(char *str)
-- 
1.7.9.5

[PATCH 06/19] x86, msi: Use IRQ remapping specific setup_msi_irqs routine

[Joerg Roedel]

Use seperate routines to setup MSI IRQs for both
irq_remapping_enabled cases.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/x86/include/asm/irq_remapping.h |   12 -------
 arch/x86/include/asm/pci.h           |    2 ++
 arch/x86/kernel/apic/io_apic.c       |   26 ++--------------
 drivers/iommu/irq_remapping.c        |   57 ++++++++++++++++++++++++++++++++--
 4 files changed, 59 insertions(+), 38 deletions(-)

diff --git a/arch/x86/include/asm/irq_remapping.h b/arch/x86/include/asm/irq_remapping.h
index 5fb9bbb..0ee1e88 100644
--- a/arch/x86/include/asm/irq_remapping.h
+++ b/arch/x86/include/asm/irq_remapping.h
@@ -47,9 +47,6 @@ extern void free_remapped_irq(int irq);
 extern void compose_remapped_msi_msg(struct pci_dev *pdev,
 				     unsigned int irq, unsigned int dest,
 				     struct msi_msg *msg, u8 hpet_id);
-extern int msi_alloc_remapped_irq(struct pci_dev *pdev, int irq, int nvec);
-extern int msi_setup_remapped_irq(struct pci_dev *pdev, unsigned int irq,
-				  int index, int sub_handle);
 extern int setup_hpet_msi_remapped(unsigned int irq, unsigned int id);
 
 #else  /* CONFIG_IRQ_REMAP */
@@ -83,15 +80,6 @@ static inline void compose_remapped_msi_msg(struct pci_dev *pdev,
 					    struct msi_msg *msg, u8 hpet_id)
 {
 }
-static inline int msi_alloc_remapped_irq(struct pci_dev *pdev, int irq, int nvec)
-{
-	return -ENODEV;
-}
-static inline int msi_setup_remapped_irq(struct pci_dev *pdev, unsigned int irq,
-					 int index, int sub_handle)
-{
-	return -ENODEV;
-}
 static inline int setup_hpet_msi_remapped(unsigned int irq, unsigned int id)
 {
 	return -ENODEV;
diff --git a/arch/x86/include/asm/pci.h b/arch/x86/include/asm/pci.h
index df75d07..1470b53 100644
--- a/arch/x86/include/asm/pci.h
+++ b/arch/x86/include/asm/pci.h
@@ -121,9 +121,11 @@ static inline void x86_restore_msi_irqs(struct pci_dev *dev, int irq)
 #define arch_teardown_msi_irq x86_teardown_msi_irq
 #define arch_restore_msi_irqs x86_restore_msi_irqs
 /* implemented in arch/x86/kernel/apic/io_apic. */
+struct msi_desc;
 int native_setup_msi_irqs(struct pci_dev *dev, int nvec, int type);
 void native_teardown_msi_irq(unsigned int irq);
 void native_restore_msi_irqs(struct pci_dev *dev, int irq);
+int setup_msi_irq(struct pci_dev *dev, struct msi_desc *msidesc, int irq);
 /* default to the implementation in drivers/lib/msi.c */
 #define HAVE_DEFAULT_MSI_TEARDOWN_IRQS
 #define HAVE_DEFAULT_MSI_RESTORE_IRQS
diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index 3d58775..811e930 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -3135,7 +3135,7 @@ static struct irq_chip msi_chip = {
 	.irq_retrigger		= ioapic_retrigger_irq,
 };
 
-static int setup_msi_irq(struct pci_dev *dev, struct msi_desc *msidesc, int irq)
+int setup_msi_irq(struct pci_dev *dev, struct msi_desc *msidesc, int irq)
 {
 	struct irq_chip *chip = &msi_chip;
 	struct msi_msg msg;
@@ -3162,9 +3162,9 @@ static int setup_msi_irq(struct pci_dev *dev, struct msi_desc *msidesc, int irq)
 
 int native_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
 {
-	int node, ret, sub_handle, index = 0;
 	unsigned int irq, irq_want;
 	struct msi_desc *msidesc;
+	int node, ret;
 
 	/* x86 doesn't support multiple MSI yet */
 	if (type == PCI_CAP_ID_MSI && nvec > 1)
@@ -3172,36 +3172,16 @@ int native_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
 
 	node = dev_to_node(&dev->dev);
 	irq_want = nr_irqs_gsi;
-	sub_handle = 0;
 	list_for_each_entry(msidesc, &dev->msi_list, list) {
 		irq = create_irq_nr(irq_want, node);
 		if (irq == 0)
 			return -1;
+
 		irq_want = irq + 1;
-		if (!irq_remapping_enabled)
-			goto no_ir;
 
-		if (!sub_handle) {
-			/*
-			 * allocate the consecutive block of IRTE's
-			 * for 'nvec'
-			 */
-			index = msi_alloc_remapped_irq(dev, irq, nvec);
-			if (index < 0) {
-				ret = index;
-				goto error;
-			}
-		} else {
-			ret = msi_setup_remapped_irq(dev, irq, index,
-						     sub_handle);
-			if (ret < 0)
-				goto error;
-		}
-no_ir:
 		ret = setup_msi_irq(dev, msidesc, irq);
 		if (ret < 0)
 			goto error;
-		sub_handle++;
 	}
 	return 0;
 
diff --git a/drivers/iommu/irq_remapping.c b/drivers/iommu/irq_remapping.c
index f439151..984104b 100644
--- a/drivers/iommu/irq_remapping.c
+++ b/drivers/iommu/irq_remapping.c
@@ -4,6 +4,8 @@
 #include <linux/cpumask.h>
 #include <linux/errno.h>
 #include <linux/msi.h>
+#include <linux/irq.h>
+#include <linux/pci.h>
 
 #include <asm/hw_irq.h>
 #include <asm/irq_remapping.h>
@@ -21,6 +23,10 @@ int no_x2apic_optout;
 
 static struct irq_remap_ops *remap_ops;
 
+static int msi_alloc_remapped_irq(struct pci_dev *pdev, int irq, int nvec);
+static int msi_setup_remapped_irq(struct pci_dev *pdev, unsigned int irq,
+				  int index, int sub_handle);
+
 static void irq_remapping_disable_io_apic(void)
 {
 	/*
@@ -34,9 +40,54 @@ static void irq_remapping_disable_io_apic(void)
 		disconnect_bsp_APIC(0);
 }
 
+static int irq_remapping_setup_msi_irqs(struct pci_dev *dev,
+					int nvec, int type)
+{
+	int node, ret, sub_handle, index = 0;
+	struct msi_desc *msidesc;
+	unsigned int irq;
+
+	/* We don't support multiple MSI yet */
+	if (type == PCI_CAP_ID_MSI && nvec > 1)
+		return 1;
+
+	node		= dev_to_node(&dev->dev);
+	irq		= get_nr_irqs_gsi();
+	sub_handle	= 0;
+
+	list_for_each_entry(msidesc, &dev->msi_list, list) {
+
+		irq = create_irq_nr(irq, node);
+		if (irq == 0)
+			return -1;
+
+		if (sub_handle == 0)
+			ret = index = msi_alloc_remapped_irq(dev, irq, nvec);
+		else
+			ret = msi_setup_remapped_irq(dev, irq, index, sub_handle);
+
+		if (ret < 0)
+			goto error;
+
+		ret = setup_msi_irq(dev, msidesc, irq);
+		if (ret < 0)
+			goto error;
+
+		sub_handle += 1;
+		irq        += 1;
+	}
+
+	return 0;
+
+error:
+	destroy_irq(irq);
+	return ret;
+}
+
 static void __init irq_remapping_modify_x86_ops(void)
 {
 	x86_io_apic_ops.disable		= irq_remapping_disable_io_apic;
+	x86_msi.setup_msi_irqs		= irq_remapping_setup_msi_irqs;
 	x86_msi.setup_hpet_msi		= setup_hpet_msi_remapped;
 }
 
@@ -181,7 +232,7 @@ void compose_remapped_msi_msg(struct pci_dev *pdev,
 	remap_ops->compose_msi_msg(pdev, irq, dest, msg, hpet_id);
 }
 
-int msi_alloc_remapped_irq(struct pci_dev *pdev, int irq, int nvec)
+static int msi_alloc_remapped_irq(struct pci_dev *pdev, int irq, int nvec)
 {
 	if (!remap_ops || !remap_ops->msi_alloc_irq)
 		return -ENODEV;
@@ -189,8 +240,8 @@ int msi_alloc_remapped_irq(struct pci_dev *pdev, int irq, int nvec)
 	return remap_ops->msi_alloc_irq(pdev, irq, nvec);
 }
 
-int msi_setup_remapped_irq(struct pci_dev *pdev, unsigned int irq,
-			   int index, int sub_handle)
+static int msi_setup_remapped_irq(struct pci_dev *pdev, unsigned int irq,
+				  int index, int sub_handle)
 {
 	if (!remap_ops || !remap_ops->msi_setup_irq)
 		return -ENODEV;
-- 
1.7.9.5

Re: [PATCH 06/19] x86, msi: Use IRQ remapping specific setup_msi_irqs routine

[Sebastian Andrzej Siewior]

On Mon, Aug 20, 2012 at 03:55:52PM +0200, Joerg Roedel wrote:
> diff --git a/drivers/iommu/irq_remapping.c b/drivers/iommu/irq_remapping.c
> index f439151..984104b 100644
> --- a/drivers/iommu/irq_remapping.c
> +++ b/drivers/iommu/irq_remapping.c
> @@ -181,7 +232,7 @@ void compose_remapped_msi_msg(struct pci_dev *pdev,
>  	remap_ops->compose_msi_msg(pdev, irq, dest, msg, hpet_id);
>  }
>  
> -int msi_alloc_remapped_irq(struct pci_dev *pdev, int irq, int nvec)
> +static int msi_alloc_remapped_irq(struct pci_dev *pdev, int irq, int nvec)
>  {
>  	if (!remap_ops || !remap_ops->msi_alloc_irq)
>  		return -ENODEV;
> @@ -189,8 +240,8 @@ int msi_alloc_remapped_irq(struct pci_dev *pdev, int irq, int nvec)
>  	return remap_ops->msi_alloc_irq(pdev, irq, nvec);
>  }
>  
> -int msi_setup_remapped_irq(struct pci_dev *pdev, unsigned int irq,
> -			   int index, int sub_handle)
> +static int msi_setup_remapped_irq(struct pci_dev *pdev, unsigned int irq,
> +				  int index, int sub_handle)
>  {
>  	if (!remap_ops || !remap_ops->msi_setup_irq)
>  		return -ENODEV;

Both functions, that is msi_alloc_remapped_irq() and msi_setup_remapped_irq()
and are now called only from irq_remapping_setup_msi_irqs() which can only be
called once remap_ops are set.

Sebastian

[PATCH 07/19] x86, io_apic: Introduce set_affinity function pointer

[Joerg Roedel]

With interrupt remapping a special function is used to
change the affinity of an IO-APIC interrupt. Abstract this
with a function pointer.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/x86/include/asm/io_apic.h       |    4 ++++
 arch/x86/include/asm/irq_remapping.h |    9 ---------
 arch/x86/include/asm/x86_init.h      |    6 ++++++
 arch/x86/kernel/apic/io_apic.c       |   17 +++++++----------
 arch/x86/kernel/x86_init.c           |    1 +
 drivers/iommu/irq_remapping.c        |    4 ++++
 6 files changed, 22 insertions(+), 19 deletions(-)

diff --git a/arch/x86/include/asm/io_apic.h b/arch/x86/include/asm/io_apic.h
index 21aa81e..a744cbb 100644
--- a/arch/x86/include/asm/io_apic.h
+++ b/arch/x86/include/asm/io_apic.h
@@ -182,6 +182,9 @@ extern void native_io_apic_modify(unsigned int apic, unsigned int reg, unsigned
 extern void native_disable_io_apic(void);
 extern void native_io_apic_print_entries(unsigned int apic, unsigned int nr_entries);
 extern void intel_ir_io_apic_print_entries(unsigned int apic, unsigned int nr_entries);
+extern int native_ioapic_set_affinity(struct irq_data *,
+				      const struct cpumask *,
+				      bool);
 
 static inline unsigned int io_apic_read(unsigned int apic, unsigned int reg)
 {
@@ -228,6 +231,7 @@ static inline void disable_ioapic_support(void) { }
 #define native_io_apic_modify		NULL
 #define native_disable_io_apic		NULL
 #define native_io_apic_print_entries	NULL
+#define native_ioapic_set_affinity	NULL
 #endif
 
 #endif /* _ASM_X86_IO_APIC_H */
diff --git a/arch/x86/include/asm/irq_remapping.h b/arch/x86/include/asm/irq_remapping.h
index 0ee1e88..f1afa04 100644
--- a/arch/x86/include/asm/irq_remapping.h
+++ b/arch/x86/include/asm/irq_remapping.h
@@ -40,9 +40,6 @@ extern int setup_ioapic_remapped_entry(int irq,
 				       unsigned int destination,
 				       int vector,
 				       struct io_apic_irq_attr *attr);
-extern int set_remapped_irq_affinity(struct irq_data *data,
-				     const struct cpumask *mask,
-				     bool force);
 extern void free_remapped_irq(int irq);
 extern void compose_remapped_msi_msg(struct pci_dev *pdev,
 				     unsigned int irq, unsigned int dest,
@@ -68,12 +65,6 @@ static inline int setup_ioapic_remapped_entry(int irq,
 {
 	return -ENODEV;
 }
-static inline int set_remapped_irq_affinity(struct irq_data *data,
-					    const struct cpumask *mask,
-					    bool force)
-{
-	return 0;
-}
 static inline void free_remapped_irq(int irq) { }
 static inline void compose_remapped_msi_msg(struct pci_dev *pdev,
 					    unsigned int irq, unsigned int dest,
diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
index 1f49c8a..f5f712b 100644
--- a/arch/x86/include/asm/x86_init.h
+++ b/arch/x86/include/asm/x86_init.h
@@ -189,6 +189,9 @@ struct x86_msi_ops {
 	int  (*setup_hpet_msi)(unsigned int irq, unsigned int id);
 };
 
+struct irq_data;
+struct cpumask;
+
 struct x86_io_apic_ops {
 	void		(*init)   (void);
 	unsigned int	(*read)   (unsigned int apic, unsigned int reg);
@@ -196,6 +199,9 @@ struct x86_io_apic_ops {
 	void		(*modify) (unsigned int apic, unsigned int reg, unsigned int value);
 	void		(*disable)(void);
 	void		(*print_entries)(unsigned int apic, unsigned int nr_entries);
+	int		(*set_affinity)(struct irq_data *data,
+					const struct cpumask *mask,
+					bool force);
 };
 
 extern struct x86_init_ops x86_init;
diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index 811e930..fb91d1a 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -2368,9 +2368,10 @@ int __ioapic_set_affinity(struct irq_data *data, const struct cpumask *mask,
 	return 0;
 }
 
-static int
-ioapic_set_affinity(struct irq_data *data, const struct cpumask *mask,
-		    bool force)
+
+int native_ioapic_set_affinity(struct irq_data *data,
+			       const struct cpumask *mask,
+			       bool force)
 {
 	unsigned int dest, irq = data->irq;
 	unsigned long flags;
@@ -2569,8 +2570,7 @@ static void irq_remap_modify_chip_defaults(struct irq_chip *chip)
 	chip->irq_print_chip = ir_print_prefix;
 	chip->irq_ack = ir_ack_apic_edge;
 	chip->irq_eoi = ir_ack_apic_level;
-
-	chip->irq_set_affinity = set_remapped_irq_affinity;
+	chip->irq_set_affinity = x86_io_apic_ops.set_affinity;
 }
 #endif /* CONFIG_IRQ_REMAP */
 
@@ -2581,7 +2581,7 @@ static struct irq_chip ioapic_chip __read_mostly = {
 	.irq_unmask		= unmask_ioapic_irq,
 	.irq_ack		= ack_apic_edge,
 	.irq_eoi		= ack_apic_level,
-	.irq_set_affinity	= ioapic_set_affinity,
+	.irq_set_affinity	= native_ioapic_set_affinity,
 	.irq_retrigger		= ioapic_retrigger_irq,
 };
 
@@ -3656,10 +3656,7 @@ void __init setup_ioapic_dest(void)
 		else
 			mask = apic->target_cpus();
 
-		if (irq_remapping_enabled)
-			set_remapped_irq_affinity(idata, mask, false);
-		else
-			ioapic_set_affinity(idata, mask, false);
+		x86_io_apic_ops.set_affinity(idata, mask, false);
 	}
 
 }
diff --git a/arch/x86/kernel/x86_init.c b/arch/x86/kernel/x86_init.c
index 339e108..8806d0b 100644
--- a/arch/x86/kernel/x86_init.c
+++ b/arch/x86/kernel/x86_init.c
@@ -128,4 +128,5 @@ struct x86_io_apic_ops x86_io_apic_ops = {
 	.modify			= native_io_apic_modify,
 	.disable		= native_disable_io_apic,
 	.print_entries		= native_io_apic_print_entries,
+	.set_affinity		= native_ioapic_set_affinity,
 };
diff --git a/drivers/iommu/irq_remapping.c b/drivers/iommu/irq_remapping.c
index 984104b..ccf87e5 100644
--- a/drivers/iommu/irq_remapping.c
+++ b/drivers/iommu/irq_remapping.c
@@ -26,6 +26,9 @@ static struct irq_remap_ops *remap_ops;
 static int msi_alloc_remapped_irq(struct pci_dev *pdev, int irq, int nvec);
 static int msi_setup_remapped_irq(struct pci_dev *pdev, unsigned int irq,
 				  int index, int sub_handle);
+static int set_remapped_irq_affinity(struct irq_data *data,
+				     const struct cpumask *mask,
+				     bool force);
 
 static void irq_remapping_disable_io_apic(void)
 {
@@ -87,6 +90,7 @@ error:
 static void __init irq_remapping_modify_x86_ops(void)
 {
 	x86_io_apic_ops.disable		= irq_remapping_disable_io_apic;
+	x86_io_apic_ops.set_affinity	= set_remapped_irq_affinity;
 	x86_msi.setup_msi_irqs		= irq_remapping_setup_msi_irqs;
 	x86_msi.setup_hpet_msi		= setup_hpet_msi_remapped;
 }
-- 
1.7.9.5

[PATCH 08/19] x86, io_apic: Convert setup_ioapic_entry to function pointer

[Joerg Roedel]

This pointer is changed to a different function when IRQ
remapping is enabled.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/x86/include/asm/io_apic.h  |    5 +++++
 arch/x86/include/asm/x86_init.h |    5 +++++
 arch/x86/kernel/apic/io_apic.c  |   14 +++++---------
 arch/x86/kernel/x86_init.c      |    1 +
 drivers/iommu/irq_remapping.c   |    1 +
 5 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/arch/x86/include/asm/io_apic.h b/arch/x86/include/asm/io_apic.h
index a744cbb..71f5f08 100644
--- a/arch/x86/include/asm/io_apic.h
+++ b/arch/x86/include/asm/io_apic.h
@@ -149,6 +149,10 @@ extern int io_apic_set_pci_routing(struct device *dev, int irq,
 void setup_IO_APIC_irq_extra(u32 gsi);
 extern void ioapic_insert_resources(void);
 
+extern int native_setup_ioapic_entry(int, struct IO_APIC_route_entry *,
+				     unsigned int, int,
+				     struct io_apic_irq_attr *);
+
 int io_apic_setup_irq_pin_once(unsigned int irq, int node, struct io_apic_irq_attr *attr);
 
 extern int save_ioapic_entries(void);
@@ -232,6 +236,7 @@ static inline void disable_ioapic_support(void) { }
 #define native_disable_io_apic		NULL
 #define native_io_apic_print_entries	NULL
 #define native_ioapic_set_affinity	NULL
+#define native_setup_ioapic_entry	NULL
 #endif
 
 #endif /* _ASM_X86_IO_APIC_H */
diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
index f5f712b..bc13022 100644
--- a/arch/x86/include/asm/x86_init.h
+++ b/arch/x86/include/asm/x86_init.h
@@ -189,6 +189,8 @@ struct x86_msi_ops {
 	int  (*setup_hpet_msi)(unsigned int irq, unsigned int id);
 };
 
+struct IO_APIC_route_entry;
+struct io_apic_irq_attr;
 struct irq_data;
 struct cpumask;
 
@@ -202,6 +204,9 @@ struct x86_io_apic_ops {
 	int		(*set_affinity)(struct irq_data *data,
 					const struct cpumask *mask,
 					bool force);
+	int		(*setup_entry)(int irq, struct IO_APIC_route_entry *entry,
+				       unsigned int destination, int vector,
+				       struct io_apic_irq_attr *attr);
 };
 
 extern struct x86_init_ops x86_init;
diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index fb91d1a..5d372be 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -1319,14 +1319,10 @@ static void ioapic_register_intr(unsigned int irq, struct irq_cfg *cfg,
 				      fasteoi ? "fasteoi" : "edge");
 }
 
-static int setup_ioapic_entry(int irq, struct IO_APIC_route_entry *entry,
-			       unsigned int destination, int vector,
-			       struct io_apic_irq_attr *attr)
+int native_setup_ioapic_entry(int irq, struct IO_APIC_route_entry *entry,
+			      unsigned int destination, int vector,
+			      struct io_apic_irq_attr *attr)
 {
-	if (irq_remapping_enabled)
-		return setup_ioapic_remapped_entry(irq, entry, destination,
-						   vector, attr);
-
 	memset(entry, 0, sizeof(*entry));
 
 	entry->delivery_mode = apic->irq_delivery_mode;
@@ -1374,8 +1370,8 @@ static void setup_ioapic_irq(unsigned int irq, struct irq_cfg *cfg,
 		    attr->ioapic, mpc_ioapic_id(attr->ioapic), attr->ioapic_pin,
 		    cfg->vector, irq, attr->trigger, attr->polarity, dest);
 
-	if (setup_ioapic_entry(irq, &entry, dest, cfg->vector, attr)) {
-		pr_warn("Failed to setup ioapic entry for ioapic %d, pin %d\n",
+	if (x86_io_apic_ops.setup_entry(irq, &entry, dest, cfg->vector, attr)) {
+		pr_warn("Failed to setup ioapic entry for ioapic  %d, pin %d\n",
 			mpc_ioapic_id(attr->ioapic), attr->ioapic_pin);
 		__clear_irq_vector(irq, cfg);
 
diff --git a/arch/x86/kernel/x86_init.c b/arch/x86/kernel/x86_init.c
index 8806d0b..3cd6bf7 100644
--- a/arch/x86/kernel/x86_init.c
+++ b/arch/x86/kernel/x86_init.c
@@ -129,4 +129,5 @@ struct x86_io_apic_ops x86_io_apic_ops = {
 	.disable		= native_disable_io_apic,
 	.print_entries		= native_io_apic_print_entries,
 	.set_affinity		= native_ioapic_set_affinity,
+	.setup_entry		= native_setup_ioapic_entry,
 };
diff --git a/drivers/iommu/irq_remapping.c b/drivers/iommu/irq_remapping.c
index ccf87e5..5b28e28 100644
--- a/drivers/iommu/irq_remapping.c
+++ b/drivers/iommu/irq_remapping.c
@@ -91,6 +91,7 @@ static void __init irq_remapping_modify_x86_ops(void)
 {
 	x86_io_apic_ops.disable		= irq_remapping_disable_io_apic;
 	x86_io_apic_ops.set_affinity	= set_remapped_irq_affinity;
+	x86_io_apic_ops.setup_entry	= setup_ioapic_remapped_entry;
 	x86_msi.setup_msi_irqs		= irq_remapping_setup_msi_irqs;
 	x86_msi.setup_hpet_msi		= setup_hpet_msi_remapped;
 }
-- 
1.7.9.5

[PATCH 09/19] x86, io_apic: Move irq_remapping_enabled checks out of check_timer()

[Joerg Roedel]

Move these checks to IRQ remapping code by introducing the
panic_on_irq_remap() function.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/x86/include/asm/irq_remapping.h |    5 +++++
 arch/x86/kernel/apic/io_apic.c       |    6 ++----
 drivers/iommu/irq_remapping.c        |    6 ++++++
 3 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/irq_remapping.h b/arch/x86/include/asm/irq_remapping.h
index f1afa04..fb99a73 100644
--- a/arch/x86/include/asm/irq_remapping.h
+++ b/arch/x86/include/asm/irq_remapping.h
@@ -45,6 +45,7 @@ extern void compose_remapped_msi_msg(struct pci_dev *pdev,
 				     unsigned int irq, unsigned int dest,
 				     struct msi_msg *msg, u8 hpet_id);
 extern int setup_hpet_msi_remapped(unsigned int irq, unsigned int id);
+extern void panic_if_irq_remap(const char *msg);
 
 #else  /* CONFIG_IRQ_REMAP */
 
@@ -75,6 +76,10 @@ static inline int setup_hpet_msi_remapped(unsigned int irq, unsigned int id)
 {
 	return -ENODEV;
 }
+
+static inline void panic_if_irq_remap(const char *msg)
+{
+}
 #endif /* CONFIG_IRQ_REMAP */
 
 #endif /* __X86_IRQ_REMAPPING_H */
diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index 5d372be..8816280 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -2776,8 +2776,7 @@ static inline void __init check_timer(void)
 	 * 8259A.
 	 */
 	if (pin1 == -1) {
-		if (irq_remapping_enabled)
-			panic("BIOS bug: timer not connected to IO-APIC");
+		panic_if_irq_remap("BIOS bug: timer not connected to IO-APIC");
 		pin1 = pin2;
 		apic1 = apic2;
 		no_pin1 = 1;
@@ -2809,8 +2808,7 @@ static inline void __init check_timer(void)
 				clear_IO_APIC_pin(0, pin1);
 			goto out;
 		}
-		if (irq_remapping_enabled)
-			panic("timer doesn't work through Interrupt-remapped IO-APIC");
+		panic_if_irq_remap("timer doesn't work through Interrupt-remapped IO-APIC");
 		local_irq_disable();
 		clear_IO_APIC_pin(apic1, pin1);
 		if (!no_pin1)
diff --git a/drivers/iommu/irq_remapping.c b/drivers/iommu/irq_remapping.c
index 5b28e28..827f8a1 100644
--- a/drivers/iommu/irq_remapping.c
+++ b/drivers/iommu/irq_remapping.c
@@ -261,3 +261,9 @@ int setup_hpet_msi_remapped(unsigned int irq, unsigned int id)
 
 	return remap_ops->setup_hpet_msi(irq, id);
 }
+
+void panic_if_irq_remap(const char *msg)
+{
+	if (irq_remapping_enabled)
+		panic(msg);
+}
-- 
1.7.9.5

[PATCH 10/19] x86, io_apic: Introduce function pointer for setup_timer_IRQ0_pin

[Joerg Roedel]

This function must be a NOP with interrupt remapping
enabled. So use a funtion pointer for it which points to an
empty function when irq_remapping_enabled == true.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/x86/include/asm/io_apic.h  |    6 ++++++
 arch/x86/include/asm/x86_init.h |    3 +++
 arch/x86/kernel/apic/io_apic.c  |   11 ++++-------
 arch/x86/kernel/x86_init.c      |    1 +
 drivers/iommu/irq_remapping.c   |    7 +++++++
 5 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/arch/x86/include/asm/io_apic.h b/arch/x86/include/asm/io_apic.h
index 71f5f08..bb5caee 100644
--- a/arch/x86/include/asm/io_apic.h
+++ b/arch/x86/include/asm/io_apic.h
@@ -152,6 +152,11 @@ extern void ioapic_insert_resources(void);
 extern int native_setup_ioapic_entry(int, struct IO_APIC_route_entry *,
 				     unsigned int, int,
 				     struct io_apic_irq_attr *);
+extern int native_setup_ioapic_entry(int, struct IO_APIC_route_entry *,
+				     unsigned int, int,
+				     struct io_apic_irq_attr *);
+extern void __init native_setup_timer_pin(unsigned int ioapic_idx,
+					  unsigned int pin, int vector);
 
 int io_apic_setup_irq_pin_once(unsigned int irq, int node, struct io_apic_irq_attr *attr);
 
@@ -237,6 +242,7 @@ static inline void disable_ioapic_support(void) { }
 #define native_io_apic_print_entries	NULL
 #define native_ioapic_set_affinity	NULL
 #define native_setup_ioapic_entry	NULL
+#define native_setup_timer_pin		NULL
 #endif
 
 #endif /* _ASM_X86_IO_APIC_H */
diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
index bc13022..ffe5860 100644
--- a/arch/x86/include/asm/x86_init.h
+++ b/arch/x86/include/asm/x86_init.h
@@ -207,6 +207,9 @@ struct x86_io_apic_ops {
 	int		(*setup_entry)(int irq, struct IO_APIC_route_entry *entry,
 				       unsigned int destination, int vector,
 				       struct io_apic_irq_attr *attr);
+	void		(*setup_timer_pin)(unsigned int ioapic_idx,
+					   unsigned int pin, int vector);
+
 };
 
 extern struct x86_init_ops x86_init;
diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index 8816280..22e6bef 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -1473,15 +1473,12 @@ void setup_IO_APIC_irq_extra(u32 gsi)
 /*
  * Set up the timer pin, possibly with the 8259A-master behind.
  */
-static void __init setup_timer_IRQ0_pin(unsigned int ioapic_idx,
-					unsigned int pin, int vector)
+void __init native_setup_timer_pin(unsigned int ioapic_idx,
+				   unsigned int pin, int vector)
 {
 	struct IO_APIC_route_entry entry;
 	unsigned int dest;
 
-	if (irq_remapping_enabled)
-		return;
-
 	memset(&entry, 0, sizeof(entry));
 
 	/*
@@ -2791,7 +2788,7 @@ static inline void __init check_timer(void)
 		 */
 		if (no_pin1) {
 			add_pin_to_irq_node(cfg, node, apic1, pin1);
-			setup_timer_IRQ0_pin(apic1, pin1, cfg->vector);
+			x86_io_apic_ops.setup_timer_pin(apic1, pin1, cfg->vector);
 		} else {
 			/* for edge trigger, setup_ioapic_irq already
 			 * leave it unmasked.
@@ -2823,7 +2820,7 @@ static inline void __init check_timer(void)
 		 * legacy devices should be connected to IO APIC #0
 		 */
 		replace_pin_at_irq_node(cfg, node, apic1, pin1, apic2, pin2);
-		setup_timer_IRQ0_pin(apic2, pin2, cfg->vector);
+		x86_io_apic_ops.setup_timer_pin(apic2, pin2, cfg->vector);
 		legacy_pic->unmask(0);
 		if (timer_irq_works()) {
 			apic_printk(APIC_QUIET, KERN_INFO "....... works.\n");
diff --git a/arch/x86/kernel/x86_init.c b/arch/x86/kernel/x86_init.c
index 3cd6bf7..eba02e5 100644
--- a/arch/x86/kernel/x86_init.c
+++ b/arch/x86/kernel/x86_init.c
@@ -130,4 +130,5 @@ struct x86_io_apic_ops x86_io_apic_ops = {
 	.print_entries		= native_io_apic_print_entries,
 	.set_affinity		= native_ioapic_set_affinity,
 	.setup_entry		= native_setup_ioapic_entry,
+	.setup_timer_pin	= native_setup_timer_pin,
 };
diff --git a/drivers/iommu/irq_remapping.c b/drivers/iommu/irq_remapping.c
index 827f8a1..71824fc 100644
--- a/drivers/iommu/irq_remapping.c
+++ b/drivers/iommu/irq_remapping.c
@@ -87,11 +87,18 @@ error:
 	return ret;
 }
 
+static void __init irq_remapping_setup_timer_pin(unsigned int ioapic_idx,
+						 unsigned int pin, int vector)
+{
+	/* Not needed with interrupt remapping */
+}
+
 static void __init irq_remapping_modify_x86_ops(void)
 {
 	x86_io_apic_ops.disable		= irq_remapping_disable_io_apic;
 	x86_io_apic_ops.set_affinity	= set_remapped_irq_affinity;
 	x86_io_apic_ops.setup_entry	= setup_ioapic_remapped_entry;
+	x86_io_apic_ops.setup_timer_pin	= irq_remapping_setup_timer_pin;
 	x86_msi.setup_msi_irqs		= irq_remapping_setup_msi_irqs;
 	x86_msi.setup_hpet_msi		= setup_hpet_msi_remapped;
 }
-- 
1.7.9.5

Re: [PATCH 10/19] x86, io_apic: Introduce function pointer for setup_timer_IRQ0_pin

[Sebastian Andrzej Siewior]

On Mon, Aug 20, 2012 at 03:55:56PM +0200, Joerg Roedel wrote:
> diff --git a/drivers/iommu/irq_remapping.c b/drivers/iommu/irq_remapping.c
> index 827f8a1..71824fc 100644
> --- a/drivers/iommu/irq_remapping.c
> +++ b/drivers/iommu/irq_remapping.c
> @@ -87,11 +87,18 @@ error:
>  	return ret;
>  }
>  
> +static void __init irq_remapping_setup_timer_pin(unsigned int ioapic_idx,
> +						 unsigned int pin, int vector)
> +{
> +	/* Not needed with interrupt remapping */

I see that this is not needed because it is empty. But *why* isn't it needed?
That comment would make sense here. So please drop this or explain :)

> +}
> +

Sebastian

Re: [PATCH 10/19] x86, io_apic: Introduce function pointer for setup_timer_IRQ0_pin

[Joerg Roedel]

On Sun, Aug 26, 2012 at 08:16:03PM +0200, Sebastian Andrzej Siewior wrote:
> On Mon, Aug 20, 2012 at 03:55:56PM +0200, Joerg Roedel wrote:
> > diff --git a/drivers/iommu/irq_remapping.c b/drivers/iommu/irq_remapping.c
> > index 827f8a1..71824fc 100644
> > --- a/drivers/iommu/irq_remapping.c
> > +++ b/drivers/iommu/irq_remapping.c
> > @@ -87,11 +87,18 @@ error:
> >  	return ret;
> >  }
> >  
> > +static void __init irq_remapping_setup_timer_pin(unsigned int ioapic_idx,
> > +						 unsigned int pin, int vector)
> > +{
> > +	/* Not needed with interrupt remapping */
> 
> I see that this is not needed because it is empty. But *why* isn't it needed?
> That comment would make sense here. So please drop this or explain :)

Well, the function is never called with interrupt-remapping enabled
anyway, so it is not needed. On the other side this makes the complete
patch pointless :) I'll remove it from the patch-set.


	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

[PATCH 11/19] x86, irq: Move irq_remapping_enabled declaration to iommu code

[Joerg Roedel]

Remove the last left-over from this flag from x86 code.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/x86/include/asm/irq_remapping.h |    4 ----
 drivers/iommu/dmar.c                 |    2 ++
 drivers/iommu/intel-iommu.c          |    2 ++
 drivers/iommu/irq_remapping.h        |    3 +++
 4 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/arch/x86/include/asm/irq_remapping.h b/arch/x86/include/asm/irq_remapping.h
index fb99a73..6f4b48b 100644
--- a/arch/x86/include/asm/irq_remapping.h
+++ b/arch/x86/include/asm/irq_remapping.h
@@ -26,8 +26,6 @@
 
 #ifdef CONFIG_IRQ_REMAP
 
-extern int irq_remapping_enabled;
-
 extern void setup_irq_remapping_ops(void);
 extern int irq_remapping_supported(void);
 extern int irq_remapping_prepare(void);
@@ -49,8 +47,6 @@ extern void panic_if_irq_remap(const char *msg);
 
 #else  /* CONFIG_IRQ_REMAP */
 
-#define irq_remapping_enabled	0
-
 static inline void setup_irq_remapping_ops(void) { }
 static inline int irq_remapping_supported(void) { return 0; }
 static inline int irq_remapping_prepare(void) { return -ENODEV; }
diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
index 86e2f4a..174bb65 100644
--- a/drivers/iommu/dmar.c
+++ b/drivers/iommu/dmar.c
@@ -41,6 +41,8 @@
 #include <asm/irq_remapping.h>
 #include <asm/iommu_table.h>
 
+#include "irq_remapping.h"
+
 /* No locks are needed as DMA remapping hardware unit
  * list is constructed at boot time and hotplug of
  * these units are not supported by the architecture.
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 2297ec1..6d713db 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -46,6 +46,8 @@
 #include <asm/cacheflush.h>
 #include <asm/iommu.h>
 
+#include "irq_remapping.h"
+
 #define ROOT_SIZE		VTD_PAGE_SIZE
 #define CONTEXT_SIZE		VTD_PAGE_SIZE
 
diff --git a/drivers/iommu/irq_remapping.h b/drivers/iommu/irq_remapping.h
index b12974c..1003297 100644
--- a/drivers/iommu/irq_remapping.h
+++ b/drivers/iommu/irq_remapping.h
@@ -34,6 +34,7 @@ struct msi_msg;
 extern int disable_irq_remap;
 extern int disable_sourceid_checking;
 extern int no_x2apic_optout;
+extern int irq_remapping_enabled;
 
 struct irq_remap_ops {
 	/* Check whether Interrupt Remapping is supported */
@@ -83,6 +84,8 @@ struct irq_remap_ops {
 
 extern struct irq_remap_ops intel_irq_remap_ops;
 
+#else  /* CONFIG_IRQ_REMAP */
+#define irq_remapping_enabled 0
 #endif /* CONFIG_IRQ_REMAP */
 
 #endif /* __IRQ_REMAPPING_H */
-- 
1.7.9.5

[PATCH 12/19] x86, irq: Add data structure to keep AMD specific irq remapping information

[Joerg Roedel]

Add a data structure to store information the IOMMU driver
can use to get from a 'struct irq_cfg' to the remapping
entry.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/x86/include/asm/hw_irq.h |   12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/hw_irq.h b/arch/x86/include/asm/hw_irq.h
index eb92a6e..fc89a2a 100644
--- a/arch/x86/include/asm/hw_irq.h
+++ b/arch/x86/include/asm/hw_irq.h
@@ -101,6 +101,7 @@ static inline void set_io_apic_irq_attr(struct io_apic_irq_attr *irq_attr,
 	irq_attr->polarity	= polarity;
 }
 
+/* Intel specific interrupt remapping information */
 struct irq_2_iommu {
 	struct intel_iommu *iommu;
 	u16 irte_index;
@@ -108,6 +109,12 @@ struct irq_2_iommu {
 	u8  irte_mask;
 };
 
+/* AMD specific interrupt remapping information */
+struct irq_2_irte {
+	u16 devid; /* Device ID for IRTE table */
+	u16 index; /* Index into IRTE table*/
+};
+
 /*
  * This is performance-critical, we want to do it O(1)
  *
@@ -120,7 +127,10 @@ struct irq_cfg {
 	u8			vector;
 	u8			move_in_progress : 1;
 #ifdef CONFIG_IRQ_REMAP
-	struct irq_2_iommu	irq_2_iommu;
+	union {
+		struct irq_2_iommu irq_2_iommu;
+		struct irq_2_irte  irq_2_irte;
+	};
 #endif
 };
 
-- 
1.7.9.5

Re: [PATCH 12/19] x86, irq: Add data structure to keep AMD specific irq remapping information

[Sebastian Andrzej Siewior]

On Mon, Aug 20, 2012 at 03:55:58PM +0200, Joerg Roedel wrote:
> --- a/arch/x86/include/asm/hw_irq.h
> +++ b/arch/x86/include/asm/hw_irq.h
> @@ -101,6 +101,7 @@ static inline void set_io_apic_irq_attr(struct io_apic_irq_attr *irq_attr,
>  	irq_attr->polarity	= polarity;
>  }
>  
> +/* Intel specific interrupt remapping information */
>  struct irq_2_iommu {
>  	struct intel_iommu *iommu;
>  	u16 irte_index;
> @@ -108,6 +109,12 @@ struct irq_2_iommu {
>  	u8  irte_mask;
>  };
>  
> +/* AMD specific interrupt remapping information */
> +struct irq_2_irte {
> +	u16 devid; /* Device ID for IRTE table */
> +	u16 index; /* Index into IRTE table*/
> +};
so irte_index like in iommu case would make the comment superfluous. However
the struct is already named _irte...
However, the "{Intel|AMD} specific..." is helpfull since irte and such are
present in both.

Sebastian

Re: [PATCH 12/19] x86, irq: Add data structure to keep AMD specific irq remapping information

[Joerg Roedel]

On Sun, Aug 26, 2012 at 08:21:26PM +0200, Sebastian Andrzej Siewior wrote:
> On Mon, Aug 20, 2012 at 03:55:58PM +0200, Joerg Roedel wrote:
> > --- a/arch/x86/include/asm/hw_irq.h
> > +++ b/arch/x86/include/asm/hw_irq.h
> > @@ -101,6 +101,7 @@ static inline void set_io_apic_irq_attr(struct io_apic_irq_attr *irq_attr,
> >  	irq_attr->polarity	= polarity;
> >  }
> >  
> > +/* Intel specific interrupt remapping information */
> >  struct irq_2_iommu {
> >  	struct intel_iommu *iommu;
> >  	u16 irte_index;
> > @@ -108,6 +109,12 @@ struct irq_2_iommu {
> >  	u8  irte_mask;
> >  };
> >  
> > +/* AMD specific interrupt remapping information */
> > +struct irq_2_irte {
> > +	u16 devid; /* Device ID for IRTE table */
> > +	u16 index; /* Index into IRTE table*/
> > +};
> so irte_index like in iommu case would make the comment superfluous. However
> the struct is already named _irte...
> However, the "{Intel|AMD} specific..." is helpfull since irte and such are
> present in both.

Yes, the structs also represent the differences in the remapping
hardware between Intel and AMD very well. On Intel there is one
remapping table per hardware IOMMU so it is important to get from a
given irq to the IOMMU and its remapping table to find the IRTE.

On AMD hardware there is a remapping table per device. So the
coresponding device id is stored in the struct to find the right
remapping table.


	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

[PATCH 13/19] x86, io-apic: Move CONFIG_IRQ_REMAP code out of x86 core

[Joerg Roedel]

Move all the code to either to the header file
asm/irq_remapping.h or to drivers/iommu/.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/x86/include/asm/hw_irq.h        |    1 +
 arch/x86/include/asm/io_apic.h       |    2 ++
 arch/x86/include/asm/irq_remapping.h |   17 +++++++++++++
 arch/x86/kernel/apic/io_apic.c       |   44 +---------------------------------
 drivers/iommu/intel_irq_remapping.c  |    4 ++++
 drivers/iommu/irq_remapping.c        |   25 +++++++++++++++++++
 6 files changed, 50 insertions(+), 43 deletions(-)

diff --git a/arch/x86/include/asm/hw_irq.h b/arch/x86/include/asm/hw_irq.h
index fc89a2a..c84403e 100644
--- a/arch/x86/include/asm/hw_irq.h
+++ b/arch/x86/include/asm/hw_irq.h
@@ -127,6 +127,7 @@ struct irq_cfg {
 	u8			vector;
 	u8			move_in_progress : 1;
 #ifdef CONFIG_IRQ_REMAP
+	bool			remapped;
 	union {
 		struct irq_2_iommu irq_2_iommu;
 		struct irq_2_irte  irq_2_irte;
diff --git a/arch/x86/include/asm/io_apic.h b/arch/x86/include/asm/io_apic.h
index bb5caee..0db4429 100644
--- a/arch/x86/include/asm/io_apic.h
+++ b/arch/x86/include/asm/io_apic.h
@@ -144,6 +144,7 @@ extern int timer_through_8259;
 	(mp_irq_entries && !skip_ioapic_setup && io_apic_irqs)
 
 struct io_apic_irq_attr;
+struct irq_cfg;
 extern int io_apic_set_pci_routing(struct device *dev, int irq,
 		 struct io_apic_irq_attr *irq_attr);
 void setup_IO_APIC_irq_extra(u32 gsi);
@@ -157,6 +158,7 @@ extern int native_setup_ioapic_entry(int, struct IO_APIC_route_entry *,
 				     struct io_apic_irq_attr *);
 extern void __init native_setup_timer_pin(unsigned int ioapic_idx,
 					  unsigned int pin, int vector);
+extern void eoi_ioapic_irq(unsigned int irq, struct irq_cfg *cfg);
 
 int io_apic_setup_irq_pin_once(unsigned int irq, int node, struct io_apic_irq_attr *attr);
 
diff --git a/arch/x86/include/asm/irq_remapping.h b/arch/x86/include/asm/irq_remapping.h
index 6f4b48b..1888148 100644
--- a/arch/x86/include/asm/irq_remapping.h
+++ b/arch/x86/include/asm/irq_remapping.h
@@ -45,6 +45,13 @@ extern void compose_remapped_msi_msg(struct pci_dev *pdev,
 extern int setup_hpet_msi_remapped(unsigned int irq, unsigned int id);
 extern void panic_if_irq_remap(const char *msg);
 
+static inline bool irq_remapped(struct irq_cfg *cfg)
+{
+	return cfg->remapped;
+}
+
+void irq_remap_modify_chip_defaults(struct irq_chip *chip);
+
 #else  /* CONFIG_IRQ_REMAP */
 
 static inline void setup_irq_remapping_ops(void) { }
@@ -76,6 +83,16 @@ static inline int setup_hpet_msi_remapped(unsigned int irq, unsigned int id)
 static inline void panic_if_irq_remap(const char *msg)
 {
 }
+
+static inline bool irq_remapped(struct irq_cfg *cfg)
+{
+	return false;
+}
+
+static inline void irq_remap_modify_chip_defaults(struct irq_chip *chip)
+{
+}
+
 #endif /* CONFIG_IRQ_REMAP */
 
 #endif /* __X86_IRQ_REMAPPING_H */
diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index 22e6bef..fa6bfb3 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -68,22 +68,6 @@
 #define for_each_irq_pin(entry, head) \
 	for (entry = head; entry; entry = entry->next)
 
-#ifdef CONFIG_IRQ_REMAP
-static void irq_remap_modify_chip_defaults(struct irq_chip *chip);
-static inline bool irq_remapped(struct irq_cfg *cfg)
-{
-	return cfg->irq_2_iommu.iommu != NULL;
-}
-#else
-static inline bool irq_remapped(struct irq_cfg *cfg)
-{
-	return false;
-}
-static inline void irq_remap_modify_chip_defaults(struct irq_chip *chip)
-{
-}
-#endif
-
 /*
  *      Is the SiS APIC rmw bug present ?
  *      -1 = don't know, 0 = no, 1 = yes
@@ -606,7 +590,7 @@ static void __eoi_ioapic_pin(int apic, int pin, int vector, struct irq_cfg *cfg)
 	}
 }
 
-static void eoi_ioapic_irq(unsigned int irq, struct irq_cfg *cfg)
+void eoi_ioapic_irq(unsigned int irq, struct irq_cfg *cfg)
 {
 	struct irq_pin_list *entry;
 	unsigned long flags;
@@ -2541,32 +2525,6 @@ static void ack_apic_level(struct irq_data *data)
 	ioapic_irqd_unmask(data, cfg, masked);
 }
 
-#ifdef CONFIG_IRQ_REMAP
-static void ir_ack_apic_edge(struct irq_data *data)
-{
-	ack_APIC_irq();
-}
-
-static void ir_ack_apic_level(struct irq_data *data)
-{
-	ack_APIC_irq();
-	eoi_ioapic_irq(data->irq, data->chip_data);
-}
-
-static void ir_print_prefix(struct irq_data *data, struct seq_file *p)
-{
-	seq_printf(p, " IR-%s", data->chip->name);
-}
-
-static void irq_remap_modify_chip_defaults(struct irq_chip *chip)
-{
-	chip->irq_print_chip = ir_print_prefix;
-	chip->irq_ack = ir_ack_apic_edge;
-	chip->irq_eoi = ir_ack_apic_level;
-	chip->irq_set_affinity = x86_io_apic_ops.set_affinity;
-}
-#endif /* CONFIG_IRQ_REMAP */
-
 static struct irq_chip ioapic_chip __read_mostly = {
 	.name			= "IO-APIC",
 	.irq_startup		= startup_ioapic_irq,
diff --git a/drivers/iommu/intel_irq_remapping.c b/drivers/iommu/intel_irq_remapping.c
index 321db1a..5299f5b 100644
--- a/drivers/iommu/intel_irq_remapping.c
+++ b/drivers/iommu/intel_irq_remapping.c
@@ -68,6 +68,7 @@ static int alloc_irte(struct intel_iommu *iommu, int irq, u16 count)
 {
 	struct ir_table *table = iommu->ir_table;
 	struct irq_2_iommu *irq_iommu = irq_2_iommu(irq);
+	struct irq_cfg *cfg = irq_get_chip_data(irq);
 	u16 index, start_index;
 	unsigned int mask = 0;
 	unsigned long flags;
@@ -115,6 +116,7 @@ static int alloc_irte(struct intel_iommu *iommu, int irq, u16 count)
 	for (i = index; i < index + count; i++)
 		table->base[i].present = 1;
 
+	cfg->remapped = true;
 	irq_iommu->iommu = iommu;
 	irq_iommu->irte_index =  index;
 	irq_iommu->sub_handle = 0;
@@ -155,6 +157,7 @@ static int map_irq_to_irte_handle(int irq, u16 *sub_handle)
 static int set_irte_irq(int irq, struct intel_iommu *iommu, u16 index, u16 subhandle)
 {
 	struct irq_2_iommu *irq_iommu = irq_2_iommu(irq);
+	struct irq_cfg *cfg = irq_get_chip_data(irq);
 	unsigned long flags;
 
 	if (!irq_iommu)
@@ -162,6 +165,7 @@ static int set_irte_irq(int irq, struct intel_iommu *iommu, u16 index, u16 subha
 
 	raw_spin_lock_irqsave(&irq_2_ir_lock, flags);
 
+	cfg->remapped = true;
 	irq_iommu->iommu = iommu;
 	irq_iommu->irte_index = index;
 	irq_iommu->sub_handle = subhandle;
diff --git a/drivers/iommu/irq_remapping.c b/drivers/iommu/irq_remapping.c
index 71824fc..3957fb2 100644
--- a/drivers/iommu/irq_remapping.c
+++ b/drivers/iommu/irq_remapping.c
@@ -1,3 +1,4 @@
+#include <linux/seq_file.h>
 #include <linux/cpumask.h>
 #include <linux/kernel.h>
 #include <linux/string.h>
@@ -274,3 +275,27 @@ void panic_if_irq_remap(const char *msg)
 	if (irq_remapping_enabled)
 		panic(msg);
 }
+
+static void ir_ack_apic_edge(struct irq_data *data)
+{
+	ack_APIC_irq();
+}
+
+static void ir_ack_apic_level(struct irq_data *data)
+{
+	ack_APIC_irq();
+	eoi_ioapic_irq(data->irq, data->chip_data);
+}
+
+static void ir_print_prefix(struct irq_data *data, struct seq_file *p)
+{
+	seq_printf(p, " IR-%s", data->chip->name);
+}
+
+void irq_remap_modify_chip_defaults(struct irq_chip *chip)
+{
+	chip->irq_print_chip = ir_print_prefix;
+	chip->irq_ack = ir_ack_apic_edge;
+	chip->irq_eoi = ir_ack_apic_level;
+	chip->irq_set_affinity = x86_io_apic_ops.set_affinity;
+}
-- 
1.7.9.5

Re: [PATCH 13/19] x86, io-apic: Move CONFIG_IRQ_REMAP code out of x86 core

[Sebastian Andrzej Siewior]

On Mon, Aug 20, 2012 at 03:55:59PM +0200, Joerg Roedel wrote:
> index fc89a2a..c84403e 100644
> --- a/arch/x86/include/asm/hw_irq.h
> +++ b/arch/x86/include/asm/hw_irq.h
> @@ -127,6 +127,7 @@ struct irq_cfg {
>  	u8			vector;
>  	u8			move_in_progress : 1;
>  #ifdef CONFIG_IRQ_REMAP
> +	bool			remapped;
  	u8			remapped : 1;
?

>  	union {
>  		struct irq_2_iommu irq_2_iommu;
>  		struct irq_2_irte  irq_2_irte;

Sebastian

[PATCH 14/19] x86, io-apic: Remove !irq_remapped() check from __target_IO_APIC_irq()

[Joerg Roedel]

This function is only called from default_ioapic_set_affinity()
which is only used when interrupt remapping is disabled. So
the check will always evaluate as true and can be removed.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/x86/kernel/apic/io_apic.c |    8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index fa6bfb3..e95325f 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -2298,12 +2298,8 @@ static void __target_IO_APIC_irq(unsigned int irq, unsigned int dest, struct irq
 
 		apic = entry->apic;
 		pin = entry->pin;
-		/*
-		 * With interrupt-remapping, destination information comes
-		 * from interrupt-remapping table entry.
-		 */
-		if (!irq_remapped(cfg))
-			io_apic_write(apic, 0x11 + pin*2, dest);
+
+		io_apic_write(apic, 0x11 + pin*2, dest);
 		reg = io_apic_read(apic, 0x10 + pin*2);
 		reg &= ~IO_APIC_REDIR_VECTOR_MASK;
 		reg |= vector;
-- 
1.7.9.5

Re: [PATCH 14/19] x86, io-apic: Remove !irq_remapped() check from __target_IO_APIC_irq()

[Sebastian Andrzej Siewior]

On Mon, Aug 20, 2012 at 03:56:00PM +0200, Joerg Roedel wrote:
> This function is only called from default_ioapic_set_affinity()
> which is only used when interrupt remapping is disabled. So
> the check will always evaluate as true and can be removed.

Since commit ("x86, io_apic: Introduce set_affinity function pointer") I
believe and this information would helpfull.

Sebastian

[PATCH 15/19] x86, irq: Move irq_remapped() check into free_remapped_irq

[Joerg Roedel]

The function is called unconditionally now in IO-APIC code
removing another irq_remapped() check from x86 core code.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/x86/kernel/apic/io_apic.c |    4 ++--
 drivers/iommu/irq_remapping.c  |    5 ++++-
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index e95325f..6022e4c 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -2981,8 +2981,8 @@ void destroy_irq(unsigned int irq)
 
 	irq_set_status_flags(irq, IRQ_NOREQUEST|IRQ_NOPROBE);
 
-	if (irq_remapped(cfg))
-		free_remapped_irq(irq);
+	free_remapped_irq(irq);
+
 	raw_spin_lock_irqsave(&vector_lock, flags);
 	__clear_irq_vector(irq, cfg);
 	raw_spin_unlock_irqrestore(&vector_lock, flags);
diff --git a/drivers/iommu/irq_remapping.c b/drivers/iommu/irq_remapping.c
index 3957fb2..f2affc7 100644
--- a/drivers/iommu/irq_remapping.c
+++ b/drivers/iommu/irq_remapping.c
@@ -229,10 +229,13 @@ int set_remapped_irq_affinity(struct irq_data *data, const struct cpumask *mask,
 
 void free_remapped_irq(int irq)
 {
+	struct irq_cfg *cfg = irq_get_chip_data(irq);
+
 	if (!remap_ops || !remap_ops->free_irq)
 		return;
 
-	remap_ops->free_irq(irq);
+	if (cfg && irq_remapped(cfg))
+		remap_ops->free_irq(irq);
 }
 
 void compose_remapped_msi_msg(struct pci_dev *pdev,
-- 
1.7.9.5

Re: [PATCH 15/19] x86, irq: Move irq_remapped() check into free_remapped_irq

[Sebastian Andrzej Siewior]

On Mon, Aug 20, 2012 at 03:56:01PM +0200, Joerg Roedel wrote:
> --- a/arch/x86/kernel/apic/io_apic.c
> +++ b/arch/x86/kernel/apic/io_apic.c
> @@ -2981,8 +2981,8 @@ void destroy_irq(unsigned int irq)
>  
>  	irq_set_status_flags(irq, IRQ_NOREQUEST|IRQ_NOPROBE);
>  
> -	if (irq_remapped(cfg))
> -		free_remapped_irq(irq);
> +	free_remapped_irq(irq);
> +
>  	raw_spin_lock_irqsave(&vector_lock, flags);
>  	__clear_irq_vector(irq, cfg);
>  	raw_spin_unlock_irqrestore(&vector_lock, flags);
> diff --git a/drivers/iommu/irq_remapping.c b/drivers/iommu/irq_remapping.c
> index 3957fb2..f2affc7 100644
> --- a/drivers/iommu/irq_remapping.c
> +++ b/drivers/iommu/irq_remapping.c
> @@ -229,10 +229,13 @@ int set_remapped_irq_affinity(struct irq_data *data, const struct cpumask *mask,
>  
>  void free_remapped_irq(int irq)
>  {
> +	struct irq_cfg *cfg = irq_get_chip_data(irq);
> +
>  	if (!remap_ops || !remap_ops->free_irq)
>  		return;
>  
> -	remap_ops->free_irq(irq);
> +	if (cfg && irq_remapped(cfg))

You shuffle code with the exception that you now check for cfg. Why?

> +		remap_ops->free_irq(irq);
>  }
>  
>  void compose_remapped_msi_msg(struct pci_dev *pdev,

Sebastian

Re: [PATCH 15/19] x86, irq: Move irq_remapped() check into free_remapped_irq

[Joerg Roedel]

On Sun, Aug 26, 2012 at 08:27:55PM +0200, Sebastian Andrzej Siewior wrote:
> On Mon, Aug 20, 2012 at 03:56:01PM +0200, Joerg Roedel wrote:
> > --- a/arch/x86/kernel/apic/io_apic.c
> > +++ b/arch/x86/kernel/apic/io_apic.c
> > @@ -2981,8 +2981,8 @@ void destroy_irq(unsigned int irq)
> >  
> >  	irq_set_status_flags(irq, IRQ_NOREQUEST|IRQ_NOPROBE);
> >  
> > -	if (irq_remapped(cfg))
> > -		free_remapped_irq(irq);
> > +	free_remapped_irq(irq);
> > +
> >  	raw_spin_lock_irqsave(&vector_lock, flags);
> >  	__clear_irq_vector(irq, cfg);
> >  	raw_spin_unlock_irqrestore(&vector_lock, flags);
> > diff --git a/drivers/iommu/irq_remapping.c b/drivers/iommu/irq_remapping.c
> > index 3957fb2..f2affc7 100644
> > --- a/drivers/iommu/irq_remapping.c
> > +++ b/drivers/iommu/irq_remapping.c
> > @@ -229,10 +229,13 @@ int set_remapped_irq_affinity(struct irq_data *data, const struct cpumask *mask,
> >  
> >  void free_remapped_irq(int irq)
> >  {
> > +	struct irq_cfg *cfg = irq_get_chip_data(irq);
> > +
> >  	if (!remap_ops || !remap_ops->free_irq)
> >  		return;
> >  
> > -	remap_ops->free_irq(irq);
> > +	if (cfg && irq_remapped(cfg))
> 
> You shuffle code with the exception that you now check for cfg. Why?

Just to be sure it is not NULL, but looking again at the previous code
it is not necessary. I remove the check for cfg.


	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

[PATCH 16/19] x86, irq: Introduce setup_remapped_irq()

[Joerg Roedel]

This function does irq-remapping specific interrupt setup
like modifying the chip defaults.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/x86/include/asm/irq_remapping.h |    9 +++++++++
 arch/x86/kernel/apic/io_apic.c       |   13 +++----------
 drivers/iommu/irq_remapping.c        |   11 +++++++++++
 3 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/arch/x86/include/asm/irq_remapping.h b/arch/x86/include/asm/irq_remapping.h
index 1888148..b670580 100644
--- a/arch/x86/include/asm/irq_remapping.h
+++ b/arch/x86/include/asm/irq_remapping.h
@@ -44,6 +44,9 @@ extern void compose_remapped_msi_msg(struct pci_dev *pdev,
 				     struct msi_msg *msg, u8 hpet_id);
 extern int setup_hpet_msi_remapped(unsigned int irq, unsigned int id);
 extern void panic_if_irq_remap(const char *msg);
+extern bool setup_remapped_irq(int irq,
+			       struct irq_cfg *cfg,
+			       struct irq_chip *chip);
 
 static inline bool irq_remapped(struct irq_cfg *cfg)
 {
@@ -93,6 +96,12 @@ static inline void irq_remap_modify_chip_defaults(struct irq_chip *chip)
 {
 }
 
+static inline bool setup_remapped_irq(int irq,
+				      struct irq_cfg *cfg,
+				      struct irq_chip *chip)
+{
+	return false;
+}
 #endif /* CONFIG_IRQ_REMAP */
 
 #endif /* __X86_IRQ_REMAPPING_H */
diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index 6022e4c..c2f2d2d 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -1292,11 +1292,8 @@ static void ioapic_register_intr(unsigned int irq, struct irq_cfg *cfg,
 		fasteoi = false;
 	}
 
-	if (irq_remapped(cfg)) {
-		irq_set_status_flags(irq, IRQ_MOVE_PCNTXT);
-		irq_remap_modify_chip_defaults(chip);
+	if (setup_remapped_irq(irq, cfg, chip))
 		fasteoi = trigger != 0;
-	}
 
 	hdl = fasteoi ? handle_fasteoi_irq : handle_edge_irq;
 	irq_set_chip_and_handler_name(irq, chip, hdl,
@@ -3093,10 +3090,7 @@ int setup_msi_irq(struct pci_dev *dev, struct msi_desc *msidesc, int irq)
 	irq_set_msi_desc(irq, msidesc);
 	write_msi_msg(irq, &msg);
 
-	if (irq_remapped(irq_get_chip_data(irq))) {
-		irq_set_status_flags(irq, IRQ_MOVE_PCNTXT);
-		irq_remap_modify_chip_defaults(chip);
-	}
+	setup_remapped_irq(irq, irq_get_chip_data(irq), chip);
 
 	irq_set_chip_and_handler_name(irq, chip, handle_edge_irq, "edge");
 
@@ -3234,8 +3228,7 @@ int default_setup_hpet_msi(unsigned int irq, unsigned int id)
 
 	hpet_msi_write(irq_get_handler_data(irq), &msg);
 	irq_set_status_flags(irq, IRQ_MOVE_PCNTXT);
-	if (irq_remapped(irq_get_chip_data(irq)))
-		irq_remap_modify_chip_defaults(chip);
+	setup_remapped_irq(irq, irq_get_chip_data(irq), chip);
 
 	irq_set_chip_and_handler_name(irq, chip, handle_edge_irq, "edge");
 	return 0;
diff --git a/drivers/iommu/irq_remapping.c b/drivers/iommu/irq_remapping.c
index f2affc7..d9cd920f 100644
--- a/drivers/iommu/irq_remapping.c
+++ b/drivers/iommu/irq_remapping.c
@@ -302,3 +302,14 @@ void irq_remap_modify_chip_defaults(struct irq_chip *chip)
 	chip->irq_eoi = ir_ack_apic_level;
 	chip->irq_set_affinity = x86_io_apic_ops.set_affinity;
 }
+
+bool setup_remapped_irq(int irq, struct irq_cfg *cfg, struct irq_chip *chip)
+{
+	if (irq_remapped(cfg)) {
+		irq_set_status_flags(irq, IRQ_MOVE_PCNTXT);
+		irq_remap_modify_chip_defaults(chip);
+		return true;
+	} else {
+		return false;
+	}
+}
-- 
1.7.9.5

Re: [PATCH 16/19] x86, irq: Introduce setup_remapped_irq()

[Sebastian Andrzej Siewior]

On Mon, Aug 20, 2012 at 03:56:02PM +0200, Joerg Roedel wrote:
> --- a/arch/x86/include/asm/irq_remapping.h
> +++ b/arch/x86/include/asm/irq_remapping.h
> @@ -44,6 +44,9 @@ extern void compose_remapped_msi_msg(struct pci_dev *pdev,
>  				     struct msi_msg *msg, u8 hpet_id);
>  extern int setup_hpet_msi_remapped(unsigned int irq, unsigned int id);
>  extern void panic_if_irq_remap(const char *msg);
> +extern bool setup_remapped_irq(int irq,
> +			       struct irq_cfg *cfg,
> +			       struct irq_chip *chip);
here and in the next definition, is it possible to fit it in one line or
alteast in two?

>  
>  static inline bool irq_remapped(struct irq_cfg *cfg)
>  {
> @@ -93,6 +96,12 @@ static inline void irq_remap_modify_chip_defaults(struct irq_chip *chip)

> --- a/drivers/iommu/irq_remapping.c
> +++ b/drivers/iommu/irq_remapping.c
> @@ -302,3 +302,14 @@ void irq_remap_modify_chip_defaults(struct irq_chip *chip)
>  	chip->irq_eoi = ir_ack_apic_level;
>  	chip->irq_set_affinity = x86_io_apic_ops.set_affinity;
>  }
> +
> +bool setup_remapped_irq(int irq, struct irq_cfg *cfg, struct irq_chip *chip)
> +{
> +	if (irq_remapped(cfg)) {
> +		irq_set_status_flags(irq, IRQ_MOVE_PCNTXT);
> +		irq_remap_modify_chip_defaults(chip);
> +		return true;
> +	} else {
> +		return false;
> +	}
> +}

What about

bool setup_remapped_irq(int irq, struct irq_cfg *cfg, struct irq_chip *chip)
{
	if (!irq_remapped(cfg))
		return false;
	irq_set_status_flags(irq, IRQ_MOVE_PCNTXT);
	irq_remap_modify_chip_defaults(chip);
	return true;
}

Sebastian

Re: [PATCH 16/19] x86, irq: Introduce setup_remapped_irq()

[Joerg Roedel]

On Sun, Aug 26, 2012 at 08:30:30PM +0200, Sebastian Andrzej Siewior wrote:
> On Mon, Aug 20, 2012 at 03:56:02PM +0200, Joerg Roedel wrote:
> > --- a/arch/x86/include/asm/irq_remapping.h
> > +++ b/arch/x86/include/asm/irq_remapping.h
> > @@ -44,6 +44,9 @@ extern void compose_remapped_msi_msg(struct pci_dev *pdev,
> >  				     struct msi_msg *msg, u8 hpet_id);
> >  extern int setup_hpet_msi_remapped(unsigned int irq, unsigned int id);
> >  extern void panic_if_irq_remap(const char *msg);
> > +extern bool setup_remapped_irq(int irq,
> > +			       struct irq_cfg *cfg,
> > +			       struct irq_chip *chip);
> here and in the next definition, is it possible to fit it in one line or
> alteast in two?

It doesn't fit in one line (without violating the 80 column
recommendation). So I think this is the best readable alternative
format.

> What about
> 
> bool setup_remapped_irq(int irq, struct irq_cfg *cfg, struct irq_chip *chip)
> {
> 	if (!irq_remapped(cfg))
> 		return false;
> 	irq_set_status_flags(irq, IRQ_MOVE_PCNTXT);
> 	irq_remap_modify_chip_defaults(chip);
> 	return true;
> }

You are right, that is better. I changed it.

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

[PATCH 17/19] x86, msi: Introduce x86_msi.compose_msi_msg call-back

[Joerg Roedel]

This call-back points to the right function for initializing
the msi_msg structure.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/x86/include/asm/io_apic.h  |    4 +++
 arch/x86/include/asm/x86_init.h |    4 +++
 arch/x86/kernel/apic/io_apic.c  |   59 ++++++++++++++++++++-------------------
 arch/x86/kernel/x86_init.c      |    1 +
 drivers/iommu/irq_remapping.c   |    9 ++++--
 5 files changed, 46 insertions(+), 31 deletions(-)

diff --git a/arch/x86/include/asm/io_apic.h b/arch/x86/include/asm/io_apic.h
index 0db4429..59a3f4e 100644
--- a/arch/x86/include/asm/io_apic.h
+++ b/arch/x86/include/asm/io_apic.h
@@ -160,6 +160,9 @@ extern void __init native_setup_timer_pin(unsigned int ioapic_idx,
 					  unsigned int pin, int vector);
 extern void eoi_ioapic_irq(unsigned int irq, struct irq_cfg *cfg);
 
+extern void native_compose_msi_msg(struct pci_dev *pdev,
+				   unsigned int irq, unsigned int dest,
+				   struct msi_msg *msg, u8 hpet_id);
 int io_apic_setup_irq_pin_once(unsigned int irq, int node, struct io_apic_irq_attr *attr);
 
 extern int save_ioapic_entries(void);
@@ -245,6 +248,7 @@ static inline void disable_ioapic_support(void) { }
 #define native_ioapic_set_affinity	NULL
 #define native_setup_ioapic_entry	NULL
 #define native_setup_timer_pin		NULL
+#define native_compose_msi_msg		NULL
 #endif
 
 #endif /* _ASM_X86_IO_APIC_H */
diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
index ffe5860..1430af0 100644
--- a/arch/x86/include/asm/x86_init.h
+++ b/arch/x86/include/asm/x86_init.h
@@ -180,9 +180,13 @@ struct x86_platform_ops {
 };
 
 struct pci_dev;
+struct msi_msg;
 
 struct x86_msi_ops {
 	int (*setup_msi_irqs)(struct pci_dev *dev, int nvec, int type);
+	void (*compose_msi_msg)(struct pci_dev *dev, unsigned int irq,
+				unsigned int dest, struct msi_msg *msg,
+			       u8 hpet_id);
 	void (*teardown_msi_irq)(unsigned int irq);
 	void (*teardown_msi_irqs)(struct pci_dev *dev);
 	void (*restore_msi_irqs)(struct pci_dev *dev, int irq);
diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index c2f2d2d..aac3f62 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -2989,37 +2989,16 @@ void destroy_irq(unsigned int irq)
 /*
  * MSI message composition
  */
-#ifdef CONFIG_PCI_MSI
-static int msi_compose_msg(struct pci_dev *pdev, unsigned int irq,
-			   struct msi_msg *msg, u8 hpet_id)
+void native_compose_msi_msg(struct pci_dev *pdev,
+			    unsigned int irq, unsigned int dest,
+			    struct msi_msg *msg, u8 hpet_id)
 {
-	struct irq_cfg *cfg;
-	int err;
-	unsigned dest;
-
-	if (disable_apic)
-		return -ENXIO;
-
-	cfg = irq_cfg(irq);
-	err = assign_irq_vector(irq, cfg, apic->target_cpus());
-	if (err)
-		return err;
-
-	err = apic->cpu_mask_to_apicid_and(cfg->domain,
-					   apic->target_cpus(), &dest);
-	if (err)
-		return err;
+	struct irq_cfg *cfg = irq_cfg(irq);
 
-	if (irq_remapped(cfg)) {
-		compose_remapped_msi_msg(pdev, irq, dest, msg, hpet_id);
-		return err;
-	}
+	msg->address_hi = MSI_ADDR_BASE_HI;
 
 	if (x2apic_enabled())
-		msg->address_hi = MSI_ADDR_BASE_HI |
-				  MSI_ADDR_EXT_DEST_ID(dest);
-	else
-		msg->address_hi = MSI_ADDR_BASE_HI;
+		msg->address_hi |= MSI_ADDR_EXT_DEST_ID(dest);
 
 	msg->address_lo =
 		MSI_ADDR_BASE_LO |
@@ -3038,8 +3017,32 @@ static int msi_compose_msg(struct pci_dev *pdev, unsigned int irq,
 			MSI_DATA_DELIVERY_FIXED:
 			MSI_DATA_DELIVERY_LOWPRI) |
 		MSI_DATA_VECTOR(cfg->vector);
+}
 
-	return err;
+#ifdef CONFIG_PCI_MSI
+static int msi_compose_msg(struct pci_dev *pdev, unsigned int irq,
+			   struct msi_msg *msg, u8 hpet_id)
+{
+	struct irq_cfg *cfg;
+	int err;
+	unsigned dest;
+
+	if (disable_apic)
+		return -ENXIO;
+
+	cfg = irq_cfg(irq);
+	err = assign_irq_vector(irq, cfg, apic->target_cpus());
+	if (err)
+		return err;
+
+	err = apic->cpu_mask_to_apicid_and(cfg->domain,
+					   apic->target_cpus(), &dest);
+	if (err)
+		return err;
+
+	x86_msi.compose_msi_msg(pdev, irq, dest, msg, hpet_id);
+
+	return 0;
 }
 
 static int
diff --git a/arch/x86/kernel/x86_init.c b/arch/x86/kernel/x86_init.c
index eba02e5..af5874e 100644
--- a/arch/x86/kernel/x86_init.c
+++ b/arch/x86/kernel/x86_init.c
@@ -115,6 +115,7 @@ struct x86_platform_ops x86_platform = {
 EXPORT_SYMBOL_GPL(x86_platform);
 struct x86_msi_ops x86_msi = {
 	.setup_msi_irqs		= native_setup_msi_irqs,
+	.compose_msi_msg	= native_compose_msi_msg,
 	.teardown_msi_irq	= native_teardown_msi_irq,
 	.teardown_msi_irqs	= default_teardown_msi_irqs,
 	.restore_msi_irqs	= default_restore_msi_irqs,
diff --git a/drivers/iommu/irq_remapping.c b/drivers/iommu/irq_remapping.c
index d9cd920f..e61a174 100644
--- a/drivers/iommu/irq_remapping.c
+++ b/drivers/iommu/irq_remapping.c
@@ -102,6 +102,7 @@ static void __init irq_remapping_modify_x86_ops(void)
 	x86_io_apic_ops.setup_timer_pin	= irq_remapping_setup_timer_pin;
 	x86_msi.setup_msi_irqs		= irq_remapping_setup_msi_irqs;
 	x86_msi.setup_hpet_msi		= setup_hpet_msi_remapped;
+	x86_msi.compose_msi_msg		= compose_remapped_msi_msg;
 }
 
 static __init int setup_nointremap(char *str)
@@ -242,10 +243,12 @@ void compose_remapped_msi_msg(struct pci_dev *pdev,
 			      unsigned int irq, unsigned int dest,
 			      struct msi_msg *msg, u8 hpet_id)
 {
-	if (!remap_ops || !remap_ops->compose_msi_msg)
-		return;
+	struct irq_cfg *cfg = irq_get_chip_data(irq);
 
-	remap_ops->compose_msi_msg(pdev, irq, dest, msg, hpet_id);
+	if (cfg && !irq_remapped(cfg))
+		native_compose_msi_msg(pdev, irq, dest, msg, hpet_id);
+	else if (remap_ops && remap_ops->compose_msi_msg)
+		remap_ops->compose_msi_msg(pdev, irq, dest, msg, hpet_id);
 }
 
 static int msi_alloc_remapped_irq(struct pci_dev *pdev, int irq, int nvec)
-- 
1.7.9.5

Re: [PATCH 17/19] x86, msi: Introduce x86_msi.compose_msi_msg call-back

[Konrad Rzeszutek Wilk]

On Mon, Aug 20, 2012 at 03:56:03PM +0200, Joerg Roedel wrote:
> This call-back points to the right function for initializing
> the msi_msg structure.

What is the 'hpet_id' purpose in this?

> 
> Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
> ---
>  arch/x86/include/asm/io_apic.h  |    4 +++
>  arch/x86/include/asm/x86_init.h |    4 +++
>  arch/x86/kernel/apic/io_apic.c  |   59 ++++++++++++++++++++-------------------
>  arch/x86/kernel/x86_init.c      |    1 +
>  drivers/iommu/irq_remapping.c   |    9 ++++--
>  5 files changed, 46 insertions(+), 31 deletions(-)
> 
> diff --git a/arch/x86/include/asm/io_apic.h b/arch/x86/include/asm/io_apic.h
> index 0db4429..59a3f4e 100644
> --- a/arch/x86/include/asm/io_apic.h
> +++ b/arch/x86/include/asm/io_apic.h
> @@ -160,6 +160,9 @@ extern void __init native_setup_timer_pin(unsigned int ioapic_idx,
>  					  unsigned int pin, int vector);
>  extern void eoi_ioapic_irq(unsigned int irq, struct irq_cfg *cfg);
>  
> +extern void native_compose_msi_msg(struct pci_dev *pdev,
> +				   unsigned int irq, unsigned int dest,
> +				   struct msi_msg *msg, u8 hpet_id);
>  int io_apic_setup_irq_pin_once(unsigned int irq, int node, struct io_apic_irq_attr *attr);
>  
>  extern int save_ioapic_entries(void);
> @@ -245,6 +248,7 @@ static inline void disable_ioapic_support(void) { }
>  #define native_ioapic_set_affinity	NULL
>  #define native_setup_ioapic_entry	NULL
>  #define native_setup_timer_pin		NULL
> +#define native_compose_msi_msg		NULL
>  #endif
>  
>  #endif /* _ASM_X86_IO_APIC_H */
> diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
> index ffe5860..1430af0 100644
> --- a/arch/x86/include/asm/x86_init.h
> +++ b/arch/x86/include/asm/x86_init.h
> @@ -180,9 +180,13 @@ struct x86_platform_ops {
>  };
>  
>  struct pci_dev;
> +struct msi_msg;
>  
>  struct x86_msi_ops {
>  	int (*setup_msi_irqs)(struct pci_dev *dev, int nvec, int type);
> +	void (*compose_msi_msg)(struct pci_dev *dev, unsigned int irq,
> +				unsigned int dest, struct msi_msg *msg,
> +			       u8 hpet_id);
>  	void (*teardown_msi_irq)(unsigned int irq);
>  	void (*teardown_msi_irqs)(struct pci_dev *dev);
>  	void (*restore_msi_irqs)(struct pci_dev *dev, int irq);
> diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
> index c2f2d2d..aac3f62 100644
> --- a/arch/x86/kernel/apic/io_apic.c
> +++ b/arch/x86/kernel/apic/io_apic.c
> @@ -2989,37 +2989,16 @@ void destroy_irq(unsigned int irq)
>  /*
>   * MSI message composition
>   */
> -#ifdef CONFIG_PCI_MSI
> -static int msi_compose_msg(struct pci_dev *pdev, unsigned int irq,
> -			   struct msi_msg *msg, u8 hpet_id)
> +void native_compose_msi_msg(struct pci_dev *pdev,
> +			    unsigned int irq, unsigned int dest,
> +			    struct msi_msg *msg, u8 hpet_id)
>  {
> -	struct irq_cfg *cfg;
> -	int err;
> -	unsigned dest;
> -
> -	if (disable_apic)
> -		return -ENXIO;
> -
> -	cfg = irq_cfg(irq);
> -	err = assign_irq_vector(irq, cfg, apic->target_cpus());
> -	if (err)
> -		return err;
> -
> -	err = apic->cpu_mask_to_apicid_and(cfg->domain,
> -					   apic->target_cpus(), &dest);
> -	if (err)
> -		return err;
> +	struct irq_cfg *cfg = irq_cfg(irq);
>  
> -	if (irq_remapped(cfg)) {
> -		compose_remapped_msi_msg(pdev, irq, dest, msg, hpet_id);
> -		return err;
> -	}
> +	msg->address_hi = MSI_ADDR_BASE_HI;
>  
>  	if (x2apic_enabled())
> -		msg->address_hi = MSI_ADDR_BASE_HI |
> -				  MSI_ADDR_EXT_DEST_ID(dest);
> -	else
> -		msg->address_hi = MSI_ADDR_BASE_HI;
> +		msg->address_hi |= MSI_ADDR_EXT_DEST_ID(dest);
>  
>  	msg->address_lo =
>  		MSI_ADDR_BASE_LO |
> @@ -3038,8 +3017,32 @@ static int msi_compose_msg(struct pci_dev *pdev, unsigned int irq,
>  			MSI_DATA_DELIVERY_FIXED:
>  			MSI_DATA_DELIVERY_LOWPRI) |
>  		MSI_DATA_VECTOR(cfg->vector);
> +}
>  
> -	return err;
> +#ifdef CONFIG_PCI_MSI
> +static int msi_compose_msg(struct pci_dev *pdev, unsigned int irq,
> +			   struct msi_msg *msg, u8 hpet_id)
> +{
> +	struct irq_cfg *cfg;
> +	int err;
> +	unsigned dest;
> +
> +	if (disable_apic)
> +		return -ENXIO;
> +
> +	cfg = irq_cfg(irq);
> +	err = assign_irq_vector(irq, cfg, apic->target_cpus());
> +	if (err)
> +		return err;
> +
> +	err = apic->cpu_mask_to_apicid_and(cfg->domain,
> +					   apic->target_cpus(), &dest);
> +	if (err)
> +		return err;
> +
> +	x86_msi.compose_msi_msg(pdev, irq, dest, msg, hpet_id);
> +
> +	return 0;
>  }
>  
>  static int
> diff --git a/arch/x86/kernel/x86_init.c b/arch/x86/kernel/x86_init.c
> index eba02e5..af5874e 100644
> --- a/arch/x86/kernel/x86_init.c
> +++ b/arch/x86/kernel/x86_init.c
> @@ -115,6 +115,7 @@ struct x86_platform_ops x86_platform = {
>  EXPORT_SYMBOL_GPL(x86_platform);
>  struct x86_msi_ops x86_msi = {
>  	.setup_msi_irqs		= native_setup_msi_irqs,
> +	.compose_msi_msg	= native_compose_msi_msg,
>  	.teardown_msi_irq	= native_teardown_msi_irq,
>  	.teardown_msi_irqs	= default_teardown_msi_irqs,
>  	.restore_msi_irqs	= default_restore_msi_irqs,
> diff --git a/drivers/iommu/irq_remapping.c b/drivers/iommu/irq_remapping.c
> index d9cd920f..e61a174 100644
> --- a/drivers/iommu/irq_remapping.c
> +++ b/drivers/iommu/irq_remapping.c
> @@ -102,6 +102,7 @@ static void __init irq_remapping_modify_x86_ops(void)
>  	x86_io_apic_ops.setup_timer_pin	= irq_remapping_setup_timer_pin;
>  	x86_msi.setup_msi_irqs		= irq_remapping_setup_msi_irqs;
>  	x86_msi.setup_hpet_msi		= setup_hpet_msi_remapped;
> +	x86_msi.compose_msi_msg		= compose_remapped_msi_msg;
>  }
>  
>  static __init int setup_nointremap(char *str)
> @@ -242,10 +243,12 @@ void compose_remapped_msi_msg(struct pci_dev *pdev,
>  			      unsigned int irq, unsigned int dest,
>  			      struct msi_msg *msg, u8 hpet_id)
>  {
> -	if (!remap_ops || !remap_ops->compose_msi_msg)
> -		return;
> +	struct irq_cfg *cfg = irq_get_chip_data(irq);
>  
> -	remap_ops->compose_msi_msg(pdev, irq, dest, msg, hpet_id);
> +	if (cfg && !irq_remapped(cfg))
> +		native_compose_msi_msg(pdev, irq, dest, msg, hpet_id);
> +	else if (remap_ops && remap_ops->compose_msi_msg)
> +		remap_ops->compose_msi_msg(pdev, irq, dest, msg, hpet_id);
>  }
>  
>  static int msi_alloc_remapped_irq(struct pci_dev *pdev, int irq, int nvec)
> -- 
> 1.7.9.5
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [PATCH 17/19] x86, msi: Introduce x86_msi.compose_msi_msg call-back

[Joerg Roedel]

On Mon, Aug 20, 2012 at 10:08:44AM -0400, Konrad Rzeszutek Wilk wrote:
> On Mon, Aug 20, 2012 at 03:56:03PM +0200, Joerg Roedel wrote:
> > This call-back points to the right function for initializing
> > the msi_msg structure.
> 
> What is the 'hpet_id' purpose in this?

The VT-d implementation uses it. This function is used to create the MSI
message for PCI devices and the HPET. When it is called for the HPET the
pdev parameter is NULL and hpet_id is valid.


	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

Re: [PATCH 17/19] x86, msi: Introduce x86_msi.compose_msi_msg call-back

[Konrad Rzeszutek Wilk]

On Tue, Aug 21, 2012 at 10:40:07AM +0200, Joerg Roedel wrote:
> On Mon, Aug 20, 2012 at 10:08:44AM -0400, Konrad Rzeszutek Wilk wrote:
> > On Mon, Aug 20, 2012 at 03:56:03PM +0200, Joerg Roedel wrote:
> > > This call-back points to the right function for initializing
> > > the msi_msg structure.
> > 
> > What is the 'hpet_id' purpose in this?
> 
> The VT-d implementation uses it. This function is used to create the MSI
> message for PCI devices and the HPET. When it is called for the HPET the
> pdev parameter is NULL and hpet_id is valid.

Perhaps then a more generic term? 'void *platform_priv' ?

Re: [PATCH 17/19] x86, msi: Introduce x86_msi.compose_msi_msg call-back

[Joerg Roedel]

On Tue, Aug 21, 2012 at 10:42:57AM -0400, Konrad Rzeszutek Wilk wrote:
> On Tue, Aug 21, 2012 at 10:40:07AM +0200, Joerg Roedel wrote:
> > The VT-d implementation uses it. This function is used to create the MSI
> > message for PCI devices and the HPET. When it is called for the HPET the
> > pdev parameter is NULL and hpet_id is valid.
> 
> Perhaps then a more generic term? 'void *platform_priv' ?

Doesn't work this way here. The Intel code detects on pdev==NULL that it
needs to use hpet_id. What could work is to split the code-paths between
pci_dev msi composition and the one for hpet.
But that approach has the downside of code duplication to some degree.


Regards,

	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

Re: [PATCH 17/19] x86, msi: Introduce x86_msi.compose_msi_msg call-back

[Joerg Roedel]

On Tue, Aug 21, 2012 at 10:42:57AM -0400, Konrad Rzeszutek Wilk wrote:
> On Tue, Aug 21, 2012 at 10:40:07AM +0200, Joerg Roedel wrote:
> > On Mon, Aug 20, 2012 at 10:08:44AM -0400, Konrad Rzeszutek Wilk wrote:
> > > On Mon, Aug 20, 2012 at 03:56:03PM +0200, Joerg Roedel wrote:
> > > > This call-back points to the right function for initializing
> > > > the msi_msg structure.
> > > 
> > > What is the 'hpet_id' purpose in this?
> > 
> > The VT-d implementation uses it. This function is used to create the MSI
> > message for PCI devices and the HPET. When it is called for the HPET the
> > pdev parameter is NULL and hpet_id is valid.
> 
> Perhaps then a more generic term? 'void *platform_priv' ?

Well, the best approach is probably to remove both, the pdev and the
hpet parameter and replace it with an req_id parameter that is passed
down instead. IOMMU code needs to expose the hpet request-ids then with
another call-back. But that is cleaner than the current compose_msi_msg
call-back.


	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

Re: [PATCH 17/19] x86, msi: Introduce x86_msi.compose_msi_msg call-back

[Joerg Roedel]

On Tue, Aug 21, 2012 at 10:42:57AM -0400, Konrad Rzeszutek Wilk wrote:
> On Tue, Aug 21, 2012 at 10:40:07AM +0200, Joerg Roedel wrote:
> > On Mon, Aug 20, 2012 at 10:08:44AM -0400, Konrad Rzeszutek Wilk wrote:
> > > On Mon, Aug 20, 2012 at 03:56:03PM +0200, Joerg Roedel wrote:
> > > > This call-back points to the right function for initializing
> > > > the msi_msg structure.
> > > 
> > > What is the 'hpet_id' purpose in this?
> > 
> > The VT-d implementation uses it. This function is used to create the MSI
> > message for PCI devices and the HPET. When it is called for the HPET the
> > pdev parameter is NULL and hpet_id is valid.
> 
> Perhaps then a more generic term? 'void *platform_priv' ?

Okay, I looked into the code a bit more and the best solution is to
remove the pdev and hpet_id parameters completly from the
msi_compose_msg path. The only user of these parameters is the Intel
interrupt remapping code which can also set the source-ids in the
irq-alloc path. But this is a larger effort which is best done in a
seperate patch-set.


	Joerg

Re: [PATCH 17/19] x86, msi: Introduce x86_msi.compose_msi_msg call-back

[Sebastian Andrzej Siewior]

On Mon, Aug 20, 2012 at 03:56:03PM +0200, Joerg Roedel wrote:
> This call-back points to the right function for initializing
> the msi_msg structure.

So you pull out the compose_remapped_msi_msg() out of msi_compose_msg() and
put this in a callback. Such information is good since you don't see this in
diff and even --patience does not help here.

> --- a/drivers/iommu/irq_remapping.c
> +++ b/drivers/iommu/irq_remapping.c
> @@ -242,10 +243,12 @@ void compose_remapped_msi_msg(struct pci_dev *pdev,
>  			      unsigned int irq, unsigned int dest,
>  			      struct msi_msg *msg, u8 hpet_id)
>  {
> -	if (!remap_ops || !remap_ops->compose_msi_msg)
> -		return;
> +	struct irq_cfg *cfg = irq_get_chip_data(irq);
>  
> -	remap_ops->compose_msi_msg(pdev, irq, dest, msg, hpet_id);
> +	if (cfg && !irq_remapped(cfg))
> +		native_compose_msi_msg(pdev, irq, dest, msg, hpet_id);
> +	else if (remap_ops && remap_ops->compose_msi_msg)
> +		remap_ops->compose_msi_msg(pdev, irq, dest, msg, hpet_id);

cfg _has_ to be valid here and if it is not than you shouldn't assume that
this irq is remapped.
Also remap_ops has to be set here. And ->compose_msi_msg() should be set
here as well. Would it make sense if it is not set?

>  }
>  
>  static int msi_alloc_remapped_irq(struct pci_dev *pdev, int irq, int nvec)

Sebastian

Re: [PATCH 17/19] x86, msi: Introduce x86_msi.compose_msi_msg call-back

[Joerg Roedel]

On Sun, Aug 26, 2012 at 08:41:40PM +0200, Sebastian Andrzej Siewior wrote:
> On Mon, Aug 20, 2012 at 03:56:03PM +0200, Joerg Roedel wrote:
> > This call-back points to the right function for initializing
> > the msi_msg structure.
> 
> So you pull out the compose_remapped_msi_msg() out of msi_compose_msg() and
> put this in a callback. Such information is good since you don't see this in
> diff and even --patience does not help here.

Added that to the commit message.

> 
> > --- a/drivers/iommu/irq_remapping.c
> > +++ b/drivers/iommu/irq_remapping.c
> > @@ -242,10 +243,12 @@ void compose_remapped_msi_msg(struct pci_dev *pdev,
> >  			      unsigned int irq, unsigned int dest,
> >  			      struct msi_msg *msg, u8 hpet_id)
> >  {
> > -	if (!remap_ops || !remap_ops->compose_msi_msg)
> > -		return;
> > +	struct irq_cfg *cfg = irq_get_chip_data(irq);
> >  
> > -	remap_ops->compose_msi_msg(pdev, irq, dest, msg, hpet_id);
> > +	if (cfg && !irq_remapped(cfg))
> > +		native_compose_msi_msg(pdev, irq, dest, msg, hpet_id);
> > +	else if (remap_ops && remap_ops->compose_msi_msg)
> > +		remap_ops->compose_msi_msg(pdev, irq, dest, msg, hpet_id);
> 
> cfg _has_ to be valid here and if it is not than you shouldn't assume that
> this irq is remapped.
> Also remap_ops has to be set here. And ->compose_msi_msg() should be set
> here as well. Would it make sense if it is not set?

I removed the check for cfg, but I am still conservative on calling
function pointers without checking them first.


	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

[PATCH 18/19] x86, io_apic: Introduce eoi_ioapic_pin call-back

[Joerg Roedel]

This callback replaces the old __eoi_ioapic_pin function
which needs a special path for interrupt remapping.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/x86/include/asm/io_apic.h  |    5 +++++
 arch/x86/include/asm/x86_init.h |    1 +
 arch/x86/kernel/apic/io_apic.c  |   20 ++++++--------------
 arch/x86/kernel/x86_init.c      |    1 +
 drivers/iommu/irq_remapping.c   |   13 +++++++++++++
 5 files changed, 26 insertions(+), 14 deletions(-)

diff --git a/arch/x86/include/asm/io_apic.h b/arch/x86/include/asm/io_apic.h
index 59a3f4e..16f28fe 100644
--- a/arch/x86/include/asm/io_apic.h
+++ b/arch/x86/include/asm/io_apic.h
@@ -163,6 +163,7 @@ extern void eoi_ioapic_irq(unsigned int irq, struct irq_cfg *cfg);
 extern void native_compose_msi_msg(struct pci_dev *pdev,
 				   unsigned int irq, unsigned int dest,
 				   struct msi_msg *msg, u8 hpet_id);
+extern void native_eoi_ioapic_pin(int apic, int pin, int vector);
 int io_apic_setup_irq_pin_once(unsigned int irq, int node, struct io_apic_irq_attr *attr);
 
 extern int save_ioapic_entries(void);
@@ -213,6 +214,9 @@ static inline void io_apic_modify(unsigned int apic, unsigned int reg, unsigned
 {
 	x86_io_apic_ops.modify(apic, reg, value);
 }
+
+extern void io_apic_eoi(unsigned int apic, unsigned int vector);
+
 #else  /* !CONFIG_X86_IO_APIC */
 
 #define io_apic_assign_pci_irqs 0
@@ -249,6 +253,7 @@ static inline void disable_ioapic_support(void) { }
 #define native_setup_ioapic_entry	NULL
 #define native_setup_timer_pin		NULL
 #define native_compose_msi_msg		NULL
+#define native_eoi_ioapic_pin		NULL
 #endif
 
 #endif /* _ASM_X86_IO_APIC_H */
diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
index 1430af0..5d7faf7 100644
--- a/arch/x86/include/asm/x86_init.h
+++ b/arch/x86/include/asm/x86_init.h
@@ -213,6 +213,7 @@ struct x86_io_apic_ops {
 				       struct io_apic_irq_attr *attr);
 	void		(*setup_timer_pin)(unsigned int ioapic_idx,
 					   unsigned int pin, int vector);
+	void		(*eoi_ioapic_pin)(int apic, int pin, int vector);
 
 };
 
diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
index aac3f62..d4db390 100644
--- a/arch/x86/kernel/apic/io_apic.c
+++ b/arch/x86/kernel/apic/io_apic.c
@@ -310,7 +310,7 @@ static __attribute_const__ struct io_apic __iomem *io_apic_base(int idx)
 		+ (mpc_ioapic_addr(idx) & ~PAGE_MASK);
 }
 
-static inline void io_apic_eoi(unsigned int apic, unsigned int vector)
+void io_apic_eoi(unsigned int apic, unsigned int vector)
 {
 	struct io_apic __iomem *io_apic = io_apic_base(apic);
 	writel(vector, &io_apic->eoi);
@@ -557,19 +557,10 @@ static void unmask_ioapic_irq(struct irq_data *data)
  * Otherwise, we simulate the EOI message manually by changing the trigger
  * mode to edge and then back to level, with RTE being masked during this.
  */
-static void __eoi_ioapic_pin(int apic, int pin, int vector, struct irq_cfg *cfg)
+void native_eoi_ioapic_pin(int apic, int pin, int vector)
 {
 	if (mpc_ioapic_ver(apic) >= 0x20) {
-		/*
-		 * Intr-remapping uses pin number as the virtual vector
-		 * in the RTE. Actual vector is programmed in
-		 * intr-remapping table entry. Hence for the io-apic
-		 * EOI we use the pin number.
-		 */
-		if (cfg && irq_remapped(cfg))
-			io_apic_eoi(apic, pin);
-		else
-			io_apic_eoi(apic, vector);
+		io_apic_eoi(apic, vector);
 	} else {
 		struct IO_APIC_route_entry entry, entry1;
 
@@ -597,7 +588,8 @@ void eoi_ioapic_irq(unsigned int irq, struct irq_cfg *cfg)
 
 	raw_spin_lock_irqsave(&ioapic_lock, flags);
 	for_each_irq_pin(entry, cfg->irq_2_pin)
-		__eoi_ioapic_pin(entry->apic, entry->pin, cfg->vector, cfg);
+		x86_io_apic_ops.eoi_ioapic_pin(entry->apic, entry->pin,
+					       cfg->vector);
 	raw_spin_unlock_irqrestore(&ioapic_lock, flags);
 }
 
@@ -634,7 +626,7 @@ static void clear_IO_APIC_pin(unsigned int apic, unsigned int pin)
 		}
 
 		raw_spin_lock_irqsave(&ioapic_lock, flags);
-		__eoi_ioapic_pin(apic, pin, entry.vector, NULL);
+		x86_io_apic_ops.eoi_ioapic_pin(apic, pin, entry.vector);
 		raw_spin_unlock_irqrestore(&ioapic_lock, flags);
 	}
 
diff --git a/arch/x86/kernel/x86_init.c b/arch/x86/kernel/x86_init.c
index af5874e..589fb84 100644
--- a/arch/x86/kernel/x86_init.c
+++ b/arch/x86/kernel/x86_init.c
@@ -132,4 +132,5 @@ struct x86_io_apic_ops x86_io_apic_ops = {
 	.set_affinity		= native_ioapic_set_affinity,
 	.setup_entry		= native_setup_ioapic_entry,
 	.setup_timer_pin	= native_setup_timer_pin,
+	.eoi_ioapic_pin		= native_eoi_ioapic_pin,
 };
diff --git a/drivers/iommu/irq_remapping.c b/drivers/iommu/irq_remapping.c
index e61a174..d62e757 100644
--- a/drivers/iommu/irq_remapping.c
+++ b/drivers/iommu/irq_remapping.c
@@ -94,12 +94,24 @@ static void __init irq_remapping_setup_timer_pin(unsigned int ioapic_idx,
 	/* Not needed with interrupt remapping */
 }
 
+void eoi_ioapic_pin_remapped(int apic, int pin, int vector)
+{
+	/*
+	 * Intr-remapping uses pin number as the virtual vector
+	 * in the RTE. Actual vector is programmed in
+	 * intr-remapping table entry. Hence for the io-apic
+	 * EOI we use the pin number.
+	 */
+	io_apic_eoi(apic, pin);
+}
+
 static void __init irq_remapping_modify_x86_ops(void)
 {
 	x86_io_apic_ops.disable		= irq_remapping_disable_io_apic;
 	x86_io_apic_ops.set_affinity	= set_remapped_irq_affinity;
 	x86_io_apic_ops.setup_entry	= setup_ioapic_remapped_entry;
 	x86_io_apic_ops.setup_timer_pin	= irq_remapping_setup_timer_pin;
+	x86_io_apic_ops.eoi_ioapic_pin	= eoi_ioapic_pin_remapped;
 	x86_msi.setup_msi_irqs		= irq_remapping_setup_msi_irqs;
 	x86_msi.setup_hpet_msi		= setup_hpet_msi_remapped;
 	x86_msi.compose_msi_msg		= compose_remapped_msi_msg;
@@ -316,3 +328,4 @@ bool setup_remapped_irq(int irq, struct irq_cfg *cfg, struct irq_chip *chip)
 		return false;
 	}
 }
+
-- 
1.7.9.5

Re: [PATCH 18/19] x86, io_apic: Introduce eoi_ioapic_pin call-back

[Sebastian Andrzej Siewior]

On Mon, Aug 20, 2012 at 03:56:04PM +0200, Joerg Roedel wrote:
> diff --git a/arch/x86/kernel/apic/io_apic.c b/arch/x86/kernel/apic/io_apic.c
> index aac3f62..d4db390 100644
> --- a/arch/x86/kernel/apic/io_apic.c
> +++ b/arch/x86/kernel/apic/io_apic.c
> @@ -557,19 +557,10 @@ static void unmask_ioapic_irq(struct irq_data *data)
>   * Otherwise, we simulate the EOI message manually by changing the trigger
>   * mode to edge and then back to level, with RTE being masked during this.
>   */
> -static void __eoi_ioapic_pin(int apic, int pin, int vector, struct irq_cfg *cfg)
> +void native_eoi_ioapic_pin(int apic, int pin, int vector)
>  {
>  	if (mpc_ioapic_ver(apic) >= 0x20) {
> -		/*
> -		 * Intr-remapping uses pin number as the virtual vector
> -		 * in the RTE. Actual vector is programmed in
> -		 * intr-remapping table entry. Hence for the io-apic
> -		 * EOI we use the pin number.
> -		 */
> -		if (cfg && irq_remapped(cfg))
> -			io_apic_eoi(apic, pin);
> -		else
> -			io_apic_eoi(apic, vector);
> +		io_apic_eoi(apic, vector);
>  	} else {
>  		struct IO_APIC_route_entry entry, entry1;
>  
> diff --git a/drivers/iommu/irq_remapping.c b/drivers/iommu/irq_remapping.c
> index e61a174..d62e757 100644
> --- a/drivers/iommu/irq_remapping.c
> +++ b/drivers/iommu/irq_remapping.c
> @@ -94,12 +94,24 @@ static void __init irq_remapping_setup_timer_pin(unsigned int ioapic_idx,
>  	/* Not needed with interrupt remapping */
>  }
>  
> +void eoi_ioapic_pin_remapped(int apic, int pin, int vector)
> +{
> +	/*
> +	 * Intr-remapping uses pin number as the virtual vector
> +	 * in the RTE. Actual vector is programmed in
> +	 * intr-remapping table entry. Hence for the io-apic
> +	 * EOI we use the pin number.
> +	 */
> +	io_apic_eoi(apic, pin);
> +}
> +

Basically you shuffle the code from up there, down there and call from behind
a function pointer. There are two things different this time:
- no version check >= 0x20
  I belive this is obsolete since this runs only on x86-64 with x(2)apic
  support and assumes more or less version 0x20+ or we wouldn't be here at
  all.
- the irq_remapped(cfg) check is gone. The cfg thing is per-interrupt basis
  created so you _now_ you act like the interrupt is remapped even if it is
  not. Or am I wrong here?

> @@ -316,3 +328,4 @@ bool setup_remapped_irq(int irq, struct irq_cfg *cfg, struct irq_chip *chip)
>  		return false;
>  	}
>  }
> +
New line and the end? Why would that be?

Sebastian

Re: [PATCH 18/19] x86, io_apic: Introduce eoi_ioapic_pin call-back

[Joerg Roedel]

On Sun, Aug 26, 2012 at 08:52:33PM +0200, Sebastian Andrzej Siewior wrote:
> Basically you shuffle the code from up there, down there and call from behind
> a function pointer. There are two things different this time:
> - no version check >= 0x20
>   I belive this is obsolete since this runs only on x86-64 with x(2)apic
>   support and assumes more or less version 0x20+ or we wouldn't be here at
>   all.

Right, all systems that have interrupt remapping have an io-apic version
of 0x20 or higher.

> - the irq_remapped(cfg) check is gone. The cfg thing is per-interrupt basis
>   created so you _now_ you act like the interrupt is remapped even if it is
>   not. Or am I wrong here?

That comes from the fact that with the current implementation (with and
without my patches) IO-APIC interrupts are always remapped. So it is
safe to remove that check.

> > +
> New line and the end? Why would that be?

Coding leftover, I'll remove that.


	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

[PATCH 19/19] x86, irq: Move irq_remapped out of x86 core code

[Joerg Roedel]

The irq_remapped function is only used in IOMMU code after
the last patch. So move its definition there too.

Signed-off-by: Joerg Roedel <joerg.roedel@amd.com>
---
 arch/x86/include/asm/irq_remapping.h |   10 ----------
 drivers/iommu/irq_remapping.c        |    5 +++++
 2 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/arch/x86/include/asm/irq_remapping.h b/arch/x86/include/asm/irq_remapping.h
index b670580..95fd352 100644
--- a/arch/x86/include/asm/irq_remapping.h
+++ b/arch/x86/include/asm/irq_remapping.h
@@ -48,11 +48,6 @@ extern bool setup_remapped_irq(int irq,
 			       struct irq_cfg *cfg,
 			       struct irq_chip *chip);
 
-static inline bool irq_remapped(struct irq_cfg *cfg)
-{
-	return cfg->remapped;
-}
-
 void irq_remap_modify_chip_defaults(struct irq_chip *chip);
 
 #else  /* CONFIG_IRQ_REMAP */
@@ -87,11 +82,6 @@ static inline void panic_if_irq_remap(const char *msg)
 {
 }
 
-static inline bool irq_remapped(struct irq_cfg *cfg)
-{
-	return false;
-}
-
 static inline void irq_remap_modify_chip_defaults(struct irq_chip *chip)
 {
 }
diff --git a/drivers/iommu/irq_remapping.c b/drivers/iommu/irq_remapping.c
index d62e757..bf8be56 100644
--- a/drivers/iommu/irq_remapping.c
+++ b/drivers/iommu/irq_remapping.c
@@ -31,6 +31,11 @@ static int set_remapped_irq_affinity(struct irq_data *data,
 				     const struct cpumask *mask,
 				     bool force);
 
+static bool irq_remapped(struct irq_cfg *cfg)
+{
+	return cfg->remapped;
+}
+
 static void irq_remapping_disable_io_apic(void)
 {
 	/*
-- 
1.7.9.5

Re: [PATCH 00/19 v2] Improve IRQ remapping abstraction in x86 core code

[Sebastian Andrzej Siewior]

On Mon, Aug 20, 2012 at 03:55:46PM +0200, Joerg Roedel wrote:
> Please review.

Finally. Usually you don't add/change code but you just move common
irq remapping pieces out of geric io-apic code and put them in once place. I
think it would be good, if you would note this in the description of your
patch.
Altogether it makes a good impression.

After browsing through the new functions in irq_remapping_modify_x86_ops() I
see that some of them test for "remap_ops" which is pointless because you don't
call irq_remapping_modify_x86_ops() if it is not the case. This also goes mostly
for irq_remapping_enabled.
The only reason when you can disable (or say irq_remapping_disable() is
called) is in the suspend path. And the remap is enabled again in via
irq_remapping_reenable() in resume. Now if this goes wrong what is next? You
don't even return an error if the callback is missing. The variable
irq_remapping_enabled does not save your ass here because some function
behave now different.

But back to the realisitic world: If something goes wrong in resume and you
can't re-enable irq remapping, the system is not really useable or is it (even
before your series)?

> Thanks,
> 
> 	Joerg
> 

Sebastian

Re: [PATCH 00/19 v2] Improve IRQ remapping abstraction in x86 core code

[Joerg Roedel]

Hi Sebastian,

finally I am back from vacation and resume work on this :)

On Sun, Aug 26, 2012 at 09:17:49PM +0200, Sebastian Andrzej Siewior wrote:
> On Mon, Aug 20, 2012 at 03:55:46PM +0200, Joerg Roedel wrote:
> > Please review.
> 
> Finally. Usually you don't add/change code but you just move common
> irq remapping pieces out of geric io-apic code and put them in once place. I
> think it would be good, if you would note this in the description of your
> patch.
> Altogether it makes a good impression.

Thanks, and thanks for your review.

> After browsing through the new functions in irq_remapping_modify_x86_ops() I
> see that some of them test for "remap_ops" which is pointless because you don't
> call irq_remapping_modify_x86_ops() if it is not the case. This also goes mostly
> for irq_remapping_enabled.

Okay, I am usually a bit conservative when it comes to checking function
pointers and conditions. One reason is that the Linux kernel had severe
security bugs in the past with NULL functions pointers being called.
Especially when future changes modify the code path these function
pointers it is easy to forget re-adding the checks. But I will look
again at the patches to check which checks can be removed and which
should better stay.

> The only reason when you can disable (or say irq_remapping_disable() is
> called) is in the suspend path. And the remap is enabled again in via
> irq_remapping_reenable() in resume. Now if this goes wrong what is next? You
> don't even return an error if the callback is missing. The variable
> irq_remapping_enabled does not save your ass here because some function
> behave now different.
> 
> But back to the realisitic world: If something goes wrong in resume and you
> can't re-enable irq remapping, the system is not really useable or is it (even
> before your series)?

Well, when something goes wrong on resume and irq-remapping doesn't work
anymore the system is usually doomed (with and without this patch-set).
When irq-remapping does not work the dma-remapping will likely also not
work anymore which is even worse.

Regards,

	Joerg

-- 
AMD Operating System Research Center

Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632

Re: [PATCH 00/19 v2] Improve IRQ remapping abstraction in x86 core code

[Sebastian Andrzej Siewior]

On Wed, Sep 19, 2012 at 04:52:27PM +0200, Joerg Roedel wrote:
> Hi Sebastian,
Hi Joerg,

> > After browsing through the new functions in irq_remapping_modify_x86_ops() I
> > see that some of them test for "remap_ops" which is pointless because you don't
> > call irq_remapping_modify_x86_ops() if it is not the case. This also goes mostly
> > for irq_remapping_enabled.
> 
> Okay, I am usually a bit conservative when it comes to checking function
> pointers and conditions. One reason is that the Linux kernel had severe
> security bugs in the past with NULL functions pointers being called.
> Especially when future changes modify the code path these function
> pointers it is easy to forget re-adding the checks. But I will look
> again at the patches to check which checks can be removed and which
> should better stay.

But if you forget to set the pointer and you simply return doing nothing it
is kind of bad. So if you do a setup, make check at the beginning and then you
*can* expect that pointers are set. And you can never prepare against people
touching code and not thinking much.
If you look at the IRQ code or x86'ops code for instance, there are a few
places where function pointers are checked / set at the beginning and then are
called. And sometimes the functions are simple "void / return 0" code if this
is really not implemented / required but you don't have to check for it.

> Well, when something goes wrong on resume and irq-remapping doesn't work
> anymore the system is usually doomed (with and without this patch-set).
> When irq-remapping does not work the dma-remapping will likely also not
> work anymore which is even worse.

understood.

> 
> Regards,
> 
> 	Joerg
> 

Sebastian