On 5 October 2015 at 20:56, Davorin Mista <davorin.mi...@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.mi...@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.mi...@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
