Re: [PULL 02/20] target/arm: Correct encoding of Debug Communications Channel registers

[Fabiano Rosas <farosas@suse.de>]

Peter Maydell <peter.maydell@linaro.org> writes:

> We don't implement the Debug Communications Channel (DCC), but
> we do attempt to provide dummy versions of its system registers
> so that software that tries to access them doesn't fall over.
>
> However, we got the tx/rx register definitions wrong. These
> should be:
>
> AArch32:
>   DBGDTRTX   p14 0 c0 c5 0  (on writes)
>   DBGDTRRX   p14 0 c0 c5 0  (on reads)
>
> AArch64:
>   DBGDTRTX_EL0  2 3 0 5 0 (on writes)
>   DBGDTRRX_EL0  2 3 0 5 0 (on reads)
>   DBGDTR_EL0    2 3 0 4 0 (reads and writes)
>
> where DBGDTRTX and DBGDTRRX are effectively different names for the
> same 32-bit register, which has tx behaviour on writes and rx
> behaviour on reads.  The AArch64-only DBGDTR_EL0 is a 64-bit wide
> register whose top and bottom halves map to the DBGDTRRX and DBGDTRTX
> registers.
>
> Currently we have just one cpreg struct, which:
>  * calls itself DBGDTR_EL0
>  * uses the DBGDTRTX_EL0/DBGDTRRX_EL0 encoding
>  * is marked as ARM_CP_STATE_BOTH but has the wrong opc1
>    value for AArch32
>  * is implemented as RAZ/WI
>
> Correct the encoding so:
>  * we name the DBGDTRTX/DBGDTRRX register correctly
>  * we split it into AA64 and AA32 versions so we can get the
>    AA32 encoding right
>  * we implement DBGDTR_EL0 at its correct encoding
>
> Cc: qemu-stable@nongnu.org
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2986
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Message-id: 20250708141049.778361-1-peter.maydell@linaro.org
> ---
>  target/arm/debug_helper.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/target/arm/debug_helper.c b/target/arm/debug_helper.c
> index 69fb1d0d9ff..aee06d4d426 100644
> --- a/target/arm/debug_helper.c
> +++ b/target/arm/debug_helper.c
> @@ -988,11 +988,20 @@ static const ARMCPRegInfo debug_cp_reginfo[] = {
>        .opc0 = 2, .opc1 = 0, .crn = 0, .crm = 3, .opc2 = 2,
>        .access = PL1_RW, .accessfn = access_tdcc,
>        .type = ARM_CP_CONST, .resetvalue = 0 },
> -    /* DBGDTRTX_EL0/DBGDTRRX_EL0 depend on direction */
> -    { .name = "DBGDTR_EL0", .state = ARM_CP_STATE_BOTH, .cp = 14,
> +    /* Architecturally DBGDTRTX is named DBGDTRRX when used for reads */
> +    { .name = "DBGDTRTX_EL0", .state = ARM_CP_STATE_AA64,
>        .opc0 = 2, .opc1 = 3, .crn = 0, .crm = 5, .opc2 = 0,
>        .access = PL0_RW, .accessfn = access_tdcc,
>        .type = ARM_CP_CONST, .resetvalue = 0 },
> +    { .name = "DBGDTRTX", .state = ARM_CP_STATE_AA32, .cp = 14,
> +      .opc1 = 0, .crn = 0, .crm = 5, .opc2 = 0,
> +      .access = PL0_RW, .accessfn = access_tdcc,
> +      .type = ARM_CP_CONST, .resetvalue = 0 },
> +    /* This is AArch64-only and is a combination of DBGDTRTX and DBGDTRRX */
> +    { .name = "DBGDTR_EL0", .state = ARM_CP_STATE_AA64,
> +      .opc0 = 2, .opc1 = 3, .crn = 0, .crm = 4, .opc2 = 0,
> +      .access = PL0_RW, .accessfn = access_tdcc,
> +      .type = ARM_CP_CONST, .resetvalue = 0 },
>      /*
>       * OSECCR_EL1 provides a mechanism for an operating system
>       * to access the contents of EDECCR. EDECCR is not implemented though,

Hi, this patch breaks migration. I'm leaving for the day and will take a
closer look in the morning. But since we have timezones, here it is:

$ cd build

$ sed -i 's/-cpu max/-cpu neoverse-n1/' ../tests/qtest/migration/framework.c
(sorry about this^, I just now got around to looking into it)

$ make

# v10.0.0 -> v10.1.0-rc0
$ QTEST_QEMU_BINARY=./build-10.0.0/qemu-system-aarch64 \
QTEST_QEMU_BINARY_DST=./qemu-system-aarch64 \
./tests/qtest/migration-test -p /aarch64/migration/precopy/file
...

# starting QEMU: exec build-10.0.0/qemu-system-aarch64 -qtest
  unix:/tmp/qtest-4328.sock -qtest-log /dev/null -chardev
  socket,path=/tmp/qtest-4328.qmp,id=char0 -mon
  chardev=char0,mode=control -display none -audio none -accel kvm -accel
  tcg -machine virt-10.0,gic-version=3 -name source,debug-threads=on -m
  150M -serial file:/tmp/migration-test-GXLFA3/src_serial -cpu
  neoverse-n1 -kernel /tmp/migration-test-GXLFA3/bootsect -accel qtest

# starting QEMU: exec ./qemu-system-aarch64 -qtest
  unix:/tmp/qtest-4328.sock -qtest-log /dev/null -chardev
  socket,path=/tmp/qtest-4328.qmp,id=char0 -mon
  chardev=char0,mode=control -display none -audio none -accel kvm -accel
  tcg -machine virt-10.0,gic-version=3 -name target,debug-threads=on -m
  150M -serial file:/tmp/migration-test-GXLFA3/dest_serial -incoming
  defer -cpu neoverse-n1 -kernel /tmp/migration-test-GXLFA3/bootsect
  -accel qtest

qemu-system-aarch64: error while loading state for instance 0x0 of device 'cpu'
qemu-system-aarch64: load of migration failed: Operation not permitted
Broken pipe
../tests/qtest/libqtest.c:199: kill_qemu() tried to terminate QEMU process but encountered exit status 1 (expected 0)
Aborted (core dumped)