[PATCH v3 0/4] s390x: Increase architectural compliance

[PATCH v3 0/4] s390x: Increase architectural compliance

[Janosch Frank]

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.
And we need to set the short psw bit indication in the bios when doing
a reset.

Patches are also in my cleanup branch.
v2 of the KVM patch for the resets will be posted soon.

Janosch Frank (4):
  Header sync
  s390x: Add missing vcpu reset functions
  s390x: Fix cpu normal reset ri clearing
  pc-bios/s390x: Fix reset psw mask

 linux-headers/linux/kvm.h   |  4 ++++
 pc-bios/s390-ccw/jump2ipl.c | 12 ++++++-----
 target/s390x/cpu.c          | 23 +++++++++++++++++---
 target/s390x/cpu.h          |  7 ++++++-
 target/s390x/kvm-stub.c     | 10 ++++++++-
 target/s390x/kvm.c          | 42 ++++++++++++++++++++++++++++++-------
 target/s390x/kvm_s390x.h    |  4 +++-
 7 files changed, 83 insertions(+), 19 deletions(-)

-- 
2.20.1

[PATCH v3 1/4] Header sync

[Janosch Frank]

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 180
 
 #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

[PATCH v3 2/4] s390x: Add missing vcpu reset functions

[Janosch Frank]

Up to now we only had an ioctl to reset vcpu data QEMU couldn't reach
for the initial reset, and that 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       | 16 +++++++++++++--
 target/s390x/kvm-stub.c  | 10 +++++++++-
 target/s390x/kvm.c       | 42 ++++++++++++++++++++++++++++++++--------
 target/s390x/kvm_s390x.h |  4 +++-
 4 files changed, 60 insertions(+), 12 deletions(-)

diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 829ce6ad54..4973365d6c 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -139,8 +139,20 @@ 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;
+        default:
+            g_assert_not_reached();
+        }
     }
 }
 
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

[PATCH v3 3/4] s390x: Fix cpu normal reset ri clearing

[Janosch Frank]

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>
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 target/s390x/cpu.c | 7 ++++++-
 target/s390x/cpu.h | 7 ++++++-
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 4973365d6c..c282c36b69 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 */
@@ -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

[PATCH v3 4/4] pc-bios/s390x: Fix reset psw mask

[Janosch Frank]

We need to set the short psw indication bit in the reset psw, as it is
a short psw.

fixes: 9629823290 ("pc-bios/s390-ccw: do a subsystem reset before running the guest")
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 pc-bios/s390-ccw/jump2ipl.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
index 266f1502b9..da13c43cc0 100644
--- a/pc-bios/s390-ccw/jump2ipl.c
+++ b/pc-bios/s390-ccw/jump2ipl.c
@@ -12,11 +12,11 @@
 #define KERN_IMAGE_START 0x010000UL
 #define PSW_MASK_64 0x0000000100000000ULL
 #define PSW_MASK_32 0x0000000080000000ULL
-#define IPL_PSW_MASK (PSW_MASK_32 | PSW_MASK_64)
+#define PSW_MASK_SHORTPSW 0x0008000000000000ULL
+#define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_32 | PSW_MASK_64)
 
 typedef struct ResetInfo {
-    uint32_t ipl_mask;
-    uint32_t ipl_addr;
+    uint64_t ipl_psw;
     uint32_t ipl_continue;
 } ResetInfo;
 
@@ -50,7 +50,9 @@ void jump_to_IPL_code(uint64_t address)
     ResetInfo *current = 0;
 
     save = *current;
-    current->ipl_addr = (uint32_t) (uint64_t) &jump_to_IPL_2;
+
+    current->ipl_psw = (uint64_t) &jump_to_IPL_2;
+    current->ipl_psw |= RESET_PSW_MASK;
     current->ipl_continue = address & 0x7fffffff;
 
     debug_print_int("set IPL addr to", current->ipl_continue);
@@ -82,7 +84,7 @@ void jump_to_low_kernel(void)
     }
 
     /* Trying to get PSW at zero address */
