[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 2/2] target-mips: Misaligned memory accesses
From: |
Leon Alrae |
Subject: |
Re: [Qemu-devel] [PATCH v2 2/2] target-mips: Misaligned memory accesses for MSA |
Date: |
Tue, 12 May 2015 10:43:22 +0100 |
User-agent: |
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20100101 Thunderbird/24.5.0 |
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