| Message ID | 150506059354.19604.5050182852156612042.stgit@frigg.lan (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 09/10/2017 09:23 AM, Lluís Vilanova wrote: > Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu> > --- > accel/tcg/translator.c | 18 ++++++++++++++++++ > trace-events | 9 +++++++++ > 2 files changed, 27 insertions(+) > > diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c > index 287d27b4f7..6598931171 100644 > --- a/accel/tcg/translator.c > +++ b/accel/tcg/translator.c > @@ -70,6 +70,8 @@ void translator_loop(const TranslatorOps *ops, DisasContextBase *db, > > while (true) { > target_ulong pc_insn = db->pc_next; > + TCGv_i32 insn_size_tcg = 0; > + int insn_size_opcode_idx; Initializing a TCGv_i32 is wrong. And surely insn_size_opcode is surely uninitialized? > + if (TRACE_GUEST_INST_INFO_BEFORE_EXEC_ENABLED) { > + insn_size_tcg = tcg_temp_new_i32(); > + insn_size_opcode_idx = tcg_op_buf_count(); > + tcg_gen_movi_i32(insn_size_tcg, 0xdeadbeef); > + > + trace_guest_inst_info_before_tcg( > + cpu, tcg_ctx.tcg_env, pc_insn, insn_size_tcg); > + > + tcg_temp_free_i32(insn_size_tcg); There's no reason you can't declare insn_size_tcg right here and avoid the incorrect initialization above. Is there a reason to have both "guest_insn" and "guest_insn_info"? r~
Richard Henderson writes: > On 09/10/2017 09:23 AM, Lluís Vilanova wrote: >> Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu> >> --- >> accel/tcg/translator.c | 18 ++++++++++++++++++ >> trace-events | 9 +++++++++ >> 2 files changed, 27 insertions(+) >> >> diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c >> index 287d27b4f7..6598931171 100644 >> --- a/accel/tcg/translator.c >> +++ b/accel/tcg/translator.c >> @@ -70,6 +70,8 @@ void translator_loop(const TranslatorOps *ops, DisasContextBase *db, >> >> while (true) { >> target_ulong pc_insn = db->pc_next; >> + TCGv_i32 insn_size_tcg = 0; >> + int insn_size_opcode_idx; > Initializing a TCGv_i32 is wrong. > And surely insn_size_opcode is surely uninitialized? >> + if (TRACE_GUEST_INST_INFO_BEFORE_EXEC_ENABLED) { >> + insn_size_tcg = tcg_temp_new_i32(); >> + insn_size_opcode_idx = tcg_op_buf_count(); >> + tcg_gen_movi_i32(insn_size_tcg, 0xdeadbeef); >> + >> + trace_guest_inst_info_before_tcg( >> + cpu, tcg_ctx.tcg_env, pc_insn, insn_size_tcg); >> + >> + tcg_temp_free_i32(insn_size_tcg); > There's no reason you can't declare insn_size_tcg right here and avoid the > incorrect initialization above. Yes, I guess I did not move the declaration here by error after refactoring the code. > Is there a reason to have both "guest_insn" and "guest_insn_info"? I initially wanted to have a bare-bones event with simple information, and an *_info variant with more detailed information like register usage and physical addresses (which would be disabled by default to avoid performance impact). We had a discussion long time ago that led to decide that register usage information as I implemented it was only partial (it did not capture register usage helpers), and thus was not worth adding. Since physical address information is not gonna be added in this series (if at all), what do you say about hoisting instruction length info into guest_insn_before/after and dropping the *_info variants? Thanks, Lluis
On 09/14/2017 07:59 AM, Lluís Vilanova wrote: > Since physical address information is not gonna be added in this series (if at > all), what do you say about hoisting instruction length info into > guest_insn_before/after and dropping the *_info variants? That should be fine. r~
diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c index 287d27b4f7..6598931171 100644 --- a/accel/tcg/translator.c +++ b/accel/tcg/translator.c @@ -70,6 +70,8 @@ void translator_loop(const TranslatorOps *ops, DisasContextBase *db, while (true) { target_ulong pc_insn = db->pc_next; + TCGv_i32 insn_size_tcg = 0; + int insn_size_opcode_idx; db->num_insns++; ops->insn_start(db, cpu); @@ -99,6 +101,16 @@ void translator_loop(const TranslatorOps *ops, DisasContextBase *db, trace_guest_bbl_before_tcg(cpu, tcg_ctx.tcg_env, db->pc_first); } trace_guest_inst_before_tcg(cpu, tcg_ctx.tcg_env, pc_insn); + if (TRACE_GUEST_INST_INFO_BEFORE_EXEC_ENABLED) { + insn_size_tcg = tcg_temp_new_i32(); + insn_size_opcode_idx = tcg_op_buf_count(); + tcg_gen_movi_i32(insn_size_tcg, 0xdeadbeef); + + trace_guest_inst_info_before_tcg( + cpu, tcg_ctx.tcg_env, pc_insn, insn_size_tcg); + + tcg_temp_free_i32(insn_size_tcg); + } /* Disassemble one instruction. The translate_insn hook should update db->pc_next and db->is_jmp to indicate what should be @@ -113,6 +125,12 @@ void translator_loop(const TranslatorOps *ops, DisasContextBase *db, ops->translate_insn(db, cpu); } + /* Tracing after (patched values) */ + if (TRACE_GUEST_INST_INFO_BEFORE_EXEC_ENABLED) { + unsigned int insn_size = db->pc_next - pc_insn; + tcg_set_insn_param(insn_size_opcode_idx, 1, insn_size); + } + /* Stop translation if translate_insn so indicated. */ if (db->is_jmp != DISAS_NEXT) { break; diff --git a/trace-events b/trace-events index 46457c6158..4e61697297 100644 --- a/trace-events +++ b/trace-events @@ -107,6 +107,15 @@ vcpu tcg guest_bbl_before(uint64_t vaddr) "vaddr=0x%016"PRIx64, "vaddr=0x%016"PR # Targets: TCG(all) vcpu tcg guest_inst_before(uint64_t vaddr) "vaddr=0x%016"PRIx64, "vaddr=0x%016"PRIx64 +# @vaddr: Instruction's virtual address +# @size: Instruction's size in bytes +# +# Same as 'guest_inst_before', with additional information. +# +# Mode: user, softmmu +# Targets: TCG(all) +disable vcpu tcg guest_inst_info_before(uint64_t vaddr, TCGv_i32 size) "vaddr=0x%016"PRIx64, "vaddr=0x%016"PRIx64" size=%d" + # @vaddr: Access' virtual address. # @info : Access' information (see below). #
Signed-off-by: Lluís Vilanova <vilanova@ac.upc.edu> --- accel/tcg/translator.c | 18 ++++++++++++++++++ trace-events | 9 +++++++++ 2 files changed, 27 insertions(+)