s390x/kvm: fix and cleanup storing CPU status

target/s390x/kvm.c +5 -0

+struct sigp_save_area {
+    uint64_t    fprs[16];                       /* 0x0000 */
+    uint64_t    grs[16];                        /* 0x0080 */
+    PSW         psw;                            /* 0x0100 */
+    uint8_t     pad_0x0110[0x0118 - 0x0110];    /* 0x0110 */

QEMU_BUILD_BUG_ON(sizeof(*sa) != 512);

Quoted 12 lines · David Hildenbrand

target/s390x/kvm.c +12 -0

+struct sigp_save_area {
+    uint64_t    fprs[16];                       /* 0x0000 */
+    uint64_t    grs[16];                        /* 0x0080 */
+    PSW         psw;                            /* 0x0100 */
+    uint8_t     pad_0x0110[0x0118 - 0x0110];    /* 0x0110 */
+    uint32_t    prefix;                         /* 0x0118 */
+    uint32_t    fpc;                            /* 0x011c */
+    uint8_t     pad_0x0120[0x0124 - 0x0120];    /* 0x0120 */
+    uint32_t    todpr;                          /* 0x0124 */
+    uint64_t    cputm;                          /* 0x0128 */
+    uint64_t    ckc;                            /* 0x0130 */
+    uint8_t     pad_0x0138[0x0140 - 0x0138];    /* 0x0138 */

target/s390x/kvm.c static int do_store_adtl_status(S390CPU *cpu, hwaddr addr, hwaddr len) · line 1565

Quoted 3 lines

target/s390x/kvm.c +3 -0

+    uint32_t    ars[16];                        /* 0x0140 */
+    uint64_t    crs[16];                        /* 0x0384 */
+} QEMU_PACKED;

I don't believe you need to use QEMU_PACKED. Unless the "addr" that's passed in can really be arbitrary? (If so, the memory returned by cpu_physical_memory_map will be unaligned and so we do need the PACKED to force the compiler to generate unaligned stores into the structure.)

Quoted 2 lines

QEMU_BUILD_BUG_ON(sizeof(*sa) != 512);

Put this right next to the structure?

r~

Quoted 4 lines · Richard Henderson

I don't believe you need to use QEMU_PACKED. Unless the "addr" that's passed in can really be arbitrary? (If so, the memory returned by cpu_physical_memory_map will be unaligned and so we do need the PACKED to force the compiler to generate unaligned stores into the structure.)

The address is either 0x1200 or it is masked with 0x7ffffe00u.

So most probably this should be fine without QEMU_PACKED, right?

env->psa is a 64bit value, while we copy 4 bytes into the save area, resulting always in 0 getting stored.

Let's try to reduce such errors by using a proper structure. While at it, use correct cpu->be conversion (and get_psw_mask()), as we will be reusing this code for TCG soon.

Signed-off-by David Hildenbrand <[email protected]>

---

Found with my fancy SIGP kvm-unit-tests I am currently working on. And looks like there is even more ... stay tuned :)

 target/s390x/kvm.c | 63 +++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 43 insertions(+), 20 deletions(-)

target/s390x/kvm.c +43 -20

index 720cb1d833..21c22f48b9 100644
@@ -1548,22 +1548,38 @@ static int do_store_adtl_status(S390CPU *cpu, hwaddr addr, hwaddr len)
     return 0;
 }
 