-    if (*((uint64_t *)0) & IPL_PSW_MASK) {
+    if (*((uint64_t *)0) & RESET_PSW_MASK) {
         jump_to_IPL_code((*((uint64_t *)0)) & 0x7fffffff);
     }
 
-- 
2.20.1

Re: [PATCH v3 4/4] pc-bios/s390x: Fix reset psw mask

[Christian Borntraeger]

On 03.12.19 14:28, Janosch Frank wrote:
> We need to set the short psw indication bit in the reset psw, as it is
> a short psw.
> 
> fixes: 9629823290 ("pc-bios/s390-ccw: do a subsystem reset before running the guest")
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

We should also add 
commit 24bb1fa36ff7b25ee774dbe4a18830dc782b54bf (HEAD, github-cohuck/s390-next)
Author:     Janosch Frank <frankja@linux.ibm.com>
AuthorDate: Fri Nov 29 09:20:23 2019 -0500
Commit:     Cornelia Huck <cohuck@redhat.com>
CommitDate: Mon Dec 2 09:58:57 2019 +0100

    s390x: Properly fetch and test the short psw on diag308 subc 0/1

or whatever the final commit id will be. While this patch is not "broken"
it exposes the bug.



> ---
>  pc-bios/s390-ccw/jump2ipl.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
> index 266f1502b9..da13c43cc0 100644
> --- a/pc-bios/s390-ccw/jump2ipl.c
> +++ b/pc-bios/s390-ccw/jump2ipl.c
> @@ -12,11 +12,11 @@
>  #define KERN_IMAGE_START 0x010000UL
>  #define PSW_MASK_64 0x0000000100000000ULL
>  #define PSW_MASK_32 0x0000000080000000ULL
> -#define IPL_PSW_MASK (PSW_MASK_32 | PSW_MASK_64)
> +#define PSW_MASK_SHORTPSW 0x0008000000000000ULL
> +#define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_32 | PSW_MASK_64)
>  
>  typedef struct ResetInfo {
> -    uint32_t ipl_mask;
> -    uint32_t ipl_addr;
> +    uint64_t ipl_psw;
>      uint32_t ipl_continue;
>  } ResetInfo;
>  
> @@ -50,7 +50,9 @@ void jump_to_IPL_code(uint64_t address)
>      ResetInfo *current = 0;
>  
>      save = *current;
> -    current->ipl_addr = (uint32_t) (uint64_t) &jump_to_IPL_2;
> +
> +    current->ipl_psw = (uint64_t) &jump_to_IPL_2;
> +    current->ipl_psw |= RESET_PSW_MASK;
>      current->ipl_continue = address & 0x7fffffff;
>  
>      debug_print_int("set IPL addr to", current->ipl_continue);
> @@ -82,7 +84,7 @@ void jump_to_low_kernel(void)
>      }
>  
>      /* Trying to get PSW at zero address */
> -    if (*((uint64_t *)0) & IPL_PSW_MASK) {
> +    if (*((uint64_t *)0) & RESET_PSW_MASK) {
>          jump_to_IPL_code((*((uint64_t *)0)) & 0x7fffffff);
>      }
>  
> 

Re: [PATCH v3 4/4] pc-bios/s390x: Fix reset psw mask

[Cornelia Huck]

On Tue, 3 Dec 2019 14:33:25 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 03.12.19 14:28, Janosch Frank wrote:
> > We need to set the short psw indication bit in the reset psw, as it is
> > a short psw.
> > 
> > fixes: 9629823290 ("pc-bios/s390-ccw: do a subsystem reset before running the guest")
> > Signed-off-by: Janosch Frank <frankja@linux.ibm.com>  
> 
> We should also add 
> commit 24bb1fa36ff7b25ee774dbe4a18830dc782b54bf (HEAD, github-cohuck/s390-next)
> Author:     Janosch Frank <frankja@linux.ibm.com>
> AuthorDate: Fri Nov 29 09:20:23 2019 -0500
> Commit:     Cornelia Huck <cohuck@redhat.com>
> CommitDate: Mon Dec 2 09:58:57 2019 +0100
> 
>     s390x: Properly fetch and test the short psw on diag308 subc 0/1
> 
> or whatever the final commit id will be. While this patch is not "broken"
> it exposes the bug.

Probably better to use "Exposed by (...)", then?

We'll be fine as long as we have a bios rebuilt with this fix, and
anybody using the official QEMU should get that. Pointing to the commit
should be enough for any backporters.

> 
> 
> 
> > ---
> >  pc-bios/s390-ccw/jump2ipl.c | 12 +++++++-----
> >  1 file changed, 7 insertions(+), 5 deletions(-)

Re: [PATCH v3 4/4] pc-bios/s390x: Fix reset psw mask

[Cornelia Huck]

On Tue,  3 Dec 2019 08:28:13 -0500
Janosch Frank <frankja@linux.ibm.com> wrote:

> We need to set the short psw indication bit in the reset psw, as it is
> a short psw.
> 
> fixes: 9629823290 ("pc-bios/s390-ccw: do a subsystem reset before running the guest")

s/fixes: 9629823290/Fixes: 962982329029/

> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  pc-bios/s390-ccw/jump2ipl.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
> index 266f1502b9..da13c43cc0 100644
> --- a/pc-bios/s390-ccw/jump2ipl.c
> +++ b/pc-bios/s390-ccw/jump2ipl.c
> @@ -12,11 +12,11 @@
>  #define KERN_IMAGE_START 0x010000UL
>  #define PSW_MASK_64 0x0000000100000000ULL
>  #define PSW_MASK_32 0x0000000080000000ULL
> -#define IPL_PSW_MASK (PSW_MASK_32 | PSW_MASK_64)
> +#define PSW_MASK_SHORTPSW 0x0008000000000000ULL
> +#define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_32 | PSW_MASK_64)
>  
>  typedef struct ResetInfo {
> -    uint32_t ipl_mask;
> -    uint32_t ipl_addr;
> +    uint64_t ipl_psw;
>      uint32_t ipl_continue;
>  } ResetInfo;
>  
> @@ -50,7 +50,9 @@ void jump_to_IPL_code(uint64_t address)
>      ResetInfo *current = 0;
>  
>      save = *current;
> -    current->ipl_addr = (uint32_t) (uint64_t) &jump_to_IPL_2;
> +
> +    current->ipl_psw = (uint64_t) &jump_to_IPL_2;
> +    current->ipl_psw |= RESET_PSW_MASK;
>      current->ipl_continue = address & 0x7fffffff;
>  
>      debug_print_int("set IPL addr to", current->ipl_continue);
> @@ -82,7 +84,7 @@ void jump_to_low_kernel(void)
>      }
>  
>      /* Trying to get PSW at zero address */
> -    if (*((uint64_t *)0) & IPL_PSW_MASK) {
> +    if (*((uint64_t *)0) & RESET_PSW_MASK) {
>          jump_to_IPL_code((*((uint64_t *)0)) & 0x7fffffff);
>      }
>  

Looks sane to me, but would like an ack from bios maintainers.

As this is independent of the other patches (which depend on a headers
update), I can pick this and do a rebuild of the bios(es). Unless one
of the bios maintainers wants to do that and send me a pull req :), but
that seems like overkill.

