Patchwork [2/3] s390: clear registers, psw and prefix at vcpu reset

login
register
mail settings
Submitter Jens Freimann
Date Nov. 23, 2012, 10:18 a.m.
Message ID <1353665892-35445-3-git-send-email-jfrei@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/201284/
State New
Headers show

Comments

Jens Freimann - Nov. 23, 2012, 10:18 a.m.
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>
---
 target-s390x/kvm.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)
Alexander Graf - Nov. 23, 2012, 1:40 p.m.
On 23.11.2012, at 11:18, Jens Freimann 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>
> ---
> target-s390x/kvm.c | 26 +++++++++++++++++++++++++-
> 1 file changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
> index 94de764..b1b791e 100644
> --- a/target-s390x/kvm.c
> +++ b/target-s390x/kvm.c

This needs to go into generic vcpu reset code.


Alex

> @@ -85,7 +85,31 @@ int kvm_arch_init_vcpu(CPUS390XState *env)
> 
> void kvm_arch_reset_vcpu(CPUS390XState *env)
> {
> -    /* FIXME: add code to reset vcpu. */
> +    int i;
> +
> +    /* The initial reset call is needed here to reset in-kernel
> +     * vcpu data that we can't access directly from QEMU. Before
> +     * this ioctl cpu_synchronize_state() is called in common kvm
> +     * code (kvm-all). What remains is clearing registers and psw
> +     * in QEMU cpu state */
> +    if (kvm_vcpu_ioctl(env, KVM_S390_INITIAL_RESET, NULL)) {
> +        perror("Can't reset vcpu\n");
> +    }
> +    env->halted = 1;
> +    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;
> +    }
> +    /* architectured initial values for CR 0 and 14 */
> +    env->cregs[0] = 0xE0UL;
> +    env->cregs[14] = 0xC2000000UL;
> +    env->fpc = 0;
> +    env->psw.mask = 0;
> +    env->psw.addr = 0;
> +    env->psa = 0;
> }
> 
> int kvm_arch_put_registers(CPUS390XState *env, int level)
> -- 
> 1.7.12.4
>
Jens Freimann - Nov. 23, 2012, 2:17 p.m.
On Fri, Nov 23, 2012 at 02:40:10PM +0100, Alexander Graf wrote:
> 
> On 23.11.2012, at 11:18, Jens Freimann 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>
> > ---
> > target-s390x/kvm.c | 26 +++++++++++++++++++++++++-
> > 1 file changed, 25 insertions(+), 1 deletion(-)
> > 
> > diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
> > index 94de764..b1b791e 100644
> > --- a/target-s390x/kvm.c
> > +++ b/target-s390x/kvm.c
> 
> This needs to go into generic vcpu reset code.

Ok. Will send a new patch.

Jens
Christian Borntraeger - Nov. 23, 2012, 2:32 p.m.
On 23/11/12 14:40, Alexander Graf wrote:
> 
> On 23.11.2012, at 11:18, Jens Freimann 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>
>> ---
>> target-s390x/kvm.c | 26 +++++++++++++++++++++++++-
>> 1 file changed, 25 insertions(+), 1 deletion(-)
>>
>> diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
>> index 94de764..b1b791e 100644
>> --- a/target-s390x/kvm.c
>> +++ b/target-s390x/kvm.c
> 
> This needs to go into generic vcpu reset code.

The kvm ioctl certainly not, no? (definitely necessary for kernels
without sync regs).

I guess you are talking about moving the register initialisation 
into s390_cpu_reset (target-s390x/cpu.c). Right? Jens can you have
a look?

Christian
Alexander Graf - Nov. 23, 2012, 2:34 p.m.
On 23.11.2012, at 15:32, Christian Borntraeger wrote:

> On 23/11/12 14:40, Alexander Graf wrote:
>> 
>> On 23.11.2012, at 11:18, Jens Freimann 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>
>>> ---
>>> target-s390x/kvm.c | 26 +++++++++++++++++++++++++-
>>> 1 file changed, 25 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
>>> index 94de764..b1b791e 100644
>>> --- a/target-s390x/kvm.c
>>> +++ b/target-s390x/kvm.c
>> 
>> This needs to go into generic vcpu reset code.
> 
> The kvm ioctl certainly not, no? (definitely necessary for kernels
> without sync regs).

Yes, of course :).

> I guess you are talking about moving the register initialisation 
> into s390_cpu_reset (target-s390x/cpu.c). Right? Jens can you have
> a look?

Yup. This is just normal reset logic that needs to be in the normal CPU reset functions.


