Re: [PATCH 3/6] target/s390x/cpu_models: Make kvm_s390_get_host_cpu_model() return boolean

Re: [PATCH 3/6] target/s390x/cpu_models: Make kvm_s390_get_host_cpu_model() return boolean

[Philippe Mathieu-Daudé]

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;
>   }

Re: [PATCH 0/6] s390x/cpu_models: Misc cleanup on returned error code and local @err variables

[Philippe Mathieu-Daudé]

On 19/4/24 08:57, Zhao Liu wrote:
> From: Zhao Liu <zhao1.liu@intel.com>


> ---
> Zhao Liu (6):
>    target/s390x/cpu_model: Make check_compatibility() return boolean
>    target/s390x/cpu_model: Drop local @err in s390_realize_cpu_model()
>    target/s390x/cpu_models: Make kvm_s390_get_host_cpu_model() return
>      boolean
>    target/s390x/cpu_models: Drop local @err in get_max_cpu_model()
>    target/s390x/cpu_models: Make kvm_s390_apply_cpu_model() return
>      boolean
>    target/s390x/cpu_models_sysemu: Drop local @err in apply_cpu_model()

Series:
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 0/6] s390x/cpu_models: Misc cleanup on returned error code and local @err variables

[Zhao Liu]

From: Zhao Liu <zhao1.liu@intel.com>

Hi list,

This series is the followup of Thomas' suggestion in previous
ERRP_GUARD() cleanup[1]. And based on Thomas' thoughts, I tried to clean
up as many of the other related places (in s390x/cpu_models.c).

[1]: https://lore.kernel.org/qemu-devel/6e7eff95-cfd3-46f9-9937-7597b2e4f3ee@redhat.com/

Regards,
Zhao
---
Zhao Liu (6):
  target/s390x/cpu_model: Make check_compatibility() return boolean
  target/s390x/cpu_model: Drop local @err in s390_realize_cpu_model()
  target/s390x/cpu_models: Make kvm_s390_get_host_cpu_model() return
    boolean
  target/s390x/cpu_models: Drop local @err in get_max_cpu_model()
  target/s390x/cpu_models: Make kvm_s390_apply_cpu_model() return
    boolean
  target/s390x/cpu_models_sysemu: Drop local @err in apply_cpu_model()

 target/s390x/cpu_models.c        | 25 ++++++++++---------------
 target/s390x/cpu_models.h        | 10 ++++++----
 target/s390x/cpu_models_sysemu.c |  5 +----
 target/s390x/kvm/kvm.c           | 28 +++++++++++++++-------------
 4 files changed, 32 insertions(+), 36 deletions(-)

-- 
2.34.1

[PATCH 1/6] target/s390x/cpu_model: Make check_compatibility() return boolean

[Zhao Liu]

From: Zhao Liu <zhao1.liu@intel.com>

As error.h suggested, the best practice for callee is to return
something to indicate success / failure.

With returned boolean, there's no need to check @err.

Suggested-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 target/s390x/cpu_models.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index 8ed3bb6a27b3..8cb47d905fb4 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -510,7 +510,7 @@ static void check_compat_model_failed(Error **errp,
     return;
 }
 
