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