+struct sigp_save_area {
+    uint64_t    fprs[16];                       /* 0x0000 */
+    uint64_t    grs[16];                        /* 0x0080 */
+    PSW         psw;                            /* 0x0100 */
+    uint8_t     pad_0x0110[0x0118 - 0x0110];    /* 0x0110 */
+    uint32_t    prefix;                         /* 0x0118 */
+    uint32_t    fpc;                            /* 0x011c */
+    uint8_t     pad_0x0120[0x0124 - 0x0120];    /* 0x0120 */
 
+    uint32_t    todpr;                          /* 0x0124 */
+    uint64_t    cputm;                          /* 0x0128 */
+    uint64_t    ckc;                            /* 0x0130 */
+    uint8_t     pad_0x0138[0x0140 - 0x0138];    /* 0x0138 */
+    uint32_t    ars[16];                        /* 0x0140 */
+    uint64_t    crs[16];                        /* 0x0384 */
+} QEMU_PACKED;
+
 #define KVM_S390_STORE_STATUS_DEF_ADDR offsetof(LowCore, floating_pt_save_area)
-#define SAVE_AREA_SIZE 512
 static int kvm_s390_store_status(S390CPU *cpu, hwaddr addr, bool store_arch)
 {
     static const uint8_t ar_id = 1;
-    uint64_t ckc = cpu->env.ckc >> 8;
-    void *mem;
+    struct sigp_save_area *sa;
+    hwaddr len = sizeof(*sa);
     int i;
-    hwaddr len = SAVE_AREA_SIZE;
 
-    mem = cpu_physical_memory_map(addr, &len, 1);
-    if (!mem) {
+    QEMU_BUILD_BUG_ON(sizeof(*sa) != 512);
+
+    sa = cpu_physical_memory_map(addr, &len, 1);
+    if (!sa) {
         return -EFAULT;
     }
-    if (len != SAVE_AREA_SIZE) {
-        cpu_physical_memory_unmap(mem, len, 1, 0);
+    if (len != sizeof(*sa)) {
+        cpu_physical_memory_unmap(sa, len, 1, 0);
         return -EFAULT;
     }

@@ -1571,19 +1587,26 @@ static int kvm_s390_store_status(S390CPU *cpu, hwaddr addr, bool store_arch)
         cpu_physical_memory_write(offsetof(LowCore, ar_access_id), &ar_id, 1);
     }
     for (i = 0; i < 16; ++i) {
-        *((uint64_t *)mem + i) = get_freg(&cpu->env, i)->ll;
-    }
-    memcpy(mem + 128, &cpu->env.regs, 128);
-    memcpy(mem + 256, &cpu->env.psw, 16);
-    memcpy(mem + 280, &cpu->env.psa, 4);
-    memcpy(mem + 284, &cpu->env.fpc, 4);
-    memcpy(mem + 292, &cpu->env.todpr, 4);
-    memcpy(mem + 296, &cpu->env.cputm, 8);
-    memcpy(mem + 304, &ckc, 8);
-    memcpy(mem + 320, &cpu->env.aregs, 64);
-    memcpy(mem + 384, &cpu->env.cregs, 128);
+        sa->fprs[i] = cpu_to_be64(get_freg(&cpu->env, i)->ll);
+    }
+    for (i = 0; i < 16; ++i) {
+        sa->grs[i] = cpu_to_be64(cpu->env.regs[i]);
+    }
+    sa->psw.addr = cpu_to_be64(cpu->env.psw.addr);
+    sa->psw.mask = cpu_to_be64(get_psw_mask(&cpu->env));
+    sa->prefix = cpu_to_be32(cpu->env.psa);
+    sa->fpc = cpu_to_be32(cpu->env.fpc);
+    sa->todpr = cpu_to_be32(cpu->env.todpr);
+    sa->cputm = cpu_to_be64(cpu->env.cputm);
+    sa->ckc = cpu_to_be64(cpu->env.ckc >> 8);
+    for (i = 0; i < 16; ++i) {
+        sa->ars[i] = cpu_to_be32(cpu->env.aregs[i]);
+    }
+    for (i = 0; i < 16; ++i) {
+        sa->ars[i] = cpu_to_be64(cpu->env.cregs[i]);
+    }
 
-    cpu_physical_memory_unmap(mem, len, 1, len);
+    cpu_physical_memory_unmap(sa, len, 1, len);
 
     return 0;
 }