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

[Warner Losh]

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.

Warner