MSR access fix on ALIX2D13 running OpenBSD-5.3

[Leonardo Guardati]

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.

2. got last flashrom version (trunk)

3. applied given patch http://patchwork.coreboot.org/patch/3932/ 
 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.

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

The 3 patches are attached.

I confirm running "./flashrom -p internal -r bios.bin" extracts the same bios image
got from flashrom run on linux. (md5 checks)

Thank You.

Leonardo.

[Stefan Tauner]

On Wed, 10 Apr 2013 20:36:48 +0200
Leonardo Guardati  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:
> 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?
> got last flashrom version (trunk)
>
> applied given patch http://patchwork.coreboot.org/patch/3932/

that one is included in trunk now.
> applied given patch http://patchwork.coreboot.org/patch/3933/
>
> 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...
> 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.

[Leonardo Guardati]

On Sat, Apr 13, 2013 at 01:49:54AM +0200, Stefan Tauner wrote:
> On Wed, 10 Apr 2013 20:36:48 +0200
> Leonardo Guardati  wrote:
> > ...
> > 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?

I've sent a message on [e-mail] ml; but I've an update for this problem.
I've managed to flash coreboot on the ALIX board (had to use crossgcc to successfully
compile) and magically now the probe works! So I suppose the original tinyBIOS did
something wrong (for example disable something GP-related knowing the board doesn't 
have any display interface)

Attached the dmesg using the original tinyBIOS and the diff using coreboot.
> ...
>
> 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.

Sure, as soon as I can I will checkout the last trunk and test again.

Note: There is also a warning about a second CS5536 chipset being found.

Thank You.

Leonardo.

[Leonardo Guardati]

On Sat, Apr 13, 2013 at 01:49:54AM +0200, Stefan Tauner wrote:
> 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.

sorry for the delay, I've just checked out the last version of flashrom,
then I applied your 2 patches:
 -  http://patchwork.coreboot.org/patch/3936/ (hwaccess.c)
 -  http://patchwork.coreboot.org/patch/3937/ (physmap.c)

There is only a wrong member name to fix on physmap.c (line 415)
(the patch attached works with patch #3937 already applied)

After the fix the code works: I've tested the checksum of an already
flashed image, then wrote another image and checked again the checksum.

Thank You.

Leonardo.

P.S.
I confirm the OpenBSD msr probe failure is tinyBIOS related.
When the bios is coreboot, the probe is successfull.

[Stefan Tauner]

On Wed, 17 Apr 2013 12:01:01 +0200
Leonardo Guardati  wrote:
> On Sat, Apr 13, 2013 at 01:49:54AM +0200, Stefan Tauner wrote:
> > 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.
>
> sorry for the delay, I've just checked out the last version of flashrom,
> then I applied your 2 patches:
> http://patchwork.coreboot.org/patch/3936/ (hwaccess.c)
> http://patchwork.coreboot.org/patch/3937/ (physmap.c)
>
> There is only a wrong member name to fix on physmap.c (line 415)
> (the patch attached works with patch #3937 already applied)

I should definitely not code when tired without being able to compile
test my nonsense. :) sorry!
> After the fix the code works: I've tested the checksum of an already
> flashed image, then wrote another image and checked again the checksum.
>
> Thank You.
>
> Leonardo.
>
> P.S.
> I confirm the OpenBSD msr probe failure is tinyBIOS related.
> When the bios is coreboot, the probe is successfull.

Ok, I'll wait for someone to check the NetBSD-related changes and
commit the two patches then. Thanks a lot for testing - this kind of
contribution is invaluable to us.