On Mon, Oct 5, 2015 at 2:25 PM, Davorin Mista <davorin.mi...@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.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 >> >
