From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54024) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZjDGR-0003yq-5q for qemu-devel@nongnu.org; Mon, 05 Oct 2015 17:25:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZjDGN-0005ST-TG for qemu-devel@nongnu.org; Mon, 05 Oct 2015 17:25:43 -0400 Received: from mail-pa0-f41.google.com ([209.85.220.41]:35607) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZjDGN-0005RL-Jw for qemu-devel@nongnu.org; Mon, 05 Oct 2015 17:25:39 -0400 Received: by pacfv12 with SMTP id fv12so191273324pac.2 for ; Mon, 05 Oct 2015 14:25:38 -0700 (PDT) References: <1444074993-2820-1-git-send-email-davorin.mista@aggios.com> From: Davorin Mista Message-ID: <5612EAD0.1030400@aggios.com> Date: Mon, 5 Oct 2015 14:25:36 -0700 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCHv2] target-arm: Implement AArch64 OSLSR_EL1 sysreg List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: Edgar Iglesias , alistai@xilinx.com, 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 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 > > 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 >> --- >> 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 >