> A few code style comments inline. > > J > > diff --git a/hw/cpu/cpu-slot.c b/hw/cpu/cpu-slot.c > > index 1cc3b32ed675..2d16a2729501 100644 > > --- a/hw/cpu/cpu-slot.c > > +++ b/hw/cpu/cpu-slot.c > > > + > > +bool machine_parse_custom_topo_config(MachineState *ms, > > + const SMPConfiguration *config, > > + Error **errp) > > +{ > > + MachineClass *mc = MACHINE_GET_CLASS(ms); > > + CPUSlot *slot = ms->topo; > > + bool is_valid; > > + int maxcpus; > > + > > + if (!slot) { > > + return true; > > + } > > + > > + is_valid = config->has_maxsockets && config->maxsockets; > > + if (mc->smp_props.custom_topo_supported) { > > + slot->stat.entries[CPU_TOPOLOGY_LEVEL_SOCKET].max_limit = > > + is_valid ? config->maxsockets : ms->smp.sockets; > > + } else if (is_valid) { > > + error_setg(errp, "maxsockets > 0 not supported " > > + "by this machine's CPU topology"); > > + return false; > > + } else { > > + slot->stat.entries[CPU_TOPOLOGY_LEVEL_SOCKET].max_limit = > > + ms->smp.sockets; > > + } > Having the error condition in the middle is rather confusing to > read to my eyes. Playing with equivalents I wonder what works best.. Figuring out how to clearly express the logic here was indeed a bit of a struggle for me at first. :-) > if (!is_valid) { > slot->stat.entries[CPU_TOPOLOGY_LEVEL_SOCKET].max_limit = > ms->smp.sockets; > } else if (mc->smp_props.custom_topo_supported) { > slot->stat.entries[CPU_TOPOLOGY_LEVEL_SOCKET].max_limit = > config->max_sockets; > } else { > error_setg... > return false; > } > > or take the bad case out first. Maybe this is a little obscure > though (assuming I even got it right) as it relies on the fact > that is_valid must be false for the legacy path. > > if (!mc->smp_props.custom_topo_supported && is_valid) { > error_setg(); > return false; > } > > slot->stat.entries[CPU_TOPOLOGY_LEVEL_SOCKET].max_limit = > is_valid ? config->maxsockets : ms->smp.sockets; > > Similar for other cases. I prefer the first style, as it's more natural and clear enough! Many thanks! [snip] > > + maxcpus = 1; > > + /* Initizlize max_limit to 1, as members of CpuTopology. */ > > + for (int i = 0; i < CPU_TOPOLOGY_LEVEL__MAX; i++) { > > + maxcpus *= slot->stat.entries[i].max_limit; > > + } > > + > > + if (!config->has_maxcpus) { > > + ms->smp.max_cpus = maxcpus; > Maybe early return here to get rid of need for the else? Yes, it's better to reduce else. > > + } else { > > + if (maxcpus != ms->smp.max_cpus) { > > Unless this is going to get more complex later, else if probably appropriate here > (if you don't drop the else above. I can organize it like: if (!config->has_maxcpus) { ... return true; } if (maxcpus != ms->smp.max_cpus) { error_steg... return false; } return true; As you suggested to get rid of a "else". :) > > + error_setg(errp, "maxcpus (%d) should be equal to " > > + "the product of the remaining max parameters (%d)", > > + ms->smp.max_cpus, maxcpus); > > + return false; > > + } > > + } > > + > > + return true; > > +} Regards, Zhao