[RFC,3/4] Add l2ctlr cp register to CPUARMState

Since the irq bit seems to not be updated, exclude it from the check done
while copying data during migration.

Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com>
---
 target-arm/cpu.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

On 25 February 2014 16:52, Alvise Rigo <a.rigo@virtualopensystems.com> wrote:
> Since the irq bit seems to not be updated, exclude it from the check done
> while copying data during migration.
>
> Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com>
> ---
>  target-arm/cpu.c | 22 ++++++++++++++++++----
>  1 file changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> index 6e7ce89..e8db00e 100644
> --- a/target-arm/cpu.c
> +++ b/target-arm/cpu.c
> @@ -681,20 +681,34 @@ static void cortex_a9_initfn(Object *obj)
>  }
>
>  #ifndef CONFIG_USER_ONLY
> -static uint64_t a15_l2ctlr_read(CPUARMState *env, const ARMCPRegInfo *ri)
> +static void a15_l2ctlr_reset(CPUARMState *env, const ARMCPRegInfo *opaque)
>  {
>      /* Linux wants the number of processors from here.
>       * Might as well set the interrupt-controller bit too.
>       */
> -    return ((smp_cpus - 1) << 24) | (1 << 23);
> +    env->cp15.c9_l2ctlr = ((smp_cpus - 1) << 24) | (1 << 23);
> +}
> +
> +static void a15_l2ctlr_write(CPUARMState *env, const ARMCPRegInfo *ri,
> +                           uint64_t value)
> +{
> +    int smp_cpus_new = ((value >> 24) & 3);
> +    int smp_cpus_old = ((env->cp15.c9_l2ctlr >> 24) & 3);
> +
> +    if (smp_cpus_new != smp_cpus_old) {
> +        return;
> +    }
> +
> +    env->cp15.c9_l2ctlr = value;
>  }
>  #endif
>
>  static const ARMCPRegInfo cortexa15_cp_reginfo[] = {
>  #ifndef CONFIG_USER_ONLY
>      { .name = "L2CTLR", .cp = 15, .crn = 9, .crm = 0, .opc1 = 1, .opc2 = 2,
> -      .access = PL1_RW, .resetvalue = 0, .readfn = a15_l2ctlr_read,
> -      .writefn = arm_cp_write_ignore, },
> +      .access = PL1_RW, .resetvalue = 0,
> +      .resetfn = a15_l2ctlr_reset, .writefn = a15_l2ctlr_write,
> +      .fieldoffset = offsetof(CPUARMState, cp15.c9_l2ctlr) },
>  #endif

Why are you changing the behaviour of this register from
read-only/writes-ignored to permitting writes? This looks
wrong.

thanks
-- PMM

This was a polite way to alter the value of the register in order to not
fail the
migration when KVM migrates a value different from the TCG one.
KVM in particular does not set the irq bit while TCG does.

alvise


On Tue, Feb 25, 2014 at 7:22 PM, Peter Maydell <peter.maydell@linaro.org>wrote:

> On 25 February 2014 16:52, Alvise Rigo <a.rigo@virtualopensystems.com>
> wrote:
> > Since the irq bit seems to not be updated, exclude it from the check done
> > while copying data during migration.
> >
> > Signed-off-by: Alvise Rigo <a.rigo@virtualopensystems.com>
> > ---
> >  target-arm/cpu.c | 22 ++++++++++++++++++----
> >  1 file changed, 18 insertions(+), 4 deletions(-)
> >
> > diff --git a/target-arm/cpu.c b/target-arm/cpu.c
> > index 6e7ce89..e8db00e 100644
> > --- a/target-arm/cpu.c
> > +++ b/target-arm/cpu.c
> > @@ -681,20 +681,34 @@ static void cortex_a9_initfn(Object *obj)
> >  }
> >
> >  #ifndef CONFIG_USER_ONLY
> > -static uint64_t a15_l2ctlr_read(CPUARMState *env, const ARMCPRegInfo
> *ri)
> > +static void a15_l2ctlr_reset(CPUARMState *env, const ARMCPRegInfo
> *opaque)
> >  {
> >      /* Linux wants the number of processors from here.
> >       * Might as well set the interrupt-controller bit too.
> >       */
> > -    return ((smp_cpus - 1) << 24) | (1 << 23);
> > +    env->cp15.c9_l2ctlr = ((smp_cpus - 1) << 24) | (1 << 23);
> > +}
> > +
> > +static void a15_l2ctlr_write(CPUARMState *env, const ARMCPRegInfo *ri,
> > +                           uint64_t value)
> > +{
> > +    int smp_cpus_new = ((value >> 24) & 3);
> > +    int smp_cpus_old = ((env->cp15.c9_l2ctlr >> 24) & 3);
> > +
> > +    if (smp_cpus_new != smp_cpus_old) {
> > +        return;
> > +    }
> > +
> > +    env->cp15.c9_l2ctlr = value;
> >  }
> >  #endif
> >
> >  static const ARMCPRegInfo cortexa15_cp_reginfo[] = {
> >  #ifndef CONFIG_USER_ONLY
> >      { .name = "L2CTLR", .cp = 15, .crn = 9, .crm = 0, .opc1 = 1, .opc2
> = 2,
> > -      .access = PL1_RW, .resetvalue = 0, .readfn = a15_l2ctlr_read,
> > -      .writefn = arm_cp_write_ignore, },
> > +      .access = PL1_RW, .resetvalue = 0,
> > +      .resetfn = a15_l2ctlr_reset, .writefn = a15_l2ctlr_write,
> > +      .fieldoffset = offsetof(CPUARMState, cp15.c9_l2ctlr) },
> >  #endif
>
> Why are you changing the behaviour of this register from
> read-only/writes-ignored to permitting writes? This looks
> wrong.
>
> thanks
> -- PMM
>

On 26 February 2014 09:17, alvise rigo <a.rigo@virtualopensystems.com> wrote:
> This was a polite way to alter the value of the register in order to not
> fail the
> migration when KVM migrates a value different from the TCG one.
> KVM in particular does not set the irq bit while TCG does.

You mean the "interrupt controller" bit 23? That seems like a bug
in the KVM emulation of this register which should be fixed in KVM.
Alternatively if it's a documentation error and the actual hardware
A15 doesn't set this bit (ie if it's not set when you're running outside
KVM) then we should fix the TCG emulation not to set the bit.

thanks
-- PMM