| Message ID | 1355317734-55761-4-git-send-email-jfrei@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
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 >
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 >
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
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.
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
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
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
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~
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
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
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