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

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

[Claudio Fontana]

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());
 }
 
 S390CPUModel *get_max_cpu_model(Error **errp)
-- 
2.26.2

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)

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

[Claudio Fontana]

On 3/14/24 20:44, Nina Schoetterl-Glausch wrote:
> 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.

Here you are referencing specifically the last hunk right? Ie the "Some features requested..." message.

> 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.

Interesting I'll try something along these lines.

> 
> 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.

better greppability in the log (as the error message in the log), or in the source code (or both)?
I am generally in favor of both, but there might be constraints on line length, although scripts/checkpatch.pl did not complain when I attempted this (I wonder if bug or feature).

docs/devel/style.rst on the code line length topic says:
"Lines should be 80 characters; try not to make them longer..."

and it does talk about exceptions. In the case of error message strings I think this could be one one of those exceptions.

In terms of logs, I did not find anything either, the most pertinent section should be "Error handling and reporting" in the same file,
but there is nothing about breaking up [or not] a single message in errors with newlines.

>>  
>>  S390CPUModel *get_max_cpu_model(Error **errp)
> 

Thanks,

Claudio

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

[Claudio Fontana]

On 3/14/24 21:10, Claudio Fontana wrote:
> On 3/14/24 20:44, Nina Schoetterl-Glausch wrote:
>> 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.
> 
> Here you are referencing specifically the last hunk right? Ie the "Some features requested..." message.
> 
>> 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.
> 
> Interesting I'll try something along these lines.
> 
>>
>> 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.
> 
> better greppability in the log (as the error message in the log), or in the source code (or both)?
> I am generally in favor of both, but there might be constraints on line length, although scripts/checkpatch.pl did not complain when I attempted this (I wonder if bug or feature).
> 
> docs/devel/style.rst on the code line length topic says:
> "Lines should be 80 characters; try not to make them longer..."
> 
> and it does talk about exceptions. In the case of error message strings I think this could be one one of those exceptions.
> 
> In terms of logs, I did not find anything either, the most pertinent section should be "Error handling and reporting" in the same file,
> but there is nothing about breaking up [or not] a single message in errors with newlines.

Ah I forgot the mythical include/qapi/error.h:

for error_setg we have:

"The resulting message should be a single phrase, with no newline or trailing punctuation."

so this helps.

> 
>>>  
>>>  S390CPUModel *get_max_cpu_model(Error **errp)
>>
> 
> Thanks,
> 
> Claudio

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

[Christian Borntraeger]

Am 14.03.24 um 20:00 schrieb Claudio Fontana:
> 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>

Independent on which message we end up with (see comments
in this mail thread), I agree that improving the
error message is helpful.

Acked-by: Christian Borntraeger <borntraeger@linux.ibm.com>

> ---
>   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());
>   }
>   
>   S390CPUModel *get_max_cpu_model(Error **errp)