[Qemu-devel] [PATCH 0/3] s390: vcpu reset, -cpu ? and minor sclp fix

[Qemu-devel] [PATCH 0/3] s390: vcpu reset, -cpu ? and minor sclp fix

[Jens Freimann]

Hi Alex,

a few more s390 patches:
Patch 1 enables the -cpu ? option for s390 CPU models.
Patch 2 implements vcpu reset
Patch 3 takes care of an possibly uninitialized variable in SCLP code

regards
Jens

Cornelia Huck (1):
  sclp: Fix uninitialized var in handle_write_event_buf().

Jens Freimann (1):
  s390: clear registers, psw and prefix at vcpu reset

Viktor Mihajlovski (1):
  S390: Basic CPU model support

 hw/s390-virtio.c          |  5 +++++
 hw/s390x/event-facility.c |  3 ++-
 target-s390x/cpu.c        |  6 ++++++
 target-s390x/cpu.h        |  3 +++
 target-s390x/kvm.c        | 26 +++++++++++++++++++++++++-
 5 files changed, 41 insertions(+), 2 deletions(-)

-- 
1.7.12.4

[Qemu-devel] [PATCH 1/3] S390: Basic CPU model support

[Jens Freimann]

From: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>

This enables qemu -cpu ? to return the list of supported CPU models
on s390. Since only the host model is supported at this point in time
this is pretty straight-forward. Further, a validity check for the
requested CPU model was added.
This change is needed to allow libvirt exploiters (like OpenStack)
to specify a CPU model.

Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 hw/s390-virtio.c   | 5 +++++
 target-s390x/cpu.c | 6 ++++++
 target-s390x/cpu.h | 3 +++
 3 files changed, 14 insertions(+)

diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
index 685cb54..7144ac1 100644
--- a/hw/s390-virtio.c
+++ b/hw/s390-virtio.c
@@ -209,6 +209,11 @@ static void s390_init(QEMUMachineInitArgs *args)
         cpu_model = "host";
     }
 
