[PATCH] x86-64: make enabling of MMCFG on AMD Fam10 CPUs actually work

[PATCH] x86-64: make enabling of MMCFG on AMD Fam10 CPUs actually work

[Jan Beulich]

Forcibly enabling the MMCFG space on AMD Fam10 CPUs cannot be expected
to work, since with the firmware not being aware of the address range
used, it cannot possibly reserve the space in E820 or ACPI resources.
Hence we need to manually insert the range into the E820 table, and
enable the range only when the insertion actually works without
conflict.

Adding the calls to the respective E820 handling functions additionally
requires to deal with modpost's mismatch checking: The function doing
those calls needs to be __init, and it is being made sure that it gets
called (from its __cpuinit caller) only on the BSP, and through a stub
(silencing the warning).

Signed-off-by: Jan Beulich <jbeulich@novell.com>
Cc: Yinghai Lu <yinghai@kernel.org>

---
 arch/x86/kernel/mmconf-fam10h_64.c |   22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

--- 3.0-rc7/arch/x86/kernel/mmconf-fam10h_64.c
+++ 3.0-rc7-x86_64-mmcfg-fam10/arch/x86/kernel/mmconf-fam10h_64.c
@@ -14,6 +14,7 @@
 #include <asm/io.h>
 #include <asm/msr.h>
 #include <asm/acpi.h>
+#include <asm/e820.h>
 #include <asm/mmconfig.h>
 #include <asm/pci_x86.h>
 
