From: Damien Hedde <damien.hedde@greensocs.com>
To: "Alex Bennée" <alex.bennee@linaro.org>, qemu-devel@nongnu.org
Cc: "Peter Maydell" <peter.maydell@linaro.org>,
"Cornelia Huck" <cohuck@redhat.com>,
luis.machado@linaro.org,
"Sagar Karandikar" <sagark@eecs.berkeley.edu>,
"David Hildenbrand" <david@redhat.com>,
"Mark Cave-Ayland" <mark.cave-ayland@ilande.co.uk>,
"Max Filippov" <jcmvbkbc@gmail.com>,
"Alistair Francis" <Alistair.Francis@wdc.com>,
"Edgar E. Iglesias" <edgar.iglesias@gmail.com>,
"Marek Vasut" <marex@denx.de>,
alan.hayward@arm.com,
"open list:PowerPC TCG CPUs" <qemu-ppc@nongnu.org>,
"Aleksandar Rikalo" <aleksandar.rikalo@rt-rk.com>,
"Richard Henderson" <rth@twiddle.net>,
"Philippe Mathieu-Daudé" <philmd@redhat.com>,
"Artyom Tarasenko" <atar4qemu@gmail.com>,
"Eduardo Habkost" <ehabkost@redhat.com>,
richard.henderson@linaro.org,
"open list:S390 TCG CPUs" <qemu-s390x@nongnu.org>,
"open list:ARM TCG CPUs" <qemu-arm@nongnu.org>,
"Stafford Horne" <shorne@gmail.com>,
"David Gibson" <david@gibson.dropbear.id.au>,
"open list:RISC-V TCG CPUs" <qemu-riscv@nongnu.org>,
"Bastian Koppelmann" <kbastian@mail.uni-paderborn.de>,
"Chris Wulff" <crwulff@gmail.com>,
"Laurent Vivier" <laurent@vivier.eu>,
"Michael Walle" <michael@walle.cc>,
"Palmer Dabbelt" <palmer@dabbelt.com>,
"Aleksandar Markovic" <amarkovic@wavecomp.com>,
"Paolo Bonzini" <pbonzini@redhat.com>,
"Aurelien Jarno" <aurelien@aurel32.net>
Subject: Re: [PATCH v3 08/20] gdbstub: extend GByteArray to read register helpers
Date: Wed, 11 Dec 2019 19:31:59 +0100 [thread overview]
Message-ID: <b65e722f-7524-9269-3f36-6045ab5393c5@greensocs.com> (raw)
In-Reply-To: <20191211170520.7747-9-alex.bennee@linaro.org>
Hi Alex,
On 12/11/19 6:05 PM, Alex Bennée wrote:
> Instead of passing a pointer to memory now just extend the GByteArray
> to all the read register helpers. They can then safely append their
> data through the normal way. We don't bother with this abstraction for
> write registers as we have already ensured the buffer being copied
> from is the correct size.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
[...]
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 0ac950d6c71..6476245e789 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -47,30 +47,27 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>
> static void switch_mode(CPUARMState *env, int mode);
>
> -static int vfp_gdb_get_reg(CPUARMState *env, uint8_t *buf, int reg)
> +static int vfp_gdb_get_reg(CPUARMState *env, GByteArray *buf, int reg)
> {
> int nregs;
>
> /* VFP data registers are always little-endian. */
> nregs = arm_feature(env, ARM_FEATURE_VFP3) ? 32 : 16;
> if (reg < nregs) {
> - stq_le_p(buf, *aa32_vfp_dreg(env, reg));
> - return 8;
> + return gdb_get_reg64(buf, *aa32_vfp_dreg(env, reg));
It was a little-endian version, you've put a target-endian version.
Is that what you meant ?
> }
> if (arm_feature(env, ARM_FEATURE_NEON)) {
> /* Aliases for Q regs. */
> nregs += 16;
> if (reg < nregs) {
> uint64_t *q = aa32_vfp_qreg(env, reg - 32);
> - stq_le_p(buf, q[0]);
> - stq_le_p(buf + 8, q[1]);
> - return 16;
> + return gdb_get_reg128(buf, q[0], q[1]);
Ditto here.
> }
> }
> switch (reg - nregs) {
> - case 0: stl_p(buf, env->vfp.xregs[ARM_VFP_FPSID]); return 4;
> - case 1: stl_p(buf, vfp_get_fpscr(env)); return 4;
> - case 2: stl_p(buf, env->vfp.xregs[ARM_VFP_FPEXC]); return 4;
> + case 0: return gdb_get_reg32(buf, env->vfp.xregs[ARM_VFP_FPSID]); break;
> + case 1: return gdb_get_reg32(buf, vfp_get_fpscr(env)); break;
> + case 2: return gdb_get_reg32(buf, env->vfp.xregs[ARM_VFP_FPEXC]); break;
> }
> return 0;
> }
> @@ -101,7 +98,7 @@ static int vfp_gdb_set_reg(CPUARMState *env, uint8_t *buf, int reg)
> return 0;
> }
>
> -static int aarch64_fpu_gdb_get_reg(CPUARMState *env, uint8_t *buf, int reg)
> +static int aarch64_fpu_gdb_get_reg(CPUARMState *env, GByteArray *buf, int reg)
> {
> switch (reg) {
> case 0 ... 31:
> @@ -204,7 +201,7 @@ static void write_raw_cp_reg(CPUARMState *env, const ARMCPRegInfo *ri,
> }
> }
>
> -static int arm_gdb_get_sysreg(CPUARMState *env, uint8_t *buf, int reg)
> +static int arm_gdb_get_sysreg(CPUARMState *env, GByteArray *buf, int reg)
> {
> ARMCPU *cpu = env_archcpu(env);
> const ARMCPRegInfo *ri;
[...]
> diff --git a/target/ppc/gdbstub.c b/target/ppc/gdbstub.c
> index 823759c92e7..6f08021cc22 100644
> --- a/target/ppc/gdbstub.c
> +++ b/target/ppc/gdbstub.c
> @@ -114,10 +114,11 @@ void ppc_maybe_bswap_register(CPUPPCState *env, uint8_t *mem_buf, int len)
> * the FP regs zero size when talking to a newer gdb.
> */
>
> -int ppc_cpu_gdb_read_register(CPUState *cs, uint8_t *mem_buf, int n)
> +int ppc_cpu_gdb_read_register(CPUState *cs, GByteArray *buf, int n)
> {
> PowerPCCPU *cpu = POWERPC_CPU(cs);
> CPUPPCState *env = &cpu->env;
> + uint8_t *mem_buf;
> int r = ppc_gdb_register_len(n);
>
> if (!r) {
> @@ -126,17 +127,17 @@ int ppc_cpu_gdb_read_register(CPUState *cs, uint8_t *mem_buf, int n)
>
> if (n < 32) {
> /* gprs */
> - gdb_get_regl(mem_buf, env->gpr[n]);
> + gdb_get_regl(buf, env->gpr[n]);
> } else if (n < 64) {
> /* fprs */
> - stfq_p(mem_buf, *cpu_fpr_ptr(env, n - 32));
> + gdb_get_reg64(buf, *cpu_fpr_ptr(env, n - 32));
> } else {
> switch (n) {
> case 64:
> - gdb_get_regl(mem_buf, env->nip);
> + gdb_get_regl(buf, env->nip);
> break;
> case 65:
> - gdb_get_regl(mem_buf, env->msr);
> + gdb_get_regl(buf, env->msr);
> break;
> case 66:
> {
> @@ -145,31 +146,33 @@ int ppc_cpu_gdb_read_register(CPUState *cs, uint8_t *mem_buf, int n)
> for (i = 0; i < 8; i++) {
> cr |= env->crf[i] << (32 - ((i + 1) * 4));
> }
> - gdb_get_reg32(mem_buf, cr);
> + gdb_get_reg32(buf, cr);
> break;
> }
> case 67:
> - gdb_get_regl(mem_buf, env->lr);
> + gdb_get_regl(buf, env->lr);
> break;
> case 68:
> - gdb_get_regl(mem_buf, env->ctr);
> + gdb_get_regl(buf, env->ctr);
> break;
> case 69:
> - gdb_get_reg32(mem_buf, env->xer);
> + gdb_get_reg32(buf, env->xer);
> break;
> case 70:
> - gdb_get_reg32(mem_buf, env->fpscr);
> + gdb_get_reg32(buf, env->fpscr);
> break;
> }
> }
> + mem_buf = buf->data - r;
Should it not be something more like this ?
mem_buf = buf->data + buf->len - r;
There seem to be the same issue below for every
ppc_maybe_bswap_register() call.
> ppc_maybe_bswap_register(env, mem_buf, r);
> return r;
> }
>
> -int ppc_cpu_gdb_read_register_apple(CPUState *cs, uint8_t *mem_buf, int n)
> +int ppc_cpu_gdb_read_register_apple(CPUState *cs, GByteArray *buf, int n)
> {
> PowerPCCPU *cpu = POWERPC_CPU(cs);
> CPUPPCState *env = &cpu->env;
> + uint8_t *mem_buf;
> int r = ppc_gdb_register_len_apple(n);
>
> if (!r) {
> @@ -178,21 +181,21 @@ int ppc_cpu_gdb_read_register_apple(CPUState *cs, uint8_t *mem_buf, int n)
>
> if (n < 32) {
> /* gprs */
> - gdb_get_reg64(mem_buf, env->gpr[n]);
> + gdb_get_reg64(buf, env->gpr[n]);
> } else if (n < 64) {
> /* fprs */
> - stfq_p(mem_buf, *cpu_fpr_ptr(env, n - 32));
> + gdb_get_reg64(buf, *cpu_fpr_ptr(env, n - 32));
> } else if (n < 96) {
> /* Altivec */
> - stq_p(mem_buf, n - 64);
> - stq_p(mem_buf + 8, 0);
> + gdb_get_reg64(buf, n - 64);
> + gdb_get_reg64(buf, 0);
> } else {
> switch (n) {
> case 64 + 32:
> - gdb_get_reg64(mem_buf, env->nip);
> + gdb_get_reg64(buf, env->nip);
> break;
> case 65 + 32:
> - gdb_get_reg64(mem_buf, env->msr);
> + gdb_get_reg64(buf, env->msr);
> break;
> case 66 + 32:
> {
> @@ -201,23 +204,24 @@ int ppc_cpu_gdb_read_register_apple(CPUState *cs, uint8_t *mem_buf, int n)
> for (i = 0; i < 8; i++) {
> cr |= env->crf[i] << (32 - ((i + 1) * 4));
> }
> - gdb_get_reg32(mem_buf, cr);
> + gdb_get_reg32(buf, cr);
> break;
> }
> case 67 + 32:
> - gdb_get_reg64(mem_buf, env->lr);
> + gdb_get_reg64(buf, env->lr);
> break;
> case 68 + 32:
> - gdb_get_reg64(mem_buf, env->ctr);
> + gdb_get_reg64(buf, env->ctr);
> break;
> case 69 + 32:
> - gdb_get_reg32(mem_buf, env->xer);
> + gdb_get_reg32(buf, env->xer);
> break;
> case 70 + 32:
> - gdb_get_reg64(mem_buf, env->fpscr);
> + gdb_get_reg64(buf, env->fpscr);
> break;
> }
> }
> + mem_buf = buf->data - r;
> ppc_maybe_bswap_register(env, mem_buf, r);
> return r;
> }
> diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
> index ba726dec4d0..154f876e44c 100644
> --- a/target/ppc/translate_init.inc.c
> +++ b/target/ppc/translate_init.inc.c
> @@ -9587,7 +9587,7 @@ static int gdb_find_spr_idx(CPUPPCState *env, int n)
> return -1;
> }
>
> -static int gdb_get_spr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
> +static int gdb_get_spr_reg(CPUPPCState *env, GByteArray *buf, int n)
> {
> int reg;
> int len;
> @@ -9598,8 +9598,8 @@ static int gdb_get_spr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
> }
>
> len = TARGET_LONG_SIZE;
> - stn_p(mem_buf, len, env->spr[reg]);
> - ppc_maybe_bswap_register(env, mem_buf, len);
> + gdb_get_regl(buf, env->spr[reg]);
> + ppc_maybe_bswap_register(env, buf->data - len, len);
> return len;
> }
>
> @@ -9621,15 +9621,18 @@ static int gdb_set_spr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
> }
> #endif
>
> -static int gdb_get_float_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
> +static int gdb_get_float_reg(CPUPPCState *env, GByteArray *buf, int n)
> {
> + uint8_t *mem_buf;
> if (n < 32) {
> - stfq_p(mem_buf, *cpu_fpr_ptr(env, n));
> + gdb_get_reg64(buf, *cpu_fpr_ptr(env, n));
> + mem_buf = buf->data - 8;
> ppc_maybe_bswap_register(env, mem_buf, 8);
> return 8;
> }
> if (n == 32) {
> - stl_p(mem_buf, env->fpscr);
> + gdb_get_reg32(buf, env->fpscr);
> + mem_buf = buf->data - 4;
> ppc_maybe_bswap_register(env, mem_buf, 4);
> return 4;
> }
> @@ -9651,28 +9654,31 @@ static int gdb_set_float_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
> return 0;
> }
>
> -static int gdb_get_avr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
> +static int gdb_get_avr_reg(CPUPPCState *env, GByteArray *buf, int n)
> {
> + uint8_t *mem_buf;
> +
> if (n < 32) {
> ppc_avr_t *avr = cpu_avr_ptr(env, n);
> if (!avr_need_swap(env)) {
> - stq_p(mem_buf, avr->u64[0]);
> - stq_p(mem_buf + 8, avr->u64[1]);
> + gdb_get_reg128(buf, avr->u64[0] , avr->u64[1]);
> } else {
> - stq_p(mem_buf, avr->u64[1]);
> - stq_p(mem_buf + 8, avr->u64[0]);
> + gdb_get_reg128(buf, avr->u64[1] , avr->u64[0]);
> }
> + mem_buf = buf->data - 16;
> ppc_maybe_bswap_register(env, mem_buf, 8);
> ppc_maybe_bswap_register(env, mem_buf + 8, 8);
> return 16;
> }
> if (n == 32) {
> - stl_p(mem_buf, helper_mfvscr(env));
> + gdb_get_reg32(buf, helper_mfvscr(env));
> + mem_buf = buf->data - 4;
> ppc_maybe_bswap_register(env, mem_buf, 4);> return 4;
> }
> if (n == 33) {
> - stl_p(mem_buf, (uint32_t)env->spr[SPR_VRSAVE]);
> + gdb_get_reg32(buf, (uint32_t)env->spr[SPR_VRSAVE]);
> + mem_buf = buf->data - 4;
> ppc_maybe_bswap_register(env, mem_buf, 4);
> return 4;
> }
> @@ -9707,25 +9713,25 @@ static int gdb_set_avr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
> return 0;
> }
>
> -static int gdb_get_spe_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
> +static int gdb_get_spe_reg(CPUPPCState *env, GByteArray *buf, int n)
> {
> if (n < 32) {
> #if defined(TARGET_PPC64)
> - stl_p(mem_buf, env->gpr[n] >> 32);
> - ppc_maybe_bswap_register(env, mem_buf, 4);
> + gdb_get_reg32(buf, env->gpr[n] >> 32);
> + ppc_maybe_bswap_register(env, buf->data - 4, 4);
> #else
> - stl_p(mem_buf, env->gprh[n]);
> + gdb_get_reg32(buf, env->gprh[n]);
> #endif
> return 4;
> }
> if (n == 32) {
> - stq_p(mem_buf, env->spe_acc);
> - ppc_maybe_bswap_register(env, mem_buf, 8);
> + gdb_get_reg64(buf, env->spe_acc);
> + ppc_maybe_bswap_register(env, buf->data - 8, 8);
> return 8;
> }
> if (n == 33) {
> - stl_p(mem_buf, env->spe_fscr);
> - ppc_maybe_bswap_register(env, mem_buf, 4);
> + gdb_get_reg32(buf, env->spe_fscr);
> + ppc_maybe_bswap_register(env, buf->data - 4, 4);
> return 4;
> }
> return 0;
> @@ -9760,11 +9766,11 @@ static int gdb_set_spe_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
> return 0;
> }
>
> -static int gdb_get_vsx_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
> +static int gdb_get_vsx_reg(CPUPPCState *env, GByteArray *buf, int n)
> {
> if (n < 32) {
> - stq_p(mem_buf, *cpu_vsrl_ptr(env, n));
> - ppc_maybe_bswap_register(env, mem_buf, 8);
> + gdb_get_reg64(buf, *cpu_vsrl_ptr(env, n));
> + ppc_maybe_bswap_register(env, buf->data - 8, 8);
> return 8;
> }
> return 0;
Otherwise, other files seem ok.
Regards,
--
Damien
WARNING: multiple messages have this Message-ID (diff)
From: Damien Hedde <damien.hedde@greensocs.com>
To: "Alex Bennée" <alex.bennee@linaro.org>, qemu-devel@nongnu.org
Cc: richard.henderson@linaro.org, alan.hayward@arm.com,
luis.machado@linaro.org,
"Philippe Mathieu-Daudé" <philmd@redhat.com>,
"Eduardo Habkost" <ehabkost@redhat.com>,
"Marcel Apfelbaum" <marcel.apfelbaum@gmail.com>,
"Richard Henderson" <rth@twiddle.net>,
"Peter Maydell" <peter.maydell@linaro.org>,
"Edgar E. Iglesias" <edgar.iglesias@gmail.com>,
"Paolo Bonzini" <pbonzini@redhat.com>,
"Michael Walle" <michael@walle.cc>,
"Laurent Vivier" <laurent@vivier.eu>,
"Aurelien Jarno" <aurelien@aurel32.net>,
"Aleksandar Markovic" <amarkovic@wavecomp.com>,
"Aleksandar Rikalo" <aleksandar.rikalo@rt-rk.com>,
"Chris Wulff" <crwulff@gmail.com>, "Marek Vasut" <marex@denx.de>,
"Stafford Horne" <shorne@gmail.com>,
"David Gibson" <david@gibson.dropbear.id.au>,
"Palmer Dabbelt" <palmer@dabbelt.com>,
"Alistair Francis" <Alistair.Francis@wdc.com>,
"Sagar Karandikar" <sagark@eecs.berkeley.edu>,
"Bastian Koppelmann" <kbastian@mail.uni-paderborn.de>,
"David Hildenbrand" <david@redhat.com>,
"Cornelia Huck" <cohuck@redhat.com>,
"Mark Cave-Ayland" <mark.cave-ayland@ilande.co.uk>,
"Artyom Tarasenko" <atar4qemu@gmail.com>,
"Max Filippov" <jcmvbkbc@gmail.com>,
"open list:ARM TCG CPUs" <qemu-arm@nongnu.org>,
"open list:PowerPC TCG CPUs" <qemu-ppc@nongnu.org>,
"open list:RISC-V TCG CPUs" <qemu-riscv@nongnu.org>,
"open list:S390 TCG CPUs" <qemu-s390x@nongnu.org>
Subject: Re: [PATCH v3 08/20] gdbstub: extend GByteArray to read register helpers
Date: Wed, 11 Dec 2019 19:31:59 +0100 [thread overview]
Message-ID: <b65e722f-7524-9269-3f36-6045ab5393c5@greensocs.com> (raw)
In-Reply-To: <20191211170520.7747-9-alex.bennee@linaro.org>
Hi Alex,
On 12/11/19 6:05 PM, Alex Bennée wrote:
> Instead of passing a pointer to memory now just extend the GByteArray
> to all the read register helpers. They can then safely append their
> data through the normal way. We don't bother with this abstraction for
> write registers as we have already ensured the buffer being copied
> from is the correct size.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
[...]
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 0ac950d6c71..6476245e789 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -47,30 +47,27 @@ static bool get_phys_addr_lpae(CPUARMState *env, target_ulong address,
>
> static void switch_mode(CPUARMState *env, int mode);
>
> -static int vfp_gdb_get_reg(CPUARMState *env, uint8_t *buf, int reg)
> +static int vfp_gdb_get_reg(CPUARMState *env, GByteArray *buf, int reg)
> {
> int nregs;
>
> /* VFP data registers are always little-endian. */
> nregs = arm_feature(env, ARM_FEATURE_VFP3) ? 32 : 16;
> if (reg < nregs) {
> - stq_le_p(buf, *aa32_vfp_dreg(env, reg));
> - return 8;
> + return gdb_get_reg64(buf, *aa32_vfp_dreg(env, reg));
It was a little-endian version, you've put a target-endian version.
Is that what you meant ?
> }
> if (arm_feature(env, ARM_FEATURE_NEON)) {
> /* Aliases for Q regs. */
> nregs += 16;
> if (reg < nregs) {
> uint64_t *q = aa32_vfp_qreg(env, reg - 32);
> - stq_le_p(buf, q[0]);
> - stq_le_p(buf + 8, q[1]);
> - return 16;
> + return gdb_get_reg128(buf, q[0], q[1]);
Ditto here.
> }
> }
> switch (reg - nregs) {
> - case 0: stl_p(buf, env->vfp.xregs[ARM_VFP_FPSID]); return 4;
> - case 1: stl_p(buf, vfp_get_fpscr(env)); return 4;
> - case 2: stl_p(buf, env->vfp.xregs[ARM_VFP_FPEXC]); return 4;
> + case 0: return gdb_get_reg32(buf, env->vfp.xregs[ARM_VFP_FPSID]); break;
> + case 1: return gdb_get_reg32(buf, vfp_get_fpscr(env)); break;
> + case 2: return gdb_get_reg32(buf, env->vfp.xregs[ARM_VFP_FPEXC]); break;
> }
> return 0;
> }
> @@ -101,7 +98,7 @@ static int vfp_gdb_set_reg(CPUARMState *env, uint8_t *buf, int reg)
> return 0;
> }
>
> -static int aarch64_fpu_gdb_get_reg(CPUARMState *env, uint8_t *buf, int reg)
> +static int aarch64_fpu_gdb_get_reg(CPUARMState *env, GByteArray *buf, int reg)
> {
> switch (reg) {
> case 0 ... 31:
> @@ -204,7 +201,7 @@ static void write_raw_cp_reg(CPUARMState *env, const ARMCPRegInfo *ri,
> }
> }
>
> -static int arm_gdb_get_sysreg(CPUARMState *env, uint8_t *buf, int reg)
> +static int arm_gdb_get_sysreg(CPUARMState *env, GByteArray *buf, int reg)
> {
> ARMCPU *cpu = env_archcpu(env);
> const ARMCPRegInfo *ri;
[...]
> diff --git a/target/ppc/gdbstub.c b/target/ppc/gdbstub.c
> index 823759c92e7..6f08021cc22 100644
> --- a/target/ppc/gdbstub.c
> +++ b/target/ppc/gdbstub.c
> @@ -114,10 +114,11 @@ void ppc_maybe_bswap_register(CPUPPCState *env, uint8_t *mem_buf, int len)
> * the FP regs zero size when talking to a newer gdb.
> */
>
> -int ppc_cpu_gdb_read_register(CPUState *cs, uint8_t *mem_buf, int n)
> +int ppc_cpu_gdb_read_register(CPUState *cs, GByteArray *buf, int n)
> {
> PowerPCCPU *cpu = POWERPC_CPU(cs);
> CPUPPCState *env = &cpu->env;
> + uint8_t *mem_buf;
> int r = ppc_gdb_register_len(n);
>
> if (!r) {
> @@ -126,17 +127,17 @@ int ppc_cpu_gdb_read_register(CPUState *cs, uint8_t *mem_buf, int n)
>
> if (n < 32) {
> /* gprs */
> - gdb_get_regl(mem_buf, env->gpr[n]);
> + gdb_get_regl(buf, env->gpr[n]);
> } else if (n < 64) {
> /* fprs */
> - stfq_p(mem_buf, *cpu_fpr_ptr(env, n - 32));
> + gdb_get_reg64(buf, *cpu_fpr_ptr(env, n - 32));
> } else {
> switch (n) {
> case 64:
> - gdb_get_regl(mem_buf, env->nip);
> + gdb_get_regl(buf, env->nip);
> break;
> case 65:
> - gdb_get_regl(mem_buf, env->msr);
> + gdb_get_regl(buf, env->msr);
> break;
> case 66:
> {
> @@ -145,31 +146,33 @@ int ppc_cpu_gdb_read_register(CPUState *cs, uint8_t *mem_buf, int n)
> for (i = 0; i < 8; i++) {
> cr |= env->crf[i] << (32 - ((i + 1) * 4));
> }
> - gdb_get_reg32(mem_buf, cr);
> + gdb_get_reg32(buf, cr);
> break;
> }
> case 67:
> - gdb_get_regl(mem_buf, env->lr);
> + gdb_get_regl(buf, env->lr);
> break;
> case 68:
> - gdb_get_regl(mem_buf, env->ctr);
> + gdb_get_regl(buf, env->ctr);
> break;
> case 69:
> - gdb_get_reg32(mem_buf, env->xer);
> + gdb_get_reg32(buf, env->xer);
> break;
> case 70:
> - gdb_get_reg32(mem_buf, env->fpscr);
> + gdb_get_reg32(buf, env->fpscr);
> break;
> }
> }
> + mem_buf = buf->data - r;
Should it not be something more like this ?
mem_buf = buf->data + buf->len - r;
There seem to be the same issue below for every
ppc_maybe_bswap_register() call.
> ppc_maybe_bswap_register(env, mem_buf, r);
> return r;
> }
>
> -int ppc_cpu_gdb_read_register_apple(CPUState *cs, uint8_t *mem_buf, int n)
> +int ppc_cpu_gdb_read_register_apple(CPUState *cs, GByteArray *buf, int n)
> {
> PowerPCCPU *cpu = POWERPC_CPU(cs);
> CPUPPCState *env = &cpu->env;
> + uint8_t *mem_buf;
> int r = ppc_gdb_register_len_apple(n);
>
> if (!r) {
> @@ -178,21 +181,21 @@ int ppc_cpu_gdb_read_register_apple(CPUState *cs, uint8_t *mem_buf, int n)
>
> if (n < 32) {
> /* gprs */
> - gdb_get_reg64(mem_buf, env->gpr[n]);
> + gdb_get_reg64(buf, env->gpr[n]);
> } else if (n < 64) {
> /* fprs */
> - stfq_p(mem_buf, *cpu_fpr_ptr(env, n - 32));
> + gdb_get_reg64(buf, *cpu_fpr_ptr(env, n - 32));
> } else if (n < 96) {
> /* Altivec */
> - stq_p(mem_buf, n - 64);
> - stq_p(mem_buf + 8, 0);
> + gdb_get_reg64(buf, n - 64);
> + gdb_get_reg64(buf, 0);
> } else {
> switch (n) {
> case 64 + 32:
> - gdb_get_reg64(mem_buf, env->nip);
> + gdb_get_reg64(buf, env->nip);
> break;
> case 65 + 32:
> - gdb_get_reg64(mem_buf, env->msr);
> + gdb_get_reg64(buf, env->msr);
> break;
> case 66 + 32:
> {
> @@ -201,23 +204,24 @@ int ppc_cpu_gdb_read_register_apple(CPUState *cs, uint8_t *mem_buf, int n)
> for (i = 0; i < 8; i++) {
> cr |= env->crf[i] << (32 - ((i + 1) * 4));
> }
> - gdb_get_reg32(mem_buf, cr);
> + gdb_get_reg32(buf, cr);
> break;
> }
> case 67 + 32:
> - gdb_get_reg64(mem_buf, env->lr);
> + gdb_get_reg64(buf, env->lr);
> break;
> case 68 + 32:
> - gdb_get_reg64(mem_buf, env->ctr);
> + gdb_get_reg64(buf, env->ctr);
> break;
> case 69 + 32:
> - gdb_get_reg32(mem_buf, env->xer);
> + gdb_get_reg32(buf, env->xer);
> break;
> case 70 + 32:
> - gdb_get_reg64(mem_buf, env->fpscr);
> + gdb_get_reg64(buf, env->fpscr);
> break;
> }
> }
> + mem_buf = buf->data - r;
> ppc_maybe_bswap_register(env, mem_buf, r);
> return r;
> }
> diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
> index ba726dec4d0..154f876e44c 100644
> --- a/target/ppc/translate_init.inc.c
> +++ b/target/ppc/translate_init.inc.c
> @@ -9587,7 +9587,7 @@ static int gdb_find_spr_idx(CPUPPCState *env, int n)
> return -1;
> }
>
> -static int gdb_get_spr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
> +static int gdb_get_spr_reg(CPUPPCState *env, GByteArray *buf, int n)
> {
> int reg;
> int len;
> @@ -9598,8 +9598,8 @@ static int gdb_get_spr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
> }
>
> len = TARGET_LONG_SIZE;
> - stn_p(mem_buf, len, env->spr[reg]);
> - ppc_maybe_bswap_register(env, mem_buf, len);
> + gdb_get_regl(buf, env->spr[reg]);
> + ppc_maybe_bswap_register(env, buf->data - len, len);
> return len;
> }
>
> @@ -9621,15 +9621,18 @@ static int gdb_set_spr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
> }
> #endif
>
> -static int gdb_get_float_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
> +static int gdb_get_float_reg(CPUPPCState *env, GByteArray *buf, int n)
> {
> + uint8_t *mem_buf;
> if (n < 32) {
> - stfq_p(mem_buf, *cpu_fpr_ptr(env, n));
> + gdb_get_reg64(buf, *cpu_fpr_ptr(env, n));
> + mem_buf = buf->data - 8;
> ppc_maybe_bswap_register(env, mem_buf, 8);
> return 8;
> }
> if (n == 32) {
> - stl_p(mem_buf, env->fpscr);
> + gdb_get_reg32(buf, env->fpscr);
> + mem_buf = buf->data - 4;
> ppc_maybe_bswap_register(env, mem_buf, 4);
> return 4;
> }
> @@ -9651,28 +9654,31 @@ static int gdb_set_float_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
> return 0;
> }
>
> -static int gdb_get_avr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
> +static int gdb_get_avr_reg(CPUPPCState *env, GByteArray *buf, int n)
> {
> + uint8_t *mem_buf;
> +
> if (n < 32) {
> ppc_avr_t *avr = cpu_avr_ptr(env, n);
> if (!avr_need_swap(env)) {
> - stq_p(mem_buf, avr->u64[0]);
> - stq_p(mem_buf + 8, avr->u64[1]);
> + gdb_get_reg128(buf, avr->u64[0] , avr->u64[1]);
> } else {
> - stq_p(mem_buf, avr->u64[1]);
> - stq_p(mem_buf + 8, avr->u64[0]);
> + gdb_get_reg128(buf, avr->u64[1] , avr->u64[0]);
> }
> + mem_buf = buf->data - 16;
> ppc_maybe_bswap_register(env, mem_buf, 8);
> ppc_maybe_bswap_register(env, mem_buf + 8, 8);
> return 16;
> }
> if (n == 32) {
> - stl_p(mem_buf, helper_mfvscr(env));
> + gdb_get_reg32(buf, helper_mfvscr(env));
> + mem_buf = buf->data - 4;
> ppc_maybe_bswap_register(env, mem_buf, 4);> return 4;
> }
> if (n == 33) {
> - stl_p(mem_buf, (uint32_t)env->spr[SPR_VRSAVE]);
> + gdb_get_reg32(buf, (uint32_t)env->spr[SPR_VRSAVE]);
> + mem_buf = buf->data - 4;
> ppc_maybe_bswap_register(env, mem_buf, 4);
> return 4;
> }
> @@ -9707,25 +9713,25 @@ static int gdb_set_avr_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
> return 0;
> }
>
> -static int gdb_get_spe_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
> +static int gdb_get_spe_reg(CPUPPCState *env, GByteArray *buf, int n)
> {
> if (n < 32) {
> #if defined(TARGET_PPC64)
> - stl_p(mem_buf, env->gpr[n] >> 32);
> - ppc_maybe_bswap_register(env, mem_buf, 4);
> + gdb_get_reg32(buf, env->gpr[n] >> 32);
> + ppc_maybe_bswap_register(env, buf->data - 4, 4);
> #else
> - stl_p(mem_buf, env->gprh[n]);
> + gdb_get_reg32(buf, env->gprh[n]);
> #endif
> return 4;
> }
> if (n == 32) {
> - stq_p(mem_buf, env->spe_acc);
> - ppc_maybe_bswap_register(env, mem_buf, 8);
> + gdb_get_reg64(buf, env->spe_acc);
> + ppc_maybe_bswap_register(env, buf->data - 8, 8);
> return 8;
> }
> if (n == 33) {
> - stl_p(mem_buf, env->spe_fscr);
> - ppc_maybe_bswap_register(env, mem_buf, 4);
> + gdb_get_reg32(buf, env->spe_fscr);
> + ppc_maybe_bswap_register(env, buf->data - 4, 4);
> return 4;
> }
> return 0;
> @@ -9760,11 +9766,11 @@ static int gdb_set_spe_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
> return 0;
> }
>
> -static int gdb_get_vsx_reg(CPUPPCState *env, uint8_t *mem_buf, int n)
> +static int gdb_get_vsx_reg(CPUPPCState *env, GByteArray *buf, int n)
> {
> if (n < 32) {
> - stq_p(mem_buf, *cpu_vsrl_ptr(env, n));
> - ppc_maybe_bswap_register(env, mem_buf, 8);
> + gdb_get_reg64(buf, *cpu_vsrl_ptr(env, n));
> + ppc_maybe_bswap_register(env, buf->data - 8, 8);
> return 8;
> }
> return 0;
Otherwise, other files seem ok.
Regards,
--
Damien
next prev parent reply other threads:[~2019-12-11 18:34 UTC|newest]
Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-11 17:05 [PATCH v3 00/20] gdbstub refactor and SVE support (+check-tcg tweaks) Alex Bennée
2019-12-11 17:05 ` [PATCH v3 01/20] gdbstub: make GDBState static and have common init function Alex Bennée
2019-12-11 17:05 ` [PATCH v3 02/20] gdbstub: stop passing GDBState * around and use global Alex Bennée
2019-12-11 17:05 ` [PATCH v3 03/20] gdbstub: move str_buf to GDBState and use GString Alex Bennée
2019-12-11 17:05 ` [PATCH v3 04/20] gdbstub: move mem_buf to GDBState and use GByteArray Alex Bennée
2019-12-13 12:31 ` Damien Hedde
2019-12-19 14:44 ` Alex Bennée
2019-12-11 17:05 ` [PATCH v3 05/20] gdbstub: add helper for 128 bit registers Alex Bennée
2019-12-11 17:05 ` [PATCH v3 06/20] target/arm: use gdb_get_reg helpers Alex Bennée
2019-12-12 1:44 ` Richard Henderson
2019-12-11 17:05 ` [PATCH v3 07/20] target/m68k: " Alex Bennée
2019-12-11 17:08 ` Laurent Vivier
2019-12-11 17:05 ` [PATCH v3 08/20] gdbstub: extend GByteArray to read register helpers Alex Bennée
2019-12-11 17:05 ` Alex Bennée
2019-12-11 18:31 ` Damien Hedde [this message]
2019-12-11 18:31 ` Damien Hedde
2019-12-19 17:50 ` Alex Bennée
2019-12-19 17:50 ` Alex Bennée
2019-12-12 1:55 ` Richard Henderson
2019-12-12 1:55 ` Richard Henderson
2019-12-11 17:05 ` [PATCH v3 09/20] target/arm: prepare for multiple dynamic XMLs Alex Bennée
2019-12-11 17:05 ` [PATCH v3 10/20] target/arm: explicitly encode regnum in our XML Alex Bennée
2019-12-11 17:05 ` [PATCH v3 11/20] target/arm: default SVE length to 64 bytes for linux-user Alex Bennée
2019-12-12 2:09 ` Richard Henderson
2019-12-11 17:05 ` [PATCH v3 12/20] target/arm: generate xml description of our SVE registers Alex Bennée
2019-12-12 2:26 ` Richard Henderson
2019-12-12 8:24 ` Alex Bennée
2019-12-19 19:15 ` Alex Bennée
2019-12-20 11:45 ` Luis Machado
2019-12-20 13:14 ` Alex Bennée
2019-12-20 13:18 ` Luis Machado
2020-01-08 15:57 ` Alan Hayward
2020-01-09 12:08 ` Alex Bennée
2020-01-09 14:10 ` Alan Hayward
2019-12-11 17:05 ` [PATCH v3 13/20] tests/tcg: add a configure compiler check for ARMv8.1 and SVE Alex Bennée
2019-12-11 17:05 ` [PATCH v3 14/20] target/arm: don't bother with id_aa64pfr0_read for USER_ONLY Alex Bennée
2019-12-12 2:29 ` Richard Henderson
2019-12-11 17:05 ` [PATCH v3 15/20] tests/tcg/aarch64: userspace system register test Alex Bennée
2019-12-11 17:05 ` [PATCH v3 16/20] tests/tcg: ensure we re-configure if configure.sh is updated Alex Bennée
2019-12-12 2:34 ` Richard Henderson
2019-12-11 17:05 ` [PATCH v3 17/20] tests/guest-debug: add a simple test runner Alex Bennée
2019-12-11 17:05 ` [PATCH v3 18/20] tests/tcg/aarch64: add a gdbstub testcase for SVE registers Alex Bennée
2019-12-11 17:05 ` [PATCH v3 19/20] tests/tcg/aarch64: add SVE iotcl test Alex Bennée
2019-12-12 2:37 ` Richard Henderson
2019-12-11 17:05 ` [PATCH v3 20/20] tests/tcg/aarch64: add test-sve-ioctl guest-debug test Alex Bennée
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=b65e722f-7524-9269-3f36-6045ab5393c5@greensocs.com \
--to=damien.hedde@greensocs.com \
--cc=Alistair.Francis@wdc.com \
--cc=alan.hayward@arm.com \
--cc=aleksandar.rikalo@rt-rk.com \
--cc=alex.bennee@linaro.org \
--cc=amarkovic@wavecomp.com \
--cc=atar4qemu@gmail.com \
--cc=aurelien@aurel32.net \
--cc=cohuck@redhat.com \
--cc=crwulff@gmail.com \
--cc=david@gibson.dropbear.id.au \
--cc=david@redhat.com \
--cc=edgar.iglesias@gmail.com \
--cc=ehabkost@redhat.com \
--cc=jcmvbkbc@gmail.com \
--cc=kbastian@mail.uni-paderborn.de \
--cc=laurent@vivier.eu \
--cc=luis.machado@linaro.org \
--cc=marex@denx.de \
--cc=mark.cave-ayland@ilande.co.uk \
--cc=michael@walle.cc \
--cc=palmer@dabbelt.com \
--cc=pbonzini@redhat.com \
--cc=peter.maydell@linaro.org \
--cc=philmd@redhat.com \
--cc=qemu-arm@nongnu.org \
--cc=qemu-devel@nongnu.org \
--cc=qemu-ppc@nongnu.org \
--cc=qemu-riscv@nongnu.org \
--cc=qemu-s390x@nongnu.org \
--cc=richard.henderson@linaro.org \
--cc=rth@twiddle.net \
--cc=sagark@eecs.berkeley.edu \
--cc=shorne@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.