QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2 0/3] s390x: Increase architectural compliance
@ 2019-12-02 14:01 Janosch Frank
  2019-12-02 14:01 ` [PATCH v2 1/3] Header sync Janosch Frank
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Janosch Frank @ 2019-12-02 14:01 UTC (permalink / raw)
  To: qemu-devel; +Cc: thuth, david, cohuck, borntraeger, qemu-s390x, mihajlov

On a cpu reset normal, we need to clear local cpus. Unfortunately we
need a new API for that, since KVM only exposes one of the three
resets.

Also we need to clear the riccb and the PSW ri mask on a normal reset.

Patches are also in my cleanup branch.

Janosch Frank (3):
  Header sync
  s390x: Add missing vcpu reset functions
  s390x: Fix cpu normal reset ri clearing

 linux-headers/linux/kvm.h |  4 ++++
 target/s390x/cpu.c        | 19 ++++++++++++++++--
 target/s390x/cpu.h        |  7 ++++++-
 target/s390x/kvm-stub.c   | 10 +++++++++-
 target/s390x/kvm.c        | 42 +++++++++++++++++++++++++++++++--------
 target/s390x/kvm_s390x.h  |  4 +++-
 6 files changed, 73 insertions(+), 13 deletions(-)

-- 
2.20.1



^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2 1/3] Header sync
  2019-12-02 14:01 [PATCH v2 0/3] s390x: Increase architectural compliance Janosch Frank
@ 2019-12-02 14:01 ` Janosch Frank
  2019-12-02 14:01 ` [PATCH v2 2/3] s390x: Add missing vcpu reset functions Janosch Frank
  2019-12-02 14:01 ` [PATCH v2 3/3] s390x: Fix cpu normal reset ri clearing Janosch Frank
  2 siblings, 0 replies; 11+ messages in thread
From: Janosch Frank @ 2019-12-02 14:01 UTC (permalink / raw)
  To: qemu-devel; +Cc: thuth, david, cohuck, borntraeger, qemu-s390x, mihajlov

