[Qemu-devel] [PATCH v2 0/8] s390x: storage key migration

[Qemu-devel] [PATCH v2 0/8] s390x: storage key migration

[Cornelia Huck]

Here's the second edition of the storage key migration patches.

Changes from v1:
- have the dump-skeys qmp command use qemu_fopen() and friends
- handle failures of the skeys-obtaining commands by filling the
  stream with zeroes and setting an error flag

Would like to send a pull request soonish.

Cornelia Huck (1):
  s390x: add 2.5 compat s390-ccw-virtio machine

Jason J. Herne (7):
  s390x: Create QOM device for s390 storage keys
  s390x: Enable new s390-storage-keys device
  s390x: Dump storage keys qmp command
  s390x: Dump-skeys hmp support
  s390x: Info skeys sub-command
  s390x: Migrate guest storage keys (initial memory only)
  s390x: Disable storage key migration on old machine type

 MAINTAINERS                     |   1 +
 hmp-commands.hx                 |  18 ++
 hw/s390x/Makefile.objs          |   2 +
 hw/s390x/s390-skeys-kvm.c       |  75 +++++++
 hw/s390x/s390-skeys.c           | 425 ++++++++++++++++++++++++++++++++++++++++
 hw/s390x/s390-virtio-ccw.c      |  39 +++-
 hw/s390x/s390-virtio.c          |  11 +-
 hw/s390x/s390-virtio.h          |   2 +-
 include/hw/s390x/storage-keys.h |  60 ++++++
 monitor.c                       |  20 ++
 qapi-schema.json                |  13 ++
 qmp-commands.hx                 |  25 +++
 target-s390x/cpu.h              |   2 -
 target-s390x/mem_helper.c       |  46 ++++-
 target-s390x/mmu_helper.c       |  28 ++-
 trace-events                    |   4 +
 16 files changed, 745 insertions(+), 26 deletions(-)
 create mode 100644 hw/s390x/s390-skeys-kvm.c
 create mode 100644 hw/s390x/s390-skeys.c
 create mode 100644 include/hw/s390x/storage-keys.h

-- 
2.5.0

[Qemu-devel] [PATCH v2 1/8] s390x: add 2.5 compat s390-ccw-virtio machine

[Cornelia Huck]

Reviewed-by: Jason J. Herne <jjherne@linux.vnet.ibm.com>
Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 hw/s390x/s390-virtio-ccw.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 4c51d1a..71df282 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -287,9 +287,7 @@ static void ccw_machine_2_4_class_init(ObjectClass *oc, void *data)
     MachineClass *mc = MACHINE_CLASS(oc);
 
     mc->name = "s390-ccw-virtio-2.4";
-    mc->alias = "s390-ccw-virtio";
     mc->desc = "VirtIO-ccw based S390 machine v2.4";
