Re: [Qemu-devel] [RFC PATCH v2 0/9] Core based CPU hotplug for PowerPC sPAPR

[Igor Mammedov]

On Mon, 21 Mar 2016 14:57:53 +1100
David Gibson <address@hidden> wrote:

> On Fri, Mar 18, 2016 at 08:59:32AM +0530, Bharata B Rao wrote:
> > On Thu, Mar 17, 2016 at 09:03:43PM +1100, David Gibson wrote:  
> > > On Wed, Mar 16, 2016 at 04:48:50PM +0100, Igor Mammedov wrote:  
> > > > On Wed, 16 Mar 2016 09:18:03 +0530
> > > > Bharata B Rao <address@hidden> wrote:
> > > >   
> > > > > On Mon, Mar 14, 2016 at 10:47:28AM +0100, Igor Mammedov wrote:  
> > > > > > On Fri, 11 Mar 2016 10:24:29 +0530
> > > > > > Bharata B Rao <address@hidden> wrote:
> > > > > >     
> > > > > > > Hi,
> > > > > > > 
> > > > > > > This is the next version of "Core based CPU hotplug for PowerPC 
> > > > > > > sPAPR" that
> > > > > > > was posted at
> > > > > > > https://lists.gnu.org/archive/html/qemu-ppc/2016-03/msg00081.html
> > > > > > > 
> > > > > > > device_add semantics
> > > > > > > --------------------
> > > > > > > For -smp 16,sockets=1,cores=2,threads=8,maxcpus=32
> > > > > > > (qemu) device_add 
> > > > > > > spapr-cpu-core,id=core2,core=16,cpu_model=host[,threads=8]    
> > > > > > do you plan to allow user to hotplug different cpu_models?
> > > > > > If not it would be better to hide cpu_model from user
> > > > > > and set it from machine pre_plug handler.    
> > > > > 
> > > > > In my earlier implementations I derived cpu model from -cpu and 
> > > > > threads from
> > > > > -smp,threads= commandline options and never exposed them to device_add
> > > > > command.
> > > > > 
> > > > > Though we don't support heterogenous systems (different cpu models 
> > > > > and/or
> > > > > threads) now, it was felt that it should be easy enough to support 
> > > > > such
> > > > > systems if required in future, that's how cpu_model and threads became
> > > > > options for device_add.
> > > > > 
> > > > > One of the things that David felt was missing from my earlier QMP 
> > > > > query
> > > > > command (and which is true in your QMP query implementation also) is 
> > > > > that
> > > > > we aren't exporting cpu_model at all, at least for not-yet-plugged 
> > > > > cores.
> > > > > So should we include that or let management figure that out since it
> > > > > would already know about the CPU model.  
> > > > 1.
> > > > so since you are not planning supporting heterogeneous setup yet,
> > > > I'd suggest to refrain from making user to provide cpu_model at
> > > > device_add time. Instead make machine code to set it for cores it
> > > > creates before core.realize() (yet another use for pre_plug()).
> > > > 
> > > > That way mgmt doesn't have to figure out what cpu_model to set at
> > > > device_add time and doesn't have find out what property to use for it.  
> > > 
> > > Yes.. of course you could also do the same thing for nr_threads, so
> > > I'm wondering whether there's a good argument to keep one in
> > > pre_plug() and one in query-hotpluggable-cpus.  
> > 
> > Right, so what should be the way forward ? Should we keep cpu_model= and
> > threads= options with device_add or just threads=  or neither ?  
> 
> I'm inclined to keep them both in device_add - I like the idea of
> having an example on day 0 of advertising extra properties (beyond
> nr_threads and location) to set from query-hotpluggable-cpus.
> 
> But, I'd probably change my mind if Igor or someone has a stronger
> opinion.
I don't have a strong opinion on this, but you have to keep in mind
that one you make it ABI you probably would have to maintain it forever.

So far 'threads' and 'cpu_model' look like a constant values,
fixed at start-up time for every core.
Taking in account that user is not supposed to change them during
hotplug time and that they are the same for every core,
I'd go for conservative route and hide them in pre_plug() for now.
You always can expose them later if needed.

> If we advertise cpu_model, however, it should probably be changed to
> cpu thread class name, since IIUC that's an existing advertised part
> of the QOM interface, but cpu_model isn't.
I still think that spapr-cpu-core should be an abstract type
with a concrete set of derived cores types per each thread type.
But this question is not related to hotplug, but rather to
start-up of QEMU from scratch with -device and supported types
discovery. So I'd postpone question for later and that's yet another
reason why I'd like to hide cpu_model from user for now.