Page MenuHomeFreeBSD

Split SMBIOS detection into two function calls
ClosedPublic

Authored by grembo on Jan 26 2015, 11:43 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, May 8, 5:33 PM
Unknown Object (File)
Sat, May 4, 2:34 PM
Unknown Object (File)
Sat, May 4, 6:42 AM
Unknown Object (File)
Sat, May 4, 4:57 AM
Unknown Object (File)
Fri, May 3, 4:19 PM
Unknown Object (File)
Fri, May 3, 4:03 PM
Unknown Object (File)
Sat, Apr 20, 6:26 PM
Unknown Object (File)
Sat, Apr 20, 6:26 PM
Subscribers

Details

Summary

To allow bios quirks early in the boot process, specifically
biosmem.c, before malloc is available, this splits SMBIOS
detection into two calls - smbios_detect (which looks for
SMBIOS and populates various data structures) amd smbios_setup,
which parses the SMBIOS table and sets up the kernel environment.

Also provide a function named smbios_match which can be used
to match a bios_vendor, maker and product. Each of the input
variables is optional, so it allows partial matching (e.g. only
look for a specific vendor).

The complete patch showing how this is intended to be used in
a future patch is available here:
http://blog.grem.de/bits/20150124_c720_loader.patch

Explaining some of the changes:

  • Introducing an smbios_attr struct: This already used four different globals, as there were more to come, grouping them in a struct seemed cleaner.
  • smbios_getstring: Before, smbios_setenv would only do this implicitly, so this has been factored out to allow direct strcmp (smbios_setenv makes use of it)
  • Get rid of the ver parameter in smbios_parse_table, as the function is (and was) using globals anyway. Since we're storing ver now, the parameter isn't necessary anymore.
  • smbios_find_struct: Allows finding a specific SMBIOS table structure by type - required to find the BIOS and SYSTEM structures.
  • Split smbios_detetect into smbios_detect and smbios_setup
  • Introduce match functions (not used yet, but this should make it easier to understand the intend of the patch).
  • Change loader to call smbios_detect early and smbios_setup where it used to call smbios_detect.
Test Plan

Build and boot, compare kenv, should essentially be a noop
in terms of features.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

grembo retitled this revision from to Split SMBIOS detection into two function calls.
grembo updated this object.
grembo edited the test plan for this revision. (Show Details)
grembo added reviewers: adrian, jhb.
grembo added a subscriber: kib.

I just have one suggestion which would be to rename the new smbios_detect() to something like 'smbios_probe()' and have smbios_match() and smbios_setup() (renamed back to smbios_detect()) call smbios_probe() if SMBIOS hasn't been probed yet. This will cause smbios to be probed the first time it is needed instead of having to hardcode when it is called in the loader's main. This also means that other places can use smbios_match() without having to worry if smbios is ready or not.

In D1679#4, @jhb wrote:

I just have one suggestion which would be to rename the new smbios_detect() to something like 'smbios_probe()' and have smbios_match() and smbios_setup() (renamed back to smbios_detect()) call smbios_probe() if SMBIOS hasn't been probed yet. This will cause smbios to be probed the first time it is needed instead of having to hardcode when it is called in the loader's main. This also means that other places can use smbios_match() without having to worry if smbios is ready or not.

That was actually my original design (call probe in both functions "on demand"). I then changed this so it gets called explicitly before biosmem.c to avoid the implicit call as a side effect of match in biosmem.c, which might lead to different execution paths and harder to detect problems as a side effect of future maintenance. At the moment (future patch) smbios_match is only called in biosmem.c if e820 doesn't return enough memory. So if somebody changes smbios_detect() in the future to require malloc, it might actually break the bootloader for a subset of machines, even though most machines still will be fine and it'll be harder to track down the issue.

Maybe that's too paranoid?. We could still have a call to smbios_detect() in smbios_match() so it can be used in different places (if this is an actual concern).

In simpler terms:

My concern was that people might change smbios_probe function in an incompatible way and calling it explicitly and unconditionally at boot might be a safeguard against that. Plus it might be easier for people reading the source to understand what's going on.

If you think this is unnecessary, I'm more than happy to change to "probe on demand".

I don't think smbios_probe() will change. The only thing that might change later is the smbios_detect() routine though even that seems very doubtful. I think it is more likely that we find some future need for smbios_match() than that smbios_probe() changes.

grembo edited edge metadata.

Remove changes in loader/main.c, rename detect to probe, setup to
detect and call probe in detect and match. Keep track if we
probed in new probed member in smbios_attr.

jhb edited edge metadata.

This looks good to me, thanks!

This revision is now accepted and ready to land.Jan 30 2015, 3:02 PM
grembo updated this revision to Diff 3549.

Closed by commit rS277949 (authored by @grembo).