Hi Gautam, Thanks for the patch. My review comments inline below:
Gautam Menghani <[email protected]> writes: > When debugging issues in KVM guests, it is sometimes helpful to have a > unified trace log of both guest and host to see where things are going > wrong. Expose the TB (timebase) offset through QEMU monitor to enable > capturing of unified log. > > The below steps can be then used to get a unified log: > 1. In host > trace-cmd record -e kvm_hv:kvm_guest_enter -e kvm_hv:kvm_guest_exit \ > -C ppc-tb -o trace_host.dat > > 2. In guest > trace-cmd record -e powerpc:hcall_entry -e powerpc:hcall_exit -C ppc-tb \ > --ts-offset <TB offset from QEMU monitor> -o trace_guest.dat > > 3. Transfer the guest logs to the host with scp/rsync > > 4. Unify the logs > trace-cmd report -i trace_host.dat -i trace_guest.dat > combined_log > > Signed-off-by: Gautam Menghani <[email protected]> > --- > hw/ppc/ppc.c | 5 +++++ > target/ppc/cpu.h | 1 + > target/ppc/cpu_init.c | 4 +++- > 3 files changed, 9 insertions(+), 1 deletion(-) > > diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c > index a512d4fa64..1e8eb0ceef 100644 > --- a/hw/ppc/ppc.c > +++ b/hw/ppc/ppc.c > @@ -515,6 +515,11 @@ uint64_t cpu_ppc_get_tb(ppc_tb_t *tb_env, uint64_t > vmclk, int64_t tb_offset) > return ns_to_tb(tb_env->tb_freq, vmclk) + tb_offset; > } > > +int64_t cpu_ppc_load_tb_offset(CPUPPCState *env) > +{ > + return llabs(*(&env->tb_env->tb_offset)); Why are you doing both pointer ref and deref ? Also why llabs is needed for tb_offset. You are anyways returning signed value and printing signed value later in ppc_cpu_dump_state. I think you can just do return env->tb_env->tb_offset; > +} > + > uint64_t cpu_ppc_load_tbl (CPUPPCState *env) > { > ppc_tb_t *tb_env = env->tb_env; > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h > index 057c54bbb8..cbd5964b1a 100644 > --- a/target/ppc/cpu.h > +++ b/target/ppc/cpu.h > @@ -1693,6 +1693,7 @@ void cpu_ppc_store_hdecr(CPUPPCState *env, target_ulong > value); > void cpu_ppc_store_tbu40(CPUPPCState *env, uint64_t value); > uint64_t cpu_ppc_load_purr(CPUPPCState *env); > void cpu_ppc_store_purr(CPUPPCState *env, uint64_t value); > +int64_t cpu_ppc_load_tb_offset(CPUPPCState *env); > #if !defined(CONFIG_USER_ONLY) > target_ulong load_40x_pit(CPUPPCState *env); > void store_40x_pit(CPUPPCState *env, target_ulong val); > diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c > index a02187ce5a..a77adc3e62 100644 > --- a/target/ppc/cpu_init.c > +++ b/target/ppc/cpu_init.c > @@ -7620,8 +7620,10 @@ void ppc_cpu_dump_state(CPUState *cs, FILE *f, int > flags) > #if !defined(CONFIG_USER_ONLY) > if (env->tb_env) { > qemu_fprintf(f, "TB %08" PRIu32 " %08" PRIu64 > + " TB_OFFSET %016" PRId64 Minor: There might be some userspace tooling depending on this dump format. Would prefer if you introduce the new field to the end of string. > " DECR " TARGET_FMT_lu "\n", cpu_ppc_load_tbu(env), > - cpu_ppc_load_tbl(env), cpu_ppc_load_decr(env)); > + cpu_ppc_load_tbl(env), cpu_ppc_load_tb_offset(env), > + cpu_ppc_load_decr(env)); > } > #else > qemu_fprintf(f, "TB %08" PRIu32 " %08" PRIu64 "\n", > cpu_ppc_load_tbu(env), > -- > 2.54.0 > -- Cheers ~ Vaibhav
