Intel SBX82 blades do not have a keyboard controller and thus do not reset by traditional means. This also has the implication that they sit there and do nothing after running reboot(8). These blades rely on ACPI for rebooting, as discussed in the ACPI specification 4.7.3.6 (PDF page 95). Without this support, it is impossible to reboot these blades from within the operating system (without doing evil things to the CPU). Fix: I (very briefly) spoke with njl@ on this issue in an email. He suggested putting it somewhere else, though I'm sort of curious as to where it should go otherwise. The patch is in acpi_shutdown_final, which is supposed to execute after everything, since it is used to halt the system. I'd be happy to put this in its own function and use EVENTHANDLER(9) to run it later, if possible. The patch is attached here, and is also available at http://www.sitetronics.com/~dodell/acpi.diff +#include <vm/vm.h> +#include <vm/pmap.h> + #include <machine/clock.h> #include <machine/resource.h> #include <machine/bus.h> +#include <machine/cpufunc.h> +#include <machine/pci_cfgreg.h> #include <sys/rman.h> #include <isa/isavar.h> #include <isa/pnpvar.h> @@ -1636,6 +1641,49 @@ DELAY(1000000); printf("ACPI power-off failed - timeout\n"); } + } else if ((howto & RB_AUTOBOOT) != 0 && AcpiGbl_FADT->ResetRegSup) { + switch (AcpiGbl_FADT->ResetRegister.AddressSpaceId) { + case ACPI_ADR_SPACE_SYSTEM_MEMORY: + { + char *addr; + + if (AcpiGbl_FADT->ResetRegister.Address == 0) + panic("NULL Reset Address"); + + addr = pmap_mapdev( + (vm_offset_t)AcpiGbl_FADT->ResetRegister.Address, + (size_t)AcpiGbl_FADT->ResetRegister.RegisterBitWidth); + *addr = AcpiGbl_FADT->ResetValue; + } + break; + case ACPI_ADR_SPACE_SYSTEM_IO: + { + uint64_t port; + + port = AcpiGbl_FADT->ResetRegister.Address; + outb(port, AcpiGbl_FADT->ResetValue); + } + break; + case ACPI_ADR_SPACE_PCI_CONFIG: + { + uint64_t gas = AcpiGbl_FADT->ResetRegister.Address; + int bus, device, function, offset; + + bus = 0; + device = (gas & ACPI_GAS_DEVICE_MASK) >> ACPI_GAS_DEVICE_SHIFT; + function = (gas & ACPI_GAS_FUNCTION_MASK) >> + ACPI_GAS_FUNCTION_SHIFT; + offset = (gas & ACPI_GAS_OFFSET_MASK); + + pci_cfgregwrite(bus, device, function, offset, + AcpiGbl_FADT->ResetValue, 1); + } + break; + default: + printf("I didn't match any address space types!\n"); + } + DELAY(1000000); + printf("ACPI reboot failed - timeout\n"); } else if (panicstr == NULL) { printf("Shutting down ACPI\n"); AcpiTerminate(); +#define ACPI_GAS_DEVICE_MASK 0x0000FFFF00000000ULL +#define ACPI_GAS_DEVICE_SHIFT 32 +#define ACPI_GAS_FUNCTION_MASK 0x00000000FFFF0000ULL +#define ACPI_GAS_FUNCTION_SHIFT 16 +#define ACPI_GAS_OFFSET_MASK 0x000000000000FFFFULL + #endif /* _KERNEL */ #endif /* !_ACPIVAR_H_ */--iCIriixWqXJB0Wjia0DQ6Hvnq3hAl3iX0RXjUO6Ao8M3EB8R Content-Type: text/plain; name="file.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="file.diff" Index: sys/dev/acpica/acpi.c =================================================================== RCS file: /home/dodell/cvs/src/sys/dev/acpica/acpi.c,v retrieving revision 1.219 diff -u -r1.219 acpi.c --- sys/dev/acpica/acpi.c 7 Nov 2005 21:52:06 -0000 1.219 +++ sys/dev/acpica/acpi.c 25 Mar 2006 23:04:11 -0000 @@ -48,9 +48,14 @@ #include <sys/sbuf.h> #include <sys/smp.h> How-To-Repeat: Obtain root privileges on a blade. Run reboot(8).
Nate, I'm curious where you think this code should go if not here? I'd imagine we don't want to do this after AcpiTerminate() since perhaps the specified register may no longer be available (I might be wrong though, I haven't checked the spec). -- John Baldwin <jhb@FreeBSD.org> <>< http://www.FreeBSD.org/~jhb/ "Power Users Use the Power to Serve" = http://www.FreeBSD.org
John Baldwin wrote: > Nate, > > I'm curious where you think this code should go if not here? I'd imagine > we don't want to do this after AcpiTerminate() since perhaps the specified > register may no longer be available (I might be wrong though, I haven't > checked the spec). > I don't have a specific idea since I didn't look at it closely. I think there might be some requirements of writes to the reset register (delays, expectation of chipset configuration, order with other shutdown tasks). Here are the requirements from the spec: >>> 4.7.3.6 Reset Register The optional ACPI reset mechanism specifies a standard mechanism that provides a complete system reset. When implemented, this mechanism must reset the entire system. This includes processors, core logic, all buses, and all peripherals. From an OSPM perspective, asserting the reset mechanism is the logical equivalent to power cycling the machine. Upon gaining control after a reset, OSPM will perform actions in like manner to a cold boot. ... The system must reset immediately following the write to this register. OSPM assumes that the processor will not execute beyond the write instruction. OSPM should execute spin loops on the CPUs in the system following a write to this register. >>> So I'm ok with the patch being committed if no other tasks need to happen after this shutdown handler is called. Also, all APs should be stopped before this happens and it should only occur once on the BSP. -- Nate
> The system must reset immediately following the write to this register. > OSPM assumes that the processor will not execute beyond the write > instruction. OSPM should execute spin loops on the CPUs in the system > following a write to this register. > >>> My interpretation of this is ``don't do anything else after the write to the register, because you can't expect to do it.'' Since they say that the system ``must reset immediately following the write'', it seems that this is implemented in hardware, and we can't assume that we will be able to do anything afterwards, anyway. > So I'm ok with the patch being committed if no other tasks need to > happen after this shutdown handler is called. Also, all APs should be > stopped before this happens and it should only occur once on the BSP. I was curious if anything happens after this handler is called -- if there is, we definitely need to move it back to later in the process. Again, I put the code here because it looked to me like the procedure already assumed nothing else is happening, but it sounds like there are other procedures that are in the call queue after this one. --Devon > -- > Nate
Does the reset mechanism require that ACPI be "functioning"? That is, does it have to happen before the call to AcpiTerminate() or can it happen afterwards? If it can happen afterwards, it should probably be moved to be part of cpu_reset_real(). -- John Baldwin <jhb@FreeBSD.org> <>< http://www.FreeBSD.org/~jhb/ "Power Users Use the Power to Serve" = http://www.FreeBSD.org
> Does the reset mechanism require that ACPI be "functioning"? That is, > does it have to happen before the call to AcpiTerminate() or can it > happen afterwards? If it can happen afterwards, it should probably be > moved to be part of cpu_reset_real(). It *probably* works without acpi enabled because on x86 at least, it's just a write to the SMM io port, which triggers an SMI and the handler resets the machine. SMM is present whether in legacy mode or acpi mode. However, I don't want to put acpi-specific resets in cpu_reset_real() because then acpi is mandatory for linking the kernel. Let's just try it in the place the patch puts it for now and see if there are any problems. The patch has some other major problems that should be addressed before committing. It should not manually be parsing the GAS and mapping memory etc. Instead, it should just use AcpiHwLowLevelWrite(): ACPI_STATUS AcpiHwLowLevelWrite ( UINT32 Width, UINT32 Value, ACPI_GENERIC_ADDRESS *Reg); Width should be 8, value should be the reset value in the FADT, and Reg should be the FADT GAS struct. -- Nate
> It *probably* works without acpi enabled because on x86 at least, it's > just a write to the SMM io port, which triggers an SMI and the handler > resets the machine. SMM is present whether in legacy mode or acpi mode. > However, I don't want to put acpi-specific resets in cpu_reset_real() > because then acpi is mandatory for linking the kernel. Let's just try > it in the place the patch puts it for now and see if there are any proble= ms. Ok. What I was thinking was to save the AcpiGBL_FADT->{$FOO} into some local kernel structure and then to put it into cpu_reset_real(). I suppose this would mean that I would have to then manually parse the GAS, which, as you outline below, is an outstanding issue. > The patch has some other major problems that should be addressed before > committing. It should not manually be parsing the GAS and mapping > memory etc. Instead, it should just use AcpiHwLowLevelWrite(): > > ACPI_STATUS > AcpiHwLowLevelWrite ( > UINT32 Width, > UINT32 Value, > ACPI_GENERIC_ADDRESS *Reg); > > Width should be 8, value should be the reset value in the FADT, and Reg > should be the FADT GAS struct. AHA! I figured there was some function for this, but I kept looking for something that took an ACPI_GENERIC_ADDRESS and never found it. I'll rework the patch to use this procedure instead. I sent a couple other mails but they seem to have been eaten by some network restructuring that seems to have gone unnoticed for several months :\. --Devon > -- > Nate
> It *probably* works without acpi enabled because on x86 at least, it's > just a write to the SMM io port, which triggers an SMI and the handler > resets the machine. SMM is present whether in legacy mode or acpi mode. > However, I don't want to put acpi-specific resets in cpu_reset_real() > because then acpi is mandatory for linking the kernel. Let's just try > it in the place the patch puts it for now and see if there are any problems. Well, the place it happens now the APs aren't spinning yet, and it won't work for the 'reset' command in ddb (though that is perhaps less important). -- John Baldwin <jhb@FreeBSD.org> <>< http://www.FreeBSD.org/~jhb/ "Power Users Use the Power to Serve" = http://www.FreeBSD.org
> The patch has some other major problems that should be addressed before > committing. It should not manually be parsing the GAS and mapping > memory etc. Instead, it should just use AcpiHwLowLevelWrite(): > > ACPI_STATUS > AcpiHwLowLevelWrite ( > UINT32 Width, > UINT32 Value, > ACPI_GENERIC_ADDRESS *Reg); > > Width should be 8, value should be the reset value in the FADT, and Reg > should be the FADT GAS struct. Please see http://www.sitetronics.com/~dodell/acpi2.diff (also attached). This addresses this issue and significantly reduces the size of the diff :) --Devon
> Please see http://www.sitetronics.com/~dodell/acpi2.diff (also > attached). This addresses this issue and significantly reduces the > size of the diff :) Maybe it should use AcpiGbl_FADT->ResetRegister.RegisterBitWidth rather than hardcoding 8? -- John Baldwin <jhb@FreeBSD.org> <>< http://www.FreeBSD.org/~jhb/ "Power Users Use the Power to Serve" = http://www.FreeBSD.org
> Maybe it should use AcpiGbl_FADT->ResetRegister.RegisterBitWidth rather > than hardcoding 8? You're right, I should have used that. It will always be 8 as is stated in the specification, but that's a better idea. I shouldn't have missed that. Thanks! http://www.sitetronics.com/~dodell/acpi3.diff -- attached for audit trail convenience. --Devon
*bangs head on keyboard* Sorry for the flood of stuff. And now, acpi4.diff which actually includes the correct header file, too. *sighs* sorry about that. I'm not going to attach the diff here because gmail seems to find it necessary to base64-encode it and transfer it as application/octet-stream :( http://www.sitetronics.com/~dodell/acpi4.diff --Devon
Committed with some minor fixes, thanks. We'll MFC after it's shown not to harm reboots on other systems. -- Nate
> I was curious if anything happens after this handler is > called -- if there is, we definitely need to move it back > to later in the process. Again, I put the code here because it > looked to me like the procedure already assumed nothing else > is happening, but it sounds like there are other procedures > that are in the call queue after this one. It really should be much later I think: in cpu_reset_real() as that is the only place that you know that the APs are stopped. -- John Baldwin <jhb@FreeBSD.org> =A0<>< =A0http://www.FreeBSD.org/~jhb/ "Power Users Use the Power to Serve" =A0=3D =A0http://www.FreeBSD.org
> It really should be much later I think: in cpu_reset_real() as that > is the only place that you know that the APs are stopped. I'm not near a BSD box today. Is there a simple, MI way of hooking there that doesn't require ACPI compiled into the kernel? If it's a simple matter of moving it to a different shutdown handler or adding a way for acpi to conditionally override cpu_reset_real, that's ok with me. I don't want acpi being partially merged into the main kernel. -- Nate
> I'm not near a BSD box today. Is there a simple, MI way of hooking > there that doesn't require ACPI compiled into the kernel? If it's a > simple matter of moving it to a different shutdown handler or adding a > way for acpi to conditionally override cpu_reset_real, that's ok with > me. I don't want acpi being partially merged into the main kernel. I can move this to its own event handler that will be executed later on. > -- > Nate
> I'm not near a BSD box today. Is there a simple, MI way of hooking > there that doesn't require ACPI compiled into the kernel? If it's a > simple matter of moving it to a different shutdown handler or adding a > way for acpi to conditionally override cpu_reset_real, that's ok with > me. I don't want acpi being partially merged into the main kernel. Not currently. -- John Baldwin <jhb@FreeBSD.org> <>< http://www.FreeBSD.org/~jhb/ "Power Users Use the Power to Serve" = http://www.FreeBSD.org
State Changed From-To: open->feedback Did this code ever get committed?
State Changed From-To: feedback->closed feedback timeout, please notify us when you have additional feedback!