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

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

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

On Mon, Feb 22, 2016 at 04:32:25PM +0100, Andreas Färber wrote:
> 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.

No, it's not an spapr term.  *By design* it doesn't match -smp
precedence, as noted elsewhere the point is to not lock the unit of
hotplug granularity to a fixed level of the -smp heirarchy, because
there doesn't seem to be a level there we can pick which works for all
platforms.

-- 
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: 819 bytes --]