Re: [dmidecode] [patch] UEFI support on FreeBSD

[Jean Delvare]

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