Page MenuHomeFreeBSD

loader.efi environment related cleanups
ClosedPublic

Authored by tsoome on Jan 12 2017, 11:21 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Apr 12, 5:29 PM
Unknown Object (File)
Fri, Apr 12, 2:19 PM
Unknown Object (File)
Fri, Apr 12, 7:11 AM
Unknown Object (File)
Fri, Apr 12, 7:11 AM
Unknown Object (File)
Jan 18 2024, 3:32 AM
Unknown Object (File)
Dec 30 2023, 10:48 PM
Unknown Object (File)
Dec 23 2023, 4:00 AM
Unknown Object (File)
Dec 16 2023, 11:11 AM
Subscribers

Details

Summary

Since we have dedicated libefi/env.c file for variable support, the following
changes are done:

Simple cstyle changes in env.c
Moved efi variable related commands from loader/main.c to libefi/env.c
Did create function to set "efi-version" environment variable in env.c.

This function does serve two purposes: for first a  small clean up of the
loader main(), and for second, it does replace the otherwise unused
efi_variable_support hack.

A bit of cleanup of ficl backend functions. The TEST_MAIN has no meaning,
and removed few memory leaks.

The forth code is updated to use "efi-version" variable, instead of ficl
environment check.

Test Plan

Build and boot test. I have not yet done buildworld.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 6969
Build 7154: arc lint + arc unit

Event Timeline

tsoome retitled this revision from to loader.efi environment related cleanups.
tsoome updated this object.
tsoome edited the test plan for this revision. (Show Details)
tsoome added reviewers: imp, allanjude.
imp edited edge metadata.

OK. The code motion is fine. However the UTF conversion is wrong. It mostly works for ASCII / English, but fails for more complicated things. libefivar has the right routines, and it would be good to find a way for libstand to share that with libefivar and use the libstand routines. That can be done in a followup commit though.

sys/boot/efi/libefi/env.c
109

Does %S due strictly wide characters, or does it do UTF-16 encoded strings? That was one of my cleanup mental notes.

122–125

I'm not so sure about this heuristic.... I wrote it, but I'm not longer sure it's a reasonable thing. Comments?

sys/boot/efi/libefi/wchar.c
3

Yea, you didn't write these routines, I did. However, they are wrong. I forgot to go back and do them correctly based on getting libefivar working.

40

This is wrong. It was wrong before you started though. See what libefivar does, since that's right. Likewise for cpy8to16 and cpy16to8. It's only mostly right :(

This revision is now accepted and ready to land.Jan 13 2017, 5:10 AM
sys/boot/efi/libefi/env.c
109

%S is just sending CHAR16 string to console as is, assuming the SimpleTextOutput protocol is dealing with it.

122–125

We actually need to check the names of the variables and interpret based on the description in UEFI specification. For example, the some of them are to be converted to devicepath strings etc.

sys/boot/efi/libefi/wchar.c
3

Oh yes, I was just moving the functions and did not think too much:D can you please provide proper line for it, so I'll update before going for commit.

tsoome edited edge metadata.

ficlEfiSetenv() already does initialize the string variables

This revision now requires review to proceed.Jan 13 2017, 1:54 PM
tsoome edited edge metadata.

libefi/Makefile needs to build efi.c even without BOOT_FORTH

smh requested changes to this revision.Jan 23 2017, 9:51 AM
smh added a reviewer: smh.
smh added a subscriber: smh.
smh added inline comments.
sys/boot/efi/libefi/env.c
156

Alignment issues or just reviews.?

176

unneeded warp?

209

missing \n

250

Can't this check be done earlier in a why that makes it clear how may args are really supported?

272

switch might make this clearer to read as it clearly associates the EFI_SUCCESS check below with this request.

274

Do we not have a realloc?

This revision now requires changes to proceed.Jan 23 2017, 9:51 AM
tsoome edited edge metadata.
tsoome marked 2 inline comments as done.

use realloc(), add missing \n.

sys/boot/efi/libefi/env.c
156

This is phabricator issue with dealing with tabs.

176

&uuid_status does not fit into 80 chars line.

250

I'm quite sure it can be, but I would leave it as is for now, as the main purpose for this change is to move the code from efi/loader/main.c. Also there are some additional changes related to variables, so we have to return to this code anyhow, and then it is better time to review the logic here.

272

This re-allocation also means we need to go for next loop iteration to repeat the GetNextVariableName() with larger buffer, the switch() will complicate the usage of "continue" there.

274

we do indeed.

sys/boot/efi/libefi/wchar.c
40

Yep, agree, but I do not want to touch it in this update, leaving those for followups.

tsoome edited edge metadata.

realloc() need to use temporary variable for result check, or
we will get memory leak.

imp edited edge metadata.
This revision was automatically updated to reflect the committed changes.