From owner-freebsd-hackers@freebsd.org Mon Dec 10 13:06:04 2018 Return-Path: Delivered-To: freebsd-hackers@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id E87A71325479 for ; Mon, 10 Dec 2018 13:06:03 +0000 (UTC) (envelope-from byuu@tutanota.com) Received: from w1.tutanota.de (w1.tutanota.de [81.3.6.162]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "tutanota.de", Issuer "COMODO RSA Domain Validation Secure Server CA" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id DA3FA77550; Mon, 10 Dec 2018 13:06:02 +0000 (UTC) (envelope-from byuu@tutanota.com) Received: from w2.tutanota.de (unknown [192.168.1.163]) by w1.tutanota.de (Postfix) with ESMTP id 4AD98FA0621; Mon, 10 Dec 2018 13:05:55 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tutanota.com; s=20161216; t=1544447155; bh=stpDA2i8Z69Jp25ctxXiRrGuoR9kM79ubrgPBAZlXDU=; h=Date:From:To:In-Reply-To:References:Subject:From; b=0S3dxAt68NBmfP8AaVffd64rM3YvSmdM1zPSNWzP5TliAstlpkhA7sccs8dHslZBm wTtDkcxNA6ykwqX3CyiiV4RLRK9rWv1RvmTjtQcb2jTmHMO9nWGY6uc8o+kVn921P9 Ex9Ck3qJaGeUpt+M5fvliftUBmlpdXsJGxaB5do7U7ULleUO2JvtLyw6J5HZXGFmqG 6cXaSq0XOdfDuYeMdaCOQ9pew1sSgthn9zO/vtR2RJKdhIbqDcNo7QktogdaGtzkIz 7xtxNk+zgRNmEH4ZA4i00dXyoZy8kLX/1n02hFnNMHwbc8PfPbQY3GqMFPK75+5PpR C4UKw3HeLKSig== Date: Mon, 10 Dec 2018 14:05:55 +0100 (CET) From: To: Conrad Meyer , Freebsd Hackers Message-ID: In-Reply-To: References: <> Subject: Re: I'd like to submit a kernel patch for a new sysctl to use EFI instead of ACPI for poweroff + reboot MIME-Version: 1.0 X-Rspamd-Queue-Id: DA3FA77550 X-Spamd-Result: default: False [-0.60 / 15.00]; ARC_NA(0.00)[]; R_DKIM_ALLOW(-0.20)[tutanota.com]; IP_SCORE(-0.00)[country: DE(-0.01)]; R_SPF_ALLOW(-0.20)[+ip4:81.3.6.160/28]; TO_MATCH_ENVRCPT_ALL(0.00)[]; MIME_GOOD(-0.10)[multipart/alternative,text/plain]; NEURAL_SPAM_SHORT(0.21)[0.206,0]; NEURAL_HAM_LONG(-0.79)[-0.794,0]; NEURAL_SPAM_MEDIUM(0.10)[0.101,0]; URI_COUNT_ODD(1.00)[3]; TO_DN_ALL(0.00)[]; DKIM_TRACE(0.00)[tutanota.com:+]; RCPT_COUNT_TWO(0.00)[2]; FROM_NO_DN(0.00)[]; MX_GOOD(-0.01)[mail.tutanota.de,mail.tutanota.de]; DMARC_POLICY_ALLOW(-0.50)[tutanota.com,none]; RCVD_IN_DNSWL_LOW(-0.10)[162.6.3.81.list.dnswl.org : 127.0.10.1]; FROM_EQ_ENVFROM(0.00)[]; RCVD_TLS_LAST(0.00)[]; ASN(0.00)[asn:24679, ipnet:81.3.0.0/18, country:DE]; MID_RHS_MATCH_FROM(0.00)[]; RCVD_COUNT_TWO(0.00)[2] X-Rspamd-Server: mx1.freebsd.org Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.29 X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 10 Dec 2018 13:06:04 -0000 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 #include #include +#include +#include #include #include @@ -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 > > 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 >> mail= ing list >> https://lists.freebsd.org/mailman/listinfo/freebsd-hackers >> To unsubscribe, send any mail to ">> freebsd-hackers-unsubscribe@freebsd= .org >> " >>