Re: [Qemu-devel] [PATCH for-2.9] tco: do not generate an NMI

[Paolo Bonzini]

On 05/04/2017 11:30, Laszlo Ersek wrote:
> CC Paulo
> 
> On 04/05/17 10:12, Paolo Bonzini wrote:
>> This behavior is not indicated in the datasheet and can confuse the OS.
> 
> * The message of commit 920557971b60 ("ich9: add TCO interface
> emulation", 2015-06-28) says
> 
>> ... This patch adds support to TCO watchdog logic and few other
>> features like mapping NMIs to SMIs (NMI2SMI_EN bit), system intruder
>> detection, etc. are not implemented yet.
> 
> (Terrible punctuation -- "and" should have been replaced by a full stop
> -- but the intent is clear.)
> 
> Looking at the ICH9 spec,
> 
> - TCO1_STS has a bit called NMI2SMI_STS, where value 1 means "Set by the
> ICH9 when an SMI# occurs because an event occurred that would
> otherwise have caused an NMI (because NMI2SMI_EN is set)."
> 
> - TCO1_CNT contains the bit called NMI2SMI_EN, where value 0 corresponds
> to "Normal NMI functionality."
> 
> These do appear to suggest that the normal timeout action for the
> watchdog is to inject and NMI, don't they?

They do, but that's not the normal timeout action.  There is evidence
through the ICH9 spec, but you need multiple clues to understand what's
really going on.

If you look at Table 5-28 (Causes of SMI# and SCI), you have the
following entries:

TCO SMI Logic
                enabled by TCO_EN=1, reported in TCO_STS
TCO SMI — TCO TIMEROUT
                no additional enables, reported in TIMEOUT
TCO SMI — NMI occurred (and NMIs mapped to SMI)
                enabled by NMI2SMI_EN=1, reported in NMI2SMI_STS


The NMI2SMI_STS bit in TCO1_STS is read-only (unlike TIMEOUT and others)
and it says (13.9.4):

0 = Cleared by clearing the associated NMI status bit.
1 = Set by the ICH9 when an SMI# occurs because an event occurred that
        would otherwise have caused an NMI (because NMI2SMI_EN is set).


"Otherwise" is the key word here, and is confirmed by two other parts of
the manual.  In 13.9.6, NMI2SMI_EN is described like this:

0 = Normal NMI functionality.
1 = Forces all NMIs to instead cause SMIs.


In Table 5-23, the following NMI sources are listed:

- SERR# goes active (either internally, externally via SERR# signal, or
via message from (G)MCH)

- IOCHK# goes active via SERIRQ# stream (ISA system Error)

Ignoring the acronym soup, the right column is interesting: "Can instead
be routed to generate an SCI, through the NMI2SCI_EN bit (Device
31:Function 0, TCO Base + 08h, bit 11)".  There is _no_ NMI2SCI_EN bit;
that bit is "TCO Timer Halt" while NMI2SMI_EN is bit 9 in TCOBASE+08h.

The typo has been propagated in pretty much every Intel southbridge
manual including ICH2 (year 2000) and X79 (aka Patsburg, year 2013), but
a plausible way to read it is "Can instead be routed to generate an SMI,
through the NMI2SMI_EN bit (Device 31:Function 0, TCO Base + 08h, bit 9)".

So if you put all these together:

- NMI2SMI_EN refers to system error NMIs (SERR#/IOCHK#).  The SERR/IOCHK
status bits in port 61h (ICH9 datasheet 13.7.1) are the "associated NMI
status bits" mentioned in the documentation for NMI2SMI_STS

- both watchdog SMIs and "mapped NMI" SMIs are enabled by TCO_EN

- even if firmware needed TCO SMIs, it would not necessarily prevent the
operating system from using the TCO watchdog.  As long as the firmware's
TCO SMI handler does not touch TCO_RLD, the extra SMI is not an issue
(of course, in the real world many firmwares do).

> * Does this patch keep "tests/tco-test.c" in working shape?

Yes, tco-test only tests the registers and the watchdog actions, not the
interrupts.

> * Would blacklisting the iTCO_wdt module in the guest be a suitable
> counter-measure? Should OS installers be modified to blacklist
> "iTCO_wdt" by default if they detect a KVM guest?

No, on the contrary I'd like to use iTCO_wdt :) but right now the
unexpected NMI causes a "dazed and confused" message on Linux.  If I
disable both NMI and SMI (and pass "-global ICH9-LPC.noreboot=false" so
that the watchdog does reboot the machine), everything works without
ugly messages.

> (Idea taken from "Documentation/sysctl/kernel.txt", near "nmi_watchdog"
> -- "The NMI watchdog is disabled by default if the kernel is running as
> a guest in a KVM virtual machine.")

The Linux NMI watchdog is something else, it is obtained by programming
a hardware performance counter on every processor.  On Linux,
performance counters raise NMIs.

Paolo

>>
>> Signed-off-by: Paolo Bonzini <address@hidden>
>> ---
>>  hw/acpi/tco.c          | 2 --
>>  hw/isa/lpc_ich9.c      | 5 -----
>>  include/hw/i386/ich9.h | 1 -
>>  3 files changed, 8 deletions(-)
>>
>> diff --git a/hw/acpi/tco.c b/hw/acpi/tco.c
>> index b4adac8..05b9d7b 100644
>> --- a/hw/acpi/tco.c
>> +++ b/hw/acpi/tco.c
>> @@ -75,8 +75,6 @@ static void tco_timer_expired(void *opaque)
>>  
>>      if (pm->smi_en & ICH9_PMIO_SMI_EN_TCO_EN) {
>>          ich9_generate_smi();
>> -    } else {
>> -        ich9_generate_nmi();
>>      }
>>      tr->tco.rld = tr->tco.tmr;
>>      tco_timer_reload(tr);
>> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
>> index 59930dd..a0866c3 100644
>> --- a/hw/isa/lpc_ich9.c
>> +++ b/hw/isa/lpc_ich9.c
>> @@ -312,11 +312,6 @@ void ich9_generate_smi(void)
>>      cpu_interrupt(first_cpu, CPU_INTERRUPT_SMI);
>>  }
>>  
>> -void ich9_generate_nmi(void)
>> -{
>> -    cpu_interrupt(first_cpu, CPU_INTERRUPT_NMI);
>> -}
>> -
>>  static int ich9_lpc_sci_irq(ICH9LPCState *lpc)
>>  {
>>      switch (lpc->d.config[ICH9_LPC_ACPI_CTRL] &
>> diff --git a/include/hw/i386/ich9.h b/include/hw/i386/ich9.h
>> index 18dcca7..673d13d 100644
>> --- a/include/hw/i386/ich9.h
>> +++ b/include/hw/i386/ich9.h
>> @@ -21,7 +21,6 @@ void ich9_lpc_pm_init(PCIDevice *pci_lpc, bool 
>> smm_enabled);
>>  I2CBus *ich9_smb_init(PCIBus *bus, int devfn, uint32_t smb_io_base);
>>  
>>  void ich9_generate_smi(void);
>> -void ich9_generate_nmi(void);
>>  
>>  #define ICH9_CC_SIZE (16 * 1024) /* 16KB. Chipset configuration registers */
>>  
>>
>