[PATCH] x86: ich9: fix default value of 'No Reboot' bit in GCS

[PATCH] x86: ich9: fix default value of 'No Reboot' bit in GCS

[Igor Mammedov]

[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

Re: [PATCH] x86: ich9: fix default value of 'No Reboot' bit in GCS

[Daniel P. Berrangé]

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 :|

Re: [PATCH] x86: ich9: fix default value of 'No Reboot' bit in GCS

[Igor Mammedov]

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

Re: [PATCH] x86: ich9: fix default value of 'No Reboot' bit in GCS

[Daniel P. Berrangé]

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 :|