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

  }


Reply via email to