On Fri, 10 Jan 2020 10:34:07 +0100
Philippe Mathieu-Daudé <philmd@xxxxxxxxxx> wrote:
> On 1/9/20 6:43 PM, Greg Kurz wrote:
> > On Thu, 9 Jan 2020 16:21:22 +0100
> > Philippe Mathieu-Daudé <philmd@xxxxxxxxxx> 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@xxxxxxxxxx>
> >> ---
> >> 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,"
> >
>
