[flashrom] MSR access fix on ALIX2D13 running OpenBSD-5.3

[Stefan Tauner]

On Wed, 10 Apr 2013 20:36:48 +0200
Leonardo Guardati <leonardo at guardati.it> wrote:

> Using flashrom-trunk on PCEngines' ALIX2D13 board running 
> OpenBSD-5.3 (current) doesn't work.
> 
> The chipset is AMD Geode CS5536.
> 
> Here the steps:
> 
>  1. Applied patch amdmsr.patch to openbsd kernel (/usr/src/sys/arch/i386/i386/amdmsr.c)
> Explaination: the original code checks for GPU presence (alix has no GPU) and fails; I
> just skip the check. 

interesting. does upstream know about this?

>  2. got last flashrom version (trunk)
> 
>  3. applied given patch http://patchwork.coreboot.org/patch/3932/ 

that one is included in trunk now.

>  4. applied given patch http://patchwork.coreboot.org/patch/3933/
> 
>  5. applied patch hwaccess.patch
> Explaination: openbsd uses a different way to retrieve IO Priviledge Level, see i386_iopl(2) man page.

did you try without it? hwaccess.h should take care of that already,
while your change would break on amd64...

>  6. applied patch physmap.patch 
> Explaination: minor code mismatches fixed and /dev/amdmsr0 changed to /dev/amdmsr.

let's look at those changes in detail.

> --- ../physmap.c	Wed Apr 10 19:58:38 2013
> +++ physmap-fix.c	Wed Apr 10 20:15:50 2013
> @@ -388,11 +388,16 @@
>  #include <sys/ioctl.h>
>  #include <machine/amdmsr.h>
>  
> +typedef struct {
> +	int msr;
> +	uint64_t data;
> +} cpu_msr_args_t;
> +

This should not be needed, because we should use struct amdmsr_req, see below.

>  static int fd_msr = -1;
>  
>  msr_t rdmsr(int addr)
>  {
> -	amdmsr_req args;
> +	struct amdmsr_req args;

just like here...

>  
>  	msr_t msr = { 0xffffffff, 0xffffffff };
>  
> @@ -415,7 +420,7 @@
>  	cpu_msr_args_t args;
here    ^^^^^^^^^^^^^^^^^^^^

>  
>  	args.msr = addr;
> -	args.val = (((uint64_t)msr.hi) << 32) | msr.lo;
> +	args.data = (((uint64_t)msr.hi) << 32) | msr.lo;

then this is not needed either

>  	if (ioctl(fd_msr, WRMSR, &args) < 0) {
>  		msg_perr("Error while executing WRMSR ioctl: %s\n", strerror(errno));
> @@ -430,7 +435,7 @@
>  {
>  	char msrfilename[64];
>  	memset(msrfilename, 0, sizeof(msrfilename));
> -	snprintf(msrfilename, sizeof(msrfilename), "/dev/amdmsr%d", cpu);
> +	snprintf(msrfilename, sizeof(msrfilename), "/dev/amdmsr");

that's fine though. i was confused by the search result for "amdmsr0" i
think.

i'll post new patches in a minute (one corrected msr patch and an
independent hwaccess.c patch that should fix netbsd hopefully and makes
the defines more usable and sane and should be tested on openbsd too).
i'd be very glad if you could test them on top of current HEAD again.

-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner