Re: [Qemu-devel] [PATCH v2] s390: clear registers, psw and prefix at vcpu reset

[Alexander Graf]

On 11/27/2012 06:13 PM, Jens Freimann wrote:
> On Tue, Nov 27, 2012 at 10:37:25AM +0100, Alexander Graf wrote:
> > On 27.11.2012, at 09:33, Jens Freimann<address@hidden>  wrote:
> >
> > > When resetting vcpus on s390/kvm we have to clear registers, psw
> > > and prefix as described in the z/Architecture PoP, otherwise a
> > > reboot won't work. IPL PSW and prefix are set later on by the
> > > s390-ipl device reset code.
> > >
> > > Signed-off-by: Jens Freimann<address@hidden>
> > > ---
> > >
> > > changes v1->v2:
> > > - moved cpu reset code from kvm.c to cpu.c
> > > - only kvm initial_reset ioctl remains in kvm.c
> > > - registered reset handler for s390 cpu reset, like x86 does it
> > >
> > > target-s390x/cpu.c | 29 +++++++++++++++++++++++++++--
> > > target-s390x/kvm.c |  9 ++++++++-
> > > 2 files changed, 35 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
> > > index 619b202..f74d6f6 100644
> > > --- a/target-s390x/cpu.c
> > > +++ b/target-s390x/cpu.c
> > > @@ -24,10 +24,21 @@
> > > #include "qemu-common.h"
> > > #include "qemu-timer.h"
> > >
> > > +#ifndef CONFIG_USER_ONLY
> > > +#include "hw/s390x/sclp.h"
> > > +
> > > +/* TODO: remove me, when reset over QOM tree is implemented */
> > > +static void s390_cpu_machine_reset_cb(void *opaque)
> > > +{
> > > +    S390CPU *cpu = opaque;
> > > +    cpu_reset(CPU(cpu));
> > > +}
> > > +#endif
> > >
> > > /* CPUClass::reset() */
> > > static void s390_cpu_reset(CPUState *s)
> > > {
> > > +    int i;
> > >      S390CPU *cpu = S390_CPU(s);
> > >      S390CPUClass *scc = S390_CPU_GET_CLASS(cpu);
> > >      CPUS390XState *env =&cpu->env;
> > > @@ -40,9 +51,22 @@ static void s390_cpu_reset(CPUState *s)
> > >      scc->parent_reset(s);
> > >
> > >      memset(env, 0, offsetof(CPUS390XState, breakpoints));
> > > -    /* FIXME: reset vector? */
> > > +
> > > +    env->halted = 1;
> > Every cpu would start in halted state? So how does the primary one get rolling?
> The first cpu is set to not-halted by the ipl device reset code.

Please document this here.

> > > +    env->exception_index = EXCP_HLT;
> > > +    for (i = 0; i<  16; i++) {
> > > +        env->regs[i] = 0;
> > > +        env->aregs[i] = 0;
> > > +        env->cregs[i] = 0;
> > > +        env->fregs[i].ll = 0;
> > > +    }
> > Please make this more self-adjusting. For example using memset(sizeof));. You 
> > could also make the clear implicit by ensuring the registers are in the cpu 
> > struct before breakpoints. But explicit tends to be more readable ;).
> Ok
>
> > > +    /* architectured initial values for CR 0 and 14 */
> > > +    env->cregs[0] = 0xE0UL;
> > > +    env->cregs[14] = 0xC2000000UL;
> > > +    env->psw.mask = 0;
> > > +    env->psw.addr = 0;
> > > +    env->psa = 0;
> > >      tlb_flush(env, 1);
> > > -    s390_add_running_cpu(env);
> > Why can we remove this one?
> Good point. I took a closer look and found that we add an additional
> cpu to the counter every time we reboot. Will fix this and send a new
> version.

Yeah, if anything this should be a separate patch :).


Alex