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
next prev parent reply other threads:[~2019-12-11 18:34 UTC|newest]
Thread overview: 41+ 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 18:31 ` Damien Hedde [this message]
2019-12-19 17:50 ` Alex Bennée
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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).