From: Claudio Fontana <claudio.fontana@huawei.com>
To: Peter Maydell <peter.maydell@linaro.org>, qemu-devel@nongnu.org
Cc: patches@linaro.org
Subject: Re: [Qemu-devel] [PATCH 4/5] target-arm/translate.c: Don't pass CPUARMState around in the decoder
Date: Tue, 4 Nov 2014 09:57:42 +0100 [thread overview]
Message-ID: <54589506.4070208@huawei.com> (raw)
In-Reply-To: <1414524244-20316-5-git-send-email-peter.maydell@linaro.org>
Reviewed-by: Claudio Fontana <claudio.fontana@huawei.com>
On 28.10.2014 20:24, Peter Maydell wrote:
> Passing the CPUARMState around in the decoder is a recipe for
> bugs where we accidentally generate code that depends on CPU
> state which isn't reflected in the TB flags. Stop doing this
> and instead use DisasContext as a way to pass around those
> bits of CPU state which are known to be safe to use.
>
> This commit simply removes initial "CPUARMState *env" parameters
> from various function definitions, and removes the initial "env"
> argument from the places where those functions are called.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> target-arm/translate.c | 94 +++++++++++++++++++++++++++-----------------------
> 1 file changed, 50 insertions(+), 44 deletions(-)
>
> diff --git a/target-arm/translate.c b/target-arm/translate.c
> index 5119fb9..9e2dda2 100644
> --- a/target-arm/translate.c
> +++ b/target-arm/translate.c
> @@ -834,8 +834,7 @@ static inline void gen_bx(DisasContext *s, TCGv_i32 var)
> /* Variant of store_reg which uses branch&exchange logic when storing
> to r15 in ARM architecture v7 and above. The source must be a temporary
> and will be marked as dead. */
> -static inline void store_reg_bx(CPUARMState *env, DisasContext *s,
> - int reg, TCGv_i32 var)
> +static inline void store_reg_bx(DisasContext *s, int reg, TCGv_i32 var)
> {
> if (reg == 15 && ENABLE_ARCH_7) {
> gen_bx(s, var);
> @@ -848,8 +847,7 @@ static inline void store_reg_bx(CPUARMState *env, DisasContext *s,
> * to r15 in ARM architecture v5T and above. This is used for storing
> * the results of a LDR/LDM/POP into r15, and corresponds to the cases
> * in the ARM ARM which use the LoadWritePC() pseudocode function. */
> -static inline void store_reg_from_load(CPUARMState *env, DisasContext *s,
> - int reg, TCGv_i32 var)
> +static inline void store_reg_from_load(DisasContext *s, int reg, TCGv_i32 var)
> {
> if (reg == 15 && ENABLE_ARCH_5) {
> gen_bx(s, var);
> @@ -1541,7 +1539,7 @@ static inline int gen_iwmmxt_shift(uint32_t insn, uint32_t mask, TCGv_i32 dest)
>
> /* Disassemble an iwMMXt instruction. Returns nonzero if an error occurred
> (ie. an undefined instruction). */
> -static int disas_iwmmxt_insn(CPUARMState *env, DisasContext *s, uint32_t insn)
> +static int disas_iwmmxt_insn(DisasContext *s, uint32_t insn)
> {
> int rd, wrd;
> int rdhi, rdlo, rd0, rd1, i;
> @@ -2547,7 +2545,7 @@ static int disas_iwmmxt_insn(CPUARMState *env, DisasContext *s, uint32_t insn)
>
> /* Disassemble an XScale DSP instruction. Returns nonzero if an error occurred
> (ie. an undefined instruction). */
> -static int disas_dsp_insn(CPUARMState *env, DisasContext *s, uint32_t insn)
> +static int disas_dsp_insn(DisasContext *s, uint32_t insn)
> {
> int acc, rd0, rd1, rdhi, rdlo;
> TCGv_i32 tmp, tmp2;
> @@ -2966,7 +2964,7 @@ static const uint8_t fp_decode_rm[] = {
> FPROUNDING_NEGINF,
> };
>
> -static int disas_vfp_v8_insn(CPUARMState *env, DisasContext *s, uint32_t insn)
> +static int disas_vfp_v8_insn(DisasContext *s, uint32_t insn)
> {
> uint32_t rd, rn, rm, dp = extract32(insn, 8, 1);
>
> @@ -3002,7 +3000,7 @@ static int disas_vfp_v8_insn(CPUARMState *env, DisasContext *s, uint32_t insn)
>
> /* Disassemble a VFP instruction. Returns nonzero if an error occurred
> (ie. an undefined instruction). */
> -static int disas_vfp_insn(CPUARMState * env, DisasContext *s, uint32_t insn)
> +static int disas_vfp_insn(DisasContext *s, uint32_t insn)
> {
> uint32_t rd, rn, rm, op, i, n, offset, delta_d, delta_m, bank_mask;
> int dp, veclen;
> @@ -3039,7 +3037,7 @@ static int disas_vfp_insn(CPUARMState * env, DisasContext *s, uint32_t insn)
> /* Encodings with T=1 (Thumb) or unconditional (ARM):
> * only used in v8 and above.
> */
> - return disas_vfp_v8_insn(env, s, insn);
> + return disas_vfp_v8_insn(s, insn);
> }
>
> dp = ((insn & 0xf00) == 0xb00);
> @@ -3962,7 +3960,8 @@ static inline void gen_mulxy(TCGv_i32 t0, TCGv_i32 t1, int x, int y)
> }
>
> /* Return the mask of PSR bits set by a MSR instruction. */
> -static uint32_t msr_mask(CPUARMState *env, DisasContext *s, int flags, int spsr) {
> +static uint32_t msr_mask(DisasContext *s, int flags, int spsr)
> +{
> uint32_t mask;
>
> mask = 0;
> @@ -4312,7 +4311,7 @@ static struct {
>
> /* Translate a NEON load/store element instruction. Return nonzero if the
> instruction is invalid. */
> -static int disas_neon_ls_insn(CPUARMState * env, DisasContext *s, uint32_t insn)
> +static int disas_neon_ls_insn(DisasContext *s, uint32_t insn)
> {
> int rd, rn, rm;
> int op;
> @@ -5054,7 +5053,7 @@ static const uint8_t neon_2rm_sizes[] = {
> We process data in a mixture of 32-bit and 64-bit chunks.
> Mostly we use 32-bit chunks so we can use normal scalar instructions. */
>
> -static int disas_neon_data_insn(CPUARMState * env, DisasContext *s, uint32_t insn)
> +static int disas_neon_data_insn(DisasContext *s, uint32_t insn)
> {
> int op;
> int q;
> @@ -7049,7 +7048,7 @@ static int disas_neon_data_insn(CPUARMState * env, DisasContext *s, uint32_t ins
> return 0;
> }
>
> -static int disas_coproc_insn(CPUARMState * env, DisasContext *s, uint32_t insn)
> +static int disas_coproc_insn(DisasContext *s, uint32_t insn)
> {
> int cpnum, is64, crn, crm, opc1, opc2, isread, rt, rt2;
> const ARMCPRegInfo *ri;
> @@ -7062,9 +7061,9 @@ static int disas_coproc_insn(CPUARMState * env, DisasContext *s, uint32_t insn)
> return 1;
> }
> if (arm_dc_feature(s, ARM_FEATURE_IWMMXT)) {
> - return disas_iwmmxt_insn(env, s, insn);
> + return disas_iwmmxt_insn(s, insn);
> } else if (arm_dc_feature(s, ARM_FEATURE_XSCALE)) {
> - return disas_dsp_insn(env, s, insn);
> + return disas_dsp_insn(s, insn);
> }
> return 1;
> }
> @@ -7592,8 +7591,9 @@ static void disas_arm_insn(CPUARMState * env, DisasContext *s)
> goto illegal_op;
> }
>
> - if (disas_neon_data_insn(env, s, insn))
> + if (disas_neon_data_insn(s, insn)) {
> goto illegal_op;
> + }
> return;
> }
> if ((insn & 0x0f100000) == 0x04000000) {
> @@ -7602,13 +7602,14 @@ static void disas_arm_insn(CPUARMState * env, DisasContext *s)
> goto illegal_op;
> }
>
> - if (disas_neon_ls_insn(env, s, insn))
> + if (disas_neon_ls_insn(s, insn)) {
> goto illegal_op;
> + }
> return;
> }
> if ((insn & 0x0f000e10) == 0x0e000a00) {
> /* VFP. */
> - if (disas_vfp_insn(env, s, insn)) {
> + if (disas_vfp_insn(s, insn)) {
> goto illegal_op;
> }
> return;
> @@ -7732,7 +7733,7 @@ static void disas_arm_insn(CPUARMState * env, DisasContext *s)
> if (arm_dc_feature(s, ARM_FEATURE_IWMMXT)) {
> /* iWMMXt register transfer. */
> if (extract32(s->c15_cpar, 1, 1)) {
> - if (!disas_iwmmxt_insn(env, s, insn)) {
> + if (!disas_iwmmxt_insn(s, insn)) {
> return;
> }
> }
> @@ -7805,8 +7806,10 @@ static void disas_arm_insn(CPUARMState * env, DisasContext *s)
> if (shift)
> val = (val >> shift) | (val << (32 - shift));
> i = ((insn & (1 << 22)) != 0);
> - if (gen_set_psr_im(s, msr_mask(env, s, (insn >> 16) & 0xf, i), i, val))
> + if (gen_set_psr_im(s, msr_mask(s, (insn >> 16) & 0xf, i),
> + i, val)) {
> goto illegal_op;
> + }
> }
> }
> } else if ((insn & 0x0f900000) == 0x01000000
> @@ -7821,7 +7824,7 @@ static void disas_arm_insn(CPUARMState * env, DisasContext *s)
> /* PSR = reg */
> tmp = load_reg(s, rm);
> i = ((op1 & 2) != 0);
> - if (gen_set_psr(s, msr_mask(env, s, (insn >> 16) & 0xf, i), i, tmp))
> + if (gen_set_psr(s, msr_mask(s, (insn >> 16) & 0xf, i), i, tmp))
> goto illegal_op;
> } else {
> /* reg = PSR */
> @@ -8059,14 +8062,14 @@ static void disas_arm_insn(CPUARMState * env, DisasContext *s)
> if (logic_cc) {
> gen_logic_CC(tmp);
> }
> - store_reg_bx(env, s, rd, tmp);
> + store_reg_bx(s, rd, tmp);
> break;
> case 0x01:
> tcg_gen_xor_i32(tmp, tmp, tmp2);
> if (logic_cc) {
> gen_logic_CC(tmp);
> }
> - store_reg_bx(env, s, rd, tmp);
> + store_reg_bx(s, rd, tmp);
> break;
> case 0x02:
> if (set_cc && rd == 15) {
> @@ -8082,7 +8085,7 @@ static void disas_arm_insn(CPUARMState * env, DisasContext *s)
> } else {
> tcg_gen_sub_i32(tmp, tmp, tmp2);
> }
> - store_reg_bx(env, s, rd, tmp);
> + store_reg_bx(s, rd, tmp);
> }
> break;
> case 0x03:
> @@ -8091,7 +8094,7 @@ static void disas_arm_insn(CPUARMState * env, DisasContext *s)
> } else {
> tcg_gen_sub_i32(tmp, tmp2, tmp);
> }
> - store_reg_bx(env, s, rd, tmp);
> + store_reg_bx(s, rd, tmp);
> break;
> case 0x04:
> if (set_cc) {
> @@ -8099,7 +8102,7 @@ static void disas_arm_insn(CPUARMState * env, DisasContext *s)
> } else {
> tcg_gen_add_i32(tmp, tmp, tmp2);
> }
> - store_reg_bx(env, s, rd, tmp);
> + store_reg_bx(s, rd, tmp);
> break;
> case 0x05:
> if (set_cc) {
> @@ -8107,7 +8110,7 @@ static void disas_arm_insn(CPUARMState * env, DisasContext *s)
> } else {
> gen_add_carry(tmp, tmp, tmp2);
> }
> - store_reg_bx(env, s, rd, tmp);
> + store_reg_bx(s, rd, tmp);
> break;
> case 0x06:
> if (set_cc) {
> @@ -8115,7 +8118,7 @@ static void disas_arm_insn(CPUARMState * env, DisasContext *s)
> } else {
> gen_sub_carry(tmp, tmp, tmp2);
> }
> - store_reg_bx(env, s, rd, tmp);
> + store_reg_bx(s, rd, tmp);
> break;
> case 0x07:
> if (set_cc) {
> @@ -8123,7 +8126,7 @@ static void disas_arm_insn(CPUARMState * env, DisasContext *s)
> } else {
> gen_sub_carry(tmp, tmp2, tmp);
> }
> - store_reg_bx(env, s, rd, tmp);
> + store_reg_bx(s, rd, tmp);
> break;
> case 0x08:
> if (set_cc) {
> @@ -8156,7 +8159,7 @@ static void disas_arm_insn(CPUARMState * env, DisasContext *s)
> if (logic_cc) {
> gen_logic_CC(tmp);
> }
> - store_reg_bx(env, s, rd, tmp);
> + store_reg_bx(s, rd, tmp);
> break;
> case 0x0d:
> if (logic_cc && rd == 15) {
> @@ -8169,7 +8172,7 @@ static void disas_arm_insn(CPUARMState * env, DisasContext *s)
> if (logic_cc) {
> gen_logic_CC(tmp2);
> }
> - store_reg_bx(env, s, rd, tmp2);
> + store_reg_bx(s, rd, tmp2);
> }
> break;
> case 0x0e:
> @@ -8177,7 +8180,7 @@ static void disas_arm_insn(CPUARMState * env, DisasContext *s)
> if (logic_cc) {
> gen_logic_CC(tmp);
> }
> - store_reg_bx(env, s, rd, tmp);
> + store_reg_bx(s, rd, tmp);
> break;
> default:
> case 0x0f:
> @@ -8185,7 +8188,7 @@ static void disas_arm_insn(CPUARMState * env, DisasContext *s)
> if (logic_cc) {
> gen_logic_CC(tmp2);
> }
> - store_reg_bx(env, s, rd, tmp2);
> + store_reg_bx(s, rd, tmp2);
> break;
> }
> if (op1 != 0x0f && op1 != 0x0d) {
> @@ -8823,7 +8826,7 @@ static void disas_arm_insn(CPUARMState * env, DisasContext *s)
> }
> if (insn & (1 << 20)) {
> /* Complete the load. */
> - store_reg_from_load(env, s, rd, tmp);
> + store_reg_from_load(s, rd, tmp);
> }
> break;
> case 0x08:
> @@ -8886,7 +8889,7 @@ static void disas_arm_insn(CPUARMState * env, DisasContext *s)
> loaded_var = tmp;
> loaded_base = 1;
> } else {
> - store_reg_from_load(env, s, i, tmp);
> + store_reg_from_load(s, i, tmp);
> }
> } else {
> /* store */
> @@ -8969,10 +8972,10 @@ static void disas_arm_insn(CPUARMState * env, DisasContext *s)
> case 0xe:
> if (((insn >> 8) & 0xe) == 10) {
> /* VFP. */
> - if (disas_vfp_insn(env, s, insn)) {
> + if (disas_vfp_insn(s, insn)) {
> goto illegal_op;
> }
> - } else if (disas_coproc_insn(env, s, insn)) {
> + } else if (disas_coproc_insn(s, insn)) {
> /* Coprocessor. */
> goto illegal_op;
> }
> @@ -9453,7 +9456,7 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
> gen_arm_shift_reg(tmp, op, tmp2, logic_cc);
> if (logic_cc)
> gen_logic_CC(tmp);
> - store_reg_bx(env, s, rd, tmp);
> + store_reg_bx(s, rd, tmp);
> break;
> case 1: /* Sign/zero extend. */
> tmp = load_reg(s, rm);
> @@ -9735,17 +9738,19 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
> if (((insn >> 24) & 3) == 3) {
> /* Translate into the equivalent ARM encoding. */
> insn = (insn & 0xe2ffffff) | ((insn & (1 << 28)) >> 4) | (1 << 28);
> - if (disas_neon_data_insn(env, s, insn))
> + if (disas_neon_data_insn(s, insn)) {
> goto illegal_op;
> + }
> } else if (((insn >> 8) & 0xe) == 10) {
> - if (disas_vfp_insn(env, s, insn)) {
> + if (disas_vfp_insn(s, insn)) {
> goto illegal_op;
> }
> } else {
> if (insn & (1 << 28))
> goto illegal_op;
> - if (disas_coproc_insn (env, s, insn))
> + if (disas_coproc_insn(s, insn)) {
> goto illegal_op;
> + }
> }
> break;
> case 8: case 9: case 10: case 11:
> @@ -9821,7 +9826,7 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
> }
> tmp = load_reg(s, rn);
> if (gen_set_psr(s,
> - msr_mask(env, s, (insn >> 8) & 0xf, op == 1),
> + msr_mask(s, (insn >> 8) & 0xf, op == 1),
> op == 1, tmp))
> goto illegal_op;
> break;
> @@ -10087,8 +10092,9 @@ static int disas_thumb2_insn(CPUARMState *env, DisasContext *s, uint16_t insn_hw
> int writeback = 0;
> int memidx;
> if ((insn & 0x01100000) == 0x01000000) {
> - if (disas_neon_ls_insn(env, s, insn))
> + if (disas_neon_ls_insn(s, insn)) {
> goto illegal_op;
> + }
> break;
> }
> op = ((insn >> 21) & 3) | ((insn >> 22) & 4);
> @@ -10784,7 +10790,7 @@ static void disas_thumb_insn(CPUARMState *env, DisasContext *s)
> store_reg(s, 13, addr);
> /* set the new PC value */
> if ((insn & 0x0900) == 0x0900) {
> - store_reg_from_load(env, s, 15, tmp);
> + store_reg_from_load(s, 15, tmp);
> }
> break;
>
>
--
Claudio Fontana
Server Virtualization Architect
Huawei Technologies Duesseldorf GmbH
Riesstraße 25 - 80992 München
office: +49 89 158834 4135
mobile: +49 15253060158
next prev parent reply other threads:[~2014-11-04 8:58 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-10-28 19:23 [Qemu-devel] [PATCH 0/5] target-arm: Avoid passing CPUARMState around the decoder Peter Maydell
2014-10-28 19:24 ` [Qemu-devel] [PATCH 1/5] target-arm/translate.c: Use arm_dc_feature() in ENABLE_ARCH_ macros Peter Maydell
2014-10-31 13:09 ` Alex Bennée
2014-11-04 9:04 ` Claudio Fontana
2014-11-04 12:04 ` Peter Maydell
2014-10-28 19:24 ` [Qemu-devel] [PATCH 2/5] target-arm/translate.c: Use arm_dc_feature() rather than arm_feature() Peter Maydell
2014-10-31 13:40 ` Alex Bennée
2014-11-04 9:02 ` Claudio Fontana
2014-10-28 19:24 ` [Qemu-devel] [PATCH 3/5] target-arm/translate.c: Don't use IS_M() Peter Maydell
2014-10-31 13:42 ` Alex Bennée
2014-11-04 0:38 ` Peter Maydell
2014-11-04 8:59 ` Claudio Fontana
2014-10-28 19:24 ` [Qemu-devel] [PATCH 4/5] target-arm/translate.c: Don't pass CPUARMState around in the decoder Peter Maydell
2014-10-31 13:43 ` Alex Bennée
2014-11-04 8:57 ` Claudio Fontana [this message]
2014-10-28 19:24 ` [Qemu-devel] [PATCH 5/5] target-arm/translate.c: Don't pass CPUARMState * to disas_arm_insn() Peter Maydell
2014-10-31 13:47 ` Alex Bennée
2014-11-04 0:40 ` Peter Maydell
2014-11-04 8:55 ` Claudio Fontana
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=54589506.4070208@huawei.com \
--to=claudio.fontana@huawei.com \
--cc=patches@linaro.org \
--cc=peter.maydell@linaro.org \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.