Re: [PATCH 6/6] target/microblaze: Set OPB bits in tlb_fill, not in transaction_failed

["Edgar E. Iglesias" <edgar.iglesias@gmail.com>]

On Thu, Jun 03, 2021 at 11:03:10AM +0200, Philippe Mathieu-Daudé wrote:
> Per the 'MicroBlaze Processor Reference Guide' UG081 (v9.0),
> "Hardware Exceptions" chapter:
> 
>   Exception Causes:
> 
>   * Instruction Bus Exception
> 
>   The instruction On-chip Peripheral Bus exception is caused by an
>   active error signal from the slave (IOPB_errAck) or timeout signal
>   from the arbiter (IOPB_timeout).
> 
>   * Data Bus Exception
> 
>   The data On-chip Peripheral Bus exception is caused by an active
>   error signal from the slave (DOPB_errAck) or timeout signal from
>   the arbiter (DOPB_timeout).
> 
> the table 1-24 (Processor Version Register 2):
> 
>   * IOPBEXC:  Generate exception for IOPB error
> 
>   * DOPBEXC: Generate exception for DOPB error
> 
> and the table 2-12 (MPD Parameters):
> 
>   * C_IOPB_BUS_EXCEPTION
> 
>   Enable exception handling for IOPB bus error
> 
>   * C_DOPB_BUS_EXCEPTION
> 
>   Enable exception handling for DOPB bus error
> 
> So if PVR2.[ID]OPBEXC feature is disabled, no exception will be
> generated. Thus we can not get to the transaction_failed() handler.
> The ESR bits have to be set in tlb_fill().
> 
> However we never implemented the MMU check whether the address belong
> to the On-chip Peripheral Bus interface, so simply add a stub for it,
> warning the feature is not implemented.


This doesn't look correct either...

The OPB interface that you're refering to is implemented. It's the
insn and data memory ports of the cpu. Most MB designs today use
AXI though, but the name originated from the old DT bindings.

The transaction_failed() you're editing is not related to the MMU.
It's related to bus errors (e.g device slave errors).
These are not "avoided" by the CPU, if SW issues a transactions that
results in one, the MSR_EE flag and the properties invovled here
determine if the core will generate an trap for SW to handle the
bus error or if the error will be ignored.

Cheers,
Edgar



> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  target/microblaze/helper.c    | 19 +++++++++++++++++++
>  target/microblaze/op_helper.c | 13 -------------
>  2 files changed, 19 insertions(+), 13 deletions(-)
> 
> diff --git a/target/microblaze/helper.c b/target/microblaze/helper.c
> index d537f300ca6..60e62bc0710 100644
> --- a/target/microblaze/helper.c
> +++ b/target/microblaze/helper.c
> @@ -56,6 +56,18 @@ static bool mb_cpu_access_is_secure(MicroBlazeCPU *cpu,
>      }
>  }
>  
> +/* On-chip Peripheral Bus (OPB) interface */
> +static bool mb_cpu_address_is_opb(MicroBlazeCPU *cpu,
> +                                  vaddr address, unsigned size)
> +{
> +    if (cpu->cfg.iopb_bus_exception || cpu->cfg.dopb_bus_exception) {
> +        /* TODO */
> +        warn_report_once("On-chip Peripheral Bus (OPB) interface "
> +                         "feature not implemented.");
> +    }
> +    return false;
> +}
> +
>  bool mb_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>                       MMUAccessType access_type, int mmu_idx,
>                       bool probe, uintptr_t retaddr)
> @@ -119,6 +131,13 @@ bool mb_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
>      default:
>          abort();
>      }
> +    if (mb_cpu_address_is_opb(cpu, address, size)) {
> +        if (access_type == MMU_INST_FETCH) {
> +            env->esr = ESR_EC_INSN_BUS;
> +        } else {
> +           env->esr = ESR_EC_DATA_BUS;
> +        }
> +    }
>  
>      if (cs->exception_index == EXCP_MMU) {
>          cpu_abort(cs, "recursive faults\n");
> diff --git a/target/microblaze/op_helper.c b/target/microblaze/op_helper.c
> index 1048e656e27..171c4cf99a0 100644
> --- a/target/microblaze/op_helper.c
> +++ b/target/microblaze/op_helper.c
> @@ -123,19 +123,6 @@ void mb_cpu_transaction_failed(CPUState *cs, hwaddr physaddr, vaddr addr,
>                    (access_type == MMU_DATA_LOAD ? "DATA_LOAD" : "DATA_STORE"));
>  
>      assert(env->msr & MSR_EE);
> -
> -    if (access_type == MMU_INST_FETCH) {
> -        if (!cpu->cfg.iopb_bus_exception) {
> -            return;
> -        }
> -        env->esr = ESR_EC_INSN_BUS;
> -    } else {
> -        if (!cpu->cfg.dopb_bus_exception) {
> -            return;
> -        }
> -        env->esr = ESR_EC_DATA_BUS;
> -    }
> -
>      env->ear = addr;
>      cs->exception_index = EXCP_HW_EXCP;
>      cpu_loop_exit_restore(cs, retaddr);
> -- 
> 2.26.3
>