Re: [PATCH v2 6/6] kvm: i386: irqchip: take BQL only if there is an interrupt

[Igor Mammedov <imammedo@redhat.com>]

On Fri, 1 Aug 2025 12:26:26 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> The patch is not wrong but complicates things more than it should.
> 
> Also, as we do more of these tricks it may be worth adding wrapper APIs 
> for interrupt_request access, but that needs to be done tree-wide so you 
> can do it separately.

Thanks,
I'll respin this with minimal changes for this series
and post another one on top with tree wide refactoring as suggested

> 
> On 7/30/25 14:39, Igor Mammedov wrote:
> >      if (cpu->interrupt_request & (CPU_INTERRUPT_INIT | CPU_INTERRUPT_TPR)) {
> > +        if (!kvm_pic_in_kernel()) {
> > +            bql_lock();
> > +            release_bql = true;
> > +        }  
> 
> This bql_lock() is not needed, all the writes in the "if" are local to 
> the current CPU.
> 
> When the outer bql_lock() was added, cpu_interrupt() was not thread-safe 
> at all, and taking the lock was needed in order to read 
> cpu->interrupt_request.  But now it is ok to read outside the lock, 
> which you can use to simplify this patch a lot.
> 
> >          if ((cpu->interrupt_request & CPU_INTERRUPT_INIT) &&
> >              !(env->hflags & HF_SMM_MASK)) {
> >              cpu->exit_request = 1;  
> 
> A patch that changes all these accesses to 
> qatomic_set(&cpu->exit_request, 1), tree-wide, would be nice.
> 
> > +        if (cpu->interrupt_request & CPU_INTERRUPT_HARD) {  
> 
> This should be qatomic_read(&cpu->interrupt_request).  Not a blocker for 
> now, but this is where I would suggest adding a wrapper like 
> cpu_test_interrupt(cpu, CPU_INTERRUPT_HARD).
> 
> > +            if (!release_bql) {
> > +                bql_lock();
> > +                release_bql = true;
> > +            }  
> 
> With the above simplification, this can be done unconditionally.
> 
> > +            /* Try to inject an interrupt if the guest can accept it */
> > +            if (run->ready_for_interrupt_injection &&
> > +                (cpu->interrupt_request & CPU_INTERRUPT_HARD) &&
> > +                (env->eflags & IF_MASK)) {
> > +                int irq;
> > +
> > +                cpu->interrupt_request &= ~CPU_INTERRUPT_HARD;  
> 
> Reads and writes to cpu->interrupt_request still take the BQL, which is 
> consistent with include/hw/core/cpu.h, so yeah here the bql_lock() is 
> needed.
> 
> Like above, writing it's a data race with readers outside the BQL, so 
> qatomic_read()/qatomic_set() would be needed to respect the C standard. 
> Even better could be to add a function cpu_reset_interrupt_locked() that 
> does
> 
>     assert(bql_locked());
>     qatomic_set(&cpu->interrupt_request, cpu->interrupt_request & ~mask);
> 
> But neither of these wrappers (which should be applied tree-wide) are an 
> absolute necessity for this series.
> 
> > @@ -5531,7 +5540,14 @@ void kvm_arch_pre_run(CPUState *cpu, struct kvm_run *run)
> >   
> >           DPRINTF("setting tpr\n");
> >           run->cr8 = cpu_get_apic_tpr(x86_cpu->apic_state);
> > +        /*
> > +         * make sure that request_interrupt_window/cr8 are set
> > +         * before KVM_RUN might read them
> > +         */
> > +        smp_mb();  
> 
> This is not needed, ->cr8 is only read for the same CPU (in 
> kvm_arch_vcpu_ioctl_run).
> 
> > +    }
> >   
> > +    if (release_bql) {
> >           bql_unlock();
> >       }  
> 
> And since release_bql is not needed anymore, the bql_unlock() can be 
> left where it was.
> 
> Paolo
> 
> >   }  
>