On Fri, 1 Aug 2025 12:26:26 +0200
Paolo Bonzini <[email protected]> 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
>
> > }
>
