On 29/11/2017 20:13, David Hildenbrand wrote:
> The conditional memory barrier not only looks strange but actually is
> wrong.
>
> On s390x, I can reproduce interrupts via cpu_interrupt() not leading to
> a proper kick out of emulation every now and then. cpu_interrupt() is
> especially used for inter CPU communication via SIGP (esp. external
> calls and emergency interrupts).
>
> With this patch, I was not able to reproduce. (esp. no stalls or hangs
> in the guest).
>
> My setup is s390x MTTCG with 16 VCPUs on 8 CPU host, running make -j16.
>
> Signed-off-by: David Hildenbrand <da...@redhat.com>
> ---
> accel/tcg/cpu-exec.c | 12 ++++--------
> 1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
> index 9b544d88c8..dfba5ebd29 100644
> --- a/accel/tcg/cpu-exec.c
> +++ b/accel/tcg/cpu-exec.c
> @@ -525,19 +525,15 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
> TranslationBlock **last_tb)
> {
> CPUClass *cc = CPU_GET_CLASS(cpu);
> - int32_t insns_left;
>
> /* Clear the interrupt flag now since we're processing
> * cpu->interrupt_request and cpu->exit_request.
> */
> - insns_left = atomic_read(&cpu->icount_decr.u32);
> atomic_set(&cpu->icount_decr.u16.high, 0);
> - if (unlikely(insns_left < 0)) {
> - /* Ensure the zeroing of icount_decr comes before the next read
> - * of cpu->exit_request or cpu->interrupt_request.
> - */
> - smp_mb();
> - }
> + /* Ensure zeroing happens before reading cpu->exit_request or
> + * cpu->interrupt_request. (also see cpu_exit())
> + */
> + smp_mb();
>
> if (unlikely(atomic_read(&cpu->interrupt_request))) {
> int interrupt_request;
>
atomic_mb_set can be a little faster on x86, so:
diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index dfba5ebd29..4452cd9856 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -528,12 +528,10 @@ static inline bool cpu_handle_interrupt(CPUState *cpu,
/* Clear the interrupt flag now since we're processing
* cpu->interrupt_request and cpu->exit_request.
+ * Ensure zeroing happens before reading cpu->exit_request or
+ * cpu->interrupt_request (see also smp_wmb in cpu_exit())
*/
- atomic_set(&cpu->icount_decr.u16.high, 0);
- /* Ensure zeroing happens before reading cpu->exit_request or
- * cpu->interrupt_request. (also see cpu_exit())
- */
- smp_mb();
+ atomic_mb_set(&cpu->icount_decr.u16.high, 0);
if (unlikely(atomic_read(&cpu->interrupt_request))) {
int interrupt_request;
