[patch 1/2] x2apic: fix reserved APIC register accesses in print_local_APIC()

[patch 1/2] x2apic: fix reserved APIC register accesses in print_local_APIC()

[Suresh Siddha]

[-- Attachment #1: fix_reserved_apic_reg_access.patch --]
[-- Type: text/plain, Size: 2513 bytes --]

From: Yinghai Lu <yhlu.kernel@gmail.com>
Subject: x2apic: fix reserved APIC register accesses in print_local_APIC()

APIC_ARBPRI is a reserved register for XAPIC and beyond.
APIC_RRR is a reserved register for integrated APIC's.
APIC_EOI is a write only register.
APIC_DFR is reserved in x2apic mode.

Access to these registers in x2apic will result in #GP fault. Fix these
apic register accesses.

Signed-off-by: Yinghai Lu <yhlu.kernel@gmail.com>
Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
---

Index: tip/arch/x86/kernel/io_apic.c
===================================================================
--- tip.orig/arch/x86/kernel/io_apic.c	2008-09-02 16:04:04.000000000 -0700
+++ tip/arch/x86/kernel/io_apic.c	2008-09-02 16:17:13.000000000 -0700
@@ -1751,21 +1751,24 @@
 	printk(KERN_DEBUG "... APIC TASKPRI: %08x (%02x)\n", v, v & APIC_TPRI_MASK);
 
 	if (APIC_INTEGRATED(ver)) {                     /* !82489DX */
-		v = apic_read(APIC_ARBPRI);
-		printk(KERN_DEBUG "... APIC ARBPRI: %08x (%02x)\n", v,
-			v & APIC_ARBPRI_MASK);
+		if (!APIC_XAPIC(ver)) {
+			v = apic_read(APIC_ARBPRI);
+			printk(KERN_DEBUG "... APIC ARBPRI: %08x (%02x)\n", v,
+			       v & APIC_ARBPRI_MASK);
+		}
 		v = apic_read(APIC_PROCPRI);
 		printk(KERN_DEBUG "... APIC PROCPRI: %08x\n", v);
+	} else {
+		v = apic_read(APIC_RRR);
+		printk(KERN_DEBUG "... APIC RRR: %08x\n", v);
 	}
 
-	v = apic_read(APIC_EOI);
-	printk(KERN_DEBUG "... APIC EOI: %08x\n", v);
-	v = apic_read(APIC_RRR);
-	printk(KERN_DEBUG "... APIC RRR: %08x\n", v);
 	v = apic_read(APIC_LDR);
 	printk(KERN_DEBUG "... APIC LDR: %08x\n", v);
-	v = apic_read(APIC_DFR);
-	printk(KERN_DEBUG "... APIC DFR: %08x\n", v);
+	if (!x2apic_enabled()) {
+		v = apic_read(APIC_DFR);
+		printk(KERN_DEBUG "... APIC DFR: %08x\n", v);
+	}
 	v = apic_read(APIC_SPIV);
 	printk(KERN_DEBUG "... APIC SPIV: %08x\n", v);
 
Index: tip/include/asm-x86/apic.h
===================================================================
--- tip.orig/include/asm-x86/apic.h	2008-09-02 16:04:04.000000000 -0700
+++ tip/include/asm-x86/apic.h	2008-09-02 16:15:42.000000000 -0700
@@ -98,6 +98,20 @@
 extern void enable_x2apic(void);
 extern void enable_IR_x2apic(void);
 extern void x2apic_icr_write(u32 low, u32 id);
+static inline int x2apic_enabled(void)
+{
+	int msr, msr2;
+
+	if (!cpu_has_x2apic)
+		return 0;
+
+	rdmsr(MSR_IA32_APICBASE, msr, msr2);
+	if (msr & X2APIC_ENABLE)
+		return 1;
+	return 0;
+}
+#else
+#define x2apic_enabled()	0
 #endif
 
 struct apic_ops {

-- 

[patch 2/2] x2apic: fix using early fixmap mapping for DMAR table parsing

[Suresh Siddha]

[-- Attachment #1: dmar_fix_fixmap_mapping.patch --]
[-- Type: text/plain, Size: 3019 bytes --]

From: Yinghai Lu <yhlu.kernel@gmail.com>
Subject: x2apic, dmar: fix using early fixmap mapping for DMAR table parsing

Very early detection of the DMAR tables will setup fixmap mapping. For
parsing these tables later (while enabling dma and/or interrupt remapping),
early fixmap mapping shouldn't be used. Fix it by calling table detection
routines again, which will call generic apci_get_table() for setting up
the correct mapping.

Signed-off-by: Yinghai Lu <yhlu.kernel@gmail.com>
Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
---

Index: tip/drivers/pci/dmar.c
===================================================================
--- tip.orig/drivers/pci/dmar.c	2008-09-02 16:04:04.000000000 -0700
+++ tip/drivers/pci/dmar.c	2008-09-02 16:41:31.000000000 -0700
@@ -289,6 +289,24 @@
 	}
 }
 
+/**
+ * dmar_table_detect - checks to see if the platform supports DMAR devices
+ */
+static int __init dmar_table_detect(void)
+{
+	acpi_status status = AE_OK;
+
+	/* if we could find DMAR table, then there are DMAR devices */
+	status = acpi_get_table(ACPI_SIG_DMAR, 0,
+				(struct acpi_table_header **)&dmar_tbl);
+
+	if (ACPI_SUCCESS(status) && !dmar_tbl) {
+		printk (KERN_WARNING PREFIX "Unable to map DMAR\n");
+		status = AE_NOT_FOUND;
+	}
+
+	return (ACPI_SUCCESS(status) ? 1 : 0);
+}
 
 /**
  * parse_dmar_table - parses the DMA reporting table
@@ -300,6 +318,12 @@
 	struct acpi_dmar_header *entry_header;
 	int ret = 0;
 
+	/*
+	 * Do it again, earlier dmar_tbl mapping could be mapped with
+	 * fixed map.
+	 */
+	dmar_table_detect();
+
 	dmar = (struct acpi_table_dmar *)dmar_tbl;
 	if (!dmar)
 		return -ENODEV;
@@ -430,30 +454,11 @@
 	return 0;
 }
 