Alex
Jens Freimann - Nov. 23, 2012, 2:52 p.m.
On Fri, Nov 23, 2012 at 03:32:16PM +0100, Christian Borntraeger wrote:
> On 23/11/12 14:40, Alexander Graf wrote:
> > 
> > On 23.11.2012, at 11:18, Jens Freimann 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>
> >> ---
> >> target-s390x/kvm.c | 26 +++++++++++++++++++++++++-
> >> 1 file changed, 25 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
> >> index 94de764..b1b791e 100644
> >> --- a/target-s390x/kvm.c
> >> +++ b/target-s390x/kvm.c
> > 
> > This needs to go into generic vcpu reset code.
> 
> The kvm ioctl certainly not, no? (definitely necessary for kernels
> without sync regs).
> 
> I guess you are talking about moving the register initialisation 
> into s390_cpu_reset (target-s390x/cpu.c). Right? Jens can you have
> a look?

Yes, I'm  already looking into it. s390_cpu_reset() is only called when booting
the first time however because it's not registered as a reset handler. Trying to
find out how if I can convert it to a qemu reset handler or if I have to
do some QOM magic.

Jens

> Christian
>
Peter Maydell - Nov. 23, 2012, 3:02 p.m.
On 23 November 2012 10:18, Jens Freimann <jfrei@linux.vnet.ibm.com> wrote:
> +    /* The initial reset call is needed here to reset in-kernel
> +     * vcpu data that we can't access directly from QEMU.

If there's in-kernel vcpu data we can't access from QEMU,
doesn't this cause problems for migration?

> Before
> +     * this ioctl cpu_synchronize_state() is called in common kvm
> +     * code (kvm-all). What remains is clearing registers and psw
> +     * in QEMU cpu state */
> +    if (kvm_vcpu_ioctl(env, KVM_S390_INITIAL_RESET, NULL)) {
> +        perror("Can't reset vcpu\n");
> +    }

Doesn't this mean we're now out-of-sync again, since the
kernel state will be whatever INITIAL_RESET says it should
be but the QEMU state won't have been changed?

Generally, I think an arch-independent 'reset vcpu' ioctl
would be useful. At the moment for ARM you have to pull
everything out and back in again via the GET/SET_REG interface,
which works [you can keep all the state around to write back
on vcpu reset] but is ugly.

-- PMM
Alexander Graf - Nov. 23, 2012, 3:13 p.m.
On 23.11.2012, at 16:02, Peter Maydell wrote:

> On 23 November 2012 10:18, Jens Freimann <jfrei@linux.vnet.ibm.com> wrote:
>> +    /* The initial reset call is needed here to reset in-kernel
>> +     * vcpu data that we can't access directly from QEMU.
> 
> If there's in-kernel vcpu data we can't access from QEMU,
> doesn't this cause problems for migration?

This one is for older kernels. For newer kernels, we have sync regs and ONE_REG interfaces to everything we need FWIW :).

> 
>> Before
>> +     * this ioctl cpu_synchronize_state() is called in common kvm
>> +     * code (kvm-all). What remains is clearing registers and psw
>> +     * in QEMU cpu state */
>> +    if (kvm_vcpu_ioctl(env, KVM_S390_INITIAL_RESET, NULL)) {
>> +        perror("Can't reset vcpu\n");
>> +    }
> 
> Doesn't this mean we're now out-of-sync again, since the
> kernel state will be whatever INITIAL_RESET says it should
> be but the QEMU state won't have been changed?
> 
> Generally, I think an arch-independent 'reset vcpu' ioctl
> would be useful. At the moment for ARM you have to pull
> everything out and back in again via the GET/SET_REG interface,
> which works [you can keep all the state around to write back
> on vcpu reset] but is ugly.

Actually it's the way it's supposed to work. The reset call is just a hack, because it means we need to duplicate the reset logic for KVM & TCG. But it dates back to when KVM on s390 wasn't using QEMU.


Alex

Patch

diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
index 94de764..b1b791e 100644
--- a/target-s390x/kvm.c
+++ b/target-s390x/kvm.c
@@ -85,7 +85,31 @@  int kvm_arch_init_vcpu(CPUS390XState *env)
 
 void kvm_arch_reset_vcpu(CPUS390XState *env)
 {
-    /* FIXME: add code to reset vcpu. */
+    int i;
+
+    /* The initial reset call is needed here to reset in-kernel
+     * vcpu data that we can't access directly from QEMU. Before
+     * this ioctl cpu_synchronize_state() is called in common kvm
+     * code (kvm-all). What remains is clearing registers and psw
+     * in QEMU cpu state */
+    if (kvm_vcpu_ioctl(env, KVM_S390_INITIAL_RESET, NULL)) {
+        perror("Can't reset vcpu\n");
+    }
+    env->halted = 1;
+    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;
+    }
+    /* architectured initial values for CR 0 and 14 */
+    env->cregs[0] = 0xE0UL;
+    env->cregs[14] = 0xC2000000UL;
+    env->fpc = 0;
+    env->psw.mask = 0;
+    env->psw.addr = 0;
+    env->psa = 0;
 }
 
 int kvm_arch_put_registers(CPUS390XState *env, int level)