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

["Richard W.M. Jones" <rjones@redhat.com>]

On Mon, Oct 31, 2022 at 01:19:34PM +0000, Daniel P. Berrangé wrote:
> 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

Typo: "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.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

Add Fixes: or some other reference to the BZs?  We have a few!

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

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

Acked-by: Richard W.M. Jones <rjones@redhat.com>

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW