| Message ID | 1485868319-16151-3-git-send-email-thuth@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Jan 31, 2017 at 02:11:59PM +0100, Thomas Huth wrote: > When running with KVM on POWER, we register some CPU types during > the initialization function of the ppc64 KVM code (which unfortunately > also can not be done via a type_init() like it is done on x86). So to > be able to see these updates in the CPU help text, the code that calls > list_cpus() has to be run after configure_accelerator(). This move should > be fine since the "cpu_model" variable is also never used before the call > to configure_accelerator(), and thus there should not be any unwanted > side effects in the code before configure_accelerator() if the user > started QEMU with "-cpu ?" or "-cpu help". > > Signed-off-by: Thomas Huth <thuth@redhat.com> Reviewed-by: David Gibson <david@gibson.dropbear.id.au> This looks fine to me, but it's not within my area to apply. Paolo, if you want to ack I'm happy to take it through my tree if that's convenient for you. > --- > vl.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/vl.c b/vl.c > index 0b72b12..315c5c3 100644 > --- a/vl.c > +++ b/vl.c > @@ -4055,11 +4055,6 @@ int main(int argc, char **argv, char **envp) > qemu_set_hw_version(machine_class->hw_version); > } > > - if (cpu_model && is_help_option(cpu_model)) { > - list_cpus(stdout, &fprintf, cpu_model); > - exit(0); > - } > - > if (!trace_init_backends()) { > exit(1); > } > @@ -4298,6 +4293,11 @@ int main(int argc, char **argv, char **envp) > > configure_accelerator(current_machine); > > + if (cpu_model && is_help_option(cpu_model)) { > + list_cpus(stdout, &fprintf, cpu_model); > + exit(0); > + } > + > if (qtest_chrdev) { > qtest_init(qtest_chrdev, qtest_log, &error_fatal); > }
On 31/01/2017 16:08, David Gibson wrote: > On Tue, Jan 31, 2017 at 02:11:59PM +0100, Thomas Huth wrote: >> When running with KVM on POWER, we register some CPU types during >> the initialization function of the ppc64 KVM code (which unfortunately >> also can not be done via a type_init() like it is done on x86). So to >> be able to see these updates in the CPU help text, the code that calls >> list_cpus() has to be run after configure_accelerator(). This move should >> be fine since the "cpu_model" variable is also never used before the call >> to configure_accelerator(), and thus there should not be any unwanted >> side effects in the code before configure_accelerator() if the user >> started QEMU with "-cpu ?" or "-cpu help". >> >> Signed-off-by: Thomas Huth <thuth@redhat.com> > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > > This looks fine to me, but it's not within my area to apply. Paolo, > if you want to ack I'm happy to take it through my tree if that's > convenient for you. > >> --- >> vl.c | 10 +++++----- >> 1 file changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/vl.c b/vl.c >> index 0b72b12..315c5c3 100644 >> --- a/vl.c >> +++ b/vl.c >> @@ -4055,11 +4055,6 @@ int main(int argc, char **argv, char **envp) >> qemu_set_hw_version(machine_class->hw_version); >> } >> >> - if (cpu_model && is_help_option(cpu_model)) { >> - list_cpus(stdout, &fprintf, cpu_model); >> - exit(0); >> - } >> - >> if (!trace_init_backends()) { >> exit(1); >> } >> @@ -4298,6 +4293,11 @@ int main(int argc, char **argv, char **envp) >> >> configure_accelerator(current_machine); >> >> + if (cpu_model && is_help_option(cpu_model)) { >> + list_cpus(stdout, &fprintf, cpu_model); >> + exit(0); >> + } >> + >> if (qtest_chrdev) { >> qtest_init(qtest_chrdev, qtest_log, &error_fatal); >> } > Eduardo is the one who did most of the accel and CPU definitions work. Paolo
On Tue, Jan 31, 2017 at 02:11:59PM +0100, Thomas Huth wrote: > When running with KVM on POWER, we register some CPU types during > the initialization function of the ppc64 KVM code (which unfortunately > also can not be done via a type_init() like it is done on x86). Can you elaborate why it can't be done via type_init()? If the QOM type hierarchy depends on any runtime data unavailable at type_init(), we should fix that. > So to > be able to see these updates in the CPU help text, the code that calls > list_cpus() has to be run after configure_accelerator(). This move should > be fine since the "cpu_model" variable is also never used before the call > to configure_accelerator(), and thus there should not be any unwanted > side effects in the code before configure_accelerator() if the user > started QEMU with "-cpu ?" or "-cpu help". > > Signed-off-by: Thomas Huth <thuth@redhat.com> I am not convinced that the output of "-cpu help" and "-cpu help -machine accel=kvm" should look different. Do you have an example of what exactly is wrong with the output currently? I actually believe list_cpus() needs to be called _earlier_, not later. Otherwise we won't be able to fix this bug: $ qemu-system-arm -cpu help qemu-system-arm:/usr/local/etc/qemu/qemu.conf:1: No machine specified, and there is no default Use -machine help to list supported machines $ > --- > vl.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/vl.c b/vl.c > index 0b72b12..315c5c3 100644 > --- a/vl.c > +++ b/vl.c > @@ -4055,11 +4055,6 @@ int main(int argc, char **argv, char **envp) > qemu_set_hw_version(machine_class->hw_version); > } > > - if (cpu_model && is_help_option(cpu_model)) { > - list_cpus(stdout, &fprintf, cpu_model); > - exit(0); > - } > - > if (!trace_init_backends()) { > exit(1); > } > @@ -4298,6 +4293,11 @@ int main(int argc, char **argv, char **envp) > > configure_accelerator(current_machine); > > + if (cpu_model && is_help_option(cpu_model)) { > + list_cpus(stdout, &fprintf, cpu_model); > + exit(0); > + } > + > if (qtest_chrdev) { > qtest_init(qtest_chrdev, qtest_log, &error_fatal); > } > -- > 1.8.3.1 > >
On Fri, Mar 03, 2017 at 11:58:07AM -0300, Eduardo Habkost wrote: > On Tue, Jan 31, 2017 at 02:11:59PM +0100, Thomas Huth wrote: > > When running with KVM on POWER, we register some CPU types during > > the initialization function of the ppc64 KVM code (which unfortunately > > also can not be done via a type_init() like it is done on x86). > > Can you elaborate why it can't be done via type_init()? If the > QOM type hierarchy depends on any runtime data unavailable at > type_init(), we should fix that. Hmm.. how? This is specifically for the special 'host' cpu in the case of KVM. We can't use a static configuration here, because there are things on the host that could limit what features of the CPU are available for guest use. So, we need KVM to be initialized in order to query that information. > > So to > > be able to see these updates in the CPU help text, the code that calls > > list_cpus() has to be run after configure_accelerator(). This move should > > be fine since the "cpu_model" variable is also never used before the call > > to configure_accelerator(), and thus there should not be any unwanted > > side effects in the code before configure_accelerator() if the user > > started QEMU with "-cpu ?" or "-cpu help". > > > > Signed-off-by: Thomas Huth <thuth@redhat.com> > > I am not convinced that the output of "-cpu help" and > "-cpu help -machine accel=kvm" should look different. Do you have > an example of what exactly is wrong with the output currently? > > I actually believe list_cpus() needs to be called _earlier_, not > later. Otherwise we won't be able to fix this bug: > > $ qemu-system-arm -cpu help > qemu-system-arm:/usr/local/etc/qemu/qemu.conf:1: No machine specified, and there is no default > Use -machine help to list supported machines > $ > > > --- > > vl.c | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/vl.c b/vl.c > > index 0b72b12..315c5c3 100644 > > --- a/vl.c > > +++ b/vl.c > > @@ -4055,11 +4055,6 @@ int main(int argc, char **argv, char **envp) > > qemu_set_hw_version(machine_class->hw_version); > > } > > > > - if (cpu_model && is_help_option(cpu_model)) { > > - list_cpus(stdout, &fprintf, cpu_model); > > - exit(0); > > - } > > - > > if (!trace_init_backends()) { > > exit(1); > > } > > @@ -4298,6 +4293,11 @@ int main(int argc, char **argv, char **envp) > > > > configure_accelerator(current_machine); > > > > + if (cpu_model && is_help_option(cpu_model)) { > > + list_cpus(stdout, &fprintf, cpu_model); > > + exit(0); > > + } > > + > > if (qtest_chrdev) { > > qtest_init(qtest_chrdev, qtest_log, &error_fatal); > > } >
On Mon, Mar 06, 2017 at 10:06:27AM +1100, David Gibson wrote: > On Fri, Mar 03, 2017 at 11:58:07AM -0300, Eduardo Habkost wrote: > > On Tue, Jan 31, 2017 at 02:11:59PM +0100, Thomas Huth wrote: > > > When running with KVM on POWER, we register some CPU types during > > > the initialization function of the ppc64 KVM code (which unfortunately > > > also can not be done via a type_init() like it is done on x86). > > > > Can you elaborate why it can't be done via type_init()? If the > > QOM type hierarchy depends on any runtime data unavailable at > > type_init(), we should fix that. > > Hmm.. how? This is specifically for the special 'host' cpu in the > case of KVM. We can't use a static configuration here, because there > are things on the host that could limit what features of the CPU are > available for guest use. So, we need KVM to be initialized in order > to query that information. There's nothing saying you need to query that information before type_register() or during class_init, does it? The behavior of TYPE_HOST_POWERPC_CPU after object_new() is called can be as dynamic as you want it to, but the QOM type hierarchy is supposed to be static. Registering QOM types outside type_init() is only causing us problems and we should stop doing that. > > > > So to > > > be able to see these updates in the CPU help text, the code that calls > > > list_cpus() has to be run after configure_accelerator(). This move should > > > be fine since the "cpu_model" variable is also never used before the call > > > to configure_accelerator(), and thus there should not be any unwanted > > > side effects in the code before configure_accelerator() if the user > > > started QEMU with "-cpu ?" or "-cpu help". > > > > > > Signed-off-by: Thomas Huth <thuth@redhat.com> > > > > I am not convinced that the output of "-cpu help" and > > "-cpu help -machine accel=kvm" should look different. Do you have > > an example of what exactly is wrong with the output currently? > > > > I actually believe list_cpus() needs to be called _earlier_, not > > later. Otherwise we won't be able to fix this bug: > > > > $ qemu-system-arm -cpu help > > qemu-system-arm:/usr/local/etc/qemu/qemu.conf:1: No machine specified, and there is no default > > Use -machine help to list supported machines > > $ > > > > > --- > > > vl.c | 10 +++++----- > > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > > > diff --git a/vl.c b/vl.c > > > index 0b72b12..315c5c3 100644 > > > --- a/vl.c > > > +++ b/vl.c > > > @@ -4055,11 +4055,6 @@ int main(int argc, char **argv, char **envp) > > > qemu_set_hw_version(machine_class->hw_version); > > > } > > > > > > - if (cpu_model && is_help_option(cpu_model)) { > > > - list_cpus(stdout, &fprintf, cpu_model); > > > - exit(0); > > > - } > > > - > > > if (!trace_init_backends()) { > > > exit(1); > > > } > > > @@ -4298,6 +4293,11 @@ int main(int argc, char **argv, char **envp) > > > > > > configure_accelerator(current_machine); > > > > > > + if (cpu_model && is_help_option(cpu_model)) { > > > + list_cpus(stdout, &fprintf, cpu_model); > > > + exit(0); > > > + } > > > + > > > if (qtest_chrdev) { > > > qtest_init(qtest_chrdev, qtest_log, &error_fatal); > > > } > > > > -- > David Gibson | I'll have my music baroque, and my code > david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ > | _way_ _around_! > http://www.ozlabs.org/~dgibson
On Mon, Mar 06, 2017 at 08:47:52AM -0300, Eduardo Habkost wrote: > On Mon, Mar 06, 2017 at 10:06:27AM +1100, David Gibson wrote: > > On Fri, Mar 03, 2017 at 11:58:07AM -0300, Eduardo Habkost wrote: > > > On Tue, Jan 31, 2017 at 02:11:59PM +0100, Thomas Huth wrote: > > > > When running with KVM on POWER, we register some CPU types during > > > > the initialization function of the ppc64 KVM code (which unfortunately > > > > also can not be done via a type_init() like it is done on x86). > > > > > > Can you elaborate why it can't be done via type_init()? If the > > > QOM type hierarchy depends on any runtime data unavailable at > > > type_init(), we should fix that. > > > > Hmm.. how? This is specifically for the special 'host' cpu in the > > case of KVM. We can't use a static configuration here, because there > > are things on the host that could limit what features of the CPU are > > available for guest use. So, we need KVM to be initialized in order > > to query that information. > > There's nothing saying you need to query that information before > type_register() or during class_init, does it? The behavior of > TYPE_HOST_POWERPC_CPU after object_new() is called can be as > dynamic as you want it to, but the QOM type hierarchy is supposed > to be static. So, the thing is we have some properties that logically belong in the CPU class, rather than instance, and that's correct for all TCG CPUs, but not for the KVM host CPU. It seems nasty to have to force all those things into the instance just because of KVM. > Registering QOM types outside type_init() is only causing us > problems and we should stop doing that. > > > > > > > So to > > > > be able to see these updates in the CPU help text, the code that calls > > > > list_cpus() has to be run after configure_accelerator(). This move should > > > > be fine since the "cpu_model" variable is also never used before the call > > > > to configure_accelerator(), and thus there should not be any unwanted > > > > side effects in the code before configure_accelerator() if the user > > > > started QEMU with "-cpu ?" or "-cpu help". > > > > > > > > Signed-off-by: Thomas Huth <thuth@redhat.com> > > > > > > I am not convinced that the output of "-cpu help" and > > > "-cpu help -machine accel=kvm" should look different. Do you have > > > an example of what exactly is wrong with the output currently? > > > > > > I actually believe list_cpus() needs to be called _earlier_, not > > > later. Otherwise we won't be able to fix this bug: > > > > > > $ qemu-system-arm -cpu help > > > qemu-system-arm:/usr/local/etc/qemu/qemu.conf:1: No machine specified, and there is no default > > > Use -machine help to list supported machines > > > $ > > > > > > > --- > > > > vl.c | 10 +++++----- > > > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/vl.c b/vl.c > > > > index 0b72b12..315c5c3 100644 > > > > --- a/vl.c > > > > +++ b/vl.c > > > > @@ -4055,11 +4055,6 @@ int main(int argc, char **argv, char **envp) > > > > qemu_set_hw_version(machine_class->hw_version); > > > > } > > > > > > > > - if (cpu_model && is_help_option(cpu_model)) { > > > > - list_cpus(stdout, &fprintf, cpu_model); > > > > - exit(0); > > > > - } > > > > - > > > > if (!trace_init_backends()) { > > > > exit(1); > > > > } > > > > @@ -4298,6 +4293,11 @@ int main(int argc, char **argv, char **envp) > > > > > > > > configure_accelerator(current_machine); > > > > > > > > + if (cpu_model && is_help_option(cpu_model)) { > > > > + list_cpus(stdout, &fprintf, cpu_model); > > > > + exit(0); > > > > + } > > > > + > > > > if (qtest_chrdev) { > > > > qtest_init(qtest_chrdev, qtest_log, &error_fatal); > > > > } > > > > > > > >
On 03.03.2017 15:58, Eduardo Habkost wrote: [...] > I am not convinced that the output of "-cpu help" and > "-cpu help -machine accel=kvm" should look different. Do you have > an example of what exactly is wrong with the output currently? The problem is that on POWER, we've got a "family" of CPUs with different sub-types, e.g. for POWER8: $ qemu-system-ppc64 -cpu ? | grep POWER8 PowerPC POWER8E_v2.1 PVR 004b0201 PowerPC POWER8E (alias for POWER8E_v2.1) PowerPC POWER8NVL_v1.0 PVR 004c0100 PowerPC POWER8NVL (alias for POWER8NVL_v1.0) PowerPC POWER8_v2.0 PVR 004d0200 PowerPC POWER8 (alias for POWER8_v2.0) Most of the users don't know about the current subtype that they are using, and just want to use "-cpu POWER8" - and for example we've also got an agreement with the libvirt folks that they can always use "-cpu POWER8" for any kind of POWER8 system, no matter whether the host is using a POWER8E or POWER8NVL chip. So the "POWER8" alias now gets updated internally in QEMU to the correct host CPU type ... but the output of "-cpu help" is then still wrong. I agree that it's kind of ugly to have different help texts depending on whether "accel=kvm" has been used or not, but that sounds still better to me than printing wrong information here. Thinking about this again ... maybe it would be better if we'd rework the help text to print out something like this instead: PowerPC POWER8 (alias for any POWER8 chip) ... so that we simply get rid of the version/subtype information here completely? Thomas
On Tue, Mar 07, 2017 at 02:31:05PM +1100, David Gibson wrote: > On Mon, Mar 06, 2017 at 08:47:52AM -0300, Eduardo Habkost wrote: > > On Mon, Mar 06, 2017 at 10:06:27AM +1100, David Gibson wrote: > > > On Fri, Mar 03, 2017 at 11:58:07AM -0300, Eduardo Habkost wrote: > > > > On Tue, Jan 31, 2017 at 02:11:59PM +0100, Thomas Huth wrote: > > > > > When running with KVM on POWER, we register some CPU types during > > > > > the initialization function of the ppc64 KVM code (which unfortunately > > > > > also can not be done via a type_init() like it is done on x86). > > > > > > > > Can you elaborate why it can't be done via type_init()? If the > > > > QOM type hierarchy depends on any runtime data unavailable at > > > > type_init(), we should fix that. > > > > > > Hmm.. how? This is specifically for the special 'host' cpu in the > > > case of KVM. We can't use a static configuration here, because there > > > are things on the host that could limit what features of the CPU are > > > available for guest use. So, we need KVM to be initialized in order > > > to query that information. > > > > There's nothing saying you need to query that information before > > type_register() or during class_init, does it? The behavior of > > TYPE_HOST_POWERPC_CPU after object_new() is called can be as > > dynamic as you want it to, but the QOM type hierarchy is supposed > > to be static. > > So, the thing is we have some properties that logically belong in the > CPU class, rather than instance, and that's correct for all TCG CPUs, > but not for the KVM host CPU. It seems nasty to have to force all > those things into the instance just because of KVM. You can still register any class properties you want, without querying KVM first. Are there properties that you are able to register only after querying KVM? Is the set of class properties on TYPE_HOST_POWERPC_CPU different depending on the host capabilities? I'm looking at target/ppc/cpu-models.c, and I only see the class_init function setting PowerPCCPUClass::pvr and PowerPCCPUClass::svr. Am I missing anything? You can still keep pvr and svr on the CPU class. The only thing that's different on TYPE_HOST_POWERPC_CPU is that it won't have the preset values on PowerPCCPUClass. But it can initialize the instance values on ->instance_init() or ->realize() instead. I even see ppc_cpu_class_by_pvr*() treating TYPE_HOST_POWERPC_CPU as special on PVR lookup, already. So it looks like the code is almost ready for that? > > > Registering QOM types outside type_init() is only causing us > > problems and we should stop doing that. > > > > > > > > > > So to > > > > > be able to see these updates in the CPU help text, the code that calls > > > > > list_cpus() has to be run after configure_accelerator(). This move should > > > > > be fine since the "cpu_model" variable is also never used before the call > > > > > to configure_accelerator(), and thus there should not be any unwanted > > > > > side effects in the code before configure_accelerator() if the user > > > > > started QEMU with "-cpu ?" or "-cpu help". > > > > > > > > > > Signed-off-by: Thomas Huth <thuth@redhat.com> > > > > > > > > I am not convinced that the output of "-cpu help" and > > > > "-cpu help -machine accel=kvm" should look different. Do you have > > > > an example of what exactly is wrong with the output currently? > > > > > > > > I actually believe list_cpus() needs to be called _earlier_, not > > > > later. Otherwise we won't be able to fix this bug: > > > > > > > > $ qemu-system-arm -cpu help > > > > qemu-system-arm:/usr/local/etc/qemu/qemu.conf:1: No machine specified, and there is no default > > > > Use -machine help to list supported machines > > > > $ > > > > > > > > > --- > > > > > vl.c | 10 +++++----- > > > > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > > > > > > > diff --git a/vl.c b/vl.c > > > > > index 0b72b12..315c5c3 100644 > > > > > --- a/vl.c > > > > > +++ b/vl.c > > > > > @@ -4055,11 +4055,6 @@ int main(int argc, char **argv, char **envp) > > > > > qemu_set_hw_version(machine_class->hw_version); > > > > > } > > > > > > > > > > - if (cpu_model && is_help_option(cpu_model)) { > > > > > - list_cpus(stdout, &fprintf, cpu_model); > > > > > - exit(0); > > > > > - } > > > > > - > > > > > if (!trace_init_backends()) { > > > > > exit(1); > > > > > } > > > > > @@ -4298,6 +4293,11 @@ int main(int argc, char **argv, char **envp) > > > > > > > > > > configure_accelerator(current_machine); > > > > > > > > > > + if (cpu_model && is_help_option(cpu_model)) { > > > > > + list_cpus(stdout, &fprintf, cpu_model); > > > > > + exit(0); > > > > > + } > > > > > + > > > > > if (qtest_chrdev) { > > > > > qtest_init(qtest_chrdev, qtest_log, &error_fatal); > > > > > } > > > > > > > > > > > > > > > -- > David Gibson | I'll have my music baroque, and my code > david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ > | _way_ _around_! > http://www.ozlabs.org/~dgibson
On Tue, Mar 07, 2017 at 10:02:26AM +0100, Thomas Huth wrote: > On 03.03.2017 15:58, Eduardo Habkost wrote: > [...] > > I am not convinced that the output of "-cpu help" and > > "-cpu help -machine accel=kvm" should look different. Do you have > > an example of what exactly is wrong with the output currently? > > The problem is that on POWER, we've got a "family" of CPUs with > different sub-types, e.g. for POWER8: > > $ qemu-system-ppc64 -cpu ? | grep POWER8 > PowerPC POWER8E_v2.1 PVR 004b0201 > PowerPC POWER8E (alias for POWER8E_v2.1) > PowerPC POWER8NVL_v1.0 PVR 004c0100 > PowerPC POWER8NVL (alias for POWER8NVL_v1.0) > PowerPC POWER8_v2.0 PVR 004d0200 > PowerPC POWER8 (alias for POWER8_v2.0) > > Most of the users don't know about the current subtype that they are > using, and just want to use "-cpu POWER8" - and for example we've also > got an agreement with the libvirt folks that they can always use "-cpu > POWER8" for any kind of POWER8 system, no matter whether the host is > using a POWER8E or POWER8NVL chip. > So the "POWER8" alias now gets updated internally in QEMU to the correct > host CPU type ... but the output of "-cpu help" is then still wrong. > I agree that it's kind of ugly to have different help texts depending on > whether "accel=kvm" has been used or not, but that sounds still better > to me than printing wrong information here. I agree that incorrect information is even worse than showing different help information depending on accel=kvm, but: > Thinking about this again ... maybe it would be better if we'd rework > the help text to print out something like this instead: > > PowerPC POWER8 (alias for any POWER8 chip) > > ... so that we simply get rid of the version/subtype information here > completely? Yes, making help output not depend on accel=kvm sounds better to me. This seems to be affected only by the alias table, so it can be fixed even before we address the late-type_register() issue I was discussing with David?
On Tue, Mar 07, 2017 at 09:02:37AM -0300, Eduardo Habkost wrote: > On Tue, Mar 07, 2017 at 02:31:05PM +1100, David Gibson wrote: > > On Mon, Mar 06, 2017 at 08:47:52AM -0300, Eduardo Habkost wrote: > > > On Mon, Mar 06, 2017 at 10:06:27AM +1100, David Gibson wrote: > > > > On Fri, Mar 03, 2017 at 11:58:07AM -0300, Eduardo Habkost wrote: > > > > > On Tue, Jan 31, 2017 at 02:11:59PM +0100, Thomas Huth wrote: > > > > > > When running with KVM on POWER, we register some CPU types during > > > > > > the initialization function of the ppc64 KVM code (which unfortunately > > > > > > also can not be done via a type_init() like it is done on x86). > > > > > > > > > > Can you elaborate why it can't be done via type_init()? If the > > > > > QOM type hierarchy depends on any runtime data unavailable at > > > > > type_init(), we should fix that. > > > > > > > > Hmm.. how? This is specifically for the special 'host' cpu in the > > > > case of KVM. We can't use a static configuration here, because there > > > > are things on the host that could limit what features of the CPU are > > > > available for guest use. So, we need KVM to be initialized in order > > > > to query that information. > > > > > > There's nothing saying you need to query that information before > > > type_register() or during class_init, does it? The behavior of > > > TYPE_HOST_POWERPC_CPU after object_new() is called can be as > > > dynamic as you want it to, but the QOM type hierarchy is supposed > > > to be static. > > > > So, the thing is we have some properties that logically belong in the > > CPU class, rather than instance, and that's correct for all TCG CPUs, > > but not for the KVM host CPU. It seems nasty to have to force all > > those things into the instance just because of KVM. > > You can still register any class properties you want, without > querying KVM first. Are there properties that you are able to > register only after querying KVM? Is the set of class properties > on TYPE_HOST_POWERPC_CPU different depending on the host > capabilities? Sorry, I used "properties" sloppily, not meaning QOM properties. There are several fields in the CPU class which describe CPU capabilities - bitmasks indicating which instructions are available, and L1 cacheline sizes. There are some other things that are in the CPU instance at the moment, but arguably would belong better in the class: available page sizes and PTE encodings, for example. At the real hardware level these things are dependent only the model of CPU - hence belonging in cpu class, not instance. But, because of the way virtualization works on POWER, some of the features may not be available to guests, due to configuration of the hypervisor. So for the "host" cpu we need to query KVM to see which CPU features are actually available. > I'm looking at target/ppc/cpu-models.c, and I only see the > class_init function setting PowerPCCPUClass::pvr and > PowerPCCPUClass::svr. Am I missing anything? Yes. Notice that the .parent set there is not the base powerpc cpu class, but a "family" class. Those families are defined in translate_init.c by another hairy macro POWERPC_FAMILY(). The family class_init function (generally done as a block below the macro invocation) initializes several other things, including insns_flags. kvmppc_host_cpu_class_init(), uses the family's copy of insns_flags as a base, but needs to query the host and adjust it for a couple of instruction groups that can be disabled from the hypervisor. [Aside: we also update the cache sizes based on host information. The reasons for that are complicated. This particular case doesn't require a late class init though, because although it's queried from the host, it's not queried from kvm specifically] > You can still keep pvr and svr on the CPU class. The only thing > that's different on TYPE_HOST_POWERPC_CPU is that it won't have > the preset values on PowerPCCPUClass. But it can initialize the > instance values on ->instance_init() or ->realize() instead. > > I even see ppc_cpu_class_by_pvr*() treating TYPE_HOST_POWERPC_CPU > as special on PVR lookup, already. So it looks like the code is > almost ready for that? PVR is a red herring. insn_flags is the real issue. > > > > > > Registering QOM types outside type_init() is only causing us > > > problems and we should stop doing that. > > > > > > > > > > > > > So to > > > > > > be able to see these updates in the CPU help text, the code that calls > > > > > > list_cpus() has to be run after configure_accelerator(). This move should > > > > > > be fine since the "cpu_model" variable is also never used before the call > > > > > > to configure_accelerator(), and thus there should not be any unwanted > > > > > > side effects in the code before configure_accelerator() if the user > > > > > > started QEMU with "-cpu ?" or "-cpu help". > > > > > > > > > > > > Signed-off-by: Thomas Huth <thuth@redhat.com> > > > > > > > > > > I am not convinced that the output of "-cpu help" and > > > > > "-cpu help -machine accel=kvm" should look different. Do you have > > > > > an example of what exactly is wrong with the output currently? > > > > > > > > > > I actually believe list_cpus() needs to be called _earlier_, not > > > > > later. Otherwise we won't be able to fix this bug: > > > > > > > > > > $ qemu-system-arm -cpu help > > > > > qemu-system-arm:/usr/local/etc/qemu/qemu.conf:1: No machine specified, and there is no default > > > > > Use -machine help to list supported machines > > > > > $ > > > > > > > > > > > --- > > > > > > vl.c | 10 +++++----- > > > > > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > > > > > > > > > diff --git a/vl.c b/vl.c > > > > > > index 0b72b12..315c5c3 100644 > > > > > > --- a/vl.c > > > > > > +++ b/vl.c > > > > > > @@ -4055,11 +4055,6 @@ int main(int argc, char **argv, char **envp) > > > > > > qemu_set_hw_version(machine_class->hw_version); > > > > > > } > > > > > > > > > > > > - if (cpu_model && is_help_option(cpu_model)) { > > > > > > - list_cpus(stdout, &fprintf, cpu_model); > > > > > > - exit(0); > > > > > > - } > > > > > > - > > > > > > if (!trace_init_backends()) { > > > > > > exit(1); > > > > > > } > > > > > > @@ -4298,6 +4293,11 @@ int main(int argc, char **argv, char **envp) > > > > > > > > > > > > configure_accelerator(current_machine); > > > > > > > > > > > > + if (cpu_model && is_help_option(cpu_model)) { > > > > > > + list_cpus(stdout, &fprintf, cpu_model); > > > > > > + exit(0); > > > > > > + } > > > > > > + > > > > > > if (qtest_chrdev) { > > > > > > qtest_init(qtest_chrdev, qtest_log, &error_fatal); > > > > > > } > > > > > > > > > > > > > > > > > > > > > > >
On Wed, Mar 08, 2017 at 01:33:45PM +1100, David Gibson wrote: > On Tue, Mar 07, 2017 at 09:02:37AM -0300, Eduardo Habkost wrote: > > On Tue, Mar 07, 2017 at 02:31:05PM +1100, David Gibson wrote: > > > On Mon, Mar 06, 2017 at 08:47:52AM -0300, Eduardo Habkost wrote: > > > > On Mon, Mar 06, 2017 at 10:06:27AM +1100, David Gibson wrote: > > > > > On Fri, Mar 03, 2017 at 11:58:07AM -0300, Eduardo Habkost wrote: > > > > > > On Tue, Jan 31, 2017 at 02:11:59PM +0100, Thomas Huth wrote: > > > > > > > When running with KVM on POWER, we register some CPU types during > > > > > > > the initialization function of the ppc64 KVM code (which unfortunately > > > > > > > also can not be done via a type_init() like it is done on x86). > > > > > > > > > > > > Can you elaborate why it can't be done via type_init()? If the > > > > > > QOM type hierarchy depends on any runtime data unavailable at > > > > > > type_init(), we should fix that. > > > > > > > > > > Hmm.. how? This is specifically for the special 'host' cpu in the > > > > > case of KVM. We can't use a static configuration here, because there > > > > > are things on the host that could limit what features of the CPU are > > > > > available for guest use. So, we need KVM to be initialized in order > > > > > to query that information. > > > > > > > > There's nothing saying you need to query that information before > > > > type_register() or during class_init, does it? The behavior of > > > > TYPE_HOST_POWERPC_CPU after object_new() is called can be as > > > > dynamic as you want it to, but the QOM type hierarchy is supposed > > > > to be static. > > > > > > So, the thing is we have some properties that logically belong in the > > > CPU class, rather than instance, and that's correct for all TCG CPUs, > > > but not for the KVM host CPU. It seems nasty to have to force all > > > those things into the instance just because of KVM. > > > > You can still register any class properties you want, without > > querying KVM first. Are there properties that you are able to > > register only after querying KVM? Is the set of class properties > > on TYPE_HOST_POWERPC_CPU different depending on the host > > capabilities? > > Sorry, I used "properties" sloppily, not meaning QOM properties. > > There are several fields in the CPU class which describe CPU > capabilities - bitmasks indicating which instructions are available, > and L1 cacheline sizes. There are some other things that are in the > CPU instance at the moment, but arguably would belong better in the > class: available page sizes and PTE encodings, for example. > > At the real hardware level these things are dependent only the model > of CPU - hence belonging in cpu class, not instance. But, because of > the way virtualization works on POWER, some of the features may not be > available to guests, due to configuration of the hypervisor. So for > the "host" cpu we need to query KVM to see which CPU features are > actually available. > I see. If there is data that is available only at PowerPCCPUClass and you don't want to duplicate it at PowerPCCPU, we can have a solution for that: instead of late-registration of the class, we could leave those fields to be populated after KVM is initialized. Anyway, I don't think this is urgent: the code already treats "host" as an exception in ppc_cpu_list(), and (AFAICS) the original problem this patch addresses is related to the inaccurate alias information only (which is not a consequence of the late type_register() call). > > I'm looking at target/ppc/cpu-models.c, and I only see the > > class_init function setting PowerPCCPUClass::pvr and > > PowerPCCPUClass::svr. Am I missing anything? > > Yes. Notice that the .parent set there is not the base powerpc cpu > class, but a "family" class. Those families are defined in > translate_init.c by another hairy macro POWERPC_FAMILY(). The family > class_init function (generally done as a block below the macro > invocation) initializes several other things, including insns_flags. > > kvmppc_host_cpu_class_init(), uses the family's copy of insns_flags as > a base, but needs to query the host and adjust it for a couple of > instruction groups that can be disabled from the hypervisor. > > [Aside: we also update the cache sizes based on host information. > The reasons for that are complicated. This particular case doesn't > require a late class init though, because although it's queried from > the host, it's not queried from kvm specifically] > > > You can still keep pvr and svr on the CPU class. The only thing > > that's different on TYPE_HOST_POWERPC_CPU is that it won't have > > the preset values on PowerPCCPUClass. But it can initialize the > > instance values on ->instance_init() or ->realize() instead. > > > > I even see ppc_cpu_class_by_pvr*() treating TYPE_HOST_POWERPC_CPU > > as special on PVR lookup, already. So it looks like the code is > > almost ready for that? > > PVR is a red herring. insn_flags is the real issue. > > > > > > > > > > Registering QOM types outside type_init() is only causing us > > > > problems and we should stop doing that. > > > > > > > > > > > > > > > > So to > > > > > > > be able to see these updates in the CPU help text, the code that calls > > > > > > > list_cpus() has to be run after configure_accelerator(). This move should > > > > > > > be fine since the "cpu_model" variable is also never used before the call > > > > > > > to configure_accelerator(), and thus there should not be any unwanted > > > > > > > side effects in the code before configure_accelerator() if the user > > > > > > > started QEMU with "-cpu ?" or "-cpu help". > > > > > > > > > > > > > > Signed-off-by: Thomas Huth <thuth@redhat.com> > > > > > > > > > > > > I am not convinced that the output of "-cpu help" and > > > > > > "-cpu help -machine accel=kvm" should look different. Do you have > > > > > > an example of what exactly is wrong with the output currently? > > > > > > > > > > > > I actually believe list_cpus() needs to be called _earlier_, not > > > > > > later. Otherwise we won't be able to fix this bug: > > > > > > > > > > > > $ qemu-system-arm -cpu help > > > > > > qemu-system-arm:/usr/local/etc/qemu/qemu.conf:1: No machine specified, and there is no default > > > > > > Use -machine help to list supported machines > > > > > > $ > > > > > > > > > > > > > --- > > > > > > > vl.c | 10 +++++----- > > > > > > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > > > > > > > > > > > diff --git a/vl.c b/vl.c > > > > > > > index 0b72b12..315c5c3 100644 > > > > > > > --- a/vl.c > > > > > > > +++ b/vl.c > > > > > > > @@ -4055,11 +4055,6 @@ int main(int argc, char **argv, char **envp) > > > > > > > qemu_set_hw_version(machine_class->hw_version); > > > > > > > } > > > > > > > > > > > > > > - if (cpu_model && is_help_option(cpu_model)) { > > > > > > > - list_cpus(stdout, &fprintf, cpu_model); > > > > > > > - exit(0); > > > > > > > - } > > > > > > > - > > > > > > > if (!trace_init_backends()) { > > > > > > > exit(1); > > > > > > > } > > > > > > > @@ -4298,6 +4293,11 @@ int main(int argc, char **argv, char **envp) > > > > > > > > > > > > > > configure_accelerator(current_machine); > > > > > > > > > > > > > > + if (cpu_model && is_help_option(cpu_model)) { > > > > > > > + list_cpus(stdout, &fprintf, cpu_model); > > > > > > > + exit(0); > > > > > > > + } > > > > > > > + > > > > > > > if (qtest_chrdev) { > > > > > > > qtest_init(qtest_chrdev, qtest_log, &error_fatal); > > > > > > > } > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > David Gibson | I'll have my music baroque, and my code > david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ > | _way_ _around_! > http://www.ozlabs.org/~dgibson
On 3 March 2017 at 15:58, Eduardo Habkost <ehabkost@redhat.com> wrote: > On Tue, Jan 31, 2017 at 02:11:59PM +0100, Thomas Huth wrote: >> When running with KVM on POWER, we register some CPU types during >> the initialization function of the ppc64 KVM code (which unfortunately >> also can not be done via a type_init() like it is done on x86). > > Can you elaborate why it can't be done via type_init()? If the > QOM type hierarchy depends on any runtime data unavailable at > type_init(), we should fix that. On ARM we (currently) have the KVM-only 'host' CPU type be added to the type hierarchy only at runtime in kvm_init(), but we deal with the '-help' problem by having arm_cpu_list() do #ifdef CONFIG_KVM /* The 'host' CPU type is dynamically registered only if KVM is * enabled, so we have to special-case it here: */ (*cpu_fprintf)(f, " host (only available in KVM mode)\n"); #endif thanks -- PMM
On Wed, Mar 08, 2017 at 03:31:01PM +0100, Peter Maydell wrote: > On 3 March 2017 at 15:58, Eduardo Habkost <ehabkost@redhat.com> wrote: > > On Tue, Jan 31, 2017 at 02:11:59PM +0100, Thomas Huth wrote: > >> When running with KVM on POWER, we register some CPU types during > >> the initialization function of the ppc64 KVM code (which unfortunately > >> also can not be done via a type_init() like it is done on x86). > > > > Can you elaborate why it can't be done via type_init()? If the > > QOM type hierarchy depends on any runtime data unavailable at > > type_init(), we should fix that. > > On ARM we (currently) have the KVM-only 'host' CPU type be > added to the type hierarchy only at runtime in kvm_init(), > but we deal with the '-help' problem by having arm_cpu_list() do > > #ifdef CONFIG_KVM > /* The 'host' CPU type is dynamically registered only if KVM is > * enabled, so we have to special-case it here: > */ > (*cpu_fprintf)(f, " host (only available in KVM mode)\n"); > #endif > We already have similar code at ppc_cpu_list(): #ifdef CONFIG_KVM cpu_fprintf(f, "\n"); cpu_fprintf(f, "PowerPC %-16s\n", "host"); #endif If I understood it correctly, the current problem is just inaccurate alias information being printed because the alias table is modified by the accelerator. My current suggestion is to avoid printing anything that depends on the current machine/accelerator at the help output.
On 08.03.2017 15:50, Eduardo Habkost wrote: > On Wed, Mar 08, 2017 at 03:31:01PM +0100, Peter Maydell wrote: >> On 3 March 2017 at 15:58, Eduardo Habkost <ehabkost@redhat.com> wrote: >>> On Tue, Jan 31, 2017 at 02:11:59PM +0100, Thomas Huth wrote: >>>> When running with KVM on POWER, we register some CPU types during >>>> the initialization function of the ppc64 KVM code (which unfortunately >>>> also can not be done via a type_init() like it is done on x86). >>> >>> Can you elaborate why it can't be done via type_init()? If the >>> QOM type hierarchy depends on any runtime data unavailable at >>> type_init(), we should fix that. >> >> On ARM we (currently) have the KVM-only 'host' CPU type be >> added to the type hierarchy only at runtime in kvm_init(), >> but we deal with the '-help' problem by having arm_cpu_list() do >> >> #ifdef CONFIG_KVM >> /* The 'host' CPU type is dynamically registered only if KVM is >> * enabled, so we have to special-case it here: >> */ >> (*cpu_fprintf)(f, " host (only available in KVM mode)\n"); >> #endif >> > > We already have similar code at ppc_cpu_list(): > > #ifdef CONFIG_KVM > cpu_fprintf(f, "\n"); > cpu_fprintf(f, "PowerPC %-16s\n", "host"); > #endif > > If I understood it correctly, the current problem is just > inaccurate alias information being printed because the alias > table is modified by the accelerator. Yes. > My current suggestion is to avoid printing anything that depends > on the current machine/accelerator at the help output. I'll have a look at that... Thomas
On Wed, Mar 08, 2017 at 09:08:50AM -0300, Eduardo Habkost wrote: > On Wed, Mar 08, 2017 at 01:33:45PM +1100, David Gibson wrote: > > On Tue, Mar 07, 2017 at 09:02:37AM -0300, Eduardo Habkost wrote: > > > On Tue, Mar 07, 2017 at 02:31:05PM +1100, David Gibson wrote: > > > > On Mon, Mar 06, 2017 at 08:47:52AM -0300, Eduardo Habkost wrote: > > > > > On Mon, Mar 06, 2017 at 10:06:27AM +1100, David Gibson wrote: > > > > > > On Fri, Mar 03, 2017 at 11:58:07AM -0300, Eduardo Habkost wrote: > > > > > > > On Tue, Jan 31, 2017 at 02:11:59PM +0100, Thomas Huth wrote: > > > > > > > > When running with KVM on POWER, we register some CPU types during > > > > > > > > the initialization function of the ppc64 KVM code (which unfortunately > > > > > > > > also can not be done via a type_init() like it is done on x86). > > > > > > > > > > > > > > Can you elaborate why it can't be done via type_init()? If the > > > > > > > QOM type hierarchy depends on any runtime data unavailable at > > > > > > > type_init(), we should fix that. > > > > > > > > > > > > Hmm.. how? This is specifically for the special 'host' cpu in the > > > > > > case of KVM. We can't use a static configuration here, because there > > > > > > are things on the host that could limit what features of the CPU are > > > > > > available for guest use. So, we need KVM to be initialized in order > > > > > > to query that information. > > > > > > > > > > There's nothing saying you need to query that information before > > > > > type_register() or during class_init, does it? The behavior of > > > > > TYPE_HOST_POWERPC_CPU after object_new() is called can be as > > > > > dynamic as you want it to, but the QOM type hierarchy is supposed > > > > > to be static. > > > > > > > > So, the thing is we have some properties that logically belong in the > > > > CPU class, rather than instance, and that's correct for all TCG CPUs, > > > > but not for the KVM host CPU. It seems nasty to have to force all > > > > those things into the instance just because of KVM. > > > > > > You can still register any class properties you want, without > > > querying KVM first. Are there properties that you are able to > > > register only after querying KVM? Is the set of class properties > > > on TYPE_HOST_POWERPC_CPU different depending on the host > > > capabilities? > > > > Sorry, I used "properties" sloppily, not meaning QOM properties. > > > > There are several fields in the CPU class which describe CPU > > capabilities - bitmasks indicating which instructions are available, > > and L1 cacheline sizes. There are some other things that are in the > > CPU instance at the moment, but arguably would belong better in the > > class: available page sizes and PTE encodings, for example. > > > > At the real hardware level these things are dependent only the model > > of CPU - hence belonging in cpu class, not instance. But, because of > > the way virtualization works on POWER, some of the features may not be > > available to guests, due to configuration of the hypervisor. So for > > the "host" cpu we need to query KVM to see which CPU features are > > actually available. > > > > I see. If there is data that is available only at PowerPCCPUClass > and you don't want to duplicate it at PowerPCCPU, we can have a > solution for that: instead of late-registration of the class, we > could leave those fields to be populated after KVM is > initialized. Ok, that sounds workable. > Anyway, I don't think this is urgent: the code already treats > "host" as an exception in ppc_cpu_list(), and (AFAICS) the > original problem this patch addresses is related to the > inaccurate alias information only (which is not a consequence of > the late type_register() call). I agree.
On Tue, 2017-03-07 at 10:02 +0100, Thomas Huth wrote: > The problem is that on POWER, we've got a "family" of CPUs with > different sub-types, e.g. for POWER8: > > $ qemu-system-ppc64 -cpu ? | grep POWER8 > PowerPC POWER8E_v2.1 PVR 004b0201 > PowerPC POWER8E (alias for POWER8E_v2.1) > PowerPC POWER8NVL_v1.0 PVR 004c0100 > PowerPC POWER8NVL (alias for POWER8NVL_v1.0) > PowerPC POWER8_v2.0 PVR 004d0200 > PowerPC POWER8 (alias for POWER8_v2.0) > > Most of the users don't know about the current subtype that they are > using, and just want to use "-cpu POWER8" - and for example we've also > got an agreement with the libvirt folks that they can always use "-cpu > POWER8" for any kind of POWER8 system, no matter whether the host is > using a POWER8E or POWER8NVL chip. Yup, thanks for bringing that up and keeping an eye out to make sure it keeps working :) > So the "POWER8" alias now gets updated internally in QEMU to the correct > host CPU type ... but the output of "-cpu help" is then still wrong. > I agree that it's kind of ugly to have different help texts depending on > whether "accel=kvm" has been used or not, but that sounds still better > to me than printing wrong information here. > Thinking about this again ... maybe it would be better if we'd rework > the help text to print out something like this instead: > > PowerPC POWER8 (alias for any POWER8 chip) > > ... so that we simply get rid of the version/subtype information here > completely? I was initially concerned about this, because I remembered libvirt parsing the string after "(alias for ", but I checked and it looks like we do so only for machine types, not for CPU models. So the change should be safe, and since we're using QMP to fetch CPU definitions these days, I don't expect we'll need to worry about this change even in the future. Still, did you try changing the description and using the resulting QEMU binary along with libvirt? -- Andrea Bolognani / Red Hat / Virtualization
diff --git a/vl.c b/vl.c index 0b72b12..315c5c3 100644 --- a/vl.c +++ b/vl.c @@ -4055,11 +4055,6 @@ int main(int argc, char **argv, char **envp) qemu_set_hw_version(machine_class->hw_version); } - if (cpu_model && is_help_option(cpu_model)) { - list_cpus(stdout, &fprintf, cpu_model); - exit(0); - } - if (!trace_init_backends()) { exit(1); } @@ -4298,6 +4293,11 @@ int main(int argc, char **argv, char **envp) configure_accelerator(current_machine); + if (cpu_model && is_help_option(cpu_model)) { + list_cpus(stdout, &fprintf, cpu_model); + exit(0); + } + if (qtest_chrdev) { qtest_init(qtest_chrdev, qtest_log, &error_fatal); }
When running with KVM on POWER, we register some CPU types during the initialization function of the ppc64 KVM code (which unfortunately also can not be done via a type_init() like it is done on x86). So to be able to see these updates in the CPU help text, the code that calls list_cpus() has to be run after configure_accelerator(). This move should be fine since the "cpu_model" variable is also never used before the call to configure_accelerator(), and thus there should not be any unwanted side effects in the code before configure_accelerator() if the user started QEMU with "-cpu ?" or "-cpu help". Signed-off-by: Thomas Huth <thuth@redhat.com> --- vl.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)