Re: [PATCH v3 2/4] s390x: Add missing vcpu reset functions

[Cornelia Huck]

On Tue,  3 Dec 2019 08:28:11 -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, and that 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       | 16 +++++++++++++--
>  target/s390x/kvm-stub.c  | 10 +++++++++-
>  target/s390x/kvm.c       | 42 ++++++++++++++++++++++++++++++++--------
>  target/s390x/kvm_s390x.h |  4 +++-
>  4 files changed, 60 insertions(+), 12 deletions(-)
> 
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index 829ce6ad54..4973365d6c 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -139,8 +139,20 @@ static void s390_cpu_reset(CPUState *s, cpu_reset_type type)
>      }
>  
>      /* Reset state inside the kernel that we cannot access yet from QEMU. */

For the last iteration, I asked about the 'yet' here...

> -    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;
> +        default:
> +            g_assert_not_reached();
> +        }
>      }
>  }
>  

(...)

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

...and this reference to 'older kernels' here.

Are the comments still correct/relevant?

> +     * 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);
> +    }
> +}

Re: [PATCH v3 4/4] pc-bios/s390x: Fix reset psw mask

[Christian Borntraeger]

On 03.12.19 14:28, Janosch Frank wrote:
> We need to set the short psw indication bit in the reset psw, as it is
> a short psw.
> 
> fixes: 9629823290 ("pc-bios/s390-ccw: do a subsystem reset before running the guest")
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>

Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  pc-bios/s390-ccw/jump2ipl.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
> index 266f1502b9..da13c43cc0 100644
> --- a/pc-bios/s390-ccw/jump2ipl.c
> +++ b/pc-bios/s390-ccw/jump2ipl.c
> @@ -12,11 +12,11 @@
>  #define KERN_IMAGE_START 0x010000UL
>  #define PSW_MASK_64 0x0000000100000000ULL
>  #define PSW_MASK_32 0x0000000080000000ULL
> -#define IPL_PSW_MASK (PSW_MASK_32 | PSW_MASK_64)
> +#define PSW_MASK_SHORTPSW 0x0008000000000000ULL
> +#define RESET_PSW_MASK (PSW_MASK_SHORTPSW | PSW_MASK_32 | PSW_MASK_64)
>  
>  typedef struct ResetInfo {
> -    uint32_t ipl_mask;
> -    uint32_t ipl_addr;
> +    uint64_t ipl_psw;
>      uint32_t ipl_continue;
>  } ResetInfo;
>  
> @@ -50,7 +50,9 @@ void jump_to_IPL_code(uint64_t address)
>      ResetInfo *current = 0;
>  
>      save = *current;
> -    current->ipl_addr = (uint32_t) (uint64_t) &jump_to_IPL_2;
> +
> +    current->ipl_psw = (uint64_t) &jump_to_IPL_2;
> +    current->ipl_psw |= RESET_PSW_MASK;


