Re: [Qemu-devel] [PATCH v2 2/2] target-mips: Misaligned memory accesses for MSA

[Leon Alrae <leon.alrae@imgtec.com>]

On 11/05/2015 12:30, Yongbok Kim wrote:
> @@ -391,6 +391,37 @@ hwaddr cpu_mips_translate_address(CPUMIPSState *env, target_ulong address, int r
>      }
>  }
>  
> +bool cpu_mips_validate_msa_block_access(CPUMIPSState *env, target_ulong addr,
> +                                        int rw, int mmu_idx)
> +{
> +    target_ulong vaddr = addr & TARGET_PAGE_MASK;

This deserves more descriptive name, maybe "page_addr"?

> +    target_ulong badvaddr = addr;
> +
> +    CPUState *cs = CPU(mips_env_get_cpu(env));
> +    int ret;
> +
> +    ret = mips_cpu_handle_mmu_fault(cs, vaddr, rw, mmu_idx);
> +    if (ret != TLBRET_MATCH) {
> +        /* calling raise_mmu_exeception again to correct badvaddr */
> +        raise_mmu_exception(env, badvaddr, rw, ret);

mips_cpu_handle_mmu_fault() already calls raise_mmu_exception() where
appropriate registers get updated. Why calling it again here?

> +        return false;
> +    }
> +    if (unlikely(((addr & ~TARGET_PAGE_MASK) + MSA_WRLEN - 1)
> +                 >= TARGET_PAGE_SIZE)) {

This isn’t required, you already do this before calling this function.

> +        vaddr += TARGET_PAGE_SIZE;
> +        ret = mips_cpu_handle_mmu_fault(cs, vaddr, rw, mmu_idx);
> +        if (ret != TLBRET_MATCH) {
> +            if (ret != TLBRET_BADADDR) {
> +                badvaddr = vaddr;
> +            }
> +            /* calling raise_mmu_exeception again to correct badvaddr */
> +            raise_mmu_exception(env, badvaddr, rw, ret);
> +            return false;
> +        }
> +    }
> +    return true;
> +}
> +
>  static const char * const excp_names[EXCP_LAST + 1] = {
>      [EXCP_RESET] = "reset",
>      [EXCP_SRESET] = "soft reset",
> @@ -3571,33 +3576,47 @@ void helper_msa_ld_df(CPUMIPSState *env, uint32_t df, uint32_t wd, uint32_t rs,
>      wr_t *pwd = &(env->active_fpu.fpr[wd].wr);
>      target_ulong addr = env->active_tc.gpr[rs] + (s10 << df);
>      int i;
> +    int mmu_idx = cpu_mmu_index(env);
> +
> +#if !defined(CONFIG_USER_ONLY)
> +    if (unlikely(((addr & ~TARGET_PAGE_MASK) + MSA_WRLEN - 1)
> +                 >= TARGET_PAGE_SIZE)) {

MSA_WRLEN/8

> +        if (!cpu_mips_validate_msa_block_access(env, addr, MMU_DATA_LOAD,
> +                                                mmu_idx)) {
> +            CPUState *cs = CPU(mips_env_get_cpu(env));
> +            helper_raise_exception_err(env, cs->exception_index,
> +                                       env->error_code);

Wouldn’t it be better to fold it into cpu_mips_validate_msa_block_access()?

Thanks,
Leon