-    mc->is_default = 1;
 }
 
 static const TypeInfo ccw_machine_2_4_info = {
@@ -298,10 +296,27 @@ static const TypeInfo ccw_machine_2_4_info = {
     .class_init    = ccw_machine_2_4_class_init,
 };
 
+static void ccw_machine_2_5_class_init(ObjectClass *oc, void *data)
+{
+    MachineClass *mc = MACHINE_CLASS(oc);
+
+    mc->name = "s390-ccw-virtio-2.5";
+    mc->alias = "s390-ccw-virtio";
+    mc->desc = "VirtIO-ccw based S390 machine v2.5";
+    mc->is_default = 1;
+}
+
+static const TypeInfo ccw_machine_2_5_info = {
+    .name          = TYPE_S390_CCW_MACHINE "2.5",
+    .parent        = TYPE_S390_CCW_MACHINE,
+    .class_init    = ccw_machine_2_5_class_init,
+};
+
 static void ccw_machine_register_types(void)
 {
     type_register_static(&ccw_machine_info);
     type_register_static(&ccw_machine_2_4_info);
+    type_register_static(&ccw_machine_2_5_info);
 }
 
 type_init(ccw_machine_register_types)
-- 
2.5.0

[Qemu-devel] [PATCH v2 2/8] s390x: Create QOM device for s390 storage keys

[Cornelia Huck]

From: "Jason J. Herne" <jjherne@linux.vnet.ibm.com>

A new QOM style device is provided to back guest storage keys. A special
version for KVM is created, which handles the storage key access via
KVM_S390_GET_SKEYS and KVM_S390_SET_SKEYS ioctl.

Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
Signed-off-by: Jason J. Herne <jjherne@linux.vnet.ibm.com>
Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
 MAINTAINERS                     |   1 +
 hw/s390x/Makefile.objs          |   2 +
 hw/s390x/s390-skeys-kvm.c       |  75 +++++++++++++++++++++
 hw/s390x/s390-skeys.c           | 141 ++++++++++++++++++++++++++++++++++++++++
 include/hw/s390x/storage-keys.h |  55 ++++++++++++++++
 5 files changed, 274 insertions(+)
 create mode 100644 hw/s390x/s390-skeys-kvm.c
 create mode 100644 hw/s390x/s390-skeys.c
 create mode 100644 include/hw/s390x/storage-keys.h

diff --git a/MAINTAINERS b/MAINTAINERS
index a059d5d..c7a90a9 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -560,6 +560,7 @@ F: hw/s390x/css.[hc]
 F: hw/s390x/sclp*.[hc]
 F: hw/s390x/ipl*.[hc]
 F: hw/s390x/*pci*.[hc]
+F: hw/s390x/s390-skeys*.c
 F: include/hw/s390x/
 F: pc-bios/s390-ccw/
 T: git git://github.com/cohuck/qemu virtio-ccw-upstr
diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
index 27cd75a..527d754 100644
--- a/hw/s390x/Makefile.objs
+++ b/hw/s390x/Makefile.objs
@@ -9,3 +9,5 @@ obj-y += css.o
 obj-y += s390-virtio-ccw.o
 obj-y += virtio-ccw.o
 obj-y += s390-pci-bus.o s390-pci-inst.o
+obj-y += s390-skeys.o
+obj-$(CONFIG_KVM) += s390-skeys-kvm.o
diff --git a/hw/s390x/s390-skeys-kvm.c b/hw/s390x/s390-skeys-kvm.c
new file mode 100644
index 0000000..682949a
--- /dev/null
+++ b/hw/s390x/s390-skeys-kvm.c
@@ -0,0 +1,75 @@
+/*
+ * s390 storage key device
+ *
+ * Copyright 2015 IBM Corp.
+ * Author(s): Jason J. Herne <jjherne@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or (at
+ * your option) any later version. See the COPYING file in the top-level
+ * directory.
+ */
+
+#include "hw/s390x/storage-keys.h"
+#include "sysemu/kvm.h"
+#include "qemu/error-report.h"
+
+static int kvm_s390_skeys_enabled(S390SKeysState *ss)
+{
+    S390SKeysClass *skeyclass = S390_SKEYS_GET_CLASS(ss);
+    uint8_t single_key;
+    int r;
+
+    r = skeyclass->get_skeys(ss, 0, 1, &single_key);
+    if (r != 0 && r != KVM_S390_GET_SKEYS_NONE) {
+        error_report("S390_GET_KEYS error %d\n", r);
+    }
+    return (r == 0);
+}
+
+static int kvm_s390_skeys_get(S390SKeysState *ss, uint64_t start_gfn,
+                              uint64_t count, uint8_t *keys)
+{
+    struct kvm_s390_skeys args = {
+        .start_gfn = start_gfn,
+        .count = count,
+        .skeydata_addr = (__u64)keys
+    };
+
+    return kvm_vm_ioctl(kvm_state, KVM_S390_GET_SKEYS, &args);
+}
+
+static int kvm_s390_skeys_set(S390SKeysState *ss, uint64_t start_gfn,
+                              uint64_t count, uint8_t *keys)
+{
+    struct kvm_s390_skeys args = {
+        .start_gfn = start_gfn,
+        .count = count,
+        .skeydata_addr = (__u64)keys
+    };
+
+    return kvm_vm_ioctl(kvm_state, KVM_S390_SET_SKEYS, &args);
+}
+
+static void kvm_s390_skeys_class_init(ObjectClass *oc, void *data)
+{
+    S390SKeysClass *skeyclass = S390_SKEYS_CLASS(oc);
+
+    skeyclass->skeys_enabled = kvm_s390_skeys_enabled;
+    skeyclass->get_skeys = kvm_s390_skeys_get;
+    skeyclass->set_skeys = kvm_s390_skeys_set;
+}
+
+static const TypeInfo kvm_s390_skeys_info = {
+    .name          = TYPE_KVM_S390_SKEYS,
+    .parent        = TYPE_S390_SKEYS,
+    .instance_size = sizeof(S390SKeysState),
+    .class_init    = kvm_s390_skeys_class_init,
+    .class_size    = sizeof(S390SKeysClass),
+};
+
+static void kvm_s390_skeys_register_types(void)
+{
+    type_register_static(&kvm_s390_skeys_info);
+}
+
+type_init(kvm_s390_skeys_register_types)
diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
new file mode 100644
index 0000000..77c42ff
--- /dev/null
+++ b/hw/s390x/s390-skeys.c
@@ -0,0 +1,141 @@
+/*
+ * s390 storage key device
+ *
+ * Copyright 2015 IBM Corp.
+ * Author(s): Jason J. Herne <jjherne@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or (at
+ * your option) any later version. See the COPYING file in the top-level
+ * directory.
+ */
+
+#include "hw/boards.h"
+#include "hw/s390x/storage-keys.h"
+#include "qemu/error-report.h"
+
+S390SKeysState *s390_get_skeys_device(void)
+{
+    S390SKeysState *ss;
+
+    ss = S390_SKEYS(object_resolve_path_type("", TYPE_S390_SKEYS, NULL));
+    assert(ss);
+    return ss;
+}
+
+void s390_skeys_init(void)
+{
+    Object *obj;
+
+    if (kvm_enabled()) {
+        obj = object_new(TYPE_KVM_S390_SKEYS);
+    } else {
+        obj = object_new(TYPE_QEMU_S390_SKEYS);
+    }
+    object_property_add_child(qdev_get_machine(), TYPE_S390_SKEYS,
+                              obj, NULL);
+    object_unref(obj);
+
+    qdev_init_nofail(DEVICE(obj));
+}
+
+static void qemu_s390_skeys_init(Object *obj)
+{
+    QEMUS390SKeysState *skeys = QEMU_S390_SKEYS(obj);
+    MachineState *machine = MACHINE(qdev_get_machine());
+
+    skeys->key_count = machine->maxram_size / TARGET_PAGE_SIZE;
+    skeys->keydata = g_malloc0(skeys->key_count);
+}
+
+static int qemu_s390_skeys_enabled(S390SKeysState *ss)
+{
+    return 1;
+}
+
+/*
+ * TODO: for memory hotplug support qemu_s390_skeys_set and qemu_s390_skeys_get
+ * will have to make sure that the given gfn belongs to a memory region and not
+ * a memory hole.
+ */
+static int qemu_s390_skeys_set(S390SKeysState *ss, uint64_t start_gfn,
+                              uint64_t count, uint8_t *keys)
+{
+    QEMUS390SKeysState *skeydev = QEMU_S390_SKEYS(ss);
+    int i;
+
+    /* Check for uint64 overflow and access beyond end of key data */
+    if (start_gfn + count > skeydev->key_count || start_gfn + count < count) {
+        error_report("Error: Setting storage keys for page beyond the end "
+                "of memory. gfn=%" PRIx64 " count=%" PRId64 "\n", start_gfn,
+                count);
+        return -EINVAL;
+    }
+
+    for (i = 0; i < count; i++) {
+        skeydev->keydata[start_gfn + i] = keys[i];
+    }
+    return 0;
+}
+
+static int qemu_s390_skeys_get(S390SKeysState *ss, uint64_t start_gfn,
+                               uint64_t count, uint8_t *keys)
+{
+    QEMUS390SKeysState *skeydev = QEMU_S390_SKEYS(ss);
+    int i;
+
+    /* Check for uint64 overflow and access beyond end of key data */
+    if (start_gfn + count > skeydev->key_count || start_gfn + count < count) {
+        error_report("Error: Getting storage keys for page beyond the end "
+                "of memory. gfn=%" PRIx64 " count=%" PRId64 "\n", start_gfn,
+                count);
+        return -EINVAL;
+    }
+
+    for (i = 0; i < count; i++) {
+        keys[i] = skeydev->keydata[start_gfn + i];
+    }
+    return 0;
+}
+
+static void qemu_s390_skeys_class_init(ObjectClass *oc, void *data)
+{
+    S390SKeysClass *skeyclass = S390_SKEYS_CLASS(oc);
+
+    skeyclass->skeys_enabled = qemu_s390_skeys_enabled;
+    skeyclass->get_skeys = qemu_s390_skeys_get;
+    skeyclass->set_skeys = qemu_s390_skeys_set;
+}
+
+static const TypeInfo qemu_s390_skeys_info = {
+    .name          = TYPE_QEMU_S390_SKEYS,
+    .parent        = TYPE_S390_SKEYS,
+    .instance_init = qemu_s390_skeys_init,
+    .instance_size = sizeof(QEMUS390SKeysState),
+    .class_init    = qemu_s390_skeys_class_init,
+    .instance_size = sizeof(S390SKeysClass),
+};
+
+static void s390_skeys_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+
+    dc->hotpluggable = false;
+    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
+}
+
+static const TypeInfo s390_skeys_info = {
+    .name          = TYPE_S390_SKEYS,
+    .parent        = TYPE_DEVICE,
+    .instance_size = sizeof(S390SKeysState),
+    .class_init    = s390_skeys_class_init,
+    .class_size    = sizeof(S390SKeysClass),
+    .abstract = true,
+};
+
+static void qemu_s390_skeys_register_types(void)
+{
+    type_register_static(&s390_skeys_info);
+    type_register_static(&qemu_s390_skeys_info);
+}
+
+type_init(qemu_s390_skeys_register_types)
diff --git a/include/hw/s390x/storage-keys.h b/include/hw/s390x/storage-keys.h
new file mode 100644
index 0000000..cfd7da7
--- /dev/null
+++ b/include/hw/s390x/storage-keys.h
@@ -0,0 +1,55 @@
+/*
+ * s390 storage key device
+ *
+ * Copyright 2015 IBM Corp.
+ * Author(s): Jason J. Herne <jjherne@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or (at
+ * your option) any later version. See the COPYING file in the top-level
+ * directory.
+ */
+
+#ifndef __S390_STORAGE_KEYS_H
+#define __S390_STORAGE_KEYS_H
+
+#include <hw/qdev.h>
+
+#define TYPE_S390_SKEYS "s390-skeys"
+#define S390_SKEYS(obj) \
+    OBJECT_CHECK(S390SKeysState, (obj), TYPE_S390_SKEYS)
+
+typedef struct S390SKeysState {
+    DeviceState parent_obj;
+
+} S390SKeysState;
+
+#define S390_SKEYS_CLASS(klass) \
+    OBJECT_CLASS_CHECK(S390SKeysClass, (klass), TYPE_S390_SKEYS)
+#define S390_SKEYS_GET_CLASS(obj) \
+    OBJECT_GET_CLASS(S390SKeysClass, (obj), TYPE_S390_SKEYS)
+
+typedef struct S390SKeysClass {
+    DeviceClass parent_class;
+    int (*skeys_enabled)(S390SKeysState *ks);
+    int (*get_skeys)(S390SKeysState *ks, uint64_t start_gfn, uint64_t count,
+                     uint8_t *keys);
+    int (*set_skeys)(S390SKeysState *ks, uint64_t start_gfn, uint64_t count,
+                     uint8_t *keys);
+} S390SKeysClass;
+
+#define TYPE_KVM_S390_SKEYS "s390-skeys-kvm"
+#define TYPE_QEMU_S390_SKEYS "s390-skeys-qemu"
+#define QEMU_S390_SKEYS(obj) \
+    OBJECT_CHECK(QEMUS390SKeysState, (obj), TYPE_QEMU_S390_SKEYS)
+
+typedef struct QEMUS390SKeysState {
+    S390SKeysState parent_obj;
+    uint8_t *keydata;
+    uint32_t key_count;
+} QEMUS390SKeysState;
+
+void s390_skeys_init(void);
+
+S390SKeysState *s390_get_skeys_device(void);
+
+#endif /* __S390_STORAGE_KEYS_H */
-- 
2.5.0