Sync in the new vcpu resets.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 linux-headers/linux/kvm.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
index 3d9b18f7f8..f9fc3f6dc0 100644
--- a/linux-headers/linux/kvm.h
+++ b/linux-headers/linux/kvm.h
@@ -1000,6 +1000,7 @@ struct kvm_ppc_resize_hpt {
 #define KVM_CAP_PMU_EVENT_FILTER 173
 #define KVM_CAP_ARM_IRQ_LINE_LAYOUT_2 174
 #define KVM_CAP_HYPERV_DIRECT_TLBFLUSH 175
+#define KVM_CAP_S390_VCPU_RESETS 176
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
@@ -1461,6 +1462,9 @@ struct kvm_enc_region {
 /* Available with KVM_CAP_ARM_SVE */
 #define KVM_ARM_VCPU_FINALIZE	  _IOW(KVMIO,  0xc2, int)
 
+#define KVM_S390_NORMAL_RESET	  _IO(KVMIO,  0xc3)
+#define KVM_S390_CLEAR_RESET	  _IO(KVMIO,  0xc4)
+
 /* Secure Encrypted Virtualization command */
 enum sev_cmd_id {
 	/* Guest initialization commands */
-- 
2.20.1



^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2 2/3] s390x: Add missing vcpu reset functions
  2019-12-02 14:01 [PATCH v2 0/3] s390x: Increase architectural compliance Janosch Frank
  2019-12-02 14:01 ` [PATCH v2 1/3] Header sync Janosch Frank
@ 2019-12-02 14:01 ` Janosch Frank
  2019-12-02 14:22   ` David Hildenbrand
                     ` (3 more replies)
  2019-12-02 14:01 ` [PATCH v2 3/3] s390x: Fix cpu normal reset ri clearing Janosch Frank
  2 siblings, 4 replies; 11+ messages in thread
From: Janosch Frank @ 2019-12-02 14:01 UTC (permalink / raw)
  To: qemu-devel; +Cc: thuth, david, cohuck, borntraeger, qemu-s390x, mihajlov

Up to now we only had an ioctl to reset vcpu data QEMU couldn't reach
for the initial reset, which was also called for the clear reset. To
be architecture compliant, we also need to clear local interrupts on a
normal reset.

Because of this and the upcoming protvirt support we need to add
ioctls for the missing clear and normal resets.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 target/s390x/cpu.c       | 14 ++++++++++++--
 target/s390x/kvm-stub.c  | 10 +++++++++-
 target/s390x/kvm.c       | 42 ++++++++++++++++++++++++++++++++--------
 target/s390x/kvm_s390x.h |  4 +++-
 4 files changed, 58 insertions(+), 12 deletions(-)

diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 829ce6ad54..906285888e 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -139,8 +139,18 @@ static void s390_cpu_reset(CPUState *s, cpu_reset_type type)
     }
 
     /* Reset state inside the kernel that we cannot access yet from QEMU. */
-    if (kvm_enabled() && type != S390_CPU_RESET_NORMAL) {
-        kvm_s390_reset_vcpu(cpu);
+    if (kvm_enabled()) {
+        switch (type) {
+        case S390_CPU_RESET_CLEAR:
+            kvm_s390_reset_vcpu_clear(cpu);
+            break;
+        case S390_CPU_RESET_INITIAL:
+            kvm_s390_reset_vcpu_initial(cpu);
+            break;
+        case S390_CPU_RESET_NORMAL:
+            kvm_s390_reset_vcpu_normal(cpu);
+            break;
+        }
     }
 }
 
diff --git a/target/s390x/kvm-stub.c b/target/s390x/kvm-stub.c
index 5152e2bdf1..c4cd497f85 100644
--- a/target/s390x/kvm-stub.c
+++ b/target/s390x/kvm-stub.c
@@ -83,7 +83,15 @@ void kvm_s390_cmma_reset(void)
 {
 }
 
-void kvm_s390_reset_vcpu(S390CPU *cpu)
+void kvm_s390_reset_vcpu_initial(S390CPU *cpu)
+{
+}
+
+void kvm_s390_reset_vcpu_clear(S390CPU *cpu)
+{
+}
+
+void kvm_s390_reset_vcpu_normal(S390CPU *cpu)
 {
 }
 
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index ad6e38c876..7a2ec8b9f8 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -151,6 +151,7 @@ static int cap_s390_irq;
 static int cap_ri;
 static int cap_gs;
 static int cap_hpage_1m;
+static int cap_vcpu_resets;
 
 static int active_cmma;
 
@@ -342,6 +343,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
     cap_async_pf = kvm_check_extension(s, KVM_CAP_ASYNC_PF);
     cap_mem_op = kvm_check_extension(s, KVM_CAP_S390_MEM_OP);
     cap_s390_irq = kvm_check_extension(s, KVM_CAP_S390_INJECT_IRQ);
+    cap_vcpu_resets = kvm_check_extension(s, KVM_CAP_S390_VCPU_RESETS);
 
     if (!kvm_check_extension(s, KVM_CAP_S390_GMAP)
         || !kvm_check_extension(s, KVM_CAP_S390_COW)) {
@@ -403,20 +405,44 @@ int kvm_arch_destroy_vcpu(CPUState *cs)
     return 0;
 }
 
-void kvm_s390_reset_vcpu(S390CPU *cpu)
+static void kvm_s390_reset_vcpu(S390CPU *cpu, unsigned long type)
 {
     CPUState *cs = CPU(cpu);
 
-    /* The initial reset call is needed here to reset in-kernel
-     * vcpu data that we can't access directly from QEMU
-     * (i.e. with older kernels which don't support sync_regs/ONE_REG).
-     * Before this ioctl cpu_synchronize_state() is called in common kvm
-     * code (kvm-all) */
-    if (kvm_vcpu_ioctl(cs, KVM_S390_INITIAL_RESET, NULL)) {
-        error_report("Initial CPU reset failed on CPU %i", cs->cpu_index);
+    /*
+     * The reset call is needed here to reset in-kernel vcpu data that
+     * we can't access directly from QEMU (i.e. with older kernels
+     * which don't support sync_regs/ONE_REG).  Before this ioctl
+     * cpu_synchronize_state() is called in common kvm code
+     * (kvm-all).
+     */
+    if (kvm_vcpu_ioctl(cs, type)) {
+        error_report("CPU reset failed on CPU %i", cs->cpu_index);
     }
 }
 
+void kvm_s390_reset_vcpu_initial(S390CPU *cpu)
+{
+    kvm_s390_reset_vcpu(cpu, KVM_S390_INITIAL_RESET);
+}
+
+void kvm_s390_reset_vcpu_clear(S390CPU *cpu)
+{
+    if (!cap_vcpu_resets) {
+        kvm_s390_reset_vcpu(cpu, KVM_S390_INITIAL_RESET);
+    } else {
+        kvm_s390_reset_vcpu(cpu, KVM_S390_CLEAR_RESET);
+    }
+}
+
+void kvm_s390_reset_vcpu_normal(S390CPU *cpu)
+{
+    if (!cap_vcpu_resets) {
+        return;
+    }
+    kvm_s390_reset_vcpu(cpu, KVM_S390_NORMAL_RESET);
+}
+
 static int can_sync_regs(CPUState *cs, int regs)
 {
     return cap_sync_regs && (cs->kvm_run->kvm_valid_regs & regs) == regs;
diff --git a/target/s390x/kvm_s390x.h b/target/s390x/kvm_s390x.h
index caf985955b..0b21789796 100644
--- a/target/s390x/kvm_s390x.h
+++ b/target/s390x/kvm_s390x.h
@@ -34,7 +34,9 @@ int kvm_s390_assign_subch_ioeventfd(EventNotifier *notifier, uint32_t sch,
                                     int vq, bool assign);
 int kvm_s390_cmma_active(void);
 void kvm_s390_cmma_reset(void);
-void kvm_s390_reset_vcpu(S390CPU *cpu);
+void kvm_s390_reset_vcpu_clear(S390CPU *cpu);
+void kvm_s390_reset_vcpu_normal(S390CPU *cpu);
+void kvm_s390_reset_vcpu_initial(S390CPU *cpu);
 int kvm_s390_set_mem_limit(uint64_t new_limit, uint64_t *hw_limit);
 void kvm_s390_set_max_pagesize(uint64_t pagesize, Error **errp);
 void kvm_s390_crypto_reset(void);
-- 
2.20.1



^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2 3/3] s390x: Fix cpu normal reset ri clearing
  2019-12-02 14:01 [PATCH v2 0/3] s390x: Increase architectural compliance Janosch Frank
  2019-12-02 14:01 ` [PATCH v2 1/3] Header sync Janosch Frank
  2019-12-02 14:01 ` [PATCH v2 2/3] s390x: Add missing vcpu reset functions Janosch Frank
@ 2019-12-02 14:01 ` Janosch Frank
  2019-12-03 10:07   ` Christian Borntraeger
  2019-12-03 10:21   ` David Hildenbrand
  2 siblings, 2 replies; 11+ messages in thread
From: Janosch Frank @ 2019-12-02 14:01 UTC (permalink / raw)
  To: qemu-devel; +Cc: thuth, david, cohuck, borntraeger, qemu-s390x, mihajlov

As it turns out we need to clear the ri controls and PSW enablement
bit to be architecture compliant.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 target/s390x/cpu.c | 5 +++++
 target/s390x/cpu.h | 7 ++++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 906285888e..c192e6b3b9 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -131,6 +131,11 @@ static void s390_cpu_reset(CPUState *s, cpu_reset_type type)
                                   &env->fpu_status);
        /* fall through */
     case S390_CPU_RESET_NORMAL:
+        env->psw.mask &= ~PSW_MASK_RI;
+        memset(&env->start_normal_reset_fields, 0,
+               offsetof(CPUS390XState, end_reset_fields) -
+               offsetof(CPUS390XState, start_normal_reset_fields));
+
         env->pfault_token = -1UL;
         env->bpbc = false;
         break;
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index d5e18b096e..7f5fa1d35b 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -58,7 +58,6 @@ struct CPUS390XState {
      */
     uint64_t vregs[32][2] QEMU_ALIGNED(16);  /* vector registers */
     uint32_t aregs[16];    /* access registers */
-    uint8_t riccb[64];     /* runtime instrumentation control */
     uint64_t gscb[4];      /* guarded storage control */
     uint64_t etoken;       /* etoken */
     uint64_t etoken_extension; /* etoken extension */
@@ -114,6 +113,10 @@ struct CPUS390XState {
     uint64_t gbea;
     uint64_t pp;
 
+    /* Fields up to this point are not cleared by normal CPU reset */
+    struct {} start_normal_reset_fields;
+    uint8_t riccb[64];     /* runtime instrumentation control */
+
     /* Fields up to this point are cleared by a CPU reset */
     struct {} end_reset_fields;
 
@@ -252,6 +255,7 @@ extern const VMStateDescription vmstate_s390_cpu;
 #undef PSW_SHIFT_ASC
 #undef PSW_MASK_CC
 #undef PSW_MASK_PM
+#undef PSW_MASK_RI
 #undef PSW_SHIFT_MASK_PM
 #undef PSW_MASK_64
 #undef PSW_MASK_32
@@ -274,6 +278,7 @@ extern const VMStateDescription vmstate_s390_cpu;
 #define PSW_MASK_CC             0x0000300000000000ULL
 #define PSW_MASK_PM             0x00000F0000000000ULL
 #define PSW_SHIFT_MASK_PM       40
+#define PSW_MASK_RI             0x0000008000000000ULL
 #define PSW_MASK_64             0x0000000100000000ULL
 #define PSW_MASK_32             0x0000000080000000ULL
 #define PSW_MASK_ESA_ADDR       0x000000007fffffffULL
-- 
2.20.1



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 2/3] s390x: Add missing vcpu reset functions
  2019-12-02 14:01 ` [PATCH v2 2/3] s390x: Add missing vcpu reset functions Janosch Frank
@ 2019-12-02 14:22   ` David Hildenbrand
  2019-12-02 14:28   ` Thomas Huth
                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 11+ messages in thread
From: David Hildenbrand @ 2019-12-02 14:22 UTC (permalink / raw)
  To: Janosch Frank, qemu-devel
  Cc: borntraeger, thuth, mihajlov, qemu-s390x, cohuck

On 02.12.19 15:01, Janosch Frank wrote:
> Up to now we only had an ioctl to reset vcpu data QEMU couldn't reach
> for the initial reset, which was also called for the clear reset. To
> be architecture compliant, we also need to clear local interrupts on a
> normal reset.
> 
> Because of this and the upcoming protvirt support we need to add
> ioctls for the missing clear and normal resets.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  target/s390x/cpu.c       | 14 ++++++++++++--
>  target/s390x/kvm-stub.c  | 10 +++++++++-
>  target/s390x/kvm.c       | 42 ++++++++++++++++++++++++++++++++--------
>  target/s390x/kvm_s390x.h |  4 +++-
>  4 files changed, 58 insertions(+), 12 deletions(-)
> 
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index 829ce6ad54..906285888e 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -139,8 +139,18 @@ static void s390_cpu_reset(CPUState *s, cpu_reset_type type)
>      }
>  
>      /* Reset state inside the kernel that we cannot access yet from QEMU. */
> -    if (kvm_enabled() && type != S390_CPU_RESET_NORMAL) {
> -        kvm_s390_reset_vcpu(cpu);
> +    if (kvm_enabled()) {
> +        switch (type) {
> +        case S390_CPU_RESET_CLEAR:
> +            kvm_s390_reset_vcpu_clear(cpu);
> +            break;
> +        case S390_CPU_RESET_INITIAL:
> +            kvm_s390_reset_vcpu_initial(cpu);
> +            break;
> +        case S390_CPU_RESET_NORMAL:
> +            kvm_s390_reset_vcpu_normal(cpu);
> +            break;
> +        }
>      }
>  }
>  
> diff --git a/target/s390x/kvm-stub.c b/target/s390x/kvm-stub.c
> index 5152e2bdf1..c4cd497f85 100644
> --- a/target/s390x/kvm-stub.c
> +++ b/target/s390x/kvm-stub.c
> @@ -83,7 +83,15 @@ void kvm_s390_cmma_reset(void)
>  {
>  }
>  
> -void kvm_s390_reset_vcpu(S390CPU *cpu)
> +void kvm_s390_reset_vcpu_initial(S390CPU *cpu)
> +{
> +}
> +
> +void kvm_s390_reset_vcpu_clear(S390CPU *cpu)
> +{
> +}
> +
> +void kvm_s390_reset_vcpu_normal(S390CPU *cpu)
>  {
>  }
>  
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index ad6e38c876..7a2ec8b9f8 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -151,6 +151,7 @@ static int cap_s390_irq;
>  static int cap_ri;
>  static int cap_gs;
>  static int cap_hpage_1m;
> +static int cap_vcpu_resets;
>  
>  static int active_cmma;
>  
> @@ -342,6 +343,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>      cap_async_pf = kvm_check_extension(s, KVM_CAP_ASYNC_PF);
>      cap_mem_op = kvm_check_extension(s, KVM_CAP_S390_MEM_OP);
>      cap_s390_irq = kvm_check_extension(s, KVM_CAP_S390_INJECT_IRQ);
> +    cap_vcpu_resets = kvm_check_extension(s, KVM_CAP_S390_VCPU_RESETS);
>  
>      if (!kvm_check_extension(s, KVM_CAP_S390_GMAP)
>          || !kvm_check_extension(s, KVM_CAP_S390_COW)) {
> @@ -403,20 +405,44 @@ int kvm_arch_destroy_vcpu(CPUState *cs)
>      return 0;
>  }
>  
> -void kvm_s390_reset_vcpu(S390CPU *cpu)
> +static void kvm_s390_reset_vcpu(S390CPU *cpu, unsigned long type)
>  {
>      CPUState *cs = CPU(cpu);
>  
> -    /* The initial reset call is needed here to reset in-kernel
> -     * vcpu data that we can't access directly from QEMU
> -     * (i.e. with older kernels which don't support sync_regs/ONE_REG).
> -     * Before this ioctl cpu_synchronize_state() is called in common kvm
> -     * code (kvm-all) */
> -    if (kvm_vcpu_ioctl(cs, KVM_S390_INITIAL_RESET, NULL)) {
> -        error_report("Initial CPU reset failed on CPU %i", cs->cpu_index);
> +    /*
> +     * The reset call is needed here to reset in-kernel vcpu data that
> +     * we can't access directly from QEMU (i.e. with older kernels
> +     * which don't support sync_regs/ONE_REG).  Before this ioctl
> +     * cpu_synchronize_state() is called in common kvm code
> +     * (kvm-all).
> +     */
> +    if (kvm_vcpu_ioctl(cs, type)) {
> +        error_report("CPU reset failed on CPU %i", cs->cpu_index);
>      }
>  }
>  
> +void kvm_s390_reset_vcpu_initial(S390CPU *cpu)
> +{
> +    kvm_s390_reset_vcpu(cpu, KVM_S390_INITIAL_RESET);
> +}
> +
> +void kvm_s390_reset_vcpu_clear(S390CPU *cpu)
> +{
> +    if (!cap_vcpu_resets) {
> +        kvm_s390_reset_vcpu(cpu, KVM_S390_INITIAL_RESET);
> +    } else {
> +        kvm_s390_reset_vcpu(cpu, KVM_S390_CLEAR_RESET);
> +    }
> +}
> +
> +void kvm_s390_reset_vcpu_normal(S390CPU *cpu)
> +{
> +    if (!cap_vcpu_resets) {
> +        return;
> +    }
> +    kvm_s390_reset_vcpu(cpu, KVM_S390_NORMAL_RESET);

if (cap_vcpu_resets) {
    kvm_s390_reset_vcpu(cpu, KVM_S390_NORMAL_RESET);
}

saves one LOC ;)

Acked-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 2/3] s390x: Add missing vcpu reset functions
  2019-12-02 14:01 ` [PATCH v2 2/3] s390x: Add missing vcpu reset functions Janosch Frank
  2019-12-02 14:22   ` David Hildenbrand
@ 2019-12-02 14:28   ` Thomas Huth
  2019-12-02 14:48   ` [PATCH v3] " Janosch Frank
  2019-12-02 15:46   ` [PATCH v2 2/3] " Cornelia Huck
  3 siblings, 0 replies; 11+ messages in thread
From: Thomas Huth @ 2019-12-02 14:28 UTC (permalink / raw)
  To: Janosch Frank, qemu-devel
  Cc: cohuck, borntraeger, qemu-s390x, mihajlov, david

On 02/12/2019 15.01, Janosch Frank wrote:
> Up to now we only had an ioctl to reset vcpu data QEMU couldn't reach
> for the initial reset, which was also called for the clear reset. To
> be architecture compliant, we also need to clear local interrupts on a
> normal reset.
> 
> Because of this and the upcoming protvirt support we need to add
> ioctls for the missing clear and normal resets.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
[...]
> @@ -403,20 +405,44 @@ int kvm_arch_destroy_vcpu(CPUState *cs)
>      return 0;
>  }
>  
> -void kvm_s390_reset_vcpu(S390CPU *cpu)
> +static void kvm_s390_reset_vcpu(S390CPU *cpu, unsigned long type)
>  {
>      CPUState *cs = CPU(cpu);
>  
> -    /* The initial reset call is needed here to reset in-kernel
> -     * vcpu data that we can't access directly from QEMU
> -     * (i.e. with older kernels which don't support sync_regs/ONE_REG).
> -     * Before this ioctl cpu_synchronize_state() is called in common kvm
> -     * code (kvm-all) */
> -    if (kvm_vcpu_ioctl(cs, KVM_S390_INITIAL_RESET, NULL)) {
> -        error_report("Initial CPU reset failed on CPU %i", cs->cpu_index);
> +    /*
> +     * The reset call is needed here to reset in-kernel vcpu data that
> +     * we can't access directly from QEMU (i.e. with older kernels
> +     * which don't support sync_regs/ONE_REG).  Before this ioctl
> +     * cpu_synchronize_state() is called in common kvm code
> +     * (kvm-all).
> +     */
> +    if (kvm_vcpu_ioctl(cs, type)) {
> +        error_report("CPU reset failed on CPU %i", cs->cpu_index);

In case you respin, maybe also print the "type" variable here?

>      }
>  }
>  
> +void kvm_s390_reset_vcpu_initial(S390CPU *cpu)
> +{
> +    kvm_s390_reset_vcpu(cpu, KVM_S390_INITIAL_RESET);
> +}
> +
> +void kvm_s390_reset_vcpu_clear(S390CPU *cpu)
> +{
> +    if (!cap_vcpu_resets) {
> +        kvm_s390_reset_vcpu(cpu, KVM_S390_INITIAL_RESET);
> +    } else {
> +        kvm_s390_reset_vcpu(cpu, KVM_S390_CLEAR_RESET);
> +    }

Cosmetic nit: It's a tiny bit nicer the other way round:

    if (cap_vcpu_resets) {
        kvm_s390_reset_vcpu(cpu, KVM_S390_CLEAR_RESET);
    } else {
        kvm_s390_reset_vcpu(cpu, KVM_S390_INITIAL_RESET);
    }

Anyway,
Reviewed-by: Thomas Huth <thuth@redhat.com>



^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v3] s390x: Add missing vcpu reset functions
  2019-12-02 14:01 ` [PATCH v2 2/3] s390x: Add missing vcpu reset functions Janosch Frank
  2019-12-02 14:22   ` David Hildenbrand
  2019-12-02 14:28   ` Thomas Huth
@ 2019-12-02 14:48   ` " Janosch Frank
  2019-12-02 15:46   ` [PATCH v2 2/3] " Cornelia Huck
  3 siblings, 0 replies; 11+ messages in thread
From: Janosch Frank @ 2019-12-02 14:48 UTC (permalink / raw)
  To: qemu-devel; +Cc: thuth, david, cohuck, borntraeger, qemu-s390x, mihajlov

Up to now we only had an ioctl to reset vcpu data QEMU couldn't reach
for the initial reset, which was also called for the clear reset. To
be architecture compliant, we also need to clear local interrupts on a
normal reset.

Because of this and the upcoming protvirt support we need to add
ioctls for the missing clear and normal resets.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Acked-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/cpu.c       | 14 ++++++++++++--
 target/s390x/kvm-stub.c  | 10 +++++++++-
 target/s390x/kvm.c       | 42 ++++++++++++++++++++++++++++++++--------
 target/s390x/kvm_s390x.h |  4 +++-
 4 files changed, 58 insertions(+), 12 deletions(-)

diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 829ce6ad54..906285888e 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -139,8 +139,18 @@ static void s390_cpu_reset(CPUState *s, cpu_reset_type type)
     }
 
     /* Reset state inside the kernel that we cannot access yet from QEMU. */
-    if (kvm_enabled() && type != S390_CPU_RESET_NORMAL) {
-        kvm_s390_reset_vcpu(cpu);
+    if (kvm_enabled()) {
+        switch (type) {
+        case S390_CPU_RESET_CLEAR:
+            kvm_s390_reset_vcpu_clear(cpu);
+            break;
+        case S390_CPU_RESET_INITIAL:
+            kvm_s390_reset_vcpu_initial(cpu);
+            break;
+        case S390_CPU_RESET_NORMAL:
+            kvm_s390_reset_vcpu_normal(cpu);
+            break;
+        }
     }
 }
 
diff --git a/target/s390x/kvm-stub.c b/target/s390x/kvm-stub.c
index 5152e2bdf1..c4cd497f85 100644
--- a/target/s390x/kvm-stub.c
+++ b/target/s390x/kvm-stub.c
@@ -83,7 +83,15 @@ void kvm_s390_cmma_reset(void)
 {
 }
 
-void kvm_s390_reset_vcpu(S390CPU *cpu)
+void kvm_s390_reset_vcpu_initial(S390CPU *cpu)
+{
+}
+
+void kvm_s390_reset_vcpu_clear(S390CPU *cpu)
+{
+}
+
+void kvm_s390_reset_vcpu_normal(S390CPU *cpu)
 {
 }
 
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index ad6e38c876..f633472980 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -151,6 +151,7 @@ static int cap_s390_irq;
 static int cap_ri;
 static int cap_gs;
 static int cap_hpage_1m;
+static int cap_vcpu_resets;
 
 static int active_cmma;
 
@@ -342,6 +343,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
     cap_async_pf = kvm_check_extension(s, KVM_CAP_ASYNC_PF);
     cap_mem_op = kvm_check_extension(s, KVM_CAP_S390_MEM_OP);
     cap_s390_irq = kvm_check_extension(s, KVM_CAP_S390_INJECT_IRQ);
+    cap_vcpu_resets = kvm_check_extension(s, KVM_CAP_S390_VCPU_RESETS);
 
     if (!kvm_check_extension(s, KVM_CAP_S390_GMAP)
         || !kvm_check_extension(s, KVM_CAP_S390_COW)) {
@@ -403,17 +405,41 @@ int kvm_arch_destroy_vcpu(CPUState *cs)
     return 0;
 }
 
-void kvm_s390_reset_vcpu(S390CPU *cpu)
+static void kvm_s390_reset_vcpu(S390CPU *cpu, unsigned long type)
 {
     CPUState *cs = CPU(cpu);
 
-    /* The initial reset call is needed here to reset in-kernel
-     * vcpu data that we can't access directly from QEMU
-     * (i.e. with older kernels which don't support sync_regs/ONE_REG).
-     * Before this ioctl cpu_synchronize_state() is called in common kvm
-     * code (kvm-all) */
-    if (kvm_vcpu_ioctl(cs, KVM_S390_INITIAL_RESET, NULL)) {
-        error_report("Initial CPU reset failed on CPU %i", cs->cpu_index);
+    /*
+     * The reset call is needed here to reset in-kernel vcpu data that
+     * we can't access directly from QEMU (i.e. with older kernels
+     * which don't support sync_regs/ONE_REG).  Before this ioctl
+     * cpu_synchronize_state() is called in common kvm code
+     * (kvm-all).
+     */
+    if (kvm_vcpu_ioctl(cs, type)) {
+        error_report("CPU reset failed on CPU %i type %lx",
+                     cs->cpu_index, type);
+    }
+}
+
+void kvm_s390_reset_vcpu_initial(S390CPU *cpu)
+{
+    kvm_s390_reset_vcpu(cpu, KVM_S390_INITIAL_RESET);
+}
+
+void kvm_s390_reset_vcpu_clear(S390CPU *cpu)
+{
+    if (cap_vcpu_resets) {
+        kvm_s390_reset_vcpu(cpu, KVM_S390_CLEAR_RESET);
+    } else {
+        kvm_s390_reset_vcpu(cpu, KVM_S390_INITIAL_RESET);
+    }
+}
+
+void kvm_s390_reset_vcpu_normal(S390CPU *cpu)
+{
+    if (cap_vcpu_resets) {
+        kvm_s390_reset_vcpu(cpu, KVM_S390_NORMAL_RESET);
     }
 }
 
diff --git a/target/s390x/kvm_s390x.h b/target/s390x/kvm_s390x.h
index caf985955b..0b21789796 100644
--- a/target/s390x/kvm_s390x.h
+++ b/target/s390x/kvm_s390x.h
@@ -34,7 +34,9 @@ int kvm_s390_assign_subch_ioeventfd(EventNotifier *notifier, uint32_t sch,
                                     int vq, bool assign);
 int kvm_s390_cmma_active(void);
 void kvm_s390_cmma_reset(void);
-void kvm_s390_reset_vcpu(S390CPU *cpu);
+void kvm_s390_reset_vcpu_clear(S390CPU *cpu);
+void kvm_s390_reset_vcpu_normal(S390CPU *cpu);
+void kvm_s390_reset_vcpu_initial(S390CPU *cpu);
 int kvm_s390_set_mem_limit(uint64_t new_limit, uint64_t *hw_limit);
 void kvm_s390_set_max_pagesize(uint64_t pagesize, Error **errp);
 void kvm_s390_crypto_reset(void);
-- 
2.20.1



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 2/3] s390x: Add missing vcpu reset functions
  2019-12-02 14:01 ` [PATCH v2 2/3] s390x: Add missing vcpu reset functions Janosch Frank
                     ` (2 preceding siblings ...)
  2019-12-02 14:48   ` [PATCH v3] " Janosch Frank
@ 2019-12-02 15:46   ` " Cornelia Huck
  2019-12-03 13:03     ` Janosch Frank
  3 siblings, 1 reply; 11+ messages in thread
From: Cornelia Huck @ 2019-12-02 15:46 UTC (permalink / raw)
  To: Janosch Frank; +Cc: thuth, david, qemu-devel, borntraeger, qemu-s390x, mihajlov

On Mon,  2 Dec 2019 09:01:45 -0500
Janosch Frank <frankja@linux.ibm.com> wrote:

> Up to now we only had an ioctl to reset vcpu data QEMU couldn't reach
> for the initial reset, which was also called for the clear reset. To

s/which/and that/ ?

> be architecture compliant, we also need to clear local interrupts on a
> normal reset.
> 
> Because of this and the upcoming protvirt support we need to add
> ioctls for the missing clear and normal resets.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  target/s390x/cpu.c       | 14 ++++++++++++--
>  target/s390x/kvm-stub.c  | 10 +++++++++-
>  target/s390x/kvm.c       | 42 ++++++++++++++++++++++++++++++++--------
>  target/s390x/kvm_s390x.h |  4 +++-
>  4 files changed, 58 insertions(+), 12 deletions(-)
> 
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index 829ce6ad54..906285888e 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -139,8 +139,18 @@ static void s390_cpu_reset(CPUState *s, cpu_reset_type type)
>      }
>  
>      /* Reset state inside the kernel that we cannot access yet from QEMU. */

Hm, why does this comment talk about 'yet'? Did we have any plans to
change that?

> -    if (kvm_enabled() && type != S390_CPU_RESET_NORMAL) {
> -        kvm_s390_reset_vcpu(cpu);
> +    if (kvm_enabled()) {
> +        switch (type) {
> +        case S390_CPU_RESET_CLEAR:
> +            kvm_s390_reset_vcpu_clear(cpu);
> +            break;
> +        case S390_CPU_RESET_INITIAL:
> +            kvm_s390_reset_vcpu_initial(cpu);
> +            break;
> +        case S390_CPU_RESET_NORMAL:
> +            kvm_s390_reset_vcpu_normal(cpu);
> +            break;

Add a default case to catch errors?

> +        }
>      }
>  }
>  

(...)

> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index ad6e38c876..7a2ec8b9f8 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -151,6 +151,7 @@ static int cap_s390_irq;
>  static int cap_ri;
>  static int cap_gs;
>  static int cap_hpage_1m;
> +static int cap_vcpu_resets;
>  
>  static int active_cmma;
>  
> @@ -342,6 +343,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>      cap_async_pf = kvm_check_extension(s, KVM_CAP_ASYNC_PF);
>      cap_mem_op = kvm_check_extension(s, KVM_CAP_S390_MEM_OP);
>      cap_s390_irq = kvm_check_extension(s, KVM_CAP_S390_INJECT_IRQ);
> +    cap_vcpu_resets = kvm_check_extension(s, KVM_CAP_S390_VCPU_RESETS);
>  
>      if (!kvm_check_extension(s, KVM_CAP_S390_GMAP)
>          || !kvm_check_extension(s, KVM_CAP_S390_COW)) {
> @@ -403,20 +405,44 @@ int kvm_arch_destroy_vcpu(CPUState *cs)
>      return 0;
>  }
>  
> -void kvm_s390_reset_vcpu(S390CPU *cpu)
> +static void kvm_s390_reset_vcpu(S390CPU *cpu, unsigned long type)
>  {
>      CPUState *cs = CPU(cpu);
>  
> -    /* The initial reset call is needed here to reset in-kernel
> -     * vcpu data that we can't access directly from QEMU
> -     * (i.e. with older kernels which don't support sync_regs/ONE_REG).
> -     * Before this ioctl cpu_synchronize_state() is called in common kvm
> -     * code (kvm-all) */
> -    if (kvm_vcpu_ioctl(cs, KVM_S390_INITIAL_RESET, NULL)) {
> -        error_report("Initial CPU reset failed on CPU %i", cs->cpu_index);
> +    /*
> +     * The reset call is needed here to reset in-kernel vcpu data that
> +     * we can't access directly from QEMU (i.e. with older kernels
> +     * which don't support sync_regs/ONE_REG).  Before this ioctl

Is the reference to sync_regs/ONE_REG still relevant? I'm a bit
confused here, especially with regard to what we'll need for protected
virt.

> +     * cpu_synchronize_state() is called in common kvm code
> +     * (kvm-all).
> +     */
> +    if (kvm_vcpu_ioctl(cs, type)) {
> +        error_report("CPU reset failed on CPU %i", cs->cpu_index);
>      }
>  }
>  
> +void kvm_s390_reset_vcpu_initial(S390CPU *cpu)
> +{
> +    kvm_s390_reset_vcpu(cpu, KVM_S390_INITIAL_RESET);
> +}
> +
> +void kvm_s390_reset_vcpu_clear(S390CPU *cpu)
> +{
> +    if (!cap_vcpu_resets) {
> +        kvm_s390_reset_vcpu(cpu, KVM_S390_INITIAL_RESET);
> +    } else {
> +        kvm_s390_reset_vcpu(cpu, KVM_S390_CLEAR_RESET);
> +    }

kvm_s390_reset_vcpu(cpu, cap_vcpu_resets ? KVM_S390_CLEAR_RESET : KVM_S390_INITIAL_RESET);

One line, but maybe the conventional if is still better :)

> +}
> +
> +void kvm_s390_reset_vcpu_normal(S390CPU *cpu)
> +{
> +    if (!cap_vcpu_resets) {
> +        return;
> +    }
> +    kvm_s390_reset_vcpu(cpu, KVM_S390_NORMAL_RESET);
> +}
> +
>  static int can_sync_regs(CPUState *cs, int regs)
>  {
>      return cap_sync_regs && (cs->kvm_run->kvm_valid_regs & regs) == regs;
> diff --git a/target/s390x/kvm_s390x.h b/target/s390x/kvm_s390x.h
> index caf985955b..0b21789796 100644
> --- a/target/s390x/kvm_s390x.h
> +++ b/target/s390x/kvm_s390x.h
> @@ -34,7 +34,9 @@ int kvm_s390_assign_subch_ioeventfd(EventNotifier *notifier, uint32_t sch,
>                                      int vq, bool assign);
>  int kvm_s390_cmma_active(void);
>  void kvm_s390_cmma_reset(void);
> -void kvm_s390_reset_vcpu(S390CPU *cpu);
> +void kvm_s390_reset_vcpu_clear(S390CPU *cpu);
> +void kvm_s390_reset_vcpu_normal(S390CPU *cpu);
> +void kvm_s390_reset_vcpu_initial(S390CPU *cpu);
>  int kvm_s390_set_mem_limit(uint64_t new_limit, uint64_t *hw_limit);
>  void kvm_s390_set_max_pagesize(uint64_t pagesize, Error **errp);
>  void kvm_s390_crypto_reset(void);



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 3/3] s390x: Fix cpu normal reset ri clearing
  2019-12-02 14:01 ` [PATCH v2 3/3] s390x: Fix cpu normal reset ri clearing Janosch Frank
@ 2019-12-03 10:07   ` Christian Borntraeger
  2019-12-03 10:21   ` David Hildenbrand
  1 sibling, 0 replies; 11+ messages in thread
From: Christian Borntraeger @ 2019-12-03 10:07 UTC (permalink / raw)
  To: Janosch Frank, qemu-devel; +Cc: cohuck, thuth, mihajlov, qemu-s390x, david



On 02.12.19 15:01, Janosch Frank wrote:
> As it turns out we need to clear the ri controls and PSW enablement
> bit to be architecture compliant.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  target/s390x/cpu.c | 5 +++++
>  target/s390x/cpu.h | 7 ++++++-
>  2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index 906285888e..c192e6b3b9 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -131,6 +131,11 @@ static void s390_cpu_reset(CPUState *s, cpu_reset_type type)
>                                    &env->fpu_status);
>         /* fall through */

As this is a fall through, do we want to change the INITIAL RESET to only clear up to 
start_normal_reset_fields, e.g. something like this on top

diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 829ce6ad5491..58ac721687a9 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -108,7 +108,7 @@ static void s390_cpu_reset(CPUState *s, cpu_reset_type type)
     case S390_CPU_RESET_INITIAL:
         /* initial reset does not clear everything! */
         memset(&env->start_initial_reset_fields, 0,
-               offsetof(CPUS390XState, end_reset_fields) -
+               offsetof(CPUS390XState, start_normal_reset_fields) -
                offsetof(CPUS390XState, start_initial_reset_fields));
 
         /* architectured initial value for Breaking-Event-Address register */


to avoid double memsetting. 


Other than this question
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>


>      case S390_CPU_RESET_NORMAL:
> +        env->psw.mask &= ~PSW_MASK_RI;
> +        memset(&env->start_normal_reset_fields, 0,
> +               offsetof(CPUS390XState, end_reset_fields) -
> +               offsetof(CPUS390XState, start_normal_reset_fields));
> +
>          env->pfault_token = -1UL;
>          env->bpbc = false;
>          break;
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index d5e18b096e..7f5fa1d35b 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -58,7 +58,6 @@ struct CPUS390XState {
>       */
>      uint64_t vregs[32][2] QEMU_ALIGNED(16);  /* vector registers */
>      uint32_t aregs[16];    /* access registers */
> -    uint8_t riccb[64];     /* runtime instrumentation control */
>      uint64_t gscb[4];      /* guarded storage control */
>      uint64_t etoken;       /* etoken */
>      uint64_t etoken_extension; /* etoken extension */
> @@ -114,6 +113,10 @@ struct CPUS390XState {
>      uint64_t gbea;
>      uint64_t pp;
>  
> +    /* Fields up to this point are not cleared by normal CPU reset */
> +    struct {} start_normal_reset_fields;
> +    uint8_t riccb[64];     /* runtime instrumentation control */
> +
>      /* Fields up to this point are cleared by a CPU reset */
>      struct {} end_reset_fields;
>  
> @@ -252,6 +255,7 @@ extern const VMStateDescription vmstate_s390_cpu;
>  #undef PSW_SHIFT_ASC
>  #undef PSW_MASK_CC
>  #undef PSW_MASK_PM
> +#undef PSW_MASK_RI
>  #undef PSW_SHIFT_MASK_PM
>  #undef PSW_MASK_64
>  #undef PSW_MASK_32
> @@ -274,6 +278,7 @@ extern const VMStateDescription vmstate_s390_cpu;
>  #define PSW_MASK_CC             0x0000300000000000ULL
>  #define PSW_MASK_PM             0x00000F0000000000ULL
>  #define PSW_SHIFT_MASK_PM       40
> +#define PSW_MASK_RI             0x0000008000000000ULL
>  #define PSW_MASK_64             0x0000000100000000ULL
>  #define PSW_MASK_32             0x0000000080000000ULL
>  #define PSW_MASK_ESA_ADDR       0x000000007fffffffULL
> 



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 3/3] s390x: Fix cpu normal reset ri clearing
  2019-12-02 14:01 ` [PATCH v2 3/3] s390x: Fix cpu normal reset ri clearing Janosch Frank
  2019-12-03 10:07   ` Christian Borntraeger
@ 2019-12-03 10:21   ` David Hildenbrand
  1 sibling, 0 replies; 11+ messages in thread
From: David Hildenbrand @ 2019-12-03 10:21 UTC (permalink / raw)
  To: Janosch Frank, qemu-devel
  Cc: borntraeger, thuth, mihajlov, qemu-s390x, cohuck

On 02.12.19 15:01, Janosch Frank wrote:
> As it turns out we need to clear the ri controls and PSW enablement
> bit to be architecture compliant.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  target/s390x/cpu.c | 5 +++++
>  target/s390x/cpu.h | 7 ++++++-
>  2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index 906285888e..c192e6b3b9 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -131,6 +131,11 @@ static void s390_cpu_reset(CPUState *s, cpu_reset_type type)
>                                    &env->fpu_status);
>         /* fall through */
>      case S390_CPU_RESET_NORMAL:
> +        env->psw.mask &= ~PSW_MASK_RI;
> +        memset(&env->start_normal_reset_fields, 0,
> +               offsetof(CPUS390XState, end_reset_fields) -
> +               offsetof(CPUS390XState, start_normal_reset_fields));
> +
>          env->pfault_token = -1UL;
>          env->bpbc = false;
>          break;
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index d5e18b096e..7f5fa1d35b 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -58,7 +58,6 @@ struct CPUS390XState {
>       */
>      uint64_t vregs[32][2] QEMU_ALIGNED(16);  /* vector registers */
>      uint32_t aregs[16];    /* access registers */
> -    uint8_t riccb[64];     /* runtime instrumentation control */
>      uint64_t gscb[4];      /* guarded storage control */
>      uint64_t etoken;       /* etoken */
>      uint64_t etoken_extension; /* etoken extension */
> @@ -114,6 +113,10 @@ struct CPUS390XState {
>      uint64_t gbea;
>      uint64_t pp;
>  
> +    /* Fields up to this point are not cleared by normal CPU reset */
> +    struct {} start_normal_reset_fields;
> +    uint8_t riccb[64];     /* runtime instrumentation control */
> +
>      /* Fields up to this point are cleared by a CPU reset */
>      struct {} end_reset_fields;
>  
> @@ -252,6 +255,7 @@ extern const VMStateDescription vmstate_s390_cpu;
>  #undef PSW_SHIFT_ASC
>  #undef PSW_MASK_CC
>  #undef PSW_MASK_PM
> +#undef PSW_MASK_RI
>  #undef PSW_SHIFT_MASK_PM
>  #undef PSW_MASK_64
>  #undef PSW_MASK_32
> @@ -274,6 +278,7 @@ extern const VMStateDescription vmstate_s390_cpu;
>  #define PSW_MASK_CC             0x0000300000000000ULL
>  #define PSW_MASK_PM             0x00000F0000000000ULL
>  #define PSW_SHIFT_MASK_PM       40
> +#define PSW_MASK_RI             0x0000008000000000ULL
>  #define PSW_MASK_64             0x0000000100000000ULL
>  #define PSW_MASK_32             0x0000000080000000ULL
>  #define PSW_MASK_ESA_ADDR       0x000000007fffffffULL
> 

I'm afraid I can't help because the documentation is confidential. It
does look sane to me, at least.

-- 
Thanks,

David / dhildenb



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 2/3] s390x: Add missing vcpu reset functions
  2019-12-02 15:46   ` [PATCH v2 2/3] " Cornelia Huck
@ 2019-12-03 13:03     ` Janosch Frank
  0 siblings, 0 replies; 11+ messages in thread
From: Janosch Frank @ 2019-12-03 13:03 UTC (permalink / raw)
  To: qemu-devel

[-- Attachment #1.1: Type: text/plain, Size: 6001 bytes --]

On 12/2/19 4:46 PM, Cornelia Huck wrote:
> On Mon,  2 Dec 2019 09:01:45 -0500
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> Up to now we only had an ioctl to reset vcpu data QEMU couldn't reach
>> for the initial reset, which was also called for the clear reset. To
> 
> s/which/and that/ ?

Ok

> 
>> be architecture compliant, we also need to clear local interrupts on a
>> normal reset.
>>
>> Because of this and the upcoming protvirt support we need to add
>> ioctls for the missing clear and normal resets.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  target/s390x/cpu.c       | 14 ++++++++++++--
>>  target/s390x/kvm-stub.c  | 10 +++++++++-
>>  target/s390x/kvm.c       | 42 ++++++++++++++++++++++++++++++++--------
>>  target/s390x/kvm_s390x.h |  4 +++-
>>  4 files changed, 58 insertions(+), 12 deletions(-)
>>
>> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
>> index 829ce6ad54..906285888e 100644
>> --- a/target/s390x/cpu.c
>> +++ b/target/s390x/cpu.c
>> @@ -139,8 +139,18 @@ static void s390_cpu_reset(CPUState *s, cpu_reset_type type)
>>      }
>>  
>>      /* Reset state inside the kernel that we cannot access yet from QEMU. */
> 
> Hm, why does this comment talk about 'yet'? Did we have any plans to
> change that?

You're asking the wrong person :)

> 
>> -    if (kvm_enabled() && type != S390_CPU_RESET_NORMAL) {
>> -        kvm_s390_reset_vcpu(cpu);
>> +    if (kvm_enabled()) {
>> +        switch (type) {
>> +        case S390_CPU_RESET_CLEAR:
>> +            kvm_s390_reset_vcpu_clear(cpu);
>> +            break;
>> +        case S390_CPU_RESET_INITIAL:
>> +            kvm_s390_reset_vcpu_initial(cpu);
>> +            break;
>> +        case S390_CPU_RESET_NORMAL:
>> +            kvm_s390_reset_vcpu_normal(cpu);
>> +            break;
> 
> Add a default case to catch errors?

Sure, just did

> 
>> +        }
>>      }
>>  }
>>  
> 
> (...)
> 
>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>> index ad6e38c876..7a2ec8b9f8 100644
>> --- a/target/s390x/kvm.c
>> +++ b/target/s390x/kvm.c
>> @@ -151,6 +151,7 @@ static int cap_s390_irq;
>>  static int cap_ri;
>>  static int cap_gs;
>>  static int cap_hpage_1m;
>> +static int cap_vcpu_resets;
>>  
>>  static int active_cmma;
>>  
>> @@ -342,6 +343,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>>      cap_async_pf = kvm_check_extension(s, KVM_CAP_ASYNC_PF);
>>      cap_mem_op = kvm_check_extension(s, KVM_CAP_S390_MEM_OP);
>>      cap_s390_irq = kvm_check_extension(s, KVM_CAP_S390_INJECT_IRQ);
>> +    cap_vcpu_resets = kvm_check_extension(s, KVM_CAP_S390_VCPU_RESETS);
>>  
>>      if (!kvm_check_extension(s, KVM_CAP_S390_GMAP)
>>          || !kvm_check_extension(s, KVM_CAP_S390_COW)) {
>> @@ -403,20 +405,44 @@ int kvm_arch_destroy_vcpu(CPUState *cs)
>>      return 0;
>>  }
>>  
>> -void kvm_s390_reset_vcpu(S390CPU *cpu)
>> +static void kvm_s390_reset_vcpu(S390CPU *cpu, unsigned long type)
>>  {
>>      CPUState *cs = CPU(cpu);
>>  
>> -    /* The initial reset call is needed here to reset in-kernel
>> -     * vcpu data that we can't access directly from QEMU
>> -     * (i.e. with older kernels which don't support sync_regs/ONE_REG).
>> -     * Before this ioctl cpu_synchronize_state() is called in common kvm
>> -     * code (kvm-all) */
>> -    if (kvm_vcpu_ioctl(cs, KVM_S390_INITIAL_RESET, NULL)) {
>> -        error_report("Initial CPU reset failed on CPU %i", cs->cpu_index);
>> +    /*
>> +     * The reset call is needed here to reset in-kernel vcpu data that
>> +     * we can't access directly from QEMU (i.e. with older kernels
>> +     * which don't support sync_regs/ONE_REG).  Before this ioctl
> 
> Is the reference to sync_regs/ONE_REG still relevant? I'm a bit
> confused here, especially with regard to what we'll need for protected
> virt.

I just didn't want to move/remove stuff
Even with kvm_run we do a lot of stuff for the initial reset.

> 
>> +     * cpu_synchronize_state() is called in common kvm code
>> +     * (kvm-all).
>> +     */
>> +    if (kvm_vcpu_ioctl(cs, type)) {
>> +        error_report("CPU reset failed on CPU %i", cs->cpu_index);
>>      }
>>  }
>>  
>> +void kvm_s390_reset_vcpu_initial(S390CPU *cpu)
>> +{
>> +    kvm_s390_reset_vcpu(cpu, KVM_S390_INITIAL_RESET);
>> +}
>> +
>> +void kvm_s390_reset_vcpu_clear(S390CPU *cpu)
>> +{
>> +    if (!cap_vcpu_resets) {
>> +        kvm_s390_reset_vcpu(cpu, KVM_S390_INITIAL_RESET);
>> +    } else {
>> +        kvm_s390_reset_vcpu(cpu, KVM_S390_CLEAR_RESET);
>> +    }
> 
> kvm_s390_reset_vcpu(cpu, cap_vcpu_resets ? KVM_S390_CLEAR_RESET : KVM_S390_INITIAL_RESET);
> 
> One line, but maybe the conventional if is still better :)

I'd like to keep it as is.

> 
>> +}
>> +
>> +void kvm_s390_reset_vcpu_normal(S390CPU *cpu)
>> +{
>> +    if (!cap_vcpu_resets) {
>> +        return;
>> +    }
>> +    kvm_s390_reset_vcpu(cpu, KVM_S390_NORMAL_RESET);
>> +}
>> +
>>  static int can_sync_regs(CPUState *cs, int regs)
>>  {
>>      return cap_sync_regs && (cs->kvm_run->kvm_valid_regs & regs) == regs;
>> diff --git a/target/s390x/kvm_s390x.h b/target/s390x/kvm_s390x.h
>> index caf985955b..0b21789796 100644
>> --- a/target/s390x/kvm_s390x.h
>> +++ b/target/s390x/kvm_s390x.h
>> @@ -34,7 +34,9 @@ int kvm_s390_assign_subch_ioeventfd(EventNotifier *notifier, uint32_t sch,
>>                                      int vq, bool assign);
>>  int kvm_s390_cmma_active(void);
>>  void kvm_s390_cmma_reset(void);
>> -void kvm_s390_reset_vcpu(S390CPU *cpu);
>> +void kvm_s390_reset_vcpu_clear(S390CPU *cpu);
>> +void kvm_s390_reset_vcpu_normal(S390CPU *cpu);
>> +void kvm_s390_reset_vcpu_initial(S390CPU *cpu);
>>  int kvm_s390_set_mem_limit(uint64_t new_limit, uint64_t *hw_limit);
>>  void kvm_s390_set_max_pagesize(uint64_t pagesize, Error **errp);
>>  void kvm_s390_crypto_reset(void);
> 
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, back to index

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-02 14:01 [PATCH v2 0/3] s390x: Increase architectural compliance Janosch Frank
2019-12-02 14:01 ` [PATCH v2 1/3] Header sync Janosch Frank
2019-12-02 14:01 ` [PATCH v2 2/3] s390x: Add missing vcpu reset functions Janosch Frank
2019-12-02 14:22   ` David Hildenbrand
2019-12-02 14:28   ` Thomas Huth
2019-12-02 14:48   ` [PATCH v3] " Janosch Frank
2019-12-02 15:46   ` [PATCH v2 2/3] " Cornelia Huck
2019-12-03 13:03     ` Janosch Frank
2019-12-02 14:01 ` [PATCH v2 3/3] s390x: Fix cpu normal reset ri clearing Janosch Frank
2019-12-03 10:07   ` Christian Borntraeger
2019-12-03 10:21   ` David Hildenbrand

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git