Re: [Qemu-devel] [PATCHv2] target-arm: Implement AArch64 OSLSR_EL1 sysreg

[Davorin Mista]

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 <address@hidden> 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 <address@hidden>
>
> 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 <address@hidden>
> > ---
> >   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