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

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

[Paolo Bonzini]

This behavior is not indicated in the datasheet and can confuse the OS.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 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 */
 
-- 
2.9.3

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

[Laszlo Ersek]

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?

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

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

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

Thanks,
Laszlo

> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  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 */
>  
> 

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 <pbonzini@redhat.com>
>> ---
>>  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 */
>>  
>>
> 

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 <pbonzini@redhat.com>
>>> ---
>>>  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 */
>>>  
>>>
>>

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

[Paolo Bonzini]

On 05/04/2017 15:35, Laszlo Ersek wrote:
> 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? :)

Because that's not what SERR# is for? :)  And especially because the
effect of a watchdog timeout is described elsewhere without including
SERR# in the picture.

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

Both Paulo and Paolo were confused. :)

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

I think the reason is not that it's wrong; it's that by default KVM
guests do not have performance counters at all.

Paolo

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

[Laszlo Ersek]

On 04/05/17 15:49, Paolo Bonzini wrote:
> 
> 
> On 05/04/2017 15:35, Laszlo Ersek wrote:
>> 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? :)
> 
> Because that's not what SERR# is for? :)  And especially because the
> effect of a watchdog timeout is described elsewhere without including
> SERR# in the picture.

That's a valid argument.

> 
>>> 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?
> 
> Both Paulo and Paolo were confused. :)
> 
>>>> (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.
>>
>> 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.
> 
> I think the reason is not that it's wrong; it's that by default KVM
> guests do not have performance counters at all.

Makes sense.

Thank you for taking the time to explain it all.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Cheers!
Laszlo