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