| Message ID | 20201104172512.2381656-2-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qom: Field properties type safety | expand |
On 04/11/2020 17:25, Eduardo Habkost wrote: > The "iu-version" property is declared as uint64_t but points to a > target_ulong struct field. On the sparc 32-bit target, This > makes every write of iu-version corrupt the 4 bytes after > sparc_def_t.iu_version (where the fpu_version field is located). > > Change the type of the iu_version struct field to uint64_t, > and just use DEFINE_PROP_UINT64. > > The only place where env.def.iu_version field is read is the > env->version = env->def.iu_version; > assignment at sparc_cpu_realizefn(). This means behavior will be > kept exactly the same (except for the memory corruption bug fix). > > It would be nice to explicitly validate iu_version against > target_ulong limits, but that would be a new feature (since the > first version of this code, iu-version was parsed as 64-bit value > value). It can be done later, once we have an appropriate API to > define limits for integer properties. > > Fixes: de05005bf785 ("sparc: convert cpu features to qdev properties") > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > Cc: Artyom Tarasenko <atar4qemu@gmail.com> > Cc: qemu-devel@nongnu.org > --- > target/sparc/cpu.h | 2 +- > target/sparc/cpu.c | 5 ++--- > 2 files changed, 3 insertions(+), 4 deletions(-) > > diff --git a/target/sparc/cpu.h b/target/sparc/cpu.h > index b9369398f2..8ed0f4fef3 100644 > --- a/target/sparc/cpu.h > +++ b/target/sparc/cpu.h > @@ -252,7 +252,7 @@ typedef struct trap_state { > > struct sparc_def_t { > const char *name; > - target_ulong iu_version; > + uint64_t iu_version; > uint32_t fpu_version; > uint32_t mmu_version; > uint32_t mmu_bm; > diff --git a/target/sparc/cpu.c b/target/sparc/cpu.c > index ec59a13eb8..5a9397f19a 100644 > --- a/target/sparc/cpu.c > +++ b/target/sparc/cpu.c > @@ -576,7 +576,7 @@ void sparc_cpu_list(void) > unsigned int i; > > for (i = 0; i < ARRAY_SIZE(sparc_defs); i++) { > - qemu_printf("Sparc %16s IU " TARGET_FMT_lx > + qemu_printf("Sparc %16s IU %" PRIx64 > " FPU %08x MMU %08x NWINS %d ", > sparc_defs[i].name, > sparc_defs[i].iu_version, > @@ -838,8 +838,7 @@ static Property sparc_cpu_properties[] = { > DEFINE_PROP_BIT("hypv", SPARCCPU, env.def.features, 11, false), > DEFINE_PROP_BIT("cmt", SPARCCPU, env.def.features, 12, false), > DEFINE_PROP_BIT("gl", SPARCCPU, env.def.features, 13, false), > - DEFINE_PROP_UNSIGNED("iu-version", SPARCCPU, env.def.iu_version, 0, > - prop_info_uint64, target_ulong), > + DEFINE_PROP_UINT64("iu-version", SPARCCPU, env.def.iu_version, 0), > DEFINE_PROP_UINT32("fpu-version", SPARCCPU, env.def.fpu_version, 0), > DEFINE_PROP_UINT32("mmu-version", SPARCCPU, env.def.mmu_version, 0), > DEFINE_PROP("nwindows", SPARCCPU, env.def.nwindows, Acked-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> ATB, Mark.
diff --git a/target/sparc/cpu.h b/target/sparc/cpu.h index b9369398f2..8ed0f4fef3 100644 --- a/target/sparc/cpu.h +++ b/target/sparc/cpu.h @@ -252,7 +252,7 @@ typedef struct trap_state { struct sparc_def_t { const char *name; - target_ulong iu_version; + uint64_t iu_version; uint32_t fpu_version; uint32_t mmu_version; uint32_t mmu_bm; diff --git a/target/sparc/cpu.c b/target/sparc/cpu.c index ec59a13eb8..5a9397f19a 100644 --- a/target/sparc/cpu.c +++ b/target/sparc/cpu.c @@ -576,7 +576,7 @@ void sparc_cpu_list(void) unsigned int i; for (i = 0; i < ARRAY_SIZE(sparc_defs); i++) { - qemu_printf("Sparc %16s IU " TARGET_FMT_lx + qemu_printf("Sparc %16s IU %" PRIx64 " FPU %08x MMU %08x NWINS %d ", sparc_defs[i].name, sparc_defs[i].iu_version, @@ -838,8 +838,7 @@ static Property sparc_cpu_properties[] = { DEFINE_PROP_BIT("hypv", SPARCCPU, env.def.features, 11, false), DEFINE_PROP_BIT("cmt", SPARCCPU, env.def.features, 12, false), DEFINE_PROP_BIT("gl", SPARCCPU, env.def.features, 13, false), - DEFINE_PROP_UNSIGNED("iu-version", SPARCCPU, env.def.iu_version, 0, - prop_info_uint64, target_ulong), + DEFINE_PROP_UINT64("iu-version", SPARCCPU, env.def.iu_version, 0), DEFINE_PROP_UINT32("fpu-version", SPARCCPU, env.def.fpu_version, 0), DEFINE_PROP_UINT32("mmu-version", SPARCCPU, env.def.mmu_version, 0), DEFINE_PROP("nwindows", SPARCCPU, env.def.nwindows,
The "iu-version" property is declared as uint64_t but points to a target_ulong struct field. On the sparc 32-bit target, This makes every write of iu-version corrupt the 4 bytes after sparc_def_t.iu_version (where the fpu_version field is located). Change the type of the iu_version struct field to uint64_t, and just use DEFINE_PROP_UINT64. The only place where env.def.iu_version field is read is the env->version = env->def.iu_version; assignment at sparc_cpu_realizefn(). This means behavior will be kept exactly the same (except for the memory corruption bug fix). It would be nice to explicitly validate iu_version against target_ulong limits, but that would be a new feature (since the first version of this code, iu-version was parsed as 64-bit value value). It can be done later, once we have an appropriate API to define limits for integer properties. Fixes: de05005bf785 ("sparc: convert cpu features to qdev properties") Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> Cc: Artyom Tarasenko <atar4qemu@gmail.com> Cc: qemu-devel@nongnu.org --- target/sparc/cpu.h | 2 +- target/sparc/cpu.c | 5 ++--- 2 files changed, 3 insertions(+), 4 deletions(-)