Re: [RFC PATCH] target/ppc: don't print TB in ppc_cpu_dump_state if it's not initialized

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4683 bytes --]

On Tue, Jul 12, 2022 at 06:13:44PM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 7/12/22 16:25, Matheus Ferst wrote:
> > When using "-machine none", env->tb_env is not allocated, causing the
> > segmentation fault reported in issue #85 (launchpad bug #811683). To
> > avoid this problem, check if the pointer != NULL before calling the
> > methods to print TBU/TBL/DECR.
> > 
> > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/85
> > Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
> > ---
> > This patch fixes the reported problem, but may be an incomplete solution
> > since many other places dereference env->tb_env without checking for
> > NULL. AFAICS, "-machine none" is the only way to trigger this problem,
> > and I'm not familiar with the use-cases for this option.
> 
> The "none"  machine type is mainly used by libvirt to do instrospection
> of the available options/capabilities of the QEMU binary. It starts a QEMU
> process like the following:
> 
> ./x86_64-softmmu/qemu-system-x86_64 -S -no-user-config -nodefaults \
>       -nographic -machine none,accel=kvm:tcg -daemonize
> 
> And then it uses QMP to probe the binary.
> 
> Aside from this libvirt usage I am not aware of anyone else using -machine
> none extensively.

Right.  -machine none basically cannot work as a real machine for
POWER (maybe some other CPUs as well).  At least the more modern POWER
CPUs simply cannot boot without a bunch of supporting board/system
level elements, and there's not really a sane way to encode those into
individual emulated devices at present (maybe ever).

One of those things is that POWER expects the timebases to be
synchronized across all CPUs in the system, which obviously can't be
done locally to a single CPU chip.  It requires system level
operations, which is why it's handled by the machine type

[Example: a typical sequence which might be handled in hardware by
 low-level firmware would be to use machine-specific board-level
 registers to suspend the clock pulse to the CPUs which drives the
 timebase, then write the same value to the TB on each CPU, then
 (atomically) restart the clock pulse using board registers again]
 
> > Should we stop assuming env->tb_env != NULL and add checks everywhere?
> > Or should we find a way to provide Time Base/Decrementer for
> > "-machine none"?
> > ---
> 
> Are there other cases where env->tb_env can be NULL, aside from the case
> reported in the bug?

If there are, I'd say that's a bug in the machine type.  Setting up
(and synchronizing) the timebase is part of the machine's job.

> I don't mind the bug fix, but I'm not fond of the idea of adding additional
> checks because of this particular issue. I mean, the bug is using  the 'prep'
> machine that Thomas removed year ago in b2ce76a0730. If there's no other
> foreseeable problem, that we care about, with env->tb_env being NULL, IMO
> let's fix the bug and move on.
> 
> 
> 
> Thanks,
> 
> 
> Daniel
> 
> 
> 
> 
> >   target/ppc/cpu_init.c | 16 ++++++++--------
> >   1 file changed, 8 insertions(+), 8 deletions(-)
> > 
> > diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
> > index 86ad28466a..7e96baac9f 100644
> > --- a/target/ppc/cpu_init.c
> > +++ b/target/ppc/cpu_init.c
> > @@ -7476,18 +7476,18 @@ void ppc_cpu_dump_state(CPUState *cs, FILE *f, int flags)
> >                    "%08x iidx %d didx %d\n",
> >                    env->msr, env->spr[SPR_HID0], env->hflags,
> >                    cpu_mmu_index(env, true), cpu_mmu_index(env, false));
> > -#if !defined(NO_TIMER_DUMP)
> > -    qemu_fprintf(f, "TB %08" PRIu32 " %08" PRIu64
> > +    if (env->tb_env) {
> > +        qemu_fprintf(f, "TB %08" PRIu32 " %08" PRIu64
> >   #if !defined(CONFIG_USER_ONLY)
> > -                 " DECR " TARGET_FMT_lu
> > +                     " DECR " TARGET_FMT_lu
> >   #endif
> > -                 "\n",
> > -                 cpu_ppc_load_tbu(env), cpu_ppc_load_tbl(env)
> > +                     "\n",
> > +                     cpu_ppc_load_tbu(env), cpu_ppc_load_tbl(env)
> >   #if !defined(CONFIG_USER_ONLY)
> > -                 , cpu_ppc_load_decr(env)
> > -#endif
> > -        );
> > +                     , cpu_ppc_load_decr(env)
> >   #endif
> > +            );
> > +    }
> >       for (i = 0; i < 32; i++) {
> >           if ((i & (RGPL - 1)) == 0) {
> >               qemu_fprintf(f, "GPR%02d", i);
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]