[Qemu-devel] [PATCH 0/2 v3] [S390-KVM] Regsync: Allow selective runtime register synchronization

[Qemu-devel] [PATCH 0/2 v3] [S390-KVM] Regsync: Allow selective runtime register synchronization

[Jason J. Herne]

Selective KVm Register synchronization work for S390. This is re-work of the
patch set submitted here:
https://lists.nongnu.org/archive/html/qemu-devel/2013-01/msg01631.html

The selective runtime register sync code has been made S390 specific.

[Qemu-devel] [PATCH 1/2 v3] [S390-KVM] Regsync: Allow selective runtime register synchronization

[Jason J. Herne]

We want to avoid expensive register synchronization IOCTL's on the hot path so
a new kvm_s390_get_registers_partial() is introduced as a compliment to
kvm_arch_get_registers().  The new function is called on the hot path, and
kvm_arch_get_registers() is called when we need the complete runtime register
state.

kvm_arch_put_registers() is updated to only sync the partial runtime set when
we've only dirtied the partial runtime set.  This is to avoid sending bad data
back to KVM if we've only partially synced the runtime register set.

Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 target-s390x/cpu.h |   17 +++++++++++++
 target-s390x/kvm.c |   67 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 84 insertions(+)

diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
index 9cb739d..69ecb05 100644
--- a/target-s390x/cpu.h
+++ b/target-s390x/cpu.h
@@ -78,6 +78,11 @@ typedef struct MchkQueue {
     uint16_t type;
 } MchkQueue;
 
