On Fri, 22 May 2020 at 13:44, Peter Maydell <peter.mayd...@linaro.org> wrote: > > On Fri, 22 May 2020 at 17:15, Robert Foley <robert.fo...@linaro.org> wrote: > > > > For example: > > WARNING: ThreadSanitizer: data race (pid=11134) > > Atomic write of size 4 at 0x7bbc0000e0ac by main thread (mutexes: write > > M875): > > #0 __tsan_atomic32_store <null> (qemu-system-aarch64+0x394d84) > > #1 cpu_reset_interrupt hw/core/cpu.c:107:5 > > (qemu-system-aarch64+0x842f90) > > #2 arm_cpu_set_irq target/arm/cpu.c (qemu-system-aarch64+0x615a55) > > > > Previous read of size 4 at 0x7bbc0000e0ac by thread T7: > > #0 arm_cpu_has_work target/arm/cpu.c:78:16 > > (qemu-system-aarch64+0x6178ba) > > #1 cpu_has_work include/hw/core/cpu.h:700:12 > > (qemu-system-aarch64+0x68be2e) > > > > Cc: Peter Maydell <peter.mayd...@linaro.org> > > Cc: Richard Henderson <richard.hender...@linaro.org> > > Signed-off-by: Robert Foley <robert.fo...@linaro.org> > > --- > > target/arm/cpu.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/target/arm/cpu.c b/target/arm/cpu.c > > index 32bec156f2..cdb90582ee 100644 > > --- a/target/arm/cpu.c > > +++ b/target/arm/cpu.c > > @@ -75,7 +75,7 @@ static bool arm_cpu_has_work(CPUState *cs) > > ARMCPU *cpu = ARM_CPU(cs); > > > > return (cpu->power_state != PSCI_OFF) > > - && cs->interrupt_request & > > + && atomic_read(&cs->interrupt_request) & > > (CPU_INTERRUPT_FIQ | CPU_INTERRUPT_HARD > > | CPU_INTERRUPT_VFIQ | CPU_INTERRUPT_VIRQ > > | CPU_INTERRUPT_EXITTB); > > Every target's has_work function seems to access > cs->interrupt_request without using atomic_read() : > why does Arm need to do something special here? > > More generally, the only place that currently > uses atomic_read() on the interrupt_request field > is cpu_handle_interrupt(), so if this field needs > special precautions to access then a lot of code > needs updating.
TSan flagged this case as a potential data race. It does not mean necessarily that there is an issue here, just that two threads were accessing the data without TSan detecting the synchronization. TSan gives a few options to silence the warning, such as changing the locking, making it atomic, or adding various types of annotations to tell TSan to ignore it. So in this case we had a few options, such as to change it to atomic or to simply annotate it and silence it. We started our TSan testing using arm, and have been working to iron out the TSan warnings there, and there alone initially. Assuming that we are OK with making this particular change, to silence the TSan warning, then certainly it is a good point that we need to consider changing the other places that access this field, since they will all see similar TSan warnings. Of course if we are not OK with these changes to silence the TSan tool, that's OK too :). In that case we can certainly just add an annotation either in the code or via our suppressions/blacklist and leave the code functionally unchanged. Thanks & Regards, -Rob > > thanks > -- PMM