+    if (strcmp(cpu_model, "host")) {
+        fprintf(stderr, "S390 only supports host CPU model\n");
+        exit(1);
+    }
+
     ipi_states = g_malloc(sizeof(S390CPU *) * smp_cpus);
 
     for (i = 0; i < smp_cpus; i++) {
diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
index 619b202..03fdc31 100644
--- a/target-s390x/cpu.c
+++ b/target-s390x/cpu.c
@@ -25,6 +25,12 @@
 #include "qemu-timer.h"
 
 
+/* generate CPU information for cpu -? */
+void s390_cpu_list(FILE *f, fprintf_function cpu_fprintf)
+{
+    (*cpu_fprintf)(f, "s390 %16s\n", "[host]");
+}
+
 /* CPUClass::reset() */
 static void s390_cpu_reset(CPUState *s)
 {
diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
index 0f9a1f7..3513976 100644
--- a/target-s390x/cpu.h
+++ b/target-s390x/cpu.h
@@ -350,6 +350,9 @@ static inline void cpu_set_tls(CPUS390XState *env, target_ulong newtls)
 #define cpu_gen_code cpu_s390x_gen_code
 #define cpu_signal_handler cpu_s390x_signal_handler
 
+void s390_cpu_list(FILE *f, fprintf_function cpu_fprintf);
+#define cpu_list s390_cpu_list
+
 #include "exec-all.h"
 
 #ifdef CONFIG_USER_ONLY
-- 
1.7.12.4

[Qemu-devel] [PATCH 2/3] s390: clear registers, psw and prefix at vcpu reset

[Jens Freimann]

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
@@ -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

[Qemu-devel] [PATCH 3/3] sclp: Fix uninitialized var in handle_write_event_buf().

[Jens Freimann]

From: Cornelia Huck <cornelia.huck@de.ibm.com>

Some gcc versions rightly complain about a possibly unitialized rc,
so let's move setting it before the QTAILQ_FOREACH().

Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
---
 hw/s390x/event-facility.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
index 9367660..bc9cea9 100644
--- a/hw/s390x/event-facility.c
+++ b/hw/s390x/event-facility.c
@@ -112,12 +112,13 @@ static uint16_t handle_write_event_buf(SCLPEventFacility *ef,
     SCLPEvent *event;
     SCLPEventClass *ec;
 
+    rc = SCLP_RC_INVALID_FUNCTION;
+
     QTAILQ_FOREACH(kid, &ef->sbus.qbus.children, sibling) {
         DeviceState *qdev = kid->child;
         event = (SCLPEvent *) qdev;
         ec = SCLP_EVENT_GET_CLASS(event);
 
-        rc = SCLP_RC_INVALID_FUNCTION;
         if (ec->write_event_data &&
             ec->event_type() == event_buf->type) {
             rc = ec->write_event_data(event, event_buf);
-- 
1.7.12.4

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

[Alexander Graf]

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
> 

Re: [Qemu-devel] [PATCH 3/3] sclp: Fix uninitialized var in handle_write_event_buf().

[Alexander Graf]

On 23.11.2012, at 11:18, Jens Freimann wrote:

> From: Cornelia Huck <cornelia.huck@de.ibm.com>
> 
> Some gcc versions rightly complain about a possibly unitialized rc,
> so let's move setting it before the QTAILQ_FOREACH().
> 
> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>

Thanks, applied to s390-next.


Alex

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

[Jens Freimann]

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

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

[Christian Borntraeger]

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

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

[Alexander Graf]

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

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

[Jens Freimann]

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
> 

-- 
Mit freundlichen Grüßen / Kind regards 

Jens Freimann 

-- 
IBM Linux Technology Center / Boeblingen lab
IBM Systems &Technology Group, Systems Software Development
-------------------------------------------------------------
IBM Deutschland
Schoenaicher Str 220
71032 Boeblingen
Phone: +49-7031-16 x1122
E-Mail: jfrei@de.ibm.com
-------------------------------------------------------------
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

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

[Peter Maydell]

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

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

[Alexander Graf]

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

Re: [Qemu-devel] [PATCH 1/3] S390: Basic CPU model support

[Andreas Färber]

Am 23.11.2012 11:18, schrieb Jens Freimann:
> From: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
> 
> This enables qemu -cpu ? to return the list of supported CPU models
> on s390. Since only the host model is supported at this point in time
> this is pretty straight-forward. Further, a validity check for the
> requested CPU model was added.
> This change is needed to allow libvirt exploiters (like OpenStack)
> to specify a CPU model.
> 
> Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com>
> Signed-off-by: Jens Freimann <jfrei@linux.vnet.ibm.com>
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  hw/s390-virtio.c   | 5 +++++
>  target-s390x/cpu.c | 6 ++++++
>  target-s390x/cpu.h | 3 +++
>  3 files changed, 14 insertions(+)
> 
> diff --git a/hw/s390-virtio.c b/hw/s390-virtio.c
> index 685cb54..7144ac1 100644
> --- a/hw/s390-virtio.c
> +++ b/hw/s390-virtio.c
> @@ -209,6 +209,11 @@ static void s390_init(QEMUMachineInitArgs *args)
>          cpu_model = "host";
>      }
>  
> +    if (strcmp(cpu_model, "host")) {
> +        fprintf(stderr, "S390 only supports host CPU model\n");
> +        exit(1);
> +    }

When Cornelia introduces a second machine for virtio-ccw, you will have
to duplicate this logic. Other targets have an, e.g., cpu_s390_init()
function that encapsulates the instantiation logic.

Also, "host" doesn't make a lot of sense on non-s390 host, no?

> +
>      ipi_states = g_malloc(sizeof(S390CPU *) * smp_cpus);
>  
>      for (i = 0; i < smp_cpus; i++) {
> diff --git a/target-s390x/cpu.c b/target-s390x/cpu.c
> index 619b202..03fdc31 100644
> --- a/target-s390x/cpu.c
> +++ b/target-s390x/cpu.c
> @@ -25,6 +25,12 @@
>  #include "qemu-timer.h"
>  
>  
> +/* generate CPU information for cpu -? */
> +void s390_cpu_list(FILE *f, fprintf_function cpu_fprintf)
> +{
> +    (*cpu_fprintf)(f, "s390 %16s\n", "[host]");

The other targets don't use a target prefix ("s390 "), but then again
the text and layout are fully target-specific at this time. :)

Regards,
Andreas

> +}
> +
>  /* CPUClass::reset() */
>  static void s390_cpu_reset(CPUState *s)
>  {
> diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
> index 0f9a1f7..3513976 100644
> --- a/target-s390x/cpu.h
> +++ b/target-s390x/cpu.h
> @@ -350,6 +350,9 @@ static inline void cpu_set_tls(CPUS390XState *env, target_ulong newtls)
>  #define cpu_gen_code cpu_s390x_gen_code
>  #define cpu_signal_handler cpu_s390x_signal_handler
>  
> +void s390_cpu_list(FILE *f, fprintf_function cpu_fprintf);
> +#define cpu_list s390_cpu_list
> +
>  #include "exec-all.h"
>  
>  #ifdef CONFIG_USER_ONLY
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg