Re: [Qemu-devel] [RFC PATCH v0 0/8] cpu-package hotplug

[Andreas Färber]

Hi Bharata,

Am 22.02.2016 um 06:01 schrieb Bharata B Rao:
> This is an attempt to implement David Gibson's RFC that was posted at
> https://lists.gnu.org/archive/html/qemu-ppc/2016-02/msg00000.html
> I am not sure if I have followed all the aspects of the RFC fully, but we
> can make changes going forward.

I am not familiar with David's RFC beyond what was portrayed on the KVM
call - this is not what we discussed on the call and I don't like it.

Further, your commits are pretty cryptic to me. Please improve your
commit messages.

For example, you add a cpu_type field and you assign it the value
TYPE_POWERPC_CPU. That's not the user-chosen CPU type then, it's a base
CPU type that cannot be instantiated. Either name it cpu_base_type or
fill it in with proper values in one patch - that patch on its own does
not create value and does not explain your claim:
"Storing CPU typename in MachineState lets us to create CPU threads
for all architectures in uniform manner from arch-neutral code."
I'm pretty sure that CPU threads cannot be created from that type, as it
would run into an assertion.

Next, you make a functionally correct refactoring of cpu_generic_init(),
but I don't understand why you duplicate that code. cpu_foo_init() still
expects things to be realized, so instead of realizing once in a central
place you do it in nine different places. Had you touched all helper
functions we might be able to move that to three places, once for
softmmu, once or twice for linux-user and once for bsd-user. But I
rather get the feeling that you misunderstand those legacy helper
functions, they're for -cpu handling and not to my knowledge used for
cpu-add at all. You should not be using them and then won't need to
touch them in this way. By using them in your supposedly QOM code you
are hiding an object_new() call inside deep layers of helper functions
instead of using QOM native functions such as object_initialize(),
object_new() and object_property_set*().

Is "CPU package" some IBM sPAPR term? It is new to me and does not match
-smp precedence, so I really don't think we should be forcing that term
on all architectures for no good reason.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton; HRB 21284 (AG Nürnberg)