Stefan: Could you please test this one on Mac OS X? In theory it should even allow us to kill the start=0x400 special case for Mac OS X.
Create a physical memory mapping function which requests cached readonly memory. This should take care of picky Linux kernels which do not allow uncached mappings to cached areas. Handle mapping failure gracefully (no forced exit()) if the caller specifies it.
Such cached areas which can handle mapping failure are DMI tables and coreboot tables. On retry and repeated failure we just ignore those tables. That is not perfect, but a lot better than aborting flashrom due to an error in nonessential functionality.
This should fix flashrom on a sizable number of machines where it currently aborts early.
Yes, I could have exploited a Linux kernel bug to "solve" this, but relying on such bugs is not exactly the best idea.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: flashrom-physmap_graceful_error_cbtable/flash.h =================================================================== --- flashrom-physmap_graceful_error_cbtable/flash.h (Revision 886) +++ flashrom-physmap_graceful_error_cbtable/flash.h (Arbeitskopie) @@ -343,6 +343,7 @@
/* physmap.c */ void *physmap(const char *descr, unsigned long phys_addr, size_t len); +void *physmap_try_ro(const char *descr, unsigned long phys_addr, size_t len); void physunmap(void *virt_addr, size_t len); int setup_cpu_msr(int cpu); void cleanup_cpu_msr(void); Index: flashrom-physmap_graceful_error_cbtable/physmap.c =================================================================== --- flashrom-physmap_graceful_error_cbtable/physmap.c (Revision 886) +++ flashrom-physmap_graceful_error_cbtable/physmap.c (Arbeitskopie) @@ -3,6 +3,7 @@ * * Copyright (C) 2009 Peter Stuge peter@stuge.se * Copyright (C) 2009 coresystems GmbH + * Copyright (C) 2010 Carl-Daniel Hailfinger * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -36,6 +37,10 @@ return map_physical(phys_addr, len); }
+/* The OS X driver does not differentiate between mapping types. */ +#define sys_physmap_rw_uncached sys_physmap +#define sys_physmap_ro_cached sys_physmap + void physunmap(void *virt_addr, size_t len) { unmap_physical(virt_addr, len); @@ -51,8 +56,10 @@ #endif
static int fd_mem = -1; +static int fd_mem_cached = -1;
-void *sys_physmap(unsigned long phys_addr, size_t len) +/* For MMIO access. Must be uncached, doesn't make sense to restrict to ro. */ +void *sys_physmap_rw_uncached(unsigned long phys_addr, size_t len) { void *virt_addr;
@@ -69,6 +76,26 @@ return MAP_FAILED == virt_addr ? NULL : virt_addr; }
+/* For reading DMI/coreboot/whatever tables. We should never write, and we + * do not care about caching. + */ +void *sys_physmap_ro_cached(unsigned long phys_addr, size_t len) +{ + void *virt_addr; + + if (-1 == fd_mem_cached) { + /* Open the memory device CACHED. */ + if (-1 == (fd_mem_cached = open(MEM_DEV, O_RDWR))) { + perror("Critical error: open(" MEM_DEV ")"); + exit(2); + } + } + + virt_addr = mmap(0, len, PROT_READ, MAP_SHARED, + fd_mem_cached, (off_t)phys_addr); + return MAP_FAILED == virt_addr ? NULL : virt_addr; +} + void physunmap(void *virt_addr, size_t len) { if (len == 0) { @@ -80,7 +107,12 @@ } #endif
-void *physmap(const char *descr, unsigned long phys_addr, size_t len) +#define PHYSMAP_NOFAIL 0 +#define PHYSMAP_MAYFAIL 1 +#define PHYSMAP_RW 0 +#define PHYSMAP_RO 1 + +void *physmap_common(const char *descr, unsigned long phys_addr, size_t len, int mayfail, int readonly) { void *virt_addr;
@@ -100,7 +132,11 @@ descr, (unsigned long)len, phys_addr); }
- virt_addr = sys_physmap(phys_addr, len); + if (readonly) { + virt_addr = sys_physmap_ro_cached(phys_addr, len); + } else { + virt_addr = sys_physmap_rw_uncached(phys_addr, len); + }
if (NULL == virt_addr) { if (NULL == descr) @@ -114,12 +150,23 @@ fprintf(stderr, "You can override CONFIG_X86_PAT at boot with the nopat kernel parameter but\n"); fprintf(stderr, "disabling the other option unfortunately requires a kernel recompile. Sorry!\n"); } - exit(3); + if (!mayfail) + exit(3); }
return virt_addr; }
+void *physmap(const char *descr, unsigned long phys_addr, size_t len) +{ + return physmap_common(descr, phys_addr, len, PHYSMAP_NOFAIL, PHYSMAP_RW); +} + +void *physmap_try_ro(const char *descr, unsigned long phys_addr, size_t len) +{ + return physmap_common(descr, phys_addr, len, PHYSMAP_MAYFAIL, PHYSMAP_RO); +} + #ifdef __linux__ /* * Reading and writing to MSRs, however requires instructions rdmsr/wrmsr, Index: flashrom-physmap_graceful_error_cbtable/cbtable.c =================================================================== --- flashrom-physmap_graceful_error_cbtable/cbtable.c (Revision 886) +++ flashrom-physmap_graceful_error_cbtable/cbtable.c (Arbeitskopie) @@ -6,6 +6,7 @@ * (Written by Eric Biederman ebiederman@lnxi.com for Linux Networx) * Copyright (C) 2006-2009 coresystems GmbH * (Written by Stefan Reinauer stepan@coresystems.de for coresystems GmbH) + * Copyright (C) 2010 Carl-Daniel Hailfinger * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -194,24 +195,36 @@ #define BYTES_TO_MAP (1024*1024) int coreboot_init(void) { - uint8_t *table_area; + uint8_t *table_area = NULL; unsigned long addr, start; - struct lb_header *lb_table; + struct lb_header *lb_table = NULL; struct lb_record *rec, *last;
#ifdef __DARWIN__ /* This is a hack. DirectIO fails to map physical address 0x00000000. * Why? + * FIXME: This hack should be unnecessary with the low megabyte retries. */ start = 0x400; #else start = 0x0; #endif - table_area = physmap("low megabyte", start, BYTES_TO_MAP); + table_area = physmap_try_ro("low megabyte", start, BYTES_TO_MAP - start); + while (!table_area) { + msg_pinfo("Trying a smaller mapping.\n"); + start += 0x400; + /* Outside lower 1 MB? Abort. */ + if (start > 0x100000) { + msg_perr("Failed getting access to low megabyte. " + "Ignoring any coreboot tables.\n"); + return -1; + } + table_area = physmap_try_ro("low megabyte", start, BYTES_TO_MAP - start); + }
lb_table = find_lb_table(table_area, 0x00000, 0x1000); if (!lb_table) - lb_table = find_lb_table(table_area, 0xf0000, BYTES_TO_MAP); + lb_table = find_lb_table(table_area, 0xf0000 - start, BYTES_TO_MAP - start); if (lb_table) { struct lb_forward *forward = (struct lb_forward *) (((char *)lb_table) + lb_table->header_bytes); @@ -219,7 +232,12 @@ start = forward->forward; start &= ~(getpagesize() - 1); physunmap(table_area, BYTES_TO_MAP); - table_area = physmap("high tables", start, BYTES_TO_MAP); + table_area = physmap_try_ro("high tables", start, BYTES_TO_MAP); + if (!table_area) { + msg_perr("Failed getting access to coreboot " + "high tables.\n"); + return -1; + } lb_table = find_lb_table(table_area, 0x00000, 0x1000); } }
Hi Vincent,
can you please try this patch on top of an unmodified flashrom source? It is available for download at http://patchwork.coreboot.org/patch/861/raw/
On 01.02.2010 05:41, Carl-Daniel Hailfinger wrote:
Stefan: Could you please test this one on Mac OS X? In theory it should even allow us to kill the start=0x400 special case for Mac OS X.
Create a physical memory mapping function which requests cached readonly memory. This should take care of picky Linux kernels which do not allow uncached mappings to cached areas. Handle mapping failure gracefully (no forced exit()) if the caller specifies it.
Such cached areas which can handle mapping failure are DMI tables and coreboot tables. On retry and repeated failure we just ignore those tables. That is not perfect, but a lot better than aborting flashrom due to an error in nonessential functionality.
This should fix flashrom on a sizable number of machines where it currently aborts early.
Yes, I could have exploited a Linux kernel bug to "solve" this, but relying on such bugs is not exactly the best idea.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Please run "flashrom -V", send us the output and if detection works for you, please add the following line to your mail: Acked-by: Vincent Pelletier your@email.address
Thanks.
Regards, Carl-Daniel
Le lundi 01 février 2010 12:55:14, Carl-Daniel Hailfinger a écrit :
Please run "flashrom -V", send us the output and if detection works for you, please add the following line to your mail: Acked-by: Vincent Pelletier your@email.address
Tested and it fixes the problem. Congratulations ! See attachment for output.
Acked-by: Vincent Pelletier plr.vincent@gmail.com
On 01.02.2010 19:49, Vincent Pelletier wrote:
Tested and it fixes the problem. Congratulations ! See attachment for output.
Thanks for testing!
Acked-by: Vincent Pelletier plr.vincent@gmail.com
Here is a new patch without the retrying code which turned out to be unneeded. ----- Create a physical memory mapping function which requests cached readonly memory. This should take care of picky Linux kernels which do not allow uncached mappings to cached areas. Handle mapping failure gracefully (no forced exit()) if the caller specifies it.
Such cached areas which can handle mapping failure are DMI tables and coreboot tables. On failure we just ignore those tables. That is not perfect, but a lot better than aborting flashrom due to an error in nonessential functionality.
This should fix flashrom on a sizable number of machines where it currently aborts early.
Yes, I could have exploited a Linux kernel bug to "solve" this, but relying on such bugs is not exactly the best idea.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net Acked-by: Vincent Pelletier plr.vincent@gmail.com
Index: flashrom-physmap_graceful_error_cbtable/flash.h =================================================================== --- flashrom-physmap_graceful_error_cbtable/flash.h (Revision 888) +++ flashrom-physmap_graceful_error_cbtable/flash.h (Arbeitskopie) @@ -343,6 +343,7 @@
/* physmap.c */ void *physmap(const char *descr, unsigned long phys_addr, size_t len); +void *physmap_try_ro(const char *descr, unsigned long phys_addr, size_t len); void physunmap(void *virt_addr, size_t len); int setup_cpu_msr(int cpu); void cleanup_cpu_msr(void); Index: flashrom-physmap_graceful_error_cbtable/physmap.c =================================================================== --- flashrom-physmap_graceful_error_cbtable/physmap.c (Revision 888) +++ flashrom-physmap_graceful_error_cbtable/physmap.c (Arbeitskopie) @@ -3,6 +3,7 @@ * * Copyright (C) 2009 Peter Stuge peter@stuge.se * Copyright (C) 2009 coresystems GmbH + * Copyright (C) 2010 Carl-Daniel Hailfinger * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -36,6 +37,10 @@ return map_physical(phys_addr, len); }
+/* The OS X driver does not differentiate between mapping types. */ +#define sys_physmap_rw_uncached sys_physmap +#define sys_physmap_ro_cached sys_physmap + void physunmap(void *virt_addr, size_t len) { unmap_physical(virt_addr, len); @@ -51,8 +56,10 @@ #endif
static int fd_mem = -1; +static int fd_mem_cached = -1;
-void *sys_physmap(unsigned long phys_addr, size_t len) +/* For MMIO access. Must be uncached, doesn't make sense to restrict to ro. */ +void *sys_physmap_rw_uncached(unsigned long phys_addr, size_t len) { void *virt_addr;
@@ -69,6 +76,26 @@ return MAP_FAILED == virt_addr ? NULL : virt_addr; }
+/* For reading DMI/coreboot/whatever tables. We should never write, and we + * do not care about caching. + */ +void *sys_physmap_ro_cached(unsigned long phys_addr, size_t len) +{ + void *virt_addr; + + if (-1 == fd_mem_cached) { + /* Open the memory device CACHED. */ + if (-1 == (fd_mem_cached = open(MEM_DEV, O_RDWR))) { + perror("Critical error: open(" MEM_DEV ")"); + exit(2); + } + } + + virt_addr = mmap(0, len, PROT_READ, MAP_SHARED, + fd_mem_cached, (off_t)phys_addr); + return MAP_FAILED == virt_addr ? NULL : virt_addr; +} + void physunmap(void *virt_addr, size_t len) { if (len == 0) { @@ -80,7 +107,12 @@ } #endif
-void *physmap(const char *descr, unsigned long phys_addr, size_t len) +#define PHYSMAP_NOFAIL 0 +#define PHYSMAP_MAYFAIL 1 +#define PHYSMAP_RW 0 +#define PHYSMAP_RO 1 + +void *physmap_common(const char *descr, unsigned long phys_addr, size_t len, int mayfail, int readonly) { void *virt_addr;
@@ -100,7 +132,11 @@ descr, (unsigned long)len, phys_addr); }
- virt_addr = sys_physmap(phys_addr, len); + if (readonly) { + virt_addr = sys_physmap_ro_cached(phys_addr, len); + } else { + virt_addr = sys_physmap_rw_uncached(phys_addr, len); + }
if (NULL == virt_addr) { if (NULL == descr) @@ -114,12 +150,23 @@ fprintf(stderr, "You can override CONFIG_X86_PAT at boot with the nopat kernel parameter but\n"); fprintf(stderr, "disabling the other option unfortunately requires a kernel recompile. Sorry!\n"); } - exit(3); + if (!mayfail) + exit(3); }
return virt_addr; }
+void *physmap(const char *descr, unsigned long phys_addr, size_t len) +{ + return physmap_common(descr, phys_addr, len, PHYSMAP_NOFAIL, PHYSMAP_RW); +} + +void *physmap_try_ro(const char *descr, unsigned long phys_addr, size_t len) +{ + return physmap_common(descr, phys_addr, len, PHYSMAP_MAYFAIL, PHYSMAP_RO); +} + #ifdef __linux__ /* * Reading and writing to MSRs, however requires instructions rdmsr/wrmsr, Index: flashrom-physmap_graceful_error_cbtable/cbtable.c =================================================================== --- flashrom-physmap_graceful_error_cbtable/cbtable.c (Revision 888) +++ flashrom-physmap_graceful_error_cbtable/cbtable.c (Arbeitskopie) @@ -6,6 +6,7 @@ * (Written by Eric Biederman ebiederman@lnxi.com for Linux Networx) * Copyright (C) 2006-2009 coresystems GmbH * (Written by Stefan Reinauer stepan@coresystems.de for coresystems GmbH) + * Copyright (C) 2010 Carl-Daniel Hailfinger * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -207,11 +208,15 @@ #else start = 0x0; #endif - table_area = physmap("low megabyte", start, BYTES_TO_MAP); + table_area = physmap_try_ro("low megabyte", start, BYTES_TO_MAP - start); + if (!table_area) { + msg_perr("Failed getting access to coreboot low tables.\n"); + return -1; + }
lb_table = find_lb_table(table_area, 0x00000, 0x1000); if (!lb_table) - lb_table = find_lb_table(table_area, 0xf0000, BYTES_TO_MAP); + lb_table = find_lb_table(table_area, 0xf0000 - start, BYTES_TO_MAP - start); if (lb_table) { struct lb_forward *forward = (struct lb_forward *) (((char *)lb_table) + lb_table->header_bytes); @@ -219,7 +224,12 @@ start = forward->forward; start &= ~(getpagesize() - 1); physunmap(table_area, BYTES_TO_MAP); - table_area = physmap("high tables", start, BYTES_TO_MAP); + table_area = physmap_try_ro("high tables", start, BYTES_TO_MAP); + if (!table_area) { + msg_perr("Failed getting access to coreboot " + "high tables.\n"); + return -1; + } lb_table = find_lb_table(table_area, 0x00000, 0x1000); } }
On 02.02.2010 12:04, Carl-Daniel Hailfinger wrote:
Create a physical memory mapping function which requests cached readonly memory. This should take care of picky Linux kernels which do not allow uncached mappings to cached areas.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net Acked-by: Vincent Pelletier plr.vincent@gmail.com
And committed in r889.
Vincent, flashrom should now work for you out of the box (no patches needed).
Regards, Carl-Daniel
On 2/1/10 5:41 AM, Carl-Daniel Hailfinger wrote:
Stefan: Could you please test this one on Mac OS X? In theory it should even allow us to kill the start=0x400 special case for Mac OS X.
Sorry I don't have a machine with OSX and coreboot table at my disposal right now, but on my macbookpro I get this:
flashrom v0.9.1-r888 Mapping low megabyte at 0x00000400, unaligned size 0xffc00. Mapping low megabyte, 0xffc00 bytes at unaligned 0x00000400. No coreboot table found. sh: dmidecode: command not found Found chipset "Intel ICH8M", enabling flash write... OK. This chipset supports the following protocols: FWH,SPI. Calibrating delay loop... OK. Found chip "SST SST25VF016B" (2048 KB, SPI) at physical address 0xffe00000. === This flash part has status UNTESTED for operations: ERASE Please email a report to flashrom@flashrom.org if any of the above operations work correctly for you with this flash part. Please include the flashrom output with the additional -V option for all operations you tested (-V, -rV, -wV, -EV), and mention which mainboard or programmer you tested. Thanks for your help! === No operations were specified.
However, I'd prefer a 1 line special case over adding 64 lines to the normal case for the reason ;-)
On 01.02.2010 17:12, Stefan Reinauer wrote:
On 2/1/10 5:41 AM, Carl-Daniel Hailfinger wrote:
Stefan: Could you please test this one on Mac OS X? In theory it should even allow us to kill the start=0x400 special case for Mac OS X.
Sorry I don't have a machine with OSX and coreboot table at my disposal right now, but on my macbookpro I get this:
flashrom v0.9.1-r888 Mapping low megabyte at 0x00000400, unaligned size 0xffc00. Mapping low megabyte, 0xffc00 bytes at unaligned 0x00000400. No coreboot table found. sh: dmidecode: command not found Found chipset "Intel ICH8M", enabling flash write... OK. This chipset supports the following protocols: FWH,SPI. Calibrating delay loop... OK. Found chip "SST SST25VF016B" (2048 KB, SPI) at physical address 0xffe00000. === This flash part has status UNTESTED for operations: ERASE Please email a report to flashrom@flashrom.org if any of the above operations work correctly for you with this flash part. Please include the flashrom output with the additional -V option for all operations you tested (-V, -rV, -wV, -EV), and mention which mainboard or programmer you tested. Thanks for your help! === No operations were specified.
However, I'd prefer a 1 line special case over adding 64 lines to the normal case for the reason ;-)
Thanks for testing.
The problem was that the mapping failed on some Linux machines because the kernel detected a mismatch between the PAT/MTRR (cached write-combining) and the requested mapping type (uncached) for the coreboot tables. That's why I introduced all the special code (a one-liner wouldn't have worked). The side effect of being able to use start=0x0 in the cbtables code under OS X is just a lucky coincidence.
For me, three things are important about this patch: 1. Does it fix the mapping on some Linux kernels/machines? 2. Does it still work on previously working Linux machines? 3. Does it still work on Mac OS X?
So far, at least requirement 2 seems to be fulfilled.
Regards, Carl-Daniel
-
Carl-Daniel Hailfinger -
Stefan Reinauer -
Vincent Pelletier