On Wed, Jun 24, 2026 at 06:16:06PM +0530, Vaibhav Jain wrote:
> 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.
Since tb_offset is negative, I thought it would be better from user
perspective if the offset is a positive number. Copy pasting it in the
trace-cmd command would be easier. But on a second thought, I think we
can leave it as negative number since this is for debugging anyways,
I'll update the steps to reflect this.
>
> I think you can just do
>
> return env->tb_env->tb_offset;
Ack. Will change it in v2.
>
> > +}
> > +
> > 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.
Sure will do.
Thanks,
Gautam
