Patchwork misc coreboot utility patches

login
register
about
Submitter Stefan Reinauer
Date 2009-08-31 18:25:48
Message ID <4A9C15AC.7060009@coresystems.de>
Download mbox | patch
Permalink /patch/233/
State Accepted
Headers show

Comments

Stefan Reinauer - 2009-08-31 18:25:48
See attachments
Peter Stuge - 2009-08-31 19:27:11
Stefan Reinauer wrote:
> port msrtool to darwin.
> 
> Signed-off-by: Stefan Reinauer <stepan@coresystems.de>

With comments addressed, go for it!

Acked-by: Peter Stuge <peter@stuge.se>


> +int darwin_probe(const struct sysdef *system)
> +{
> +	return iopl(3) == 0;
> +}

Ideally this would check that system is really Darwin.


>  struct cpuid_t *cpuid(void) {
>  	uint32_t outeax;
> +
> +#ifdef __DARWIN__
> +        asm volatile (
> +                "pushl %%ebx    \n"
> +                "cpuid          \n"
> +                "popl %%ebx     \n"
> +                : "=a" (outeax) : "a" (1) : "%ecx", "%edx"
> +        );
> +#else
>  	asm ("cpuid" : "=a" (outeax) : "a" (1) : "%ebx", "%ecx", "%edx");
> +#endif
> +

I have no problem with this, but can you explain why the manual
push/pop is needed? Maybe the new variant is the only one we need?


> -LDFLAGS=`trylink "libpci (from pciutils)" "${pc_LDFLAGS}" "-lpci -lz" "-L/usr/local/lib -lpci -lz" "-framework IOKit -L/usr/local/lib -lpci -lz"` || {
> +LDFLAGS=`trylink "libpci (from pciutils)" "${pc_LDFLAGS}" "-lpci -lz" "-lpci -lz" "-framework IOKit -framework DirectIO -lpci -lz"` || {

Why remove /usr/local/lib ? I'd like to have that still in there.


//Peter
Stefan Reinauer - 2009-08-31 19:48:04
On 8/31/09 9:27 PM, Peter Stuge wrote:

>> +int darwin_probe(const struct sysdef *system)
>> +{
>> +	return iopl(3) == 0;
>> +}
>>     
> Ideally this would check that system is really Darwin.
>
>   

Ah right.. I misunderstood the API.

Shouldn't the code for Darwin be only compiled into the Darwin
executable, and the code for Linux only on Linux systems?

Doing a check every time the program runs when it can be checked at
compile time seems a bit overkill, on the first thought.


>   
>>  struct cpuid_t *cpuid(void) {
>>  	uint32_t outeax;
>> +
>> +#ifdef __DARWIN__
>> +        asm volatile (
>> +                "pushl %%ebx    \n"
>> +                "cpuid          \n"
>> +                "popl %%ebx     \n"
>> +                : "=a" (outeax) : "a" (1) : "%ecx", "%edx"
>> +        );
>> +#else
>>  	asm ("cpuid" : "=a" (outeax) : "a" (1) : "%ebx", "%ecx", "%edx");
>> +#endif
>> +
>>     
> I have no problem with this, but can you explain why the manual
> push/pop is needed? Maybe the new variant is the only one we need?
>   
ebx is a reserved register on OSX/PIC


>
>   
>> -LDFLAGS=`trylink "libpci (from pciutils)" "${pc_LDFLAGS}" "-lpci -lz" "-L/usr/local/lib -lpci -lz" "-framework IOKit -L/usr/local/lib -lpci -lz"` || {
>> +LDFLAGS=`trylink "libpci (from pciutils)" "${pc_LDFLAGS}" "-lpci -lz" "-lpci -lz" "-framework IOKit -framework DirectIO -lpci -lz"` || {
>>     
> Why remove /usr/local/lib ? I'd like to have that still in there.
>   

Because the default install location for the packages is in /usr.. The
libpci in /usr/local comes from another source which is incompatible
with the one compiled with directio.

Stefan
Peter Stuge - 2009-09-01 13:57:13
Stefan Reinauer wrote:
> Ah right.. I misunderstood the API.

No problem.


> Shouldn't the code for Darwin be only compiled into the Darwin
> executable, and the code for Linux only on Linux systems?
> 
> Doing a check every time the program runs when it can be checked at
> compile time seems a bit overkill, on the first thought.

Yes, if there's no chance for the binary to be used on another
system. (File formats, etc.)

The idea behind the system list is that some systems may be similar
enough to use the same binary, but will still need different register
access implementations and that some systems could support multiple
register access implementations.

But maybe there will only ever be two? Native, and inline asm rdmsr?
There's no use left for the probing then.


> > I have no problem with this, but can you explain why the manual
> > push/pop is needed? Maybe the new variant is the only one we need?
> 
> ebx is a reserved register on OSX/PIC

Ok. Do you still recommend to keep both implementations?


> >> -LDFLAGS=`trylink "libpci (from pciutils)" "${pc_LDFLAGS}" "-lpci -lz" "-L/usr/local/lib -lpci -lz" "-framework IOKit -L/usr/local/lib -lpci -lz"` || {
> >> +LDFLAGS=`trylink "libpci (from pciutils)" "${pc_LDFLAGS}" "-lpci -lz" "-lpci -lz" "-framework IOKit -framework DirectIO -lpci -lz"` || {
> >>     
> > Why remove /usr/local/lib ? I'd like to have that still in there.
> 
> Because the default install location for the packages is in /usr..

Not in pciutils-3.1.2.tar.gz;

$ tar xfz /usr/portage/distfiles/pciutils-3.1.2.tar.gz
$ cd pciutils-3.1.2/
$ grep PREFIX Makefile
PREFIX=/usr/local
...


> The libpci in /usr/local comes from another source which is
> incompatible with the one compiled with directio.

I understand. Will it build successfully but fail to run - or also
fail to build? Hm, in any case I think it's possible to fix this up.
I suggest:

LDFLAGS=`trylink "libpci (from pciutils)" "${pc_LDFLAGS}" "-lpci -lz" "-L/usr/local/lib -lpci -lz" "-framework IOKit -framework DirectIO -lpci -lz" "-framework IOKit -framework DirectIO -L/usr/local/lib -lpci -lz"` || {

The entries without frameworks should fail on Darwin but build OK on
systems with libpci in /usr/local, right?

You could also take out the very last argument if you want.


//Peter

Patch

Index: inteltool-r4620/inteltool.h
===================================================================
--- inteltool-r4620/inteltool.h
+++ inteltool-r4620/inteltool.h
@@ -18,12 +18,13 @@ 
  */
 
 #include <stdint.h>
-#ifndef DARWIN
+
+#if defined(__GLIBC__)
 #include <sys/io.h>
-#else
-/* DirectIO is available here:
- * http://www.coresystems.de/en/directio
- */
+#endif
+#if (defined(__MACH__) && defined(__APPLE__))
+/* DirectIO is available here: http://www.coresystems.de/en/directio */
+#define __DARWIN__
 #include <DirectIO/darwinio.h>
 #endif
 #include <pci/pci.h>
@@ -55,7 +56,7 @@ 
 
 #define ARRAY_SIZE(a) ((int)(sizeof(a) / sizeof((a)[0])))
 
-#ifndef DARWIN
+#ifndef __DARWIN__
 typedef struct { uint32_t hi, lo; } msr_t;
 #endif
 typedef struct { uint16_t addr; int size; char *name; } io_register_t;
Index: inteltool-r4620/cpu.c
===================================================================
--- inteltool-r4620/cpu.c
+++ inteltool-r4620/cpu.c
@@ -32,7 +32,7 @@ 
 {
 	unsigned int ret;
 	unsigned int dummy2, dummy3, dummy4;
-#if DARWIN
+#ifdef __DARWIN__
 	asm volatile ( 
 		"pushl %%ebx	\n"
 		"cpuid		\n"
@@ -53,7 +53,7 @@ 
 	return ret;
 }
 
-#ifndef DARWIN
+#ifndef __DARWIN__
 int msr_readerror = 0;
 
 msr_t rdmsr(int addr)
@@ -288,7 +288,7 @@ 
 		return -1;
 	}
 
-#ifndef DARWIN
+#ifndef __DARWIN__
 	fd_msr = open("/dev/cpu/0/msr", O_RDWR);
 	if (fd_msr < 0) {
 		perror("Error while opening /dev/cpu/0/msr");
@@ -309,7 +309,7 @@ 
 	close(fd_msr);
 
 	for (core = 0; core < 8; core++) {
-#ifndef DARWIN
+#ifndef __DARWIN__
 		char msrfilename[64];
 		memset(msrfilename, 0, 64);
 		sprintf(msrfilename, "/dev/cpu/%d/msr", core);
@@ -330,12 +330,12 @@ 
 			       cpu->per_core_msrs[i].number, msr.hi, msr.lo,
 			       cpu->per_core_msrs[i].name);
 		}
-#ifndef DARWIN
+#ifndef __DARWIN__
 		close(fd_msr);
 #endif
 	}
 
-#ifndef DARWIN
+#ifndef __DARWIN__
 	if (msr_readerror)
 		printf("\n(*) Some MSRs could not be read. The marked values are unreliable.\n");
 #endif
Index: inteltool-r4620/inteltool.c
===================================================================
--- inteltool-r4620/inteltool.c
+++ inteltool-r4620/inteltool.c
@@ -50,7 +50,7 @@ 
 	{ PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ICH, "ICH" }
 };
 
-#ifndef DARWIN
+#ifndef __DARWIN__
 static int fd_mem;
 
 void *map_physical(unsigned long phys_addr, int len)
@@ -192,7 +192,7 @@ 
 		exit(1);
 	}
 
-#ifndef DARWIN
+#ifndef __DARWIN__
 	if ((fd_mem = open("/dev/mem", O_RDWR)) < 0) {
 		perror("Can not open /dev/mem");
 		exit(1);
Index: inteltool-r4620/Makefile
===================================================================
--- inteltool-r4620/Makefile
+++ inteltool-r4620/Makefile
@@ -31,9 +31,7 @@ 
 
 OS_ARCH	= $(shell uname)
 ifeq ($(OS_ARCH), Darwin)
-CFLAGS += -DDARWIN -I/usr/local/include 
-LDFLAGS = -framework IOKit -framework DirectIO -L/usr/local/lib -lpci -lz
-# OBJS += darwinio.o
+LDFLAGS = -framework IOKit -framework DirectIO -lpci -lz
 endif
 
 all: pciutils dep $(PROGRAM)