Re: [Qemu-devel] [PATCH 2/3] s390: Add CPU reset handler

[Alexander Graf]

On 04.01.2013, at 15:09, Jens Freimann wrote:

> On Thu, Jan 03, 2013 at 01:55:19PM +0100, Alexander Graf wrote:
>> 
>> On 18.12.2012, at 18:50, Jens Freimann wrote:
>> 
>>> Add a CPU reset handler to have all CPUs in a PoP compliant
>>> state.
>>> 
>>> Signed-off-by: Jens Freimann <address@hidden>
>>> 
>>> ---
>>> v2 -> v3:
>>> * remove FIXME
>>> * separate parent reset from local reset by adding a while line
>>> * use defines for register reset values
>>> 
>>> v1 -> v2:
>>> * move setting of control registers and psa to s390_cpu_reset
>>> and call it from the new s390_machine_cpu_reset_cb()
>>> This makes it more similar to how it is done on x86
>>> * in s390_cpu_reset() set env->halted state of cpu after
>>> the memset. This is needed to keep our s390_cpu_running
>>> counter in sync when s390_cpu_reset is called via the
>>> qemu_devices_reset path
>>> * set env->halted state in s390_cpu_initfn to 1 to avoid
>>> decrementing the cpu counter during first reset
>>> ---
>>> target-s390x/cpu.c | 35 +++++++++++++++++++++++++++++++++--
>>> target-s390x/kvm.c |  9 ++++++++-
>>> 2 files changed, 41 insertions(+), 3 deletions(-)
>>> 
>>> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
>>> index 619b202..58e412a 100644
>>> --- a/target-s390x/cpu.c
>>> +++ b/target-s390x/cpu.c
>>> @@ -4,6 +4,7 @@
>>> * Copyright (c) 2009 Ulrich Hecht
>>> * Copyright (c) 2011 Alexander Graf
>>> * Copyright (c) 2012 SUSE LINUX Products GmbH
>>> + * Copyright (c) 2012 IBM Corp.
>>> *
>>> * This library is free software; you can redistribute it and/or
>>> * modify it under the terms of the GNU Lesser General Public
>>> @@ -18,12 +19,19 @@
>>> * You should have received a copy of the GNU Lesser General Public
>>> * License along with this library; if not, see
>>> * <http://www.gnu.org/licenses/lgpl-2.1.html>
>>> + * Contributions after 2012-12-11 are licensed under the terms of the
>>> + * GNU GPL, version 2 or (at your option) any later version.
>>> + *
>>> */
>>> 
>>> #include "cpu.h"
>>> +#include "hw/hw.h"
>> 
>> Also, have you verified that this doesn't break s390x-linux-user?
> 
> I verified s390x-linux-user still builds. 
> 
>>> #include "qemu-common.h"
>>> #include "qemu-timer.h"
>>> 
>>> +#define IPL_PSW_MASK    0x0000000180000000ULL
>>> +#define CR0_RESET       0xE0UL
>>> +#define CR14_RESET      0xC2000000UL;
>>> 
>>> /* CPUClass::reset() */
>>> static void s390_cpu_reset(CPUState *s)
>>> @@ -37,12 +45,30 @@ static void s390_cpu_reset(CPUState *s)
>>>        log_cpu_state(env, 0);
>>>    }
>>> 
>>> +    s390_del_running_cpu(env);
>>> +
>>>    scc->parent_reset(s);
>>> 
>>>    memset(env, 0, offsetof(CPUS390XState, breakpoints));
>>> -    /* FIXME: reset vector? */
>>> +
>>> +    /* architectured initial values for CR 0 and 14 */
>>> +    env->cregs[0] = CR0_RESET;
>>> +    env->cregs[14] = CR14_RESET;
>>> +    /* set to z/Architecture mode */
>>> +    env->psw.mask = IPL_PSW_MASK;
>> 
>> In fact this one is correct for CONFIG_USER_ONLY.
>> 
>>> +    env->psa = 0;
>>> +    /* set halted to 1 to make sure we can add the cpu in
>>> +     * s390_ipl_cpu code, where env->halted is set back to 0
>>> +     * after incrementing the cpu counter */
>>> +    env->halted = 1;
>> 
>> While this again probably breaks s390x-linux-user, no?
> 
> It still builds fine, if that's what you mean? env->halted is
> not within an #ifdef !CONFIG_USER_ONLY clause.

Does it run? Or does the vcpu come up halted and never gets around to execute 
code?


Alex