>      current->ipl_continue = address & 0x7fffffff;
>  
>      debug_print_int("set IPL addr to", current->ipl_continue);
> @@ -82,7 +84,7 @@ void jump_to_low_kernel(void)
>      }
>  
>      /* Trying to get PSW at zero address */
> -    if (*((uint64_t *)0) & IPL_PSW_MASK) {
> +    if (*((uint64_t *)0) & RESET_PSW_MASK) {
>          jump_to_IPL_code((*((uint64_t *)0)) & 0x7fffffff);
>      }
>  
> 

Re: [PATCH v3 2/4] s390x: Add missing vcpu reset functions

[Janosch Frank]

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

On 12/3/19 6:44 PM, Cornelia Huck wrote:
> On Tue,  3 Dec 2019 08:28:11 -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, and that 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       | 16 +++++++++++++--
>>  target/s390x/kvm-stub.c  | 10 +++++++++-
>>  target/s390x/kvm.c       | 42 ++++++++++++++++++++++++++++++++--------
>>  target/s390x/kvm_s390x.h |  4 +++-
>>  4 files changed, 60 insertions(+), 12 deletions(-)
>>
>> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
>> index 829ce6ad54..4973365d6c 100644
>> --- a/target/s390x/cpu.c
>> +++ b/target/s390x/cpu.c
>> @@ -139,8 +139,20 @@ static void s390_cpu_reset(CPUState *s, cpu_reset_type type)
>>      }
>>  
>>      /* Reset state inside the kernel that we cannot access yet from QEMU. */
> 
> For the last iteration, I asked about the 'yet' here...

I have not written those comments, I merely refuse to delete them :)
We still reset some state in the kernel, I'm not sure how much of that
is already exposed via ioctls to QEMU, so I won't remove the comment.

> 
>> -    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;
>> +        default:
>> +            g_assert_not_reached();
>> +        }
>>      }
>>  }
>>  
> 
> (...)
> 
>> @@ -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
> 
> ...and this reference to 'older kernels' here.
> 
> Are the comments still correct/relevant?

See above

> 
>> +     * 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);
>> +    }
>> +}
> 



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

Re: [PATCH v3 2/4] s390x: Add missing vcpu reset functions

[Cornelia Huck]

[-- Attachment #1: Type: text/plain, Size: 3915 bytes --]

On Wed, 4 Dec 2019 10:00:45 +0100
Janosch Frank <frankja@linux.ibm.com> wrote:

> On 12/3/19 6:44 PM, Cornelia Huck wrote:
> > On Tue,  3 Dec 2019 08:28:11 -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, and that 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       | 16 +++++++++++++--
> >>  target/s390x/kvm-stub.c  | 10 +++++++++-
> >>  target/s390x/kvm.c       | 42 ++++++++++++++++++++++++++++++++--------
> >>  target/s390x/kvm_s390x.h |  4 +++-
> >>  4 files changed, 60 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> >> index 829ce6ad54..4973365d6c 100644
> >> --- a/target/s390x/cpu.c
> >> +++ b/target/s390x/cpu.c
> >> @@ -139,8 +139,20 @@ static void s390_cpu_reset(CPUState *s, cpu_reset_type type)
> >>      }
> >>  
> >>      /* Reset state inside the kernel that we cannot access yet from QEMU. */  
> > 
> > For the last iteration, I asked about the 'yet' here...  
> 
> I have not written those comments, I merely refuse to delete them :)
> We still reset some state in the kernel, I'm not sure how much of that
> is already exposed via ioctls to QEMU, so I won't remove the comment.
> 
> >   
> >> -    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;
> >> +        default:
> >> +            g_assert_not_reached();
> >> +        }
> >>      }
> >>  }
> >>    
> > 
> > (...)
> >   
> >> @@ -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  
> > 
> > ...and this reference to 'older kernels' here.
> > 
> > Are the comments still correct/relevant?  
> 
> See above

Fair enough, let's keep them for now and revisit this later.

> 
> >   
> >> +     * 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);
> >> +    }
> >> +}  
> >   
> 
> 


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

Re: [PATCH v3 2/4] s390x: Add missing vcpu reset functions

[Cornelia Huck]

On Tue,  3 Dec 2019 08:28:11 -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, and that was also called for the clear
> reset. To be architecture compliant, we also need to clear local
> interrupts on a normal reset.

Do we also need to do something like that for tcg? David?

> 
> 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       | 16 +++++++++++++--
>  target/s390x/kvm-stub.c  | 10 +++++++++-
>  target/s390x/kvm.c       | 42 ++++++++++++++++++++++++++++++++--------
>  target/s390x/kvm_s390x.h |  4 +++-
>  4 files changed, 60 insertions(+), 12 deletions(-)

Re: [PATCH v3 2/4] s390x: Add missing vcpu reset functions

[David Hildenbrand]

On 04.12.19 15:59, Cornelia Huck wrote:
> On Tue,  3 Dec 2019 08:28:11 -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, and that was also called for the clear
>> reset. To be architecture compliant, we also need to clear local
>> interrupts on a normal reset.
> 
> Do we also need to do something like that for tcg? David?
> 

