[PATCH] x86/dmi: Switch dmi_remap to ioremap_cache

[PATCH] x86/dmi: Switch dmi_remap to ioremap_cache

[Andy Lutomirski]

DMI cacheability is very confused on x86.

dmi_early_remap uses early_ioremap, which uses FIXMAP_PAGE_IO, which
is __PAGE_KERNEL_IO, which is __PAGE_KERNEL, which is cached.  Don't
ask me why this makes any sense.

dmi_remap uses ioremap, which requests an uncached mapping.
However, on non-EFI systems, the DMI data generally lives between
0xf0000 and 0x100000, which is in the legacy ISA range, which
triggers a special case in the PAT code that overrides the cache
mode requested by ioremap and forces a WB mapping.

On a UEFI boot, however, the DMI table can live at any physical
address.  On my laptop, it's around 0x77dd0000.  That's nowhere near
the legacy ISA range, so the ioremap implicit uncached type is
honored and we end up with a UC- mapping.

UC- is a very, very slow way to read from main memory, so dmi_walk
is likely to take much longer than necessary.

Given that, even on UEFI, we do early cached DMI reads, it seems
safe to just ask for cached access.  Switch to ioremap_cache.

I haven't tried to benchmark this, but I'd guess it saves several
milliseconds of boot time.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/include/asm/dmi.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/dmi.h b/arch/x86/include/asm/dmi.h
index 535192f6bfad..3c69fed215c5 100644
--- a/arch/x86/include/asm/dmi.h
+++ b/arch/x86/include/asm/dmi.h
@@ -15,7 +15,7 @@ static __always_inline __init void *dmi_alloc(unsigned len)
 /* Use early IO mappings for DMI because it's initialized early */
 #define dmi_early_remap		early_ioremap
 #define dmi_early_unmap		early_iounmap
-#define dmi_remap		ioremap
+#define dmi_remap		ioremap_cache
 #define dmi_unmap		iounmap
 
 #endif /* _ASM_X86_DMI_H */
-- 
2.5.0

[tip:x86/mm] x86/dmi: Switch dmi_remap() from ioremap() [uncached ] to ioremap_cache()

[tip-bot for Andy Lutomirski]

Commit-ID:  ce1143aa60273220a9f89012f2aaaed04f97e9a2
Gitweb:     http://git.kernel.org/tip/ce1143aa60273220a9f89012f2aaaed04f97e9a2
Author:     Andy Lutomirski <luto@kernel.org>
AuthorDate: Mon, 25 Jan 2016 23:06:49 -0800
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Tue, 9 Feb 2016 14:36:43 +0100

x86/dmi: Switch dmi_remap() from ioremap() [uncached] to ioremap_cache()

DMI cacheability is very confused on x86.

dmi_early_remap() uses early_ioremap(), which uses FIXMAP_PAGE_IO,
which is __PAGE_KERNEL_IO, which is __PAGE_KERNEL, which is cached.

Don't ask me why this makes any sense.

dmi_remap() uses ioremap(), which requests an uncached mapping.

However, on non-EFI systems, the DMI data generally lives between
0xf0000 and 0x100000, which is in the legacy ISA range, which
triggers a special case in the PAT code that overrides the cache
mode requested by ioremap() and forces a WB mapping.

On a UEFI boot, however, the DMI table can live at any physical
address.  On my laptop, it's around 0x77dd0000.  That's nowhere near
the legacy ISA range, so the ioremap() implicit uncached type is
honored and we end up with a UC- mapping.

UC- is a very, very slow way to read from main memory, so dmi_walk()
is likely to take much longer than necessary.

Given that, even on UEFI, we do early cached DMI reads, it seems
safe to just ask for cached access.  Switch to ioremap_cache().

I haven't tried to benchmark this, but I'd guess it saves several
milliseconds of boot time.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Arjan van de Ven <arjan@linux.intel.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Jean Delvare <jdelvare@suse.de>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Luis R. Rodriguez <mcgrof@suse.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Toshi Kani <toshi.kani@hp.com>
Link: http://lkml.kernel.org/r/3147c38e51f439f3c8911db34c7d4ab22d854915.1453791969.git.luto@kernel.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/include/asm/dmi.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/dmi.h b/arch/x86/include/asm/dmi.h
index 535192f..3c69fed 100644
--- a/arch/x86/include/asm/dmi.h
+++ b/arch/x86/include/asm/dmi.h
@@ -15,7 +15,7 @@ static __always_inline __init void *dmi_alloc(unsigned len)
 /* Use early IO mappings for DMI because it's initialized early */
 #define dmi_early_remap		early_ioremap
 #define dmi_early_unmap		early_iounmap
-#define dmi_remap		ioremap
+#define dmi_remap		ioremap_cache
 #define dmi_unmap		iounmap
 
 #endif /* _ASM_X86_DMI_H */

Re: [PATCH] x86/dmi: Switch dmi_remap to ioremap_cache

[Julian Brost]

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

