Date: Mon, 10 Dec 2018 14:05:55 +0100 (CET) From: <byuu@tutanota.com> To: Conrad Meyer <cem@freebsd.org>, Freebsd Hackers <freebsd-hackers@freebsd.org> Subject: Re: I'd like to submit a kernel patch for a new sysctl to use EFI instead of ACPI for poweroff + reboot Message-ID: <LTNDKVv--3-1@tutanota.com> In-Reply-To: <CAG6CVpU5dczi16TpbvBrPKn8xSa-z2kGWL-RJ-5=nF1r7BfBsg@mail.gmail.com> References: <LTDwcvo--3-1@tutanota.com> <<LTDwcvo--3-1@tutanota.com>> <CAG6CVpU5dczi16TpbvBrPKn8xSa-z2kGWL-RJ-5=nF1r7BfBsg@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
Thank you, this information helped a lot!
I am not sure where I should submit my patch to ... I'll post it here if th=
at'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 successfu=
lly on 12.0-RC3. It allows me to power off my ASRock X399M (mATX) motherboa=
rd, 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 t=
o other users in the future who experience ACPI shutdown issues as a fallba=
ck option.
I relinquish the entire copyright to the FreeBSD project. It's public domai=
n, no credit is necessary.
Some notes:
As requested, I moved IPMI to PRI-2 instead of PRI-1. I then placed EFI shu=
tdown 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 e=
ver uses efi_reset_system(void), nor is it exposed via the /dev/efi ioctl i=
nterface. 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 vario=
us other tunables. Please feel free to rename the tunable to anything you l=
ike.
I think it would be nice if we exposed this tunable to the "sysctl" tool, a=
nd 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 r=
emains a "secret" /boot/loader.conf option only.
Lastly, I added the needed event handler headers, and added the new EFI_RES=
ET_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 yo=
u very much!
diff -ru old/sys/dev/efidev/efirt.c new/sys/dev/efidev/efirt.c
--- old/sys/dev/efidev/efirt.c=092018-12-10 04:32:01.231607000 +0900
+++ new/sys/dev/efidev/efirt.c=092018-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)
+{
+=09int efi_poweroff;
+
+=09/*
+=09 * On some systems, ACPI S5 is missing or does not function properly.
+=09 * Allow shutdown via EFI Runtime Services instead with a sysctl tunabl=
e.
+=09 */
+=09if((howto & RB_POWEROFF) !=3D 0) {
+=09=09efi_poweroff =3D 0;
+=09=09TUNABLE_INT_FETCH("hw.efi.poweroff", &efi_poweroff);
+=09=09if (efi_poweroff =3D=3D 1) {
+=09=09=09efi_reset_system(EFI_RESET_SHUTDOWN);
+=09=09}
+=09}
+}
+
static int
efi_init(void)
{
@@ -214,6 +234,13 @@
}
#endif
+=09/*
+=09 * We use SHUTDOWN_PRI_LAST - 1 to trigger after IPMI, but before ACPI.
+=09 * When not enabled via sysctl tunable, this will fall through to ACPI.
+=09 */
+=09EVENTHANDLER_REGISTER(shutdown_final, efi_shutdown_final, NULL,
+=09=09SHUTDOWN_PRI_LAST - 1);
+
return (0);
}
@@ -411,16 +438,20 @@
}
int
-efi_reset_system(void)
+efi_reset_system(int type)
{
struct efirt_callinfo ec;
+=09if (type !=3D EFI_RESET_COLD
+=09 && type !=3D EFI_RESET_WARM
+=09 && type !=3D EFI_RESET_SHUTDOWN)
+=09=09return (EINVAL);
if (efi_runtime =3D=3D NULL)
return (ENXIO);
bzero(&ec, sizeof(ec));
ec.ec_name =3D "rt_reset";
ec.ec_argcnt =3D 4;
-=09ec.ec_arg1 =3D (uintptr_t)EFI_RESET_WARM;
+=09ec.ec_arg1 =3D (uintptr_t)type;
ec.ec_arg2 =3D (uintptr_t)0;
ec.ec_arg3 =3D (uintptr_t)0;
ec.ec_arg4 =3D (uintptr_t)NULL;
diff -ru old/sys/dev/ipmi/ipmi.c new/sys/dev/ipmi/ipmi.c
--- old/sys/dev/ipmi/ipmi.c=092018-12-10 04:34:07.535522000 +0900
+++ new/sys/dev/ipmi/ipmi.c=092018-12-10 04:36:35.757611000 +0900
@@ -938,14 +938,14 @@
} else if (!on)
(void)ipmi_set_watchdog(sc, 0);
/*
-=09 * Power cycle the system off using IPMI. We use last - 1 since we don'=
t
+=09 * 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 =3D EVENTHANDLER_REGISTER(shutdown_final,
-=09=09=C2=A0=C2=A0=C2=A0 ipmi_power_cycle, sc, SHUTDOWN_PRI_LAST - 1);
+=09=09=C2=A0=C2=A0=C2=A0 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=092018-12-10 04:32:34.850892000 +0900
+++ new/sys/sys/efi.h=092018-12-10 04:35:41.011396000 +0900
@@ -43,7 +43,8 @@
enum efi_reset {
EFI_RESET_COLD,
-=09EFI_RESET_WARM
+=09EFI_RESET_WARM,
+=09EFI_RESET_SHUTDOWN
};
typedef uint16_t=09efi_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,
=C2=A0=C2=A0=C2=A0=C2=A0 size_t *datasize, void *data);
Dec 9, 2018, 3:35 AM by cem@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@tutanota.com <mailto:byuu@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 i=
t 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/e=
fi.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 dir=
ectly after EFI_RESET_WARM.)
>>
>> (These two functions are wrappers to invoke the EFI Runtime Services fun=
ction 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 a=
cpi_shutdown_final to check a new kernel sysctl, not picky about the name, =
but something like "hw.acpi.efi_shutdown=3D0", which can be optionally chan=
ged 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() in=
stead of AcpiEnterSleepStatePrep(ACPI_STATE_S5)
>>
>> This is well after all services have been halted, all disks have been de=
tached, and all USB devices have been detached. acpi_shutdown_final calls A=
cpiEnterSleepStatePrep (_PTS), followed by AcpiEnterSleepState (_S5), and t=
hat'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 e=
fi_reset_system().
>>
>> ...
>>
>> The reason I am requesting this, is because I am the owner of a Threadri=
pper 2950X with an ASRock Taichi X399M motherboard, and when I attempt to s=
hutdown my system from FreeBSD 11.2-RELEASE or 12.0-RC3, the system hangs f=
orever 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 ove=
r a dozen things to find a fix for this: analyzing my DSDT ASL code via acp=
idump, disabling SMT + all cores, wrapping pause in a critical section, try=
ing SLEEP instead of pause, trying a spinloop instead of pause, trying to s=
kip the AcpiEnterSleepStatePrep call, building a kernel with options ACPI_D=
EBUG and trying to selectively disable every device driver and ACPI subsyst=
em possible (including USB) while still allowing me to get to a login promp=
t, tweaking every sysctl I could find around ACPI, USB, debugging, etc. Not=
hing 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 missi=
ng _S5 entry, as well as for when there are bugs in FreeBSD, or the motherb=
oard 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 plat=
forms while users wait for a fix.
>>
>> If someone with kernel commit access would be interested, I can write al=
l of the code against -CURRENT, and submit a diff patch for review. It woul=
d be only a small amount of code, maybe 30 lines or so, with appropriate er=
ror 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 confirme=
d that the concept works.
>>
>> If there's no interest, then I will drop the matter.
>>
>> Thank you everyone for your time!
>>
>> _______________________________________________
>> freebsd-hackers@freebsd.org <mailto:freebsd-hackers@freebsd.org>>> mail=
ing list
>> https://lists.freebsd.org/mailman/listinfo/freebsd-hackers <https://list=
s.freebsd.org/mailman/listinfo/freebsd-hackers>
>> To unsubscribe, send any mail to ">> freebsd-hackers-unsubscribe@freebsd=
.org <mailto:freebsd-hackers-unsubscribe@freebsd.org>>> "
>>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?LTNDKVv--3-1>