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