Re: [Qemu-devel] [PATCH] spapr: Don't support query-hotpluggable-cpus on earlier pseries machine types

Re: [Qemu-devel] [PATCH] spapr: Don't support query-hotpluggable-cpus on earlier pseries machine types

[David Gibson]

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

On Tue, Aug 02, 2016 at 10:34:34AM +0530, Bharata B Rao wrote:
> On Tue, Aug 02, 2016 at 02:25:08PM +1000, David Gibson wrote:
> > On Power, support for vCPU hotplug is new in qemu 2.7.  However, we
> > currently implement the query_hotpluggable_cpus hook the same for all
> > pseries machine type versions.
> > 
> > However, the old-style CPU initialization doesn't work with the new query
> > code, meaning that attempting to use query-hotpluggable-cpus on a
> > pseries-2.6 or earlier VM will cause qemu to SEGV.
> > 
> > This fixes the problem by simply disabling the hook for earlier machine
> > types.
> 
> I had sent a patch to fix this and a couple of other related issues
> some time back and it indeed was accepted into your ppc-for-2.7 branch.
> 
> https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg01539.html
> 
> Only now I am realizing that somehow that patch didn't make it to mainline.

Oh.. good point.  Sorry, that one somehow slipped through the cracks.

So, the remaining question is, what's the preferred behaviour for
older machine types:

  1) should query-hotpluggable-cpus give an error, the same as it does
     on machine types which have never supported it (this is what my
     patch does)

or

  2) Should query-hotpluggable-cpus succeed, but return an empty list?
     (this is what Bharata's patch does)

Igor and / or Peter, do you have an opinion on which behaviour is preferable?

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

Re: [Qemu-devel] [PATCH] spapr: Don't support query-hotpluggable-cpus on earlier pseries machine types

[Peter Krempa]

On Tue, Aug 02, 2016 at 16:20:50 +1000, David Gibson wrote:
> On Tue, Aug 02, 2016 at 10:34:34AM +0530, Bharata B Rao wrote:
> > On Tue, Aug 02, 2016 at 02:25:08PM +1000, David Gibson wrote:
> > > On Power, support for vCPU hotplug is new in qemu 2.7.  However, we
> > > currently implement the query_hotpluggable_cpus hook the same for all
> > > pseries machine type versions.
> > > 
> > > However, the old-style CPU initialization doesn't work with the new query
> > > code, meaning that attempting to use query-hotpluggable-cpus on a
> > > pseries-2.6 or earlier VM will cause qemu to SEGV.
> > > 
> > > This fixes the problem by simply disabling the hook for earlier machine
> > > types.
> > 
> > I had sent a patch to fix this and a couple of other related issues
> > some time back and it indeed was accepted into your ppc-for-2.7 branch.
> > 
> > https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg01539.html
> > 
> > Only now I am realizing that somehow that patch didn't make it to mainline.
> 
> Oh.. good point.  Sorry, that one somehow slipped through the cracks.
> 
> So, the remaining question is, what's the preferred behaviour for
> older machine types:
> 
>   1) should query-hotpluggable-cpus give an error, the same as it does
>      on machine types which have never supported it (this is what my
>      patch does)
> 
> or
> 
>   2) Should query-hotpluggable-cpus succeed, but return an empty list?
>      (this is what Bharata's patch does)
> 
> Igor and / or Peter, do you have an opinion on which behaviour is preferable?

I don't really care which option you select as long as the
'hotpluggable-cpus' field which is repored in 'query-machines' is set to
false for machine types which don't support it. Libvirt then won't even
call query-hotpluggable-cpus.

Peter

Re: [Qemu-devel] [PATCH] spapr: Don't support query-hotpluggable-cpus on earlier pseries machine types

[David Gibson]

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

On Tue, Aug 02, 2016 at 08:24:13AM +0200, Peter Krempa wrote:
> On Tue, Aug 02, 2016 at 16:20:50 +1000, David Gibson wrote:
> > On Tue, Aug 02, 2016 at 10:34:34AM +0530, Bharata B Rao wrote:
> > > On Tue, Aug 02, 2016 at 02:25:08PM +1000, David Gibson wrote:
> > > > On Power, support for vCPU hotplug is new in qemu 2.7.  However, we
> > > > currently implement the query_hotpluggable_cpus hook the same for all
> > > > pseries machine type versions.
> > > > 
> > > > However, the old-style CPU initialization doesn't work with the new query
> > > > code, meaning that attempting to use query-hotpluggable-cpus on a
> > > > pseries-2.6 or earlier VM will cause qemu to SEGV.
> > > > 
> > > > This fixes the problem by simply disabling the hook for earlier machine
> > > > types.
> > > 
> > > I had sent a patch to fix this and a couple of other related issues
> > > some time back and it indeed was accepted into your ppc-for-2.7 branch.
> > > 
> > > https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg01539.html
> > > 
> > > Only now I am realizing that somehow that patch didn't make it to mainline.
> > 
> > Oh.. good point.  Sorry, that one somehow slipped through the cracks.
> > 
> > So, the remaining question is, what's the preferred behaviour for
> > older machine types:
> > 
> >   1) should query-hotpluggable-cpus give an error, the same as it does
> >      on machine types which have never supported it (this is what my
> >      patch does)
> > 
> > or
> > 
> >   2) Should query-hotpluggable-cpus succeed, but return an empty list?
> >      (this is what Bharata's patch does)
> > 
> > Igor and / or Peter, do you have an opinion on which behaviour is preferable?
> 
> I don't really care which option you select as long as the
> 'hotpluggable-cpus' field which is repored in 'query-machines' is set to
> false for machine types which don't support it. Libvirt then won't even
> call query-hotpluggable-cpus.

Ah.. and looking at it more closely, Bharata's older patch will also
get that wrong.  However, it does some other checks which might make
sense.  I'll try to merge his approach and mine.

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

Re: [Qemu-devel] [PATCH] spapr: Don't support query-hotpluggable-cpus on earlier pseries machine types

[Igor Mammedov]

On Tue, 2 Aug 2016 16:20:50 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Tue, Aug 02, 2016 at 10:34:34AM +0530, Bharata B Rao wrote:
> > On Tue, Aug 02, 2016 at 02:25:08PM +1000, David Gibson wrote:  
> > > On Power, support for vCPU hotplug is new in qemu 2.7.  However, we
> > > currently implement the query_hotpluggable_cpus hook the same for all
> > > pseries machine type versions.
> > > 
> > > However, the old-style CPU initialization doesn't work with the new query
> > > code, meaning that attempting to use query-hotpluggable-cpus on a
> > > pseries-2.6 or earlier VM will cause qemu to SEGV.
> > > 
> > > This fixes the problem by simply disabling the hook for earlier machine
> > > types.  
> > 
> > I had sent a patch to fix this and a couple of other related issues
> > some time back and it indeed was accepted into your ppc-for-2.7 branch.
> > 
> > https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg01539.html
> > 
> > Only now I am realizing that somehow that patch didn't make it to mainline.  
> 
> Oh.. good point.  Sorry, that one somehow slipped through the cracks.
> 
> So, the remaining question is, what's the preferred behaviour for
> older machine types:
> 
>   1) should query-hotpluggable-cpus give an error, the same as it does
>      on machine types which have never supported it (this is what my
>      patch does)
> 
> or
> 
>   2) Should query-hotpluggable-cpus succeed, but return an empty list?
>      (this is what Bharata's patch does)
> 
> Igor and / or Peter, do you have an opinion on which behaviour is preferable?
> 

If it doesn't have any output then it makes sense to set handler to NULL
so it would return error.