Thanks for the comment :) Please see my reply in the comments.
On 27.11.19 15:43, Nadav Har'El wrote:
Hi, thanks for the patch! I have some questions:
On Wed, Nov 27, 2019 at 3:46 PM 'YuChen Qian' via OSv Development
<[email protected] <mailto:[email protected]>> wrote:
The original keyboard reset does not wait a for short period, so
we see
the triple fault being triggered even if the keyboard reset is
successful sometimes. Added a wait there to make sure the keyboard
reset
completes before triggering the triple fault.
Out of curiosity, what is the downside of not waiting, and trying
another reboot method even if the previous one will have eventually
worked?
Isn't the end-result the same - that it will reboot?
Is the risk that the machine will reboot for a second time right after
the first one?
The end result will be the same, but it is generally better to wait for
a little while and make sure the keyboard reset actually worked. We see
triple faults being triggered randomly even when the keyboard reset
works. Linux kernel also waits a little (arch/x86/kernel/reboot.c).
Added PCI reset, and enabled ACPI reset. ACPI reset does not work
currently on kvm and qemu, but nevertheless we should try it first.
Signed-off-by: YuChen Qian <[email protected]
<mailto:[email protected]>>
Reviewed-by: Hendrik Borghorst <[email protected]
<mailto:[email protected]>>
Reviewed-by: Marius Hillenbrand <[email protected]
<mailto:[email protected]>>
Cc-Team: kaos-brimstone <[email protected]
<mailto:[email protected]>>
CR: https://code.amazon.com/reviews/CR-14659421
---
arch/x64/power.cc | 30 +++++++++++++++++++++++-------
1 file changed, 23 insertions(+), 7 deletions(-)
diff --git a/arch/x64/power.cc b/arch/x64/power.cc
index 61d56bcc..d62ed84e 100644
--- a/arch/x64/power.cc
+++ b/arch/x64/power.cc
@@ -54,16 +54,32 @@ void poweroff(void)
halt();
}
+void pci_reboot(void) {
Please make these functions "static", so they are not visible from
outside this source file (there is no reason why they should be).
Thanks for pointing that out :) I will submit a new patch.
+ u8 v = processor::inb(0x0cf9) & ~6;
+ processor::outb(v|2, 0x0cf9); // request hard reset
+ usleep(50);
+ processor::outb(v|6, 0x0cf9); // actually do the reset
+ usleep(50);
Nice, I wasn't familiar with this method before.
Is it really necessary to write 0x2 once, wait, and then write 0x2 |
0x4 ("6") in a second write?
Won't a single write of 6 - once - work, as the (virtual) hardware
realizes that both 0x4 (do a reset) and 0x2 (make it a hardware reset)
bits are turned on?
I think, it is better to look at the Intel spec for this question. The
reset control register is located at offset CF9h. Bit 0 is reserved, bit
1 is used for system reset (SYS_RST), bit 2 is used for reset cpu
(RST_CPU), bit 3 is used for full reset (FULL_RST), and bit4-7 are also
reserved.
"If SYS_RST is 0 when RST_CPU goes from 0 to 1, then the PMC will force
INIT# active for 16 PCI clocks. /If SYS_RST is 1 when RST_CPU goes from
0 to 1, then the PMC will force PCI reset active for about 1 ms, however
the PMU_SLP_S3_B and PMU_SLP_S4_B signals assertion is dependent on the
value of the FULL_RST./
The RST_CPU bit will cause either a hard or soft reset to the CPU
depending on the state of the SYS_RST The software will cause the reset
by setting bit 2 from a 0 to a 1."
In other words, we need to first set the SYS_RST to 1, and then set the
RST_CPU to 1 (which forces a hard reset). The wait probably matters more
on real hardware.
Linux also implemented this reset method similarly, in
arch/x86/kernel/reboot.c
+}
+
+void kbd_reboot(void) {
+ while (processor::inb(0x64) & 0x02); // clear all keyboard
buffers
+ processor::outb(0xfe, 0x64);
+ usleep(50);
+}
+
void reboot(void)
{
- // It would be nice if AcpiReset() worked, but it doesn't
seem to work
- // (on qemu & kvm), so let's resort to other techniques,
which appear
- // to work. Hopefully one of them will work on any hypervisor.
- // Method 1: "fast reset" via System Control Port A (port 0x92)
- processor::outb(1, 0x92);
+ // Method 1: AcpiReset, does not work on qemu or kvm now
because the reset
+ // register is not supported. Nevertheless, we should try it
first
+ AcpiReset();
// Method 2: Reset using the 8042 PS/2 Controller ("keyboard
controller")
- processor::outb(0xfe, 0x64);
- // Method 3: Cause triple fault by loading a broken IDT and
triggering an
+ kbd_reboot();
+ // Method 3: PCI reboot
+ pci_reboot();
+ // Method 4: "fast reset" via System Control Port A (port 0x92)
+ processor::outb(1, 0x92);
I'm curious why you moved this, which was the first method we tried
until now, to be the fourth. What's
the downside of doing it first?
Yes, there is no downside. I will submit a new patch with the original
order.
+ // Method 5: Cause triple fault by loading a broken IDT and
triggering an
// interrupt.
processor::lidt(processor::desc_ptr(0, 0));
__asm__ __volatile__("int3");
--
2.17.1
Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879
--
You received this message because you are subscribed to the Google
Groups "OSv Development" group.
To unsubscribe from this group and stop receiving emails from it,
send an email to [email protected]
<mailto:osv-dev%[email protected]>.
To view this discussion on the web visit
https://groups.google.com/d/msgid/osv-dev/20191127134607.15293-1-yuchenq%40amazon.de.
--
You received this message because you are subscribed to the Google Groups "OSv
Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To view this discussion on the web visit
https://groups.google.com/d/msgid/osv-dev/c395dc69-cc94-42aa-9527-76c019978829%40amazon.com.
