| Message ID | 20241010150144.986655-5-armbru@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | error: Eliminate QERR_PROPERTY_VALUE_OUT_OF_RANGE | expand |
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; > } >
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; }
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(-)