Improve memory detection in biosmem.c
ClosedPublic

Authored by grembo on Jan 30 2015, 8:07 PM.

Details

Summary

Original motivation is to make the Acer C720 (and potentially other
chromebooks boot. This patch is an extended version of adrian's patch
that can be found here (the changes in biosdisk.c were found to be
unnecessary by kib and are therefore not part of this anymore):

https://people.freebsd.org/~adrian/c720/20150117-seabios-loader-changes-1.diff

  • Add a quirks table based on BIOS vendor / maker / product (NULL matches any), making use of the new smbios_match function
  • pass in an smap+acpi entry size for e820; like what biossmap.c does;
  • don't blindly believe e820 if it doesn't give us enough memory to successfully load things, but ONLY if the machine is listed as BQ_DISTRUST_E820_EXTMEM in the quirks table (in this patch this is only the Acer C720 aka Peppy, since it was the reason to write this patch). There are probably other models out there with these issues, but without data it's probably best to add them one at a time.
  • don't blindly concat cx/dx with e801 - ensure that cx indicates there's no hole and there's a full 15mb of ram before concat'ing;
  • truncate how much RAM we probe for via e801 (which adrian doesn't entirely think is needed)
  • if we use e801, don't use the heap as-is; use the end of bios_extmem instead as if we didn't get anything from E820;
  • add a boot loader command "biosmem" which gives details about how memory was detected
Test Plan

Boot on various machines, escape to boot loader to test biosmem command.
I tested this on the Chromebook that's in the quirks table and could
boot successfully.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
grembo retitled this revision from to Improve memory detection in biosmem.c.Jan 30 2015, 8:07 PM
grembo updated this object.
grembo edited the test plan for this revision. (Show Details)
grembo added reviewers: jhb, adrian.
grembo added a subscriber: kib.
grembo updated this revision to Diff 3604.Feb 3 2015, 12:55 PM

Return actual quirk and not 1.

adrian accepted this revision.Feb 3 2015, 7:02 PM
This revision is now accepted and ready to land.Feb 3 2015, 7:02 PM
grembo requested a review of this revision.Feb 10 2015, 9:53 PM

@adrian acknowledged this revision (thanks), but he also pointed out, that he thinks this needs more testing before committing. I tried to get some attention on freebsd-current, but this isn't a very interesting topic for people to test, so even though it's been only a few days, I don't think this will result in sufficient testing.

Since I don't think this is really in the spirit of a code review (once it's accepted it should be ready to get committed) and since the "dangerous" part of the patch was written by adrian himself, I reopen this request and propose two solutions:

  1. Someone else accepts the revision, so I can go ahead and commit it
  2. I change it, so that adrian's code is only used if a quirk is set. This way I could accomplish my primary target of making this code work on this specific hardware and leave the change for all machines to a second commit. If it then turns out the change doesn't work well, a rollback won't affect my primary goal.

Your input would be appreciated.

jhb added inline comments.Feb 11 2015, 3:44 PM
sys/boot/i386/libi386/biosmem.c
119 ↗(On Diff #3604)

I think you don't need the 32MB check now if you have the quirk check.

If you do, please reformat the comment. It should be a block comment instead of two line comments, but also fix the punctuation / capitalization while you are there and add a blank before the comment block. (I'd probably just drop the 32MB check though.)

171 ↗(On Diff #3604)

Please add a blank line here before the comment.

172 ↗(On Diff #3604)

Please use '%cx' for register names.

241 ↗(On Diff #3604)

Why "smap"? This should probably be 'biosmem' instead. I think it is just used to name the internal structure stored in the linker set, but it would be more consistent to have the name here match the command name (or at least be relevant).

grembo updated this revision to Diff 3736.Feb 11 2015, 6:20 PM

Changes like requested, I will add a few more inline comments.

grembo added inline comments.Feb 11 2015, 6:25 PM
sys/boot/i386/libi386/biosmem.c
122 ↗(On Diff #3736)

I kept the check in here, the idea is that we might want to be a bit bolder in the quirks table in the future (let's say: all coreboot machines), but only use the fallback code, in case the numbers returned aren't good.

Does this make sense to you or do you think that's not a realistic scenario anyway?

176 ↗(On Diff #3736)

I hope I got this right (%cx)

grembo updated this revision to Diff 3783.Feb 15 2015, 8:22 PM

Removed 32MiB check from code like suggested (reverted comment changes).

There might be another compatible way to improve memory detection:
Instead of the explicit quirk BQ_DISTRUST_E820_EXTMEM, introduce one
called BQ_REQ_SUFFICIENT_EXTMEM.

This would only accept extmem if it's >32MBit, otherwise set it to
0 and therfore fall through to the next method.

I might be overcomplicating this though, so if you think the current
solution is ok, I can totally live with that.

Is there anything else I could do to move this forward?

adrian accepted this revision.Feb 22 2015, 4:12 AM

I think it's ready. Thanks for perservering!

This revision is now accepted and ready to land.Feb 22 2015, 4:12 AM
grembo closed this revision.Feb 23 2015, 10:59 PM
grembo updated this revision to Diff 3934.

Closed by commit rS279222 (authored by @grembo).

jhb added a comment.Mar 6 2015, 4:50 PM

Just to say that I think this version was ok.