Re: [PATCH 04/15] hw/ppc/spapr_rtas: Restrict variables scope to single switch case

From: Greg Kurz <groug@kaod.org>
To: "Philippe Mathieu-Daudé" <philmd@redhat.com>
Cc: qemu-devel@nongnu.org, Peter Maydell <peter.maydell@linaro.org>,
	Eduardo Habkost <ehabkost@redhat.com>,
	kvm@vger.kernel.org, Juan Quintela <quintela@redhat.com>,
	qemu-ppc@nongnu.org, Marcelo Tosatti <mtosatti@redhat.com>,
	"Dr. David Alan Gilbert" <dgilbert@redhat.com>,
	qemu-arm@nongnu.org, Alistair Francis <alistair.francis@wdc.com>,
	Marcel Apfelbaum <marcel.apfelbaum@gmail.com>,
	Paolo Bonzini <pbonzini@redhat.com>,
	David Gibson <david@gibson.dropbear.id.au>,
	Richard Henderson <rth@twiddle.net>,
	Eric Blake <eblake@redhat.com>
Subject: Re: [PATCH 04/15] hw/ppc/spapr_rtas: Restrict variables scope to single switch case
Date: Fri, 10 Jan 2020 10:50:55 +0100	[thread overview]
Message-ID: <20200110105055.3e72ddf4@bahia.lan> (raw)
In-Reply-To: <9870f8ed-3fa0-1deb-860d-7481cb3db556@redhat.com>

On Fri, 10 Jan 2020 10:34:07 +0100
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> On 1/9/20 6:43 PM, Greg Kurz wrote:
> > On Thu,  9 Jan 2020 16:21:22 +0100
> > Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> > 
> >> We only access these variables in RTAS_SYSPARM_SPLPAR_CHARACTERISTICS
> >> case, restrict their scope to avoid unnecessary initialization.
> >>
> > 
> > I guess a decent compiler can be smart enough detect that the initialization
> > isn't needed outside of the RTAS_SYSPARM_SPLPAR_CHARACTERISTICS branch...
> > Anyway, reducing scope isn't bad. The only hitch I could see is that some
> > people do prefer to have all variables declared upfront, but there's a nested
> > param_val variable already so I guess it's okay.
> 
> I don't want to outsmart compilers :)
> 
> The MACHINE() macro is not a simple cast, it does object introspection 
> with OBJECT_CHECK(), thus is not free. Since 

Sure, I understand the motivation in avoiding an unneeded call
to calling object_dynamic_cast_assert().

> object_dynamic_cast_assert() argument is not const, I'm not sure the 
> compiler can remove the call.
> 

Not remove the call, but delay it to the branch that uses it,
ie. parameter == RTAS_SYSPARM_SPLPAR_CHARACTERISTICS.

> Richard, Eric, do you know?
> 
> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> >> ---
> >>   hw/ppc/spapr_rtas.c | 4 ++--
> >>   1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> >> index 6f06e9d7fe..7237e5ebf2 100644
> >> --- a/hw/ppc/spapr_rtas.c
> >> +++ b/hw/ppc/spapr_rtas.c
> >> @@ -267,8 +267,6 @@ static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu,
> >>                                             uint32_t nret, target_ulong rets)
> >>   {
> >>       PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> >> -    MachineState *ms = MACHINE(spapr);
> >> -    unsigned int max_cpus = ms->smp.max_cpus;
> >>       target_ulong parameter = rtas_ld(args, 0);
> >>       target_ulong buffer = rtas_ld(args, 1);
> >>       target_ulong length = rtas_ld(args, 2);
> >> @@ -276,6 +274,8 @@ static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu,
> >>   
> >>       switch (parameter) {
> >>       case RTAS_SYSPARM_SPLPAR_CHARACTERISTICS: {
> >> +        MachineState *ms = MACHINE(spapr);
> >> +        unsigned int max_cpus = ms->smp.max_cpus;
> > 
> > The max_cpus variable used to be a global. Now that it got moved
> > below ms->smp, I'm not sure it's worth keeping it IMHO. What about
> > dropping it completely and do:
> > 
> >          char *param_val = g_strdup_printf("MaxEntCap=%d,"
> >                                            "DesMem=%" PRIu64 ","
> >                                            "DesProcs=%d,"
> >                                            "MaxPlatProcs=%d",
> >                                            ms->smp.max_cpus,
> >                                            current_machine->ram_size / MiB,
> >                                            ms->smp.cpus,
> >                                            ms->smp.max_cpus);
> 
> OK, good idea.
> 
> > And maybe insert an empty line between the declaration of param_val
> > and the code for a better readability ?
> > 
> >>           char *param_val = g_strdup_printf("MaxEntCap=%d,"
> >>                                             "DesMem=%" PRIu64 ","
> >>                                             "DesProcs=%d,"
> > 
> 