-static void check_compatibility(const S390CPUModel *max_model,
+static bool check_compatibility(const S390CPUModel *max_model,
                                 const S390CPUModel *model, Error **errp)
 {
     ERRP_GUARD();
@@ -518,11 +518,11 @@ static void check_compatibility(const S390CPUModel *max_model,
 
     if (model->def->gen > max_model->def->gen) {
         check_compat_model_failed(errp, max_model, "Selected CPU generation is too new");
-        return;
+        return false;
     } else if (model->def->gen == max_model->def->gen &&
                model->def->ec_ga > max_model->def->ec_ga) {
         check_compat_model_failed(errp, max_model, "Selected CPU GA level is too new");
-        return;
+        return false;
     }
 
 #ifndef CONFIG_USER_ONLY
@@ -530,14 +530,14 @@ static void check_compatibility(const S390CPUModel *max_model,
         error_setg(errp, "The unpack facility is not compatible with "
                    "the --only-migratable option. You must remove either "
                    "the 'unpack' facility or the --only-migratable option");
-        return;
+        return false;
     }
 #endif
 
     /* detect the missing features to properly report them */
     bitmap_andnot(missing, model->features, max_model->features, S390_FEAT_MAX);
     if (bitmap_empty(missing, S390_FEAT_MAX)) {
-        return;
+        return true;
     }
 
     error_setg(errp, " ");
@@ -546,6 +546,7 @@ static void check_compatibility(const S390CPUModel *max_model,
                   "available in the current configuration: ");
     error_append_hint(errp,
                       "Consider a different accelerator, QEMU, or kernel version\n");
+    return false;
 }
 
 S390CPUModel *get_max_cpu_model(Error **errp)
@@ -605,8 +606,7 @@ void s390_realize_cpu_model(CPUState *cs, Error **errp)
     cpu->model->cpu_ver = max_model->cpu_ver;
 
     check_consistency(cpu->model);
-    check_compatibility(max_model, cpu->model, &err);
-    if (err) {
+    if (!check_compatibility(max_model, cpu->model, &err)) {
         error_propagate(errp, err);
         return;
     }
-- 
2.34.1

[PATCH 2/6] target/s390x/cpu_model: Drop local @err in s390_realize_cpu_model()

[Zhao Liu]

From: Zhao Liu <zhao1.liu@intel.com>

Use @errp to fetech error information directly and drop the local
virable @err.

Suggested-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 target/s390x/cpu_models.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index 8cb47d905fb4..052540a866ac 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -577,7 +577,6 @@ S390CPUModel *get_max_cpu_model(Error **errp)
 void s390_realize_cpu_model(CPUState *cs, Error **errp)
 {
     ERRP_GUARD();
-    Error *err = NULL;
     S390CPUClass *xcc = S390_CPU_GET_CLASS(cs);
     S390CPU *cpu = S390_CPU(cs);
     const S390CPUModel *max_model;
@@ -606,8 +605,7 @@ void s390_realize_cpu_model(CPUState *cs, Error **errp)
     cpu->model->cpu_ver = max_model->cpu_ver;
 
     check_consistency(cpu->model);
-    if (!check_compatibility(max_model, cpu->model, &err)) {
-        error_propagate(errp, err);
+    if (!check_compatibility(max_model, cpu->model, errp)) {
         return;
     }
 
-- 
2.34.1

[PATCH 3/6] target/s390x/cpu_models: Make kvm_s390_get_host_cpu_model() return boolean

[Zhao Liu]

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()) {
-        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)
-- 
2.34.1

[PATCH 4/6] target/s390x/cpu_models: Drop local @err in get_max_cpu_model()

[Zhao Liu]

From: Zhao Liu <zhao1.liu@intel.com>

Use @errp to fetech error information directly and drop the local
virable @err.

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 target/s390x/cpu_models.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index a0e4acb707d7..aae452cfd3fc 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -551,7 +551,6 @@ static bool check_compatibility(const S390CPUModel *max_model,
 
 S390CPUModel *get_max_cpu_model(Error **errp)
 {
-    Error *err = NULL;
     static S390CPUModel max_model;
     static bool cached;
 
@@ -560,8 +559,7 @@ S390CPUModel *get_max_cpu_model(Error **errp)
     }
 
     if (kvm_enabled()) {
-        if (!kvm_s390_get_host_cpu_model(&max_model, &err)) {
-            error_propagate(errp, err);
+        if (!kvm_s390_get_host_cpu_model(&max_model, errp)) {
             return NULL;
         }
     } else {
-- 
2.34.1

[PATCH 5/6] target/s390x/cpu_models: Make kvm_s390_apply_cpu_model() return boolean

[Zhao Liu]

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_apply_cpu_model() return boolean and check the
returned boolean in apply_cpu_model() instead of accessing @err.

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 target/s390x/cpu_models.h        |  5 +++--
 target/s390x/cpu_models_sysemu.c |  3 +--
 target/s390x/kvm/kvm.c           | 15 ++++++++-------
 3 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/target/s390x/cpu_models.h b/target/s390x/cpu_models.h
index 5041c1e10fed..1be94294319d 100644
--- a/target/s390x/cpu_models.h
+++ b/target/s390x/cpu_models.h
@@ -117,16 +117,17 @@ 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);
 bool kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp);
-void kvm_s390_apply_cpu_model(const S390CPUModel *model,  Error **errp);
+bool kvm_s390_apply_cpu_model(const S390CPUModel *model,  Error **errp);
 #else
 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,
+static inline bool kvm_s390_apply_cpu_model(const S390CPUModel *model,
                                             Error **errp)
 {
+    return true;
 }
 static inline bool kvm_s390_cpu_models_supported(void)
 {
diff --git a/target/s390x/cpu_models_sysemu.c b/target/s390x/cpu_models_sysemu.c
index 2d99218069cb..bf855c659d5e 100644
--- a/target/s390x/cpu_models_sysemu.c
+++ b/target/s390x/cpu_models_sysemu.c
@@ -405,8 +405,7 @@ void apply_cpu_model(const S390CPUModel *model, Error **errp)
     }
 
     if (kvm_enabled()) {
-        kvm_s390_apply_cpu_model(model, &err);
-        if (err) {
+        if (!kvm_s390_apply_cpu_model(model, &err)) {
             error_propagate(errp, err);
             return;
         }
diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
index 57937b4ddbef..6334fb84141b 100644
--- a/target/s390x/kvm/kvm.c
+++ b/target/s390x/kvm/kvm.c
@@ -2543,7 +2543,7 @@ static void kvm_s390_configure_apie(bool interpret)
     }
 }
 
-void kvm_s390_apply_cpu_model(const S390CPUModel *model, Error **errp)
+bool kvm_s390_apply_cpu_model(const S390CPUModel *model, Error **errp)
 {
     struct kvm_s390_vm_cpu_processor prop  = {
         .fac_list = { 0 },
@@ -2560,11 +2560,11 @@ void kvm_s390_apply_cpu_model(const S390CPUModel *model, Error **errp)
         if (kvm_s390_cmma_available()) {
             kvm_s390_enable_cmma();
         }
-        return;
+        return true;
     }
     if (!kvm_s390_cpu_models_supported()) {
         error_setg(errp, "KVM doesn't support CPU models");
-        return;
+        return false;
     }
     prop.cpuid = s390_cpuid_from_cpu_model(model);
     prop.ibc = s390_ibc_from_cpu_model(model);
@@ -2574,19 +2574,19 @@ void kvm_s390_apply_cpu_model(const S390CPUModel *model, Error **errp)
     rc = kvm_vm_ioctl(kvm_state, KVM_SET_DEVICE_ATTR, &attr);
     if (rc) {
         error_setg(errp, "KVM: Error configuring the CPU model: %d", rc);
-        return;
+        return false;
     }
     /* configure cpu features indicated e.g. via SCLP */
     rc = configure_cpu_feat(model->features);
     if (rc) {
         error_setg(errp, "KVM: Error configuring CPU features: %d", rc);
-        return;
+        return false;
     }
     /* configure cpu subfunctions indicated via query / test bit */
     rc = configure_cpu_subfunc(model->features);
     if (rc) {
         error_setg(errp, "KVM: Error configuring CPU subfunctions: %d", rc);
-        return;
+        return false;
     }
     /* enable CMM via CMMA */
     if (test_bit(S390_FEAT_CMM, model->features)) {
@@ -2601,8 +2601,9 @@ void kvm_s390_apply_cpu_model(const S390CPUModel *model, Error **errp)
     rc = configure_uv_feat_guest(model->features);
     if (rc) {
         error_setg(errp, "KVM: Error configuring CPU UV features %d", rc);
-        return;
+        return false;
     }
+    return true;
 }
 
 void kvm_s390_restart_interrupt(S390CPU *cpu)
-- 
2.34.1

[PATCH 6/6] target/s390x/cpu_models_sysemu: Drop local @err in apply_cpu_model()

[Zhao Liu]

From: Zhao Liu <zhao1.liu@intel.com>

Use @errp to fetech error information directly and drop the local
virable @err.

Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
---
 target/s390x/cpu_models_sysemu.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/target/s390x/cpu_models_sysemu.c b/target/s390x/cpu_models_sysemu.c
index bf855c659d5e..15be729c3d48 100644
--- a/target/s390x/cpu_models_sysemu.c
+++ b/target/s390x/cpu_models_sysemu.c
@@ -389,7 +389,6 @@ CpuModelBaselineInfo *qmp_query_cpu_model_baseline(CpuModelInfo *infoa,
 
 void apply_cpu_model(const S390CPUModel *model, Error **errp)
 {
-    Error *err = NULL;
     static S390CPUModel applied_model;
     static bool applied;
 
@@ -405,8 +404,7 @@ void apply_cpu_model(const S390CPUModel *model, Error **errp)
     }
 
     if (kvm_enabled()) {
-        if (!kvm_s390_apply_cpu_model(model, &err)) {
-            error_propagate(errp, err);
+        if (!kvm_s390_apply_cpu_model(model, errp)) {
             return;
         }
     }
-- 
2.34.1

Re: [PATCH 3/6] target/s390x/cpu_models: Make kvm_s390_get_host_cpu_model() return boolean

[Zhao Liu]

> > 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;
> >   }
> 

Re: [PATCH 1/6] target/s390x/cpu_model: Make check_compatibility() return boolean

[Thomas Huth]

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.
> 
> With returned boolean, there's no need to check @err.
> 
> Suggested-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
>   target/s390x/cpu_models.c | 14 +++++++-------
>   1 file changed, 7 insertions(+), 7 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 2/6] target/s390x/cpu_model: Drop local @err in s390_realize_cpu_model()

[Thomas Huth]

On 19/04/2024 08.57, Zhao Liu wrote:
> From: Zhao Liu <zhao1.liu@intel.com>
> 
> Use @errp to fetech error information directly and drop the local

s/fetech/fetch/

> virable @err.

s/virable/variable/

> Suggested-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
>   target/s390x/cpu_models.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)

With the typos fixed:
Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 3/6] target/s390x/cpu_models: Make kvm_s390_get_host_cpu_model() return boolean

[Thomas Huth]

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

Re: [PATCH 4/6] target/s390x/cpu_models: Drop local @err in get_max_cpu_model()

[Thomas Huth]

On 19/04/2024 08.57, Zhao Liu wrote:
> From: Zhao Liu <zhao1.liu@intel.com>
> 
> Use @errp to fetech error information directly and drop the local
> virable @err.

Copy-n-paste of the same typos as in patch 2 ;-)

> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
>   target/s390x/cpu_models.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
> index a0e4acb707d7..aae452cfd3fc 100644
> --- a/target/s390x/cpu_models.c
> +++ b/target/s390x/cpu_models.c
> @@ -551,7 +551,6 @@ static bool check_compatibility(const S390CPUModel *max_model,
>   
>   S390CPUModel *get_max_cpu_model(Error **errp)
>   {
> -    Error *err = NULL;
>       static S390CPUModel max_model;
>       static bool cached;
>   
> @@ -560,8 +559,7 @@ S390CPUModel *get_max_cpu_model(Error **errp)
>       }
>   
>       if (kvm_enabled()) {
> -        if (!kvm_s390_get_host_cpu_model(&max_model, &err)) {
> -            error_propagate(errp, err);
> +        if (!kvm_s390_get_host_cpu_model(&max_model, errp)) {
>               return NULL;
>           }
>       } else {

With the typos fixed:
Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 5/6] target/s390x/cpu_models: Make kvm_s390_apply_cpu_model() return boolean

[Thomas Huth]

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_apply_cpu_model() return boolean and check the
> returned boolean in apply_cpu_model() instead of accessing @err.
> 
> Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> ---
>   target/s390x/cpu_models.h        |  5 +++--
>   target/s390x/cpu_models_sysemu.c |  3 +--
>   target/s390x/kvm/kvm.c           | 15 ++++++++-------
>   3 files changed, 12 insertions(+), 11 deletions(-)

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 6/6] target/s390x/cpu_models_sysemu: Drop local @err in apply_cpu_model()

[Thomas Huth]

On 19/04/2024 08.57, Zhao Liu wrote:
> From: Zhao Liu <zhao1.liu@intel.com>
> 
> Use @errp to fetech error information directly and drop the local
> virable @err.

With the typos fixed:

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 3/6] target/s390x/cpu_models: Make kvm_s390_get_host_cpu_model() return boolean

[Zhao Liu]

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

Re: [PATCH 2/6] target/s390x/cpu_model: Drop local @err in s390_realize_cpu_model()

[Zhao Liu]

On Fri, Apr 19, 2024 at 09:39:53AM +0200, Thomas Huth wrote:
> Date: Fri, 19 Apr 2024 09:39:53 +0200
> From: Thomas Huth <thuth@redhat.com>
> Subject: Re: [PATCH 2/6] target/s390x/cpu_model: Drop local @err in
>  s390_realize_cpu_model()
> 
> On 19/04/2024 08.57, Zhao Liu wrote:
> > From: Zhao Liu <zhao1.liu@intel.com>
> > 
> > Use @errp to fetech error information directly and drop the local
> 
> s/fetech/fetch/
> 
> > virable @err.
> 
> s/virable/variable/
> 
> > Suggested-by: Thomas Huth <thuth@redhat.com>
> > Signed-off-by: Zhao Liu <zhao1.liu@intel.com>
> > ---
> >   target/s390x/cpu_models.c | 4 +---
> >   1 file changed, 1 insertion(+), 3 deletions(-)
> 
> With the typos fixed:
> Reviewed-by: Thomas Huth <thuth@redhat.com>
>

Thanks! --codespell check also missed them. Will fix!

Regards,
Zhao

Re: [PATCH 3/6] target/s390x/cpu_models: Make kvm_s390_get_host_cpu_model() return boolean

[Philippe Mathieu-Daudé]

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/

Re: [PATCH 3/6] target/s390x/cpu_models: Make kvm_s390_get_host_cpu_model() return boolean

[Zhao Liu]

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.