[PATCH 0/3] s390x: Increase architectural compliance

[PATCH 0/3] s390x: Increase architectural compliance

[Janosch Frank]

On a diag 308 subcode 0 and 1 we need to load the whole reset PSW and
not just the address.

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.

Patches are also in my cleanup branch.

Janosch Frank (3):
  s390x: Properly fetch and test the short psw on diag308 subc 0/1
  Sync reset
  s390x: protvirt: Add new VCPU reset functions

 linux-headers/linux/kvm.h |  7 +++++++
 target/s390x/cpu.c        | 26 ++++++++++++++++++++++----
 target/s390x/cpu.h        |  1 +
 target/s390x/kvm-stub.c   | 10 +++++++++-
 target/s390x/kvm.c        | 38 ++++++++++++++++++++++++++++++++------
 target/s390x/kvm_s390x.h  |  4 +++-
 6 files changed, 74 insertions(+), 12 deletions(-)

-- 
2.20.1

[PATCH 1/3] s390x: Properly fetch and test the short psw on diag308 subc 0/1

[Janosch Frank]

We need to actually fetch the cpu mask and set it. As we invert the
short psw indication in the mask, SIE will report a specification
exception, if it wasn't present in the reset psw.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
---
 target/s390x/cpu.c | 12 ++++++++++--
 target/s390x/cpu.h |  1 +
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index bd39cb54b7..829ce6ad54 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -76,8 +76,16 @@ static bool s390_cpu_has_work(CPUState *cs)
 static void s390_cpu_load_normal(CPUState *s)
 {
     S390CPU *cpu = S390_CPU(s);
-    cpu->env.psw.addr = ldl_phys(s->as, 4) & PSW_MASK_ESA_ADDR;
-    cpu->env.psw.mask = PSW_MASK_32 | PSW_MASK_64;
+    uint64_t spsw = ldq_phys(s->as, 0);
+
+    cpu->env.psw.mask = spsw & 0xffffffff80000000ULL;
+    /*
+     * Invert short psw indication, so SIE will report a specification
+     * exception if it was not set.
+     */
+    cpu->env.psw.mask ^= PSW_MASK_SHORTPSW;
+    cpu->env.psw.addr = spsw & 0x7fffffffULL;
+
     s390_cpu_set_state(S390_CPU_STATE_OPERATING, cpu);
 }
 #endif
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index d2af13b345..d5e18b096e 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -265,6 +265,7 @@ extern const VMStateDescription vmstate_s390_cpu;
 #define PSW_MASK_EXT            0x0100000000000000ULL
 #define PSW_MASK_KEY            0x00F0000000000000ULL
 #define PSW_SHIFT_KEY           52
+#define PSW_MASK_SHORTPSW       0x0008000000000000ULL
 #define PSW_MASK_MCHECK         0x0004000000000000ULL
 #define PSW_MASK_WAIT           0x0002000000000000ULL
 #define PSW_MASK_PSTATE         0x0001000000000000ULL
-- 
2.20.1

Re: [PATCH 1/3] s390x: Properly fetch and test the short psw on diag308 subc 0/1

[Cornelia Huck]

On Fri, 29 Nov 2019 09:20:23 -0500
Janosch Frank <frankja@linux.ibm.com> wrote:

> We need to actually fetch the cpu mask and set it. As we invert the
> short psw indication in the mask, SIE will report a specification
> exception, if it wasn't present in the reset psw.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> ---
>  target/s390x/cpu.c | 12 ++++++++++--
>  target/s390x/cpu.h |  1 +
>  2 files changed, 11 insertions(+), 2 deletions(-)

Thanks, applied.

Re: [PATCH 1/3] s390x: Properly fetch and test the short psw on diag308 subc 0/1

[Cornelia Huck]

On Fri, 29 Nov 2019 09:20:23 -0500
Janosch Frank <frankja@linux.ibm.com> wrote:

> We need to actually fetch the cpu mask and set it. As we invert the
> short psw indication in the mask, SIE will report a specification
> exception, if it wasn't present in the reset psw.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Reviewed-by: David Hildenbrand <david@redhat.com>
> ---
>  target/s390x/cpu.c | 12 ++++++++++--
>  target/s390x/cpu.h |  1 +
>  2 files changed, 11 insertions(+), 2 deletions(-)

Thanks, requeued.

[PATCH 2/3] Sync reset

[Janosch Frank]

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

diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
index 3d9b18f7f8..3dd7c407e0 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 180
 
 #ifdef KVM_CAP_IRQ_ROUTING
 
