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

[Fabiano Rosas <farosas@suse.de>]

Fabiano Rosas <farosas@suse.de> writes:

> 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;
>         }
>  

Ok, I spotted the sorting now.