qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: "Daniel P. Berrangé" <berrange@redhat.com>
To: qemu-devel@nongnu.org
Cc: "Laurent Vivier" <lvivier@redhat.com>,
	"Richard Henderson" <richard.henderson@linaro.org>,
	"Ani Sinha" <ani@anisinha.ca>,
	"Igor Mammedov" <imammedo@redhat.com>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	"Thomas Huth" <thuth@redhat.com>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Eduardo Habkost" <eduardo@habkost.net>,
	"Marcel Apfelbaum" <marcel.apfelbaum@gmail.com>,
	"Richard W.M. Jones" <rjones@redhat.com>,
	"Daniel P. Berrangé" <berrange@redhat.com>
Subject: [PATCH 4/4] hw/isa: enable TCO watchdog reboot pin strap by default
Date: Mon, 31 Oct 2022 13:19:34 +0000	[thread overview]
Message-ID: <20221031131934.425448-5-berrange@redhat.com> (raw)
In-Reply-To: <20221031131934.425448-1-berrange@redhat.com>

The TCO watchdog implementation default behaviour from POV of the
guest OS relies on the initial values for two I/O ports:

  * TCO1_CNT == 0x0

    Since bit 11 (TCO Timer Halt) is clear, the watchdog state
    is considered to be initially running

  * GCS == 0x20

    Since bit 5 (No Reboot) is set, the watchdog will not trigger
    when the timer expires

This is a safe default, because the No Reboot bit will prevent the
watchdog from triggering if the guest OS is unaware of its existance,
or is slow in configuring it. When a Linux guest initializes the TCO
watchdog, it will attempt to clear the "No Reboot" flag, and read the
value back. If the clear was honoured, the driver will treat this as
an indicator that the watchdog is functional and create the geust
watchdog device.

QEMU implements a second "no reboot" flag, however, via pin straps
which overrides the behaviour of the guest controlled "no reboot"
flag:

  commit 5add35bec1e249bb5345a47008c8f298d4760be4
  Author: Paulo Alcantara <pcacjr@gmail.com>
  Date:   Sun Jun 28 14:58:58 2015 -0300

    ich9: implement strap SPKR pin logic

This second 'noreboot' pin was defaulted to high, which also inhibits
triggering of the requested watchdog actions, unless QEMU is launched
with the magic flag "-global ICH9-LPC.noreboot=false".

This is a bad default as we are exposing a watchdog to every guest OS
using the q35 machine type, but preventing it from actually doing what
it is designed to do. What is worse is that the guest OS and its apps
have no way to know that the watchdog is never going to fire, due to
this second 'noreboot' pin.

If a guest OS had no watchdog device at all, then apps whose operation
and/or data integrity relies on a watchdog can refuse to launch, and
alert the administrator of the problematic deployment. With Q35 machines
unconditionally exposing a watchdog though, apps will think their
deployment is correct but in fact have no protection at all.

This patch flips the default of the second 'no reboot' flag, so that
configured watchdog actions will be honoured out of the box for the
7.2 Q35 machine type onwards, if the guest enables use of the watchdog.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 hw/i386/pc.c           | 4 +++-
 hw/isa/lpc_ich9.c      | 2 +-
 tests/qtest/tco-test.c | 2 +-
 3 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 3e86083db3..e814f62fc6 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -108,7 +108,9 @@
     { "qemu64-" TYPE_X86_CPU, "model-id", "QEMU Virtual CPU version " v, },\
     { "athlon-" TYPE_X86_CPU, "model-id", "QEMU Virtual CPU version " v, },
 
-GlobalProperty pc_compat_7_1[] = {};
+GlobalProperty pc_compat_7_1[] = {
+    { "ICH9-LPC", "noreboot", "true" },
+};
 const size_t pc_compat_7_1_len = G_N_ELEMENTS(pc_compat_7_1);
 
 GlobalProperty pc_compat_7_0[] = {};
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index 66062a344c..f9ce2ee1dc 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -789,7 +789,7 @@ static const VMStateDescription vmstate_ich9_lpc = {
 };
 
 static Property ich9_lpc_properties[] = {
-    DEFINE_PROP_BOOL("noreboot", ICH9LPCState, pin_strap.spkr_hi, true),
+    DEFINE_PROP_BOOL("noreboot", ICH9LPCState, pin_strap.spkr_hi, false),
     DEFINE_PROP_BOOL("smm-compat", ICH9LPCState, pm.smm_compat, false),
     DEFINE_PROP_BIT64("x-smi-broadcast", ICH9LPCState, smi_host_features,
                       ICH9_LPC_SMI_F_BROADCAST_BIT, true),
diff --git a/tests/qtest/tco-test.c b/tests/qtest/tco-test.c
index 254f735370..caabcac6e5 100644
--- a/tests/qtest/tco-test.c
+++ b/tests/qtest/tco-test.c
@@ -60,7 +60,7 @@ static void test_init(TestData *d)
     QTestState *qs;
 
     qs = qtest_initf("-machine q35 %s %s",
-                     d->noreboot ? "" : "-global ICH9-LPC.noreboot=false",
+                     d->noreboot ? "-global ICH9-LPC.noreboot=true" : "",
                      !d->args ? "" : d->args);
     qtest_irq_intercept_in(qs, "ioapic");
 
-- 
2.37.3



  parent reply	other threads:[~2022-10-31 13:21 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-10-31 13:19 [PATCH 0/4] hw: make TCO watchdog actually work by default for Q35 Daniel P. Berrangé
2022-10-31 13:19 ` [PATCH 1/4] hw/acpi: add trace events for TCO watchdog register access Daniel P. Berrangé
2022-10-31 13:34   ` Richard W.M. Jones
2022-10-31 13:19 ` [PATCH 2/4] hw/isa: add trace events for ICH9 LPC chip config access Daniel P. Berrangé
2022-10-31 13:36   ` Richard W.M. Jones
2022-10-31 13:19 ` [PATCH 3/4] hw/watchdog: add trace events for watchdog action handling Daniel P. Berrangé
2022-10-31 13:36   ` Richard W.M. Jones
2022-10-31 15:46   ` Philippe Mathieu-Daudé
2022-10-31 13:19 ` Daniel P. Berrangé [this message]
2022-10-31 13:40   ` [PATCH 4/4] hw/isa: enable TCO watchdog reboot pin strap by default Richard W.M. Jones
2022-10-31 13:50 ` [PATCH 0/4] hw: make TCO watchdog actually work by default for Q35 Daniel P. Berrangé
2022-10-31 15:48   ` Michael S. Tsirkin
2022-11-01 12:57     ` Igor Mammedov
2022-11-01 13:03       ` Daniel P. Berrangé
2022-11-10 16:29         ` Michael S. Tsirkin
2022-11-10 18:21           ` Daniel P. Berrangé
2022-11-10 16:30 ` Michael S. Tsirkin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20221031131934.425448-5-berrange@redhat.com \
    --to=berrange@redhat.com \
    --cc=ani@anisinha.ca \
    --cc=eduardo@habkost.net \
    --cc=imammedo@redhat.com \
    --cc=lvivier@redhat.com \
    --cc=marcel.apfelbaum@gmail.com \
    --cc=mst@redhat.com \
    --cc=pbonzini@redhat.com \
    --cc=qemu-devel@nongnu.org \
    --cc=richard.henderson@linaro.org \
    --cc=rjones@redhat.com \
    --cc=thuth@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).