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 <pca...@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é <berra...@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 <rjo...@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
