All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] target/arm: Correct the encoding of MDCCSR_EL0
@ 2021-06-29  8:15 Nick Hudson
  2021-06-29 14:00 ` Richard Henderson
  2021-06-29 14:12 ` Peter Maydell
  0 siblings, 2 replies; 10+ messages in thread
From: Nick Hudson @ 2021-06-29  8:15 UTC (permalink / raw)
  Cc: Peter Maydell, Nick Hudson, open list:ARM TCG CPUs,
	Mohannad Ismail, open list:All patches CC here

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), },
-- 
2.31.1



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] target/arm: Correct the encoding of MDCCSR_EL0
  2021-06-29  8:15 [PATCH] target/arm: Correct the encoding of MDCCSR_EL0 Nick Hudson
@ 2021-06-29 14:00 ` Richard Henderson
  2021-06-29 14:12 ` Peter Maydell
  1 sibling, 0 replies; 10+ messages in thread
From: Richard Henderson @ 2021-06-29 14:00 UTC (permalink / raw)
  To: Nick Hudson
  Cc: Peter Maydell, open list:ARM TCG CPUs, Mohannad Ismail,
	open list:All patches CC here

On 6/29/21 1:15 AM, Nick Hudson wrote:
>       { .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,

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] target/arm: Correct the encoding of MDCCSR_EL0
  2021-06-29  8:15 [PATCH] target/arm: Correct the encoding of MDCCSR_EL0 Nick Hudson
  2021-06-29 14:00 ` Richard Henderson
@ 2021-06-29 14:12 ` Peter Maydell
  1 sibling, 0 replies; 10+ messages in thread
From: Peter Maydell @ 2021-06-29 14:12 UTC (permalink / raw)
  To: Nick Hudson
  Cc: open list:ARM TCG CPUs, Mohannad Ismail, open list:All patches CC here

On Tue, 29 Jun 2021 at 14:03, Nick Hudson <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), },
> --
> 2.31.1

Is this just a repost of the same version you sent previously, or am
I missing something ?

thanks
-- PMM


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] target/arm: Correct the encoding of MDCCSR_EL0
  2021-07-02 15:01       ` Nick Hudson
@ 2021-07-05 12:51         ` Peter Maydell
  0 siblings, 0 replies; 10+ messages in thread
From: Peter Maydell @ 2021-07-05 12:51 UTC (permalink / raw)
  To: Nick Hudson
  Cc: open list:ARM TCG CPUs, Mohannad Ismail, open list:All patches CC here

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] target/arm: Correct the encoding of MDCCSR_EL0
  2021-06-29 11:50     ` Peter Maydell
  2021-07-02 15:01       ` Nick Hudson
@ 2021-07-02 15:01       ` Nick Hudson
  1 sibling, 0 replies; 10+ messages in thread
From: Nick Hudson @ 2021-07-02 15:01 UTC (permalink / raw)
  To: Peter Maydell
  Cc: open list:ARM TCG CPUs, Mohannad Ismail, open list:All patches CC here



> 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] target/arm: Correct the encoding of MDCCSR_EL0
  2021-06-29 11:50     ` Peter Maydell
@ 2021-07-02 15:01       ` Nick Hudson
  2021-07-05 12:51         ` Peter Maydell
  2021-07-02 15:01       ` Nick Hudson
  1 sibling, 1 reply; 10+ messages in thread
From: Nick Hudson @ 2021-07-02 15:01 UTC (permalink / raw)
  To: Peter Maydell
  Cc: open list:ARM TCG CPUs, Mohannad Ismail, open list:All patches CC here



> 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] target/arm: Correct the encoding of MDCCSR_EL0
  2021-06-29 10:41   ` Nick Hudson
@ 2021-06-29 11:50     ` Peter Maydell
  2021-07-02 15:01       ` Nick Hudson
  2021-07-02 15:01       ` Nick Hudson
  0 siblings, 2 replies; 10+ messages in thread
From: Peter Maydell @ 2021-06-29 11:50 UTC (permalink / raw)
  To: Nick Hudson
  Cc: open list:ARM TCG CPUs, Mohannad Ismail, open list:All patches CC here

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].

thanks
-- PMM


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] target/arm: Correct the encoding of MDCCSR_EL0
  2021-06-29  9:49 ` Peter Maydell
@ 2021-06-29 10:41   ` Nick Hudson
  2021-06-29 11:50     ` Peter Maydell
  0 siblings, 1 reply; 10+ messages in thread
From: Nick Hudson @ 2021-06-29 10:41 UTC (permalink / raw)
  To: Peter Maydell
  Cc: open list:ARM TCG CPUs, Mohannad Ismail, open list:All patches CC here



> 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] target/arm: Correct the encoding of MDCCSR_EL0
  2021-06-29  8:27 hnick
@ 2021-06-29  9:49 ` Peter Maydell
  2021-06-29 10:41   ` Nick Hudson
  0 siblings, 1 reply; 10+ messages in thread
From: Peter Maydell @ 2021-06-29  9:49 UTC (permalink / raw)
  To: hnick
  Cc: open list:ARM TCG CPUs, Mohannad Ismail, open list:All patches CC here

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), },

thanks
-- PMM


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH] target/arm: Correct the encoding of MDCCSR_EL0
@ 2021-06-29  8:27 hnick
  2021-06-29  9:49 ` Peter Maydell
  0 siblings, 1 reply; 10+ messages in thread
From: hnick @ 2021-06-29  8:27 UTC (permalink / raw)
  Cc: Peter Maydell, Nick Hudson, open list:ARM TCG CPUs,
	Mohannad Ismail, open list:All patches CC here

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), },
-- 
2.17.1



^ permalink raw reply related	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2021-07-05 12:54 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-29  8:15 [PATCH] target/arm: Correct the encoding of MDCCSR_EL0 Nick Hudson
2021-06-29 14:00 ` Richard Henderson
2021-06-29 14:12 ` Peter Maydell
2021-06-29  8:27 hnick
2021-06-29  9:49 ` Peter Maydell
2021-06-29 10:41   ` Nick Hudson
2021-06-29 11:50     ` Peter Maydell
2021-07-02 15:01       ` Nick Hudson
2021-07-05 12:51         ` Peter Maydell
2021-07-02 15:01       ` Nick Hudson

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.