Attached is a patch that enables AMD Geode CS5536 chipset support. I have tested it successfully on a MSM800 board from digital logic. It does, however, have a few issues that I would like some feedback on.
In my discussions with Marc Jones, Geode systems write protect the BIOS via RCONFs (cache settings similar to MTRRs). Unlocking requires changing MSR 0x1808 top byte to 0x22. Reading and writing to the msr, however, requires instrucitons rdmsr/wrmsr, which are ring0 privileged instructions so only the kernel can do the read/write. So my patch uses the msr kernel module to access these instructions from user space using the /dev/cpu/0/msr device.
My questions are: - I do not think this is portable beyond linux. Is that an issue?
- My code assumes the msr kernel module is already loaded. Is there a way to force a kernel module to load from the C code? My code does die gracefully with a message reminding them to load the kernel module if things fail.
- It seems like there should be a way to revert the msr back after flashing is completed to put the bios back in write protect mode. Is there a cleanup mechanism available? Something like disable_flash...
Thanks, Lane Brooks
Dear Lane!
Thank you very much for your patch.
* Lane Brooks lbrooks@MIT.EDU [071002 18:20]:
- It seems like there should be a way to revert the msr back after flashing
is completed to put the bios back in write protect mode. Is there a cleanup mechanism available? Something like disable_flash...
Unfortunately no. Any patches that generically implement such a mechanism are welcome though!
+#define _LARGEFILE64_SOURCE +#include <sys/types.h> +#include <sys/stat.h> +#include <fcntl.h> +#include <unistd.h>
- unsigned long addr = 0x1808;
- lseek64(fd_msr, (off64_t) addr, SEEK_SET);
Why do you use/need large file support for seeking to an offset of 0x1808 ?
Please sign off your patches: http://linuxbios.org/index.php/Development_Guidelines#How_to_contribute
On 02/10/07 18:27 +0200, Stefan Reinauer wrote:
Dear Lane!
Thank you very much for your patch.
- Lane Brooks lbrooks@MIT.EDU [071002 18:20]:
- It seems like there should be a way to revert the msr back after flashing
is completed to put the bios back in write protect mode. Is there a cleanup mechanism available? Something like disable_flash...
Unfortunately no. Any patches that generically implement such a mechanism are welcome though!
+#define _LARGEFILE64_SOURCE +#include <sys/types.h> +#include <sys/stat.h> +#include <fcntl.h> +#include <unistd.h>
- unsigned long addr = 0x1808;
- lseek64(fd_msr, (off64_t) addr, SEEK_SET);
Why do you use/need large file support for seeking to an offset of 0x1808 ?
Thats original generic MSR behavior from rdmsr/wrmsr - we need the large file support to access the high MSRs (0x80000000) and above. Its not needed in this case - but is it hurting?
Jordan
BTW there is another unsolved issue here. The chip can support FWH or LPC mode. I don't know how we should be handling that option.
ron
Jordan Crouse wrote:
On 02/10/07 18:27 +0200, Stefan Reinauer wrote:
Dear Lane!
Thank you very much for your patch.
- Lane Brooks lbrooks@MIT.EDU [071002 18:20]:
- It seems like there should be a way to revert the msr back after flashing
is completed to put the bios back in write protect mode. Is there a cleanup mechanism available? Something like disable_flash...
Unfortunately no. Any patches that generically implement such a mechanism are welcome though!
+#define _LARGEFILE64_SOURCE +#include <sys/types.h> +#include <sys/stat.h> +#include <fcntl.h> +#include <unistd.h>
- unsigned long addr = 0x1808;
- lseek64(fd_msr, (off64_t) addr, SEEK_SET);
Why do you use/need large file support for seeking to an offset of 0x1808 ?
Thats original generic MSR behavior from rdmsr/wrmsr - we need the large file support to access the high MSRs (0x80000000) and above. Its not needed in this case - but is it hurting?
Jordan
Jordan brings up a point that I forgot to mention previously. This is my first time contributing to an open source project, and I am not extremely familiar with protocols in terms of acknowledging code. I based this patch on rdmsr/wrmsr code I found on the OLPC webpage, and it seems attributable to Ron Minnich. Should I acknowledge this fact in the code?
Furthermore, I debated whether I should include the generic rdmsr/wrmsr functions rather than incorporating them into the flash_enable_cs5526() function. I chose not to include the generic functions because it seemed a cleaner approach in the grand scheme of the flashrom code base given that non of the other chipset enable functions seem to require helper functions. Any feedback on this point? Will other chipsets ever need to use the MSR to unlock the bios?
Lane
On 02/10/07 11:32 -0600, Lane Brooks wrote:
Jordan Crouse wrote:
On 02/10/07 18:27 +0200, Stefan Reinauer wrote:
Dear Lane!
Thank you very much for your patch.
- Lane Brooks lbrooks@MIT.EDU [071002 18:20]:
- It seems like there should be a way to revert the msr back after
flashing is completed to put the bios back in write protect mode. Is there a cleanup mechanism available? Something like disable_flash...
Unfortunately no. Any patches that generically implement such a mechanism are welcome though!
+#define _LARGEFILE64_SOURCE +#include <sys/types.h> +#include <sys/stat.h> +#include <fcntl.h> +#include <unistd.h>
- unsigned long addr = 0x1808;
- lseek64(fd_msr, (off64_t) addr, SEEK_SET);
Why do you use/need large file support for seeking to an offset of 0x1808 ?
Thats original generic MSR behavior from rdmsr/wrmsr - we need the large file support to access the high MSRs (0x80000000) and above. Its not needed in this case - but is it hurting?
Jordan
Jordan brings up a point that I forgot to mention previously. This is my first time contributing to an open source project, and I am not extremely familiar with protocols in terms of acknowledging code. I based this patch on rdmsr/wrmsr code I found on the OLPC webpage, and it seems attributable to Ron Minnich. Should I acknowledge this fact in the code?
I can't remember if it was Ron or I that wrote them - maybe a little of both. Those were good times.. :)
Anyway - I am of the opinion that this code is of such trival nature that it borders on the obvious - its neither enlightening nor novel - and as such attribution probably isn't called for - unless you really want to.
Jordan
On 10/2/07, Lane Brooks lbrooks@mit.edu wrote:
Jordan brings up a point that I forgot to mention previously. This is my first time contributing to an open source project, and I am not extremely familiar with protocols in terms of acknowledging code. I based this patch on rdmsr/wrmsr code I found on the OLPC webpage, and it seems attributable to Ron Minnich. Should I acknowledge this fact in the code?
Ollie Lo wrote that, but it's pretty standard stuff. No need I think unless Ollie wants it.
ron
On Tue, 2007-10-02 at 12:10 -0700, ron minnich wrote:
On 10/2/07, Lane Brooks lbrooks@mit.edu wrote:
Jordan brings up a point that I forgot to mention previously. This is my first time contributing to an open source project, and I am not extremely familiar with protocols in terms of acknowledging code. I based this patch on rdmsr/wrmsr code I found on the OLPC webpage, and it seems attributable to Ron Minnich. Should I acknowledge this fact in the code?
Ollie Lo wrote that, but it's pretty standard stuff. No need I think unless Ollie wants it.
It was nothing. Take it as public domain if I didn't put any license in it.
Ollie
On 02/10/07 10:20 -0600, Lane Brooks wrote:
Attached is a patch that enables AMD Geode CS5536 chipset support. I have tested it successfully on a MSM800 board from digital logic. It does, however, have a few issues that I would like some feedback on.
In my discussions with Marc Jones, Geode systems write protect the BIOS via RCONFs (cache settings similar to MTRRs). Unlocking requires changing MSR 0x1808 top byte to 0x22. Reading and writing to the msr, however, requires instrucitons rdmsr/wrmsr, which are ring0 privileged instructions so only the kernel can do the read/write. So my patch uses the msr kernel module to access these instructions from user space using the /dev/cpu/0/msr device.
My questions are:
- I do not think this is portable beyond linux. Is that an issue?
Right now, no. Any other *nixish OS will have to solve the MSR problem for themselves. If they chose something different, then we'll cross that bridge when we get to it.
- My code assumes the msr kernel module is already loaded. Is there a
way to force a kernel module to load from the C code? My code does die gracefully with a message reminding them to load the kernel module if things fail.
Thats the best we can do, without getting more non-portable. I think.
Jordan
On 10/2/07, Lane Brooks lbrooks@mit.edu wrote:
Attached is a patch that enables AMD Geode CS5536 chipset support. I have tested it successfully on a MSM800 board from digital logic. It does, however, have a few issues that I would like some feedback on.
In my discussions with Marc Jones, Geode systems write protect the BIOS via RCONFs (cache settings similar to MTRRs). Unlocking requires changing MSR 0x1808 top byte to 0x22. Reading and writing to the msr, however, requires instrucitons rdmsr/wrmsr, which are ring0 privileged instructions so only the kernel can do the read/write. So my patch uses the msr kernel module to access these instructions from user space using the /dev/cpu/0/msr device.
My questions are:
- I do not think this is portable beyond linux. Is that an issue?
it's not, but it is not really an issue at present.
- My code assumes the msr kernel module is already loaded. Is there a
way to force a kernel module to load from the C code? My code does die gracefully with a message reminding them to load the kernel module if things fail.
it is possible I think to have udev help with this, but I am not sure. Graceful failure is certainly a good start.
- It seems like there should be a way to revert the msr back after
flashing is completed to put the bios back in write protect mode. Is there a cleanup mechanism available? Something like disable_flash...
I thought that was in the structure of flashrom. Now that I look, it seems like we lost it! I propose this at the end of flashrom: board_flash_disable(lb_vendor, lb_part); chipset_flash_disable(chipset);
but we'll need to change some things to make this all work. We need a penable struct * to use for the disable; no point in searching each time we touch a chip. or not?
one comment on your patch: you use /dev/cpu/0/msr. This is great for a single-cpu machine, and will always be fine on geode. Lots of times, people use one piece of flashrom to write another. Imagine some future hacker, in a year or two, who has copied your code for some SMP system, not understanding why sometimes flashrom fails (i.e. they set CPU0 msr but end up running on cpu1). We had this very problem when we were getting graphics going (and, earlier, SMP going). I suggest a comment as to why the /dev/cpu/0/msr is always safe on geode, but future hackers beware on SMP. Or something like that.
That's up to you, however :-)
If you had a signed-off-by line, I would ack and commit for you :-)
ron
ron
* ron minnich rminnich@gmail.com [071002 18:38]:
My questions are:
- I do not think this is portable beyond linux. Is that an issue?
it's not, but it is not really an issue at present.
Maybe the code should be #ifdef'ed __linux__ with an #else case just printing a warning a la "Not supported by this OS yet."
I thought that was in the structure of flashrom. Now that I look, it seems like we lost it!
Flashrom never did any such cleanup. I was about to implement it when we renamed the tool but then I stumbled upon this:
printf("OK, only ENABLING flash write, but NOT FLASHING.\n");
So obviously at some point there was a sense in leaving the system in such a state. But I want to propose that we drop this behavior and instead try to always leave the machine in the state we entered it. Especially when not flashing.
While one might want to mess with an unprotected flash on purpose, for 99% of the cases this is just opening another security issue. That one % that theoretically might use this as a feature is welcome to improve the flashrom utility instead of running it for flash unprotection before running another utility.
I propose this at the end of flashrom: board_flash_disable(lb_vendor, lb_part); chipset_flash_disable(chipset);
yepp. Agreed.
but we'll need to change some things to make this all work. We need a penable struct * to use for the disable; no point in searching each time we touch a chip. or not?
To achieve this
struct board_pciid_enable *board in board_enable.c:board_flash_enable()
and
enables[i] in chipset_enable.c:chipset_flash_enable()
should be globally available.
On 02.10.2007 19:01, Stefan Reinauer wrote:
- ron minnich rminnich@gmail.com [071002 18:38]:
My questions are:
- I do not think this is portable beyond linux. Is that an issue?
it's not, but it is not really an issue at present.
Maybe the code should be #ifdef'ed __linux__ with an #else case just printing a warning a la "Not supported by this OS yet."
I thought that was in the structure of flashrom. Now that I look, it seems like we lost it!
Flashrom never did any such cleanup. I was about to implement it when we renamed the tool but then I stumbled upon this:
printf("OK, only ENABLING flash write, but NOT FLASHING.\n");
So obviously at some point there was a sense in leaving the system in such a state. But I want to propose that we drop this behavior and instead try to always leave the machine in the state we entered it. Especially when not flashing.
While one might want to mess with an unprotected flash on purpose, for 99% of the cases this is just opening another security issue. That one % that theoretically might use this as a feature is welcome to improve the flashrom utility instead of running it for flash unprotection before running another utility.
Agreed.
I propose this at the end of flashrom: board_flash_disable(lb_vendor, lb_part); chipset_flash_disable(chipset);
yepp. Agreed.
Disagree. We still have to store the previous state to be able to revert to it.
but we'll need to change some things to make this all work. We need a penable struct * to use for the disable; no point in searching each time we touch a chip. or not?
To achieve this
struct board_pciid_enable *board in board_enable.c:board_flash_enable()
and
enables[i] in chipset_enable.c:chipset_flash_enable()
should be globally available.
Different proposal: If a particular enable function has been run successfully, it should store a pointer to the corresponding cleanup function and a pointer to the data which has to be restored.
At least the restore information has to be available somewhere. Otherwise the exercise is totally pointless since we can't know the state before the enable function was run.
Carl-Daniel
Here is the patch again with an updated comment regarding SMP and a signed-off-by line.
Signed-off-by: Lane Brooks lbrooks@mit.edu
ron minnich wrote:
On 10/2/07, Lane Brooks lbrooks@mit.edu wrote:
Attached is a patch that enables AMD Geode CS5536 chipset support. I have tested it successfully on a MSM800 board from digital logic. It does, however, have a few issues that I would like some feedback on.
In my discussions with Marc Jones, Geode systems write protect the BIOS via RCONFs (cache settings similar to MTRRs). Unlocking requires changing MSR 0x1808 top byte to 0x22. Reading and writing to the msr, however, requires instrucitons rdmsr/wrmsr, which are ring0 privileged instructions so only the kernel can do the read/write. So my patch uses the msr kernel module to access these instructions from user space using the /dev/cpu/0/msr device.
My questions are:
- I do not think this is portable beyond linux. Is that an issue?
it's not, but it is not really an issue at present.
- My code assumes the msr kernel module is already loaded. Is there a
way to force a kernel module to load from the C code? My code does die gracefully with a message reminding them to load the kernel module if things fail.
it is possible I think to have udev help with this, but I am not sure. Graceful failure is certainly a good start.
- It seems like there should be a way to revert the msr back after
flashing is completed to put the bios back in write protect mode. Is there a cleanup mechanism available? Something like disable_flash...
I thought that was in the structure of flashrom. Now that I look, it seems like we lost it! I propose this at the end of flashrom: board_flash_disable(lb_vendor, lb_part); chipset_flash_disable(chipset);
but we'll need to change some things to make this all work. We need a penable struct * to use for the disable; no point in searching each time we touch a chip. or not?
one comment on your patch: you use /dev/cpu/0/msr. This is great for a single-cpu machine, and will always be fine on geode. Lots of times, people use one piece of flashrom to write another. Imagine some future hacker, in a year or two, who has copied your code for some SMP system, not understanding why sometimes flashrom fails (i.e. they set CPU0 msr but end up running on cpu1). We had this very problem when we were getting graphics going (and, earlier, SMP going). I suggest a comment as to why the /dev/cpu/0/msr is always safe on geode, but future hackers beware on SMP. Or something like that.
That's up to you, however :-)
If you had a signed-off-by line, I would ack and commit for you :-)
ron
ron
Here it is again with the patch actually attached.
Lane Brooks wrote:
Here is the patch again with an updated comment regarding SMP and a signed-off-by line.
Signed-off-by: Lane Brooks lbrooks@mit.edu
ron minnich wrote:
On 10/2/07, Lane Brooks lbrooks@mit.edu wrote:
Attached is a patch that enables AMD Geode CS5536 chipset support. I have tested it successfully on a MSM800 board from digital logic. It does, however, have a few issues that I would like some feedback on.
In my discussions with Marc Jones, Geode systems write protect the BIOS via RCONFs (cache settings similar to MTRRs). Unlocking requires changing MSR 0x1808 top byte to 0x22. Reading and writing to the msr, however, requires instrucitons rdmsr/wrmsr, which are ring0 privileged instructions so only the kernel can do the read/write. So my patch uses the msr kernel module to access these instructions from user space using the /dev/cpu/0/msr device.
My questions are:
- I do not think this is portable beyond linux. Is that an issue?
it's not, but it is not really an issue at present.
- My code assumes the msr kernel module is already loaded. Is there a
way to force a kernel module to load from the C code? My code does die gracefully with a message reminding them to load the kernel module if things fail.
it is possible I think to have udev help with this, but I am not sure. Graceful failure is certainly a good start.
- It seems like there should be a way to revert the msr back after
flashing is completed to put the bios back in write protect mode. Is there a cleanup mechanism available? Something like disable_flash...
I thought that was in the structure of flashrom. Now that I look, it seems like we lost it! I propose this at the end of flashrom: board_flash_disable(lb_vendor, lb_part); chipset_flash_disable(chipset);
but we'll need to change some things to make this all work. We need a penable struct * to use for the disable; no point in searching each time we touch a chip. or not?
one comment on your patch: you use /dev/cpu/0/msr. This is great for a single-cpu machine, and will always be fine on geode. Lots of times, people use one piece of flashrom to write another. Imagine some future hacker, in a year or two, who has copied your code for some SMP system, not understanding why sometimes flashrom fails (i.e. they set CPU0 msr but end up running on cpu1). We had this very problem when we were getting graphics going (and, earlier, SMP going). I suggest a comment as to why the /dev/cpu/0/msr is always safe on geode, but future hackers beware on SMP. Or something like that.
That's up to you, however :-)
If you had a signed-off-by line, I would ack and commit for you :-)
ron
ron
Lane Brooks wrote:
Here it is again with the patch actually attached.
Lane Brooks wrote:
Here is the patch again with an updated comment regarding SMP and a signed-off-by line.
Signed-off-by: Lane Brooks lbrooks@mit.edu
ron minnich wrote:
On 10/2/07, Lane Brooks lbrooks@mit.edu wrote:
Attached is a patch that enables AMD Geode CS5536 chipset support. I have tested it successfully on a MSM800 board from digital logic. It does, however, have a few issues that I would like some feedback on.
In my discussions with Marc Jones, Geode systems write protect the BIOS via RCONFs (cache settings similar to MTRRs). Unlocking requires changing MSR 0x1808 top byte to 0x22. Reading and writing to the msr, however, requires instrucitons rdmsr/wrmsr, which are ring0 privileged instructions so only the kernel can do the read/write. So my patch uses the msr kernel module to access these instructions from user space using the /dev/cpu/0/msr device.
My questions are:
- I do not think this is portable beyond linux. Is that an issue?
it's not, but it is not really an issue at present.
- My code assumes the msr kernel module is already loaded. Is there a
way to force a kernel module to load from the C code? My code does die gracefully with a message reminding them to load the kernel module if things fail.
it is possible I think to have udev help with this, but I am not sure. Graceful failure is certainly a good start.
- It seems like there should be a way to revert the msr back after
flashing is completed to put the bios back in write protect mode. Is there a cleanup mechanism available? Something like disable_flash...
I thought that was in the structure of flashrom. Now that I look, it seems like we lost it! I propose this at the end of flashrom: board_flash_disable(lb_vendor, lb_part); chipset_flash_disable(chipset);
but we'll need to change some things to make this all work. We need a penable struct * to use for the disable; no point in searching each time we touch a chip. or not?
one comment on your patch: you use /dev/cpu/0/msr. This is great for a single-cpu machine, and will always be fine on geode. Lots of times, people use one piece of flashrom to write another. Imagine some future hacker, in a year or two, who has copied your code for some SMP system, not understanding why sometimes flashrom fails (i.e. they set CPU0 msr but end up running on cpu1). We had this very problem when we were getting graphics going (and, earlier, SMP going). I suggest a comment as to why the /dev/cpu/0/msr is always safe on geode, but future hackers beware on SMP. Or something like that.
That's up to you, however :-)
If you had a signed-off-by line, I would ack and commit for you :-)
ron
ron
Did anyone ack and commit this last version?
Marc
On 02/10/07 10:20 -0600, Lane Brooks wrote:
Attached is a patch that enables AMD Geode CS5536 chipset support. I have tested it successfully on a MSM800 board from digital logic. It does, however, have a few issues that I would like some feedback on.
NAK for now - its not working on the Norwich. I am debugging.
Jordan
On 02/10/07 10:57 -0600, Jordan Crouse wrote:
On 02/10/07 10:20 -0600, Lane Brooks wrote:
Attached is a patch that enables AMD Geode CS5536 chipset support. I have tested it successfully on a MSM800 board from digital logic. It does, however, have a few issues that I would like some feedback on.
NAK for now - its not working on the Norwich. I am debugging.
NAK my NAK (making it an ACK). I put in a larger ROM and confused the MSR.
Jordan
On Tue, Oct 02, 2007 at 11:38:02AM -0600, Jordan Crouse wrote:
On 02/10/07 10:57 -0600, Jordan Crouse wrote:
On 02/10/07 10:20 -0600, Lane Brooks wrote:
Attached is a patch that enables AMD Geode CS5536 chipset support. I have tested it successfully on a MSM800 board from digital logic. It does, however, have a few issues that I would like some feedback on.
NAK for now - its not working on the Norwich. I am debugging.
NAK my NAK (making it an ACK). I put in a larger ROM and confused the MSR.
Has this been resolved? Possibly in that commit which no longer blindly sets the RCONF value but only flips the relevant bits?
//Peter
-
Carl-Daniel Hailfinger -
Jordan Crouse -
Lane Brooks -
Li-Ta Lo -
Marc Jones -
Peter Stuge -
ron minnich -
Stefan Reinauer