* [Qemu-devel] [PATCHv2] target-arm: Implement AArch64 OSLSR_EL1 sysreg @ 2015-10-05 19:56 Davorin Mista 2015-10-05 20:27 ` Peter Maydell 0 siblings, 1 reply; 4+ messages in thread From: Davorin Mista @ 2015-10-05 19:56 UTC (permalink / raw) To: qemu-devel; +Cc: Davorin Mista, peter.maydell, edgari, alistai, sorenb 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(-) 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; + /* 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)); +} + 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, + .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) }, /* 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCHv2] target-arm: Implement AArch64 OSLSR_EL1 sysreg 2015-10-05 19:56 [Qemu-devel] [PATCHv2] target-arm: Implement AArch64 OSLSR_EL1 sysreg Davorin Mista @ 2015-10-05 20:27 ` Peter Maydell 2015-10-05 21:25 ` Davorin Mista 0 siblings, 1 reply; 4+ messages in thread From: Peter Maydell @ 2015-10-05 20:27 UTC (permalink / raw) To: Davorin Mista; +Cc: Edgar Iglesias, alistai, QEMU Developers, Soren Brinkmann 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCHv2] target-arm: Implement AArch64 OSLSR_EL1 sysreg 2015-10-05 20:27 ` Peter Maydell @ 2015-10-05 21:25 ` Davorin Mista 2015-10-05 21:36 ` Alistair Francis 0 siblings, 1 reply; 4+ messages in thread From: Davorin Mista @ 2015-10-05 21:25 UTC (permalink / raw) To: Peter Maydell; +Cc: Edgar Iglesias, alistai, QEMU Developers, Soren Brinkmann 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 > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCHv2] target-arm: Implement AArch64 OSLSR_EL1 sysreg 2015-10-05 21:25 ` Davorin Mista @ 2015-10-05 21:36 ` Alistair Francis 0 siblings, 0 replies; 4+ messages in thread From: Alistair Francis @ 2015-10-05 21:36 UTC (permalink / raw) To: Davorin Mista Cc: Peter Maydell, Edgar Iglesias, Alistair Francis, QEMU Developers, Soren Brinkmann 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 >> > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-10-05 21:37 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-10-05 19:56 [Qemu-devel] [PATCHv2] target-arm: Implement AArch64 OSLSR_EL1 sysreg Davorin Mista 2015-10-05 20:27 ` Peter Maydell 2015-10-05 21:25 ` Davorin Mista 2015-10-05 21:36 ` Alistair Francis
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).