* [PATCH] x86: ich9: fix default value of 'No Reboot' bit in GCS
@ 2025-09-22 13:26 Igor Mammedov
2025-09-22 14:24 ` Daniel P. Berrangé
0 siblings, 1 reply; 4+ messages in thread
From: Igor Mammedov @ 2025-09-22 13:26 UTC (permalink / raw)
To: qemu-devel; +Cc: mst, imammedo
[2] initialized 'No Reboot' bit to 1 by default. And due to quirk it happened
to work with linux iTCO_wdt driver (which clears it on module load).
However spec [1] states:
"
R/W. This bit is set when the “No Reboot” strap (SPKR pin on
ICH9) is sampled high on PWROK.
"
So it should be set only when '-global ICH9-LPC.noreboot=true' and cleared
when it's false (which should be default).
Fix it to behave according to spec and set 'No Reboot' bit only when
'-global ICH9-LPC.noreboot=true'.
1)
Intel I/O Controller Hub 9 (ICH9) Family Datasheet (rev: 004)
2)
Fixes: 920557971b6 (ich9: add TCO interface emulation)
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
include/hw/southbridge/ich9.h | 2 +-
hw/isa/lpc_ich9.c | 7 ++++++-
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/include/hw/southbridge/ich9.h b/include/hw/southbridge/ich9.h
index 1e231e89c9..2c35dd0484 100644
--- a/include/hw/southbridge/ich9.h
+++ b/include/hw/southbridge/ich9.h
@@ -95,7 +95,7 @@ struct ICH9LPCState {
#define ICH9_CC_OIC 0x31FF
#define ICH9_CC_OIC_AEN 0x1
#define ICH9_CC_GCS 0x3410
-#define ICH9_CC_GCS_DEFAULT 0x00000020
+#define ICH9_CC_GCS_DEFAULT 0x00000000
#define ICH9_CC_GCS_NO_REBOOT (1 << 5)
/* D28:F[0-5] */
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index 304dffac32..c9cb8f7779 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -132,6 +132,11 @@ static void ich9_cc_init(ICH9LPCState *lpc)
static void ich9_cc_reset(ICH9LPCState *lpc)
{
uint8_t *c = lpc->chip_config;
+ uint32_t gcs = ICH9_CC_GCS_DEFAULT;
+
+ if (lpc->pin_strap.spkr_hi) {
+ gcs |= ICH9_CC_GCS_NO_REBOOT;
+ }
memset(lpc->chip_config, 0, sizeof(lpc->chip_config));
@@ -142,7 +147,7 @@ static void ich9_cc_reset(ICH9LPCState *lpc)
pci_set_long(c + ICH9_CC_D27IR, ICH9_CC_DIR_DEFAULT);
pci_set_long(c + ICH9_CC_D26IR, ICH9_CC_DIR_DEFAULT);
pci_set_long(c + ICH9_CC_D25IR, ICH9_CC_DIR_DEFAULT);
- pci_set_long(c + ICH9_CC_GCS, ICH9_CC_GCS_DEFAULT);
+ pci_set_long(c + ICH9_CC_GCS, gcs);
ich9_cc_update(lpc);
}
--
2.47.3
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] x86: ich9: fix default value of 'No Reboot' bit in GCS
2025-09-22 13:26 [PATCH] x86: ich9: fix default value of 'No Reboot' bit in GCS Igor Mammedov
@ 2025-09-22 14:24 ` Daniel P. Berrangé
2025-09-23 8:40 ` Igor Mammedov
0 siblings, 1 reply; 4+ messages in thread
From: Daniel P. Berrangé @ 2025-09-22 14:24 UTC (permalink / raw)
To: Igor Mammedov; +Cc: qemu-devel, mst
On Mon, Sep 22, 2025 at 03:26:00PM +0200, Igor Mammedov wrote:
> [2] initialized 'No Reboot' bit to 1 by default. And due to quirk it happened
> to work with linux iTCO_wdt driver (which clears it on module load).
>
> However spec [1] states:
> "
> R/W. This bit is set when the “No Reboot” strap (SPKR pin on
> ICH9) is sampled high on PWROK.
> "
>
> So it should be set only when '-global ICH9-LPC.noreboot=true' and cleared
> when it's false (which should be default).
>
> Fix it to behave according to spec and set 'No Reboot' bit only when
> '-global ICH9-LPC.noreboot=true'.
Is there a real-world problem you hit that is being solved by
this change, or is it just a theoretical spec compliance fix ?
>
> 1)
> Intel I/O Controller Hub 9 (ICH9) Family Datasheet (rev: 004)
> 2)
> Fixes: 920557971b6 (ich9: add TCO interface emulation)
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> include/hw/southbridge/ich9.h | 2 +-
> hw/isa/lpc_ich9.c | 7 ++++++-
> 2 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/include/hw/southbridge/ich9.h b/include/hw/southbridge/ich9.h
> index 1e231e89c9..2c35dd0484 100644
> --- a/include/hw/southbridge/ich9.h
> +++ b/include/hw/southbridge/ich9.h
> @@ -95,7 +95,7 @@ struct ICH9LPCState {
> #define ICH9_CC_OIC 0x31FF
> #define ICH9_CC_OIC_AEN 0x1
> #define ICH9_CC_GCS 0x3410
> -#define ICH9_CC_GCS_DEFAULT 0x00000020
> +#define ICH9_CC_GCS_DEFAULT 0x00000000
> #define ICH9_CC_GCS_NO_REBOOT (1 << 5)
>
> /* D28:F[0-5] */
> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> index 304dffac32..c9cb8f7779 100644
> --- a/hw/isa/lpc_ich9.c
> +++ b/hw/isa/lpc_ich9.c
> @@ -132,6 +132,11 @@ static void ich9_cc_init(ICH9LPCState *lpc)
> static void ich9_cc_reset(ICH9LPCState *lpc)
> {
> uint8_t *c = lpc->chip_config;
> + uint32_t gcs = ICH9_CC_GCS_DEFAULT;
> +
> + if (lpc->pin_strap.spkr_hi) {
> + gcs |= ICH9_CC_GCS_NO_REBOOT;
> + }
>
> memset(lpc->chip_config, 0, sizeof(lpc->chip_config));
>
> @@ -142,7 +147,7 @@ static void ich9_cc_reset(ICH9LPCState *lpc)
> pci_set_long(c + ICH9_CC_D27IR, ICH9_CC_DIR_DEFAULT);
> pci_set_long(c + ICH9_CC_D26IR, ICH9_CC_DIR_DEFAULT);
> pci_set_long(c + ICH9_CC_D25IR, ICH9_CC_DIR_DEFAULT);
> - pci_set_long(c + ICH9_CC_GCS, ICH9_CC_GCS_DEFAULT);
> + pci_set_long(c + ICH9_CC_GCS, gcs);
>
> ich9_cc_update(lpc);
> }
> --
> 2.47.3
>
>
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] x86: ich9: fix default value of 'No Reboot' bit in GCS
2025-09-22 14:24 ` Daniel P. Berrangé
@ 2025-09-23 8:40 ` Igor Mammedov
2025-09-23 15:24 ` Daniel P. Berrangé
0 siblings, 1 reply; 4+ messages in thread
From: Igor Mammedov @ 2025-09-23 8:40 UTC (permalink / raw)
To: Daniel P. Berrangé; +Cc: qemu-devel, mst, Gerd Hoffmann
On Mon, 22 Sep 2025 15:24:09 +0100
Daniel P. Berrangé <berrange@redhat.com> wrote:
> On Mon, Sep 22, 2025 at 03:26:00PM +0200, Igor Mammedov wrote:
> > [2] initialized 'No Reboot' bit to 1 by default. And due to quirk it happened
> > to work with linux iTCO_wdt driver (which clears it on module load).
> >
> > However spec [1] states:
> > "
> > R/W. This bit is set when the “No Reboot” strap (SPKR pin on
> > ICH9) is sampled high on PWROK.
> > "
> >
> > So it should be set only when '-global ICH9-LPC.noreboot=true' and cleared
> > when it's false (which should be default).
> >
> > Fix it to behave according to spec and set 'No Reboot' bit only when
> > '-global ICH9-LPC.noreboot=true'.
>
> Is there a real-world problem you hit that is being solved by
> this change, or is it just a theoretical spec compliance fix ?
I've stumbled upon it when implementing ACPI watchdog POC
https://gitlab.com/imammedo/qemu/-/commits/wadt_poc
I'm not sure that watchdog table belongs to QEMU,
but the ICH fix definitely is.
(wrt watchdog, unless we want to configure it from QEMU CLI
incl. all logistics to propagate it up mgmt stack9(s),
it should be implemented in firmware as in real world)
> >
> > 1)
> > Intel I/O Controller Hub 9 (ICH9) Family Datasheet (rev: 004)
> > 2)
> > Fixes: 920557971b6 (ich9: add TCO interface emulation)
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > include/hw/southbridge/ich9.h | 2 +-
> > hw/isa/lpc_ich9.c | 7 ++++++-
> > 2 files changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/hw/southbridge/ich9.h b/include/hw/southbridge/ich9.h
> > index 1e231e89c9..2c35dd0484 100644
> > --- a/include/hw/southbridge/ich9.h
> > +++ b/include/hw/southbridge/ich9.h
> > @@ -95,7 +95,7 @@ struct ICH9LPCState {
> > #define ICH9_CC_OIC 0x31FF
> > #define ICH9_CC_OIC_AEN 0x1
> > #define ICH9_CC_GCS 0x3410
> > -#define ICH9_CC_GCS_DEFAULT 0x00000020
> > +#define ICH9_CC_GCS_DEFAULT 0x00000000
> > #define ICH9_CC_GCS_NO_REBOOT (1 << 5)
> >
> > /* D28:F[0-5] */
> > diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> > index 304dffac32..c9cb8f7779 100644
> > --- a/hw/isa/lpc_ich9.c
> > +++ b/hw/isa/lpc_ich9.c
> > @@ -132,6 +132,11 @@ static void ich9_cc_init(ICH9LPCState *lpc)
> > static void ich9_cc_reset(ICH9LPCState *lpc)
> > {
> > uint8_t *c = lpc->chip_config;
> > + uint32_t gcs = ICH9_CC_GCS_DEFAULT;
> > +
> > + if (lpc->pin_strap.spkr_hi) {
> > + gcs |= ICH9_CC_GCS_NO_REBOOT;
> > + }
> >
> > memset(lpc->chip_config, 0, sizeof(lpc->chip_config));
> >
> > @@ -142,7 +147,7 @@ static void ich9_cc_reset(ICH9LPCState *lpc)
> > pci_set_long(c + ICH9_CC_D27IR, ICH9_CC_DIR_DEFAULT);
> > pci_set_long(c + ICH9_CC_D26IR, ICH9_CC_DIR_DEFAULT);
> > pci_set_long(c + ICH9_CC_D25IR, ICH9_CC_DIR_DEFAULT);
> > - pci_set_long(c + ICH9_CC_GCS, ICH9_CC_GCS_DEFAULT);
> > + pci_set_long(c + ICH9_CC_GCS, gcs);
> >
> > ich9_cc_update(lpc);
> > }
> > --
> > 2.47.3
> >
> >
>
> With regards,
> Daniel
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] x86: ich9: fix default value of 'No Reboot' bit in GCS
2025-09-23 8:40 ` Igor Mammedov
@ 2025-09-23 15:24 ` Daniel P. Berrangé
0 siblings, 0 replies; 4+ messages in thread
From: Daniel P. Berrangé @ 2025-09-23 15:24 UTC (permalink / raw)
To: Igor Mammedov; +Cc: qemu-devel, mst, Gerd Hoffmann
On Tue, Sep 23, 2025 at 10:40:51AM +0200, Igor Mammedov wrote:
> On Mon, 22 Sep 2025 15:24:09 +0100
> Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> > On Mon, Sep 22, 2025 at 03:26:00PM +0200, Igor Mammedov wrote:
> > > [2] initialized 'No Reboot' bit to 1 by default. And due to quirk it happened
> > > to work with linux iTCO_wdt driver (which clears it on module load).
> > >
> > > However spec [1] states:
> > > "
> > > R/W. This bit is set when the “No Reboot” strap (SPKR pin on
> > > ICH9) is sampled high on PWROK.
> > > "
> > >
> > > So it should be set only when '-global ICH9-LPC.noreboot=true' and cleared
> > > when it's false (which should be default).
> > >
> > > Fix it to behave according to spec and set 'No Reboot' bit only when
> > > '-global ICH9-LPC.noreboot=true'.
> >
> > Is there a real-world problem you hit that is being solved by
> > this change, or is it just a theoretical spec compliance fix ?
>
> I've stumbled upon it when implementing ACPI watchdog POC
>
> https://gitlab.com/imammedo/qemu/-/commits/wadt_poc
> I'm not sure that watchdog table belongs to QEMU,
> but the ICH fix definitely is.
I've tested this as follows [1]
$ make-tiny-image.py --kmod lpc_ich --kmod iTCO_wdt --kmod i2c_i801
$ qemu-system-x86_64 \
-kernel /lib/modules/6.15.9-201.fc42.x86_64/vmlinuz \
-initrd tiny-initrd.img \
-append 'console=ttyS0 quiet' \
-m 1000 \
-display none \
-serial stdio \
-accel kvm \
-M q35 \
-global ICH9-LPC.noreboot=false \
-watchdog-action poweroff \
-trace ich9* -trace tco*
ich9_cc_read addr=0x3410 val=0x0 len=4
ich9_cc_write addr=0x3410 val=0x0 len=4
ich9_cc_read addr=0x3410 val=0x0 len=4
tco_io_write addr=0x4 val=0x8
tco_io_write addr=0x6 val=0x2
tco_io_write addr=0x6 val=0x4
tco_io_read addr=0x8 val=0x0
tco_io_read addr=0x12 val=0x4
tco_io_write addr=0x12 val=0x32
tco_io_read addr=0x12 val=0x32
tco_io_write addr=0x0 val=0x1
tco_timer_reload ticks=50 (30000 ms)
~ # mknod /dev/watchdog0 c 10 130
~ # cat /dev/watchdog0
tco_io_write addr=0x0 val=0x1
tco_timer_reload ticks=50 (30000 ms)
cat: read error: Invalid argument
[ 5.646147] watchdog: watchdog0: watchdog did not stop!
tco_io_write addr=0x0 val=0x1
tco_timer_reload ticks=50 (30000 ms)
~ # tco_timer_expired timeouts_no=0 no_reboot=0/0
tco_timer_reload ticks=50 (30000 ms)
tco_timer_expired timeouts_no=1 no_reboot=0/0
And the same, but with ICH9-LPC.noreboot=true.
I see no functional change from Linux guest POV in either
scenario before/after this patch, so
Tested-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
With regards,
Daniel
[1] https://gitlab.com/berrange/tiny-vm-tools/-/blob/master/make-tiny-image.py
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-09-23 15:25 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-09-22 13:26 [PATCH] x86: ich9: fix default value of 'No Reboot' bit in GCS Igor Mammedov
2025-09-22 14:24 ` Daniel P. Berrangé
2025-09-23 8:40 ` Igor Mammedov
2025-09-23 15:24 ` Daniel P. Berrangé
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).