* [Qemu-devel] Add proper alignment check and pending 'C' extension for riscv @ 2019-02-22 16:25 ` amagdy.afifi 0 siblings, 0 replies; 20+ messages in thread From: amagdy.afifi @ 2019-02-22 16:25 UTC (permalink / raw) To: qemu-devel Cc: qemu-riscv, mjc, palmer, Alistair.Francis, sagark, kbastian, amagdy.afifi Dear All, I'm submiting this patch to properly check the next instruction alignment and scheduale compression extenstion enable upon 'MISA' register writes to later aligned instruction through exporting next instruction 'pc' to riscv cpu state Thanks, Ahmed ^ permalink raw reply [flat|nested] 20+ messages in thread
* [Qemu-riscv] Add proper alignment check and pending 'C' extension for riscv @ 2019-02-22 16:25 ` amagdy.afifi 0 siblings, 0 replies; 20+ messages in thread From: amagdy.afifi @ 2019-02-22 16:25 UTC (permalink / raw) To: qemu-devel Cc: qemu-riscv, mjc, palmer, Alistair.Francis, sagark, kbastian, amagdy.afifi Dear All, I'm submiting this patch to properly check the next instruction alignment and scheduale compression extenstion enable upon 'MISA' register writes to later aligned instruction through exporting next instruction 'pc' to riscv cpu state Thanks, Ahmed ^ permalink raw reply [flat|nested] 20+ messages in thread
* [Qemu-devel] [PATCH] riscv: Add proper alignment check and pending 'C' extension upon misa writes 2019-02-22 16:25 ` [Qemu-riscv] " amagdy.afifi @ 2019-02-22 16:25 ` amagdy.afifi -1 siblings, 0 replies; 20+ messages in thread From: amagdy.afifi @ 2019-02-22 16:25 UTC (permalink / raw) To: qemu-devel Cc: qemu-riscv, mjc, palmer, Alistair.Francis, sagark, kbastian, amagdy.afifi From: ahmed_magdy <amagdy.afifi@gmail.com> Signed-off-by: ahmed_magdy <amagdy.afifi@gmail.com> --- target/riscv/cpu.h | 2 ++ target/riscv/csr.c | 5 +++-- target/riscv/translate.c | 14 ++++++++++---- 3 files changed, 15 insertions(+), 6 deletions(-) diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h index a269c07..b49bdb3 100644 --- a/target/riscv/cpu.h +++ b/target/riscv/cpu.h @@ -109,6 +109,8 @@ struct CPURISCVState { target_ulong gpr[32]; uint64_t fpr[32]; /* assume both F and D extensions */ target_ulong pc; + target_ulong pc_next; /* next target pc */ + int pending_rvc; target_ulong load_res; target_ulong load_val; diff --git a/target/riscv/csr.c b/target/riscv/csr.c index 90f6866..b0886f0 100644 --- a/target/riscv/csr.c +++ b/target/riscv/csr.c @@ -373,9 +373,10 @@ static int write_misa(CPURISCVState *env, int csrno, target_ulong val) } /* Suppress 'C' if next instruction is not aligned - TODO: this should check next_pc */ - if ((val & RVC) && (GETPC() & ~3) != 0) { + check next target pc */ + if ((val & RVC) && (env->pc_next & 3) != 0) { val &= ~RVC; + env->pending_rvc = 1; } /* misa.MXL writes are not supported by QEMU */ diff --git a/target/riscv/translate.c b/target/riscv/translate.c index 2321bba..c9d84ea 100644 --- a/target/riscv/translate.c +++ b/target/riscv/translate.c @@ -1999,20 +1999,26 @@ static void decode_RV32_64G(DisasContext *ctx) } } -static void decode_opc(DisasContext *ctx) +static void decode_opc(DisasContext *ctx, CPUState *cpu) { + CPURISCVState *env = cpu->env_ptr; /* check for compressed insn */ if (extract32(ctx->opcode, 0, 2) != 3) { if (!has_ext(ctx, RVC)) { gen_exception_illegal(ctx); } else { - ctx->pc_succ_insn = ctx->base.pc_next + 2; + env->pc_next = ctx->pc_succ_insn = ctx->base.pc_next + 2; decode_RV32_64C(ctx); } } else { - ctx->pc_succ_insn = ctx->base.pc_next + 4; + env->pc_next = ctx->pc_succ_insn = ctx->base.pc_next + 4; decode_RV32_64G(ctx); } + /* check pending RVC */ + if (env->pending_rvc && ((env->pc_next & 3) != 0)) { + env->misa |= RVC; + env->pending_rvc = 0; + } } static void riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cpu) @@ -2061,7 +2067,7 @@ static void riscv_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu) CPURISCVState *env = cpu->env_ptr; ctx->opcode = cpu_ldl_code(env, ctx->base.pc_next); - decode_opc(ctx); + decode_opc(ctx, cpu); ctx->base.pc_next = ctx->pc_succ_insn; if (ctx->base.is_jmp == DISAS_NEXT) { -- 1.9.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [Qemu-riscv] [PATCH] riscv: Add proper alignment check and pending 'C' extension upon misa writes @ 2019-02-22 16:25 ` amagdy.afifi 0 siblings, 0 replies; 20+ messages in thread From: amagdy.afifi @ 2019-02-22 16:25 UTC (permalink / raw) To: qemu-devel Cc: qemu-riscv, mjc, palmer, Alistair.Francis, sagark, kbastian, amagdy.afifi From: ahmed_magdy <amagdy.afifi@gmail.com> Signed-off-by: ahmed_magdy <amagdy.afifi@gmail.com> --- target/riscv/cpu.h | 2 ++ target/riscv/csr.c | 5 +++-- target/riscv/translate.c | 14 ++++++++++---- 3 files changed, 15 insertions(+), 6 deletions(-) diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h index a269c07..b49bdb3 100644 --- a/target/riscv/cpu.h +++ b/target/riscv/cpu.h @@ -109,6 +109,8 @@ struct CPURISCVState { target_ulong gpr[32]; uint64_t fpr[32]; /* assume both F and D extensions */ target_ulong pc; + target_ulong pc_next; /* next target pc */ + int pending_rvc; target_ulong load_res; target_ulong load_val; diff --git a/target/riscv/csr.c b/target/riscv/csr.c index 90f6866..b0886f0 100644 --- a/target/riscv/csr.c +++ b/target/riscv/csr.c @@ -373,9 +373,10 @@ static int write_misa(CPURISCVState *env, int csrno, target_ulong val) } /* Suppress 'C' if next instruction is not aligned - TODO: this should check next_pc */ - if ((val & RVC) && (GETPC() & ~3) != 0) { + check next target pc */ + if ((val & RVC) && (env->pc_next & 3) != 0) { val &= ~RVC; + env->pending_rvc = 1; } /* misa.MXL writes are not supported by QEMU */ diff --git a/target/riscv/translate.c b/target/riscv/translate.c index 2321bba..c9d84ea 100644 --- a/target/riscv/translate.c +++ b/target/riscv/translate.c @@ -1999,20 +1999,26 @@ static void decode_RV32_64G(DisasContext *ctx) } } -static void decode_opc(DisasContext *ctx) +static void decode_opc(DisasContext *ctx, CPUState *cpu) { + CPURISCVState *env = cpu->env_ptr; /* check for compressed insn */ if (extract32(ctx->opcode, 0, 2) != 3) { if (!has_ext(ctx, RVC)) { gen_exception_illegal(ctx); } else { - ctx->pc_succ_insn = ctx->base.pc_next + 2; + env->pc_next = ctx->pc_succ_insn = ctx->base.pc_next + 2; decode_RV32_64C(ctx); } } else { - ctx->pc_succ_insn = ctx->base.pc_next + 4; + env->pc_next = ctx->pc_succ_insn = ctx->base.pc_next + 4; decode_RV32_64G(ctx); } + /* check pending RVC */ + if (env->pending_rvc && ((env->pc_next & 3) != 0)) { + env->misa |= RVC; + env->pending_rvc = 0; + } } static void riscv_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cpu) @@ -2061,7 +2067,7 @@ static void riscv_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu) CPURISCVState *env = cpu->env_ptr; ctx->opcode = cpu_ldl_code(env, ctx->base.pc_next); - decode_opc(ctx); + decode_opc(ctx, cpu); ctx->base.pc_next = ctx->pc_succ_insn; if (ctx->base.is_jmp == DISAS_NEXT) { -- 1.9.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH] riscv: Add proper alignment check and pending 'C' extension upon misa writes 2019-02-22 16:25 ` [Qemu-riscv] " amagdy.afifi @ 2019-02-22 23:57 ` Richard Henderson -1 siblings, 0 replies; 20+ messages in thread From: Richard Henderson @ 2019-02-22 23:57 UTC (permalink / raw) To: amagdy.afifi, qemu-devel Cc: qemu-riscv, sagark, kbastian, palmer, mjc, Alistair.Francis On 2/22/19 8:25 AM, amagdy.afifi@gmail.com wrote: > @@ -373,9 +373,10 @@ static int write_misa(CPURISCVState *env, int csrno, target_ulong val) > } > > /* Suppress 'C' if next instruction is not aligned > - TODO: this should check next_pc */ > - if ((val & RVC) && (GETPC() & ~3) != 0) { > + check next target pc */ > + if ((val & RVC) && (env->pc_next & 3) != 0) { > val &= ~RVC; > + env->pending_rvc = 1; > } > > /* misa.MXL writes are not supported by QEMU */ > diff --git a/target/riscv/translate.c b/target/riscv/translate.c > index 2321bba..c9d84ea 100644 > --- a/target/riscv/translate.c > +++ b/target/riscv/translate.c > @@ -1999,20 +1999,26 @@ static void decode_RV32_64G(DisasContext *ctx) > } > } > > -static void decode_opc(DisasContext *ctx) > +static void decode_opc(DisasContext *ctx, CPUState *cpu) > { > + CPURISCVState *env = cpu->env_ptr; > /* check for compressed insn */ > if (extract32(ctx->opcode, 0, 2) != 3) { > if (!has_ext(ctx, RVC)) { > gen_exception_illegal(ctx); > } else { > - ctx->pc_succ_insn = ctx->base.pc_next + 2; > + env->pc_next = ctx->pc_succ_insn = ctx->base.pc_next + 2; > decode_RV32_64C(ctx); > } > } else { > - ctx->pc_succ_insn = ctx->base.pc_next + 4; > + env->pc_next = ctx->pc_succ_insn = ctx->base.pc_next + 4; > decode_RV32_64G(ctx); > } > + /* check pending RVC */ > + if (env->pending_rvc && ((env->pc_next & 3) != 0)) { > + env->misa |= RVC; > + env->pending_rvc = 0; You cannot manipulate env like this during translation. Neither the write to env->pc_next nor the read from env->pending_rvc here will be in any synchronization with the execution of write_misa. What semantics are you attempting to implement wrt setting/clearing RVC from MISA? > @@ -2061,7 +2067,7 @@ static void riscv_tr_translate_insn > CPURISCVState *env = cpu->env_ptr; > > ctx->opcode = cpu_ldl_code(env, ctx->base.pc_next); > - decode_opc(ctx); > + decode_opc(ctx, cpu); This is exactly the reason why cpu is *not* passed down to decode_opc, so that you cannot make this kind of mistake. r~ ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-riscv] [Qemu-devel] [PATCH] riscv: Add proper alignment check and pending 'C' extension upon misa writes @ 2019-02-22 23:57 ` Richard Henderson 0 siblings, 0 replies; 20+ messages in thread From: Richard Henderson @ 2019-02-22 23:57 UTC (permalink / raw) To: amagdy.afifi, qemu-devel Cc: qemu-riscv, sagark, kbastian, palmer, mjc, Alistair.Francis On 2/22/19 8:25 AM, amagdy.afifi@gmail.com wrote: > @@ -373,9 +373,10 @@ static int write_misa(CPURISCVState *env, int csrno, target_ulong val) > } > > /* Suppress 'C' if next instruction is not aligned > - TODO: this should check next_pc */ > - if ((val & RVC) && (GETPC() & ~3) != 0) { > + check next target pc */ > + if ((val & RVC) && (env->pc_next & 3) != 0) { > val &= ~RVC; > + env->pending_rvc = 1; > } > > /* misa.MXL writes are not supported by QEMU */ > diff --git a/target/riscv/translate.c b/target/riscv/translate.c > index 2321bba..c9d84ea 100644 > --- a/target/riscv/translate.c > +++ b/target/riscv/translate.c > @@ -1999,20 +1999,26 @@ static void decode_RV32_64G(DisasContext *ctx) > } > } > > -static void decode_opc(DisasContext *ctx) > +static void decode_opc(DisasContext *ctx, CPUState *cpu) > { > + CPURISCVState *env = cpu->env_ptr; > /* check for compressed insn */ > if (extract32(ctx->opcode, 0, 2) != 3) { > if (!has_ext(ctx, RVC)) { > gen_exception_illegal(ctx); > } else { > - ctx->pc_succ_insn = ctx->base.pc_next + 2; > + env->pc_next = ctx->pc_succ_insn = ctx->base.pc_next + 2; > decode_RV32_64C(ctx); > } > } else { > - ctx->pc_succ_insn = ctx->base.pc_next + 4; > + env->pc_next = ctx->pc_succ_insn = ctx->base.pc_next + 4; > decode_RV32_64G(ctx); > } > + /* check pending RVC */ > + if (env->pending_rvc && ((env->pc_next & 3) != 0)) { > + env->misa |= RVC; > + env->pending_rvc = 0; You cannot manipulate env like this during translation. Neither the write to env->pc_next nor the read from env->pending_rvc here will be in any synchronization with the execution of write_misa. What semantics are you attempting to implement wrt setting/clearing RVC from MISA? > @@ -2061,7 +2067,7 @@ static void riscv_tr_translate_insn > CPURISCVState *env = cpu->env_ptr; > > ctx->opcode = cpu_ldl_code(env, ctx->base.pc_next); > - decode_opc(ctx); > + decode_opc(ctx, cpu); This is exactly the reason why cpu is *not* passed down to decode_opc, so that you cannot make this kind of mistake. r~ ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH] riscv: Add proper alignment check and pending 'C' extension upon misa writes 2019-02-22 23:57 ` [Qemu-riscv] " Richard Henderson @ 2019-02-24 7:57 ` Amed Magdy -1 siblings, 0 replies; 20+ messages in thread From: Amed Magdy @ 2019-02-24 7:57 UTC (permalink / raw) To: Richard Henderson Cc: qemu-devel, qemu-riscv, sagark, kbastian, palmer, mjc, Alistair.Francis Thank you for your review and feedback, Richard. As Eric mentioned, this is the first time contribution. I have been exploring Qemu for some time and try to understand main flow, internals, ..etc. > You cannot manipulate env like this during translation. > Neither the write to env->pc_next nor the read from env->pending_rvc here will > be in any synchronization with the execution of write_misa. Does this applies for translated code in a single translation block only or for different TBs also ? So should I manipulate "env" from translation context through helpers only ? for example: TCGv temp; tcg_gen_movi_tl(temp, ctx->pc_succ_insn); gen_helper_next_pc(cpu_env, temp); while the helper function definition like that: void helper_next_pc(CPURISCVState *env, target_ulong pc_next) { env->pc_next = pc_next; } and the same to read "env->pending_rvc" I'm thinking of a way to add 'C' extension at run time through waiting the correct aligned instruction, which I believe it might be after branch something quite similar to switching between ARM and THUMB states in ARM, for misa 'RVC' enable to take effect since it will be no possibility to check alignment with 'C' extension. Thanks, Ahmed On Sat, 23 Feb 2019 at 01:57, Richard Henderson < richard.henderson@linaro.org> wrote: > On 2/22/19 8:25 AM, amagdy.afifi@gmail.com wrote: > > @@ -373,9 +373,10 @@ static int write_misa(CPURISCVState *env, int > csrno, target_ulong val) > > } > > > > /* Suppress 'C' if next instruction is not aligned > > - TODO: this should check next_pc */ > > - if ((val & RVC) && (GETPC() & ~3) != 0) { > > + check next target pc */ > > + if ((val & RVC) && (env->pc_next & 3) != 0) { > > val &= ~RVC; > > + env->pending_rvc = 1; > > } > > > > /* misa.MXL writes are not supported by QEMU */ > > diff --git a/target/riscv/translate.c b/target/riscv/translate.c > > index 2321bba..c9d84ea 100644 > > --- a/target/riscv/translate.c > > +++ b/target/riscv/translate.c > > @@ -1999,20 +1999,26 @@ static void decode_RV32_64G(DisasContext *ctx) > > } > > } > > > > -static void decode_opc(DisasContext *ctx) > > +static void decode_opc(DisasContext *ctx, CPUState *cpu) > > { > > + CPURISCVState *env = cpu->env_ptr; > > /* check for compressed insn */ > > if (extract32(ctx->opcode, 0, 2) != 3) { > > if (!has_ext(ctx, RVC)) { > > gen_exception_illegal(ctx); > > } else { > > - ctx->pc_succ_insn = ctx->base.pc_next + 2; > > + env->pc_next = ctx->pc_succ_insn = ctx->base.pc_next + 2; > > decode_RV32_64C(ctx); > > } > > } else { > > - ctx->pc_succ_insn = ctx->base.pc_next + 4; > > + env->pc_next = ctx->pc_succ_insn = ctx->base.pc_next + 4; > > decode_RV32_64G(ctx); > > } > > + /* check pending RVC */ > > + if (env->pending_rvc && ((env->pc_next & 3) != 0)) { > > + env->misa |= RVC; > > + env->pending_rvc = 0; > > You cannot manipulate env like this during translation. > > Neither the write to env->pc_next nor the read from env->pending_rvc here > will > be in any synchronization with the execution of write_misa. > > What semantics are you attempting to implement wrt setting/clearing RVC > from MISA? > > > @@ -2061,7 +2067,7 @@ static void riscv_tr_translate_insn > > CPURISCVState *env = cpu->env_ptr; > > > > ctx->opcode = cpu_ldl_code(env, ctx->base.pc_next); > > - decode_opc(ctx); > > + decode_opc(ctx, cpu); > > This is exactly the reason why cpu is *not* passed down to decode_opc, so > that > you cannot make this kind of mistake. > > > r~ > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-riscv] [Qemu-devel] [PATCH] riscv: Add proper alignment check and pending 'C' extension upon misa writes @ 2019-02-24 7:57 ` Amed Magdy 0 siblings, 0 replies; 20+ messages in thread From: Amed Magdy @ 2019-02-24 7:57 UTC (permalink / raw) To: Richard Henderson Cc: qemu-devel, qemu-riscv, sagark, kbastian, palmer, mjc, Alistair.Francis [-- Attachment #1: Type: text/plain, Size: 3679 bytes --] Thank you for your review and feedback, Richard. As Eric mentioned, this is the first time contribution. I have been exploring Qemu for some time and try to understand main flow, internals, ..etc. > You cannot manipulate env like this during translation. > Neither the write to env->pc_next nor the read from env->pending_rvc here will > be in any synchronization with the execution of write_misa. Does this applies for translated code in a single translation block only or for different TBs also ? So should I manipulate "env" from translation context through helpers only ? for example: TCGv temp; tcg_gen_movi_tl(temp, ctx->pc_succ_insn); gen_helper_next_pc(cpu_env, temp); while the helper function definition like that: void helper_next_pc(CPURISCVState *env, target_ulong pc_next) { env->pc_next = pc_next; } and the same to read "env->pending_rvc" I'm thinking of a way to add 'C' extension at run time through waiting the correct aligned instruction, which I believe it might be after branch something quite similar to switching between ARM and THUMB states in ARM, for misa 'RVC' enable to take effect since it will be no possibility to check alignment with 'C' extension. Thanks, Ahmed On Sat, 23 Feb 2019 at 01:57, Richard Henderson < richard.henderson@linaro.org> wrote: > On 2/22/19 8:25 AM, amagdy.afifi@gmail.com wrote: > > @@ -373,9 +373,10 @@ static int write_misa(CPURISCVState *env, int > csrno, target_ulong val) > > } > > > > /* Suppress 'C' if next instruction is not aligned > > - TODO: this should check next_pc */ > > - if ((val & RVC) && (GETPC() & ~3) != 0) { > > + check next target pc */ > > + if ((val & RVC) && (env->pc_next & 3) != 0) { > > val &= ~RVC; > > + env->pending_rvc = 1; > > } > > > > /* misa.MXL writes are not supported by QEMU */ > > diff --git a/target/riscv/translate.c b/target/riscv/translate.c > > index 2321bba..c9d84ea 100644 > > --- a/target/riscv/translate.c > > +++ b/target/riscv/translate.c > > @@ -1999,20 +1999,26 @@ static void decode_RV32_64G(DisasContext *ctx) > > } > > } > > > > -static void decode_opc(DisasContext *ctx) > > +static void decode_opc(DisasContext *ctx, CPUState *cpu) > > { > > + CPURISCVState *env = cpu->env_ptr; > > /* check for compressed insn */ > > if (extract32(ctx->opcode, 0, 2) != 3) { > > if (!has_ext(ctx, RVC)) { > > gen_exception_illegal(ctx); > > } else { > > - ctx->pc_succ_insn = ctx->base.pc_next + 2; > > + env->pc_next = ctx->pc_succ_insn = ctx->base.pc_next + 2; > > decode_RV32_64C(ctx); > > } > > } else { > > - ctx->pc_succ_insn = ctx->base.pc_next + 4; > > + env->pc_next = ctx->pc_succ_insn = ctx->base.pc_next + 4; > > decode_RV32_64G(ctx); > > } > > + /* check pending RVC */ > > + if (env->pending_rvc && ((env->pc_next & 3) != 0)) { > > + env->misa |= RVC; > > + env->pending_rvc = 0; > > You cannot manipulate env like this during translation. > > Neither the write to env->pc_next nor the read from env->pending_rvc here > will > be in any synchronization with the execution of write_misa. > > What semantics are you attempting to implement wrt setting/clearing RVC > from MISA? > > > @@ -2061,7 +2067,7 @@ static void riscv_tr_translate_insn > > CPURISCVState *env = cpu->env_ptr; > > > > ctx->opcode = cpu_ldl_code(env, ctx->base.pc_next); > > - decode_opc(ctx); > > + decode_opc(ctx, cpu); > > This is exactly the reason why cpu is *not* passed down to decode_opc, so > that > you cannot make this kind of mistake. > > > r~ > [-- Attachment #2: Type: text/html, Size: 5099 bytes --] ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH] riscv: Add proper alignment check and pending 'C' extension upon misa writes 2019-02-24 7:57 ` [Qemu-riscv] " Amed Magdy @ 2019-02-24 19:04 ` Richard Henderson -1 siblings, 0 replies; 20+ messages in thread From: Richard Henderson @ 2019-02-24 19:04 UTC (permalink / raw) To: Amed Magdy Cc: qemu-devel, qemu-riscv, sagark, kbastian, palmer, mjc, Alistair.Francis On 2/23/19 11:57 PM, Amed Magdy wrote: > Thank you for your review and feedback, Richard. > As Eric mentioned, this is the first time contribution. I have been exploring > Qemu for some time and try to understand main flow, internals, ..etc. > >> You cannot manipulate env like this during translation. > >> Neither the write to env->pc_next nor the read from env->pending_rvc here will >> be in any synchronization with the execution of write_misa. > > Does this applies for translated code in a single translation block only or for > different TBs also ? All of the time this will not work. > So should I manipulate "env" from translation context through helpers only ? > for example: > > TCGv temp; > tcg_gen_movi_tl(temp, ctx->pc_succ_insn); > gen_helper_next_pc(cpu_env, temp); > > while the helper function definition like that: > void helper_next_pc(CPURISCVState *env, target_ulong pc_next) > { > env->pc_next = pc_next; > } This is a correct way to write pc_next. (Although I still don't understand why you need it.) > and the same to read "env->pending_rvc" No, you cannot read pending_rvc this way. You would need to incorporate pending_rvc into the flags set by cpu_get_tb_cpu_state. (Although I still don't understand why you need it.) > I'm thinking of a way to add 'C' extension at run time through waiting the > correct aligned instruction, which I believe it might be after branch something > quite similar to switching between ARM and THUMB states in ARM, for misa 'RVC' > enable to take effect since it will be no possibility to check alignment with > 'C' extension. It seems to me that the C extension can be enabled at any point, since if C is off, you know that the next insn is aligned modulo 4. It is only if the C extension is enabled, and you want to disable it, that is when we must check to see if the next insn is aligned mod 4. It is trivial to arrange for a particular instruction to be aligned, via assembler directives. So it seems silly to make the definition of the csr write to misa any more complicated than it is. You are right that the existing code is broken, in that it is checking the alignment of the host PC and not the guest PC. However, I see no reason to introduce a new "pc_next" field when we already update the pc field ... > static void gen_system(DisasContext *ctx, uint32_t opc, int rd, int rs1, > int csr) > { > TCGv source1, csr_store, dest, rs1_pass, imm_rs1; > source1 = tcg_temp_new(); > csr_store = tcg_temp_new(); > dest = tcg_temp_new(); > rs1_pass = tcg_temp_new(); > imm_rs1 = tcg_temp_new(); > gen_get_gpr(source1, rs1); > tcg_gen_movi_tl(cpu_pc, ctx->base.pc_next); ... here. So as far as I can see, the only thing that needs fixing is GETPC. r~ diff --git a/target/riscv/csr.c b/target/riscv/csr.c index 960d2b0aa9..8726ef802e 100644 --- a/target/riscv/csr.c +++ b/target/riscv/csr.c @@ -370,10 +370,11 @@ static int write_misa(CPURISCVState *env, int csrno, target_ulong val) val &= ~RVD; } - /* Suppress 'C' if next instruction is not aligned - * TODO: this should check next_pc + /* + * Suppress 'C' if next instruction is not aligned. + * We updated env->pc to the next insn in the translator. */ - if ((val & RVC) && (GETPC() & ~3) != 0) { + if ((val & RVC) && (env->pc & ~3) != 0) { val &= ~RVC; } ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [Qemu-riscv] [Qemu-devel] [PATCH] riscv: Add proper alignment check and pending 'C' extension upon misa writes @ 2019-02-24 19:04 ` Richard Henderson 0 siblings, 0 replies; 20+ messages in thread From: Richard Henderson @ 2019-02-24 19:04 UTC (permalink / raw) To: Amed Magdy Cc: qemu-devel, qemu-riscv, sagark, kbastian, palmer, mjc, Alistair.Francis On 2/23/19 11:57 PM, Amed Magdy wrote: > Thank you for your review and feedback, Richard. > As Eric mentioned, this is the first time contribution. I have been exploring > Qemu for some time and try to understand main flow, internals, ..etc. > >> You cannot manipulate env like this during translation. > >> Neither the write to env->pc_next nor the read from env->pending_rvc here will >> be in any synchronization with the execution of write_misa. > > Does this applies for translated code in a single translation block only or for > different TBs also ? All of the time this will not work. > So should I manipulate "env" from translation context through helpers only ? > for example: > > TCGv temp; > tcg_gen_movi_tl(temp, ctx->pc_succ_insn); > gen_helper_next_pc(cpu_env, temp); > > while the helper function definition like that: > void helper_next_pc(CPURISCVState *env, target_ulong pc_next) > { > env->pc_next = pc_next; > } This is a correct way to write pc_next. (Although I still don't understand why you need it.) > and the same to read "env->pending_rvc" No, you cannot read pending_rvc this way. You would need to incorporate pending_rvc into the flags set by cpu_get_tb_cpu_state. (Although I still don't understand why you need it.) > I'm thinking of a way to add 'C' extension at run time through waiting the > correct aligned instruction, which I believe it might be after branch something > quite similar to switching between ARM and THUMB states in ARM, for misa 'RVC' > enable to take effect since it will be no possibility to check alignment with > 'C' extension. It seems to me that the C extension can be enabled at any point, since if C is off, you know that the next insn is aligned modulo 4. It is only if the C extension is enabled, and you want to disable it, that is when we must check to see if the next insn is aligned mod 4. It is trivial to arrange for a particular instruction to be aligned, via assembler directives. So it seems silly to make the definition of the csr write to misa any more complicated than it is. You are right that the existing code is broken, in that it is checking the alignment of the host PC and not the guest PC. However, I see no reason to introduce a new "pc_next" field when we already update the pc field ... > static void gen_system(DisasContext *ctx, uint32_t opc, int rd, int rs1, > int csr) > { > TCGv source1, csr_store, dest, rs1_pass, imm_rs1; > source1 = tcg_temp_new(); > csr_store = tcg_temp_new(); > dest = tcg_temp_new(); > rs1_pass = tcg_temp_new(); > imm_rs1 = tcg_temp_new(); > gen_get_gpr(source1, rs1); > tcg_gen_movi_tl(cpu_pc, ctx->base.pc_next); ... here. So as far as I can see, the only thing that needs fixing is GETPC. r~ diff --git a/target/riscv/csr.c b/target/riscv/csr.c index 960d2b0aa9..8726ef802e 100644 --- a/target/riscv/csr.c +++ b/target/riscv/csr.c @@ -370,10 +370,11 @@ static int write_misa(CPURISCVState *env, int csrno, target_ulong val) val &= ~RVD; } - /* Suppress 'C' if next instruction is not aligned - * TODO: this should check next_pc + /* + * Suppress 'C' if next instruction is not aligned. + * We updated env->pc to the next insn in the translator. */ - if ((val & RVC) && (GETPC() & ~3) != 0) { + if ((val & RVC) && (env->pc & ~3) != 0) { val &= ~RVC; } ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH] riscv: Add proper alignment check and pending 'C' extension upon misa writes 2019-02-24 19:04 ` [Qemu-riscv] " Richard Henderson @ 2019-02-26 7:58 ` Amed Magdy -1 siblings, 0 replies; 20+ messages in thread From: Amed Magdy @ 2019-02-26 7:58 UTC (permalink / raw) To: Richard Henderson Cc: qemu-devel, qemu-riscv, sagark, kbastian, palmer, mjc, Alistair.Francis > > It seems to me that the C extension can be enabled at any point, since > if C is > > off, you know that the next insn is aligned modulo 4. > > > Ok, This is mostly right. When C extension is enabled 32-bit base instructions can be aligned on 2 bytes boundaries instead of 4 bytes only. So multiple enables and disables of C bit at different code areas theoretically may require this check on C extension enable. I'm not really sure, may be this is might not be a practical use scenario. > It is only if the C extension is enabled, and you want to disable it, > that is > > when we must check to see if the next insn is aligned mod 4. It is > trivial to > > arrange for a particular instruction to be aligned, via assembler > directives. > > So it seems silly to make the definition of the csr write to misa any > more > > complicated than it is. > > I completely agree with you that C extension disable should have alignment check. > > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c > > index 960d2b0aa9..8726ef802e 100644 > > --- a/target/riscv/csr.c > > +++ b/target/riscv/csr.c > > @@ -370,10 +370,11 @@ static int write_misa(CPURISCVState *env, int > csrno, > > target_ulong val) > > val &= ~RVD; > > } > > > - /* Suppress 'C' if next instruction is not aligned > > - * TODO: this should check next_pc > > + /* > > + * Suppress 'C' if next instruction is not aligned. > > + * We updated env->pc to the next insn in the translator. > > */ > > - if ((val & RVC) && (GETPC() & ~3) != 0) { > > + if ((val & RVC) && (env->pc & ~3) != 0) { > > val &= ~RVC; > > } Just a hint, (env->pc & 3) instead of (env->pc & ~3) , right ? > Thanks, Ahmed ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-riscv] [Qemu-devel] [PATCH] riscv: Add proper alignment check and pending 'C' extension upon misa writes @ 2019-02-26 7:58 ` Amed Magdy 0 siblings, 0 replies; 20+ messages in thread From: Amed Magdy @ 2019-02-26 7:58 UTC (permalink / raw) To: Richard Henderson Cc: qemu-devel, qemu-riscv, sagark, kbastian, palmer, mjc, Alistair.Francis [-- Attachment #1: Type: text/plain, Size: 1719 bytes --] > > It seems to me that the C extension can be enabled at any point, since > if C is > > off, you know that the next insn is aligned modulo 4. > > > Ok, This is mostly right. When C extension is enabled 32-bit base instructions can be aligned on 2 bytes boundaries instead of 4 bytes only. So multiple enables and disables of C bit at different code areas theoretically may require this check on C extension enable. I'm not really sure, may be this is might not be a practical use scenario. > It is only if the C extension is enabled, and you want to disable it, > that is > > when we must check to see if the next insn is aligned mod 4. It is > trivial to > > arrange for a particular instruction to be aligned, via assembler > directives. > > So it seems silly to make the definition of the csr write to misa any > more > > complicated than it is. > > I completely agree with you that C extension disable should have alignment check. > > > diff --git a/target/riscv/csr.c b/target/riscv/csr.c > > index 960d2b0aa9..8726ef802e 100644 > > --- a/target/riscv/csr.c > > +++ b/target/riscv/csr.c > > @@ -370,10 +370,11 @@ static int write_misa(CPURISCVState *env, int > csrno, > > target_ulong val) > > val &= ~RVD; > > } > > > - /* Suppress 'C' if next instruction is not aligned > > - * TODO: this should check next_pc > > + /* > > + * Suppress 'C' if next instruction is not aligned. > > + * We updated env->pc to the next insn in the translator. > > */ > > - if ((val & RVC) && (GETPC() & ~3) != 0) { > > + if ((val & RVC) && (env->pc & ~3) != 0) { > > val &= ~RVC; > > } Just a hint, (env->pc & 3) instead of (env->pc & ~3) , right ? > Thanks, Ahmed [-- Attachment #2: Type: text/html, Size: 2881 bytes --] ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH] riscv: Add proper alignment check and pending 'C' extension upon misa writes 2019-02-26 7:58 ` [Qemu-riscv] " Amed Magdy @ 2019-02-26 8:11 ` Amed Magdy -1 siblings, 0 replies; 20+ messages in thread From: Amed Magdy @ 2019-02-26 8:11 UTC (permalink / raw) To: Richard Henderson Cc: qemu-devel, qemu-riscv, sagark, kbastian, palmer, mjc, Alistair.Francis > > >> > It seems to me that the C extension can be enabled at any point, since >> if C is >> > off, you know that the next insn is aligned modulo 4. >> > >> > > > Ok, This is mostly right. When C extension is enabled 32-bit base > instructions can be aligned on 2 bytes boundaries instead of 4 bytes only. > > So multiple enables and disables of C bit at different code areas > theoretically may require this check on C extension enable. I'm not really > >sure, may be this is might not be a practical use scenario. > This paragraph considers the current implementation of csr write for misa register. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-riscv] [Qemu-devel] [PATCH] riscv: Add proper alignment check and pending 'C' extension upon misa writes @ 2019-02-26 8:11 ` Amed Magdy 0 siblings, 0 replies; 20+ messages in thread From: Amed Magdy @ 2019-02-26 8:11 UTC (permalink / raw) To: Richard Henderson Cc: qemu-devel, qemu-riscv, sagark, kbastian, palmer, mjc, Alistair.Francis [-- Attachment #1: Type: text/plain, Size: 607 bytes --] > > >> > It seems to me that the C extension can be enabled at any point, since >> if C is >> > off, you know that the next insn is aligned modulo 4. >> > >> > > > Ok, This is mostly right. When C extension is enabled 32-bit base > instructions can be aligned on 2 bytes boundaries instead of 4 bytes only. > > So multiple enables and disables of C bit at different code areas > theoretically may require this check on C extension enable. I'm not really > >sure, may be this is might not be a practical use scenario. > This paragraph considers the current implementation of csr write for misa register. [-- Attachment #2: Type: text/html, Size: 1165 bytes --] ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH] riscv: Add proper alignment check and pending 'C' extension upon misa writes 2019-02-22 16:25 ` [Qemu-riscv] " amagdy.afifi @ 2019-02-23 21:45 ` Eric Blake -1 siblings, 0 replies; 20+ messages in thread From: Eric Blake @ 2019-02-23 21:45 UTC (permalink / raw) To: amagdy.afifi, qemu-devel Cc: qemu-riscv, sagark, kbastian, palmer, mjc, Alistair.Francis On 2/22/19 10:25 AM, amagdy.afifi@gmail.com wrote: > From: ahmed_magdy <amagdy.afifi@gmail.com> > > Signed-off-by: ahmed_magdy <amagdy.afifi@gmail.com> This appears to be your first contribution to qemu. Welcome to the community! Typically, a Signed-off-by designation should be a proper name (what you would sign a legal document with, as it has a legal significance on your right to contribute the code). Using all lowercase and _ instead of space looks like a username, and while I am not one to tell you it can't be a legal name, it is unusual enough to at least raise my suspicions. Furthermore, your commit message doesn't give any details beyond the "what" in the subject line. The body of the commit message should explain the "why" (what bug are you fixing, how to reproduce it), so that a reviewer stands a chance of determining if the code matches the description you gave, and if the issue you describe really does warrant the inclusion of your patch. You gave a brief "why" in your cover letter: "I'm submiting this patch to properly check the next instruction alignment and scheduale compression extenstion enable upon 'MISA' register writes to later aligned instruction through exporting next instruction 'pc' to riscv cpu state" where it would be wise to include an improved version of that text with this commit proper (since the cover letter does not get applied to git). For that matter, when sending a single patch, a cover letter is optional (it is only mandatory when sending a multi-patch series). For more patch submission hints, see: https://wiki.qemu.org/Contribute/SubmitAPatch -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-riscv] [Qemu-devel] [PATCH] riscv: Add proper alignment check and pending 'C' extension upon misa writes @ 2019-02-23 21:45 ` Eric Blake 0 siblings, 0 replies; 20+ messages in thread From: Eric Blake @ 2019-02-23 21:45 UTC (permalink / raw) To: amagdy.afifi, qemu-devel Cc: qemu-riscv, sagark, kbastian, palmer, mjc, Alistair.Francis On 2/22/19 10:25 AM, amagdy.afifi@gmail.com wrote: > From: ahmed_magdy <amagdy.afifi@gmail.com> > > Signed-off-by: ahmed_magdy <amagdy.afifi@gmail.com> This appears to be your first contribution to qemu. Welcome to the community! Typically, a Signed-off-by designation should be a proper name (what you would sign a legal document with, as it has a legal significance on your right to contribute the code). Using all lowercase and _ instead of space looks like a username, and while I am not one to tell you it can't be a legal name, it is unusual enough to at least raise my suspicions. Furthermore, your commit message doesn't give any details beyond the "what" in the subject line. The body of the commit message should explain the "why" (what bug are you fixing, how to reproduce it), so that a reviewer stands a chance of determining if the code matches the description you gave, and if the issue you describe really does warrant the inclusion of your patch. You gave a brief "why" in your cover letter: "I'm submiting this patch to properly check the next instruction alignment and scheduale compression extenstion enable upon 'MISA' register writes to later aligned instruction through exporting next instruction 'pc' to riscv cpu state" where it would be wise to include an improved version of that text with this commit proper (since the cover letter does not get applied to git). For that matter, when sending a single patch, a cover letter is optional (it is only mandatory when sending a multi-patch series). For more patch submission hints, see: https://wiki.qemu.org/Contribute/SubmitAPatch -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH] riscv: Add proper alignment check and pending 'C' extension upon misa writes 2019-02-23 21:45 ` [Qemu-riscv] " Eric Blake @ 2019-02-24 8:07 ` Amed Magdy -1 siblings, 0 replies; 20+ messages in thread From: Amed Magdy @ 2019-02-24 8:07 UTC (permalink / raw) To: Eric Blake Cc: qemu-devel, qemu-riscv, sagark, kbastian, palmer, mjc, Alistair.Francis Thank you so much, Eric. Sorry about this unclear description. I forgot to fix user name in git configuration before submitting the patch. Sorry about any inconvenience. Thanks, Ahmed On Sat, 23 Feb 2019 at 23:46, Eric Blake <eblake@redhat.com> wrote: > On 2/22/19 10:25 AM, amagdy.afifi@gmail.com wrote: > > From: ahmed_magdy <amagdy.afifi@gmail.com> > > > > Signed-off-by: ahmed_magdy <amagdy.afifi@gmail.com> > > This appears to be your first contribution to qemu. Welcome to the > community! > > Typically, a Signed-off-by designation should be a proper name (what you > would sign a legal document with, as it has a legal significance on your > right to contribute the code). Using all lowercase and _ instead of > space looks like a username, and while I am not one to tell you it can't > be a legal name, it is unusual enough to at least raise my suspicions. > > Furthermore, your commit message doesn't give any details beyond the > "what" in the subject line. The body of the commit message should > explain the "why" (what bug are you fixing, how to reproduce it), so > that a reviewer stands a chance of determining if the code matches the > description you gave, and if the issue you describe really does warrant > the inclusion of your patch. You gave a brief "why" in your cover letter: > > "I'm submiting this patch to properly check the next instruction > alignment and scheduale compression extenstion enable upon 'MISA' > register writes to later aligned instruction through exporting next > instruction 'pc' to riscv cpu state" > > where it would be wise to include an improved version of that text with > this commit proper (since the cover letter does not get applied to git). > For that matter, when sending a single patch, a cover letter is > optional (it is only mandatory when sending a multi-patch series). > > For more patch submission hints, see: > https://wiki.qemu.org/Contribute/SubmitAPatch > > -- > Eric Blake, Principal Software Engineer > Red Hat, Inc. +1-919-301-3226 > Virtualization: qemu.org | libvirt.org > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-riscv] [Qemu-devel] [PATCH] riscv: Add proper alignment check and pending 'C' extension upon misa writes @ 2019-02-24 8:07 ` Amed Magdy 0 siblings, 0 replies; 20+ messages in thread From: Amed Magdy @ 2019-02-24 8:07 UTC (permalink / raw) To: Eric Blake Cc: qemu-devel, qemu-riscv, sagark, kbastian, palmer, mjc, Alistair.Francis [-- Attachment #1: Type: text/plain, Size: 2065 bytes --] Thank you so much, Eric. Sorry about this unclear description. I forgot to fix user name in git configuration before submitting the patch. Sorry about any inconvenience. Thanks, Ahmed On Sat, 23 Feb 2019 at 23:46, Eric Blake <eblake@redhat.com> wrote: > On 2/22/19 10:25 AM, amagdy.afifi@gmail.com wrote: > > From: ahmed_magdy <amagdy.afifi@gmail.com> > > > > Signed-off-by: ahmed_magdy <amagdy.afifi@gmail.com> > > This appears to be your first contribution to qemu. Welcome to the > community! > > Typically, a Signed-off-by designation should be a proper name (what you > would sign a legal document with, as it has a legal significance on your > right to contribute the code). Using all lowercase and _ instead of > space looks like a username, and while I am not one to tell you it can't > be a legal name, it is unusual enough to at least raise my suspicions. > > Furthermore, your commit message doesn't give any details beyond the > "what" in the subject line. The body of the commit message should > explain the "why" (what bug are you fixing, how to reproduce it), so > that a reviewer stands a chance of determining if the code matches the > description you gave, and if the issue you describe really does warrant > the inclusion of your patch. You gave a brief "why" in your cover letter: > > "I'm submiting this patch to properly check the next instruction > alignment and scheduale compression extenstion enable upon 'MISA' > register writes to later aligned instruction through exporting next > instruction 'pc' to riscv cpu state" > > where it would be wise to include an improved version of that text with > this commit proper (since the cover letter does not get applied to git). > For that matter, when sending a single patch, a cover letter is > optional (it is only mandatory when sending a multi-patch series). > > For more patch submission hints, see: > https://wiki.qemu.org/Contribute/SubmitAPatch > > -- > Eric Blake, Principal Software Engineer > Red Hat, Inc. +1-919-301-3226 > Virtualization: qemu.org | libvirt.org > [-- Attachment #2: Type: text/html, Size: 3054 bytes --] ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH] riscv: Add proper alignment check and pending 'C' extension upon misa writes 2019-02-24 8:07 ` [Qemu-riscv] " Amed Magdy @ 2019-02-25 14:14 ` Eric Blake -1 siblings, 0 replies; 20+ messages in thread From: Eric Blake @ 2019-02-25 14:14 UTC (permalink / raw) To: Amed Magdy Cc: qemu-devel, qemu-riscv, sagark, kbastian, palmer, mjc, Alistair.Francis On 2/24/19 2:07 AM, Amed Magdy wrote: > Thank you so much, Eric. > > Sorry about this unclear description. > I forgot to fix user name in git configuration before submitting the patch. > > Sorry about any inconvenience. Don't worry about it. We were all once first-time contributors, and it can be a large learning curve. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-riscv] [Qemu-devel] [PATCH] riscv: Add proper alignment check and pending 'C' extension upon misa writes @ 2019-02-25 14:14 ` Eric Blake 0 siblings, 0 replies; 20+ messages in thread From: Eric Blake @ 2019-02-25 14:14 UTC (permalink / raw) To: Amed Magdy Cc: qemu-devel, qemu-riscv, sagark, kbastian, palmer, mjc, Alistair.Francis On 2/24/19 2:07 AM, Amed Magdy wrote: > Thank you so much, Eric. > > Sorry about this unclear description. > I forgot to fix user name in git configuration before submitting the patch. > > Sorry about any inconvenience. Don't worry about it. We were all once first-time contributors, and it can be a large learning curve. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2019-02-26 8:11 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-02-22 16:25 [Qemu-devel] Add proper alignment check and pending 'C' extension for riscv amagdy.afifi 2019-02-22 16:25 ` [Qemu-riscv] " amagdy.afifi 2019-02-22 16:25 ` [Qemu-devel] [PATCH] riscv: Add proper alignment check and pending 'C' extension upon misa writes amagdy.afifi 2019-02-22 16:25 ` [Qemu-riscv] " amagdy.afifi 2019-02-22 23:57 ` [Qemu-devel] " Richard Henderson 2019-02-22 23:57 ` [Qemu-riscv] " Richard Henderson 2019-02-24 7:57 ` Amed Magdy 2019-02-24 7:57 ` [Qemu-riscv] " Amed Magdy 2019-02-24 19:04 ` Richard Henderson 2019-02-24 19:04 ` [Qemu-riscv] " Richard Henderson 2019-02-26 7:58 ` Amed Magdy 2019-02-26 7:58 ` [Qemu-riscv] " Amed Magdy 2019-02-26 8:11 ` Amed Magdy 2019-02-26 8:11 ` [Qemu-riscv] " Amed Magdy 2019-02-23 21:45 ` Eric Blake 2019-02-23 21:45 ` [Qemu-riscv] " Eric Blake 2019-02-24 8:07 ` Amed Magdy 2019-02-24 8:07 ` [Qemu-riscv] " Amed Magdy 2019-02-25 14:14 ` Eric Blake 2019-02-25 14:14 ` [Qemu-riscv] " Eric Blake
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.