On 26.01.2016 08:06, Andy Lutomirski wrote:
> DMI cacheability is very confused on x86.
> 
> dmi_early_remap uses early_ioremap, which uses FIXMAP_PAGE_IO, which
> is __PAGE_KERNEL_IO, which is __PAGE_KERNEL, which is cached.  Don't
> ask me why this makes any sense.
> 
> dmi_remap uses ioremap, which requests an uncached mapping.
> However, on non-EFI systems, the DMI data generally lives between
> 0xf0000 and 0x100000, which is in the legacy ISA range, which
> triggers a special case in the PAT code that overrides the cache
> mode requested by ioremap and forces a WB mapping.
> 
> On a UEFI boot, however, the DMI table can live at any physical
> address.  On my laptop, it's around 0x77dd0000.  That's nowhere near
> the legacy ISA range, so the ioremap implicit uncached type is
> honored and we end up with a UC- mapping.
> 
> UC- is a very, very slow way to read from main memory, so dmi_walk
> is likely to take much longer than necessary.
> 
> Given that, even on UEFI, we do early cached DMI reads, it seems
> safe to just ask for cached access.  Switch to ioremap_cache.
> 
> I haven't tried to benchmark this, but I'd guess it saves several
> milliseconds of boot time.
> 
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>  arch/x86/include/asm/dmi.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/dmi.h b/arch/x86/include/asm/dmi.h
> index 535192f6bfad..3c69fed215c5 100644
> --- a/arch/x86/include/asm/dmi.h
> +++ b/arch/x86/include/asm/dmi.h
> @@ -15,7 +15,7 @@ static __always_inline __init void *dmi_alloc(unsigned len)
>  /* Use early IO mappings for DMI because it's initialized early */
>  #define dmi_early_remap		early_ioremap
>  #define dmi_early_unmap		early_iounmap
> -#define dmi_remap		ioremap
> +#define dmi_remap		ioremap_cache
>  #define dmi_unmap		iounmap
>  
>  #endif /* _ASM_X86_DMI_H */
> 

Hi,

after updating from kernel 4.4 to 4.9, the hp-health tools stopped
working on our HP servers (ProLiant DL380p Gen8 and DL380 Gen9, no
others tested). Bisecting has shown commit ce1143a to be the cause.
Reverting it on top of 4.9.13 solved the issue for us.

In more detail, the hp-health tools fail to start because hpasmlited
segfaults with the following log messages:

hpasmlited[3218]: (SendRecv_CHIF) CpqCiSend: error=4, "Bad name for the
target connection object.".
hpasmlited[3218]: (SendRecv_CHIF) CpqCiSend: error=4, "Bad name for the
target connection object.".
hpasmlited[3218]: ehpsmb_parse_SMBIOS: SMBIOSInitTable was not successful.
hpasrd[3272]: WARNING: Can not open /dev/cpqhealth/casr.
hpasrd[3272]: ERROR: Failed to get ASR enabled state.
kernel: x86/PAT: hpasmlited:3218 map pfn expected mapping type
uncached-minus for [mem 0xbfbdb000-0xbfbdcfff], got write-back
kernel: hpasmlited[3218]: segfault at 0 ip 000000000040605b sp
00007ffea5f3beb0 error 4 in hpasmlited[400000+3a000]

According to strace, this is one of the last actions of hpasmlited
before it segfaults:

mmap(NULL, 6176, PROT_READ, MAP_SHARED, 13</dev/mem>, 0xbfbdb000) = -1
EAGAIN (Resource temporarily unavailable)

I'm note entirely sure whether it's actually the kernel or HP to blame,
but for now, hp-health is completely broken on 4.9 (probably on
everything starting from 4.6), so this patch should be reviewed again.

Regards,
Julian


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 6110 bytes --]

Re: [PATCH] x86/dmi: Switch dmi_remap to ioremap_cache

[Arjan van de Ven]

On 3/9/2017 9:48 AM, Julian Brost wrote:

> I'm note entirely sure whether it's actually the kernel or HP to blame,
> but for now, hp-health is completely broken on 4.9 (probably on
> everything starting from 4.6), so this patch should be reviewed again.

it looks like another kernel driver is doing a conflicting mapping.
do these HP tools come with their own kernel drivers or are those in the
upstream kernel nowadays?

Re: [PATCH] x86/dmi: Switch dmi_remap to ioremap_cache

[Julian Brost]

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

On 09.03.2017 18:51, Arjan van de Ven wrote:
> On 3/9/2017 9:48 AM, Julian Brost wrote:
> 
>> I'm note entirely sure whether it's actually the kernel or HP to blame,
>> but for now, hp-health is completely broken on 4.9 (probably on
>> everything starting from 4.6), so this patch should be reviewed again.
> 
> it looks like another kernel driver is doing a conflicting mapping.
> do these HP tools come with their own kernel drivers or are those in the
> upstream kernel nowadays?
> 

We don't have any third-party modules loaded. The hp-health package from
https://downloads.linux.hpe.com/SDR/repo/mcp/debian/ does not include
any kernel modules either.

Regards,
Julian


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 6110 bytes --]

Re: [PATCH] x86/dmi: Switch dmi_remap to ioremap_cache

[Andy Lutomirski]

On Thu, Mar 9, 2017 at 9:51 AM, Arjan van de Ven <arjan@linux.intel.com> wrote:
> On 3/9/2017 9:48 AM, Julian Brost wrote:
>
>> I'm note entirely sure whether it's actually the kernel or HP to blame,
>> but for now, hp-health is completely broken on 4.9 (probably on
>> everything starting from 4.6), so this patch should be reviewed again.
>
>
> it looks like another kernel driver is doing a conflicting mapping.
> do these HP tools come with their own kernel drivers or are those in the
> upstream kernel nowadays?
>

I assume that the kernel DMI driver itself is now a conflicting
mapping.  I wonder if it could register some kind of resource so that
/dev/mem knows to just map it writeback.

On the other hand, the right answer is to use /sys/firmware/dmi.  I'm
not sure the kernel ABI stability guarantee applies to /dev/mem.

--Andy