[PATCH] i386/xen: consistent locking around Xen singleshot timers

[David Woodhouse <dwmw2@infradead.org>]

[-- Attachment #1: Type: text/plain, Size: 3505 bytes --]

From: David Woodhouse <dwmw@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 <dwmw@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



[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]