Date: Mon, 10 Dec 2018 14:12:06 +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: <LTNEkN7--3-1@tutanota.com> In-Reply-To: <LTNDKVv--3-1@tutanota.com> References: <LTDwcvo--3-1@tutanota.com> <<LTDwcvo--3-1@tutanota.com>> <CAG6CVpU5dczi16TpbvBrPKn8xSa-z2kGWL-RJ-5=nF1r7BfBsg@mail.gmail.com> <LTNDKVv--3-1@tutanota.com>
next in thread | previous in thread | raw e-mail | index | archive | help
In case the formatting was damaged by posting it through the mailing list .= .. here's another copy of the patch: https://pastebin.com/raw/US5VbZGp <https://pastebin.com/raw/US5VbZGp>; Dec 10, 2018, 10:05 PM by byuu@tutanota.com: > 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 success= fully on 12.0-RC3. It allows me to power off my ASRock X399M (mATX) motherb= oard, and should also work for ASRock X399 (ATX) users who have reported th= e 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 fall= back option. > > I relinquish the entire copyright to the FreeBSD project. It's public dom= ain, no credit is necessary. > > Some notes: > > As requested, I moved IPMI to PRI-2 instead of PRI-1. I then placed EFI s= hutdown at PRI-1. > > In the spirit of DRY, I modified efi_reset_system(void) to efi_reset_syst= em(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 var= ious 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 patc= h, but if you're okay with that, please add that as well. However, if you'r= e 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_R= ESET_SHUTDOWN define. > > I tried to respect all formatting conventions of the existing code. But a= s 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=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 tuna= ble. > +=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 ACP= I. > +=09 * When not enabled via sysctl tunable, this will fall through to ACP= I. > +=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 do= n't > +=09 * Power cycle the system off using IPMI. We use last - 2 since we do= n'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 <mailto: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@tutano= ta.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_syste= m() 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 identica= l 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 di= rectly after EFI_RESET_WARM.) >>> >>> (These two functions are wrappers to invoke the EFI Runtime Services fu= nction 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=3D0", which can be optionally cha= nged 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 ar= e running on an EFI system), I would like to invoke efi_poweroff_system() i= nstead of AcpiEnterSleepStatePrep(ACPI_STATE_S5) >>> >>> This is well after all services have been halted, all disks have been d= etached, 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 Threadr= ipper 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 ov= er a dozen things to find a fix for this: analyzing my DSDT ASL code via ac= pidump, disabling SMT + all cores, wrapping pause in a critical section, tr= ying 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 subsys= tem possible (including USB) while still allowing me to get to a login prom= pt, tweaking every sysctl I could find around ACPI, USB, debugging, etc. No= thing helped. >>> >>> Looking into Linux' ACPI support, they have acpi_no_s5, which they clai= m 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 miss= ing _S5 entry, as well as for when there are bugs in FreeBSD, or the mother= board 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 pla= tforms while users wait for a fix. >>> >>> If someone with kernel commit access would be interested, I can write a= ll of the code against -CURRENT, and submit a diff patch for review. It wou= ld be only a small amount of code, maybe 30 lines or so, with appropriate e= rror checking and fallbacks in place. Again, I'm very new to this, so pleas= e bear with me. >>> >>> Personally, for now, I just monkey patched it into acpica.c and confirm= ed 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>>>> ma= iling list >>> https://lists.freebsd.org/mailman/listinfo/freebsd-hackers <https://lis= ts.freebsd.org/mailman/listinfo/freebsd-hackers> >>> To unsubscribe, send any mail to ">>> freebsd-hackers-unsubscribe@freeb= sd.org <mailto:freebsd-hackers-unsubscribe@freebsd.org>>>> " >>> > >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?LTNEkN7--3-1>