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

[Jean Delvare]

Hi Alexey,

On Wed, 20 Sep 2017 00:13:57 +0700, Alexey Dokuchaev wrote:
> On Thu, Sep 14, 2017 at 10:51:34AM +0200, Jean Delvare wrote:
> > 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.  
> 
> Be my guest; I'm not trying to interfere with you coding style in any way;
> feel free to reorder #include's to your liking.

Done.

> > > + *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.)  
> 
> Well, again, I wanted to have a working implementation, and if you can
> refactor it to a better shape, I'll happily test the final patch to make
> sure it works on FreeBSD for both BIOS and UEFI cases.

I'll write a separate patch, and post it so you can test it.

> > 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.  
> 
> Right, I've seen this warning, I think.  Do you want me to fix it, or
> rather address it on your own?

I fixed it.

> > 2* I know that sysfs is Linux-specific, but /proc is not. Are you
> > certain that no other operating system implements /proc/efi/systab?  
> 
> Traditionally /proc was solely about processes, and FreeBSD adheres
> to this, being Unix.  Linux, on the other hand, had decided to abuse
> /proc for a number of other, unrelated things, but than again: Linux
> is not strictly a Unix, just a look-alike.
> 
> That said, I do believe that /proc/efi/systab path is Linux-specific
> and won't count on it being present and/or expectedly populated in
> some other operating system.

OK, thanks. Can you please test the following patch and confirm it
still works for you?

---
 dmidecode.c |   31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

--- dmidecode.c.orig    2017-05-23 10:15:36.397386352 +0200
+++ dmidecode.c 2017-09-26 08:42:08.339871831 +0200
@@ -64,6 +64,11 @@
 #include <stdlib.h>
 #include <unistd.h>
 
+#ifdef __FreeBSD__
+#include <errno.h>
+#include <kenv.h>
+#endif
+
 #include "version.h"
 #include "config.h"
 #include "types.h"
@@ -4934,13 +4939,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
@@ -4972,6 +4982,27 @@ static int address_from_efi(off_t *addre
 
        if (ret == EFI_NO_SMBIOS)
                fprintf(stderr, "%s: SMBIOS entry point missing\n", filename);
+#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);
+#else
+       ret = EFI_NOT_FOUND;
+#endif
        return ret;
 }
 

-- 
Jean Delvare
SUSE L3 Support