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:

> On Wed, 23 Jul 2025 at 23:20, Fabiano Rosas <farosas@suse.de> wrote:
>>
>> 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:
>
> Thanks for the report; I can repro this. It happens because
> the loop in cpu_post_load hits the "register in their list but
> not ours" check, because the source VM has the AArch32
> {.cp = 14, .opc0 = 2, .opc1 = 3, .crn = 0, .crm = 5, .opc2 = 0}
> register which should never have existed.
>

My debugging (in dst) shows:

(gdb) x/16x 0x555557ad5d20 //cpu->cpreg_vmstate_indexes[0x18 to 0x1c]
0x555557ad5d20: 0x200e0205      0x40200000      0x200e0284<     0x40200000
0x555557ad5d30: 0x200e0285      0x40200000      0x200e0298      0x40200000
0x555557ad5d40: 0x200e0302      0x40200000      0x200e0380      0x40200000
0x555557ad5d50: 0x200e0800      0x40200000      0x200e0838      0x40200000

(gdb) x/16x 0x555557ad4ac0 //cpu->cpreg_indexes[0x18 to 0x1c]
p0x555557ad4ac0: 0x200e0205      0x40200000      0x200e0280<     0x40200000
0x555557ad4ad0: 0x200e0284      0x40200000      0x200e0285      0x40200000
0x555557ad4ae0: 0x200e0302      0x40200000      0x200e0380      0x40200000
0x555557ad4af0: 0x200e0800      0x40200000      0x200e0838      0x40200000


> I'm not sure how to handle this, as we have no mechanism for
> "ignore this incoming register value, it is bogus". I'm surprised
> we've never run into this before...
>

I was thinking the same.

I actually don't understand what the encoding in cpu->cpreg_indexes is
supposed to represent. How does comparing the indexes implies "in our
list"/"in their list"? Is there some sort of ISA level assumption?

       if (cpu->cpreg_vmstate_indexes[v] > cpu->cpreg_indexes[i]) {
            /* register in our list but not incoming : skip it */
            continue;
        }
        if (cpu->cpreg_vmstate_indexes[v] < cpu->cpreg_indexes[i]) {
            /* register in their list but not ours: fail migration */
            return -1;
        }
 
> I won't be able to look at this further til the tail end of
> next week.
>
> As an aside, it is a shame that post_load hooks do not
> take an Error** -- if they did we would be able to report
> this more usefully to the user by saying why the migration
> failed instead of just returning -1. Perhaps it would be
> worth adding _err versions of the hook fields in
> VMStateDescription so that devices can optionally
> implement them instead if they have interesting or
> complicated errors to report ?
>

Definitely, there's work happening at moment in the upper layer that
will allow errors to be propagated from post_load.

https://lore.kernel.org/r/20250725-propagate_tpm_error-v7-0-d52704443975@redhat.com

> thanks
> -- PMM