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
}