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

[Igor Mammedov]

On Tue, 22 Mar 2016 11:22:56 +1100
David Gibson <address@hidden> wrote:

> On Mon, Mar 21, 2016 at 11:43:34AM +0100, Igor Mammedov wrote:
> > 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.  
> 
> Hm, yes, I see your point.  Hiding them in pre-plug does also make
> life more convenient for someone (not libvirt) manually experimenting
> with this - less to type on their device-add command.
> 
> > > 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.  
> 
> Ah.. yes, I think you're probably right, and we should have derived
> types.  It's a little bit awkward for spapr, but we can probably do
> some macro magic to make it not too bad.
Just to note derived types are not must have for hotplug but
if they are introduced later (i.e. not in the same QEMU release),
we would be forced to keep spapr-core-cpu as non abstract class
for compat reasons wrt migration.