[Qemu-devel] [PATCH] rtc: introduce nmi disable bit handler for cmos

[Qemu-devel] [PATCH] rtc: introduce nmi disable bit handler for cmos

[Gonglei]

The Non-Maskable Interrupt (NMI) Enable bit is 0x80 bit of
Port 0x70 (and its aliases). This bit must be 0b to enable
the hardware chipset to send a Non-Maskable Interrupt. When
set to a 1b, NMI's are disabled. This bit is commonly accessed
by applications, BIOS, and even the operating system since it is
used to block NMI assertions when sensitive code is executing.

Currently, QEMU do no not handle the bit, means Qemu cannot
block NMI occur, sometimes maybe cause a race between the CMOS
read/write and the NMI handler. If you are setting the CMOS clock
or reading CMOS RAM and an NMI occurs, Bad values could be written
to or read from the CMOS RAM, or the NMI operation might not
occur correctly.

This patch introduce nmi disable bit handler to fix the problem
and make the emulated CMOS like the real hardware.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 Please refer to:
   https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg00616.html
 
 Note: We can't reproduce the problem, what a pity :(
 I holp the patch can fix it. Please review, thanks!
---
 hw/i386/kvm/apic.c                  |  4 +++-
 hw/timer/mc146818rtc.c              | 11 +++++++++++
 include/hw/timer/mc146818rtc_regs.h |  3 +++
 include/sysemu/sysemu.h             |  1 +
 target-i386/kvm.c                   |  4 ++--
 vl.c                                |  1 +
 6 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c
index 5b47056..deea49f 100644
--- a/hw/i386/kvm/apic.c
+++ b/hw/i386/kvm/apic.c
@@ -12,6 +12,7 @@
 #include "hw/i386/apic_internal.h"
 #include "hw/pci/msi.h"
 #include "sysemu/kvm.h"
+#include "sysemu/sysemu.h"
 
 static inline void kvm_apic_set_reg(struct kvm_lapic_state *kapic,
                                     int reg_id, uint32_t val)
@@ -132,7 +133,8 @@ static void do_inject_external_nmi(void *data)
     cpu_synchronize_state(cpu);
 
     lvt = s->lvt[APIC_LVT_LINT1];
