On Fri, Feb 08, 2019 at 11:15:27 +0000, Alex Bennée wrote:
>
> Emilio G. Cota <c...@braap.org> writes:
>
> > Reviewed-by: Richard Henderson <richard.hender...@linaro.org>
> > Signed-off-by: Emilio G. Cota <c...@braap.org>
> > ---
> > target/i386/kvm.c | 54 +++++++++++++++++++++++++++--------------------
> > 1 file changed, 31 insertions(+), 23 deletions(-)
> >
> > diff --git a/target/i386/kvm.c b/target/i386/kvm.c
> > index ca2629f0fe..3f3c670897 100644
> > --- a/target/i386/kvm.c
> > +++ b/target/i386/kvm.c
> <snip>
> > @@ -3183,14 +3186,14 @@ void kvm_arch_pre_run(CPUState *cpu, struct kvm_run
> > *run)
> > {
> > X86CPU *x86_cpu = X86_CPU(cpu);
> > CPUX86State *env = &x86_cpu->env;
> > + uint32_t interrupt_request;
> > int ret;
> >
> > + interrupt_request = cpu_interrupt_request(cpu);
> > /* Inject NMI */
> > - if (cpu->interrupt_request & (CPU_INTERRUPT_NMI | CPU_INTERRUPT_SMI)) {
> > - if (cpu->interrupt_request & CPU_INTERRUPT_NMI) {
> > - qemu_mutex_lock_iothread();
> > + if (interrupt_request & (CPU_INTERRUPT_NMI | CPU_INTERRUPT_SMI)) {
> > + if (interrupt_request & CPU_INTERRUPT_NMI) {
> > cpu_reset_interrupt(cpu, CPU_INTERRUPT_NMI);
> > - qemu_mutex_unlock_iothread();
> > DPRINTF("injected NMI\n");
> > ret = kvm_vcpu_ioctl(cpu, KVM_NMI);
> > if (ret < 0) {
> > @@ -3198,10 +3201,8 @@ void kvm_arch_pre_run(CPUState *cpu, struct kvm_run
> > *run)
> > strerror(-ret));
> > }
> > }
> > - if (cpu->interrupt_request & CPU_INTERRUPT_SMI) {
> > - qemu_mutex_lock_iothread();
> > + if (interrupt_request & CPU_INTERRUPT_SMI) {
> > cpu_reset_interrupt(cpu, CPU_INTERRUPT_SMI);
> > - qemu_mutex_unlock_iothread();
> > DPRINTF("injected SMI\n");
> > ret = kvm_vcpu_ioctl(cpu, KVM_SMI);
> > if (ret < 0) {
> > @@ -3215,16 +3216,18 @@ void kvm_arch_pre_run(CPUState *cpu, struct kvm_run
> > *run)
> > qemu_mutex_lock_iothread();
> > }
> >
> > + interrupt_request = cpu_interrupt_request(cpu);
> > +
>
> This seems a bit smelly as we have already read interrupt_request once
> before. So this says that something may have triggered an IRQ while we
> were dealing with the above. It requires a comment at least.
There are a few cpu_reset_interrupt() calls above, so I thought a comment
wasn't necessary. I've added the following comment:
+ /*
+ * We might have cleared some bits in cpu->interrupt_request since reading
+ * it; read it again.
+ */
interrupt_request = cpu_interrupt_request(cpu);
> Otherwise:
>
> Reviewed-by: Alex Bennée <alex.ben...@linaro.org>
Thanks,
E.