So, we have

/* Fields up to this point are not cleared by initial CPU reset */
struct {} start_initial_reset_fields;
[...]
int pending_int
uint16_t external_call_addr;
DECLARE_BITMAP(emergency_signals, S390_MAX_CPUS);
[...]
/* Fields up to this point are cleared by a CPU reset */
struct {} end_reset_fields;

This means, local interrupts will be cleared by everything that zeroes
"start_initial_reset_fields->end_reset_fields"

So, they will get cleared by S390_CPU_RESET_INITIAL only if I am not
wrong. In order to clear them on S390_CPU_RESET_NORMAL, we have to
manually set them to zero.

(we really should wrap TCG-only fields by ifdefs)

-- 
Thanks,

David / dhildenb

Re: [PATCH v3 2/4] s390x: Add missing vcpu reset functions

[David Hildenbrand]

On 04.12.19 16:07, David Hildenbrand wrote:
> On 04.12.19 15:59, Cornelia Huck wrote:
>> On Tue,  3 Dec 2019 08:28:11 -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, and that was also called for the clear
>>> reset. To be architecture compliant, we also need to clear local
>>> interrupts on a normal reset.
>>
>> Do we also need to do something like that for tcg? David?
>>
> 
> So, we have
> 
> /* Fields up to this point are not cleared by initial CPU reset */
> struct {} start_initial_reset_fields;
> [...]
> int pending_int
> uint16_t external_call_addr;
> DECLARE_BITMAP(emergency_signals, S390_MAX_CPUS);
> [...]
> /* Fields up to this point are cleared by a CPU reset */
> struct {} end_reset_fields;
> 
> This means, local interrupts will be cleared by everything that zeroes
> "start_initial_reset_fields->end_reset_fields"
> 
> So, they will get cleared by S390_CPU_RESET_INITIAL only if I am not
> wrong. In order to clear them on S390_CPU_RESET_NORMAL, we have to
> manually set them to zero.

Sorry, by S390_CPU_RESET_INITIAL and S390_CPU_RESET_CLEAR.

> 
> (we really should wrap TCG-only fields by ifdefs)
> 


-- 
Thanks,

David / dhildenb

Re: [PATCH v3 2/4] s390x: Add missing vcpu reset functions

[Cornelia Huck]

On Wed, 4 Dec 2019 16:08:48 +0100
David Hildenbrand <david@redhat.com> wrote:

> On 04.12.19 16:07, David Hildenbrand wrote:
> > On 04.12.19 15:59, Cornelia Huck wrote:  
> >> On Tue,  3 Dec 2019 08:28:11 -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, and that was also called for the clear
> >>> reset. To be architecture compliant, we also need to clear local
> >>> interrupts on a normal reset.  
> >>
> >> Do we also need to do something like that for tcg? David?
> >>  
> > 
> > So, we have
> > 
> > /* Fields up to this point are not cleared by initial CPU reset */
> > struct {} start_initial_reset_fields;
> > [...]
> > int pending_int
> > uint16_t external_call_addr;
> > DECLARE_BITMAP(emergency_signals, S390_MAX_CPUS);
> > [...]
> > /* Fields up to this point are cleared by a CPU reset */
> > struct {} end_reset_fields;
> > 
> > This means, local interrupts will be cleared by everything that zeroes
> > "start_initial_reset_fields->end_reset_fields"
> > 
> > So, they will get cleared by S390_CPU_RESET_INITIAL only if I am not
> > wrong. In order to clear them on S390_CPU_RESET_NORMAL, we have to
> > manually set them to zero.  
> 
> Sorry, by S390_CPU_RESET_INITIAL and S390_CPU_RESET_CLEAR.

Ok. Will you cook up a patch, or should I do it?

> 
> > 
> > (we really should wrap TCG-only fields by ifdefs)

They probably do not get into the way, but making it obvious that they
a tcg-only is something we really should do.

> >   
> 
> 

Re: [PATCH v3 2/4] s390x: Add missing vcpu reset functions

[David Hildenbrand]

