Re: [PATCH 13/16] target/arm/hvf: Lock BQL outside of the vCPU inner execution loop

On 17/8/26 18:11, Paolo Bonzini wrote:

On 8/13/26 20:16, Philippe Mathieu-Daudé wrote:

Reduce lock contention by acquiring / releasing the BQL
outside of the entire vCPU inner loop.

Signed-off-by: Philippe Mathieu-Daudé <[email protected]>
---
  target/arm/hvf/hvf.c | 13 ++++++++-----
  1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/target/arm/hvf/hvf.c b/target/arm/hvf/hvf.c
index c14ea54a67f..9c6408d2e25 100644
--- a/target/arm/hvf/hvf.c
+++ b/target/arm/hvf/hvf.c
@@ -2598,20 +2598,20 @@ int hvf_arch_vcpu_exec(CPUState *cpu)
          }
      }
+    bql_unlock();
+    cpu_exec_start(cpu);
+
      /* Inner vCPU loop */
      do {
          if (!(cpu->singlestep_flags & SSTEP_NOIRQ) &&
              hvf_inject_interrupts(cpu)) {

It always return zero, so you can make it void and change to

           if (!(cpu->singlestep_flags & SSTEP_NOIRQ)) {
               hvf_inject_interrupts(cpu);
       }

However, a bigger question: who calls cpu_reset_interrupt() here:

     if (cpu_test_interrupt(cpu, CPU_INTERRUPT_FIQ)) {
         trace_hvf_inject_fiq();

        hv_vcpu_set_pending_interrupt(cpu->accel->fd, HV_INTERRUPT_TYPE_FIQ,

                                       true);
     }

     if (cpu_test_interrupt(cpu, CPU_INTERRUPT_HARD)) {
         trace_hvf_inject_irq();

        hv_vcpu_set_pending_interrupt(cpu->accel->fd, HV_INTERRUPT_TYPE_IRQ,

                                       true);
     }

and can this be changed to do something like arm_cpu_kvm_set_irq() instead?
Having the function called only with level == true is a bit unsettling...

Digging further this other change from a86024eb2df56f looks dubious:

 int hvf_arch_vcpu_exec(CPUState *cpu)
 {
     int ret;
     hv_return_t r;

     if (cpu->halted) {
-        return EXCP_HLT;
+        if (!cpu_has_work(cpu)) {
+            return EXCP_HLT;
+        }
+        cpu->halted = 0;
+        if (!hvf_irqchip_in_kernel()) {
+            timer_del(cpu->accel->wfi_timer);
+        }
     }

Paolo

-            return EXCP_INTERRUPT;
+            ret = EXCP_INTERRUPT;
+            break;
          }
          flush_cpu_state(cpu);
-        bql_unlock();
-        cpu_exec_start(cpu);
          r = hv_vcpu_run(cpu->accel->fd);
-        cpu_exec_end(cpu);
-        bql_lock();
          switch (r) {
          case HV_SUCCESS:
              ret = hvf_handle_vmexit(cpu, cpu->accel->exit);
@@ -2624,6 +2624,9 @@ int hvf_arch_vcpu_exec(CPUState *cpu)
          }
      } while (ret == 0);
+    cpu_exec_end(cpu);
+    bql_lock();
+
      return ret;
  }