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

[Alexander Graf <agraf@suse.de>]

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<jfrei@linux.vnet.ibm.com>  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<jfrei@linux.vnet.ibm.com>
>>> ---
>>>
>>> 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