| Message ID | 33183CC9F5247A488A2544077AF19020B02B5191@SZXEMA503-MBS.china.huawei.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
> > 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
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 */