-/**
- * early_dmar_detect - checks to see if the platform supports DMAR devices
- */
-int __init early_dmar_detect(void)
-{
-	acpi_status status = AE_OK;
-
-	/* if we could find DMAR table, then there are DMAR devices */
-	status = acpi_get_table(ACPI_SIG_DMAR, 0,
-				(struct acpi_table_header **)&dmar_tbl);
-
-	if (ACPI_SUCCESS(status) && !dmar_tbl) {
-		printk (KERN_WARNING PREFIX "Unable to map DMAR\n");
-		status = AE_NOT_FOUND;
-	}
-
-	return (ACPI_SUCCESS(status) ? 1 : 0);
-}
-
 void __init detect_intel_iommu(void)
 {
 	int ret;
 
-	ret = early_dmar_detect();
+	ret = dmar_table_detect();
 
 #ifdef CONFIG_DMAR
 	{
@@ -479,14 +484,16 @@
 			       " x2apic support\n");
 
 			dmar_disabled = 1;
-			return;
+			goto end;
 		}
 
 		if (ret && !no_iommu && !iommu_detected && !swiotlb &&
 		    !dmar_disabled)
 			iommu_detected = 1;
 	}
+end:
 #endif
+	dmar_tbl = NULL;
 }
 
 
Index: tip/include/linux/dmar.h
===================================================================
--- tip.orig/include/linux/dmar.h	2008-09-02 16:04:04.000000000 -0700
+++ tip/include/linux/dmar.h	2008-09-02 16:40:40.000000000 -0700
@@ -45,7 +45,6 @@
 	list_for_each_entry(drhd, &dmar_drhd_units, list)
 
 extern int dmar_table_init(void);
-extern int early_dmar_detect(void);
 extern int dmar_dev_scope_init(void);
 
 /* Intel IOMMU detection */

-- 

Re: [patch 1/2] x2apic: fix reserved APIC register accesses in print_local_APIC()

[Maciej W. Rozycki]

