[PATCHv2] target-arm: Implement AArch64 OSLSR_EL1 sysreg

Added oslsr_write function to OSLAR_EL1 sysreg, using a status variable 
in ARMCPUState struct (os_lock_status).

Linux reads from this register during its suspend/resume procedure.

Signed-off-by: Davorin Mista <davorin.mista@aggios.com>

---
Changed in v2:
-switched from using dummy registers to an actual register implementation
-implemented write function for OSLAR_EL1 sysreg
-added state variable to ARMCPUState struct

Signed-off-by: Davorin Mista <davorin.mista@aggios.com>
---
 target-arm/cpu.h    |  3 +++
 target-arm/helper.c | 16 +++++++++++++++-
 2 files changed, 18 insertions(+), 1 deletion(-)

On 5 October 2015 at 20:56, Davorin Mista <davorin.mista@aggios.com> wrote:
> Added oslsr_write function to OSLAR_EL1 sysreg, using a status variable
> in ARMCPUState struct (os_lock_status).
>
> Linux reads from this register during its suspend/resume procedure.
>
> Signed-off-by: Davorin Mista <davorin.mista@aggios.com>

Thanks for this patch. I'm afraid it still needs some changes;
comments below.

> ---
> Changed in v2:
> -switched from using dummy registers to an actual register implementation
> -implemented write function for OSLAR_EL1 sysreg
> -added state variable to ARMCPUState struct
>
> Signed-off-by: Davorin Mista <davorin.mista@aggios.com>
> ---
>  target-arm/cpu.h    |  3 +++
>  target-arm/helper.c | 16 +++++++++++++++-
>  2 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index 5ea11a6..5aab654 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -500,6 +500,9 @@ typedef struct CPUARMState {
>          uint32_t cregs[16];
>      } iwmmxt;
>
> +    /* OS Lock Status: accessed via OSLAR/OSLSR registers */
> +    uint64_t os_lock_status;

Can you call this "oslsr_el1" and put it inside the cp15 substruct
with the other sysreg fields, please?

> +
>      /* For mixed endian mode.  */
>      bool bswap_code;
>
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index 9d62c4c..a6fad7a 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -3147,6 +3147,13 @@ static void dcc_write(CPUARMState *env, const ARMCPRegInfo *ri,
>      putchar(value);
>  }
>
> +/* write to os_lock_status state variable */
> +static void oslsr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
> +{
> +    /* only bit 1 can be modified, register is always 0b10x0 */
> +    raw_write(env, ri, 8 + (value & 2));

This is the write function for OSLAR, not OSLSR, so you should
call it oslar_write.

Your logic isn't implementing the behaviour the ARM ARM requires,
which is:
 * for AArch64 accesses, copy bit 0 of the written value into
   bit 1 of oslsr_el1
 * for AArch32 accesses, if the written value is 0xC5ACCE55
   then write 1 into bit 1 of oslsr_el1, else write 0

That looks something like:

    int oslock;

    if (ri->state == ARM_CP_STATE_AA32) {
        oslock = (value == 0xC5ACCE55);
    } else {
        oslock = value & 1;
    }

    env->cp15.oslsr_el1 = deposit32(env->cp15.oslsr_el1, 1, 1, oslock);


> +}
> +
>  static const ARMCPRegInfo debug_cp_reginfo[] = {
>      /* DBGDRAR, DBGDSAR: always RAZ since we don't implement memory mapped
>       * debug components. The AArch64 version of DBGDRAR is named MDRAR_EL1;
> @@ -3179,7 +3186,14 @@ static const ARMCPRegInfo debug_cp_reginfo[] = {
>      /* We define a dummy WI OSLAR_EL1, because Linux writes to it. */
>      { .name = "OSLAR_EL1", .state = ARM_CP_STATE_BOTH,
>        .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 1, .crm = 0, .opc2 = 4,
> -      .access = PL1_W, .type = ARM_CP_NOP },
> +      .access = PL1_W, .resetvalue = 10,

Write only registers don't need a reset value. You also
need .type = ARM_CP_NO_RAW, because a raw access to this register
doesn't make sense.

> +      .fieldoffset = offsetof(CPUARMState, os_lock_status),
> +      .writefn = oslsr_write },
> +    /* We define a dummy OSLSR_EL1, because Linux reads from it. */
> +    { .name = "OSLSR_EL1", .state = ARM_CP_STATE_BOTH,
> +      .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 1, .crm = 1, .opc2 = 4,
> +      .access = PL1_R,
> +      .fieldoffset = offsetof(CPUARMState, os_lock_status) },

