From: David Woodhouse <d...@amazon.co.uk> Coverity points out (CID 1507534) that we sometimes access env->xen_singleshot_timer_ns under the protection of env->xen_timers_lock (eg in xen_vcpu_singleshot_timer_event()) and sometimes not (the specific case Coverity complains about is in do_vcpu_soft_reset()).
This isn't strictly an issue. There are two modes for the timers; if the kernel supports the EVTCHN_SEND capability then it handles all the timer hypercalls and delivery internally, and all we need to do is an ioctl to get/set the next timer as part of the vCPU state. If the kernel doesn't have that support, then we do all the emulation within qemu, and *those* are the code paths where we actually care about the locking. But it doesn't hurt to be a little bit more consistent and avoid having to explain *why* it's OK. Signed-off-by: David Woodhouse <d...@amazon.co.uk> --- On Tue, 2023-05-09 at 15:55 +0100, Peter Maydell wrote: > Hi; Coverity points out (CID 1507534) that we seem to sometimes > access env->xen_singleshot_timer_ns under the protection of > env->xen_timers_lock (eg in xen_vcpu_singleshot_timer_event()) > and sometimes not (the specific case Coverity complains about is > in do_vcpu_soft_reset()). Is this a false positive, or is there Thanks. As noted, I think it's harmless but it doesn't hurt to clean it up a bit. I've pushed this to my tree at https://git.infradead.org/users/dwmw2/qemu.git/shortlog/refs/heads/xenfv on top of the other fixes that didn't make 8.0. target/i386/kvm/xen-emu.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/target/i386/kvm/xen-emu.c b/target/i386/kvm/xen-emu.c index d7c7eb8d9c..36d2dd8d02 100644 --- a/target/i386/kvm/xen-emu.c +++ b/target/i386/kvm/xen-emu.c @@ -43,6 +43,7 @@ static void xen_vcpu_singleshot_timer_event(void *opaque); static void xen_vcpu_periodic_timer_event(void *opaque); +static int vcpuop_stop_singleshot_timer(CPUState *cs); #ifdef TARGET_X86_64 #define hypercall_compat32(longmode) (!(longmode)) @@ -483,6 +484,7 @@ static int kvm_xen_set_vcpu_timer(CPUState *cs) static void do_set_vcpu_timer_virq(CPUState *cs, run_on_cpu_data data) { + QEMU_LOCK_GUARD(&X86_CPU(cs)->env.xen_timers_lock); kvm_xen_set_vcpu_timer(cs); } @@ -545,7 +547,6 @@ static void do_vcpu_soft_reset(CPUState *cs, run_on_cpu_data data) env->xen_vcpu_time_info_gpa = INVALID_GPA; env->xen_vcpu_runstate_gpa = INVALID_GPA; env->xen_vcpu_callback_vector = 0; - env->xen_singleshot_timer_ns = 0; memset(env->xen_virq, 0, sizeof(env->xen_virq)); set_vcpu_info(cs, INVALID_GPA); @@ -555,8 +556,13 @@ static void do_vcpu_soft_reset(CPUState *cs, run_on_cpu_data data) INVALID_GPA); if (kvm_xen_has_cap(EVTCHN_SEND)) { kvm_xen_set_vcpu_callback_vector(cs); + + QEMU_LOCK_GUARD(&X86_CPU(cs)->env.xen_timers_lock); + env->xen_singleshot_timer_ns = 0; kvm_xen_set_vcpu_timer(cs); - } + } else { + vcpuop_stop_singleshot_timer(cs); + }; } @@ -1844,10 +1850,7 @@ int kvm_put_xen_state(CPUState *cs) } if (env->xen_virq[VIRQ_TIMER]) { - ret = kvm_xen_set_vcpu_timer(cs); - if (ret < 0) { - return ret; - } + do_set_vcpu_timer_virq(cs, RUN_ON_CPU_HOST_INT(env->xen_virq[VIRQ_TIMER])); } return 0; } -- 2.34.1
smime.p7s
Description: S/MIME cryptographic signature
