Re: [PATCH v2] [PING] target/i386/gdbstub: Fix a bug about order of FPU stack in 'g' packets.

From: "Alex Bennée" <alex.bennee@linaro.org>
To: TaiseiIto <taisei1212@outlook.jp>
Cc: qemu-devel@nongnu.org, richard.henderson@linaro.org,
	Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [PATCH v2] [PING] target/i386/gdbstub: Fix a bug about order of FPU stack in 'g' packets.
Date: Sat, 07 Jan 2023 10:15:26 +0000	[thread overview]
Message-ID: <87zgau1wgc.fsf@linaro.org> (raw)
In-Reply-To: <TY0PR0101MB4285AD60FE3976F1AD5C6D02A4F89@TY0PR0101MB4285.apcprd01.prod.exchangelabs.com>

TaiseiIto <taisei1212@outlook.jp> writes:

> This is a ping to the patch below.
>
> https://patchew.org/QEMU/TY0PR0101MB42855925D8414E4773D6FA36A41D9@TY0PR0101MB4285.apcprd01.prod.exchangelabs.com/
>
> Before this commit, when GDB attached an OS working on QEMU, order of FPU
> stack registers printed by GDB command 'info float' was wrong. There was a
> bug causing the problem in 'g' packets sent by QEMU to GDB. The packets have
> values of registers of machine emulated by QEMU containing FPU stack
> registers. There are 2 ways to specify a x87 FPU stack register. The first
> is specifying by absolute indexed register names (R0, ..., R7). The second
> is specifying by stack top relative indexed register names (ST0, ..., ST7).
> Values of the FPU stack registers should be located in 'g' packet and be
> ordered by the relative index. But QEMU had located these registers ordered
> by the absolute index. After this commit, when QEMU reads registers to make
> a 'g' packet, QEMU specifies FPU stack registers by the relative index.
> Then, the registers are ordered correctly in the packet. As a result, GDB,
> the packet receiver, can print FPU stack registers in the correct order.
>
> Signed-off-by: TaiseiIto <taisei1212@outlook.jp>
> ---
>  target/i386/gdbstub.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/target/i386/gdbstub.c b/target/i386/gdbstub.c
> index c3a2cf6f28..786971284a 100644
> --- a/target/i386/gdbstub.c
> +++ b/target/i386/gdbstub.c
> @@ -121,7 +121,9 @@ int x86_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
>              return gdb_get_reg32(mem_buf, env->regs[gpr_map32[n]]);
>          }
>      } else if (n >= IDX_FP_REGS && n < IDX_FP_REGS + 8) {
> -        floatx80 *fp = (floatx80 *) &env->fpregs[n - IDX_FP_REGS];
> +        int st_index = n - IDX_FP_REGS;
> +        int r_index = (st_index + env->fpstt) % 8;
> +        floatx80 *fp = &env->fpregs[r_index].d;
>          int len = gdb_get_reg64(mem_buf, cpu_to_le64(fp->low));
>          len += gdb_get_reg16(mem_buf, cpu_to_le16(fp->high));
>          return len;

I'm sorry I though Paolo had already grabbed this, or is this a second
fix to the FP handling?

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro