* [Qemu-devel] [PATCH for-2.5 1/8] s390x: add 2.5 compat s390-ccw-virtio machine
2015-07-20 13:49 [Qemu-devel] [PATCH for-2.5 0/8] s390x: storage key migration Cornelia Huck
@ 2015-07-20 13:49 ` Cornelia Huck
2015-07-20 13:58 ` Christian Borntraeger
2015-07-20 13:49 ` [Qemu-devel] [PATCH for-2.5 2/8] s390x: Create QOM device for s390 storage keys Cornelia Huck
` (6 subsequent siblings)
7 siblings, 1 reply; 17+ messages in thread
From: Cornelia Huck @ 2015-07-20 13:49 UTC (permalink / raw)
To: qemu-devel; +Cc: Cornelia Huck, borntraeger, jfrei, agraf, jjherne
Reviewed-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 | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 4c51d1a..708763e 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -289,7 +289,6 @@ static void ccw_machine_2_4_class_init(ObjectClass *oc, void *data)
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 +297,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.4.6
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [Qemu-devel] [PATCH for-2.5 1/8] s390x: add 2.5 compat s390-ccw-virtio machine
2015-07-20 13:49 ` [Qemu-devel] [PATCH for-2.5 1/8] s390x: add 2.5 compat s390-ccw-virtio machine Cornelia Huck
@ 2015-07-20 13:58 ` Christian Borntraeger
0 siblings, 0 replies; 17+ messages in thread
From: Christian Borntraeger @ 2015-07-20 13:58 UTC (permalink / raw)
To: Cornelia Huck, qemu-devel; +Cc: jfrei, agraf, jjherne
Am 20.07.2015 um 15:49 schrieb Cornelia Huck:
> Reviewed-by: Jason J. Herne <jjherne@linux.vnet.ibm.com>
> Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com>
for post 2.4
Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
> hw/s390x/s390-virtio-ccw.c | 18 +++++++++++++++++-
> 1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 4c51d1a..708763e 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -289,7 +289,6 @@ static void ccw_machine_2_4_class_init(ObjectClass *oc, void *data)
> 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 +297,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)
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH for-2.5 2/8] s390x: Create QOM device for s390 storage keys
2015-07-20 13:49 [Qemu-devel] [PATCH for-2.5 0/8] s390x: storage key migration Cornelia Huck
2015-07-20 13:49 ` [Qemu-devel] [PATCH for-2.5 1/8] s390x: add 2.5 compat s390-ccw-virtio machine Cornelia Huck
@ 2015-07-20 13:49 ` Cornelia Huck
2015-07-20 13:49 ` [Qemu-devel] [PATCH for-2.5 3/8] s390x: Enable new s390-storage-keys device Cornelia Huck
` (5 subsequent siblings)
7 siblings, 0 replies; 17+ messages in thread
From: Cornelia Huck @ 2015-07-20 13:49 UTC (permalink / raw)
To: qemu-devel; +Cc: Cornelia Huck, borntraeger, jfrei, agraf, jjherne
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 978b717..1387537 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.4.6
^ permalink raw reply related [flat|nested] 17+ messages in thread* [Qemu-devel] [PATCH for-2.5 3/8] s390x: Enable new s390-storage-keys device
2015-07-20 13:49 [Qemu-devel] [PATCH for-2.5 0/8] s390x: storage key migration Cornelia Huck
2015-07-20 13:49 ` [Qemu-devel] [PATCH for-2.5 1/8] s390x: add 2.5 compat s390-ccw-virtio machine Cornelia Huck
2015-07-20 13:49 ` [Qemu-devel] [PATCH for-2.5 2/8] s390x: Create QOM device for s390 storage keys Cornelia Huck
@ 2015-07-20 13:49 ` Cornelia Huck
2015-07-20 13:49 ` [Qemu-devel] [PATCH for-2.5 4/8] s390x: Dump storage keys qmp command Cornelia Huck
` (4 subsequent siblings)
7 siblings, 0 replies; 17+ messages in thread
From: Cornelia Huck @ 2015-07-20 13:49 UTC (permalink / raw)
To: qemu-devel; +Cc: Cornelia Huck, borntraeger, jfrei, agraf, jjherne
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 708763e..8f1b1fc 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 2d395c5..bc59586 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.4.6
^ permalink raw reply related [flat|nested] 17+ messages in thread* [Qemu-devel] [PATCH for-2.5 4/8] s390x: Dump storage keys qmp command
2015-07-20 13:49 [Qemu-devel] [PATCH for-2.5 0/8] s390x: storage key migration Cornelia Huck
` (2 preceding siblings ...)
2015-07-20 13:49 ` [Qemu-devel] [PATCH for-2.5 3/8] s390x: Enable new s390-storage-keys device Cornelia Huck
@ 2015-07-20 13:49 ` Cornelia Huck
2015-07-20 14:32 ` Eric Blake
2015-07-20 13:49 ` [Qemu-devel] [PATCH for-2.5 5/8] s390x: Dump-skeys hmp support Cornelia Huck
` (3 subsequent siblings)
7 siblings, 1 reply; 17+ messages in thread
From: Cornelia Huck @ 2015-07-20 13:49 UTC (permalink / raw)
To: qemu-devel; +Cc: Cornelia Huck, borntraeger, jfrei, agraf, jjherne
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 | 91 +++++++++++++++++++++++++++++++++++++++++++++++++++
monitor.c | 7 ++++
qapi-schema.json | 13 ++++++++
qmp-commands.hx | 25 ++++++++++++++
4 files changed, 136 insertions(+)
diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
index 77c42ff..a7b7a01 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,94 @@ void s390_skeys_init(void)
qdev_init_nofail(DEVICE(obj));
}
+static void write_keys(FILE *f, uint8_t *keys, uint64_t startgfn,
+ uint64_t count, Error **errp)
+{
+ uint64_t curpage = startgfn;
+ uint64_t maxpage = curpage + count - 1;
+ int r;
+
+ for (; curpage <= maxpage; curpage++) {
+ uint8_t acc = (*keys & 0xF0) >> 4;
+ int fp = (*keys & 0x08);
+ int ref = (*keys & 0x04);
+ int ch = (*keys & 0x02);
+ int reserved = (*keys & 0x01);
+
+ r = fprintf(f, "page=%03" PRIx64 ": key(%d) => ACC=%X, FP=%d, REF=%d,"
+ " ch=%d, reserved=%d\n", curpage, *keys, acc, fp, ref,
+ ch, reserved);
+ if (r < 0) {
+ error_setg(errp, "I/O error");
+ return;
+ }
+ keys++;
+ }
+}
+
+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;
+ FILE *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 = 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:
+ 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 aeea2b5..f1501cd 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 1285b8c..d1c1c25 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.4.6
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [Qemu-devel] [PATCH for-2.5 4/8] s390x: Dump storage keys qmp command
2015-07-20 13:49 ` [Qemu-devel] [PATCH for-2.5 4/8] s390x: Dump storage keys qmp command Cornelia Huck
@ 2015-07-20 14:32 ` Eric Blake
0 siblings, 0 replies; 17+ messages in thread
From: Eric Blake @ 2015-07-20 14:32 UTC (permalink / raw)
To: Cornelia Huck, qemu-devel; +Cc: borntraeger, jfrei, agraf, jjherne
[-- Attachment #1: Type: text/plain, Size: 1496 bytes --]
On 07/20/2015 07:49 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>
> ---
> +void qmp_dump_skeys(const char *filename, Error **errp)
> +{
> +
> + f = fopen(filename, "wb");
If you'll use qemu_fopen() here...
> +++ 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' } }
then this command will automatically accept /dev/fdset/NNN notation for
allowing the user to pass in a file descriptor with add-fd then tying
that fd to this command (useful for when qemu is restricted from
directly calling open() for security reasons).
--
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 --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH for-2.5 5/8] s390x: Dump-skeys hmp support
2015-07-20 13:49 [Qemu-devel] [PATCH for-2.5 0/8] s390x: storage key migration Cornelia Huck
` (3 preceding siblings ...)
2015-07-20 13:49 ` [Qemu-devel] [PATCH for-2.5 4/8] s390x: Dump storage keys qmp command Cornelia Huck
@ 2015-07-20 13:49 ` Cornelia Huck
2015-07-20 13:49 ` [Qemu-devel] [PATCH for-2.5 6/8] s390x: Info skeys sub-command Cornelia Huck
` (2 subsequent siblings)
7 siblings, 0 replies; 17+ messages in thread
From: Cornelia Huck @ 2015-07-20 13:49 UTC (permalink / raw)
To: qemu-devel; +Cc: Cornelia Huck, borntraeger, jfrei, agraf, jjherne
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 a7b7a01..5e2948d 100644
--- a/hw/s390x/s390-skeys.c
+++ b/hw/s390x/s390-skeys.c
@@ -66,6 +66,18 @@ static void write_keys(FILE *f, uint8_t *keys, uint64_t startgfn,
}
}
+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 f1501cd..cfe31a4 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.4.6
^ permalink raw reply related [flat|nested] 17+ messages in thread* [Qemu-devel] [PATCH for-2.5 6/8] s390x: Info skeys sub-command
2015-07-20 13:49 [Qemu-devel] [PATCH for-2.5 0/8] s390x: storage key migration Cornelia Huck
` (4 preceding siblings ...)
2015-07-20 13:49 ` [Qemu-devel] [PATCH for-2.5 5/8] s390x: Dump-skeys hmp support Cornelia Huck
@ 2015-07-20 13:49 ` Cornelia Huck
2015-07-20 13:49 ` [Qemu-devel] [PATCH for-2.5 7/8] s390x: Migrate guest storage keys (initial memory only) Cornelia Huck
2015-07-20 13:49 ` [Qemu-devel] [PATCH for-2.5 8/8] s390x: Disable storage key migration on old machine type Cornelia Huck
7 siblings, 0 replies; 17+ messages in thread
From: Cornelia Huck @ 2015-07-20 13:49 UTC (permalink / raw)
To: qemu-devel; +Cc: Cornelia Huck, borntraeger, jfrei, agraf, jjherne
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 5e2948d..d355c8f 100644
--- a/hw/s390x/s390-skeys.c
+++ b/hw/s390x/s390-skeys.c
@@ -66,6 +66,29 @@ static void write_keys(FILE *f, uint8_t *keys, uint64_t startgfn,
}
}
+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 cfe31a4..d2153fa 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.4.6
^ permalink raw reply related [flat|nested] 17+ messages in thread* [Qemu-devel] [PATCH for-2.5 7/8] s390x: Migrate guest storage keys (initial memory only)
2015-07-20 13:49 [Qemu-devel] [PATCH for-2.5 0/8] s390x: storage key migration Cornelia Huck
` (5 preceding siblings ...)
2015-07-20 13:49 ` [Qemu-devel] [PATCH for-2.5 6/8] s390x: Info skeys sub-command Cornelia Huck
@ 2015-07-20 13:49 ` Cornelia Huck
2015-07-21 8:08 ` Thomas Huth
2015-07-20 13:49 ` [Qemu-devel] [PATCH for-2.5 8/8] s390x: Disable storage key migration on old machine type Cornelia Huck
7 siblings, 1 reply; 17+ messages in thread
From: Cornelia Huck @ 2015-07-20 13:49 UTC (permalink / raw)
To: qemu-devel; +Cc: Cornelia Huck, borntraeger, jfrei, agraf, jjherne
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 | 113 ++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 113 insertions(+)
diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
index d355c8f..a927c98 100644
--- a/hw/s390x/s390-skeys.c
+++ b/hw/s390x/s390-skeys.c
@@ -11,10 +11,13 @@
#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
S390SKeysState *s390_get_skeys_device(void)
{
@@ -241,6 +244,115 @@ 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);
+ const uint64_t total_count = ram_size / TARGET_PAGE_SIZE;
+ uint64_t cur_count, handled_count = 0;
+ vaddr cur_gfn = 0;
+ uint8_t *buf;
+ int ret;
+
+ 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 initital 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, total_count);
+
+ while (handled_count < total_count) {
+ 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_report("S390_GET_KEYS error %d\n", ret);
+ break;
+ }
+
+ /* write keys to stream */
+ qemu_put_buffer(f, buf, cur_count);
+
+ cur_gfn += cur_count;
+ handled_count += cur_count;
+ }
+
+ g_free(buf);
+end_stream:
+ qemu_put_be64(f, S390_SKEYS_SAVE_FLAG_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_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);
@@ -252,6 +364,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.4.6
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [Qemu-devel] [PATCH for-2.5 7/8] s390x: Migrate guest storage keys (initial memory only)
2015-07-20 13:49 ` [Qemu-devel] [PATCH for-2.5 7/8] s390x: Migrate guest storage keys (initial memory only) Cornelia Huck
@ 2015-07-21 8:08 ` Thomas Huth
2015-07-21 10:37 ` David Hildenbrand
0 siblings, 1 reply; 17+ messages in thread
From: Thomas Huth @ 2015-07-21 8:08 UTC (permalink / raw)
To: Cornelia Huck, qemu-devel; +Cc: borntraeger, jfrei, agraf, jjherne
[-- Attachment #1: Type: text/plain, Size: 5346 bytes --]
On 20/07/15 15:49, Cornelia Huck wrote:
> 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 | 113 ++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 113 insertions(+)
>
> diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
> index d355c8f..a927c98 100644
> --- a/hw/s390x/s390-skeys.c
> +++ b/hw/s390x/s390-skeys.c
> @@ -11,10 +11,13 @@
>
> #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
>
> S390SKeysState *s390_get_skeys_device(void)
> {
> @@ -241,6 +244,115 @@ 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);
> + const uint64_t total_count = ram_size / TARGET_PAGE_SIZE;
> + uint64_t cur_count, handled_count = 0;
> + vaddr cur_gfn = 0;
> + uint8_t *buf;
> + int ret;
> +
> + 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 initital 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, total_count);
So if I've got this code right, you send here a "header" that announces
a packet with all pages ...
> + while (handled_count < total_count) {
> + 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_report("S390_GET_KEYS error %d\n", ret);
> + break;
... but when an error occurs here, you suddenly stop in the middle of
that "packet" with all pages ...
> + }
> +
> + /* write keys to stream */
> + qemu_put_buffer(f, buf, cur_count);
> +
> + cur_gfn += cur_count;
> + handled_count += cur_count;
> + }
> +
> + g_free(buf);
> +end_stream:
> + qemu_put_be64(f, S390_SKEYS_SAVE_FLAG_EOS);
... and send an EOS marker here instead ...
> +}
> +
> +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);
... while the receiver can not handle the EOS marker here.
This looks fishy to me (or I might have just missed something), but
anyway please double check whether your error handling in the sender
really makes sense.
> + 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_EOS:
> + /* normal exit */
> + return 0;
> + default:
> + error_report("Unexpected storage key flag data: %#x", flags);
> + ret = -EINVAL;
> + }
> + }
> +
> + return ret;
> +}
Thomas
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [Qemu-devel] [PATCH for-2.5 7/8] s390x: Migrate guest storage keys (initial memory only)
2015-07-21 8:08 ` Thomas Huth
@ 2015-07-21 10:37 ` David Hildenbrand
2015-07-30 15:00 ` Jason J. Herne
` (2 more replies)
0 siblings, 3 replies; 17+ messages in thread
From: David Hildenbrand @ 2015-07-21 10:37 UTC (permalink / raw)
To: Thomas Huth, jjherne; +Cc: Cornelia Huck, borntraeger, jfrei, qemu-devel, agraf
>
> So if I've got this code right, you send here a "header" that announces
> a packet with all pages ...
>
> > + while (handled_count < total_count) {
> > + 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_report("S390_GET_KEYS error %d\n", ret);
> > + break;
>
> ... but when an error occurs here, you suddenly stop in the middle of
> that "packet" with all pages ...
Indeed, although that should never fail, we never know.
We don't want to overengineer the protocol but still abort migration at least
on the loading side in that (theoretical) case.
>
> > + }
> > +
> > + /* write keys to stream */
> > + qemu_put_buffer(f, buf, cur_count);
> > +
> > + cur_gfn += cur_count;
> > + handled_count += cur_count;
> > + }
> > +
> > + g_free(buf);
> > +end_stream:
> > + qemu_put_be64(f, S390_SKEYS_SAVE_FLAG_EOS);
>
> ... and send an EOS marker here instead ...
>
> > +}
> > +
> > +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);
>
> ... while the receiver can not handle the EOS marker here.
>
> This looks fishy to me (or I might have just missed something), but
> anyway please double check whether your error handling in the sender
> really makes sense.
My shot would be, to send invalid storage keys if getting the keys from the
kernel fails. So we can detect it on the loading side and abort migration
gracefully.
>
> > + 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_EOS:
> > + /* normal exit */
> > + return 0;
> > + default:
> > + error_report("Unexpected storage key flag data: %#x", flags);
> > + ret = -EINVAL;
> > + }
> > + }
> > +
> > + return ret;
> > +}
>
> Thomas
Thanks Thomas!
David
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [Qemu-devel] [PATCH for-2.5 7/8] s390x: Migrate guest storage keys (initial memory only)
2015-07-21 10:37 ` David Hildenbrand
@ 2015-07-30 15:00 ` Jason J. Herne
2015-07-30 15:12 ` Thomas Huth
2015-08-13 14:11 ` Jason J. Herne
2015-08-13 15:44 ` Christian Borntraeger
2 siblings, 1 reply; 17+ messages in thread
From: Jason J. Herne @ 2015-07-30 15:00 UTC (permalink / raw)
To: David Hildenbrand, Thomas Huth
Cc: Cornelia Huck, borntraeger, jfrei, qemu-devel, agraf
On 07/21/2015 06:37 AM, David Hildenbrand wrote:
>>
>> So if I've got this code right, you send here a "header" that announces
>> a packet with all pages ...
>>
>>> + while (handled_count < total_count) {
>>> + 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_report("S390_GET_KEYS error %d\n", ret);
>>> + break;
>>
>> ... but when an error occurs here, you suddenly stop in the middle of
>> that "packet" with all pages ...
>
> Indeed, although that should never fail, we never know.
> We don't want to overengineer the protocol but still abort migration at least
> on the loading side in that (theoretical) case.
>
I don't have a strong opinion on this either way. I think it is fine
just the way
it is (for the reasons David described above). However, if people are
worried I
can see about writing some code that sends fake keys to the destination as
described below. Thoughts?
>>
>>> + }
>>> +
>>> + /* write keys to stream */
>>> + qemu_put_buffer(f, buf, cur_count);
>>> +
>>> + cur_gfn += cur_count;
>>> + handled_count += cur_count;
>>> + }
>>> +
>>> + g_free(buf);
>>> +end_stream:
>>> + qemu_put_be64(f, S390_SKEYS_SAVE_FLAG_EOS);
>>
>> ... and send an EOS marker here instead ...
>>
>>> +}
>>> +
>>> +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);
>>
>> ... while the receiver can not handle the EOS marker here.
>>
>> This looks fishy to me (or I might have just missed something), but
>> anyway please double check whether your error handling in the sender
>> really makes sense.
>
> My shot would be, to send invalid storage keys if getting the keys from the
> kernel fails. So we can detect it on the loading side and abort migration
> gracefully.
>
>>
>>> + 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_EOS:
>>> + /* normal exit */
>>> + return 0;
>>> + default:
>>> + error_report("Unexpected storage key flag data: %#x", flags);
>>> + ret = -EINVAL;
>>> + }
>>> + }
>>> +
>>> + return ret;
>>> +}
>>
>> Thomas
>
> Thanks Thomas!
>
>
> David
>
--
-- Jason J. Herne (jjherne@linux.vnet.ibm.com)
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [Qemu-devel] [PATCH for-2.5 7/8] s390x: Migrate guest storage keys (initial memory only)
2015-07-30 15:00 ` Jason J. Herne
@ 2015-07-30 15:12 ` Thomas Huth
0 siblings, 0 replies; 17+ messages in thread
From: Thomas Huth @ 2015-07-30 15:12 UTC (permalink / raw)
To: jjherne, David Hildenbrand
Cc: Cornelia Huck, borntraeger, jfrei, qemu-devel, agraf
On 30/07/15 17:00, Jason J. Herne wrote:
> On 07/21/2015 06:37 AM, David Hildenbrand wrote:
>>>
>>> So if I've got this code right, you send here a "header" that announces
>>> a packet with all pages ...
>>>
>>>> + while (handled_count < total_count) {
>>>> + 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_report("S390_GET_KEYS error %d\n", ret);
>>>> + break;
>>>
>>> ... but when an error occurs here, you suddenly stop in the middle of
>>> that "packet" with all pages ...
>>
>> Indeed, although that should never fail, we never know.
>> We don't want to overengineer the protocol but still abort migration
>> at least
>> on the loading side in that (theoretical) case.
>>
>
> I don't have a strong opinion on this either way. I think it is fine
> just the way
> it is (for the reasons David described above). However, if people are
> worried I
> can see about writing some code that sends fake keys to the destination as
> described below. Thoughts?
If David is right and the skeyclass->get_skeys() really never fails (I
did not check), then simply do an "assert (ret == 0)" afterwards - that
way you can be sure that it really never fails. And if it ever fails,
you notice it immediately - and that's certainly way much better than
debugging the currently-wrong error handling code.
Thomas
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.5 7/8] s390x: Migrate guest storage keys (initial memory only)
2015-07-21 10:37 ` David Hildenbrand
2015-07-30 15:00 ` Jason J. Herne
@ 2015-08-13 14:11 ` Jason J. Herne
2015-08-13 15:44 ` Christian Borntraeger
2 siblings, 0 replies; 17+ messages in thread
From: Jason J. Herne @ 2015-08-13 14:11 UTC (permalink / raw)
To: David Hildenbrand, Thomas Huth
Cc: Cornelia Huck, borntraeger, jfrei, qemu-devel, agraf
On 07/21/2015 06:37 AM, David Hildenbrand wrote:
>>
>> So if I've got this code right, you send here a "header" that announces
>> a packet with all pages ...
>>
>>> + while (handled_count < total_count) {
>>> + 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_report("S390_GET_KEYS error %d\n", ret);
>>> + break;
>>
>> ... but when an error occurs here, you suddenly stop in the middle of
>> that "packet" with all pages ...
>
> Indeed, although that should never fail, we never know.
> We don't want to overengineer the protocol but still abort migration at least
> on the loading side in that (theoretical) case.
>
>>
>>> + }
>>> +
>>> + /* write keys to stream */
>>> + qemu_put_buffer(f, buf, cur_count);
>>> +
>>> + cur_gfn += cur_count;
>>> + handled_count += cur_count;
>>> + }
>>> +
>>> + g_free(buf);
>>> +end_stream:
>>> + qemu_put_be64(f, S390_SKEYS_SAVE_FLAG_EOS);
>>
>> ... and send an EOS marker here instead ...
>>
>>> +}
>>> +
>>> +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);
>>
>> ... while the receiver can not handle the EOS marker here.
>>
>> This looks fishy to me (or I might have just missed something), but
>> anyway please double check whether your error handling in the sender
>> really makes sense.
>
> My shot would be, to send invalid storage keys if getting the keys from the
> kernel fails. So we can detect it on the loading side and abort migration
> gracefully.
>
What storage key value would you consider invalid? All combinations of
the upper four bits are valid. And of the lower four, we have the FP,
reference and change bits with the final bit marked as reserved. The
only possible answer would be to abuse the reserved bit and set it to 1
when there is an error. The major problem with that: This bit could be
used for something someday which would require us to stop using it for
an error indicator. Another problem is that we would then have to check
every single storage key for this error bit on the destination side.
This ioctl should not fail if we've made it this far. If it does we are
still covered because the sudden hole in the data will throw off
everything else. It could (in VERY rare cases, if at all) cause error
messages to surface that are unrelated to the problem but the correct
"S390_GET_KEYS error %d" message will still be displayed first.
Certainly it is not 100% perfect but since the sending side is not
allowed to fail there seems to be simple option here. We could
re-engineer the protocol to send packets of [Length][KeyData] and we
could decide on an error value for length (0xFFFFFFFF) that would
indicate an error, perhaps with 0x0 indicating end of data. I'm happy to
do the work if requested but is it really worth it?
>>
>>> + 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_EOS:
>>> + /* normal exit */
>>> + return 0;
>>> + default:
>>> + error_report("Unexpected storage key flag data: %#x", flags);
>>> + ret = -EINVAL;
>>> + }
>>> + }
>>> +
>>> + return ret;
>>> +}
>>
>> Thomas
>
> Thanks Thomas!
>
>
> David
>
--
-- Jason J. Herne (jjherne@linux.vnet.ibm.com)
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [Qemu-devel] [PATCH for-2.5 7/8] s390x: Migrate guest storage keys (initial memory only)
2015-07-21 10:37 ` David Hildenbrand
2015-07-30 15:00 ` Jason J. Herne
2015-08-13 14:11 ` Jason J. Herne
@ 2015-08-13 15:44 ` Christian Borntraeger
2 siblings, 0 replies; 17+ messages in thread
From: Christian Borntraeger @ 2015-08-13 15:44 UTC (permalink / raw)
To: David Hildenbrand, Thomas Huth, jjherne
Cc: Cornelia Huck, jfrei, qemu-devel, agraf
Am 21.07.2015 um 12:37 schrieb David Hildenbrand:
>>
>> So if I've got this code right, you send here a "header" that announces
>> a packet with all pages ...
>>
>>> + while (handled_count < total_count) {
>>> + 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_report("S390_GET_KEYS error %d\n", ret);
>>> + break;
>>
>> ... but when an error occurs here, you suddenly stop in the middle of
>> that "packet" with all pages ...
>
> Indeed, although that should never fail, we never know.
I think -ENOMEM would be a possible return code.
> We don't want to overengineer the protocol but still abort migration at least
> on the loading side in that (theoretical) case.
[..]
>>> +end_stream:
>>> + qemu_put_be64(f, S390_SKEYS_SAVE_FLAG_EOS);
what about sending zeroes for failed keys
defining an error code
#define S390_SKEYS_SAVE_FLAG_EOS 0x01
#define S390_SKEYS_SAVE_FLAG_SKEYS 0x02
+#define S390_SKEYS_SAVE_FLAG_FAILED 0x03
and then do
if (ok) {
qemu_put_be64(f, S390_SKEYS_SAVE_FLAG_EOS);
} else {
qemu_put_be64(f, S390_SKEYS_SAVE_FLAG_FAILED);
}
and the let target system fail migration (which should let the guest run at the source system?)
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH for-2.5 8/8] s390x: Disable storage key migration on old machine type
2015-07-20 13:49 [Qemu-devel] [PATCH for-2.5 0/8] s390x: storage key migration Cornelia Huck
` (6 preceding siblings ...)
2015-07-20 13:49 ` [Qemu-devel] [PATCH for-2.5 7/8] s390x: Migrate guest storage keys (initial memory only) Cornelia Huck
@ 2015-07-20 13:49 ` Cornelia Huck
7 siblings, 0 replies; 17+ messages in thread
From: Cornelia Huck @ 2015-07-20 13:49 UTC (permalink / raw)
To: qemu-devel; +Cc: Cornelia Huck, borntraeger, jfrei, agraf, jjherne
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 | 28 +++++++++++++++++++++++++---
hw/s390x/s390-virtio-ccw.c | 12 ++++++++++++
include/hw/s390x/storage-keys.h | 1 +
3 files changed, 38 insertions(+), 3 deletions(-)
diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
index a927c98..2f23600 100644
--- a/hw/s390x/s390-skeys.c
+++ b/hw/s390x/s390-skeys.c
@@ -345,12 +345,34 @@ 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);
- register_savevm(NULL, TYPE_S390_SKEYS, 0, 1, s390_storage_keys_save,
- s390_storage_keys_load, ss);
+ return ss->migration_enabled;
+}
+
+static inline void s390_skeys_set_migration_enabled(Object *obj, bool value,
+ Error **errp)
+{
+ S390SKeysState *ss = S390_SKEYS(obj);
+
+ 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 8f1b1fc..80d4714 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -282,13 +282,25 @@ 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->alias = "s390-ccw-virtio";
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.4.6
^ permalink raw reply related [flat|nested] 17+ messages in thread