[Qemu-devel] [PATCH v2] target-arm: Add MDCR_EL2

[Qemu-devel] [PATCH v2] target-arm: Add MDCR_EL2

[Sergey Fedorov]

Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com>
---

Changes in v2:
 * Reset value is simply made zero

 target-arm/cpu.h    |  1 +
 target-arm/helper.c | 11 +++++++++++
 2 files changed, 12 insertions(+)

diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index cc1578c..51d8ed1 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -378,6 +378,7 @@ typedef struct CPUARMState {
         uint64_t dbgwvr[16]; /* watchpoint value registers */
         uint64_t dbgwcr[16]; /* watchpoint control registers */
         uint64_t mdscr_el1;
+        uint64_t mdcr_el2;
         /* If the counter is enabled, this stores the last time the counter
          * was reset. Otherwise it stores the counter value
          */
diff --git a/target-arm/helper.c b/target-arm/helper.c
index 8367997..ec4097b 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -3223,6 +3223,9 @@ static const ARMCPRegInfo el3_no_el2_cp_reginfo[] = {
     { .name = "CNTHP_CTL_EL2", .state = ARM_CP_STATE_BOTH,
       .opc0 = 3, .opc1 = 4, .crn = 14, .crm = 2, .opc2 = 1,
       .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
+    { .name = "MDCR_EL2", .state = ARM_CP_STATE_BOTH,
+      .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 1,
+      .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
     REGINFO_SENTINEL
 };
 
@@ -3576,6 +3579,14 @@ static const ARMCPRegInfo debug_cp_reginfo[] = {
     { .name = "MDRAR_EL1", .state = ARM_CP_STATE_AA64,
       .opc0 = 2, .opc1 = 0, .crn = 1, .crm = 0, .opc2 = 0,
       .access = PL1_R, .type = ARM_CP_CONST, .resetvalue = 0 },
+    /* The only field of MDCR_EL2 that has a defined architectural reset value
+     * is MDCR_EL2.HPMN which should reset to the value of PMCR_EL0.N; but we
+     * don't impelment any PMU event counters, so using zero as a reset
+     * value for MDCR_EL2 is okay */
+    { .name = "MDCR_EL2", .state = ARM_CP_STATE_BOTH,
+      .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 1,
+      .access = PL2_RW, .resetvalue = 0,
+      .fieldoffset = offsetof(CPUARMState, cp15.mdcr_el2), },
     { .name = "DBGDSAR", .cp = 14, .crn = 2, .crm = 0, .opc1 = 0, .opc2 = 0,
       .access = PL0_R, .type = ARM_CP_CONST, .resetvalue = 0 },
     /* Monitor debug system control register; the 32-bit alias is DBGDSCRext. */
-- 
1.9.1

Re: [Qemu-devel] [PATCH v2] target-arm: Add MDCR_EL2

[Peter Maydell]

On 9 October 2015 at 10:43, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
> Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com>
> ---
>
> Changes in v2:
>  * Reset value is simply made zero
>
>  target-arm/cpu.h    |  1 +
>  target-arm/helper.c | 11 +++++++++++
>  2 files changed, 12 insertions(+)
>

Applied to target-arm.next, thanks (I expanded the commit
message a little).

-- PMM

Re: [Qemu-devel] [PATCH v2] target-arm: Add MDCR_EL2

[Sergey Fedorov]

On 13.10.2015 20:27, Peter Maydell wrote:
> On 9 October 2015 at 10:43, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
>> Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com>
>> ---
>>
>> Changes in v2:
>>  * Reset value is simply made zero
>>
>>  target-arm/cpu.h    |  1 +
>>  target-arm/helper.c | 11 +++++++++++
>>  2 files changed, 12 insertions(+)
>>
> Applied to target-arm.next, thanks (I expanded the commit
> message a little).

Thanks! So now you are going to re-apply
https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg04862.html again?

Best regards,
Sergey

Re: [Qemu-devel] [PATCH v2] target-arm: Add MDCR_EL2

[Peter Maydell]

On 14 October 2015 at 10:35, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
> On 13.10.2015 20:27, Peter Maydell wrote:
>> On 9 October 2015 at 10:43, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
>>> Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com>
>>> ---
>>>
>>> Changes in v2:
>>>  * Reset value is simply made zero
>>>
>>>  target-arm/cpu.h    |  1 +
>>>  target-arm/helper.c | 11 +++++++++++
>>>  2 files changed, 12 insertions(+)
>>>
>> Applied to target-arm.next, thanks (I expanded the commit
>> message a little).
>
> Thanks! So now you are going to re-apply
> https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg04862.html again?

I was expecting a resend, but if it applies without changes
I'll put it back on my list.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2] target-arm: Add MDCR_EL2

[Sergey Fedorov]

On 14.10.2015 22:34, Peter Maydell wrote:
> On 14 October 2015 at 10:35, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
>> On 13.10.2015 20:27, Peter Maydell wrote:
>>> On 9 October 2015 at 10:43, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
>>>> Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com>
>>>> ---
>>>>
>>>> Changes in v2:
>>>>  * Reset value is simply made zero
>>>>
>>>>  target-arm/cpu.h    |  1 +
>>>>  target-arm/helper.c | 11 +++++++++++
>>>>  2 files changed, 12 insertions(+)
>>>>
>>> Applied to target-arm.next, thanks (I expanded the commit
>>> message a little).
>> Thanks! So now you are going to re-apply
>> https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg04862.html again?
> I was expecting a resend, but if it applies without changes
> I'll put it back on my list.

Yes, it applies without a change, so no need to resend :)

Best regards,
Sergey

Re: [Qemu-devel] [PATCH v2] target-arm: Add MDCR_EL2

[Peter Maydell]

On 9 October 2015 at 10:43, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
> Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com>
> ---
>
> Changes in v2:
>  * Reset value is simply made zero
>
>  target-arm/cpu.h    |  1 +
>  target-arm/helper.c | 11 +++++++++++
>  2 files changed, 12 insertions(+)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index cc1578c..51d8ed1 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -378,6 +378,7 @@ typedef struct CPUARMState {
>          uint64_t dbgwvr[16]; /* watchpoint value registers */
>          uint64_t dbgwcr[16]; /* watchpoint control registers */
>          uint64_t mdscr_el1;
> +        uint64_t mdcr_el2;
>          /* If the counter is enabled, this stores the last time the counter
>           * was reset. Otherwise it stores the counter value
>           */
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 8367997..ec4097b 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -3223,6 +3223,9 @@ static const ARMCPRegInfo el3_no_el2_cp_reginfo[] = {
>      { .name = "CNTHP_CTL_EL2", .state = ARM_CP_STATE_BOTH,
>        .opc0 = 3, .opc1 = 4, .crn = 14, .crm = 2, .opc2 = 1,
>        .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
> +    { .name = "MDCR_EL2", .state = ARM_CP_STATE_BOTH,
> +      .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 1,
> +      .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
>      REGINFO_SENTINEL
>  };
>
> @@ -3576,6 +3579,14 @@ static const ARMCPRegInfo debug_cp_reginfo[] = {
>      { .name = "MDRAR_EL1", .state = ARM_CP_STATE_AA64,
>        .opc0 = 2, .opc1 = 0, .crn = 1, .crm = 0, .opc2 = 0,
>        .access = PL1_R, .type = ARM_CP_CONST, .resetvalue = 0 },
> +    /* The only field of MDCR_EL2 that has a defined architectural reset value
> +     * is MDCR_EL2.HPMN which should reset to the value of PMCR_EL0.N; but we
> +     * don't impelment any PMU event counters, so using zero as a reset
> +     * value for MDCR_EL2 is okay */
> +    { .name = "MDCR_EL2", .state = ARM_CP_STATE_BOTH,
> +      .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 1,
> +      .access = PL2_RW, .resetvalue = 0,
> +      .fieldoffset = offsetof(CPUARMState, cp15.mdcr_el2), },
>      { .name = "DBGDSAR", .cp = 14, .crn = 2, .crm = 0, .opc1 = 0, .opc2 = 0,
>        .access = PL0_R, .type = ARM_CP_CONST, .resetvalue = 0 },
>      /* Monitor debug system control register; the 32-bit alias is DBGDSCRext. */

This fails 'make check':

GTESTER check-qtest-arm
Register redefined: cp=15 32 bit crn=1 crm=1 opc1=4 opc2=1, was
MDCR_EL2, now MDCR_EL2
**
ERROR:/home/petmay01/linaro/qemu-from-laptop/qemu/target-arm/helper.c:4740:add_cpreg_to_hashtable:
code should not be reached

The second MDCR_EL2 definition needs to be in el2_cp_reginfo[],
not debug_cp_reginfo[]; otherwise a CPU with EL2 but not EL2
will try to define both.

I will squash in the change which moves the definition.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2] target-arm: Add MDCR_EL2

[Sergey Fedorov]

On 16.10.2015 15:34, Peter Maydell wrote:
> On 9 October 2015 at 10:43, Sergey Fedorov <serge.fdrv@gmail.com> wrote:
>> Signed-off-by: Sergey Fedorov <serge.fdrv@gmail.com>
>> ---
>>
>> Changes in v2:
>>  * Reset value is simply made zero
>>
>>  target-arm/cpu.h    |  1 +
>>  target-arm/helper.c | 11 +++++++++++
>>  2 files changed, 12 insertions(+)
>>
>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>> index cc1578c..51d8ed1 100644
>> --- a/target-arm/cpu.h
>> +++ b/target-arm/cpu.h
>> @@ -378,6 +378,7 @@ typedef struct CPUARMState {
>>          uint64_t dbgwvr[16]; /* watchpoint value registers */
>>          uint64_t dbgwcr[16]; /* watchpoint control registers */
>>          uint64_t mdscr_el1;
>> +        uint64_t mdcr_el2;
>>          /* If the counter is enabled, this stores the last time the counter
>>           * was reset. Otherwise it stores the counter value
>>           */
>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>> index 8367997..ec4097b 100644
>> --- a/target-arm/helper.c
>> +++ b/target-arm/helper.c
>> @@ -3223,6 +3223,9 @@ static const ARMCPRegInfo el3_no_el2_cp_reginfo[] = {
>>      { .name = "CNTHP_CTL_EL2", .state = ARM_CP_STATE_BOTH,
>>        .opc0 = 3, .opc1 = 4, .crn = 14, .crm = 2, .opc2 = 1,
>>        .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
>> +    { .name = "MDCR_EL2", .state = ARM_CP_STATE_BOTH,
>> +      .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 1,
>> +      .access = PL2_RW, .type = ARM_CP_CONST, .resetvalue = 0 },
>>      REGINFO_SENTINEL
>>  };
>>
>> @@ -3576,6 +3579,14 @@ static const ARMCPRegInfo debug_cp_reginfo[] = {
>>      { .name = "MDRAR_EL1", .state = ARM_CP_STATE_AA64,
>>        .opc0 = 2, .opc1 = 0, .crn = 1, .crm = 0, .opc2 = 0,
>>        .access = PL1_R, .type = ARM_CP_CONST, .resetvalue = 0 },
>> +    /* The only field of MDCR_EL2 that has a defined architectural reset value
>> +     * is MDCR_EL2.HPMN which should reset to the value of PMCR_EL0.N; but we
>> +     * don't impelment any PMU event counters, so using zero as a reset
>> +     * value for MDCR_EL2 is okay */
>> +    { .name = "MDCR_EL2", .state = ARM_CP_STATE_BOTH,
>> +      .opc0 = 3, .opc1 = 4, .crn = 1, .crm = 1, .opc2 = 1,
>> +      .access = PL2_RW, .resetvalue = 0,
>> +      .fieldoffset = offsetof(CPUARMState, cp15.mdcr_el2), },
>>      { .name = "DBGDSAR", .cp = 14, .crn = 2, .crm = 0, .opc1 = 0, .opc2 = 0,
>>        .access = PL0_R, .type = ARM_CP_CONST, .resetvalue = 0 },
>>      /* Monitor debug system control register; the 32-bit alias is DBGDSCRext. */
> This fails 'make check':
>
> GTESTER check-qtest-arm
> Register redefined: cp=15 32 bit crn=1 crm=1 opc1=4 opc2=1, was
> MDCR_EL2, now MDCR_EL2
> **
> ERROR:/home/petmay01/linaro/qemu-from-laptop/qemu/target-arm/helper.c:4740:add_cpreg_to_hashtable:
> code should not be reached
>
> The second MDCR_EL2 definition needs to be in el2_cp_reginfo[],
> not debug_cp_reginfo[]; otherwise a CPU with EL2 but not EL2
> will try to define both.
>
> I will squash in the change which moves the definition.

Oops... I should make it a rule and do 'make check' before submitting
changes.

Thanks,
Sergey