@@ -1461,6 +1462,12 @@ struct kvm_enc_region {
 /* Available with KVM_CAP_ARM_SVE */
 #define KVM_ARM_VCPU_FINALIZE	  _IOW(KVMIO,  0xc2, int)
 
+#define KVM_S390_VCPU_RESET_NORMAL	0
+#define KVM_S390_VCPU_RESET_INITIAL	1
+#define KVM_S390_VCPU_RESET_CLEAR	2
+
+#define KVM_S390_VCPU_RESET    _IO(KVMIO,   0xc3)
+
 /* Secure Encrypted Virtualization command */
 enum sev_cmd_id {
 	/* Guest initialization commands */
-- 
2.20.1

[PATCH 3/3] s390x: protvirt: Add new VCPU reset functions

[Janosch Frank]

CPU resets for protected guests need to be done via Ultravisor
calls. Hence we need a way to issue these calls for each reset.

As we formerly had only one reset function and it was called for
initial, as well as for the clear reset, we now need a new interface.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 target/s390x/cpu.c       | 14 ++++++++++++--
 target/s390x/kvm-stub.c  | 10 +++++++++-
 target/s390x/kvm.c       | 38 ++++++++++++++++++++++++++++++++------
 target/s390x/kvm_s390x.h |  4 +++-
 4 files changed, 56 insertions(+), 10 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..502fc71664 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) */
+    /*
+     * 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 (cap_vcpu_resets) {
+        if (kvm_vcpu_ioctl(cs, KVM_S390_VCPU_RESET, type)) {
+            error_report("CPU reset type %ld failed on CPU %i",
+                         type, cs->cpu_index);
+        }
+        return;
+    }
     if (kvm_vcpu_ioctl(cs, KVM_S390_INITIAL_RESET, NULL)) {
         error_report("Initial CPU reset failed on CPU %i", cs->cpu_index);
     }
 }
 
+void kvm_s390_reset_vcpu_initial(S390CPU *cpu)
+{
+    kvm_s390_reset_vcpu(cpu, KVM_S390_VCPU_RESET_INITIAL);
+}
+
+void kvm_s390_reset_vcpu_clear(S390CPU *cpu)
+{
+    kvm_s390_reset_vcpu(cpu, KVM_S390_VCPU_RESET_CLEAR);
+}
+
+void kvm_s390_reset_vcpu_normal(S390CPU *cpu)
+{
+    kvm_s390_reset_vcpu(cpu, KVM_S390_VCPU_RESET_NORMAL);
+}
+
 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

Re: [PATCH 3/3] s390x: protvirt: Add new VCPU reset functions

[David Hildenbrand]

On 29.11.19 15:20, Janosch Frank wrote:
> CPU resets for protected guests need to be done via Ultravisor
> calls. Hence we need a way to issue these calls for each reset.
> 
> As we formerly had only one reset function and it was called for
> initial, as well as for the clear reset, we now need a new interface.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  target/s390x/cpu.c       | 14 ++++++++++++--
>  target/s390x/kvm-stub.c  | 10 +++++++++-
>  target/s390x/kvm.c       | 38 ++++++++++++++++++++++++++++++++------
>  target/s390x/kvm_s390x.h |  4 +++-
>  4 files changed, 56 insertions(+), 10 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..502fc71664 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) */
> +    /*
> +     * 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 (cap_vcpu_resets) {
> +        if (kvm_vcpu_ioctl(cs, KVM_S390_VCPU_RESET, type)) {
> +            error_report("CPU reset type %ld failed on CPU %i",
> +                         type, cs->cpu_index);
> +        }
> +        return;
> +    }
>      if (kvm_vcpu_ioctl(cs, KVM_S390_INITIAL_RESET, NULL)) {
>          error_report("Initial CPU reset failed on CPU %i", cs->cpu_index);
>      }
>  }

I think I commented this already: The issue is that without
cap_vcpu_requests, you would no do a KVM_S390_INITIAL_RESET for type ==
S390_CPU_RESET_NORMAL. You have to fence that.


if (type != S390_CPU_RESET_NORMAL &&
    kvm_vcpu_ioctl(cs, KVM_S390_INITIAL_RESET, NULL)) {
	...
}

-- 
Thanks,

David / dhildenb

Re: [PATCH 3/3] s390x: protvirt: Add new VCPU reset functions

[Janosch Frank]

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

On 11/29/19 3:24 PM, David Hildenbrand wrote:
> On 29.11.19 15:20, Janosch Frank wrote:
>> CPU resets for protected guests need to be done via Ultravisor
>> calls. Hence we need a way to issue these calls for each reset.
>>
>> As we formerly had only one reset function and it was called for
>> initial, as well as for the clear reset, we now need a new interface.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  target/s390x/cpu.c       | 14 ++++++++++++--
>>  target/s390x/kvm-stub.c  | 10 +++++++++-
>>  target/s390x/kvm.c       | 38 ++++++++++++++++++++++++++++++++------
>>  target/s390x/kvm_s390x.h |  4 +++-
>>  4 files changed, 56 insertions(+), 10 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..502fc71664 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) */
>> +    /*
>> +     * 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 (cap_vcpu_resets) {
>> +        if (kvm_vcpu_ioctl(cs, KVM_S390_VCPU_RESET, type)) {
>> +            error_report("CPU reset type %ld failed on CPU %i",
>> +                         type, cs->cpu_index);
>> +        }
>> +        return;
>> +    }
>>      if (kvm_vcpu_ioctl(cs, KVM_S390_INITIAL_RESET, NULL)) {
>>          error_report("Initial CPU reset failed on CPU %i", cs->cpu_index);
>>      }
>>  }
> 
> I think I commented this already: The issue is that without
> cap_vcpu_requests, you would no do a KVM_S390_INITIAL_RESET for type ==
> S390_CPU_RESET_NORMAL. You have to fence that.

I did fix that, but not on this branch -_-

> 
> 
> if (type != S390_CPU_RESET_NORMAL &&
>     kvm_vcpu_ioctl(cs, KVM_S390_INITIAL_RESET, NULL)) {
> 	...
> }
> 



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

Re: [PATCH 3/3] s390x: protvirt: Add new VCPU reset functions

[David Hildenbrand]

On 29.11.19 15:34, Janosch Frank wrote:
> On 11/29/19 3:24 PM, David Hildenbrand wrote:
>> On 29.11.19 15:20, Janosch Frank wrote:
>>> CPU resets for protected guests need to be done via Ultravisor
>>> calls. Hence we need a way to issue these calls for each reset.
>>>
>>> As we formerly had only one reset function and it was called for
>>> initial, as well as for the clear reset, we now need a new interface.
>>>
>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>> ---
>>>  target/s390x/cpu.c       | 14 ++++++++++++--
>>>  target/s390x/kvm-stub.c  | 10 +++++++++-
>>>  target/s390x/kvm.c       | 38 ++++++++++++++++++++++++++++++++------
>>>  target/s390x/kvm_s390x.h |  4 +++-
>>>  4 files changed, 56 insertions(+), 10 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..502fc71664 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) */
>>> +    /*
>>> +     * 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 (cap_vcpu_resets) {
>>> +        if (kvm_vcpu_ioctl(cs, KVM_S390_VCPU_RESET, type)) {
>>> +            error_report("CPU reset type %ld failed on CPU %i",
>>> +                         type, cs->cpu_index);
>>> +        }
>>> +        return;
>>> +    }
>>>      if (kvm_vcpu_ioctl(cs, KVM_S390_INITIAL_RESET, NULL)) {
>>>          error_report("Initial CPU reset failed on CPU %i", cs->cpu_index);
>>>      }
>>>  }
>>
>> I think I commented this already: The issue is that without
>> cap_vcpu_requests, you would no do a KVM_S390_INITIAL_RESET for type ==
>> S390_CPU_RESET_NORMAL. You have to fence that.
> 
> I did fix that, but not on this branch -_-

You can avoid that completely once you switch to two new separate ioctls ;)


-- 
Thanks,

David / dhildenb

Re: [PATCH 0/3] s390x: Increase architectural compliance

[Cornelia Huck]

On Fri, 29 Nov 2019 09:20:22 -0500
Janosch Frank <frankja@linux.ibm.com> wrote:

> On a diag 308 subcode 0 and 1 we need to load the whole reset PSW and
> not just the address.
> 
> 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.
> 
> Patches are also in my cleanup branch.
> 
> Janosch Frank (3):
>   s390x: Properly fetch and test the short psw on diag308 subc 0/1
>   Sync reset
>   s390x: protvirt: Add new VCPU reset functions
> 
>  linux-headers/linux/kvm.h |  7 +++++++
>  target/s390x/cpu.c        | 26 ++++++++++++++++++++++----
>  target/s390x/cpu.h        |  1 +
>  target/s390x/kvm-stub.c   | 10 +++++++++-
>  target/s390x/kvm.c        | 38 ++++++++++++++++++++++++++++++++------
>  target/s390x/kvm_s390x.h  |  4 +++-
>  6 files changed, 74 insertions(+), 12 deletions(-)
> 

Ok, it seems I should just go ahead and pick patch 1, and defer the
remainder until after we agree on the interface, right?

Re: [PATCH 0/3] s390x: Increase architectural compliance

[Janosch Frank]

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

On 12/2/19 9:57 AM, Cornelia Huck wrote:
> On Fri, 29 Nov 2019 09:20:22 -0500
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> On a diag 308 subcode 0 and 1 we need to load the whole reset PSW and
>> not just the address.
>>
>> 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.
>>
>> Patches are also in my cleanup branch.
>>
>> Janosch Frank (3):
>>   s390x: Properly fetch and test the short psw on diag308 subc 0/1
>>   Sync reset
>>   s390x: protvirt: Add new VCPU reset functions
>>
>>  linux-headers/linux/kvm.h |  7 +++++++
>>  target/s390x/cpu.c        | 26 ++++++++++++++++++++++----
>>  target/s390x/cpu.h        |  1 +
>>  target/s390x/kvm-stub.c   | 10 +++++++++-
>>  target/s390x/kvm.c        | 38 ++++++++++++++++++++++++++++++++------
>>  target/s390x/kvm_s390x.h  |  4 +++-
>>  6 files changed, 74 insertions(+), 12 deletions(-)
>>
> 
> Ok, it seems I should just go ahead and pick patch 1, and defer the
> remainder until after we agree on the interface, right?
> 

Yes. I'm currently reworking and testing the reset patch and there's one
more patch that'll come into this series, as I found another problem.


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