[PATCH] misc coreboot utility patches

[Stefan Reinauer]

See attachments

[Peter Stuge]

Stefan Reinauer wrote:
> port msrtool to darwin.
>
> Signed-off-by: Stefan Reinauer

With comments addressed, go for it!

Acked-by: Peter Stuge
> +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]

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]

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