[systemd-devel] [PATCH v2] Add reboot to EFI support

[Lennart Poettering]

On Thu, 02.04.15 15:51, Jan Janssen (medhefgo at web.de) wrote:

> Hi,
> 
> On 2015-04-02 11:34, Lennart Poettering wrote:
> >On Thu, 26.03.15 16:09, Jan Janssen (medhefgo at web.de) wrote:
> >
> >Heya,
> >
> >Hmm, so we already support passing special reboot() parameters, and
> >this is done by manipulating a file in /run, without introducing any
> >new targets. To me it appears that boot-into-firmware-setup is
> >something hat should be handled the same way, i.e. as a special
> >parameter for the *normal* poweroff path, instead of introducing a new
> >poweroff path for it. Of course, instead of manipulating /run for this
> >we should directly manipulate the respective EFI variable.
> >
> >I hence think this should be a new switch --firmware-setup or so to
> >systemctl. Of course, that sounds awfully specific and I don't really
> >like too much adding a new switch just for this flag, but it's the
> >least best option I see.
> 
> That was my original approach. Kay said "--firmware" sounded weird.

Yeah, he initially said that but 6 hours later suggested suggested
"--firmware" again.

http://lists.freedesktop.org/archives/systemd-devel/2015-March/029589.html

That said, I think it really should be "--firmware-setup" and not
"--firmware", since otherwise one might think this simply uses a
different way to reboot, using some firmware call or so.

Also note that I figure "systemctl poweroff --firmware-setup" might be
a valid operation too, not just "systemctl reboot --firmware-setup". I
mean, it's not too useful, but probably valid...

> >The existing boot argument is passed as-is to the kernel, hence
> >giving the argument "efi" a special meaning would mean once couldn't
> >pass that parameter anymore to the kernel.
> 
> I had the same reservation, but it was suggested to ignore this and just
> piggyback on this instead.

Nah, I am very much against that, that'd be hackish.

> >I would strongly prefer naming the switch something like "firmware"
> >instead of "EFI", since we shouldn't encode the technology here, but
> >the generic term. Also, this should mention that this is about the
> >setup tool of the firmware, since EFI is available all the time, and
> >this is really about the *setup* tool of the firmware...
> 
> Someone suggested "firmware" is too generic, so I switched to EFI. Would be
> nice if people made up their mind on that one...

Hmm, can't see this on the ML.

The logic is independent of any particular firmware technology, and I
am pretty sure we should not encode that this is about EFI in the
name. I mean, in the thread it was suggested that Nexus devices
support a similar reboot scheme, without being EFI. Maybe one day we
want to hook that up too under the same switch?

> >I think ultimately we need to expose this even in GNOME, similar to
> >the way Window exposes this. To cover that we should probably add a
> >bus API to logind in some form to manipulate the EFI var in question,
> >and "systemctl reboot --firmware-setup" would use that. (And yes, a
> >similar bus API for specifying the generic reboot parameter probably
> >should exist alongside it).
> 
> Äääähm... this is exactly what this patch does, adding CanRebootToEfi()
> and a RebootToEfi() functions. What did I miss?
> 
> Unless you mean changing those into a pair of properties to just set the
> indication and then the bus client would have to manually trigger
> Reboot()?

Yes, precisely.

Though, it should be a read-only property, plus a call to change it,
plus a call that tells you whether it can be change it. It should not
be a writable property, since we need to do polkit when the value is
changed, and property changes should not really require interactive
auth.

Hence:

- A boolean property RebootToFirmware
- A method call SetRebootToFirmware() taking a boolean
- A method call CanRebootToFirmware() returning a string, "yes", "no", "challenge"

> In fact, that's what I kind of got in my mind after sending this patch. It
> would also work nicely with a separate RebootArguments property without the
> hassle of introducing more complex logic into the target related functions
> in logind. My original approach was adding a RebootWithArguments function,
> but my brain cannot get the code to look nicely. But making them into
> properties and requiring the client to issue a Reboot themselves would be a
> neat way around that.

Yeah, a RebootArgument bus interface would be useful too, much like
the above:

- A string property RebootArgument
- A method call SetRebootArgument() taking a string

But probably no CanRebootArgument since this is not a discoverable feature...

> >Of course the bus API should also support a CanFirmwareSetup() call or
> >so, that reports whether the logic is available at all.
> 
> As I said, this patch adds this. Though, it would be nice if some consensus
> would come about whether to call this firmware or EFI. I think being
> specific is probably nicer. Unless this were to return a string indicating
> what kind of firmware setup is supported (if ever any others would come
> about in the future), returning "efi" for EFI systems.

Whether EFI is used or something else is really an implementation
detail. It should not be visible in the UI or bus API.

Lennart

-- 
Lennart Poettering, Red Hat