[PATCH 0/5] hw: make TCO watchdog actually work by default for Q35

[PATCH 0/5] hw: make TCO watchdog actually work by default for Q35

[Daniel P. Berrangé]

The TCO watchdog is unconditionally integrated into the Q35 machine
type by default, but at the same time is unconditionally disabled
from firing by a host config option that overrides guest OS attempts
to enable it. People have to know to set a magic -global to make
it non-broken

IOW we're exposing a broken watchdog by default to all Q35 machines,
but which to the guest OS & its apps looks fully functional :-(

This behaviour was set in response to feedback from Michael:

  https://lists.gnu.org/archive/html/qemu-devel/2015-06/msg07128.html

    "I think sample high is a safer default."

but as explained in the commit message in the last patch, I think the
watchdog defaults were already safe without that pin strap setting.
The guest OS needs to take explicit action to clear the guest visible
'no reboot' flag, and so we don't need a second guest hidden 'no reboot'
flag to override that choice IMHO. Am I missing something ?

NB, I'm toggling this for 7.2 machine type since that's the current
git latest machine. Obviously this will need to be changed once
the 8.0 machine types are merged.

Changed in v2:

  - Added extra patch to honour the 'enable_tco' property

Daniel P. Berrangé (5):
  hw/acpi: add trace events for TCO watchdog register access
  hw/isa: add trace events for ICH9 LPC chip config access
  hw/watchdog: add trace events for watchdog action handling
  hw/isa: enable TCO watchdog reboot pin strap by default
  ich9: honour 'enable_tco' property

 hw/acpi/ich9.c           |  6 ++++--
 hw/acpi/tco.c            | 41 +++++++++++++++++++++++++++-------------
 hw/acpi/trace-events     |  2 ++
 hw/i386/pc.c             |  4 +++-
 hw/isa/lpc_ich9.c        |  5 ++++-
 hw/isa/trace-events      |  4 ++++
 hw/watchdog/trace-events |  4 ++++
 hw/watchdog/watchdog.c   |  4 ++++
 tests/qtest/tco-test.c   |  2 +-
 9 files changed, 54 insertions(+), 18 deletions(-)

-- 
2.38.1

[PATCH 1/5] hw/acpi: add trace events for TCO watchdog register access

[Daniel P. Berrangé]

These tracepoints aid in understanding and debugging the guest drivers
for the TCO watchdog.

Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 hw/acpi/tco.c        | 41 ++++++++++++++++++++++++++++-------------
 hw/acpi/trace-events |  2 ++
 2 files changed, 30 insertions(+), 13 deletions(-)

diff --git a/hw/acpi/tco.c b/hw/acpi/tco.c
index 4783721e4e..9ebd3e5e64 100644
--- a/hw/acpi/tco.c
+++ b/hw/acpi/tco.c
@@ -86,6 +86,7 @@ static inline int can_start_tco_timer(TCOIORegs *tr)
 static uint32_t tco_ioport_readw(TCOIORegs *tr, uint32_t addr)
 {
     uint16_t rld;
+    uint32_t ret = 0;
 
     switch (addr) {
     case TCO_RLD:
@@ -96,35 +97,49 @@ static uint32_t tco_ioport_readw(TCOIORegs *tr, uint32_t addr)
         } else {
             rld = tr->tco.rld;
         }
-        return rld;
+        ret = rld;
+        break;
     case TCO_DAT_IN:
-        return tr->tco.din;
+        ret = tr->tco.din;
+        break;
     case TCO_DAT_OUT:
-        return tr->tco.dout;
+        ret = tr->tco.dout;
+        break;
     case TCO1_STS:
-        return tr->tco.sts1;
+        ret = tr->tco.sts1;
+        break;
     case TCO2_STS:
-        return tr->tco.sts2;
+        ret = tr->tco.sts2;
+        break;
     case TCO1_CNT:
-        return tr->tco.cnt1;
+        ret = tr->tco.cnt1;
+        break;
     case TCO2_CNT:
-        return tr->tco.cnt2;
+        ret = tr->tco.cnt2;
+        break;
     case TCO_MESSAGE1:
-        return tr->tco.msg1;
+        ret = tr->tco.msg1;
+        break;
     case TCO_MESSAGE2:
-        return tr->tco.msg2;
+        ret = tr->tco.msg2;
+        break;
     case TCO_WDCNT:
-        return tr->tco.wdcnt;
+        ret = tr->tco.wdcnt;
+        break;
     case TCO_TMR:
-        return tr->tco.tmr;
+        ret = tr->tco.tmr;
+        break;
     case SW_IRQ_GEN:
-        return tr->sw_irq_gen;
+        ret = tr->sw_irq_gen;
+        break;
     }
-    return 0;
+    trace_tco_io_read(addr, ret);
+    return ret;
 }
 
 static void tco_ioport_writew(TCOIORegs *tr, uint32_t addr, uint32_t val)
 {
+    trace_tco_io_write(addr, val);
     switch (addr) {
     case TCO_RLD:
         tr->timeouts_no = 0;
diff --git a/hw/acpi/trace-events b/hw/acpi/trace-events
index eb60b04f9b..78e0a8670e 100644
--- a/hw/acpi/trace-events
+++ b/hw/acpi/trace-events
@@ -55,6 +55,8 @@ piix4_gpe_writeb(uint64_t addr, unsigned width, uint64_t val) "addr: 0x%" PRIx64
 # tco.c
 tco_timer_reload(int ticks, int msec) "ticks=%d (%d ms)"
 tco_timer_expired(int timeouts_no, bool strap, bool no_reboot) "timeouts_no=%d no_reboot=%d/%d"
+tco_io_write(uint64_t addr, uint32_t val) "addr=0x%" PRIx64 " val=0x%" PRIx32
+tco_io_read(uint64_t addr, uint32_t val) "addr=0x%" PRIx64 " val=0x%" PRIx32
 
 # erst.c
 acpi_erst_reg_write(uint64_t addr, uint64_t val, unsigned size) "addr: 0x%04" PRIx64 " <== 0x%016" PRIx64 " (size: %u)"
-- 
2.38.1

[PATCH 2/5] hw/isa: add trace events for ICH9 LPC chip config access

[Daniel P. Berrangé]

These tracepoints aid in understanding and debugging the guest drivers
for the TCO watchdog.

Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 hw/isa/lpc_ich9.c   | 3 +++
 hw/isa/trace-events | 4 ++++
 2 files changed, 7 insertions(+)

diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index 6c44cc9767..cea73924b4 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -52,6 +52,7 @@
 #include "hw/nvram/fw_cfg.h"
 #include "qemu/cutils.h"
 #include "hw/acpi/acpi_aml_interface.h"
+#include "trace.h"
 
 /*****************************************************************************/
 /* ICH9 LPC PCI to ISA bridge */
@@ -162,6 +163,7 @@ static void ich9_cc_write(void *opaque, hwaddr addr,
 {
     ICH9LPCState *lpc = (ICH9LPCState *)opaque;
 
+    trace_ich9_cc_write(addr, val, len);
     ich9_cc_addr_len(&addr, &len);
     memcpy(lpc->chip_config + addr, &val, len);
     pci_bus_fire_intx_routing_notifier(pci_get_bus(&lpc->d));
@@ -177,6 +179,7 @@ static uint64_t ich9_cc_read(void *opaque, hwaddr addr,
     uint32_t val = 0;
     ich9_cc_addr_len(&addr, &len);
     memcpy(&val, lpc->chip_config + addr, len);
+    trace_ich9_cc_read(addr, val, len);
     return val;
 }
 
diff --git a/hw/isa/trace-events b/hw/isa/trace-events
index b8f877e1ed..c4567a9b47 100644
--- a/hw/isa/trace-events
+++ b/hw/isa/trace-events
@@ -21,3 +21,7 @@ via_pm_io_read(uint32_t addr, uint32_t val, int len) "addr 0x%x val 0x%x len 0x%
 via_pm_io_write(uint32_t addr, uint32_t val, int len) "addr 0x%x val 0x%x len 0x%x"
 via_superio_read(uint8_t addr, uint8_t val) "addr 0x%x val 0x%x"
 via_superio_write(uint8_t addr, uint32_t val) "addr 0x%x val 0x%x"
+
+# lpc_ich9.c
+ich9_cc_write(uint64_t addr, uint64_t val, unsigned len) "addr=0x%"PRIx64 " val=0x%"PRIx64 " len=%u"
+ich9_cc_read(uint64_t addr, uint64_t val, unsigned len) "addr=0x%"PRIx64 " val=0x%"PRIx64 " len=%u"
-- 
2.38.1

[PATCH 3/5] hw/watchdog: add trace events for watchdog action handling

[Daniel P. Berrangé]

The tracepoints aid in debugging the triggering of watchdog devices.

Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 hw/watchdog/trace-events | 4 ++++
 hw/watchdog/watchdog.c   | 4 ++++
 2 files changed, 8 insertions(+)

diff --git a/hw/watchdog/trace-events b/hw/watchdog/trace-events
index 89ccbcfdfd..54371ae075 100644
--- a/hw/watchdog/trace-events
+++ b/hw/watchdog/trace-events
@@ -16,3 +16,7 @@ spapr_watchdog_stop(uint64_t num, uint64_t ret) "num=%" PRIu64 " ret=%" PRId64
 spapr_watchdog_query(uint64_t caps) "caps=0x%" PRIx64
 spapr_watchdog_query_lpm(uint64_t caps) "caps=0x%" PRIx64
 spapr_watchdog_expired(uint64_t num, unsigned action) "num=%" PRIu64 " action=%u"
+
+# watchdog.c
+watchdog_perform_action(unsigned int action) "action=%u"
+watchdog_set_action(unsigned int action) "action=%u"
diff --git a/hw/watchdog/watchdog.c b/hw/watchdog/watchdog.c
index 6c082a3263..955046161b 100644
--- a/hw/watchdog/watchdog.c
+++ b/hw/watchdog/watchdog.c
@@ -30,6 +30,7 @@
 #include "sysemu/watchdog.h"
 #include "hw/nmi.h"
 #include "qemu/help_option.h"
+#include "trace.h"
 
 static WatchdogAction watchdog_action = WATCHDOG_ACTION_RESET;
 
@@ -43,6 +44,8 @@ WatchdogAction get_watchdog_action(void)
  */
 void watchdog_perform_action(void)
 {
+    trace_watchdog_perform_action(watchdog_action);
+
     switch (watchdog_action) {
     case WATCHDOG_ACTION_RESET:     /* same as 'system_reset' in monitor */
         qapi_event_send_watchdog(WATCHDOG_ACTION_RESET);
@@ -89,4 +92,5 @@ void watchdog_perform_action(void)
 void qmp_watchdog_set_action(WatchdogAction action, Error **errp)
 {
     watchdog_action = action;
+    trace_watchdog_set_action(watchdog_action);
 }
-- 
2.38.1

[PATCH 4/5] hw/isa: enable TCO watchdog reboot pin strap by default

[Daniel P. Berrangé]

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

See also related bug reports

  https://bugzilla.redhat.com/show_bug.cgi?id=2080207
  https://bugzilla.redhat.com/show_bug.cgi?id=2136889
  https://bugzilla.redhat.com/show_bug.cgi?id=2137346

Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
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 fa69b6f43e..b2501fa6ab 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -107,7 +107,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 cea73924b4..8d541e2b54 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -792,7 +792,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.38.1

[PATCH 5/5] ich9: honour 'enable_tco' property

[Daniel P. Berrangé]

An 'ICH9-LPC.enable_tco' property has been exposed for a
very long time, but attempts to set it have never been
honoured.

Originally, any user provided 'enable_tco' value was force
replaced by a value passed from the machine type setup
code that was determine by machine type compat properties.

  commit d6b304ba924b95d12edfddaac99777b577301309
  Author: Eduardo Habkost <ehabkost@redhat.com>
  Date:   Sat Jan 23 14:02:10 2016 -0200

    machine: Remove no_tco field

    The field is always set to zero, so it is not necessary anymore.

After legacy Q35 machine types were deleted in:

  commit 86165b499edf8b03bb2d0e926d116c2f12a95bfe
  Author: Eduardo Habkost <ehabkost@redhat.com>
  Date:   Sat Jan 23 14:02:09 2016 -0200

    q35: Remove old machine versions

the machine type code ended up just unconditionally passing
'true', all the time, so this was further simplified in

  commit d6b304ba924b95d12edfddaac99777b577301309
  Author: Eduardo Habkost <ehabkost@redhat.com>
  Date:   Sat Jan 23 14:02:10 2016 -0200

    machine: Remove no_tco field

    The field is always set to zero, so it is not necessary anymore.

  commit 18d6abae3ea092950629e5d26aff1dcfc9a2d78e
  Author: Eduardo Habkost <ehabkost@redhat.com>
  Date:   Sat Jan 23 14:02:11 2016 -0200

    ich9: Remove enable_tco arguments from init functions

    The enable_tco arguments are always true, so they are not needed
    anymore.

Leaving the ich9_pm_init to just force set 'enable_tco' to true.
This still overrides any user specified property. The initialization
of property defaults should be done when properties are first
registered, rather than during object construction.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 hw/acpi/ich9.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index bd9bbade70..ea4182256d 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -316,8 +316,9 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
 
     pm->smm_enabled = smm_enabled;
 
-    pm->enable_tco = true;
-    acpi_pm_tco_init(&pm->tco_regs, &pm->io);
+    if (pm->enable_tco) {
+        acpi_pm_tco_init(&pm->tco_regs, &pm->io);
+    }
 
     if (pm->use_acpi_hotplug_bridge) {
         acpi_pcihp_init(OBJECT(lpc_pci),
@@ -440,6 +441,7 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm)
     pm->s4_val = 2;
     pm->use_acpi_hotplug_bridge = true;
     pm->keep_pci_slot_hpc = true;
+    pm->enable_tco = true;
 
     object_property_add_uint32_ptr(obj, ACPI_PM_PROP_PM_IO_BASE,
                                    &pm->pm_io_base, OBJ_PROP_FLAG_READ);
-- 
2.38.1