[Qemu-devel] [PATCH] block: curl: Allow passing cookies via QCryptoSecret

[Peter Krempa]

Since cookies can contain sensitive data (session ID, etc ...) it is
desired to hide them from the prying eyes of users. Add a possibility to
pass them via the secret infrastructure.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1447413

Signed-off-by: Peter Krempa <pkre...@redhat.com>
---
 block/curl.c | 24 +++-
 qapi/block-core.json | 12 ++--
 2 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/block/curl.c b/block/curl.c
index 2708d57c2f..483640b14a 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -85,6 +85,7 @@ static CURLMcode __curl_multi_socket_action(CURLM 
*multi_handle,
 #define CURL_BLOCK_OPT_SSLVERIFY "sslverify"
 #define CURL_BLOCK_OPT_TIMEOUT "timeout"
 #define CURL_BLOCK_OPT_COOKIE"cookie"
+#define CURL_BLOCK_OPT_COOKIE_SECRET "cookie-secret"
 #define CURL_BLOCK_OPT_USERNAME "username"
 #define CURL_BLOCK_OPT_PASSWORD_SECRET "password-secret"
 #define CURL_BLOCK_OPT_PROXY_USERNAME "proxy-username"
@@ -624,6 +625,11 @@ static QemuOptsList runtime_opts = {
 .help = "Pass the cookie or list of cookies with each request"
 },
 {
+.name = CURL_BLOCK_OPT_COOKIE_SECRET,
+.type = QEMU_OPT_STRING,
+.help = "ID of secret used as cookie passed with each request"
+},
+{
 .name = CURL_BLOCK_OPT_USERNAME,
 .type = QEMU_OPT_STRING,
 .help = "Username for HTTP auth"
@@ -657,6 +663,7 @@ static int curl_open(BlockDriverState *bs, QDict *options, 
int flags,
 Error *local_err = NULL;
 const char *file;
 const char *cookie;
+const char *cookie_secret;
 double d;
 const char *secretid;
 const char *protocol_delimiter;
@@ -693,7 +700,22 @@ static int curl_open(BlockDriverState *bs, QDict *options, 
int flags,
 s->sslverify = qemu_opt_get_bool(opts, CURL_BLOCK_OPT_SSLVERIFY, true);

 cookie = qemu_opt_get(opts, CURL_BLOCK_OPT_COOKIE);
-s->cookie = g_strdup(cookie);
+cookie_secret = qemu_opt_get(opts, CURL_BLOCK_OPT_COOKIE_SECRET);
+
+if (cookie && cookie_secret) {
+error_setg(errp,
+   "curl driver cannot handle both cookie and cookie secret");
+goto out_noclean;
+}
+
+if (cookie_secret) {
+s->cookie = qcrypto_secret_lookup_as_utf8(cookie_secret, errp);
+if (!s->cookie) {
+goto out_noclean;
+}
+} else {
+s->cookie = g_strdup(cookie);
+}

 file = qemu_opt_get(opts, CURL_BLOCK_OPT_URL);
 if (file == NULL) {
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 87fb747ab6..b1643d2032 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2782,11 +2782,15 @@
 #   "name1=content1; name2=content2;" as explained by
 #   CURLOPT_COOKIE(3). Defaults to no cookies.
 #
+# @cookie-secret: ID of a QCryptoSecret object providing the cookie data in a
+# secure way. See @cookie for the format. (since 2.10)
+#
 # Since: 2.9
 ##
 { 'struct': 'BlockdevOptionsCurlHttp',
   'base': 'BlockdevOptionsCurlBase',
-  'data': { '*cookie': 'str' } }
+  'data': { '*cookie': 'str',
+'*cookie-secret': 'str'} }

 ##
 # @BlockdevOptionsCurlHttps:
@@ -2801,12 +2805,16 @@
 # @sslverify:   Whether to verify the SSL certificate's validity (defaults to
 #   true)
 #
+# @cookie-secret: ID of a QCryptoSecret object providing the cookie data in a
+# secure way. See @cookie for the format. (since 2.10)
+#
 # Since: 2.9
 ##
 { 'struct': 'BlockdevOptionsCurlHttps',
   'base': 'BlockdevOptionsCurlBase',
   'data': { '*cookie': 'str',
-'*sslverify': 'bool' } }
+'*sslverify': 'bool',
+'*cookie-secret': 'str'} }

 ##
 # @BlockdevOptionsCurlFtp:
-- 
2.12.2

Re: [Qemu-devel] nbd: Possible regression in 2.9 RCs

[Peter Krempa]

On Tue, Apr 04, 2017 at 15:19:02 +0200, Kevin Wolf wrote:
> Am 03.04.2017 um 14:51 hat Peter Krempa geschrieben:
> > On Mon, Apr 03, 2017 at 10:15:42 +0200, Kevin Wolf wrote:
> > > Am 31.03.2017 um 19:43 hat Max Reitz geschrieben:
> > > > On 31.03.2017 18:03, Ciprian Barbu wrote:

[...]

> > > Peter, Eric, what is the status on the libvirt side here?
> > 
> > Libvirt currently uses the NBD server only for non-shared storage
> > migration. At that point the disk is not shared (while qemu may think
> > so) since the other side is not actually running until the mirror
> > reaches synchronized state.
> 
> Yes, I misunderstood the situation at first.
> 
> Anyway, is there already a libvirt patch for the cases where the image
> is actually shared?

As said, we use the nbd server only for migration. There's no other
usage I know of and thus no patch.


signature.asc
Description: PGP signature

Re: [Qemu-devel] nbd: Possible regression in 2.9 RCs

[Peter Krempa]

On Mon, Apr 03, 2017 at 15:00:41 +0200, Kevin Wolf wrote:
> Am 03.04.2017 um 14:39 hat Max Reitz geschrieben:
> > On 03.04.2017 10:15, Kevin Wolf wrote:
> > > Am 31.03.2017 um 19:43 hat Max Reitz geschrieben:
> > 
> > [...]
> > 
> > >> So in theory all that's necessary is to set share-rw=on for the device
> > >> in the management layer. But I'm not sure whether that's practical.
> > > 
> > > Yes, libvirt needs to provide this option if the guest supports sharing.
> > > If it doesn't support sharing, rejecting a read-write NBD client seems
> > > correct to me.
> > > 
> > > Peter, Eric, what is the status on the libvirt side here?
> > > 
> > >> As for just allowing the NBD server write access to the device... To me
> > >> that appears pretty difficult from an implementation perspective. We
> > >> assert that nobody can write without having requested write access and
> > >> we make sure that nobody can request write access without it being
> > >> allowed. Making an exception for NBD seems very difficult and would
> > >> probably mean we'd have to drop the assertion for write accesses 
> > >> altogether.
> > > 
> > > Making an exception would simply be wrong.
> > 
> > Indeed. That is why it would be so difficult.
> > 
> > The question remains whether it is practical not to make an exception.
> > As far as I know, libvirt is only guaranteed to support older qemu
> > versions, not necessarily future ones. So we should be allowed to break
> > existing use cases here until libvirt is updated (assuming it is
> > possible for libvirt to express "guest device allows shared writes" as
> > an option for its next release).
> 
> If I understand correctly, this is a case of incoming live migration,
> i.e. the virtio-blk device which is blocking the writes to the image
> doesn't really belong to a running guest yet.

Yes, exactly. libvirt starts the NBD server on the destination of
the migration. Until then the VM did not ever run yet. The VM will run
once the memory migration finishes, so the guest front-end will not
write anything at that point.

> So if we need to make an exception (and actually reading the context
> makes it appear so), I guess it would have to be that devices actually
> can share the write permission during incoming migration, but not any
> more later (unless the share-rw flag is set).

Yes, this would be desired to avoid a regression. Libvirt then tears
down the mirror prior to resuming the VM (afaik).


signature.asc
Description: PGP signature

Re: [Qemu-devel] nbd: Possible regression in 2.9 RCs

[Peter Krempa]

On Mon, Apr 03, 2017 at 10:15:42 +0200, Kevin Wolf wrote:
> Am 31.03.2017 um 19:43 hat Max Reitz geschrieben:
> > On 31.03.2017 18:03, Ciprian Barbu wrote:

[...]

> > So this doesn't work:
> > 
> > $ x86_64-softmmu/qemu-system-x86_64 \
> > -blockdev node-name=image,driver=qcow2,\
> > file.driver=file,file.filename=foo.qcow2 \
> > -device virtio-blk,drive=image \
> > -qmp stdio
> > {"QMP": {"version": {"qemu": {"micro": 92, "minor": 8, "major": 2},
> > "package": " (v2.8.0-2038-g6604c893d0)"}, "capabilities": []}}
> > {'execute':'qmp_capabilities'}
> > {"return": {}}
> > {'execute':'nbd-server-start','arguments':{'addr':{'type':'inet','data':{'host':'localhost','port':'10809'
> > {"return": {}}
> > {'execute':'nbd-server-add','arguments':{'device':'image','writable':true}}
> > {"error": {"class": "GenericError", "desc": "Conflicts with use by
> > /machine/peripheral-anon/device[0]/virtio-backend as 'root', which does
> > not allow 'write' on image"}
> > 
> > But this works:
> > 
> > $ x86_64-softmmu/qemu-system-x86_64 \
> > -blockdev node-name=image,driver=qcow2,\
> > file.driver=file,file.filename=foo.qcow2 \
> > -device virtio-blk,drive=image,share-rw=on \
> > -qmp stdio
> > {"QMP": {"version": {"qemu": {"micro": 92, "minor": 8, "major": 2},
> > "package": " (v2.8.0-2038-g6604c893d0)"}, "capabilities": []}}
> > {'execute':'qmp_capabilities'}
> > {"return": {}}
> > {'execute':'nbd-server-start','arguments':{'addr':{'type':'inet','data':{'host':'localhost','port':'10809'
> > {"return": {}}
> > {'execute':'nbd-server-add','arguments':{'device':'image','writable':true}}
> > {"return": {}}
> > 
> > (The difference is the share-rw=on in the -device parameter.)
> > 
> > So in theory all that's necessary is to set share-rw=on for the device
> > in the management layer. But I'm not sure whether that's practical.
> 
> Yes, libvirt needs to provide this option if the guest supports sharing.
> If it doesn't support sharing, rejecting a read-write NBD client seems
> correct to me.
> 
> Peter, Eric, what is the status on the libvirt side here?

Libvirt currently uses the NBD server only for non-shared storage
migration. At that point the disk is not shared (while qemu may think
so) since the other side is not actually running until the mirror
reaches synchronized state.

The fix should be trivial.

> > As for just allowing the NBD server write access to the device... To me
> > that appears pretty difficult from an implementation perspective. We
> > assert that nobody can write without having requested write access and
> > we make sure that nobody can request write access without it being
> > allowed. Making an exception for NBD seems very difficult and would
> > probably mean we'd have to drop the assertion for write accesses altogether.
> 
> Making an exception would simply be wrong.
> 
> Kevin




signature.asc
Description: PGP signature

Re: [Qemu-devel] [libvirt] Fail to start 2nd guest

[Peter Krempa]

On Mon, Feb 27, 2017 at 13:31:30 +, Stefan Hajnoczi wrote:
> On Mon, Feb 27, 2017 at 08:50:03PM +0800, Xiong Zhou wrote:
> > On Mon, Feb 27, 2017 at 10:11:04AM +, Stefan Hajnoczi wrote:
> > > On Mon, Feb 27, 2017 at 05:40:50PM +0800, Xiong Zhou wrote:

[...]

> > > > sh-4.2# virsh start 73us
> > > > error: Failed to start domain 73us
> > > > error: internal error: qemu unexpectedly closed the monitor: 
> > > > ((null):11497): Spice-Warning **: reds.c:2499:reds_init_socket: listen: 
> > > > Address already in use
> > > > 2017-02-27T09:33:42.335708Z qemu-kvm: failed to initialize spice server
> > > 
> > > The error message says that the spice remote desktop cannot listen on
> > > -spice port=5900,addr=127.0.0.1.
> > > 
> > > Did you hardcode port 5900 in the domain XML?  That could explain why
> > No.
> > > the second guest fails to launch - you need to use unique port numbers
> > > or let libvirt automatically assign them.  Check the domain XML:
> > > 
> > >   
> > 
> > It looks like:
> > 
> > 
> >   
> >   
> > 
> > 
> > 
> > > 
> > > Another possibility is that a process running on the host is already
> > > using port 5900.  Perhaps a guest or VNC server that was launched
> > > outside of libvirt?  You can check this with:
> > > 
> > >   netstat -alpn | grep 5900
> > # netstat -alpn | grep 5900
> > tcp0  0 127.0.0.1:5900  0.0.0.0:*   LISTEN  
> > 11065/qemu-kvm  
> 
> Please check that 11065/qemu-kvm was launched by the same libvirtd and
> its domain XML also uses autoport='yes'.
> 
> I have CCed the libvirt mailing list because they may be able to explain
> why there is a collision on TCP port 5900.

There was a bug in the code. There could be a race between startup of
two VMs:
https://bugzilla.redhat.com/show_bug.cgi?id=1397440


signature.asc
Description: PGP signature

Re: [Qemu-devel] Non-flat command line option argument syntax

[Peter Krempa]

On Thu, Feb 02, 2017 at 20:42:33 +0100, Markus Armbruster wrote:

[...]

> === Comparison ===
> 
> In my opinion, dotted keys are weird and ugly, but at least they don't
> add to the quoting mess.  Structured values look better, except when
> they do add to the quoting mess.

From libvirt's point of view anything that can be easily generated from
the JSON representation into command line arguments is acceptable.
Neither uglyness, nor escaping worry us that much. Libvirt users usually
don't come into contact with the command line and if they do it's messy
anyways.

> I'm having a hard time deciding which one I like less :)

I don't like plain JSON. On the other hand I like the dotted syntax ...
mostly as I've already written the convertors for it a while ago :) [1]

Currently we already to that with anything '-object' related that we do
in libvirt (memory backing objects, master encryption key, etc) and for
a few certain network disk backing protocols:

I stole this example above:

We already generate this internally:
'{ "file": {
   "driver": "gluster", "volume": "testvol",
   "path": "/path/a.qcow2", "debug": 9,
   "server": [ { "type": "tcp",
 "host": "1.2.3.4", "port": "24007"},
   { "type": "unix",
 "socket": "/var/run/glusterd.socket" } ] } }'

And convert it to this:
> -drive driver=qcow2,file.driver=gluster,
>file.volume=testvol,file.path=/path/a.qcow2,file.debug=9,
>file.server.0.type=tcp,
>file.server.0.host=1.2.3.4,
>file.server.0.port=24007,
>file.server.1.type=unix,
>file.server.1.socket=/var/run/glusterd.socket

There are unfortunately two implementations of the array generator:
For -object we use the repeated key method, and for gluster we use the
numbered method. It might be due to the fact that I tried to do bitmap
to array conversion, where the bitmap is represented by a list of
ranges. It was necessary for memory hotplug

Peter

[1] 331b2583ec2928b in libvirt introduces the first generator (2015/01)
b7eef33df20dc19 adds the numbered array methot so that it can be
uset with gluster (2016/07)


signature.asc
Description: PGP signature

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] [RFC 1/2] spapr: Reverse order of hotpluggable cpus list

[Peter Krempa]

On Wed, Jul 20, 2016 at 11:00:05 +1000, David Gibson wrote:
> On Tue, Jul 19, 2016 at 01:52:40PM +0200, Peter Krempa wrote:
> > On Mon, Jul 18, 2016 at 19:19:19 +1000, David Gibson wrote:
> > > The spapr implementation of query-hotpluggable-cpus builds the list of
> > > hotpluggable cores from the end (most removed from the list head)
> > > because that's the easiest way with a singly linked list.  Because it
> > > also traverses the possible CPU cores starting from low indexes the
> > > resulting list has the cpu cores listed in reverse order by core-id.
> > > 
> > > That's not generally harmful, but it means the output from "info
> > > hotpluggable-cpus" is a little harder to read than it could be.
> > > 
> > > Therefore, traverse the cpus in reverse order so that the final list
> > > ends up in increasing-core-id order.
> > 
> > To make this interface usable with in-order hotplug the ordering of the
> > entries should be codified in the schema documentation. (see my response
> > on the cover letter for justification).
> 
> I'm not really sure what you mean by this.

Currently the order of entries in reply of query-hotpluggable-cpus is
arbitrary as this patch hints.

If qemu won't support arbitrary order hotplug but libvirt should be able
to use the new interface, then the order of replies of
query-hotpluggable-cpus need to corelate (in a documented fashion) with
the order we need to plug the cpus in. By not documenting any order
libvirt can just guess it (by reimplementing the algorithm in qemu).

I've pointed this out in a sub-thread of the cover-letter.

Peter

Re: [Qemu-devel] [RFC 1/2] spapr: Reverse order of hotpluggable cpus list

[Peter Krempa]

On Mon, Jul 18, 2016 at 19:19:19 +1000, David Gibson wrote:
> The spapr implementation of query-hotpluggable-cpus builds the list of
> hotpluggable cores from the end (most removed from the list head)
> because that's the easiest way with a singly linked list.  Because it
> also traverses the possible CPU cores starting from low indexes the
> resulting list has the cpu cores listed in reverse order by core-id.
> 
> That's not generally harmful, but it means the output from "info
> hotpluggable-cpus" is a little harder to read than it could be.
> 
> Therefore, traverse the cpus in reverse order so that the final list
> ends up in increasing-core-id order.

To make this interface usable with in-order hotplug the ordering of the
entries should be codified in the schema documentation. (see my response
on the cover letter for justification).

Re: [Qemu-devel] [RFC 2/2] qmp: Implement cpu-add in terms of query-hotpluggable-cpus when available

[Peter Krempa]

On Tue, Jul 19, 2016 at 09:51:00 +0200, Igor Mammedov wrote:
> On Tue, 19 Jul 2016 09:32:08 +0530
> Bharata B Rao <bhar...@linux.vnet.ibm.com> wrote:
> > On Mon, Jul 18, 2016 at 04:01:18PM +0200, Peter Krempa wrote:
> > > On Mon, Jul 18, 2016 at 19:19:20 +1000, David Gibson wrote:  

[...]

> > Is this compatability handling any different from how libvirt already
> > handles memory specified by -m and by -device pc-dimm ? Isn't cpus specified
> > by -smp and by -device kind of comparable to the memory case ?
> it is not,
> when one hotplugs pc-dimm, then on target side -m keeps the same values as
> on source, the only change to target CLI is added -device for hotplugged 
> memory.
> Behavior is basically the same as with any other device that could be 
> hotplugged
> and migrated.
> 
> In cpu-add case, semantics are different. CLI option -smp X on source becomes
> -smp (x+#hotplugge_cpus) since there were not means to create CPUs using 
> -device
> and code that is creating initial CPUs would create additional CPUs that were 
> added
> on source. Hence inability to add any random CPU (only the next free) so that 
> target
> could reproduce setup.
> 
> After enabling -device/device_add for cpus we can ditch cpu-add in favor of
> usual -device semantics where -smp X on target stays the same as on source
> and we manipulate only with -device for hotplugged cpus.
> 
> 
> I'm not sure that enabling cpu-add for spapr would bring simplify life of
> libvirt.

Agreed. As the device_add interface looks very alike this one it makes
little sense to add this interface (given that qemu will be able to
provide enough information to allow selecting the correct entry of
query-hotpluggable-cpus to add in sequence).

Re: [Qemu-devel] [RFC 0/2] cpu-add compatibility for query-hotpluggable-cpus implementations

[Peter Krempa]

On Mon, Jul 18, 2016 at 18:20:35 +0200, Igor Mammedov wrote:
> On Mon, 18 Jul 2016 17:06:18 +0200
> Peter Krempa <pkre...@redhat.com> wrote:
> > On Mon, Jul 18, 2016 at 19:19:18 +1000, David Gibson wrote:

[...]

> > First of the problem is the missing link between the NUMA topology
> > (currently confirured via 'cpu id' which is not linked in any way to the
> > query-hotpluggable-cpus entries). This basically means that I'll have to
> > re-implement the qemu numbering scheme and hope that it doesn't change
> > until a better approach is added.
> with current 'in order' plug/unplug limitation behavior is the same as
> for cpu-add (wrt x86) so device_add could be used as direct replacement
> of cpu-add in NUMA case.
> 
> Numa node to CPU in query-hotpluggable-cpus a missing part
> but once numa mapping for hotplugged CPUs (which is broken now) is fixed
> (fix https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg00595.html)
> I'll be ready to extend x86.query-hotpluggable-cpus with numa mapping
> that -numa cpus=1,2,3... happened to configure.

So this is one instance where we need to know the relation between the
cpu 'number' and the entry in query-hotpluggable-cpus. (see below ...)

I'm aware though that for the NUMA mapping there is a plan to do it
differently in an upcomming release so that may eventually be solved

> (note: that device_add cpu,node=X that doesn't match whatever has been
> configured with -numa cpus=... will rise error, as numa configuration
> is static and fixed at VM creation time, meaning that "node" option
> in query-hotpluggable-cpus is optional and only to inform users to
> which node cpu belongs)

That is okay in this regard, I'm not planing on modifying the
configuration once we start it. We although need to allow keeping an
arbitrary configuration as it is possible now.

> > Secondly from my understanding of the current state it's impossible to
> > select an arbitrary cpu to hotplug but they need to happen 'in order' of
> > the cpu id pointed out above (which is not accessible). The grand plan
> > is to allow adding the cpus in any order. This makes the feature look
> > like a proof of concept rather than something useful.
> having out-of-order plug/unplug would be nice but that wasn't

Not only nice but it's really necessary for NUMA enabled guests so that
we can plug cpus into a given node. Otherwise NUMA guests can't take any
advantage of this since you can't control where to add vcpus.

> the grand plan. Main reason is to replace cpu-add with 'device_add cpu' and
> on top of that provide support for 'device_del cpu' instead of adding cpu-del
> command.

Unfortunately combination of the following:

- necessity to plug the vcpus in a certain order

- query-hotpluggable-cpus not reporting any ordering information

- order of entries in query-hotpluggable-cpus is arbitrary
  The documentation doesn't codify any ordering of the entries. This
  series also contains a patch that changes the order, thus the order
  information is unreliable.

make the interface unusable as-is.

With the interface there isn't a certain way how we could select the
correct entry to plug. We can just guess (basically reimplement qemu's
algorithm for numbering the cpus).

By codifying the order of entries (in any order, but it shall not be
changed afterthat) or numbering the entries we can at least eliminate the
guessing it would be possible to actually use this. (This basically
means that either the order or a index will in the end encode the
information that I've requested earlier [1])

As for the NUMA node numbering we can still guess (reimplement qemu's
algorithm for the numbering) with the data scraped from the above
information (thus basically infer the 'index' of the cpus [1]). This can
be later changed to the new interface once it will be done.

The gist of the above is that by disallowing arbitrary order of hotplug
you basically need to tell libvirt the 'index' of the cpus either
directly or indirectly by inference from the order of entries in
query-hotpluggable-cpus and the 'vcpus-count' field.

> And as result of migration to device_add to avoid changing -smp to match
> present cpus count on target and reuse the same interface as other devices.
> 
> We can still pick 'out of order' device_add cpu using migration_id patch
> and revert in-order limit patch. It would work for x86,
> but I think there were issues with SPAPR, that's why I'm in favor of
> in-order limit approach.

[...]

> > > To make this work, I need to broaden the semantics of cpu-add: it will
> > > a single entry from query-hotpluggable-cpus, which means it may add
> > > multiple vcpus, which the x86 implementation did not previously do.  
> > 
> > See my response to 2/2. If this requires to add -device

Re: [Qemu-devel] [RFC 0/2] cpu-add compatibility for query-hotpluggable-cpus implementations

[Peter Krempa]

On Mon, Jul 18, 2016 at 19:19:18 +1000, David Gibson wrote:
> I'm not entirely sure if this is a good idea, and if it is whether
> this is a good approach to it.  But I'd like to discuss it and see if
> anyone has better ideas.
> 
> As you may know we've hit a bunch of complications with cpu_index
> which will impose some limitations with what we can do with the new
> query-hotpluggable-cpus interface, and we've run out of time to
> address these in qemu-2.7.
>
> At the same time we're hitting complications with the fact that the
> new qemu interface requires a new libvirt interface to use properly,
> and that has follow on effects further up the stack.

The libvirt interface is basically now depending on adding a working
implementation for qemu or a different hypervisor. APIs without
implementation are not accepted upstream.

It looks like there are the following problems which make the above
hard:

First of the problem is the missing link between the NUMA topology
(currently confirured via 'cpu id' which is not linked in any way to the
query-hotpluggable-cpus entries). This basically means that I'll have to
re-implement the qemu numbering scheme and hope that it doesn't change
until a better approach is added.

Secondly from my understanding of the current state it's impossible to
select an arbitrary cpu to hotplug but they need to happen 'in order' of
the cpu id pointed out above (which is not accessible). The grand plan
is to allow adding the cpus in any order. This makes the feature look
like a proof of concept rather than something useful.

The two problems above make this feature hard to implement and hard to
sell to libvirt's upstream.

> Together this means a bunch more delays to having usable CPU hotplug
> on Power for downstream users, which is unfortunate.

I'm not in favor of adding upstream hacks for sake of downstream
deadlines.

> This is an attempt to get something limited working in a shorter time
> frame, by implementing the old cpu-add interface in terms of the new
> interface.  Obviously this can't fully exploit the new interface's
> capabilities, but you can do basic in-order cpu hotplug without removal.

As a side note, cpu-add technically allows out of order usage. Libvirt
didn't use it that way though.

> To make this work, I need to broaden the semantics of cpu-add: it will
> a single entry from query-hotpluggable-cpus, which means it may add
> multiple vcpus, which the x86 implementation did not previously do.

See my response to 2/2. If this requires to add -device for the
hotplugged entries when migrating it basically doesn't help at all.

> I'm not sure if the intended semantics of cpu-add were ever defined
> well enough to say if this is "wrong" or not.

For x86 I'll also need to experiment with the combined use of cpu-add
and device_add interfaces. I plan to add a implementation which
basically uses the old API in libvirt but calls the new APIs in qemu if
they were used previously. (We still need to fall back to the old API
for migration compatibility)

> Because of this, I suspect libvirt will still need some work, but I'm
> hoping it might be less that the full new API implementation.

Mostly as adding a single entry via the interface will result in
multiple entries in query-cpus. Also libvirt's interface takes the
target number of vcpus as argument so any increment that is not
divisible by the thread count needs to be rejected.

Peter

Re: [Qemu-devel] [RFC 2/2] qmp: Implement cpu-add in terms of query-hotpluggable-cpus when available

[Peter Krempa]

On Mon, Jul 18, 2016 at 19:19:20 +1000, David Gibson wrote:
> We've recently added a new device_add based cpu hotplug
> implementation, with the spapr machine type being the first user.  In
> order to overcome the limitations of the old cpu_add interface, it
> works very differently.  That's going to require a new interface in
> libvirt to properly use the new interface in qemu, which will in turn
> require work further up the stack to make use of it.
> 
> While that's certainly necessary in the long run, it would be nice if
> we could have at least something working with the old cpu_add
> interface.  This patch is an attempt to do so.
> 
> This patch implements cpu-add on machines which don't support it
> directly but which do support the query-hotpluggable-cpus interface.
> Essentially,
>   cpu-add 
> will be treated as a device_add of the n-th entry in the list provided
> by query-hotpluggable-cpus.

Does this have the same implications when libvirt will need to re-create
a matching qemu process for migration target? (Will we need to specify
-device cpu,... for every CPU 'entity' added this way on the command
line?)

When using cpu_add libvirt recreates the process by just increasing the
number of active cpus specified via '-smp'.

In case where libvirt would need to add the -device entries this would
create just problems with compatibility. Libvirt needs to add XML
specification of the cpus in order to allow tracking which cpus need to
be added on the commandline.

> This means that on spapr cpu-add will add a whole core, not a single
> vcpu as on x86. That's arguably cheating on the cpu-add semantics, but
> AFAICT those were never terribly well defined anyway.

Peter

Re: [Qemu-devel] [RFC PATCH 2/2] numa: Add node_id data in query-hotpluggable-cpus

[Peter Krempa]

On Fri, Jul 08, 2016 at 14:10:59 +0200, Igor Mammedov wrote:
> On Fri, 8 Jul 2016 14:04:23 +0200
> Peter Krempa <pkre...@redhat.com> wrote:
> 
> > On Fri, Jul 08, 2016 at 13:54:58 +0200, Igor Mammedov wrote:
> > > On Thu,  7 Jul 2016 17:17:14 +0200
> > > Peter Krempa <pkre...@redhat.com> wrote:

[...]

> > > it's along the way start QEMU -smp 1,maxcpus=X and then add
> > > remaining CPUs with device_add after getting properties from
> > > query_hotpluggable_cpus().  
> > 
> > I'm going to use a similar approach even for the hotpluggable cpus so I
> > can query the data for a new VM. On the other hand I can't make libvirt
> > use the approach with -smp 1,... all the time since we guarantee that a
> > XML that worked on a older version will be migratable back to the older
> > version.
> Could you explain a little bit more about issue?

Libvirt attempts to maintain compatibility of the XML definition file
even for migrations to older versions.

If you are able to start a VM with a XML on libvirt version 'A' then
when you use the same XML to start it on a newer version 'B' we still
need to be able to migrate the VM back to 'A'. This means that vCPUs
added via -device/device_add can be used only for configurations that
will explicitly have configuration enabled.

This shouldn't be a problem generally, but this means that we still
either the 'old' way to work properly or a approach that is compatible
with migration.

For the specific case of vCPU hotplug this means that if you don't
hotplug any CPU it needs to work with older libvirt including the numa
topology and all other possible stuff.

Re: [Qemu-devel] [RFC PATCH 2/2] numa: Add node_id data in query-hotpluggable-cpus

[Peter Krempa]

On Fri, Jul 08, 2016 at 14:06:31 +0200, Igor Mammedov wrote:
> On Fri, 8 Jul 2016 09:46:00 +0200
> Peter Krempa <pkre...@redhat.com> wrote:
> 
> [...]
> > Note: For libvirt it's a no-go to start a throwaway qemu process just to
> > query the information and thus it's desired to have a way to configure
> > all this without the need to query with a specific machine
> > type/topology.
> Is it no-go to start throwaway qemu on the new domain creation time?

Yes. The policy is that once we are starting the VM the qemu process
that we create is the one running the VM later. We can tweak stuff using
QMP and/or kill it if the configuration requested by the user can't be
achieved, but starting a different qemu process would not be accepted
upstream.

Capability querying is done prior to that with a qemu process with -M
none and the results are cached for the given combination of qemu binary
and libvirtd binary (even across restarts of the libvirtd binary).

> i.e. user create a new domain, with specific -machine/-smp layout
> libvirt stores results of query-hotpluggable-cpus and then allow user
> in (eg)virt-manager to pick which CPUs are enabled and optionally to
> which numa nodes they are mapped.

We can update certain stuff that was not explicitly configured by the
user to the state that will then be used by qemu.

Re: [Qemu-devel] [RFC PATCH 2/2] numa: Add node_id data in query-hotpluggable-cpus

[Peter Krempa]

On Fri, Jul 08, 2016 at 13:54:58 +0200, Igor Mammedov wrote:
> On Thu,  7 Jul 2016 17:17:14 +0200
> Peter Krempa <pkre...@redhat.com> wrote:
> 
> > Add a helper that looks up the NUMA node for a given CPU and use it to
> > fill the node_id in the PPC and X86 impls of query-hotpluggable-cpus.
> > 
> > Signed-off-by: Peter Krempa <pkre...@redhat.com>
> > ---
> >  hw/i386/pc.c  |  7 +++
> >  hw/ppc/spapr.c|  8 ++--
> >  include/sysemu/numa.h |  1 +
> >  numa.c| 13 +
> >  4 files changed, 27 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index 4ba02c4..a0b9507 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -2115,6 +2115,7 @@ static HotpluggableCPUList 
> > *pc_query_hotpluggable_cpus(MachineState *machine)
> >  HotpluggableCPUList *head = NULL;
> >  PCMachineState *pcms = PC_MACHINE(machine);
> >  const char *cpu_type;
> > +int node_id;
> > 
> >  cpu = pcms->possible_cpus->cpus[0].cpu;
> >  assert(cpu); /* BSP is always present */
> > @@ -2138,6 +2139,12 @@ static HotpluggableCPUList 
> > *pc_query_hotpluggable_cpus(MachineState *machine)
> >  cpu_props->core_id = topo.core_id;
> >  cpu_props->has_thread_id = true;
> >  cpu_props->thread_id = topo.smt_id;
> > +
> > +if ((node_id = numa_node_get_by_cpu_index(i)) >= 0) {
> > +cpu_props->has_node_id = true;
> > +cpu_props->node_id = node_id;
> > +}
> I've not included node_id for a reason,
>  "-numa cpus=1,2,3..." looks to me hopelessly broken now but
> I've not came up with an idea how to redo it in nice and clean way yet.
> 
> Alternative could be CLI-less numa configuration, where QEMU is started
> without "-numa cpus" but with "-S" then mgmt could call
> query_hotpluggable_cpus() to get possible CPUs and then
> map them to numa  nodes with a new QMP command using attributes
> it got from query_hotpluggable_cpus().

I think this could work for libvirt. The CPU index we currently expose
in the XML would become just a libvirt internal detail and the new QMP
command would be used to do the setup. Adding some QMP calls during VM
startup is okay.

> it's along the way start QEMU -smp 1,maxcpus=X and then add
> remaining CPUs with device_add after getting properties from
> query_hotpluggable_cpus().

I'm going to use a similar approach even for the hotpluggable cpus so I
can query the data for a new VM. On the other hand I can't make libvirt
use the approach with -smp 1,... all the time since we guarantee that a
XML that worked on a older version will be migratable back to the older
version.

> then at machine_done time we can adjust DT/ACPI data to reflect
> configured mapping.

In such case this series can be dropped since it provides what I need
differently.

Thanks,

Peter

Re: [Qemu-devel] [RFC PATCH 2/2] numa: Add node_id data in query-hotpluggable-cpus

[Peter Krempa]

On Fri, Jul 08, 2016 at 12:23:08 +1000, David Gibson wrote:
> On Thu,  7 Jul 2016 17:17:14 +0200
> Peter Krempa <pkre...@redhat.com> wrote:
> 
> > Add a helper that looks up the NUMA node for a given CPU and use it to
> > fill the node_id in the PPC and X86 impls of query-hotpluggable-cpus.
> 
> 
> IIUC how the query thing works this means that the node id issued by
> query-hotpluggable-cpus will be echoed back to device add by libvirt.

It will be echoed back, but the problem is that it's configured
separately ...

> I'm not sure we actually process that information in the core at
> present, so I don't know that that's right.
> 
> We need to be clear on which direction information is flowing here.
> Does query-hotpluggable-cpus *define* the NUMA node allocation which is
> then passed to the core device which implements it.  Or is the NUMA
> allocation defined elsewhere, and query-hotpluggable-cpus just reports
> it.

Currently (in the pre-hotplug era) the NUMA topology is defined by
specifying a CPU numbers (see previous patch) on the commandline:

-numa node=1,cpus=1-5,cpus=8,cpus=11...

This is then reported to the guest.

For a machine started with:

-smp 5,maxcpus=8,sockets=2,cores=2,threads=2
-numa node,nodeid=0,cpus=0,cpus=2,cpus=4,cpus=6,mem=500
-numa node,nodeid=1,cpus=1,cpus=3,cpus=5,cpus=7,mem=500

you get the following topology that is not really possible with
hardware:

# lscpu
Architecture:  x86_64
CPU op-mode(s):32-bit, 64-bit
Byte Order:Little Endian
CPU(s):5
On-line CPU(s) list:   0-4
Thread(s) per core:1
Core(s) per socket:2
Socket(s): 2
NUMA node(s):  2
Vendor ID: GenuineIntel
CPU family:6
Model: 6
Model name:QEMU Virtual CPU version 2.5+
Stepping:  3
CPU MHz:   3504.318
BogoMIPS:  7008.63
Hypervisor vendor: KVM
Virtualization type:   full
L1d cache: 32K
L1i cache: 32K
L2 cache:  4096K
NUMA node0 CPU(s): 0,2,4
NUMA node1 CPU(s): 1,3

Note that the count of cpus per numa node does not need to be identical.

As of the above 'query-hotpluggable-cpus' will need to report the data
that was configured above even if it doesn't make much sense in a real
world.

I did not try the above on a PPC host and thus I'm not sure whether the
config above is allowed or not.

While for the hotplug cpus it would be possible to plug in the correct
one according to the requested use the queried data but with the current
approach it's impossible to set the initial vcpus differently.

Peter

Note: For libvirt it's a no-go to start a throwaway qemu process just to
query the information and thus it's desired to have a way to configure
all this without the need to query with a specific machine
type/topology.

Re: [Qemu-devel] [RFC PATCH 0/2] cpu hotplug: Extend data provided by query-hotpluggable-cpus

[Peter Krempa]

On Thu, Jul 07, 2016 at 17:17:12 +0200, Peter Krempa wrote:
> Few entries that would help me in adding the stuff to libvirt.

I forgot to mention that I didn't really test the PPC64 stuff as I
currently don't have a box at hand.

[Qemu-devel] [RFC PATCH 2/2] numa: Add node_id data in query-hotpluggable-cpus

[Peter Krempa]

Add a helper that looks up the NUMA node for a given CPU and use it to
fill the node_id in the PPC and X86 impls of query-hotpluggable-cpus.

Signed-off-by: Peter Krempa <pkre...@redhat.com>
---
 hw/i386/pc.c  |  7 +++
 hw/ppc/spapr.c|  8 ++--
 include/sysemu/numa.h |  1 +
 numa.c| 13 +
 4 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 4ba02c4..a0b9507 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -2115,6 +2115,7 @@ static HotpluggableCPUList 
*pc_query_hotpluggable_cpus(MachineState *machine)
 HotpluggableCPUList *head = NULL;
 PCMachineState *pcms = PC_MACHINE(machine);
 const char *cpu_type;
+int node_id;

 cpu = pcms->possible_cpus->cpus[0].cpu;
 assert(cpu); /* BSP is always present */
@@ -2138,6 +2139,12 @@ static HotpluggableCPUList 
*pc_query_hotpluggable_cpus(MachineState *machine)
 cpu_props->core_id = topo.core_id;
 cpu_props->has_thread_id = true;
 cpu_props->thread_id = topo.smt_id;
+
+if ((node_id = numa_node_get_by_cpu_index(i)) >= 0) {
+cpu_props->has_node_id = true;
+cpu_props->node_id = node_id;
+}
+
 cpu_item->props = cpu_props;

 cpu = pcms->possible_cpus->cpus[i].cpu;
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index d1f5195..06ba7fc 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2370,6 +2370,7 @@ static HotpluggableCPUList 
*spapr_query_hotpluggable_cpus(MachineState *machine)
 sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
 int spapr_max_cores = max_cpus / smp_threads;
 int smt = kvmppc_smt_threads();
+int node_id;

 for (i = 0; i < spapr_max_cores; i++) {
 HotpluggableCPUList *list_item = g_new0(typeof(*list_item), 1);
@@ -2381,8 +2382,11 @@ static HotpluggableCPUList 
*spapr_query_hotpluggable_cpus(MachineState *machine)
 cpu_item->vcpu_id = i;
 cpu_props->has_core_id = true;
 cpu_props->core_id = i * smt;
-/* TODO: add 'has_node/node' here to describe
-   to which node core belongs */
+
+if ((node_id = numa_node_get_by_cpu_index(i)) >= 0) {
+cpu_props->has_node_id = true;
+cpu_props->node_id = node_id;
+}

 cpu_item->props = cpu_props;
 if (spapr->cores[i]) {
diff --git a/include/sysemu/numa.h b/include/sysemu/numa.h
index bb184c9..04d7097 100644
--- a/include/sysemu/numa.h
+++ b/include/sysemu/numa.h
@@ -31,5 +31,6 @@ extern QemuOptsList qemu_numa_opts;
 void numa_set_mem_node_id(ram_addr_t addr, uint64_t size, uint32_t node);
 void numa_unset_mem_node_id(ram_addr_t addr, uint64_t size, uint32_t node);
 uint32_t numa_get_node(ram_addr_t addr, Error **errp);
+int numa_node_get_by_cpu_index(int cpu_index);

 #endif
diff --git a/numa.c b/numa.c
index cbae430..365738a 100644
--- a/numa.c
+++ b/numa.c
@@ -506,6 +506,19 @@ void query_numa_node_mem(uint64_t node_mem[])
 }
 }

+int numa_node_get_by_cpu_index(int cpu_index)
+{
+int i;
+
+for (i = 0; i < nb_numa_nodes; i++) {
+if (test_bit(cpu_index, numa_info[i].node_cpu)) {
+return i;
+}
+}
+
+return -1;
+}
+
 static int query_memdev(Object *obj, void *opaque)
 {
 MemdevList **list = opaque;
-- 
2.9.0

[Qemu-devel] [RFC PATCH 0/2] cpu hotplug: Extend data provided by query-hotpluggable-cpus

[Peter Krempa]

Few entries that would help me in adding the stuff to libvirt.

Peter Krempa (2):
  qapi: Add vcpu id to query-hotpluggable-cpus output
  numa: Add node_id data in query-hotpluggable-cpus

 hmp.c |  1 +
 hw/i386/pc.c  |  8 
 hw/ppc/spapr.c|  9 +++--
 include/sysemu/numa.h |  1 +
 numa.c| 13 +
 qapi-schema.json  |  2 ++
 6 files changed, 32 insertions(+), 2 deletions(-)

-- 
2.9.0

[Qemu-devel] [RFC PATCH 1/2] qapi: Add vcpu id to query-hotpluggable-cpus output

[Peter Krempa]

Add 'vcpu index' to the output of query hotpluggable cpus. This output
is identical to the linear cpu index taken by the 'cpus' attribute
passed to -numa.

This will allow to reliably map the cpu number to a given topology
element without making mgmt apps to reimplement the mapping.

Signed-off-by: Peter Krempa <pkre...@redhat.com>
---
 hmp.c| 1 +
 hw/i386/pc.c | 1 +
 hw/ppc/spapr.c   | 1 +
 qapi-schema.json | 2 ++
 4 files changed, 5 insertions(+)

diff --git a/hmp.c b/hmp.c
index 0cf5baa..613601e 100644
--- a/hmp.c
+++ b/hmp.c
@@ -2450,6 +2450,7 @@ void hmp_hotpluggable_cpus(Monitor *mon, const QDict 
*qdict)
 monitor_printf(mon, "  type: \"%s\"\n", l->value->type);
 monitor_printf(mon, "  vcpus_count: \"%" PRIu64 "\"\n",
l->value->vcpus_count);
+monitor_printf(mon, "  vcpu_id: \"%" PRIu64 "\"\n", l->value->vcpu_id);
 if (l->value->has_qom_path) {
 monitor_printf(mon, "  qom_path: \"%s\"\n", l->value->qom_path);
 }
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index f293a0c..4ba02c4 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -2131,6 +2131,7 @@ static HotpluggableCPUList 
*pc_query_hotpluggable_cpus(MachineState *machine)

 cpu_item->type = g_strdup(cpu_type);
 cpu_item->vcpus_count = 1;
+cpu_item->vcpu_id = i;
 cpu_props->has_socket_id = true;
 cpu_props->socket_id = topo.pkg_id;
 cpu_props->has_core_id = true;
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 7f33a1b..d1f5195 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2378,6 +2378,7 @@ static HotpluggableCPUList 
*spapr_query_hotpluggable_cpus(MachineState *machine)

 cpu_item->type = spapr_get_cpu_core_type(machine->cpu_model);
 cpu_item->vcpus_count = smp_threads;
+cpu_item->vcpu_id = i;
 cpu_props->has_core_id = true;
 cpu_props->core_id = i * smt;
 /* TODO: add 'has_node/node' here to describe
diff --git a/qapi-schema.json b/qapi-schema.json
index ba3bf14..6db9294 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -4292,6 +4292,7 @@
 # @type: CPU object type for usage with device_add command
 # @props: list of properties to be used for hotplugging CPU
 # @vcpus-count: number of logical VCPU threads @HotpluggableCPU provides
+# @vcpu-id: linear index of the vcpu
 # @qom-path: #optional link to existing CPU object if CPU is present or
 #omitted if CPU is not present.
 #
@@ -4300,6 +4301,7 @@
 { 'struct': 'HotpluggableCPU',
   'data': { 'type': 'str',
 'vcpus-count': 'int',
+'vcpu-id': 'int',
 'props': 'CpuInstanceProperties',
 '*qom-path': 'str'
   }
-- 
2.9.0

Re: [Qemu-devel] [PATCH] qmp: fix spapr example of query-hotpluggable-cpus

[Peter Krempa]

On Thu, Jun 30, 2016 at 10:17:54 +0200, Igor Mammedov wrote:
> 27393c33 qapi: keep names in 'CpuInstanceProperties' in sync with struct 
> CPUCore
> added -id suffix to property names but forgot to fix example in 
> qmp-commands.hx
> 
> Fix example to have 'core-id' instead of 'core' to match current code
> 
> Signed-off-by: Igor Mammedov <imamm...@redhat.com>
> ---
>  qmp-commands.hx | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Oops; thanks for cleaning up after me.

Reviewed-by: Peter Krempa <pkre...@redhat.com>

Re: [Qemu-devel] [PATCH 1/3] qapi: Report support for -device cpu hotplug in query-machines

[Peter Krempa]

On Fri, Jun 24, 2016 at 16:56:21 +1000, David Gibson wrote:
> On Fri, 24 Jun 2016 07:41:11 +0200
> Peter Krempa <pkre...@redhat.com> wrote:
> 
> > On Fri, Jun 24, 2016 at 14:56:51 +1000, David Gibson wrote:
> > 
> > [...]
> > 
> > > > You are correct - query-commands says whether 'query-hotpluggable-cpus'
> > > > exists as a command.  But that is insufficient.  See my review, or the
> > > > v2 patch, where the above poor wording was corrected to say what was
> > > > really meant: knowing whether query-hotpluggable-cpus exists is
> > > > insufficient to tell you whether a given cpu type can be hotplugged.  So
> > > > adding one more piece of witness (for every type of cpu supported, we
> > > > also advertise if it is hotpluggable) is enough for libvirt to
> > > > efficiently take advantage of the new query-hotpluggable-cpus command.  
> > > 
> > > Ah, right.  Or to put it another way, the availability of
> > > query-hotpluggable-cpus is global across qemu, whereas actually being
> > > able to use it for hotplug is per machine type.
> > > 
> > > Would it be possible to do this instead by attempting to invoke
> > > query-hopluggable-cpus and seeing if it returns any information?  
> > 
> > It is not strictly necessary for us to have this in the context of
> > usability. If the user requests using the new hotplug feature we will
> > try it unconditionally and call query-hotpluggable-cpus before even
> > starting guest execution. A failure to query the state will then result
> > in termination of the VM.
> 
> Oh.. I wasn't expecting the feature would be enabled at user request -
> I thought libvirt would just use it if available.

Hmm, I think that will be possible to use the feature all the time after
all. I'll need to add multiple states for that though to track it in
the XML so that we can re-create it on migration the right way.

> > It is necessary though to report the availability of the feature to the
> > user via our domain capabilities API which some higher layer management
> > apps use to make decisions.
> 
> Right... what advantage does adding the machine flag have over
> attempting the query-hotpluggable-cpus?

Mostly the ability for oVirt or open stack being able to query whether
it's available for given machine type prior to even attempting to launch
the VM. Otherwise the'll be left to either guessing or re-implementing
the support matrix.

This means that the ability to query it with machine types is not
strictly necessary for implementing the feature in libvirt but a
very-nice-to-have thing to add to our capabilities queries.

> > This would also be necessary if we wanted to switch by default to the
> > new approach, but that's not really possible as libvirt tries to
> > guarantee that a config valid on certain version will be still valid
> > even when it was migrated to a newer version and then back.
> 
> Sorry, I've lost track of what the "This" is that would be necessary.

Never mind on this point. If I design the XML a bit differently it will
be possible to use this this feature if present all the time. Some cpus
just won't be available for unplug.

> > My current plan is to start qemu with -smp cpus=1,... and then call
> > query-hotpluggable-cpus and then hotplug all of them until the requested
> > configuration is satisfied. This approach is necessary so that we can
> > query for the model and topology info so that we don't need to
> > re-implement all the numbering and naming logic from qemu.
> 
> Um.. why?  What's the problem with just staring with -smp cpus=whatever
> and then using query-hotpluggable-cpus?
> 
> > Additionally this will require us to mark one CPU as non-hotpluggable as
> > -smp cpus=0,maxcpus=10 is basically translated to -smp
> > cpus=10,maxcpus=10.
> 
> The latter is true, but I'm not clear why it implies the former.

It necessarily doesn't imply it. I originally wanted to have most cpus
unpluggable but thinking again it is not really necessary I just need to
adapt the interface design.

Let's see how it turns out.

Peter

Re: [Qemu-devel] [PATCH 1/3] qapi: Report support for -device cpu hotplug in query-machines

[Peter Krempa]

On Fri, Jun 24, 2016 at 14:56:51 +1000, David Gibson wrote:

[...]

> > You are correct - query-commands says whether 'query-hotpluggable-cpus'
> > exists as a command.  But that is insufficient.  See my review, or the
> > v2 patch, where the above poor wording was corrected to say what was
> > really meant: knowing whether query-hotpluggable-cpus exists is
> > insufficient to tell you whether a given cpu type can be hotplugged.  So
> > adding one more piece of witness (for every type of cpu supported, we
> > also advertise if it is hotpluggable) is enough for libvirt to
> > efficiently take advantage of the new query-hotpluggable-cpus command.
> 
> Ah, right.  Or to put it another way, the availability of
> query-hotpluggable-cpus is global across qemu, whereas actually being
> able to use it for hotplug is per machine type.
> 
> Would it be possible to do this instead by attempting to invoke
> query-hopluggable-cpus and seeing if it returns any information?

It is not strictly necessary for us to have this in the context of
usability. If the user requests using the new hotplug feature we will
try it unconditionally and call query-hotpluggable-cpus before even
starting guest execution. A failure to query the state will then result
in termination of the VM.

It is necessary though to report the availability of the feature to the
user via our domain capabilities API which some higher layer management
apps use to make decisions.

This would also be necessary if we wanted to switch by default to the
new approach, but that's not really possible as libvirt tries to
guarantee that a config valid on certain version will be still valid
even when it was migrated to a newer version and then back.

My current plan is to start qemu with -smp cpus=1,... and then call
query-hotpluggable-cpus and then hotplug all of them until the requested
configuration is satisfied. This approach is necessary so that we can
query for the model and topology info so that we don't need to
re-implement all the numbering and naming logic from qemu.

Additionally this will require us to mark one CPU as non-hotpluggable as
-smp cpus=0,maxcpus=10 is basically translated to -smp
cpus=10,maxcpus=10.

Peter

[Qemu-devel] [PATCH v2 2/2] qapi: keep names in 'CpuInstanceProperties' in sync with struct CPUCore

[Peter Krempa]

struct CPUCore uses 'id' suffix in the property name. As docs for
query-hotpluggable-cpus state that the cpu core properties should be
passed back to device_add by management in case new members are added
and thus the names for the fields should be kept in sync.

Signed-off-by: Peter Krempa <pkre...@redhat.com>
---
 hmp.c | 16 
 hw/ppc/spapr.c|  4 ++--
 include/hw/cpu/core.h |  3 +++
 qapi-schema.json  | 19 ++-
 4 files changed, 23 insertions(+), 19 deletions(-)

diff --git a/hmp.c b/hmp.c
index 997a768..925601a 100644
--- a/hmp.c
+++ b/hmp.c
@@ -2457,17 +2457,17 @@ void hmp_hotpluggable_cpus(Monitor *mon, const QDict 
*qdict)

 c = l->value->props;
 monitor_printf(mon, "  CPUInstance Properties:\n");
-if (c->has_node) {
-monitor_printf(mon, "node: \"%" PRIu64 "\"\n", c->node);
+if (c->has_node_id) {
+monitor_printf(mon, "node-id: \"%" PRIu64 "\"\n", c->node_id);
 }
-if (c->has_socket) {
-monitor_printf(mon, "socket: \"%" PRIu64 "\"\n", c->socket);
+if (c->has_socket_id) {
+monitor_printf(mon, "socket-id: \"%" PRIu64 "\"\n", 
c->socket_id);
 }
-if (c->has_core) {
-monitor_printf(mon, "core: \"%" PRIu64 "\"\n", c->core);
+if (c->has_core_id) {
+monitor_printf(mon, "core-id: \"%" PRIu64 "\"\n", c->core_id);
 }
-if (c->has_thread) {
-monitor_printf(mon, "thread: \"%" PRIu64 "\"\n", c->thread);
+if (c->has_thread_id) {
+monitor_printf(mon, "thread-id: \"%" PRIu64 "\"\n", 
c->thread_id);
 }

 l = l->next;
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 778fa25..0b6bb9c 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2367,8 +2367,8 @@ static HotpluggableCPUList 
*spapr_query_hotpluggable_cpus(MachineState *machine)

 cpu_item->type = spapr_get_cpu_core_type(machine->cpu_model);
 cpu_item->vcpus_count = smp_threads;
-cpu_props->has_core = true;
-cpu_props->core = i * smt;
+cpu_props->has_core_id = true;
+cpu_props->core_id = i * smt;
 /* TODO: add 'has_node/node' here to describe
to which node core belongs */

diff --git a/include/hw/cpu/core.h b/include/hw/cpu/core.h
index 4540a7d..79ac79c 100644
--- a/include/hw/cpu/core.h
+++ b/include/hw/cpu/core.h
@@ -26,6 +26,9 @@ typedef struct CPUCore {
 int nr_threads;
 } CPUCore;

+/* Note: topology field names need to be kept in sync with
+ * 'CpuInstanceProperties' */
+
 #define CPU_CORE_PROP_CORE_ID "core-id"

 #endif
diff --git a/qapi-schema.json b/qapi-schema.json
index 24ede28..d0c4be1 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -4267,20 +4267,21 @@
 # Note: currently there are 4 properties that could be present
 # but management should be prepared to pass through other
 # properties with device_add command to allow for future
-# interface extension.
+# interface extension. This also requires the filed names to be kept in sync
+# sync with the properties passed to -device/device_add.
 #
-# @node: #optional NUMA node ID the CPU belongs to
-# @socket: #optional socket number within node/board the CPU belongs to
-# @core: #optional core number within socket the CPU belongs to
-# @thread: #optional thread number within core the CPU belongs to
+# @node-id: #optional NUMA node ID the CPU belongs to
+# @socket-id: #optional socket number within node/board the CPU belongs to
+# @core-id: #optional core number within socket the CPU belongs to
+# @thread-id: #optional thread number within core the CPU belongs to
 #
 # Since: 2.7
 ##
 { 'struct': 'CpuInstanceProperties',
-  'data': { '*node': 'int',
-'*socket': 'int',
-'*core': 'int',
-'*thread': 'int'
+  'data': { '*node-id': 'int',
+'*socket-id': 'int',
+'*core-id': 'int',
+'*thread-id': 'int'
   }
 }

-- 
2.8.3

[Qemu-devel] [PATCH v2 0/2] qapi: Fix up cpu hotplug property names and add witness for cpu hotplug support

[Peter Krempa]

Version 2:
- fix typos/incompetence/drowsiness based language errors in commit message
- select version 1 as prefered way
- add -id suffix to all members of CpuInstanceProperties
- note in qapi-schema the need to keep members in sync
- fix output text field names in HMP impl

Peter Krempa (2):
  qapi: Report support for -device cpu hotplug in query-machines
  qapi: keep names in 'CpuInstanceProperties' in sync with struct
CPUCore

 hmp.c | 16 
 hw/ppc/spapr.c|  4 ++--
 include/hw/cpu/core.h |  3 +++
 qapi-schema.json  | 24 ++--
 vl.c  |  1 +
 5 files changed, 28 insertions(+), 20 deletions(-)

-- 
2.8.3

[Qemu-devel] [PATCH v2 1/2] qapi: Report support for -device cpu hotplug in query-machines

[Peter Krempa]

For management apps it's very useful to know whether the selected
machine type supports cpu hotplug via the new -device approach. Using
the presence of 'query-hotpluggable-cpus' alone is not enough as a
witness.

Add a property to 'MachineInfo' called 'hotpluggable-cpus' that will
report the presence of this feature.

Example of output:
{
"hotpluggable-cpus": false,
"name": "mac99",
"cpu-max": 1
},
{
"hotpluggable-cpus": true,
"name": "pseries-2.7",
"is-default": true,
    "cpu-max": 255,
"alias": "pseries"
},

Signed-off-by: Peter Krempa <pkre...@redhat.com>
Reviewed-by: Eric Blake <ebl...@redhat.com>
---
 qapi-schema.json | 5 -
 vl.c | 1 +
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index 0964eec..24ede28 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2986,11 +2986,14 @@
 # @cpu-max: maximum number of CPUs supported by the machine type
 #   (since 1.5.0)
 #
+# @hotpluggable-cpus: cpu hotplug via -device is supported (since 2.7.0)
+#
 # Since: 1.2.0
 ##
 { 'struct': 'MachineInfo',
   'data': { 'name': 'str', '*alias': 'str',
-'*is-default': 'bool', 'cpu-max': 'int' } }
+'*is-default': 'bool', 'cpu-max': 'int',
+'hotpluggable-cpus': 'bool'} }

 ##
 # @query-machines:
diff --git a/vl.c b/vl.c
index c85833a..4c1f9ae 100644
--- a/vl.c
+++ b/vl.c
@@ -1524,6 +1524,7 @@ MachineInfoList *qmp_query_machines(Error **errp)

 info->name = g_strdup(mc->name);
 info->cpu_max = !mc->max_cpus ? 1 : mc->max_cpus;
+info->hotpluggable_cpus = !!mc->query_hotpluggable_cpus;

 entry = g_malloc0(sizeof(*entry));
 entry->value = info;
-- 
2.8.3

[Qemu-devel] [PATCH 2/3] [VARIANT 1] qapi: Change 'core' to 'core-id' in 'CpuInstanceProperties'

[Peter Krempa]

struct CPUCore uses 'core-id' as the property name. As docs for
query-hotpluggable-cpus state that the cpu core properties should be
passed back to device_add by management in case new members are added
and thus the names for the fields should be kept in sync.

Signed-off-by: Peter Krempa <pkre...@redhat.com>
---
 hmp.c | 4 ++--
 hw/ppc/spapr.c| 4 ++--
 include/hw/cpu/core.h | 3 +++
 qapi-schema.json  | 4 ++--
 4 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/hmp.c b/hmp.c
index 997a768..543f087 100644
--- a/hmp.c
+++ b/hmp.c
@@ -2463,8 +2463,8 @@ void hmp_hotpluggable_cpus(Monitor *mon, const QDict 
*qdict)
 if (c->has_socket) {
 monitor_printf(mon, "socket: \"%" PRIu64 "\"\n", c->socket);
 }
-if (c->has_core) {
-monitor_printf(mon, "core: \"%" PRIu64 "\"\n", c->core);
+if (c->has_core_id) {
+monitor_printf(mon, "core: \"%" PRIu64 "\"\n", c->core_id);
 }
 if (c->has_thread) {
 monitor_printf(mon, "thread: \"%" PRIu64 "\"\n", c->thread);
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 778fa25..0b6bb9c 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2367,8 +2367,8 @@ static HotpluggableCPUList 
*spapr_query_hotpluggable_cpus(MachineState *machine)

 cpu_item->type = spapr_get_cpu_core_type(machine->cpu_model);
 cpu_item->vcpus_count = smp_threads;
-cpu_props->has_core = true;
-cpu_props->core = i * smt;
+cpu_props->has_core_id = true;
+cpu_props->core_id = i * smt;
 /* TODO: add 'has_node/node' here to describe
to which node core belongs */

diff --git a/include/hw/cpu/core.h b/include/hw/cpu/core.h
index 4540a7d..79ac79c 100644
--- a/include/hw/cpu/core.h
+++ b/include/hw/cpu/core.h
@@ -26,6 +26,9 @@ typedef struct CPUCore {
 int nr_threads;
 } CPUCore;

+/* Note: topology field names need to be kept in sync with
+ * 'CpuInstanceProperties' */
+
 #define CPU_CORE_PROP_CORE_ID "core-id"

 #endif
diff --git a/qapi-schema.json b/qapi-schema.json
index 24ede28..37ef5fd 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -4271,7 +4271,7 @@
 #
 # @node: #optional NUMA node ID the CPU belongs to
 # @socket: #optional socket number within node/board the CPU belongs to
-# @core: #optional core number within socket the CPU belongs to
+# @core-id: #optional core number within socket the CPU belongs to
 # @thread: #optional thread number within core the CPU belongs to
 #
 # Since: 2.7
@@ -4279,7 +4279,7 @@
 { 'struct': 'CpuInstanceProperties',
   'data': { '*node': 'int',
 '*socket': 'int',
-'*core': 'int',
+'*core-id': 'int',
 '*thread': 'int'
   }
 }
-- 
2.8.3

[Qemu-devel] [PATCH 1/3] qapi: Report support for -device cpu hotplug in query-machines

[Peter Krempa]

For management apps it's very useful to know whether the selected
machine type supports cpu hotplug via the new -device approach. Using
the presence of 'query-hotpluggable-cpus' is enough for a withess.

Add a property to 'MachineInfo' called 'hotpluggable-cpus' that will
report the presence of this feature.

Example of output:
{
"hotpluggable-cpus": false,
"name": "mac99",
"cpu-max": 1
},
{
"hotpluggable-cpus": true,
"name": "pseries-2.7",
"is-default": true,
    "cpu-max": 255,
"alias": "pseries"
},

Signed-off-by: Peter Krempa <pkre...@redhat.com>
---
 qapi-schema.json | 5 -
 vl.c | 1 +
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index 0964eec..24ede28 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2986,11 +2986,14 @@
 # @cpu-max: maximum number of CPUs supported by the machine type
 #   (since 1.5.0)
 #
+# @hotpluggable-cpus: cpu hotplug via -device is supported (since 2.7.0)
+#
 # Since: 1.2.0
 ##
 { 'struct': 'MachineInfo',
   'data': { 'name': 'str', '*alias': 'str',
-'*is-default': 'bool', 'cpu-max': 'int' } }
+'*is-default': 'bool', 'cpu-max': 'int',
+'hotpluggable-cpus': 'bool'} }

 ##
 # @query-machines:
diff --git a/vl.c b/vl.c
index c85833a..4c1f9ae 100644
--- a/vl.c
+++ b/vl.c
@@ -1524,6 +1524,7 @@ MachineInfoList *qmp_query_machines(Error **errp)

 info->name = g_strdup(mc->name);
 info->cpu_max = !mc->max_cpus ? 1 : mc->max_cpus;
+info->hotpluggable_cpus = !!mc->query_hotpluggable_cpus;

 entry = g_malloc0(sizeof(*entry));
 entry->value = info;
-- 
2.8.3

[Qemu-devel] [PATCH 3/3] [VARIANT 2] qapi: Change 'core-id' to 'core' in 'struct CPUCore'

[Peter Krempa]

CpuInstanceProperties uses 'core' as the property name. As docs for
query-hotpluggable-cpus state that the cpu core properties should be
passed back to device_add by management in case new members are added
and thus the names for the fields should be kept in sync.

Signed-off-by: Peter Krempa <pkre...@redhat.com>
---
 hw/cpu/core.c   |  6 +++---
 hw/ppc/spapr_cpu_core.c | 16 
 include/hw/cpu/core.h   |  4 ++--
 3 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/hw/cpu/core.c b/hw/cpu/core.c
index eff90c1..8484cab 100644
--- a/hw/cpu/core.c
+++ b/hw/cpu/core.c
@@ -15,7 +15,7 @@ static void core_prop_get_core_id(Object *obj, Visitor *v, 
const char *name,
   void *opaque, Error **errp)
 {
 CPUCore *core = CPU_CORE(obj);
-int64_t value = core->core_id;
+int64_t value = core->core;

 visit_type_int(v, name, , errp);
 }
@@ -33,7 +33,7 @@ static void core_prop_set_core_id(Object *obj, Visitor *v, 
const char *name,
 return;
 }

-core->core_id = value;
+core->core = value;
 }

 static void core_prop_get_nr_threads(Object *obj, Visitor *v, const char *name,
@@ -65,7 +65,7 @@ static void cpu_core_instance_init(Object *obj)
 {
 CPUCore *core = CPU_CORE(obj);

-object_property_add(obj, "core-id", "int", core_prop_get_core_id,
+object_property_add(obj, "core", "int", core_prop_get_core_id,
 core_prop_set_core_id, NULL, NULL, NULL);
 object_property_add(obj, "nr-threads", "int", core_prop_get_nr_threads,
 core_prop_set_nr_threads, NULL, NULL, NULL);
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 3a5da09..74aeda5 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -118,7 +118,7 @@ static void spapr_core_release(DeviceState *dev, void 
*opaque)
 object_unparent(obj);
 }

-spapr->cores[cc->core_id / smt] = NULL;
+spapr->cores[cc->core / smt] = NULL;

 g_free(core->threads);
 object_unparent(OBJECT(dev));
@@ -163,8 +163,8 @@ void spapr_core_plug(HotplugHandler *hotplug_dev, 
DeviceState *dev,
 int index;
 int smt = kvmppc_smt_threads();

-drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, cc->core_id);
-index = cc->core_id / smt;
+drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, cc->core);
+index = cc->core / smt;
 spapr->cores[index] = OBJECT(dev);

 if (!smc->dr_cpu_enabled) {
@@ -239,19 +239,19 @@ void spapr_core_pre_plug(HotplugHandler *hotplug_dev, 
DeviceState *dev,
 goto out;
 }

-if (cc->core_id % smt) {
-error_setg(_err, "invalid core id %d\n", cc->core_id);
+if (cc->core % smt) {
+error_setg(_err, "invalid core id %d\n", cc->core);
 goto out;
 }

-index = cc->core_id / smt;
+index = cc->core / smt;
 if (index < 0 || index >= spapr_max_cores) {
-error_setg(_err, "core id %d out of range", cc->core_id);
+error_setg(_err, "core id %d out of range", cc->core);
 goto out;
 }

 if (spapr->cores[index]) {
-error_setg(_err, "core %d already populated", cc->core_id);
+error_setg(_err, "core %d already populated", cc->core);
 goto out;
 }

diff --git a/include/hw/cpu/core.h b/include/hw/cpu/core.h
index 79ac79c..3b00c37 100644
--- a/include/hw/cpu/core.h
+++ b/include/hw/cpu/core.h
@@ -22,13 +22,13 @@ typedef struct CPUCore {
 DeviceState parent_obj;

 /*< public >*/
-int core_id;
+int core;
 int nr_threads;
 } CPUCore;

 /* Note: topology field names need to be kept in sync with
  * 'CpuInstanceProperties' */

-#define CPU_CORE_PROP_CORE_ID "core-id"
+#define CPU_CORE_PROP_CORE_ID "core"

 #endif
-- 
2.8.3

[Qemu-devel] [PATCH 0/3] qapi: Fix up cpu hotplug property names and add witness for cpu hotplug support

[Peter Krempa]

I started figuring out how to add support for this to libvirt and discovered a
few problems that made it not-easy to use for management.

Patch 1 adds a witness to allow us to properly detect the support for the
feature.

Patches 2 and 3 are two options to fix discrepancy between names that need to
be used with -device and those reported by query-hotpluggable-cpus.

Peter Krempa (3):
  qapi: Report support for -device cpu hotplug in query-machines
  VARIANT 1: qapi: Change 'core' to 'core-id' in 'CpuInstanceProperties'
  VARIANT 2: qapi: Change 'core-id' to 'core' in 'struct CPUCore'

 hmp.c   |  4 ++--
 hw/cpu/core.c   |  6 +++---
 hw/ppc/spapr.c  |  4 ++--
 hw/ppc/spapr_cpu_core.c | 16 
 include/hw/cpu/core.h   |  7 +--
 qapi-schema.json|  9 ++---
 vl.c|  1 +
 7 files changed, 27 insertions(+), 20 deletions(-)

-- 
2.8.3

Re: [Qemu-devel] [PATCH RFC 4/8] target-i386: cpu: consolidate calls of object_property_parse() in x86_cpu_parse_featurestr

[Peter Krempa]

On Thu, Jun 02, 2016 at 18:31:04 +0200, Igor Mammedov wrote:
> On Thu, 2 Jun 2016 17:05:06 +0200
> Peter Krempa <pkre...@redhat.com> wrote:
> > On Thu, Jun 02, 2016 at 11:53:22 -0300, Eduardo Habkost wrote:
> > > On Thu, Jun 02, 2016 at 02:22:22PM +0200, Igor Mammedov wrote:

[...]

> > I couldn't find anything regarding xlevel (so we might actually not
> > support it at all), but we indeed do limit the hv_spinlock count:
> > 
> > 
> > if (def->hyperv_spinlocks < 0xFFF) {
> > virReportError(VIR_ERR_XML_ERROR, "%s",
> >_("HyperV spinlock retry count must be "
> >  "at least 4095"));
> > goto error;
> > }
> > 
> > Peter
> Peter,
> Does libvirt still uses -cpu xxx,+feat1,-feat2 syntax
> or canonical property syntax there feat1=on,feat2=off

We use the legacy one:

-cpu core2duo,+ds,+acpi,+ht,+tm,+ds_cpl, ...

and

-cpu 'qemu32,hv_relaxed,hv_vapic, ... 

for the hyperv features.

We probably can switch to the new one if there's a reasonable way how to
detect that qemu is supporting the new one.

Peter

Re: [Qemu-devel] [PATCH RFC 4/8] target-i386: cpu: consolidate calls of object_property_parse() in x86_cpu_parse_featurestr

[Peter Krempa]

On Thu, Jun 02, 2016 at 11:53:22 -0300, Eduardo Habkost wrote:
> (CCing libvirt folks)
> 
> BTW:
> 
> On Thu, Jun 02, 2016 at 02:22:22PM +0200, Igor Mammedov wrote:
> [...]
> > > /* Special cases: */
> > > if (!strcmp(name, "xlevel")) {
> > > numvalue = strtoul(val, , 0);
> > > if (!*val || *err) {
> > > error_setg(errp, "bad numerical value %s", val);
> > > return;
> > > }
> > > if (numvalue < 0x8000) {
> > > error_report("xlevel value shall always be >= 0x8000"
> > >  ", fixup will be removed in future 
> > > versions");
> > > numvalue += 0x8000;
> > > snprintf(num, sizeof(num), "%" PRIu32, numvalue);
> > > val = num;
> [...]
> > > } else if (!strcmp(name, "hv-spinlocks")) {
> > > const int min = 0xFFF;
> > > 
> > > numvalue = strtoul(val, , 0);
> > > if (!*val || *err) {
> > > error_setg(errp, "bad numerical value %s", val);
> > > return;
> > > }
> > > if (numvalue < min) {
> > > error_report("hv-spinlocks value shall always be >= 0x%x"
> > >  ", fixup will be removed in future versions",
> > >  min);
> > > numvalue = min;
> > > }
> 
> Those "fixup will be removed in future versions" warnings are
> present since QEMU 1.7. Assuming that libvirt never allowed those
> invalid values to be used in the configuration (did it?), I
> believe we can safely remove the hv-spinlocks and xlevel fixups
> in QEMU 2.7.

I couldn't find anything regarding xlevel (so we might actually not
support it at all), but we indeed do limit the hv_spinlock count:


if (def->hyperv_spinlocks < 0xFFF) {
virReportError(VIR_ERR_XML_ERROR, "%s",
   _("HyperV spinlock retry count must be "
 "at least 4095"));
goto error;
}

Peter

Re: [Qemu-devel] [libvirt] [RFC 12/42] pc: initialize legacy hotplug only for 2.6 and older machine types

[Peter Krempa]

On Thu, May 12, 2016 at 15:20:51 +0200, Igor Mammedov wrote:
> On Thu, 12 May 2016 13:04:23 +0200
> Peter Krempa <pkre...@redhat.com> wrote:
> 
> > On Thu, May 12, 2016 at 07:52:23 -0300, Eduardo Habkost wrote:
> > > I don't think we should do that, unless users already had time to
> > > update their scripts and libvirt had time to implement code
> > > supporting the new method.
> > > 
> > > I believe libvirt (and people's scripts) use maxcpus only when
> > > they want CPU hotplug, so making max_cpus > smp_cpus enable CPU
> > > hotplug implicitly would probably solve the compatibility issue.  
> > 
> > Libvirt uses maxcpus only if the configuration explicitly has less
> > active cpus than the maximum number. This option would be the best IMO.
> > 
> > > If we want to deprecate the use of maxcpus to enable CPU hotplug,
> > > then we can make it print a warning for a few releases, so people
> > > have time to update their code.  
> > 
> > At that point libvirt also needs a way to detect that the new argument
> > is supported by qemu, so we can start passing it on the command line
> > basically every time we now pass 'maxcpus'.
> > 
> > The warning will get most probably ignored by people using libvirt as
> > the stdout/err of qemu is not visible to them.
> Ok, to make things less complicated I'll drop machine.cpu-hotplug
> and leave it always enabled as it used to be and as Michael suggested.
> 
> I'll drop following patches: 12, 13, 14, 20, 23 and respin series

I actually don't mind disabling it. But I'd be glad if it was based on
the max_cpus value as Eduardo suggested.

Peter


signature.asc
Description: Digital signature

Re: [Qemu-devel] [libvirt] [RFC 12/42] pc: initialize legacy hotplug only for 2.6 and older machine types

[Peter Krempa]

On Thu, May 12, 2016 at 07:52:23 -0300, Eduardo Habkost wrote:
> I don't think we should do that, unless users already had time to
> update their scripts and libvirt had time to implement code
> supporting the new method.
> 
> I believe libvirt (and people's scripts) use maxcpus only when
> they want CPU hotplug, so making max_cpus > smp_cpus enable CPU
> hotplug implicitly would probably solve the compatibility issue.

Libvirt uses maxcpus only if the configuration explicitly has less
active cpus than the maximum number. This option would be the best IMO.

> If we want to deprecate the use of maxcpus to enable CPU hotplug,
> then we can make it print a warning for a few releases, so people
> have time to update their code.

At that point libvirt also needs a way to detect that the new argument
is supported by qemu, so we can start passing it on the command line
basically every time we now pass 'maxcpus'.

The warning will get most probably ignored by people using libvirt as
the stdout/err of qemu is not visible to them.

Peter


signature.asc
Description: Digital signature

[Qemu-devel] [PATCH] audio: pa: Set volume of recording stream instead of recording device

[Peter Krempa]

Since pulseaudio 1.0 it's possible to set the individual stream volume
rather than setting the device volume. With this, setting hardware mixer
of a emulated sound card doesn't mess up the volume configuration of the
host.

A side effect is that this limits compatible pulseaudio version to 1.0
which was released on 2011-09-27.

Signed-off-by: Peter Krempa <pkre...@redhat.com>
---
This patch is keeping the old coding style by keeping the space after function
name so that the styles are not mixed in this file.

 audio/paaudio.c | 11 +--
 configure   |  4 ++--
 2 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/audio/paaudio.c b/audio/paaudio.c
index 57678e7..65beb6f 100644
--- a/audio/paaudio.c
+++ b/audio/paaudio.c
@@ -781,23 +781,22 @@ static int qpa_ctl_in (HWVoiceIn *hw, int cmd, ...)

 pa_threaded_mainloop_lock (g->mainloop);

-/* FIXME: use the upcoming "set_source_output_{volume,mute}" */
-op = pa_context_set_source_volume_by_index (g->context,
-pa_stream_get_device_index (pa->stream),
+op = pa_context_set_source_output_volume (g->context,
+pa_stream_get_index (pa->stream),
 , NULL, NULL);
 if (!op) {
 qpa_logerr (pa_context_errno (g->context),
-"set_source_volume() failed\n");
+"set_source_output_volume() failed\n");
 } else {
 pa_operation_unref(op);
 }

-op = pa_context_set_source_mute_by_index (g->context,
+op = pa_context_set_source_output_mute (g->context,
 pa_stream_get_index (pa->stream),
 sw->vol.mute, NULL, NULL);
 if (!op) {
 qpa_logerr (pa_context_errno (g->context),
-"set_source_mute() failed\n");
+"set_source_output_mute() failed\n");
 } else {
 pa_operation_unref (op);
 }
diff --git a/configure b/configure
index c37fc5f..d71f27c 100755
--- a/configure
+++ b/configure
@@ -2795,8 +2795,8 @@ for drv in $audio_drv_list; do
 ;;

 pa)
-audio_drv_probe $drv pulse/mainloop.h "-lpulse" \
-"pa_mainloop *m = 0; pa_mainloop_free (m); return 0;"
+audio_drv_probe $drv pulse/pulseaudio.h "-lpulse" \
+"pa_context_set_source_output_volume(NULL, 0, NULL, NULL, NULL); 
return 0;"
 libs_softmmu="-lpulse $libs_softmmu"
 audio_pt_int="yes"
 ;;
-- 
2.8.2

Re: [Qemu-devel] [PATCH 2/2] hmp: Extend drive_del to delete nodes without BB

[Peter Krempa]

On Wed, Feb 24, 2016 at 18:54:45 +0100, Max Reitz wrote:
> On 23.02.2016 18:16, Kevin Wolf wrote:
> > Now that we can use drive_add to create new nodes without a BB, we also
> > want to be able to delete such nodes again.
> > 
> > Signed-off-by: Kevin Wolf 
> > ---
> >  blockdev.c | 9 +
> >  1 file changed, 9 insertions(+)

[..]

> > 
> 
> It's a bit strange to require the user to specify the node name using
> "node-name" for drive_add, but the to use "id" in drive_del; especially
> because x-blockdev-del uses "node-name", too.

Well, since 'x-blockdev-del' is considered unstable yet I can't really
use it in libvirt, thus we discussed using drive_del as a workaround.

It makes partially sense since we'd add the new node with 'drive_add' in
the first place.

Peter


signature.asc
Description: Digital signature

Re: [Qemu-devel] KVM call for agenda for 2016-03-01

[Peter Krempa]

On Thu, Feb 25, 2016 at 09:40:34 +0100, Thomas Huth wrote:
> On 23.02.2016 16:33, Juan Quintela wrote:
> > 
> > Hi
> > 
> > Please, send any topic that you are interested in covering.
> > 
> > At the end of Monday I will send an email with the agenda or the
> > cancellation of the call, so hurry up.
> 
> Shall we talk about CPU hotplug again? There are a lot of discussions
> going on, but as far as I can tell, it seems there is still no 100%
> common understanding yet how the final solution should really look like...

I'll gladly join for the libvirt part if the agenda will include this.

Peter


signature.asc
Description: Digital signature

Re: [Qemu-devel] [PATCH 2/8] qmp: create qmp_savevm command

[Peter Krempa]

On Fri, Jan 22, 2016 at 16:31:04 +0100, Markus Armbruster wrote:
> "Denis V. Lunev"  writes:
> > On 01/19/2016 09:11 PM, Markus Armbruster wrote:
> >> "Denis V. Lunev"  writes:

[...]

> >> This would be like HMP savevm with the following differences:
> >>
> >> * Separate parameters for tag and ID.  I'll have none of this
> >>overloading nonsense in QMP.
> > why do we need both 'name' and 'id' values in the future at all?
> > Why single descriptive parameter is not enough? From my point
> > of view all this overloading is non-sense and in future one
> > name parameter should be kept.
> 
> Ever since we support multiple snapshots (commit faea38e, Aug 2006), a
> snapshot has both an ID and a tag.  We can debate whether that's a good
> idea, but we can't go back in time and change the image formats.
> 
> Monitor commands and qemu-img let you identify a snapshot by ID or by
> tag.  They generally use a single, overloaded parameter, and match it
> against both.  Exception: QMP blockdev-snapshot-delete-internal-sync has
> two unoverloaded parameters, to avoid ambiguity.  Quoting commit
> 44e3e05:
> 
> This interface use id and name as optional parameters, to handle the
> case that one image contain multiple snapshots with same name which
> may be '', but with different id.
> 
> Adding parameter id is for historical compatiability reason, and
> that case is not possible in qemu's new interface for internal
> snapshot at block device level, but still possible in qemu-img.
> 
> Are you proposing to ditch the ability to identify snapshots by ID?  I
> doubt we can.

Speaking for libvirt: We don't care, we use the snapshot name as the
identifier, so ID can be killed from our PoV. Backwards compatibility
can be hard though ...

> 
> Since HMP commands should be build on top of QMP commands, HMP savevm's
> underlying QMP commands cannot be less general than HMP savevm.  HMP
> savevm can do both ID and tag.  Therefore, the underlying QMP commands
> must be able to do that, too.
> 
> Once we accept that, the only question is whether to continue to do that
> with a single overloaded parameter, or two unambigous ones.  For QMP, my
> answer is unambigous.
> 
> >> * Specify the destination block device.  I'll have none of this "pick a
> >>device in some magic, undocumented way" in QMP.
> > agreed
> >
> >> * Leave alone the other block devices.  Adding transaction actions to
> >>snapshot all the writable block devices to get a full system snapshot
> >>is the user's responsibility.
> > this requires clarification:
> > - do you mean that the list of the devices should come from the
> > management layer?
> 
> See below.
> 
> > If so this is error prone at the restoration stage. From my point of
> > view restoration
> > must check that all RW devices are restored properly and there are no missed
> > ones.
> 
> Agreed.
> 
> >   Do you propose to put this into libvirt? From my point of view
> > this flexibility
> > is not that necessary.
> 
> VM snapshot / restore has much in common with migration: both save and
> restore machine state.  Another similarity is that you have to ensure
> identical block device contents.  For migration, this is the management
> layer's job.  My point is: the management layer is already prepared to
> track block device contents.
> 
> For the special case all-internal snapshot / restore, QEMU provides
> convenience features:
> 
> * You can take a consistent, complete, all-internal snapshot with
>   savevm.
> 
> * You can restore all parts of an all-internal snapshot with loadvm.
> 
> * You can delete all parts of an all-internal snapshot with delvm.
> 
> Except "all parts" is a lie: if you manage to lose a block device since
> savevm, QEMU won't notice.  Ensuring you still have them all is left to
> the management layer even there.

If you work only on internal snapshots with libvirt we always make sure
that all parts of the 

> 
> Once external snapshots come into play, QEMU doesn't even try.  It has
> no idea where to find external snapshots.  It's completely left to the
> management layer.

This is still mostly buggy/unimplemented in libvirt although the plan is
to do it properly.

> 
> I'm not fundamentally opposed to convenience features in HMP or even
> QMP.  But I don't want to start with them.  I want us to get the core
> functionality right before we add convenience.

This depends on whether anybody would want to use that interface
manually. As we are speaking about QMP it's not really likely that
anyone would need convenience wrappers, since you need a lot of data and
decisions to do such operation properly anyways.

> 
> I wrote:
> 
> A point-in-time snapshot of a system consists of a snapshot of its
> machine state and snapshots of its storage.  All the snapshots need
> to be made at the same point in time for the result to be
> consistent.
> 
> and
> 
> I figure a fully general QMP interface 

Re: [Qemu-devel] [PATCH 2/8] qmp: create qmp_savevm command

[Peter Krempa]

On Tue, Jan 19, 2016 at 19:11:05 +0100, Markus Armbruster wrote:
> "Denis V. Lunev"  writes:
> 
> > On 01/18/2016 06:58 PM, Markus Armbruster wrote:
> >> "Denis V. Lunev"  writes:

...

> 
> The overloaded interface you propose is more complex than it seems.  You
> hide the complexity by not documenting its workings.  Not even to the
> (insufficient!) degree the HMP interface documents how its overloaded
> name parameters work.
> 
> Merely copying over the HMP documentation won't cut it.  The bar for new
> QMP interfaces is a fair bit higher than "no worse than HMP".  The new
> interface must reasonably sane for *QMP*, and sufficiently documented.
> 
> If we can't make a sane QMP interface, I'd rather have no QMP interface.
> However, I believe we *can* make a sane QMP interface if we put in the
> design work.

I have to agree on this one. A broken interface will introduce just more
pain in the long run. Management tools still can fall back to the old
command if they need to (granted that error handling sucks there though
.. [1]) and use it as previously.

[1]
http://libvirt.org/git/?p=libvirt.git;a=commitdiff;h=4ac14cde9ae925515009400c2186d7eec5478b05

> 
> The design work must start with a review of what we're trying to
> accomplish, and how to fit it into the rest of the system.  Here's my
> attempt.  Since my knowledge on snapshots is rather superficial, I'm
> cc'ing Kevin for additional snapshot expertise.  Kevin, please correct
> me when I talk nonsense.  I'm further cc'ing Eric and Peter for the
> management layer perspective.
> 
> A point-in-time snapshot of a system consists of a snapshot of its
> machine state and snapshots of its storage.  All the snapshots need to
> be made at the same point in time for the result to be consistent.
> 
> Snapshots of read-only storage carry no information and are commonly
> omitted.
> 
> Isolated storage snapshots can make sense, but snapshotting the machine
> state without also snapshotting the machine's storage doesn't sound
> useful to me.

Well, technically if all your storage is read-only then a full snapshot
is reduced to a machine state snapshot. Libvirt actually allows this for
external snapshots. For internal ones, it obviously doesn't work.

> 
> Both storage and machine state snapshots come in two flavours: internal
> and external.
> 
> External ones can be made with any block backend, but internal storage
> snapshots work only with certain formats, notably qcow2.  QMP supports
> both kinds of storage snapshots.
> 
> Both kinds of storage snapshots need exclusive access while they work.
> They're relatively quick, but the delay could be noticable for large
> internal snapshots, and perhaps for external snapshots on really slow
> storage.
> 
> Internal machine state snapshots are currently only available via HMP's
> savevm, which integrates internal machine state and storage snapshots.
> This is non-live, i.e. the guest is stopped while the snapshot gets
> saved.  I figure we could make it live if we really wanted to.  Another
> instance of the emerging background job concept.
> 
> On the implementation level, QCOW2 can't currently store a machine state
> snapshot without also storing a storage snapshot.  I guess we could
> change this if we really wanted to.
> 
> External machine state snapshots are basically migrate to file.
> Supported by QMP.
> 
> Live migration to file is possible, but currently wastes space, because
> memory dirtied during migration gets saved multiple times.  Could be
> fixed either by making migration update previously saved memory instead
> of appending (beware, random I/O), or by compacting the file afterwards.
> 
> Non-live migration to file doesn't waste space that way.
> 
> To take multiple *consistent* snapshots, you have to bundle them up in a
> transaction.  Transactions currently support only *storage* snapshots,
> though.
> 
> Work-around for external machine state snapshot: migrate to file
> (possibly live), leaving the guest sopped on completion, take storage
> snapshots, resume guest.

This is exactly what I've implemented in libvirt.

> 
> You can combine internal and external storage snapshots with an external
> machine state snapshot to get a mixed system snapshot.

Libvirt allows this for two separate snapshots of a single vm, but it's
really buggy. You can take such snapshot, but reverting or deleting it
is where the fun begins. In a mixed snapshot you need to track the
backing files along with the internal snapshots in qcow properly and
libvirt currently doesn't really allow that.

> 
> You currently can't do that with an internal machine state snapshot: the
> only way to take one is HMP savevm, which insists on internally
> snapshotting all writable storage, and doesn't transact together with
> external storage snapshots.
> 
> Except for the case "purely internal snapshot with just one writable
> storage device", a system snapshot consists of multiple parts stored in
> 

Re: [Qemu-devel] [RFC PATCH v0 0/9] Generic cpu-core device

[Peter Krempa]

On Wed, Dec 16, 2015 at 16:11:08 +0100, Igor Mammedov wrote:
> On Fri, 11 Dec 2015 09:27:57 +0530
> Bharata B Rao  wrote:
> 
> > On Thu, Dec 10, 2015 at 01:35:05PM +0100, Igor Mammedov wrote:
> > > On Thu, 10 Dec 2015 11:45:35 +0530
> > > Bharata B Rao  wrote:

[...]

> > > For initial conversion of x86-cpus to device-add we could do pretty
> > > much the same like we do now, where cpu devices will appear under:
> > > /machine (pc-i440fx-2.5-machine)
> > >   /unattached (container)
> > > /device[x] (qemu64-x86_64-cpu)
> > > 
> > > since we don't have to maintain/model dummy socket/core objects.
> > > 
> > > PowerPC could do the similar only at core level since it has
> > > need for modeling core objects.
> > > 
> > > It doesn't change anything wrt current introspection state, since
> > > cpus could be still found by mgmt tools that parse QOM tree.
> > > 
> > > We probably should split 2 conflicting goals we are trying to meet
> > > here,
> > > 
> > >  1. make device-add/dell work with cpus /
> > >  drop support for cpu-add in favor of device_add 
> > > 
> > >  2. how to model QOM tree view for CPUs in arch independent manner
> > > to make mgmt layer life easier.
> > > 
> > > and work on them independently instead of arguing for years,
> > > that would allow us to make progress in #1 while still thinking
> > > about how to do #2 the right way if we really need it.
> > 
> > Makes sense, s390 developer also recommends the same. Given that we
> > have CPU hotplug patchsets from x86, PowerPC and s390 all
> > implementing device_add semantics pending on the list, can we hope to
> > get them merged for QEMU-2.6 ?
> > 
> > So as seen below, the device is either "cpu_model-cpu_type" or just
> > "cpu_type".
> generic device_add isn't able to deal with 'cpu_model' stuff, so
> it should be concrete 'cpu_type'.
> Question is if libvirt can get a list of supported CPU types. 
> 
> > 
> > -device POWER8-powerpc64-cpu (pseries)
> > -device qemu64-x86_64-cpu (pc)
> > -device s390-cpu (s390)
> > 
> > Is this going to be the final acceptable semantics ? Would libvirt be
> > able to work with this different CPU device names for different
> > guests ?
> CCing Peter to check if libvirt could do it nad if his is ok with
> proposed device_add semantics as in the it's he who will deal with it
> on libvirt side.

Well, this depends entirely on the implementation and the variety of the
cpu device models. Libvirt requires that the cpu model for a given
arch/machine type/whatever can be inferred either completely offline or
via monitor commands that are available when qemu is started with the
'none' machine type. This is required as we query capabilities of a qemu
binary beforehand and then use the cached data to create the command
line. Running a qemu process just to query a cpu model is not acceptable
as it adds significant overhead.

Peter


signature.asc
Description: Digital signature

Re: [Qemu-devel] [PATCH 1/1] qmp: process system-reset event in paused state

[Peter Krempa]

On Wed, Dec 16, 2015 at 12:32:13 +0300, Denis V. Lunev wrote:
> On 12/16/2015 12:12 PM, Paolo Bonzini wrote:
> >
> > On 16/12/2015 10:00, Denis V. Lunev wrote:
> >> With pvpanic or HyperV panic devices could be moved into the paused state
> >> with ' preserve'. In this state VM reacts only to
> >> 'virsh destroy' or 'continue'.
> >>
> >> 'virsh reset' command is usually used to force guest reset. The expectation
> >> of the behavior of this command is that the guest will be force restarted.
> >> This is not true at the moment.
> > Does "virsh reset" + "virsh continue" work, and if not why?
> as far as I can see there is no such command in virsh at all :(

Paolo probably meant 'virsh resume $VM'.



signature.asc
Description: Digital signature

Re: [Qemu-devel] [PATCH 1/1] qmp: process system-reset event in paused state

[Peter Krempa]

On Wed, Dec 16, 2015 at 10:12:20 +0100, Paolo Bonzini wrote:
> 
> 
> On 16/12/2015 10:00, Denis V. Lunev wrote:
> > With pvpanic or HyperV panic devices could be moved into the paused state
> > with ' preserve'. In this state VM reacts only to
> > 'virsh destroy' or 'continue'.
> > 
> > 'virsh reset' command is usually used to force guest reset. The expectation
> > of the behavior of this command is that the guest will be force restarted.
> > This is not true at the moment.
> 
> Does "virsh reset" + "virsh continue" work, and if not why?

So .. it won't work:

state = virDomainObjGetState(vm, NULL);
if (state == VIR_DOMAIN_PMSUSPENDED) {
virReportError(VIR_ERR_OPERATION_INVALID,
   "%s", _("domain is pmsuspended"));
goto endjob;
} else if (state == VIR_DOMAIN_PAUSED) {
if (qemuProcessStartCPUs(driver, vm, dom->conn,
 VIR_DOMAIN_RUNNING_UNPAUSED,
 QEMU_ASYNC_JOB_NONE) < 0) {
if (virGetLastError() == NULL)
virReportError(VIR_ERR_OPERATION_FAILED,
   "%s", _("resume operation failed"));
goto endjob;
}
event = virDomainEventLifecycleNewFromObj(vm,
 VIR_DOMAIN_EVENT_RESUMED,
 VIR_DOMAIN_EVENT_RESUMED_UNPAUSED);
}


We check that the state is "paused" and continue the vCPUs only in that
case. The panic devices will move the VM to 'crashed' state.
The code that is issuing 'system_reset' does not modify the state
in any way.

> 
> > Thus it is quite natural to process 'virh reset' aka qmp_system_reset
> > this way, i.e. allow to reset the guest. This behavior is similar to
> > one observed with 'reset' button on real hardware :)
> 
> Paolo
> 
> > Signed-off-by: Denis V. Lunev 
> > CC: Markus Armbruster 
> > CC: Dmitry Andreev 
> > ---
> >  qmp.c | 4 
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/qmp.c b/qmp.c
> > index 0a1fa19..df17a33 100644
> > --- a/qmp.c
> > +++ b/qmp.c
> > @@ -112,6 +112,10 @@ void qmp_stop(Error **errp)
> >  void qmp_system_reset(Error **errp)
> >  {
> >  qemu_system_reset_request();
> > +
> > +if (!runstate_is_running()) {
> > +vm_start();
> > +}

I'd say NACK here. This will break the possibility to reset a system
while the vCPUs are paused. The problem should be fixed in libvirt.

> >  }
> >  
> >  void qmp_system_powerdown(Error **erp)
> > 
> 

Peter


signature.asc
Description: Digital signature

Re: [Qemu-devel] [PATCH v5 00/10] sPAPR CPU hotplug

[Peter Krempa]

On Fri, Nov 20, 2015 at 18:24:29 +0530, Bharata B Rao wrote:
> This patchset adds CPU hotplug support for sPAPR PowerPC guests using
> device_add and device_del commands
> 
> (qemu) device_add POWER8-powerpc64-cpu,id=cpu0

Is there a reason why this uses 'device_add' rather than the 'cpu_add'
command? Libvirt uses two separate approaches already. Due to legacy
reasons we support the HMP 'cpu_set' command, and lately we added
support for QMP 'cpu-add'. Using device_add here will introduce a
different approach and will require yet another compatibility layer in
libvirt to support this.

Peter



signature.asc
Description: Digital signature

Re: [Qemu-devel] [PATCH 3/3] block/gluster: add support for multiple gluster servers

[Peter Krempa]

On Thu, Nov 05, 2015 at 07:45:50 -0500, Prasanna Kumar Kalever wrote:
> On Thursday, November 5, 2015 6:07:06 PM, Prasanna Kumar Kalever wrote:
> > This patch adds a way to specify multiple volfile servers to the gluster
> > block backend of QEMU with tcp|rdma transport types and their port numbers.
> > 
> > Problem:
> > 
> > Currently VM Image on gluster volume is specified like this:
> 

[...]

> > @@ -345,7 +676,7 @@ static int qemu_gluster_open(BlockDriverState *bs, QDict
> > *options,
> >  
> >  out:
> >  qemu_opts_del(opts);
> > -qemu_gluster_gconf_free(gconf);
> > +qapi_free_BlockdevOptionsGluster(gconf);
> 
> Can some one help me please ?
> This leads to crash in the second iteration i.e. while freeing 
> "gconf->servers->next->value"

So, prior to this you allocate a array of the data structures as:

+gsconf = g_new0(GlusterServer, num_servers);
+
+ptr = qemu_opt_get(opts, GLUSTER_OPT_VOLUME);
+if (!ptr) {
+error_setg(_err, "Error: qemu_gluster: please provide 'volume' "
+   "option");
+goto out;
+}

Then you use the following code to fill the linked list:

+  if (gconf->servers == NULL) {
+gconf->servers = g_new0(GlusterServerList, 1);
+gconf->servers->value = [i];

So here you set the value. For a i of 0 the '[i]' expression will
be a pointer with equal address to 'gsconf'. For explanation:

'gsconf[i]' can be written as '*(gsconf + i)', so
'[i]' becomes basically '&(*(gsconf + i))'

This can be also simplified to:
'gsconf + i'. For a i of 0 this becomes the same pointer as 'gsconf'

And once you use that with free(), the whole gsconf array will be freed.
All the other pointers that you've filled to the linked list become
invalid, since they were pointing into the same array that was
completely freed in the first iteration.

Peter


signature.asc
Description: Digital signature

Re: [Qemu-devel] [PATCH v9 3/3] block/gluster: add support for multiple gluster servers

[Peter Krempa]

On Wed, Oct 21, 2015 at 19:04:11 +0530, Prasanna Kumar Kalever wrote:

...

> ---
>  block/gluster.c  | 420 
> +--
>  qapi/block-core.json |  62 +++-
>  2 files changed, 433 insertions(+), 49 deletions(-)
> 
> diff --git a/block/gluster.c b/block/gluster.c
> index ededda2..62b6656 100644
> --- a/block/gluster.c
> +++ b/block/gluster.c

...

> +
>  
>  static void qemu_gluster_gconf_free(GlusterConf *gconf)
>  {
>  if (gconf) {
> -g_free(gconf->host);
>  g_free(gconf->volume);
>  g_free(gconf->path);
> -g_free(gconf->transport);
> +if (gconf->gsconf) {
> +g_free(gconf->gsconf[0].host);
> +g_free(gconf->gsconf[0].transport);
> +g_free(gconf->gsconf);

Looks like this leaks second and any further server config struct.

> +}
>  g_free(gconf);
>  }
>  }

Peter



signature.asc
Description: Digital signature

Re: [Qemu-devel] [PATCH v7 1/1] block/gluster: add support for multiple gluster backup volfile servers

[Peter Krempa]

On Wed, Oct 14, 2015 at 12:36:58 +0530, Prasanna Kumar Kalever wrote:
> This patch adds a way to specify multiple volfile servers to the gluster
> block backend of QEMU with tcp|rdma transport types and their port numbers.
> 
> Problem:
> 
> Currenly VM Image on gluster volume is specified like this:
> 
> file=gluster[+tcp]://host[:port]/testvol/a.img
> 
> Assuming we have three hosts in trustred pool with replica 3 volume
> in action and unfortunately host (mentioned in the command above) went down
> for some reason, since the volume is replica 3 we now have other 2 hosts
> active from which we can boot the VM.
> 
> But currently there is no mechanism to pass the other 2 gluster host
> addresses to qemu.
> 
> Solution:
> 
> New way of specifying VM Image on gluster volume with volfile servers:
> (We still support old syntax to maintain backward compatibility)
> 
> Basic command line syntax looks like:
> 
> Pattern I:
>  -drive driver=gluster,
> volume=testvol,path=/path/a.raw,
> servers.0.host=1.2.3.4,
>[servers.0.port=24007,]
>[servers.0.transport=tcp,]
> servers.1.host=5.6.7.8,
>[servers.1.port=24008,]
>[servers.1.transport=rdma,] ...
> 
> Pattern II:
>  'json:{"driver":"qcow2","file":{"driver":"gluster",
>"volume":"testvol","path":"/path/a.qcow2",
>"servers":[{tuple0},{tuple1}, ...{tupleN}]}}'
> 
>driver  => 'gluster' (protocol name)
>volume  => name of gluster volume where our VM image resides
>path=> absolute path of image in gluster volume
> 
>   {tuple}  => {"host":"1.2.3.4"[,"port":"24007","transport":"tcp"]}
> 
>host=> host address (hostname/ipv4/ipv6 addresses)
>port=> port number on which glusterd is listening. (default 24007)
>tranport=> transport type used to connect to gluster management daemon,

s/tranport/transport/

>it can be tcp|rdma (default 'tcp')
> 
> Examples:
> 1.
>  -drive driver=qcow2,file.driver=gluster,
> file.volume=testvol,file.path=/path/a.qcow2,
> file.servers.0.host=1.2.3.4,
> file.servers.0.port=24007,
> file.servers.0.transport=tcp,
> file.servers.1.host=5.6.7.8,
> file.servers.1.port=24008,
> file.servers.1.transport=rdma
> 2.
>  'json:{"driver":"qcow2","file":{"driver":"gluster","volume":"testvol",
>  "path":"/path/a.qcow2","servers":
>  [{"host":"1.2.3.4","port":"24007","transport":"tcp"},
>   {"host":"4.5.6.7","port":"24008","transport":"rdma"}] } }'
> 
> This patch gives a mechanism to provide all the server addresses, which are in
> replica set, so in case host1 is down VM can still boot from any of the
> active hosts.
> 
> This is equivalent to the backup-volfile-servers option supported by
> mount.glusterfs (FUSE way of mounting gluster volume)
> 
> This patch depends on a recent fix in libgfapi raised as part of this work:
> http://review.gluster.org/#/c/12114/

According to the commit message of the gluster change this is not really
required since the code below actually uses it's own defaults if the
user didn't provide any so the libgfapi is never called with NULL/0

> 
> Credits: Sincere thanks to Kevin Wolf  and
> "Deepak C Shetty"  for inputs and all their support
> 
> Signed-off-by: Prasanna Kumar Kalever 
> ---

[...]

> @@ -24,22 +35,108 @@ typedef struct BDRVGlusterState {
>  struct glfs_fd *fd;
>  } BDRVGlusterState;
>  
> -typedef struct GlusterConf {
> -char *server;
> -int port;
> -char *volname;
> -char *image;
> +typedef struct BDRVGlusterReopenState {
> +struct glfs *glfs;
> +struct glfs_fd *fd;
> +} BDRVGlusterReopenState;
> +
> +typedef struct GlusterServerConf {
> +char *host;
> +int   port;
>  char *transport;
> +} GlusterServerConf;
> +
> +typedef struct GlusterConf {
> +char *volume;
> +char *path;
> +GlusterServerConf *gsconf;
>  } GlusterConf;

I'd suggest you split up the renaming process of the variables from the
actual addition of the multi-host support. As you will see later, it
makes this patch cluttered with irrelevant changes.

>  
> +static QemuOptsList qemu_gluster_create_opts = {
> +.name = "qemu-gluster-create-opts",
> +.head = QTAILQ_HEAD_INITIALIZER(qemu_gluster_create_opts.head),
> +.desc = {
> +{
> +.name = BLOCK_OPT_SIZE,
> +.type = QEMU_OPT_SIZE,
> +.help = "Virtual disk size"
> +},
> +{
> +.name = BLOCK_OPT_PREALLOC,
> +.type = QEMU_OPT_STRING,
> +.help = "Preallocation mode (allowed values: off, full)"
> +},
> +{ /* end of list */ }
> +}
> +};
> +
> +static QemuOptsList runtime_opts = {
> +.name = "gluster",
> +.head = QTAILQ_HEAD_INITIALIZER(runtime_opts.head),
> +.desc = {
> +{
> +.name = GLUSTER_OPT_FILENAME,

Re: [Qemu-devel] [PATCH v5 1/1] block/gluster: add support for multiple gluster backup volfile servers

[Peter Krempa]

On Mon, Sep 28, 2015 at 18:06:12 +0530, Prasanna Kumar Kalever wrote:
> This patch adds a way to specify multiple volfile servers to the gluster
> block backend of QEMU with tcp|rdma transport types and their port numbers.
> 
> Problem:
> 
> Currenly VM Image on gluster volume is specified like this:
> 
> file=gluster[+tcp]://server1[:port]/testvol/a.img
> 
> Assuming we have have three servers in trustred pool with replica 3 volume
> in action and unfortunately server1 (mentioned in the command above) went down
> for some reason, since the volume is replica 3 we now have other 2 servers
> active from which we can boot the VM.
> 
> But currently there is no mechanism to pass the other 2 gluster server
> addresses to qemu.
> 
> Solution:
> 
> New way of specifying VM Image on gluster volume with volfile servers:
> (We still support old syntax to maintain backward compatibility)
> 
> Basic command line syntax looks like:
> 
> Pattern I:
>  -drive driver=gluster,
> volname=testvol,image-path=/path/a.raw,
> volfile-servers.0.server=1.2.3.4,

I still think 'volfile-servers' should be just 'server'. I don't
understand why it needs to contain anything else. See below for
suggestions ...

>[volfile-servers.0.port=24007,]
>[volfile-servers.0.transport=tcp,]
> volfile-servers.1.server=5.6.7.8,
>[volfile-servers.1.port=24008,]
>[volfile-servers.1.transport=rdma,] ...
> 
> Pattern II:
>  'json:{"driver":"qcow2","file":{"driver":"gluster",
>"volname":"testvol","image-path":"/path/a.qcow2",
>"volfile-servers":[{tuple0},{tuple1}, ...{tupleN}]}}'
> 
>driver   => 'gluster' (protocol name)
>volname  => name of gluster volume where our VM image resides
>image-path   => is the absolute path of image in gluster volume
> 
>   {tuple}   => {"server":"1.2.3.4"[,"port":"24007","transport":"tcp"]}
> 
>server   => server address (hostname/ipv4/ipv6 addresses)
>port => port number on which glusterd is listening. (default 24007)
>tranport => transport type used to connect to gluster management 
> daemon,
> it can be tcp|rdma (default 'tcp')
> 
> Examples:
> 1.
>  -drive driver=qcow2,file.driver=gluster,
> file.volname=testvol,file.image-path=/path/a.qcow2,
> file.volfile-servers.0.server=1.2.3.4,
> file.volfile-servers.0.port=24007,
> file.volfile-servers.0.transport=tcp,
> file.volfile-servers.1.server=5.6.7.8,
> file.volfile-servers.1.port=24008,
> file.volfile-servers.1.transport=rdma
> 2.
>  'json:{"driver":"qcow2","file":{"driver":"gluster","volname":"testvol",
>  "image-path":"/path/a.qcow2","volfile-servers":
>  [{"server":"1.2.3.4","port":"24007","transport":"tcp"},
>   {"server":"4.5.6.7","port":"24008","transport":"rdma"}] } }'

  -drive driver=qcow2,file.driver=gluster,
 file.volume=testvol,
 file.path=/path/a.qcow2,
 file.server.0.host=1.2.3.4,
 file.server.0.port=24007,
 file.server.0.transport=tcp,
 file.server.1.host=5.6.7.8,
 file.server.1.port=24008,
 file.server.1.transport=rdma

I'm suggesting the above naming scheme.
So:
'path' instead of 'image-path'
'volume' instead of 'volname'
'server' instead of 'volfile-servers'
'host' instead of 'server'

 2.
  'json:{"driver":"qcow2","file":{"driver":"gluster","volume":"testvol",
  "path":"/path/a.qcow2","server":
  [{"host":"1.2.3.4","port":"24007","transport":"tcp"},
   {"host":"4.5.6.7","port":"24008","transport":"rdma"}] } }'

> 
> This patch gives a mechanism to provide all the server addresses which are in
> replica set, so in case server1 is down VM can still boot from any of the
> active servers.
> 
> This is equivalent to the volfile-servers option supported by
> mount.glusterfs (FUSE way of mounting gluster volume)

I don't think qemu needs to follow mount.glusterfs in naming.

> 
> This patch depends on a recent fix in libgfapi raised as part of this work:
> http://review.gluster.org/#/c/12114/
> 
> Credits: Sincere thanks to Kevin Wolf  and
> "Deepak C Shetty"  for inputs and all their support
> 
> Signed-off-by: Prasanna Kumar Kalever 
> ---

[snip]

> diff --git a/block/gluster.c b/block/gluster.c
> index 1eb3a8c..63c3dcb 100644
> --- a/block/gluster.c
> +++ b/block/gluster.c
> @@ -11,6 +11,15 @@
>  #include "block/block_int.h"
>  #include "qemu/uri.h"
>  
> +#define GLUSTER_OPT_FILENAME  "filename"
> +#define GLUSTER_OPT_VOLNAME   "volname"
> +#define GLUSTER_OPT_IMAGE_PATH"image-path"
> +#define GLUSTER_OPT_SERVER"server"
> +#define GLUSTER_OPT_PORT  "port"
> +#define GLUSTER_OPT_TRANSPORT "transport"
> +#define GLUSTER_OPT_READ_PATTERN  "volfile-servers."
> +
> +
>  typedef struct GlusterAIOCB {
>  int64_t size;
>  int ret;
> @@ -43,6 +52,60 @@ 

Re: [Qemu-devel] [PATCH v5 1/1] block/gluster: add support for multiple gluster backup volfile servers

[Peter Krempa]

[ trimmed the CC list for this ]

On Wed, Oct 07, 2015 at 06:15:59 -0400, Prasanna Kalever wrote:
> Hi Peter & Kevin,
> 
> Thanks for your detailed review comments. I shall try to incorporate these 
> changes as a next patch-set.
> 
> - Prasanna Kumar Kalever
> 
>

Please don't top post on technical lists ... and ...

> 
> > On Mon, Sep 28, 2015 at 18:06:12 +0530, Prasanna Kumar Kalever wrote:
> > > This patch adds a way to specify multiple volfile servers to the gluster
> > > block backend of QEMU with tcp|rdma transport types and their port 
> > > numbers.
> > > 
> > > Problem:

... trim parts of messages that are no longer relevant.

http://www.idallen.com/topposting.html


signature.asc
Description: Digital signature

Re: [Qemu-devel] [PATCH v3 1/1] block/gluster: add support for multiple gluster backup volfile servers

[Peter Krempa]

On Tue, Sep 22, 2015 at 04:06:54 -0400, Prasanna Kalever wrote:
> 
> > On 09/21/2015 05:24 AM, Prasanna Kumar Kalever wrote:
> > > This patch adds a way to specify multiple backup volfile servers to the
> > > gluster
> > > block backend of QEMU with tcp|rdma transport types and their port 
> > > numbers.
> > > 
> > 

...

> 
> > > +##
> > > +{ 'struct': 'BlockdevOptionsGluster',
> > > +  'data': { 'volname': 'str',
> > > +'image-path': 'str',
> > > +'backup-volfile-servers': [ 'GlusterTuplePattern' ] } }
> > 
> > Shouldn't this be simply 'volfile-servers', as you are including the
> > primary server in addition to the backup servers?
> >
> 
> Again I want to maintain naming as mount.glusterfs do for fuse.

Well, I have to agree with Eric here. I think the option name doesn't
need to be kept in sync with the gluster implementation since they don't
share anything with qemu and since the array contains also the primary
server to be queried the word backup doesn't make snese there.

Peter


signature.asc
Description: Digital signature

Re: [Qemu-devel] [PATCH 1/1] block/gluster: add support for multiple gluster backup volfile servers

[Peter Krempa]

On Tue, Sep 08, 2015 at 18:34:09 +0530, Prasanna Kumar Kalever wrote:
> This patch adds a way to specify multiple backup volfile servers to the 
> gluster
> block backend of QEMU with both tcp and rdma transport types.
> 
> Problem:
> 
> Currenly VM Image on gluster volume is specified like this:
> 
> file=gluster[+tcp]://server1:24007/testvol/a.img
> 
> Assuming we have have three servers in trustred pool with replica 3 volume
> in action and unfortunately server1 (mentioned in the command above) went down
> for some reason, since the volume is replica 3 we now have other 2 servers
> active from which we can boot the VM.
> 
> But currently there is no mechanism to pass the other 2 gluster server
> addresses to qemu.
> 
> Solution:
> 
> New way of specifying VM Image on gluster volume with backup volfile servers:
> 
> file=gluster[+transport-type]://server1:24007/testvol/a.img\
>  ?backup-volfile-servers=server2=server3

This syntax doesn't seem to support different port numbers for the
backup volfile servers since it's possible to specify the port number
for the primary one, the backup ones should allow that too to support
all possible configurations.

> 
> This patch gives a mechanism to provide all the server addresses which are in
> replica set, so in case server1 is down VM can still boot from any of the
> active servers.
> 
> This is equivalent to the backup-volfile-servers option supported by
> mount.glusterfs (FUSE way of mounting gluster volume)
> 
> Signed-off-by: Prasanna Kumar Kalever 
> ---
>  block/gluster.c | 118 
> +++-
>  1 file changed, 83 insertions(+), 35 deletions(-)
> 

Peter


signature.asc
Description: Digital signature

[Qemu-devel] [PATCH v2] util: socket: Add missing localaddr and localport option for DGRAM socket

[Peter Krempa]

The 'socket_optslist' structure does not contain the 'localaddr' and
'localport' options that are parsed in case you are creating a
'connect' type UDP character device.

I've noticed it happening after commit f43e47dbf6de24db20ec9b588bb6cc762
made qemu abort() after seeing the invalid option.

A minimal reproducer for the case is:
$ qemu-system-x86_64 -chardev 
udp,id=charrng0,host=127.0.0.1,port=1234,localaddr=,localport=1234
qemu-system-x86_64: -chardev 
udp,id=charrng0,host=127.0.0.1,port=1234,localaddr=,localport=1234: Invalid 
parameter 'localaddr'
Aborted (core dumped)

Prior to the commit mentioned above the error would be printed but the
value for localaddr and localport was simply ignored. I did not go
trhough the code to find out when it was broken.

Add the two fields so that the options can again be parsed correctly and
qemu doesn't abort().

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1220252

Signed-off-by: Peter Krempa pkre...@redhat.com
Reviewed-by: Eric Blake ebl...@redhat.com
---

Notes:
Version 2:
- improved commit message as suggested by Markus

Cc: Markus Armbruster arm...@redhat.com

 util/qemu-sockets.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 87c9bc6..72066be 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -45,6 +45,12 @@ QemuOptsList socket_optslist = {
 .name = port,
 .type = QEMU_OPT_STRING,
 },{
+.name = localaddr,
+.type = QEMU_OPT_STRING,
+},{
+.name = localport,
+.type = QEMU_OPT_STRING,
+},{
 .name = to,
 .type = QEMU_OPT_NUMBER,
 },{
-- 
2.3.5

[Qemu-devel] [PATCH] util: socket: Add missing localaddr and localport option for DGRAM socket

[Peter Krempa]

The 'socket_optslist' structure does not contain the 'localaddr' and
'localport' options that are parsed in case you are creating a
'connect' type UDP character device. This causes abort of qemu after
commit:

commit f43e47dbf6de24db20ec9b588bb6cc762093dd69
Author: Markus Armbruster arm...@redhat.com
Date:   Thu Feb 12 17:52:20 2015 +0100

QemuOpts: Drop qemu_opt_set(), rename qemu_opt_set_err(), fix use

Add the two fields so that the options can again be parsed correctly.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1220252

Signed-off-by: Peter Krempa pkre...@redhat.com
---
Cc: Eric Blake ebl...@redhat.com
Cc: Markus Armbruster arm...@redhat.com

 util/qemu-sockets.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 87c9bc6..72066be 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -45,6 +45,12 @@ QemuOptsList socket_optslist = {
 .name = port,
 .type = QEMU_OPT_STRING,
 },{
+.name = localaddr,
+.type = QEMU_OPT_STRING,
+},{
+.name = localport,
+.type = QEMU_OPT_STRING,
+},{
 .name = to,
 .type = QEMU_OPT_NUMBER,
 },{
-- 
2.3.5

Re: [Qemu-devel] [PATCH 1/2] qemu-options.hx: improve -m description

[Peter Krempa]

On Thu, Feb 26, 2015 at 14:49:15 -0500, Luiz Capitulino wrote:
 Add memory hotplug options to the command-line format. Also,
 add a complete command-line example and improve description.
 
 Signed-off-by: Luiz Capitulino lcapitul...@redhat.com
 ---
  qemu-options.hx | 23 ++-
  1 file changed, 18 insertions(+), 5 deletions(-)
 
 diff --git a/qemu-options.hx b/qemu-options.hx
 index 85ca3ad..1634175 100644
 --- a/qemu-options.hx
 +++ b/qemu-options.hx
 @@ -237,12 +237,25 @@ DEF(m, HAS_ARG, QEMU_OPTION_m,
  NOTE: Some architectures might enforce a specific granularity\n,
  QEMU_ARCH_ALL)
  STEXI
 -@item -m [size=]@var{megs}
 +@item -m [size=]@var{megs}[,slots=n,maxmem=size]
  @findex -m
 -Set virtual RAM size to @var{megs} megabytes. Default is 128 MiB.  
 Optionally,
 -a suffix of ``M'' or ``G'' can be used to signify a value in megabytes or
 -gigabytes respectively. Optional pair @var{slots}, @var{maxmem} could be used
 -to set amount of hotluggable memory slots and possible maximum amount of 
 memory.
 +Sets guest startup RAM size to @var{megs} megabytes. Default is 128 MiB.
 +Optionally, a suffix of ``M'' or ``G'' can be used to signify a value in
 +megabytes or gigabytes respectively. Optional pair @var{slots}, @var{maxmem}
 +could be used to set amount of hotpluggable memory slots and maximum amount 
 of
 +memory.
 +
 +For example, the following command-line sets the guest startup RAM size to
 +1GB, creates 3 slots to hotplug additional memory and sets the maximum
 +memory the guest can reach to 4GB:
 +
 +@example
 +qemy-system-i386 -m 1G,slots=3,maxmem=4G
 +@end example
 +
 +If @var{slots} and @var{maxmem} are not specified, memory hotplug won't
 +be enabled and the guest startup RAM will never increase (although it can
 +be decreased with the use of ballooning).
  ETEXI

It might be also worth noting that maxmem has to be aligned to the page
size. See:
http://lists.gnu.org/archive/html/qemu-devel/2015-01/msg04345.html

(unfortunately it was not merged yet for some reason ...)

Peter


signature.asc
Description: Digital signature

Re: [Qemu-devel] [PATCH 2/2] docs: add memory-hotplug.txt

[Peter Krempa]

On Thu, Feb 26, 2015 at 14:49:16 -0500, Luiz Capitulino wrote:
 This document describes how to use memory hotplug in QEMU.
 
 Signed-off-by: Luiz Capitulino lcapitul...@redhat.com
 ---
  docs/memory-hotplug.txt | 77 
 +
  1 file changed, 77 insertions(+)
  create mode 100644 docs/memory-hotplug.txt
 
 diff --git a/docs/memory-hotplug.txt b/docs/memory-hotplug.txt
 new file mode 100644
 index 000..e821449
 --- /dev/null
 +++ b/docs/memory-hotplug.txt
 @@ -0,0 +1,77 @@
 +QEMU memory hotplug
 +===
 +
 +This document explains how to use the memory hotplug feature in QEMU,
 +which is present since v2.1.0.
 +
 +Please, note that memory hot unplug is not supported yet. This means
 +that you're able to add memory, but you're not able to remove it.
 +Also, proper proper guest support is required for memory hotplug
 +to work.
 +
 +Basic RAM hotplug
 +-
 +
 +In order to be able to hotplug memory, QEMU has to be told how many
 +hotpluggable memory slots to create and what is the maximum amount of
 +memory the guest can grow. This is done at startup time by means of
 +the -m command-line option, which has the following format:
 +
 + -m [size=]megs[,slots=n,maxmem=size]
 +
 +Where,
 +
 + - megs is the startup RAM. It is the RAM the guest will boot with
 + - slots is the number of hotpluggable memory slots

Limit is 256 IIRC.

 + - maxmem is the maximum RAM size the guest can grow

And this has to be aligned properly (see reply to 1/2).

 +
 +For example, the following command-line:
 +
 + qemu [...] 1G,slots=3,maxmem=4G
 +
 +Creates a guest with 1GB of memory and three hotpluggable memory slots.
 +The hotpluggable memory slots are empty when the guest is booted, so all
 +memory the guest will see after boot is 1GB. The maximum memory the
 +guest can reach is 4GB. This means that three additional gigas can be
 +hotplugged by using any combination of the available memory slots.
 +
 +Two monitor commands are used to hotplug memory:
 +
 + - object_add: creates a memory backend object
 + - device_add: creates the front-end pc-dimm device and inserts it
 + into an empty slot
 +
 +For example, the following commands add another 1GB to the guest
 +discussed earlier:
 +
 +  (qemu) object_add memory-backend-ram,id=mem1,size=1G

The size here is also checked for alignment AFAIK. Also a sane minimum
for linux guests is 128MiB.

 +  (qemu) device_add pc-dimm,id=dimm1,memdev=mem1
 +
 +Using the file backend
 +--
 +
 +Besides basic RAM hotplug, QEMU also supports using files as a memory
 +backend. This is useful for using hugetlbfs in Linux, which provides
 +access to bigger page sizes.
 +
 +For example, assuming that the host has 1GB hugepages available in
 +the /mnt/hugepages-1GB directory, a 1GB hugepage could be hotplugged
 +into the guest from the previous section with the following commnands:
 +
 +  (qemu) object_add 
 memory-backend-file,id=mem1,size=1G,mem-path=/mnt/hugepages-1GB
 +  (qemu) device_add pc-dimm,id=dimm1,memdev=mem1
 +
 +It's also possible to start a guest with memory plugged into the
 +hotpluggable memory slots. This might seem counterintuitive at first,
 +but this allows for a lot of flexibility when using the file backend.
 +
 +In the following command-line example, a 8GB guest is created where 6GB
 +comes from regular RAM, 1GB is a 1GB hugepage page and 256MB is from
 +2MB pages. Also, the guest has additional memory slots to hotplug more
 +2GB if needed:
 +
 + qemu [...] -m 6GB,slots=4,maxmem=10G \
 +   -object memory-backend-file,id=mem1,size=1G,mem-path=/mnt/hugepages-1G \
 +   -device pc-dimm,id=dimm1,memdev=mem1 \
 +   -object memory-backend-file,id=mem2,size=256M,mem-path=/mnt/hugepages-2MB 
 \
 +   -device pc-dimm,id=dimm2,memdev=mem2

Otherwise a very helpful text :). Shame that it wasn't available sooner
I had to figure out the details myself.

Peter


signature.asc
Description: Digital signature

[Qemu-devel] [PATCHv3 0/2] pc: Fix startup with memory hotplug enabled

[Peter Krempa]

Tweak error messages to make sense and add check to verify that maxmem_size is
properly aligned right away rather than just crashing afterwards.

Peter Krempa (2):
  vl.c: Fix error messages when parsing maxmem parameters
  pc: memory: Validate alignment of maxram_size to page size

 hw/i386/pc.c |  7 +++
 vl.c | 34 --
 2 files changed, 23 insertions(+), 18 deletions(-)

-- 
2.2.2

[Qemu-devel] [PATCHv3 1/2] vl.c: Fix error messages when parsing maxmem parameters

[Peter Krempa]

Produce more human readable error messages and fix few spelling
mistakes.

Also remove a redundant check for the max memory size.

Signed-off-by: Peter Krempa pkre...@redhat.com
---

Notes:
Version 3:
- forgot a word in one of the error messages

Version 2:
- fixed spacing in error message
- changed control flow to allow maxmem == ram_size in case slots == 0

 vl.c | 34 --
 1 file changed, 16 insertions(+), 18 deletions(-)

diff --git a/vl.c b/vl.c
index 983259b..b162465 100644
--- a/vl.c
+++ b/vl.c
@@ -2694,29 +2694,27 @@ static void set_memory_options(uint64_t *ram_slots, 
ram_addr_t *maxram_size)
 uint64_t slots;

 sz = qemu_opt_get_size(opts, maxmem, 0);
+slots = qemu_opt_get_number(opts, slots, 0);
 if (sz  ram_size) {
-error_report(invalid -m option value: maxmem 
-(0x% PRIx64 ) = initial memory (0x
-RAM_ADDR_FMT ), sz, ram_size);
+error_report(invalid value of -m option maxmem: 
+ maximum memory size (0x% PRIx64 ) must be at least 

+ the initial memory size (0x RAM_ADDR_FMT ),
+ sz, ram_size);
 exit(EXIT_FAILURE);
-}
-
-slots = qemu_opt_get_number(opts, slots, 0);
-if ((sz  ram_size)  !slots) {
-error_report(invalid -m option value: maxmem 
-(0x% PRIx64 ) more than initial memory (0x
-RAM_ADDR_FMT ) but no hotplug slots where 
-specified, sz, ram_size);
+} else if (sz  ram_size) {
+if (!slots) {
+error_report(invalid value of -m option: maxmem was 
+ specified, but no hotplug slots were specified);
+exit(EXIT_FAILURE);
+}
+} else if (slots) {
+error_report(invalid value of -m option maxmem: 
+ memory slots were specified but maximum memory size 
+ (0x% PRIx64 ) is equal to the initial memory size 
+ (0x RAM_ADDR_FMT ), sz, ram_size);
 exit(EXIT_FAILURE);
 }

-if ((sz = ram_size)  slots) {
-error_report(invalid -m option value:  %
-PRIu64  hotplug slots where specified but 
-maxmem (0x% PRIx64 ) = initial memory (0x
-RAM_ADDR_FMT ), slots, sz, ram_size);
-exit(EXIT_FAILURE);
-}
 *maxram_size = sz;
 *ram_slots = slots;
 } else if ((!maxmem_str  slots_str) ||
-- 
2.2.2

Re: [Qemu-devel] [PATCH v3 4/7] monitor: use cc-get_arch_id as the cpu index

[Peter Krempa]

On Thu, Jan 29, 2015 at 15:12:49 +0100, Igor Mammedov wrote:
 On Wed, 14 Jan 2015 15:27:27 +0800
 Zhu Guihua zhugh.f...@cn.fujitsu.com wrote:
 
  From: Gu Zheng guz.f...@cn.fujitsu.com
  
  Use cc-get_arch_id as the cpu index to avoid the cpu index duplicated
  issue in the QMP/HMP command output.
  
  Signed-off-by: Gu Zheng guz.f...@cn.fujitsu.com
  Signed-off-by: Zhu Guihua zhugh.f...@cn.fujitsu.com
  ---
   cpus.c| 4 +++-
   monitor.c | 4 +++-
   2 files changed, 6 insertions(+), 2 deletions(-)
  
  diff --git a/cpus.c b/cpus.c
  index 2edb5cd..d5e35c0 100644
  --- a/cpus.c
  +++ b/cpus.c
  @@ -1420,6 +1420,7 @@ CpuInfoList *qmp_query_cpus(Error **errp)
   
   CPU_FOREACH(cpu) {
   CpuInfoList *info;
  +CPUClass *cc;
   #if defined(TARGET_I386)
   X86CPU *x86_cpu = X86_CPU(cpu);
   CPUX86State *env = x86_cpu-env;
  @@ -1437,11 +1438,12 @@ CpuInfoList *qmp_query_cpus(Error **errp)
   CPUTriCoreState *env = tricore_cpu-env;
   #endif
   
  +cc = CPU_GET_CLASS(cpu);
   cpu_synchronize_state(cpu);
   
   info = g_malloc0(sizeof(*info));
   info-value = g_malloc0(sizeof(*info-value));
  -info-value-CPU = cpu-cpu_index;
  +info-value-CPU = cc-get_arch_id(cpu);
 I'm not sure what impact sparse ID numbering would be on libvirt
 CCing libvirt folks so they could check it.

Libvirt is currently using only the thread_id field of the data
returned by the query-cpus QMP command.

One place where we currently treat the cpu ID space as continuous is in
the CPU hotplug and unplug API, but the unplug part is currently
obsolete as it uses only the very old HMP command.

At any rate, changing the CPU field shouldn't change anything that
libvirt relies on.

Peter




signature.asc
Description: Digital signature

[Qemu-devel] [PATCHv3 2/2] pc: memory: Validate alignment of maxram_size to page size

[Peter Krempa]

If the maxram_size is not aligned and dimm devices were added on the
command line qemu would terminate with a rather unhelpful message:

ERROR:hw/mem/pc-dimm.c:150:pc_dimm_get_free_addr: assertion failed:
(QEMU_ALIGN_UP(address_space_size, align) == address_space_size)

In case no dimm device was originally added on the commandline qemu
exits on the assertion failure.

Signed-off-by: Peter Krempa pkre...@redhat.com
Reviewed-by: Eric Blake ebl...@redhat.com
---
 hw/i386/pc.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index c7af6aa..8cf405a 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1246,6 +1246,13 @@ FWCfgState *pc_memory_init(MachineState *machine,
 exit(EXIT_FAILURE);
 }

+if (QEMU_ALIGN_UP(machine-maxram_size,
+  TARGET_PAGE_SIZE) != machine-maxram_size) {
+error_report(maximum memory size must by aligned to multiple of 
+ %d bytes, TARGET_PAGE_SIZE);
+exit(EXIT_FAILURE);
+}
+
 pcms-hotplug_memory_base =
 ROUND_UP(0x1ULL + above_4g_mem_size, 1ULL  30);

-- 
2.2.2

[Qemu-devel] [PATCHv2 0/2] pc: Fix startup with memory hotplug enabled

[Peter Krempa]

Tweak error messages to make sense and add check to verify that maxmem_size is
properly aligned right away rather than just crashing afterwards.

Peter Krempa (2):
  vl.c: Fix error messages when parsing maxmem parameters
  pc: memory: Validate alignment of maxram_size to page size

 hw/i386/pc.c |  7 +++
 vl.c | 34 --
 2 files changed, 23 insertions(+), 18 deletions(-)

-- 
2.2.1

[Qemu-devel] [PATCHv2 1/2] vl.c: Fix error messages when parsing maxmem parameters

[Peter Krempa]

Produce more human readable error messages and fix few spelling
mistakes.

Also remove a redundant check for the max memory size.

Signed-off-by: Peter Krempa pkre...@redhat.com
---

Notes:
Version 2:
- fixed spacing in error message
- changed control flow to allow maxmem == ram_size in case slots == 0

 vl.c | 34 --
 1 file changed, 16 insertions(+), 18 deletions(-)

diff --git a/vl.c b/vl.c
index 983259b..5a012f4 100644
--- a/vl.c
+++ b/vl.c
@@ -2694,29 +2694,27 @@ static void set_memory_options(uint64_t *ram_slots, 
ram_addr_t *maxram_size)
 uint64_t slots;

 sz = qemu_opt_get_size(opts, maxmem, 0);
+slots = qemu_opt_get_number(opts, slots, 0);
 if (sz  ram_size) {
-error_report(invalid -m option value: maxmem 
-(0x% PRIx64 ) = initial memory (0x
-RAM_ADDR_FMT ), sz, ram_size);
+error_report(invalid value of -m option maxmem: 
+ maximum memory size (0x% PRIx64 ) must at least 
+ the initial memory size (0x RAM_ADDR_FMT ),
+ sz, ram_size);
 exit(EXIT_FAILURE);
-}
-
-slots = qemu_opt_get_number(opts, slots, 0);
-if ((sz  ram_size)  !slots) {
-error_report(invalid -m option value: maxmem 
-(0x% PRIx64 ) more than initial memory (0x
-RAM_ADDR_FMT ) but no hotplug slots where 
-specified, sz, ram_size);
+} else if (sz  ram_size) {
+if (!slots) {
+error_report(invalid value of -m option: maxmem was 
+ specified, but no hotplug slots were specified);
+exit(EXIT_FAILURE);
+}
+} else if (slots) {
+error_report(invalid value of -m option maxmem: 
+ memory slots were specified but maximum memory size 
+ (0x% PRIx64 ) is equal to the initial memory size 
+ (0x RAM_ADDR_FMT ), sz, ram_size);
 exit(EXIT_FAILURE);
 }

-if ((sz = ram_size)  slots) {
-error_report(invalid -m option value:  %
-PRIu64  hotplug slots where specified but 
-maxmem (0x% PRIx64 ) = initial memory (0x
-RAM_ADDR_FMT ), slots, sz, ram_size);
-exit(EXIT_FAILURE);
-}
 *maxram_size = sz;
 *ram_slots = slots;
 } else if ((!maxmem_str  slots_str) ||
-- 
2.2.1

[Qemu-devel] [PATCHv2 2/2] pc: memory: Validate alignment of maxram_size to page size

[Peter Krempa]

If the maxram_size is not aligned and dimm devices were added on the
command line qemu would terminate with a rather unhelpful message:

ERROR:hw/mem/pc-dimm.c:150:pc_dimm_get_free_addr: assertion failed:
(QEMU_ALIGN_UP(address_space_size, align) == address_space_size)

In case no dimm device was originally added on the commandline qemu
exits on the assertion failure.

Signed-off-by: Peter Krempa pkre...@redhat.com
Reviewed-by: Eric Blake ebl...@redhat.com
---
 hw/i386/pc.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index c7af6aa..8cf405a 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1246,6 +1246,13 @@ FWCfgState *pc_memory_init(MachineState *machine,
 exit(EXIT_FAILURE);
 }

+if (QEMU_ALIGN_UP(machine-maxram_size,
+  TARGET_PAGE_SIZE) != machine-maxram_size) {
+error_report(maximum memory size must by aligned to multiple of 
+ %d bytes, TARGET_PAGE_SIZE);
+exit(EXIT_FAILURE);
+}
+
 pcms-hotplug_memory_base =
 ROUND_UP(0x1ULL + above_4g_mem_size, 1ULL  30);

-- 
2.2.1

Re: [Qemu-devel] [PATCHv2 1/2] vl.c: Fix error messages when parsing maxmem parameters

[Peter Krempa]

On Wed, Jan 28, 2015 at 13:29:41 +0100, Igor Mammedov wrote:
 On Wed, 28 Jan 2015 09:35:03 +0100
 Peter Krempa pkre...@redhat.com wrote:
 
  Produce more human readable error messages and fix few spelling
  mistakes.
  
  Also remove a redundant check for the max memory size.
  
  Signed-off-by: Peter Krempa pkre...@redhat.com
  ---
  
  Notes:
  Version 2:
  - fixed spacing in error message
  - changed control flow to allow maxmem == ram_size in case slots == 0
  
   vl.c | 34 --
   1 file changed, 16 insertions(+), 18 deletions(-)
  
  diff --git a/vl.c b/vl.c
  index 983259b..5a012f4 100644
  --- a/vl.c
  +++ b/vl.c
  @@ -2694,29 +2694,27 @@ static void set_memory_options(uint64_t *ram_slots, 
  ram_addr_t *maxram_size)
   uint64_t slots;
  
   sz = qemu_opt_get_size(opts, maxmem, 0);
  +slots = qemu_opt_get_number(opts, slots, 0);
   if (sz  ram_size) {
  -error_report(invalid -m option value: maxmem 
  -(0x% PRIx64 ) = initial memory (0x
  -RAM_ADDR_FMT ), sz, ram_size);
  +error_report(invalid value of -m option maxmem: 
  + maximum memory size (0x% PRIx64 ) must at 
  least 
 typo??
 must be at least
 

Hmm, right. Should I respin the series to fix it?

Peter


signature.asc
Description: Digital signature

Re: [Qemu-devel] [PATCH 1/2] vl.c: Fix error messages when parsing maxmem parameters

[Peter Krempa]

On Tue, Jan 27, 2015 at 11:15:41 -0700, Eric Blake wrote:
 On 01/26/2015 08:31 AM, Peter Krempa wrote:
  Produce more human readable error messages and fix few spelling
  mistakes.
  
  Also remove a redundant check for the max memory size.
  
  Signed-off-by: Peter Krempa pkre...@redhat.com
  ---
   vl.c | 22 +++---
   1 file changed, 7 insertions(+), 15 deletions(-)
  
  diff --git a/vl.c b/vl.c
  index 983259b..cdc920c 100644
  --- a/vl.c
  +++ b/vl.c
  @@ -2694,29 +2694,21 @@ static void set_memory_options(uint64_t *ram_slots, 
  ram_addr_t *maxram_size)
   uint64_t slots;
  
   sz = qemu_opt_get_size(opts, maxmem, 0);
  -if (sz  ram_size) {
  -error_report(invalid -m option value: maxmem 
  -(0x% PRIx64 ) = initial memory (0x
  -RAM_ADDR_FMT ), sz, ram_size);
  +if (sz = ram_size) {
 
 Why are we changing from '' to '='?  I think the error was in the
 message, not in the code, and that setting max == size should be
 allowed. [1]
 
  +error_report(invalid value of -m option maxmem: 
  + maximum memory size (0x% PRIx64 ) must be 
  greater 
  + than initial memory size (0x  RAM_ADDR_FMT ),
 
 Why two spaces?  If I'm correct that we want '' in the condition, then
 the wording 'must be at least the initial memory size' would be better.
 
  + sz, ram_size);
   exit(EXIT_FAILURE);
   }
  
   slots = qemu_opt_get_number(opts, slots, 0);
   if ((sz  ram_size)  !slots) {
  -error_report(invalid -m option value: maxmem 
  -(0x% PRIx64 ) more than initial memory (0x
  -RAM_ADDR_FMT ) but no hotplug slots where 
  -specified, sz, ram_size);
  +error_report(invalid value of -m option: maxmem was 
  specified, 
  + but no hotplug slots were specified);
   exit(EXIT_FAILURE);
   }
  
  -if ((sz = ram_size)  slots) {
  -error_report(invalid -m option value:  %
  -PRIu64  hotplug slots where specified but 
  -maxmem (0x% PRIx64 ) = initial memory (0x
  -RAM_ADDR_FMT ), slots, sz, ram_size);
  -exit(EXIT_FAILURE);
  -}
 
 Okay, I see.  This is dead if condition [1] is changed to '=', but
 still reachable (sz == ram_size) if condition [1] is left at ''.  Maybe
 better logic would be:
 
 if (sz  ram_size) {
 max cannot be less than memory
 }
 if (sz  ram_size) {
 if (!slots) {
 max cannot be larger than size without hotplug slots
 }
 } else if (slots) {
 max must be larger than size to support hotplug slots
 }
 
 to allow max==ram_size when slots is not present.

This will only allow to specify ram_size equal to max if the slot
parameter _is_ present and set to 0. If the string is not present a
different branch is taken and the config will be rejected, as the two
parameters have to be specified together.

I'll post an updated version though, the control flow in your suggestion
seems more clear.

Thanks.

Peter


signature.asc
Description: Digital signature

[Qemu-devel] [PATCH 1/2] vl.c: Fix error messages when parsing maxmem parameters

[Peter Krempa]

Produce more human readable error messages and fix few spelling
mistakes.

Also remove a redundant check for the max memory size.

Signed-off-by: Peter Krempa pkre...@redhat.com
---
 vl.c | 22 +++---
 1 file changed, 7 insertions(+), 15 deletions(-)

diff --git a/vl.c b/vl.c
index 983259b..cdc920c 100644
--- a/vl.c
+++ b/vl.c
@@ -2694,29 +2694,21 @@ static void set_memory_options(uint64_t *ram_slots, 
ram_addr_t *maxram_size)
 uint64_t slots;

 sz = qemu_opt_get_size(opts, maxmem, 0);
-if (sz  ram_size) {
-error_report(invalid -m option value: maxmem 
-(0x% PRIx64 ) = initial memory (0x
-RAM_ADDR_FMT ), sz, ram_size);
+if (sz = ram_size) {
+error_report(invalid value of -m option maxmem: 
+ maximum memory size (0x% PRIx64 ) must be greater 
+ than initial memory size (0x  RAM_ADDR_FMT ),
+ sz, ram_size);
 exit(EXIT_FAILURE);
 }

 slots = qemu_opt_get_number(opts, slots, 0);
 if ((sz  ram_size)  !slots) {
-error_report(invalid -m option value: maxmem 
-(0x% PRIx64 ) more than initial memory (0x
-RAM_ADDR_FMT ) but no hotplug slots where 
-specified, sz, ram_size);
+error_report(invalid value of -m option: maxmem was specified, 
+ but no hotplug slots were specified);
 exit(EXIT_FAILURE);
 }

-if ((sz = ram_size)  slots) {
-error_report(invalid -m option value:  %
-PRIu64  hotplug slots where specified but 
-maxmem (0x% PRIx64 ) = initial memory (0x
-RAM_ADDR_FMT ), slots, sz, ram_size);
-exit(EXIT_FAILURE);
-}
 *maxram_size = sz;
 *ram_slots = slots;
 } else if ((!maxmem_str  slots_str) ||
-- 
2.2.1

[Qemu-devel] [PATCH 2/2] pc: memory: Validate alignment of maxram_size to page size

[Peter Krempa]

If the maxram_size is not aligned and dimm devices were added on the
command line qemu would terminate with a rather unhelpful message:

ERROR:hw/mem/pc-dimm.c:150:pc_dimm_get_free_addr: assertion failed:
(QEMU_ALIGN_UP(address_space_size, align) == address_space_size)

In case no dimm device was originally added on the commandline qemu
exits on the assertion failure.

Signed-off-by: Peter Krempa pkre...@redhat.com
---
 hw/i386/pc.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index e07f1fa..157eefe 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1246,6 +1246,13 @@ FWCfgState *pc_memory_init(MachineState *machine,
 exit(EXIT_FAILURE);
 }

+if (QEMU_ALIGN_UP(machine-maxram_size,
+  TARGET_PAGE_SIZE) != machine-maxram_size) {
+error_report(maximum memory size must by aligned to multiple of 
+ %d bytes, TARGET_PAGE_SIZE);
+exit(EXIT_FAILURE);
+}
+
 pcms-hotplug_memory_base =
 ROUND_UP(0x1ULL + above_4g_mem_size, 1ULL  30);

-- 
2.2.1

[Qemu-devel] [PATCH 0/2] pc: Fix startup with memory hotplug enabled

[Peter Krempa]

Tweak error messages to make sense and add check to verify that maxmem_size is
properly aligned right away rather than just crashing afterwards.

Peter Krempa (2):
  vl.c: Fix error messages when parsing maxmem parameters
  pc: memory: Validate alignment of maxram_size to page size

 hw/i386/pc.c |  7 +++
 vl.c | 22 +++---
 2 files changed, 14 insertions(+), 15 deletions(-)

-- 
2.2.1

Re: [Qemu-devel] [PULL 12/17] gluster: Correctly propagate errors when volume isn't accessible

[Peter Krempa]

On 05/14/14 14:36, Stefan Hajnoczi wrote:
 On Mon, May 12, 2014 at 10:07:07AM -0600, Eric Blake wrote:
 On 05/09/2014 01:03 PM, Stefan Hajnoczi wrote:
 From: Peter Krempa pkre...@redhat.com

 The docs for glfs_init suggest that the function sets errno on every
 failure. In fact it doesn't. As other functions such as
 qemu_gluster_open() in the gluster block code report their errors based
 on this fact we need to make sure that errno is set on each failure.

 This fixes a crash of qemu-img/qemu when a gluster brick isn't
 accessible from given host while the server serving the volume
 description is.

 Thread 1 (Thread 0x77fba740 (LWP 203880)):
  #0  0x777673f8 in glfs_lseek () from /usr/lib64/libgfapi.so.0
  #1  0x55574a68 in qemu_gluster_getlength ()
  #2  0x55565742 in refresh_total_sectors ()
  #3  0x5556914f in bdrv_open_common ()
  #4  0x5556e8e8 in bdrv_open ()
  #5  0x5556f02f in bdrv_open_image ()
  #6  0x5556e5f6 in bdrv_open ()
  #7  0x555c5775 in bdrv_new_open ()
  #8  0x555c5b91 in img_info ()
  #9  0x762c9c05 in __libc_start_main () from /lib64/libc.so.6
  #10 0x555648ad in _start ()

 Signed-off-by: Stefan Hajnoczi stefa...@redhat.com
 ---

 Isn't this missing S-o-b from Peter?
 
 Good catch.  Peter, can you send your Signed-off-by: please?
 
 Stefan
 

As this patch was already pulled to master as:

commit 4557117d9eed8cadc360aec23b42fc39a7011864
Author: Peter Krempa pkre...@redhat.com
Date:   Fri May 9 12:08:10 2014 +0200

gluster: Correctly propagate errors when volume isn't accessible

The docs for glfs_init suggest that the function sets errno on every
failure. In fact it doesn't. As other functions such as
qemu_gluster_open() in the gluster block code report their errors based
on this fact we need to make sure that errno is set on each failure.

This fixes a crash of qemu-img/qemu when a gluster brick isn't
accessible from given host while the server serving the volume
description is.

Thread 1 (Thread 0x77fba740 (LWP 203880)):
 #0  0x777673f8 in glfs_lseek () from /usr/lib64/libgfapi.so.0
 #1  0x55574a68 in qemu_gluster_getlength ()
 #2  0x55565742 in refresh_total_sectors ()
 #3  0x5556914f in bdrv_open_common ()
 #4  0x5556e8e8 in bdrv_open ()
 #5  0x5556f02f in bdrv_open_image ()
 #6  0x5556e5f6 in bdrv_open ()
 #7  0x555c5775 in bdrv_new_open ()
 #8  0x555c5b91 in img_info ()
 #9  0x762c9c05 in __libc_start_main () from /lib64/libc.so.6
 #10 0x555648ad in _start ()


Signed-off-by: Stefan Hajnoczi stefa...@redhat.com

I'm explicitly expressing my signoff here:

Signed-off-by: Peter Krempa pkre...@redhat.com

Sorry for the hassle.

Peter




signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH] gluster: Correctly propagate errors when volume isn't accessible

[Peter Krempa]

The docs for glfs_init suggest that the function sets errno on every
failure. In fact it doesn't. As other functions such as
qemu_gluster_open() in the gluster block code report their errors based
on this fact we need to make sure that errno is set on each failure.

This fixes a crash of qemu-img/qemu when a gluster brick isn't
accessible from given host while the server serving the volume
description is.

Thread 1 (Thread 0x77fba740 (LWP 203880)):
 #0  0x777673f8 in glfs_lseek () from /usr/lib64/libgfapi.so.0
 #1  0x55574a68 in qemu_gluster_getlength ()
 #2  0x55565742 in refresh_total_sectors ()
 #3  0x5556914f in bdrv_open_common ()
 #4  0x5556e8e8 in bdrv_open ()
 #5  0x5556f02f in bdrv_open_image ()
 #6  0x5556e5f6 in bdrv_open ()
 #7  0x555c5775 in bdrv_new_open ()
 #8  0x555c5b91 in img_info ()
 #9  0x762c9c05 in __libc_start_main () from /lib64/libc.so.6
 #10 0x555648ad in _start ()
---

Notes:
I'm also going to report a bug in libgfapi too.

 block/gluster.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/block/gluster.c b/block/gluster.c
index 8836085..d0726ec 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -207,6 +207,11 @@ static struct glfs *qemu_gluster_init(GlusterConf *gconf, 
const char *filename,
  volume=%s image=%s transport=%s, gconf-server,
  gconf-port, gconf-volname, gconf-image,
  gconf-transport);
+
+/* glfs_init sometimes doesn't set errno although docs suggest that */
+if (errno == 0)
+errno = EINVAL;
+
 goto out;
 }
 return glfs;
@@ -482,7 +487,7 @@ static int qemu_gluster_create(const char *filename,

 glfs = qemu_gluster_init(gconf, filename, errp);
 if (!glfs) {
-ret = -EINVAL;
+ret = -errno;
 goto out;
 }

-- 
1.9.2

Re: [Qemu-devel] [PATCH] gluster: Correctly propagate errors when volume isn't accessible

[Peter Krempa]

On 05/09/14 13:39, Stefan Hajnoczi wrote:
 On Fri, May 09, 2014 at 12:08:10PM +0200, Peter Krempa wrote:

...

 
 Please use scripts/checkpatch.pl to check coding style in the future.  I
 added {} around the if statement body.  QEMU always uses curlies even
 for 1-statement bodies.

Ah, right, sorry about that. I'm used to libvirt's coding style which
allows it. I'll use the patch checker next time.

 
 Thanks, applied to my block tree:
 https://github.com/stefanha/qemu/commits/block

Thanks.

 
 Stefan
 

Peter



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 2/2] chardev: fix info chardev output

[Peter Krempa]

On 05/31/13 14:36, Eric Blake wrote:
 On 05/28/2013 12:34 AM, Gerd Hoffmann wrote:
 Fill unset CharDriverState-filename with the backend name, so
 'info chardev' will return at least the chardev type.  Don't
 touch it in case the chardev init function filled it already,
 like the socket+pty chardevs do for example.

 Signed-off-by: Gerd Hoffmann kra...@redhat.com
 ---
   qemu-char.c |3 +++
   1 file changed, 3 insertions(+)

 diff --git a/qemu-char.c b/qemu-char.c
 index f825294..d04b429 100644
 --- a/qemu-char.c
 +++ b/qemu-char.c
 @@ -3801,6 +3801,9 @@ ChardevReturn *qmp_chardev_add(const char *id, 
 ChardevBackend *backend,
   chr-label = g_strdup(id);
   chr-avail_connections =
   (backend-kind == CHARDEV_BACKEND_KIND_MUX) ? MAX_MUX : 1;
 +if (!chr-filename) {
 +chr-filename = 
 g_strdup(ChardevBackendKind_lookup[backend-kind]);
 +}
   QTAILQ_INSERT_TAIL(chardevs, chr, next);
   return ret;
   } else {

 
 Peter was telling me on IRC that this patch is still broken with regards
 to libvirt; I've cc'd him to provide more details...
 

Without this patch the returned message for query-chardev is:

{
return: [
{
filename: pty:/dev/pts/8,
label: charserial0
},
{
filename: unix:/var/lib/libvirt/qemu/qemu-git.monitor,server,
label: charmonitor
}
],
id: libvirt-2
}


this patch changes it to:

{
return: [
{
filename: pty,
label: charserial0
},
{
filename: unix:/var/lib/libvirt/qemu/qemu-git.monitor,server,
label: charmonitor
}
],
id: libvirt-2
}

It's apparent that some code being executed after the code in this patch
fills the actual pty path that was allocated. With it pre-allocated the
code ignores it. Libvirt is using the output to gather names of the pty
so that they can be used to connect to the console.

Peter

Re: [Qemu-devel] [PATCH 2/2] chardev: fix info chardev output

[Peter Krempa]

On 05/31/13 15:21, Gerd Hoffmann wrote:

   Hi,



Hi,



Please double-check.  Current master
(87d23f78aa79b72da022afda358bbc8a8509ca70 to be exact) works just fine
for me.  libvirt works, including a serial line redirected to pty, and
'info chardev' looks sane too.


sorry for the fuzz. :/ Upstream is really working, One of the tested 
binaries was actually predating the pull of your fix and I forgot to 
verify that as It was built yesterday.



Again sorry for the noise, it's working now.

Peter



cheers,
   Gerd

Re: [Qemu-devel] [libvirt] [PATCH 2/4] QMP: add cpu-add command

[Peter Krempa]

On 04/30/13 15:46, Eduardo Habkost wrote:

(CCing libvir-list)

On Tue, Apr 30, 2013 at 08:34:01AM +0200, Igor Mammedov wrote:

Adds cpu-add id=xxx QMP command.

cpu-add's id argument is a CPU number in a range [0..max-cpus)

Example QMP command:
  - { execute: cpu-add, arguments: { id: 2 } }
  - { return: {} }

Signed-off-by: Igor Mammedov imamm...@redhat.com
Acked-by: Luiz Capitulino lcapitul...@redhat.com
Reviewed-by: Eric Blake ebl...@redhat.com


The only way to find out if CPU hotplug is really available on a given
machine-type is by actually trying to run cpu-add, right? Is this
sufficient for libvirt requirements?





As long as the command fails when it's not supported it's okay.

(cpu_set HMP command does not fail when offlining a cpu even if it isn't 
supported and that's real pain to use)


Peter

Re: [Qemu-devel] [PATCH v2] New cpu-max field in query-machines QMP command output

[Peter Krempa]

On 04/09/13 15:06, Luiz Capitulino wrote:

On Mon, 08 Apr 2013 11:14:32 -0600
Eric Blake ebl...@redhat.com wrote:


On 04/08/2013 10:41 AM, Michal Novotny wrote:

Alter the query-machines QMP command to output information about
maximum number of CPUs for each machine type with default value
set to 1 in case the number of max_cpus is not set.

Signed-off-by: Michal Novotny minov...@redhat.com
---
  qapi-schema.json | 4 +++-
  vl.c | 1 +
  2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index db542f6..689ca8d 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2861,11 +2861,13 @@
  #
  # @default: #optional whether the machine is default
  #
+# @cpu-max: maximum number of CPUs supported by the machine type


Typically, when adding a field in a later version than the original
introduction of the datatype, we add '(since 1.5)' to make it obvious
when to expect the field.  However, as nothing (currently) enforces this
rule, I think such an addition is minor enough that it wouldn't
invalidate the use of my:


Oh, it turns out I was making some confusion with this patch and
didn't realize it was extending the query-machines command.

I don't mean there's anything wrong with it, but my question is: doesn't
this patch invalidates query-cpu-max?


Unfortunately, for libvirt query-cpu-max isn't really usable as it needs 
us to start qemu with the correct machine type. This would increase 
overhead as we would need to start the qemu process with a safe number 
of cpus, use query-cpu-max and then restart the process.


The information added in the query-machines output can on the other hand 
be cached (we are already doing this for the machine types) and used 
later from the cache without increasing overhead.


So yes, I think it invalidates query-cpu-max and it can be removed in 
case it wasn't released.


Peter



PS: I can add the '(since 1.5)' line myself.



Reviewed-by: Eric Blake ebl...@redhat.com

Re: [Qemu-devel] virtio-rng and fd passing

[Peter Krempa]

On 03/01/13 21:04, Anthony Liguori wrote:

Eric Blake ebl...@redhat.com writes:


Stefan Berger and I discovered on IRC that virtio-rng is unable to
support fd passing.  We attempted:

qemu-system-x86_64 ... -add-fd set=4,fd=34,opaque=RDONLY:/dev/urandom
-object rng-random,id=rng0,filename=/dev/fdset/4 -device
virtio-rng-pci,rng=rng0,bus=pci.0,addr=0x6


Why are you using th rng-random backend instead of the rng-egd backend?


There are two issues with using the egd backend for unprepared devices:

1) The backend spits out \x20\x40 commands(egd blocking entropy 
request for 0x40 bytes) so it really has to be used with some kind of 
EGD server implementation otherwise it might feed your /dev/random with 
predictable bytes if used directly.


2) performance of the egd backend is terrible as I've reported here
https://bugzilla.redhat.com/show_bug.cgi?id=915381 (yes I'm aware that I 
probably should have filed a upstream bug too, but I was hoping Amit 
would do it in the process)


On my machine I managed to do 0.2KiB/s with the egd backend both with 
using constant data as a high performance source, but also with a true 
random number generator (in the Raspberry pi SoC, sources 107KiB/s of 
entropy). The rng-random backend performs a bit better averaging 1.2MiB/s.


Peter