Skip to contentSkip to conversation

target/alpha: Take BQL around clock manipulations

PATCH In review Richard Henderson 3 participants 4 messages Mar 6, 2017 – Mar 7, 2017

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 target/alpha: Take BQL around clock manipulations

---
This is similar to the patch that I saw go by for MIPS.

I hadn't noticed any problems caused by this lack of locking. This may be because interrupts cannot be delivered while in PALmode while these registers are being manipulated. However, it's always better to obey the rules, right?

r~

---
 target/alpha/sys_helper.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)
target/alpha/sys_helper.c +17 -4
index 652195d..6feb30b 100644
@@ -28,11 +28,14 @@
 uint64_t helper_load_pcc(CPUAlphaState *env)
 {
 #ifndef CONFIG_USER_ONLY
+    uint64_t pcc;
     /* In system mode we have access to a decent high-resolution clock.
        In order to make OS-level time accounting work with the RPCC,
        present it with a well-timed clock fixed at 250MHz.  */
-    return (((uint64_t)env->pcc_ofs << 32)
-            | (uint32_t)(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) >> 2));
+    qemu_mutex_lock_iothread();
+    pcc = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) >> 2;
+    qemu_mutex_unlock_iothread();
+    return deposit64(pcc, 32, 32, env->pcc_ofs);
 #else
     /* In user-mode, QEMU_CLOCK_VIRTUAL doesn't exist.  Just pass through the host cpu
        clock ticks.  Also, don't bother taking PCC_OFS into account.  */
@@ -68,24 +71,34 @@ void helper_halt(uint64_t restart)
 
 uint64_t helper_get_vmtime(void)
 {
-    return qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+    uint64_t ret;
+    qemu_mutex_lock_iothread();
+    ret = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+    qemu_mutex_unlock_iothread();
+    return ret;
 }
 
 uint64_t helper_get_walltime(void)
 {
-    return qemu_clock_get_ns(rtc_clock);
+    uint64_t ret;
+    qemu_mutex_lock_iothread();
+    ret = qemu_clock_get_ns(rtc_clock);
+    qemu_mutex_unlock_iothread();
+    return ret;
 }
 
 void helper_set_alarm(CPUAlphaState *env, uint64_t expire)
 {
     AlphaCPU *cpu = alpha_env_get_cpu(env);
 
+    qemu_mutex_lock_iothread();
     if (expire) {
         env->alarm_expire = expire;
         timer_mod(cpu->alarm_timer, expire);
     } else {
         timer_del(cpu->alarm_timer);
     }
+    qemu_mutex_unlock_iothread();
 }
 
 #endif /* CONFIG_USER_ONLY */
Signature · 1 line
2.9.3
#1 lore raw
Quoted 8 lines

---
This is similar to the patch that I saw go by for MIPS.

I hadn't noticed any problems caused by this lack of locking. This may be because interrupts cannot be delivered while in PALmode while these registers are being manipulated. However, it's always better to obey the rules, right?

This should not be necessary, clocks and timers are thread-safe. Time to make a list of the few things that are, I guess.

There are issues if data is accessed by device models and CPU out of the lock, but everything seems fine for typhoon_alarm_timer.

Paolo

Quoted 69 lines

r~

---
 target/alpha/sys_helper.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)
target/alpha/sys_helper.c +5 -2
index 652195d..6feb30b 100644
@@ -28,11 +28,14 @@
 uint64_t helper_load_pcc(CPUAlphaState *env)
 {
 #ifndef CONFIG_USER_ONLY
+    uint64_t pcc;
     /* In system mode we have access to a decent high-resolution clock.
        In order to make OS-level time accounting work with the RPCC,
        present it with a well-timed clock fixed at 250MHz.  */
-    return (((uint64_t)env->pcc_ofs << 32)
-            | (uint32_t)(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) >> 2));
+    qemu_mutex_lock_iothread();
+    pcc = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) >> 2;
+    qemu_mutex_unlock_iothread();
+    return deposit64(pcc, 32, 32, env->pcc_ofs);
 #else
     /* In user-mode, QEMU_CLOCK_VIRTUAL doesn't exist.  Just pass through
     the host cpu

clock ticks. Also, don't bother taking PCC_OFS into account. */

target/alpha/sys_helper.c +12 -3
@@ -68,24 +71,34 @@ void helper_halt(uint64_t restart)
 
 uint64_t helper_get_vmtime(void)
 {
-    return qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+    uint64_t ret;
+    qemu_mutex_lock_iothread();
+    ret = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+    qemu_mutex_unlock_iothread();
+    return ret;
 }
 
 uint64_t helper_get_walltime(void)
 {
-    return qemu_clock_get_ns(rtc_clock);
+    uint64_t ret;
+    qemu_mutex_lock_iothread();
+    ret = qemu_clock_get_ns(rtc_clock);
+    qemu_mutex_unlock_iothread();
+    return ret;
 }
 
 void helper_set_alarm(CPUAlphaState *env, uint64_t expire)
 {
     AlphaCPU *cpu = alpha_env_get_cpu(env);
 
+    qemu_mutex_lock_iothread();
     if (expire) {
         env->alarm_expire = expire;
         timer_mod(cpu->alarm_timer, expire);
     } else {
         timer_del(cpu->alarm_timer);
     }
+    qemu_mutex_unlock_iothread();
 }
 
 #endif /* CONFIG_USER_ONLY */
--

2.9.3

#2 lore raw
Quoted 9 lines · Paolo Bonzini
Quoted 8 lines

---
This is similar to the patch that I saw go by for MIPS.

I hadn't noticed any problems caused by this lack of locking. This may be because interrupts cannot be delivered while in PALmode while these registers are being manipulated. However, it's always better to obey the rules, right?

Quoted 5 lines

This should not be necessary, clocks and timers are thread-safe. Time to make a list of the few things that are, I guess.

There are issues if data is accessed by device models and CPU out of the lock, but everything seems fine for typhoon_alarm_timer.

This isn't typhoon_alarm_timer, but the move-to-special-register instruction on the cpu side.

But I guess I misunderstood the problem that was happening for MIPS. If nothing needs changing for Alpha, that's great.

r~

#3 lore raw
Quoted 13 lines · Richard Henderson
Quoted 12 lines · Paolo Bonzini
Quoted 8 lines

---
This is similar to the patch that I saw go by for MIPS.

I hadn't noticed any problems caused by this lack of locking. This may be because interrupts cannot be delivered while in PALmode while these registers are being manipulated. However, it's always better to obey the rules, right?

This should not be necessary, clocks and timers are thread-safe. Time to make a list of the few things that are, I guess.

Quoted 8 lines
Quoted 2 lines

There are issues if data is accessed by device models and CPU out of the lock, but everything seems fine for typhoon_alarm_timer.

This isn't typhoon_alarm_timer, but the move-to-special-register instruction on the cpu side.

But I guess I misunderstood the problem that was happening for MIPS. If nothing needs changing for Alpha, that's great.

Fundamentally the MIPS instructions ended up calling into hw/mips/ which could then end up triggering an IRQ (at which point the BQL assertion kicks in).

Basically crossing from target/foo/helper to hw/foo/emulation is the warning sign that you need to ensure you have appropriate device emulation locking going on.

Helpers just messing with their own env should be able to continue just fine.

Quoted 3 lines

r~

--
Alex Bennée