UEFI Runtime Services environment variable support.

[imp]

Summary

Add efivar(1) to manipulate EFI variables. It uses a similar command
line interface to the Linux program, as well as adding a number of
useful features to make using it in shell scripts easier (since we
don't have a filesystem to fall back on interacting with).

Create libefivar library. This library aims to provide the same API as
the GPL'd version of this library. It implements the common Linux API
for programatically manipulating UEFI environment varibales using the
UEFI Runtime Services the kernel provides. It replaces the old efi
library since it is programmed to a different interface, but retails
the CHAR16 to UTF-8 and vice versa conversion routines. The new name
is to match Linux program's expectations.

Create /dev/efidev to provide an ioctl interface to userland. This
device attaches to the nexus. It supports userland interfaces to UEFI
Runtime Services.

Define efi_rt_avail: Are the UEFI runtime services currently available?

[emaste]

Overall I like it.

A few bikesheddy comments for now; I'll take a real look in the next couple of days or so.

lib/Makefile

243–245 ↗

(On Diff #20951)

Perhaps use the now-preferred SUBDIR+=libefivar style, even if it ends up with a temporary mix of styles?

lib/libefivar/efivar.3

91 ↗

(On Diff #20951)

Just the last one?

94 ↗

(On Diff #20951)

Should we add something along the lines of "which defines the API implemented by this library"?

sys/amd64/conf/GENERIC.efi

1–3 ↗

(On Diff #20951)

Is this file just for testing?

sys/dev/efidev/efidev.c

119 ↗

(On Diff #20951)

feels unusual to me to indent the label

[imp]

lib/libefivar/efivar.3

91 ↗

(On Diff #20951)

Yea, just the last one.
I need to actually document all these, though.

94 ↗

(On Diff #20951)

Actually, this bug should be removed enitrely.
It was based on a bogus early understanding of what the Linux library did.

sys/amd64/conf/GENERIC.efi

1–3 ↗

(On Diff #20951)

Yes. My git fu is such that omitting it is a pain, so I'll omit it last.

sys/dev/efidev/efidev.c

119 ↗

(On Diff #20951)

It's the style I've used since the 80's...
However, looks like 99+% of rest of the kernel doesn't indent at all...
And about 1/4 of that <1% is my doing.

[martymac]

Globally looks good to me (I've added a few comments), thanks for that work :)

lib/libefivar/Makefile

1 ↗

(On Diff #20951)

It seems that copyright is quite old, shouldn't it be updated ?

lib/libefivar/efivar.3

32 ↗

(On Diff #20951)

s/Suppoert/Support/

lib/libefivar/efivar.c

217 ↗

(On Diff #20951)

Maybe check for arguments validity here : name and guid should be != NULL
(that kind of safety check should also probably be added in other functions to avoid dereferencing NULL pointers)

222 ↗

(On Diff #20951)

Maybe check for a malloc() failure here ?

229 ↗

(On Diff #20951)

For readability and as we are preparing var here, use *(var.name) instead ?

235 ↗

(On Diff #20951)

Warning : *guid can still be NULL here

243 ↗

(On Diff #20951)

Shouldn't buflen be updated just before calling realloc() to use the size returned from ioctl() ?

lib/libefivar/efivar.h

37 ↗

(On Diff #20951)

s/Shoud/Should/ :)

lib/libefivar/libefivar.c

73 ↗

(On Diff #20951)

For readability, int freeit;
then set it to 1 just after malloc() ?

122 ↗

(On Diff #20951)

Same as above

126 ↗

(On Diff #20951)

Check for malloc() failure ?

sys/dev/efidev/efidev.c

139 ↗

(On Diff #20951)

s/lenght/length/

172 ↗

(On Diff #20951)

Same test as above, use :
if (ev->datasize > 0)
for consistency ?

234 ↗

(On Diff #20951)

Is 0700 really needed ? 0600 ?

usr.sbin/efivar/efivar.8

59 ↗

(On Diff #20951)

s/part/parts/

[kib]

I only looked at the kernel bits.

sys/dev/efidev/efidev.c

39 ↗

(On Diff #20951)

I recommend going with sys/efi.h instead of machine/efi.h. I put the access KPI definitions into machine/efi.h for now only because only amd64 was handled. If any other arch grows the KPI support, most likely the declarations would migrate to sys/, and sys/efi.h includes machine/efi.h anyway.

50 ↗

(On Diff #20951)

I do not think there is any use of the lint keywords in the new code. I will remove lint in near future, I hope.

87 ↗

(On Diff #20951)

This is impossible.

120 ↗

(On Diff #20951)

The checks for NULL are not needed.

203 ↗

(On Diff #20951)

Opening brace should be on the previous line.

235 ↗

(On Diff #20951)

Why not /dev/efi or /dev/efirt ? /dev/efidev is arguably redundant.

250 ↗

(On Diff #20951)

I do not see the newbus attachment for the software-only construct useful. Do you have any other plans for the efi device_t ? IMO it is just useless code: you may fail module load if efirt unable to provide runtime access.

sys/i386/include/efi.h

35 ↗

(On Diff #20951)

This should be attribute((ms_abi)), same as on amd64.

[imp]

lib/libefivar/Makefile

1 ↗

(On Diff #20951)

Yea, all I copied was the license, so this is bogus and should be updated.

lib/libefivar/efivar.c

217 ↗

(On Diff #20951)

I'm generally lothe to do that. You deserve a core dump. I'll see what Linux does, though, to be compatible.

235 ↗

(On Diff #20951)

how?

243 ↗

(On Diff #20951)

Yes, that's a bug.

sys/dev/efidev/efidev.c

39 ↗

(On Diff #20951)

Agreed.

50 ↗

(On Diff #20951)

Yea, I can delete it. It was copied from something else.

120 ↗

(On Diff #20951)

Derp! You're right. Habit.

172 ↗

(On Diff #20951)

No, it's real. You pass in datasize = 0 when you want to delete the variable.

234 ↗

(On Diff #20951)

Yea.

235 ↗

(On Diff #20951)

Yea, /dev/efi is better.

250 ↗

(On Diff #20951)

I'm not sure which way to go. It's useful to have something in device_t to make it more visible. On the other hand, this is currently the only device exported for EFI. It's been quite useful to have the two in separate modules so I can reload this code w/o rebooting.
I'll consider the request, but here our philosophies may differ.

sys/i386/include/efi.h

35 ↗

(On Diff #20951)

Yea, but clang doesn't like it.

[imp]

new version coming soon.

lib/libefivar/efivar.c

217 ↗

(On Diff #20951)

It's easy enough to add simple checks, so I'll do that, but there are still programming errors that will lead to a core dump.

222 ↗

(On Diff #20951)

Sure.

229 ↗

(On Diff #20951)

sure.

235 ↗

(On Diff #20951)

ah, only if there's a programming error.

243 ↗

(On Diff #20951)

In fact it's two bugs.

[imp]

Address most of the comments from the review.
Still contemplating the newbus thing.

[imp]

lib/Makefile

243–245 ↗

(On Diff #20951)

I tried, but my inner OCD hobgloblin of consistency just wouldn't allow me.

[martymac]

lib/libefivar/efivar.c

235 ↗

(On Diff #20951)

yep, if guid points to a NULL pointer

sys/dev/efidev/efidev.c

172 ↗

(On Diff #20951)

Oops, right

[imp]

lib/libefivar/efivar.c

235 ↗

(On Diff #20951)

That's a programming error. It must only point to a NULL pointer when first called (when name does the same). After that, it cannot point there. It's one of many programming errors that one can make with the interface. The caller is responsible for proper care, just like strcpy().

[martymac]

lib/libefivar/efivar.c

235 ↗

(On Diff #20951)

OK, right.

[kib]

In D8128#168541, @imp wrote:

Still contemplating the newbus thing.

BTW, on i386, FPU is attached by newbus, and this causes issues. Most important is that the attachment happens rather late, and the FPU exceptions handlers are installed late. When i386 does BIOS calls during early stages of the boot in vm86 mode, any FPU errors cause spurious interrupt panics. This does not happen often, but some machines were affected.

I wonder if early RT service might be needed, e.g. if we implement EFI time of the day as an alternative to CMOS RTC, or start using EFI reset. Then the late newbus attachment would be an issue.

[imp]

In D8128#168755, @kib wrote:

In D8128#168541, @imp wrote:

Still contemplating the newbus thing.

BTW, on i386, FPU is attached by newbus, and this causes issues. Most important is that the attachment happens rather late, and the FPU exceptions handlers are installed late. When i386 does BIOS calls during early stages of the boot in vm86 mode, any FPU errors cause spurious interrupt panics. This does not happen often, but some machines were affected.

I wonder if early RT service might be needed, e.g. if we implement EFI time of the day as an alternative to CMOS RTC, or start using EFI reset. Then the late newbus attachment would be an issue.

It might, but since all this is a userland /dev device, I don't see how that might bite us. The FPU issue is good to know.

[imp]

In D8128#168756, @imp wrote:

In D8128#168755, @kib wrote:

In D8128#168541, @imp wrote:

Still contemplating the newbus thing.

BTW, on i386, FPU is attached by newbus, and this causes issues. Most important is that the attachment happens rather late, and the FPU exceptions handlers are installed late. When i386 does BIOS calls during early stages of the boot in vm86 mode, any FPU errors cause spurious interrupt panics. This does not happen often, but some machines were affected.

I wonder if early RT service might be needed, e.g. if we implement EFI time of the day as an alternative to CMOS RTC, or start using EFI reset. Then the late newbus attachment would be an issue.

It might, but since all this is a userland /dev device, I don't see how that might bite us. The FPU issue is good to know.

I think I'm moving all this code into efirt.c and will go ahead and strip out the newbus bits.

[kib]

In D8128#169384, @imp wrote:

I think I'm moving all this code into efirt.c and will go ahead and strip out the newbus bits.

I suggest it is better to keep efirt.c and efidev.c split. efidev is MI, while efirt is amd64 MD code which must be implemented on other arches. But removing efirt module and compiling efirt.c into your module might be good thing to have, at least until rt services are not used for anything else.

[imp]

In D8128#169396, @kib wrote:

In D8128#169384, @imp wrote:

I think I'm moving all this code into efirt.c and will go ahead and strip out the newbus bits.

I suggest it is better to keep efirt.c and efidev.c split. efidev is MI, while efirt is amd64 MD code which must be implemented on other arches. But removing efirt module and compiling efirt.c into your module might be good thing to have, at least until rt services are not used for anything else.

OK. I'll keep the two files separate, but compile efidev into the efirt module. I'll strip newbus too. I'll repost the review when I've done these changes.

[wblock]

sys/i386/include/efi.h

36 ↗

(On Diff #20995)

Trailing whitespace on this line.

sys/sys/efiio.h

36 ↗

(On Diff #20995)

Trailing whitespace on this line.

42 ↗

(On Diff #20995)

Trailing whitespace on this line.

usr.sbin/efivar/efivar.c

340 ↗

(On Diff #20995)

Trailing whitespace on this line.