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

[Laszlo Ersek]

On 04/05/17 14:44, Paolo Bonzini wrote:
> 
> 
> 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

Yes, I did track it back to SERR#/IOCHK#.

What wasn't obvious to me why the TCO watchdog couldn't activate SERR#.

I guess I missed the "SERR#" explanation in Table-2.6 "PCI Interface
Signals", where it is described as

  System Error: SERR# can be pulsed active by any PCI device that
  detects a system error condition. Upon sampling SERR# active, the
  ICH9 has the ability to generate an NMI, SMI#, or interrupt.

... Well, I still don't see why the watchdog cannot activate SERR#. Why
can't it? :)

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

Ah okay, I genuinely missed that the watchdog actions are handled
separately in QEMU.

Where is the TCO watchdog hooked up to the general watchdog stuff (like,
to emitting a WATCHDOG event)?

... I see it now. It's the watchdog_perform_action() call in
tco_timer_expired(). And, it is indeed handled separately from the
interrupt injection.

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

You mean "by applying is patch, and then not setting
ICH9_PMIO_SMI_EN_TCO_EN in the guest"?

> (and pass "-global ICH9-LPC.noreboot=false"

This was confusing, but I see it un-gates the very
watchdog_perform_action() call that I just found above, via
(!lpc->pin_strap.spkr_hi). I also had to read commit 5add35bec1e2
("ich9: implement strap SPKR pin logic", 2015-06-28).

> so
> that the watchdog does reboot the machine), everything works without
> ugly messages.

Thanks for the explanation. I now wonder why Paulo decided to add the
NMI in the first place -- was he too confused by the SERR# definition?

> 
>> (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.

I understood that, yes. "Documentation/sysctl/kernel.txt" -- unlike the
other text files under Documentation/ that mention "nmi_watchdog" --
made it clear. My point was rather that we could follow this
(independent) example.

Namely, for whatever reason it was wrong to raise an NMI in a KVM guest
through those performance counters, the same reason would be enough to
not raise an NMI through the iTCO_wdt driver either.

If the culprit is the device model in QEMU though, then I agree the
driver should not be blacklisted.

So why is SERR# activation off-limits for the TCO watchdog? I think if I
can understand that, then I'll see that this patch is correct too.

Thanks,
Laszlo


> 
> 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 
>>> */
>>>  
>>>
>>