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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 20 Jul 2011 10:07:31 +0100
From:	"Jan Beulich" <JBeulich@...ell.com>
To:	"Yinghai Lu" <yinghai@...nel.org>
Cc:	<mingo@...e.hu>, <tglx@...utronix.de>,
	<linux-kernel@...r.kernel.org>, <hpa@...or.com>
Subject: Re: [PATCH] x86-64: make enabling of MMCFG on AMD Fam10 CPUs
	 actually work

>>> On 19.07.11 at 18:38, Yinghai Lu <yinghai@...nel.org> wrote:
> On Tue, Jul 19, 2011 at 4:43 AM, Jan Beulich <JBeulich@...ell.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@...ell.com>
>> Cc: Yinghai Lu <yinghai@...nel.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

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