Date: Mon, 10 Dec 2018 12:38:31 -0700 From: Jason Wolfe <j@nitrology.com> To: byuu@tutanota.com Cc: Conrad Meyer <cem@freebsd.org>, Freebsd Hackers <freebsd-hackers@freebsd.org>, owner-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: <23b3fe208bfb2b6ba5e50f4b88a84ac6@nitrology.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
Hey, https://wiki.freebsd.org/Phabricator is a good guide on how to get your patch some visibility and hopefully ushered in. Jason On 2018-12-10 06:05, byuu@tutanota.com wrote: > 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@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 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@freebsd.org <mailto:freebsd-hackers@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@freebsd.org >>> <mailto:freebsd-hackers-unsubscribe@freebsd.org>>> " >>> > > _______________________________________________ > freebsd-hackers@freebsd.org mailing list > https://lists.freebsd.org/mailman/listinfo/freebsd-hackers > To unsubscribe, send any mail to > "freebsd-hackers-unsubscribe@freebsd.org"
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?23b3fe208bfb2b6ba5e50f4b88a84ac6>