On 04.12.19 16:33, Cornelia Huck wrote:
> On Wed, 4 Dec 2019 16:08:48 +0100
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 04.12.19 16:07, David Hildenbrand wrote:
>>> On 04.12.19 15:59, Cornelia Huck wrote:  
>>>> On Tue,  3 Dec 2019 08:28:11 -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, and that was also called for the clear
>>>>> reset. To be architecture compliant, we also need to clear local
>>>>> interrupts on a normal reset.  
>>>>
>>>> Do we also need to do something like that for tcg? David?
>>>>  
>>>
>>> So, we have
>>>
>>> /* Fields up to this point are not cleared by initial CPU reset */
>>> struct {} start_initial_reset_fields;
>>> [...]
>>> int pending_int
>>> uint16_t external_call_addr;
>>> DECLARE_BITMAP(emergency_signals, S390_MAX_CPUS);
>>> [...]
>>> /* Fields up to this point are cleared by a CPU reset */
>>> struct {} end_reset_fields;
>>>
>>> This means, local interrupts will be cleared by everything that zeroes
>>> "start_initial_reset_fields->end_reset_fields"
>>>
>>> So, they will get cleared by S390_CPU_RESET_INITIAL only if I am not
>>> wrong. In order to clear them on S390_CPU_RESET_NORMAL, we have to
>>> manually set them to zero.  
>>
>> Sorry, by S390_CPU_RESET_INITIAL and S390_CPU_RESET_CLEAR.
> 
> Ok. Will you cook up a patch, or should I do it?
> 

If you want to have some TCG fun, feel free. Otherwise, let me know :)

-- 
Thanks,

David / dhildenb

Re: [PATCH v3 4/4] pc-bios/s390x: Fix reset psw mask

[Cornelia Huck]

On Tue,  3 Dec 2019 08:28:13 -0500
Janosch Frank <frankja@linux.ibm.com> wrote:

> We need to set the short psw indication bit in the reset psw, as it is
> a short psw.
> 
> fixes: 9629823290 ("pc-bios/s390-ccw: do a subsystem reset before running the guest")
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  pc-bios/s390-ccw/jump2ipl.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)

Thanks, applied (together with a rebuild of the bios images.)

Re: [PATCH v3 3/4] s390x: Fix cpu normal reset ri clearing

[Cornelia Huck]

On Tue,  3 Dec 2019 08:28:12 -0500
Janosch Frank <frankja@linux.ibm.com> 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>
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  target/s390x/cpu.c | 7 ++++++-
>  target/s390x/cpu.h | 7 ++++++-
>  2 files changed, 12 insertions(+), 2 deletions(-)

Thanks, applied.

Re: [PATCH v3 4/4] pc-bios/s390x: Fix reset psw mask

[Cornelia Huck]

On Thu, 5 Dec 2019 11:12:39 +0100
Cornelia Huck <cohuck@redhat.com> wrote:

> On Tue,  3 Dec 2019 08:28:13 -0500
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
> > We need to set the short psw indication bit in the reset psw, as it is
> > a short psw.
> > 
> > fixes: 9629823290 ("pc-bios/s390-ccw: do a subsystem reset before running the guest")
> > Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> > ---
> >  pc-bios/s390-ccw/jump2ipl.c | 12 +++++++-----
> >  1 file changed, 7 insertions(+), 5 deletions(-)  
> 
> Thanks, applied (together with a rebuild of the bios images.)

This unfortunately breaks 'make check-qtest-s390x':

   TEST    check-qtest-s390x: tests/boot-serial-test
   TEST    check-qtest-s390x: tests/pxe-test
ERROR - too few tests run (expected 1, got 0)