On Tue, 2 Sep 2008, Suresh Siddha wrote:

> APIC_RRR is a reserved register for integrated APIC's.

 NAK this change -- the Remote Read register is officially supported up to 
the Pentium integrated APIC and to the best of my knowledge it works with 
the P6 APIC as well.  Any APIC using the serial inter-APIC bus should 
support it.

  Maciej

Re: [patch 1/2] x2apic: fix reserved APIC register accesses in print_local_APIC()

[Suresh Siddha]

On Tue, Sep 02, 2008 at 10:28:54PM -0700, Maciej W. Rozycki wrote:
> On Tue, 2 Sep 2008, Suresh Siddha wrote:
> 
> > APIC_RRR is a reserved register for integrated APIC's.
> 
>  NAK this change -- the Remote Read register is officially supported up to
> the Pentium integrated APIC and to the best of my knowledge it works with
> the P6 APIC as well.  Any APIC using the serial inter-APIC bus should
> support it.

from SDV Vol 3a Section 17.27.1

  The remote read delivery mode provided in the 82489DX and local APIC for
  Pentium processors is not supported in the local APIC in the Pentium 4, Intel
  Xeon, and P6 family processors.

It looks like it is not supported (officially atleast) even in APIC bus
architectures. Only the Pentium integrated APIC seems to be the exception.

I have to probably use maxlvt check, to selectively enable this debug output
for Pentium and !APIC_INTEGRATED check for 82489DX.

thanks,
suresh

Re: [patch 1/2] x2apic: fix reserved APIC register accesses in print_local_APIC()

[Maciej W. Rozycki]

On Wed, 3 Sep 2008, Suresh Siddha wrote:

> from SDV Vol 3a Section 17.27.1
> 
>   The remote read delivery mode provided in the 82489DX and local APIC for
>   Pentium processors is not supported in the local APIC in the Pentium 4, Intel
>   Xeon, and P6 family processors.
> 
> It looks like it is not supported (officially atleast) even in APIC bus
> architectures. Only the Pentium integrated APIC seems to be the exception.

 Yeah, trust documentation and see where you'll get... ;)

> I have to probably use maxlvt check, to selectively enable this debug output
> for Pentium and !APIC_INTEGRATED check for 82489DX.

 Sounds about right.  Why couldn't have the designers simply bump up the
bloody APIC version when they introduced or removed functionality other
than LVT entries, sigh?...  It's not that they are going to run out of
numbers anytime soon.

  Maciej

Re: [patch 1/2] x2apic: fix reserved APIC register accesses in print_local_APIC()

[Ingo Molnar]

* Maciej W. Rozycki <macro@linux-mips.org> wrote:

> On Wed, 3 Sep 2008, Suresh Siddha wrote:
> 
> > from SDV Vol 3a Section 17.27.1
> > 
> >   The remote read delivery mode provided in the 82489DX and local APIC for
> >   Pentium processors is not supported in the local APIC in the Pentium 4, Intel
> >   Xeon, and P6 family processors.
> > 
> > It looks like it is not supported (officially atleast) even in APIC bus
> > architectures. Only the Pentium integrated APIC seems to be the exception.
> 
>  Yeah, trust documentation and see where you'll get... ;)

hm, is the remote read debug output, now that it's not present in CPUs 
made in the past 5 years or so, really that important?

	Ingo

Re: [patch 1/2] x2apic: fix reserved APIC register accesses in print_local_APIC()

[Maciej W. Rozycki]

On Fri, 5 Sep 2008, Ingo Molnar wrote:

> hm, is the remote read debug output, now that it's not present in CPUs 
> made in the past 5 years or so, really that important?

 Maybe, maybe not, but if included it should be done correctly or
otherwise anotated why done differently.  Otherwise people may get
confused and propagate incorrect information as not everybody is inclined
to chase and cross-refer the relevant piece of documentation for each and
every bit of code fiddling with hardware.  In fact the requirement to do
so reduces trust in code it applies to and makes long-term maintenance a
nightmare.

 This observation applies universally of course.  I tend to look at code
without referring to documentation too as most often than not this is much
more effective time-wise.

  Maciej