[PATCH] osv: fix keyboard reset, add pci reset

[YuChen Qian]

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.

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 <yuc...@amazon.de>
Reviewed-by: Hendrik Borghorst <hbor...@amazon.de>
Reviewed-by: Marius Hillenbrand <mhil...@amazon.de>
Cc-Team: kaos-brimstone <kaos-br...@amazon.com>

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) {
+ 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);
+}
+
+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);
+ // 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

[Nadav Har'El]

Hi, thanks for the patch! I have some questions:

On Wed, Nov 27, 2019 at 3:46 PM 'YuChen Qian' via OSv Development <osv...@googlegroups.com> 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?

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 <yuc...@amazon.de>
Reviewed-by: Hendrik Borghorst <hbor...@amazon.de>
Reviewed-by: Marius Hillenbrand <mhil...@amazon.de>
Cc-Team: kaos-brimstone <kaos-br...@amazon.com>

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).

+    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'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?

+    // 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 osv-dev+u...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/osv-dev/20191127134607.15293-1-yuchenq%40amazon.de.

[yuc...@amazon.com]

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 <osv...@googlegroups.com> 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 <yuc...@amazon.de>
Reviewed-by: Hendrik Borghorst <hbor...@amazon.de>
Reviewed-by: Marius Hillenbrand <mhil...@amazon.de>
Cc-Team: kaos-brimstone <kaos-br...@amazon.com>

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

Yes, there is no downside. I will submit a new patch with the original order.

[YuChen Qian]

From: yuchenq <yuc...@amazon.de>

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.

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 <yuc...@amazon.de>
Reviewed-by: Hendrik Borghorst <hbor...@amazon.de>
Reviewed-by: Marius Hillenbrand <mhil...@amazon.de>
Cc-Team: kaos-brimstone <kaos-br...@amazon.com>

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..bc3124da 100644

--- a/arch/x64/power.cc
+++ b/arch/x64/power.cc
@@ -54,16 +54,32 @@ void poweroff(void)
halt();
}

+static void pci_reboot(void) {

+ 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);

+}
+
+static 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)

+ // 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: "fast reset" via System Control Port A (port 0x92)
processor::outb(1, 0x92);
- // 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

+ // Method 3: Reset using the 8042 PS/2 Controller ("keyboard controller")
+ kbd_reboot();
+ // Method 4: PCI reboot
+ pci_reboot();
+ // Method 5: Cause triple fault by loading a broken IDT and triggering an

[Nadav Har'El]

On Thu, Nov 28, 2019 at 4:03 PM <yuc...@amazon.com> wrote:

Thanks for the comment :) Please see my reply in the comments.

On 27.11.19 15:43, Nadav Har'El wrote:

+    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.

I'm not convinced by this logic - there is no single-bit addressing in x86, we write the entire byte at once. When the CPU notices RST_CPU=1, it will also see SYS_RST=1.

But I see that in Linux they did it exactly the way you did it, and also the same 50usec delay, so I guess I can live with this too :-)

Linux also implemented this reset method similarly, in arch/x86/kernel/reboot.c

Yes, I see now.

[Nadav Har'El]

Note that the comment is somewhat misleading - this code doesn't "clear all keyboard buffers".

What it really does is to wait until the keyboard controller's input buffer (and only that buffer) has room. The input buffer is what will need to hold our following command (which we send via port 0x64), and we are not supposed to send a new command before the previous one was handled. This 0x2 bit is cleared when the controller is ready for a new command, so only when it's cleared can we send that command.

Linux has a timeout in this loop (around 128ms), I guess to avoid hanging forever with malfunctioning hardware, but we can live with an infinite loop too.

I'll commit this patch as-is, if you want to further improve things, feel free to send a followup patch.

Thanks!

--
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 osv-dev+u...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/osv-dev/20191128142931.22753-1-yuchenq%40amazon.de.

[Commit Bot]

From: yuchenq <yuc...@amazon.de>
Committer: Nadav Har'El <n...@scylladb.com>
Branch: master

osv: fix keyboard reset, add pci reset

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.

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 <yuc...@amazon.de>
Reviewed-by: Hendrik Borghorst <hbor...@amazon.de>
Reviewed-by: Marius Hillenbrand <mhil...@amazon.de>
Cc-Team: kaos-brimstone <kaos-br...@amazon.com>

CR: https://code.amazon.com/reviews/CR-14659421

Message-Id: <20191128142931....@amazon.de>

---
diff --git a/arch/x64/power.cc b/arch/x64/power.cc
--- a/arch/x64/power.cc
+++ b/arch/x64/power.cc
@@ -55,16 +55,32 @@ void poweroff(void)