Re: [libvirt] [PATCH] qemu-capabilities: Fix query-cpu-model-expansion on s390 with older kernel

2017-01-13 Thread Eduardo Habkost 
On Fri, Jan 13, 2017 at 03:11:54PM +0100, Jiri Denemark wrote:
> On Fri, Jan 13, 2017 at 11:04:21 +0100, Jiri Denemark wrote:
> > On Thu, Jan 12, 2017 at 11:18:11 -0500, Collin L. Walling wrote:
> > > When running on s390 with a kernel that does not support cpu model 
> > > checking and
> > > with a Qemu new enough to support query-cpu-model-expansion, the 
> > > gathering of qemu
> > > capabilities will fail. Qemu responds to the query-cpu-model-expansion qmp
> > > command with an error because the needed kernel ioct does not exist. When 
> > > this
> > > happens a guest cannot even be defined due to missing qemu capabilities 
> > > data.
> > > 
> > > This patch fixes the problem by silently ignoring generic errors stemming 
> > > from
> > > calls to query-cpu-model-expansion.
> > > 
> > > Reported-by: Farhan Ali 
> > > Signed-off-by: Collin L. Walling 
> > > Signed-off-by: Jason J. Herne 
> > > ---
> > >  src/qemu/qemu_monitor_json.c | 9 +
> > >  1 file changed, 9 insertions(+)
> > > 
> > > diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> > > index e767437..1662749 100644
> > > --- a/src/qemu/qemu_monitor_json.c
> > > +++ b/src/qemu/qemu_monitor_json.c
> > > @@ -5041,6 +5041,15 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr 
> > > mon,
> > >  if (qemuMonitorJSONCommand(mon, cmd, ) < 0)
> > >  goto cleanup;
> > >  
> > > +/* Some QEMU architectures have the query-cpu-model-expansion
> > > + * command, but return 'GenericError' instead of simply omitting
> > > + * the command entirely.
> > > + */
> > 
> > Hmm, this comment says something a bit different than the commit
> > message. I hope the issue described by this comment was fixed in QEMU
> > within the same release the query-cpu-model-expansion command was
> > introduced. But we need to fix this nevertheless to address what the
> > commit message describes.
> > 
> > > +if (qemuMonitorJSONHasError(reply, "GenericError")) {
> > > +ret = 0;
> > > +goto cleanup;
> > > +}
> > > +
> > >  if (qemuMonitorJSONCheckError(cmd, reply) < 0)
> > >  goto cleanup;
> > 
> > However, we need to do a little bit more than just ignoring this error.
> > I'll send a v2 soon.
> 
> No I won't send the v2. I was wrong. I thought we should clear the
> QEMU_CAPS_QUERY_CPU_MODEL_EXPANSION capability when we detect the
> command is not actually working, but we can't do that since the
> existence of this command serves as an indicator of other CPU
> configuration capabilities which cannot be probed directly. Thus just
> ignoring non-working query-cpu-model-expansion command is the right
> thing to do.
> 
> But I suggest to change the comment to something like the following:
> 
> /* Even though query-cpu-model-expansion is advertised by
>  * query-commands it may just return GenericError if it is not
>  * implemented for the requested guest architecture or it is not
>  * supported in the host environment.
>  */

If the command is not implemented for the architecture, it should
be already omitted from query-commands. However, if the host
environment doesn't support CPU models, I expect the commands to
fail only for the "host" CPU model, but to be still possible to
expand the other CPU models.

Expansion of static CPU models, specifically, should be always
possible and should never be affected by the host environment.

-- 
Eduardo

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu-capabilities: Fix query-cpu-model-expansion on s390 with older kernel

