target/sparc/cpu.h | 2 +- target/sparc/cpu.c | 4 ++-- target/sparc/gdbstub.c | 5 ++++- target/sparc/machine.c | 3 ++- target/sparc/monitor.c | 2 +- 5 files changed, 10 insertions(+), 6 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>
---
Another in my occasional series of "fix an avocado failure on
s390" Friday afternoon patches :-)
target/sparc/cpu.h | 2 +-
target/sparc/cpu.c | 4 ++--
target/sparc/gdbstub.c | 5 ++++-
target/sparc/machine.c | 3 ++-
target/sparc/monitor.c | 2 +-
5 files changed, 10 insertions(+), 6 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/gdbstub.c b/target/sparc/gdbstub.c
index a1c8fdc4d55..bddb9609b7b 100644
--- a/target/sparc/gdbstub.c
+++ b/target/sparc/gdbstub.c
@@ -96,7 +96,10 @@ int sparc_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
case 83:
return gdb_get_regl(mem_buf, env->fsr);
case 84:
- return gdb_get_regl(mem_buf, env->fprs);
+ {
+ target_ulong fprs = env->fprs;
+ return gdb_get_regl(mem_buf, fprs);
+ }
case 85:
return gdb_get_regl(mem_buf, env->y);
}
diff --git a/target/sparc/machine.c b/target/sparc/machine.c
index 44b9e7d75d6..5a8502804a4 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_UINT32(env.fprs, SPARCCPU),
+ VMSTATE_UNUSED(4), /* was unused high half of uint64_t fprs */
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 7/14/23 18:26, Peter Maydell wrote:
> +++ b/target/sparc/gdbstub.c
> @@ -96,7 +96,10 @@ int sparc_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
> case 83:
> return gdb_get_regl(mem_buf, env->fsr);
> case 84:
> - return gdb_get_regl(mem_buf, env->fprs);
> + {
> + target_ulong fprs = env->fprs;
> + return gdb_get_regl(mem_buf, fprs);
> + }
Surely no change at all here. The compiler zero-extends uint32_t to target_ulong in the
parameter to gdb_get_regl.
r~
Hi Peter,
On 14/7/23 19:26, 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>
> ---
> Another in my occasional series of "fix an avocado failure on
> s390" Friday afternoon patches :-)
:)
> diff --git a/target/sparc/gdbstub.c b/target/sparc/gdbstub.c
> index a1c8fdc4d55..bddb9609b7b 100644
> --- a/target/sparc/gdbstub.c
> +++ b/target/sparc/gdbstub.c
> @@ -96,7 +96,10 @@ int sparc_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
> case 83:
> return gdb_get_regl(mem_buf, env->fsr);
> case 84:
> - return gdb_get_regl(mem_buf, env->fprs);
> + {
> + target_ulong fprs = env->fprs;
> + return gdb_get_regl(mem_buf, fprs);
Why not return gdb_get_reg32() ?
> + }
On Fri, 14 Jul 2023 at 18:52, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> Hi Peter,
>
> On 14/7/23 19:26, 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>
> > ---
> > Another in my occasional series of "fix an avocado failure on
> > s390" Friday afternoon patches :-)
>
> :)
>
> > diff --git a/target/sparc/gdbstub.c b/target/sparc/gdbstub.c
> > index a1c8fdc4d55..bddb9609b7b 100644
> > --- a/target/sparc/gdbstub.c
> > +++ b/target/sparc/gdbstub.c
> > @@ -96,7 +96,10 @@ int sparc_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
> > case 83:
> > return gdb_get_regl(mem_buf, env->fsr);
> > case 84:
> > - return gdb_get_regl(mem_buf, env->fprs);
> > + {
> > + target_ulong fprs = env->fprs;
> > + return gdb_get_regl(mem_buf, fprs);
>
> Why not return gdb_get_reg32() ?
Because that would cause different on-the-wire data to be
sent to gdb -- gdb_get_reg32() puts 4 bytes of data into
the gdb remote protocol packet, whereas gdb_get_regl() puts
either 4 or 8 bytes depending on TARGET_LONG_BITS (as
it happens, here we'll always send 8 because this register
is sparc64- specific).
Anyway, Richard is correct and we don't need to change this
at all, because gdb_get_regl() takes an integer argument,
it isn't a magic macro that implicitly takes the address
or looks at the type of what it gets passed. So passing
it env->fprs will zero-extend that and DTRT.
thanks
-- PMM
On 16/7/23 19:32, Peter Maydell wrote:
> On Fri, 14 Jul 2023 at 18:52, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>>
>> Hi Peter,
>>
>> On 14/7/23 19:26, 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>
>>> ---
>>> Another in my occasional series of "fix an avocado failure on
>>> s390" Friday afternoon patches :-)
>>
>> :)
>>
>>> diff --git a/target/sparc/gdbstub.c b/target/sparc/gdbstub.c
>>> index a1c8fdc4d55..bddb9609b7b 100644
>>> --- a/target/sparc/gdbstub.c
>>> +++ b/target/sparc/gdbstub.c
>>> @@ -96,7 +96,10 @@ int sparc_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
>>> case 83:
>>> return gdb_get_regl(mem_buf, env->fsr);
>>> case 84:
>>> - return gdb_get_regl(mem_buf, env->fprs);
>>> + {
>>> + target_ulong fprs = env->fprs;
>>> + return gdb_get_regl(mem_buf, fprs);
>>
>> Why not return gdb_get_reg32() ?
>
> Because that would cause different on-the-wire data to be
> sent to gdb -- gdb_get_reg32() puts 4 bytes of data into
> the gdb remote protocol packet, whereas gdb_get_regl() puts
> either 4 or 8 bytes depending on TARGET_LONG_BITS (as
> it happens, here we'll always send 8 because this register
> is sparc64- specific).
Right, I missed the TARGET_LONG_BITS part.
> Anyway, Richard is correct and we don't need to change this
> at all, because gdb_get_regl() takes an integer argument,
> it isn't a magic macro that implicitly takes the address
> or looks at the type of what it gets passed. So passing
> it env->fprs will zero-extend that and DTRT.
OK.
On Fri, 14 Jul 2023 at 18:26, Peter Maydell <peter.maydell@linaro.org> 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.
> diff --git a/target/sparc/machine.c b/target/sparc/machine.c
> index 44b9e7d75d6..5a8502804a4 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_UINT32(env.fprs, SPARCCPU),
> + VMSTATE_UNUSED(4), /* was unused high half of uint64_t fprs */
Whoops, these two fields are the wrong way around. The on-the-wire
format for migration is always big-endian, so we need to put
the VMSTATE_UNUSED() placeholder before the VMSTATE_UINT32(),
not after it.
thanks
-- PMM
© 2016 - 2024 Red Hat, Inc.