Re: [Qemu-devel] [PATCH v6 40/73] i386/kvm: convert to cpu_interrupt_request

[Emilio G. Cota]

On Fri, Feb 08, 2019 at 11:15:27 +0000, Alex Bennée wrote:
> 
> Emilio G. Cota <address@hidden> writes:
> 
> > Reviewed-by: Richard Henderson <address@hidden>
> > Signed-off-by: Emilio G. Cota <address@hidden>
> > ---
> >  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 <address@hidden>

Thanks,

                E.