Evgeny Zinoviev has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38349 )
Change subject: util/pmh7tool: support OpenBSD ......................................................................
util/pmh7tool: support OpenBSD
Change-Id: I8aeee4009fb38204b1bdbe6712d759ab17b2cbd9 Signed-off-by: Evgeny Zinoviev me@ch1p.io --- M util/pmh7tool/Makefile M util/pmh7tool/pmh7tool.c 2 files changed, 33 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/38349/1
diff --git a/util/pmh7tool/Makefile b/util/pmh7tool/Makefile index 034ed40..0df7984 100644 --- a/util/pmh7tool/Makefile +++ b/util/pmh7tool/Makefile @@ -19,6 +19,10 @@ INSTALL = /usr/bin/env install PREFIX = /usr/local
+ifeq ($(shell uname -s 2>/dev/null), OpenBSD) +LDFLAGS = -lamd64 +endif + all: $(PROGRAM)
$(PROGRAM): pmh7tool.o diff --git a/util/pmh7tool/pmh7tool.c b/util/pmh7tool/pmh7tool.c index f03d97e..b268f9b 100644 --- a/util/pmh7tool/pmh7tool.c +++ b/util/pmh7tool/pmh7tool.c @@ -16,13 +16,36 @@ #include <stdio.h> #include <stdlib.h> #include <getopt.h> +#if !defined __OpenBSD__ #include <sys/io.h> +#endif #include <errno.h> #include <string.h> #include <unistd.h> #include <stdint.h> #include "pmh7tool.h"
+#if defined __OpenBSD__ +#include <machine/sysarch.h> +static uint8_t inb(unsigned port) +{ + uint8_t data; + __asm volatile("inb %w1,%0" : "=a" (data) : "d" (port)); + return data; +} +static __inline void outb(uint8_t data, unsigned port) +{ + __asm volatile("outb %0,%w1" : : "a" (data), "d" (port)); +} + +#if defined __i386__ +#define iopl i386_iopl +#else +#define iopl amd64_iopl +#endif + +#endif + uint8_t pmh7_register_read(uint16_t reg) { outb(reg & 0xff, EC_LENOVO_PMH7_ADDR_L); @@ -85,7 +108,7 @@ int main(int argc, char *argv[]) { enum action act = HELP; - int opt, option_index = 0; + int i, opt, option_index = 0; long input_addr = 0, input_data = 0;
static struct option long_options[] = { @@ -185,15 +208,18 @@ fprintf(stderr, "You must be root.\n"); exit(1); } - +#if !defined __OpenBSD__ if (ioperm(EC_LENOVO_PMH7_BASE, 0x10, 1)) { +#else + if (iopl(3)) { +#endif fprintf(stderr, "ioperm: %s\n", strerror(errno)); exit(1); }
switch (act) { case DUMP: - for (int i = 0; i < 0x200; i++) { + for (i = 0; i < 0x200; i++) { if ((i % 0x10) == 0) { if (i != 0) printf("\n");
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38349 )
Change subject: util/pmh7tool: support OpenBSD ......................................................................
Patch Set 1:
(7 comments)
https://review.coreboot.org/c/coreboot/+/38349/1/util/pmh7tool/pmh7tool.c File util/pmh7tool/pmh7tool.c:
https://review.coreboot.org/c/coreboot/+/38349/1/util/pmh7tool/pmh7tool.c@30 PS1, Line 30: static uint8_t inb(unsigned port) Prefer 'unsigned int' to bare use of 'unsigned'
https://review.coreboot.org/c/coreboot/+/38349/1/util/pmh7tool/pmh7tool.c@33 PS1, Line 33: __asm volatile("inb %w1,%0" : "=a" (data) : "d" (port)); Prefer using '"%s...", __func__' to using 'inb', this function's name, in a string
https://review.coreboot.org/c/coreboot/+/38349/1/util/pmh7tool/pmh7tool.c@36 PS1, Line 36: static __inline void outb(uint8_t data, unsigned port) Prefer 'unsigned int' to bare use of 'unsigned'
https://review.coreboot.org/c/coreboot/+/38349/1/util/pmh7tool/pmh7tool.c@36 PS1, Line 36: static __inline void outb(uint8_t data, unsigned port) plain inline is preferred over __inline
https://review.coreboot.org/c/coreboot/+/38349/1/util/pmh7tool/pmh7tool.c@38 PS1, Line 38: __asm volatile("outb %0,%w1" : : "a" (data), "d" (port)); Prefer using '"%s...", __func__' to using 'outb', this function's name, in a string
https://review.coreboot.org/c/coreboot/+/38349/1/util/pmh7tool/pmh7tool.c@21... PS1, Line 214: if (iopl(3)) { please, no spaces at the start of a line
https://review.coreboot.org/c/coreboot/+/38349/1/util/pmh7tool/pmh7tool.c@21... PS1, Line 214: if (iopl(3)) { suspect code indent for conditional statements (4, 16)
Idwer Vollering has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38349 )
Change subject: util/pmh7tool: support OpenBSD ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38349/1/util/pmh7tool/Makefile File util/pmh7tool/Makefile:
https://review.coreboot.org/c/coreboot/+/38349/1/util/pmh7tool/Makefile@22 PS1, Line 22: ifeq ($(shell uname -s 2>/dev/null), OpenBSD) The redirection after 'uname -s', is that required or is it a security consideration?
Idwer Vollering has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38349 )
Change subject: util/pmh7tool: support OpenBSD ......................................................................
Patch Set 1:
ping
Evgeny Zinoviev has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38349 )
Change subject: util/pmh7tool: support OpenBSD ......................................................................
Patch Set 1:
Patch Set 1:
ping
will address comments and fix soon.
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38349
to look at the new patch set (#2).
Change subject: util/pmh7tool: Support OpenBSD ......................................................................
util/pmh7tool: Support OpenBSD
Change-Id: I8aeee4009fb38204b1bdbe6712d759ab17b2cbd9 Signed-off-by: Evgeny Zinoviev me@ch1p.io --- M util/pmh7tool/Makefile M util/pmh7tool/pmh7tool.c 2 files changed, 33 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/49/38349/2
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38349 )
Change subject: util/pmh7tool: Support OpenBSD ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38349/2/util/pmh7tool/pmh7tool.c File util/pmh7tool/pmh7tool.c:
https://review.coreboot.org/c/coreboot/+/38349/2/util/pmh7tool/pmh7tool.c@33 PS2, Line 33: __asm volatile("inb %w1,%0" : "=a" (data) : "d" (port)); Prefer using '"%s...", __func__' to using 'inb', this function's name, in a string
https://review.coreboot.org/c/coreboot/+/38349/2/util/pmh7tool/pmh7tool.c@38 PS2, Line 38: __asm volatile("outb %0,%w1" : : "a" (data), "d" (port)); Prefer using '"%s...", __func__' to using 'outb', this function's name, in a string
Evgeny Zinoviev has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38349 )
Change subject: util/pmh7tool: Support OpenBSD ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38349/1/util/pmh7tool/Makefile File util/pmh7tool/Makefile:
https://review.coreboot.org/c/coreboot/+/38349/1/util/pmh7tool/Makefile@22 PS1, Line 22: ifeq ($(shell uname -s 2>/dev/null), OpenBSD)
The redirection after 'uname -s', is that required or is it a security consideration?
It was just used in other utils (nvramtool, ectool) in similar cases. I don't see any difference when it's removed.
https://review.coreboot.org/c/coreboot/+/38349/2/util/pmh7tool/pmh7tool.c File util/pmh7tool/pmh7tool.c:
https://review.coreboot.org/c/coreboot/+/38349/2/util/pmh7tool/pmh7tool.c@33 PS2, Line 33: __asm volatile("inb %w1,%0" : "=a" (data) : "d" (port));
Prefer using '"%s... […]
I'm not sure how to fix these or should these be fixed at all.
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38349 )
Change subject: util/pmh7tool: Support OpenBSD ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/38349/2/util/pmh7tool/pmh7tool.c File util/pmh7tool/pmh7tool.c:
https://review.coreboot.org/c/coreboot/+/38349/2/util/pmh7tool/pmh7tool.c@19 PS2, Line 19: #if !defined __OpenBSD__ we generally wrap the term we're looking for: #if !defined(__OpenBSD__)
(also in a few more places)
https://review.coreboot.org/c/coreboot/+/38349/2/util/pmh7tool/pmh7tool.c@33 PS2, Line 33: __asm volatile("inb %w1,%0" : "=a" (data) : "d" (port));
I'm not sure how to fix these or should these be fixed at all.
No need to do anything about it. A false positive by our tooling.
https://review.coreboot.org/c/coreboot/+/38349/2/util/pmh7tool/pmh7tool.c@21... PS2, Line 211: #if !defined __OpenBSD__ I'd consider doing #ifdef __OpenBSD__ iopl #else ioperm #endif
because I suspect that other BSDs have the same issue, and IMHO it's slightly more readable without tons of negations.
Martin L Roth has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/38349?usp=email )
Change subject: util/pmh7tool: Support OpenBSD ......................................................................
Abandoned
This patch has not been touched in over 12 months. Anyone who wants to take over work on this patch, please feel free to restore it and do any work needed to get it merged. If you create a new patch based on this work, please credit the original author.
-
build bot (Jenkins) (Code Review) -
Evgeny Zinoviev (Code Review)
-
Idwer Vollering (Code Review)
-
Martin L Roth (Code Review)
-
Patrick Georgi (Code Review)