| 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 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
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
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)