[PATCH v2] hw: fix memory leak in IRQState allocation
At e72a7f65c1 (hw: Move declaration of IRQState to header and add init
function, 2024-06-29), we've changed qemu_allocate_irq() to use a
combination of g_new() + object_initialize() instead of
IRQ(object_new()). The latter sets obj->free, so that that the memory is
properly cleaned when the object is finalized, but the former doesn't.
Fixes: e72a7f65c1 (hw: Move declaration of IRQState to header and add init
function)
Signed-off-by: Matheus Tavares Bernardino
Reviewed-by: BALATON Zoltan
---
In v2: adjusted function init_irq_fields name to reflect it is not
public and added BALATON's Reviewed-by
hw/core/irq.c | 19 +++
1 file changed, 11 insertions(+), 8 deletions(-)
diff --git a/hw/core/irq.c b/hw/core/irq.c
index db95ffc18f..7d5b0038c1 100644
--- a/hw/core/irq.c
+++ b/hw/core/irq.c
@@ -34,13 +34,19 @@ void qemu_set_irq(qemu_irq irq, int level)
irq->handler(irq->opaque, irq->n, level);
}
+static void init_irq_fields(IRQState *irq, qemu_irq_handler handler,
+void *opaque, int n)
+{
+irq->handler = handler;
+irq->opaque = opaque;
+irq->n = n;
+}
+
void qemu_init_irq(IRQState *irq, qemu_irq_handler handler, void *opaque,
int n)
{
object_initialize(irq, sizeof(*irq), TYPE_IRQ);
-irq->handler = handler;
-irq->opaque = opaque;
-irq->n = n;
+init_irq_fields(irq, handler, opaque, n);
}
qemu_irq *qemu_extend_irqs(qemu_irq *old, int n_old, qemu_irq_handler handler,
@@ -66,11 +72,8 @@ qemu_irq *qemu_allocate_irqs(qemu_irq_handler handler, void
*opaque, int n)
qemu_irq qemu_allocate_irq(qemu_irq_handler handler, void *opaque, int n)
{
-IRQState *irq;
-
-irq = g_new(IRQState, 1);
-qemu_init_irq(irq, handler, opaque, n);
-
+IRQState *irq = IRQ(object_new(TYPE_IRQ));
+init_irq_fields(irq, handler, opaque, n);
return irq;
}
--
2.37.2
[PATCH] hw: fix memory leak in IRQState allocation
At e72a7f65c1 (hw: Move declaration of IRQState to header and add init
function, 2024-06-29), we've changed qemu_allocate_irq() to use a
combination of g_new() + object_initialize() instead of
IRQ(object_new()). The latter sets obj->free, so that that the memory is
properly cleaned when the object is finalized, but the former doesn't.
Signed-off-by: Matheus Tavares Bernardino
---
hw/core/irq.c | 19 +++
1 file changed, 11 insertions(+), 8 deletions(-)
diff --git a/hw/core/irq.c b/hw/core/irq.c
index db95ffc18f..7d80de1ca6 100644
--- a/hw/core/irq.c
+++ b/hw/core/irq.c
@@ -34,13 +34,19 @@ void qemu_set_irq(qemu_irq irq, int level)
irq->handler(irq->opaque, irq->n, level);
}
+static void qemu_init_irq_fields(IRQState *irq, qemu_irq_handler handler,
+ void *opaque, int n)
+{
+irq->handler = handler;
+irq->opaque = opaque;
+irq->n = n;
+}
+
void qemu_init_irq(IRQState *irq, qemu_irq_handler handler, void *opaque,
int n)
{
object_initialize(irq, sizeof(*irq), TYPE_IRQ);
-irq->handler = handler;
-irq->opaque = opaque;
-irq->n = n;
+qemu_init_irq_fields(irq, handler, opaque, n);
}
qemu_irq *qemu_extend_irqs(qemu_irq *old, int n_old, qemu_irq_handler handler,
@@ -66,11 +72,8 @@ qemu_irq *qemu_allocate_irqs(qemu_irq_handler handler, void
*opaque, int n)
qemu_irq qemu_allocate_irq(qemu_irq_handler handler, void *opaque, int n)
{
-IRQState *irq;
-
-irq = g_new(IRQState, 1);
-qemu_init_irq(irq, handler, opaque, n);
-
+IRQState *irq = IRQ(object_new(TYPE_IRQ));
+qemu_init_irq_fields(irq, handler, opaque, n);
return irq;
}
--
2.37.2
[PATCH v2] docs/fuzz: fix outdated mention to enable-sanitizers
This options has been removed at cb771ac1f5 (meson: Split
--enable-sanitizers to --enable-{asan, ubsan}, 2024-08-13), so let's
update its last standing mention in the docs.
Signed-off-by: Matheus Tavares Bernardino
---
In v2: fixed grammar typo and s/use-after-frees/uses-after-free/
v1:
https://lore.kernel.org/qemu-devel/a788215960b94d863baeffb736f06e3fb94275e7.1726145226.git.quic_mathb...@quicinc.com/
docs/devel/testing/fuzzing.rst | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/docs/devel/testing/fuzzing.rst b/docs/devel/testing/fuzzing.rst
index dfe1973cf8..c3ac084311 100644
--- a/docs/devel/testing/fuzzing.rst
+++ b/docs/devel/testing/fuzzing.rst
@@ -21,8 +21,9 @@ Building the fuzzers
To build the fuzzers, install a recent version of clang:
Configure with (substitute the clang binaries with the version you installed).
-Here, enable-sanitizers, is optional but it allows us to reliably detect bugs
-such as out-of-bounds accesses, use-after-frees, double-frees etc.::
+Here, enable-asan and enable-ubsan are optional but they allow us to reliably
+detect bugs such as out-of-bounds accesses, uses-after-free, double-frees
+etc.::
CC=clang-8 CXX=clang++-8 /path/to/configure \
--enable-fuzzing --enable-asan --enable-ubsan
--
2.37.2
[PATCH] docs/fuzz: fix outdated mention to enable-sanitizers
This options has been removed at cb771ac1f5 (meson: Split
--enable-sanitizers to --enable-{asan, ubsan}, 2024-08-13), so let's
update its last standing mention in the docs.
Signed-off-by: Matheus Tavares Bernardino
---
docs/devel/testing/fuzzing.rst | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/docs/devel/testing/fuzzing.rst b/docs/devel/testing/fuzzing.rst
index dfe1973cf8..e42d64d6ec 100644
--- a/docs/devel/testing/fuzzing.rst
+++ b/docs/devel/testing/fuzzing.rst
@@ -21,8 +21,9 @@ Building the fuzzers
To build the fuzzers, install a recent version of clang:
Configure with (substitute the clang binaries with the version you installed).
-Here, enable-sanitizers, is optional but it allows us to reliably detect bugs
-such as out-of-bounds accesses, use-after-frees, double-frees etc.::
+Here, enable-asan and enable-ubsan are optional but they allows us to reliably
+detect bugs such as out-of-bounds accesses, use-after-frees, double-frees
+etc.::
CC=clang-8 CXX=clang++-8 /path/to/configure \
--enable-fuzzing --enable-asan --enable-ubsan
--
2.37.2
[PATCH] target/hexagon: relicense GPL files to BSD-3
Our internal contribution guidelines for QEMU were to use the BSD 3 clause license but we used GPLv2+ in error. So relicense the GPLv2+ Hexagon files to the compatible BSD 3-Clause, also updating the verbose license boilerplate with the cleaner SPDX identifier. To keep it simple, this patch only touches Hexagon files that have been contributed exclusive from Quic Inc employees (ignoring both files directly changed by other contributors as well as files with patches that have Helped-by and Co-authored-by trailers from others). Signed-off-by: Matheus Tavares Bernardino --- target/hexagon/cpu_bits.h| 15 ++- target/hexagon/helper.h | 15 ++- target/hexagon/mmvec/mmvec.h | 15 ++- tests/tcg/hexagon/hex_test.h | 15 ++- tests/tcg/hexagon/hvx_histogram_input.h | 15 ++- tests/tcg/hexagon/hvx_misc.h | 15 ++- disas/hexagon.c | 15 ++- target/hexagon/arch.c| 15 ++- target/hexagon/gen_semantics.c | 15 ++- target/hexagon/iclass.c | 15 ++- target/hexagon/printinsn.c | 15 ++- tests/tcg/hexagon/atomics.c | 15 ++- tests/tcg/hexagon/brev.c | 15 ++- tests/tcg/hexagon/circ.c | 15 ++- tests/tcg/hexagon/dual_stores.c | 15 ++- tests/tcg/hexagon/hex_sigsegv.c | 15 ++- tests/tcg/hexagon/hvx_histogram.c| 15 ++- tests/tcg/hexagon/load_align.c | 15 ++- tests/tcg/hexagon/load_unpack.c | 15 ++- tests/tcg/hexagon/mem_noshuf.c | 15 ++- tests/tcg/hexagon/mem_noshuf_exception.c | 15 ++- tests/tcg/hexagon/misc.c | 15 ++- tests/tcg/hexagon/multi_result.c | 15 ++- tests/tcg/hexagon/overflow.c | 15 ++- tests/tcg/hexagon/read_write_overlap.c | 15 ++- tests/tcg/hexagon/reg_mut.c | 15 ++- tests/tcg/hexagon/scatter_gather.c | 15 ++- tests/tcg/hexagon/signal_context.c | 15 ++- tests/tcg/hexagon/usr.c | 13 + tests/tcg/hexagon/v68_hvx.c | 15 ++- tests/tcg/hexagon/v68_scalar.c | 15 ++- tests/tcg/hexagon/v69_hvx.c | 15 ++- tests/tcg/hexagon/v73_scalar.c | 15 ++- tests/tcg/hexagon/v6mpy_ref.c.inc| 15 ++- gdb-xml/hexagon-hvx.xml | 5 ++--- target/hexagon/imported/branch.idef | 15 ++- target/hexagon/imported/compare.idef | 15 ++- target/hexagon/imported/encode.def | 15 ++- target/hexagon/imported/encode_pp.def| 15 ++- target/hexagon/imported/encode_subinsn.def | 15 ++- target/hexagon/imported/float.idef | 15 ++- target/hexagon/imported/iclass.def | 15 ++- target/hexagon/imported/ldst.idef| 15 ++- target/hexagon/imported/mmvec/encode_ext.def | 15 ++- target/hexagon/imported/mmvec/macros.def | 15 ++- target/hexagon/imported/mpy.idef | 15 ++- target/hexagon/imported/shift.idef | 15 ++- target/hexagon/imported/subinsns.idef| 15 ++- target/hexagon/imported/system.idef | 15 ++- tests/tcg/hexagon/hvx_histogram_row.S| 15 ++- 50 files changed, 99 insertions(+), 639 deletions(-) diff --git a/target/hexagon/cpu_bits.h b/target/hexagon/cpu_bits.h index 4279281a71..31321ea92b 100644 --- a/target/hexagon/cpu_bits.h +++ b/target/hexagon/cpu_bits.h @@ -1,18 +1,7 @@ /* - * Copyright(c) 2019-2021 Qualcomm Innovation Center, Inc. All Rights Reserved. + * Copyright(c) 2019-2024 Qualcomm Innovation Center, Inc. All Rights Reserved. * - * This program is free software; you can redistribute it and/or modify - * it under the terms of the GNU General Public License as published by - * the Free Software Foundation; either version 2 of the License, or - * (at your option) any later version. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU General Public License for more details. - * - * You should have received a copy of the GNU General Public License - * along with this program; if not, see <http://www.gnu.org/licen
[PATCH v2] Hexagon: fix F2_conv_* instructions for negative zero
The implementation for these instructions handles -0 as an invalid float
point value, whereas the Hexagon hardware considers it the same as +0
(which is valid). Let's fix that and add a regression test.
Signed-off-by: Matheus Tavares Bernardino
Reviewed-by: Brian Cain
Reviewed-by: Taylor Simpson
---
v2: updated the copyright year
target/hexagon/op_helper.c | 18 +-
tests/tcg/hexagon/usr.c| 12 +++-
2 files changed, 20 insertions(+), 10 deletions(-)
diff --git a/target/hexagon/op_helper.c b/target/hexagon/op_helper.c
index ae5a605513..90e7aaa097 100644
--- a/target/hexagon/op_helper.c
+++ b/target/hexagon/op_helper.c
@@ -1,5 +1,5 @@
/*
- * Copyright(c) 2019-2023 Qualcomm Innovation Center, Inc. All Rights
Reserved.
+ * Copyright(c) 2019-2024 Qualcomm Innovation Center, Inc. All Rights
Reserved.
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
@@ -683,7 +683,7 @@ uint32_t HELPER(conv_sf2uw)(CPUHexagonState *env, float32
RsV)
uint32_t RdV;
arch_fpop_start(env);
/* Hexagon checks the sign before rounding */
-if (float32_is_neg(RsV) && !float32_is_any_nan(RsV)) {
+if (float32_is_neg(RsV) && !float32_is_any_nan(RsV) &&
!float32_is_zero(RsV)) {
float_raise(float_flag_invalid, &env->fp_status);
RdV = 0;
} else {
@@ -713,7 +713,7 @@ uint64_t HELPER(conv_sf2ud)(CPUHexagonState *env, float32
RsV)
uint64_t RddV;
arch_fpop_start(env);
/* Hexagon checks the sign before rounding */
-if (float32_is_neg(RsV) && !float32_is_any_nan(RsV)) {
+if (float32_is_neg(RsV) && !float32_is_any_nan(RsV) &&
!float32_is_zero(RsV)) {
float_raise(float_flag_invalid, &env->fp_status);
RddV = 0;
} else {
@@ -743,7 +743,7 @@ uint32_t HELPER(conv_df2uw)(CPUHexagonState *env, float64
RssV)
uint32_t RdV;
arch_fpop_start(env);
/* Hexagon checks the sign before rounding */
-if (float64_is_neg(RssV) && !float64_is_any_nan(RssV)) {
+if (float64_is_neg(RssV) && !float64_is_any_nan(RssV) &&
!float64_is_zero(RssV)) {
float_raise(float_flag_invalid, &env->fp_status);
RdV = 0;
} else {
@@ -773,7 +773,7 @@ uint64_t HELPER(conv_df2ud)(CPUHexagonState *env, float64
RssV)
uint64_t RddV;
arch_fpop_start(env);
/* Hexagon checks the sign before rounding */
-if (float64_is_neg(RssV) && !float64_is_any_nan(RssV)) {
+if (float64_is_neg(RssV) && !float64_is_any_nan(RssV) &&
!float64_is_zero(RssV)) {
float_raise(float_flag_invalid, &env->fp_status);
RddV = 0;
} else {
@@ -803,7 +803,7 @@ uint32_t HELPER(conv_sf2uw_chop)(CPUHexagonState *env,
float32 RsV)
uint32_t RdV;
arch_fpop_start(env);
/* Hexagon checks the sign before rounding */
-if (float32_is_neg(RsV) && !float32_is_any_nan(RsV)) {
+if (float32_is_neg(RsV) && !float32_is_any_nan(RsV) &&
!float32_is_zero(RsV)) {
float_raise(float_flag_invalid, &env->fp_status);
RdV = 0;
} else {
@@ -833,7 +833,7 @@ uint64_t HELPER(conv_sf2ud_chop)(CPUHexagonState *env,
float32 RsV)
uint64_t RddV;
arch_fpop_start(env);
/* Hexagon checks the sign before rounding */
-if (float32_is_neg(RsV) && !float32_is_any_nan(RsV)) {
+if (float32_is_neg(RsV) && !float32_is_any_nan(RsV) &&
!float32_is_zero(RsV)) {
float_raise(float_flag_invalid, &env->fp_status);
RddV = 0;
} else {
@@ -863,7 +863,7 @@ uint32_t HELPER(conv_df2uw_chop)(CPUHexagonState *env,
float64 RssV)
uint32_t RdV;
arch_fpop_start(env);
/* Hexagon checks the sign before rounding */
-if (float64_is_neg(RssV) && !float64_is_any_nan(RssV)) {
+if (float64_is_neg(RssV) && !float64_is_any_nan(RssV) &&
!float64_is_zero(RssV)) {
float_raise(float_flag_invalid, &env->fp_status);
RdV = 0;
} else {
@@ -893,7 +893,7 @@ uint64_t HELPER(conv_df2ud_chop)(CPUHexagonState *env,
float64 RssV)
uint64_t RddV;
arch_fpop_start(env);
/* Hexagon checks the sign before rounding */
-if (float64_is_neg(RssV) && !float64_is_any_nan(RssV)) {
+if (float64_is_neg(RssV) && !float64_is_any_nan(RssV) &&
!float64_is_zero(RssV)) {
float_raise(float_flag_invalid, &env->fp_status);
RddV = 0;
} else {
diff --git a/tests/tcg/hexagon/usr.c b/tests/tcg/hexagon/usr.c
index 92bc86a213..f0b23d312b 100644
--- a/tests/tcg/hexagon/usr.c
+++ b/tests/tcg/hexagon/usr.c
@@ -1,5 +1,5 @@
/*
- * Copyright(c) 2022-2023 Qualcomm Innovation Center, Inc. All Rights
Reserved.
+ * Copyright(c) 2022-2024 Qualcomm Innovation Center, Inc. All Rights
Reserved.
[PATCH] Hexagon: fix F2_conv_* instructions for negative zero
The implementation for these instructions handles -0 as an invalid float
point value, whereas the Hexagon hardware considers it the same as +0
(which is valid). Let's fix that and add a regression test.
Signed-off-by: Matheus Tavares Bernardino
---
target/hexagon/op_helper.c | 16
tests/tcg/hexagon/usr.c| 10 ++
2 files changed, 18 insertions(+), 8 deletions(-)
diff --git a/target/hexagon/op_helper.c b/target/hexagon/op_helper.c
index ae5a605513..e1fc88aa0d 100644
--- a/target/hexagon/op_helper.c
+++ b/target/hexagon/op_helper.c
@@ -683,7 +683,7 @@ uint32_t HELPER(conv_sf2uw)(CPUHexagonState *env, float32
RsV)
uint32_t RdV;
arch_fpop_start(env);
/* Hexagon checks the sign before rounding */
-if (float32_is_neg(RsV) && !float32_is_any_nan(RsV)) {
+if (float32_is_neg(RsV) && !float32_is_any_nan(RsV) &&
!float32_is_zero(RsV)) {
float_raise(float_flag_invalid, &env->fp_status);
RdV = 0;
} else {
@@ -713,7 +713,7 @@ uint64_t HELPER(conv_sf2ud)(CPUHexagonState *env, float32
RsV)
uint64_t RddV;
arch_fpop_start(env);
/* Hexagon checks the sign before rounding */
-if (float32_is_neg(RsV) && !float32_is_any_nan(RsV)) {
+if (float32_is_neg(RsV) && !float32_is_any_nan(RsV) &&
!float32_is_zero(RsV)) {
float_raise(float_flag_invalid, &env->fp_status);
RddV = 0;
} else {
@@ -743,7 +743,7 @@ uint32_t HELPER(conv_df2uw)(CPUHexagonState *env, float64
RssV)
uint32_t RdV;
arch_fpop_start(env);
/* Hexagon checks the sign before rounding */
-if (float64_is_neg(RssV) && !float64_is_any_nan(RssV)) {
+if (float64_is_neg(RssV) && !float64_is_any_nan(RssV) &&
!float64_is_zero(RssV)) {
float_raise(float_flag_invalid, &env->fp_status);
RdV = 0;
} else {
@@ -773,7 +773,7 @@ uint64_t HELPER(conv_df2ud)(CPUHexagonState *env, float64
RssV)
uint64_t RddV;
arch_fpop_start(env);
/* Hexagon checks the sign before rounding */
-if (float64_is_neg(RssV) && !float64_is_any_nan(RssV)) {
+if (float64_is_neg(RssV) && !float64_is_any_nan(RssV) &&
!float64_is_zero(RssV)) {
float_raise(float_flag_invalid, &env->fp_status);
RddV = 0;
} else {
@@ -803,7 +803,7 @@ uint32_t HELPER(conv_sf2uw_chop)(CPUHexagonState *env,
float32 RsV)
uint32_t RdV;
arch_fpop_start(env);
/* Hexagon checks the sign before rounding */
-if (float32_is_neg(RsV) && !float32_is_any_nan(RsV)) {
+if (float32_is_neg(RsV) && !float32_is_any_nan(RsV) &&
!float32_is_zero(RsV)) {
float_raise(float_flag_invalid, &env->fp_status);
RdV = 0;
} else {
@@ -833,7 +833,7 @@ uint64_t HELPER(conv_sf2ud_chop)(CPUHexagonState *env,
float32 RsV)
uint64_t RddV;
arch_fpop_start(env);
/* Hexagon checks the sign before rounding */
-if (float32_is_neg(RsV) && !float32_is_any_nan(RsV)) {
+if (float32_is_neg(RsV) && !float32_is_any_nan(RsV) &&
!float32_is_zero(RsV)) {
float_raise(float_flag_invalid, &env->fp_status);
RddV = 0;
} else {
@@ -863,7 +863,7 @@ uint32_t HELPER(conv_df2uw_chop)(CPUHexagonState *env,
float64 RssV)
uint32_t RdV;
arch_fpop_start(env);
/* Hexagon checks the sign before rounding */
-if (float64_is_neg(RssV) && !float64_is_any_nan(RssV)) {
+if (float64_is_neg(RssV) && !float64_is_any_nan(RssV) &&
!float64_is_zero(RssV)) {
float_raise(float_flag_invalid, &env->fp_status);
RdV = 0;
} else {
@@ -893,7 +893,7 @@ uint64_t HELPER(conv_df2ud_chop)(CPUHexagonState *env,
float64 RssV)
uint64_t RddV;
arch_fpop_start(env);
/* Hexagon checks the sign before rounding */
-if (float64_is_neg(RssV) && !float64_is_any_nan(RssV)) {
+if (float64_is_neg(RssV) && !float64_is_any_nan(RssV) &&
!float64_is_zero(RssV)) {
float_raise(float_flag_invalid, &env->fp_status);
RddV = 0;
} else {
diff --git a/tests/tcg/hexagon/usr.c b/tests/tcg/hexagon/usr.c
index 92bc86a213..95d04762bf 100644
--- a/tests/tcg/hexagon/usr.c
+++ b/tests/tcg/hexagon/usr.c
@@ -1007,6 +1007,11 @@ int main()
TEST_P_OP_R(conv_sf2d_chop, SF_QNaN, 0xULL,
USR_FPINVF);
TEST_P_OP_R(conv_sf2d_chop, SF_SNaN, 0xULL,
USR_FPINVF);
+TEST_R_OP_R(conv_sf2uw, SF_zero_neg, 0, USR_CLEAR);
+TEST_R_OP_R(conv_sf2uw_chop, SF_zero_neg, 0, USR_CLEAR);
+TEST_P_OP_R(conv_sf2ud, SF_zero_neg, 0, USR_CLEAR);
+TEST_P_OP_R(conv_sf2ud_chop, SF_zero_neg, 0, USR_CLEAR);
+
TEST_R_OP_P(conv_df2sf, DF_QNaN, SF_HEX_NaN, USR_CLEAR);
TEST_R_OP_P(conv_df2sf, DF_SNaN, SF_HEX_NaN,
US
Re: [PATCH v2] Hexagon: lldb read/write predicate registers p0/p1/p2/p3
On Thu, 13 Jun 2024 12:22:09 -0600 Taylor Simpson wrote: > > hexagon-core.xml only exposes register p3_0 which is an alias that > aggregates the predicate registers. It is more convenient for users > to interact directly with the predicate registers. > > Tested with lldb downloaded from this location > https://github.com/llvm/llvm-project/releases/download/llvmorg-18.1.4/clang+llvm-18.1.4-x86_64-linux-gnu-ubuntu-18.04.tar.xz > > BEFORE: > (lldb) reg read p3_0 > p3_0 = 0x > (lldb) reg read p0 > error: Invalid register name 'p0'. > (lldb) reg write p1 0xf > error: Register not found for 'p1'. > > AFTER: > (lldb) reg read p3_0 > p3_0 = 0x > (lldb) reg read p0 > p0 = 0x00 > (lldb) reg read -s 1 > Predicate Registers: > p0 = 0x00 > p1 = 0x00 > p2 = 0x00 > p3 = 0x00 > > (lldb) reg write p1 0xf > (lldb) reg read p3_0 > p3_0 = 0x0f00 > (lldb) reg write p3_0 0xff00ff00 > (lldb) reg read -s 1 > Predicate Registers: > p0 = 0x00 > p1 = 0xff > p2 = 0x00 > p3 = 0xff > > Signed-off-by: Taylor Simpson Reviewed-by: Matheus Tavares Bernardino
Re: [PATCH] Hexagon: lldb read/write predicate registers p0/p1/p2/p3
On Wed, 12 Jun 2024 10:42:39 -0600 Taylor Simpson
wrote:
>
> diff --git a/target/hexagon/gdbstub.c b/target/hexagon/gdbstub.c
> index 502c6987f0..e67e627fc9 100644
> --- a/target/hexagon/gdbstub.c
> +++ b/target/hexagon/gdbstub.c
> @@ -56,6 +64,15 @@ int hexagon_gdb_write_register(CPUState *cs, uint8_t
> *mem_buf, int n)
> return sizeof(target_ulong);
> }
>
> +n -= TOTAL_PER_THREAD_REGS;
> +
> +if (n < NUM_PREGS) {
> +env->pred[n] = ldtul_p(mem_buf);
> +return sizeof(uint8_t);
I wonder, shouldn't this be sizeof(target_ulong) since we wrote a target_ulong?
> +}
> +
> +n -= NUM_PREGS;
> +
> g_assert_not_reached();
> }
[PATCH] cpu: fix memleak of 'halt_cond' and 'thread'
Since a4c2735f35 (cpu: move Qemu[Thread|Cond] setup into common code, 2024-05-30) these fields are now allocated at cpu_common_initfn(). So let's make sure we also free them at cpu_common_finalize(). Furthermore, the code also frees these on round robin, but we missed 'halt_cond'. Signed-off-by: Matheus Tavares Bernardino --- accel/tcg/tcg-accel-ops-rr.c | 1 + hw/core/cpu-common.c | 3 +++ 2 files changed, 4 insertions(+) diff --git a/accel/tcg/tcg-accel-ops-rr.c b/accel/tcg/tcg-accel-ops-rr.c index 84c36c1450..48c38714bd 100644 --- a/accel/tcg/tcg-accel-ops-rr.c +++ b/accel/tcg/tcg-accel-ops-rr.c @@ -329,6 +329,7 @@ void rr_start_vcpu_thread(CPUState *cpu) /* we share the thread, dump spare data */ g_free(cpu->thread); qemu_cond_destroy(cpu->halt_cond); +g_free(cpu->halt_cond); cpu->thread = single_tcg_cpu_thread; cpu->halt_cond = single_tcg_halt_cond; diff --git a/hw/core/cpu-common.c b/hw/core/cpu-common.c index bf1a7b8892..f131cde2c0 100644 --- a/hw/core/cpu-common.c +++ b/hw/core/cpu-common.c @@ -286,6 +286,9 @@ static void cpu_common_finalize(Object *obj) g_array_free(cpu->gdb_regs, TRUE); qemu_lockcnt_destroy(&cpu->in_ioctl_lock); qemu_mutex_destroy(&cpu->work_mutex); +qemu_cond_destroy(cpu->halt_cond); +g_free(cpu->halt_cond); +g_free(cpu->thread); } static int64_t cpu_common_get_arch_id(CPUState *cpu) -- 2.37.2
[PATCH] Hexagon: fix HVX store new
At 09a7e7db0f (Hexagon (target/hexagon) Remove uses of
op_regs_generated.h.inc, 2024-03-06), we've changed the logic of
check_new_value() to use the new pre-calculated
packet->insn[...].dest_idx instead of calculating the index on the fly
using opcode_reginfo[...]. The dest_idx index is calculated roughly like
the following:
for reg in iset[tag]["syntax"]:
if reg.is_written():
dest_idx = regno
break
Thus, we take the first register that is writtable. Before that,
however, we also used to follow an alphabetical order on the register
type: 'd', 'e', 'x', and 'y'. No longer following that makes us select
the wrong register index and the HVX store new instruction does not
update the memory like expected.
Signed-off-by: Matheus Tavares Bernardino
---
tests/tcg/hexagon/hvx_misc.c | 23 +++
target/hexagon/gen_trans_funcs.py | 9 ++---
2 files changed, 29 insertions(+), 3 deletions(-)
diff --git a/tests/tcg/hexagon/hvx_misc.c b/tests/tcg/hexagon/hvx_misc.c
index 1fe14b5158..90c3733da0 100644
--- a/tests/tcg/hexagon/hvx_misc.c
+++ b/tests/tcg/hexagon/hvx_misc.c
@@ -474,6 +474,27 @@ static void test_vcombine(void)
check_output_w(__LINE__, BUFSIZE);
}
+void test_store_new()
+{
+asm volatile(
+"r0 = #0x12345678\n"
+"v0 = vsplat(r0)\n"
+"r0 = #0xff00ff00\n"
+"v1 = vsplat(r0)\n"
+"{\n"
+" vdeal(v1,v0,r0)\n"
+" vmem(%0) = v0.new\n"
+"}\n"
+:
+: "r"(&output[0])
+: "r0", "v0", "v1", "memory"
+);
+for (int i = 0; i < MAX_VEC_SIZE_BYTES / 4; i++) {
+expect[0].w[i] = 0x12345678;
+}
+check_output_w(__LINE__, 1);
+}
+
int main()
{
init_buffers();
@@ -515,6 +536,8 @@ int main()
test_vcombine();
+test_store_new();
+
puts(err ? "FAIL" : "PASS");
return err ? 1 : 0;
}
diff --git a/target/hexagon/gen_trans_funcs.py
b/target/hexagon/gen_trans_funcs.py
index 9f86b4edbd..30f0c73e0c 100755
--- a/target/hexagon/gen_trans_funcs.py
+++ b/target/hexagon/gen_trans_funcs.py
@@ -89,6 +89,7 @@ def gen_trans_funcs(f):
new_read_idx = -1
dest_idx = -1
+dest_idx_reg_id = None
has_pred_dest = "false"
for regno, (reg_type, reg_id, *_) in enumerate(regs):
reg = hex_common.get_register(tag, reg_type, reg_id)
@@ -97,9 +98,11 @@ def gen_trans_funcs(f):
"""))
if reg.is_read() and reg.is_new():
new_read_idx = regno
-# dest_idx should be the first destination, so check for -1
-if reg.is_written() and dest_idx == -1:
-dest_idx = regno
+if reg.is_written():
+# dest_idx should be the first destination alphabetically
+if dest_idx_reg_id is None or reg_id < dest_idx_reg_id:
+dest_idx = regno
+dest_idx_reg_id = reg_id
if reg_type == "P" and reg.is_written() and not reg.is_read():
has_pred_dest = "true"
--
2.37.2
[PATCH v6] Hexagon: add PC alignment check and exception
The Hexagon Programmer's Reference Manual says that the exception 0x1e
should be raised upon an unaligned program counter. Let's implement that
and also add some tests.
Signed-off-by: Matheus Tavares Bernardino
Reviewed-by: Richard Henderson
Reviewed-by: Taylor Simpson
---
Changes in v6:
- The multi COF test defines a new section for the unaligned label to
make it more robust.
- Instead of a nop in the undesired test branch, we use a trap for
SYS_EXIT
target/hexagon/cpu.h | 7 ++
target/hexagon/cpu_bits.h | 4 ++
target/hexagon/macros.h | 3 -
linux-user/hexagon/cpu_loop.c | 4 ++
target/hexagon/op_helper.c| 9 ++-
tests/tcg/hexagon/unaligned_pc.c | 107 ++
tests/tcg/hexagon/Makefile.target | 2 +
7 files changed, 128 insertions(+), 8 deletions(-)
create mode 100644 tests/tcg/hexagon/unaligned_pc.c
diff --git a/target/hexagon/cpu.h b/target/hexagon/cpu.h
index 3eef58fe8f..764f3c38cc 100644
--- a/target/hexagon/cpu.h
+++ b/target/hexagon/cpu.h
@@ -134,6 +134,10 @@ struct ArchCPU {
FIELD(TB_FLAGS, IS_TIGHT_LOOP, 0, 1)
+G_NORETURN void hexagon_raise_exception_err(CPUHexagonState *env,
+uint32_t exception,
+uintptr_t pc);
+
static inline void cpu_get_tb_cpu_state(CPUHexagonState *env, vaddr *pc,
uint64_t *cs_base, uint32_t *flags)
{
@@ -144,6 +148,9 @@ static inline void cpu_get_tb_cpu_state(CPUHexagonState
*env, vaddr *pc,
hex_flags = FIELD_DP32(hex_flags, TB_FLAGS, IS_TIGHT_LOOP, 1);
}
*flags = hex_flags;
+if (*pc & PCALIGN_MASK) {
+hexagon_raise_exception_err(env, HEX_EXCP_PC_NOT_ALIGNED, 0);
+}
}
typedef HexagonCPU ArchCPU;
diff --git a/target/hexagon/cpu_bits.h b/target/hexagon/cpu_bits.h
index 96fef71729..4279281a71 100644
--- a/target/hexagon/cpu_bits.h
+++ b/target/hexagon/cpu_bits.h
@@ -20,9 +20,13 @@
#include "qemu/bitops.h"
+#define PCALIGN 4
+#define PCALIGN_MASK (PCALIGN - 1)
+
#define HEX_EXCP_FETCH_NO_UPAGE 0x012
#define HEX_EXCP_INVALID_PACKET 0x015
#define HEX_EXCP_INVALID_OPCODE 0x015
+#define HEX_EXCP_PC_NOT_ALIGNED 0x01e
#define HEX_EXCP_PRIV_NO_UREAD 0x024
#define HEX_EXCP_PRIV_NO_UWRITE 0x025
diff --git a/target/hexagon/macros.h b/target/hexagon/macros.h
index 1376d6ccc1..f375471a98 100644
--- a/target/hexagon/macros.h
+++ b/target/hexagon/macros.h
@@ -22,9 +22,6 @@
#include "hex_regs.h"
#include "reg_fields.h"
-#define PCALIGN 4
-#define PCALIGN_MASK (PCALIGN - 1)
-
#define GET_FIELD(FIELD, REGIN) \
fEXTRACTU_BITS(REGIN, reg_field_info[FIELD].width, \
reg_field_info[FIELD].offset)
diff --git a/linux-user/hexagon/cpu_loop.c b/linux-user/hexagon/cpu_loop.c
index 7f1499ed28..d41159e52a 100644
--- a/linux-user/hexagon/cpu_loop.c
+++ b/linux-user/hexagon/cpu_loop.c
@@ -60,6 +60,10 @@ void cpu_loop(CPUHexagonState *env)
env->gpr[0] = ret;
}
break;
+case HEX_EXCP_PC_NOT_ALIGNED:
+force_sig_fault(TARGET_SIGBUS, TARGET_BUS_ADRALN,
+env->gpr[HEX_REG_R31]);
+break;
case EXCP_ATOMIC:
cpu_exec_step_atomic(cs);
break;
diff --git a/target/hexagon/op_helper.c b/target/hexagon/op_helper.c
index da10ac5847..ae5a605513 100644
--- a/target/hexagon/op_helper.c
+++ b/target/hexagon/op_helper.c
@@ -36,10 +36,9 @@
#define SF_MANTBITS23
/* Exceptions processing helpers */
-static G_NORETURN
-void do_raise_exception_err(CPUHexagonState *env,
-uint32_t exception,
-uintptr_t pc)
+G_NORETURN void hexagon_raise_exception_err(CPUHexagonState *env,
+uint32_t exception,
+uintptr_t pc)
{
CPUState *cs = env_cpu(env);
qemu_log_mask(CPU_LOG_INT, "%s: %d\n", __func__, exception);
@@ -49,7 +48,7 @@ void do_raise_exception_err(CPUHexagonState *env,
G_NORETURN void HELPER(raise_exception)(CPUHexagonState *env, uint32_t excp)
{
-do_raise_exception_err(env, excp, 0);
+hexagon_raise_exception_err(env, excp, 0);
}
void log_store32(CPUHexagonState *env, target_ulong addr,
diff --git a/tests/tcg/hexagon/unaligned_pc.c b/tests/tcg/hexagon/unaligned_pc.c
new file mode 100644
index 00..e9dc7cb8b5
--- /dev/null
+++ b/tests/tcg/hexagon/unaligned_pc.c
@@ -0,0 +1,107 @@
+#include
+#include
+#include
+#include
+
+/* will be changed in signal handler */
+volatile sig_atomic_t completed_tests;
+static jmp_buf after_test;
+static int nr_tests;
+
+void __attribute__((naked)) test_return(void)
+{
+asm volatile(
+"allocframe(#0x8)\n"
+"r0 = #0x\n"
+"framekey = r0\n"
+
Re: [PATCH v5] Hexagon: add PC alignment check and exception
On Thu, 2 May 2024 13:00:34 -0700 Richard Henderson
wrote:
>
> On 5/2/24 12:20, Matheus Tavares Bernardino wrote:
> >
> > +
> > +void test_multi_cof(void)
> > +{
> > +asm volatile(
> > +"p0 = cmp.eq(r0, r0)\n"
> > +"{\n"
> > +"if (p0) jump test_multi_cof_unaligned\n"
> > +"if (!p0) jump 1f\n"
> > +"}\n"
> > +"1: nop\n"
>
> Does it work to write "jump 1f+1" or something?
Unfortunately no :( The assembler will align the address when encoding the
instruction. The only working examples I could think of is using a separated
file, like before, or manually encoding the instruction with a misaligned
address and place it with a `.word` directive... Any preferences, or other
suggestions?
> While it shouldn't matter, perhaps trap[01] would be better than nop here?
Ok!
> Also, the bike shed should be green.
hehe :)
[PATCH v5] Hexagon: add PC alignment check and exception
The Hexagon Programmer's Reference Manual says that the exception 0x1e
should be raised upon an unaligned program counter. Let's implement that
and also add some tests.
Signed-off-by: Matheus Tavares Bernardino
Reviewed-by: Richard Henderson
Reviewed-by: Taylor Simpson
---
Changes in v5:
- Merged asm and C test files into a single file.
target/hexagon/cpu.h | 7 +++
target/hexagon/cpu_bits.h | 4 ++
target/hexagon/macros.h | 3 -
linux-user/hexagon/cpu_loop.c | 4 ++
target/hexagon/op_helper.c| 9 ++-
tests/tcg/hexagon/unaligned_pc.c | 98 +++
tests/tcg/hexagon/Makefile.target | 2 +
7 files changed, 119 insertions(+), 8 deletions(-)
create mode 100644 tests/tcg/hexagon/unaligned_pc.c
diff --git a/target/hexagon/cpu.h b/target/hexagon/cpu.h
index 3eef58fe8f..764f3c38cc 100644
--- a/target/hexagon/cpu.h
+++ b/target/hexagon/cpu.h
@@ -134,6 +134,10 @@ struct ArchCPU {
FIELD(TB_FLAGS, IS_TIGHT_LOOP, 0, 1)
+G_NORETURN void hexagon_raise_exception_err(CPUHexagonState *env,
+uint32_t exception,
+uintptr_t pc);
+
static inline void cpu_get_tb_cpu_state(CPUHexagonState *env, vaddr *pc,
uint64_t *cs_base, uint32_t *flags)
{
@@ -144,6 +148,9 @@ static inline void cpu_get_tb_cpu_state(CPUHexagonState
*env, vaddr *pc,
hex_flags = FIELD_DP32(hex_flags, TB_FLAGS, IS_TIGHT_LOOP, 1);
}
*flags = hex_flags;
+if (*pc & PCALIGN_MASK) {
+hexagon_raise_exception_err(env, HEX_EXCP_PC_NOT_ALIGNED, 0);
+}
}
typedef HexagonCPU ArchCPU;
diff --git a/target/hexagon/cpu_bits.h b/target/hexagon/cpu_bits.h
index 96fef71729..4279281a71 100644
--- a/target/hexagon/cpu_bits.h
+++ b/target/hexagon/cpu_bits.h
@@ -20,9 +20,13 @@
#include "qemu/bitops.h"
+#define PCALIGN 4
+#define PCALIGN_MASK (PCALIGN - 1)
+
#define HEX_EXCP_FETCH_NO_UPAGE 0x012
#define HEX_EXCP_INVALID_PACKET 0x015
#define HEX_EXCP_INVALID_OPCODE 0x015
+#define HEX_EXCP_PC_NOT_ALIGNED 0x01e
#define HEX_EXCP_PRIV_NO_UREAD 0x024
#define HEX_EXCP_PRIV_NO_UWRITE 0x025
diff --git a/target/hexagon/macros.h b/target/hexagon/macros.h
index 1376d6ccc1..f375471a98 100644
--- a/target/hexagon/macros.h
+++ b/target/hexagon/macros.h
@@ -22,9 +22,6 @@
#include "hex_regs.h"
#include "reg_fields.h"
-#define PCALIGN 4
-#define PCALIGN_MASK (PCALIGN - 1)
-
#define GET_FIELD(FIELD, REGIN) \
fEXTRACTU_BITS(REGIN, reg_field_info[FIELD].width, \
reg_field_info[FIELD].offset)
diff --git a/linux-user/hexagon/cpu_loop.c b/linux-user/hexagon/cpu_loop.c
index 7f1499ed28..d41159e52a 100644
--- a/linux-user/hexagon/cpu_loop.c
+++ b/linux-user/hexagon/cpu_loop.c
@@ -60,6 +60,10 @@ void cpu_loop(CPUHexagonState *env)
env->gpr[0] = ret;
}
break;
+case HEX_EXCP_PC_NOT_ALIGNED:
+force_sig_fault(TARGET_SIGBUS, TARGET_BUS_ADRALN,
+env->gpr[HEX_REG_R31]);
+break;
case EXCP_ATOMIC:
cpu_exec_step_atomic(cs);
break;
diff --git a/target/hexagon/op_helper.c b/target/hexagon/op_helper.c
index da10ac5847..ae5a605513 100644
--- a/target/hexagon/op_helper.c
+++ b/target/hexagon/op_helper.c
@@ -36,10 +36,9 @@
#define SF_MANTBITS23
/* Exceptions processing helpers */
-static G_NORETURN
-void do_raise_exception_err(CPUHexagonState *env,
-uint32_t exception,
-uintptr_t pc)
+G_NORETURN void hexagon_raise_exception_err(CPUHexagonState *env,
+uint32_t exception,
+uintptr_t pc)
{
CPUState *cs = env_cpu(env);
qemu_log_mask(CPU_LOG_INT, "%s: %d\n", __func__, exception);
@@ -49,7 +48,7 @@ void do_raise_exception_err(CPUHexagonState *env,
G_NORETURN void HELPER(raise_exception)(CPUHexagonState *env, uint32_t excp)
{
-do_raise_exception_err(env, excp, 0);
+hexagon_raise_exception_err(env, excp, 0);
}
void log_store32(CPUHexagonState *env, target_ulong addr,
diff --git a/tests/tcg/hexagon/unaligned_pc.c b/tests/tcg/hexagon/unaligned_pc.c
new file mode 100644
index 00..de50e5be9d
--- /dev/null
+++ b/tests/tcg/hexagon/unaligned_pc.c
@@ -0,0 +1,98 @@
+#include
+#include
+#include
+#include
+
+/* will be changed in signal handler */
+volatile sig_atomic_t completed_tests;
+static jmp_buf after_test;
+static int nr_tests;
+
+void __attribute__((naked)) test_return(void)
+{
+asm volatile(
+"allocframe(#0x8)\n"
+"r0 = #0x\n"
+"framekey = r0\n"
+"dealloc_return\n"
+:
+:
+: "r0", "r29", "r30", "r31"
[PATCH v4] Hexagon: add PC alignment check and exception
The Hexagon Programmer's Reference Manual says that the exception 0x1e
should be raised upon an unaligned program counter. Let's implement that
and also add some tests.
Signed-off-by: Matheus Tavares Bernardino
Reviewed-by: Richard Henderson
---
v3:
https://lore.kernel.org/qemu-devel/5c90567ec28723865e144f386b36f5b676b7a5d3.1714486874.git.quic_mathb...@quicinc.com/
Changes in v4:
- Added missing regs to clobber list as mentioned by Taylor.
- Avoided undefined behavior on package with multiple branches
(at test_multi_cof), as suggested offline by Brian.
target/hexagon/cpu.h | 7 ++
target/hexagon/cpu_bits.h | 4 +
target/hexagon/macros.h| 3 -
linux-user/hexagon/cpu_loop.c | 4 +
target/hexagon/op_helper.c | 9 +--
tests/tcg/hexagon/unaligned_pc.c | 93 ++
tests/tcg/hexagon/Makefile.target | 4 +
tests/tcg/hexagon/unaligned_pc_multi_cof.S | 5 ++
8 files changed, 121 insertions(+), 8 deletions(-)
create mode 100644 tests/tcg/hexagon/unaligned_pc.c
create mode 100644 tests/tcg/hexagon/unaligned_pc_multi_cof.S
diff --git a/target/hexagon/cpu.h b/target/hexagon/cpu.h
index 3eef58fe8f..764f3c38cc 100644
--- a/target/hexagon/cpu.h
+++ b/target/hexagon/cpu.h
@@ -134,6 +134,10 @@ struct ArchCPU {
FIELD(TB_FLAGS, IS_TIGHT_LOOP, 0, 1)
+G_NORETURN void hexagon_raise_exception_err(CPUHexagonState *env,
+uint32_t exception,
+uintptr_t pc);
+
static inline void cpu_get_tb_cpu_state(CPUHexagonState *env, vaddr *pc,
uint64_t *cs_base, uint32_t *flags)
{
@@ -144,6 +148,9 @@ static inline void cpu_get_tb_cpu_state(CPUHexagonState
*env, vaddr *pc,
hex_flags = FIELD_DP32(hex_flags, TB_FLAGS, IS_TIGHT_LOOP, 1);
}
*flags = hex_flags;
+if (*pc & PCALIGN_MASK) {
+hexagon_raise_exception_err(env, HEX_EXCP_PC_NOT_ALIGNED, 0);
+}
}
typedef HexagonCPU ArchCPU;
diff --git a/target/hexagon/cpu_bits.h b/target/hexagon/cpu_bits.h
index 96fef71729..4279281a71 100644
--- a/target/hexagon/cpu_bits.h
+++ b/target/hexagon/cpu_bits.h
@@ -20,9 +20,13 @@
#include "qemu/bitops.h"
+#define PCALIGN 4
+#define PCALIGN_MASK (PCALIGN - 1)
+
#define HEX_EXCP_FETCH_NO_UPAGE 0x012
#define HEX_EXCP_INVALID_PACKET 0x015
#define HEX_EXCP_INVALID_OPCODE 0x015
+#define HEX_EXCP_PC_NOT_ALIGNED 0x01e
#define HEX_EXCP_PRIV_NO_UREAD 0x024
#define HEX_EXCP_PRIV_NO_UWRITE 0x025
diff --git a/target/hexagon/macros.h b/target/hexagon/macros.h
index 1376d6ccc1..f375471a98 100644
--- a/target/hexagon/macros.h
+++ b/target/hexagon/macros.h
@@ -22,9 +22,6 @@
#include "hex_regs.h"
#include "reg_fields.h"
-#define PCALIGN 4
-#define PCALIGN_MASK (PCALIGN - 1)
-
#define GET_FIELD(FIELD, REGIN) \
fEXTRACTU_BITS(REGIN, reg_field_info[FIELD].width, \
reg_field_info[FIELD].offset)
diff --git a/linux-user/hexagon/cpu_loop.c b/linux-user/hexagon/cpu_loop.c
index 7f1499ed28..d41159e52a 100644
--- a/linux-user/hexagon/cpu_loop.c
+++ b/linux-user/hexagon/cpu_loop.c
@@ -60,6 +60,10 @@ void cpu_loop(CPUHexagonState *env)
env->gpr[0] = ret;
}
break;
+case HEX_EXCP_PC_NOT_ALIGNED:
+force_sig_fault(TARGET_SIGBUS, TARGET_BUS_ADRALN,
+env->gpr[HEX_REG_R31]);
+break;
case EXCP_ATOMIC:
cpu_exec_step_atomic(cs);
break;
diff --git a/target/hexagon/op_helper.c b/target/hexagon/op_helper.c
index da10ac5847..ae5a605513 100644
--- a/target/hexagon/op_helper.c
+++ b/target/hexagon/op_helper.c
@@ -36,10 +36,9 @@
#define SF_MANTBITS23
/* Exceptions processing helpers */
-static G_NORETURN
-void do_raise_exception_err(CPUHexagonState *env,
-uint32_t exception,
-uintptr_t pc)
+G_NORETURN void hexagon_raise_exception_err(CPUHexagonState *env,
+uint32_t exception,
+uintptr_t pc)
{
CPUState *cs = env_cpu(env);
qemu_log_mask(CPU_LOG_INT, "%s: %d\n", __func__, exception);
@@ -49,7 +48,7 @@ void do_raise_exception_err(CPUHexagonState *env,
G_NORETURN void HELPER(raise_exception)(CPUHexagonState *env, uint32_t excp)
{
-do_raise_exception_err(env, excp, 0);
+hexagon_raise_exception_err(env, excp, 0);
}
void log_store32(CPUHexagonState *env, target_ulong addr,
diff --git a/tests/tcg/hexagon/unaligned_pc.c b/tests/tcg/hexagon/unaligned_pc.c
new file mode 100644
index 00..798e0a0f63
--- /dev/null
+++ b/tests/tcg/hexagon/unaligned_pc.c
@@ -0,0 +1,93 @@
+#include
+#include
+#include
+#include
+
+/* will be changed in signal handler */
+volatile sig_atomic_t compl
Re: [PATCH v3] Hexagon: add PC alignment check and exception
On Tue, 30 Apr 2024 08:52:36 -0700 Richard Henderson
wrote:
>
> On 4/30/24 07:25, Matheus Tavares Bernardino wrote:
> > +void test_multi_cof(void)
> > +{
> > +asm volatile(
> > +"p0 = cmp.eq(r0, r0)\n"
> > +"{\n"
> > +"if (p0) jump test_multi_cof_unaligned\n"
> > +"jump 1f\n"
> > +"}\n"
> > +"1: nop\n"
> > +: : : "p0");
> > +}
>
> I will say you could just add the label to the end of the asm here, like
>
> .byte 0
> test_multi_cof_unaligned:
>
> rather than use a separate source file.
>
That would be nice, but unfortunately that doesn't work because the label gets
aligned by the assembler :(
diff --git a/tests/tcg/hexagon/unaligned_pc.c b/tests/tcg/hexagon/unaligned_pc.c
index 1add2d0d99..3772947a86 100644
--- a/tests/tcg/hexagon/unaligned_pc.c
+++ b/tests/tcg/hexagon/unaligned_pc.c
@@ -33,10 +33,12 @@ void test_multi_cof(void)
asm volatile(
"p0 = cmp.eq(r0, r0)\n"
"{\n"
-"if (p0) jump test_multi_cof_unaligned\n"
+"if (p0) jump 2f\n"
"jump 1f\n"
"}\n"
"1: nop\n"
+".byte 0\n"
+"2: nop\n"
: : : "p0");
}
Ends up producing:
00020dc0 :
20dc0: 00 c0 9d a0 a09dc000 { allocframe(#0x0) }
20dc4: 00 c0 00 f2 f200c000 { p0 = cmp.eq(r0,r0) }
20dc8: 06 40 00 5c 5c004006 { if (p0) jump:nt 0x20dd4
20dcc: 04 c0 00 58 5800c004jump 0x20dd0
}
20dd0: 00 c0 00 7f 7f00c000 { nop }
20dd4: 00 00 c0 00
[PATCH v3] Hexagon: add PC alignment check and exception
The Hexagon Programmer's Reference Manual says that the exception 0x1e
should be raised upon an unaligned program counter. Let's implement that
and also add some tests.
Signed-off-by: Matheus Tavares Bernardino
---
v2:
https://lore.kernel.org/qemu-devel/e559b521d1920f804df10244c8c07564431aeba5.1714419461.git.quic_mathb...@quicinc.com/
Thanks for the comments, Richard and Taylor!
Changed in v3:
- Removed now unnecessary pkt_raises_exception addition.
- Added HEX_EXCP_PC_NOT_ALIGNED handling at
linux-user/hexagon/cpu_loop.c.
- Merged all tests into a C file that uses signal handler to check
that the exception was raised.
target/hexagon/cpu.h | 7 ++
target/hexagon/cpu_bits.h | 4 +
target/hexagon/macros.h| 3 -
linux-user/hexagon/cpu_loop.c | 4 +
target/hexagon/op_helper.c | 9 +--
tests/tcg/hexagon/unaligned_pc.c | 85 ++
tests/tcg/hexagon/Makefile.target | 4 +
tests/tcg/hexagon/unaligned_pc_multi_cof.S | 5 ++
8 files changed, 113 insertions(+), 8 deletions(-)
create mode 100644 tests/tcg/hexagon/unaligned_pc.c
create mode 100644 tests/tcg/hexagon/unaligned_pc_multi_cof.S
diff --git a/target/hexagon/cpu.h b/target/hexagon/cpu.h
index 3eef58fe8f..764f3c38cc 100644
--- a/target/hexagon/cpu.h
+++ b/target/hexagon/cpu.h
@@ -134,6 +134,10 @@ struct ArchCPU {
FIELD(TB_FLAGS, IS_TIGHT_LOOP, 0, 1)
+G_NORETURN void hexagon_raise_exception_err(CPUHexagonState *env,
+uint32_t exception,
+uintptr_t pc);
+
static inline void cpu_get_tb_cpu_state(CPUHexagonState *env, vaddr *pc,
uint64_t *cs_base, uint32_t *flags)
{
@@ -144,6 +148,9 @@ static inline void cpu_get_tb_cpu_state(CPUHexagonState
*env, vaddr *pc,
hex_flags = FIELD_DP32(hex_flags, TB_FLAGS, IS_TIGHT_LOOP, 1);
}
*flags = hex_flags;
+if (*pc & PCALIGN_MASK) {
+hexagon_raise_exception_err(env, HEX_EXCP_PC_NOT_ALIGNED, 0);
+}
}
typedef HexagonCPU ArchCPU;
diff --git a/target/hexagon/cpu_bits.h b/target/hexagon/cpu_bits.h
index 96fef71729..4279281a71 100644
--- a/target/hexagon/cpu_bits.h
+++ b/target/hexagon/cpu_bits.h
@@ -20,9 +20,13 @@
#include "qemu/bitops.h"
+#define PCALIGN 4
+#define PCALIGN_MASK (PCALIGN - 1)
+
#define HEX_EXCP_FETCH_NO_UPAGE 0x012
#define HEX_EXCP_INVALID_PACKET 0x015
#define HEX_EXCP_INVALID_OPCODE 0x015
+#define HEX_EXCP_PC_NOT_ALIGNED 0x01e
#define HEX_EXCP_PRIV_NO_UREAD 0x024
#define HEX_EXCP_PRIV_NO_UWRITE 0x025
diff --git a/target/hexagon/macros.h b/target/hexagon/macros.h
index 1376d6ccc1..f375471a98 100644
--- a/target/hexagon/macros.h
+++ b/target/hexagon/macros.h
@@ -22,9 +22,6 @@
#include "hex_regs.h"
#include "reg_fields.h"
-#define PCALIGN 4
-#define PCALIGN_MASK (PCALIGN - 1)
-
#define GET_FIELD(FIELD, REGIN) \
fEXTRACTU_BITS(REGIN, reg_field_info[FIELD].width, \
reg_field_info[FIELD].offset)
diff --git a/linux-user/hexagon/cpu_loop.c b/linux-user/hexagon/cpu_loop.c
index 7f1499ed28..d41159e52a 100644
--- a/linux-user/hexagon/cpu_loop.c
+++ b/linux-user/hexagon/cpu_loop.c
@@ -60,6 +60,10 @@ void cpu_loop(CPUHexagonState *env)
env->gpr[0] = ret;
}
break;
+case HEX_EXCP_PC_NOT_ALIGNED:
+force_sig_fault(TARGET_SIGBUS, TARGET_BUS_ADRALN,
+env->gpr[HEX_REG_R31]);
+break;
case EXCP_ATOMIC:
cpu_exec_step_atomic(cs);
break;
diff --git a/target/hexagon/op_helper.c b/target/hexagon/op_helper.c
index da10ac5847..ae5a605513 100644
--- a/target/hexagon/op_helper.c
+++ b/target/hexagon/op_helper.c
@@ -36,10 +36,9 @@
#define SF_MANTBITS23
/* Exceptions processing helpers */
-static G_NORETURN
-void do_raise_exception_err(CPUHexagonState *env,
-uint32_t exception,
-uintptr_t pc)
+G_NORETURN void hexagon_raise_exception_err(CPUHexagonState *env,
+uint32_t exception,
+uintptr_t pc)
{
CPUState *cs = env_cpu(env);
qemu_log_mask(CPU_LOG_INT, "%s: %d\n", __func__, exception);
@@ -49,7 +48,7 @@ void do_raise_exception_err(CPUHexagonState *env,
G_NORETURN void HELPER(raise_exception)(CPUHexagonState *env, uint32_t excp)
{
-do_raise_exception_err(env, excp, 0);
+hexagon_raise_exception_err(env, excp, 0);
}
void log_store32(CPUHexagonState *env, target_ulong addr,
diff --git a/tests/tcg/hexagon/unaligned_pc.c b/tests/tcg/hexagon/unaligned_pc.c
new file mode 100644
index 00..1add2d0d99
--- /dev/null
+++ b/tests/tcg/hexagon/unaligned_pc.c
@@ -0,0 +1,85 @@
+#include
+#include
+#include
+#include
+
+/*
[PATCH v2] Hexagon: add PC alignment check and exception
The Hexagon Programmer's Reference Manual says that the exception 0x1e
should be raised upon an unaligned program counter. Let's implement that
and also add tests for both the most common case as well as packets with
multiple change-of-flow instructions.
Signed-off-by: Matheus Tavares Bernardino
---
v1:
https://lore.kernel.org/qemu-devel/c7af62451b02ffdc1d68bc00093b40a8080bc3ff.1714155331.git.quic_mathb...@quicinc.com/
Changes in v2:
- Moved PC alignment check from gen_write_new_pc_addr to
cpu_get_tb_cpu_state, in order to get the right PC address at
exception handling. (And also include relative PC instructions)
- Added test for endloop.
- Updated pkt_raises_exception for A_COF instructions.
target/hexagon/cpu.h | 7 +++
target/hexagon/cpu_bits.h | 4
target/hexagon/macros.h| 3 ---
target/hexagon/op_helper.c | 9 -
target/hexagon/translate.c | 5 +++--
tests/tcg/hexagon/Makefile.target | 16
tests/tcg/hexagon/unaligned_pc.S | 10 ++
tests/tcg/hexagon/unaligned_pc_endloop.S | 8
tests/tcg/hexagon/unaligned_pc_multi_cof.S | 13 +
9 files changed, 65 insertions(+), 10 deletions(-)
create mode 100644 tests/tcg/hexagon/unaligned_pc.S
create mode 100644 tests/tcg/hexagon/unaligned_pc_endloop.S
create mode 100644 tests/tcg/hexagon/unaligned_pc_multi_cof.S
diff --git a/target/hexagon/cpu.h b/target/hexagon/cpu.h
index 3eef58fe8f..764f3c38cc 100644
--- a/target/hexagon/cpu.h
+++ b/target/hexagon/cpu.h
@@ -134,6 +134,10 @@ struct ArchCPU {
FIELD(TB_FLAGS, IS_TIGHT_LOOP, 0, 1)
+G_NORETURN void hexagon_raise_exception_err(CPUHexagonState *env,
+uint32_t exception,
+uintptr_t pc);
+
static inline void cpu_get_tb_cpu_state(CPUHexagonState *env, vaddr *pc,
uint64_t *cs_base, uint32_t *flags)
{
@@ -144,6 +148,9 @@ static inline void cpu_get_tb_cpu_state(CPUHexagonState
*env, vaddr *pc,
hex_flags = FIELD_DP32(hex_flags, TB_FLAGS, IS_TIGHT_LOOP, 1);
}
*flags = hex_flags;
+if (*pc & PCALIGN_MASK) {
+hexagon_raise_exception_err(env, HEX_EXCP_PC_NOT_ALIGNED, 0);
+}
}
typedef HexagonCPU ArchCPU;
diff --git a/target/hexagon/cpu_bits.h b/target/hexagon/cpu_bits.h
index 96fef71729..4279281a71 100644
--- a/target/hexagon/cpu_bits.h
+++ b/target/hexagon/cpu_bits.h
@@ -20,9 +20,13 @@
#include "qemu/bitops.h"
+#define PCALIGN 4
+#define PCALIGN_MASK (PCALIGN - 1)
+
#define HEX_EXCP_FETCH_NO_UPAGE 0x012
#define HEX_EXCP_INVALID_PACKET 0x015
#define HEX_EXCP_INVALID_OPCODE 0x015
+#define HEX_EXCP_PC_NOT_ALIGNED 0x01e
#define HEX_EXCP_PRIV_NO_UREAD 0x024
#define HEX_EXCP_PRIV_NO_UWRITE 0x025
diff --git a/target/hexagon/macros.h b/target/hexagon/macros.h
index 1376d6ccc1..f375471a98 100644
--- a/target/hexagon/macros.h
+++ b/target/hexagon/macros.h
@@ -22,9 +22,6 @@
#include "hex_regs.h"
#include "reg_fields.h"
-#define PCALIGN 4
-#define PCALIGN_MASK (PCALIGN - 1)
-
#define GET_FIELD(FIELD, REGIN) \
fEXTRACTU_BITS(REGIN, reg_field_info[FIELD].width, \
reg_field_info[FIELD].offset)
diff --git a/target/hexagon/op_helper.c b/target/hexagon/op_helper.c
index da10ac5847..ae5a605513 100644
--- a/target/hexagon/op_helper.c
+++ b/target/hexagon/op_helper.c
@@ -36,10 +36,9 @@
#define SF_MANTBITS23
/* Exceptions processing helpers */
-static G_NORETURN
-void do_raise_exception_err(CPUHexagonState *env,
-uint32_t exception,
-uintptr_t pc)
+G_NORETURN void hexagon_raise_exception_err(CPUHexagonState *env,
+uint32_t exception,
+uintptr_t pc)
{
CPUState *cs = env_cpu(env);
qemu_log_mask(CPU_LOG_INT, "%s: %d\n", __func__, exception);
@@ -49,7 +48,7 @@ void do_raise_exception_err(CPUHexagonState *env,
G_NORETURN void HELPER(raise_exception)(CPUHexagonState *env, uint32_t excp)
{
-do_raise_exception_err(env, excp, 0);
+hexagon_raise_exception_err(env, excp, 0);
}
void log_store32(CPUHexagonState *env, target_ulong addr,
diff --git a/target/hexagon/translate.c b/target/hexagon/translate.c
index 47a870f42d..26e6809976 100644
--- a/target/hexagon/translate.c
+++ b/target/hexagon/translate.c
@@ -346,8 +346,9 @@ static void mark_implicit_pred_writes(DisasContext *ctx)
static bool pkt_raises_exception(Packet *pkt)
{
if (check_for_attrib(pkt, A_LOAD) ||
-check_for_attrib(pkt, A_STORE)) {
-return true;
+check_for_attrib(pkt, A_STORE) ||
+check_for_attrib(pkt, A_COF)) {
+return true;
}
return false;
}
diff --git a/tests/tcg/hexagon/Makefile.target
b/tests/tc
RE: [PATCH] Hexagon: add PC alignment check and exception
Hi, Taylor On Mon, 29 Apr 2024 09:51:16 -0500 wrote: > > PS You should also update the pkt_raises_exception function in translate.c > to return true for packets that contain these instructions. This will > ensure that none of the machine state is changed before the check is > complete. Will do, thanks. > > -Original Message- > > From: ltaylorsimp...@gmail.com > > > > > > > > > -Original Message- > > > From: Matheus Tavares Bernardino > > > > > > > > > --- a/target/hexagon/genptr.c > > > +++ b/target/hexagon/genptr.c > > > @@ -473,6 +473,7 @@ static void gen_write_new_pc_addr(DisasContext > > > > You haven't added the check to gen_write_new_pc_pcrel. It's not needed > > there because the encoding guarantees the target is always aligned - > right? > > However, there is a call to gen_write_new_pc_addr inside that function. > In > > this case, we'll add a check that isn't necessary. Consider adding a > parameter > > to indicate if the check can be avoided. Actually, I had missed this spot and I think we do need the check at gen_write_new_pc_pcrel too. The added test `unaligned_pc_multi_cof.S` will exercise it with a relative PC addr that is not aligned. I'll fix that, thanks. > > > a/tests/tcg/hexagon/Makefile.target > > > b/tests/tcg/hexagon/Makefile.target > > > index f839b2c0d5..02d7fff34c 100644 > > > --- a/tests/tcg/hexagon/Makefile.target > > > +++ b/tests/tcg/hexagon/Makefile.target > > > @@ -51,6 +51,19 @@ HEX_TESTS += scatter_gather HEX_TESTS +> hvx_misc > > > HEX_TESTS += hvx_histogram HEX_TESTS += invalid-slots > > > +HEX_TESTS += unaligned_pc > > > +HEX_TESTS += unaligned_pc_multi_cof > > > + > > > +run-unaligned_pc: unaligned_pc > > > +run-unaligned_pc_multi_cof: unaligned_pc_multi_cof run-unaligned_pc > > > +run-unaligned_pc_multi_cof: > > > + $(call run-test, $<, $(QEMU) $< 2> $<.stderr,"$< on > > > $(TARGET_NAME)"); \ > > > + if [ $$? -ne 1 ] ; then \ > > > + return 1; \ > > > + fi > > > + $(call quiet-command, \ > > > + grep -q "exception 0x1e" $<.stderr, \ > > > + "GREP", "exception 0x1e"); > > > > We should also test endloop instructions. Good idea, will do.
[PATCH] Hexagon: add PC alignment check and exception
The Hexagon Programmer's Reference Manual says that the exception 0x1e
should be raised upon an unaligned program counter. Let's implement that
and also add tests for both the most common case as well as packets with
multiple change-of-flow instructions.
Signed-off-by: Matheus Tavares Bernardino
---
target/hexagon/cpu_bits.h | 1 +
target/hexagon/translate.h | 2 ++
target/hexagon/genptr.c| 21 -
target/hexagon/translate.c | 2 +-
tests/tcg/hexagon/Makefile.target | 13 +
tests/tcg/hexagon/unaligned_pc.S | 10 ++
tests/tcg/hexagon/unaligned_pc_multi_cof.S | 13 +
7 files changed, 56 insertions(+), 6 deletions(-)
create mode 100644 tests/tcg/hexagon/unaligned_pc.S
create mode 100644 tests/tcg/hexagon/unaligned_pc_multi_cof.S
diff --git a/target/hexagon/cpu_bits.h b/target/hexagon/cpu_bits.h
index 96fef71729..d6900c8bda 100644
--- a/target/hexagon/cpu_bits.h
+++ b/target/hexagon/cpu_bits.h
@@ -23,6 +23,7 @@
#define HEX_EXCP_FETCH_NO_UPAGE 0x012
#define HEX_EXCP_INVALID_PACKET 0x015
#define HEX_EXCP_INVALID_OPCODE 0x015
+#define HEX_EXCP_PC_NOT_ALIGNED 0x01e
#define HEX_EXCP_PRIV_NO_UREAD 0x024
#define HEX_EXCP_PRIV_NO_UWRITE 0x025
diff --git a/target/hexagon/translate.h b/target/hexagon/translate.h
index 4dd59c6726..daf11eb584 100644
--- a/target/hexagon/translate.h
+++ b/target/hexagon/translate.h
@@ -75,6 +75,8 @@ typedef struct DisasContext {
TCGv dczero_addr;
} DisasContext;
+void gen_exception_end_tb(DisasContext *ctx, int excp);
+
static inline void ctx_log_pred_write(DisasContext *ctx, int pnum)
{
if (!test_bit(pnum, ctx->pregs_written)) {
diff --git a/target/hexagon/genptr.c b/target/hexagon/genptr.c
index dbae6c570a..c96edd9379 100644
--- a/target/hexagon/genptr.c
+++ b/target/hexagon/genptr.c
@@ -473,6 +473,7 @@ static void gen_write_new_pc_addr(DisasContext *ctx, TCGv
addr,
TCGCond cond, TCGv pred)
{
TCGLabel *pred_false = NULL;
+TCGLabel *branch_taken = NULL;
if (cond != TCG_COND_ALWAYS) {
pred_false = gen_new_label();
tcg_gen_brcondi_tl(cond, pred, 0, pred_false);
@@ -480,12 +481,22 @@ static void gen_write_new_pc_addr(DisasContext *ctx, TCGv
addr,
if (ctx->pkt->pkt_has_multi_cof) {
/* If there are multiple branches in a packet, ignore the second one */
-tcg_gen_movcond_tl(TCG_COND_NE, hex_gpr[HEX_REG_PC],
- ctx->branch_taken, tcg_constant_tl(0),
- hex_gpr[HEX_REG_PC], addr);
+branch_taken = gen_new_label();
+tcg_gen_brcondi_tl(TCG_COND_NE, ctx->branch_taken, 0, branch_taken);
tcg_gen_movi_tl(ctx->branch_taken, 1);
-} else {
-tcg_gen_mov_tl(hex_gpr[HEX_REG_PC], addr);
+}
+
+TCGLabel *pc_aligned = gen_new_label();
+TCGv pc_remainder = tcg_temp_new();
+tcg_gen_andi_tl(pc_remainder, addr, PCALIGN_MASK);
+tcg_gen_brcondi_tl(TCG_COND_EQ, pc_remainder, 0, pc_aligned);
+gen_exception_end_tb(ctx, HEX_EXCP_PC_NOT_ALIGNED);
+gen_set_label(pc_aligned);
+
+tcg_gen_mov_tl(hex_gpr[HEX_REG_PC], addr);
+
+if (ctx->pkt->pkt_has_multi_cof) {
+gen_set_label(branch_taken);
}
if (cond != TCG_COND_ALWAYS) {
diff --git a/target/hexagon/translate.c b/target/hexagon/translate.c
index f163eefe97..e6ee63a53e 100644
--- a/target/hexagon/translate.c
+++ b/target/hexagon/translate.c
@@ -185,7 +185,7 @@ static void gen_end_tb(DisasContext *ctx)
ctx->base.is_jmp = DISAS_NORETURN;
}
-static void gen_exception_end_tb(DisasContext *ctx, int excp)
+void gen_exception_end_tb(DisasContext *ctx, int excp)
{
gen_exec_counters(ctx);
tcg_gen_movi_tl(hex_gpr[HEX_REG_PC], ctx->next_PC);
diff --git a/tests/tcg/hexagon/Makefile.target
b/tests/tcg/hexagon/Makefile.target
index f839b2c0d5..02d7fff34c 100644
--- a/tests/tcg/hexagon/Makefile.target
+++ b/tests/tcg/hexagon/Makefile.target
@@ -51,6 +51,19 @@ HEX_TESTS += scatter_gather
HEX_TESTS += hvx_misc
HEX_TESTS += hvx_histogram
HEX_TESTS += invalid-slots
+HEX_TESTS += unaligned_pc
+HEX_TESTS += unaligned_pc_multi_cof
+
+run-unaligned_pc: unaligned_pc
+run-unaligned_pc_multi_cof: unaligned_pc_multi_cof
+run-unaligned_pc run-unaligned_pc_multi_cof:
+ $(call run-test, $<, $(QEMU) $< 2> $<.stderr,"$< on $(TARGET_NAME)"); \
+ if [ $$? -ne 1 ] ; then \
+ return 1; \
+ fi
+ $(call quiet-command, \
+ grep -q "exception 0x1e" $<.stderr, \
+ "GREP", "exception 0x1e");
run-and-check-exception = $(call run-test,$2,$3 2>$2.stderr; \
test $$? -eq 1 && grep -q "exception $(strip $1)" $2.stderr)
diff --git a/tests/tcg/hexagon/unaligned_pc.S b/tests/tcg/hexagon/unaligned_pc.S
new fi
Re: [PATCH] Makefile: fix use of -j without an argument
On Fri, 12 Apr 2024 10:02:54 +0200 Paolo Bonzini wrote: > > On Thu, Apr 11, 2024 at 5:46 PM Matheus Tavares Bernardino > wrote: > > +$(if $(filter -j, $(MAKEFLAGS)) \ > > +,, \ > > +$(or \ > > + $(filter -l% -j%, $(MAKEFLAGS)), \ > > + $(if $(filter --jobserver-auth=%, $(MAKEFLAGS)),, -j1)) \ > > +) -d keepdepfile > > This is more easily written as $(filter-out -j, $(or ...)). > > I've sent a v2. Thanks!
Re: [PATCH] Makefile: fix use of -j without an argument
Hi, Philippe On Thu, 11 Apr 2024 17:29:58 +0200 =?UTF-8?Q?Philippe_Mathieu-Daud=C3=A9?= wrote: > > Hi Matheus, > > On 11/4/24 17:09, Matheus Tavares Bernardino wrote: > > Our Makefile massages the given make arguments to invoke ninja > > accordingly. One key difference is that ninja will parallelize by > > default, whereas make only does so with -j or -j. The make man page > > says that "if the -j option is given without an argument, make will not > > limit the number of jobs that can run simultaneously". We use to support > > that by replacing -j with "" (empty string) when calling ninja, so that > > it would do its auto-parallelization based on the number of CPU cores. > > > > This was accidentally broken at d1ce2cc95b (Makefile: preserve > > --jobserver-auth argument when calling ninja, 2024-04-02), > > causing `make -j` to fail: > > > > $ make -j V=1 > >/usr/bin/ninja -v -j -d keepdepfile all | cat > >make -C contrib/plugins/ V="1" TARGET_DIR="contrib/plugins/" all > >ninja: fatal: invalid -j parameter > >make: *** [Makefile:161: run-ninja] Error > > > > Let's fix that and indent the touched code for better readability. > > > > Signed-off-by: Matheus Tavares Bernardino > > --- > > Makefile | 8 ++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/Makefile b/Makefile > > index 183756018f..d299c14dab 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -142,8 +142,12 @@ MAKE.k = $(findstring k,$(firstword $(filter-out > > --%,$(MAKEFLAGS > > MAKE.q = $(findstring q,$(firstword $(filter-out --%,$(MAKEFLAGS > > MAKE.nq = $(if $(word 2, $(MAKE.n) $(MAKE.q)),nq) > > NINJAFLAGS = $(if $V,-v) $(if $(MAKE.n), -n) $(if $(MAKE.k), -k0) \ > > -$(or $(filter -l% -j%, $(MAKEFLAGS)), $(if $(filter > > --jobserver-auth=%, $(MAKEFLAGS)),, -j1)) \ > > --d keepdepfile > > +$(if $(filter -j, $(MAKEFLAGS)) \ > > +,, \ > > +$(or \ > > + $(filter -l% -j%, $(MAKEFLAGS)), \ > > + $(if $(filter --jobserver-auth=%, $(MAKEFLAGS)),, -j1)) \ > > +) -d keepdepfile > > ninja-cmd-goals = $(or $(MAKECMDGOALS), all) > > ninja-cmd-goals += $(foreach g, $(MAKECMDGOALS), $(.ninja-goals.$g)) > > > > Apparently Martin sent the same patch (although not as nicely > indented) and Paolo queued it: > https://lore.kernel.org/qemu-devel/20240402081738.1051560-1-mar...@geanix.com/ Actually, this patch is a follow-up to that one, fixing a feature that was accidentally broken.
[PATCH] Makefile: fix use of -j without an argument
Our Makefile massages the given make arguments to invoke ninja accordingly. One key difference is that ninja will parallelize by default, whereas make only does so with -j or -j. The make man page says that "if the -j option is given without an argument, make will not limit the number of jobs that can run simultaneously". We use to support that by replacing -j with "" (empty string) when calling ninja, so that it would do its auto-parallelization based on the number of CPU cores. This was accidentally broken at d1ce2cc95b (Makefile: preserve --jobserver-auth argument when calling ninja, 2024-04-02), causing `make -j` to fail: $ make -j V=1 /usr/bin/ninja -v -j -d keepdepfile all | cat make -C contrib/plugins/ V="1" TARGET_DIR="contrib/plugins/" all ninja: fatal: invalid -j parameter make: *** [Makefile:161: run-ninja] Error Let's fix that and indent the touched code for better readability. Signed-off-by: Matheus Tavares Bernardino --- Makefile | 8 ++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index 183756018f..d299c14dab 100644 --- a/Makefile +++ b/Makefile @@ -142,8 +142,12 @@ MAKE.k = $(findstring k,$(firstword $(filter-out --%,$(MAKEFLAGS MAKE.q = $(findstring q,$(firstword $(filter-out --%,$(MAKEFLAGS MAKE.nq = $(if $(word 2, $(MAKE.n) $(MAKE.q)),nq) NINJAFLAGS = $(if $V,-v) $(if $(MAKE.n), -n) $(if $(MAKE.k), -k0) \ -$(or $(filter -l% -j%, $(MAKEFLAGS)), $(if $(filter --jobserver-auth=%, $(MAKEFLAGS)),, -j1)) \ --d keepdepfile +$(if $(filter -j, $(MAKEFLAGS)) \ +,, \ +$(or \ + $(filter -l% -j%, $(MAKEFLAGS)), \ + $(if $(filter --jobserver-auth=%, $(MAKEFLAGS)),, -j1)) \ +) -d keepdepfile ninja-cmd-goals = $(or $(MAKECMDGOALS), all) ninja-cmd-goals += $(foreach g, $(MAKECMDGOALS), $(.ninja-goals.$g)) -- 2.37.2
Re: [PATCH 4/9] Hexagon (target/hexagon) Mark has_pred_dest in trans functions
On Mon, 26 Feb 2024 13:17:17 -0700 Taylor Simpson wrote: > > diff --git a/target/hexagon/gen_trans_funcs.py > b/target/hexagon/gen_trans_funcs.py > index 07292e0170..f1972fd2dd 100755 > --- a/target/hexagon/gen_trans_funcs.py > +++ b/target/hexagon/gen_trans_funcs.py > @@ -86,6 +86,7 @@ def gen_trans_funcs(f): > > new_read_idx = -1 > dest_idx = -1 > +has_pred_dest = "false" > for regno, regstruct in enumerate(regs): > reg_type, reg_id, _, _ = regstruct > reg = hex_common.get_register(tag, reg_type, reg_id) > @@ -96,6 +97,8 @@ def gen_trans_funcs(f): > new_read_idx = regno > if reg.is_written() and dest_idx == -1: > dest_idx = regno > +if reg_type == "P" and not reg.is_read(): > +has_pred_dest = "true" I got a bit confused here. Why do we use "not reg.is_read()"? I though this would be "reg.is_written()".
Re: [PATCH 3/9] Hexagon (target/hexagon) Mark dest_idx in trans functions
On Mon, 26 Feb 2024 13:17:16 -0700 Taylor Simpson wrote: > > diff --git a/target/hexagon/gen_trans_funcs.py > b/target/hexagon/gen_trans_funcs.py > index 79475b2946..07292e0170 100755 > --- a/target/hexagon/gen_trans_funcs.py > +++ b/target/hexagon/gen_trans_funcs.py > @@ -85,6 +85,7 @@ def gen_trans_funcs(f): > """)) > > new_read_idx = -1 > +dest_idx = -1 > for regno, regstruct in enumerate(regs): > reg_type, reg_id, _, _ = regstruct > reg = hex_common.get_register(tag, reg_type, reg_id) > @@ -93,6 +94,8 @@ def gen_trans_funcs(f): > """)) > if reg.is_read() and reg.is_new(): > new_read_idx = regno > +if reg.is_written() and dest_idx == -1: > +dest_idx = regno I was first wondering what should we do when "reg.is_written()" and "dest_idx != -1". But then I remembered we previously used strchr(), so we would stop at the first match anyways.
Re: [PATCH 2/9] Hexagon (target/hexagon) Mark new_read_idx in trans functions
On Mon, 26 Feb 2024 13:17:15 -0700 Taylor Simpson
wrote:
>
> diff --git a/target/hexagon/gen_trans_funcs.py
> b/target/hexagon/gen_trans_funcs.py
> index 53e844a44b..79475b2946 100755
> --- a/target/hexagon/gen_trans_funcs.py
> +++ b/target/hexagon/gen_trans_funcs.py
> @@ -84,14 +84,15 @@ def gen_trans_funcs(f):
> insn->opcode = {tag};
> """))
>
> -regno = 0
> -for reg in regs:
> -reg_type = reg[0]
> -reg_id = reg[1]
> +new_read_idx = -1
> +for regno, regstruct in enumerate(regs):
> +reg_type, reg_id, _, _ = regstruct
> +reg = hex_common.get_register(tag, reg_type, reg_id)
Nit: since we don't care about the remaining elements of regstruct, we
could simplify (and future-proof) this even further to:
reg_type, reg_id, *_ = regstruct
Or perhaps even eliminate the variable entirely:
for regno, (reg_type, reg_id, *_) in enumerate(regs):
...
> f.write(code_fmt(f"""\
> insn->regno[{regno}] = args->{reg_type}{reg_id};
> """))
> -regno += 1
> +if reg.is_read() and reg.is_new():
> +new_read_idx = regno
>
> if len(imms) != 0:
> mark_which_imm_extended(f, tag)
Re: [PATCH] tests/docker: Hexagon toolchain update
On Sun, 14 Jan 2024 15:23:54 -0800 Brian Cain wrote: > > This update includes support for privileged instructions. > > Signed-off-by: Brian Cain > --- Reviewed-by: Matheus Tavares Bernardino Tested-by: Matheus Tavares Bernardino > diff --git a/tests/docker/dockerfiles/debian-hexagon-cross.docker > b/tests/docker/dockerfiles/debian-hexagon-cross.docker > index 7c38d7c9e4..60bd8faa20 100644 > --- a/tests/docker/dockerfiles/debian-hexagon-cross.docker > +++ b/tests/docker/dockerfiles/debian-hexagon-cross.docker > @@ -38,9 +38,9 @@ RUN apt-get update && \ > RUN /usr/bin/pip3 install tomli > > ENV TOOLCHAIN_INSTALL /opt > -ENV TOOLCHAIN_RELEASE 16.0.0 > +ENV TOOLCHAIN_RELEASE 12.Dec.2023 FWIW, the changes can be seen here: https://github.com/quic/toolchain_for_hexagon/compare/v16.0.0...v12.Dec.2023
Re: [PATCH] Reduce scope of def_regnum, remove dead assignment
On Sun, 14 Jan 2024 15:44:53 -0800 Brian Cain wrote: > > This is intended to address a coverity finding: CID 1527408. > > Signed-off-by: Brian Cain > --- Reviewed-by: Matheus Tavares Bernardino
Re: [RFC PATCH] Hexagon (target/hexagon) Make generators object oriented
Taylor Simpson wrote: > > RFC - This patch handles gen_tcg_funcs.py. I'd like to get comments > on the general approach before working on the other Python scripts. > > The generators are generally a bunch of Python if-then-else > statements based on the regtype and regid. Encapsulate regtype/regid > into a class hierarchy. Clients lookup the register and invoke > methods. > > This has several advantages for making the code easier to read, > understand, and maintain > - The class name makes it more clear what the operand does > - All the methods for a given type of operand are together > - Don't need as many calls to hex_common.bad_register > - We can remove the functions in hex_common that use regtype/regid > (e.g., is_read) > > Signed-off-by: Taylor Simpson Really nice! I personally think it's a great separation and it improves the code readability.
prefix gets lost on config regen for cross Windows build
[resending as it looks like there was some delivery issue with the first msg] Hi, It seems that we lose the install prefix option when regenerating config for a Windows cross-build. Looks like this behavior appeared with c36dd41ba2 (configure: move target-specific defaults to an external machine file, 2023-10-16), but I couldn't find the specific root cause yet. To reproduce the issue, first run: ../configure --prefix=/tmp/install --without-default-features \ --cross-prefix=x86_64-w64-mingw32- --target-list=aarch64-softmmu Which correctly prints: Directories [...] Install prefix: /tmp/install But then if we run `touch ../meson.build && make`, we get: Directories [...] Install prefix: /qemu Removing the `prefix = '/qemu'` line from configs/meson/windows.txt does fix the issue, but I don't understand why the CLI option is not taking precedence over it Or even if this could be a meson bug itself. I'd appreciate any help. Thanks, Matheus
--prefix gets lost on config regen for cross Windows build
Hi, It seems that we lose the install prefix option when regenerating config for a Windows cross-build. Looks like this behavior appeared with c36dd41ba2 (configure: move target-specific defaults to an external machine file, 2023-10-16), but I couldn't find the specific root cause yet. To reproduce the issue, first run: ../configure --prefix=/tmp/install --without-default-features \ --cross-prefix=x86_64-w64-mingw32- --target-list=aarch64-softmmu Which correctly prints: Directories [...] Install prefix: /tmp/install But then if we run `touch ../meson.build && make`, we get: Directories [...] Install prefix: /qemu Reverting c36dd41ba2 does fix the issue, but I couldn't yet find what is causing this among the changes. Or even if it could be a meson bug itself. I'd appreciate any help. Thanks, Matheus
[PATCH v2] semihosting: fix memleak at semihosting_arg_fallback
We duplicate "cmd" as strtok may modify its argument, but we forgot
to free it later. Furthermore, add_semihosting_arg doesn't take
responsibility for this memory either (it strdup's the argument).
Signed-off-by: Matheus Tavares Bernardino
Reviewed-by: Philippe Mathieu-Daudé
---
semihosting/config.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/semihosting/config.c b/semihosting/config.c
index 249a377ae8..56283b5c3c 100644
--- a/semihosting/config.c
+++ b/semihosting/config.c
@@ -113,12 +113,13 @@ static int add_semihosting_arg(void *opaque,
void semihosting_arg_fallback(const char *file, const char *cmd)
{
char *cmd_token;
+g_autofree char *cmd_dup = g_strdup(cmd);
/* argv[0] */
add_semihosting_arg(&semihosting, "arg", file, NULL);
/* split -append and initialize argv[1..n] */
-cmd_token = strtok(g_strdup(cmd), " ");
+cmd_token = strtok(cmd_dup, " ");
while (cmd_token) {
add_semihosting_arg(&semihosting, "arg", cmd_token, NULL);
cmd_token = strtok(NULL, " ");
--
2.37.2
[PATCH] semihosting: fix memleak at semihosting_arg_fallback
We duplicate "cmd" as strtok may modify its argument, but we forgot
to free it later. Furthermore, add_semihosting_arg doesn't take
responsibility for this memory either (it strdup's the argument).
Signed-off-by: Matheus Tavares Bernardino
---
semihosting/config.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/semihosting/config.c b/semihosting/config.c
index 249a377ae8..32aa210460 100644
--- a/semihosting/config.c
+++ b/semihosting/config.c
@@ -112,17 +112,19 @@ static int add_semihosting_arg(void *opaque,
/* Use strings passed via -kernel/-append to initialize semihosting.argv[] */
void semihosting_arg_fallback(const char *file, const char *cmd)
{
-char *cmd_token;
+char *cmd_token, *cmd_dup;
/* argv[0] */
add_semihosting_arg(&semihosting, "arg", file, NULL);
/* split -append and initialize argv[1..n] */
-cmd_token = strtok(g_strdup(cmd), " ");
+cmd_dup = g_strdup(cmd);
+cmd_token = strtok(cmd_dup, " ");
while (cmd_token) {
add_semihosting_arg(&semihosting, "arg", cmd_token, NULL);
cmd_token = strtok(NULL, " ");
}
+g_free(cmd_dup);
}
void qemu_semihosting_enable(void)
--
2.37.2
[PATCH] hw/display: fix memleak from virtio_add_resource
When the given uuid is already present in the hash table,
virtio_add_resource() does not add the passed VirtioSharedObject. In
this case, free it in the callers to avoid leaking memory. This fixed
the following `make check` error, when built with --enable-sanitizers:
4/166 qemu:unit / test-virtio-dmabuf ERROR 1.51s exit status 1
==7716==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 320 byte(s) in 20 object(s) allocated from:
#0 0x7f6fc16e3808 in __interceptor_malloc
../../../../src/libsanitizer/asan/asan_malloc_linux.cc:144
#1 0x7f6fc1503e98 in g_malloc
(/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x57e98)
#2 0x564d63cafb6b in test_add_invalid_resource
../tests/unit/test-virtio-dmabuf.c:100
#3 0x7f6fc152659d (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x7a59d)
SUMMARY: AddressSanitizer: 320 byte(s) leaked in 20 allocation(s).
The changes at virtio_add_resource() itself are not strictly necessary
for the memleak fix, but they make it more obvious that, on an error
return, the passed object is not added to the hash.
Signed-off-by: Matheus Tavares Bernardino
---
hw/display/virtio-dmabuf.c | 12 ++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/hw/display/virtio-dmabuf.c b/hw/display/virtio-dmabuf.c
index 4a8e430f3d..3dba4577ca 100644
--- a/hw/display/virtio-dmabuf.c
+++ b/hw/display/virtio-dmabuf.c
@@ -29,7 +29,7 @@ static int uuid_equal_func(const void *lhv, const void *rhv)
static bool virtio_add_resource(QemuUUID *uuid, VirtioSharedObject *value)
{
-bool result = false;
+bool result = true;
g_mutex_lock(&lock);
if (resource_uuids == NULL) {
@@ -39,7 +39,9 @@ static bool virtio_add_resource(QemuUUID *uuid,
VirtioSharedObject *value)
g_free);
}
if (g_hash_table_lookup(resource_uuids, uuid) == NULL) {
-result = g_hash_table_insert(resource_uuids, uuid, value);
+g_hash_table_insert(resource_uuids, uuid, value);
+} else {
+result = false;
}
g_mutex_unlock(&lock);
@@ -57,6 +59,9 @@ bool virtio_add_dmabuf(QemuUUID *uuid, int udmabuf_fd)
vso->type = TYPE_DMABUF;
vso->value = GINT_TO_POINTER(udmabuf_fd);
result = virtio_add_resource(uuid, vso);
+if (!result) {
+g_free(vso);
+}
return result;
}
@@ -72,6 +77,9 @@ bool virtio_add_vhost_device(QemuUUID *uuid, struct vhost_dev
*dev)
vso->type = TYPE_VHOST_DEV;
vso->value = dev;
result = virtio_add_resource(uuid, vso);
+if (!result) {
+g_free(vso);
+}
return result;
}
--
2.37.2
Re: [PULL v2 40/44] gdbstub: add test for untimely stop-reply packets
Hi, Richard Richard Henderson wrote: > > On 5/18/23 13:04, Taylor Simpson wrote: > > From: Matheus Tavares Bernardino > > > > In the previous commit, we modified gdbstub.c to only send stop-reply > > packets as a response to GDB commands that accept it. Now, let's add a > > test for this intended behavior. Running this test before the fix from > > the previous commit fails as QEMU sends a stop-reply packet > > asynchronously, when GDB was in fact waiting an ACK. > > > > Signed-off-by: Matheus Tavares Bernardino > > Acked-by: Alex Bennée > > Signed-off-by: Taylor Simpson > > Message-Id: > > > > --- > > tests/guest-debug/run-test.py| 16 > > .../tcg/multiarch/system/Makefile.softmmu-target | 16 +++- > > 2 files changed, 27 insertions(+), 5 deletions(-) > > This test is failing for me on x86_64 and aarch64 host, aarch64 guest: > > > qemu-system-aarch64: -gdb > unix:path=/tmp/tmptlr0fa8hqemu-gdbstub/gdbstub.socket,server=on: > info: QEMU waiting for connection on: > disconnected:unix:/tmp/tmptlr0fa8hqemu-gdbstub/gdbstub.socket,server=on > qemu-system-aarch64: warning: gdbstub: client sent packet while target running > >GREPfile >untimely-packet.gdb.err > make[1]: *** > [/home/rth/qemu/src/tests/tcg/multiarch/system/Makefile.softmmu-target:33: > run-gdbstub-untimely-packet] Error 1 This looks like the recent breakage I reported at https://lore.kernel.org/qemu-devel/456ed3318421dd7946bdfb5ceda7e05332da368c.1690910333.git.quic_mathb...@quicinc.com/
Re: [PATCH] gdbstub: use 0 ("any process") on packets with no PID
Ilya Leoshkevich wrote: > > On Tue, 2023-08-01 at 12:37 -0300, Matheus Tavares Bernardino wrote: > > Previously, qemu-user would always report PID 1 to GDB. This was > > changed > > at dc14a7a6e9 (gdbstub: Report the actual qemu-user pid, 2023-06-30), > > but read_thread_id() still considers GDB packets with "no PID" as > > "PID > > 1", which is not the qemu-user PID. Fix that by parsing "no PID" as > > "0", > > which the GDB Remote Protocol defines as "any process". > > > > Note that this should have no effect for system emulation as, in this > > case, gdb_create_default_process() will assign PID 1 for the first > > process and that is what the gdbstub uses for GDB requests with no > > PID, > > or PID 0. > > > > This issue was found with hexagon-lldb, which sends a "Hq" packet > > with > > only the thread-id, but no process-id, leading to the invalid usage > > of > > "PID 1" by qemu-hexagon and a subsequent "E22" reply. > > Did you mean "Hg"? Oops, that's right, thanks.
Re: [PATCH] gdbstub: Fix client Ctrl-C handling
Hi, Nick.
> Nicholas Piggin wrote:
>
> On Tue Jul 11, 2023 at 9:03 PM AEST, Matheus Tavares Bernardino wrote:
> > > Nicholas Piggin wrote:
> > >
> > > diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> > > index 6911b73c07..ce8b42eb15 100644
> > > --- a/gdbstub/gdbstub.c
> > > +++ b/gdbstub/gdbstub.c
> > > @@ -2051,8 +2051,17 @@ void gdb_read_byte(uint8_t ch)
> > > return;
> > > }
> > > if (runstate_is_running()) {
> > > -/* when the CPU is running, we cannot do anything except stop
> > > - it when receiving a char */
> > > +/*
> > > + * When the CPU is running, we cannot do anything except stop
> > > + * it when receiving a char. This is expected on a Ctrl-C in the
> > > + * gdb client. Because we are in all-stop mode, gdb sends a
> > > + * 0x03 byte which is not a usual packet, so we handle it
> > > specially
> > > + * here, but it does expect a stop reply.
> > > + */
> > > +if (ch != 0x03) {
> > > +warn_report("gdbstub: client sent packet while target
> > > running\n");
> > > +}
> > > +gdbserver_state.allow_stop_reply = true;
> > > vm_stop(RUN_STATE_PAUSED);
> > > } else
> > > #endif
> >
> > Makes sense to me, but shouldn't we send the stop-reply packet only for
> > Ctrl+C/0x03?
>
> Good question.
>
> I think if we get a character here that's not a 3, we're already in
> trouble, and we eat it so even worse. Since we only send a stop packet
> back when the vm stops, then if we don't send one now we might never
> send it. At least if we send one then the client might have some chance
> to get back to a sane state.
I just noticed now (as I was integrating the latest upstream patches
with our downstream qemu-system-hexagon) that this causes the
gdbstub-untimely-packet tcg test to fail.
My first thought was that, if 0x3 is the only valid case where we will
read a char when the cpu is running, perhaps not issuing the stop-reply
isn't that bad as GDB would ignore it anyways. E.g. from a `set debug
remote 1` output:
Sending packet: $qSupported:multiprocess+;swbreak+;hwbreak+;qRelocInsn+;
fork-events+;vfork-events+;exec-events+;vContSupported+;
QThreadEvents+;no-resumed+;
xmlRegisters=i386#6a...
Packet instead of Ack, ignoring it
So, perhaps, we could do:
diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index f123b40ce7..8af066301a 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -2055,8 +2055,9 @@ void gdb_read_byte(uint8_t ch)
*/
if (ch != 0x03) {
warn_report("gdbstub: client sent packet while target running\n");
+} else {
+gdbserver_state.allow_stop_reply = true;
}
-gdbserver_state.allow_stop_reply = true;
vm_stop(RUN_STATE_PAUSED);
} else
#endif
-- >8 --
Alternatively, since GDB ignores the packet anyways, should we just let
this be and refactor/remove the test?
[PATCH] gdbstub: use 0 ("any process") on packets with no PID
Previously, qemu-user would always report PID 1 to GDB. This was changed
at dc14a7a6e9 (gdbstub: Report the actual qemu-user pid, 2023-06-30),
but read_thread_id() still considers GDB packets with "no PID" as "PID
1", which is not the qemu-user PID. Fix that by parsing "no PID" as "0",
which the GDB Remote Protocol defines as "any process".
Note that this should have no effect for system emulation as, in this
case, gdb_create_default_process() will assign PID 1 for the first
process and that is what the gdbstub uses for GDB requests with no PID,
or PID 0.
This issue was found with hexagon-lldb, which sends a "Hq" packet with
only the thread-id, but no process-id, leading to the invalid usage of
"PID 1" by qemu-hexagon and a subsequent "E22" reply.
Signed-off-by: Matheus Tavares Bernardino
---
gdbstub/gdbstub.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index ce8b42eb15..e74ecc78cc 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -537,7 +537,7 @@ static GDBThreadIdKind read_thread_id(const char *buf,
const char **end_buf,
/* Skip '.' */
buf++;
} else {
-p = 1;
+p = 0;
}
ret = qemu_strtoul(buf, &buf, 16, &t);
--
2.37.2
Re: [PATCH v3] Hexagon: move GETPC() calls to top level helpers
Matheus Tavares Bernardino wrote: > > Subject: [PATCH v3] Hexagon: move GETPC() calls to top level helpers Apologies, I had some problems with my send-email and server configuration, thus ended up sending this v3 multiple times. Please ignore the others and consider only this one (i.e. https://lore.kernel.org/qemu-devel/2c74c3696946edba7cc5b2942cf296a5af532052.1689070412.git.quic_mathb...@quicinc.com/). Thanks, Matheus
[PATCH v3] Hexagon: move GETPC() calls to top level helpers
As docs/devel/loads-stores.rst states:
``GETPC()`` should be used with great care: calling
it in other functions that are *not* the top level
``HELPER(foo)`` will cause unexpected behavior. Instead, the
value of ``GETPC()`` should be read from the helper and passed
if needed to the functions that the helper calls.
Let's fix the GETPC() usage in Hexagon, making sure it's always called
from top level helpers and passed down to the places where it's
needed. There are a few snippets where that is not currently the case:
- probe_store(), which is only called from two helpers, so it's easy to
move GETPC() up.
- mem_load*() functions, which are also called directly from helpers,
but through the MEM_LOAD*() set of macros. Note that this are only
used when compiling with --disable-hexagon-idef-parser.
In this case, we also take this opportunity to simplify the code,
unifying the mem_load*() functions.
- HELPER(probe_hvx_stores), when called from another helper, ends up
using its own GETPC() expansion instead of the top level caller.
Signed-off-by: Matheus Tavares Bernardino
---
v2:
https://lore.kernel.org/qemu-devel/93a2ca786530cbc8a94f7c7a6451f4f1f47c8a9b.1688581908.git.quic_mathb...@quicinc.com/
Changed in v3: included fix for HELPER(prove_hvx_store), which was being
called by HELPER(probe_pkt_scalar_hvx_stores).
target/hexagon/macros.h| 19 +-
target/hexagon/op_helper.h | 11 ++
target/hexagon/op_helper.c | 74 ++
3 files changed, 38 insertions(+), 66 deletions(-)
diff --git a/target/hexagon/macros.h b/target/hexagon/macros.h
index 5451b061ee..dafa0df6ed 100644
--- a/target/hexagon/macros.h
+++ b/target/hexagon/macros.h
@@ -173,15 +173,6 @@
#define MEM_STORE8(VA, DATA, SLOT) \
MEM_STORE8_FUNC(DATA)(cpu_env, VA, DATA, SLOT)
#else
-#define MEM_LOAD1s(VA) ((int8_t)mem_load1(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD1u(VA) ((uint8_t)mem_load1(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD2s(VA) ((int16_t)mem_load2(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD2u(VA) ((uint16_t)mem_load2(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD4s(VA) ((int32_t)mem_load4(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD4u(VA) ((uint32_t)mem_load4(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD8s(VA) ((int64_t)mem_load8(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD8u(VA) ((uint64_t)mem_load8(env, pkt_has_store_s1, slot, VA))
-
#define MEM_STORE1(VA, DATA, SLOT) log_store32(env, VA, DATA, 1, SLOT)
#define MEM_STORE2(VA, DATA, SLOT) log_store32(env, VA, DATA, 2, SLOT)
#define MEM_STORE4(VA, DATA, SLOT) log_store32(env, VA, DATA, 4, SLOT)
@@ -530,8 +521,16 @@ static inline TCGv gen_read_ireg(TCGv result, TCGv val,
int shift)
#ifdef QEMU_GENERATE
#define fLOAD(NUM, SIZE, SIGN, EA, DST) MEM_LOAD##SIZE##SIGN(DST, EA)
#else
+#define MEM_LOAD1 cpu_ldub_data_ra
+#define MEM_LOAD2 cpu_lduw_data_ra
+#define MEM_LOAD4 cpu_ldl_data_ra
+#define MEM_LOAD8 cpu_ldq_data_ra
+
#define fLOAD(NUM, SIZE, SIGN, EA, DST) \
-DST = (size##SIZE##SIGN##_t)MEM_LOAD##SIZE##SIGN(EA)
+do { \
+check_noshuf(env, pkt_has_store_s1, slot, EA, SIZE, GETPC()); \
+DST = (size##SIZE##SIGN##_t)MEM_LOAD##SIZE(env, EA, GETPC()); \
+} while (0)
#endif
#define fMEMOP(NUM, SIZE, SIGN, EA, FNTYPE, VALUE)
diff --git a/target/hexagon/op_helper.h b/target/hexagon/op_helper.h
index 8f3764d15e..7744e819ef 100644
--- a/target/hexagon/op_helper.h
+++ b/target/hexagon/op_helper.h
@@ -19,15 +19,8 @@
#define HEXAGON_OP_HELPER_H
/* Misc functions */
-uint8_t mem_load1(CPUHexagonState *env, bool pkt_has_store_s1,
- uint32_t slot, target_ulong vaddr);
-uint16_t mem_load2(CPUHexagonState *env, bool pkt_has_store_s1,
- uint32_t slot, target_ulong vaddr);
-uint32_t mem_load4(CPUHexagonState *env, bool pkt_has_store_s1,
- uint32_t slot, target_ulong vaddr);
-uint64_t mem_load8(CPUHexagonState *env, bool pkt_has_store_s1,
- uint32_t slot, target_ulong vaddr);
-
+void check_noshuf(CPUHexagonState *env, bool pkt_has_store_s1,
+ uint32_t slot, target_ulong vaddr, int size, uintptr_t ra);
void log_store64(CPUHexagonState *env, target_ulong addr,
int64_t val, int width, int slot);
void log_store32(CPUHexagonState *env, target_ulong addr,
diff --git a/target/hexagon/op_helper.c b/target/hexagon/op_helper.c
index 12967ac21e..1150178591 100644
--- a/target/hexagon/op_helper.c
+++ b/target/hexagon/op_helper.c
@@ -95,9 +95,8 @@ void HELPER(debug_check_store_width)(CPUHexagonState *env,
int slot, int check)
}
}
-void HELPER(commit_store)(CPUHexagonState *env, int slot_num)
+static inline void commit_store(CPUHexagonState *env, int slot_num, uintptr_t
ra)
{
-uintptr_t ra = GETPC();
uint8_t width = env->mem_log_stores[slot_num].width;
target_ulong va = env->mem_log_stores[slot_nu
[PATCH v3] Hexagon: move GETPC() calls to top level helpers
As docs/devel/loads-stores.rst states:
``GETPC()`` should be used with great care: calling
it in other functions that are *not* the top level
``HELPER(foo)`` will cause unexpected behavior. Instead, the
value of ``GETPC()`` should be read from the helper and passed
if needed to the functions that the helper calls.
Let's fix the GETPC() usage in Hexagon, making sure it's always called
from top level helpers and passed down to the places where it's
needed. There are two snippets where that is not currently the case:
- probe_store(), which is only called from two helpers, so it's easy to
move GETPC() up.
- mem_load*() functions, which are also called directly from helpers,
but through the MEM_LOAD*() set of macros. Note that this are only
used when compiling with --disable-hexagon-idef-parser.
In this case, we also take this opportunity to simplify the code,
unifying the mem_load*() functions.
Signed-off-by: Matheus Tavares Bernardino
Reviewed-by: Taylor Simpson
---
v2:
https://lore.kernel.org/qemu-devel/93a2ca786530cbc8a94f7c7a6451f4f1f47c8a9b.1688581908.git.quic_mathb...@quicinc.com/
Changes in v3: refactored fLOAD macro with 'do {...} while(0)' as
suggested by Taylor and added his Reviewed-by.
target/hexagon/macros.h| 19 ++--
target/hexagon/op_helper.h | 11 ++-
target/hexagon/op_helper.c | 62 +++---
3 files changed, 29 insertions(+), 63 deletions(-)
diff --git a/target/hexagon/macros.h b/target/hexagon/macros.h
index 5451b061ee..dafa0df6ed 100644
--- a/target/hexagon/macros.h
+++ b/target/hexagon/macros.h
@@ -173,15 +173,6 @@
#define MEM_STORE8(VA, DATA, SLOT) \
MEM_STORE8_FUNC(DATA)(cpu_env, VA, DATA, SLOT)
#else
-#define MEM_LOAD1s(VA) ((int8_t)mem_load1(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD1u(VA) ((uint8_t)mem_load1(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD2s(VA) ((int16_t)mem_load2(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD2u(VA) ((uint16_t)mem_load2(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD4s(VA) ((int32_t)mem_load4(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD4u(VA) ((uint32_t)mem_load4(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD8s(VA) ((int64_t)mem_load8(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD8u(VA) ((uint64_t)mem_load8(env, pkt_has_store_s1, slot, VA))
-
#define MEM_STORE1(VA, DATA, SLOT) log_store32(env, VA, DATA, 1, SLOT)
#define MEM_STORE2(VA, DATA, SLOT) log_store32(env, VA, DATA, 2, SLOT)
#define MEM_STORE4(VA, DATA, SLOT) log_store32(env, VA, DATA, 4, SLOT)
@@ -530,8 +521,16 @@ static inline TCGv gen_read_ireg(TCGv result, TCGv val,
int shift)
#ifdef QEMU_GENERATE
#define fLOAD(NUM, SIZE, SIGN, EA, DST) MEM_LOAD##SIZE##SIGN(DST, EA)
#else
+#define MEM_LOAD1 cpu_ldub_data_ra
+#define MEM_LOAD2 cpu_lduw_data_ra
+#define MEM_LOAD4 cpu_ldl_data_ra
+#define MEM_LOAD8 cpu_ldq_data_ra
+
#define fLOAD(NUM, SIZE, SIGN, EA, DST) \
-DST = (size##SIZE##SIGN##_t)MEM_LOAD##SIZE##SIGN(EA)
+do { \
+check_noshuf(env, pkt_has_store_s1, slot, EA, SIZE, GETPC()); \
+DST = (size##SIZE##SIGN##_t)MEM_LOAD##SIZE(env, EA, GETPC()); \
+} while (0)
#endif
#define fMEMOP(NUM, SIZE, SIGN, EA, FNTYPE, VALUE)
diff --git a/target/hexagon/op_helper.h b/target/hexagon/op_helper.h
index 8f3764d15e..7744e819ef 100644
--- a/target/hexagon/op_helper.h
+++ b/target/hexagon/op_helper.h
@@ -19,15 +19,8 @@
#define HEXAGON_OP_HELPER_H
/* Misc functions */
-uint8_t mem_load1(CPUHexagonState *env, bool pkt_has_store_s1,
- uint32_t slot, target_ulong vaddr);
-uint16_t mem_load2(CPUHexagonState *env, bool pkt_has_store_s1,
- uint32_t slot, target_ulong vaddr);
-uint32_t mem_load4(CPUHexagonState *env, bool pkt_has_store_s1,
- uint32_t slot, target_ulong vaddr);
-uint64_t mem_load8(CPUHexagonState *env, bool pkt_has_store_s1,
- uint32_t slot, target_ulong vaddr);
-
+void check_noshuf(CPUHexagonState *env, bool pkt_has_store_s1,
+ uint32_t slot, target_ulong vaddr, int size, uintptr_t ra);
void log_store64(CPUHexagonState *env, target_ulong addr,
int64_t val, int width, int slot);
void log_store32(CPUHexagonState *env, target_ulong addr,
diff --git a/target/hexagon/op_helper.c b/target/hexagon/op_helper.c
index 12967ac21e..abc9fc4724 100644
--- a/target/hexagon/op_helper.c
+++ b/target/hexagon/op_helper.c
@@ -95,9 +95,8 @@ void HELPER(debug_check_store_width)(CPUHexagonState *env,
int slot, int check)
}
}
-void HELPER(commit_store)(CPUHexagonState *env, int slot_num)
+static void commit_store(CPUHexagonState *env, int slot_num, uintptr_t ra)
{
-uintptr_t ra = GETPC();
uint8_t width = env->mem_log_stores[slot_num].width;
target_ulong va = env->mem_log_stores[slot_num].va;
@@ -119,6 +118,12 @@ void HELPER(commit_store)(CPUHexagonState *env, int
slot_num)
}
}
+void
[PATCH v3] Hexagon: move GETPC() calls to top level helpers
As docs/devel/loads-stores.rst states:
``GETPC()`` should be used with great care: calling
it in other functions that are *not* the top level
``HELPER(foo)`` will cause unexpected behavior. Instead, the
value of ``GETPC()`` should be read from the helper and passed
if needed to the functions that the helper calls.
Let's fix the GETPC() usage in Hexagon, making sure it's always called
from top level helpers and passed down to the places where it's
needed. There are a few snippets where that is not currently the case:
- probe_store(), which is only called from two helpers, so it's easy to
move GETPC() up.
- mem_load*() functions, which are also called directly from helpers,
but through the MEM_LOAD*() set of macros. Note that this are only
used when compiling with --disable-hexagon-idef-parser.
In this case, we also take this opportunity to simplify the code,
unifying the mem_load*() functions.
- HELPER(probe_hvx_stores), when called from another helper, ends up
using its own GETPC() expansion instead of the top level caller.
Signed-off-by: Matheus Tavares Bernardino
---
v2:
https://lore.kernel.org/qemu-devel/93a2ca786530cbc8a94f7c7a6451f4f1f47c8a9b.1688581908.git.quic_mathb...@quicinc.com/
Changes in v3: also included fix for nested helper call in
probe_hvx_stores, which I had missed in previous iterations.
target/hexagon/macros.h| 19 +-
target/hexagon/op_helper.h | 11 ++
target/hexagon/op_helper.c | 74 ++
3 files changed, 38 insertions(+), 66 deletions(-)
diff --git a/target/hexagon/macros.h b/target/hexagon/macros.h
index 5451b061ee..dafa0df6ed 100644
--- a/target/hexagon/macros.h
+++ b/target/hexagon/macros.h
@@ -173,15 +173,6 @@
#define MEM_STORE8(VA, DATA, SLOT) \
MEM_STORE8_FUNC(DATA)(cpu_env, VA, DATA, SLOT)
#else
-#define MEM_LOAD1s(VA) ((int8_t)mem_load1(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD1u(VA) ((uint8_t)mem_load1(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD2s(VA) ((int16_t)mem_load2(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD2u(VA) ((uint16_t)mem_load2(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD4s(VA) ((int32_t)mem_load4(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD4u(VA) ((uint32_t)mem_load4(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD8s(VA) ((int64_t)mem_load8(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD8u(VA) ((uint64_t)mem_load8(env, pkt_has_store_s1, slot, VA))
-
#define MEM_STORE1(VA, DATA, SLOT) log_store32(env, VA, DATA, 1, SLOT)
#define MEM_STORE2(VA, DATA, SLOT) log_store32(env, VA, DATA, 2, SLOT)
#define MEM_STORE4(VA, DATA, SLOT) log_store32(env, VA, DATA, 4, SLOT)
@@ -530,8 +521,16 @@ static inline TCGv gen_read_ireg(TCGv result, TCGv val,
int shift)
#ifdef QEMU_GENERATE
#define fLOAD(NUM, SIZE, SIGN, EA, DST) MEM_LOAD##SIZE##SIGN(DST, EA)
#else
+#define MEM_LOAD1 cpu_ldub_data_ra
+#define MEM_LOAD2 cpu_lduw_data_ra
+#define MEM_LOAD4 cpu_ldl_data_ra
+#define MEM_LOAD8 cpu_ldq_data_ra
+
#define fLOAD(NUM, SIZE, SIGN, EA, DST) \
-DST = (size##SIZE##SIGN##_t)MEM_LOAD##SIZE##SIGN(EA)
+do { \
+check_noshuf(env, pkt_has_store_s1, slot, EA, SIZE, GETPC()); \
+DST = (size##SIZE##SIGN##_t)MEM_LOAD##SIZE(env, EA, GETPC()); \
+} while (0)
#endif
#define fMEMOP(NUM, SIZE, SIGN, EA, FNTYPE, VALUE)
diff --git a/target/hexagon/op_helper.h b/target/hexagon/op_helper.h
index 8f3764d15e..7744e819ef 100644
--- a/target/hexagon/op_helper.h
+++ b/target/hexagon/op_helper.h
@@ -19,15 +19,8 @@
#define HEXAGON_OP_HELPER_H
/* Misc functions */
-uint8_t mem_load1(CPUHexagonState *env, bool pkt_has_store_s1,
- uint32_t slot, target_ulong vaddr);
-uint16_t mem_load2(CPUHexagonState *env, bool pkt_has_store_s1,
- uint32_t slot, target_ulong vaddr);
-uint32_t mem_load4(CPUHexagonState *env, bool pkt_has_store_s1,
- uint32_t slot, target_ulong vaddr);
-uint64_t mem_load8(CPUHexagonState *env, bool pkt_has_store_s1,
- uint32_t slot, target_ulong vaddr);
-
+void check_noshuf(CPUHexagonState *env, bool pkt_has_store_s1,
+ uint32_t slot, target_ulong vaddr, int size, uintptr_t ra);
void log_store64(CPUHexagonState *env, target_ulong addr,
int64_t val, int width, int slot);
void log_store32(CPUHexagonState *env, target_ulong addr,
diff --git a/target/hexagon/op_helper.c b/target/hexagon/op_helper.c
index 12967ac21e..1150178591 100644
--- a/target/hexagon/op_helper.c
+++ b/target/hexagon/op_helper.c
@@ -95,9 +95,8 @@ void HELPER(debug_check_store_width)(CPUHexagonState *env,
int slot, int check)
}
}
-void HELPER(commit_store)(CPUHexagonState *env, int slot_num)
+static inline void commit_store(CPUHexagonState *env, int slot_num, uintptr_t
ra)
{
-uintptr_t ra = GETPC();
uint8_t width = env->mem_log_stores[slot_num].width;
target_ulong va = env->mem_log_stores[slot_nu
Re: [PATCH] gdbstub: Fix client Ctrl-C handling
> Nicholas Piggin wrote:
>
> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> index 6911b73c07..ce8b42eb15 100644
> --- a/gdbstub/gdbstub.c
> +++ b/gdbstub/gdbstub.c
> @@ -2051,8 +2051,17 @@ void gdb_read_byte(uint8_t ch)
> return;
> }
> if (runstate_is_running()) {
> -/* when the CPU is running, we cannot do anything except stop
> - it when receiving a char */
> +/*
> + * When the CPU is running, we cannot do anything except stop
> + * it when receiving a char. This is expected on a Ctrl-C in the
> + * gdb client. Because we are in all-stop mode, gdb sends a
> + * 0x03 byte which is not a usual packet, so we handle it specially
> + * here, but it does expect a stop reply.
> + */
> +if (ch != 0x03) {
> +warn_report("gdbstub: client sent packet while target
> running\n");
> +}
> +gdbserver_state.allow_stop_reply = true;
> vm_stop(RUN_STATE_PAUSED);
> } else
> #endif
Makes sense to me, but shouldn't we send the stop-reply packet only for
Ctrl+C/0x03?
[PATCH v3] Hexagon: move GETPC() calls to top level helpers
As docs/devel/loads-stores.rst states:
``GETPC()`` should be used with great care: calling
it in other functions that are *not* the top level
``HELPER(foo)`` will cause unexpected behavior. Instead, the
value of ``GETPC()`` should be read from the helper and passed
if needed to the functions that the helper calls.
Let's fix the GETPC() usage in Hexagon, making sure it's always called
from top level helpers and passed down to the places where it's
needed. There are a few snippets where that is not currently the case:
- probe_store(), which is only called from two helpers, so it's easy to
move GETPC() up.
- mem_load*() functions, which are also called directly from helpers,
but through the MEM_LOAD*() set of macros. Note that this are only
used when compiling with --disable-hexagon-idef-parser.
In this case, we also take this opportunity to simplify the code,
unifying the mem_load*() functions.
- HELPER(probe_hvx_stores), when called from another helper, ends up
using its own GETPC() expansion instead of the top level caller.
Signed-off-by: Matheus Tavares Bernardino
Reviewed-by: Taylor Simpson
---
v2:
https://lore.kernel.org/qemu-devel/93a2ca786530cbc8a94f7c7a6451f4f1f47c8a9b.1688581908.git.quic_mathb...@quicinc.com/
Changes since v2:
- Made check_noshuf static again (thanks, Taylor!)
- Included fix for nested helper call using wrong GETPC value at
HELPER(probe_pkt_scalar_store_s0) -> HELPER(probe_hvx_store)
- Included Taylor's Reviewed-by.
target/hexagon/macros.h| 19 +-
target/hexagon/op_helper.h | 9 -
target/hexagon/op_helper.c | 75 +++---
3 files changed, 38 insertions(+), 65 deletions(-)
diff --git a/target/hexagon/macros.h b/target/hexagon/macros.h
index 5451b061ee..dafa0df6ed 100644
--- a/target/hexagon/macros.h
+++ b/target/hexagon/macros.h
@@ -173,15 +173,6 @@
#define MEM_STORE8(VA, DATA, SLOT) \
MEM_STORE8_FUNC(DATA)(cpu_env, VA, DATA, SLOT)
#else
-#define MEM_LOAD1s(VA) ((int8_t)mem_load1(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD1u(VA) ((uint8_t)mem_load1(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD2s(VA) ((int16_t)mem_load2(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD2u(VA) ((uint16_t)mem_load2(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD4s(VA) ((int32_t)mem_load4(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD4u(VA) ((uint32_t)mem_load4(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD8s(VA) ((int64_t)mem_load8(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD8u(VA) ((uint64_t)mem_load8(env, pkt_has_store_s1, slot, VA))
-
#define MEM_STORE1(VA, DATA, SLOT) log_store32(env, VA, DATA, 1, SLOT)
#define MEM_STORE2(VA, DATA, SLOT) log_store32(env, VA, DATA, 2, SLOT)
#define MEM_STORE4(VA, DATA, SLOT) log_store32(env, VA, DATA, 4, SLOT)
@@ -530,8 +521,16 @@ static inline TCGv gen_read_ireg(TCGv result, TCGv val,
int shift)
#ifdef QEMU_GENERATE
#define fLOAD(NUM, SIZE, SIGN, EA, DST) MEM_LOAD##SIZE##SIGN(DST, EA)
#else
+#define MEM_LOAD1 cpu_ldub_data_ra
+#define MEM_LOAD2 cpu_lduw_data_ra
+#define MEM_LOAD4 cpu_ldl_data_ra
+#define MEM_LOAD8 cpu_ldq_data_ra
+
#define fLOAD(NUM, SIZE, SIGN, EA, DST) \
-DST = (size##SIZE##SIGN##_t)MEM_LOAD##SIZE##SIGN(EA)
+do { \
+check_noshuf(env, pkt_has_store_s1, slot, EA, SIZE, GETPC()); \
+DST = (size##SIZE##SIGN##_t)MEM_LOAD##SIZE(env, EA, GETPC()); \
+} while (0)
#endif
#define fMEMOP(NUM, SIZE, SIGN, EA, FNTYPE, VALUE)
diff --git a/target/hexagon/op_helper.h b/target/hexagon/op_helper.h
index 8f3764d15e..66119cf3d4 100644
--- a/target/hexagon/op_helper.h
+++ b/target/hexagon/op_helper.h
@@ -19,15 +19,6 @@
#define HEXAGON_OP_HELPER_H
/* Misc functions */
-uint8_t mem_load1(CPUHexagonState *env, bool pkt_has_store_s1,
- uint32_t slot, target_ulong vaddr);
-uint16_t mem_load2(CPUHexagonState *env, bool pkt_has_store_s1,
- uint32_t slot, target_ulong vaddr);
-uint32_t mem_load4(CPUHexagonState *env, bool pkt_has_store_s1,
- uint32_t slot, target_ulong vaddr);
-uint64_t mem_load8(CPUHexagonState *env, bool pkt_has_store_s1,
- uint32_t slot, target_ulong vaddr);
-
void log_store64(CPUHexagonState *env, target_ulong addr,
int64_t val, int width, int slot);
void log_store32(CPUHexagonState *env, target_ulong addr,
diff --git a/target/hexagon/op_helper.c b/target/hexagon/op_helper.c
index 12967ac21e..06bfe92c7b 100644
--- a/target/hexagon/op_helper.c
+++ b/target/hexagon/op_helper.c
@@ -95,9 +95,8 @@ void HELPER(debug_check_store_width)(CPUHexagonState *env,
int slot, int check)
}
}
-void HELPER(commit_store)(CPUHexagonState *env, int slot_num)
+static void commit_store(CPUHexagonState *env, int slot_num, uintptr_t ra)
{
-uintptr_t ra = GETPC();
uint8_t width = env->mem_log_stores[slot_num].width;
target_ulong va = env->mem_log_stores[slot_nu
Re: [PATCH v2] Hexagon: move GETPC() calls to top level helpers
> ltaylorsimp...@gmail.com wrote:
>
> > -Original Message-
> > From: Matheus Tavares Bernardino
> > Sent: Wednesday, July 5, 2023 12:35 PM
> > To: qemu-devel@nongnu.org
> > Cc: quic_mathb...@quicinc.com; bc...@quicinc.com;
> > ltaylorsimp...@gmail.com; quic_mlie...@quicinc.com;
> > richard.hender...@linaro.org
> > Subject: [PATCH v2] Hexagon: move GETPC() calls to top level helpers
> >
> > diff --git a/target/hexagon/macros.h b/target/hexagon/macros.h index
> > 5451b061ee..e44a932434 100644
> > --- a/target/hexagon/macros.h
> > +++ b/target/hexagon/macros.h
> > @@ -173,15 +173,6 @@
> > #define fLOAD(NUM, SIZE, SIGN, EA, DST) \
> > -DST = (size##SIZE##SIGN##_t)MEM_LOAD##SIZE##SIGN(EA)
> > +DST = (size##SIZE##SIGN##_t)({ \
> > +check_noshuf(env, pkt_has_store_s1, slot, EA, SIZE, GETPC()); \
> > +MEM_LOAD##SIZE(env, EA, GETPC()); \
> > +})
> > #endif
>
> This should be formatted as
> #define fLOAD(...) \
> do { \
> check_noshuf(...); \
> DST = ...; \
> } while (0)
Ah, indeed, thanks!
> > a/target/hexagon/op_helper.h b/target/hexagon/op_helper.h index
> > 8f3764d15e..7744e819ef 100644
> > --- a/target/hexagon/op_helper.h
> > +++ b/target/hexagon/op_helper.h
> > +void check_noshuf(CPUHexagonState *env, bool pkt_has_store_s1,
> > + uint32_t slot, target_ulong vaddr, int size,
> > +uintptr_t ra);
>
> Are you sure this needs to be non-static?
Yeah, since we remove the mem_load*() functions, check_noshuf() must
now be visible to the other compilation units that include macros.h,
as we will expand the fLOAD() macro to call it.
[PATCH v2] Hexagon: move GETPC() calls to top level helpers
As docs/devel/loads-stores.rst states:
``GETPC()`` should be used with great care: calling
it in other functions that are *not* the top level
``HELPER(foo)`` will cause unexpected behavior. Instead, the
value of ``GETPC()`` should be read from the helper and passed
if needed to the functions that the helper calls.
Let's fix the GETPC() usage in Hexagon, making sure it's always called
from top level helpers and passed down to the places where it's
needed. There are two snippets where that is not currently the case:
- probe_store(), which is only called from two helpers, so it's easy to
move GETPC() up.
- mem_load*() functions, which are also called directly from helpers,
but through the MEM_LOAD*() set of macros. Note that this are only
used when compiling with --disable-hexagon-idef-parser.
In this case, we also take this opportunity to simplify the code,
unifying the mem_load*() functions.
Signed-off-by: Matheus Tavares Bernardino
---
v1:
d40fabcf9d6e92e4cd8d6a144e9b2a9acf4580dc.1688420966.git.quic_mathb...@quicinc.com
Changes in v2:
- Fixed wrong cpu_ld* unification from previous version.
- Passed retaddr down to check_noshuf() and further, as Taylor
suggested.
- Reorganized macros for simplification.
target/hexagon/macros.h| 19 ++--
target/hexagon/op_helper.h | 11 ++-
target/hexagon/op_helper.c | 62 +++---
3 files changed, 29 insertions(+), 63 deletions(-)
diff --git a/target/hexagon/macros.h b/target/hexagon/macros.h
index 5451b061ee..e44a932434 100644
--- a/target/hexagon/macros.h
+++ b/target/hexagon/macros.h
@@ -173,15 +173,6 @@
#define MEM_STORE8(VA, DATA, SLOT) \
MEM_STORE8_FUNC(DATA)(cpu_env, VA, DATA, SLOT)
#else
-#define MEM_LOAD1s(VA) ((int8_t)mem_load1(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD1u(VA) ((uint8_t)mem_load1(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD2s(VA) ((int16_t)mem_load2(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD2u(VA) ((uint16_t)mem_load2(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD4s(VA) ((int32_t)mem_load4(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD4u(VA) ((uint32_t)mem_load4(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD8s(VA) ((int64_t)mem_load8(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD8u(VA) ((uint64_t)mem_load8(env, pkt_has_store_s1, slot, VA))
-
#define MEM_STORE1(VA, DATA, SLOT) log_store32(env, VA, DATA, 1, SLOT)
#define MEM_STORE2(VA, DATA, SLOT) log_store32(env, VA, DATA, 2, SLOT)
#define MEM_STORE4(VA, DATA, SLOT) log_store32(env, VA, DATA, 4, SLOT)
@@ -530,8 +521,16 @@ static inline TCGv gen_read_ireg(TCGv result, TCGv val,
int shift)
#ifdef QEMU_GENERATE
#define fLOAD(NUM, SIZE, SIGN, EA, DST) MEM_LOAD##SIZE##SIGN(DST, EA)
#else
+#define MEM_LOAD1 cpu_ldub_data_ra
+#define MEM_LOAD2 cpu_lduw_data_ra
+#define MEM_LOAD4 cpu_ldl_data_ra
+#define MEM_LOAD8 cpu_ldq_data_ra
+
#define fLOAD(NUM, SIZE, SIGN, EA, DST) \
-DST = (size##SIZE##SIGN##_t)MEM_LOAD##SIZE##SIGN(EA)
+DST = (size##SIZE##SIGN##_t)({ \
+check_noshuf(env, pkt_has_store_s1, slot, EA, SIZE, GETPC()); \
+MEM_LOAD##SIZE(env, EA, GETPC()); \
+})
#endif
#define fMEMOP(NUM, SIZE, SIGN, EA, FNTYPE, VALUE)
diff --git a/target/hexagon/op_helper.h b/target/hexagon/op_helper.h
index 8f3764d15e..7744e819ef 100644
--- a/target/hexagon/op_helper.h
+++ b/target/hexagon/op_helper.h
@@ -19,15 +19,8 @@
#define HEXAGON_OP_HELPER_H
/* Misc functions */
-uint8_t mem_load1(CPUHexagonState *env, bool pkt_has_store_s1,
- uint32_t slot, target_ulong vaddr);
-uint16_t mem_load2(CPUHexagonState *env, bool pkt_has_store_s1,
- uint32_t slot, target_ulong vaddr);
-uint32_t mem_load4(CPUHexagonState *env, bool pkt_has_store_s1,
- uint32_t slot, target_ulong vaddr);
-uint64_t mem_load8(CPUHexagonState *env, bool pkt_has_store_s1,
- uint32_t slot, target_ulong vaddr);
-
+void check_noshuf(CPUHexagonState *env, bool pkt_has_store_s1,
+ uint32_t slot, target_ulong vaddr, int size, uintptr_t ra);
void log_store64(CPUHexagonState *env, target_ulong addr,
int64_t val, int width, int slot);
void log_store32(CPUHexagonState *env, target_ulong addr,
diff --git a/target/hexagon/op_helper.c b/target/hexagon/op_helper.c
index 12967ac21e..abc9fc4724 100644
--- a/target/hexagon/op_helper.c
+++ b/target/hexagon/op_helper.c
@@ -95,9 +95,8 @@ void HELPER(debug_check_store_width)(CPUHexagonState *env,
int slot, int check)
}
}
-void HELPER(commit_store)(CPUHexagonState *env, int slot_num)
+static void commit_store(CPUHexagonState *env, int slot_num, uintptr_t ra)
{
-uintptr_t ra = GETPC();
uint8_t width = env->mem_log_stores[slot_num].width;
target_ulong va = env->mem_log_stores[slot_num].va;
@@ -119,6 +118,12 @@ void HELPER(commit_store)(CPUHexagonState *env, int
slot_num)
}
}
+void HELPER(com
Re: [PATCH] Hexagon: move GETPC() calls to top level helpers
> Taylor wrote:
>
> > Matheus Tavares Bernardino wrote:
> >
> > diff --git a/target/hexagon/macros.h b/target/hexagon/macros.h index
> > 5451b061ee..efb8013912 100644
> > --- a/target/hexagon/macros.h
> > +++ b/target/hexagon/macros.h
> > +
> > +#define MEM_LOADn(SIZE, VA) ({ \
> > +check_noshuf(env, pkt_has_store_s1, slot, VA, SIZE); \
> > +cpu_ldub_data_ra(env, VA, GETPC()); \
> > +})
>
> Note that check_noshuf calls HELPER(probe_noshuf_load) and
> HELPER(commit_store). Both of those call GETPC() from within. So, you'll
> need to pull the contents into separate functions that take ra as an
> argument.
Ah, good point. It was my understanding that, in case of a memory
exception in one of those nested helper calls, the GETPC() we would want
to use for unwinding was the one from the most recent helper. I'm still
trying to wrap my head around these concepts, though, so I might have
misunderstood it. Is this not the case?
> Does this pass the test suite? You are only using the SIZE parameter in
> check_noshuf, but cpu_ldub_data_ra only reads a single byte.
My oversight, this has to be fixed, thanks.
Re: [PATCH] Hexagon: move GETPC() calls to top level helpers
> Matheus Tavares Bernardino wrote:
>
> diff --git a/target/hexagon/macros.h b/target/hexagon/macros.h
> index 5451b061ee..efb8013912 100644
> --- a/target/hexagon/macros.h
> +++ b/target/hexagon/macros.h
> @@ -173,14 +173,20 @@
> #define MEM_STORE8(VA, DATA, SLOT) \
> MEM_STORE8_FUNC(DATA)(cpu_env, VA, DATA, SLOT)
> #else
> -#define MEM_LOAD1s(VA) ((int8_t)mem_load1(env, pkt_has_store_s1, slot, VA))
> -#define MEM_LOAD1u(VA) ((uint8_t)mem_load1(env, pkt_has_store_s1, slot, VA))
> -#define MEM_LOAD2s(VA) ((int16_t)mem_load2(env, pkt_has_store_s1, slot, VA))
> -#define MEM_LOAD2u(VA) ((uint16_t)mem_load2(env, pkt_has_store_s1, slot, VA))
> -#define MEM_LOAD4s(VA) ((int32_t)mem_load4(env, pkt_has_store_s1, slot, VA))
> -#define MEM_LOAD4u(VA) ((uint32_t)mem_load4(env, pkt_has_store_s1, slot, VA))
> -#define MEM_LOAD8s(VA) ((int64_t)mem_load8(env, pkt_has_store_s1, slot, VA))
> -#define MEM_LOAD8u(VA) ((uint64_t)mem_load8(env, pkt_has_store_s1, slot, VA))
> +
> +#define MEM_LOADn(SIZE, VA) ({ \
> +check_noshuf(env, pkt_has_store_s1, slot, VA, SIZE); \
> +cpu_ldub_data_ra(env, VA, GETPC()); \
> +})
> +
> +#define MEM_LOAD1s(VA) ((int8_t)MEM_LOADn(1, VA))
> +#define MEM_LOAD1u(VA) ((uint8_t)MEM_LOADn(1, VA))
> +#define MEM_LOAD2s(VA) ((int16_t)MEM_LOADn(2, VA))
> +#define MEM_LOAD2u(VA) ((uint16_t)MEM_LOADn(2, VA))
> +#define MEM_LOAD4s(VA) ((int32_t)MEM_LOADn(4, VA))
> +#define MEM_LOAD4u(VA) ((uint32_t)MEM_LOADn(4, VA))
> +#define MEM_LOAD8s(VA) ((int64_t)MEM_LOADn(8, VA))
> +#define MEM_LOAD8u(VA) ((uint64_t)MEM_LOADn(8, VA))
Oops, an oversight from my side: this simplification is not correct
since the mem_load*() functions all call different variants of
cpu_ld*_data_ra(). I'll send a v2 correcting that.
[PATCH] Hexagon: move GETPC() calls to top level helpers
As docs/devel/loads-stores.rst states:
``GETPC()`` should be used with great care: calling
it in other functions that are *not* the top level
``HELPER(foo)`` will cause unexpected behavior. Instead, the
value of ``GETPC()`` should be read from the helper and passed
if needed to the functions that the helper calls.
Let's fix the GETPC() usage in Hexagon, making sure it's always called
from top level helpers and passed down to the places where it's
needed. There are two snippets where that is not currently the case:
- probe_store(), which is only called from two helpers, so it's easy to
move GETPC() up.
- mem_load*() functions, which are also called directly from helpers,
but through the MEM_LOAD*() set of macros. Note that this are only
used when compiling with --disable-hexagon-idef-parser.
In this case, we also take this opportunity to simplify the code,
unifying the mem_load*() functions.
Signed-off-by: Matheus Tavares Bernardino
---
target/hexagon/macros.h| 22 ++---
target/hexagon/op_helper.h | 11 ++---
target/hexagon/op_helper.c | 49 +++---
3 files changed, 25 insertions(+), 57 deletions(-)
diff --git a/target/hexagon/macros.h b/target/hexagon/macros.h
index 5451b061ee..efb8013912 100644
--- a/target/hexagon/macros.h
+++ b/target/hexagon/macros.h
@@ -173,14 +173,20 @@
#define MEM_STORE8(VA, DATA, SLOT) \
MEM_STORE8_FUNC(DATA)(cpu_env, VA, DATA, SLOT)
#else
-#define MEM_LOAD1s(VA) ((int8_t)mem_load1(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD1u(VA) ((uint8_t)mem_load1(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD2s(VA) ((int16_t)mem_load2(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD2u(VA) ((uint16_t)mem_load2(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD4s(VA) ((int32_t)mem_load4(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD4u(VA) ((uint32_t)mem_load4(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD8s(VA) ((int64_t)mem_load8(env, pkt_has_store_s1, slot, VA))
-#define MEM_LOAD8u(VA) ((uint64_t)mem_load8(env, pkt_has_store_s1, slot, VA))
+
+#define MEM_LOADn(SIZE, VA) ({ \
+check_noshuf(env, pkt_has_store_s1, slot, VA, SIZE); \
+cpu_ldub_data_ra(env, VA, GETPC()); \
+})
+
+#define MEM_LOAD1s(VA) ((int8_t)MEM_LOADn(1, VA))
+#define MEM_LOAD1u(VA) ((uint8_t)MEM_LOADn(1, VA))
+#define MEM_LOAD2s(VA) ((int16_t)MEM_LOADn(2, VA))
+#define MEM_LOAD2u(VA) ((uint16_t)MEM_LOADn(2, VA))
+#define MEM_LOAD4s(VA) ((int32_t)MEM_LOADn(4, VA))
+#define MEM_LOAD4u(VA) ((uint32_t)MEM_LOADn(4, VA))
+#define MEM_LOAD8s(VA) ((int64_t)MEM_LOADn(8, VA))
+#define MEM_LOAD8u(VA) ((uint64_t)MEM_LOADn(8, VA))
#define MEM_STORE1(VA, DATA, SLOT) log_store32(env, VA, DATA, 1, SLOT)
#define MEM_STORE2(VA, DATA, SLOT) log_store32(env, VA, DATA, 2, SLOT)
diff --git a/target/hexagon/op_helper.h b/target/hexagon/op_helper.h
index 8f3764d15e..845c3d197e 100644
--- a/target/hexagon/op_helper.h
+++ b/target/hexagon/op_helper.h
@@ -19,15 +19,8 @@
#define HEXAGON_OP_HELPER_H
/* Misc functions */
-uint8_t mem_load1(CPUHexagonState *env, bool pkt_has_store_s1,
- uint32_t slot, target_ulong vaddr);
-uint16_t mem_load2(CPUHexagonState *env, bool pkt_has_store_s1,
- uint32_t slot, target_ulong vaddr);
-uint32_t mem_load4(CPUHexagonState *env, bool pkt_has_store_s1,
- uint32_t slot, target_ulong vaddr);
-uint64_t mem_load8(CPUHexagonState *env, bool pkt_has_store_s1,
- uint32_t slot, target_ulong vaddr);
-
+void check_noshuf(CPUHexagonState *env, bool pkt_has_store_s1,
+ uint32_t slot, target_ulong vaddr, int size);
void log_store64(CPUHexagonState *env, target_ulong addr,
int64_t val, int width, int slot);
void log_store32(CPUHexagonState *env, target_ulong addr,
diff --git a/target/hexagon/op_helper.c b/target/hexagon/op_helper.c
index 12967ac21e..1bc9c7fc2e 100644
--- a/target/hexagon/op_helper.c
+++ b/target/hexagon/op_helper.c
@@ -467,13 +467,12 @@ int32_t HELPER(cabacdecbin_pred)(int64_t RssV, int64_t
RttV)
}
static void probe_store(CPUHexagonState *env, int slot, int mmu_idx,
-bool is_predicated)
+bool is_predicated, uintptr_t retaddr)
{
if (!is_predicated || !(env->slot_cancelled & (1 << slot))) {
size1u_t width = env->mem_log_stores[slot].width;
target_ulong va = env->mem_log_stores[slot].va;
-uintptr_t ra = GETPC();
-probe_write(env, va, width, mmu_idx, ra);
+probe_write(env, va, width, mmu_idx, retaddr);
}
}
@@ -494,7 +493,8 @@ void HELPER(probe_pkt_scalar_store_s0)(CPUHexagonState
*env, int args)
int mmu_idx = FIELD_EX32(args, PROBE_PKT_SCALAR_STORE_S0, MMU_IDX);
bool is_predicated =
FIELD_EX32(args, PROBE_PKT_SCALAR_STORE_S0, IS_PREDICATED);
-probe_store(env, 0, mmu_idx, is_predicated);
+uintptr_t ra = GE
Re: [PATCH] gdbstub: Permit reverse step/break to provide stop response
Peter Maydell wrote: > > On Fri, 23 Jun 2023 at 13:19, Matheus Tavares Bernardino > wrote: > > > > Ah, it's interesting that [1] doesn't include 'bc' and 'bs' in the list > > of cmds that may respond with a stop-reply packet: > > > > "The 'C', 'c', 'S', 's', 'vCont', 'vAttach', 'vRun', 'vStopped', and > > '?' packets can receive any of the below as a reply." > > > > But their definitions at [2] do say the following: > > > > 'bc' (and 'bc') > > [...] > > Reply: See Stop Reply Packets, for the reply specifications. > > > > So I guess the list from [1] is not exhaustive. Anyway, thanks for the > > fix! > > That looks like it's probably a gdb docs bug (forgetting to > update that list when the bc/bs packets were added); we > should probably report that to upstream gdb. Good idea, done: https://sourceware.org/pipermail/gdb/2023-June/050804.html
Re: [PATCH] gdbstub: Permit reverse step/break to provide stop response
> Nicholas Piggin wrote: > > The final part of the reverse step and break handling is to bring > the machine back to a debug stop state. gdb expects a response. > > A gdb 'rsi' command hangs forever because the gdbstub filters out > the response (also observable with reverse_debugging.py avocado > tests). > > Fix by setting allow_stop_reply for the gdb backward packets. Ah, it's interesting that [1] doesn't include 'bc' and 'bs' in the list of cmds that may respond with a stop-reply packet: "The 'C', 'c', 'S', 's', 'vCont', 'vAttach', 'vRun', 'vStopped', and '?' packets can receive any of the below as a reply." But their definitions at [2] do say the following: 'bc' (and 'bc') [...] Reply: See Stop Reply Packets, for the reply specifications. So I guess the list from [1] is not exhaustive. Anyway, thanks for the fix! Acked-by: Matheus Tavares Bernardino [1]: https://sourceware.org/gdb/onlinedocs/gdb/Stop-Reply-Packets.html#Stop-Reply-Packets [2]: https://sourceware.org/gdb/onlinedocs/gdb/Packets.html#Packets
[PATCH 2/2] Hexagon (tests/.../hex_test.h): use portable printf formats
This fixes compiler messages like "warning: format specifies type
'unsigned int' but the argument has type 'uint32_t' (aka 'unsigned
long') [-Wformat]".
Signed-off-by: Matheus Tavares Bernardino
---
tests/tcg/hexagon/hex_test.h | 14 ++
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/tests/tcg/hexagon/hex_test.h b/tests/tcg/hexagon/hex_test.h
index cfed06a58b..fe253b56e5 100644
--- a/tests/tcg/hexagon/hex_test.h
+++ b/tests/tcg/hexagon/hex_test.h
@@ -19,10 +19,13 @@
#ifndef HEX_TEST_H
#define HEX_TEST_H
+#include
+
static inline void __check32(int line, uint32_t val, uint32_t expect)
{
if (val != expect) {
-printf("ERROR at line %d: 0x%08x != 0x%08x\n", line, val, expect);
+printf("ERROR at line %d: 0x%08"PRIx32" != 0x%08"PRIx32"\n",
+ line, val, expect);
err++;
}
}
@@ -32,7 +35,8 @@ static inline void __check32(int line, uint32_t val, uint32_t
expect)
static inline void __check64(int line, uint64_t val, uint64_t expect)
{
if (val != expect) {
-printf("ERROR at line %d: 0x%016llx != 0x%016llx\n", line, val,
expect);
+printf("ERROR at line %d: 0x%016"PRIx64" != 0x%016"PRIx64"\n",
+ line, val, expect);
err++;
}
}
@@ -62,7 +66,8 @@ static inline void __checkp(int line, void *p, void *expect)
static inline void __check32_ne(int line, uint32_t val, uint32_t expect)
{
if (val == expect) {
-printf("ERROR at line %d: 0x%08x == 0x%08x\n", line, val, expect);
+printf("ERROR at line %d: 0x%08"PRIx32" == 0x%08"PRIx32"\n",
+ line, val, expect);
err++;
}
}
@@ -72,7 +77,8 @@ static inline void __check32_ne(int line, uint32_t val,
uint32_t expect)
static inline void __check64_ne(int line, uint64_t val, uint64_t expect)
{
if (val == expect) {
-printf("ERROR at line %d: 0x%016llx == 0x%016llx\n", line, val,
expect);
+printf("ERROR at line %d: 0x%016"PRIx64" == 0x%016"PRIx64"\n",
+ line, val, expect);
err++;
}
}
--
2.37.2
[PATCH 0/2] Hexagon tests: fix test_load_tmp2 and non-portable format
This series includes two fixes on hexagon test files: one for a non-porable printf specifier, and the other for the use of an uninitialized register. Marco Liebel (1): Hexagon (hvx_misc test): fix uninitialized regs at test_load_tmp2 Matheus Tavares Bernardino (1): Hexagon (tests/.../hex_test.h): use portable printf formats tests/tcg/hexagon/hex_test.h | 14 ++ tests/tcg/hexagon/hvx_misc.c | 7 ++- 2 files changed, 16 insertions(+), 5 deletions(-) -- 2.37.2
[PATCH 1/2] Hexagon (hvx_misc test): fix uninitialized regs at test_load_tmp2
From: Marco Liebel
This test case was using some vector registers which were not properly
initialized.
Signed-off-by: Marco Liebel
Signed-off-by: Matheus Tavares Bernardino
---
tests/tcg/hexagon/hvx_misc.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/tests/tcg/hexagon/hvx_misc.c b/tests/tcg/hexagon/hvx_misc.c
index b45170acd1..b56b8f9cc5 100644
--- a/tests/tcg/hexagon/hvx_misc.c
+++ b/tests/tcg/hexagon/hvx_misc.c
@@ -66,6 +66,11 @@ static void test_load_tmp2(void)
void *pout1 = &output[1];
asm volatile(
+"r0 = #0x0\n\t"
+"v14 = vsplat(r0)\n\t"
+"v15 = vsplat(r0)\n\t"
+"v24 = vsplat(r0)\n\t"
+"v25 = vsplat(r0)\n\t"
"r0 = #0x03030303\n\t"
"v16 = vsplat(r0)\n\t"
"r0 = #0x04040404\n\t"
@@ -79,7 +84,7 @@ static void test_load_tmp2(void)
"vmem(%0 + #0) = v24\n\t"
"vmem(%1 + #0) = v25\n\t"
: : "r"(pout0), "r"(pout1)
-: "r0", "v16", "v18", "v21", "v24", "v25", "memory"
+: "r0", "v14", "v15", "v16", "v18", "v21", "v24", "v25", "memory"
);
for (int i = 0; i < MAX_VEC_SIZE_BYTES / 4; ++i) {
--
2.37.2
[PATCH v2 1/2] target/hexagon/*.py: clean up used 'toss' and 'numregs' vars
Many Hexagon python scripts call hex_common.get_tagregs(), but only one call site use the full reg structure given by this function. To make the code cleaner, let's make get_tagregs() filter out the unused fields (i.e. 'toss' and 'numregs'), properly removed the unused variables at the call sites. The hex_common.bad_register() function is also adjusted to work exclusively with 'regtype' and 'regid' args. For the single call site that does use toss/numregs, we provide an optional parameter to get_tagregs() which will restore the old full behavior. Suggested-by: Taylor Simpson Signed-off-by: Matheus Tavares Bernardino --- target/hexagon/gen_analyze_funcs.py | 10 +++--- target/hexagon/gen_helper_funcs.py | 30 target/hexagon/gen_helper_protos.py | 22 ++-- target/hexagon/gen_idef_parser_funcs.py | 4 +-- target/hexagon/gen_op_regs.py | 4 +-- target/hexagon/gen_tcg_funcs.py | 46 - target/hexagon/hex_common.py| 24 ++--- 7 files changed, 70 insertions(+), 70 deletions(-) diff --git a/target/hexagon/gen_analyze_funcs.py b/target/hexagon/gen_analyze_funcs.py index 00868cc6cb..c3b521abef 100755 --- a/target/hexagon/gen_analyze_funcs.py +++ b/target/hexagon/gen_analyze_funcs.py @@ -165,7 +165,7 @@ def analyze_opn_new(f, tag, regtype, regid, regno): hex_common.bad_register(regtype, regid) -def analyze_opn(f, tag, regtype, regid, toss, numregs, i): +def analyze_opn(f, tag, regtype, regid, i): if hex_common.is_pair(regid): analyze_opn_old(f, tag, regtype, regid, i) elif hex_common.is_single(regid): @@ -174,9 +174,9 @@ def analyze_opn(f, tag, regtype, regid, toss, numregs, i): elif hex_common.is_new_val(regtype, regid, tag): analyze_opn_new(f, tag, regtype, regid, i) else: -hex_common.bad_register(regtype, regid, toss, numregs) +hex_common.bad_register(regtype, regid) else: -hex_common.bad_register(regtype, regid, toss, numregs) +hex_common.bad_register(regtype, regid) ## @@ -202,8 +202,8 @@ def gen_analyze_func(f, tag, regs, imms): i = 0 ## Analyze all the registers -for regtype, regid, toss, numregs in regs: -analyze_opn(f, tag, regtype, regid, toss, numregs, i) +for regtype, regid in regs: +analyze_opn(f, tag, regtype, regid, i) i += 1 has_generated_helper = not hex_common.skip_qemu_helper( diff --git a/target/hexagon/gen_helper_funcs.py b/target/hexagon/gen_helper_funcs.py index e80550f94e..ce21d3b688 100755 --- a/target/hexagon/gen_helper_funcs.py +++ b/target/hexagon/gen_helper_funcs.py @@ -87,9 +87,9 @@ def gen_helper_arg_opn(f, regtype, regid, i, tag): elif hex_common.is_new_val(regtype, regid, tag): gen_helper_arg_new(f, regtype, regid, i) else: -hex_common.bad_register(regtype, regid, toss, numregs) +hex_common.bad_register(regtype, regid) else: -hex_common.bad_register(regtype, regid, toss, numregs) +hex_common.bad_register(regtype, regid) def gen_helper_arg_imm(f, immlett): @@ -135,7 +135,7 @@ def gen_helper_dest_decl_opn(f, regtype, regid, i): else: gen_helper_dest_decl(f, regtype, regid, i) else: -hex_common.bad_register(regtype, regid, toss, numregs) +hex_common.bad_register(regtype, regid) def gen_helper_src_var_ext(f, regtype, regid): @@ -185,7 +185,7 @@ def gen_helper_return_opn(f, regtype, regid, i): else: gen_helper_return(f, regtype, regid, i) else: -hex_common.bad_register(regtype, regid, toss, numregs) +hex_common.bad_register(regtype, regid) ## @@ -208,7 +208,7 @@ def gen_helper_function(f, tag, tagregs, tagimms): numresults = 0 numscalarresults = 0 numscalarreadwrite = 0 -for regtype, regid, toss, numregs in regs: +for regtype, regid in regs: if hex_common.is_written(regid): numresults += 1 if hex_common.is_scalar_reg(regtype): @@ -226,7 +226,7 @@ def gen_helper_function(f, tag, tagregs, tagimms): ## The return type of the function is the type of the destination ## register (if scalar) i = 0 -for regtype, regid, toss, numregs in regs: +for regtype, regid in regs: if hex_common.is_written(regid): if hex_common.is_pair(regid): if hex_common.is_hvx_reg(regtype): @@ -239,7 +239,7 @@ def gen_helper_function(f, tag, tagregs, tagimms): else: gen_helper_return_type(f, regtype, regid, i) else: -hex_common.bad_register(regtype, regid, toss, numregs) +hex_common.bad_register(regtype, regid) i += 1 if numscalarresults == 0: @@ -248,7 +248
[PATCH v2 0/2] Hexagon: minor cleanups to comments and python scripts
These are two minor follow ups to the last Hexagon pull, updating some stale code comments and removing undefined variables at python scripts. Changes in v2: - Patch 1: adjusted get_regtags() to filter out the unused regs, also removing the unused variables at the call sites. - Patch 2: replaced outdated comments with pseudocode v1: https://lore.kernel.org/qemu-devel/cover.1684873957.git.quic_mathb...@quicinc.com/ Matheus Tavares Bernardino (2): target/hexagon/*.py: clean up used 'toss' and 'numregs' vars Hexagon: fix outdated `hex_new_*` comments target/hexagon/genptr.c | 26 +++--- target/hexagon/translate.c | 2 +- target/hexagon/gen_analyze_funcs.py | 10 +++--- target/hexagon/gen_helper_funcs.py | 30 target/hexagon/gen_helper_protos.py | 22 ++-- target/hexagon/gen_idef_parser_funcs.py | 4 +-- target/hexagon/gen_op_regs.py | 4 +-- target/hexagon/gen_tcg_funcs.py | 46 - target/hexagon/hex_common.py| 24 ++--- 9 files changed, 83 insertions(+), 85 deletions(-) -- 2.37.2
[PATCH v2 2/2] Hexagon: fix outdated `hex_new_*` comments
Some code comments refer to hex_new_value and hex_new_pred_value, which
have been transferred to DisasContext and, in the case of hex_new_value,
should now be accessed through get_result_gpr().
In order to fix this outdated comments and also avoid having to tweak
them whenever we make a variable name change in the future, let's
replace them with pseudocode.
Suggested-by: Taylor Simpson
Signed-off-by: Matheus Tavares Bernardino
---
target/hexagon/genptr.c| 26 --
target/hexagon/translate.c | 2 +-
2 files changed, 13 insertions(+), 15 deletions(-)
diff --git a/target/hexagon/genptr.c b/target/hexagon/genptr.c
index cb2aa28a19..bcb287dd8b 100644
--- a/target/hexagon/genptr.c
+++ b/target/hexagon/genptr.c
@@ -878,9 +878,9 @@ static void gen_endloop0(DisasContext *ctx)
*/
if (!ctx->is_tight_loop) {
/*
- *if (hex_gpr[HEX_REG_LC0] > 1) {
- *PC = hex_gpr[HEX_REG_SA0];
- *hex_new_value[HEX_REG_LC0] = hex_gpr[HEX_REG_LC0] - 1;
+ *if (LC0 > 1) {
+ *PC = SA0;
+ *LC0--;
*}
*/
TCGLabel *label3 = gen_new_label();
@@ -897,9 +897,9 @@ static void gen_endloop0(DisasContext *ctx)
static void gen_endloop1(DisasContext *ctx)
{
/*
- *if (hex_gpr[HEX_REG_LC1] > 1) {
- *PC = hex_gpr[HEX_REG_SA1];
- *hex_new_value[HEX_REG_LC1] = hex_gpr[HEX_REG_LC1] - 1;
+ *if (LC1 > 1) {
+ *PC = SA1;
+ *LC1--;
*}
*/
TCGLabel *label = gen_new_label();
@@ -946,14 +946,12 @@ static void gen_endloop01(DisasContext *ctx)
gen_set_label(label2);
/*
- *if (hex_gpr[HEX_REG_LC0] > 1) {
- *PC = hex_gpr[HEX_REG_SA0];
- *hex_new_value[HEX_REG_LC0] = hex_gpr[HEX_REG_LC0] - 1;
- *} else {
- *if (hex_gpr[HEX_REG_LC1] > 1) {
- *hex_next_pc = hex_gpr[HEX_REG_SA1];
- *hex_new_value[HEX_REG_LC1] = hex_gpr[HEX_REG_LC1] - 1;
- *}
+ *if (LC0 > 1) {
+ *PC = SA0;
+ *LC0--;
+ *} else if (LC1 > 1) {
+ *PC = SA1;
+ *LC1--;
*}
*/
tcg_gen_brcondi_tl(TCG_COND_LEU, hex_gpr[HEX_REG_LC0], 1, label3);
diff --git a/target/hexagon/translate.c b/target/hexagon/translate.c
index b18f1a9051..8838ab2364 100644
--- a/target/hexagon/translate.c
+++ b/target/hexagon/translate.c
@@ -556,7 +556,7 @@ static void gen_start_packet(DisasContext *ctx)
}
/*
- * Preload the predicated pred registers into hex_new_pred_value[pred_num]
+ * Preload the predicated pred registers into ctx->new_pred_value[pred_num]
* Only endloop instructions conditionally write to pred registers
*/
if (ctx->need_commit && pkt->pkt_has_endloop) {
--
2.37.2
[PATCH 1/2] target/hexagon/*.py: remove undef vars from bad_register()
Some calls to `hex_common.bad_register()` in Hexagon python files are passing undefined variables. Let's remove those. Signed-off-by: Matheus Tavares Bernardino --- target/hexagon/gen_helper_funcs.py | 8 target/hexagon/gen_tcg_funcs.py| 6 +++--- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/target/hexagon/gen_helper_funcs.py b/target/hexagon/gen_helper_funcs.py index e80550f94e..367d08aceb 100755 --- a/target/hexagon/gen_helper_funcs.py +++ b/target/hexagon/gen_helper_funcs.py @@ -87,9 +87,9 @@ def gen_helper_arg_opn(f, regtype, regid, i, tag): elif hex_common.is_new_val(regtype, regid, tag): gen_helper_arg_new(f, regtype, regid, i) else: -hex_common.bad_register(regtype, regid, toss, numregs) +hex_common.bad_register(regtype, regid) else: -hex_common.bad_register(regtype, regid, toss, numregs) +hex_common.bad_register(regtype, regid) def gen_helper_arg_imm(f, immlett): @@ -135,7 +135,7 @@ def gen_helper_dest_decl_opn(f, regtype, regid, i): else: gen_helper_dest_decl(f, regtype, regid, i) else: -hex_common.bad_register(regtype, regid, toss, numregs) +hex_common.bad_register(regtype, regid) def gen_helper_src_var_ext(f, regtype, regid): @@ -185,7 +185,7 @@ def gen_helper_return_opn(f, regtype, regid, i): else: gen_helper_return(f, regtype, regid, i) else: -hex_common.bad_register(regtype, regid, toss, numregs) +hex_common.bad_register(regtype, regid) ## diff --git a/target/hexagon/gen_tcg_funcs.py b/target/hexagon/gen_tcg_funcs.py index c73467b840..c87ea856f7 100755 --- a/target/hexagon/gen_tcg_funcs.py +++ b/target/hexagon/gen_tcg_funcs.py @@ -354,9 +354,9 @@ def genptr_src_read_opn(f, regtype, regid, tag): elif hex_common.is_new_val(regtype, regid, tag): genptr_src_read_new(f, regtype, regid) else: -hex_common.bad_register(regtype, regid, toss, numregs) +hex_common.bad_register(regtype, regid) else: -hex_common.bad_register(regtype, regid, toss, numregs) +hex_common.bad_register(regtype, regid) def gen_helper_call_opn(f, tag, regtype, regid, toss, numregs, i): @@ -468,7 +468,7 @@ def genptr_dst_write_opn(f, regtype, regid, tag): else: genptr_dst_write(f, tag, regtype, regid) else: -hex_common.bad_register(regtype, regid, toss, numregs) +hex_common.bad_register(regtype, regid) ## -- 2.37.2
[PATCH 2/2] Hexagon: fix outdated `hex_new_*` references in comments
Some code comments refer to hex_new_value and hex_new_pred_value, which
have been transferred to DisasContext and, in the case of hex_new_value,
should now be accessed through get_result_gpr(). Let's update these
comments to reflect the new state of the codebase. Since they are only
meant to assist developers, we can replace the old names with some
pseudocode when convenient.
Signed-off-by: Matheus Tavares Bernardino
---
target/hexagon/genptr.c| 10 +-
target/hexagon/translate.c | 2 +-
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/target/hexagon/genptr.c b/target/hexagon/genptr.c
index cb2aa28a19..8d11d928c9 100644
--- a/target/hexagon/genptr.c
+++ b/target/hexagon/genptr.c
@@ -880,7 +880,7 @@ static void gen_endloop0(DisasContext *ctx)
/*
*if (hex_gpr[HEX_REG_LC0] > 1) {
*PC = hex_gpr[HEX_REG_SA0];
- *hex_new_value[HEX_REG_LC0] = hex_gpr[HEX_REG_LC0] - 1;
+ *result_gpr(HEX_REG_LC0) = hex_gpr[HEX_REG_LC0] - 1;
*}
*/
TCGLabel *label3 = gen_new_label();
@@ -899,7 +899,7 @@ static void gen_endloop1(DisasContext *ctx)
/*
*if (hex_gpr[HEX_REG_LC1] > 1) {
*PC = hex_gpr[HEX_REG_SA1];
- *hex_new_value[HEX_REG_LC1] = hex_gpr[HEX_REG_LC1] - 1;
+ *result_gpr(HEX_REG_LC1) = hex_gpr[HEX_REG_LC1] - 1;
*}
*/
TCGLabel *label = gen_new_label();
@@ -948,11 +948,11 @@ static void gen_endloop01(DisasContext *ctx)
/*
*if (hex_gpr[HEX_REG_LC0] > 1) {
*PC = hex_gpr[HEX_REG_SA0];
- *hex_new_value[HEX_REG_LC0] = hex_gpr[HEX_REG_LC0] - 1;
+ *result_gpr(HEX_REG_LC0) = hex_gpr[HEX_REG_LC0] - 1;
*} else {
*if (hex_gpr[HEX_REG_LC1] > 1) {
- *hex_next_pc = hex_gpr[HEX_REG_SA1];
- *hex_new_value[HEX_REG_LC1] = hex_gpr[HEX_REG_LC1] - 1;
+ *next_pc = hex_gpr[HEX_REG_SA1];
+ *result_gpr(HEX_REG_LC1) = hex_gpr[HEX_REG_LC1] - 1;
*}
*}
*/
diff --git a/target/hexagon/translate.c b/target/hexagon/translate.c
index b18f1a9051..8838ab2364 100644
--- a/target/hexagon/translate.c
+++ b/target/hexagon/translate.c
@@ -556,7 +556,7 @@ static void gen_start_packet(DisasContext *ctx)
}
/*
- * Preload the predicated pred registers into hex_new_pred_value[pred_num]
+ * Preload the predicated pred registers into ctx->new_pred_value[pred_num]
* Only endloop instructions conditionally write to pred registers
*/
if (ctx->need_commit && pkt->pkt_has_endloop) {
--
2.37.2
[PATCH 0/2] Hexagon: two minor cleanups to comments and python scripts
These are two minor follow ups to the last Hexagon pull, updating some stale code comments and removing undefined variables from error messages at python scripts. Matheus Tavares Bernardino (2): target/hexagon/*.py: remove undef vars from bad_register() Hexagon: fix outdated `hex_new_*` references in comments target/hexagon/genptr.c| 10 +- target/hexagon/translate.c | 2 +- target/hexagon/gen_helper_funcs.py | 8 target/hexagon/gen_tcg_funcs.py| 6 +++--- 4 files changed, 13 insertions(+), 13 deletions(-) -- 2.37.2
[PATCH v2] Hexagon (decode): look for pkts with multiple insns at the same slot
Each slot in a packet can be assigned to at most one instruction.
Although the assembler generally ought to enforce this rule, we better
be safe than sorry and also do some check to properly throw an "invalid
packet" exception on wrong slot assignments.
This should also make it easier to debug possible future errors caused
by missing updates to `find_iclass_slots()` rules in
target/hexagon/iclass.c.
Co-authored-by: Taylor Simpson
Signed-off-by: Taylor Simpson
Signed-off-by: Matheus Tavares Bernardino
---
Changes in v2:
- Only call decode_set_slot_number() with !disas_only, fixing the -d
in_asm case.
v1:
https://lore.kernel.org/qemu-devel/7a90f0925f182e56cf49ec3ec01484739fa2f174.1683226473.git.quic_mathb...@quicinc.com/
target/hexagon/decode.c | 30 +++---
tests/tcg/hexagon/invalid-slots.c | 29 +
tests/tcg/hexagon/Makefile.target | 11 +++
3 files changed, 67 insertions(+), 3 deletions(-)
create mode 100644 tests/tcg/hexagon/invalid-slots.c
diff --git a/target/hexagon/decode.c b/target/hexagon/decode.c
index 041c8de751..946c55cc71 100644
--- a/target/hexagon/decode.c
+++ b/target/hexagon/decode.c
@@ -1,5 +1,5 @@
/*
- * Copyright(c) 2019-2022 Qualcomm Innovation Center, Inc. All Rights
Reserved.
+ * Copyright(c) 2019-2023 Qualcomm Innovation Center, Inc. All Rights
Reserved.
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
@@ -797,7 +797,26 @@ static bool decode_parsebits_is_loopend(uint32_t
encoding32)
return bits == 0x2;
}
-static void
+static bool has_valid_slot_assignment(Packet *pkt)
+{
+int used_slots = 0;
+for (int i = 0; i < pkt->num_insns; i++) {
+int slot_mask;
+Insn *insn = &pkt->insn[i];
+if (decode_opcode_ends_loop(insn->opcode)) {
+/* We overload slot 0 for endloop. */
+continue;
+}
+slot_mask = 1 << insn->slot;
+if (used_slots & slot_mask) {
+return false;
+}
+used_slots |= slot_mask;
+}
+return true;
+}
+
+static bool
decode_set_slot_number(Packet *pkt)
{
int slot;
@@ -886,6 +905,8 @@ decode_set_slot_number(Packet *pkt)
/* Then push it to slot0 */
pkt->insn[slot1_iidx].slot = 0;
}
+
+return has_valid_slot_assignment(pkt);
}
/*
@@ -961,8 +982,11 @@ int decode_packet(int max_words, const uint32_t *words,
Packet *pkt,
decode_apply_extenders(pkt);
if (!disas_only) {
decode_remove_extenders(pkt);
+if (!decode_set_slot_number(pkt)) {
+/* Invalid packet */
+return 0;
+}
}
-decode_set_slot_number(pkt);
decode_fill_newvalue_regno(pkt);
if (pkt->pkt_has_hvx) {
diff --git a/tests/tcg/hexagon/invalid-slots.c
b/tests/tcg/hexagon/invalid-slots.c
new file mode 100644
index 00..366ce4f42f
--- /dev/null
+++ b/tests/tcg/hexagon/invalid-slots.c
@@ -0,0 +1,29 @@
+/*
+ * Copyright(c) 2023 Qualcomm Innovation Center, Inc. All Rights Reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+char mem[8] __attribute__((aligned(8)));
+
+int main()
+{
+asm volatile(
+"r0 = #mem\n"
+/* Invalid packet (2 instructions at slot 0): */
+".word 0xa1804100\n" /* { memw(r0) = r1; */
+".word 0x28032804\n" /* r3 = #0; r4 = #0 } */
+: : : "r0", "r3", "r4", "memory");
+return 0;
+}
diff --git a/tests/tcg/hexagon/Makefile.target
b/tests/tcg/hexagon/Makefile.target
index 7c94db4bc4..0c69216c6c 100644
--- a/tests/tcg/hexagon/Makefile.target
+++ b/tests/tcg/hexagon/Makefile.target
@@ -49,6 +49,17 @@ HEX_TESTS += vector_add_int
HEX_TESTS += scatter_gather
HEX_TESTS += hvx_misc
HEX_TESTS += hvx_histogram
+HEX_TESTS += invalid-slots
+
+run-and-check-exception = $(call run-test,$2,$3 2>$2.stderr; \
+ test $$? -eq 1 && grep -q "exception $(strip $1)" $2.stderr)
+
+run-invalid-slots: invalid-slots
+ $(call run-and-check-exception, 0x15, $@, $(QEMU) $(QEMU_OPTS) $<)
+
+run-plugin-invalid-slots-with-%: invalid-slots
+ $(call run-and-check-exception, 0x15
[PATCH] Hexagon (decode): look for pkts with multiple insns at the same slot
Each slot in a packet can be assigned to at most one instruction.
Although the assembler generally ought to enforce this rule, we better
be safe than sorry and also do some check to properly throw an "invalid
packet" exception on wrong slot assignments.
This should also make it easier to debug possible future errors caused
by missing updates to `find_iclass_slots()` rules in
target/hexagon/iclass.c.
Signed-off-by: Matheus Tavares Bernardino
Reviewed-by: Taylor Simpson
---
I extracted this patch from [1]. There are more changes needed to
support v73 in qemu, so the first patch in that series doesn't make
sense to be incorporated yet. This one is useful nonetheless.
[1]:
https://lore.kernel.org/qemu-devel/cover.1673616964.git.quic_mathb...@quicinc.com/
target/hexagon/decode.c | 30 +++---
tests/tcg/hexagon/invalid-slots.c | 29 +
tests/tcg/hexagon/Makefile.target | 11 +++
3 files changed, 67 insertions(+), 3 deletions(-)
create mode 100644 tests/tcg/hexagon/invalid-slots.c
diff --git a/target/hexagon/decode.c b/target/hexagon/decode.c
index 041c8de751..65ebf516a5 100644
--- a/target/hexagon/decode.c
+++ b/target/hexagon/decode.c
@@ -1,5 +1,5 @@
/*
- * Copyright(c) 2019-2022 Qualcomm Innovation Center, Inc. All Rights
Reserved.
+ * Copyright(c) 2019-2023 Qualcomm Innovation Center, Inc. All Rights
Reserved.
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
@@ -797,7 +797,26 @@ static bool decode_parsebits_is_loopend(uint32_t
encoding32)
return bits == 0x2;
}
-static void
+static bool has_valid_slot_assignment(Packet *pkt)
+{
+int used_slots = 0;
+for (int i = 0; i < pkt->num_insns; i++) {
+int slot_mask;
+Insn *insn = &pkt->insn[i];
+if (decode_opcode_ends_loop(insn->opcode)) {
+/* We overload slot 0 for endloop. */
+continue;
+}
+slot_mask = 1 << insn->slot;
+if (used_slots & slot_mask) {
+return false;
+}
+used_slots |= slot_mask;
+}
+return true;
+}
+
+static bool
decode_set_slot_number(Packet *pkt)
{
int slot;
@@ -886,6 +905,8 @@ decode_set_slot_number(Packet *pkt)
/* Then push it to slot0 */
pkt->insn[slot1_iidx].slot = 0;
}
+
+return has_valid_slot_assignment(pkt);
}
/*
@@ -962,7 +983,10 @@ int decode_packet(int max_words, const uint32_t *words,
Packet *pkt,
if (!disas_only) {
decode_remove_extenders(pkt);
}
-decode_set_slot_number(pkt);
+if (!decode_set_slot_number(pkt)) {
+/* Invalid packet */
+return 0;
+}
decode_fill_newvalue_regno(pkt);
if (pkt->pkt_has_hvx) {
diff --git a/tests/tcg/hexagon/invalid-slots.c
b/tests/tcg/hexagon/invalid-slots.c
new file mode 100644
index 00..366ce4f42f
--- /dev/null
+++ b/tests/tcg/hexagon/invalid-slots.c
@@ -0,0 +1,29 @@
+/*
+ * Copyright(c) 2023 Qualcomm Innovation Center, Inc. All Rights Reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+char mem[8] __attribute__((aligned(8)));
+
+int main()
+{
+asm volatile(
+"r0 = #mem\n"
+/* Invalid packet (2 instructions at slot 0): */
+".word 0xa1804100\n" /* { memw(r0) = r1; */
+".word 0x28032804\n" /* r3 = #0; r4 = #0 } */
+: : : "r0", "r3", "r4", "memory");
+return 0;
+}
diff --git a/tests/tcg/hexagon/Makefile.target
b/tests/tcg/hexagon/Makefile.target
index 7c94db4bc4..0c69216c6c 100644
--- a/tests/tcg/hexagon/Makefile.target
+++ b/tests/tcg/hexagon/Makefile.target
@@ -49,6 +49,17 @@ HEX_TESTS += vector_add_int
HEX_TESTS += scatter_gather
HEX_TESTS += hvx_misc
HEX_TESTS += hvx_histogram
+HEX_TESTS += invalid-slots
+
+run-and-check-exception = $(call run-test,$2,$3 2>$2.stderr; \
+ test $$? -eq 1 && grep -q "exception $(strip $1)" $2.stderr)
+
+run-invalid-slots: invalid-slots
+ $(call run-and-check-exception, 0x15, $@, $(QEMU) $(QEMU_OPTS) $<)
+
+run-plugin-invalid-slots-with-%: invalid-slots
+ $(call run-and-check-exception, 0x15, $@, $(QEMU) $(QEMU_OPTS) \
+ -plugin $(PLUGIN_LIB)/$(call extract-plugin,$@) $(call strip-plugin,$<))
HEX_TESTS += test_abs
HEX_TESTS += test_bitcnt
--
2.37.2
[PATCH 0/2] Hexagon: improve output for arch version debugging
If we run qemu with an Hexagon binary compiled to an arch version that
is higher than the threshold modeled by qemu, we will get the following
error:
qemu-hexagon: unable to find CPU model 'unknown'
This can be confusing ("Was qemu unable to read the arch version from
this binary? Or did it read but does not know such version?").
And running `qemu-hexagon -cpu help` doesn't help either, as it just
errors out with no output. This patchset tries to improve this process.
https://lore.kernel.org/qemu-devel/cover.1673616964.git.quic_mathb...@quicinc.com/
Matheus Tavares Bernardino (2):
Hexagon: list available CPUs with `-cpu help`
Hexagon: append eflags to unknown cpu model string
linux-user/hexagon/target_elf.h | 7 ++-
target/hexagon/cpu.h| 3 +++
target/hexagon/cpu.c| 20
3 files changed, 29 insertions(+), 1 deletion(-)
--
2.37.2
[PATCH 2/2] Hexagon: append eflags to unknown cpu model string
Running qemu-hexagon with a binary that was compiled for an arch version
unknown by qemu can produce a somewhat confusing message:
qemu-hexagon: unable to find CPU model 'unknown'
Let's give a bit more info by appending the eflags so that the message
becomes:
qemu-hexagon: unable to find CPU model 'unknown (0x69)'
Signed-off-by: Matheus Tavares Bernardino
---
linux-user/hexagon/target_elf.h | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/linux-user/hexagon/target_elf.h b/linux-user/hexagon/target_elf.h
index b4e9f40527..f47e130537 100644
--- a/linux-user/hexagon/target_elf.h
+++ b/linux-user/hexagon/target_elf.h
@@ -20,6 +20,9 @@
static inline const char *cpu_get_model(uint32_t eflags)
{
+static char buf[32];
+int err;
+
/* For now, treat anything newer than v5 as a v67 */
/* FIXME - Disable instructions that are newer than the specified arch */
if (eflags == 0x04 ||/* v5 */
@@ -34,7 +37,9 @@ static inline const char *cpu_get_model(uint32_t eflags)
) {
return "v67";
}
-return "unknown";
+
+err = snprintf(buf, sizeof(buf), "unknown (0x%x)", eflags);
+return err >= 0 && err < sizeof(buf) ? buf : "unknown";
}
#endif
--
2.37.2
[PATCH 1/2] Hexagon: list available CPUs with `-cpu help`
Currently, qemu-hexagon only models the v67 cpu. Nonetheless if we try
to get this information with `-cpu help`, qemu just exists with an error
code and no output. Let's correct that.
The code is basically a copy from target/alpha/cpu.h, but we strip the
"-hexagon-cpu" suffix before printing. This is to avoid confusing
situations like the following:
$ qemu-hexagon -cpu help
Available CPUs:
v67-hexagon-cpu
$ qemu-hexagon -cpu v67-hexagon-cpu ./prog
qemu-hexagon: unable to find CPU model 'v67-hexagon-cpu'
Signed-off-by: Matheus Tavares Bernardino
---
target/hexagon/cpu.h | 3 +++
target/hexagon/cpu.c | 20
2 files changed, 23 insertions(+)
diff --git a/target/hexagon/cpu.h b/target/hexagon/cpu.h
index 81b663ecfb..d59e5bbff8 100644
--- a/target/hexagon/cpu.h
+++ b/target/hexagon/cpu.h
@@ -44,6 +44,9 @@
#define TYPE_HEXAGON_CPU_V67 HEXAGON_CPU_TYPE_NAME("v67")
+void hexagon_cpu_list(void);
+#define cpu_list hexagon_cpu_list
+
#define MMU_USER_IDX 0
typedef struct {
diff --git a/target/hexagon/cpu.c b/target/hexagon/cpu.c
index ab40cfc283..e8c2b5e910 100644
--- a/target/hexagon/cpu.c
+++ b/target/hexagon/cpu.c
@@ -29,6 +29,26 @@ static void hexagon_v67_cpu_init(Object *obj)
{
}
+static void hexagon_cpu_list_entry(gpointer data, gpointer user_data)
+{
+ObjectClass *oc = data;
+char *name = g_strdup(object_class_get_name(oc));
+if (g_str_has_suffix(name, HEXAGON_CPU_TYPE_SUFFIX)) {
+name[strlen(name) - strlen(HEXAGON_CPU_TYPE_SUFFIX)] = '\0';
+}
+qemu_printf(" %s\n", name);
+g_free(name);
+}
+
+void hexagon_cpu_list(void)
+{
+GSList *list;
+list = object_class_get_list_sorted(TYPE_HEXAGON_CPU, false);
+qemu_printf("Available CPUs:\n");
+g_slist_foreach(list, hexagon_cpu_list_entry, NULL);
+g_slist_free(list);
+}
+
static ObjectClass *hexagon_cpu_class_by_name(const char *cpu_model)
{
ObjectClass *oc;
--
2.37.2
[PATCH] Hexagon (target/hexagon/*.py): raise exception on reg parsing error
Currently, the python scripts used for the hexagon building will not
abort the compilation when there is an error parsing a register. Let's
make the compilation properly fail in such cases by rasing an exception
instead of just printing a warning message, which might get lost in the
output.
This patch was generated with:
git grep -l "Bad register" *hexagon* | \
xargs sed -i "" -e 's/print("Bad register parse: "[,
]*\([^)]*\))/hex_common.bad_register(\1)/g'
Plus the bad_register() helper added to hex_common.py.
Signed-off-by: Matheus Tavares Bernardino
---
target/hexagon/gen_analyze_funcs.py | 30 +-
target/hexagon/gen_helper_funcs.py | 14 ++---
target/hexagon/gen_helper_protos.py | 2 +-
target/hexagon/gen_idef_parser_funcs.py | 2 +-
target/hexagon/gen_tcg_funcs.py | 78 -
target/hexagon/hex_common.py| 3 +
6 files changed, 66 insertions(+), 63 deletions(-)
diff --git a/target/hexagon/gen_analyze_funcs.py
b/target/hexagon/gen_analyze_funcs.py
index c74443da78..0584ef9d4d 100755
--- a/target/hexagon/gen_analyze_funcs.py
+++ b/target/hexagon/gen_analyze_funcs.py
@@ -45,7 +45,7 @@ def analyze_opn_old(f, tag, regtype, regid, regno):
f.write(f"const int {regN} = insn->regno[{regno}];\n")
f.write(f"ctx_log_reg_write(ctx, {regN}, {predicated});\n")
else:
-print("Bad register parse: ", regtype, regid)
+hex_common.bad_register(regtype, regid)
elif regtype == "P":
if regid in {"s", "t", "u", "v"}:
f.write(f"//const int {regN} = insn->regno[{regno}];\n")
@@ -53,7 +53,7 @@ def analyze_opn_old(f, tag, regtype, regid, regno):
f.write(f"const int {regN} = insn->regno[{regno}];\n")
f.write(f"ctx_log_pred_write(ctx, {regN});\n")
else:
-print("Bad register parse: ", regtype, regid)
+hex_common.bad_register(regtype, regid)
elif regtype == "C":
if regid == "ss":
f.write(
@@ -70,12 +70,12 @@ def analyze_opn_old(f, tag, regtype, regid, regno):
f.write(f"const int {regN} = insn->regno[{regno}] " "+
HEX_REG_SA0;\n")
f.write(f"ctx_log_reg_write(ctx, {regN}, {predicated});\n")
else:
-print("Bad register parse: ", regtype, regid)
+hex_common.bad_register(regtype, regid)
elif regtype == "M":
if regid == "u":
f.write(f"//const int {regN} = insn->regno[{regno}];\n")
else:
-print("Bad register parse: ", regtype, regid)
+hex_common.bad_register(regtype, regid)
elif regtype == "V":
newv = "EXT_DFL"
if hex_common.is_new_result(tag):
@@ -95,7 +95,7 @@ def analyze_opn_old(f, tag, regtype, regid, regno):
f.write(f"const int {regN} = insn->regno[{regno}];\n")
f.write(f"ctx_log_vreg_write(ctx, {regN}, {newv}, "
f"{predicated});\n")
else:
-print("Bad register parse: ", regtype, regid)
+hex_common.bad_register(regtype, regid)
elif regtype == "Q":
if regid in {"d", "e", "x"}:
f.write(f"const int {regN} = insn->regno[{regno}];\n")
@@ -103,7 +103,7 @@ def analyze_opn_old(f, tag, regtype, regid, regno):
elif regid in {"s", "t", "u", "v"}:
f.write(f"//const int {regN} = insn->regno[{regno}];\n")
else:
-print("Bad register parse: ", regtype, regid)
+hex_common.bad_register(regtype, regid)
elif regtype == "G":
if regid in {"dd"}:
f.write(f"//const int {regN} = insn->regno[{regno}];\n")
@@ -114,7 +114,7 @@ def analyze_opn_old(f, tag, regtype, regid, regno):
elif regid in {"s"}:
f.write(f"//const int {regN} = insn->regno[{regno}];\n")
else:
-print("Bad register parse: ", regtype, regid)
+hex_common.bad_register(regtype, regid)
elif regtype == "S":
if regid in {"dd"}:
f.write(f"//const int {regN} = insn->regno[{regno}];\n")
@@ -125,9 +125,9 @@ def analyze_opn_old(f, tag, regtype, regid, regno):
elif regid in {"s"}:
f.write(f"//const int {regN} = insn->regno[{regno}];\n")
else:
-print("Bad register parse: ", regtype, regid)
+
[PATCH v3 6/6] Hexagon (linux-user/hexagon): handle breakpoints
This enables LLDB to work with hexagon linux-user mode through the GDB remote protocol. Helped-by: Richard Henderson Signed-off-by: Matheus Tavares Bernardino --- linux-user/hexagon/cpu_loop.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/linux-user/hexagon/cpu_loop.c b/linux-user/hexagon/cpu_loop.c index b84e25bf71..7f1499ed28 100644 --- a/linux-user/hexagon/cpu_loop.c +++ b/linux-user/hexagon/cpu_loop.c @@ -63,6 +63,9 @@ void cpu_loop(CPUHexagonState *env) case EXCP_ATOMIC: cpu_exec_step_atomic(cs); break; +case EXCP_DEBUG: +force_sig_fault(TARGET_SIGTRAP, TARGET_TRAP_BRKPT, 0); +break; default: EXCP_DUMP(env, "\nqemu: unhandled CPU exception %#x - aborting\n", trapnr); -- 2.37.2
[PATCH v3 3/6] Hexagon: add core gdbstub xml data for LLDB
Signed-off-by: Matheus Tavares Bernardino --- MAINTAINERS| 1 + configs/targets/hexagon-linux-user.mak | 1 + target/hexagon/cpu.c | 3 +- gdb-xml/hexagon-core.xml | 84 ++ 4 files changed, 88 insertions(+), 1 deletion(-) create mode 100644 gdb-xml/hexagon-core.xml diff --git a/MAINTAINERS b/MAINTAINERS index b22b85bc3a..95037d9f34 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -225,6 +225,7 @@ F: tests/tcg/hexagon/ F: disas/hexagon.c F: configs/targets/hexagon-linux-user/default.mak F: docker/dockerfiles/debian-hexagon-cross.docker +F: gdb-xml/hexagon*.xml Hexagon idef-parser M: Alessandro Di Federico diff --git a/configs/targets/hexagon-linux-user.mak b/configs/targets/hexagon-linux-user.mak index 003ed0a408..fd5e222d4f 100644 --- a/configs/targets/hexagon-linux-user.mak +++ b/configs/targets/hexagon-linux-user.mak @@ -1 +1,2 @@ TARGET_ARCH=hexagon +TARGET_XML_FILES=gdb-xml/hexagon-core.xml diff --git a/target/hexagon/cpu.c b/target/hexagon/cpu.c index ab40cfc283..a59d964574 100644 --- a/target/hexagon/cpu.c +++ b/target/hexagon/cpu.c @@ -358,8 +358,9 @@ static void hexagon_cpu_class_init(ObjectClass *c, void *data) cc->get_pc = hexagon_cpu_get_pc; cc->gdb_read_register = hexagon_gdb_read_register; cc->gdb_write_register = hexagon_gdb_write_register; -cc->gdb_num_core_regs = TOTAL_PER_THREAD_REGS + NUM_VREGS + NUM_QREGS; +cc->gdb_num_core_regs = TOTAL_PER_THREAD_REGS; cc->gdb_stop_before_watchpoint = true; +cc->gdb_core_xml_file = "hexagon-core.xml"; cc->disas_set_info = hexagon_cpu_disas_set_info; cc->tcg_ops = &hexagon_tcg_ops; } diff --git a/gdb-xml/hexagon-core.xml b/gdb-xml/hexagon-core.xml new file mode 100644 index 00..e181163cff --- /dev/null +++ b/gdb-xml/hexagon-core.xml @@ -0,0 +1,84 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + -- 2.37.2
[PATCH v3 4/6] Hexagon (gdbstub): fix p3:0 read and write via stub
From: Brian Cain
Signed-off-by: Brian Cain
Co-authored-by: Sid Manning
Signed-off-by: Sid Manning
Co-authored-by: Matheus Tavares Bernardino
Signed-off-by: Matheus Tavares Bernardino
Reviewed-by: Taylor Simpson
---
target/hexagon/gdbstub.c | 16
1 file changed, 16 insertions(+)
diff --git a/target/hexagon/gdbstub.c b/target/hexagon/gdbstub.c
index 46083da620..a06fed9f18 100644
--- a/target/hexagon/gdbstub.c
+++ b/target/hexagon/gdbstub.c
@@ -25,6 +25,14 @@ int hexagon_gdb_read_register(CPUState *cs, GByteArray
*mem_buf, int n)
HexagonCPU *cpu = HEXAGON_CPU(cs);
CPUHexagonState *env = &cpu->env;
+if (n == HEX_REG_P3_0_ALIASED) {
+uint32_t p3_0 = 0;
+for (int i = 0; i < NUM_PREGS; i++) {
+p3_0 = deposit32(p3_0, i * 8, 8, env->pred[i]);
+}
+return gdb_get_regl(mem_buf, p3_0);
+}
+
if (n < TOTAL_PER_THREAD_REGS) {
return gdb_get_regl(mem_buf, env->gpr[n]);
}
@@ -37,6 +45,14 @@ int hexagon_gdb_write_register(CPUState *cs, uint8_t
*mem_buf, int n)
HexagonCPU *cpu = HEXAGON_CPU(cs);
CPUHexagonState *env = &cpu->env;
+if (n == HEX_REG_P3_0_ALIASED) {
+uint32_t p3_0 = ldtul_p(mem_buf);
+for (int i = 0; i < NUM_PREGS; i++) {
+env->pred[i] = extract32(p3_0, i * 8, 8);
+}
+return sizeof(target_ulong);
+}
+
if (n < TOTAL_PER_THREAD_REGS) {
env->gpr[n] = ldtul_p(mem_buf);
return sizeof(target_ulong);
--
2.37.2
[PATCH v3 5/6] Hexagon (gdbstub): add HVX support
From: Taylor Simpson
Signed-off-by: Taylor Simpson
Co-authored-by: Brian Cain
Signed-off-by: Brian Cain
Co-authored-by: Matheus Tavares Bernardino
Signed-off-by: Matheus Tavares Bernardino
---
configs/targets/hexagon-linux-user.mak | 2 +-
target/hexagon/internal.h | 2 +
target/hexagon/cpu.c | 6 ++
target/hexagon/gdbstub.c | 68 ++
gdb-xml/hexagon-hvx.xml| 96 ++
5 files changed, 173 insertions(+), 1 deletion(-)
create mode 100644 gdb-xml/hexagon-hvx.xml
diff --git a/configs/targets/hexagon-linux-user.mak
b/configs/targets/hexagon-linux-user.mak
index fd5e222d4f..2765a4c563 100644
--- a/configs/targets/hexagon-linux-user.mak
+++ b/configs/targets/hexagon-linux-user.mak
@@ -1,2 +1,2 @@
TARGET_ARCH=hexagon
-TARGET_XML_FILES=gdb-xml/hexagon-core.xml
+TARGET_XML_FILES=gdb-xml/hexagon-core.xml gdb-xml/hexagon-hvx.xml
diff --git a/target/hexagon/internal.h b/target/hexagon/internal.h
index b1bfadc3f5..d732b6bb3c 100644
--- a/target/hexagon/internal.h
+++ b/target/hexagon/internal.h
@@ -33,6 +33,8 @@
int hexagon_gdb_read_register(CPUState *cpu, GByteArray *buf, int reg);
int hexagon_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
+int hexagon_hvx_gdb_read_register(CPUHexagonState *env, GByteArray *mem_buf,
int n);
+int hexagon_hvx_gdb_write_register(CPUHexagonState *env, uint8_t *mem_buf, int
n);
void hexagon_debug_vreg(CPUHexagonState *env, int regnum);
void hexagon_debug_qreg(CPUHexagonState *env, int regnum);
diff --git a/target/hexagon/cpu.c b/target/hexagon/cpu.c
index a59d964574..2e36903d9d 100644
--- a/target/hexagon/cpu.c
+++ b/target/hexagon/cpu.c
@@ -24,6 +24,7 @@
#include "hw/qdev-properties.h"
#include "fpu/softfloat-helpers.h"
#include "tcg/tcg.h"
+#include "exec/gdbstub.h"
static void hexagon_v67_cpu_init(Object *obj)
{
@@ -315,6 +316,11 @@ static void hexagon_cpu_realize(DeviceState *dev, Error
**errp)
return;
}
+gdb_register_coprocessor(cs, hexagon_hvx_gdb_read_register,
+ hexagon_hvx_gdb_write_register,
+ NUM_VREGS + NUM_QREGS,
+ "hexagon-hvx.xml", 0);
+
qemu_init_vcpu(cs);
cpu_reset(cs);
diff --git a/target/hexagon/gdbstub.c b/target/hexagon/gdbstub.c
index a06fed9f18..54d37e006e 100644
--- a/target/hexagon/gdbstub.c
+++ b/target/hexagon/gdbstub.c
@@ -60,3 +60,71 @@ int hexagon_gdb_write_register(CPUState *cs, uint8_t
*mem_buf, int n)
g_assert_not_reached();
}
+
+static int gdb_get_vreg(CPUHexagonState *env, GByteArray *mem_buf, int n)
+{
+int total = 0;
+int i;
+for (i = 0; i < ARRAY_SIZE(env->VRegs[n].uw); i++) {
+total += gdb_get_regl(mem_buf, env->VRegs[n].uw[i]);
+}
+return total;
+}
+
+static int gdb_get_qreg(CPUHexagonState *env, GByteArray *mem_buf, int n)
+{
+int total = 0;
+int i;
+for (i = 0; i < ARRAY_SIZE(env->QRegs[n].uw); i++) {
+total += gdb_get_regl(mem_buf, env->QRegs[n].uw[i]);
+}
+return total;
+}
+
+int hexagon_hvx_gdb_read_register(CPUHexagonState *env, GByteArray *mem_buf,
int n)
+{
+if (n < NUM_VREGS) {
+return gdb_get_vreg(env, mem_buf, n);
+}
+n -= NUM_VREGS;
+
+if (n < NUM_QREGS) {
+return gdb_get_qreg(env, mem_buf, n);
+}
+
+g_assert_not_reached();
+}
+
+static int gdb_put_vreg(CPUHexagonState *env, uint8_t *mem_buf, int n)
+{
+int i;
+for (i = 0; i < ARRAY_SIZE(env->VRegs[n].uw); i++) {
+env->VRegs[n].uw[i] = ldtul_p(mem_buf);
+mem_buf += 4;
+}
+return MAX_VEC_SIZE_BYTES;
+}
+
+static int gdb_put_qreg(CPUHexagonState *env, uint8_t *mem_buf, int n)
+{
+int i;
+for (i = 0; i < ARRAY_SIZE(env->QRegs[n].uw); i++) {
+env->QRegs[n].uw[i] = ldtul_p(mem_buf);
+mem_buf += 4;
+}
+return MAX_VEC_SIZE_BYTES / 8;
+}
+
+int hexagon_hvx_gdb_write_register(CPUHexagonState *env, uint8_t *mem_buf, int
n)
+{
+ if (n < NUM_VREGS) {
+return gdb_put_vreg(env, mem_buf, n);
+}
+n -= NUM_VREGS;
+
+if (n < NUM_QREGS) {
+return gdb_put_qreg(env, mem_buf, n);
+}
+
+g_assert_not_reached();
+}
diff --git a/gdb-xml/hexagon-hvx.xml b/gdb-xml/hexagon-hvx.xml
new file mode 100644
index 00..5f2e220733
--- /dev/null
+++ b/gdb-xml/hexagon-hvx.xml
@@ -0,0 +1,96 @@
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
--
2.37.2
[PATCH v3 2/6] gdbstub: add test for untimely stop-reply packets
In the previous commit, we modified gdbstub.c to only send stop-reply
packets as a response to GDB commands that accept it. Now, let's add a
test for this intended behavior. Running this test before the fix from
the previous commit fails as QEMU sends a stop-reply packet
asynchronously, when GDB was in fact waiting an ACK.
Signed-off-by: Matheus Tavares Bernardino
Acked-by: Alex Bennée
---
tests/guest-debug/run-test.py| 16
.../tcg/multiarch/system/Makefile.softmmu-target | 16 +++-
2 files changed, 27 insertions(+), 5 deletions(-)
diff --git a/tests/guest-debug/run-test.py b/tests/guest-debug/run-test.py
index d865e46ecd..de6106a5e5 100755
--- a/tests/guest-debug/run-test.py
+++ b/tests/guest-debug/run-test.py
@@ -26,11 +26,12 @@ def get_args():
parser.add_argument("--qargs", help="Qemu arguments for test")
parser.add_argument("--binary", help="Binary to debug",
required=True)
-parser.add_argument("--test", help="GDB test script",
-required=True)
+parser.add_argument("--test", help="GDB test script")
parser.add_argument("--gdb", help="The gdb binary to use",
default=None)
+parser.add_argument("--gdb-args", help="Additional gdb arguments")
parser.add_argument("--output", help="A file to redirect output to")
+parser.add_argument("--stderr", help="A file to redirect stderr to")
return parser.parse_args()
@@ -58,6 +59,10 @@ def log(output, msg):
output = open(args.output, "w")
else:
output = None
+if args.stderr:
+stderr = open(args.stderr, "w")
+else:
+stderr = None
socket_dir = TemporaryDirectory("qemu-gdbstub")
socket_name = os.path.join(socket_dir.name, "gdbstub.socket")
@@ -77,6 +82,8 @@ def log(output, msg):
# Now launch gdb with our test and collect the result
gdb_cmd = "%s %s" % (args.gdb, args.binary)
+if args.gdb_args:
+gdb_cmd += " %s" % (args.gdb_args)
# run quietly and ignore .gdbinit
gdb_cmd += " -q -n -batch"
# disable prompts in case of crash
@@ -84,13 +91,14 @@ def log(output, msg):
# connect to remote
gdb_cmd += " -ex 'target remote %s'" % (socket_name)
# finally the test script itself
-gdb_cmd += " -x %s" % (args.test)
+if args.test:
+gdb_cmd += " -x %s" % (args.test)
sleep(1)
log(output, "GDB CMD: %s" % (gdb_cmd))
-result = subprocess.call(gdb_cmd, shell=True, stdout=output)
+result = subprocess.call(gdb_cmd, shell=True, stdout=output, stderr=stderr)
# A result of greater than 128 indicates a fatal signal (likely a
# crash due to gdb internal failure). That's a problem for GDB and
diff --git a/tests/tcg/multiarch/system/Makefile.softmmu-target
b/tests/tcg/multiarch/system/Makefile.softmmu-target
index 5f432c95f3..fe40195d39 100644
--- a/tests/tcg/multiarch/system/Makefile.softmmu-target
+++ b/tests/tcg/multiarch/system/Makefile.softmmu-target
@@ -27,6 +27,20 @@ run-gdbstub-memory: memory
"-monitor none -display none -chardev
file$(COMMA)path=$<.out$(COMMA)id=output $(QEMU_OPTS)" \
--bin $< --test $(MULTIARCH_SRC)/gdbstub/memory.py, \
softmmu gdbstub support)
+
+run-gdbstub-untimely-packet: hello
+ $(call run-test, $@, $(GDB_SCRIPT) \
+ --gdb $(HAVE_GDB_BIN) \
+ --gdb-args "-ex 'set debug remote 1'" \
+ --output untimely-packet.gdb.out \
+ --stderr untimely-packet.gdb.err \
+ --qemu $(QEMU) \
+ --bin $< --qargs \
+ "-monitor none -display none -chardev
file$(COMMA)path=untimely-packet.out$(COMMA)id=output $(QEMU_OPTS)", \
+ "softmmu gdbstub untimely packets")
+ $(call quiet-command, \
+ (! grep -Fq 'Packet instead of Ack, ignoring it'
untimely-packet.gdb.err), \
+ "GREP", "file untimely-packet.gdb.err")
else
run-gdbstub-%:
$(call skip-test, "gdbstub test $*", "no guest arch support")
@@ -36,4 +50,4 @@ run-gdbstub-%:
$(call skip-test, "gdbstub test $*", "need working gdb")
endif
-MULTIARCH_RUNS += run-gdbstub-memory
+MULTIARCH_RUNS += run-gdbstub-memory run-gdbstub-untimely-packet
--
2.37.2
[PATCH v3 0/6] Hexagon: add lldb support
This series allows hexagon programs to be debugged under qemu user-mode
through LLDB and qemu's gdbstub. LLDB implements the GDB remote serial
protocol, so most of the necessary changes are in the Hexagon part itself.
However, one fix is needed at the arch-independent side too.
Changes in v3:
- Patches 1 & 2: added Alex ack.
- Patch 1: added missing allow_stop_reply guard to
gdbstub/user.c:gdb_exit().
- Patches 3 & 5: replaced qRegisterInfo with gdb-xml.
- Patch 6: Used force_sig_fault().
v2:
https://lore.kernel.org/qemu-devel/cover.1681993775.git.quic_mathb...@quicinc.com/
v1:
https://lore.kernel.org/qemu-devel/cover.1680808943.git.quic_mathb...@quicinc.com/
Brian Cain (1):
Hexagon (gdbstub): fix p3:0 read and write via stub
Matheus Tavares Bernardino (4):
gdbstub: only send stop-reply packets when allowed to
gdbstub: add test for untimely stop-reply packets
Hexagon: add core gdbstub xml data for LLDB
Hexagon (linux-user/hexagon): handle breakpoints
Taylor Simpson (1):
Hexagon (gdbstub): add HVX support
MAINTAINERS | 1 +
configs/targets/hexagon-linux-user.mak| 1 +
gdbstub/internals.h | 5 +
target/hexagon/internal.h | 2 +
gdbstub/gdbstub.c | 37 +--
gdbstub/softmmu.c | 13 ++-
gdbstub/user.c| 24 +++--
linux-user/hexagon/cpu_loop.c | 3 +
target/hexagon/cpu.c | 9 +-
target/hexagon/gdbstub.c | 84
gdb-xml/hexagon-core.xml | 84
gdb-xml/hexagon-hvx.xml | 96 +++
tests/guest-debug/run-test.py | 16 +++-
.../multiarch/system/Makefile.softmmu-target | 16 +++-
14 files changed, 366 insertions(+), 25 deletions(-)
create mode 100644 gdb-xml/hexagon-core.xml
create mode 100644 gdb-xml/hexagon-hvx.xml
Range-diff against v2:
1: b4ec188e67 ! 1: a49c0897fc gdbstub: only send stop-reply packets when
allowed to
@@ gdbstub/softmmu.c: void gdb_exit(int code)
}
## gdbstub/user.c ##
+@@ gdbstub/user.c: void gdb_exit(int code)
+
+ trace_gdbstub_op_exiting((uint8_t)code);
+
+-snprintf(buf, sizeof(buf), "W%02x", (uint8_t)code);
+-gdb_put_packet(buf);
++if (gdbserver_state.allow_stop_reply) {
++snprintf(buf, sizeof(buf), "W%02x", (uint8_t)code);
++gdb_put_packet(buf);
++gdbserver_state.allow_stop_reply = false;
++}
+ }
+
+ int gdb_handlesig(CPUState *cpu, int sig)
@@ gdbstub/user.c: int gdb_handlesig(CPUState *cpu, int sig)
if (sig != 0) {
2: a91ec99036 = 2: a30d93b9a8 gdbstub: add test for untimely stop-reply
packets
3: 40aa60ee50 < -: -- gdbstub: add support for the qRegisterInfo query
4: 090707eea1 < -: -- Hexagon: support qRegisterInfo at gdbstub
-: -- > 3: d25a3a7933 Hexagon: add core gdbstub xml data for LLDB
5: 06ed954dab ! 4: 32e7de567c Hexagon (gdbstub): fix p3:0 read and write via
stub
@@ Metadata
## Commit message ##
Hexagon (gdbstub): fix p3:0 read and write via stub
+Signed-off-by: Brian Cain
Co-authored-by: Sid Manning
Signed-off-by: Sid Manning
-Signed-off-by: Brian Cain
Co-authored-by: Matheus Tavares Bernardino
Signed-off-by: Matheus Tavares Bernardino
Reviewed-by: Taylor Simpson
6: 880c86bf2b ! 5: 17cb32f34d Hexagon (gdbstub): add HVX support
@@ Metadata
## Commit message ##
Hexagon (gdbstub): add HVX support
+Signed-off-by: Taylor Simpson
Co-authored-by: Brian Cain
Signed-off-by: Brian Cain
-Signed-off-by: Taylor Simpson
Co-authored-by: Matheus Tavares Bernardino
Signed-off-by: Matheus Tavares Bernardino
- ## target/hexagon/gdbstub.c ##
+ ## configs/targets/hexagon-linux-user.mak ##
+@@
+ TARGET_ARCH=hexagon
+-TARGET_XML_FILES=gdb-xml/hexagon-core.xml
++TARGET_XML_FILES=gdb-xml/hexagon-core.xml gdb-xml/hexagon-hvx.xml
+
+ ## target/hexagon/internal.h ##
@@
- #include "cpu.h"
- #include "internal.h"
+ int hexagon_gdb_read_register(CPUState *cpu, GByteArray *buf, int reg);
+ int hexagon_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);
++int hexagon_hvx_gdb_read_register(CPUHexagonState *env, GByteArray
*mem_buf, int n);
++int hexagon_hvx_gdb_write_register(CPUHexagonState *env, uint8_t
*mem_buf, int n);
+
+ void hexagon_debug_vreg(CPUHexagonState *env, int regnum);
+ void hexagon_debug_qreg(CPUHexagonState *env, int regnum);
+
+ ## target/hexagon/cpu.c ##
+@@
+ #include
[PATCH v3 1/6] gdbstub: only send stop-reply packets when allowed to
GDB's remote serial protocol allows stop-reply messages to be sent by
the stub either as a notification packet or as a reply to a GDB command
(provided that the cmd accepts such a response). QEMU currently does not
implement notification packets, so it should only send stop-replies
synchronously and when requested. Nevertheless, it still issues
unsolicited stop messages through gdb_vm_state_change().
Although this behavior doesn't seem to cause problems with GDB itself
(the messages are just ignored), it can impact other debuggers that
implement the GDB remote serial protocol, like hexagon-lldb. Let's
change the gdbstub to send stop messages only as a response to a
previous GDB command that accepts such a reply.
Signed-off-by: Matheus Tavares Bernardino
Acked-by: Alex Bennée
---
gdbstub/internals.h | 5 +
gdbstub/gdbstub.c | 37 -
gdbstub/softmmu.c | 13 +++--
gdbstub/user.c | 24
4 files changed, 60 insertions(+), 19 deletions(-)
diff --git a/gdbstub/internals.h b/gdbstub/internals.h
index 94ddff4495..33d21d6488 100644
--- a/gdbstub/internals.h
+++ b/gdbstub/internals.h
@@ -65,6 +65,11 @@ typedef struct GDBState {
GByteArray *mem_buf;
int sstep_flags;
int supported_sstep_flags;
+/*
+ * Whether we are allowed to send a stop reply packet at this moment.
+ * Must be set off after sending the stop reply itself.
+ */
+bool allow_stop_reply;
} GDBState;
/* lives in main gdbstub.c */
diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index 0760d78685..be18568d0a 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -777,6 +777,10 @@ typedef void (*GdbCmdHandler)(GArray *params, void
*user_ctx);
/*
* cmd_startswith -> cmd is compared using startswith
*
+ * allow_stop_reply -> true iff the gdbstub can respond to this command with a
+ * "stop reply" packet. The list of commands that accept such response is
+ * defined at the GDB Remote Serial Protocol documentation. see:
+ *
https://sourceware.org/gdb/onlinedocs/gdb/Stop-Reply-Packets.html#Stop-Reply-Packets.
*
* schema definitions:
* Each schema parameter entry consists of 2 chars,
@@ -802,6 +806,7 @@ typedef struct GdbCmdParseEntry {
const char *cmd;
bool cmd_startswith;
const char *schema;
+bool allow_stop_reply;
} GdbCmdParseEntry;
static inline int startswith(const char *string, const char *pattern)
@@ -835,6 +840,7 @@ static int process_string_cmd(void *user_ctx, const char
*data,
}
}
+gdbserver_state.allow_stop_reply = cmd->allow_stop_reply;
cmd->handler(params, user_ctx);
return 0;
}
@@ -1283,11 +1289,14 @@ static void handle_v_attach(GArray *params, void
*user_ctx)
gdbserver_state.g_cpu = cpu;
gdbserver_state.c_cpu = cpu;
-g_string_printf(gdbserver_state.str_buf, "T%02xthread:", GDB_SIGNAL_TRAP);
-gdb_append_thread_id(cpu, gdbserver_state.str_buf);
-g_string_append_c(gdbserver_state.str_buf, ';');
+if (gdbserver_state.allow_stop_reply) {
+g_string_printf(gdbserver_state.str_buf, "T%02xthread:",
GDB_SIGNAL_TRAP);
+gdb_append_thread_id(cpu, gdbserver_state.str_buf);
+g_string_append_c(gdbserver_state.str_buf, ';');
+gdbserver_state.allow_stop_reply = false;
cleanup:
-gdb_put_strbuf();
+gdb_put_strbuf();
+}
}
static void handle_v_kill(GArray *params, void *user_ctx)
@@ -1310,12 +1319,14 @@ static const GdbCmdParseEntry gdb_v_commands_table[] = {
.handler = handle_v_cont,
.cmd = "Cont",
.cmd_startswith = 1,
+.allow_stop_reply = true,
.schema = "s0"
},
{
.handler = handle_v_attach,
.cmd = "Attach;",
.cmd_startswith = 1,
+.allow_stop_reply = true,
.schema = "l0"
},
{
@@ -1698,10 +1709,13 @@ static void handle_gen_set(GArray *params, void
*user_ctx)
static void handle_target_halt(GArray *params, void *user_ctx)
{
-g_string_printf(gdbserver_state.str_buf, "T%02xthread:", GDB_SIGNAL_TRAP);
-gdb_append_thread_id(gdbserver_state.c_cpu, gdbserver_state.str_buf);
-g_string_append_c(gdbserver_state.str_buf, ';');
-gdb_put_strbuf();
+if (gdbserver_state.allow_stop_reply) {
+g_string_printf(gdbserver_state.str_buf, "T%02xthread:",
GDB_SIGNAL_TRAP);
+gdb_append_thread_id(gdbserver_state.c_cpu, gdbserver_state.str_buf);
+g_string_append_c(gdbserver_state.str_buf, ';');
+gdb_put_strbuf();
+gdbserver_state.allow_stop_reply = false;
+}
/*
* Remove all the breakpoints when this query is issued,
* because gdb is doing an initial connect and the state
@@ -1725,7 +1739,8 @@ static int gdb_handle_packet(cons
Re: [PATCH v2 RESEND 4/7] Hexagon: support qRegisterInfo at gdbstub
Philippe Mathieu-Daudé wrote: > > > Matheus Tavares wrote: > > > > diff --git a/target/hexagon/gdb_qreginfo.h b/target/hexagon/gdb_qreginfo.h > > new file mode 100644 > > index 00..64631ddd58 > > --- /dev/null > > +++ b/target/hexagon/gdb_qreginfo.h > > This should be gdb_qreginfo.h.inc Ok, thanks. > > @@ -358,6 +359,8 @@ static void hexagon_cpu_class_init(ObjectClass *c, void > > *data) > > cc->get_pc = hexagon_cpu_get_pc; > > cc->gdb_read_register = hexagon_gdb_read_register; > > cc->gdb_write_register = hexagon_gdb_write_register; > > +cc->gdb_qreg_info_lines = (const char **)hexagon_qreg_descs; > > No need to cast if fixing gdb_qreg_info_lines's prototype > (see previous patch review). Ah, good call, thanks. I'll try to avoid the whole qRegisterInfo implemenation in the next round, as Alex suggested [1]. But if not possible, I'll make sure to add this changes to the types and cast. Thanks! [1]: https://lore.kernel.org/qemu-devel/20230421113420.67122-1-quic_mathb...@quicinc.com/
Re: [PATCH v2 RESEND 3/7] gdbstub: add support for the qRegisterInfo query
Alex Bennée wrote:
>
> > Matheus Tavares wrote:
> >
> > diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> > index be18568d0a..f19f8c58c3 100644
> > --- a/gdbstub/gdbstub.c
> > +++ b/gdbstub/gdbstub.c
> > @@ -1578,6 +1599,12 @@ static const GdbCmdParseEntry gdb_gen_query_table[]
> > = {
> > .handler = handle_query_curr_tid,
> > .cmd = "C",
> > },
> > +{
> > +.handler = handle_query_regs,
> > +.cmd = "RegisterInfo",
> > +.cmd_startswith = 1,
> > +.schema = "l0"
> > +},
>
> Where is this defined in the protocol spec, I can't see it in:
>
>
> https://sourceware.org/gdb/onlinedocs/gdb/General-Query-Packets.html#General-Query-Packets
>
> and it seems to be information that is handled by the xml register
> description. Is there a reason that isn't used for Hexagon?
Good point. It's actually an lldb extension to the protocol:
https://github.com/llvm/llvm-project/blob/main/lldb/docs/lldb-gdb-remote.txt#L573
But indeed, lldb should be able to use the xml register description as
well. I'll take a look and try to do that instead.
Thanks!
Re: [PATCH v2 RESEND 1/7] gdbstub: only send stop-reply packets when allowed to
Alex Bennée wrote:
>
> > Matheus Tavares wrote:
> >
> > diff --git a/gdbstub/user.c b/gdbstub/user.c
> > index 80488b6bb9..bb03622c83 100644
> > --- a/gdbstub/user.c
> > +++ b/gdbstub/user.c
> > @@ -174,12 +177,14 @@ void gdb_signalled(CPUArchState *env, int sig)
> > {
> > char buf[4];
> >
> > -if (!gdbserver_state.init || gdbserver_user_state.fd < 0) {
> > +if (!gdbserver_state.init || gdbserver_user_state.fd < 0 ||
> > +!gdbserver_state.allow_stop_reply) {
> > return;
> > }
> >
> > snprintf(buf, sizeof(buf), "X%02x", gdb_target_signal_to_gdb(sig));
> > gdb_put_packet(buf);
> > +gdbserver_state.allow_stop_reply = false;
>
> Did I miss an equivalent for softmmu mode here?
Hmm, there doesn't seem to be other "X aa" stop-replies sent from
softmmu in our gdbstub. However, I just realize I did miss another spot
of "W aa" at gdbstub/user.c:gdb_exit(). I'll add the allow_stop_reply
guard there for the next iteration.
[PATCH v2 RESEND 5/7] Hexagon (gdbstub): fix p3:0 read and write via stub
From: Brian Cain
Co-authored-by: Sid Manning
Signed-off-by: Sid Manning
Signed-off-by: Brian Cain
Co-authored-by: Matheus Tavares Bernardino
Signed-off-by: Matheus Tavares Bernardino
Reviewed-by: Taylor Simpson
---
target/hexagon/gdbstub.c | 16
1 file changed, 16 insertions(+)
diff --git a/target/hexagon/gdbstub.c b/target/hexagon/gdbstub.c
index 46083da620..a06fed9f18 100644
--- a/target/hexagon/gdbstub.c
+++ b/target/hexagon/gdbstub.c
@@ -25,6 +25,14 @@ int hexagon_gdb_read_register(CPUState *cs, GByteArray
*mem_buf, int n)
HexagonCPU *cpu = HEXAGON_CPU(cs);
CPUHexagonState *env = &cpu->env;
+if (n == HEX_REG_P3_0_ALIASED) {
+uint32_t p3_0 = 0;
+for (int i = 0; i < NUM_PREGS; i++) {
+p3_0 = deposit32(p3_0, i * 8, 8, env->pred[i]);
+}
+return gdb_get_regl(mem_buf, p3_0);
+}
+
if (n < TOTAL_PER_THREAD_REGS) {
return gdb_get_regl(mem_buf, env->gpr[n]);
}
@@ -37,6 +45,14 @@ int hexagon_gdb_write_register(CPUState *cs, uint8_t
*mem_buf, int n)
HexagonCPU *cpu = HEXAGON_CPU(cs);
CPUHexagonState *env = &cpu->env;
+if (n == HEX_REG_P3_0_ALIASED) {
+uint32_t p3_0 = ldtul_p(mem_buf);
+for (int i = 0; i < NUM_PREGS; i++) {
+env->pred[i] = extract32(p3_0, i * 8, 8);
+}
+return sizeof(target_ulong);
+}
+
if (n < TOTAL_PER_THREAD_REGS) {
env->gpr[n] = ldtul_p(mem_buf);
return sizeof(target_ulong);
--
2.39.1
[PATCH v2 RESEND 2/7] gdbstub: add test for untimely stop-reply packets
In the previous commit, we modified gdbstub.c to only send stop-reply
packets as a response to GDB commands that accept it. Now, let's add a
test for this intended behavior. Running this test before the fix from
the previous commit fails as QEMU sends a stop-reply packet
asynchronously, when GDB was in fact waiting an ACK.
Signed-off-by: Matheus Tavares Bernardino
---
tests/guest-debug/run-test.py| 16
.../tcg/multiarch/system/Makefile.softmmu-target | 16 +++-
2 files changed, 27 insertions(+), 5 deletions(-)
diff --git a/tests/guest-debug/run-test.py b/tests/guest-debug/run-test.py
index d865e46ecd..de6106a5e5 100755
--- a/tests/guest-debug/run-test.py
+++ b/tests/guest-debug/run-test.py
@@ -26,11 +26,12 @@ def get_args():
parser.add_argument("--qargs", help="Qemu arguments for test")
parser.add_argument("--binary", help="Binary to debug",
required=True)
-parser.add_argument("--test", help="GDB test script",
-required=True)
+parser.add_argument("--test", help="GDB test script")
parser.add_argument("--gdb", help="The gdb binary to use",
default=None)
+parser.add_argument("--gdb-args", help="Additional gdb arguments")
parser.add_argument("--output", help="A file to redirect output to")
+parser.add_argument("--stderr", help="A file to redirect stderr to")
return parser.parse_args()
@@ -58,6 +59,10 @@ def log(output, msg):
output = open(args.output, "w")
else:
output = None
+if args.stderr:
+stderr = open(args.stderr, "w")
+else:
+stderr = None
socket_dir = TemporaryDirectory("qemu-gdbstub")
socket_name = os.path.join(socket_dir.name, "gdbstub.socket")
@@ -77,6 +82,8 @@ def log(output, msg):
# Now launch gdb with our test and collect the result
gdb_cmd = "%s %s" % (args.gdb, args.binary)
+if args.gdb_args:
+gdb_cmd += " %s" % (args.gdb_args)
# run quietly and ignore .gdbinit
gdb_cmd += " -q -n -batch"
# disable prompts in case of crash
@@ -84,13 +91,14 @@ def log(output, msg):
# connect to remote
gdb_cmd += " -ex 'target remote %s'" % (socket_name)
# finally the test script itself
-gdb_cmd += " -x %s" % (args.test)
+if args.test:
+gdb_cmd += " -x %s" % (args.test)
sleep(1)
log(output, "GDB CMD: %s" % (gdb_cmd))
-result = subprocess.call(gdb_cmd, shell=True, stdout=output)
+result = subprocess.call(gdb_cmd, shell=True, stdout=output, stderr=stderr)
# A result of greater than 128 indicates a fatal signal (likely a
# crash due to gdb internal failure). That's a problem for GDB and
diff --git a/tests/tcg/multiarch/system/Makefile.softmmu-target
b/tests/tcg/multiarch/system/Makefile.softmmu-target
index 5f432c95f3..fe40195d39 100644
--- a/tests/tcg/multiarch/system/Makefile.softmmu-target
+++ b/tests/tcg/multiarch/system/Makefile.softmmu-target
@@ -27,6 +27,20 @@ run-gdbstub-memory: memory
"-monitor none -display none -chardev
file$(COMMA)path=$<.out$(COMMA)id=output $(QEMU_OPTS)" \
--bin $< --test $(MULTIARCH_SRC)/gdbstub/memory.py, \
softmmu gdbstub support)
+
+run-gdbstub-untimely-packet: hello
+ $(call run-test, $@, $(GDB_SCRIPT) \
+ --gdb $(HAVE_GDB_BIN) \
+ --gdb-args "-ex 'set debug remote 1'" \
+ --output untimely-packet.gdb.out \
+ --stderr untimely-packet.gdb.err \
+ --qemu $(QEMU) \
+ --bin $< --qargs \
+ "-monitor none -display none -chardev
file$(COMMA)path=untimely-packet.out$(COMMA)id=output $(QEMU_OPTS)", \
+ "softmmu gdbstub untimely packets")
+ $(call quiet-command, \
+ (! grep -Fq 'Packet instead of Ack, ignoring it'
untimely-packet.gdb.err), \
+ "GREP", "file untimely-packet.gdb.err")
else
run-gdbstub-%:
$(call skip-test, "gdbstub test $*", "no guest arch support")
@@ -36,4 +50,4 @@ run-gdbstub-%:
$(call skip-test, "gdbstub test $*", "need working gdb")
endif
-MULTIARCH_RUNS += run-gdbstub-memory
+MULTIARCH_RUNS += run-gdbstub-memory run-gdbstub-untimely-packet
--
2.39.1
[PATCH v2 RESEND 6/7] Hexagon (gdbstub): add HVX support
From: Taylor Simpson
Co-authored-by: Brian Cain
Signed-off-by: Brian Cain
Signed-off-by: Taylor Simpson
Co-authored-by: Matheus Tavares Bernardino
Signed-off-by: Matheus Tavares Bernardino
---
target/hexagon/gdbstub.c | 60
1 file changed, 60 insertions(+)
diff --git a/target/hexagon/gdbstub.c b/target/hexagon/gdbstub.c
index a06fed9f18..6c5a15da68 100644
--- a/target/hexagon/gdbstub.c
+++ b/target/hexagon/gdbstub.c
@@ -20,6 +20,26 @@
#include "cpu.h"
#include "internal.h"
+static int gdb_get_vreg(CPUHexagonState *env, GByteArray *mem_buf, int n)
+{
+int total = 0;
+int i;
+for (i = 0; i < ARRAY_SIZE(env->VRegs[n].uw); i++) {
+total += gdb_get_regl(mem_buf, env->VRegs[n].uw[i]);
+}
+return total;
+}
+
+static int gdb_get_qreg(CPUHexagonState *env, GByteArray *mem_buf, int n)
+{
+int total = 0;
+int i;
+for (i = 0; i < ARRAY_SIZE(env->QRegs[n].uw); i++) {
+total += gdb_get_regl(mem_buf, env->QRegs[n].uw[i]);
+}
+return total;
+}
+
int hexagon_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
{
HexagonCPU *cpu = HEXAGON_CPU(cs);
@@ -36,10 +56,40 @@ int hexagon_gdb_read_register(CPUState *cs, GByteArray
*mem_buf, int n)
if (n < TOTAL_PER_THREAD_REGS) {
return gdb_get_regl(mem_buf, env->gpr[n]);
}
+n -= TOTAL_PER_THREAD_REGS;
+
+if (n < NUM_VREGS) {
+return gdb_get_vreg(env, mem_buf, n);
+}
+n -= NUM_VREGS;
+
+if (n < NUM_QREGS) {
+return gdb_get_qreg(env, mem_buf, n);
+}
g_assert_not_reached();
}
+static int gdb_put_vreg(CPUHexagonState *env, uint8_t *mem_buf, int n)
+{
+int i;
+for (i = 0; i < ARRAY_SIZE(env->VRegs[n].uw); i++) {
+env->VRegs[n].uw[i] = ldtul_p(mem_buf);
+mem_buf += 4;
+}
+return MAX_VEC_SIZE_BYTES;
+}
+
+static int gdb_put_qreg(CPUHexagonState *env, uint8_t *mem_buf, int n)
+{
+int i;
+for (i = 0; i < ARRAY_SIZE(env->QRegs[n].uw); i++) {
+env->QRegs[n].uw[i] = ldtul_p(mem_buf);
+mem_buf += 4;
+}
+return MAX_VEC_SIZE_BYTES / 8;
+}
+
int hexagon_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
{
HexagonCPU *cpu = HEXAGON_CPU(cs);
@@ -57,6 +107,16 @@ int hexagon_gdb_write_register(CPUState *cs, uint8_t
*mem_buf, int n)
env->gpr[n] = ldtul_p(mem_buf);
return sizeof(target_ulong);
}
+n -= TOTAL_PER_THREAD_REGS;
+
+if (n < NUM_VREGS) {
+return gdb_put_vreg(env, mem_buf, n);
+}
+n -= NUM_VREGS;
+
+if (n < NUM_QREGS) {
+return gdb_put_qreg(env, mem_buf, n);
+}
g_assert_not_reached();
}
--
2.39.1
[PATCH v2 RESEND 4/7] Hexagon: support qRegisterInfo at gdbstub
From: Brian Cain
Signed-off-by: Brian Cain
Signed-off-by: Matheus Tavares Bernardino
---
target/hexagon/gdb_qreginfo.h | 124 ++
target/hexagon/cpu.c | 3 +
2 files changed, 127 insertions(+)
create mode 100644 target/hexagon/gdb_qreginfo.h
diff --git a/target/hexagon/gdb_qreginfo.h b/target/hexagon/gdb_qreginfo.h
new file mode 100644
index 00..64631ddd58
--- /dev/null
+++ b/target/hexagon/gdb_qreginfo.h
@@ -0,0 +1,124 @@
+/*
+ * Copyright(c) 2023 Qualcomm Innovation Center, Inc. All Rights Reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef HEXAGON_GDB_QREGINFO_H
+#define HEXAGON_GDB_QREGINFO_H
+
+const char * const hexagon_qreg_descs[] = {
+
"name:r00;alt-name:r0;bitsize:32;offset=0;variable-size:0;encoding:uint;format:hex;set:Thread
Registers;dwarf:0;generic:r00;",
+
"name:r01;alt-name:r1;bitsize:32;offset=4;variable-size:0;encoding:uint;format:hex;set:Thread
Registers;dwarf:1;generic:r01;",
+
"name:r02;alt-name:r2;bitsize:32;offset=8;variable-size:0;encoding:uint;format:hex;set:Thread
Registers;dwarf:2;generic:r02;",
+
"name:r03;alt-name:r3;bitsize:32;offset=12;variable-size:0;encoding:uint;format:hex;set:Thread
Registers;dwarf:3;generic:r03;",
+
"name:r04;alt-name:r4;bitsize:32;offset=16;variable-size:0;encoding:uint;format:hex;set:Thread
Registers;dwarf:4;generic:r04;",
+
"name:r05;alt-name:r5;bitsize:32;offset=20;variable-size:0;encoding:uint;format:hex;set:Thread
Registers;dwarf:5;generic:r05;",
+
"name:r06;alt-name:r6;bitsize:32;offset=24;variable-size:0;encoding:uint;format:hex;set:Thread
Registers;dwarf:6;generic:r06;",
+
"name:r07;alt-name:r7;bitsize:32;offset=28;variable-size:0;encoding:uint;format:hex;set:Thread
Registers;dwarf:7;generic:r07;",
+
"name:r08;alt-name:r8;bitsize:32;offset=32;variable-size:0;encoding:uint;format:hex;set:Thread
Registers;dwarf:8;generic:r08;",
+
"name:r09;alt-name:r9;bitsize:32;offset=36;variable-size:0;encoding:uint;format:hex;set:Thread
Registers;dwarf:9;generic:r09;",
+
"name:r10;alt-name:;bitsize:32;offset=40;variable-size:0;encoding:uint;format:hex;set:Thread
Registers;dwarf:10;generic:;",
+
"name:r11;alt-name:;bitsize:32;offset=44;variable-size:0;encoding:uint;format:hex;set:Thread
Registers;dwarf:11;generic:;",
+
"name:r12;alt-name:;bitsize:32;offset=48;variable-size:0;encoding:uint;format:hex;set:Thread
Registers;dwarf:12;generic:;",
+
"name:r13;alt-name:;bitsize:32;offset=52;variable-size:0;encoding:uint;format:hex;set:Thread
Registers;dwarf:13;generic:;",
+
"name:r14;alt-name:;bitsize:32;offset=56;variable-size:0;encoding:uint;format:hex;set:Thread
Registers;dwarf:14;generic:;",
+
"name:r15;alt-name:;bitsize:32;offset=60;variable-size:0;encoding:uint;format:hex;set:Thread
Registers;dwarf:15;generic:;",
+
"name:r16;alt-name:;bitsize:32;offset=64;variable-size:0;encoding:uint;format:hex;set:Thread
Registers;dwarf:16;generic:;",
+
"name:r17;alt-name:;bitsize:32;offset=68;variable-size:0;encoding:uint;format:hex;set:Thread
Registers;dwarf:17;generic:;",
+
"name:r18;alt-name:;bitsize:32;offset=72;variable-size:0;encoding:uint;format:hex;set:Thread
Registers;dwarf:18;generic:;",
+
"name:r19;alt-name:;bitsize:32;offset=76;variable-size:0;encoding:uint;format:hex;set:Thread
Registers;dwarf:19;generic:;",
+
"name:r20;alt-name:;bitsize:32;offset=80;variable-size:0;encoding:uint;format:hex;set:Thread
Registers;dwarf:20;generic:;",
+
"name:r21;alt-name:;bitsize:32;offset=84;variable-size:0;encoding:uint;format:hex;set:Thread
Registers;dwarf:21;generic:;",
+
"name:r22;alt-name:;bitsize:32;offset=88;variable-size:0;encoding:uint;format:hex;set:Thread
Registers;dwarf:22;generic:;",
+
"name:r23;alt-name:;bitsize:32;offset=92;variable-size:0;encoding:uint;format:hex;set:Thread
Registers;dwarf:23;generic:;",
+
"name:r24;alt-name:;bitsize:32;offset=96;variable-size:0;encoding:uint;format:hex;set:Thread
Registers;dwarf:24;generic:;",
+
"name:r25;alt-name:;bitsize:32;offset=100;variable-size:0;en
[PATCH v2 RESEND 3/7] gdbstub: add support for the qRegisterInfo query
From: Brian Cain
Signed-off-by: Brian Cain
Signed-off-by: Matheus Tavares Bernardino
---
include/hw/core/cpu.h | 4
gdbstub/gdbstub.c | 27 +++
2 files changed, 31 insertions(+)
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 397fd3ac68..cfdf5514d9 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -124,6 +124,8 @@ struct SysemuCPUOps;
* its Harvard architecture split code and data.
* @gdb_num_core_regs: Number of core registers accessible to GDB.
* @gdb_core_xml_file: File name for core registers GDB XML description.
+ * @gdb_qreg_info_lines: Array of lines of registers qRegisterInfo description.
+ * @gdb_qreg_info_line_count: Count of lines for @gdb_qreg_info_lines.
* @gdb_stop_before_watchpoint: Indicates whether GDB expects the CPU to stop
* before the insn which triggers a watchpoint rather than after it.
* @gdb_arch_name: Optional callback that returns the architecture name known
@@ -159,6 +161,8 @@ struct CPUClass {
vaddr (*gdb_adjust_breakpoint)(CPUState *cpu, vaddr addr);
const char *gdb_core_xml_file;
+const char **gdb_qreg_info_lines;
+int gdb_qreg_info_line_count;
gchar * (*gdb_arch_name)(CPUState *cpu);
const char * (*gdb_get_dynamic_xml)(CPUState *cpu, const char *xmlname);
diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index be18568d0a..f19f8c58c3 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -1409,6 +1409,27 @@ static void handle_query_curr_tid(GArray *params, void
*user_ctx)
gdb_put_strbuf();
}
+static void handle_query_regs(GArray *params, void *user_ctx)
+{
+if (!params->len) {
+return;
+}
+
+CPUClass *cc = CPU_GET_CLASS(gdbserver_state.g_cpu);
+if (!cc->gdb_qreg_info_lines) {
+gdb_put_packet("");
+return;
+}
+
+int reg_num = get_param(params, 0)->val_ul;
+if (reg_num >= cc->gdb_qreg_info_line_count) {
+gdb_put_packet("");
+return;
+}
+
+gdb_put_packet(cc->gdb_qreg_info_lines[reg_num]);
+}
+
static void handle_query_threads(GArray *params, void *user_ctx)
{
if (!gdbserver_state.query_cpu) {
@@ -1578,6 +1599,12 @@ static const GdbCmdParseEntry gdb_gen_query_table[] = {
.handler = handle_query_curr_tid,
.cmd = "C",
},
+{
+.handler = handle_query_regs,
+.cmd = "RegisterInfo",
+.cmd_startswith = 1,
+.schema = "l0"
+},
{
.handler = handle_query_threads,
.cmd = "sThreadInfo",
--
2.39.1
[PATCH v2 RESEND 1/7] gdbstub: only send stop-reply packets when allowed to
GDB's remote serial protocol allows stop-reply messages to be sent by
the stub either as a notification packet or as a reply to a GDB command
(provided that the cmd accepts such a response). QEMU currently does not
implement notification packets, so it should only send stop-replies
synchronously and when requested. Nevertheless, it still issues
unsolicited stop messages through gdb_vm_state_change().
Although this behavior doesn't seem to cause problems with GDB itself
(the messages are just ignored), it can impact other debuggers that
implement the GDB remote serial protocol, like hexagon-lldb. Let's
change the gdbstub to send stop messages only as a response to a
previous GDB command that accepts such a reply.
Signed-off-by: Matheus Tavares Bernardino
---
gdbstub/internals.h | 5 +
gdbstub/gdbstub.c | 37 -
gdbstub/softmmu.c | 13 +++--
gdbstub/user.c | 17 +++--
4 files changed, 55 insertions(+), 17 deletions(-)
diff --git a/gdbstub/internals.h b/gdbstub/internals.h
index 94ddff4495..33d21d6488 100644
--- a/gdbstub/internals.h
+++ b/gdbstub/internals.h
@@ -65,6 +65,11 @@ typedef struct GDBState {
GByteArray *mem_buf;
int sstep_flags;
int supported_sstep_flags;
+/*
+ * Whether we are allowed to send a stop reply packet at this moment.
+ * Must be set off after sending the stop reply itself.
+ */
+bool allow_stop_reply;
} GDBState;
/* lives in main gdbstub.c */
diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index 0760d78685..be18568d0a 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -777,6 +777,10 @@ typedef void (*GdbCmdHandler)(GArray *params, void
*user_ctx);
/*
* cmd_startswith -> cmd is compared using startswith
*
+ * allow_stop_reply -> true iff the gdbstub can respond to this command with a
+ * "stop reply" packet. The list of commands that accept such response is
+ * defined at the GDB Remote Serial Protocol documentation. see:
+ *
https://sourceware.org/gdb/onlinedocs/gdb/Stop-Reply-Packets.html#Stop-Reply-Packets.
*
* schema definitions:
* Each schema parameter entry consists of 2 chars,
@@ -802,6 +806,7 @@ typedef struct GdbCmdParseEntry {
const char *cmd;
bool cmd_startswith;
const char *schema;
+bool allow_stop_reply;
} GdbCmdParseEntry;
static inline int startswith(const char *string, const char *pattern)
@@ -835,6 +840,7 @@ static int process_string_cmd(void *user_ctx, const char
*data,
}
}
+gdbserver_state.allow_stop_reply = cmd->allow_stop_reply;
cmd->handler(params, user_ctx);
return 0;
}
@@ -1283,11 +1289,14 @@ static void handle_v_attach(GArray *params, void
*user_ctx)
gdbserver_state.g_cpu = cpu;
gdbserver_state.c_cpu = cpu;
-g_string_printf(gdbserver_state.str_buf, "T%02xthread:", GDB_SIGNAL_TRAP);
-gdb_append_thread_id(cpu, gdbserver_state.str_buf);
-g_string_append_c(gdbserver_state.str_buf, ';');
+if (gdbserver_state.allow_stop_reply) {
+g_string_printf(gdbserver_state.str_buf, "T%02xthread:",
GDB_SIGNAL_TRAP);
+gdb_append_thread_id(cpu, gdbserver_state.str_buf);
+g_string_append_c(gdbserver_state.str_buf, ';');
+gdbserver_state.allow_stop_reply = false;
cleanup:
-gdb_put_strbuf();
+gdb_put_strbuf();
+}
}
static void handle_v_kill(GArray *params, void *user_ctx)
@@ -1310,12 +1319,14 @@ static const GdbCmdParseEntry gdb_v_commands_table[] = {
.handler = handle_v_cont,
.cmd = "Cont",
.cmd_startswith = 1,
+.allow_stop_reply = true,
.schema = "s0"
},
{
.handler = handle_v_attach,
.cmd = "Attach;",
.cmd_startswith = 1,
+.allow_stop_reply = true,
.schema = "l0"
},
{
@@ -1698,10 +1709,13 @@ static void handle_gen_set(GArray *params, void
*user_ctx)
static void handle_target_halt(GArray *params, void *user_ctx)
{
-g_string_printf(gdbserver_state.str_buf, "T%02xthread:", GDB_SIGNAL_TRAP);
-gdb_append_thread_id(gdbserver_state.c_cpu, gdbserver_state.str_buf);
-g_string_append_c(gdbserver_state.str_buf, ';');
-gdb_put_strbuf();
+if (gdbserver_state.allow_stop_reply) {
+g_string_printf(gdbserver_state.str_buf, "T%02xthread:",
GDB_SIGNAL_TRAP);
+gdb_append_thread_id(gdbserver_state.c_cpu, gdbserver_state.str_buf);
+g_string_append_c(gdbserver_state.str_buf, ';');
+gdb_put_strbuf();
+gdbserver_state.allow_stop_reply = false;
+}
/*
* Remove all the breakpoints when this query is issued,
* because gdb is doing an initial connect and the state
@@ -1725,7 +1739,8 @@ static int gdb_handle_packet(const char *line_buf)
static const
[PATCH v2 RESEND 7/7] Hexagon (linux-user/hexagon): handle breakpoints
From: Taylor Simpson
This enables LLDB to work with hexagon linux-user mode through the GDB
remote protocol.
Signed-off-by: Taylor Simpson
Signed-off-by: Matheus Tavares Bernardino
---
linux-user/hexagon/cpu_loop.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/linux-user/hexagon/cpu_loop.c b/linux-user/hexagon/cpu_loop.c
index b84e25bf71..00c12bbb55 100644
--- a/linux-user/hexagon/cpu_loop.c
+++ b/linux-user/hexagon/cpu_loop.c
@@ -33,6 +33,7 @@ void cpu_loop(CPUHexagonState *env)
target_ulong ret;
for (;;) {
+target_siginfo_t info;
cpu_exec_start(cs);
trapnr = cpu_exec(cs);
cpu_exec_end(cs);
@@ -63,6 +64,15 @@ void cpu_loop(CPUHexagonState *env)
case EXCP_ATOMIC:
cpu_exec_step_atomic(cs);
break;
+case EXCP_DEBUG:
+info = (target_siginfo_t) {
+.si_signo = TARGET_SIGTRAP,
+.si_errno = 0,
+.si_code = TARGET_TRAP_BRKPT,
+._sifields._sigfault._addr = 0
+};
+queue_signal(env, info.si_signo, QEMU_SI_KILL, &info);
+break;
default:
EXCP_DUMP(env, "\nqemu: unhandled CPU exception %#x - aborting\n",
trapnr);
--
2.39.1
[PATCH v2 RESEND 0/7] Hexagon: add lldb support
This series allows hexagon programs to be debugged under qemu user-mode through LLDB and qemu's gdbstub. LLDB implements the GDB remote serial protocol, so most of the necessary changes are in the Hexagon part itself. However, one fix is needed at the arch-independent side too. Changes in v2: - Rebased on current master - Added Taylor's Reviewed-by at patch 5 v1: https://lore.kernel.org/qemu-devel/cover.1680808943.git.quic_mathb...@quicinc.com/ Brian Cain (3): gdbstub: add support for the qRegisterInfo query Hexagon: support qRegisterInfo at gdbstub Hexagon (gdbstub): fix p3:0 read and write via stub Matheus Tavares Bernardino (2): gdbstub: only send stop-reply packets when allowed to gdbstub: add test for untimely stop-reply packets Taylor Simpson (2): Hexagon (gdbstub): add HVX support Hexagon (linux-user/hexagon): handle breakpoints gdbstub/internals.h | 5 + include/hw/core/cpu.h | 4 + target/hexagon/gdb_qreginfo.h | 124 ++ gdbstub/gdbstub.c | 64 +++-- gdbstub/softmmu.c | 13 +- gdbstub/user.c| 17 ++- linux-user/hexagon/cpu_loop.c | 10 ++ target/hexagon/cpu.c | 3 + target/hexagon/gdbstub.c | 76 +++ tests/guest-debug/run-test.py | 16 ++- .../multiarch/system/Makefile.softmmu-target | 16 ++- 11 files changed, 326 insertions(+), 22 deletions(-) create mode 100644 target/hexagon/gdb_qreginfo.h -- 2.39.1
[PATCH 7/7] Hexagon (linux-user/hexagon): handle breakpoints
From: Taylor Simpson
This enables LLDB to work with hexagon linux-user mode through the GDB
remote protocol.
Signed-off-by: Taylor Simpson
Signed-off-by: Matheus Tavares Bernardino
---
linux-user/hexagon/cpu_loop.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/linux-user/hexagon/cpu_loop.c b/linux-user/hexagon/cpu_loop.c
index b84e25bf71..00c12bbb55 100644
--- a/linux-user/hexagon/cpu_loop.c
+++ b/linux-user/hexagon/cpu_loop.c
@@ -33,6 +33,7 @@ void cpu_loop(CPUHexagonState *env)
target_ulong ret;
for (;;) {
+target_siginfo_t info;
cpu_exec_start(cs);
trapnr = cpu_exec(cs);
cpu_exec_end(cs);
@@ -63,6 +64,15 @@ void cpu_loop(CPUHexagonState *env)
case EXCP_ATOMIC:
cpu_exec_step_atomic(cs);
break;
+case EXCP_DEBUG:
+info = (target_siginfo_t) {
+.si_signo = TARGET_SIGTRAP,
+.si_errno = 0,
+.si_code = TARGET_TRAP_BRKPT,
+._sifields._sigfault._addr = 0
+};
+queue_signal(env, info.si_signo, QEMU_SI_KILL, &info);
+break;
default:
EXCP_DUMP(env, "\nqemu: unhandled CPU exception %#x - aborting\n",
trapnr);
--
2.39.1
[PATCH 4/7] Hexagon: support qRegisterInfo at gdbstub
From: Brian Cain
Signed-off-by: Brian Cain
Signed-off-by: Matheus Tavares Bernardino
---
target/hexagon/gdb_qreginfo.h | 124 ++
target/hexagon/cpu.c | 3 +
2 files changed, 127 insertions(+)
create mode 100644 target/hexagon/gdb_qreginfo.h
diff --git a/target/hexagon/gdb_qreginfo.h b/target/hexagon/gdb_qreginfo.h
new file mode 100644
index 00..64631ddd58
--- /dev/null
+++ b/target/hexagon/gdb_qreginfo.h
@@ -0,0 +1,124 @@
+/*
+ * Copyright(c) 2023 Qualcomm Innovation Center, Inc. All Rights Reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef HEXAGON_GDB_QREGINFO_H
+#define HEXAGON_GDB_QREGINFO_H
+
+const char * const hexagon_qreg_descs[] = {
+
"name:r00;alt-name:r0;bitsize:32;offset=0;variable-size:0;encoding:uint;format:hex;set:Thread
Registers;dwarf:0;generic:r00;",
+
"name:r01;alt-name:r1;bitsize:32;offset=4;variable-size:0;encoding:uint;format:hex;set:Thread
Registers;dwarf:1;generic:r01;",
+
"name:r02;alt-name:r2;bitsize:32;offset=8;variable-size:0;encoding:uint;format:hex;set:Thread
Registers;dwarf:2;generic:r02;",
+
"name:r03;alt-name:r3;bitsize:32;offset=12;variable-size:0;encoding:uint;format:hex;set:Thread
Registers;dwarf:3;generic:r03;",
+
"name:r04;alt-name:r4;bitsize:32;offset=16;variable-size:0;encoding:uint;format:hex;set:Thread
Registers;dwarf:4;generic:r04;",
+
"name:r05;alt-name:r5;bitsize:32;offset=20;variable-size:0;encoding:uint;format:hex;set:Thread
Registers;dwarf:5;generic:r05;",
+
"name:r06;alt-name:r6;bitsize:32;offset=24;variable-size:0;encoding:uint;format:hex;set:Thread
Registers;dwarf:6;generic:r06;",
+
"name:r07;alt-name:r7;bitsize:32;offset=28;variable-size:0;encoding:uint;format:hex;set:Thread
Registers;dwarf:7;generic:r07;",
+
"name:r08;alt-name:r8;bitsize:32;offset=32;variable-size:0;encoding:uint;format:hex;set:Thread
Registers;dwarf:8;generic:r08;",
+
"name:r09;alt-name:r9;bitsize:32;offset=36;variable-size:0;encoding:uint;format:hex;set:Thread
Registers;dwarf:9;generic:r09;",
+
"name:r10;alt-name:;bitsize:32;offset=40;variable-size:0;encoding:uint;format:hex;set:Thread
Registers;dwarf:10;generic:;",
+
"name:r11;alt-name:;bitsize:32;offset=44;variable-size:0;encoding:uint;format:hex;set:Thread
Registers;dwarf:11;generic:;",
+
"name:r12;alt-name:;bitsize:32;offset=48;variable-size:0;encoding:uint;format:hex;set:Thread
Registers;dwarf:12;generic:;",
+
"name:r13;alt-name:;bitsize:32;offset=52;variable-size:0;encoding:uint;format:hex;set:Thread
Registers;dwarf:13;generic:;",
+
"name:r14;alt-name:;bitsize:32;offset=56;variable-size:0;encoding:uint;format:hex;set:Thread
Registers;dwarf:14;generic:;",
+
"name:r15;alt-name:;bitsize:32;offset=60;variable-size:0;encoding:uint;format:hex;set:Thread
Registers;dwarf:15;generic:;",
+
"name:r16;alt-name:;bitsize:32;offset=64;variable-size:0;encoding:uint;format:hex;set:Thread
Registers;dwarf:16;generic:;",
+
"name:r17;alt-name:;bitsize:32;offset=68;variable-size:0;encoding:uint;format:hex;set:Thread
Registers;dwarf:17;generic:;",
+
"name:r18;alt-name:;bitsize:32;offset=72;variable-size:0;encoding:uint;format:hex;set:Thread
Registers;dwarf:18;generic:;",
+
"name:r19;alt-name:;bitsize:32;offset=76;variable-size:0;encoding:uint;format:hex;set:Thread
Registers;dwarf:19;generic:;",
+
"name:r20;alt-name:;bitsize:32;offset=80;variable-size:0;encoding:uint;format:hex;set:Thread
Registers;dwarf:20;generic:;",
+
"name:r21;alt-name:;bitsize:32;offset=84;variable-size:0;encoding:uint;format:hex;set:Thread
Registers;dwarf:21;generic:;",
+
"name:r22;alt-name:;bitsize:32;offset=88;variable-size:0;encoding:uint;format:hex;set:Thread
Registers;dwarf:22;generic:;",
+
"name:r23;alt-name:;bitsize:32;offset=92;variable-size:0;encoding:uint;format:hex;set:Thread
Registers;dwarf:23;generic:;",
+
"name:r24;alt-name:;bitsize:32;offset=96;variable-size:0;encoding:uint;format:hex;set:Thread
Registers;dwarf:24;generic:;",
+
"name:r25;alt-name:;bitsize:32;offset=100;variable-size:0;en
[PATCH 6/7] Hexagon (gdbstub): add HVX support
From: Taylor Simpson
Co-authored-by: Brian Cain
Signed-off-by: Brian Cain
Signed-off-by: Taylor Simpson
Co-authored-by: Matheus Tavares Bernardino
Signed-off-by: Matheus Tavares Bernardino
---
target/hexagon/gdbstub.c | 60
1 file changed, 60 insertions(+)
diff --git a/target/hexagon/gdbstub.c b/target/hexagon/gdbstub.c
index a06fed9f18..6c5a15da68 100644
--- a/target/hexagon/gdbstub.c
+++ b/target/hexagon/gdbstub.c
@@ -20,6 +20,26 @@
#include "cpu.h"
#include "internal.h"
+static int gdb_get_vreg(CPUHexagonState *env, GByteArray *mem_buf, int n)
+{
+int total = 0;
+int i;
+for (i = 0; i < ARRAY_SIZE(env->VRegs[n].uw); i++) {
+total += gdb_get_regl(mem_buf, env->VRegs[n].uw[i]);
+}
+return total;
+}
+
+static int gdb_get_qreg(CPUHexagonState *env, GByteArray *mem_buf, int n)
+{
+int total = 0;
+int i;
+for (i = 0; i < ARRAY_SIZE(env->QRegs[n].uw); i++) {
+total += gdb_get_regl(mem_buf, env->QRegs[n].uw[i]);
+}
+return total;
+}
+
int hexagon_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
{
HexagonCPU *cpu = HEXAGON_CPU(cs);
@@ -36,10 +56,40 @@ int hexagon_gdb_read_register(CPUState *cs, GByteArray
*mem_buf, int n)
if (n < TOTAL_PER_THREAD_REGS) {
return gdb_get_regl(mem_buf, env->gpr[n]);
}
+n -= TOTAL_PER_THREAD_REGS;
+
+if (n < NUM_VREGS) {
+return gdb_get_vreg(env, mem_buf, n);
+}
+n -= NUM_VREGS;
+
+if (n < NUM_QREGS) {
+return gdb_get_qreg(env, mem_buf, n);
+}
g_assert_not_reached();
}
+static int gdb_put_vreg(CPUHexagonState *env, uint8_t *mem_buf, int n)
+{
+int i;
+for (i = 0; i < ARRAY_SIZE(env->VRegs[n].uw); i++) {
+env->VRegs[n].uw[i] = ldtul_p(mem_buf);
+mem_buf += 4;
+}
+return MAX_VEC_SIZE_BYTES;
+}
+
+static int gdb_put_qreg(CPUHexagonState *env, uint8_t *mem_buf, int n)
+{
+int i;
+for (i = 0; i < ARRAY_SIZE(env->QRegs[n].uw); i++) {
+env->QRegs[n].uw[i] = ldtul_p(mem_buf);
+mem_buf += 4;
+}
+return MAX_VEC_SIZE_BYTES / 8;
+}
+
int hexagon_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
{
HexagonCPU *cpu = HEXAGON_CPU(cs);
@@ -57,6 +107,16 @@ int hexagon_gdb_write_register(CPUState *cs, uint8_t
*mem_buf, int n)
env->gpr[n] = ldtul_p(mem_buf);
return sizeof(target_ulong);
}
+n -= TOTAL_PER_THREAD_REGS;
+
+if (n < NUM_VREGS) {
+return gdb_put_vreg(env, mem_buf, n);
+}
+n -= NUM_VREGS;
+
+if (n < NUM_QREGS) {
+return gdb_put_qreg(env, mem_buf, n);
+}
g_assert_not_reached();
}
--
2.39.1
[PATCH 1/7] gdbstub: only send stop-reply packets when allowed to
GDB's remote serial protocol allows stop-reply messages to be sent by
the stub either as a notification packet or as a reply to a GDB command
(provided that the cmd accepts such a response). QEMU currently does not
implement notification packets, so it should only send stop-replies
synchronously and when requested. Nevertheless, it still issues
unsolicited stop messages through gdb_vm_state_change().
Although this behavior doesn't seem to cause problems with GDB itself
(the messages are just ignored), it can impact other debuggers that
implement the GDB remote serial protocol, like hexagon-lldb. Let's
change the gdbstub to send stop messages only as a response to a
previous GDB command that accepts such a reply.
Signed-off-by: Matheus Tavares Bernardino
---
gdbstub/internals.h | 5 +
gdbstub/gdbstub.c | 37 -
gdbstub/softmmu.c | 13 +++--
gdbstub/user.c | 17 +++--
4 files changed, 55 insertions(+), 17 deletions(-)
diff --git a/gdbstub/internals.h b/gdbstub/internals.h
index 94ddff4495..33d21d6488 100644
--- a/gdbstub/internals.h
+++ b/gdbstub/internals.h
@@ -65,6 +65,11 @@ typedef struct GDBState {
GByteArray *mem_buf;
int sstep_flags;
int supported_sstep_flags;
+/*
+ * Whether we are allowed to send a stop reply packet at this moment.
+ * Must be set off after sending the stop reply itself.
+ */
+bool allow_stop_reply;
} GDBState;
/* lives in main gdbstub.c */
diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index 2a66371aa5..9d326a1d2d 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -777,6 +777,10 @@ typedef void (*GdbCmdHandler)(GArray *params, void
*user_ctx);
/*
* cmd_startswith -> cmd is compared using startswith
*
+ * allow_stop_reply -> true iff the gdbstub can respond to this command with a
+ * "stop reply" packet. The list of commands that accept such response is
+ * defined at the GDB Remote Serial Protocol documentation. see:
+ *
https://sourceware.org/gdb/onlinedocs/gdb/Stop-Reply-Packets.html#Stop-Reply-Packets.
*
* schema definitions:
* Each schema parameter entry consists of 2 chars,
@@ -802,6 +806,7 @@ typedef struct GdbCmdParseEntry {
const char *cmd;
bool cmd_startswith;
const char *schema;
+bool allow_stop_reply;
} GdbCmdParseEntry;
static inline int startswith(const char *string, const char *pattern)
@@ -835,6 +840,7 @@ static int process_string_cmd(void *user_ctx, const char
*data,
}
}
+gdbserver_state.allow_stop_reply = cmd->allow_stop_reply;
cmd->handler(params, user_ctx);
return 0;
}
@@ -1283,11 +1289,14 @@ static void handle_v_attach(GArray *params, void
*user_ctx)
gdbserver_state.g_cpu = cpu;
gdbserver_state.c_cpu = cpu;
-g_string_printf(gdbserver_state.str_buf, "T%02xthread:", GDB_SIGNAL_TRAP);
-gdb_append_thread_id(cpu, gdbserver_state.str_buf);
-g_string_append_c(gdbserver_state.str_buf, ';');
+if (gdbserver_state.allow_stop_reply) {
+g_string_printf(gdbserver_state.str_buf, "T%02xthread:",
GDB_SIGNAL_TRAP);
+gdb_append_thread_id(cpu, gdbserver_state.str_buf);
+g_string_append_c(gdbserver_state.str_buf, ';');
+gdbserver_state.allow_stop_reply = false;
cleanup:
-gdb_put_strbuf();
+gdb_put_strbuf();
+}
}
static void handle_v_kill(GArray *params, void *user_ctx)
@@ -1310,12 +1319,14 @@ static const GdbCmdParseEntry gdb_v_commands_table[] = {
.handler = handle_v_cont,
.cmd = "Cont",
.cmd_startswith = 1,
+.allow_stop_reply = true,
.schema = "s0"
},
{
.handler = handle_v_attach,
.cmd = "Attach;",
.cmd_startswith = 1,
+.allow_stop_reply = true,
.schema = "l0"
},
{
@@ -1698,10 +1709,13 @@ static void handle_gen_set(GArray *params, void
*user_ctx)
static void handle_target_halt(GArray *params, void *user_ctx)
{
-g_string_printf(gdbserver_state.str_buf, "T%02xthread:", GDB_SIGNAL_TRAP);
-gdb_append_thread_id(gdbserver_state.c_cpu, gdbserver_state.str_buf);
-g_string_append_c(gdbserver_state.str_buf, ';');
-gdb_put_strbuf();
+if (gdbserver_state.allow_stop_reply) {
+g_string_printf(gdbserver_state.str_buf, "T%02xthread:",
GDB_SIGNAL_TRAP);
+gdb_append_thread_id(gdbserver_state.c_cpu, gdbserver_state.str_buf);
+g_string_append_c(gdbserver_state.str_buf, ';');
+gdb_put_strbuf();
+gdbserver_state.allow_stop_reply = false;
+}
/*
* Remove all the breakpoints when this query is issued,
* because gdb is doing an initial connect and the state
@@ -1725,7 +1739,8 @@ static int gdb_handle_packet(const char *line_buf)
static const
[PATCH 5/7] Hexagon (gdbstub): fix p3:0 read and write via stub
From: Brian Cain
Co-authored-by: Sid Manning
Signed-off-by: Sid Manning
Signed-off-by: Brian Cain
Co-authored-by: Matheus Tavares Bernardino
Signed-off-by: Matheus Tavares Bernardino
---
target/hexagon/gdbstub.c | 16
1 file changed, 16 insertions(+)
diff --git a/target/hexagon/gdbstub.c b/target/hexagon/gdbstub.c
index 46083da620..a06fed9f18 100644
--- a/target/hexagon/gdbstub.c
+++ b/target/hexagon/gdbstub.c
@@ -25,6 +25,14 @@ int hexagon_gdb_read_register(CPUState *cs, GByteArray
*mem_buf, int n)
HexagonCPU *cpu = HEXAGON_CPU(cs);
CPUHexagonState *env = &cpu->env;
+if (n == HEX_REG_P3_0_ALIASED) {
+uint32_t p3_0 = 0;
+for (int i = 0; i < NUM_PREGS; i++) {
+p3_0 = deposit32(p3_0, i * 8, 8, env->pred[i]);
+}
+return gdb_get_regl(mem_buf, p3_0);
+}
+
if (n < TOTAL_PER_THREAD_REGS) {
return gdb_get_regl(mem_buf, env->gpr[n]);
}
@@ -37,6 +45,14 @@ int hexagon_gdb_write_register(CPUState *cs, uint8_t
*mem_buf, int n)
HexagonCPU *cpu = HEXAGON_CPU(cs);
CPUHexagonState *env = &cpu->env;
+if (n == HEX_REG_P3_0_ALIASED) {
+uint32_t p3_0 = ldtul_p(mem_buf);
+for (int i = 0; i < NUM_PREGS; i++) {
+env->pred[i] = extract32(p3_0, i * 8, 8);
+}
+return sizeof(target_ulong);
+}
+
if (n < TOTAL_PER_THREAD_REGS) {
env->gpr[n] = ldtul_p(mem_buf);
return sizeof(target_ulong);
--
2.39.1
[PATCH 3/7] gdbstub: add support for the qRegisterInfo query
From: Brian Cain
Signed-off-by: Brian Cain
Signed-off-by: Matheus Tavares Bernardino
---
include/hw/core/cpu.h | 4
gdbstub/gdbstub.c | 27 +++
2 files changed, 31 insertions(+)
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 821e937020..b16b4e0de5 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -124,6 +124,8 @@ struct SysemuCPUOps;
* its Harvard architecture split code and data.
* @gdb_num_core_regs: Number of core registers accessible to GDB.
* @gdb_core_xml_file: File name for core registers GDB XML description.
+ * @gdb_qreg_info_lines: Array of lines of registers qRegisterInfo description.
+ * @gdb_qreg_info_line_count: Count of lines for @gdb_qreg_info_lines.
* @gdb_stop_before_watchpoint: Indicates whether GDB expects the CPU to stop
* before the insn which triggers a watchpoint rather than after it.
* @gdb_arch_name: Optional callback that returns the architecture name known
@@ -159,6 +161,8 @@ struct CPUClass {
vaddr (*gdb_adjust_breakpoint)(CPUState *cpu, vaddr addr);
const char *gdb_core_xml_file;
+const char **gdb_qreg_info_lines;
+int gdb_qreg_info_line_count;
gchar * (*gdb_arch_name)(CPUState *cpu);
const char * (*gdb_get_dynamic_xml)(CPUState *cpu, const char *xmlname);
diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
index 9d326a1d2d..2edd7d6d88 100644
--- a/gdbstub/gdbstub.c
+++ b/gdbstub/gdbstub.c
@@ -1409,6 +1409,27 @@ static void handle_query_curr_tid(GArray *params, void
*user_ctx)
gdb_put_strbuf();
}
+static void handle_query_regs(GArray *params, void *user_ctx)
+{
+if (!params->len) {
+return;
+}
+
+CPUClass *cc = CPU_GET_CLASS(gdbserver_state.g_cpu);
+if (!cc->gdb_qreg_info_lines) {
+gdb_put_packet("");
+return;
+}
+
+int reg_num = get_param(params, 0)->val_ul;
+if (reg_num >= cc->gdb_qreg_info_line_count) {
+gdb_put_packet("");
+return;
+}
+
+gdb_put_packet(cc->gdb_qreg_info_lines[reg_num]);
+}
+
static void handle_query_threads(GArray *params, void *user_ctx)
{
if (!gdbserver_state.query_cpu) {
@@ -1578,6 +1599,12 @@ static const GdbCmdParseEntry gdb_gen_query_table[] = {
.handler = handle_query_curr_tid,
.cmd = "C",
},
+{
+.handler = handle_query_regs,
+.cmd = "RegisterInfo",
+.cmd_startswith = 1,
+.schema = "l0"
+},
{
.handler = handle_query_threads,
.cmd = "sThreadInfo",
--
2.39.1
[PATCH 2/7] gdbstub: add test for untimely stop-reply packets
In the previous commit, we modified gdbstub.c to only send stop-reply
packets as a response to GDB commands that accept it. Now, let's add a
test for this intended behavior. Running this test before the fix from
the previous commit fails as QEMU sends a stop-reply packet
asynchronously, when GDB was in fact waiting an ACK.
Signed-off-by: Matheus Tavares Bernardino
---
tests/guest-debug/run-test.py| 16
.../tcg/multiarch/system/Makefile.softmmu-target | 16 +++-
2 files changed, 27 insertions(+), 5 deletions(-)
diff --git a/tests/guest-debug/run-test.py b/tests/guest-debug/run-test.py
index d865e46ecd..de6106a5e5 100755
--- a/tests/guest-debug/run-test.py
+++ b/tests/guest-debug/run-test.py
@@ -26,11 +26,12 @@ def get_args():
parser.add_argument("--qargs", help="Qemu arguments for test")
parser.add_argument("--binary", help="Binary to debug",
required=True)
-parser.add_argument("--test", help="GDB test script",
-required=True)
+parser.add_argument("--test", help="GDB test script")
parser.add_argument("--gdb", help="The gdb binary to use",
default=None)
+parser.add_argument("--gdb-args", help="Additional gdb arguments")
parser.add_argument("--output", help="A file to redirect output to")
+parser.add_argument("--stderr", help="A file to redirect stderr to")
return parser.parse_args()
@@ -58,6 +59,10 @@ def log(output, msg):
output = open(args.output, "w")
else:
output = None
+if args.stderr:
+stderr = open(args.stderr, "w")
+else:
+stderr = None
socket_dir = TemporaryDirectory("qemu-gdbstub")
socket_name = os.path.join(socket_dir.name, "gdbstub.socket")
@@ -77,6 +82,8 @@ def log(output, msg):
# Now launch gdb with our test and collect the result
gdb_cmd = "%s %s" % (args.gdb, args.binary)
+if args.gdb_args:
+gdb_cmd += " %s" % (args.gdb_args)
# run quietly and ignore .gdbinit
gdb_cmd += " -q -n -batch"
# disable prompts in case of crash
@@ -84,13 +91,14 @@ def log(output, msg):
# connect to remote
gdb_cmd += " -ex 'target remote %s'" % (socket_name)
# finally the test script itself
-gdb_cmd += " -x %s" % (args.test)
+if args.test:
+gdb_cmd += " -x %s" % (args.test)
sleep(1)
log(output, "GDB CMD: %s" % (gdb_cmd))
-result = subprocess.call(gdb_cmd, shell=True, stdout=output)
+result = subprocess.call(gdb_cmd, shell=True, stdout=output, stderr=stderr)
# A result of greater than 128 indicates a fatal signal (likely a
# crash due to gdb internal failure). That's a problem for GDB and
diff --git a/tests/tcg/multiarch/system/Makefile.softmmu-target
b/tests/tcg/multiarch/system/Makefile.softmmu-target
index 5f432c95f3..fe40195d39 100644
--- a/tests/tcg/multiarch/system/Makefile.softmmu-target
+++ b/tests/tcg/multiarch/system/Makefile.softmmu-target
@@ -27,6 +27,20 @@ run-gdbstub-memory: memory
"-monitor none -display none -chardev
file$(COMMA)path=$<.out$(COMMA)id=output $(QEMU_OPTS)" \
--bin $< --test $(MULTIARCH_SRC)/gdbstub/memory.py, \
softmmu gdbstub support)
+
+run-gdbstub-untimely-packet: hello
+ $(call run-test, $@, $(GDB_SCRIPT) \
+ --gdb $(HAVE_GDB_BIN) \
+ --gdb-args "-ex 'set debug remote 1'" \
+ --output untimely-packet.gdb.out \
+ --stderr untimely-packet.gdb.err \
+ --qemu $(QEMU) \
+ --bin $< --qargs \
+ "-monitor none -display none -chardev
file$(COMMA)path=untimely-packet.out$(COMMA)id=output $(QEMU_OPTS)", \
+ "softmmu gdbstub untimely packets")
+ $(call quiet-command, \
+ (! grep -Fq 'Packet instead of Ack, ignoring it'
untimely-packet.gdb.err), \
+ "GREP", "file untimely-packet.gdb.err")
else
run-gdbstub-%:
$(call skip-test, "gdbstub test $*", "no guest arch support")
@@ -36,4 +50,4 @@ run-gdbstub-%:
$(call skip-test, "gdbstub test $*", "need working gdb")
endif
-MULTIARCH_RUNS += run-gdbstub-memory
+MULTIARCH_RUNS += run-gdbstub-memory run-gdbstub-untimely-packet
--
2.39.1
[PATCH 0/7] Hexagon: add lldb support
This series allows hexagon programs to be debugged under qemu user-mode through LLDB and qemu's gdbstub. LLDB implements the GDB remote serial protocol, so most of the necessary changes are in the Hexagon part itself. However, one fix is needed at the arch-independent side too. This comes from [1], which is now incorporated in this series. [1]: https://lore.kernel.org/qemu-devel/cover.1663677789.git.quic_mathb...@quicinc.com/ Brian Cain (3): gdbstub: add support for the qRegisterInfo query Hexagon: support qRegisterInfo at gdbstub Hexagon (gdbstub): fix p3:0 read and write via stub Matheus Tavares Bernardino (2): gdbstub: only send stop-reply packets when allowed to gdbstub: add test for untimely stop-reply packets Taylor Simpson (2): Hexagon (gdbstub): add HVX support Hexagon (linux-user/hexagon): handle breakpoints gdbstub/internals.h | 5 + include/hw/core/cpu.h | 4 + target/hexagon/gdb_qreginfo.h | 124 ++ gdbstub/gdbstub.c | 64 +++-- gdbstub/softmmu.c | 13 +- gdbstub/user.c| 17 ++- linux-user/hexagon/cpu_loop.c | 10 ++ target/hexagon/cpu.c | 3 + target/hexagon/gdbstub.c | 76 +++ tests/guest-debug/run-test.py | 16 ++- .../multiarch/system/Makefile.softmmu-target | 16 ++- 11 files changed, 326 insertions(+), 22 deletions(-) create mode 100644 target/hexagon/gdb_qreginfo.h -- 2.39.1
[PATCH] Hexagon (translate.c): avoid redundant PC updates on COF
When there is a conditional change of flow or an endloop instruction, we
preload HEX_REG_PC with ctx->next_PC at gen_start_packet(). Nonetheless,
we still generate TCG code to do this update again at gen_goto_tb() when
the condition for the COF is not met, thus producing redundant
instructions. This can be seen with the following packet:
0x004002e4: 0x5c20d000 { if (!P0) jump:t PC+0 }
Which generates this TCG code:
004002e4
-> mov_i32 pc,$0x4002e8
and_i32 loc9,p0,$0x1
mov_i32 branch_taken,loc9
add_i32 pkt_cnt,pkt_cnt,$0x2
add_i32 insn_cnt,insn_cnt,$0x2
brcond_i32 branch_taken,$0x0,ne,$L1
goto_tb $0x0
mov_i32 pc,$0x4002e4
exit_tb $0x7fb0c36e5200
set_label $L1
goto_tb $0x1
-> mov_i32 pc,$0x4002e8
exit_tb $0x7fb0c36e5201
set_label $L0
exit_tb $0x7fb0c36e5203
Note that even after optimizations, the redundant PC update is still
present:
004002e4
-> mov_i32 pc,$0x4002e8 sync: 0 dead: 0 1 pref=0x
mov_i32 branch_taken,$0x1sync: 0 dead: 0 1 pref=0x
add_i32 pkt_cnt,pkt_cnt,$0x2 sync: 0 dead: 0 1 pref=0x
add_i32 insn_cnt,insn_cnt,$0x2 sync: 0 dead: 0 1 2 pref=0x
goto_tb $0x1
-> mov_i32 pc,$0x4002e8 sync: 0 dead: 0 1 pref=0x
exit_tb $0x7fb0c36e5201
set_label $L0
exit_tb $0x7fb0c36e5203
With this patch, the second redundant update is properly discarded.
Note that we need the additional "move_to_pc" flag instead of just
avoiding the update whenever `dest == ctx->next_PC`, as that could
potentially skip updates from a COF with met condition, whose
ctx->branch_dest just happens to be equal to ctx->next_PC.
Signed-off-by: Matheus Tavares Bernardino
---
target/hexagon/translate.c | 21 +
1 file changed, 13 insertions(+), 8 deletions(-)
diff --git target/hexagon/translate.c target/hexagon/translate.c
index 665476ab48..58d638f734 100644
--- target/hexagon/translate.c
+++ target/hexagon/translate.c
@@ -128,14 +128,19 @@ static bool use_goto_tb(DisasContext *ctx, target_ulong
dest)
return translator_use_goto_tb(&ctx->base, dest);
}
-static void gen_goto_tb(DisasContext *ctx, int idx, target_ulong dest)
+static void gen_goto_tb(DisasContext *ctx, int idx, target_ulong dest, bool
+move_to_pc)
{
if (use_goto_tb(ctx, dest)) {
tcg_gen_goto_tb(idx);
-tcg_gen_movi_tl(hex_gpr[HEX_REG_PC], dest);
+if (move_to_pc) {
+tcg_gen_movi_tl(hex_gpr[HEX_REG_PC], dest);
+}
tcg_gen_exit_tb(ctx->base.tb, idx);
} else {
-tcg_gen_movi_tl(hex_gpr[HEX_REG_PC], dest);
+if (move_to_pc) {
+tcg_gen_movi_tl(hex_gpr[HEX_REG_PC], dest);
+}
tcg_gen_lookup_and_goto_ptr();
}
}
@@ -150,11 +155,11 @@ static void gen_end_tb(DisasContext *ctx)
if (ctx->branch_cond != TCG_COND_ALWAYS) {
TCGLabel *skip = gen_new_label();
tcg_gen_brcondi_tl(ctx->branch_cond, hex_branch_taken, 0, skip);
-gen_goto_tb(ctx, 0, ctx->branch_dest);
+gen_goto_tb(ctx, 0, ctx->branch_dest, true);
gen_set_label(skip);
-gen_goto_tb(ctx, 1, ctx->next_PC);
+gen_goto_tb(ctx, 1, ctx->next_PC, false);
} else {
-gen_goto_tb(ctx, 0, ctx->branch_dest);
+gen_goto_tb(ctx, 0, ctx->branch_dest, true);
}
} else if (ctx->is_tight_loop &&
pkt->insn[pkt->num_insns - 1].opcode == J2_endloop0) {
@@ -165,9 +170,9 @@ static void gen_end_tb(DisasContext *ctx)
TCGLabel *skip = gen_new_label();
tcg_gen_brcondi_tl(TCG_COND_LEU, hex_gpr[HEX_REG_LC0], 1, skip);
tcg_gen_subi_tl(hex_gpr[HEX_REG_LC0], hex_gpr[HEX_REG_LC0], 1);
-gen_goto_tb(ctx, 0, ctx->base.tb->pc);
+gen_goto_tb(ctx, 0, ctx->base.tb->pc, true);
gen_set_label(skip);
-gen_goto_tb(ctx, 1, ctx->next_PC);
+gen_goto_tb(ctx, 1, ctx->next_PC, false);
} else {
tcg_gen_lookup_and_goto_ptr();
}
--
2.37.2
[PATCH RESEND v2 2/2] migration/xbzrle: fix out-of-bounds write with axv512
xbzrle_encode_buffer_avx512() checks for overflows too scarcely in its
outer loop, causing out-of-bounds writes:
$ ../configure --target-list=aarch64-softmmu --enable-sanitizers
--enable-avx512bw
$ make tests/unit/test-xbzrle && ./tests/unit/test-xbzrle
==5518==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6210b100
at pc 0x561109a7714d bp 0x7ffed712a440 sp 0x7ffed712a430
WRITE of size 1 at 0x6210b100 thread T0
#0 0x561109a7714c in uleb128_encode_small ../util/cutils.c:831
#1 0x561109b67f6a in xbzrle_encode_buffer_avx512 ../migration/xbzrle.c:275
#2 0x5611099a7428 in test_encode_decode_overflow
../tests/unit/test-xbzrle.c:153
#3 0x7fb2fb65a58d (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x7a58d)
#4 0x7fb2fb65a333 (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x7a333)
#5 0x7fb2fb65aa79 in g_test_run_suite
(/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x7aa79)
#6 0x7fb2fb65aa94 in g_test_run
(/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x7aa94)
#7 0x5611099a3a23 in main ../tests/unit/test-xbzrle.c:218
#8 0x7fb2fa78c082 in __libc_start_main
(/lib/x86_64-linux-gnu/libc.so.6+0x24082)
#9 0x5611099a608d in _start (/qemu/build/tests/unit/test-xbzrle+0x28408d)
0x6210b100 is located 0 bytes to the right of 4096-byte region
[0x6210a100,0x6210b100)
allocated by thread T0 here:
#0 0x7fb2fb823a06 in __interceptor_calloc
../../../../src/libsanitizer/asan/asan_malloc_linux.cc:153
#1 0x7fb2fb637ef0 in g_malloc0
(/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x57ef0)
Fix that by performing the overflow check in the inner loop, instead.
Signed-off-by: Matheus Tavares Bernardino
---
migration/xbzrle.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/migration/xbzrle.c b/migration/xbzrle.c
index 21b92d4eae..c6f8b20917 100644
--- a/migration/xbzrle.c
+++ b/migration/xbzrle.c
@@ -197,10 +197,6 @@ int xbzrle_encode_buffer_avx512(uint8_t *old_buf, uint8_t
*new_buf, int slen,
__m512i r = _mm512_set1_epi32(0);
while (count512s) {
-if (d + 2 > dlen) {
-return -1;
-}
-
int bytes_to_check = 64;
uint64_t mask = 0x;
if (count512s == 1) {
@@ -216,6 +212,9 @@ int xbzrle_encode_buffer_avx512(uint8_t *old_buf, uint8_t
*new_buf, int slen,
bool is_same = (comp & 0x1);
while (bytes_to_check) {
+if (d + 2 > dlen) {
+return -1;
+}
if (is_same) {
if (nzrun_len) {
d += uleb128_encode_small(dst + d, nzrun_len);
--
2.39.1
[PATCH RESEND v2 1/2] migration/xbzrle: use ctz64 to avoid undefined result
__builtin_ctzll() produces undefined results when the argument is 0. This can be seen through test-xbzrle, which produces the following warning: ../migration/xbzrle.c:265: runtime error: passing zero to ctz(), which is not a valid argument Replace __builtin_ctzll() with our ctz64() wrapper which properly handles 0. Signed-off-by: Matheus Tavares Bernardino --- migration/xbzrle.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/migration/xbzrle.c b/migration/xbzrle.c index 05366e86c0..21b92d4eae 100644 --- a/migration/xbzrle.c +++ b/migration/xbzrle.c @@ -12,6 +12,7 @@ */ #include "qemu/osdep.h" #include "qemu/cutils.h" +#include "qemu/host-utils.h" #include "xbzrle.h" /* @@ -233,7 +234,7 @@ int xbzrle_encode_buffer_avx512(uint8_t *old_buf, uint8_t *new_buf, int slen, break; } never_same = false; -num = __builtin_ctzll(~comp); +num = ctz64(~comp); num = (num < bytes_to_check) ? num : bytes_to_check; zrun_len += num; bytes_to_check -= num; @@ -262,7 +263,7 @@ int xbzrle_encode_buffer_avx512(uint8_t *old_buf, uint8_t *new_buf, int slen, nzrun_len += 64; break; } -num = __builtin_ctzll(comp); +num = ctz64(comp); num = (num < bytes_to_check) ? num : bytes_to_check; nzrun_len += num; bytes_to_check -= num; -- 2.39.1
[PATCH RESEND v2 0/2] migration/xbzrle: fix two avx512 runtime issues
This patchset strives to fix two bugs at xvzrle when --enable-avx512 is used: an out-of-bounds write and an invalid argument to __builtin_ctz(). Those two errors can be seen in the test suite running: $ ../configure --target-list=aarch64-softmmu --enable-sanitizers --enable-avx512bw $ make tests/unit/test-xbzrle && ./tests/unit/test-xbzrle ==5518==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6210b100 at pc 0x561109a7714d bp 0x7ffed712a440 sp 0x7ffed712a430 WRITE of size 1 at 0x6210b100 thread T0 #0 0x561109a7714c in uleb128_encode_small ../util/cutils.c:831 #1 0x561109b67f6a in xbzrle_encode_buffer_avx512 ../migration/xbzrle.c:275 #2 0x5611099a7428 in test_encode_decode_overflow ../tests/unit/test-xbzrle.c:153 #3 0x7fb2fb65a58d (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x7a58d) #4 0x7fb2fb65a333 (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x7a333) #5 0x7fb2fb65aa79 in g_test_run_suite (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x7aa79) #6 0x7fb2fb65aa94 in g_test_run (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x7aa94) #7 0x5611099a3a23 in main ../tests/unit/test-xbzrle.c:218 #8 0x7fb2fa78c082 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x24082) #9 0x5611099a608d in _start (/qemu/build/tests/unit/test-xbzrle+0x28408d) 0x6210b100 is located 0 bytes to the right of 4096-byte region [0x6210a100,0x6210b100) allocated by thread T0 here: #0 0x7fb2fb823a06 in __interceptor_calloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cc:153 #1 0x7fb2fb637ef0 in g_malloc0 (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x57ef0) ../migration/xbzrle.c:265: runtime error: passing zero to ctz(), which is not a valid argument v1: https://lore.kernel.org/qemu-devel/cover.1678199971.git.quic_mathb...@quicinc.com/ (No changes since v1, only rebased on current master) Matheus Tavares Bernardino (2): migration/xbzrle: use ctz64 to avoid undefined result migration/xbzrle: fix out-of-bounds write with axv512 migration/xbzrle.c | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) -- 2.39.1
[PATCH 2/2] migration/xbzrle: fix out-of-bounds write with axv512
xbzrle_encode_buffer_avx512() checks for overflows too scarcely in its
outer loop, causing out-of-bounds writes:
$ ../configure --target-list=aarch64-softmmu --enable-sanitizers
--enable-avx512bw
$ make tests/unit/test-xbzrle && ./tests/unit/test-xbzrle
==5518==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6210b100
at pc 0x561109a7714d bp 0x7ffed712a440 sp 0x7ffed712a430
WRITE of size 1 at 0x6210b100 thread T0
#0 0x561109a7714c in uleb128_encode_small ../util/cutils.c:831
#1 0x561109b67f6a in xbzrle_encode_buffer_avx512 ../migration/xbzrle.c:275
#2 0x5611099a7428 in test_encode_decode_overflow
../tests/unit/test-xbzrle.c:153
#3 0x7fb2fb65a58d (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x7a58d)
#4 0x7fb2fb65a333 (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x7a333)
#5 0x7fb2fb65aa79 in g_test_run_suite
(/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x7aa79)
#6 0x7fb2fb65aa94 in g_test_run
(/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x7aa94)
#7 0x5611099a3a23 in main ../tests/unit/test-xbzrle.c:218
#8 0x7fb2fa78c082 in __libc_start_main
(/lib/x86_64-linux-gnu/libc.so.6+0x24082)
#9 0x5611099a608d in _start (/qemu/build/tests/unit/test-xbzrle+0x28408d)
0x6210b100 is located 0 bytes to the right of 4096-byte region
[0x6210a100,0x6210b100)
allocated by thread T0 here:
#0 0x7fb2fb823a06 in __interceptor_calloc
../../../../src/libsanitizer/asan/asan_malloc_linux.cc:153
#1 0x7fb2fb637ef0 in g_malloc0
(/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x57ef0)
Fix that by performing the overflow check in the inner loop, instead.
Signed-off-by: Matheus Tavares Bernardino
---
migration/xbzrle.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/migration/xbzrle.c b/migration/xbzrle.c
index 21b92d4eae..c6f8b20917 100644
--- a/migration/xbzrle.c
+++ b/migration/xbzrle.c
@@ -197,10 +197,6 @@ int xbzrle_encode_buffer_avx512(uint8_t *old_buf, uint8_t
*new_buf, int slen,
__m512i r = _mm512_set1_epi32(0);
while (count512s) {
-if (d + 2 > dlen) {
-return -1;
-}
-
int bytes_to_check = 64;
uint64_t mask = 0x;
if (count512s == 1) {
@@ -216,6 +212,9 @@ int xbzrle_encode_buffer_avx512(uint8_t *old_buf, uint8_t
*new_buf, int slen,
bool is_same = (comp & 0x1);
while (bytes_to_check) {
+if (d + 2 > dlen) {
+return -1;
+}
if (is_same) {
if (nzrun_len) {
d += uleb128_encode_small(dst + d, nzrun_len);
--
2.39.1
[PATCH 1/2] migration/xbzrle: use ctz64 to avoid undefined result
__builtin_ctzll() produces undefined results when the argument is 0. This can be seen through test-xbzrle, which produces the following warning: ../migration/xbzrle.c:265: runtime error: passing zero to ctz(), which is not a valid argument Replace __builtin_ctzll() with our ctz64() wrapper which properly handles 0. Signed-off-by: Matheus Tavares Bernardino --- migration/xbzrle.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/migration/xbzrle.c b/migration/xbzrle.c index 05366e86c0..21b92d4eae 100644 --- a/migration/xbzrle.c +++ b/migration/xbzrle.c @@ -12,6 +12,7 @@ */ #include "qemu/osdep.h" #include "qemu/cutils.h" +#include "qemu/host-utils.h" #include "xbzrle.h" /* @@ -233,7 +234,7 @@ int xbzrle_encode_buffer_avx512(uint8_t *old_buf, uint8_t *new_buf, int slen, break; } never_same = false; -num = __builtin_ctzll(~comp); +num = ctz64(~comp); num = (num < bytes_to_check) ? num : bytes_to_check; zrun_len += num; bytes_to_check -= num; @@ -262,7 +263,7 @@ int xbzrle_encode_buffer_avx512(uint8_t *old_buf, uint8_t *new_buf, int slen, nzrun_len += 64; break; } -num = __builtin_ctzll(comp); +num = ctz64(comp); num = (num < bytes_to_check) ? num : bytes_to_check; nzrun_len += num; bytes_to_check -= num; -- 2.39.1
[PATCH 0/2] migration/xbzrle: fix two avx512 runtime issues
Matheus Tavares Bernardino (2): migration/xbzrle: use ctz64 to avoid undefined result migration/xbzrle: fix out-of-bounds write with axv512 migration/xbzrle.c | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) -- 2.39.1
[PATCH] io/channel-tls: plug memory leakage on GSource
This leakage can be seen through test-io-channel-tls: $ ../configure --target-list=aarch64-softmmu --enable-sanitizers $ make ./tests/unit/test-io-channel-tls $ ./tests/unit/test-io-channel-tls Indirect leak of 104 byte(s) in 1 object(s) allocated from: #0 0x7f81d1725808 in __interceptor_malloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cc:144 #1 0x7f81d135ae98 in g_malloc (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x57e98) #2 0x55616c5d4c1b in object_new_with_propv ../qom/object.c:795 #3 0x55616c5d4a83 in object_new_with_props ../qom/object.c:768 #4 0x55616c5c5415 in test_tls_creds_create ../tests/unit/test-io-channel-tls.c:70 #5 0x55616c5c5a6b in test_io_channel_tls ../tests/unit/test-io-channel-tls.c:158 #6 0x7f81d137d58d (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x7a58d) Indirect leak of 32 byte(s) in 1 object(s) allocated from: #0 0x7f81d1725a06 in __interceptor_calloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cc:153 #1 0x7f81d1472a20 in gnutls_dh_params_init (/lib/x86_64-linux-gnu/libgnutls.so.30+0x46a20) #2 0x55616c6485ff in qcrypto_tls_creds_x509_load ../crypto/tlscredsx509.c:634 #3 0x55616c648ba2 in qcrypto_tls_creds_x509_complete ../crypto/tlscredsx509.c:694 #4 0x55616c5e1fea in user_creatable_complete ../qom/object_interfaces.c:28 #5 0x55616c5d4c8c in object_new_with_propv ../qom/object.c:807 #6 0x55616c5d4a83 in object_new_with_props ../qom/object.c:768 #7 0x55616c5c5415 in test_tls_creds_create ../tests/unit/test-io-channel-tls.c:70 #8 0x55616c5c5a6b in test_io_channel_tls ../tests/unit/test-io-channel-tls.c:158 #9 0x7f81d137d58d (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x7a58d) ... SUMMARY: AddressSanitizer: 49143 byte(s) leaked in 184 allocation(s). The docs for `g_source_add_child_source(source, child_source)` says "source will hold a reference on child_source while child_source is attached to it." Therefore, we should unreference the child source at `qio_channel_tls_read_watch()` after attaching it to `source`. With this change, ./tests/unit/test-io-channel-tls shows no leakages. Signed-off-by: Matheus Tavares Bernardino --- io/channel-tls.c | 1 + 1 file changed, 1 insertion(+) diff --git a/io/channel-tls.c b/io/channel-tls.c index 8052945ba0..5a7a3d48d6 100644 --- a/io/channel-tls.c +++ b/io/channel-tls.c @@ -446,6 +446,7 @@ qio_channel_tls_read_watch(QIOChannelTLS *tioc, GSource *source) object_ref(OBJECT(tioc)); g_source_add_child_source(source, child); +g_source_unref(child); } static GSource *qio_channel_tls_create_watch(QIOChannel *ioc, -- 2.37.2