2017-01-13 Thread Eduardo Habkost 
On Fri, Jan 13, 2017 at 09:06:52AM -0500, Jason J. Herne wrote:
> On 01/13/2017 05:04 AM, Jiri Denemark wrote:
> > On Thu, Jan 12, 2017 at 11:18:11 -0500, Collin L. Walling wrote:
> > > When running on s390 with a kernel that does not support cpu model 
> > > checking and
> > > with a Qemu new enough to support query-cpu-model-expansion, the 
> > > gathering of qemu
> > > capabilities will fail. Qemu responds to the query-cpu-model-expansion qmp
> > > command with an error because the needed kernel ioct does not exist. When 
> > > this
> > > happens a guest cannot even be defined due to missing qemu capabilities 
> > > data.
> > > 
> > > This patch fixes the problem by silently ignoring generic errors stemming 
> > > from
> > > calls to query-cpu-model-expansion.
> > > 
> > > Reported-by: Farhan Ali 
> > > Signed-off-by: Collin L. Walling 
> > > Signed-off-by: Jason J. Herne 
> > > ---
> > >  src/qemu/qemu_monitor_json.c | 9 +
> > >  1 file changed, 9 insertions(+)
> > > 
> > > diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> > > index e767437..1662749 100644
> > > --- a/src/qemu/qemu_monitor_json.c
> > > +++ b/src/qemu/qemu_monitor_json.c
> > > @@ -5041,6 +5041,15 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr 
> > > mon,
> > >  if (qemuMonitorJSONCommand(mon, cmd, ) < 0)
> > >  goto cleanup;
> > > 
> > > +/* Some QEMU architectures have the query-cpu-model-expansion
> > > + * command, but return 'GenericError' instead of simply omitting
> > > + * the command entirely.
> > > + */
> > 
> > Hmm, this comment says something a bit different than the commit
> > message. I hope the issue described by this comment was fixed in QEMU
> > within the same release the query-cpu-model-expansion command was
> > introduced. But we need to fix this nevertheless to address what the
> > commit message describes.
> > 
> 
> The issue still exists in Qemu. I can work up a patch to handle it. My first
> idea is to simply test that the ioctl exists and functions at Qemu
> initialization and deregister the query-cpu-model-expansion command in the
> event that the ioctl does not exist or fails. Thoughts?

I'm not sure, probably unregistering the command after KVM is
initialized is too late. Even if today QMP is available only
after the accelerator is already initialized, we might want to
delay accelerator initialization in the future (so the
accelerator could be configured using QMP commands).

Also, I'm not sure when/how exactly the query-cpu-model-expansion
call fails. If the ioctl isn't available, shouldn't
query-cpu-model-expansion fail only when querying "host", but let
the other CPU models to be queried? In this case, unregistering
the command doesn't seem to be the right solution. Probably
omitting "host" (or flagging it as unavailable?) on
query-cpu-model-definitions would be better.

-- 
Eduardo

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu-capabilities: Fix query-cpu-model-expansion on s390 with older kernel

2017-01-13 Thread Jiri Denemark 
On Fri, Jan 13, 2017 at 09:20:40 -0500, Jason J. Herne wrote:
> On 01/13/2017 09:11 AM, Jiri Denemark wrote:
> > But I suggest to change the comment to something like the following:
> >
> > /* Even though query-cpu-model-expansion is advertised by
> >  * query-commands it may just return GenericError if it is not
> >  * implemented for the requested guest architecture or it is not
> >  * supported in the host environment.
> >  */
> >
> > ACK to this patch and please let me know if you agree with the modified
> > comment.
> >
> 
> Jiri, yes, I agree with the comment change. Thanks for taking a look at 
> this.

OK, I pushed this with the modified comment. Thanks.

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu-capabilities: Fix query-cpu-model-expansion on s390 with older kernel

2017-01-13 Thread Jason J. Herne 

On 01/13/2017 09:11 AM, Jiri Denemark wrote:

On Fri, Jan 13, 2017 at 11:04:21 +0100, Jiri Denemark wrote:

On Thu, Jan 12, 2017 at 11:18:11 -0500, Collin L. Walling wrote:

When running on s390 with a kernel that does not support cpu model checking and
with a Qemu new enough to support query-cpu-model-expansion, the gathering of 
qemu
capabilities will fail. Qemu responds to the query-cpu-model-expansion qmp
command with an error because the needed kernel ioct does not exist. When this
happens a guest cannot even be defined due to missing qemu capabilities data.

This patch fixes the problem by silently ignoring generic errors stemming from
calls to query-cpu-model-expansion.

Reported-by: Farhan Ali 
Signed-off-by: Collin L. Walling 
Signed-off-by: Jason J. Herne 
---
 src/qemu/qemu_monitor_json.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index e767437..1662749 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -5041,6 +5041,15 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon,
 if (qemuMonitorJSONCommand(mon, cmd, ) < 0)
 goto cleanup;

+/* Some QEMU architectures have the query-cpu-model-expansion
+ * command, but return 'GenericError' instead of simply omitting
+ * the command entirely.
+ */


Hmm, this comment says something a bit different than the commit
message. I hope the issue described by this comment was fixed in QEMU
within the same release the query-cpu-model-expansion command was
introduced. But we need to fix this nevertheless to address what the
commit message describes.