[Qemu-devel] [PATCH v2 3/8] s390x: Enable new s390-storage-keys device

[Cornelia Huck]

From: "Jason J. Herne" <jjherne@linux.vnet.ibm.com>

s390 guest initialization is modified to make use of new s390-storage-keys
device. Old code that globally allocated storage key array is removed.
The new device enables storage key access for kvm guests.

Cache storage key QOM objects in frequently used helper functions to avoid a
performance hit every time we use one of these functions.

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Reviewed-by: Thomas Huth <thuth@linux.vnet.ibm.com>
Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
Signed-off-by: Jason J. Herne <jjherne@linux.vnet.ibm.com>
Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
 hw/s390x/s390-virtio-ccw.c |  8 ++++----
 hw/s390x/s390-virtio.c     | 11 +++++------
 hw/s390x/s390-virtio.h     |  2 +-
 target-s390x/cpu.h         |  2 --
 target-s390x/mem_helper.c  | 46 ++++++++++++++++++++++++++++++++++++++++------
 target-s390x/mmu_helper.c  | 28 +++++++++++++++++++++++-----
 trace-events               |  4 ++++
 7 files changed, 77 insertions(+), 24 deletions(-)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 71df282..0a057ae 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -19,6 +19,7 @@
 #include "virtio-ccw.h"
 #include "qemu/config-file.h"
 #include "s390-pci-bus.h"
+#include "hw/s390x/storage-keys.h"
 
 #define TYPE_S390_CCW_MACHINE               "s390-ccw-machine"
 
@@ -105,7 +106,6 @@ static void ccw_init(MachineState *machine)
     MemoryRegion *sysmem = get_system_memory();
     MemoryRegion *ram = g_new(MemoryRegion, 1);
     sclpMemoryHotplugDev *mhd = init_sclp_memory_hotplug_dev();
-    uint8_t *storage_keys;
     int ret;
     VirtualCssBus *css_bus;
     DeviceState *dev;
@@ -179,11 +179,11 @@ static void ccw_init(MachineState *machine)
         mhd->standby_mem_size = standby_mem_size;
     }
 
-    /* allocate storage keys */
-    storage_keys = g_malloc0(my_ram_size / TARGET_PAGE_SIZE);
+    /* Initialize storage key device */
+    s390_skeys_init();
 
     /* init CPUs */
-    s390_init_cpus(machine->cpu_model, storage_keys);
+    s390_init_cpus(machine->cpu_model);
 
     if (kvm_enabled()) {
         kvm_s390_enable_css_support(s390_cpu_addr2state(0));
diff --git a/hw/s390x/s390-virtio.c b/hw/s390x/s390-virtio.c
index 1284e77..6cc6b5d 100644
--- a/hw/s390x/s390-virtio.c
+++ b/hw/s390x/s390-virtio.c
@@ -38,6 +38,7 @@
 #include "hw/s390x/sclp.h"
 #include "hw/s390x/s390_flic.h"
 #include "hw/s390x/s390-virtio.h"
+#include "hw/s390x/storage-keys.h"
 #include "cpu.h"
 
 //#define DEBUG_S390
@@ -164,7 +165,7 @@ void s390_init_ipl_dev(const char *kernel_filename,
     qdev_init_nofail(dev);
 }
 
-void s390_init_cpus(const char *cpu_model, uint8_t *storage_keys)
+void s390_init_cpus(const char *cpu_model)
 {
     int i;
 
@@ -184,7 +185,6 @@ void s390_init_cpus(const char *cpu_model, uint8_t *storage_keys)
         ipi_states[i] = cpu;
         cs->halted = 1;
         cs->exception_index = EXCP_HLT;
-        cpu->env.storage_keys = storage_keys;
     }
 }
 
@@ -264,7 +264,6 @@ static void s390_init(MachineState *machine)
     MemoryRegion *sysmem = get_system_memory();
     MemoryRegion *ram = g_new(MemoryRegion, 1);
     int increment_size = 20;
-    uint8_t *storage_keys;
     void *virtio_region;
     hwaddr virtio_region_len;
     hwaddr virtio_region_start;
@@ -306,11 +305,11 @@ static void s390_init(MachineState *machine)
     cpu_physical_memory_unmap(virtio_region, virtio_region_len, 1,
                               virtio_region_len);
 
-    /* allocate storage keys */
-    storage_keys = g_malloc0(my_ram_size / TARGET_PAGE_SIZE);
+    /* Initialize storage key device */
+    s390_skeys_init();
 
     /* init CPUs */
-    s390_init_cpus(machine->cpu_model, storage_keys);
+    s390_init_cpus(machine->cpu_model);
 
     /* Create VirtIO network adapters */
     s390_create_virtio_net((BusState *)s390_bus, "virtio-net-s390");
diff --git a/hw/s390x/s390-virtio.h b/hw/s390x/s390-virtio.h
index c847853..cf68796 100644
--- a/hw/s390x/s390-virtio.h
+++ b/hw/s390x/s390-virtio.h
@@ -19,7 +19,7 @@
 typedef int (*s390_virtio_fn)(const uint64_t *args);
 void s390_register_virtio_hypercall(uint64_t code, s390_virtio_fn fn);
 
