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

[Edgar E. Iglesias]

On Thu, Aug 27, 2020 at 02:58:06AM -0700, Richard Henderson wrote:
> 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?

yes, kind of. It's a memory store to a device that raises an interrupt as a
result of that store. The interrupt propagates to the CPU and on real HW if
the pipeline depth of the core is small, it gets taken within a couple of
cycles after the barrier completes. In QEMU, that delay can become very long
if we don't break the TB.

Architecturally, it would be wrong to make such assumptions about the pipeline
but there's code out there already..



> 
> > 
> >> +    /* 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.

Thanks! With your incremental patch:

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>



> 
> 
> 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;
>  }
>