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