target/sparc/cpu.h | 2 +- target/sparc/cpu.c | 4 ++-- target/sparc/machine.c | 3 ++- target/sparc/monitor.c | 2 +- 4 files changed, 6 insertions(+), 5 deletions(-)
In CPUSparcState we define the fprs field as uint64_t. However we
then refer to it in translate.c via a TCGv_i32 which we set up with
tcg_global_mem_new_ptr(). This means that on a big-endian host when
the guest does something to writo te the FPRS register this value
ends up in the wrong half of the uint64_t, and the QEMU C code that
refers to env->fprs sees the wrong value. The effect of this is that
guest code that enables the FPU crashes with spurious FPU Disabled
exceptions. In particular, this is why
tests/avocado/machine_sparc64_sun4u.py:Sun4uMachine.test_sparc64_sun4u
times out on an s390 host.
There are multiple ways we could fix this; since there are actually
only three bits in the FPRS register and the code in translate.c
would be a bit painful to convert to dealing with a TCGv_i64, change
the type of the CPU state struct field to match what translate.c is
expecting.
(None of the other fields referenced by the r32[] array in
sparc_tcg_init() have the wrong type.)
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Changes v1->v2:
* drop unnecessary change to gdbstub.c
* put the vmstate fields the correct way around
NB: I believe the vmstate changes to be correct, but sparc64
seems unable to successfully do a savevm/loadvm even before
this change due to some other bug (the guest kernel panics
immediately after the loadvm).
---
target/sparc/cpu.h | 2 +-
target/sparc/cpu.c | 4 ++--
target/sparc/machine.c | 3 ++-
target/sparc/monitor.c | 2 +-
4 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/target/sparc/cpu.h b/target/sparc/cpu.h
index 95d2d0da71d..98044572f26 100644
--- a/target/sparc/cpu.h
+++ b/target/sparc/cpu.h
@@ -521,7 +521,7 @@ struct CPUArchState {
uint64_t igregs[8]; /* interrupt general registers */
uint64_t mgregs[8]; /* mmu general registers */
uint64_t glregs[8 * MAXTL_MAX];
- uint64_t fprs;
+ uint32_t fprs;
uint64_t tick_cmpr, stick_cmpr;
CPUTimer *tick, *stick;
#define TICK_NPT_MASK 0x8000000000000000ULL
diff --git a/target/sparc/cpu.c b/target/sparc/cpu.c
index e329a7aece5..130ab8f5781 100644
--- a/target/sparc/cpu.c
+++ b/target/sparc/cpu.c
@@ -673,8 +673,8 @@ static void sparc_cpu_dump_state(CPUState *cs, FILE *f, int flags)
"cleanwin: %d cwp: %d\n",
env->cansave, env->canrestore, env->otherwin, env->wstate,
env->cleanwin, env->nwindows - 1 - env->cwp);
- qemu_fprintf(f, "fsr: " TARGET_FMT_lx " y: " TARGET_FMT_lx " fprs: "
- TARGET_FMT_lx "\n", env->fsr, env->y, env->fprs);
+ qemu_fprintf(f, "fsr: " TARGET_FMT_lx " y: " TARGET_FMT_lx " fprs: %016x\n",
+ env->fsr, env->y, env->fprs);
#else
qemu_fprintf(f, "psr: %08x (icc: ", cpu_get_psr(env));
diff --git a/target/sparc/machine.c b/target/sparc/machine.c
index 44b9e7d75d6..274e1217dfb 100644
--- a/target/sparc/machine.c
+++ b/target/sparc/machine.c
@@ -168,7 +168,8 @@ const VMStateDescription vmstate_sparc_cpu = {
VMSTATE_UINT64_ARRAY(env.bgregs, SPARCCPU, 8),
VMSTATE_UINT64_ARRAY(env.igregs, SPARCCPU, 8),
VMSTATE_UINT64_ARRAY(env.mgregs, SPARCCPU, 8),
- VMSTATE_UINT64(env.fprs, SPARCCPU),
+ VMSTATE_UNUSED(4), /* was unused high half of uint64_t fprs */
+ VMSTATE_UINT32(env.fprs, SPARCCPU),
VMSTATE_UINT64(env.tick_cmpr, SPARCCPU),
VMSTATE_UINT64(env.stick_cmpr, SPARCCPU),
VMSTATE_CPU_TIMER(env.tick, SPARCCPU),
diff --git a/target/sparc/monitor.c b/target/sparc/monitor.c
index 318413686aa..73f15aa272d 100644
--- a/target/sparc/monitor.c
+++ b/target/sparc/monitor.c
@@ -154,7 +154,7 @@ const MonitorDef monitor_defs[] = {
{ "otherwin", offsetof(CPUSPARCState, otherwin) },
{ "wstate", offsetof(CPUSPARCState, wstate) },
{ "cleanwin", offsetof(CPUSPARCState, cleanwin) },
- { "fprs", offsetof(CPUSPARCState, fprs) },
+ { "fprs", offsetof(CPUSPARCState, fprs), NULL, MD_I32 },
#endif
{ NULL },
};
--
2.34.1On 17/07/2023 11:35, Peter Maydell wrote:
> In CPUSparcState we define the fprs field as uint64_t. However we
> then refer to it in translate.c via a TCGv_i32 which we set up with
> tcg_global_mem_new_ptr(). This means that on a big-endian host when
> the guest does something to writo te the FPRS register this value
> ends up in the wrong half of the uint64_t, and the QEMU C code that
> refers to env->fprs sees the wrong value. The effect of this is that
> guest code that enables the FPU crashes with spurious FPU Disabled
> exceptions. In particular, this is why
> tests/avocado/machine_sparc64_sun4u.py:Sun4uMachine.test_sparc64_sun4u
> times out on an s390 host.
>
> There are multiple ways we could fix this; since there are actually
> only three bits in the FPRS register and the code in translate.c
> would be a bit painful to convert to dealing with a TCGv_i64, change
> the type of the CPU state struct field to match what translate.c is
> expecting.
>
> (None of the other fields referenced by the r32[] array in
> sparc_tcg_init() have the wrong type.)
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Changes v1->v2:
> * drop unnecessary change to gdbstub.c
> * put the vmstate fields the correct way around
>
> NB: I believe the vmstate changes to be correct, but sparc64
> seems unable to successfully do a savevm/loadvm even before
> this change due to some other bug (the guest kernel panics
> immediately after the loadvm).
> ---
> target/sparc/cpu.h | 2 +-
> target/sparc/cpu.c | 4 ++--
> target/sparc/machine.c | 3 ++-
> target/sparc/monitor.c | 2 +-
> 4 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/target/sparc/cpu.h b/target/sparc/cpu.h
> index 95d2d0da71d..98044572f26 100644
> --- a/target/sparc/cpu.h
> +++ b/target/sparc/cpu.h
> @@ -521,7 +521,7 @@ struct CPUArchState {
> uint64_t igregs[8]; /* interrupt general registers */
> uint64_t mgregs[8]; /* mmu general registers */
> uint64_t glregs[8 * MAXTL_MAX];
> - uint64_t fprs;
> + uint32_t fprs;
> uint64_t tick_cmpr, stick_cmpr;
> CPUTimer *tick, *stick;
> #define TICK_NPT_MASK 0x8000000000000000ULL
> diff --git a/target/sparc/cpu.c b/target/sparc/cpu.c
> index e329a7aece5..130ab8f5781 100644
> --- a/target/sparc/cpu.c
> +++ b/target/sparc/cpu.c
> @@ -673,8 +673,8 @@ static void sparc_cpu_dump_state(CPUState *cs, FILE *f, int flags)
> "cleanwin: %d cwp: %d\n",
> env->cansave, env->canrestore, env->otherwin, env->wstate,
> env->cleanwin, env->nwindows - 1 - env->cwp);
> - qemu_fprintf(f, "fsr: " TARGET_FMT_lx " y: " TARGET_FMT_lx " fprs: "
> - TARGET_FMT_lx "\n", env->fsr, env->y, env->fprs);
> + qemu_fprintf(f, "fsr: " TARGET_FMT_lx " y: " TARGET_FMT_lx " fprs: %016x\n",
> + env->fsr, env->y, env->fprs);
>
> #else
> qemu_fprintf(f, "psr: %08x (icc: ", cpu_get_psr(env));
> diff --git a/target/sparc/machine.c b/target/sparc/machine.c
> index 44b9e7d75d6..274e1217dfb 100644
> --- a/target/sparc/machine.c
> +++ b/target/sparc/machine.c
> @@ -168,7 +168,8 @@ const VMStateDescription vmstate_sparc_cpu = {
> VMSTATE_UINT64_ARRAY(env.bgregs, SPARCCPU, 8),
> VMSTATE_UINT64_ARRAY(env.igregs, SPARCCPU, 8),
> VMSTATE_UINT64_ARRAY(env.mgregs, SPARCCPU, 8),
> - VMSTATE_UINT64(env.fprs, SPARCCPU),
> + VMSTATE_UNUSED(4), /* was unused high half of uint64_t fprs */
> + VMSTATE_UINT32(env.fprs, SPARCCPU),
> VMSTATE_UINT64(env.tick_cmpr, SPARCCPU),
> VMSTATE_UINT64(env.stick_cmpr, SPARCCPU),
> VMSTATE_CPU_TIMER(env.tick, SPARCCPU),
> diff --git a/target/sparc/monitor.c b/target/sparc/monitor.c
> index 318413686aa..73f15aa272d 100644
> --- a/target/sparc/monitor.c
> +++ b/target/sparc/monitor.c
> @@ -154,7 +154,7 @@ const MonitorDef monitor_defs[] = {
> { "otherwin", offsetof(CPUSPARCState, otherwin) },
> { "wstate", offsetof(CPUSPARCState, wstate) },
> { "cleanwin", offsetof(CPUSPARCState, cleanwin) },
> - { "fprs", offsetof(CPUSPARCState, fprs) },
> + { "fprs", offsetof(CPUSPARCState, fprs), NULL, MD_I32 },
> #endif
> { NULL },
> };
To the best of my knowledge there is no-one actively requesting migration
compatibility for SPARC, so I'm perfectly fine if any improvements here include a
migration version bump if you think it makes life easier/cleaner. Otherwise:
Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
ATB,
Mark.
On 7/17/23 11:35, Peter Maydell wrote: > In CPUSparcState we define the fprs field as uint64_t. However we > then refer to it in translate.c via a TCGv_i32 which we set up with > tcg_global_mem_new_ptr(). This means that on a big-endian host when > the guest does something to writo te the FPRS register this value > ends up in the wrong half of the uint64_t, and the QEMU C code that > refers to env->fprs sees the wrong value. The effect of this is that > guest code that enables the FPU crashes with spurious FPU Disabled > exceptions. In particular, this is why > tests/avocado/machine_sparc64_sun4u.py:Sun4uMachine.test_sparc64_sun4u > times out on an s390 host. > > There are multiple ways we could fix this; since there are actually > only three bits in the FPRS register and the code in translate.c > would be a bit painful to convert to dealing with a TCGv_i64, change > the type of the CPU state struct field to match what translate.c is > expecting. > > (None of the other fields referenced by the r32[] array in > sparc_tcg_init() have the wrong type.) > > Signed-off-by: Peter Maydell<peter.maydell@linaro.org> > --- > Changes v1->v2: > * drop unnecessary change to gdbstub.c > * put the vmstate fields the correct way around > > NB: I believe the vmstate changes to be correct, but sparc64 > seems unable to successfully do a savevm/loadvm even before > this change due to some other bug (the guest kernel panics > immediately after the loadvm). > --- > target/sparc/cpu.h | 2 +- > target/sparc/cpu.c | 4 ++-- > target/sparc/machine.c | 3 ++- > target/sparc/monitor.c | 2 +- > 4 files changed, 6 insertions(+), 5 deletions(-) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On 17/7/23 12:35, Peter Maydell wrote: > In CPUSparcState we define the fprs field as uint64_t. However we > then refer to it in translate.c via a TCGv_i32 which we set up with > tcg_global_mem_new_ptr(). This means that on a big-endian host when > the guest does something to writo te the FPRS register this value > ends up in the wrong half of the uint64_t, and the QEMU C code that > refers to env->fprs sees the wrong value. The effect of this is that > guest code that enables the FPU crashes with spurious FPU Disabled > exceptions. In particular, this is why > tests/avocado/machine_sparc64_sun4u.py:Sun4uMachine.test_sparc64_sun4u > times out on an s390 host. > > There are multiple ways we could fix this; since there are actually > only three bits in the FPRS register and the code in translate.c > would be a bit painful to convert to dealing with a TCGv_i64, change > the type of the CPU state struct field to match what translate.c is > expecting. > > (None of the other fields referenced by the r32[] array in > sparc_tcg_init() have the wrong type.) > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > Changes v1->v2: > * drop unnecessary change to gdbstub.c > * put the vmstate fields the correct way around > > NB: I believe the vmstate changes to be correct, but sparc64 > seems unable to successfully do a savevm/loadvm even before > this change due to some other bug (the guest kernel panics > immediately after the loadvm). > --- > target/sparc/cpu.h | 2 +- > target/sparc/cpu.c | 4 ++-- > target/sparc/machine.c | 3 ++- > target/sparc/monitor.c | 2 +- > 4 files changed, 6 insertions(+), 5 deletions(-) Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
On 17/7/23 13:30, Philippe Mathieu-Daudé wrote: > On 17/7/23 12:35, Peter Maydell wrote: >> In CPUSparcState we define the fprs field as uint64_t. However we >> then refer to it in translate.c via a TCGv_i32 which we set up with >> tcg_global_mem_new_ptr(). This means that on a big-endian host when >> the guest does something to writo te the FPRS register this value (typo around "write") >> ends up in the wrong half of the uint64_t, and the QEMU C code that >> refers to env->fprs sees the wrong value. The effect of this is that >> guest code that enables the FPU crashes with spurious FPU Disabled >> exceptions. In particular, this is why >> tests/avocado/machine_sparc64_sun4u.py:Sun4uMachine.test_sparc64_sun4u >> times out on an s390 host. >> >> There are multiple ways we could fix this; since there are actually >> only three bits in the FPRS register and the code in translate.c >> would be a bit painful to convert to dealing with a TCGv_i64, change >> the type of the CPU state struct field to match what translate.c is >> expecting. >> >> (None of the other fields referenced by the r32[] array in >> sparc_tcg_init() have the wrong type.) >> >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> >> --- >> Changes v1->v2: >> * drop unnecessary change to gdbstub.c >> * put the vmstate fields the correct way around >> >> NB: I believe the vmstate changes to be correct, but sparc64 >> seems unable to successfully do a savevm/loadvm even before >> this change due to some other bug (the guest kernel panics >> immediately after the loadvm). >> --- >> target/sparc/cpu.h | 2 +- >> target/sparc/cpu.c | 4 ++-- >> target/sparc/machine.c | 3 ++- >> target/sparc/monitor.c | 2 +- >> 4 files changed, 6 insertions(+), 5 deletions(-) > > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> >
On Mon, 17 Jul 2023 at 12:32, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > On 17/7/23 13:30, Philippe Mathieu-Daudé wrote: > > On 17/7/23 12:35, Peter Maydell wrote: > >> In CPUSparcState we define the fprs field as uint64_t. However we > >> then refer to it in translate.c via a TCGv_i32 which we set up with > >> tcg_global_mem_new_ptr(). This means that on a big-endian host when > >> the guest does something to writo te the FPRS register this value > > (typo around "write") Yes, should be "write to" but somehow I managed to transpose the final letters of the two words... -- PMM
© 2016 - 2024 Red Hat, Inc.