From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45375) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YpixX-0003FK-AL for qemu-devel@nongnu.org; Tue, 05 May 2015 15:56:52 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YpixT-0008Ev-6t for qemu-devel@nongnu.org; Tue, 05 May 2015 15:56:51 -0400 Received: from mailapp01.imgtec.com ([195.59.15.196]:51934) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YpixT-0008Eh-0I for qemu-devel@nongnu.org; Tue, 05 May 2015 15:56:47 -0400 Received: from KLMAIL01.kl.imgtec.org (unknown [192.168.5.35]) by Websense Email Security Gateway with ESMTPS id 8093CA50049E0 for ; Tue, 5 May 2015 20:56:40 +0100 (IST) Message-ID: <55491FED.80007@imgtec.com> Date: Tue, 5 May 2015 20:54:21 +0100 From: Leon Alrae MIME-Version: 1.0 References: <1430493868-21452-1-git-send-email-yongbok.kim@imgtec.com> <1430493868-21452-4-git-send-email-yongbok.kim@imgtec.com> In-Reply-To: <1430493868-21452-4-git-send-email-yongbok.kim@imgtec.com> Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 3/3] target-mips: Misaligned Memory Accesses for MSA List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Yongbok Kim Cc: qemu-devel@nongnu.org On 01/05/15 16:24, Yongbok Kim wrote: > MIPS SIMD Architecture vector loads and stores require misalignment support. > MSA Memory access should work as an atomic operation. Therefore, it has to > check validity of all the addresses for the operation. As far as I can tell mips_cpu_do_unaligned_access() will be still generating AdE exceptions for MSA loads/stores in MIPS R5 which doesn't seem to be correct. > > Signed-off-by: Yongbok Kim > --- > target-mips/op_helper.c | 30 ++++++++++++++++++++++++++++++ > 1 files changed, 30 insertions(+), 0 deletions(-) > > diff --git a/target-mips/op_helper.c b/target-mips/op_helper.c > index dacc92b..89a7de6 100644 > --- a/target-mips/op_helper.c > +++ b/target-mips/op_helper.c > @@ -3571,6 +3571,24 @@ FOP_CONDN_S(sne, (float32_lt(fst1, fst0, &env->active_fpu.fp_status) > /* Element-by-element access macros */ > #define DF_ELEMENTS(df) (MSA_WRLEN / DF_BITS(df)) > > +#if !defined(CONFIG_USER_ONLY) > +static bool cpu_mips_validate_msa_block_access(CPUMIPSState *env, > + target_ulong address, int df, int rw) > +{ > + int i; > + for (i = 0; i < DF_ELEMENTS(df); i++) { Do we really need to check each element, wouldn't byte 0 and byte (MSA_WRLEN/8 - 1) be enough? > + if (!cpu_mips_validate_access(env, address + (i << df), > + address, (1 << df), rw)) { I believe this would look better if this line was aligned with the first argument of the function (and would be consistent with the helper below). > + CPUState *cs = CPU(mips_env_get_cpu(env)); > + helper_raise_exception_err(env, cs->exception_index, > + env->error_code); > + return false; > + } > + } > + return true; > +} > +#endif > + > void helper_msa_ld_df(CPUMIPSState *env, uint32_t df, uint32_t wd, uint32_t rs, > int32_t s10) > { > @@ -3578,6 +3596,12 @@ void helper_msa_ld_df(CPUMIPSState *env, uint32_t df, uint32_t wd, uint32_t rs, > target_ulong addr = env->active_tc.gpr[rs] + (s10 << df); > int i; > > +#if !defined(CONFIG_USER_ONLY) > + if (!cpu_mips_validate_msa_block_access(env, addr, df, MMU_DATA_LOAD)) { > + return; > + } Shouldn't this be called only for cases where page boundary is crossed? Otherwise I don't think this validation is needed. > +#endif > + > switch (df) { > case DF_BYTE: > for (i = 0; i < DF_ELEMENTS(DF_BYTE); i++) { > @@ -3613,6 +3637,12 @@ void helper_msa_st_df(CPUMIPSState *env, uint32_t df, uint32_t wd, uint32_t rs, > target_ulong addr = env->active_tc.gpr[rs] + (s10 << df); > int i; > > +#if !defined(CONFIG_USER_ONLY) > + if (!cpu_mips_validate_msa_block_access(env, addr, df, MMU_DATA_STORE)) { > + return; > + } Same. Thanks, Leon