+if (qemuMonitorJSONHasError(reply, "GenericError")) {
+ret = 0;
+goto cleanup;
+}
+
 if (qemuMonitorJSONCheckError(cmd, reply) < 0)
 goto cleanup;


However, we need to do a little bit more than just ignoring this error.
I'll send a v2 soon.


No I won't send the v2. I was wrong. I thought we should clear the
QEMU_CAPS_QUERY_CPU_MODEL_EXPANSION capability when we detect the
command is not actually working, but we can't do that since the
existence of this command serves as an indicator of other CPU
configuration capabilities which cannot be probed directly. Thus just
ignoring non-working query-cpu-model-expansion command is the right
thing to do.

But I suggest to change the comment to something like the following:

/* Even though query-cpu-model-expansion is advertised by
 * query-commands it may just return GenericError if it is not
 * implemented for the requested guest architecture or it is not
 * supported in the host environment.
 */

ACK to this patch and please let me know if you agree with the modified
comment.



Jiri, yes, I agree with the comment change. Thanks for taking a look at 
this.


--
-- Jason J. Herne (jjhe...@linux.vnet.ibm.com)

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu-capabilities: Fix query-cpu-model-expansion on s390 with older kernel

2017-01-13 Thread Jiri Denemark 
On Fri, Jan 13, 2017 at 11:04:21 +0100, Jiri Denemark wrote:
> On Thu, Jan 12, 2017 at 11:18:11 -0500, Collin L. Walling wrote:
> > When running on s390 with a kernel that does not support cpu model checking 
> > and
> > with a Qemu new enough to support query-cpu-model-expansion, the gathering 
> > of qemu
> > capabilities will fail. Qemu responds to the query-cpu-model-expansion qmp
> > command with an error because the needed kernel ioct does not exist. When 
> > this
> > happens a guest cannot even be defined due to missing qemu capabilities 
> > data.
> > 
> > This patch fixes the problem by silently ignoring generic errors stemming 
> > from
> > calls to query-cpu-model-expansion.
> > 
> > Reported-by: Farhan Ali 
> > Signed-off-by: Collin L. Walling 
> > Signed-off-by: Jason J. Herne 
> > ---
> >  src/qemu/qemu_monitor_json.c | 9 +
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> > index e767437..1662749 100644
> > --- a/src/qemu/qemu_monitor_json.c
> > +++ b/src/qemu/qemu_monitor_json.c
> > @@ -5041,6 +5041,15 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr 
> > mon,
> >  if (qemuMonitorJSONCommand(mon, cmd, ) < 0)
> >  goto cleanup;
> >  
> > +/* Some QEMU architectures have the query-cpu-model-expansion
> > + * command, but return 'GenericError' instead of simply omitting
> > + * the command entirely.
> > + */
> 
> Hmm, this comment says something a bit different than the commit
> message. I hope the issue described by this comment was fixed in QEMU
> within the same release the query-cpu-model-expansion command was
> introduced. But we need to fix this nevertheless to address what the
> commit message describes.
> 
> > +if (qemuMonitorJSONHasError(reply, "GenericError")) {
> > +ret = 0;
> > +goto cleanup;
> > +}
> > +
> >  if (qemuMonitorJSONCheckError(cmd, reply) < 0)
> >  goto cleanup;
> 
> However, we need to do a little bit more than just ignoring this error.
> I'll send a v2 soon.

No I won't send the v2. I was wrong. I thought we should clear the
QEMU_CAPS_QUERY_CPU_MODEL_EXPANSION capability when we detect the
command is not actually working, but we can't do that since the
existence of this command serves as an indicator of other CPU
configuration capabilities which cannot be probed directly. Thus just
ignoring non-working query-cpu-model-expansion command is the right
thing to do.

But I suggest to change the comment to something like the following:

/* Even though query-cpu-model-expansion is advertised by
 * query-commands it may just return GenericError if it is not
 * implemented for the requested guest architecture or it is not
 * supported in the host environment.
 */

ACK to this patch and please let me know if you agree with the modified
comment.

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu-capabilities: Fix query-cpu-model-expansion on s390 with older kernel

2017-01-13 Thread Jason J. Herne 

On 01/13/2017 05:04 AM, Jiri Denemark wrote:

On Thu, Jan 12, 2017 at 11:18:11 -0500, Collin L. Walling wrote:

When running on s390 with a kernel that does not support cpu model checking and
with a Qemu new enough to support query-cpu-model-expansion, the gathering of 
qemu
capabilities will fail. Qemu responds to the query-cpu-model-expansion qmp
command with an error because the needed kernel ioct does not exist. When this
happens a guest cannot even be defined due to missing qemu capabilities data.

