| Message ID | 20180725091233.3300-1-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1] s390x/cpu_models: Add "-cpu max" support | expand |
On Wed, 25 Jul 2018 11:12:33 +0200 David Hildenbrand <david@redhat.com> wrote: > The "max" CPU model behaves like "-cpu host" when KVM is enabled, and like > a CPU with the maximum possible feature set when TCG is enabled. > > While the "host" model can not be used under TCG ("kvm_required"), the > "max" model can and "Enables all features supported by the accelerator in > the current host". > > So we can treat "host" just as a special case of "max" (like x86 does). > It differs to the "qemu" CPU model under TCG such that compatibility > handling will not be performed and that some experimental CPU features > not yet part of the "qemu" model might be indicated. > > These are right now under TCG (see "qemu_MAX"): > - stfle53 That's a z13 feature, so I think it's fine as we don't care about machine generations for the max mode anyway, correct? > - msa5-base That's just the warning, but as things are continuing to work, it's fine as well. > - zpci That one theoretically has a dependency on CONFIG_PCI, but as that is always set, I think it's fine as well. > > This will result right now in the following warning when starting QEMU TCG > with the "max" model: > "qemu-system-s390x: warning: 'msa5-base' requires 'kimd-sha-512'." > > The "qemu" model (used as default in QEMU under TCG) will continue to > work without such warnings. The "max" mdel in the current form s/mdel/model/ > might be interesting for kvm-unit-tests (where we would e.g. now also > test "msa5-base"). > > The "max" model is neither static nor migration safe (like the "host" > model). It is independent of the machine but dependends on the accelerator. > It can be used to detect the maximum CPU model also under TCG from upper > layers without having to care about CPU model names for CPU model > expansion. > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > target/s390x/cpu_models.c | 81 +++++++++++++++++++++++++++------------ > 1 file changed, 56 insertions(+), 25 deletions(-) > > +static void s390_max_cpu_model_initfn(Object *obj) > +{ > + const S390CPUModel *max_model; > + S390CPU *cpu = S390_CPU(obj); > + Error *local_err = NULL; > + > + if (kvm_enabled() && !kvm_s390_cpu_models_supported()) { > + /* "max" and "host" always work, even without CPU model support */ > + return; > + } > + > + max_model = get_max_cpu_model(&local_err); > + if (local_err) { > + g_assert(kvm_enabled()); Maybe add a comment that for kvm we try the host model, and only that can fail (i.e., for tcg this will always work)? > + error_report_err(local_err); > + /* fallback to unsupported CPU models */ > + return; > + } > + > + cpu->model = g_new(S390CPUModel, 1); > + /* copy the CPU model so we can modify it */ > + memcpy(cpu->model, max_model, sizeof(*cpu->model)); > +} > + > static void s390_cpu_model_finalize(Object *obj) > { > S390CPU *cpu = S390_CPU(obj); Looks sane to me.
On 25.07.2018 13:36, Cornelia Huck wrote: > On Wed, 25 Jul 2018 11:12:33 +0200 > David Hildenbrand <david@redhat.com> wrote: > >> The "max" CPU model behaves like "-cpu host" when KVM is enabled, and like >> a CPU with the maximum possible feature set when TCG is enabled. >> >> While the "host" model can not be used under TCG ("kvm_required"), the >> "max" model can and "Enables all features supported by the accelerator in >> the current host". >> >> So we can treat "host" just as a special case of "max" (like x86 does). >> It differs to the "qemu" CPU model under TCG such that compatibility >> handling will not be performed and that some experimental CPU features >> not yet part of the "qemu" model might be indicated. >> >> These are right now under TCG (see "qemu_MAX"): >> - stfle53 > > That's a z13 feature, so I think it's fine as we don't care about > machine generations for the max mode anyway, correct? Yes the max model really just is "give me anything you got and that you can expand to a static model (i.e. fully specify on the command line)". > >> - msa5-base > > That's just the warning, but as things are continuing to work, it's > fine as well. Yes, as these are z13 models we don't but them yet into our "stable" qemu model which is based on a z12. > >> - zpci > > That one theoretically has a dependency on CONFIG_PCI, but as that is > always set, I think it's fine as well. That is even already handled correctly, see target/s390x/cpu_models.c:register_types() 1314 #ifndef CONFIG_USER_ONLY 1315 if (!pci_available) { 1316 clear_bit(S390_FEAT_ZPCI, qemu_max_cpu_feat); 1317 } 1318 #endif [...] > Maybe add a comment that for kvm we try the host model, and only that > can fail (i.e., for tcg this will always work)? "we expect only errors under KVM, when we actually query the kernel" > >> + error_report_err(local_err); >> + /* fallback to unsupported CPU models */ >> + return; >> + } >> + >> + cpu->model = g_new(S390CPUModel, 1); >> + /* copy the CPU model so we can modify it */ >> + memcpy(cpu->model, max_model, sizeof(*cpu->model)); >> +} >> + >> static void s390_cpu_model_finalize(Object *obj) >> { >> S390CPU *cpu = S390_CPU(obj); > > Looks sane to me. > Thanks!
On Wed, 25 Jul 2018 13:58:16 +0200 David Hildenbrand <david@redhat.com> wrote: > On 25.07.2018 13:36, Cornelia Huck wrote: > > On Wed, 25 Jul 2018 11:12:33 +0200 > > David Hildenbrand <david@redhat.com> wrote: > > > >> The "max" CPU model behaves like "-cpu host" when KVM is enabled, and like > >> a CPU with the maximum possible feature set when TCG is enabled. > >> > >> While the "host" model can not be used under TCG ("kvm_required"), the > >> "max" model can and "Enables all features supported by the accelerator in > >> the current host". > >> > >> So we can treat "host" just as a special case of "max" (like x86 does). > >> It differs to the "qemu" CPU model under TCG such that compatibility > >> handling will not be performed and that some experimental CPU features > >> not yet part of the "qemu" model might be indicated. > >> > >> These are right now under TCG (see "qemu_MAX"): > >> - stfle53 > > > > That's a z13 feature, so I think it's fine as we don't care about > > machine generations for the max mode anyway, correct? > > Yes the max model really just is "give me anything you got and that you > can expand to a static model (i.e. fully specify on the command line)". > > > > >> - msa5-base > > > > That's just the warning, but as things are continuing to work, it's > > fine as well. > > Yes, as these are z13 models we don't but them yet into our "stable" > qemu model which is based on a z12. > > > > >> - zpci > > > > That one theoretically has a dependency on CONFIG_PCI, but as that is > > always set, I think it's fine as well. > > That is even already handled correctly, see > target/s390x/cpu_models.c:register_types() > > 1314 #ifndef CONFIG_USER_ONLY > 1315 if (!pci_available) { > 1316 clear_bit(S390_FEAT_ZPCI, qemu_max_cpu_feat); > 1317 } > 1318 #endif Oh, I actually added that myself :) > > [...] > > > Maybe add a comment that for kvm we try the host model, and only that > > can fail (i.e., for tcg this will always work)? > > "we expect only errors under KVM, when we actually query the kernel" "We expect errors only under KVM, where we actually query the kernel" ? If nobody else has further comments, I can squash in the change and queue it for 3.1. I'll let it sit for a bit longer on the list, though. > > > > >> + error_report_err(local_err); > >> + /* fallback to unsupported CPU models */ > >> + return; > >> + } > >> + > >> + cpu->model = g_new(S390CPUModel, 1); > >> + /* copy the CPU model so we can modify it */ > >> + memcpy(cpu->model, max_model, sizeof(*cpu->model)); > >> +} > >> + > >> static void s390_cpu_model_finalize(Object *obj) > >> { > >> S390CPU *cpu = S390_CPU(obj); > > > > Looks sane to me. > > > > Thanks! >
>> >>> Maybe add a comment that for kvm we try the host model, and only that >>> can fail (i.e., for tcg this will always work)? >> >> "we expect only errors under KVM, when we actually query the kernel" > > "We expect errors only under KVM, where we actually query the kernel" > > ? Sure, maybe you can fit it into one line (which is what I tried). > > If nobody else has further comments, I can squash in the change and > queue it for 3.1. I'll let it sit for a bit longer on the list, though. If I don't have to resend, can you fixup the subject "s390x/cpumodel: ..." ? Thanks!
On Wed, 25 Jul 2018 14:49:45 +0200 David Hildenbrand <david@redhat.com> wrote: > >> > >>> Maybe add a comment that for kvm we try the host model, and only that > >>> can fail (i.e., for tcg this will always work)? > >> > >> "we expect only errors under KVM, when we actually query the kernel" > > > > "We expect errors only under KVM, where we actually query the kernel" > > > > ? > > Sure, maybe you can fit it into one line (which is what I tried). Let's see. > > > > > If nobody else has further comments, I can squash in the change and > > queue it for 3.1. I'll let it sit for a bit longer on the list, though. > > If I don't have to resend, can you fixup the subject "s390x/cpumodel: ..." ? Sure!
On Wed, Jul 25, 2018 at 11:12:33AM +0200, David Hildenbrand wrote: > The "max" CPU model behaves like "-cpu host" when KVM is enabled, and like > a CPU with the maximum possible feature set when TCG is enabled. > > While the "host" model can not be used under TCG ("kvm_required"), the > "max" model can and "Enables all features supported by the accelerator in > the current host". > > So we can treat "host" just as a special case of "max" (like x86 does). > It differs to the "qemu" CPU model under TCG such that compatibility > handling will not be performed and that some experimental CPU features > not yet part of the "qemu" model might be indicated. > > These are right now under TCG (see "qemu_MAX"): > - stfle53 > - msa5-base > - zpci > > This will result right now in the following warning when starting QEMU TCG > with the "max" model: > "qemu-system-s390x: warning: 'msa5-base' requires 'kimd-sha-512'." > > The "qemu" model (used as default in QEMU under TCG) will continue to > work without such warnings. The "max" mdel in the current form > might be interesting for kvm-unit-tests (where we would e.g. now also > test "msa5-base"). > > The "max" model is neither static nor migration safe (like the "host" > model). It is independent of the machine but dependends on the accelerator. > It can be used to detect the maximum CPU model also under TCG from upper > layers without having to care about CPU model names for CPU model > expansion. > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > target/s390x/cpu_models.c | 81 +++++++++++++++++++++++++++------------ > 1 file changed, 56 insertions(+), 25 deletions(-) > > diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c > index 604898a882..708bf0e3ba 100644 > --- a/target/s390x/cpu_models.c > +++ b/target/s390x/cpu_models.c > @@ -307,7 +307,10 @@ static gint s390_cpu_list_compare(gconstpointer a, gconstpointer b) > const char *name_a = object_class_get_name((ObjectClass *)a); > const char *name_b = object_class_get_name((ObjectClass *)b); > > - /* move qemu and host to the top of the list, qemu first, host second */ > + /* > + * Move qemu, host and max to the top of the list, qemu first, host second, > + * max third. > + */ > if (name_a[0] == 'q') { > return -1; > } else if (name_b[0] == 'q') { > @@ -316,6 +319,10 @@ static gint s390_cpu_list_compare(gconstpointer a, gconstpointer b) > return -1; > } else if (name_b[0] == 'h') { > return 1; > + } else if (name_a[0] == 'm') { > + return -1; > + } else if (name_b[0] == 'm') { > + return 1; > } Isn't it simpler to add a S390CPUClass::ordering field? See x86_cpu_list_compare() for an example. > > /* keep the same order we have in our table (sorted by release date) */ > @@ -1077,27 +1084,6 @@ static void s390_cpu_model_initfn(Object *obj) > } > } > > -#ifdef CONFIG_KVM > -static void s390_host_cpu_model_initfn(Object *obj) > -{ > - S390CPU *cpu = S390_CPU(obj); > - Error *err = NULL; > - > - if (!kvm_enabled() || !kvm_s390_cpu_models_supported()) { > - return; > - } > - > - cpu->model = g_malloc0(sizeof(*cpu->model)); > - kvm_s390_get_host_cpu_model(cpu->model, &err); > - if (err) { > - error_report_err(err); > - g_free(cpu->model); > - /* fallback to unsupported cpu models */ > - cpu->model = NULL; > - } > -} > -#endif > - > static S390CPUDef s390_qemu_cpu_def; > static S390CPUModel s390_qemu_cpu_model; > > @@ -1136,6 +1122,30 @@ static void s390_qemu_cpu_model_initfn(Object *obj) > memcpy(cpu->model, &s390_qemu_cpu_model, sizeof(*cpu->model)); > } > > +static void s390_max_cpu_model_initfn(Object *obj) > +{ > + const S390CPUModel *max_model; > + S390CPU *cpu = S390_CPU(obj); > + Error *local_err = NULL; > + > + if (kvm_enabled() && !kvm_s390_cpu_models_supported()) { > + /* "max" and "host" always work, even without CPU model support */ > + return; > + } What's the use case that requires this check to be here? What do you expect 'query-cpu-model-expansion model=max' to return if !kvm_s390_cpu_models_supported()? > + > + max_model = get_max_cpu_model(&local_err); I've just confirmed that get_max_cpu_model() is already ready to work with TCG. > + if (local_err) { > + g_assert(kvm_enabled()); > + error_report_err(local_err); > + /* fallback to unsupported CPU models */ > + return; What about moving this outside instance_init? On x86 we have a x86_cpu_expand_features() function to allow us to handle CPU model expansion errors more gracefully. None of my comments are about new code, but existing code from "host", so: Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > + } > + > + cpu->model = g_new(S390CPUModel, 1); > + /* copy the CPU model so we can modify it */ > + memcpy(cpu->model, max_model, sizeof(*cpu->model)); > +} > + > static void s390_cpu_model_finalize(Object *obj) > { > S390CPU *cpu = S390_CPU(obj); > @@ -1209,6 +1219,20 @@ static void s390_qemu_cpu_model_class_init(ObjectClass *oc, void *data) > qemu_hw_version()); > } > > +static void s390_max_cpu_model_class_init(ObjectClass *oc, void *data) > +{ > + S390CPUClass *xcc = S390_CPU_CLASS(oc); > + > + /* > + * The "max" model is neither static nor migration safe. Under KVM > + * it represents the "host" model. Under TCG it represents some kind of > + * "qemu" CPU model without compat handling and maybe with some additional > + * CPU features that are not yet unlocked in the "qemu" model. > + */ > + xcc->desc = > + "Enables all features supported by the accelerator in the current host"; > +} > + > /* Generate type name for a cpu model. Caller has to free the string. */ > static char *s390_cpu_type_name(const char *model_name) > { > @@ -1239,12 +1263,18 @@ static const TypeInfo qemu_s390_cpu_type_info = { > .class_init = s390_qemu_cpu_model_class_init, > }; > > +static const TypeInfo max_s390_cpu_type_info = { > + .name = S390_CPU_TYPE_NAME("max"), > + .parent = TYPE_S390_CPU, > + .instance_init = s390_max_cpu_model_initfn, > + .instance_finalize = s390_cpu_model_finalize, > + .class_init = s390_max_cpu_model_class_init, > +}; > + > #ifdef CONFIG_KVM > static const TypeInfo host_s390_cpu_type_info = { > .name = S390_CPU_TYPE_NAME("host"), > - .parent = TYPE_S390_CPU, > - .instance_init = s390_host_cpu_model_initfn, > - .instance_finalize = s390_cpu_model_finalize, > + .parent = S390_CPU_TYPE_NAME("max"), > .class_init = s390_host_cpu_model_class_init, > }; > #endif > @@ -1326,6 +1356,7 @@ static void register_types(void) > } > > type_register_static(&qemu_s390_cpu_type_info); > + type_register_static(&max_s390_cpu_type_info); > #ifdef CONFIG_KVM > type_register_static(&host_s390_cpu_type_info); > #endif > -- > 2.17.1 >
On 25.07.2018 19:09, Eduardo Habkost wrote: > On Wed, Jul 25, 2018 at 11:12:33AM +0200, David Hildenbrand wrote: >> The "max" CPU model behaves like "-cpu host" when KVM is enabled, and like >> a CPU with the maximum possible feature set when TCG is enabled. >> >> While the "host" model can not be used under TCG ("kvm_required"), the >> "max" model can and "Enables all features supported by the accelerator in >> the current host". >> >> So we can treat "host" just as a special case of "max" (like x86 does). >> It differs to the "qemu" CPU model under TCG such that compatibility >> handling will not be performed and that some experimental CPU features >> not yet part of the "qemu" model might be indicated. >> >> These are right now under TCG (see "qemu_MAX"): >> - stfle53 >> - msa5-base >> - zpci >> >> This will result right now in the following warning when starting QEMU TCG >> with the "max" model: >> "qemu-system-s390x: warning: 'msa5-base' requires 'kimd-sha-512'." >> >> The "qemu" model (used as default in QEMU under TCG) will continue to >> work without such warnings. The "max" mdel in the current form >> might be interesting for kvm-unit-tests (where we would e.g. now also >> test "msa5-base"). >> >> The "max" model is neither static nor migration safe (like the "host" >> model). It is independent of the machine but dependends on the accelerator. >> It can be used to detect the maximum CPU model also under TCG from upper >> layers without having to care about CPU model names for CPU model >> expansion. >> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> target/s390x/cpu_models.c | 81 +++++++++++++++++++++++++++------------ >> 1 file changed, 56 insertions(+), 25 deletions(-) >> >> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c >> index 604898a882..708bf0e3ba 100644 >> --- a/target/s390x/cpu_models.c >> +++ b/target/s390x/cpu_models.c >> @@ -307,7 +307,10 @@ static gint s390_cpu_list_compare(gconstpointer a, gconstpointer b) >> const char *name_a = object_class_get_name((ObjectClass *)a); >> const char *name_b = object_class_get_name((ObjectClass *)b); >> >> - /* move qemu and host to the top of the list, qemu first, host second */ >> + /* >> + * Move qemu, host and max to the top of the list, qemu first, host second, >> + * max third. >> + */ >> if (name_a[0] == 'q') { >> return -1; >> } else if (name_b[0] == 'q') { >> @@ -316,6 +319,10 @@ static gint s390_cpu_list_compare(gconstpointer a, gconstpointer b) >> return -1; >> } else if (name_b[0] == 'h') { >> return 1; >> + } else if (name_a[0] == 'm') { >> + return -1; >> + } else if (name_b[0] == 'm') { >> + return 1; >> } > > Isn't it simpler to add a S390CPUClass::ordering field? See > x86_cpu_list_compare() for an example. Not sure if it is simpler, this way the whole sorting logic is located at a single place. But I agree that if this list grows bigger, we might want to use a ordering field at least for the special cases (qemu/host/max/...) > > >> >> /* keep the same order we have in our table (sorted by release date) */ >> @@ -1077,27 +1084,6 @@ static void s390_cpu_model_initfn(Object *obj) >> } >> } >> >> -#ifdef CONFIG_KVM >> -static void s390_host_cpu_model_initfn(Object *obj) >> -{ >> - S390CPU *cpu = S390_CPU(obj); >> - Error *err = NULL; >> - >> - if (!kvm_enabled() || !kvm_s390_cpu_models_supported()) { >> - return; >> - } >> - >> - cpu->model = g_malloc0(sizeof(*cpu->model)); >> - kvm_s390_get_host_cpu_model(cpu->model, &err); >> - if (err) { >> - error_report_err(err); >> - g_free(cpu->model); >> - /* fallback to unsupported cpu models */ >> - cpu->model = NULL; >> - } >> -} >> -#endif >> - >> static S390CPUDef s390_qemu_cpu_def; >> static S390CPUModel s390_qemu_cpu_model; >> >> @@ -1136,6 +1122,30 @@ static void s390_qemu_cpu_model_initfn(Object *obj) >> memcpy(cpu->model, &s390_qemu_cpu_model, sizeof(*cpu->model)); >> } >> >> +static void s390_max_cpu_model_initfn(Object *obj) >> +{ >> + const S390CPUModel *max_model; >> + S390CPU *cpu = S390_CPU(obj); >> + Error *local_err = NULL; >> + >> + if (kvm_enabled() && !kvm_s390_cpu_models_supported()) { >> + /* "max" and "host" always work, even without CPU model support */ >> + return; >> + } > > What's the use case that requires this check to be here? > > What do you expect 'query-cpu-model-expansion model=max' to > return if !kvm_s390_cpu_models_supported()? The same as for "host". We have been handling the host model like this forever. (If we have no KVM interface, we have have basically no real idea on which CPU we are running, so we can't expand/baseline) cpu_model_from_info(): if (!cpu->model) { error_setg(errp, "Details about the host CPU model are not available, " "it cannot be used."); object_unref(obj); return; } We might want to tweak this error message later maybe (use the model name instead of "host", but as "max" maps to "host" under KVM, this is not of high priority) > > >> + >> + max_model = get_max_cpu_model(&local_err); > > I've just confirmed that get_max_cpu_model() is already ready to > work with TCG. Yes, it was used for detecting "runability" already also for TCG. > >> + if (local_err) { >> + g_assert(kvm_enabled()); >> + error_report_err(local_err); >> + /* fallback to unsupported CPU models */ >> + return; > > What about moving this outside instance_init? To which place for example? We at least have to copy the CPU model for each and every CPU instance (so we can modify it without side effects using properties). And if you look closely, 99% of this function will be exactly that (as the max model is cached internally, it will only be looked up once). > > On x86 we have a x86_cpu_expand_features() function to allow us > to handle CPU model expansion errors more gracefully. > > None of my comments are about new code, but existing code from > "host", so: > > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> Thanks!
On Wed, Jul 25, 2018 at 07:50:21PM +0200, David Hildenbrand wrote: > On 25.07.2018 19:09, Eduardo Habkost wrote: [...] > >> + if (local_err) { > >> + g_assert(kvm_enabled()); > >> + error_report_err(local_err); > >> + /* fallback to unsupported CPU models */ > >> + return; > > > > What about moving this outside instance_init? > > To which place for example? We at least have to copy the CPU model > for each and every CPU instance (so we can modify it without side > effects using properties). To any code that will look at cpu->model. You are wrapping an interface that needs to report errors (kvm_s390_get_host_cpu_model()) behind an interface that is not able to report errors (object_new()). There's nothing that requires you to do that, does it? You are free to provide an API that is actually able to report errors, instead of relying on object_new() only. But I understand that the QOM/qdev API doesn't make that job easy. On x86 we have X86CPU::max_features and X86CPU::user_features because of that.
On 25.07.2018 22:14, Eduardo Habkost wrote: > On Wed, Jul 25, 2018 at 07:50:21PM +0200, David Hildenbrand wrote: >> On 25.07.2018 19:09, Eduardo Habkost wrote: > [...] >>>> + if (local_err) { >>>> + g_assert(kvm_enabled()); >>>> + error_report_err(local_err); >>>> + /* fallback to unsupported CPU models */ >>>> + return; >>> >>> What about moving this outside instance_init? >> >> To which place for example? We at least have to copy the CPU model >> for each and every CPU instance (so we can modify it without side >> effects using properties). > > To any code that will look at cpu->model. > > You are wrapping an interface that needs to report errors > (kvm_s390_get_host_cpu_model()) behind an interface that is not > able to report errors (object_new()). There's nothing that > requires you to do that, does it? You are free to provide an API > that is actually able to report errors, instead of relying on > object_new() only. I see what you mean. One solution would be to preload and store the model somewhere globally (not locally). So in the init function, we would not have to handle errors. But I am not even sure where we could do such a global initialization + be able to report errors easily. I remember that we had a hard time to get this running smoothly due to the dependency of kvm_s390_get_host_cpu_model() on: - accelerator - machine - KVM init state And initializing cpu->model in realize() is too late, because all properties have to access it. Even a pre_plug handler will not work. On the other hand, I decided to ignore all errors back then and fallback to the "host CPU model unknown" case, because there are some corner cases where we still want to allow running the "host" model even though there was a problem detecting it. So my summary would be: We ignore errors (and rather treat them like warnings) for a reason here and fallback to "unsupported CPU models", which allows to run + use QEMU even in environments where our CPU model detection fails (e.g. on a very strange new CPU model we could have in the future). Especially "!cpu->model" does not imply that there was an error. It includes disabled CPU model support or unavailable CPU model support (KVM), which is perfectly fine. Replicating initialization attempts at all places where we access "cpu->model" does therefore not sound 100% clean to me and most likely makes the code way more complicated. Right now the semantics are clear: if we have "!cpu->model" after the object has been created, details about the host CPU model are not available (models unavailable/unsupported). Modifying properties, baselining, expanding is not possible with that model then. But it can be used for execution. > > But I understand that the QOM/qdev API doesn't make that job > easy. On x86 we have X86CPU::max_features and > X86CPU::user_features because of that. >
On Thu, Jul 26, 2018 at 09:29:44AM +0200, David Hildenbrand wrote: > On 25.07.2018 22:14, Eduardo Habkost wrote: > > On Wed, Jul 25, 2018 at 07:50:21PM +0200, David Hildenbrand wrote: > >> On 25.07.2018 19:09, Eduardo Habkost wrote: > > [...] > >>>> + if (local_err) { > >>>> + g_assert(kvm_enabled()); > >>>> + error_report_err(local_err); > >>>> + /* fallback to unsupported CPU models */ > >>>> + return; > >>> > >>> What about moving this outside instance_init? > >> > >> To which place for example? We at least have to copy the CPU model > >> for each and every CPU instance (so we can modify it without side > >> effects using properties). > > > > To any code that will look at cpu->model. > > > > You are wrapping an interface that needs to report errors > > (kvm_s390_get_host_cpu_model()) behind an interface that is not > > able to report errors (object_new()). There's nothing that > > requires you to do that, does it? You are free to provide an API > > that is actually able to report errors, instead of relying on > > object_new() only. > > I see what you mean. One solution would be to preload and store the > model somewhere globally (not locally). So in the init function, we > would not have to handle errors. > > But I am not even sure where we could do such a global initialization + > be able to report errors easily. I remember that we had a hard time to > get this running smoothly due to the dependency of > kvm_s390_get_host_cpu_model() on: > - accelerator > - machine > - KVM init state If we had a S390KVMAccelerator object on machine->accelerator, S390KVMAccelerator::host_model would be a good candidate? > > And initializing cpu->model in realize() is too late, because all > properties have to access it. Even a pre_plug handler will not work. Yeah, the instance_init/realize abstraction seems insufficient here. instance_init has too many restrictions, realize is too late. > > On the other hand, I decided to ignore all errors back then and fallback > to the "host CPU model unknown" case, because there are some corner > cases where we still want to allow running the "host" model even though > there was a problem detecting it. > > So my summary would be: We ignore errors (and rather treat them like > warnings) for a reason here and fallback to "unsupported CPU models", > which allows to run + use QEMU even in environments where our CPU model > detection fails (e.g. on a very strange new CPU model we could have in > the future). > > Especially "!cpu->model" does not imply that there was an error. It > includes disabled CPU model support or unavailable CPU model support > (KVM), which is perfectly fine. Replicating initialization attempts at > all places where we access "cpu->model" does therefore not sound 100% > clean to me and most likely makes the code way more complicated. > > Right now the semantics are clear: if we have "!cpu->model" after the > object has been created, details about the host CPU model are not > available (models unavailable/unsupported). Modifying properties, > baselining, expanding is not possible with that model then. But it can > be used for execution. This is interesting. If most users of cpu->model don't care about kvm_s390_get_host_cpu_model() errors at all, the current solution sounds more reasonable. Except for the error_report_err() call inside instance_init. This still bothers me, but it's not a big deal.
On Wed, 25 Jul 2018 11:12:33 +0200 David Hildenbrand <david@redhat.com> wrote: > The "max" CPU model behaves like "-cpu host" when KVM is enabled, and like > a CPU with the maximum possible feature set when TCG is enabled. > > While the "host" model can not be used under TCG ("kvm_required"), the > "max" model can and "Enables all features supported by the accelerator in > the current host". > > So we can treat "host" just as a special case of "max" (like x86 does). > It differs to the "qemu" CPU model under TCG such that compatibility > handling will not be performed and that some experimental CPU features > not yet part of the "qemu" model might be indicated. > > These are right now under TCG (see "qemu_MAX"): > - stfle53 > - msa5-base > - zpci > > This will result right now in the following warning when starting QEMU TCG > with the "max" model: > "qemu-system-s390x: warning: 'msa5-base' requires 'kimd-sha-512'." > > The "qemu" model (used as default in QEMU under TCG) will continue to > work without such warnings. The "max" mdel in the current form > might be interesting for kvm-unit-tests (where we would e.g. now also > test "msa5-base"). > > The "max" model is neither static nor migration safe (like the "host" > model). It is independent of the machine but dependends on the accelerator. > It can be used to detect the maximum CPU model also under TCG from upper > layers without having to care about CPU model names for CPU model > expansion. > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > target/s390x/cpu_models.c | 81 +++++++++++++++++++++++++++------------ > 1 file changed, 56 insertions(+), 25 deletions(-) So, what's the outcome? Can I merge this with the discussed minor edits, or should I wait for a v2?
> If we had a S390KVMAccelerator object on machine->accelerator, > S390KVMAccelerator::host_model would be a good candidate? Depends if at that point the machine would already be initialized (we disable CPU model support for KVM on some legacy machine due to interactions). It's complicated :) [...] >> Right now the semantics are clear: if we have "!cpu->model" after the >> object has been created, details about the host CPU model are not >> available (models unavailable/unsupported). Modifying properties, >> baselining, expanding is not possible with that model then. But it can >> be used for execution. > > This is interesting. If most users of cpu->model don't care > about kvm_s390_get_host_cpu_model() errors at all, the current > solution sounds more reasonable. > > Except for the error_report_err() call inside instance_init. > This still bothers me, but it's not a big deal. Yes, we should refactor that. I'll add this to my TODO list!
On 27.07.2018 14:55, Cornelia Huck wrote: > On Wed, 25 Jul 2018 11:12:33 +0200 > David Hildenbrand <david@redhat.com> wrote: > >> The "max" CPU model behaves like "-cpu host" when KVM is enabled, and like >> a CPU with the maximum possible feature set when TCG is enabled. >> >> While the "host" model can not be used under TCG ("kvm_required"), the >> "max" model can and "Enables all features supported by the accelerator in >> the current host". >> >> So we can treat "host" just as a special case of "max" (like x86 does). >> It differs to the "qemu" CPU model under TCG such that compatibility >> handling will not be performed and that some experimental CPU features >> not yet part of the "qemu" model might be indicated. >> >> These are right now under TCG (see "qemu_MAX"): >> - stfle53 >> - msa5-base >> - zpci >> >> This will result right now in the following warning when starting QEMU TCG >> with the "max" model: >> "qemu-system-s390x: warning: 'msa5-base' requires 'kimd-sha-512'." >> >> The "qemu" model (used as default in QEMU under TCG) will continue to >> work without such warnings. The "max" mdel in the current form >> might be interesting for kvm-unit-tests (where we would e.g. now also >> test "msa5-base"). >> >> The "max" model is neither static nor migration safe (like the "host" >> model). It is independent of the machine but dependends on the accelerator. >> It can be used to detect the maximum CPU model also under TCG from upper >> layers without having to care about CPU model names for CPU model >> expansion. >> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> target/s390x/cpu_models.c | 81 +++++++++++++++++++++++++++------------ >> 1 file changed, 56 insertions(+), 25 deletions(-) > > So, what's the outcome? Can I merge this with the discussed minor > edits, or should I wait for a v2? > Eduardo identified possible optimizations independent of this patch, so we should be good to go. @Eduardo, please correct me if I'm wrong!
On Mon, Jul 30, 2018 at 11:16:38AM +0200, David Hildenbrand wrote: > On 27.07.2018 14:55, Cornelia Huck wrote: > > On Wed, 25 Jul 2018 11:12:33 +0200 > > David Hildenbrand <david@redhat.com> wrote: > > > >> The "max" CPU model behaves like "-cpu host" when KVM is enabled, and like > >> a CPU with the maximum possible feature set when TCG is enabled. > >> > >> While the "host" model can not be used under TCG ("kvm_required"), the > >> "max" model can and "Enables all features supported by the accelerator in > >> the current host". > >> > >> So we can treat "host" just as a special case of "max" (like x86 does). > >> It differs to the "qemu" CPU model under TCG such that compatibility > >> handling will not be performed and that some experimental CPU features > >> not yet part of the "qemu" model might be indicated. > >> > >> These are right now under TCG (see "qemu_MAX"): > >> - stfle53 > >> - msa5-base > >> - zpci > >> > >> This will result right now in the following warning when starting QEMU TCG > >> with the "max" model: > >> "qemu-system-s390x: warning: 'msa5-base' requires 'kimd-sha-512'." > >> > >> The "qemu" model (used as default in QEMU under TCG) will continue to > >> work without such warnings. The "max" mdel in the current form > >> might be interesting for kvm-unit-tests (where we would e.g. now also > >> test "msa5-base"). > >> > >> The "max" model is neither static nor migration safe (like the "host" > >> model). It is independent of the machine but dependends on the accelerator. > >> It can be used to detect the maximum CPU model also under TCG from upper > >> layers without having to care about CPU model names for CPU model > >> expansion. > >> > >> Signed-off-by: David Hildenbrand <david@redhat.com> > >> --- > >> target/s390x/cpu_models.c | 81 +++++++++++++++++++++++++++------------ > >> 1 file changed, 56 insertions(+), 25 deletions(-) > > > > So, what's the outcome? Can I merge this with the discussed minor > > edits, or should I wait for a v2? > > > > Eduardo identified possible optimizations independent of this patch, so > we should be good to go. @Eduardo, please correct me if I'm wrong! This version still looks good to me, my Reviewed-by line still applies. Thanks!
On Wed, 25 Jul 2018 11:12:33 +0200 David Hildenbrand <david@redhat.com> wrote: > The "max" CPU model behaves like "-cpu host" when KVM is enabled, and like > a CPU with the maximum possible feature set when TCG is enabled. > > While the "host" model can not be used under TCG ("kvm_required"), the > "max" model can and "Enables all features supported by the accelerator in > the current host". > > So we can treat "host" just as a special case of "max" (like x86 does). > It differs to the "qemu" CPU model under TCG such that compatibility > handling will not be performed and that some experimental CPU features > not yet part of the "qemu" model might be indicated. > > These are right now under TCG (see "qemu_MAX"): > - stfle53 > - msa5-base > - zpci > > This will result right now in the following warning when starting QEMU TCG > with the "max" model: > "qemu-system-s390x: warning: 'msa5-base' requires 'kimd-sha-512'." > > The "qemu" model (used as default in QEMU under TCG) will continue to > work without such warnings. The "max" mdel in the current form > might be interesting for kvm-unit-tests (where we would e.g. now also > test "msa5-base"). > > The "max" model is neither static nor migration safe (like the "host" > model). It is independent of the machine but dependends on the accelerator. > It can be used to detect the maximum CPU model also under TCG from upper > layers without having to care about CPU model names for CPU model > expansion. > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > target/s390x/cpu_models.c | 81 +++++++++++++++++++++++++++------------ > 1 file changed, 56 insertions(+), 25 deletions(-) Thanks, applied.
diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c index 604898a882..708bf0e3ba 100644 --- a/target/s390x/cpu_models.c +++ b/target/s390x/cpu_models.c @@ -307,7 +307,10 @@ static gint s390_cpu_list_compare(gconstpointer a, gconstpointer b) const char *name_a = object_class_get_name((ObjectClass *)a); const char *name_b = object_class_get_name((ObjectClass *)b); - /* move qemu and host to the top of the list, qemu first, host second */ + /* + * Move qemu, host and max to the top of the list, qemu first, host second, + * max third. + */ if (name_a[0] == 'q') { return -1; } else if (name_b[0] == 'q') { @@ -316,6 +319,10 @@ static gint s390_cpu_list_compare(gconstpointer a, gconstpointer b) return -1; } else if (name_b[0] == 'h') { return 1; + } else if (name_a[0] == 'm') { + return -1; + } else if (name_b[0] == 'm') { + return 1; } /* keep the same order we have in our table (sorted by release date) */ @@ -1077,27 +1084,6 @@ static void s390_cpu_model_initfn(Object *obj) } } -#ifdef CONFIG_KVM -static void s390_host_cpu_model_initfn(Object *obj) -{ - S390CPU *cpu = S390_CPU(obj); - Error *err = NULL; - - if (!kvm_enabled() || !kvm_s390_cpu_models_supported()) { - return; - } - - cpu->model = g_malloc0(sizeof(*cpu->model)); - kvm_s390_get_host_cpu_model(cpu->model, &err); - if (err) { - error_report_err(err); - g_free(cpu->model); - /* fallback to unsupported cpu models */ - cpu->model = NULL; - } -} -#endif - static S390CPUDef s390_qemu_cpu_def; static S390CPUModel s390_qemu_cpu_model; @@ -1136,6 +1122,30 @@ static void s390_qemu_cpu_model_initfn(Object *obj) memcpy(cpu->model, &s390_qemu_cpu_model, sizeof(*cpu->model)); } +static void s390_max_cpu_model_initfn(Object *obj) +{ + const S390CPUModel *max_model; + S390CPU *cpu = S390_CPU(obj); + Error *local_err = NULL; + + if (kvm_enabled() && !kvm_s390_cpu_models_supported()) { + /* "max" and "host" always work, even without CPU model support */ + return; + } + + max_model = get_max_cpu_model(&local_err); + if (local_err) { + g_assert(kvm_enabled()); + error_report_err(local_err); + /* fallback to unsupported CPU models */ + return; + } + + cpu->model = g_new(S390CPUModel, 1); + /* copy the CPU model so we can modify it */ + memcpy(cpu->model, max_model, sizeof(*cpu->model)); +} + static void s390_cpu_model_finalize(Object *obj) { S390CPU *cpu = S390_CPU(obj); @@ -1209,6 +1219,20 @@ static void s390_qemu_cpu_model_class_init(ObjectClass *oc, void *data) qemu_hw_version()); } +static void s390_max_cpu_model_class_init(ObjectClass *oc, void *data) +{ + S390CPUClass *xcc = S390_CPU_CLASS(oc); + + /* + * The "max" model is neither static nor migration safe. Under KVM + * it represents the "host" model. Under TCG it represents some kind of + * "qemu" CPU model without compat handling and maybe with some additional + * CPU features that are not yet unlocked in the "qemu" model. + */ + xcc->desc = + "Enables all features supported by the accelerator in the current host"; +} + /* Generate type name for a cpu model. Caller has to free the string. */ static char *s390_cpu_type_name(const char *model_name) { @@ -1239,12 +1263,18 @@ static const TypeInfo qemu_s390_cpu_type_info = { .class_init = s390_qemu_cpu_model_class_init, }; +static const TypeInfo max_s390_cpu_type_info = { + .name = S390_CPU_TYPE_NAME("max"), + .parent = TYPE_S390_CPU, + .instance_init = s390_max_cpu_model_initfn, + .instance_finalize = s390_cpu_model_finalize, + .class_init = s390_max_cpu_model_class_init, +}; + #ifdef CONFIG_KVM static const TypeInfo host_s390_cpu_type_info = { .name = S390_CPU_TYPE_NAME("host"), - .parent = TYPE_S390_CPU, - .instance_init = s390_host_cpu_model_initfn, - .instance_finalize = s390_cpu_model_finalize, + .parent = S390_CPU_TYPE_NAME("max"), .class_init = s390_host_cpu_model_class_init, }; #endif @@ -1326,6 +1356,7 @@ static void register_types(void) } type_register_static(&qemu_s390_cpu_type_info); + type_register_static(&max_s390_cpu_type_info); #ifdef CONFIG_KVM type_register_static(&host_s390_cpu_type_info); #endif
The "max" CPU model behaves like "-cpu host" when KVM is enabled, and like a CPU with the maximum possible feature set when TCG is enabled. While the "host" model can not be used under TCG ("kvm_required"), the "max" model can and "Enables all features supported by the accelerator in the current host". So we can treat "host" just as a special case of "max" (like x86 does). It differs to the "qemu" CPU model under TCG such that compatibility handling will not be performed and that some experimental CPU features not yet part of the "qemu" model might be indicated. These are right now under TCG (see "qemu_MAX"): - stfle53 - msa5-base - zpci This will result right now in the following warning when starting QEMU TCG with the "max" model: "qemu-system-s390x: warning: 'msa5-base' requires 'kimd-sha-512'." The "qemu" model (used as default in QEMU under TCG) will continue to work without such warnings. The "max" mdel in the current form might be interesting for kvm-unit-tests (where we would e.g. now also test "msa5-base"). The "max" model is neither static nor migration safe (like the "host" model). It is independent of the machine but dependends on the accelerator. It can be used to detect the maximum CPU model also under TCG from upper layers without having to care about CPU model names for CPU model expansion. Signed-off-by: David Hildenbrand <david@redhat.com> --- target/s390x/cpu_models.c | 81 +++++++++++++++++++++++++++------------ 1 file changed, 56 insertions(+), 25 deletions(-)