When I revert this, the rebuild, and "s390x: Properly fetch and test
the short psw on diag308 subc 0/1" (as it exposes the bug this commit
tried to fix), everything passes again. No idea what is wrong, though :(

For now, I've dropped the three patches mentioned above from the
s390-next branch (I plan to send a pull request later). Let's fix this
on top once we figured out whatever went wrong, no need to rush here.

Re: [PATCH v3 4/4] pc-bios/s390x: Fix reset psw mask

[Janosch Frank]

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

On 12/13/19 1:06 PM, Cornelia Huck wrote:
> On Thu, 5 Dec 2019 11:12:39 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
>> On Tue,  3 Dec 2019 08:28:13 -0500
>> Janosch Frank <frankja@linux.ibm.com> wrote:
>>
>>> We need to set the short psw indication bit in the reset psw, as it is
>>> a short psw.
>>>
>>> fixes: 9629823290 ("pc-bios/s390-ccw: do a subsystem reset before running the guest")
>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>> ---
>>>  pc-bios/s390-ccw/jump2ipl.c | 12 +++++++-----
>>>  1 file changed, 7 insertions(+), 5 deletions(-)  
>>
>> Thanks, applied (together with a rebuild of the bios images.)
> 
> This unfortunately breaks 'make check-qtest-s390x':
> 
>    TEST    check-qtest-s390x: tests/boot-serial-test
>    TEST    check-qtest-s390x: tests/pxe-test
> ERROR - too few tests run (expected 1, got 0)
> 
> When I revert this, the rebuild, and "s390x: Properly fetch and test
> the short psw on diag308 subc 0/1" (as it exposes the bug this commit
> tried to fix), everything passes again. No idea what is wrong, though :(
> 
> For now, I've dropped the three patches mentioned above from the
> s390-next branch (I plan to send a pull request later). Let's fix this
> on top once we figured out whatever went wrong, no need to rush here.
> 
> 
Sounds good


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

Re: [PATCH v3 4/4] pc-bios/s390x: Fix reset psw mask

[Thomas Huth]

On 13/12/2019 13.06, Cornelia Huck wrote:
> On Thu, 5 Dec 2019 11:12:39 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
>> On Tue,  3 Dec 2019 08:28:13 -0500
>> Janosch Frank <frankja@linux.ibm.com> wrote:
>>
>>> We need to set the short psw indication bit in the reset psw, as it is
>>> a short psw.
>>>
>>> fixes: 9629823290 ("pc-bios/s390-ccw: do a subsystem reset before running the guest")
>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>> ---
>>>  pc-bios/s390-ccw/jump2ipl.c | 12 +++++++-----
>>>  1 file changed, 7 insertions(+), 5 deletions(-)  
>>
>> Thanks, applied (together with a rebuild of the bios images.)
> 
> This unfortunately breaks 'make check-qtest-s390x':
> 
>    TEST    check-qtest-s390x: tests/boot-serial-test
>    TEST    check-qtest-s390x: tests/pxe-test
> ERROR - too few tests run (expected 1, got 0)
> 
> When I revert this, the rebuild, and "s390x: Properly fetch and test
> the short psw on diag308 subc 0/1" (as it exposes the bug this commit
> tried to fix), everything passes again. No idea what is wrong, though :(
> 
> For now, I've dropped the three patches mentioned above from the
> s390-next branch (I plan to send a pull request later). Let's fix this
> on top once we figured out whatever went wrong, no need to rush here.

I think I might have found the problem. You now need this patch on top
to get it working again:

diff a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
--- a/pc-bios/s390-ccw/jump2ipl.c
+++ b/pc-bios/s390-ccw/jump2ipl.c
@@ -18,7 +18,7 @@
 typedef struct ResetInfo {
     uint64_t ipl_psw;
     uint32_t ipl_continue;
-} ResetInfo;
+} __attribute__((packed)) ResetInfo;

 static ResetInfo save;

Without this patch, there is now a 32-bit padding in the struct. Not
sure why it then fails (I had a quick look at the disassembly of
jump2ipl.o and could not spot anything that was obviously wrong), but if
I mark the struct as "packed" to get the old size again, then the
pxe-test is working fine for me again.

 Thomas

Re: [PATCH v3 4/4] pc-bios/s390x: Fix reset psw mask

[Thomas Huth]

On 17/12/2019 13.36, Thomas Huth wrote:
> On 13/12/2019 13.06, Cornelia Huck wrote:
>> On Thu, 5 Dec 2019 11:12:39 +0100
>> Cornelia Huck <cohuck@redhat.com> wrote:
>>
>>> On Tue,  3 Dec 2019 08:28:13 -0500
>>> Janosch Frank <frankja@linux.ibm.com> wrote:
>>>
>>>> We need to set the short psw indication bit in the reset psw, as it is
>>>> a short psw.
>>>>
>>>> fixes: 9629823290 ("pc-bios/s390-ccw: do a subsystem reset before running the guest")
>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>>> ---
>>>>  pc-bios/s390-ccw/jump2ipl.c | 12 +++++++-----
>>>>  1 file changed, 7 insertions(+), 5 deletions(-)  
>>>
>>> Thanks, applied (together with a rebuild of the bios images.)
>>
>> This unfortunately breaks 'make check-qtest-s390x':
>>
>>    TEST    check-qtest-s390x: tests/boot-serial-test
>>    TEST    check-qtest-s390x: tests/pxe-test
>> ERROR - too few tests run (expected 1, got 0)
>>
>> When I revert this, the rebuild, and "s390x: Properly fetch and test
>> the short psw on diag308 subc 0/1" (as it exposes the bug this commit
>> tried to fix), everything passes again. No idea what is wrong, though :(
>>
>> For now, I've dropped the three patches mentioned above from the
>> s390-next branch (I plan to send a pull request later). Let's fix this
>> on top once we figured out whatever went wrong, no need to rush here.
> 
> I think I might have found the problem. You now need this patch on top
> to get it working again:
> 
> diff a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
> --- a/pc-bios/s390-ccw/jump2ipl.c
> +++ b/pc-bios/s390-ccw/jump2ipl.c
> @@ -18,7 +18,7 @@
>  typedef struct ResetInfo {
>      uint64_t ipl_psw;
>      uint32_t ipl_continue;
> -} ResetInfo;
> +} __attribute__((packed)) ResetInfo;
> 
>  static ResetInfo save;
> 
> Without this patch, there is now a 32-bit padding in the struct. Not
> sure why it then fails (I had a quick look at the disassembly of
> jump2ipl.o and could not spot anything that was obviously wrong), but if
> I mark the struct as "packed" to get the old size again, then the
> pxe-test is working fine for me again.

