Re: [PATCH] target/s390x: improve cpu compatibility check error message

[Nina Schoetterl-Glausch]

On Thu, 2024-03-14 at 20:00 +0100, Claudio Fontana wrote:
> some users were confused by this message showing under TCG:
> 
> Selected CPU generation is too new. Maximum supported model
> in the configuration: 'xyz'
> 
> Try to clarify that the maximum can depend on the accel by
> adding also the current accelerator to the message as such:
> 
> Selected CPU generation is too new. Maximum supported model
> in the accelerator 'tcg' configuration: 'xyz'
> 
> Signed-off-by: Claudio Fontana <cfontana@suse.de>
> ---
>  target/s390x/cpu_models.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
> index 1a1c096122..0d6d8fc727 100644
> --- a/target/s390x/cpu_models.c
> +++ b/target/s390x/cpu_models.c
> @@ -508,14 +508,14 @@ static void check_compatibility(const S390CPUModel 
> *max_model,
>  
>      if (model->def->gen > max_model->def->gen) {
>          error_setg(errp, "Selected CPU generation is too new. Maximum "
> -                   "supported model in the configuration: \'%s\'",
> -                   max_model->def->name);
> +                   "supported model in the accelerator \'%s\' configuration: 
> \'%s\'",
> +                   current_accel_name(), max_model->def->name);
>          return;
>      } else if (model->def->gen == max_model->def->gen &&
>                 model->def->ec_ga > max_model->def->ec_ga) {
>          error_setg(errp, "Selected CPU GA level is too new. Maximum "
> -                   "supported model in the configuration: \'%s\'",
> -                   max_model->def->name);
> +                   "supported model in the accelerator \'%s\' configuration: 
> \'%s\'",
> +                   current_accel_name(), max_model->def->name);
>          return;
>      }
>  
> @@ -537,7 +537,8 @@ static void check_compatibility(const S390CPUModel 
> *max_model,
>      error_setg(errp, " ");
>      s390_feat_bitmap_to_ascii(missing, errp, error_prepend_missing_feat);
>      error_prepend(errp, "Some features requested in the CPU model are not "
> -                  "available in the configuration: ");
> +                  "available in the accelerator \'%s\' configuration: ",
> +                  current_accel_name());
>  }

I wonder if these might not be confusing in other circumstances, e.g. when
running with KVM and the Linux version lacks support for some feature.
I think something along the lines of:

error_...(errp, "... supported by the current configuration ...", ...);
error_append_hint(errp, "Consider using a different accelerator, a different 
QEMU version or, when using KVM, a different kernel");

would be better.

I'm not sure about line breaks in error message, I like the better grepability
of unbroken lines but the coding style guide doesn't mention anything.
>  
>  S390CPUModel *get_max_cpu_model(Error **errp)