Re: [PATCH 64/77] target/microblaze: Convert mbar to decodetree

[Richard Henderson]

On 8/27/20 2:24 AM, Edgar E. Iglesias wrote:
>> +    /*
>> +     * Instruction access memory barrier.
>> +     * End the TB so that we recognize self-modified code immediately.
>> +     */
>> +    if (mbar_imm & 1) {
>> +        dc->cpustate_changed = 1;
>> +    }
> 
> This should be (mbar_imm & 1) == 0
> But even with that fixed it fails some of our tests because interrupts
> that should become visible within a couple of cycles after a
> memory data barrier can now be delayed longer.
> 
> I think we should always break the TB.

Ok.  I assume this follows a write to something like an interrupt controller
register?

> 
>> +    /* Data access memory barrier.  */
>> +    if (mbar_imm & 2) {
>> +        tcg_gen_mb(TCG_BAR_SC | TCG_MO_ALL);
>> +    }
> 
> This should be (mbar_imm & 2) == 0

Oops.  ;-)

Applying the following incremental patch.


r~


diff --git a/target/microblaze/translate.c b/target/microblaze/translate.c
index a391e80fb9..1e2bb529ac 100644
--- a/target/microblaze/translate.c
+++ b/target/microblaze/translate.c
@@ -1170,16 +1170,8 @@ static bool trans_mbar(DisasContext *dc, arg_mbar *arg)
 {
     int mbar_imm = arg->imm;

-    /*
-     * Instruction access memory barrier.
-     * End the TB so that we recognize self-modified code immediately.
-     */
-    if (mbar_imm & 1) {
-        dc->cpustate_changed = 1;
-    }
-
     /* Data access memory barrier.  */
-    if (mbar_imm & 2) {
+    if ((mbar_imm & 2) == 0) {
         tcg_gen_mb(TCG_BAR_SC | TCG_MO_ALL);
     }

@@ -1204,6 +1196,19 @@ static bool trans_mbar(DisasContext *dc, arg_mbar *arg)

         gen_raise_exception(dc, EXCP_HLT);
     }
+
+    /*
+     * If !(mbar_imm & 1), this is an instruction access memory barrier
+     * and we need to end the TB so that we recognize self-modified
+     * code immediately.
+     *
+     * However, there are some data mbars that need the TB break
+     * (and return to main loop) to recognize interrupts right away.
+     * E.g. recognizing a change to an interrupt controller register.
+     *
+     * Therefore, choose to end the TB always.
+     */
+    dc->cpustate_changed = 1;
     return true;
 }