@@ -49,7 +50,7 @@ static int __cpuinit cmp_range(const voi
 /* need to avoid (0xfd<<32), (0xfe<<32), and (0xff<<32), ht used space */
 #define FAM10H_PCI_MMCONF_BASE (0xfcULL<<32)
 #define BASE_VALID(b) ((b) + MMCONF_SIZE <= (0xfdULL<<32) || (b) >= (1ULL<<40))
-static void __cpuinit get_fam10h_pci_mmconf_base(void)
+static void __init _get_fam10h_pci_mmconf_base(void)
 {
 	int i;
 	unsigned bus;
@@ -163,7 +164,21 @@ static void __cpuinit get_fam10h_pci_mmc
 	return;
 
 out:
-	fam10h_pci_mmconf_base = base;
+	if(!e820_any_mapped(base, base + MMCONF_SIZE, 0)) {
+		e820_add_region(base, MMCONF_SIZE, E820_RESERVED);
+		update_e820();
+		fam10h_pci_mmconf_base = base;
+	}
+}
+
+/*
+ * Section-mismatch warning avoidance: This gets called from a ___cpuinit
+ * function (below), but only on the BSP, and needs to call the __init
+ * functions e820_add_region() and update_e820() (above).
+ */
+static void __ref get_fam10h_pci_mmconf_base(void)
+{
+	_get_fam10h_pci_mmconf_base();
 }
 
 void __cpuinit fam10h_check_enable_mmcfg(void)
@@ -199,7 +214,8 @@ void __cpuinit fam10h_check_enable_mmcfg
 	 * if it is not enabled, try to enable it and assume only one segment
 	 * with 256 buses
 	 */
-	get_fam10h_pci_mmconf_base();
+	if (!fam10h_pci_mmconf_base)
+		get_fam10h_pci_mmconf_base();
 	if (!fam10h_pci_mmconf_base) {
 		pci_probe &= ~PCI_CHECK_ENABLE_AMD_MMCONF;
 		return;

Re: [PATCH] x86-64: make enabling of MMCFG on AMD Fam10 CPUs actually work

[Yinghai Lu]

[-- Attachment #1: Type: text/plain, Size: 3083 bytes --]

On Tue, Jul 19, 2011 at 4:43 AM, Jan Beulich <JBeulich@novell.com> wrote:
> Forcibly enabling the MMCFG space on AMD Fam10 CPUs cannot be expected
> to work, since with the firmware not being aware of the address range
> used, it cannot possibly reserve the space in E820 or ACPI resources.
> Hence we need to manually insert the range into the E820 table, and
> enable the range only when the insertion actually works without
> conflict.
>
> Adding the calls to the respective E820 handling functions additionally
> requires to deal with modpost's mismatch checking: The function doing
> those calls needs to be __init, and it is being made sure that it gets
> called (from its __cpuinit caller) only on the BSP, and through a stub
> (silencing the warning).
>
> Signed-off-by: Jan Beulich <jbeulich@novell.com>
> Cc: Yinghai Lu <yinghai@kernel.org>
>
> ---
>  arch/x86/kernel/mmconf-fam10h_64.c |   22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
>
> --- 3.0-rc7/arch/x86/kernel/mmconf-fam10h_64.c
> +++ 3.0-rc7-x86_64-mmcfg-fam10/arch/x86/kernel/mmconf-fam10h_64.c
> @@ -14,6 +14,7 @@
>  #include <asm/io.h>
>  #include <asm/msr.h>
>  #include <asm/acpi.h>
> +#include <asm/e820.h>
>  #include <asm/mmconfig.h>
>  #include <asm/pci_x86.h>
>
> @@ -49,7 +50,7 @@ static int __cpuinit cmp_range(const voi
>  /* need to avoid (0xfd<<32), (0xfe<<32), and (0xff<<32), ht used space */
>  #define FAM10H_PCI_MMCONF_BASE (0xfcULL<<32)
>  #define BASE_VALID(b) ((b) + MMCONF_SIZE <= (0xfdULL<<32) || (b) >= (1ULL<<40))
> -static void __cpuinit get_fam10h_pci_mmconf_base(void)
> +static void __init _get_fam10h_pci_mmconf_base(void)
>  {
>        int i;
>        unsigned bus;
> @@ -163,7 +164,21 @@ static void __cpuinit get_fam10h_pci_mmc
>        return;
>
>  out:
> -       fam10h_pci_mmconf_base = base;
> +       if(!e820_any_mapped(base, base + MMCONF_SIZE, 0)) {
> +               e820_add_region(base, MMCONF_SIZE, E820_RESERVED);
> +               update_e820();
> +               fam10h_pci_mmconf_base = base;
> +       }
> +}
> +
> +/*
> + * Section-mismatch warning avoidance: This gets called from a ___cpuinit
> + * function (below), but only on the BSP, and needs to call the __init
> + * functions e820_add_region() and update_e820() (above).
> + */
> +static void __ref get_fam10h_pci_mmconf_base(void)
> +{
> +       _get_fam10h_pci_mmconf_base();
>  }
>
>  void __cpuinit fam10h_check_enable_mmcfg(void)
> @@ -199,7 +214,8 @@ void __cpuinit fam10h_check_enable_mmcfg
>         * if it is not enabled, try to enable it and assume only one segment
>         * with 256 buses
>         */
> -       get_fam10h_pci_mmconf_base();
> +       if (!fam10h_pci_mmconf_base)
> +               get_fam10h_pci_mmconf_base();
>        if (!fam10h_pci_mmconf_base) {
>                pci_probe &= ~PCI_CHECK_ENABLE_AMD_MMCONF;
>                return;
>
>
>

sent out some time ago...please check attached patch.

Maybe you can combine them together.

Thanks

Yinghai

[-- Attachment #2: fix_amd_mmconf.patch --]
[-- Type: text/x-patch, Size: 2108 bytes --]

Subject: [PATCH -v2] x86/pci: Add mmconf range into e820 for when it is from MSR with amd faml0h

for AMD Fam10h, it we read mmconf from MSR early, we should just trust it
because we check it and correct it already.

so add it to e820

Also correct the base calulating to make it work below 4g case.

-v2: remove unrelated sort_range changes.
     also make FAM10H_MMIO_CONF_BASE_MASK to be ULL pointed by Jan.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>
Cc: Jan Beulich <jbeulich@novell.com>

---
 arch/x86/kernel/mmconf-fam10h_64.c |   18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

Index: linux-2.6/arch/x86/kernel/mmconf-fam10h_64.c
===================================================================
--- linux-2.6.orig/arch/x86/kernel/mmconf-fam10h_64.c
+++ linux-2.6/arch/x86/kernel/mmconf-fam10h_64.c
@@ -16,6 +16,7 @@
 #include <asm/acpi.h>
 #include <asm/mmconfig.h>
 #include <asm/pci_x86.h>
+#include <asm/e820.h>
 
 struct pci_hostbridge_probe {
 	u32 bus;
@@ -26,6 +27,21 @@ struct pci_hostbridge_probe {
 
 static u64 __cpuinitdata fam10h_pci_mmconf_base;
 
+/* only on BSP */
+static void __init_refok e820_add_mmconf_range(int busnbits)
+{
+	u64 end;
+
+	end = fam10h_pci_mmconf_base + (1ULL<<(busnbits + 20)) - 1;
+	if (!e820_all_mapped(fam10h_pci_mmconf_base, end+1, E820_RESERVED)) {
+		printk(KERN_DEBUG "Fam 10h mmconf [%llx, %llx]\n",
+				 fam10h_pci_mmconf_base, end);
+		e820_add_region(fam10h_pci_mmconf_base, 1ULL<<(busnbits + 20),
+				 E820_RESERVED);
+		sanitize_e820_map();
+	}
+}
+
 static struct pci_hostbridge_probe pci_probes[] __cpuinitdata = {
 	{ 0, 0x18, PCI_VENDOR_ID_AMD, 0x1200 },
 	{ 0xff, 0, PCI_VENDOR_ID_AMD, 0x1200 },
@@ -164,6 +180,7 @@ static void __cpuinit get_fam10h_pci_mmc
 
 out:
 	fam10h_pci_mmconf_base = base;
+	e820_add_mmconf_range(8);
 }
 
 void __cpuinit fam10h_check_enable_mmcfg(void)
@@ -189,6 +206,7 @@ void __cpuinit fam10h_check_enable_mmcfg
 
 			if (!fam10h_pci_mmconf_base) {
 				fam10h_pci_mmconf_base = base;
+				e820_add_mmconf_range(busnbits);
 				return;
 			} else if (fam10h_pci_mmconf_base ==  base)
 				return;

Re: [PATCH] x86-64: make enabling of MMCFG on AMD Fam10 CPUs actually work

[Jan Beulich]

>>> On 19.07.11 at 18:38, Yinghai Lu <yinghai@kernel.org> wrote:
> On Tue, Jul 19, 2011 at 4:43 AM, Jan Beulich <JBeulich@novell.com> wrote:
>> Forcibly enabling the MMCFG space on AMD Fam10 CPUs cannot be expected
>> to work, since with the firmware not being aware of the address range
>> used, it cannot possibly reserve the space in E820 or ACPI resources.
>> Hence we need to manually insert the range into the E820 table, and
>> enable the range only when the insertion actually works without
>> conflict.
>>
>> Adding the calls to the respective E820 handling functions additionally
>> requires to deal with modpost's mismatch checking: The function doing
>> those calls needs to be __init, and it is being made sure that it gets
>> called (from its __cpuinit caller) only on the BSP, and through a stub
>> (silencing the warning).
>>
>> Signed-off-by: Jan Beulich <jbeulich@novell.com>
>> Cc: Yinghai Lu <yinghai@kernel.org>
>>
>> ---
>>  arch/x86/kernel/mmconf-fam10h_64.c |   22 +++++++++++++++++++---
>>  1 file changed, 19 insertions(+), 3 deletions(-)
>>
>> --- 3.0-rc7/arch/x86/kernel/mmconf-fam10h_64.c
>> +++ 3.0-rc7-x86_64-mmcfg-fam10/arch/x86/kernel/mmconf-fam10h_64.c
>> @@ -14,6 +14,7 @@
>>  #include <asm/io.h>
>>  #include <asm/msr.h>
>>  #include <asm/acpi.h>
>> +#include <asm/e820.h>
>>  #include <asm/mmconfig.h>
>>  #include <asm/pci_x86.h>
>>
>> @@ -49,7 +50,7 @@ static int __cpuinit cmp_range(const voi
>>  /* need to avoid (0xfd<<32), (0xfe<<32), and (0xff<<32), ht used space */
>>  #define FAM10H_PCI_MMCONF_BASE (0xfcULL<<32)
>>  #define BASE_VALID(b) ((b) + MMCONF_SIZE <= (0xfdULL<<32) || (b) >= (1ULL<<40))
>> -static void __cpuinit get_fam10h_pci_mmconf_base(void)
>> +static void __init _get_fam10h_pci_mmconf_base(void)
>>  {
>>        int i;
>>        unsigned bus;
>> @@ -163,7 +164,21 @@ static void __cpuinit get_fam10h_pci_mmc
>>        return;
>>
>>  out:
>> -       fam10h_pci_mmconf_base = base;
>> +       if(!e820_any_mapped(base, base + MMCONF_SIZE, 0)) {
>> +               e820_add_region(base, MMCONF_SIZE, E820_RESERVED);
>> +               update_e820();
>> +               fam10h_pci_mmconf_base = base;
>> +       }
>> +}
>> +
>> +/*
>> + * Section-mismatch warning avoidance: This gets called from a ___cpuinit
>> + * function (below), but only on the BSP, and needs to call the __init
>> + * functions e820_add_region() and update_e820() (above).
>> + */
>> +static void __ref get_fam10h_pci_mmconf_base(void)
>> +{
>> +       _get_fam10h_pci_mmconf_base();
>>  }
>>
>>  void __cpuinit fam10h_check_enable_mmcfg(void)
>> @@ -199,7 +214,8 @@ void __cpuinit fam10h_check_enable_mmcfg
>>         * if it is not enabled, try to enable it and assume only one segment
>>         * with 256 buses
>>         */
>> -       get_fam10h_pci_mmconf_base();
>> +       if (!fam10h_pci_mmconf_base)
>> +               get_fam10h_pci_mmconf_base();
>>        if (!fam10h_pci_mmconf_base) {
>>                pci_probe &= ~PCI_CHECK_ENABLE_AMD_MMCONF;
>>                return;
>>
>>
>>
> 
> sent out some time ago...please check attached patch.
> 
> Maybe you can combine them together.

No, I don't think what you do is valid. In particular, adding/using the
range if !e820_all_mapped(..., E820_RESERVED) rather than (as in
my variant) !e820_any_mapped(..., 0) doesn't seem appropriate.

As to whether adding the range also for the case where we read
the value from the MSR - I'm not sure, and I don't have a system to
test that case on.

So bottom line is that I think your change for that second case could
go on top of my change - if that gets accepted/applied in the first
place.

Jan

Re: [PATCH] x86-64: make enabling of MMCFG on AMD Fam10 CPUs actually work

[Ingo Molnar]

* Jan Beulich <JBeulich@novell.com> wrote:

> Forcibly enabling the MMCFG space on AMD Fam10 CPUs cannot be expected
> to work, since with the firmware not being aware of the address range
> used, it cannot possibly reserve the space in E820 or ACPI resources.
> Hence we need to manually insert the range into the E820 table, and
> enable the range only when the insertion actually works without
> conflict.
> 
> Adding the calls to the respective E820 handling functions additionally
> requires to deal with modpost's mismatch checking: The function doing
> those calls needs to be __init, and it is being made sure that it gets
> called (from its __cpuinit caller) only on the BSP, and through a stub
> (silencing the warning).
> 
> Signed-off-by: Jan Beulich <jbeulich@novell.com>
> Cc: Yinghai Lu <yinghai@kernel.org>
> 
> ---
>  arch/x86/kernel/mmconf-fam10h_64.c |   22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)

I agree with the patch (and please also add the second fix you 
extracted from Yinghai's patch, as a second patch).

This changelog is missing scope analysis: what systems are affected 
in practice (if any), were actual problems observed, if no problems 
were observed then what are the potential problems that people should 
look out for (and which this commit may fix), etc.

If none of that information is available then such a small blurb:

 'This bug was found via code review and I'm not aware of any systems
  that are affected currently - but some may exist.'

Will inform maintainers and other interested parties about the 
practical background of the patch.

Thanks,

	Ingo