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

[Igor Mammedov <imammedo@redhat.com>]

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