This is the reginfo that should have the reset value.

>      /* Dummy OSDLR_EL1: 32-bit Linux will read this */
>      { .name = "OSDLR_EL1", .state = ARM_CP_STATE_BOTH,
>        .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 1, .crm = 3, .opc2 = 4,
> --
> 2.6.0
>

thanks
-- PMM

Thanks Peter, I've made all changes as you suggested, but there is no 
property "ARM_CP_NO_RAW", there's also nothing similar to it defined in 
cpu.h, here's all the options:

#define ARM_CP_SPECIAL 1
#define ARM_CP_CONST 2
#define ARM_CP_64BIT 4
#define ARM_CP_SUPPRESS_TB_END 8
#define ARM_CP_OVERRIDE 16
#define ARM_CP_NO_MIGRATE 32
#define ARM_CP_IO 64

Cheers,
Davorin

On 10/05/2015 01:27 PM, Peter Maydell wrote:
> On 5 October 2015 at 20:56, Davorin Mista <davorin.mista@aggios.com> wrote:
>> Added oslsr_write function to OSLAR_EL1 sysreg, using a status variable
>> in ARMCPUState struct (os_lock_status).
>>
>> Linux reads from this register during its suspend/resume procedure.
>>
>> Signed-off-by: Davorin Mista <davorin.mista@aggios.com>
>
> Thanks for this patch. I'm afraid it still needs some changes;
> comments below.
>
>> ---
>> Changed in v2:
>> -switched from using dummy registers to an actual register implementation
>> -implemented write function for OSLAR_EL1 sysreg
>> -added state variable to ARMCPUState struct
>>
>> Signed-off-by: Davorin Mista <davorin.mista@aggios.com>
>> ---
>>   target-arm/cpu.h    |  3 +++
>>   target-arm/helper.c | 16 +++++++++++++++-
>>   2 files changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>> index 5ea11a6..5aab654 100644
>> --- a/target-arm/cpu.h
>> +++ b/target-arm/cpu.h
>> @@ -500,6 +500,9 @@ typedef struct CPUARMState {
>>           uint32_t cregs[16];
>>       } iwmmxt;
>>
>> +    /* OS Lock Status: accessed via OSLAR/OSLSR registers */
>> +    uint64_t os_lock_status;
>
> Can you call this "oslsr_el1" and put it inside the cp15 substruct
> with the other sysreg fields, please?
>
>> +
>>       /* For mixed endian mode.  */
>>       bool bswap_code;
>>
>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>> index 9d62c4c..a6fad7a 100644
>> --- a/target-arm/helper.c
>> +++ b/target-arm/helper.c
>> @@ -3147,6 +3147,13 @@ static void dcc_write(CPUARMState *env, const ARMCPRegInfo *ri,
>>       putchar(value);
>>   }
>>
>> +/* write to os_lock_status state variable */
>> +static void oslsr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
>> +{
>> +    /* only bit 1 can be modified, register is always 0b10x0 */
>> +    raw_write(env, ri, 8 + (value & 2));
>
> This is the write function for OSLAR, not OSLSR, so you should
> call it oslar_write.
>
> Your logic isn't implementing the behaviour the ARM ARM requires,
> which is:
>   * for AArch64 accesses, copy bit 0 of the written value into
>     bit 1 of oslsr_el1
>   * for AArch32 accesses, if the written value is 0xC5ACCE55
>     then write 1 into bit 1 of oslsr_el1, else write 0
>
> That looks something like:
>
>      int oslock;
>
>      if (ri->state == ARM_CP_STATE_AA32) {
>          oslock = (value == 0xC5ACCE55);
>      } else {
>          oslock = value & 1;
>      }
>
>      env->cp15.oslsr_el1 = deposit32(env->cp15.oslsr_el1, 1, 1, oslock);
>
>
>> +}
>> +
>>   static const ARMCPRegInfo debug_cp_reginfo[] = {
>>       /* DBGDRAR, DBGDSAR: always RAZ since we don't implement memory mapped
>>        * debug components. The AArch64 version of DBGDRAR is named MDRAR_EL1;
>> @@ -3179,7 +3186,14 @@ static const ARMCPRegInfo debug_cp_reginfo[] = {
>>       /* We define a dummy WI OSLAR_EL1, because Linux writes to it. */
>>       { .name = "OSLAR_EL1", .state = ARM_CP_STATE_BOTH,
>>         .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 1, .crm = 0, .opc2 = 4,
>> -      .access = PL1_W, .type = ARM_CP_NOP },
>> +      .access = PL1_W, .resetvalue = 10,
>
> Write only registers don't need a reset value. You also
> need .type = ARM_CP_NO_RAW, because a raw access to this register
> doesn't make sense.
>
>> +      .fieldoffset = offsetof(CPUARMState, os_lock_status),
>> +      .writefn = oslsr_write },
>> +    /* We define a dummy OSLSR_EL1, because Linux reads from it. */
>> +    { .name = "OSLSR_EL1", .state = ARM_CP_STATE_BOTH,
>> +      .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 1, .crm = 1, .opc2 = 4,
>> +      .access = PL1_R,
>> +      .fieldoffset = offsetof(CPUARMState, os_lock_status) },
>
> This is the reginfo that should have the reset value.
>
>>       /* Dummy OSDLR_EL1: 32-bit Linux will read this */
>>       { .name = "OSDLR_EL1", .state = ARM_CP_STATE_BOTH,
>>         .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 1, .crm = 3, .opc2 = 4,
>> --
>> 2.6.0
>>
>
> thanks
> -- PMM
>

On Mon, Oct 5, 2015 at 2:25 PM, Davorin Mista <davorin.mista@aggios.com> wrote:
> Thanks Peter, I've made all changes as you suggested, but there is no
> property "ARM_CP_NO_RAW", there's also nothing similar to it defined in
> cpu.h, here's all the options:
>
> #define ARM_CP_SPECIAL 1
> #define ARM_CP_CONST 2
> #define ARM_CP_64BIT 4
> #define ARM_CP_SUPPRESS_TB_END 8
> #define ARM_CP_OVERRIDE 16
> #define ARM_CP_NO_MIGRATE 32
> #define ARM_CP_IO 64

Hello Davorin,


There is an ARM_CP_NO_RAW option
(https://github.com/qemu/qemu/blob/master/target-arm/cpu.h#L1154). It
looks like you are applying this on the Xilinx tree (or another out of
date tree) instead of mainline.

Thanks,

Alistair

>
> Cheers,
> Davorin
>
>
> On 10/05/2015 01:27 PM, Peter Maydell wrote:
>>
>> On 5 October 2015 at 20:56, Davorin Mista <davorin.mista@aggios.com>
>> wrote:
>>>
>>> Added oslsr_write function to OSLAR_EL1 sysreg, using a status variable
>>> in ARMCPUState struct (os_lock_status).
>>>
>>> Linux reads from this register during its suspend/resume procedure.
>>>
>>> Signed-off-by: Davorin Mista <davorin.mista@aggios.com>
>>
>>
>> Thanks for this patch. I'm afraid it still needs some changes;
>> comments below.
>>
>>> ---
>>> Changed in v2:
>>> -switched from using dummy registers to an actual register implementation
>>> -implemented write function for OSLAR_EL1 sysreg
>>> -added state variable to ARMCPUState struct
>>>
>>> Signed-off-by: Davorin Mista <davorin.mista@aggios.com>
>>> ---
>>>   target-arm/cpu.h    |  3 +++
>>>   target-arm/helper.c | 16 +++++++++++++++-
>>>   2 files changed, 18 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
>>> index 5ea11a6..5aab654 100644
>>> --- a/target-arm/cpu.h
>>> +++ b/target-arm/cpu.h
>>> @@ -500,6 +500,9 @@ typedef struct CPUARMState {
>>>           uint32_t cregs[16];
>>>       } iwmmxt;
>>>
>>> +    /* OS Lock Status: accessed via OSLAR/OSLSR registers */
>>> +    uint64_t os_lock_status;
>>
>>
>> Can you call this "oslsr_el1" and put it inside the cp15 substruct
>> with the other sysreg fields, please?
>>
>>> +
>>>       /* For mixed endian mode.  */
>>>       bool bswap_code;
>>>
>>> diff --git a/target-arm/helper.c b/target-arm/helper.c
>>> index 9d62c4c..a6fad7a 100644
>>> --- a/target-arm/helper.c
>>> +++ b/target-arm/helper.c
>>> @@ -3147,6 +3147,13 @@ static void dcc_write(CPUARMState *env, const
>>> ARMCPRegInfo *ri,
>>>       putchar(value);
>>>   }
>>>
>>> +/* write to os_lock_status state variable */
>>> +static void oslsr_write(CPUARMState *env, const ARMCPRegInfo *ri,
>>> uint64_t value)
>>> +{
>>> +    /* only bit 1 can be modified, register is always 0b10x0 */
>>> +    raw_write(env, ri, 8 + (value & 2));
>>
>>
>> This is the write function for OSLAR, not OSLSR, so you should
>> call it oslar_write.
>>
>> Your logic isn't implementing the behaviour the ARM ARM requires,
>> which is:
>>   * for AArch64 accesses, copy bit 0 of the written value into
>>     bit 1 of oslsr_el1
>>   * for AArch32 accesses, if the written value is 0xC5ACCE55
>>     then write 1 into bit 1 of oslsr_el1, else write 0
>>
>> That looks something like:
>>
>>      int oslock;
>>
>>      if (ri->state == ARM_CP_STATE_AA32) {
>>          oslock = (value == 0xC5ACCE55);
>>      } else {
>>          oslock = value & 1;
>>      }
>>
>>      env->cp15.oslsr_el1 = deposit32(env->cp15.oslsr_el1, 1, 1, oslock);
>>
>>
>>> +}
>>> +
>>>   static const ARMCPRegInfo debug_cp_reginfo[] = {
>>>       /* DBGDRAR, DBGDSAR: always RAZ since we don't implement memory
>>> mapped
>>>        * debug components. The AArch64 version of DBGDRAR is named
>>> MDRAR_EL1;
>>> @@ -3179,7 +3186,14 @@ static const ARMCPRegInfo debug_cp_reginfo[] = {
>>>       /* We define a dummy WI OSLAR_EL1, because Linux writes to it. */
>>>       { .name = "OSLAR_EL1", .state = ARM_CP_STATE_BOTH,
>>>         .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 1, .crm = 0, .opc2 = 4,
>>> -      .access = PL1_W, .type = ARM_CP_NOP },
>>> +      .access = PL1_W, .resetvalue = 10,
>>
>>
>> Write only registers don't need a reset value. You also
>> need .type = ARM_CP_NO_RAW, because a raw access to this register
>> doesn't make sense.
>>
>>> +      .fieldoffset = offsetof(CPUARMState, os_lock_status),
>>> +      .writefn = oslsr_write },
>>> +    /* We define a dummy OSLSR_EL1, because Linux reads from it. */
>>> +    { .name = "OSLSR_EL1", .state = ARM_CP_STATE_BOTH,
>>> +      .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 1, .crm = 1, .opc2 = 4,
>>> +      .access = PL1_R,
>>> +      .fieldoffset = offsetof(CPUARMState, os_lock_status) },
>>
>>
>> This is the reginfo that should have the reset value.
>>
>>>       /* Dummy OSDLR_EL1: 32-bit Linux will read this */
>>>       { .name = "OSDLR_EL1", .state = ARM_CP_STATE_BOTH,
>>>         .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 1, .crm = 3, .opc2 = 4,
>>> --
>>> 2.6.0
>>>
>>
>> thanks
>> -- PMM
>>
>