Never mind, the problem was not in the jump2ipl.c code, but in the
pxe-test: The code only worked if the r3 register contained only zeroes
in the upper bits. The changes to jump2ipl.c here caused the compiler to
put a different value in r3, so the test suddenly crashed. I've now sent
a patch to fix the test ("tests/boot-sector: Fix the bad s390x assembler
code").

 Thomas

Re: [PATCH v3 4/4] pc-bios/s390x: Fix reset psw mask

[Janosch Frank]

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

On 12/17/19 4:09 PM, Thomas Huth wrote:
> On 17/12/2019 13.36, Thomas Huth wrote:
>> On 13/12/2019 13.06, Cornelia Huck wrote:
>>> On Thu, 5 Dec 2019 11:12:39 +0100
>>> Cornelia Huck <cohuck@redhat.com> wrote:
>>>
>>>> On Tue,  3 Dec 2019 08:28:13 -0500
>>>> Janosch Frank <frankja@linux.ibm.com> wrote:
>>>>
>>>>> We need to set the short psw indication bit in the reset psw, as it is
>>>>> a short psw.
>>>>>
>>>>> fixes: 9629823290 ("pc-bios/s390-ccw: do a subsystem reset before running the guest")
>>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>>>> ---
>>>>>  pc-bios/s390-ccw/jump2ipl.c | 12 +++++++-----
>>>>>  1 file changed, 7 insertions(+), 5 deletions(-)  
>>>>
>>>> Thanks, applied (together with a rebuild of the bios images.)
>>>
>>> This unfortunately breaks 'make check-qtest-s390x':
>>>
>>>    TEST    check-qtest-s390x: tests/boot-serial-test
>>>    TEST    check-qtest-s390x: tests/pxe-test
>>> ERROR - too few tests run (expected 1, got 0)
>>>
>>> When I revert this, the rebuild, and "s390x: Properly fetch and test
>>> the short psw on diag308 subc 0/1" (as it exposes the bug this commit
>>> tried to fix), everything passes again. No idea what is wrong, though :(
>>>
>>> For now, I've dropped the three patches mentioned above from the
>>> s390-next branch (I plan to send a pull request later). Let's fix this
>>> on top once we figured out whatever went wrong, no need to rush here.
>>
>> I think I might have found the problem. You now need this patch on top
>> to get it working again:
>>
>> diff a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
>> --- a/pc-bios/s390-ccw/jump2ipl.c
>> +++ b/pc-bios/s390-ccw/jump2ipl.c
>> @@ -18,7 +18,7 @@
>>  typedef struct ResetInfo {
>>      uint64_t ipl_psw;
>>      uint32_t ipl_continue;
>> -} ResetInfo;
>> +} __attribute__((packed)) ResetInfo;
>>
>>  static ResetInfo save;
>>
>> Without this patch, there is now a 32-bit padding in the struct. Not
>> sure why it then fails (I had a quick look at the disassembly of
>> jump2ipl.o and could not spot anything that was obviously wrong), but if
>> I mark the struct as "packed" to get the old size again, then the
>> pxe-test is working fine for me again.
> 
> Never mind, the problem was not in the jump2ipl.c code, but in the
> pxe-test: The code only worked if the r3 register contained only zeroes
> in the upper bits. The changes to jump2ipl.c here caused the compiler to
> put a different value in r3, so the test suddenly crashed. I've now sent
> a patch to fix the test ("tests/boot-sector: Fix the bad s390x assembler
> code").
> 
>  Thomas

Thank you a lot for having a look into that!




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

Re: [PATCH v3 4/4] pc-bios/s390x: Fix reset psw mask

[Cornelia Huck]

On Tue,  3 Dec 2019 08:28:13 -0500
Janosch Frank <frankja@linux.ibm.com> wrote:

> We need to set the short psw indication bit in the reset psw, as it is
> a short psw.
> 
> fixes: 9629823290 ("pc-bios/s390-ccw: do a subsystem reset before running the guest")
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  pc-bios/s390-ccw/jump2ipl.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)

FWIW, I'll re-queue this patch (and the bios rebuild) in front of the
short psw fix that exposed the problem, in order to preserve
bisectability.

Re: [PATCH v3 4/4] pc-bios/s390x: Fix reset psw mask

[Cornelia Huck]

On Tue,  3 Dec 2019 08:28:13 -0500
Janosch Frank <frankja@linux.ibm.com> wrote:

> We need to set the short psw indication bit in the reset psw, as it is
> a short psw.
> 
> fixes: 9629823290 ("pc-bios/s390-ccw: do a subsystem reset before running the guest")
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  pc-bios/s390-ccw/jump2ipl.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)

Thanks, requeued this and the bios rebuild.