+/* Defined values for CPUS390XState.runtime_reg_dirty_mask */
+#define KVM_S390_RUNTIME_DIRTY_NONE     0
+#define KVM_S390_RUNTIME_DIRTY_PARTIAL  1
+#define KVM_S390_RUNTIME_DIRTY_FULL     2
+
 typedef struct CPUS390XState {
     uint64_t regs[16];     /* GP registers */
     CPU_DoubleU fregs[16]; /* FP registers */
@@ -121,6 +126,13 @@ typedef struct CPUS390XState {
     uint64_t cputm;
     uint32_t todpr;
 
+    /* on S390 the runtime register set has two dirty states:
+     * a partial dirty state in which only the registers that
+     * are needed all the time are fetched. And a fully dirty
+     * state in which all runtime registers are fetched.
+     */
+    uint32_t runtime_reg_dirty_mask;
+
     CPU_COMMON
 
     /* reset does memset(0) up to here */
@@ -1068,6 +1080,7 @@ void kvm_s390_io_interrupt(S390CPU *cpu, uint16_t subchannel_id,
                            uint32_t io_int_word);
 void kvm_s390_crw_mchk(S390CPU *cpu);
 void kvm_s390_enable_css_support(S390CPU *cpu);
+int kvm_s390_get_registers_partial(CPUState *cpu);
 #else
 static inline void kvm_s390_io_interrupt(S390CPU *cpu,
                                         uint16_t subchannel_id,
@@ -1082,6 +1095,10 @@ static inline void kvm_s390_crw_mchk(S390CPU *cpu)
 static inline void kvm_s390_enable_css_support(S390CPU *cpu)
 {
 }
+static inline int kvm_s390_get_registers_partial(CPUState *cpu)
+{
+    return -ENOSYS;
+}
 #endif
 
 static inline void s390_io_interrupt(S390CPU *cpu,
diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
index 8f111ae..934757e 100644
--- a/target-s390x/kvm.c
+++ b/target-s390x/kvm.c
@@ -123,6 +123,7 @@ int kvm_arch_put_registers(CPUState *cs, int level)
 {
     S390CPU *cpu = S390_CPU(cs);
     CPUS390XState *env = &cpu->env;
+    struct kvm_one_reg reg;
     struct kvm_sregs sregs;
     struct kvm_regs regs;
     int ret;
@@ -147,6 +148,30 @@ int kvm_arch_put_registers(CPUState *cs, int level)
         }
     }
 
+    if (env->runtime_reg_dirty_mask == KVM_S390_RUNTIME_DIRTY_FULL) {
+        reg.id = KVM_REG_S390_CPU_TIMER;
+        reg.addr = (__u64)&(env->cputm);
+        ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
+        if (ret < 0) {
+            return ret;
+        }
+
+        reg.id = KVM_REG_S390_CLOCK_COMP;
+        reg.addr = (__u64)&(env->ckc);
+        ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
+        if (ret < 0) {
+            return ret;
+        }
+
+        reg.id = KVM_REG_S390_TODPR;
+        reg.addr = (__u64)&(env->todpr);
+        ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
+        if (ret < 0) {
+            return ret;
+        }
+    }
+    env->runtime_reg_dirty_mask = KVM_S390_RUNTIME_DIRTY_NONE;
+
     /* Do we need to save more than that? */
     if (level == KVM_PUT_RUNTIME_STATE) {
         return 0;
@@ -186,11 +211,52 @@ int kvm_arch_get_registers(CPUState *cs)
 {
     S390CPU *cpu = S390_CPU(cs);
     CPUS390XState *env = &cpu->env;
+    struct kvm_one_reg reg;
+    int r;
+
+    r = kvm_s390_get_registers_partial(cs);
+    if (r < 0) {
+        return r;
+    }
+
+    reg.id = KVM_REG_S390_CPU_TIMER;
+    reg.addr = (__u64)&(env->cputm);
+    r = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
+    if (r < 0) {
+        return r;
+    }
+
+    reg.id = KVM_REG_S390_CLOCK_COMP;
+    reg.addr = (__u64)&(env->ckc);
+    r = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
+    if (r < 0) {
+        return r;
+    }
+
+    reg.id = KVM_REG_S390_TODPR;
+    reg.addr = (__u64)&(env->todpr);
+    r = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
+    if (r < 0) {
+        return r;
+    }
+
+    env->runtime_reg_dirty_mask = KVM_S390_RUNTIME_DIRTY_FULL;
+    return 0;
+}
+
+int kvm_s390_get_registers_partial(CPUState *cs)
+{
+    S390CPU *cpu = S390_CPU(cs);
+    CPUS390XState *env = &cpu->env;
     struct kvm_sregs sregs;
     struct kvm_regs regs;
     int ret;
     int i;
 
+    if (env->runtime_reg_dirty_mask) {
+        return 0;
+    }
+
     /* get the PSW */
     env->psw.addr = cs->kvm_run->psw_addr;
     env->psw.mask = cs->kvm_run->psw_mask;
@@ -236,6 +302,7 @@ int kvm_arch_get_registers(CPUState *cs)
         /* no prefix without sync regs */
     }
 
+    env->runtime_reg_dirty_mask = KVM_S390_RUNTIME_DIRTY_PARTIAL;
     return 0;
 }
 
-- 
1.7.10.4

[Qemu-devel] [PATCH 2/2 v3] [S390-KVM] Regsync: Utilize selective runtime reg sync for hot code paths

[Jason J. Herne]

Make use of new kvm_s390_get_registers_partial() for kvm_handle_css_inst() and
handle_hypercall() since they only need registers from the partial set and they
are called quite frequently.

Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
---
 target-s390x/kvm.c |   11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
index 934757e..36861aa 100644
--- a/target-s390x/kvm.c
+++ b/target-s390x/kvm.c
@@ -468,12 +468,16 @@ static int kvm_handle_css_inst(S390CPU *cpu, struct kvm_run *run,
     int r = 0;
     int no_cc = 0;
     CPUS390XState *env = &cpu->env;
+    CPUState *cs = ENV_GET_CPU(env);
 
     if (ipa0 != 0xb2) {
         /* Not handled for now. */
         return -1;
     }
-    cpu_synchronize_state(env);
+
+    kvm_s390_get_registers_partial(cs);
+    cs->kvm_vcpu_dirty = true;
+
     switch (ipa1) {
     case PRIV_XSCH:
         r = ioinst_handle_xsch(env, env->regs[1]);
@@ -604,7 +608,10 @@ static int handle_priv(S390CPU *cpu, struct kvm_run *run,
 
 static int handle_hypercall(CPUS390XState *env, struct kvm_run *run)
 {
-    cpu_synchronize_state(env);
+    CPUState *cs = ENV_GET_CPU(env);
+
+    kvm_s390_get_registers_partial(cs);
+    cs->kvm_vcpu_dirty = true;
     env->regs[2] = s390_virtio_hypercall(env);
 
     return 0;
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 0/2 v3] [S390-KVM] Regsync: Allow selective runtime register synchronization

[Jason J. Herne]

On 03/11/2013 01:58 PM, Jason J. Herne wrote:
> Selective KVm Register synchronization work for S390. This is re-work of the
> patch set submitted here:
> https://lists.nongnu.org/archive/html/qemu-devel/2013-01/msg01631.html
>
> The selective runtime register sync code has been made S390 specific.
>
>
>

Ping.

-- 
-- Jason J. Herne (jjherne@linux.vnet.ibm.com)

Re: [Qemu-devel] [PATCH 1/2 v3] [S390-KVM] Regsync: Allow selective runtime register synchronization

[Christian Borntraeger]

On 11/03/13 18:58, Jason J. Herne wrote:
> We want to avoid expensive register synchronization IOCTL's on the hot path so
> a new kvm_s390_get_registers_partial() is introduced as a compliment to
> kvm_arch_get_registers().  The new function is called on the hot path, and
> kvm_arch_get_registers() is called when we need the complete runtime register
> state.
> 
> kvm_arch_put_registers() is updated to only sync the partial runtime set when
> we've only dirtied the partial runtime set.  This is to avoid sending bad data
> back to KVM if we've only partially synced the runtime register set.
> 
> Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>

Alex,

we have this code in our private tree (the one thats used by the testers) for a while
now and it seems to work fine. 
This should be ok to apply. 
Since this also fixes the problem that s390 has not a complete view of the kvm registers
in places were we want it (dump etc) we might even consider this a bugfix.

Your take if this is still good for 1.5.

Christian




> ---
>  target-s390x/cpu.h |   17 +++++++++++++
>  target-s390x/kvm.c |   67 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 84 insertions(+)
> 
> diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
> index 9cb739d..69ecb05 100644
> --- a/target-s390x/cpu.h
> +++ b/target-s390x/cpu.h
> @@ -78,6 +78,11 @@ typedef struct MchkQueue {
>      uint16_t type;
>  } MchkQueue;
> 
> +/* Defined values for CPUS390XState.runtime_reg_dirty_mask */
> +#define KVM_S390_RUNTIME_DIRTY_NONE     0
> +#define KVM_S390_RUNTIME_DIRTY_PARTIAL  1
> +#define KVM_S390_RUNTIME_DIRTY_FULL     2
> +
>  typedef struct CPUS390XState {
>      uint64_t regs[16];     /* GP registers */
>      CPU_DoubleU fregs[16]; /* FP registers */
> @@ -121,6 +126,13 @@ typedef struct CPUS390XState {
>      uint64_t cputm;
>      uint32_t todpr;
> 
> +    /* on S390 the runtime register set has two dirty states:
> +     * a partial dirty state in which only the registers that
> +     * are needed all the time are fetched. And a fully dirty
> +     * state in which all runtime registers are fetched.
> +     */
> +    uint32_t runtime_reg_dirty_mask;
> +
>      CPU_COMMON
> 
>      /* reset does memset(0) up to here */
> @@ -1068,6 +1080,7 @@ void kvm_s390_io_interrupt(S390CPU *cpu, uint16_t subchannel_id,
>                             uint32_t io_int_word);
>  void kvm_s390_crw_mchk(S390CPU *cpu);
>  void kvm_s390_enable_css_support(S390CPU *cpu);
> +int kvm_s390_get_registers_partial(CPUState *cpu);
>  #else
>  static inline void kvm_s390_io_interrupt(S390CPU *cpu,
>                                          uint16_t subchannel_id,
> @@ -1082,6 +1095,10 @@ static inline void kvm_s390_crw_mchk(S390CPU *cpu)
>  static inline void kvm_s390_enable_css_support(S390CPU *cpu)
>  {
>  }
> +static inline int kvm_s390_get_registers_partial(CPUState *cpu)
> +{
> +    return -ENOSYS;
> +}
>  #endif
> 
>  static inline void s390_io_interrupt(S390CPU *cpu,
> diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
> index 8f111ae..934757e 100644
> --- a/target-s390x/kvm.c
> +++ b/target-s390x/kvm.c
> @@ -123,6 +123,7 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>  {
>      S390CPU *cpu = S390_CPU(cs);
>      CPUS390XState *env = &cpu->env;
> +    struct kvm_one_reg reg;
>      struct kvm_sregs sregs;
>      struct kvm_regs regs;
>      int ret;
> @@ -147,6 +148,30 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>          }
>      }
> 
> +    if (env->runtime_reg_dirty_mask == KVM_S390_RUNTIME_DIRTY_FULL) {
> +        reg.id = KVM_REG_S390_CPU_TIMER;
> +        reg.addr = (__u64)&(env->cputm);
> +        ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
> +        if (ret < 0) {
> +            return ret;
> +        }
> +
> +        reg.id = KVM_REG_S390_CLOCK_COMP;
> +        reg.addr = (__u64)&(env->ckc);
> +        ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
> +        if (ret < 0) {
> +            return ret;
> +        }
> +
> +        reg.id = KVM_REG_S390_TODPR;
> +        reg.addr = (__u64)&(env->todpr);
> +        ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
> +        if (ret < 0) {
> +            return ret;
> +        }
> +    }
> +    env->runtime_reg_dirty_mask = KVM_S390_RUNTIME_DIRTY_NONE;
> +
>      /* Do we need to save more than that? */
>      if (level == KVM_PUT_RUNTIME_STATE) {
>          return 0;
> @@ -186,11 +211,52 @@ int kvm_arch_get_registers(CPUState *cs)
>  {
>      S390CPU *cpu = S390_CPU(cs);
>      CPUS390XState *env = &cpu->env;
> +    struct kvm_one_reg reg;
> +    int r;
> +
> +    r = kvm_s390_get_registers_partial(cs);
> +    if (r < 0) {
> +        return r;
> +    }
> +
> +    reg.id = KVM_REG_S390_CPU_TIMER;
> +    reg.addr = (__u64)&(env->cputm);
> +    r = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
> +    if (r < 0) {
> +        return r;
> +    }
> +
> +    reg.id = KVM_REG_S390_CLOCK_COMP;
> +    reg.addr = (__u64)&(env->ckc);
> +    r = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
> +    if (r < 0) {
> +        return r;
> +    }
> +
> +    reg.id = KVM_REG_S390_TODPR;
> +    reg.addr = (__u64)&(env->todpr);
> +    r = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
> +    if (r < 0) {
> +        return r;
> +    }
> +
> +    env->runtime_reg_dirty_mask = KVM_S390_RUNTIME_DIRTY_FULL;
> +    return 0;
> +}
> +
> +int kvm_s390_get_registers_partial(CPUState *cs)
> +{
> +    S390CPU *cpu = S390_CPU(cs);
> +    CPUS390XState *env = &cpu->env;
>      struct kvm_sregs sregs;
>      struct kvm_regs regs;
>      int ret;
>      int i;
> 
> +    if (env->runtime_reg_dirty_mask) {
> +        return 0;
> +    }
> +
>      /* get the PSW */
>      env->psw.addr = cs->kvm_run->psw_addr;
>      env->psw.mask = cs->kvm_run->psw_mask;
> @@ -236,6 +302,7 @@ int kvm_arch_get_registers(CPUState *cs)
>          /* no prefix without sync regs */
>      }
> 
> +    env->runtime_reg_dirty_mask = KVM_S390_RUNTIME_DIRTY_PARTIAL;
>      return 0;
>  }
> 

Re: [Qemu-devel] [PATCH 2/2 v3] [S390-KVM] Regsync: Utilize selective runtime reg sync for hot code paths

[Christian Borntraeger]

On 11/03/13 18:58, Jason J. Herne wrote:
> Make use of new kvm_s390_get_registers_partial() for kvm_handle_css_inst() and
> handle_hypercall() since they only need registers from the partial set and they
> are called quite frequently.
> 
> Signed-off-by: Jason J. Herne <jjherne@us.ibm.com>
> ---
>  target-s390x/kvm.c |   11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
> index 934757e..36861aa 100644
> --- a/target-s390x/kvm.c
> +++ b/target-s390x/kvm.c
> @@ -468,12 +468,16 @@ static int kvm_handle_css_inst(S390CPU *cpu, struct kvm_run *run,
>      int r = 0;
>      int no_cc = 0;
>      CPUS390XState *env = &cpu->env;
> +    CPUState *cs = ENV_GET_CPU(env);
> 
>      if (ipa0 != 0xb2) {
>          /* Not handled for now. */
>          return -1;
>      }
> -    cpu_synchronize_state(env);
> +
> +    kvm_s390_get_registers_partial(cs);
> +    cs->kvm_vcpu_dirty = true;
> +
>      switch (ipa1) {
>      case PRIV_XSCH:
>          r = ioinst_handle_xsch(env, env->regs[1]);


Looks good, but can you also do the same for TSCH?
This is also related to the main I/O path.


> @@ -604,7 +608,10 @@ static int handle_priv(S390CPU *cpu, struct kvm_run *run,
> 
>  static int handle_hypercall(CPUS390XState *env, struct kvm_run *run)
>  {
> -    cpu_synchronize_state(env);
> +    CPUState *cs = ENV_GET_CPU(env);
> +
> +    kvm_s390_get_registers_partial(cs);
> +    cs->kvm_vcpu_dirty = true;
>      env->regs[2] = s390_virtio_hypercall(env);
> 
>      return 0;
>