This patch fixes the problem by silently ignoring generic errors stemming from
calls to query-cpu-model-expansion.

Reported-by: Farhan Ali 
Signed-off-by: Collin L. Walling 
Signed-off-by: Jason J. Herne 
---
 src/qemu/qemu_monitor_json.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index e767437..1662749 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -5041,6 +5041,15 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon,
 if (qemuMonitorJSONCommand(mon, cmd, ) < 0)
 goto cleanup;

+/* Some QEMU architectures have the query-cpu-model-expansion
+ * command, but return 'GenericError' instead of simply omitting
+ * the command entirely.
+ */


Hmm, this comment says something a bit different than the commit
message. I hope the issue described by this comment was fixed in QEMU
within the same release the query-cpu-model-expansion command was
introduced. But we need to fix this nevertheless to address what the
commit message describes.



The issue still exists in Qemu. I can work up a patch to handle it. My first
idea is to simply test that the ioctl exists and functions at Qemu
initialization and deregister the query-cpu-model-expansion command in the
event that the ioctl does not exist or fails. Thoughts?


However, we need to do a little bit more than just ignoring this error.
I'll send a v2 soon.


--
-- Jason J. Herne (jjhe...@linux.vnet.ibm.com)

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] qemu-capabilities: Fix query-cpu-model-expansion on s390 with older kernel

2017-01-13 Thread Jiri Denemark 
On Thu, Jan 12, 2017 at 11:18:11 -0500, Collin L. Walling wrote:
> When running on s390 with a kernel that does not support cpu model checking 
> and
> with a Qemu new enough to support query-cpu-model-expansion, the gathering of 
> qemu
> capabilities will fail. Qemu responds to the query-cpu-model-expansion qmp
> command with an error because the needed kernel ioct does not exist. When this
> happens a guest cannot even be defined due to missing qemu capabilities data.
> 
> This patch fixes the problem by silently ignoring generic errors stemming from
> calls to query-cpu-model-expansion.
> 
> Reported-by: Farhan Ali 
> Signed-off-by: Collin L. Walling 
> Signed-off-by: Jason J. Herne 
> ---
>  src/qemu/qemu_monitor_json.c | 9 +
>  1 file changed, 9 insertions(+)
> 
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index e767437..1662749 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -5041,6 +5041,15 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon,
>  if (qemuMonitorJSONCommand(mon, cmd, ) < 0)
>  goto cleanup;
>  
> +/* Some QEMU architectures have the query-cpu-model-expansion
> + * command, but return 'GenericError' instead of simply omitting
> + * the command entirely.
> + */

Hmm, this comment says something a bit different than the commit
message. I hope the issue described by this comment was fixed in QEMU
within the same release the query-cpu-model-expansion command was
introduced. But we need to fix this nevertheless to address what the
commit message describes.

> +if (qemuMonitorJSONHasError(reply, "GenericError")) {
> +ret = 0;
> +goto cleanup;
> +}
> +
>  if (qemuMonitorJSONCheckError(cmd, reply) < 0)
>  goto cleanup;

However, we need to do a little bit more than just ignoring this error.
I'll send a v2 soon.

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH] qemu-capabilities: Fix query-cpu-model-expansion on s390 with older kernel

2017-01-12 Thread Collin L. Walling 
When running on s390 with a kernel that does not support cpu model checking and
with a Qemu new enough to support query-cpu-model-expansion, the gathering of 
qemu
capabilities will fail. Qemu responds to the query-cpu-model-expansion qmp
command with an error because the needed kernel ioct does not exist. When this
happens a guest cannot even be defined due to missing qemu capabilities data.

This patch fixes the problem by silently ignoring generic errors stemming from
calls to query-cpu-model-expansion.

Reported-by: Farhan Ali 
Signed-off-by: Collin L. Walling 
Signed-off-by: Jason J. Herne 
---
 src/qemu/qemu_monitor_json.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index e767437..1662749 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -5041,6 +5041,15 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon,
 if (qemuMonitorJSONCommand(mon, cmd, ) < 0)
 goto cleanup;
 
+/* Some QEMU architectures have the query-cpu-model-expansion
+ * command, but return 'GenericError' instead of simply omitting
+ * the command entirely.
+ */
+if (qemuMonitorJSONHasError(reply, "GenericError")) {
+ret = 0;
+goto cleanup;
+}
+
 if (qemuMonitorJSONCheckError(cmd, reply) < 0)
 goto cleanup;
 
-- 
2.7.4

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list