-void s390_init_cpus(const char *cpu_model, uint8_t *storage_keys);
+void s390_init_cpus(const char *cpu_model);
 void s390_init_ipl_dev(const char *kernel_filename,
                        const char *kernel_cmdline,
                        const char *initrd_filename,
diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
index 63aebf4..b650890 100644
--- a/target-s390x/cpu.h
+++ b/target-s390x/cpu.h
@@ -143,8 +143,6 @@ typedef struct CPUS390XState {
     uint32_t cpu_num;
     uint32_t machine_type;
 
-    uint8_t *storage_keys;
-
     uint64_t tod_offset;
     uint64_t tod_basetime;
     QEMUTimer *tod_timer;
diff --git a/target-s390x/mem_helper.c b/target-s390x/mem_helper.c
index 6f8bd79..84bf198 100644
--- a/target-s390x/mem_helper.c
+++ b/target-s390x/mem_helper.c
@@ -21,6 +21,7 @@
 #include "cpu.h"
 #include "exec/helper-proto.h"
 #include "exec/cpu_ldst.h"
+#include "hw/s390x/storage-keys.h"
 
 /*****************************************************************************/
 /* Softmmu support */
@@ -937,40 +938,73 @@ uint32_t HELPER(tprot)(uint64_t a1, uint64_t a2)
 /* insert storage key extended */
 uint64_t HELPER(iske)(CPUS390XState *env, uint64_t r2)
 {
+    static S390SKeysState *ss;
+    static S390SKeysClass *skeyclass;
     uint64_t addr = get_address(env, 0, 0, r2);
+    uint8_t key;
 
     if (addr > ram_size) {
         return 0;
     }
 
-    return env->storage_keys[addr / TARGET_PAGE_SIZE];
+    if (unlikely(!ss)) {
+        ss = s390_get_skeys_device();
+        skeyclass = S390_SKEYS_GET_CLASS(ss);
+    }
+
+    if (skeyclass->get_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key)) {
+        return 0;
+    }
+    return key;
 }
 
 /* set storage key extended */
 void HELPER(sske)(CPUS390XState *env, uint64_t r1, uint64_t r2)
 {
+    static S390SKeysState *ss;
+    static S390SKeysClass *skeyclass;
     uint64_t addr = get_address(env, 0, 0, r2);
+    uint8_t key;
 
     if (addr > ram_size) {
         return;
     }
 
-    env->storage_keys[addr / TARGET_PAGE_SIZE] = r1;
+    if (unlikely(!ss)) {
+        ss = s390_get_skeys_device();
+        skeyclass = S390_SKEYS_GET_CLASS(ss);
+    }
+
+    key = (uint8_t) r1;
+    skeyclass->set_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key);
 }
 
 /* reset reference bit extended */
 uint32_t HELPER(rrbe)(CPUS390XState *env, uint64_t r2)
 {
-    uint8_t re;
-    uint8_t key;
+    static S390SKeysState *ss;
+    static S390SKeysClass *skeyclass;
+    uint8_t re, key;
 
     if (r2 > ram_size) {
         return 0;
     }
 
-    key = env->storage_keys[r2 / TARGET_PAGE_SIZE];
+    if (unlikely(!ss)) {
+        ss = s390_get_skeys_device();
+        skeyclass = S390_SKEYS_GET_CLASS(ss);
+    }
+
+    if (skeyclass->get_skeys(ss, r2 / TARGET_PAGE_SIZE, 1, &key)) {
+        return 0;
+    }
+
     re = key & (SK_R | SK_C);
-    env->storage_keys[r2 / TARGET_PAGE_SIZE] = (key & ~SK_R);
+    key &= ~SK_R;
+
+    if (skeyclass->set_skeys(ss, r2 / TARGET_PAGE_SIZE, 1, &key)) {
+        return 0;
+    }
 
     /*
      * cc
diff --git a/target-s390x/mmu_helper.c b/target-s390x/mmu_helper.c
index 1ea6d81..058a370 100644
--- a/target-s390x/mmu_helper.c
+++ b/target-s390x/mmu_helper.c
@@ -19,6 +19,8 @@
 #include "exec/address-spaces.h"
 #include "cpu.h"
 #include "sysemu/kvm.h"
+#include "trace.h"
+#include "hw/s390x/storage-keys.h"
 
 /* #define DEBUG_S390 */
 /* #define DEBUG_S390_PTE */
@@ -309,8 +311,15 @@ static int mmu_translate_asce(CPUS390XState *env, target_ulong vaddr,
 int mmu_translate(CPUS390XState *env, target_ulong vaddr, int rw, uint64_t asc,
                   target_ulong *raddr, int *flags, bool exc)
 {
+    static S390SKeysState *ss;
+    static S390SKeysClass *skeyclass;
     int r = -1;
-    uint8_t *sk;
+    uint8_t key;
+
+    if (unlikely(!ss)) {
+        ss = s390_get_skeys_device();
+        skeyclass = S390_SKEYS_GET_CLASS(ss);
+    }
 
     *flags = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
     vaddr &= TARGET_PAGE_MASK;
@@ -358,14 +367,23 @@ int mmu_translate(CPUS390XState *env, target_ulong vaddr, int rw, uint64_t asc,
     /* Convert real address -> absolute address */
     *raddr = mmu_real2abs(env, *raddr);
 
-    if (*raddr < ram_size) {
-        sk = &env->storage_keys[*raddr / TARGET_PAGE_SIZE];
+    if (r == 0 && *raddr < ram_size) {
+        if (skeyclass->get_skeys(ss, *raddr / TARGET_PAGE_SIZE, 1, &key)) {
+            trace_get_skeys_nonzero(r);
+            return 0;
+        }
+
         if (*flags & PAGE_READ) {
-            *sk |= SK_R;
+            key |= SK_R;
         }
 
         if (*flags & PAGE_WRITE) {
-            *sk |= SK_C;
+            key |= SK_C;
+        }
+
+        if (skeyclass->set_skeys(ss, *raddr / TARGET_PAGE_SIZE, 1, &key)) {
+            trace_set_skeys_nonzero(r);
+            return 0;
         }
     }
 
diff --git a/trace-events b/trace-events
index 8f9614a..0a82f0c 100644
--- a/trace-events
+++ b/trace-events
@@ -1373,6 +1373,10 @@ hbitmap_iter_skip_words(const void *hb, void *hbi, uint64_t pos, unsigned long c
 hbitmap_reset(void *hb, uint64_t start, uint64_t count, uint64_t sbit, uint64_t ebit) "hb %p items %"PRIu64",%"PRIu64" bits %"PRIu64"..%"PRIu64
 hbitmap_set(void *hb, uint64_t start, uint64_t count, uint64_t sbit, uint64_t ebit) "hb %p items %"PRIu64",%"PRIu64" bits %"PRIu64"..%"PRIu64
 
+# target-s390x/mmu_helper.c
+get_skeys_nonzero(int rc) "SKEY: Call to get_skeys unexpectedly returned %d"
+set_skeys_nonzero(int rc) "SKEY: Call to set_skeys unexpectedly returned %d"
+
 # target-s390x/ioinst.c
 ioinst(const char *insn) "IOINST: %s"
 ioinst_sch_id(const char *insn, int cssid, int ssid, int schid) "IOINST: %s (%x.%x.%04x)"
-- 
2.5.0

[Qemu-devel] [PATCH v2 4/8] s390x: Dump storage keys qmp command

[Cornelia Huck]

From: "Jason J. Herne" <jjherne@linux.vnet.ibm.com>

Provide a dump-skeys qmp command to allow the end user to dump storage
keys. This is useful for debugging problems with guest storage key support
within Qemu and for guest operating system developers.

Reviewed-by: Thomas Huth <thuth@linux.vnet.ibm.com>
Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
Signed-off-by: Jason J. Herne <jjherne@linux.vnet.ibm.com>
Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
 hw/s390x/s390-skeys.c | 97 +++++++++++++++++++++++++++++++++++++++++++++++++++
 monitor.c             |  7 ++++
 qapi-schema.json      | 13 +++++++
 qmp-commands.hx       | 25 +++++++++++++
 4 files changed, 142 insertions(+)

diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
index 77c42ff..ebf6a54 100644
--- a/hw/s390x/s390-skeys.c
+++ b/hw/s390x/s390-skeys.c
@@ -10,9 +10,12 @@
  */
 
 #include "hw/boards.h"
+#include "qmp-commands.h"
 #include "hw/s390x/storage-keys.h"
 #include "qemu/error-report.h"
 
+#define S390_SKEYS_BUFFER_SIZE 131072  /* Room for 128k storage keys */
+
 S390SKeysState *s390_get_skeys_device(void)
 {
     S390SKeysState *ss;
@@ -38,6 +41,100 @@ void s390_skeys_init(void)
     qdev_init_nofail(DEVICE(obj));
 }
 
+static void write_keys(QEMUFile *f, uint8_t *keys, uint64_t startgfn,
+                       uint64_t count, Error **errp)
+{
+    uint64_t curpage = startgfn;
+    uint64_t maxpage = curpage + count - 1;
+    const char *fmt = "page=%03" PRIx64 ": key(%d) => ACC=%X, FP=%d, REF=%d,"
+                      " ch=%d, reserved=%d\n";
+    char *buf = g_try_malloc(128);
+    int len;
+
+    if (!buf) {
+        error_setg(errp, "Out of memory");
+        return;
+    }
+
+    for (; curpage <= maxpage; curpage++) {
+        uint8_t acc = (*keys & 0xF0) >> 4;
+        int fp =  (*keys & 0x08);
+        int ref = (*keys & 0x04);
+        int ch = (*keys & 0x02);
+        int res = (*keys & 0x01);
+
+        len = snprintf(buf, 128, fmt, curpage,
+                       *keys, acc, fp, ref, ch, res);
+        qemu_put_buffer(f, (uint8_t *)buf, len);
+        keys++;
+    }
+
+    g_free(buf);
+}
+
+void qmp_dump_skeys(const char *filename, Error **errp)
+{
+    S390SKeysState *ss = s390_get_skeys_device();
+    S390SKeysClass *skeyclass = S390_SKEYS_GET_CLASS(ss);
+    const uint64_t total_count = ram_size / TARGET_PAGE_SIZE;
+    uint64_t handled_count = 0, cur_count;
+    Error *lerr = NULL;
+    vaddr cur_gfn = 0;
+    uint8_t *buf;
+    int ret;
+    QEMUFile *f;
+
+    /* Quick check to see if guest is using storage keys*/
+    if (!skeyclass->skeys_enabled(ss)) {
+        error_setg(&lerr, "This guest is not using storage keys. "
+                         "Nothing to dump.");
+        error_propagate(errp, lerr);
+        return;
+    }
+
+    f = qemu_fopen(filename, "wb");
+    if (!f) {
+        error_setg(&lerr, "Could not open file");
+        error_propagate(errp, lerr);
+        return;
+    }
+
+    buf = g_try_malloc(S390_SKEYS_BUFFER_SIZE);
+    if (!buf) {
+        error_setg(&lerr, "Could not allocate memory");
+        error_propagate(errp, lerr);
+        goto out;
+    }
+
+    /* we'll only dump initial memory for now */
+    while (handled_count < total_count) {
+        /* Calculate how many keys to ask for & handle overflow case */
+        cur_count = MIN(total_count - handled_count, S390_SKEYS_BUFFER_SIZE);
+
+        ret = skeyclass->get_skeys(ss, cur_gfn, cur_count, buf);
+        if (ret < 0) {
+            error_setg(&lerr, "get_keys error %d", ret);
+            error_propagate(errp, lerr);
+            goto out_free;
+        }
+
+        /* write keys to stream */
+        write_keys(f, buf, cur_gfn, cur_count, &lerr);
+        if (lerr) {
+            error_propagate(errp, lerr);
+            goto out_free;
+        }
+
+        cur_gfn += cur_count;
+        handled_count += cur_count;
+    }
+
+out_free:
+    g_free(buf);
+out:
+    qemu_fclose(f);
+}
+
 static void qemu_s390_skeys_init(Object *obj)
 {
     QEMUS390SKeysState *skeys = QEMU_S390_SKEYS(obj);
diff --git a/monitor.c b/monitor.c
index fc32f12..daa3d98 100644
--- a/monitor.c
+++ b/monitor.c
@@ -5361,3 +5361,10 @@ void qmp_rtc_reset_reinjection(Error **errp)
     error_setg(errp, QERR_FEATURE_DISABLED, "rtc-reset-reinjection");
 }
 #endif
+
+#ifndef TARGET_S390X
+void qmp_dump_skeys(const char *filename, Error **errp)
+{
+    error_setg(errp, QERR_FEATURE_DISABLED, "dump-skeys");
+}
+#endif
diff --git a/qapi-schema.json b/qapi-schema.json
index 4342a08..1213b4e 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -2058,6 +2058,19 @@
   'returns': 'DumpGuestMemoryCapability' }
 
 ##
+# @dump-skeys
+#
+# Dump guest's storage keys.  @filename: the path to the file to dump to.
+# This command is only supported on s390 architecture.
+#
+# Returns: nothing on success
+#
+# Since: 2.5
+##
+{ 'command': 'dump-skeys',
+  'data': { 'filename': 'str' } }
+
+##
 # @netdev_add:
 #
 # Add a network backend.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index ba630b1..9848fd8 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -872,6 +872,31 @@ Example:
 
 EQMP
 
+#if defined TARGET_S390X
+    {
+        .name       = "dump-skeys",
+        .args_type  = "filename:F",
+        .mhandler.cmd_new = qmp_marshal_input_dump_skeys,
+    },
+#endif
+
+SQMP
+dump-skeys
+----------
+
+Save guest storage keys to file.
+
+Arguments:
+
+- "filename": file path (json-string)
+
+Example:
+
+-> { "execute": "dump-skeys", "arguments": { "filename": "/tmp/skeys" } }
+<- { "return": {} }
+
+EQMP
+
     {
         .name       = "netdev_add",
         .args_type  = "netdev:O",
-- 
2.5.0

[Qemu-devel] [PATCH v2 5/8] s390x: Dump-skeys hmp support

[Cornelia Huck]

From: "Jason J. Herne" <jjherne@linux.vnet.ibm.com>

Add dump-skeys command to the human monitor.

Reviewed-by: Thomas Huth <thuth@linux.vnet.ibm.com>
Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
Signed-off-by: Jason J. Herne <jjherne@linux.vnet.ibm.com>
Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
 hmp-commands.hx                 | 16 ++++++++++++++++
 hw/s390x/s390-skeys.c           | 12 ++++++++++++
 include/hw/s390x/storage-keys.h |  2 ++
 monitor.c                       |  4 ++++
 4 files changed, 34 insertions(+)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index d3b7932..803ff91 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1053,6 +1053,22 @@ gdb. Without -z|-l|-s, the dump format is ELF.
             together with begin.
 ETEXI
 
+#if defined(TARGET_S390X)
+    {
+        .name       = "dump-skeys",
+        .args_type  = "filename:F",
+        .params     = "",
+        .help       = "Save guest storage keys into file 'filename'.\n",
+        .mhandler.cmd = hmp_dump_skeys,
+    },
+#endif
+
+STEXI
+@item dump-skeys @var{filename}
+@findex dump-skeys
+Save guest storage keys to a file.
+ETEXI
+
     {
         .name       = "snapshot_blkdev",
         .args_type  = "reuse:-n,device:B,snapshot-file:s?,format:s?",
diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
index ebf6a54..f6a29ab 100644
--- a/hw/s390x/s390-skeys.c
+++ b/hw/s390x/s390-skeys.c
@@ -72,6 +72,18 @@ static void write_keys(QEMUFile *f, uint8_t *keys, uint64_t startgfn,
     g_free(buf);
 }
 
+void hmp_dump_skeys(Monitor *mon, const QDict *qdict)
+{
+    const char *filename = qdict_get_str(qdict, "filename");
+    Error *err = NULL;
+
+    qmp_dump_skeys(filename, &err);
+    if (err) {
+        monitor_printf(mon, "%s\n", error_get_pretty(err));
+        error_free(err);
+    }
+}
+
 void qmp_dump_skeys(const char *filename, Error **errp)
 {
     S390SKeysState *ss = s390_get_skeys_device();
diff --git a/include/hw/s390x/storage-keys.h b/include/hw/s390x/storage-keys.h
index cfd7da7..0d04f19 100644
--- a/include/hw/s390x/storage-keys.h
+++ b/include/hw/s390x/storage-keys.h
@@ -13,6 +13,7 @@
 #define __S390_STORAGE_KEYS_H
 
 #include <hw/qdev.h>
+#include "monitor/monitor.h"
 
 #define TYPE_S390_SKEYS "s390-skeys"
 #define S390_SKEYS(obj) \
@@ -52,4 +53,5 @@ void s390_skeys_init(void);
 
 S390SKeysState *s390_get_skeys_device(void);
 
+void hmp_dump_skeys(Monitor *mon, const QDict *qdict);
 #endif /* __S390_STORAGE_KEYS_H */
diff --git a/monitor.c b/monitor.c
index daa3d98..3deba38 100644
--- a/monitor.c
+++ b/monitor.c
@@ -82,6 +82,10 @@
 #endif
 #include "hw/lm32/lm32_pic.h"
 
+#if defined(TARGET_S390X)
+#include "hw/s390x/storage-keys.h"
+#endif
+
 /*
  * Supported types:
  *
-- 
2.5.0

[Qemu-devel] [PATCH v2 6/8] s390x: Info skeys sub-command

[Cornelia Huck]

From: "Jason J. Herne" <jjherne@linux.vnet.ibm.com>

Provide an  info skeys hmp sub-command to allow the end user to dump a storage
key for a given address. This is useful for guest operating system developers.

Reviewed-by: Thomas Huth <thuth@linux.vnet.ibm.com>
Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
Signed-off-by: Jason J. Herne <jjherne@linux.vnet.ibm.com>
Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
 hmp-commands.hx                 |  2 ++
 hw/s390x/s390-skeys.c           | 23 +++++++++++++++++++++++
 include/hw/s390x/storage-keys.h |  2 ++
 monitor.c                       |  9 +++++++++
 4 files changed, 36 insertions(+)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 803ff91..c61468e 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1806,6 +1806,8 @@ show roms
 show the TPM device
 @item info memory-devices
 show the memory devices
+@item info skeys
+Display the value of a storage key (s390 only)
 @end table
 ETEXI
 
diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
index f6a29ab..0b13d77 100644
--- a/hw/s390x/s390-skeys.c
+++ b/hw/s390x/s390-skeys.c
@@ -72,6 +72,29 @@ static void write_keys(QEMUFile *f, uint8_t *keys, uint64_t startgfn,
     g_free(buf);
 }
 
+void hmp_info_skeys(Monitor *mon, const QDict *qdict)
+{
+    S390SKeysState *ss = s390_get_skeys_device();
+    S390SKeysClass *skeyclass = S390_SKEYS_GET_CLASS(ss);
+    uint64_t addr = qdict_get_int(qdict, "addr");
+    uint8_t key;
+    int r;
+
+    /* Quick check to see if guest is using storage keys*/
+    if (!skeyclass->skeys_enabled(ss)) {
+        monitor_printf(mon, "Error: This guest is not using storage keys.\n");
+        return;
+    }
+
+    r = skeyclass->get_skeys(ss, addr / TARGET_PAGE_SIZE, 1, &key);
+    if (r < 0) {
+        monitor_printf(mon, "Error: %s\n", strerror(-r));
+        return;
+    }
+
+    monitor_printf(mon, "  key: 0x%X\n", key);
+}
+
 void hmp_dump_skeys(Monitor *mon, const QDict *qdict)
 {
     const char *filename = qdict_get_str(qdict, "filename");
diff --git a/include/hw/s390x/storage-keys.h b/include/hw/s390x/storage-keys.h
index 0d04f19..18e08d2 100644
--- a/include/hw/s390x/storage-keys.h
+++ b/include/hw/s390x/storage-keys.h
@@ -54,4 +54,6 @@ void s390_skeys_init(void);
 S390SKeysState *s390_get_skeys_device(void);
 
 void hmp_dump_skeys(Monitor *mon, const QDict *qdict);
+void hmp_info_skeys(Monitor *mon, const QDict *qdict);
+
 #endif /* __S390_STORAGE_KEYS_H */
diff --git a/monitor.c b/monitor.c
index 3deba38..451af6f 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2881,6 +2881,15 @@ static mon_cmd_t info_cmds[] = {
         .help       = "Show rocker OF-DPA groups",
         .mhandler.cmd = hmp_rocker_of_dpa_groups,
     },
+#if defined(TARGET_S390X)
+    {
+        .name       = "skeys",
+        .args_type  = "addr:l",
+        .params     = "address",
+        .help       = "Display the value of a storage key",
+        .mhandler.cmd = hmp_info_skeys,
+    },
+#endif
     {
         .name       = NULL,
     },
-- 
2.5.0

[Qemu-devel] [PATCH v2 7/8] s390x: Migrate guest storage keys (initial memory only)

[Cornelia Huck]

From: "Jason J. Herne" <jjherne@linux.vnet.ibm.com>

Routines to save/load guest storage keys are provided. register_savevm is
called to register them as migration handlers.

We prepare the protocol to support more complex parameters. So we will
later be able to support standby memory (having empty holes), compression
and "state live migration" like done for ram.

Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
Signed-off-by: Jason J. Herne <jjherne@linux.vnet.ibm.com>
Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
 hw/s390x/s390-skeys.c | 125 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 125 insertions(+)

diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
index 0b13d77..9d4a79d 100644
--- a/hw/s390x/s390-skeys.c
+++ b/hw/s390x/s390-skeys.c
@@ -11,10 +11,14 @@
 
 #include "hw/boards.h"
 #include "qmp-commands.h"
+#include "migration/qemu-file.h"
 #include "hw/s390x/storage-keys.h"
 #include "qemu/error-report.h"
 
 #define S390_SKEYS_BUFFER_SIZE 131072  /* Room for 128k storage keys */
+#define S390_SKEYS_SAVE_FLAG_EOS 0x01
+#define S390_SKEYS_SAVE_FLAG_SKEYS 0x02
+#define S390_SKEYS_SAVE_FLAG_ERROR 0x04
 
 S390SKeysState *s390_get_skeys_device(void)
 {
@@ -247,6 +251,126 @@ static const TypeInfo qemu_s390_skeys_info = {
     .instance_size = sizeof(S390SKeysClass),
 };
 
+static void s390_storage_keys_save(QEMUFile *f, void *opaque)
+{
+    S390SKeysState *ss = S390_SKEYS(opaque);
+    S390SKeysClass *skeyclass = S390_SKEYS_GET_CLASS(ss);
+    uint64_t pages_left = ram_size / TARGET_PAGE_SIZE;
+    uint64_t read_count, eos = S390_SKEYS_SAVE_FLAG_EOS;
+    vaddr cur_gfn = 0;
+    int error = 0;
+    uint8_t *buf;
+
+    if (!skeyclass->skeys_enabled(ss)) {
+        goto end_stream;
+    }
+
+    buf = g_try_malloc(S390_SKEYS_BUFFER_SIZE);
+    if (!buf) {
+        error_report("storage key save could not allocate memory\n");
+        goto end_stream;
+    }
+
+    /* We only support initial memory. Standby memory is not handled yet. */
+    qemu_put_be64(f, (cur_gfn * TARGET_PAGE_SIZE) | S390_SKEYS_SAVE_FLAG_SKEYS);
+    qemu_put_be64(f, pages_left);
+
+    while (pages_left) {
+        read_count = MIN(pages_left, S390_SKEYS_BUFFER_SIZE);
+
+        if (!error) {
+            error = skeyclass->get_skeys(ss, cur_gfn, read_count, buf);
+            if (error) {
+                /*
+                 * If error: we want to fill the stream with valid data instead
+                 * of stopping early so we pad the stream with 0x00 values and
+                 * use S390_SKEYS_SAVE_FLAG_ERROR to indicate failure to the
+                 * reading side.
+                 */
+                error_report("S390_GET_KEYS error %d\n", error);
+                memset(buf, 0, S390_SKEYS_BUFFER_SIZE);
+                eos = S390_SKEYS_SAVE_FLAG_ERROR;
+            }
+        }
+
+        qemu_put_buffer(f, buf, read_count);
+        cur_gfn += read_count;
+        pages_left -= read_count;
+    }
+
+    g_free(buf);
+end_stream:
+    qemu_put_be64(f, eos);
+}
+
+static int s390_storage_keys_load(QEMUFile *f, void *opaque, int version_id)
+{
+    S390SKeysState *ss = S390_SKEYS(opaque);
+    S390SKeysClass *skeyclass = S390_SKEYS_GET_CLASS(ss);
+    int ret = 0;
+
+    while (!ret) {
+        ram_addr_t addr;
+        int flags;
+
+        addr = qemu_get_be64(f);
+        flags = addr & ~TARGET_PAGE_MASK;
+        addr &= TARGET_PAGE_MASK;
+
+        switch (flags) {
+        case S390_SKEYS_SAVE_FLAG_SKEYS: {
+            const uint64_t total_count = qemu_get_be64(f);
+            uint64_t handled_count = 0, cur_count;
+            uint64_t cur_gfn = addr / TARGET_PAGE_SIZE;
+            uint8_t *buf = g_try_malloc(S390_SKEYS_BUFFER_SIZE);
+
+            if (!buf) {
+                error_report("storage key load could not allocate memory\n");
+                ret = -ENOMEM;
+                break;
+            }
+
+            while (handled_count < total_count) {
+                cur_count = MIN(total_count - handled_count,
+                                S390_SKEYS_BUFFER_SIZE);
+                qemu_get_buffer(f, buf, cur_count);
+
+                ret = skeyclass->set_skeys(ss, cur_gfn, cur_count, buf);
+                if (ret < 0) {
+                    error_report("S390_SET_KEYS error %d\n", ret);
+                    break;
+                }
+                handled_count += cur_count;
+                cur_gfn += cur_count;
+            }
+            g_free(buf);
+            break;
+        }
+        case S390_SKEYS_SAVE_FLAG_ERROR: {
+            error_report("Storage key data is incomplete.");
+            ret = -EINVAL;
+            break;
+        }
+        case S390_SKEYS_SAVE_FLAG_EOS:
+            /* normal exit */
+            return 0;
+        default:
+            error_report("Unexpected storage key flag data: %#x", flags);
+            ret = -EINVAL;
+        }
+    }
+
+    return ret;
+}
+
+static void s390_skeys_instance_init(Object *obj)
+{
+    S390SKeysState *ss = S390_SKEYS(obj);
+
+    register_savevm(NULL, TYPE_S390_SKEYS, 0, 1, s390_storage_keys_save,
+                    s390_storage_keys_load, ss);
+}
+
 static void s390_skeys_class_init(ObjectClass *oc, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(oc);
@@ -258,6 +382,7 @@ static void s390_skeys_class_init(ObjectClass *oc, void *data)
 static const TypeInfo s390_skeys_info = {
     .name          = TYPE_S390_SKEYS,
     .parent        = TYPE_DEVICE,
+    .instance_init = s390_skeys_instance_init,
     .instance_size = sizeof(S390SKeysState),
     .class_init    = s390_skeys_class_init,
     .class_size    = sizeof(S390SKeysClass),
-- 
2.5.0

[Qemu-devel] [PATCH v2 8/8] s390x: Disable storage key migration on old machine type

[Cornelia Huck]

From: "Jason J. Herne" <jjherne@linux.vnet.ibm.com>

This code disables storage key migration when an older machine type is
specified.

Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
Signed-off-by: Jason J. Herne <jjherne@linux.vnet.ibm.com>
Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
---
 hw/s390x/s390-skeys.c           | 33 ++++++++++++++++++++++++++++++---
 hw/s390x/s390-virtio-ccw.c      | 12 ++++++++++++
 include/hw/s390x/storage-keys.h |  1 +
 3 files changed, 43 insertions(+), 3 deletions(-)

diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
index 9d4a79d..392d547 100644
--- a/hw/s390x/s390-skeys.c
+++ b/hw/s390x/s390-skeys.c
@@ -363,12 +363,39 @@ static int s390_storage_keys_load(QEMUFile *f, void *opaque, int version_id)
     return ret;
 }
 
-static void s390_skeys_instance_init(Object *obj)
+static inline bool s390_skeys_get_migration_enabled(Object *obj, Error **errp)
+{
+    S390SKeysState *ss = S390_SKEYS(obj);
+
+    return ss->migration_enabled;
+}
+
+static inline void s390_skeys_set_migration_enabled(Object *obj, bool value,
+                                            Error **errp)
 {
     S390SKeysState *ss = S390_SKEYS(obj);
 
-    register_savevm(NULL, TYPE_S390_SKEYS, 0, 1, s390_storage_keys_save,
-                    s390_storage_keys_load, ss);
+    /* Prevent double registration of savevm handler */
+    if (ss->migration_enabled == value) {
+        return;
+    }
+
+    ss->migration_enabled = value;
+
+    if (ss->migration_enabled) {
+        register_savevm(NULL, TYPE_S390_SKEYS, 0, 1, s390_storage_keys_save,
+                        s390_storage_keys_load, ss);
+    } else {
+        unregister_savevm(DEVICE(ss), TYPE_S390_SKEYS, ss);
+    }
+}
+
+static void s390_skeys_instance_init(Object *obj)
+{
+    object_property_add_bool(obj, "migration-enabled",
+                             s390_skeys_get_migration_enabled,
+                             s390_skeys_set_migration_enabled, NULL);
+    object_property_set_bool(obj, true, "migration-enabled", NULL);
 }
 
 static void s390_skeys_class_init(ObjectClass *oc, void *data)
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 0a057ae..e2a26e9 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -282,12 +282,24 @@ static const TypeInfo ccw_machine_info = {
     },
 };
 
+#define CCW_COMPAT_2_4 \
+        {\
+            .driver   = TYPE_S390_SKEYS,\
+            .property = "migration-enabled",\
+            .value    = "off",\
+        },
+
 static void ccw_machine_2_4_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
+    static GlobalProperty compat_props[] = {
+        CCW_COMPAT_2_4
+        { /* end of list */ }
+    };
 
     mc->name = "s390-ccw-virtio-2.4";
     mc->desc = "VirtIO-ccw based S390 machine v2.4";
+    mc->compat_props = compat_props;
 }
 
 static const TypeInfo ccw_machine_2_4_info = {
diff --git a/include/hw/s390x/storage-keys.h b/include/hw/s390x/storage-keys.h
index 18e08d2..72b850c 100644
--- a/include/hw/s390x/storage-keys.h
+++ b/include/hw/s390x/storage-keys.h
@@ -21,6 +21,7 @@
 
 typedef struct S390SKeysState {
     DeviceState parent_obj;
+    bool migration_enabled;
 
 } S390SKeysState;
 
-- 
2.5.0

Re: [Qemu-devel] [PATCH v2 4/8] s390x: Dump storage keys qmp command

[Eric Blake]

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

On 08/25/2015 10:10 AM, Cornelia Huck wrote:
> From: "Jason J. Herne" <jjherne@linux.vnet.ibm.com>
> 
> Provide a dump-skeys qmp command to allow the end user to dump storage
> keys. This is useful for debugging problems with guest storage key support
> within Qemu and for guest operating system developers.
> 
> Reviewed-by: Thomas Huth <thuth@linux.vnet.ibm.com>
> Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
> Signed-off-by: Jason J. Herne <jjherne@linux.vnet.ibm.com>
> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
> ---

>  
> +static void write_keys(QEMUFile *f, uint8_t *keys, uint64_t startgfn,
> +                       uint64_t count, Error **errp)
> +{
> +    uint64_t curpage = startgfn;
> +    uint64_t maxpage = curpage + count - 1;
> +    const char *fmt = "page=%03" PRIx64 ": key(%d) => ACC=%X, FP=%d, REF=%d,"
> +                      " ch=%d, reserved=%d\n";
> +    char *buf = g_try_malloc(128);
> +    int len;
> +
> +    if (!buf) {
> +        error_setg(errp, "Out of memory");
> +        return;
> +    }

128 bytes is small enough to just stack-allocate, and forget about
malloc().  Even if you insist on malloc'ing, a simple g_malloc() is
nicer than g_try_malloc(), as it is unlikely to fail (and if it DOES
fail, something else is likely to fail soon) - we tend to reserve
g_try_malloc() for potentially large allocations where failure is more
likely.

> +
> +    for (; curpage <= maxpage; curpage++) {
> +        uint8_t acc = (*keys & 0xF0) >> 4;
> +        int fp =  (*keys & 0x08);
> +        int ref = (*keys & 0x04);
> +        int ch = (*keys & 0x02);
> +        int res = (*keys & 0x01);
> +
> +        len = snprintf(buf, 128, fmt, curpage,

If you stack-allocate buf, then sizeof(buf) is nicer than hard-coded 128
here.

> +                       *keys, acc, fp, ref, ch, res);
> +        qemu_put_buffer(f, (uint8_t *)buf, len);

Potential bug. snprintf() returns how many bytes WOULD have been printed
if the buffer is large enough, and may therefore be larger than 128 if
your buffer size guess was wrong or the format string is edited.  The
only way to safely use snprintf is to first check that the result is no
larger than the input, before passing the string on to qemu_put_buffer().

> +void qmp_dump_skeys(const char *filename, Error **errp)
> +{
> +    S390SKeysState *ss = s390_get_skeys_device();
> +    S390SKeysClass *skeyclass = S390_SKEYS_GET_CLASS(ss);
> +    const uint64_t total_count = ram_size / TARGET_PAGE_SIZE;
> +    uint64_t handled_count = 0, cur_count;
> +    Error *lerr = NULL;
> +    vaddr cur_gfn = 0;
> +    uint8_t *buf;
> +    int ret;
> +    QEMUFile *f;
> +
> +    /* Quick check to see if guest is using storage keys*/
> +    if (!skeyclass->skeys_enabled(ss)) {
> +        error_setg(&lerr, "This guest is not using storage keys. "
> +                         "Nothing to dump.");

Error messages don't usually end in '.'

> +        error_propagate(errp, lerr);

Instead of setting the local error just to propagate it, just write the
error message directly into errp, as in:

error_setg(errp, ...)

> +        return;
> +    }
> +
> +    f = qemu_fopen(filename, "wb");
> +    if (!f) {
> +        error_setg(&lerr, "Could not open file");
> +        error_propagate(errp, lerr);

Same story. Also, we have error_setg_file_open() which is more
appropriate to use here.

> +        ret = skeyclass->get_skeys(ss, cur_gfn, cur_count, buf);
> +        if (ret < 0) {
> +            error_setg(&lerr, "get_keys error %d", ret);
> +            error_propagate(errp, lerr);
> +            goto out_free;
> +        }
> +
> +        /* write keys to stream */
> +        write_keys(f, buf, cur_gfn, cur_count, &lerr);
> +        if (lerr) {
> +            error_propagate(errp, lerr);
> +            goto out_free;

Instead of propagating the error on every caller...

> +        }
> +
> +        cur_gfn += cur_count;
> +        handled_count += cur_count;
> +    }
> +
> +out_free:
> +    g_free(buf);

you could do it just once here unconditionally (it is safe to call
error_propagate(..., NULL) when no error occurred).

> +++ b/qapi-schema.json
> @@ -2058,6 +2058,19 @@
>    'returns': 'DumpGuestMemoryCapability' }
>  
>  ##
> +# @dump-skeys
> +#
> +# Dump guest's storage keys.  @filename: the path to the file to dump to.

Newline before @filename, please.

> +# This command is only supported on s390 architecture.

It would be nice if we fixed the qapi generator to allow conditional
compilation of the .json files, so that the command is not even exposed
on other platforms.  Markus mentioned that at KVM Forum as one of the
possible followups to pursue after his current pending series on
introspection lands. [1]

> +#
> +# Returns: nothing on success

The 'Returns' line adds no information, so it is better omitted.

> +#
> +# Since: 2.5
> +##
> +{ 'command': 'dump-skeys',
> +  'data': { 'filename': 'str' } }
> +
> +##
>  # @netdev_add:
>  #
>  # Add a network backend.
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index ba630b1..9848fd8 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -872,6 +872,31 @@ Example:
>  
>  EQMP
>  
> +#if defined TARGET_S390X
> +    {
> +        .name       = "dump-skeys",
> +        .args_type  = "filename:F",
> +        .mhandler.cmd_new = qmp_marshal_input_dump_skeys,
> +    },
> +#endif

[1] At any rate, as long as we have the .hx file that does support
conditional compilation, I think 'query-commands' properly shows whether
the command is present, even if Markus' addition of 'query-schema' does
not have the same luxury of omitting unused stuff.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH v2 4/8] s390x: Dump storage keys qmp command

[Jason J. Herne]

On 08/25/2015 12:51 PM, Eric Blake wrote:
> On 08/25/2015 10:10 AM, Cornelia Huck wrote:
>> From: "Jason J. Herne" <jjherne@linux.vnet.ibm.com>
>>
>> Provide a dump-skeys qmp command to allow the end user to dump storage
>> keys. This is useful for debugging problems with guest storage key support
>> within Qemu and for guest operating system developers.
>>
>> Reviewed-by: Thomas Huth <thuth@linux.vnet.ibm.com>
>> Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
>> Signed-off-by: Jason J. Herne <jjherne@linux.vnet.ibm.com>
>> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
>> ---
>
>>
>> +static void write_keys(QEMUFile *f, uint8_t *keys, uint64_t startgfn,
>> +                       uint64_t count, Error **errp)
>> +{
>> +    uint64_t curpage = startgfn;
>> +    uint64_t maxpage = curpage + count - 1;
>> +    const char *fmt = "page=%03" PRIx64 ": key(%d) => ACC=%X, FP=%d, REF=%d,"
>> +                      " ch=%d, reserved=%d\n";
>> +    char *buf = g_try_malloc(128);
>> +    int len;
>> +
>> +    if (!buf) {
>> +        error_setg(errp, "Out of memory");
>> +        return;
>> +    }
>
> 128 bytes is small enough to just stack-allocate, and forget about
> malloc().  Even if you insist on malloc'ing, a simple g_malloc() is
> nicer than g_try_malloc(), as it is unlikely to fail (and if it DOES
> fail, something else is likely to fail soon) - we tend to reserve
> g_try_malloc() for potentially large allocations where failure is more
> likely.
>

Will do.

>> +
>> +    for (; curpage <= maxpage; curpage++) {
>> +        uint8_t acc = (*keys & 0xF0) >> 4;
>> +        int fp =  (*keys & 0x08);
>> +        int ref = (*keys & 0x04);
>> +        int ch = (*keys & 0x02);
>> +        int res = (*keys & 0x01);
>> +
>> +        len = snprintf(buf, 128, fmt, curpage,
>
> If you stack-allocate buf, then sizeof(buf) is nicer than hard-coded 128
> here.
>

Will use sizeof()

>> +                       *keys, acc, fp, ref, ch, res);
>> +        qemu_put_buffer(f, (uint8_t *)buf, len);
>
> Potential bug. snprintf() returns how many bytes WOULD have been printed
> if the buffer is large enough, and may therefore be larger than 128 if
> your buffer size guess was wrong or the format string is edited.  The
> only way to safely use snprintf is to first check that the result is no
> larger than the input, before passing the string on to qemu_put_buffer().
>

I chose 128 because it is large enough to handle even the most extreme 
cases.
I understand that someone could change the format string later on but if 
that
is the case then they should update the buffer size accordingly. I can add a
comment to that effect if you think that is good. But I'd like to avoid
over engineering something that is fairly simple.

>> +void qmp_dump_skeys(const char *filename, Error **errp)
>> +{
>> +    S390SKeysState *ss = s390_get_skeys_device();
>> +    S390SKeysClass *skeyclass = S390_SKEYS_GET_CLASS(ss);
>> +    const uint64_t total_count = ram_size / TARGET_PAGE_SIZE;
>> +    uint64_t handled_count = 0, cur_count;
>> +    Error *lerr = NULL;
>> +    vaddr cur_gfn = 0;
>> +    uint8_t *buf;
>> +    int ret;
>> +    QEMUFile *f;
>> +
>> +    /* Quick check to see if guest is using storage keys*/
>> +    if (!skeyclass->skeys_enabled(ss)) {
>> +        error_setg(&lerr, "This guest is not using storage keys. "
>> +                         "Nothing to dump.");
>
> Error messages don't usually end in '.'
>

Will fix.

>> +        error_propagate(errp, lerr);
>
> Instead of setting the local error just to propagate it, just write the
> error message directly into errp, as in:
>
> error_setg(errp, ...)
>

Ok, will fix.

>> +        return;
>> +    }
>> +
>> +    f = qemu_fopen(filename, "wb");
>> +    if (!f) {
>> +        error_setg(&lerr, "Could not open file");
>> +        error_propagate(errp, lerr);
>
> Same story. Also, we have error_setg_file_open() which is more
> appropriate to use here.
>

Neat :) Will switch to that.

>> +        ret = skeyclass->get_skeys(ss, cur_gfn, cur_count, buf);
>> +        if (ret < 0) {
>> +            error_setg(&lerr, "get_keys error %d", ret);
>> +            error_propagate(errp, lerr);
>> +            goto out_free;
>> +        }
>> +
>> +        /* write keys to stream */
>> +        write_keys(f, buf, cur_gfn, cur_count, &lerr);
>> +        if (lerr) {
>> +            error_propagate(errp, lerr);
>> +            goto out_free;
>
> Instead of propagating the error on every caller...
>
>> +        }
>> +
>> +        cur_gfn += cur_count;
>> +        handled_count += cur_count;
>> +    }
>> +
>> +out_free:
>> +    g_free(buf);
>
> you could do it just once here unconditionally (it is safe to call
> error_propagate(..., NULL) when no error occurred).

Awesome, thanks for the tip.

>
>> +++ b/qapi-schema.json
>> @@ -2058,6 +2058,19 @@
>>     'returns': 'DumpGuestMemoryCapability' }
>>
>>   ##
>> +# @dump-skeys
>> +#
>> +# Dump guest's storage keys.  @filename: the path to the file to dump to.
>
> Newline before @filename, please.
>

Will fix.

>> +# This command is only supported on s390 architecture.
>
> It would be nice if we fixed the qapi generator to allow conditional
> compilation of the .json files, so that the command is not even exposed
> on other platforms.  Markus mentioned that at KVM Forum as one of the
> possible followups to pursue after his current pending series on
> introspection lands. [1]
>
>> +#
>> +# Returns: nothing on success
>
> The 'Returns' line adds no information, so it is better omitted.
>

Will fix.

>> +#
>> +# Since: 2.5
>> +##
>> +{ 'command': 'dump-skeys',
>> +  'data': { 'filename': 'str' } }
>> +
>> +##
>>   # @netdev_add:
>>   #
>>   # Add a network backend.
>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>> index ba630b1..9848fd8 100644
>> --- a/qmp-commands.hx
>> +++ b/qmp-commands.hx
>> @@ -872,6 +872,31 @@ Example:
>>
>>   EQMP
>>
>> +#if defined TARGET_S390X
>> +    {
>> +        .name       = "dump-skeys",
>> +        .args_type  = "filename:F",
>> +        .mhandler.cmd_new = qmp_marshal_input_dump_skeys,
>> +    },
>> +#endif
>
> [1] At any rate, as long as we have the .hx file that does support
> conditional compilation, I think 'query-commands' properly shows whether
> the command is present, even if Markus' addition of 'query-schema' does
> not have the same luxury of omitting unused stuff.
>

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