diff mbox series

target/arm: Correct the encoding of MDCCSR_EL0

Message ID 20210629082713.31396-1-hnick@vmware.com
State New
Headers show
Series target/arm: Correct the encoding of MDCCSR_EL0 | expand

Commit Message

Nick Hudson June 29, 2021, 8:27 a.m. UTC 
Signed-off-by: Nick Hudson <hnick@vmware.com>
---
 target/arm/helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Nick Hudson June 29, 2021, 10:41 a.m. UTC | #1 
> On 29 Jun 2021, at 10:49, Peter Maydell <peter.maydell@linaro.org> wrote:
> 
> On Tue, 29 Jun 2021 at 09:27, <hnick@vmware.com> wrote:
>> 
>> Signed-off-by: Nick Hudson <hnick@vmware.com>
>> ---
>> target/arm/helper.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/target/arm/helper.c b/target/arm/helper.c
>> index a66c1f0b9e..7267af7924 100644
>> --- a/target/arm/helper.c
>> +++ b/target/arm/helper.c
>> @@ -6330,7 +6330,7 @@ static const ARMCPRegInfo debug_cp_reginfo[] = {
>>      * We don't implement the configurable EL0 access.
>>      */
>>     { .name = "MDCCSR_EL0", .state = ARM_CP_STATE_BOTH,
>> -      .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 0, .crm = 1, .opc2 = 0,
>> +      .cp = 14, .opc0 = 2, .opc1 = 3, .crn = 0, .crm = 1, .opc2 = 0,
>>       .type = ARM_CP_ALIAS,
>>       .access = PL1_R, .accessfn = access_tda,
>>       .fieldoffset = offsetof(CPUARMState, cp15.mdscr_el1), },
> 
> This fixes the encoding for AArch64, but breaks it for AArch32,
> where it is cp=14 opc1=0 crn=0 crm=1 opc2=0. Because this is one of
> those system registers where the AArch64 and AArch32 encodings
> don't match up, to fix the AArch64 encoding we need to replace
> this ARM_CP_STATE_BOTH reginfo with separate reginfo for
> ARM_CP_STATE_AA32 and ARM_CP_STATE_AA64, something like this:
> 
>    { .name = "MDCCSR_EL0", .state = ARM_CP_STATE_AA64,
>      .opc0 = 2, .opc1 = 3, .crn = 0, .crm = 1, .opc2 = 0,
>      .type = ARM_CP_ALIAS,
>      .access = PL1_R, .accessfn = access_tda,
>      .fieldoffset = offsetof(CPUARMState, cp15.mdscr_el1), },
>    { .name = "DBGDSCRint", .state = ARM_CP_STATE_AA32,
>      .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 0, .crm = 1, .opc2 = 0,
>      .type = ARM_CP_ALIAS,
>      .access = PL1_R, .accessfn = access_tda,
>      .fieldoffset = offsetoflow32(CPUARMState, cp15.mdscr_el1), },
> 

Ah, yes.

As MDCCSR_EL0[30:29] only maps to DBGDSCRint[30:29] maybe it should be all RAZ?

Thanks,
Nick
Nick Hudson July 2, 2021, 3:01 p.m. UTC | #2 
> On 29 Jun 2021, at 12:50, Peter Maydell <peter.maydell@linaro.org> wrote:
> 
> On Tue, 29 Jun 2021 at 11:41, Nick Hudson <hnick@vmware.com> wrote:
>> 
>> 
>> 
>>> On 29 Jun 2021, at 10:49, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> 
>>> On Tue, 29 Jun 2021 at 09:27, <hnick@vmware.com> wrote:
>>>> 
>>>> Signed-off-by: Nick Hudson <hnick@vmware.com>
>>>> ---
>>>> target/arm/helper.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>> 
>>>> diff --git a/target/arm/helper.c b/target/arm/helper.c
>>>> index a66c1f0b9e..7267af7924 100644
>>>> --- a/target/arm/helper.c
>>>> +++ b/target/arm/helper.c
>>>> @@ -6330,7 +6330,7 @@ static const ARMCPRegInfo debug_cp_reginfo[] = {
>>>>     * We don't implement the configurable EL0 access.
>>>>     */
>>>>    { .name = "MDCCSR_EL0", .state = ARM_CP_STATE_BOTH,
>>>> -      .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 0, .crm = 1, .opc2 = 0,
>>>> +      .cp = 14, .opc0 = 2, .opc1 = 3, .crn = 0, .crm = 1, .opc2 = 0,
>>>>      .type = ARM_CP_ALIAS,
>>>>      .access = PL1_R, .accessfn = access_tda,
>>>>      .fieldoffset = offsetof(CPUARMState, cp15.mdscr_el1), },
>>> 
>>> This fixes the encoding for AArch64, but breaks it for AArch32,
>>> where it is cp=14 opc1=0 crn=0 crm=1 opc2=0. Because this is one of
>>> those system registers where the AArch64 and AArch32 encodings
>>> don't match up, to fix the AArch64 encoding we need to replace
>>> this ARM_CP_STATE_BOTH reginfo with separate reginfo for
>>> ARM_CP_STATE_AA32 and ARM_CP_STATE_AA64, something like this:
>>> 
>>>   { .name = "MDCCSR_EL0", .state = ARM_CP_STATE_AA64,
>>>     .opc0 = 2, .opc1 = 3, .crn = 0, .crm = 1, .opc2 = 0,
>>>     .type = ARM_CP_ALIAS,
>>>     .access = PL1_R, .accessfn = access_tda,
>>>     .fieldoffset = offsetof(CPUARMState, cp15.mdscr_el1), },
>>>   { .name = "DBGDSCRint", .state = ARM_CP_STATE_AA32,
>>>     .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 0, .crm = 1, .opc2 = 0,
>>>     .type = ARM_CP_ALIAS,
>>>     .access = PL1_R, .accessfn = access_tda,
>>>     .fieldoffset = offsetoflow32(CPUARMState, cp15.mdscr_el1), },
>>> 
>> 
>> Ah, yes.
>> 
>> As MDCCSR_EL0[30:29] only maps to DBGDSCRint[30:29] maybe it should be all RAZ?
> 
> Well, you can't make it all RAZ, because those 2 bits do still
> need to be mapped, but I guess in theory yes we should define
> read and write accessor functions for AArch64 MDCCSR_EL0 that
> mask out everything except [30:29].


Hi Peter,

Maybe I’m misreading the ARM ARM and the qemu use of mdscr_el1, but I think
this is good enough / more correct.  I’m somewhat confused by AA64 MDSCR_EL1
vs DBGSCRint vs DBGSCRext, however.

    /* MDCCSR_EL0[30:29] map to DBGDSCRint[30:29]. Simply RAZ.
     * We don't implement the configurable EL0 access.
     */
    { .name = "MDCCSR_EL0", .state = ARM_CP_STATE_AA64,
      .cp = 14, .opc0 = 2, .opc1 = 3, .crn = 0, .crm = 1, .opc2 = 0,
      .type = ARM_CP_CONST, .resetvalue = 0 },
    /* DBGDSCRint[15,12,5:2] map to MDSCR_EL1[15,12,5:2] */
    { .name = "DBGDSCRint", .state = ARM_CP_STATE_AA32,
      .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 0, .crm = 1, .opc2 = 0,
      .type = ARM_CP_ALIAS,
      .access = PL1_R, .accessfn = access_tda,
      .fieldoffset = offsetof(CPUARMState, cp15.mdscr_el1), },

Please let me know if you want me to submit a new patch.

Thanks,
Nick
Nick Hudson July 2, 2021, 3:01 p.m. UTC | #3 
> On 29 Jun 2021, at 12:50, Peter Maydell <peter.maydell@linaro.org> wrote:
> 
> On Tue, 29 Jun 2021 at 11:41, Nick Hudson <hnick@vmware.com> wrote:
>> 
>> 
>> 
>>> On 29 Jun 2021, at 10:49, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> 
>>> On Tue, 29 Jun 2021 at 09:27, <hnick@vmware.com> wrote:
>>>> 
>>>> Signed-off-by: Nick Hudson <hnick@vmware.com>
>>>> ---
>>>> target/arm/helper.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>> 
>>>> diff --git a/target/arm/helper.c b/target/arm/helper.c
>>>> index a66c1f0b9e..7267af7924 100644
>>>> --- a/target/arm/helper.c
>>>> +++ b/target/arm/helper.c
>>>> @@ -6330,7 +6330,7 @@ static const ARMCPRegInfo debug_cp_reginfo[] = {
>>>>     * We don't implement the configurable EL0 access.
>>>>     */
>>>>    { .name = "MDCCSR_EL0", .state = ARM_CP_STATE_BOTH,
>>>> -      .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 0, .crm = 1, .opc2 = 0,
>>>> +      .cp = 14, .opc0 = 2, .opc1 = 3, .crn = 0, .crm = 1, .opc2 = 0,
>>>>      .type = ARM_CP_ALIAS,
>>>>      .access = PL1_R, .accessfn = access_tda,
>>>>      .fieldoffset = offsetof(CPUARMState, cp15.mdscr_el1), },
>>> 
>>> This fixes the encoding for AArch64, but breaks it for AArch32,
>>> where it is cp=14 opc1=0 crn=0 crm=1 opc2=0. Because this is one of
>>> those system registers where the AArch64 and AArch32 encodings
>>> don't match up, to fix the AArch64 encoding we need to replace
>>> this ARM_CP_STATE_BOTH reginfo with separate reginfo for
>>> ARM_CP_STATE_AA32 and ARM_CP_STATE_AA64, something like this:
>>> 
>>>   { .name = "MDCCSR_EL0", .state = ARM_CP_STATE_AA64,
>>>     .opc0 = 2, .opc1 = 3, .crn = 0, .crm = 1, .opc2 = 0,
>>>     .type = ARM_CP_ALIAS,
>>>     .access = PL1_R, .accessfn = access_tda,
>>>     .fieldoffset = offsetof(CPUARMState, cp15.mdscr_el1), },
>>>   { .name = "DBGDSCRint", .state = ARM_CP_STATE_AA32,
>>>     .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 0, .crm = 1, .opc2 = 0,
>>>     .type = ARM_CP_ALIAS,
>>>     .access = PL1_R, .accessfn = access_tda,
>>>     .fieldoffset = offsetoflow32(CPUARMState, cp15.mdscr_el1), },
>>> 
>> 
>> Ah, yes.
>> 
>> As MDCCSR_EL0[30:29] only maps to DBGDSCRint[30:29] maybe it should be all RAZ?
> 
> Well, you can't make it all RAZ, because those 2 bits do still
> need to be mapped, but I guess in theory yes we should define
> read and write accessor functions for AArch64 MDCCSR_EL0 that
> mask out everything except [30:29].

(Apologies if you get this/similar twice - my email is doing strange things)

Hi Peter,

I think the following is acceptable in that qemu doesn’t touch MDSCR_EL1 as far as I can tell.
Perhaps I’m reading the code and the ARM ARM wrong?

    /* MDCCSR_EL0[30:29] map to DBGDSCRint[30:29]. Simply RAZ.
     * We don't implement the configurable EL0 access.
     */
    { .name = "MDCCSR_EL0", .state = ARM_CP_STATE_AA64,
      .cp = 14, .opc0 = 2, .opc1 = 3, .crn = 0, .crm = 1, .opc2 = 0,
      .type = ARM_CP_CONST, .resetvalue = 0 },
    /* DBGDSCRint[15,12,5:2] map to MDSCR_EL1[15,12,5:2] */
    { .name = "DBGDSCRint", .state = ARM_CP_STATE_AA32,
      .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 0, .crm = 1, .opc2 = 0,
      .type = ARM_CP_ALIAS,
      .access = PL1_R, .accessfn = access_tda,
      .fieldoffset = offsetof(CPUARMState, cp15.mdscr_el1), },

Please let me know if you want me to post this (or a different change) as a new diff.

Thanks,
Nick
Peter Maydell July 5, 2021, 12:51 p.m. UTC | #4 
On Fri, 2 Jul 2021 at 16:01, Nick Hudson <hnick@vmware.com> wrote:
> Maybe I’m misreading the ARM ARM and the qemu use of mdscr_el1, but I think
> this is good enough / more correct.  I’m somewhat confused by AA64 MDSCR_EL1
> vs DBGSCRint vs DBGSCRext, however.

Yeah, it is confusing and we generally haven't modeled this very
well, because we've kind of only cared when guests don't work and
some of the purpose of these registers is for external debug
which we don't model at all.

Looking more closely at the Arm ARM:
 * MDSCR_EL1 is the AArch64 register
 * DBGDSCRext is the AArch32 version of that
We model these basically correctly, I think
 * MDCCSR_EL0 is supposed to be an AArch64 read-only register which
has bits [30:29] of EDSCR (ie the JTAG-debugger-view of the TX/RX
connection)
 * DBGDSCRint is similar for AArch32, but it also has various
bits that are read-only views of bits in DBGDSCRext/MDSCR_EL1

Bits [30:29] of MDSCR_EL1 are sort-of-but-not-quite the same
bits as EDSCR [30:29], but they're close enough for our purposes.

>     /* MDCCSR_EL0[30:29] map to DBGDSCRint[30:29]. Simply RAZ.

(QEMU coding style requires "/*" on a line of its own.)

>      * We don't implement the configurable EL0 access.
>      */
>     { .name = "MDCCSR_EL0", .state = ARM_CP_STATE_AA64,
>       .cp = 14, .opc0 = 2, .opc1 = 3, .crn = 0, .crm = 1, .opc2 = 0,
>       .type = ARM_CP_CONST, .resetvalue = 0 },

This seems reasonable; we don't implement the external debug
Debug Communication Channel so we might as well model
RXfull/TXfull as 0. It might be nice to mention in the comment
that the reason we RAZ is because we don't implement the DCC.

>     /* DBGDSCRint[15,12,5:2] map to MDSCR_EL1[15,12,5:2] */
>     { .name = "DBGDSCRint", .state = ARM_CP_STATE_AA32,
>       .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 0, .crm = 1, .opc2 = 0,
>       .type = ARM_CP_ALIAS,
>       .access = PL1_R, .accessfn = access_tda,
>       .fieldoffset = offsetof(CPUARMState, cp15.mdscr_el1), },

Here we're taking advantage of the fact tha MDSCR_EL1[30:29] are
close enough that we can get away with using those. It's not
consistent with how we modelled MDCCSR_EL0's version of those
flags but it's unlikely any guest code will care.

> Please let me know if you want me to submit a new patch.

Yes, please do.

thanks
-- PMM
diff mbox series

Patch

diff --git a/target/arm/helper.c b/target/arm/helper.c
index a66c1f0b9e..7267af7924 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -6330,7 +6330,7 @@  static const ARMCPRegInfo debug_cp_reginfo[] = {
      * We don't implement the configurable EL0 access.
      */
     { .name = "MDCCSR_EL0", .state = ARM_CP_STATE_BOTH,
-      .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 0, .crm = 1, .opc2 = 0,
+      .cp = 14, .opc0 = 2, .opc1 = 3, .crn = 0, .crm = 1, .opc2 = 0,
       .type = ARM_CP_ALIAS,
       .access = PL1_R, .accessfn = access_tda,
       .fieldoffset = offsetof(CPUARMState, cp15.mdscr_el1), },