| Message ID | 20240419065712.1225038-4-zhao1.liu@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390x/cpu_models: Misc cleanup on returned error code and local @err variables | expand |
On 19/4/24 08:57, Zhao Liu wrote: > From: Zhao Liu <zhao1.liu@intel.com> > > As error.h suggested, the best practice for callee is to return > something to indicate success / failure. > > So make kvm_s390_get_host_cpu_model() return boolean and check the > returned boolean in get_max_cpu_model() instead of accessing @err. > > Additionally, since now get_max_cpu_model() returns directly if > kvm_s390_get_host_cpu_model() fills @err, so make > kvm_s390_get_host_cpu_model() return true by default for the non-KVM > case in target/s390x/cpu_models.h. > > Signed-off-by: Zhao Liu <zhao1.liu@intel.com> > --- > target/s390x/cpu_models.c | 9 ++++----- > target/s390x/cpu_models.h | 5 +++-- > target/s390x/kvm/kvm.c | 13 +++++++------ > 3 files changed, 14 insertions(+), 13 deletions(-) > > diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c > index 052540a866ac..a0e4acb707d7 100644 > --- a/target/s390x/cpu_models.c > +++ b/target/s390x/cpu_models.c > @@ -560,16 +560,15 @@ S390CPUModel *get_max_cpu_model(Error **errp) > } > > if (kvm_enabled()) { Nitpicking, we could move @err declaration here: Error *err = NULL; > - kvm_s390_get_host_cpu_model(&max_model, &err); > + if (!kvm_s390_get_host_cpu_model(&max_model, &err)) { > + error_propagate(errp, err); > + return NULL; > + } > } else { > max_model.def = s390_find_cpu_def(QEMU_MAX_CPU_TYPE, QEMU_MAX_CPU_GEN, > QEMU_MAX_CPU_EC_GA, NULL); > bitmap_copy(max_model.features, qemu_max_cpu_feat, S390_FEAT_MAX); > } > - if (err) { > - error_propagate(errp, err); > - return NULL; > - } > cached = true; > return &max_model; > }
> > diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c > > index 052540a866ac..a0e4acb707d7 100644 > > --- a/target/s390x/cpu_models.c > > +++ b/target/s390x/cpu_models.c > > @@ -560,16 +560,15 @@ S390CPUModel *get_max_cpu_model(Error **errp) > > } > > if (kvm_enabled()) { > > Nitpicking, we could move @err declaration here: > > Error *err = NULL; Yeah! Next patch removes this variable completely, so I think it's OK not to move. ;-) Thanks, Zhao > > - kvm_s390_get_host_cpu_model(&max_model, &err); > > + if (!kvm_s390_get_host_cpu_model(&max_model, &err)) { > > + error_propagate(errp, err); > > + return NULL; > > + } > > } else { > > max_model.def = s390_find_cpu_def(QEMU_MAX_CPU_TYPE, QEMU_MAX_CPU_GEN, > > QEMU_MAX_CPU_EC_GA, NULL); > > bitmap_copy(max_model.features, qemu_max_cpu_feat, S390_FEAT_MAX); > > } > > - if (err) { > > - error_propagate(errp, err); > > - return NULL; > > - } > > cached = true; > > return &max_model; > > } >
On 19/04/2024 08.57, Zhao Liu wrote: > From: Zhao Liu <zhao1.liu@intel.com> > > As error.h suggested, the best practice for callee is to return > something to indicate success / failure. > > So make kvm_s390_get_host_cpu_model() return boolean and check the > returned boolean in get_max_cpu_model() instead of accessing @err. > > Additionally, since now get_max_cpu_model() returns directly if > kvm_s390_get_host_cpu_model() fills @err, so make > kvm_s390_get_host_cpu_model() return true by default for the non-KVM > case in target/s390x/cpu_models.h. You could also argue the other way round that there should be something in *model if it returns "true" ... anyway, the stub should never be executed, so it likely doesn't matter too much, but I'd still prefer if we'd rather return "false" in the non-KVM stub instead. > index d7b89129891a..5041c1e10fed 100644 > --- a/target/s390x/cpu_models.h > +++ b/target/s390x/cpu_models.h > @@ -116,12 +116,13 @@ S390CPUDef const *s390_find_cpu_def(uint16_t type, uint8_t gen, uint8_t ec_ga, > > #ifdef CONFIG_KVM > bool kvm_s390_cpu_models_supported(void); > -void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp); > +bool kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp); > void kvm_s390_apply_cpu_model(const S390CPUModel *model, Error **errp); > #else > -static inline void kvm_s390_get_host_cpu_model(S390CPUModel *model, > +static inline bool kvm_s390_get_host_cpu_model(S390CPUModel *model, > Error **errp) > { > + return true; > } Thomas
Hi Thomas, On Fri, Apr 19, 2024 at 09:50:46AM +0200, Thomas Huth wrote: > Date: Fri, 19 Apr 2024 09:50:46 +0200 > From: Thomas Huth <thuth@redhat.com> > Subject: Re: [PATCH 3/6] target/s390x/cpu_models: Make > kvm_s390_get_host_cpu_model() return boolean > > On 19/04/2024 08.57, Zhao Liu wrote: > > From: Zhao Liu <zhao1.liu@intel.com> > > > > As error.h suggested, the best practice for callee is to return > > something to indicate success / failure. > > > > So make kvm_s390_get_host_cpu_model() return boolean and check the > > returned boolean in get_max_cpu_model() instead of accessing @err. > > > > Additionally, since now get_max_cpu_model() returns directly if > > kvm_s390_get_host_cpu_model() fills @err, so make > > kvm_s390_get_host_cpu_model() return true by default for the non-KVM > > case in target/s390x/cpu_models.h. > > You could also argue the other way round that there should be something in > *model if it returns "true" ... anyway, the stub should never be executed, > so it likely doesn't matter too much, but I'd still prefer if we'd rather > return "false" in the non-KVM stub instead. I see, since this interface in wrapped in kvm_enabled() condition, so the non-kvm sutb wouldn't be called. Thanks! Will change to return false. Regards, Zhao
On 19/4/24 10:44, Zhao Liu wrote: > Hi Thomas, > > On Fri, Apr 19, 2024 at 09:50:46AM +0200, Thomas Huth wrote: >> Date: Fri, 19 Apr 2024 09:50:46 +0200 >> From: Thomas Huth <thuth@redhat.com> >> Subject: Re: [PATCH 3/6] target/s390x/cpu_models: Make >> kvm_s390_get_host_cpu_model() return boolean >> >> On 19/04/2024 08.57, Zhao Liu wrote: >>> From: Zhao Liu <zhao1.liu@intel.com> >>> >>> As error.h suggested, the best practice for callee is to return >>> something to indicate success / failure. >>> >>> So make kvm_s390_get_host_cpu_model() return boolean and check the >>> returned boolean in get_max_cpu_model() instead of accessing @err. >>> >>> Additionally, since now get_max_cpu_model() returns directly if >>> kvm_s390_get_host_cpu_model() fills @err, so make >>> kvm_s390_get_host_cpu_model() return true by default for the non-KVM >>> case in target/s390x/cpu_models.h. >> >> You could also argue the other way round that there should be something in >> *model if it returns "true" ... anyway, the stub should never be executed, >> so it likely doesn't matter too much, but I'd still prefer if we'd rather >> return "false" in the non-KVM stub instead. > > I see, since this interface in wrapped in kvm_enabled() condition, so > the non-kvm sutb wouldn't be called. > > Thanks! Will change to return false. Or try to rebase your series on this untested patch: https://lore.kernel.org/qemu-devel/20240419090631.48055-1-philmd@linaro.org/
On Fri, Apr 19, 2024 at 11:08:22AM +0200, Philippe Mathieu-Daudé wrote: > Date: Fri, 19 Apr 2024 11:08:22 +0200 > From: Philippe Mathieu-Daudé <philmd@linaro.org> > Subject: Re: [PATCH 3/6] target/s390x/cpu_models: Make > kvm_s390_get_host_cpu_model() return boolean > > On 19/4/24 10:44, Zhao Liu wrote: > > Hi Thomas, > > > > On Fri, Apr 19, 2024 at 09:50:46AM +0200, Thomas Huth wrote: > > > Date: Fri, 19 Apr 2024 09:50:46 +0200 > > > From: Thomas Huth <thuth@redhat.com> > > > Subject: Re: [PATCH 3/6] target/s390x/cpu_models: Make > > > kvm_s390_get_host_cpu_model() return boolean > > > > > > On 19/04/2024 08.57, Zhao Liu wrote: > > > > From: Zhao Liu <zhao1.liu@intel.com> > > > > > > > > As error.h suggested, the best practice for callee is to return > > > > something to indicate success / failure. > > > > > > > > So make kvm_s390_get_host_cpu_model() return boolean and check the > > > > returned boolean in get_max_cpu_model() instead of accessing @err. > > > > > > > > Additionally, since now get_max_cpu_model() returns directly if > > > > kvm_s390_get_host_cpu_model() fills @err, so make > > > > kvm_s390_get_host_cpu_model() return true by default for the non-KVM > > > > case in target/s390x/cpu_models.h. > > > > > > You could also argue the other way round that there should be something in > > > *model if it returns "true" ... anyway, the stub should never be executed, > > > so it likely doesn't matter too much, but I'd still prefer if we'd rather > > > return "false" in the non-KVM stub instead. > > > > I see, since this interface in wrapped in kvm_enabled() condition, so > > the non-kvm sutb wouldn't be called. > > > > Thanks! Will change to return false. > > Or try to rebase your series on this untested patch: > https://lore.kernel.org/qemu-devel/20240419090631.48055-1-philmd@linaro.org/ > Good, pls let me pick this patch into my v2.
diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c index 052540a866ac..a0e4acb707d7 100644 --- a/target/s390x/cpu_models.c +++ b/target/s390x/cpu_models.c @@ -560,16 +560,15 @@ S390CPUModel *get_max_cpu_model(Error **errp) } if (kvm_enabled()) { - kvm_s390_get_host_cpu_model(&max_model, &err); + if (!kvm_s390_get_host_cpu_model(&max_model, &err)) { + error_propagate(errp, err); + return NULL; + } } else { max_model.def = s390_find_cpu_def(QEMU_MAX_CPU_TYPE, QEMU_MAX_CPU_GEN, QEMU_MAX_CPU_EC_GA, NULL); bitmap_copy(max_model.features, qemu_max_cpu_feat, S390_FEAT_MAX); } - if (err) { - error_propagate(errp, err); - return NULL; - } cached = true; return &max_model; } diff --git a/target/s390x/cpu_models.h b/target/s390x/cpu_models.h index d7b89129891a..5041c1e10fed 100644 --- a/target/s390x/cpu_models.h +++ b/target/s390x/cpu_models.h @@ -116,12 +116,13 @@ S390CPUDef const *s390_find_cpu_def(uint16_t type, uint8_t gen, uint8_t ec_ga, #ifdef CONFIG_KVM bool kvm_s390_cpu_models_supported(void); -void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp); +bool kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp); void kvm_s390_apply_cpu_model(const S390CPUModel *model, Error **errp); #else -static inline void kvm_s390_get_host_cpu_model(S390CPUModel *model, +static inline bool kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp) { + return true; } static inline void kvm_s390_apply_cpu_model(const S390CPUModel *model, Error **errp) diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c index 4ce809c5d46b..57937b4ddbef 100644 --- a/target/s390x/kvm/kvm.c +++ b/target/s390x/kvm/kvm.c @@ -2375,7 +2375,7 @@ bool kvm_s390_cpu_models_supported(void) KVM_S390_VM_CPU_MACHINE_SUBFUNC); } -void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp) +bool kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp) { struct kvm_s390_vm_cpu_machine prop = {}; struct kvm_device_attr attr = { @@ -2390,14 +2390,14 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp) if (!kvm_s390_cpu_models_supported()) { error_setg(errp, "KVM doesn't support CPU models"); - return; + return false; } /* query the basic cpu model properties */ rc = kvm_vm_ioctl(kvm_state, KVM_GET_DEVICE_ATTR, &attr); if (rc) { error_setg(errp, "KVM: Error querying host CPU model: %d", rc); - return; + return false; } cpu_type = cpuid_type(prop.cpuid); @@ -2420,13 +2420,13 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp) rc = query_cpu_feat(model->features); if (rc) { error_setg(errp, "KVM: Error querying CPU features: %d", rc); - return; + return false; } /* get supported cpu subfunctions indicated via query / test bit */ rc = query_cpu_subfunc(model->features); if (rc) { error_setg(errp, "KVM: Error querying CPU subfunctions: %d", rc); - return; + return false; } /* PTFF subfunctions might be indicated although kernel support missing */ @@ -2482,7 +2482,7 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp) } if (!model->def) { error_setg(errp, "KVM: host CPU model could not be identified"); - return; + return false; } /* for now, we can only provide the AP feature with HW support */ if (ap_available()) { @@ -2506,6 +2506,7 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp) /* strip of features that are not part of the maximum model */ bitmap_and(model->features, model->features, model->def->full_feat, S390_FEAT_MAX); + return true; } static int configure_uv_feat_guest(const S390FeatBitmap features)