Hi Alexey, On Mon, 19 Jun 2017 08:00:01 +0600, Alexey Dokuchaev wrote: > Currently, dmidecode(8) does not work on FreeBSD booted in UEFI mode. > Previously it was understandable, since there are no things like Linuxish > /proc/efi/systab or /sys/firmware/efi/systab to read from under FreeBSD. > > However, 7 months ago, ambrisko@ had added support for exposing the SMBIOS > anchor base address via kernel environment: > > https://svnweb.freebsd.org/base?view=revision&revision=307326 > > I've patched dmidecode.c to try to get the address from hint.smbios.0.mem > and fall back to traditional address space scanning. I've tested it both > on EFI (amd64 laptop) and non-EFI (i386 desktop) machines.
Thanks. Here's my review: > --- dmidecode.c.orig 2017-05-23 13:34:14 UTC > +++ dmidecode.c > @@ -58,6 +58,10 @@ > * > https://trustedcomputinggroup.org/pc-client-platform-tpm-profile-ptp-specification/ > */ > > +#ifdef __FreeBSD__ > +#include <errno.h> > +#include <kenv.h> > +#endif I think I'd prefer them after the common ones, and with a separating blank line. > #include <stdio.h> > #include <string.h> > #include <strings.h> > @@ -4934,13 +4938,18 @@ static int legacy_decode(u8 *buf, const > #define EFI_NO_SMBIOS (-2) > static int address_from_efi(off_t *address) > { > +#if defined(__linux__) > FILE *efi_systab; > const char *filename; > char linebuf[64]; > +#elif defined(__FreeBSD__) > + char addrstr[KENV_MVALLEN + 1]; > +#endif > int ret; > > *address = 0; /* Prevent compiler warning */ > > +#if defined(__linux__) > /* > * Linux up to 2.6.6: /proc/efi/systab > * Linux 2.6.7 and up: /sys/firmware/efi/systab > @@ -4973,6 +4982,25 @@ static int address_from_efi(off_t *addre > if (ret == EFI_NO_SMBIOS) > fprintf(stderr, "%s: SMBIOS entry point missing\n", filename); > return ret; > +#elif defined(__FreeBSD__) > + /* > + * On FreeBSD, SMBIOS anchor base address in UEFI mode is exposed > + * via kernel environment: > + * https://svnweb.freebsd.org/base?view=revision&revision=307326 > + */ > + ret = kenv(KENV_GET, "hint.smbios.0.mem", addrstr, sizeof(addrstr)); > + if (ret == -1) { > + if (errno != ENOENT) > + perror("kenv"); > + return EFI_NOT_FOUND; > + } > + > + *address = strtoull(addrstr, NULL, 0); > + if (!(opt.flags & FLAG_QUIET)) > + printf("# SMBIOS entry point at 0x%08llx\n", > + (unsigned long long)*address); > + return 0; This part of the code is the same as in the __linux__ case. It would be nice to avoid duplicating it. Maybe it could be moved to the end of the function, or moved to a separate function altogether (possibly easier.) > +#endif > } > > int main(int argc, char * const argv[]) While it makes sense to restrict each section to its OS, I see two problems with your patch: 1* If building neither on Linux nor on FreeBSD, you get 2 warnings: dmidecode.c: In function ‘address_from_efi’: dmidecode.c:4948:6: warning: unused variable ‘ret’ [-Wunused-variable] int ret; ^ dmidecode.c:5004:1: warning: no return statement in function returning non-void [-Wreturn-type] } ^ This needs to be addressed. 2* I know that sysfs is Linux-specific, but /proc is not. Are you certain that no other operating system implements /proc/efi/systab? Well that can be sorted out later, if needed, not a big problem. Thanks, -- Jean Delvare SUSE L3 Support _______________________________________________ https://lists.nongnu.org/mailman/listinfo/dmidecode-devel
