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

2024-04-22 Thread 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é 
> 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 
> > > 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 
> > > > 
> > > > 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-phi...@linaro.org/
>

Good, pls let me pick this patch into my v2.

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

2024-04-19 Thread 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 
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 

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-phi...@linaro.org/

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

2024-04-19 Thread 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 
> 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 
> > 
> > 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 3/6] target/s390x/cpu_models: Make kvm_s390_get_host_cpu_model() return boolean

2024-04-19 Thread Thomas Huth 

On 19/04/2024 08.57, Zhao Liu wrote:

From: Zhao Liu 

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 3/6] target/s390x/cpu_models: Make kvm_s390_get_host_cpu_model() return boolean

2024-04-19 Thread 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(_model, );
> > +if (!kvm_s390_get_host_cpu_model(_model, )) {
> > +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 _model;
> >   }
>

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

2024-04-19 Thread Philippe Mathieu-Daudé 

On 19/4/24 08:57, Zhao Liu wrote:

From: Zhao Liu 

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 
---
  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(_model, );
+if (!kvm_s390_get_host_cpu_model(_model, )) {
+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 _model;
  }

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

2024-04-19 Thread Zhao Liu 
From: Zhao Liu 

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 
---
 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(_model, );
+if (!kvm_s390_get_host_cpu_model(_model, )) {
+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 _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, );
 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