-    if (!(lvt & APIC_LVT_MASKED) && ((lvt >> 8) & 7) == APIC_DM_NMI) {
+    if (!nmi_disabled && (lvt & APIC_LVT_MASKED)
+        && ((lvt >> 8) & 7) == APIC_DM_NMI) {
         ret = kvm_vcpu_ioctl(cpu, KVM_NMI);
         if (ret < 0) {
             fprintf(stderr, "KVM: injection failed, NMI lost (%s)\n",
diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index a9f0efd..aaa8b4e 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -389,6 +389,17 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
     RTCState *s = opaque;
 
     if ((addr & 1) == 0) {
+        /*
+         * The Non-Maskable Interrupt (NMI) Enable bit is 0x80 bit of
+         * Port 0x70 (and its aliases). This bit must be 0b to enable
+         * the hardware chipset to send a Non-Maskable Interrupt. When
+         * set to a 1b, NMI's are disabled. This bit is commonly accessed
+         * by applications, BIOS, and even the operating system since it is
+         * used to block NMI assertions when sensitive code is executing.
+         */
+        nmi_disabled = !!(data & NMI_DISABLE_BIT);
+        CMOS_DPRINTF("cmos: nmi_disabled=%s\n",
+                     nmi_disabled ? "true" : "false");
         s->cmos_index = data & 0x7f;
     } else {
         CMOS_DPRINTF("cmos: write index=0x%02x val=0x%02" PRIx64 "\n",
diff --git a/include/hw/timer/mc146818rtc_regs.h b/include/hw/timer/mc146818rtc_regs.h
index ccdee42..175249f 100644
--- a/include/hw/timer/mc146818rtc_regs.h
+++ b/include/hw/timer/mc146818rtc_regs.h
@@ -64,4 +64,7 @@
 #define REG_C_AF   0x20
 #define REG_C_MASK 0x70
 
+/* PORT_CMOS_INDEX nmi disable bit */
+#define NMI_DISABLE_BIT 0x80
+
 #endif
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 3bb8897..a5b2342 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -177,6 +177,7 @@ extern uint8_t qemu_extra_params_fw[2];
 extern QEMUClockType rtc_clock;
 extern const char *mem_path;
 extern int mem_prealloc;
+extern bool nmi_disabled;
 
 #define MAX_NODES 128
 #define NUMA_NODE_UNASSIGNED MAX_NODES
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 6dc9846..abbd65b 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -2456,9 +2456,9 @@ void kvm_arch_pre_run(CPUState *cpu, struct kvm_run *run)
     CPUX86State *env = &x86_cpu->env;
     int ret;
 
-    /* Inject NMI */
+    /* Inject NMI Or SMI */
     if (cpu->interrupt_request & (CPU_INTERRUPT_NMI | CPU_INTERRUPT_SMI)) {
-        if (cpu->interrupt_request & CPU_INTERRUPT_NMI) {
+        if (!nmi_disabled && (cpu->interrupt_request & CPU_INTERRUPT_NMI)) {
             qemu_mutex_lock_iothread();
             cpu->interrupt_request &= ~CPU_INTERRUPT_NMI;
             qemu_mutex_unlock_iothread();
diff --git a/vl.c b/vl.c
index 4211ff1..ff5b06f 100644
--- a/vl.c
+++ b/vl.c
@@ -185,6 +185,7 @@ bool boot_strict;
 uint8_t *boot_splash_filedata;
 size_t boot_splash_filedata_size;
 uint8_t qemu_extra_params_fw[2];
+bool nmi_disabled;
 
 int icount_align_option;
 
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH] rtc: introduce nmi disable bit handler for cmos

[Paolo Bonzini]

On 12/12/2015 14:15, Gonglei wrote:
> The Non-Maskable Interrupt (NMI) Enable bit is 0x80 bit of
> Port 0x70 (and its aliases). This bit must be 0b to enable
> the hardware chipset to send a Non-Maskable Interrupt. When
> set to a 1b, NMI's are disabled. This bit is commonly accessed
> by applications, BIOS, and even the operating system since it is
> used to block NMI assertions when sensitive code is executing.
> 
> Currently, QEMU do no not handle the bit, means Qemu cannot
> block NMI occur, sometimes maybe cause a race between the CMOS
> read/write and the NMI handler. If you are setting the CMOS clock
> or reading CMOS RAM and an NMI occurs, Bad values could be written
> to or read from the CMOS RAM, or the NMI operation might not
> occur correctly.
> 
> This patch introduce nmi disable bit handler to fix the problem
> and make the emulated CMOS like the real hardware.

I think that this only works with -machine kernel_irqchip=off, however.
 You would have to add a new bit to struct kvm_vcpu_events, which could
for example replace nmi.pad.

>  Please refer to:
>    https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg00616.html
>  
>  Note: We can't reproduce the problem, what a pity :(
>  I holp the patch can fix it. Please review, thanks!

The effect of the patch could be tested with kvm-unit-tests.

Thanks,

Paolo

>  hw/i386/kvm/apic.c                  |  4 +++-
>  hw/timer/mc146818rtc.c              | 11 +++++++++++
>  include/hw/timer/mc146818rtc_regs.h |  3 +++
>  include/sysemu/sysemu.h             |  1 +
>  target-i386/kvm.c                   |  4 ++--
>  vl.c                                |  1 +
>  6 files changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c
> index 5b47056..deea49f 100644
> --- a/hw/i386/kvm/apic.c
> +++ b/hw/i386/kvm/apic.c
> @@ -12,6 +12,7 @@
>  #include "hw/i386/apic_internal.h"
>  #include "hw/pci/msi.h"
>  #include "sysemu/kvm.h"
> +#include "sysemu/sysemu.h"
>  
>  static inline void kvm_apic_set_reg(struct kvm_lapic_state *kapic,
>                                      int reg_id, uint32_t val)
> @@ -132,7 +133,8 @@ static void do_inject_external_nmi(void *data)
>      cpu_synchronize_state(cpu);
>  
>      lvt = s->lvt[APIC_LVT_LINT1];
> -    if (!(lvt & APIC_LVT_MASKED) && ((lvt >> 8) & 7) == APIC_DM_NMI) {
> +    if (!nmi_disabled && (lvt & APIC_LVT_MASKED)
> +        && ((lvt >> 8) & 7) == APIC_DM_NMI) {
>          ret = kvm_vcpu_ioctl(cpu, KVM_NMI);
>          if (ret < 0) {
>              fprintf(stderr, "KVM: injection failed, NMI lost (%s)\n",
> diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
> index a9f0efd..aaa8b4e 100644
> --- a/hw/timer/mc146818rtc.c
> +++ b/hw/timer/mc146818rtc.c
> @@ -389,6 +389,17 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
>      RTCState *s = opaque;
>  
>      if ((addr & 1) == 0) {
> +        /*
> +         * The Non-Maskable Interrupt (NMI) Enable bit is 0x80 bit of
> +         * Port 0x70 (and its aliases). This bit must be 0b to enable
> +         * the hardware chipset to send a Non-Maskable Interrupt. When
> +         * set to a 1b, NMI's are disabled. This bit is commonly accessed
> +         * by applications, BIOS, and even the operating system since it is
> +         * used to block NMI assertions when sensitive code is executing.
> +         */
> +        nmi_disabled = !!(data & NMI_DISABLE_BIT);
> +        CMOS_DPRINTF("cmos: nmi_disabled=%s\n",
> +                     nmi_disabled ? "true" : "false");
>          s->cmos_index = data & 0x7f;
>      } else {
>          CMOS_DPRINTF("cmos: write index=0x%02x val=0x%02" PRIx64 "\n",
> diff --git a/include/hw/timer/mc146818rtc_regs.h b/include/hw/timer/mc146818rtc_regs.h
> index ccdee42..175249f 100644
> --- a/include/hw/timer/mc146818rtc_regs.h
> +++ b/include/hw/timer/mc146818rtc_regs.h
> @@ -64,4 +64,7 @@
>  #define REG_C_AF   0x20
>  #define REG_C_MASK 0x70
>  
> +/* PORT_CMOS_INDEX nmi disable bit */
> +#define NMI_DISABLE_BIT 0x80
> +
>  #endif
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 3bb8897..a5b2342 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -177,6 +177,7 @@ extern uint8_t qemu_extra_params_fw[2];
>  extern QEMUClockType rtc_clock;
>  extern const char *mem_path;
>  extern int mem_prealloc;
> +extern bool nmi_disabled;
>  
>  #define MAX_NODES 128
>  #define NUMA_NODE_UNASSIGNED MAX_NODES
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 6dc9846..abbd65b 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -2456,9 +2456,9 @@ void kvm_arch_pre_run(CPUState *cpu, struct kvm_run *run)
>      CPUX86State *env = &x86_cpu->env;
>      int ret;
>  
> -    /* Inject NMI */
> +    /* Inject NMI Or SMI */
>      if (cpu->interrupt_request & (CPU_INTERRUPT_NMI | CPU_INTERRUPT_SMI)) {
> -        if (cpu->interrupt_request & CPU_INTERRUPT_NMI) {
> +        if (!nmi_disabled && (cpu->interrupt_request & CPU_INTERRUPT_NMI)) {
>              qemu_mutex_lock_iothread();
>              cpu->interrupt_request &= ~CPU_INTERRUPT_NMI;
>              qemu_mutex_unlock_iothread();
> diff --git a/vl.c b/vl.c
> index 4211ff1..ff5b06f 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -185,6 +185,7 @@ bool boot_strict;
>  uint8_t *boot_splash_filedata;
>  size_t boot_splash_filedata_size;
>  uint8_t qemu_extra_params_fw[2];
> +bool nmi_disabled;
>  
>  int icount_align_option;
>  
> 

Re: [Qemu-devel] [PATCH] rtc: introduce nmi disable bit handler for cmos

[Gonglei (Arei)]

Hi Paolo,

Thanks for your comments firstly.

> Subject: Re: [PATCH] rtc: introduce nmi disable bit handler for cmos
> 
> 
> 
> On 12/12/2015 14:15, Gonglei wrote:
> > The Non-Maskable Interrupt (NMI) Enable bit is 0x80 bit of Port 0x70
> > (and its aliases). This bit must be 0b to enable the hardware chipset
> > to send a Non-Maskable Interrupt. When set to a 1b, NMI's are
> > disabled. This bit is commonly accessed by applications, BIOS, and
> > even the operating system since it is used to block NMI assertions
> > when sensitive code is executing.
> >
> > Currently, QEMU do no not handle the bit, means Qemu cannot block NMI
> > occur, sometimes maybe cause a race between the CMOS read/write and
> > the NMI handler. If you are setting the CMOS clock or reading CMOS RAM
> > and an NMI occurs, Bad values could be written to or read from the
> > CMOS RAM, or the NMI operation might not occur correctly.
> >
> > This patch introduce nmi disable bit handler to fix the problem and
> > make the emulated CMOS like the real hardware.
> 
> I think that this only works with -machine kernel_irqchip=off, however.

IIRCC, the kernel_irqchip is disabled by default, and we used the default value.

>  You would have to add a new bit to struct kvm_vcpu_events, which could for
> example replace nmi.pad.
> 
You mean we should keep the value of nmi_disabled when we want to live migration?
Or do you have other meaning? I'm not familiar with this, sorry about that. :(

> >  Please refer to:
> >    https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg00616.html
> >
> >  Note: We can't reproduce the problem, what a pity :(  I holp the
> > patch can fix it. Please review, thanks!
> 
> The effect of the patch could be tested with kvm-unit-tests.
> 

I'll test this version with kvm-unit-tests.



Regards,
-Gonglei

> Thanks,
> 
> Paolo
> 
> >  hw/i386/kvm/apic.c                  |  4 +++-
> >  hw/timer/mc146818rtc.c              | 11 +++++++++++
> >  include/hw/timer/mc146818rtc_regs.h |  3 +++
> >  include/sysemu/sysemu.h             |  1 +
> >  target-i386/kvm.c                   |  4 ++--
> >  vl.c                                |  1 +
> >  6 files changed, 21 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c index
> > 5b47056..deea49f 100644
> > --- a/hw/i386/kvm/apic.c
> > +++ b/hw/i386/kvm/apic.c
> > @@ -12,6 +12,7 @@
> >  #include "hw/i386/apic_internal.h"
> >  #include "hw/pci/msi.h"
> >  #include "sysemu/kvm.h"
> > +#include "sysemu/sysemu.h"
> >
> >  static inline void kvm_apic_set_reg(struct kvm_lapic_state *kapic,
> >                                      int reg_id, uint32_t val) @@
> > -132,7 +133,8 @@ static void do_inject_external_nmi(void *data)
> >      cpu_synchronize_state(cpu);
> >
> >      lvt = s->lvt[APIC_LVT_LINT1];
> > -    if (!(lvt & APIC_LVT_MASKED) && ((lvt >> 8) & 7) == APIC_DM_NMI) {
> > +    if (!nmi_disabled && (lvt & APIC_LVT_MASKED)
> > +        && ((lvt >> 8) & 7) == APIC_DM_NMI) {
> >          ret = kvm_vcpu_ioctl(cpu, KVM_NMI);
> >          if (ret < 0) {
> >              fprintf(stderr, "KVM: injection failed, NMI lost (%s)\n",
> > diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c index
> > a9f0efd..aaa8b4e 100644
> > --- a/hw/timer/mc146818rtc.c
> > +++ b/hw/timer/mc146818rtc.c
> > @@ -389,6 +389,17 @@ static void cmos_ioport_write(void *opaque,
> hwaddr addr,
> >      RTCState *s = opaque;
> >
> >      if ((addr & 1) == 0) {
> > +        /*
> > +         * The Non-Maskable Interrupt (NMI) Enable bit is 0x80 bit of
> > +         * Port 0x70 (and its aliases). This bit must be 0b to enable
> > +         * the hardware chipset to send a Non-Maskable Interrupt. When
> > +         * set to a 1b, NMI's are disabled. This bit is commonly accessed
> > +         * by applications, BIOS, and even the operating system since it is
> > +         * used to block NMI assertions when sensitive code is executing.
> > +         */
> > +        nmi_disabled = !!(data & NMI_DISABLE_BIT);
> > +        CMOS_DPRINTF("cmos: nmi_disabled=%s\n",
> > +                     nmi_disabled ? "true" : "false");
> >          s->cmos_index = data & 0x7f;
> >      } else {
> >          CMOS_DPRINTF("cmos: write index=0x%02x val=0x%02" PRIx64
> > "\n", diff --git a/include/hw/timer/mc146818rtc_regs.h
> > b/include/hw/timer/mc146818rtc_regs.h
> > index ccdee42..175249f 100644
> > --- a/include/hw/timer/mc146818rtc_regs.h
> > +++ b/include/hw/timer/mc146818rtc_regs.h
> > @@ -64,4 +64,7 @@
> >  #define REG_C_AF   0x20
> >  #define REG_C_MASK 0x70
> >
> > +/* PORT_CMOS_INDEX nmi disable bit */ #define NMI_DISABLE_BIT 0x80
> > +
> >  #endif
> > diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h index
> > 3bb8897..a5b2342 100644
> > --- a/include/sysemu/sysemu.h
> > +++ b/include/sysemu/sysemu.h
> > @@ -177,6 +177,7 @@ extern uint8_t qemu_extra_params_fw[2];  extern
> > QEMUClockType rtc_clock;  extern const char *mem_path;  extern int
> > mem_prealloc;
> > +extern bool nmi_disabled;
> >
> >  #define MAX_NODES 128
> >  #define NUMA_NODE_UNASSIGNED MAX_NODES diff --git
> a/target-i386/kvm.c
> > b/target-i386/kvm.c index 6dc9846..abbd65b 100644
> > --- a/target-i386/kvm.c
> > +++ b/target-i386/kvm.c
> > @@ -2456,9 +2456,9 @@ void kvm_arch_pre_run(CPUState *cpu, struct
> kvm_run *run)
> >      CPUX86State *env = &x86_cpu->env;
> >      int ret;
> >
> > -    /* Inject NMI */
> > +    /* Inject NMI Or SMI */
> >      if (cpu->interrupt_request & (CPU_INTERRUPT_NMI |
> CPU_INTERRUPT_SMI)) {
> > -        if (cpu->interrupt_request & CPU_INTERRUPT_NMI) {
> > +        if (!nmi_disabled && (cpu->interrupt_request &
> > + CPU_INTERRUPT_NMI)) {
> >              qemu_mutex_lock_iothread();
> >              cpu->interrupt_request &= ~CPU_INTERRUPT_NMI;
> >              qemu_mutex_unlock_iothread(); diff --git a/vl.c b/vl.c
> > index 4211ff1..ff5b06f 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -185,6 +185,7 @@ bool boot_strict;
> >  uint8_t *boot_splash_filedata;
> >  size_t boot_splash_filedata_size;
> >  uint8_t qemu_extra_params_fw[2];
> > +bool nmi_disabled;
> >
> >  int icount_align_option;
> >
> >

Re: [Qemu-devel] [PATCH] rtc: introduce nmi disable bit handler for cmos

[Paolo Bonzini]

On 14/12/2015 13:49, Gonglei (Arei) wrote:
>>> > > This patch introduce nmi disable bit handler to fix the problem and
>>> > > make the emulated CMOS like the real hardware.
>> > 
>> > I think that this only works with -machine kernel_irqchip=off, however.
> IIRCC, the kernel_irqchip is disabled by default, and we used the default value.

No, it's enabled by default.

> >  You would have to add a new bit to struct kvm_vcpu_events, which could for
> > example replace nmi.pad.
> 
> You mean we should keep the value of nmi_disabled when we want to live migration?

Yes.  It can also be used to communicate the enabling/disabling of NMIs
when the RTC is written.

> > >  Please refer to:
> > >    https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg00616.html
> > >
> > >  Note: We can't reproduce the problem, what a pity :(  I holp the
> > > patch can fix it. Please review, thanks!
> > 
> > The effect of the patch could be tested with kvm-unit-tests.
> 
> I'll test this version with kvm-unit-tests.

Note that you'll have to write a new test. :)

Paolo

Re: [Qemu-devel] [PATCH] rtc: introduce nmi disable bit handler for cmos

[Gonglei (Arei)]

> 
> On 14/12/2015 13:49, Gonglei (Arei) wrote:
> >>> > > This patch introduce nmi disable bit handler to fix the problem
> >>> > > and make the emulated CMOS like the real hardware.
> >> >
> >> > I think that this only works with -machine kernel_irqchip=off, however.
> > IIRCC, the kernel_irqchip is disabled by default, and we used the default
> value.
> 
> No, it's enabled by default.
> 

Okay, yes, I saw the source code again. That means kmod finish the NMI injection
wrok, and the NMI will not pass Qemu side. So, you thought this patch cannot block
NMI injection when kernel_irqchip=on ?

Maybe we should pass the nmi_disable bit to Kmod when kernel_irqchip=on , right?

Regards,
-Gonglei

> > >  You would have to add a new bit to struct kvm_vcpu_events, which
> > > could for example replace nmi.pad.
> >
> > You mean we should keep the value of nmi_disabled when we want to live
> migration?
> 
> Yes.  It can also be used to communicate the enabling/disabling of NMIs when
> the RTC is written.
> 
> > > >  Please refer to:
> > > >
> > > > https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg00616.htm
> > > > l
> > > >
> > > >  Note: We can't reproduce the problem, what a pity :(  I holp the
> > > > patch can fix it. Please review, thanks!
> > >
> > > The effect of the patch could be tested with kvm-unit-tests.
> >
> > I'll test this version with kvm-unit-tests.
> 
> Note that you'll have to write a new test. :)
> 
> Paolo

Re: [Qemu-devel] [PATCH] rtc: introduce nmi disable bit handler for cmos

[Paolo Bonzini]

On 14/12/2015 14:27, Gonglei (Arei) wrote:
> 
>>
>> On 14/12/2015 13:49, Gonglei (Arei) wrote:
>>>>>>> This patch introduce nmi disable bit handler to fix the problem
>>>>>>> and make the emulated CMOS like the real hardware.
>>>>>
>>>>> I think that this only works with -machine kernel_irqchip=off, however.
>>> IIRCC, the kernel_irqchip is disabled by default, and we used the default
>> value.
>>
>> No, it's enabled by default.
>>
> 
> Okay, yes, I saw the source code again. That means kmod finish the NMI injection
> wrok, and the NMI will not pass Qemu side. So, you thought this patch cannot block
> NMI injection when kernel_irqchip=on ?

I am not sure.  It depends on which NMIs are blocked by the bit.  For
example, the IOAPIC can deliver NMIs, and they wouldn't be blocked.

Do you have any documentation, to see whether they can actually happen
on emulated hardware?  I guess we support the TCO watchdog, so yes.

> Maybe we should pass the nmi_disable bit to Kmod when kernel_irqchip=on , right?

Yes, that's the idea.

But first of all, I've read the thread you linked, and I couldn't find
the place where it says that the root cause is NMIs.

Paolo

Re: [Qemu-devel] [PATCH] rtc: introduce nmi disable bit handler for cmos

[Eduardo Habkost]

On Sat, Dec 12, 2015 at 09:15:46PM +0800, Gonglei wrote:
> The Non-Maskable Interrupt (NMI) Enable bit is 0x80 bit of
> Port 0x70 (and its aliases). This bit must be 0b to enable
> the hardware chipset to send a Non-Maskable Interrupt. When
> set to a 1b, NMI's are disabled. This bit is commonly accessed
> by applications, BIOS, and even the operating system since it is
> used to block NMI assertions when sensitive code is executing.
> 
> Currently, QEMU do no not handle the bit, means Qemu cannot
> block NMI occur, sometimes maybe cause a race between the CMOS
> read/write and the NMI handler. If you are setting the CMOS clock
> or reading CMOS RAM and an NMI occurs, Bad values could be written
> to or read from the CMOS RAM, or the NMI operation might not
> occur correctly.
> 
> This patch introduce nmi disable bit handler to fix the problem
> and make the emulated CMOS like the real hardware.
> 
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  Please refer to:
>    https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg00616.html
>  
>  Note: We can't reproduce the problem, what a pity :(
>  I holp the patch can fix it. Please review, thanks!
> ---
>  hw/i386/kvm/apic.c                  |  4 +++-
>  hw/timer/mc146818rtc.c              | 11 +++++++++++
>  include/hw/timer/mc146818rtc_regs.h |  3 +++
>  include/sysemu/sysemu.h             |  1 +
>  target-i386/kvm.c                   |  4 ++--
>  vl.c                                |  1 +
>  6 files changed, 21 insertions(+), 3 deletions(-)
> 
[...]
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 3bb8897..a5b2342 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -177,6 +177,7 @@ extern uint8_t qemu_extra_params_fw[2];
>  extern QEMUClockType rtc_clock;
>  extern const char *mem_path;
>  extern int mem_prealloc;
> +extern bool nmi_disabled;

Please, not another global variable. Doesn't this belong to
struct RTCState or APICCommonState?

-- 
Eduardo

Re: [Qemu-devel] [PATCH] rtc: introduce nmi disable bit handler for cmos

[Gonglei (Arei)]

> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> Sent: Monday, December 14, 2015 9:37 PM
> On 14/12/2015 14:27, Gonglei (Arei) wrote:
> >
> >>
> >> On 14/12/2015 13:49, Gonglei (Arei) wrote:
> >>>>>>> This patch introduce nmi disable bit handler to fix the problem
> >>>>>>> and make the emulated CMOS like the real hardware.
> >>>>>
> >>>>> I think that this only works with -machine kernel_irqchip=off, however.
> >>> IIRCC, the kernel_irqchip is disabled by default, and we used the
> >>> default
> >> value.
> >>
> >> No, it's enabled by default.
> >>
> >
> > Okay, yes, I saw the source code again. That means kmod finish the NMI
> > injection wrok, and the NMI will not pass Qemu side. So, you thought
> > this patch cannot block NMI injection when kernel_irqchip=on ?
> 
> I am not sure.  It depends on which NMIs are blocked by the bit.  For
> example, the IOAPIC can deliver NMIs, and they wouldn't be blocked.
> 
> Do you have any documentation, to see whether they can actually happen on
> emulated hardware?  I guess we support the TCO watchdog, so yes.
> 
Yes, watchdog is one case, and we have another case which need to use NMI to
tell guest do something when guest's cpu stuck or something like that.
And I can invoke qmp command "inject-nmi" when SeaBIOS try to close NMI by
invoking rtc_read() or rtc_write(). 

After the NMI injection, the guest will reboot:

[2015-12-14 16:41:57] In resume (status=0)
[2015-12-14 16:41:57] In 32bit resume
[2015-12-14 16:41:57] =====Attempting a hard reboot====
[2015-12-14 16:41:58] SeaBIOS (version rel-1.8.1-0-g4adadbd-20151214_135833-linux-jAPTBr)

[snip]

So, I think we should handle those scenarios, just like the real hardware.

> > Maybe we should pass the nmi_disable bit to Kmod when kernel_irqchip=on ,
> right?
> 
> Yes, that's the idea.
> 
That means I have much more work need to do.

> But first of all, I've read the thread you linked, and I couldn't find the place
> where it says that the root cause is NMIs.
> 
That's complete true. I haven't direct proof, but I think I eliminated
all possible causes, except NMIs. Of course, if you find any other clues,
please let me know.

The most trouble thing is I couldn't reproduce this problem. :(

Thanks,
-Gonglei

Re: [Qemu-devel] [PATCH] rtc: introduce nmi disable bit handler for cmos

[Gonglei (Arei)]

> -----Original Message-----
> From: Eduardo Habkost [mailto:ehabkost@redhat.com]
> Sent: Tuesday, December 15, 2015 2:17 AM
> To: Gonglei (Arei)
> Cc: qemu-devel@nongnu.org; pbonzini@redhat.com; rth@twiddle.net;
> kevin@koconnor.net; Huangpeng (Peter)
> Subject: Re: [PATCH] rtc: introduce nmi disable bit handler for cmos
> 
> On Sat, Dec 12, 2015 at 09:15:46PM +0800, Gonglei wrote:
> > The Non-Maskable Interrupt (NMI) Enable bit is 0x80 bit of Port 0x70
> > (and its aliases). This bit must be 0b to enable the hardware chipset
> > to send a Non-Maskable Interrupt. When set to a 1b, NMI's are
> > disabled. This bit is commonly accessed by applications, BIOS, and
> > even the operating system since it is used to block NMI assertions
> > when sensitive code is executing.
> >
> > Currently, QEMU do no not handle the bit, means Qemu cannot block NMI
> > occur, sometimes maybe cause a race between the CMOS read/write and
> > the NMI handler. If you are setting the CMOS clock or reading CMOS RAM
> > and an NMI occurs, Bad values could be written to or read from the
> > CMOS RAM, or the NMI operation might not occur correctly.
> >
> > This patch introduce nmi disable bit handler to fix the problem and
> > make the emulated CMOS like the real hardware.
> >
> > Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> > ---
> >  Please refer to:
> >    https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg00616.html
> >
> >  Note: We can't reproduce the problem, what a pity :(  I holp the
> > patch can fix it. Please review, thanks!
> > ---
> >  hw/i386/kvm/apic.c                  |  4 +++-
> >  hw/timer/mc146818rtc.c              | 11 +++++++++++
> >  include/hw/timer/mc146818rtc_regs.h |  3 +++
> >  include/sysemu/sysemu.h             |  1 +
> >  target-i386/kvm.c                   |  4 ++--
> >  vl.c                                |  1 +
> >  6 files changed, 21 insertions(+), 3 deletions(-)
> >
> [...]
> > diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h index
> > 3bb8897..a5b2342 100644
> > --- a/include/sysemu/sysemu.h
> > +++ b/include/sysemu/sysemu.h
> > @@ -177,6 +177,7 @@ extern uint8_t qemu_extra_params_fw[2];  extern
> > QEMUClockType rtc_clock;  extern const char *mem_path;  extern int
> > mem_prealloc;
> > +extern bool nmi_disabled;
> 
> Please, not another global variable. Doesn't this belong to struct RTCState or
> APICCommonState?
> 
OK, I'll think about changing this in the next version, thanks.


Regards,
-Gonglei

Re: [Qemu-devel] [PATCH] rtc: introduce nmi disable bit handler for cmos

[Gonglei (Arei)]

Hi Paolo,

/* for KVM_GET/SET_VCPU_EVENTS */
struct kvm_vcpu_events {
 ...
struct {
		__u8 injected;
		__u8 pending;
		__u8 masked;
		__u8 pad;
	} nmi;
 ...

I found that the nmi.masked property does these enable or disable NMI jobs. 
So, I think we don't need to add a new bit. Right?

Regards,
-Gonglei


> -----Original Message-----
> From: Gonglei (Arei)
> Sent: Tuesday, December 15, 2015 8:58 AM
> To: 'Paolo Bonzini'; qemu-devel@nongnu.org
> Cc: rth@twiddle.net; ehabkost@redhat.com; kevin@koconnor.net; Huangpeng
> (Peter)
> Subject: RE: [PATCH] rtc: introduce nmi disable bit handler for cmos
> 
> > From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> > Sent: Monday, December 14, 2015 9:37 PM
> > On 14/12/2015 14:27, Gonglei (Arei) wrote:
> > >
> > >>
> > >> On 14/12/2015 13:49, Gonglei (Arei) wrote:
> > >>>>>>> This patch introduce nmi disable bit handler to fix the problem
> > >>>>>>> and make the emulated CMOS like the real hardware.
> > >>>>>
> > >>>>> I think that this only works with -machine kernel_irqchip=off, however.
> > >>> IIRCC, the kernel_irqchip is disabled by default, and we used the
> > >>> default
> > >> value.
> > >>
> > >> No, it's enabled by default.
> > >>
> > >
> > > Okay, yes, I saw the source code again. That means kmod finish the NMI
> > > injection wrok, and the NMI will not pass Qemu side. So, you thought
> > > this patch cannot block NMI injection when kernel_irqchip=on ?
> >
> > I am not sure.  It depends on which NMIs are blocked by the bit.  For
> > example, the IOAPIC can deliver NMIs, and they wouldn't be blocked.
> >
> > Do you have any documentation, to see whether they can actually happen on
> > emulated hardware?  I guess we support the TCO watchdog, so yes.
> >
> Yes, watchdog is one case, and we have another case which need to use NMI
> to
> tell guest do something when guest's cpu stuck or something like that.
> And I can invoke qmp command "inject-nmi" when SeaBIOS try to close NMI by
> invoking rtc_read() or rtc_write().
> 
> After the NMI injection, the guest will reboot:
> 
> [2015-12-14 16:41:57] In resume (status=0)
> [2015-12-14 16:41:57] In 32bit resume
> [2015-12-14 16:41:57] =====Attempting a hard reboot====
> [2015-12-14 16:41:58] SeaBIOS (version
> rel-1.8.1-0-g4adadbd-20151214_135833-linux-jAPTBr)
> 
> [snip]
> 
> So, I think we should handle those scenarios, just like the real hardware.
> 
> > > Maybe we should pass the nmi_disable bit to Kmod when
> kernel_irqchip=on ,
> > right?
> >
> > Yes, that's the idea.
> >
> That means I have much more work need to do.
> 
> > But first of all, I've read the thread you linked, and I couldn't find the place
> > where it says that the root cause is NMIs.
> >
> That's complete true. I haven't direct proof, but I think I eliminated
> all possible causes, except NMIs. Of course, if you find any other clues,
> please let me know.
> 
> The most trouble thing is I couldn't reproduce this problem. :(
> 
> Thanks,
> -Gonglei

Re: [Qemu-devel] [PATCH] rtc: introduce nmi disable bit handler for cmos

[Paolo Bonzini]

> Hi Paolo,
> 
> /* for KVM_GET/SET_VCPU_EVENTS */
> struct kvm_vcpu_events {
>  ...
> struct {
> 		__u8 injected;
> 		__u8 pending;
> 		__u8 masked;
> 		__u8 pad;
> 	} nmi;
>  ...
> 
> I found that the nmi.masked property does these enable or disable NMI jobs.
> So, I think we don't need to add a new bit. Right?

nmi.masked says whether the CPU is accepting the NMIs, and is cleared
by the next IRET instruction.  This is a different thing; it probably
shouldn't affect NMI IPIs, and it definitely should remain set until
cleared via the RTC.  So it should be something like

    _u8 external_nmi_disabled;

or similar.

*However* I found this in the ICH9 datasheet:

    The ICH9's I/O APIC can only send interrupts due to interrupts which
    do not include SMI, NMI or INIT. This means that in IA-32/Intel ® 64
    based platforms, Front Side Bus interrupt message format delivery modes
    010 (SMI/PMI), 100 (NMI), and 101 (INIT) as indicated in this section,
    must not be used and is not supported.

In theory the PIIX4 could deliver such messages, but perhaps we could
disable them in the KVM IOAPIC.  If we do this, there is no need for a
change to struct kvm_vcpu_events, because all external NMI sources will
be in userspace.

Radim, what do you think?

Paolo

Re: [Qemu-devel] [PATCH] rtc: introduce nmi disable bit handler for cmos

[Radim Krcmar]

2015-12-15 05:43-0500, Paolo Bonzini:
>> Hi Paolo,
>> 
>> /* for KVM_GET/SET_VCPU_EVENTS */
>> struct kvm_vcpu_events {
>>  ...
>> struct {
>> 		__u8 injected;
>> 		__u8 pending;
>> 		__u8 masked;
>> 		__u8 pad;
>> 	} nmi;
>>  ...
>> 
>> I found that the nmi.masked property does these enable or disable NMI jobs.
>> So, I think we don't need to add a new bit. Right?
> 
> nmi.masked says whether the CPU is accepting the NMIs, and is cleared
> by the next IRET instruction.  This is a different thing; it probably
> shouldn't affect NMI IPIs, and it definitely should remain set until
> cleared via the RTC.  So it should be something like
> 
>     _u8 external_nmi_disabled;
> 
> or similar.
> 
> *However* I found this in the ICH9 datasheet:
> 
>     The ICH9's I/O APIC can only send interrupts due to interrupts which
>     do not include SMI, NMI or INIT. This means that in IA-32/Intel ® 64
>     based platforms, Front Side Bus interrupt message format delivery modes
>     010 (SMI/PMI), 100 (NMI), and 101 (INIT) as indicated in this section,
>     must not be used and is not supported.
> 
> In theory the PIIX4 could deliver such messages, but perhaps we could
> disable them in the KVM IOAPIC.  If we do this, there is no need for a
> change to struct kvm_vcpu_events, because all external NMI sources will
> be in userspace.
> 
> Radim, what do you think?

I looked at the 440fx, piix, and 82083aa(ioapic) datasheets and the
NMI_EN bit doesn't seem to be propagated into the IOAPIC.
The IOAPIC datasheet doesn't mention a thing about NMI masking and PIIX4
generates NMI on SERR# or IOCHK# so it seems that the NMI_EN feature
only changes the behavior of those two ...

I think it's best to do nothing in KVM.

(q35 guests shouldn't configure IOAPIC to send unsupported messages and
 disabling SMI/NMI/INIT in the in-kernel IOAPIC for piix is risky.)

Re: [Qemu-devel] [PATCH] rtc: introduce nmi disable bit handler for cmos

[Gonglei (Arei)]

Hi,

> -----Original Message-----
> From: Radim Krcmar [mailto:rkrcmar@redhat.com]
> Subject: Re: [PATCH] rtc: introduce nmi disable bit handler for cmos
> 
> 2015-12-15 05:43-0500, Paolo Bonzini:
> >> Hi Paolo,
> >>
> >> /* for KVM_GET/SET_VCPU_EVENTS */
> >> struct kvm_vcpu_events {
> >>  ...
> >> struct {
> >> 		__u8 injected;
> >> 		__u8 pending;
> >> 		__u8 masked;
> >> 		__u8 pad;
> >> 	} nmi;
> >>  ...
> >>
> >> I found that the nmi.masked property does these enable or disable NMI
> jobs.
> >> So, I think we don't need to add a new bit. Right?
> >
> > nmi.masked says whether the CPU is accepting the NMIs, and is cleared
> > by the next IRET instruction.  This is a different thing; it probably
> > shouldn't affect NMI IPIs, and it definitely should remain set until
> > cleared via the RTC.  So it should be something like
> >
> >     _u8 external_nmi_disabled;
> >
> > or similar.
> >
OK, I see, thanks.

> > *However* I found this in the ICH9 datasheet:
> >
> >     The ICH9's I/O APIC can only send interrupts due to interrupts which
> >     do not include SMI, NMI or INIT. This means that in IA-32/Intel ® 64
> >     based platforms, Front Side Bus interrupt message format delivery
> modes
> >     010 (SMI/PMI), 100 (NMI), and 101 (INIT) as indicated in this section,
> >     must not be used and is not supported.
> >
> > In theory the PIIX4 could deliver such messages, but perhaps we could
> > disable them in the KVM IOAPIC.  If we do this, there is no need for a
> > change to struct kvm_vcpu_events, because all external NMI sources will
> > be in userspace.
> >
> > Radim, what do you think?
> 
> I looked at the 440fx, piix, and 82083aa(ioapic) datasheets and the
> NMI_EN bit doesn't seem to be propagated into the IOAPIC.
> The IOAPIC datasheet doesn't mention a thing about NMI masking and PIIX4
> generates NMI on SERR# or IOCHK# so it seems that the NMI_EN feature
> only changes the behavior of those two ...
> 
Currently, Qemu doesn't handle SERR# (bit 7) and IOCHK# (bit 6) of port 0x61. The
Linux kernel gets the nmi reason when invoke default_do_nmi(), Qemu always
return 0.

Regards,
-Gonglei

> I think it's best to do nothing in KVM.
> 
> (q35 guests shouldn't configure IOAPIC to send unsupported messages and
>  disabling SMI/NMI/INIT in the in-kernel IOAPIC for piix is risky.)

Re: [Qemu-devel] [PATCH] rtc: introduce nmi disable bit handler for cmos

[Paolo Bonzini]

On 15/12/2015 19:53, Radim Krcmar wrote:
> 2015-12-15 05:43-0500, Paolo Bonzini:
>>> Hi Paolo,
>>>
>>> /* for KVM_GET/SET_VCPU_EVENTS */
>>> struct kvm_vcpu_events {
>>>  ...
>>> struct {
>>> 		__u8 injected;
>>> 		__u8 pending;
>>> 		__u8 masked;
>>> 		__u8 pad;
>>> 	} nmi;
>>>  ...
>>>
>>> I found that the nmi.masked property does these enable or disable NMI jobs.
>>> So, I think we don't need to add a new bit. Right?
>>
>> nmi.masked says whether the CPU is accepting the NMIs, and is cleared
>> by the next IRET instruction.  This is a different thing; it probably
>> shouldn't affect NMI IPIs, and it definitely should remain set until
>> cleared via the RTC.  So it should be something like
>>
>>     _u8 external_nmi_disabled;
>>
>> or similar.
>>
>> *However* I found this in the ICH9 datasheet:
>>
>>     The ICH9's I/O APIC can only send interrupts due to interrupts which
>>     do not include SMI, NMI or INIT. This means that in IA-32/Intel ® 64
>>     based platforms, Front Side Bus interrupt message format delivery modes
>>     010 (SMI/PMI), 100 (NMI), and 101 (INIT) as indicated in this section,
>>     must not be used and is not supported.
>>
>> In theory the PIIX4 could deliver such messages, but perhaps we could
>> disable them in the KVM IOAPIC.  If we do this, there is no need for a
>> change to struct kvm_vcpu_events, because all external NMI sources will
>> be in userspace.
>>
>> Radim, what do you think?
> 
> I looked at the 440fx, piix, and 82083aa(ioapic) datasheets and the
> NMI_EN bit doesn't seem to be propagated into the IOAPIC.
> The IOAPIC datasheet doesn't mention a thing about NMI masking and PIIX4
> generates NMI on SERR# or IOCHK# so it seems that the NMI_EN feature
> only changes the behavior of those two ...
> 
> I think it's best to do nothing in KVM.

Then Gonglei's patch (apart from the issues that Eduardo pointed out) is
fine.

Paolo

Re: [Qemu-devel] [PATCH] rtc: introduce nmi disable bit handler for cmos

[Gonglei (Arei)]

> 
> On 15/12/2015 19:53, Radim Krcmar wrote:
> > 2015-12-15 05:43-0500, Paolo Bonzini:
> >>> Hi Paolo,
> >>>
> >>> /* for KVM_GET/SET_VCPU_EVENTS */
> >>> struct kvm_vcpu_events {
> >>>  ...
> >>> struct {
> >>> 		__u8 injected;
> >>> 		__u8 pending;
> >>> 		__u8 masked;
> >>> 		__u8 pad;
> >>> 	} nmi;
> >>>  ...
> >>>
> >>> I found that the nmi.masked property does these enable or disable NMI
> jobs.
> >>> So, I think we don't need to add a new bit. Right?
> >>
> >> nmi.masked says whether the CPU is accepting the NMIs, and is cleared
> >> by the next IRET instruction.  This is a different thing; it probably
> >> shouldn't affect NMI IPIs, and it definitely should remain set until
> >> cleared via the RTC.  So it should be something like
> >>
> >>     _u8 external_nmi_disabled;
> >>
> >> or similar.
> >>
> >> *However* I found this in the ICH9 datasheet:
> >>
> >>     The ICH9's I/O APIC can only send interrupts due to interrupts which
> >>     do not include SMI, NMI or INIT. This means that in IA-32/Intel ® 64
> >>     based platforms, Front Side Bus interrupt message format delivery
> modes
> >>     010 (SMI/PMI), 100 (NMI), and 101 (INIT) as indicated in this section,
> >>     must not be used and is not supported.
> >>
> >> In theory the PIIX4 could deliver such messages, but perhaps we could
> >> disable them in the KVM IOAPIC.  If we do this, there is no need for
> >> a change to struct kvm_vcpu_events, because all external NMI sources
> >> will be in userspace.
> >>
> >> Radim, what do you think?
> >
> > I looked at the 440fx, piix, and 82083aa(ioapic) datasheets and the
> > NMI_EN bit doesn't seem to be propagated into the IOAPIC.
> > The IOAPIC datasheet doesn't mention a thing about NMI masking and
> > PIIX4 generates NMI on SERR# or IOCHK# so it seems that the NMI_EN
> > feature only changes the behavior of those two ...
> >
> > I think it's best to do nothing in KVM.
> 
> Then Gonglei's patch (apart from the issues that Eduardo pointed out) is fine.
> 
I'll move the global nmi_disabled into RTCState, then I have to add a global RTCState
Variable so that other C files can use the rtc_state->external_nmi_disabled.


diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index e770e74..c019ec0 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -67,6 +67,7 @@ typedef struct RTCState {
     MemoryRegion io;
     uint8_t cmos_data[128];
     uint8_t cmos_index;
+    bool external_nmi_disabled;
     int32_t base_year;
     uint64_t base_rtc;
     uint64_t last_update;
@@ -89,6 +90,8 @@ typedef struct RTCState {
     QLIST_ENTRY(RTCState) link;
 } RTCState;
 
+static RTCState *rtc_global_state;
+
 static void rtc_set_time(RTCState *s);
 static void rtc_update_time(RTCState *s);
 static void rtc_set_cmos(RTCState *s, const struct tm *tm);
@@ -383,6 +386,11 @@ static void rtc_update_timer(void *opaque)
     check_update_timer(s);
 }
 
+bool external_nmi_is_disabled()
+{
+    return rtc_global_state->external_nmi_disabled;
+}
+
 static void cmos_ioport_write(void *opaque, hwaddr addr,
                               uint64_t data, unsigned size)
 {
@@ -397,9 +405,9 @@ static void cmos_ioport_write(void *opaque, hwaddr addr,
          * by applications, BIOS, and even the operating system since it is
          * used to block NMI assertions when sensitive code is executing.
          */
-        nmi_disabled = !!(data & NMI_DISABLE_BIT);
+        s->external_nmi_disabled = !!(data & NMI_DISABLE_BIT);
         CMOS_DPRINTF("cmos: nmi_disabled=%s\n",
-                     nmi_disabled ? "true" : "false");
+                     s->external_nmi_disabled ? "true" : "false");
         s->cmos_index = data & 0x7f;
     } else {
         CMOS_DPRINTF("cmos: write index=0x%02x val=0x%02" PRIx64 "\n",
@@ -930,6 +938,8 @@ ISADevice *rtc_init(ISABus *bus, int base_year, qemu_irq intercept_irq)
     }
     QLIST_INSERT_HEAD(&rtc_devices, s, link);
 
+    rtc_global_state = s;
+
     return isadev;
 }
 
diff --git a/include/hw/timer/mc146818rtc.h b/include/hw/timer/mc146818rtc.h
index eaf6497..761eba2 100644
--- a/include/hw/timer/mc146818rtc.h
+++ b/include/hw/timer/mc146818rtc.h
@@ -9,5 +9,6 @@
 ISADevice *rtc_init(ISABus *bus, int base_year, qemu_irq intercept_irq);
 void rtc_set_memory(ISADevice *dev, int addr, int val);
 int rtc_get_memory(ISADevice *dev, int addr);
+bool external_nmi_is_disabled();
 
 #endif /* !MC146818RTC_H */

And one more thing is we need to keep the property in vmstate for supporting live
Migration.

Am I right?  Thanks!


Regards,
-Gonglei

Re: [Qemu-devel] [PATCH] rtc: introduce nmi disable bit handler for cmos

[Paolo Bonzini]

On 16/12/2015 11:28, Gonglei (Arei) wrote:
> I'll move the global nmi_disabled into RTCState, then I have to add a global RTCState
> Variable so that other C files can use the rtc_state->external_nmi_disabled.

Hmm, I think it should be done differently.  This is a layering
violation, the NMI_EN is essentially a pin (qemu_irq) between the ISA
bridges and the RTC.  The NMI "button" is also a component of the ISA
bridge; you should not need to touch anything except the RTC and the ISA
bridges, in particular not the APICs.

First, you need to add a qemu_irq argument to rtc_init. The RTC can
raise/lower the IRQ on writes to port 0x70.

Second, make the ISA bridges implement NMIState, where the
implementation of NMIState is similar to inject_nmi in hw/core/nmi.c:

    CPU_FOREACH(cs) {
	X86CPU *cpu = X86_CPU(cs);

	if (!cpu->apic_state) {
	    cpu_interrupt(cs, CPU_INTERRUPT_NMI);
	} else {
	    apic_deliver_nmi(cpu->apic_state);
	}
    }

Third, the ISA bridges (hw/isa/piix4.c and hw/isa/lpc_ich9.c) need to
export a qemu_irq for nmi_en IRQ (e.g. using qdev_init_gpio_in_named),
and you should modify the ISA bridge's implementation of NMIState to
latch the NMI if you send one while NMIs are disabled.  The nmi_en IRQ
can also trigger an NMI when nmi_en is enabled and an NMI was latched.
The nmi_en status and NMI latch status must be migrated in a new
subsection of the ISA bridges.

Fourth, the PC machines should use qdev_get_gpio_in_named to retrieve
the qemu_irq from the ISA bridges, and pass it to pc_basic_device_init.

I may have messed up some steps, but this is the basic idea.

Paolo

Re: [Qemu-devel] [PATCH] rtc: introduce nmi disable bit handler for cmos

[Gonglei (Arei)]

Hello Paolo,

> 
> 
> On 16/12/2015 11:28, Gonglei (Arei) wrote:
> > I'll move the global nmi_disabled into RTCState, then I have to add a global
> RTCState
> > Variable so that other C files can use the rtc_state->external_nmi_disabled.
> 
> Hmm, I think it should be done differently.  This is a layering
> violation, the NMI_EN is essentially a pin (qemu_irq) between the ISA
> bridges and the RTC.  The NMI "button" is also a component of the ISA

So, you mean the NMI_EN can only control NMI injection came from ISA bridge?
What's this NMI "button" mean? 

> bridge; you should not need to touch anything except the RTC and the ISA
> bridges, in particular not the APICs.
> 
Currently, the qmp command "inject-nmi" doesn't pass ISA bridge. How
do we address this situation?

> First, you need to add a qemu_irq argument to rtc_init. The RTC can
> raise/lower the IRQ on writes to port 0x70.
> 
> Second, make the ISA bridges implement NMIState, where the
> implementation of NMIState is similar to inject_nmi in hw/core/nmi.c:
> 
>     CPU_FOREACH(cs) {
> 	X86CPU *cpu = X86_CPU(cs);
> 
> 	if (!cpu->apic_state) {
> 	    cpu_interrupt(cs, CPU_INTERRUPT_NMI);
> 	} else {
> 	    apic_deliver_nmi(cpu->apic_state);
> 	}
>     }
> 
> Third, the ISA bridges (hw/isa/piix4.c and hw/isa/lpc_ich9.c) need to

We don't use hw/isa/piix4.c but hw/pci-host/piix.c in x86 target. Right?

> export a qemu_irq for nmi_en IRQ (e.g. using qdev_init_gpio_in_named),
> and you should modify the ISA bridge's implementation of NMIState to
> latch the NMI if you send one while NMIs are disabled.  The nmi_en IRQ
> can also trigger an NMI when nmi_en is enabled and an NMI was latched.

Sorry, I'm a bit confused. The nmi_en can trigger an NMI? Isn't a flag bit which
can enable/disable the NMI switch?

> The nmi_en status and NMI latch status must be migrated in a new
> subsection of the ISA bridges.
> 
> Fourth, the PC machines should use qdev_get_gpio_in_named to retrieve
> the qemu_irq from the ISA bridges, and pass it to pc_basic_device_init.
> 
> I may have messed up some steps, but this is the basic idea.
> 
> Paolo

Thanks,
-Gonglei

Re: [Qemu-devel] [PATCH] rtc: introduce nmi disable bit handler for cmos

[Paolo Bonzini]

On 17/12/2015 08:17, Gonglei (Arei) wrote:
>> On 16/12/2015 11:28, Gonglei (Arei) wrote:
>>> I'll move the global nmi_disabled into RTCState, then I have to add a global
>> RTCState
>>> Variable so that other C files can use the rtc_state->external_nmi_disabled.
>>
>> Hmm, I think it should be done differently.  This is a layering
>> violation, the NMI_EN is essentially a pin (qemu_irq) between the ISA
>> bridges and the RTC.  The NMI "button" is also a component of the ISA
> 
> So, you mean the NMI_EN can only control NMI injection came from ISA bridge?
> What's this NMI "button" mean? 

The NMI command in the monitor is a "virtual NMI button".

>> bridge; you should not need to touch anything except the RTC and the ISA
>> bridges, in particular not the APICs.
>>
> Currently, the qmp command "inject-nmi" doesn't pass ISA bridge. How
> do we address this situation?

That's step two below: make the ISA bridges implement NMIState.

>> First, you need to add a qemu_irq argument to rtc_init. The RTC can
>> raise/lower the IRQ on writes to port 0x70.
>>
>> Second, make the ISA bridges implement NMIState, where the
>> implementation of NMIState is similar to inject_nmi in hw/core/nmi.c:
>>
>>     CPU_FOREACH(cs) {
>> 	X86CPU *cpu = X86_CPU(cs);
>>
>> 	if (!cpu->apic_state) {
>> 	    cpu_interrupt(cs, CPU_INTERRUPT_NMI);
>> 	} else {
>> 	    apic_deliver_nmi(cpu->apic_state);
>> 	}
>>     }
>>
>> Third, the ISA bridges (hw/isa/piix4.c and hw/isa/lpc_ich9.c) need to
> 
> We don't use hw/isa/piix4.c but hw/pci-host/piix.c in x86 target. Right?

Right, I said I had certainly messed up something. :)

>> export a qemu_irq for nmi_en IRQ (e.g. using qdev_init_gpio_in_named),
>> and you should modify the ISA bridge's implementation of NMIState to
>> latch the NMI if you send one while NMIs are disabled.  The nmi_en IRQ
>> can also trigger an NMI when nmi_en is enabled and an NMI was latched.
> 
> Sorry, I'm a bit confused. The nmi_en can trigger an NMI? Isn't a flag bit which
> can enable/disable the NMI switch?

Suppose an NMI was injected with the monitor while nmi_en was disabled.
 The NMI is then latched, and triggered when you enable NMIs again with
nmi_en.

Thanks,

Paolo

Re: [Qemu-devel] [PATCH] rtc: introduce nmi disable bit handler for cmos

[Gonglei (Arei)]

> 
> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo
> Bonzini
> Sent: Thursday, December 17, 2015 4:37 PM
> 
> 
> On 17/12/2015 08:17, Gonglei (Arei) wrote:
> >> On 16/12/2015 11:28, Gonglei (Arei) wrote:
> >>> I'll move the global nmi_disabled into RTCState, then I have to add
> >>> a global
> >> RTCState
> >>> Variable so that other C files can use the
> rtc_state->external_nmi_disabled.
> >>
> >> Hmm, I think it should be done differently.  This is a layering
> >> violation, the NMI_EN is essentially a pin (qemu_irq) between the ISA
> >> bridges and the RTC.  The NMI "button" is also a component of the ISA
> >
> > So, you mean the NMI_EN can only control NMI injection came from ISA
> bridge?
> > What's this NMI "button" mean?
> 
> The NMI command in the monitor is a "virtual NMI button".
> 
Okay, I see. 

> >> bridge; you should not need to touch anything except the RTC and the
> >> ISA bridges, in particular not the APICs.
> >>
> > Currently, the qmp command "inject-nmi" doesn't pass ISA bridge. How
> > do we address this situation?
> 
> That's step two below: make the ISA bridges implement NMIState.
> 
Yes, It's more reasonable.

> >> First, you need to add a qemu_irq argument to rtc_init. The RTC can
> >> raise/lower the IRQ on writes to port 0x70.
> >>
> >> Second, make the ISA bridges implement NMIState, where the
> >> implementation of NMIState is similar to inject_nmi in hw/core/nmi.c:
> >>
> >>     CPU_FOREACH(cs) {
> >> 	X86CPU *cpu = X86_CPU(cs);
> >>
> >> 	if (!cpu->apic_state) {
> >> 	    cpu_interrupt(cs, CPU_INTERRUPT_NMI);
> >> 	} else {
> >> 	    apic_deliver_nmi(cpu->apic_state);
> >> 	}
> >>     }
> >>
> >> Third, the ISA bridges (hw/isa/piix4.c and hw/isa/lpc_ich9.c) need to
> >
> > We don't use hw/isa/piix4.c but hw/pci-host/piix.c in x86 target. Right?
> 
> Right, I said I had certainly messed up something. :)
> 
> >> export a qemu_irq for nmi_en IRQ (e.g. using
> >> qdev_init_gpio_in_named), and you should modify the ISA bridge's
> >> implementation of NMIState to latch the NMI if you send one while
> >> NMIs are disabled.  The nmi_en IRQ can also trigger an NMI when nmi_en
> is enabled and an NMI was latched.
> >
> > Sorry, I'm a bit confused. The nmi_en can trigger an NMI? Isn't a flag
> > bit which can enable/disable the NMI switch?
> 
> Suppose an NMI was injected with the monitor while nmi_en was disabled.
>  The NMI is then latched, and triggered when you enable NMIs again with
> nmi_en.
> 
It's clear. I'll try to do this as your suggestion. Thank you so much!

Regards,
-Gonglei