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

[Paolo Bonzini <pbonzini@redhat.com>]

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.

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

>   }