svn commit: r307326 - head/sys/boot/efi/loader

[John Baldwin]

On Tuesday, October 18, 2016 11:44:52 PM Warner Losh wrote:
> On Mon, Oct 17, 2016 at 11:40 AM, John Baldwin <jhb at freebsd.org> wrote:
> > On Friday, October 14, 2016 12:25:54 PM Warner Losh wrote:
> >> On Oct 14, 2016 11:55 AM, "Doug Ambrisko" <ambrisko at ambrisko.com> wrote:
> >> >
> >> > On Fri, Oct 14, 2016 at 10:33:15AM -0700, Ravi Pokala wrote:
> >> > | -----Original Message-----
> >> > | > From: <owner-src-committers at freebsd.org> on behalf of Doug Ambrisko <
> >> ambrisko at ambrisko.com>
> >> > | > Date: 2016-10-14, Friday at 10:27
> >> > | > To: Warner Losh <imp at bsdimp.com>
> >> > | > Cc: Doug Ambrisko <ambrisko at freebsd.org>, src-committers <
> >> src-committers at freebsd.org>, "svn-src-all at freebsd.org" <
> >> svn-src-all at freebsd.org>, "svn-src-head at freebsd.org" <
> >> svn-src-head at freebsd.org>
> >> > | > Subject: Re: svn commit: r307326 - head/sys/boot/efi/loader
> >> > | >
> >> > | > On Fri, Oct 14, 2016 at 11:16:02AM -0600, Warner Losh wrote:
> >> > | > | Love the functionality, but don't like using the 'hint' namespace
> >> for
> >> > | > | this. Can we change it now before too many things depend on it? We
> >> had
> >> > | > | similar issues in ACPI and moved it to the 'acpi' space. Can we move
> >> > | > | this to the 'smbios' space please?
> >> > | > |
> >> > | > | The reason is that 'hint' is special and sometimes filtered out, so
> >> it
> >> > | > | is a poor choice to export data from the boot loader to the kernel.
> >> > | >
> >> > | > The reason I picked hint was it could be put /boot/device.hints
> >> > | > to make it work as well and that it was a hint.  Other standards in
> >> the
> >> > | > future might use other methods.  Looking back over the email I had
> >> > | > with John he had suggested hint.smbios.0.anchor to make this look
> >> > | > different.  This code had been hanging around for so long I forgot
> >> > | > about that and we were using hint.smbios.0.mem in our shipping code
> >> base.
> >> > | >
> >> > | > However, I hope that nothing would use this except for smbios(4) and
> >> > | > for people to make smbios(4) useful for this info.
> >> > |
> >> > | Doug's looking at me when he says that. :-)
> >> > |
> >> > | We talked about this last night at BAFUG; right now, even if smbios(4)
> >> > | is able to find the SMBIOS info -- it currently only looks at the
> >> > | aforementioned 0xf0000 - 0xfffff range, so it can't find it on UEFI --
> >> > | smbios(4) doesn't actually provide any interface for that information.
> >> > | Doug and I have talked about making smbios(4) useful, by parsing the
> >> > | data and providing KPIs and APIs, for years now; I think I'll *finally*
> >> > | have the time and motivation to do so "soon".
> >> >
> >> > I've actually talked to a few people.  However, your the first to
> >> > step up.  This needs to be designed and will take some time and
> >> > review.  I would hope that except for smbios(4), nothing else would
> >> > use this kenv but there is nothing to prevent that :-(  I could name
> >> > it super_secret_dont_use.
> >> >
> >> > BTW, to get you started this patch prevents smbios(4) from blowing chunks
> >> > when it gets a anchor in high memory and works on legacy machines.
> >> >
> >> > --- /sys/x86/bios/smbios.c      2013-10-01 14:28:25.000000000 -0700
> >> > +++ ./smbios.c  2016-04-11 11:58:03.234969000 -0700
> >> > @@ -38,6 +38,7 @@ __FBSDID("$FreeBSD: release/9.2.0/sys/x8
> >> >  #include <machine/bus.h>
> >> >  #include <machine/resource.h>
> >> >  #include <sys/rman.h>
> >> > +#include <sys/sysctl.h>
> >> >
> >> >  #include <vm/vm.h>
> >> >  #include <vm/vm_param.h>
> >> > @@ -59,7 +60,7 @@ struct smbios_softc {
> >> >  };
> >> >
> >> >  #define        RES2EPS(res)    ((struct smbios_eps
> >> *)rman_get_virtual(res))
> >> > -#define        ADDR2EPS(addr)  ((struct smbios_eps
> >> *)BIOS_PADDRTOVADDR(addr))
> >> > +#define        ADDR2EPS(addr)  ((struct smbios_eps *)PHYS_TO_DMAP(addr))
> >> >
> >> >  static devclass_t      smbios_devclass;
> >> >
> >> > @@ -71,19 +72,26 @@ static int  smbios_modevent (module_t, in
> >> >
> >> >  static int     smbios_cksum    (struct smbios_eps *);
> >> >
> >> > +static unsigned long addr;
> >> > +static SYSCTL_NODE(_hw, OID_AUTO, smbios, CTLFLAG_RD, 0,
> >> > +    "SMBIOS driver parameters");
> >> > +SYSCTL_LONG(_hw_smbios, OID_AUTO, mem, CTLFLAG_RW,
> >> > +        &addr, 0, "");
> >> > +
> >> >  static void
> >> >  smbios_identify (driver_t *driver, device_t parent)
> >> >  {
> >> >         device_t child;
> >> > -       u_int32_t addr;
> >> >         int length;
> >> >         int rid;
> >> >
> >> >         if (!device_is_alive(parent))
> >> >                 return;
> >> >
> >> > -       addr = bios_sigsearch(SMBIOS_START, SMBIOS_SIG, SMBIOS_LEN,
> >> > -                             SMBIOS_STEP, SMBIOS_OFF);
> >> > +       if (resource_long_value("smbios", 0, "mem", &addr) != 0 ||
> >> > +           addr == 0)
> >> > +               addr = bios_sigsearch(SMBIOS_START, SMBIOS_SIG,
> >> SMBIOS_LEN,
> >> > +                                     SMBIOS_STEP, SMBIOS_OFF);
> >> >         if (addr != 0) {
> >> >                 rid = 0;
> >> >                 length = ADDR2EPS(addr)->length;
> >> >
> >> > Note I don't plan to commit this since it doesn't really do much and we
> >> > need a lot more.
> >>
> >> I was planning on exporting all smbios stuff via sysctl.
> >
> > I'm a bit hesitant to do all the type parsing in the kernel vs userland.
> > However, I think having smbios(4) export a /dev/smbios that you can either
> > read() or mmap() to access the table would be very convenient and let you
> > keep the bits to parse the table in userland (and not require root if we
> > allow read-only access to mortals on /dev/foo).
> 
> I'd support allowing this also, but we have them hidden in kenv how.
> Moving them to sysctl is trivial and adds no additional risk to the
> kernel. Having a /dev/smbios that also exports them and having
> userland tools to parse nodes that the loader doesn't export is also a
> good idea, but I don't think that would replace what I have in mind.
> sysctl is super easy to get data from in shell and other scripts.
> /dev/smbios would require another program to parse the blob that's
> exported. I think it would be cool to have that as well and wouldn't
> interfere with what I was planning on doing.

Not everything is as sensible to export via sysctl, though a subset of
things might be.  I have patches to pciconf to let it parse the smbios
table for describing physical slots and which new-bus devices are
associated with physical slots (unfortunately the tables themselves aren't
very reliable in my experience to date), and that sort of thing probably
belongs in userland.  dmidecode could be patched to use /dev/smbios
rather than grovelling around in /dev/mem rather easily.  Having a tool
to parse the table isn't the end of the world.  We do this with other
tables (e.g. the SMAP and EFI memory maps are exported as "raw" sysctl
nodes that the sysctl binary knows how to format, but that is still
done in userland).  acpidump -t, mptable, pirtool, etc. are all done in
userland as well.

-- 
John Baldwin