Re: [Qemu-devel] [PATCH] spapr: Correctly set query_hotpluggable_cpus hook based on machine version

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

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

On Mon, Aug 08, 2016 at 10:46:37AM +0200, Igor Mammedov wrote:
> On Fri, 5 Aug 2016 20:21:59 +0530
> Bharata B Rao <bharata@linux.vnet.ibm.com> wrote:
> 
> > On Fri, Aug 05, 2016 at 05:50:29PM +1000, David Gibson wrote:
> > > Prior to c8721d3 "spapr: Error out when CPU hotplug is attempted on older
> > > pseries machines", attempting to use query-hotpluggable-cpus on pseries-2.6
> > > and earlier machine types would SEGV.
> > > 
> > > That change fixed that, but due to some unexpected interactions in init
> > > order and a brown-paper-bag worthy failure to test, it accidentally
> > > disabled query-hotpluggable-cpus for all pseries machine types, including
> > > the current one which should allow it.
> > > 
> > > In fact, query_hotpluggable_cpus needs to be non-NULL when and only when
> > > the dr_cpu_enabled flag in sPAPRMachineClass is set, which makes
> > > dr_cpu_enabled itself redundant.
> > > 
> > > This patch removes dr_cpu_enabled, instead directly setting
> > > query_hotpluggable_cpus from the machine class_init functions, and using
> > > that to determine the availability of CPU hotplug when necessary.  
> > 
> > dr_cpu_enabled actually determines if CPU hotplug feature is present
> > or not. It also controls the creation of DRC-specific properties
> > in /cpus DT node like ibm,drc-indexes etc
> > 
> > query_hotpluggable_cpus just tells us if the machine supports the
> > querying of hotpluggable CPUS. query_hotpluggable_cpus definitely
> > implies dr_cpu_enabled but dr_cpu_enabled can exist on its own
> > (theoretically at the least) without query_hotpluggable_cpus.
> > 
> > So I think we should not replace dr_cpu_enabled with query_hotpluggable_cpus.
> I agree, hotplug capability shouldn't depend on availability of
> interface to operate it but rather on some platform specific bits
> which dr_cpu_enabled is (or at least looks like it's).

I still don't understand the objection here.  Once
query_hotpluggable_cpus was set correctly it really was non-NULL if
and only if dr_cpu_enabled was also true.  Are you saying there should
be an option to disable hotplug even in the newer machine types?

For now, Peter has already merged my pull request including this
patch.

-- 
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 --]