[v2] error: Eliminate QERR_PROPERTY_VALUE_OUT_OF_RANGE

[PATCH v2 4/7] target/i386/cpu: Avoid mixing signed and unsigned in property setters

Posted by Markus Armbruster 1 month, 2 weeks ago

Properties "family", "model", and "stepping" are visited as signed
integers.  They are backed by bits in CPUX86State member
@cpuid_version.  The code to extract and insert these bits mixes
signed and unsigned.  Not actually wrong, but avoiding such mixing is
good practice.

Visit them as unsigned integers instead.

This adds a few mildly ugly cast in arguments of error_setg().  The
next commit will get rid of them again.

Property "tsc-frequency" is also visited as signed integer.  The value
ultimately flows into the kernel, where it is 31 bits unsigned.  The
QEMU code freely mixes int, uint32_t, int64_t.  I elect not to attempt
draining this swamp today.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 target/i386/cpu.c | 45 +++++++++++++++++++++------------------------
 1 file changed, 21 insertions(+), 24 deletions(-)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index ff227a8c5c..4f8fa60432 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -5433,13 +5433,13 @@ static void x86_cpuid_version_get_family(Object *obj, Visitor *v,
 {
     X86CPU *cpu = X86_CPU(obj);
     CPUX86State *env = &cpu->env;
-    int64_t value;
+    uint64_t value;
 
     value = (env->cpuid_version >> 8) & 0xf;
     if (value == 0xf) {
         value += (env->cpuid_version >> 20) & 0xff;
     }
-    visit_type_int(v, name, &value, errp);
+    visit_type_uint64(v, name, &value, errp);
 }
 
 static void x86_cpuid_version_set_family(Object *obj, Visitor *v,
@@ -5448,16 +5448,15 @@ static void x86_cpuid_version_set_family(Object *obj, Visitor *v,
 {
     X86CPU *cpu = X86_CPU(obj);
     CPUX86State *env = &cpu->env;
-    const int64_t min = 0;
-    const int64_t max = 0xff + 0xf;
-    int64_t value;
+    const uint64_t max = 0xff + 0xf;
+    uint64_t value;
 
-    if (!visit_type_int(v, name, &value, errp)) {
+    if (!visit_type_uint64(v, name, &value, errp)) {
         return;
     }
-    if (value < min || value > max) {
+    if (value > max) {
         error_setg(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE, "",
-                   name ? name : "null", value, min, max);
+                   name ? name : "null", value, (int64_t)0, (int64_t)max);
         return;
     }
 
@@ -5475,11 +5474,11 @@ static void x86_cpuid_version_get_model(Object *obj, Visitor *v,
 {
     X86CPU *cpu = X86_CPU(obj);
     CPUX86State *env = &cpu->env;
-    int64_t value;
+    uint64_t value;
 
     value = (env->cpuid_version >> 4) & 0xf;
     value |= ((env->cpuid_version >> 16) & 0xf) << 4;
-    visit_type_int(v, name, &value, errp);
+    visit_type_uint64(v, name, &value, errp);
 }
 
 static void x86_cpuid_version_set_model(Object *obj, Visitor *v,
@@ -5488,16 +5487,15 @@ static void x86_cpuid_version_set_model(Object *obj, Visitor *v,
 {
     X86CPU *cpu = X86_CPU(obj);
     CPUX86State *env = &cpu->env;
-    const int64_t min = 0;
-    const int64_t max = 0xff;
-    int64_t value;
+    const uint64_t max = 0xff;
+    uint64_t value;
 
-    if (!visit_type_int(v, name, &value, errp)) {
+    if (!visit_type_uint64(v, name, &value, errp)) {
         return;
     }
-    if (value < min || value > max) {
+    if (value > max) {
         error_setg(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE, "",
-                   name ? name : "null", value, min, max);
+                   name ? name : "null", value, (int64_t)0, (int64_t)max);
         return;
     }
 
@@ -5511,10 +5509,10 @@ static void x86_cpuid_version_get_stepping(Object *obj, Visitor *v,
 {
     X86CPU *cpu = X86_CPU(obj);
     CPUX86State *env = &cpu->env;
-    int64_t value;
+    uint64_t value;
 
     value = env->cpuid_version & 0xf;
-    visit_type_int(v, name, &value, errp);
+    visit_type_uint64(v, name, &value, errp);
 }
 
 static void x86_cpuid_version_set_stepping(Object *obj, Visitor *v,
@@ -5523,16 +5521,15 @@ static void x86_cpuid_version_set_stepping(Object *obj, Visitor *v,
 {
     X86CPU *cpu = X86_CPU(obj);
     CPUX86State *env = &cpu->env;
-    const int64_t min = 0;
-    const int64_t max = 0xf;
-    int64_t value;
+    const uint64_t max = 0xf;
+    uint64_t value;
 
-    if (!visit_type_int(v, name, &value, errp)) {
+    if (!visit_type_uint64(v, name, &value, errp)) {
         return;
     }
-    if (value < min || value > max) {
+    if (value > max) {
         error_setg(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE, "",
-                   name ? name : "null", value, min, max);
+                   name ? name : "null", value, (int64_t)0, (int64_t)max);
         return;
     }
 
-- 
2.46.0

Re: [PATCH v2 4/7] target/i386/cpu: Avoid mixing signed and unsigned in property setters

Posted by Igor Mammedov 1 month, 1 week ago

On Thu, 10 Oct 2024 17:01:41 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Properties "family", "model", and "stepping" are visited as signed
> integers.  They are backed by bits in CPUX86State member
> @cpuid_version.  The code to extract and insert these bits mixes
> signed and unsigned.  Not actually wrong, but avoiding such mixing is
> good practice.
> 
> Visit them as unsigned integers instead.
> 
> This adds a few mildly ugly cast in arguments of error_setg().  The
> next commit will get rid of them again.
> 
> Property "tsc-frequency" is also visited as signed integer.  The value
> ultimately flows into the kernel, where it is 31 bits unsigned.  The
> QEMU code freely mixes int, uint32_t, int64_t.  I elect not to attempt
> draining this swamp today.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  target/i386/cpu.c | 45 +++++++++++++++++++++------------------------
>  1 file changed, 21 insertions(+), 24 deletions(-)
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index ff227a8c5c..4f8fa60432 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -5433,13 +5433,13 @@ static void x86_cpuid_version_get_family(Object *obj, Visitor *v,
>  {
>      X86CPU *cpu = X86_CPU(obj);
>      CPUX86State *env = &cpu->env;
> -    int64_t value;
> +    uint64_t value;
>  
>      value = (env->cpuid_version >> 8) & 0xf;
>      if (value == 0xf) {
>          value += (env->cpuid_version >> 20) & 0xff;
>      }
> -    visit_type_int(v, name, &value, errp);
> +    visit_type_uint64(v, name, &value, errp);
>  }
>  
>  static void x86_cpuid_version_set_family(Object *obj, Visitor *v,
> @@ -5448,16 +5448,15 @@ static void x86_cpuid_version_set_family(Object *obj, Visitor *v,
>  {
>      X86CPU *cpu = X86_CPU(obj);
>      CPUX86State *env = &cpu->env;
> -    const int64_t min = 0;
> -    const int64_t max = 0xff + 0xf;
> -    int64_t value;
> +    const uint64_t max = 0xff + 0xf;
> +    uint64_t value;
>  
> -    if (!visit_type_int(v, name, &value, errp)) {
> +    if (!visit_type_uint64(v, name, &value, errp)) {
>          return;
>      }
> -    if (value < min || value > max) {
> +    if (value > max) {
>          error_setg(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE, "",
> -                   name ? name : "null", value, min, max);
> +                   name ? name : "null", value, (int64_t)0, (int64_t)max);
>          return;
>      }
>  
> @@ -5475,11 +5474,11 @@ static void x86_cpuid_version_get_model(Object *obj, Visitor *v,
>  {
>      X86CPU *cpu = X86_CPU(obj);
>      CPUX86State *env = &cpu->env;
> -    int64_t value;
> +    uint64_t value;
>  
>      value = (env->cpuid_version >> 4) & 0xf;
>      value |= ((env->cpuid_version >> 16) & 0xf) << 4;
> -    visit_type_int(v, name, &value, errp);
> +    visit_type_uint64(v, name, &value, errp);
>  }
>  
>  static void x86_cpuid_version_set_model(Object *obj, Visitor *v,
> @@ -5488,16 +5487,15 @@ static void x86_cpuid_version_set_model(Object *obj, Visitor *v,
>  {
>      X86CPU *cpu = X86_CPU(obj);
>      CPUX86State *env = &cpu->env;
> -    const int64_t min = 0;
> -    const int64_t max = 0xff;
> -    int64_t value;
> +    const uint64_t max = 0xff;
> +    uint64_t value;
>  
> -    if (!visit_type_int(v, name, &value, errp)) {
> +    if (!visit_type_uint64(v, name, &value, errp)) {
>          return;
>      }
> -    if (value < min || value > max) {
> +    if (value > max) {
>          error_setg(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE, "",
> -                   name ? name : "null", value, min, max);
> +                   name ? name : "null", value, (int64_t)0, (int64_t)max);
>          return;
>      }
>  
> @@ -5511,10 +5509,10 @@ static void x86_cpuid_version_get_stepping(Object *obj, Visitor *v,
>  {
>      X86CPU *cpu = X86_CPU(obj);
>      CPUX86State *env = &cpu->env;
> -    int64_t value;
> +    uint64_t value;
>  
>      value = env->cpuid_version & 0xf;
> -    visit_type_int(v, name, &value, errp);
> +    visit_type_uint64(v, name, &value, errp);
>  }
>  
>  static void x86_cpuid_version_set_stepping(Object *obj, Visitor *v,
> @@ -5523,16 +5521,15 @@ static void x86_cpuid_version_set_stepping(Object *obj, Visitor *v,
>  {
>      X86CPU *cpu = X86_CPU(obj);
>      CPUX86State *env = &cpu->env;
> -    const int64_t min = 0;
> -    const int64_t max = 0xf;
> -    int64_t value;
> +    const uint64_t max = 0xf;
> +    uint64_t value;
>  
> -    if (!visit_type_int(v, name, &value, errp)) {
> +    if (!visit_type_uint64(v, name, &value, errp)) {
>          return;
>      }
> -    if (value < min || value > max) {
> +    if (value > max) {
>          error_setg(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE, "",
> -                   name ? name : "null", value, min, max);
> +                   name ? name : "null", value, (int64_t)0, (int64_t)max);
>          return;
>      }
>

[PATCH v2 1/7] error: Drop superfluous #include "qapi/qmp/qerror.h"
[PATCH v2 2/7] block: Improve errors about block sizes
[PATCH v2 3/7] block: Adjust check_block_size() signature
[PATCH v2 4/7] target/i386/cpu: Avoid mixing signed and unsigned in property setters
[PATCH v2 5/7] target/i386/cpu: Improve errors for out of bounds property values
[PATCH v2 6/7] hw/intc/openpic: Improve errors for out of bounds property values
[PATCH v2 7/7] qerror: QERR_PROPERTY_VALUE_OUT_OF_RANGE is no longer used, drop