I'd like to submit a kernel patch for a new sysctl to use EFI instead of ACPI for poweroff + reboot
byuu at tutanota.com
byuu at tutanota.com
Mon Dec 10 13:06:04 UTC 2018
Thank you, this information helped a lot!
I am not sure where I should submit my patch to ... I'll post it here if that's okay. If it needs to go somewhere else, please provide instructions or if you don't mind ... submit it on my behalf ^^;
This patch has been created against -CURRENT, and has been tested successfully on 12.0-RC3. It allows me to power off my ASRock X399M (mATX) motherboard, and should also work for ASRock X399 (ATX) users who have reported the same problem of ACPI hanging during shutdown. Hopefully it will be useful to other users in the future who experience ACPI shutdown issues as a fallback option.
I relinquish the entire copyright to the FreeBSD project. It's public domain, no credit is necessary.
Some notes:
As requested, I moved IPMI to PRI-2 instead of PRI-1. I then placed EFI shutdown at PRI-1.
In the spirit of DRY, I modified efi_reset_system(void) to efi_reset_system(int type). I did a grep -rn . search on -CURRENT, and absolutely nothing ever uses efi_reset_system(void), nor is it exposed via the /dev/efi ioctl interface. I hope this is okay.
I named the sysctl tunable "hw.efi.poweroff", after the similarly named "hw.acpi." set of tunables, and the use of "poweroff" over "shutdown" in various other tunables. Please feel free to rename the tunable to anything you like.
I think it would be nice if we exposed this tunable to the "sysctl" tool, and allowed modifying it at run-time. I didn't want to complicate the patch, but if you're okay with that, please add that as well. However, if you're worried about people mucking with the value inadvertently, I'm okay if it remains a "secret" /boot/loader.conf option only.
Lastly, I added the needed event handler headers, and added the new EFI_RESET_SHUTDOWN define.
I tried to respect all formatting conventions of the existing code. But as with everything else, please feel free to make any changes you like.
If I can be of any further assistance on this, please let me know. Thank you very much!
diff -ru old/sys/dev/efidev/efirt.c new/sys/dev/efidev/efirt.c
--- old/sys/dev/efidev/efirt.c 2018-12-10 04:32:01.231607000 +0900
+++ new/sys/dev/efidev/efirt.c 2018-12-10 04:45:29.548147000 +0900
@@ -46,6 +46,8 @@
#include <sys/sysctl.h>
#include <sys/systm.h>
#include <sys/vmmeter.h>
+#include <sys/eventhandler.h>
+#include <sys/reboot.h>
#include <machine/fpu.h>
#include <machine/efi.h>
@@ -126,6 +128,24 @@
return (false);
}
+static void
+efi_shutdown_final(void *unused, int howto)
+{
+ int efi_poweroff;
+
+ /*
+ * On some systems, ACPI S5 is missing or does not function properly.
+ * Allow shutdown via EFI Runtime Services instead with a sysctl tunable.
+ */
+ if((howto & RB_POWEROFF) != 0) {
+ efi_poweroff = 0;
+ TUNABLE_INT_FETCH("hw.efi.poweroff", &efi_poweroff);
+ if (efi_poweroff == 1) {
+ efi_reset_system(EFI_RESET_SHUTDOWN);
+ }
+ }
+}
+
static int
efi_init(void)
{
@@ -214,6 +234,13 @@
}
#endif
+ /*
+ * We use SHUTDOWN_PRI_LAST - 1 to trigger after IPMI, but before ACPI.
+ * When not enabled via sysctl tunable, this will fall through to ACPI.
+ */
+ EVENTHANDLER_REGISTER(shutdown_final, efi_shutdown_final, NULL,
+ SHUTDOWN_PRI_LAST - 1);
+
return (0);
}
@@ -411,16 +438,20 @@
}
int
-efi_reset_system(void)
+efi_reset_system(int type)
{
struct efirt_callinfo ec;
+ if (type != EFI_RESET_COLD
+ && type != EFI_RESET_WARM
+ && type != EFI_RESET_SHUTDOWN)
+ return (EINVAL);
if (efi_runtime == NULL)
return (ENXIO);
bzero(&ec, sizeof(ec));
ec.ec_name = "rt_reset";
ec.ec_argcnt = 4;
- ec.ec_arg1 = (uintptr_t)EFI_RESET_WARM;
+ ec.ec_arg1 = (uintptr_t)type;
ec.ec_arg2 = (uintptr_t)0;
ec.ec_arg3 = (uintptr_t)0;
ec.ec_arg4 = (uintptr_t)NULL;
diff -ru old/sys/dev/ipmi/ipmi.c new/sys/dev/ipmi/ipmi.c
--- old/sys/dev/ipmi/ipmi.c 2018-12-10 04:34:07.535522000 +0900
+++ new/sys/dev/ipmi/ipmi.c 2018-12-10 04:36:35.757611000 +0900
@@ -938,14 +938,14 @@
} else if (!on)
(void)ipmi_set_watchdog(sc, 0);
/*
- * Power cycle the system off using IPMI. We use last - 1 since we don't
+ * Power cycle the system off using IPMI. We use last - 2 since we don't
* handle all the other kinds of reboots. We'll let others handle them.
* We only try to do this if the BMC supports the Chassis device.
*/
if (sc->ipmi_dev_support & IPMI_ADS_CHASSIS) {
device_printf(dev, "Establishing power cycle handler\n");
sc->ipmi_power_cycle_tag = EVENTHANDLER_REGISTER(shutdown_final,
- ipmi_power_cycle, sc, SHUTDOWN_PRI_LAST - 1);
+ ipmi_power_cycle, sc, SHUTDOWN_PRI_LAST - 2);
}
}
diff -ru old/sys/sys/efi.h new/sys/sys/efi.h
--- old/sys/sys/efi.h 2018-12-10 04:32:34.850892000 +0900
+++ new/sys/sys/efi.h 2018-12-10 04:35:41.011396000 +0900
@@ -43,7 +43,8 @@
enum efi_reset {
EFI_RESET_COLD,
- EFI_RESET_WARM
+ EFI_RESET_WARM,
+ EFI_RESET_SHUTDOWN
};
typedef uint16_t efi_char;
@@ -184,7 +185,7 @@
int efi_get_table(struct uuid *uuid, void **ptr);
int efi_get_time(struct efi_tm *tm);
int efi_get_time_capabilities(struct efi_tmcap *tmcap);
-int efi_reset_system(void);
+int efi_reset_system(int type);
int efi_set_time(struct efi_tm *tm);
int efi_var_get(uint16_t *name, struct uuid *vendor, uint32_t *attrib,
size_t *datasize, void *data);
Dec 9, 2018, 3:35 AM by cem at freebsd.org:
> Hi,
>
> I think the way you would do this properly is by adding a
> 'EVENTHANDLER_REGISTER(shutdown_final, efi_shutdown_final, ...,
> SHUTDOWN_PRI_LAST - 1);' in efidev/efirt (or '- 2', see below).
> Because the priority number is *lower* than acpi's
> (SHUTDOWN_PRI_LAST), it will be invoked *before* acpi. If an EFI
> shutdown is inappropriate or the particular howto mode is unsupported
> on that system, it can just do return doing nothing; the ACPI
> acpi_shutdown_final handler will be called next.
>
> You can see numerous examples of such handlers in various ARM devices
> which don't have ACPI (grep for
> 'EVENTHANDLER_REGISTER(shutdown_final').
>
> One other relevant example on x86 is ipmi shutdown, which like I just
> suggested for efi shutdown, registers itself as SHUTDOWN_PRI_LAST - 1
> to precede ACPI shutdown. I guess if IPMI is configured, maybe it
> should supersede both EFI and ACPI. So maybe your patch should bump
> the IPMI number to SHUTDOWN_PRI_LAST - 2 and add EFI as - 1.
>
> As hinted above, your efirt_shutdown_final() would take the place of
> acpi_shutdown_final() in your callstack above, called from
> kern_reboot() -> shutdown_final event.
>
> I hope that helps,
> Conrad
> On Sat, Dec 8, 2018 at 9:53 AM <> byuu at tutanota.com <mailto:byuu at tutanota.com>> > wrote:
>
>>
>> Hello, first time poster here, please go easy on me ^-^;
>>
>> I would like to submit a patch for inclusion to the FreeBSD kernel, but want to first see if there is a chance it will be accepted before writing it or not.
>>
>> Currently, /usr/src/sys/dev/efidev/efirt.c contains the efi_reset_system() function, which calls EFI_RESET_WARM. It's exposed in /usr/src/sys/sys/efi.h
>>
>> I would like to add efi_poweroff_system() to efirt.c, which is identical to efi_reset_system(), but with EFI_RESET_SHUTDOWN as arg1, which we have to add as an enum item to efi.h as well (its value is 2, so it would go directly after EFI_RESET_WARM.)
>>
>> (These two functions are wrappers to invoke the EFI Runtime Services function ResetSystem with EfiResetWarm and EfiResetShutdown. FreeBSD's loader.eli uses them to implement its reboot and poweroff commands, for example.)
>>
>> Next, I would like to add a patch to /usr/src/sys/dev/acpica/acpi.c to acpi_shutdown_final to check a new kernel sysctl, not picky about the name, but something like "hw.acpi.efi_shutdown=0", which can be optionally changed to 1 by the users. It could be changed at run-time with no ill effect.
>>
>> When this option is set to 1, and efi_systbl_phys is not NULL (eg we are running on an EFI system), I would like to invoke efi_poweroff_system() instead of AcpiEnterSleepStatePrep(ACPI_STATE_S5)
>>
>> This is well after all services have been halted, all disks have been detached, and all USB devices have been detached. acpi_shutdown_final calls AcpiEnterSleepStatePrep (_PTS), followed by AcpiEnterSleepState (_S5), and that's it. The idea is to reuse all of the shutdown handling we can.
>>
>> If it would be desired, I could do the same for reset events to invoke efi_reset_system().
>>
>> ...
>>
>> The reason I am requesting this, is because I am the owner of a Threadripper 2950X with an ASRock Taichi X399M motherboard, and when I attempt to shutdown my system from FreeBSD 11.2-RELEASE or 12.0-RC3, the system hangs forever and fails to power down. The call frame for this is as follows:
>>
>> kern_psignal(initproc, SIGUSR2);
>> kern_reboot
>> shutdown_final
>> acpi_shutdown_final
>> AcpiEnterSleepStatePre
>> AcpiEvaluateObject
>> AcpiNsEvaluate
>> AcpiPsExecuteMethod
>> AcpiPsParseAml
>> AcpiPsParseLoop //from this point on, it likely varies per DSDT
>> WalkState->AscendingCallback
>> AcpiDsExecEndOp
>> AcpiGbl_OpTypeDispatch[OpType]
>> AcpiExOpcode_1A_0T_0R
>> AcpiExSystemDoSleep
>> AcpiOsSleep
>> pause("acpislp", 300) (eg tsleep)
>>
>> I do not know why, but the call to pause never returns. I have tried over a dozen things to find a fix for this: analyzing my DSDT ASL code via acpidump, disabling SMT + all cores, wrapping pause in a critical section, trying SLEEP instead of pause, trying a spinloop instead of pause, trying to skip the AcpiEnterSleepStatePrep call, building a kernel with options ACPI_DEBUG and trying to selectively disable every device driver and ACPI subsystem possible (including USB) while still allowing me to get to a login prompt, tweaking every sysctl I could find around ACPI, USB, debugging, etc. Nothing helped.
>>
>> Looking into Linux' ACPI support, they have acpi_no_s5, which they claim is used for certain motherboards which lack an _S5 DSDT entry.
>>
>> I believe my proposed patch would be useful both for the case of a missing _S5 entry, as well as for when there are bugs in FreeBSD, or the motherboard DSDT, or in Intel's ASL parser, or in the hardware itself. Obviously, it's most desirable to fix ACPI on the Threadripper + Taichi X399 platform, but a sysctl as I'm proposing would be beneficial for this and future platforms while users wait for a fix.
>>
>> If someone with kernel commit access would be interested, I can write all of the code against -CURRENT, and submit a diff patch for review. It would be only a small amount of code, maybe 30 lines or so, with appropriate error checking and fallbacks in place. Again, I'm very new to this, so please bear with me.
>>
>> Personally, for now, I just monkey patched it into acpica.c and confirmed that the concept works.
>>
>> If there's no interest, then I will drop the matter.
>>
>> Thank you everyone for your time!
>>
>> _______________________________________________
>> freebsd-hackers at freebsd.org <mailto:freebsd-hackers at freebsd.org>>> mailing list
>> https://lists.freebsd.org/mailman/listinfo/freebsd-hackers <https://lists.freebsd.org/mailman/listinfo/freebsd-hackers>
>> To unsubscribe, send any mail to ">> freebsd-hackers-unsubscribe at freebsd.org <mailto:freebsd-hackers-unsubscribe at freebsd.org>>> "
>>
More information about the freebsd-hackers
mailing list