Patchwork [3/3] S390: Enable -cpu help and QMP query-cpu-definitions

login
register
mail settings
Submitter Jens Freimann
Date Dec. 12, 2012, 1:08 p.m.
Message ID <1355317734-55761-4-git-send-email-jfrei@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/205504/
State New
Headers show

Comments

Jens Freimann - Dec. 12, 2012, 1:08 p.m.
From: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>

This enables qemu -cpu help to return a list of supported CPU models
on s390 and also to query for cpu definitions in the monitor.
Initially only cpu model = host is returned. This needs to be reworked
into a full-fledged CPU model handling later on.
This change is needed to allow libvirt exploiters (like OpenStack)
to specify a CPU model.

Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 hw/s390-virtio.c   |  6 +++++-
 target-s390x/cpu.c | 21 +++++++++++++++++++++
 target-s390x/cpu.h |  3 +++
 3 files changed, 29 insertions(+), 1 deletion(-)
Alexander Graf - Dec. 12, 2012, 1:40 p.m.
On 12.12.2012, at 14:08, Jens Freimann wrote:

> From: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
> 
> This enables qemu -cpu help to return a list of supported CPU models
> on s390 and also to query for cpu definitions in the monitor.
> Initially only cpu model = host is returned. This needs to be reworked
> into a full-fledged CPU model handling later on.
> This change is needed to allow libvirt exploiters (like OpenStack)
> to specify a CPU model.
> 
> Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
> Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
> hw/s390-virtio.c   |  6 +++++-
> target-s390x/cpu.c | 21 +++++++++++++++++++++
> target-s390x/cpu.h |  3 +++
> 3 files changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
> index 18050b1..5d4dd43 100644
> --- a/hw/s390-virtio.c
> +++ b/hw/s390-virtio.c
> @@ -2,6 +2,7 @@
>  * QEMU S390 virtio target
>  *
>  * Copyright (c) 2009 Alexander Graf <agraf@suse.de>
> + * Copyright IBM Corp 2012
>  *
>  * This library is free software; you can redistribute it and/or
>  * modify it under the terms of the GNU Lesser General Public
> @@ -13,7 +14,10 @@
>  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>  * Lesser General Public License for more details.
>  *
> - * You should have received a copy of the GNU Lesser General Public
> + * Contributions after 2012-10-29 are licensed under the terms of the
> + * GNU GPL, version 2 or (at your option) any later version.
> + *
> + * You should have received a copy of the GNU (Lesser) General Public
>  * License along with this library; if not, see <http://www.gnu.org/licenses/>.
>  */
> 
> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
> index 75f60b3..4a8562b 100644
> --- a/target-s390x/cpu.c
> +++ b/target-s390x/cpu.c
> @@ -28,8 +28,29 @@
> #include "hw/hw.h"
> #include "qemu-common.h"
> #include "qemu-timer.h"
> +#include "arch_init.h"
> 
> 
> +/* generate CPU information for cpu -? */
> +void s390_cpu_list(FILE *f, fprintf_function cpu_fprintf)
> +{
> +    (*cpu_fprintf)(f, "s390 %16s\n", "[host]");
> +}
> +
> +CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
> +{
> +    CpuDefinitionInfoList *entry;
> +    CpuDefinitionInfo *info;
> +
> +    info = g_malloc0(sizeof(*info));
> +    info->name = g_strdup("host");
> +
> +    entry = g_malloc0(sizeof(*entry));
> +    entry->value = info;
> +
> +    return entry;

Can't we keep all the above information in a const struct in .rodata?


Alex

> +}
> +
> /* CPUClass::reset() */
> static void s390_cpu_reset(CPUState *s)
> {
> diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
> index 0f9a1f7..3513976 100644
> --- a/target-s390x/cpu.h
> +++ b/target-s390x/cpu.h
> @@ -350,6 +350,9 @@ static inline void cpu_set_tls(CPUS390XState *env, target_ulong newtls)
> #define cpu_gen_code cpu_s390x_gen_code
> #define cpu_signal_handler cpu_s390x_signal_handler
> 
> +void s390_cpu_list(FILE *f, fprintf_function cpu_fprintf);
> +#define cpu_list s390_cpu_list
> +
> #include "exec-all.h"
> 
> #ifdef CONFIG_USER_ONLY
> -- 
> 1.7.12.4
>
Andreas Färber - Dec. 12, 2012, 1:51 p.m.
Am 12.12.2012 14:08, schrieb Jens Freimann:
> From: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
> 
> This enables qemu -cpu help to return a list of supported CPU models
> on s390 and also to query for cpu definitions in the monitor.
> Initially only cpu model = host is returned. This needs to be reworked
> into a full-fledged CPU model handling later on.
> This change is needed to allow libvirt exploiters (like OpenStack)
> to specify a CPU model.
> 
> Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
> Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  hw/s390-virtio.c   |  6 +++++-
>  target-s390x/cpu.c | 21 +++++++++++++++++++++
>  target-s390x/cpu.h |  3 +++
>  3 files changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
> index 18050b1..5d4dd43 100644
> --- a/hw/s390-virtio.c
> +++ b/hw/s390-virtio.c
> @@ -2,6 +2,7 @@
>   * QEMU S390 virtio target
>   *
>   * Copyright (c) 2009 Alexander Graf <agraf@suse.de>
> + * Copyright IBM Corp 2012
>   *
>   * This library is free software; you can redistribute it and/or
>   * modify it under the terms of the GNU Lesser General Public
> @@ -13,7 +14,10 @@
>   * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>   * Lesser General Public License for more details.
>   *
> - * You should have received a copy of the GNU Lesser General Public
> + * Contributions after 2012-10-29 are licensed under the terms of the
> + * GNU GPL, version 2 or (at your option) any later version.
> + *
> + * You should have received a copy of the GNU (Lesser) General Public
>   * License along with this library; if not, see <http://www.gnu.org/licenses/>.
>   */
>  
> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
> index 75f60b3..4a8562b 100644
> --- a/target-s390x/cpu.c
> +++ b/target-s390x/cpu.c
> @@ -28,8 +28,29 @@
>  #include "hw/hw.h"
>  #include "qemu-common.h"
>  #include "qemu-timer.h"
> +#include "arch_init.h"
>  
>  
> +/* generate CPU information for cpu -? */
> +void s390_cpu_list(FILE *f, fprintf_function cpu_fprintf)
> +{
> +    (*cpu_fprintf)(f, "s390 %16s\n", "[host]");

Note that the square-bracket notation was specific to x86 when it
distinguished between built-in and config-based models.

> +}
> +
> +CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
> +{
> +    CpuDefinitionInfoList *entry;
> +    CpuDefinitionInfo *info;
> +
> +    info = g_malloc0(sizeof(*info));
> +    info->name = g_strdup("host");
> +
> +    entry = g_malloc0(sizeof(*entry));
> +    entry->value = info;
> +
> +    return entry;
> +}

"host" only makes sense for KVM, not for TCG. So we would need one other
placeholder model for libvirt.

In cpu_s390x_init() a check should be added to reject "host" if
!kvm_enabled().

Background is that in the CPU subclasses world we would want to derive a
"host-s390-cpu" here and not let that be instantiated outside KVM. Then
we do need one always instantiatible model though.

Regards,
Andreas

> +
>  /* CPUClass::reset() */
>  static void s390_cpu_reset(CPUState *s)
>  {
> diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
> index 0f9a1f7..3513976 100644
> --- a/target-s390x/cpu.h
> +++ b/target-s390x/cpu.h
> @@ -350,6 +350,9 @@ static inline void cpu_set_tls(CPUS390XState *env, target_ulong newtls)
>  #define cpu_gen_code cpu_s390x_gen_code
>  #define cpu_signal_handler cpu_s390x_signal_handler
>  
> +void s390_cpu_list(FILE *f, fprintf_function cpu_fprintf);
> +#define cpu_list s390_cpu_list
> +
>  #include "exec-all.h"
>  
>  #ifdef CONFIG_USER_ONLY
>
Alexander Graf - Dec. 12, 2012, 2:03 p.m.
On 12.12.2012, at 14:51, Andreas Färber wrote:

> Am 12.12.2012 14:08, schrieb Jens Freimann:
>> From: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
>> 
>> This enables qemu -cpu help to return a list of supported CPU models
>> on s390 and also to query for cpu definitions in the monitor.
>> Initially only cpu model = host is returned. This needs to be reworked
>> into a full-fledged CPU model handling later on.
>> This change is needed to allow libvirt exploiters (like OpenStack)
>> to specify a CPU model.
>> 
>> Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
>> Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
>> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> ---
>> hw/s390-virtio.c   |  6 +++++-
>> target-s390x/cpu.c | 21 +++++++++++++++++++++
>> target-s390x/cpu.h |  3 +++
>> 3 files changed, 29 insertions(+), 1 deletion(-)
>> 
>> diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
>> index 18050b1..5d4dd43 100644
>> --- a/hw/s390-virtio.c
>> +++ b/hw/s390-virtio.c
>> @@ -2,6 +2,7 @@
>>  * QEMU S390 virtio target
>>  *
>>  * Copyright (c) 2009 Alexander Graf <agraf@suse.de>
>> + * Copyright IBM Corp 2012
>>  *
>>  * This library is free software; you can redistribute it and/or
>>  * modify it under the terms of the GNU Lesser General Public
>> @@ -13,7 +14,10 @@
>>  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>>  * Lesser General Public License for more details.
>>  *
>> - * You should have received a copy of the GNU Lesser General Public
>> + * Contributions after 2012-10-29 are licensed under the terms of the
>> + * GNU GPL, version 2 or (at your option) any later version.
>> + *
>> + * You should have received a copy of the GNU (Lesser) General Public
>>  * License along with this library; if not, see <http://www.gnu.org/licenses/>.
>>  */
>> 
>> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
>> index 75f60b3..4a8562b 100644
>> --- a/target-s390x/cpu.c
>> +++ b/target-s390x/cpu.c
>> @@ -28,8 +28,29 @@
>> #include "hw/hw.h"
>> #include "qemu-common.h"
>> #include "qemu-timer.h"
>> +#include "arch_init.h"
>> 
>> 
>> +/* generate CPU information for cpu -? */
>> +void s390_cpu_list(FILE *f, fprintf_function cpu_fprintf)
>> +{
>> +    (*cpu_fprintf)(f, "s390 %16s\n", "[host]");
> 
> Note that the square-bracket notation was specific to x86 when it
> distinguished between built-in and config-based models.
> 
>> +}
>> +
>> +CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
>> +{
>> +    CpuDefinitionInfoList *entry;
>> +    CpuDefinitionInfo *info;
>> +
>> +    info = g_malloc0(sizeof(*info));
>> +    info->name = g_strdup("host");
>> +
>> +    entry = g_malloc0(sizeof(*entry));
>> +    entry->value = info;
>> +
>> +    return entry;
>> +}
> 
> "host" only makes sense for KVM, not for TCG. So we would need one other
> placeholder model for libvirt.
> 
> In cpu_s390x_init() a check should be added to reject "host" if
> !kvm_enabled().
> 
> Background is that in the CPU subclasses world we would want to derive a
> "host-s390-cpu" here and not let that be instantiated outside KVM. Then
> we do need one always instantiatible model though.

The TCG model is a "z900". We can just have a "z900" model and a "host" model with tcg_enabled() vs kvm_enabled() checks on each.


Alex
Viktor Mihajlovski - Dec. 12, 2012, 2:57 p.m.
On 12/12/2012 03:03 PM, Alexander Graf wrote:
>
>
>
> The TCG model is a "z900". We can just have a "z900" model and a "host" model with tcg_enabled() vs kvm_enabled() checks on each.
>
>
> Alex
>

I would advise to not define 'artificial' names, i.e. such that cannot
be retrieved via a user-space interface on the host in order to make it
easier for systems management tools to figure out the right CPU model
in the future.

Further, if 'z900' was the host CPU model, then it should also be
accepted in kvm mode in order to behave as on x86. This can be deferred
to a future point in time.

Having said this, I have no idea whether 'z900' would be a correct
choice for a CPU model or not.
Viktor Mihajlovski - Dec. 12, 2012, 3:05 p.m.
On 12/12/2012 02:51 PM, Andreas Färber wrote:
>> +    (*cpu_fprintf)(f, "s390 %16s\n", "[host]");
>
> Note that the square-bracket notation was specific to x86 when it
> distinguished between built-in and config-based models.
>
OK, since libvirt capable of dealing with s390 cpu models will
never see this, we can change it any way that is wanted.
So, host without brackets?
> "host" only makes sense for KVM, not for TCG. So we would need one other
> placeholder model for libvirt.
see my reply to Alex' comment: the placeholder name must be chosen
carefully, i.e. future-proof
Alexander Graf - Dec. 12, 2012, 3:50 p.m.
On 12/12/2012 04:05 PM, Viktor Mihajlovski wrote:
> On 12/12/2012 02:51 PM, Andreas Färber wrote:
>>> +    (*cpu_fprintf)(f, "s390 %16s\n", "[host]");
>>
>> Note that the square-bracket notation was specific to x86 when it
>> distinguished between built-in and config-based models.
>>
> OK, since libvirt capable of dealing with s390 cpu models will
> never see this, we can change it any way that is wanted.
> So, host without brackets?
>> "host" only makes sense for KVM, not for TCG. So we would need one other
>> placeholder model for libvirt.
> see my reply to Alex' comment: the placeholder name must be chosen
> carefully, i.e. future-proof

Yeah, it's perfectly fine for me to ignore the model name in the code 
today (as we do iirc) and only add the host target for starters. Though 
that one should do a kvm_enabled() check.


Alex
Andreas Färber - Dec. 12, 2012, 4:28 p.m.
Am 12.12.2012 16:05, schrieb Viktor Mihajlovski:
> On 12/12/2012 02:51 PM, Andreas Färber wrote:
>>> +    (*cpu_fprintf)(f, "s390 %16s\n", "[host]");
>>
>> Note that the square-bracket notation was specific to x86 when it
>> distinguished between built-in and config-based models.
>>
> OK, since libvirt capable of dealing with s390 cpu models will
> never see this, we can change it any way that is wanted.
> So, host without brackets?

Yes, but see below...

>> "host" only makes sense for KVM, not for TCG. So we would need one other
>> placeholder model for libvirt.
> see my reply to Alex' comment: the placeholder name must be chosen
> carefully, i.e. future-proof

On further thoughts, didn't we discuss that the issue libvirt wants to
address is that migration from z10 to z9 must fail? That's not solved
with -cpu host, we would need two other models then. IMO ideally -cpu
host should have the same semantics as on x86, that is passing the host
features through mostly 1:1. IIUC there is currently no way to not do so?

What about future-wise having -cpu host not be a subclass and instead
behaving like Alex' -cpu best, given the above semantics? What I am just
worried about with this patch is cementing the use of -cpu host into
libvirt when that is not a mid-term solution.

Andreas
Richard Henderson - Dec. 12, 2012, 5:52 p.m.
On 12/12/2012 06:03 AM, Alexander Graf wrote:
> The TCG model is a "z900". We can just have a "z900" model and a
> "host" model with tcg_enabled() vs kvm_enabled() checks on each.

My target-s390x tcg rewrite targets something just shy of z196.

The main thing that's missing is actually STORE FACILITY LIST EXTENDED.
Which I always assumed would go along with a re-vamp of the cpu model,
just as we're considering here, so that we could in fact emulate z900
or anything in between.


r~
Alexander Graf - Dec. 12, 2012, 6:23 p.m.
On 12/12/2012 05:28 PM, Andreas Färber wrote:
> Am 12.12.2012 16:05, schrieb Viktor Mihajlovski:
>> On 12/12/2012 02:51 PM, Andreas Färber wrote:
>>>> +    (*cpu_fprintf)(f, "s390 %16s\n", "[host]");
>>> Note that the square-bracket notation was specific to x86 when it
>>> distinguished between built-in and config-based models.
>>>
>> OK, since libvirt capable of dealing with s390 cpu models will
>> never see this, we can change it any way that is wanted.
>> So, host without brackets?
> Yes, but see below...

Yes, and with #ifdef CONFIG_KVM :). That way the TCG behavior stays 
untouched.

>
>>> "host" only makes sense for KVM, not for TCG. So we would need one other
>>> placeholder model for libvirt.
>> see my reply to Alex' comment: the placeholder name must be chosen
>> carefully, i.e. future-proof
> On further thoughts, didn't we discuss that the issue libvirt wants to
> address is that migration from z10 to z9 must fail? That's not solved
> with -cpu host, we would need two other models then. IMO ideally -cpu
> host should have the same semantics as on x86, that is passing the host
> features through mostly 1:1. IIUC there is currently no way to not do so?

That is another problem that we need to solve, but one thing at a time.

> What about future-wise having -cpu host not be a subclass and instead
> behaving like Alex' -cpu best, given the above semantics? What I am just
> worried about with this patch is cementing the use of -cpu host into
> libvirt when that is not a mid-term solution.

I think on s390 we can get away with the same method as ppc for -cpu 
host, which basically is -cpu best without fuzziness.


Alex
Alexander Graf - Dec. 12, 2012, 6:25 p.m.
On 12/12/2012 06:52 PM, Richard Henderson wrote:
> On 12/12/2012 06:03 AM, Alexander Graf wrote:
>> The TCG model is a "z900". We can just have a "z900" model and a
>> "host" model with tcg_enabled() vs kvm_enabled() checks on each.
> My target-s390x tcg rewrite targets something just shy of z196.
>
> The main thing that's missing is actually STORE FACILITY LIST EXTENDED.
> Which I always assumed would go along with a re-vamp of the cpu model,
> just as we're considering here, so that we could in fact emulate z900
> or anything in between.

Yes, and it's definitely something we need to do. I'm just concerned 
about timing. Do we want to hold back a bug fix (libvirt can break with 
empty cpu lists) for a feature (proper cpu list for tcg targets)?

I'd say let's make the -cpu host target work and whoever gets around to 
it first can implement cpu models, store facility list extended, and tcg 
feature masks.


Alex

Patch

diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
index 18050b1..5d4dd43 100644
--- a/hw/s390-virtio.c
+++ b/hw/s390-virtio.c
@@ -2,6 +2,7 @@ 
  * QEMU S390 virtio target
  *
  * Copyright (c) 2009 Alexander Graf <agraf@suse.de>
+ * Copyright IBM Corp 2012
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -13,7 +14,10 @@ 
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
  * Lesser General Public License for more details.
  *
- * You should have received a copy of the GNU Lesser General Public
+ * Contributions after 2012-10-29 are licensed under the terms of the
+ * GNU GPL, version 2 or (at your option) any later version.
+ *
+ * You should have received a copy of the GNU (Lesser) General Public
  * License along with this library; if not, see <http://www.gnu.org/licenses/>.
  */
 
diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
index 75f60b3..4a8562b 100644
--- a/target-s390x/cpu.c
+++ b/target-s390x/cpu.c
@@ -28,8 +28,29 @@ 
 #include "hw/hw.h"
 #include "qemu-common.h"
 #include "qemu-timer.h"
+#include "arch_init.h"
 
 
+/* generate CPU information for cpu -? */
+void s390_cpu_list(FILE *f, fprintf_function cpu_fprintf)
+{
+    (*cpu_fprintf)(f, "s390 %16s\n", "[host]");
+}
+
+CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
+{
+    CpuDefinitionInfoList *entry;
+    CpuDefinitionInfo *info;
+
+    info = g_malloc0(sizeof(*info));
+    info->name = g_strdup("host");
+
+    entry = g_malloc0(sizeof(*entry));
+    entry->value = info;
+
+    return entry;
+}
+
 /* CPUClass::reset() */
 static void s390_cpu_reset(CPUState *s)
 {
diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
index 0f9a1f7..3513976 100644
--- a/target-s390x/cpu.h
+++ b/target-s390x/cpu.h
@@ -350,6 +350,9 @@  static inline void cpu_set_tls(CPUS390XState *env, target_ulong newtls)
 #define cpu_gen_code cpu_s390x_gen_code
 #define cpu_signal_handler cpu_s390x_signal_handler
 
+void s390_cpu_list(FILE *f, fprintf_function cpu_fprintf);
+#define cpu_list s390_cpu_list
+
 #include "exec-all.h"
 
 #ifdef CONFIG_USER_ONLY