* [PATCH v6 0/2] Use DIAG318 to set Control Program Name & Version Codes
@ 2020-01-24 22:14 Collin Walling
2020-01-24 22:14 ` [PATCH v6 1/2] s390/kvm: header sync for diag318 Collin Walling
` (3 more replies)
0 siblings, 4 replies; 23+ messages in thread
From: Collin Walling @ 2020-01-24 22:14 UTC (permalink / raw)
To: qemu-s390x, qemu-devel; +Cc: borntraeger, cohuck, david, rth
Changes from v5 -> v6
Migration and DeviceObject Code:
- load/save/needed functions now check if kvm_enabled before calling
kvm_get/set and has_feat (respectively)
QEMU->KVM Code:
- added kvm_s390_* stubs for get/set functions for TCG compilation
VCPU Discussion:
- calculate the maximum allowed cpu entries by taking the SCCB size,
subtracting the offset where the CPU entries begin, then dividing
by the size of a CPU Entry struct
- if the number of CPU entries exceeds the maximum allowed entries,
print a warning and break out of the loop
- no longer imposing a reduced CPU max
Last post: https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg05535.html
The data associated with DIAGNOSE 0x318 helps to identify the underlying
hypervisor level (pre-determined by an internal set of codes), as well as the
guest environment (such as Linux, z/VM, etc). These patches, in tandem with
KVM, allow this instruction to be enabled at the guest level, and also to
enable migration of this data.
The DIAGNOSE 0x318 instruction is a privileged instruction that is executed by
the Linux kernel once and only once during setup (IPL). This requires
interception by KVM to handle the instruction call safely. The instruction sets
an 8-byte value corresponding to the environment the control program (i.e.
guest) is running with, as well as what hypervisor it is running on.
An update to the analogous KVM patches associated with this patchset are
forthcoming and I will provide a link to the post as a reply to this chain.
Guest support for the diag 318 instruction is accomplished by implementing a
device class, a cpu model feature, and adjusting the Read Info struct. The Read
Info struct adjustment coincidentally reduces the maximum number of VCPUs we
can have for a single guest by one.
The instruction is determined by a Read Info byte 134 bit 0. This new byte
expands into the space of the Read Info SCCB, which also contains CPU entries
at the tail-end of this block of data. Due to this expansion, we lose space for
one CPU entry.
A guest can still run safely with the original 248 maximum CPUs, but will lose
access to the 248th CPU entry, meaning that the hypervisor will be unable to
retrieve any information regarding that CPU (weather this means the guest
will actually run with 247 CPUs when 248 are specified is uncertain to me, but
the guest operates just fine on my end).
A device class is used for this instruction in order to streamline the
migration and reset of the DIAG 318 related data.
A CPU model feature is added for this instruction, appropriately named diag318,
and is available starting with the zEC12 full model, though as long as KVM can
support emulation of this instruction, we can theoretically enable it for _any_
CPU model. It is recommended to explicitly enable the feature via
-cpu ...,diag318=on (or via libvirt feature XML).
Collin L. Walling (2):
s390/kvm: header sync for diag318
s390: diagnose 318 info reset and migration support
hw/s390x/Makefile.objs | 1 +
hw/s390x/diag318.c | 85 +++++++++++++++++++++++++++++++++++++
hw/s390x/diag318.h | 40 +++++++++++++++++
hw/s390x/s390-virtio-ccw.c | 17 ++++++++
hw/s390x/sclp.c | 13 ++++++
include/hw/s390x/sclp.h | 2 +
linux-headers/asm-s390/kvm.h | 4 ++
target/s390x/cpu_features.h | 1 +
target/s390x/cpu_features_def.inc.h | 3 ++
target/s390x/gen-features.c | 1 +
target/s390x/kvm-stub.c | 10 +++++
target/s390x/kvm.c | 29 +++++++++++++
target/s390x/kvm_s390x.h | 2 +
13 files changed, 208 insertions(+)
create mode 100644 hw/s390x/diag318.c
create mode 100644 hw/s390x/diag318.h
--
2.7.4
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v6 1/2] s390/kvm: header sync for diag318
2020-01-24 22:14 [PATCH v6 0/2] Use DIAG318 to set Control Program Name & Version Codes Collin Walling
@ 2020-01-24 22:14 ` Collin Walling
2020-01-24 22:14 ` [PATCH v6 2/2] s390: diagnose 318 info reset and migration support Collin Walling
` (2 subsequent siblings)
3 siblings, 0 replies; 23+ messages in thread
From: Collin Walling @ 2020-01-24 22:14 UTC (permalink / raw)
To: qemu-s390x, qemu-devel; +Cc: borntraeger, cohuck, david, rth
Signed-off-by: Collin Walling <walling@linux.ibm.com>
Acked-by: David Hildenbrand <david@redhat.com>
---
| 4 ++++
1 file changed, 4 insertions(+)
--git a/linux-headers/asm-s390/kvm.h b/linux-headers/asm-s390/kvm.h
index 0138ccb..4633090 100644
--- a/linux-headers/asm-s390/kvm.h
+++ b/linux-headers/asm-s390/kvm.h
@@ -74,6 +74,7 @@ struct kvm_s390_io_adapter_req {
#define KVM_S390_VM_CRYPTO 2
#define KVM_S390_VM_CPU_MODEL 3
#define KVM_S390_VM_MIGRATION 4
+#define KVM_S390_VM_MISC 5
/* kvm attributes for mem_ctrl */
#define KVM_S390_VM_MEM_ENABLE_CMMA 0
@@ -171,6 +172,9 @@ struct kvm_s390_vm_cpu_subfunc {
#define KVM_S390_VM_MIGRATION_START 1
#define KVM_S390_VM_MIGRATION_STATUS 2
+/* kvm attributes for KVM_S390_VM_MISC */
+#define KVM_S390_VM_MISC_DIAG318 0
+
/* for KVM_GET_REGS and KVM_SET_REGS */
struct kvm_regs {
/* general purpose regs for s390 */
--
2.7.4
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v6 2/2] s390: diagnose 318 info reset and migration support
2020-01-24 22:14 [PATCH v6 0/2] Use DIAG318 to set Control Program Name & Version Codes Collin Walling
2020-01-24 22:14 ` [PATCH v6 1/2] s390/kvm: header sync for diag318 Collin Walling
@ 2020-01-24 22:14 ` Collin Walling
2020-01-27 11:20 ` David Hildenbrand
` (2 more replies)
2020-01-24 22:22 ` [PATCH v6 0/2] Use DIAG318 to set Control Program Name & Version Codes no-reply
2020-03-17 21:34 ` Collin Walling
3 siblings, 3 replies; 23+ messages in thread
From: Collin Walling @ 2020-01-24 22:14 UTC (permalink / raw)
To: qemu-s390x, qemu-devel; +Cc: borntraeger, cohuck, david, rth
DIAGNOSE 0x318 (diag318) is a privileged s390x instruction that must
be intercepted by SIE and handled via KVM. Let's introduce some
functions to communicate between QEMU and KVM via ioctls. These
will be used to get/set the diag318 information.
The availability of this instruction is determined by byte 134, bit 0
of the Read Info block. This coincidentally expands into the space used
for CPU entries by taking away one byte, which means VMs running with
the diag318 capability will not be able to retrieve information regarding
the 248th CPU. This will not effect performance, and VMs can still be
ran with 248 CPUs.
In order to simplify the migration and system reset requirements of
the diag318 data, let's introduce it as a device class and include
a VMStateDescription.
Diag318 is set to 0 during modified clear and load normal resets.
Signed-off-by: Collin Walling <walling@linux.ibm.com>
---
hw/s390x/Makefile.objs | 1 +
hw/s390x/diag318.c | 85 +++++++++++++++++++++++++++++++++++++
hw/s390x/diag318.h | 40 +++++++++++++++++
hw/s390x/s390-virtio-ccw.c | 17 ++++++++
hw/s390x/sclp.c | 13 ++++++
include/hw/s390x/sclp.h | 2 +
target/s390x/cpu_features.h | 1 +
target/s390x/cpu_features_def.inc.h | 3 ++
target/s390x/gen-features.c | 1 +
target/s390x/kvm-stub.c | 10 +++++
target/s390x/kvm.c | 29 +++++++++++++
target/s390x/kvm_s390x.h | 2 +
12 files changed, 204 insertions(+)
create mode 100644 hw/s390x/diag318.c
create mode 100644 hw/s390x/diag318.h
diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
index e02ed80..93621dc 100644
--- a/hw/s390x/Makefile.objs
+++ b/hw/s390x/Makefile.objs
@@ -34,3 +34,4 @@ obj-$(CONFIG_KVM) += s390-stattrib-kvm.o
obj-y += s390-ccw.o
obj-y += ap-device.o
obj-y += ap-bridge.o
+obj-y += diag318.o
diff --git a/hw/s390x/diag318.c b/hw/s390x/diag318.c
new file mode 100644
index 0000000..2d30bb2
--- /dev/null
+++ b/hw/s390x/diag318.c
@@ -0,0 +1,85 @@
+/*
+ * DIAGNOSE 0x318 functions for reset and migration
+ *
+ * Copyright IBM, Corp. 2019
+ *
+ * Authors:
+ * Collin Walling <walling@linux.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/diag318.h"
+#include "qapi/error.h"
+#include "kvm_s390x.h"
+#include "sysemu/kvm.h"
+
+static int diag318_post_load(void *opaque, int version_id)
+{
+ DIAG318State *d = opaque;
+
+ if (kvm_enabled())
+ kvm_s390_set_diag318_info(d->info);
+
+ return 0;
+}
+
+static int diag318_pre_save(void *opaque)
+{
+ DIAG318State *d = opaque;
+
+ if (kvm_enabled())
+ kvm_s390_get_diag318_info(&d->info);
+
+ return 0;
+}
+
+static bool diag318_needed(void *opaque)
+{
+ return kvm_enabled() ? s390_has_feat(S390_FEAT_DIAG318) : 0;
+}
+
+const VMStateDescription vmstate_diag318 = {
+ .name = "vmstate_diag318",
+ .post_load = diag318_post_load,
+ .pre_save = diag318_pre_save,
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .needed = diag318_needed,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT64(info, DIAG318State),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+static void s390_diag318_reset(DeviceState *dev)
+{
+ if (kvm_enabled())
+ kvm_s390_set_diag318_info(0);
+}
+
+static void s390_diag318_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+
+ dc->reset = s390_diag318_reset;
+ dc->vmsd = &vmstate_diag318;
+ dc->hotpluggable = false;
+ /* Reason: Created automatically during machine instantiation */
+ dc->user_creatable = false;
+}
+
+static const TypeInfo s390_diag318_info = {
+ .class_init = s390_diag318_class_init,
+ .parent = TYPE_DEVICE,
+ .name = TYPE_S390_DIAG318,
+ .instance_size = sizeof(DIAG318State),
+};
+
+static void s390_diag318_register_types(void)
+{
+ type_register_static(&s390_diag318_info);
+}
+
+type_init(s390_diag318_register_types)
diff --git a/hw/s390x/diag318.h b/hw/s390x/diag318.h
new file mode 100644
index 0000000..06d9f67
--- /dev/null
+++ b/hw/s390x/diag318.h
@@ -0,0 +1,40 @@
+/*
+ * DIAGNOSE 0x318 functions for reset and migration
+ *
+ * Copyright IBM, Corp. 2019
+ *
+ * Authors:
+ * Collin Walling <walling@linux.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 HW_DIAG318_H
+#define HW_DIAG318_H
+
+#include "qemu/osdep.h"
+#include "migration/vmstate.h"
+#include "qom/object.h"
+#include "hw/qdev-core.h"
+
+#define TYPE_S390_DIAG318 "diag318"
+#define DIAG318(obj) \
+ OBJECT_CHECK(DIAG318State, (obj), TYPE_S390_DIAG318)
+
+typedef struct DIAG318State {
+ /*< private >*/
+ DeviceState parent_obj;
+
+ /*< public >*/
+ uint64_t info;
+} DIAG318State;
+
+typedef struct DIAG318Class {
+ /*< private >*/
+ DeviceClass parent_class;
+
+ /*< public >*/
+} DIAG318Class;
+
+#endif /* HW_DIAG318_H */
\ No newline at end of file
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index e0e2813..d5b7a33 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -41,6 +41,7 @@
#include "hw/qdev-properties.h"
#include "hw/s390x/tod.h"
#include "sysemu/sysemu.h"
+#include "hw/s390x/diag318.h"
S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
{
@@ -97,6 +98,7 @@ static const char *const reset_dev_types[] = {
"s390-sclp-event-facility",
"s390-flic",
"diag288",
+ TYPE_S390_DIAG318,
};
static void subsystem_reset(void)
@@ -237,6 +239,17 @@ static void s390_create_sclpconsole(const char *type, Chardev *chardev)
qdev_init_nofail(dev);
}
+static void s390_init_diag318(void)
+{
+ Object *new = object_new(TYPE_S390_DIAG318);
+ DeviceState *dev = DEVICE(new);
+
+ object_property_add_child(qdev_get_machine(), TYPE_S390_DIAG318,
+ new, NULL);
+ object_unref(new);
+ qdev_init_nofail(dev);
+}
+
static void ccw_init(MachineState *machine)
{
int ret;
@@ -294,6 +307,9 @@ static void ccw_init(MachineState *machine)
/* init the TOD clock */
s390_init_tod();
+
+ /* init object used for migrating diag318 info */
+ s390_init_diag318();
}
static void s390_cpu_plug(HotplugHandler *hotplug_dev,
@@ -566,6 +582,7 @@ static void machine_set_loadparm(Object *obj, const char *val, Error **errp)
ms->loadparm[i] = ' '; /* pad right with spaces */
}
}
+
static inline void s390_machine_initfn(Object *obj)
{
object_property_add_bool(obj, "aes-key-wrap",
diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
index f57ce7b..636348c 100644
--- a/hw/s390x/sclp.c
+++ b/hw/s390x/sclp.c
@@ -15,6 +15,7 @@
#include "qemu/osdep.h"
#include "qemu/units.h"
#include "qapi/error.h"
+#include "qemu/error-report.h"
#include "cpu.h"
#include "sysemu/sysemu.h"
#include "hw/boards.h"
@@ -22,6 +23,7 @@
#include "hw/s390x/event-facility.h"
#include "hw/s390x/s390-pci-bus.h"
#include "hw/s390x/ipl.h"
+#include "kvm_s390x.h"
static inline SCLPDevice *get_sclp_device(void)
{
@@ -37,10 +39,19 @@ static void prepare_cpu_entries(SCLPDevice *sclp, CPUEntry *entry, int *count)
{
MachineState *ms = MACHINE(qdev_get_machine());
uint8_t features[SCCB_CPU_FEATURE_LEN] = { 0 };
+ int max_entries;
int i;
+ /* Calculate the max number of CPU entries that can be stored in the SCCB */
+ max_entries = (SCCB_SIZE - offsetof(ReadInfo, entries)) / sizeof(CPUEntry);
+
s390_get_feat_block(S390_FEAT_TYPE_SCLP_CPU, features);
for (i = 0, *count = 0; i < ms->possible_cpus->len; i++) {
+ if (*count == max_entries) {
+ warn_report("Configuration only supports a max of %d CPU entries.",
+ max_entries);
+ break;
+ }
if (!ms->possible_cpus->cpus[i].cpu) {
continue;
}
@@ -80,6 +91,8 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
s390_get_feat_block(S390_FEAT_TYPE_SCLP_CONF_CHAR_EXT,
read_info->conf_char_ext);
+ s390_get_feat_block(S390_FEAT_TYPE_SCLP_BYTE_134, read_info->byte_134);
+
read_info->facilities = cpu_to_be64(SCLP_HAS_CPU_INFO |
SCLP_HAS_IOA_RECONFIG);
diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
index c54413b..08813b4 100644
--- a/include/hw/s390x/sclp.h
+++ b/include/hw/s390x/sclp.h
@@ -132,6 +132,8 @@ typedef struct ReadInfo {
uint16_t highest_cpu;
uint8_t _reserved5[124 - 122]; /* 122-123 */
uint32_t hmfai;
+ uint8_t _reserved7[134 - 128]; /* 128-133 */
+ uint8_t byte_134[1];
struct CPUEntry entries[0];
} QEMU_PACKED ReadInfo;
diff --git a/target/s390x/cpu_features.h b/target/s390x/cpu_features.h
index da695a8..954544e 100644
--- a/target/s390x/cpu_features.h
+++ b/target/s390x/cpu_features.h
@@ -23,6 +23,7 @@ typedef enum {
S390_FEAT_TYPE_STFL,
S390_FEAT_TYPE_SCLP_CONF_CHAR,
S390_FEAT_TYPE_SCLP_CONF_CHAR_EXT,
+ S390_FEAT_TYPE_SCLP_BYTE_134,
S390_FEAT_TYPE_SCLP_CPU,
S390_FEAT_TYPE_MISC,
S390_FEAT_TYPE_PLO,
diff --git a/target/s390x/cpu_features_def.inc.h b/target/s390x/cpu_features_def.inc.h
index 31dff0d..13d8da5 100644
--- a/target/s390x/cpu_features_def.inc.h
+++ b/target/s390x/cpu_features_def.inc.h
@@ -120,6 +120,9 @@ DEF_FEAT(SIE_CMMA, "cmma", SCLP_CONF_CHAR_EXT, 1, "SIE: Collaborative-memory-man
DEF_FEAT(SIE_PFMFI, "pfmfi", SCLP_CONF_CHAR_EXT, 9, "SIE: PFMF interpretation facility")
DEF_FEAT(SIE_IBS, "ibs", SCLP_CONF_CHAR_EXT, 10, "SIE: Interlock-and-broadcast-suppression facility")
+/* Features exposed via SCLP SCCB Byte 134 (bit numbers relative to byte-134) */
+DEF_FEAT(DIAG318, "diag318", SCLP_BYTE_134, 0, "Control program name and version codes")
+
/* Features exposed via SCLP CPU info. */
DEF_FEAT(SIE_F2, "sief2", SCLP_CPU, 4, "SIE: interception format 2 (Virtual SIE)")
DEF_FEAT(SIE_SKEY, "skey", SCLP_CPU, 5, "SIE: Storage-key facility")
diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c
index 6278845..eaf91e6 100644
--- a/target/s390x/gen-features.c
+++ b/target/s390x/gen-features.c
@@ -522,6 +522,7 @@ static uint16_t full_GEN12_GA1[] = {
S390_FEAT_AP_QUEUE_INTERRUPT_CONTROL,
S390_FEAT_AP_FACILITIES_TEST,
S390_FEAT_AP,
+ S390_FEAT_DIAG318,
};
static uint16_t full_GEN12_GA2[] = {
diff --git a/target/s390x/kvm-stub.c b/target/s390x/kvm-stub.c
index 5152e2b..7c39d6a 100644
--- a/target/s390x/kvm-stub.c
+++ b/target/s390x/kvm-stub.c
@@ -107,3 +107,13 @@ void kvm_s390_stop_interrupt(S390CPU *cpu)
void kvm_s390_restart_interrupt(S390CPU *cpu)
{
}
+
+int kvm_s390_get_diag318_info(uint64_t *info)
+{
+ return 0;
+}
+
+int kvm_s390_set_diag318_info(uint64_t info)
+{
+ return 0;
+}
diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
index 15260ae..abc9dc2 100644
--- a/target/s390x/kvm.c
+++ b/target/s390x/kvm.c
@@ -775,6 +775,28 @@ int kvm_s390_set_clock_ext(uint8_t tod_high, uint64_t tod_low)
return kvm_vm_ioctl(kvm_state, KVM_SET_DEVICE_ATTR, &attr);
}
+int kvm_s390_get_diag318_info(uint64_t *info)
+{
+ struct kvm_device_attr attr = {
+ .group = KVM_S390_VM_MISC,
+ .attr = KVM_S390_VM_MISC_DIAG318,
+ .addr = (uint64_t)info,
+ };
+
+ return kvm_vm_ioctl(kvm_state, KVM_GET_DEVICE_ATTR, &attr);
+}
+
+int kvm_s390_set_diag318_info(uint64_t info)
+{
+ struct kvm_device_attr attr = {
+ .group = KVM_S390_VM_MISC,
+ .attr = KVM_S390_VM_MISC_DIAG318,
+ .addr = (uint64_t)&info,
+ };
+
+ return kvm_vm_ioctl(kvm_state, KVM_SET_DEVICE_ATTR, &attr);
+}
+
/**
* kvm_s390_mem_op:
* @addr: the logical start address in guest memory
@@ -2347,6 +2369,13 @@ void kvm_s390_get_host_cpu_model(S390CPUModel *model, Error **errp)
KVM_S390_VM_CRYPTO_ENABLE_APIE)) {
set_bit(S390_FEAT_AP, model->features);
}
+
+ /* if KVM supports interception of diag318, then let's provide the bit */
+ if (kvm_vm_check_attr(kvm_state, KVM_S390_VM_MISC,
+ KVM_S390_VM_MISC_DIAG318)) {
+ set_bit(S390_FEAT_DIAG318, model->features);
+ }
+
/* strip of features that are not part of the maximum model */
bitmap_and(model->features, model->features, model->def->full_feat,
S390_FEAT_MAX);
diff --git a/target/s390x/kvm_s390x.h b/target/s390x/kvm_s390x.h
index caf9859..50df93e 100644
--- a/target/s390x/kvm_s390x.h
+++ b/target/s390x/kvm_s390x.h
@@ -29,6 +29,8 @@ int kvm_s390_get_clock(uint8_t *tod_high, uint64_t *tod_clock);
int kvm_s390_get_clock_ext(uint8_t *tod_high, uint64_t *tod_clock);
int kvm_s390_set_clock(uint8_t tod_high, uint64_t tod_clock);
int kvm_s390_set_clock_ext(uint8_t tod_high, uint64_t tod_clock);
+int kvm_s390_get_diag318_info(uint64_t *info);
+int kvm_s390_set_diag318_info(uint64_t info);
void kvm_s390_enable_css_support(S390CPU *cpu);
int kvm_s390_assign_subch_ioeventfd(EventNotifier *notifier, uint32_t sch,
int vq, bool assign);
--
2.7.4
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v6 0/2] Use DIAG318 to set Control Program Name & Version Codes
2020-01-24 22:14 [PATCH v6 0/2] Use DIAG318 to set Control Program Name & Version Codes Collin Walling
2020-01-24 22:14 ` [PATCH v6 1/2] s390/kvm: header sync for diag318 Collin Walling
2020-01-24 22:14 ` [PATCH v6 2/2] s390: diagnose 318 info reset and migration support Collin Walling
@ 2020-01-24 22:22 ` no-reply
2020-03-17 21:34 ` Collin Walling
3 siblings, 0 replies; 23+ messages in thread
From: no-reply @ 2020-01-24 22:22 UTC (permalink / raw)
To: walling; +Cc: david, cohuck, qemu-devel, borntraeger, qemu-s390x, rth
Patchew URL: https://patchew.org/QEMU/1579904044-20790-1-git-send-email-walling@linux.ibm.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 1579904044-20790-1-git-send-email-walling@linux.ibm.com
Subject: [PATCH v6 0/2] Use DIAG318 to set Control Program Name & Version Codes
=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
* [new tag] patchew/1579904044-20790-1-git-send-email-walling@linux.ibm.com -> patchew/1579904044-20790-1-git-send-email-walling@linux.ibm.com
Switched to a new branch 'test'
bf39c82 s390: diagnose 318 info reset and migration support
7a5ff34 s390/kvm: header sync for diag318
=== OUTPUT BEGIN ===
1/2 Checking commit 7a5ff34d93c1 (s390/kvm: header sync for diag318)
2/2 Checking commit bf39c8211f83 (s390: diagnose 318 info reset and migration support)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#38:
new file mode 100644
ERROR: braces {} are necessary for all arms of this statement
#64: FILE: hw/s390x/diag318.c:22:
+ if (kvm_enabled())
[...]
ERROR: braces {} are necessary for all arms of this statement
#74: FILE: hw/s390x/diag318.c:32:
+ if (kvm_enabled())
[...]
ERROR: braces {} are necessary for all arms of this statement
#100: FILE: hw/s390x/diag318.c:58:
+ if (kvm_enabled())
[...]
ERROR: adding a line without newline at end of file
#173: FILE: hw/s390x/diag318.h:40:
+#endif /* HW_DIAG318_H */
WARNING: line over 80 characters
#314: FILE: target/s390x/cpu_features_def.inc.h:124:
+DEF_FEAT(DIAG318, "diag318", SCLP_BYTE_134, 0, "Control program name and version codes")
total: 4 errors, 2 warnings, 310 lines checked
Patch 2/2 has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/1579904044-20790-1-git-send-email-walling@linux.ibm.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 2/2] s390: diagnose 318 info reset and migration support
2020-01-24 22:14 ` [PATCH v6 2/2] s390: diagnose 318 info reset and migration support Collin Walling
@ 2020-01-27 11:20 ` David Hildenbrand
2020-01-27 15:57 ` Collin Walling
2020-01-27 11:36 ` Thomas Huth
2020-01-27 11:47 ` Cornelia Huck
2 siblings, 1 reply; 23+ messages in thread
From: David Hildenbrand @ 2020-01-27 11:20 UTC (permalink / raw)
To: Collin Walling, qemu-s390x, qemu-devel; +Cc: borntraeger, cohuck, rth
[...]
> @@ -0,0 +1,85 @@
> +/*
> + * DIAGNOSE 0x318 functions for reset and migration
> + *
> + * Copyright IBM, Corp. 2019
Should be 2020 now.
[...]
> +static void s390_diag318_reset(DeviceState *dev)
> +{
> + if (kvm_enabled())
> + kvm_s390_set_diag318_info(0);
> +}
> +
> +static void s390_diag318_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> +
> + dc->reset = s390_diag318_reset;
> + dc->vmsd = &vmstate_diag318;
> + dc->hotpluggable = false;
> + /* Reason: Created automatically during machine instantiation */
> + dc->user_creatable = false;
> +}
> +
> +static const TypeInfo s390_diag318_info = {
> + .class_init = s390_diag318_class_init,
> + .parent = TYPE_DEVICE,
> + .name = TYPE_S390_DIAG318,
> + .instance_size = sizeof(DIAG318State),
> +};
> +
> +static void s390_diag318_register_types(void)
> +{
> + type_register_static(&s390_diag318_info);
> +}
Do we really need a new device? Can't we simply glue that extended state
to the machine state?
-> target/s390x/machine.c
> +
> +type_init(s390_diag318_register_types)
> diff --git a/hw/s390x/diag318.h b/hw/s390x/diag318.h
> new file mode 100644
> index 0000000..06d9f67
> --- /dev/null
> +++ b/hw/s390x/diag318.h
> @@ -0,0 +1,40 @@
> +/*
> + * DIAGNOSE 0x318 functions for reset and migration
> + *
> + * Copyright IBM, Corp. 2019
> + *
> + * Authors:
> + * Collin Walling <walling@linux.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 HW_DIAG318_H
> +#define HW_DIAG318_H
> +
> +#include "qemu/osdep.h"
> +#include "migration/vmstate.h"
> +#include "qom/object.h"
> +#include "hw/qdev-core.h"
> +
> +#define TYPE_S390_DIAG318 "diag318"
> +#define DIAG318(obj) \
> + OBJECT_CHECK(DIAG318State, (obj), TYPE_S390_DIAG318)
> +
> +typedef struct DIAG318State {
> + /*< private >*/
> + DeviceState parent_obj;
> +
> + /*< public >*/
> + uint64_t info;
> +} DIAG318State;
> +
> +typedef struct DIAG318Class {
> + /*< private >*/
> + DeviceClass parent_class;
> +
> + /*< public >*/
> +} DIAG318Class;
> +
> +#endif /* HW_DIAG318_H */
> \ No newline at end of file
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index e0e2813..d5b7a33 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -41,6 +41,7 @@
> #include "hw/qdev-properties.h"
> #include "hw/s390x/tod.h"
> #include "sysemu/sysemu.h"
> +#include "hw/s390x/diag318.h"
>
> S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
> {
> @@ -97,6 +98,7 @@ static const char *const reset_dev_types[] = {
> "s390-sclp-event-facility",
> "s390-flic",
> "diag288",
> + TYPE_S390_DIAG318,
> };
>
> static void subsystem_reset(void)
> @@ -237,6 +239,17 @@ static void s390_create_sclpconsole(const char *type, Chardev *chardev)
> qdev_init_nofail(dev);
> }
>
> +static void s390_init_diag318(void)
> +{
> + Object *new = object_new(TYPE_S390_DIAG318);
> + DeviceState *dev = DEVICE(new);
> +
> + object_property_add_child(qdev_get_machine(), TYPE_S390_DIAG318,
> + new, NULL);
> + object_unref(new);
> + qdev_init_nofail(dev);
> +}
> +
> static void ccw_init(MachineState *machine)
> {
> int ret;
> @@ -294,6 +307,9 @@ static void ccw_init(MachineState *machine)
>
> /* init the TOD clock */
> s390_init_tod();
> +
> + /* init object used for migrating diag318 info */
> + s390_init_diag318();
> }
>
> static void s390_cpu_plug(HotplugHandler *hotplug_dev,
> @@ -566,6 +582,7 @@ static void machine_set_loadparm(Object *obj, const char *val, Error **errp)
> ms->loadparm[i] = ' '; /* pad right with spaces */
> }
> }
> +
unrelated change.
> static inline void s390_machine_initfn(Object *obj)
> {
> object_property_add_bool(obj, "aes-key-wrap",
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index f57ce7b..636348c 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -15,6 +15,7 @@
> #include "qemu/osdep.h"
> #include "qemu/units.h"
> #include "qapi/error.h"
> +#include "qemu/error-report.h"
> #include "cpu.h"
> #include "sysemu/sysemu.h"
> #include "hw/boards.h"
> @@ -22,6 +23,7 @@
> #include "hw/s390x/event-facility.h"
> #include "hw/s390x/s390-pci-bus.h"
> #include "hw/s390x/ipl.h"
> +#include "kvm_s390x.h"
>
> static inline SCLPDevice *get_sclp_device(void)
> {
> @@ -37,10 +39,19 @@ static void prepare_cpu_entries(SCLPDevice *sclp, CPUEntry *entry, int *count)
> {
> MachineState *ms = MACHINE(qdev_get_machine());
> uint8_t features[SCCB_CPU_FEATURE_LEN] = { 0 };
> + int max_entries;
> int i;
>
> + /* Calculate the max number of CPU entries that can be stored in the SCCB */
> + max_entries = (SCCB_SIZE - offsetof(ReadInfo, entries)) / sizeof(CPUEntry);
> +
> s390_get_feat_block(S390_FEAT_TYPE_SCLP_CPU, features);
> for (i = 0, *count = 0; i < ms->possible_cpus->len; i++) {
> + if (*count == max_entries) {
> + warn_report("Configuration only supports a max of %d CPU entries.",
> + max_entries);
I remember that "the sclp response will be limited to 247 CPUs if the
feature is one". So we should have a double layout and make max_entries
depending on s390_has_feat().
Regarding the message, I'd probably do a "Due to the current CPU model,
some CPUs might be hidden from the VM (SCLP)."
A VM could still manually probe for others.
Thanks!
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 2/2] s390: diagnose 318 info reset and migration support
2020-01-24 22:14 ` [PATCH v6 2/2] s390: diagnose 318 info reset and migration support Collin Walling
2020-01-27 11:20 ` David Hildenbrand
@ 2020-01-27 11:36 ` Thomas Huth
2020-01-27 15:58 ` Collin Walling
2020-01-27 11:47 ` Cornelia Huck
2 siblings, 1 reply; 23+ messages in thread
From: Thomas Huth @ 2020-01-27 11:36 UTC (permalink / raw)
To: Collin Walling, qemu-s390x, qemu-devel; +Cc: borntraeger, rth, cohuck, david
On 24/01/2020 23.14, Collin Walling wrote:
> DIAGNOSE 0x318 (diag318) is a privileged s390x instruction that must
> be intercepted by SIE and handled via KVM. Let's introduce some
> functions to communicate between QEMU and KVM via ioctls. These
> will be used to get/set the diag318 information.
>
> The availability of this instruction is determined by byte 134, bit 0
> of the Read Info block. This coincidentally expands into the space used
> for CPU entries by taking away one byte, which means VMs running with
> the diag318 capability will not be able to retrieve information regarding
> the 248th CPU. This will not effect performance, and VMs can still be
s/effect/affect/ ?
> ran with 248 CPUs.
s/ran/run/ ?
> In order to simplify the migration and system reset requirements of
> the diag318 data, let's introduce it as a device class and include
> a VMStateDescription.
>
> Diag318 is set to 0 during modified clear and load normal resets.
>
> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> ---
> hw/s390x/Makefile.objs | 1 +
> hw/s390x/diag318.c | 85 +++++++++++++++++++++++++++++++++++++
> hw/s390x/diag318.h | 40 +++++++++++++++++
> hw/s390x/s390-virtio-ccw.c | 17 ++++++++
> hw/s390x/sclp.c | 13 ++++++
> include/hw/s390x/sclp.h | 2 +
> target/s390x/cpu_features.h | 1 +
> target/s390x/cpu_features_def.inc.h | 3 ++
> target/s390x/gen-features.c | 1 +
> target/s390x/kvm-stub.c | 10 +++++
> target/s390x/kvm.c | 29 +++++++++++++
> target/s390x/kvm_s390x.h | 2 +
> 12 files changed, 204 insertions(+)
> create mode 100644 hw/s390x/diag318.c
> create mode 100644 hw/s390x/diag318.h
>
> diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
> index e02ed80..93621dc 100644
> --- a/hw/s390x/Makefile.objs
> +++ b/hw/s390x/Makefile.objs
> @@ -34,3 +34,4 @@ obj-$(CONFIG_KVM) += s390-stattrib-kvm.o
> obj-y += s390-ccw.o
> obj-y += ap-device.o
> obj-y += ap-bridge.o
> +obj-y += diag318.o
> diff --git a/hw/s390x/diag318.c b/hw/s390x/diag318.c
> new file mode 100644
> index 0000000..2d30bb2
> --- /dev/null
> +++ b/hw/s390x/diag318.c
> @@ -0,0 +1,85 @@
> +/*
> + * DIAGNOSE 0x318 functions for reset and migration
> + *
> + * Copyright IBM, Corp. 2019
Bump to 2020 ?
> + * Authors:
> + * Collin Walling <walling@linux.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/diag318.h"
> +#include "qapi/error.h"
> +#include "kvm_s390x.h"
> +#include "sysemu/kvm.h"
> +
> +static int diag318_post_load(void *opaque, int version_id)
> +{
> + DIAG318State *d = opaque;
> +
> + if (kvm_enabled())
> + kvm_s390_set_diag318_info(d->info);
QEMU coding style requires curly braces also for single lines.
> + return 0;
> +}
> +
> +static int diag318_pre_save(void *opaque)
> +{
> + DIAG318State *d = opaque;
> +
> + if (kvm_enabled())
> + kvm_s390_get_diag318_info(&d->info);
dito
> + return 0;
> +}
> +
> +static bool diag318_needed(void *opaque)
> +{
> + return kvm_enabled() ? s390_has_feat(S390_FEAT_DIAG318) : 0;
> +}
> +
> +const VMStateDescription vmstate_diag318 = {
> + .name = "vmstate_diag318",
> + .post_load = diag318_post_load,
> + .pre_save = diag318_pre_save,
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .needed = diag318_needed,
> + .fields = (VMStateField[]) {
> + VMSTATE_UINT64(info, DIAG318State),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> +static void s390_diag318_reset(DeviceState *dev)
> +{
> + if (kvm_enabled())
> + kvm_s390_set_diag318_info(0);
dito
> +}
> +
> +static void s390_diag318_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> +
> + dc->reset = s390_diag318_reset;
> + dc->vmsd = &vmstate_diag318;
> + dc->hotpluggable = false;
> + /* Reason: Created automatically during machine instantiation */
> + dc->user_creatable = false;
> +}
> +
> +static const TypeInfo s390_diag318_info = {
> + .class_init = s390_diag318_class_init,
> + .parent = TYPE_DEVICE,
> + .name = TYPE_S390_DIAG318,
> + .instance_size = sizeof(DIAG318State),
> +};
> +
> +static void s390_diag318_register_types(void)
> +{
> + type_register_static(&s390_diag318_info);
> +}
> +
> +type_init(s390_diag318_register_types)
> diff --git a/hw/s390x/diag318.h b/hw/s390x/diag318.h
> new file mode 100644
> index 0000000..06d9f67
> --- /dev/null
> +++ b/hw/s390x/diag318.h
> @@ -0,0 +1,40 @@
> +/*
> + * DIAGNOSE 0x318 functions for reset and migration
> + *
> + * Copyright IBM, Corp. 2019
2020 ?
> + * Authors:
> + * Collin Walling <walling@linux.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 HW_DIAG318_H
> +#define HW_DIAG318_H
> +
> +#include "qemu/osdep.h"
> +#include "migration/vmstate.h"
> +#include "qom/object.h"
> +#include "hw/qdev-core.h"
> +
> +#define TYPE_S390_DIAG318 "diag318"
> +#define DIAG318(obj) \
> + OBJECT_CHECK(DIAG318State, (obj), TYPE_S390_DIAG318)
> +
> +typedef struct DIAG318State {
> + /*< private >*/
> + DeviceState parent_obj;
> +
> + /*< public >*/
> + uint64_t info;
> +} DIAG318State;
> +
> +typedef struct DIAG318Class {
> + /*< private >*/
> + DeviceClass parent_class;
> +
> + /*< public >*/
> +} DIAG318Class;
You don't use DIAG318Class anywhere. Please drop it.
> +#endif /* HW_DIAG318_H */
> \ No newline at end of file
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index e0e2813..d5b7a33 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -41,6 +41,7 @@
> #include "hw/qdev-properties.h"
> #include "hw/s390x/tod.h"
> #include "sysemu/sysemu.h"
> +#include "hw/s390x/diag318.h"
>
> S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
> {
> @@ -97,6 +98,7 @@ static const char *const reset_dev_types[] = {
> "s390-sclp-event-facility",
> "s390-flic",
> "diag288",
> + TYPE_S390_DIAG318,
> };
>
> static void subsystem_reset(void)
> @@ -237,6 +239,17 @@ static void s390_create_sclpconsole(const char *type, Chardev *chardev)
> qdev_init_nofail(dev);
> }
>
> +static void s390_init_diag318(void)
> +{
> + Object *new = object_new(TYPE_S390_DIAG318);
For the very unlikely case that we ever switch QEMU to C++ ... could you
maybe use a different variable name than "new" ? Simply "obj" maybe?
> + DeviceState *dev = DEVICE(new);
> +
> + object_property_add_child(qdev_get_machine(), TYPE_S390_DIAG318,
> + new, NULL);
> + object_unref(new);
> + qdev_init_nofail(dev);
> +}
> +
Thomas
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 2/2] s390: diagnose 318 info reset and migration support
2020-01-24 22:14 ` [PATCH v6 2/2] s390: diagnose 318 info reset and migration support Collin Walling
2020-01-27 11:20 ` David Hildenbrand
2020-01-27 11:36 ` Thomas Huth
@ 2020-01-27 11:47 ` Cornelia Huck
2020-01-27 16:39 ` Collin Walling
2 siblings, 1 reply; 23+ messages in thread
From: Cornelia Huck @ 2020-01-27 11:47 UTC (permalink / raw)
To: Collin Walling; +Cc: borntraeger, qemu-s390x, david, qemu-devel, rth
On Fri, 24 Jan 2020 17:14:04 -0500
Collin Walling <walling@linux.ibm.com> wrote:
> DIAGNOSE 0x318 (diag318) is a privileged s390x instruction that must
> be intercepted by SIE and handled via KVM. Let's introduce some
> functions to communicate between QEMU and KVM via ioctls. These
> will be used to get/set the diag318 information.
Do you want to give a hint what diag 318 actually does?
>
> The availability of this instruction is determined by byte 134, bit 0
> of the Read Info block. This coincidentally expands into the space used
"SCLP Read Info"
> for CPU entries by taking away one byte, which means VMs running with
> the diag318 capability will not be able to retrieve information regarding
> the 248th CPU. This will not effect performance, and VMs can still be
> ran with 248 CPUs.
Are there other ways in which that might affect guests? I assume Linux
can deal with it? Is it ok architecture-wise?
In any case, should go into the patch description :)
>
> In order to simplify the migration and system reset requirements of
> the diag318 data, let's introduce it as a device class and include
> a VMStateDescription.
>
> Diag318 is set to 0 during modified clear and load normal resets.
What exactly is set to 0? Stored values?
>
> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> ---
> hw/s390x/Makefile.objs | 1 +
> hw/s390x/diag318.c | 85 +++++++++++++++++++++++++++++++++++++
> hw/s390x/diag318.h | 40 +++++++++++++++++
> hw/s390x/s390-virtio-ccw.c | 17 ++++++++
> hw/s390x/sclp.c | 13 ++++++
> include/hw/s390x/sclp.h | 2 +
> target/s390x/cpu_features.h | 1 +
> target/s390x/cpu_features_def.inc.h | 3 ++
> target/s390x/gen-features.c | 1 +
> target/s390x/kvm-stub.c | 10 +++++
> target/s390x/kvm.c | 29 +++++++++++++
> target/s390x/kvm_s390x.h | 2 +
> 12 files changed, 204 insertions(+)
> create mode 100644 hw/s390x/diag318.c
> create mode 100644 hw/s390x/diag318.h
>
(...)
> diff --git a/hw/s390x/diag318.c b/hw/s390x/diag318.c
> new file mode 100644
> index 0000000..2d30bb2
> --- /dev/null
> +++ b/hw/s390x/diag318.c
> @@ -0,0 +1,85 @@
> +/*
> + * DIAGNOSE 0x318 functions for reset and migration
> + *
> + * Copyright IBM, Corp. 2019
Happy new year?
> + *
> + * Authors:
> + * Collin Walling <walling@linux.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/diag318.h"
> +#include "qapi/error.h"
> +#include "kvm_s390x.h"
> +#include "sysemu/kvm.h"
> +
> +static int diag318_post_load(void *opaque, int version_id)
> +{
> + DIAG318State *d = opaque;
> +
> + if (kvm_enabled())
As already noted by patchew, this needs some curly braces.
> + kvm_s390_set_diag318_info(d->info);
> +
> + return 0;
> +}
> +
> +static int diag318_pre_save(void *opaque)
> +{
> + DIAG318State *d = opaque;
> +
> + if (kvm_enabled())
braces
> + kvm_s390_get_diag318_info(&d->info);
> +
> + return 0;
> +}
> +
> +static bool diag318_needed(void *opaque)
> +{
> + return kvm_enabled() ? s390_has_feat(S390_FEAT_DIAG318) : 0;
Why do you need to guard this with kvm_enabled()? If tcg does not
enable the feature, we should be fine; and if it emulates this in the
future, we probably need to migrate something anyway.
> +}
> +
> +const VMStateDescription vmstate_diag318 = {
> + .name = "vmstate_diag318",
> + .post_load = diag318_post_load,
> + .pre_save = diag318_pre_save,
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .needed = diag318_needed,
> + .fields = (VMStateField[]) {
> + VMSTATE_UINT64(info, DIAG318State),
> + VMSTATE_END_OF_LIST()
> + }
> +};
> +
> +static void s390_diag318_reset(DeviceState *dev)
> +{
> + if (kvm_enabled())
braces
> + kvm_s390_set_diag318_info(0);
> +}
> +
> +static void s390_diag318_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> +
> + dc->reset = s390_diag318_reset;
> + dc->vmsd = &vmstate_diag318;
> + dc->hotpluggable = false;
> + /* Reason: Created automatically during machine instantiation */
> + dc->user_creatable = false;
> +}
> +
> +static const TypeInfo s390_diag318_info = {
> + .class_init = s390_diag318_class_init,
> + .parent = TYPE_DEVICE,
> + .name = TYPE_S390_DIAG318,
> + .instance_size = sizeof(DIAG318State),
> +};
> +
> +static void s390_diag318_register_types(void)
> +{
> + type_register_static(&s390_diag318_info);
> +}
> +
> +type_init(s390_diag318_register_types)
> diff --git a/hw/s390x/diag318.h b/hw/s390x/diag318.h
> new file mode 100644
> index 0000000..06d9f67
> --- /dev/null
> +++ b/hw/s390x/diag318.h
> @@ -0,0 +1,40 @@
> +/*
> + * DIAGNOSE 0x318 functions for reset and migration
> + *
> + * Copyright IBM, Corp. 2019
Again, the year needs an update.
> + *
> + * Authors:
> + * Collin Walling <walling@linux.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 HW_DIAG318_H
> +#define HW_DIAG318_H
> +
> +#include "qemu/osdep.h"
> +#include "migration/vmstate.h"
> +#include "qom/object.h"
> +#include "hw/qdev-core.h"
> +
> +#define TYPE_S390_DIAG318 "diag318"
> +#define DIAG318(obj) \
> + OBJECT_CHECK(DIAG318State, (obj), TYPE_S390_DIAG318)
> +
> +typedef struct DIAG318State {
> + /*< private >*/
> + DeviceState parent_obj;
> +
> + /*< public >*/
> + uint64_t info;
> +} DIAG318State;
> +
> +typedef struct DIAG318Class {
> + /*< private >*/
> + DeviceClass parent_class;
> +
> + /*< public >*/
> +} DIAG318Class;
> +
> +#endif /* HW_DIAG318_H */
> \ No newline at end of file
And please add a newline :)
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index e0e2813..d5b7a33 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -41,6 +41,7 @@
> #include "hw/qdev-properties.h"
> #include "hw/s390x/tod.h"
> #include "sysemu/sysemu.h"
> +#include "hw/s390x/diag318.h"
>
> S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
> {
> @@ -97,6 +98,7 @@ static const char *const reset_dev_types[] = {
> "s390-sclp-event-facility",
> "s390-flic",
> "diag288",
> + TYPE_S390_DIAG318,
> };
>
> static void subsystem_reset(void)
> @@ -237,6 +239,17 @@ static void s390_create_sclpconsole(const char *type, Chardev *chardev)
> qdev_init_nofail(dev);
> }
>
> +static void s390_init_diag318(void)
> +{
> + Object *new = object_new(TYPE_S390_DIAG318);
> + DeviceState *dev = DEVICE(new);
> +
> + object_property_add_child(qdev_get_machine(), TYPE_S390_DIAG318,
> + new, NULL);
> + object_unref(new);
> + qdev_init_nofail(dev);
> +}
> +
> static void ccw_init(MachineState *machine)
> {
> int ret;
> @@ -294,6 +307,9 @@ static void ccw_init(MachineState *machine)
>
> /* init the TOD clock */
> s390_init_tod();
> +
> + /* init object used for migrating diag318 info */
> + s390_init_diag318();
Doesn't that device do a bit more than 'migrating' info?
Also, it seems a bit useless unless you're running with kvm and the
feature bit switched on...
> }
>
> static void s390_cpu_plug(HotplugHandler *hotplug_dev,
> @@ -566,6 +582,7 @@ static void machine_set_loadparm(Object *obj, const char *val, Error **errp)
> ms->loadparm[i] = ' '; /* pad right with spaces */
> }
> }
> +
Still whitespace :)
> static inline void s390_machine_initfn(Object *obj)
> {
> object_property_add_bool(obj, "aes-key-wrap",
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index f57ce7b..636348c 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -15,6 +15,7 @@
> #include "qemu/osdep.h"
> #include "qemu/units.h"
> #include "qapi/error.h"
> +#include "qemu/error-report.h"
> #include "cpu.h"
> #include "sysemu/sysemu.h"
> #include "hw/boards.h"
> @@ -22,6 +23,7 @@
> #include "hw/s390x/event-facility.h"
> #include "hw/s390x/s390-pci-bus.h"
> #include "hw/s390x/ipl.h"
> +#include "kvm_s390x.h"
>
> static inline SCLPDevice *get_sclp_device(void)
> {
> @@ -37,10 +39,19 @@ static void prepare_cpu_entries(SCLPDevice *sclp, CPUEntry *entry, int *count)
> {
> MachineState *ms = MACHINE(qdev_get_machine());
> uint8_t features[SCCB_CPU_FEATURE_LEN] = { 0 };
> + int max_entries;
> int i;
>
> + /* Calculate the max number of CPU entries that can be stored in the SCCB */
> + max_entries = (SCCB_SIZE - offsetof(ReadInfo, entries)) / sizeof(CPUEntry);
> +
> s390_get_feat_block(S390_FEAT_TYPE_SCLP_CPU, features);
> for (i = 0, *count = 0; i < ms->possible_cpus->len; i++) {
> + if (*count == max_entries) {
> + warn_report("Configuration only supports a max of %d CPU entries.",
> + max_entries);
IIUC, this only moans during Read Info... but you could previously add
more cpus than what could be serviced by Read Info. So, it looks to me
you get some messages when a guest is doing Read Info; that seems more
confusing than helpful to me. Can't we rather warn at cpu instantiation
time?
> + break;
> + }
> if (!ms->possible_cpus->cpus[i].cpu) {
> continue;
> }
> @@ -80,6 +91,8 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
> s390_get_feat_block(S390_FEAT_TYPE_SCLP_CONF_CHAR_EXT,
> read_info->conf_char_ext);
>
> + s390_get_feat_block(S390_FEAT_TYPE_SCLP_BYTE_134, read_info->byte_134);
> +
> read_info->facilities = cpu_to_be64(SCLP_HAS_CPU_INFO |
> SCLP_HAS_IOA_RECONFIG);
>
(...)
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 2/2] s390: diagnose 318 info reset and migration support
2020-01-27 11:20 ` David Hildenbrand
@ 2020-01-27 15:57 ` Collin Walling
2020-01-27 17:09 ` David Hildenbrand
0 siblings, 1 reply; 23+ messages in thread
From: Collin Walling @ 2020-01-27 15:57 UTC (permalink / raw)
To: David Hildenbrand, qemu-s390x, qemu-devel; +Cc: borntraeger, cohuck, rth
On 1/27/20 6:20 AM, David Hildenbrand wrote:
> [...]
>> @@ -0,0 +1,85 @@
>> +/*
>> + * DIAGNOSE 0x318 functions for reset and migration
>> + *
>> + * Copyright IBM, Corp. 2019
>
> Should be 2020 now.
>
> [...]
Where did the time go...
>> +static void s390_diag318_reset(DeviceState *dev)
>> +{
>> + if (kvm_enabled())
>> + kvm_s390_set_diag318_info(0);
>> +}
>> +
>> +static void s390_diag318_class_init(ObjectClass *klass, void *data)
>> +{
>> + DeviceClass *dc = DEVICE_CLASS(klass);
>> +
>> + dc->reset = s390_diag318_reset;
>> + dc->vmsd = &vmstate_diag318;
>> + dc->hotpluggable = false;
>> + /* Reason: Created automatically during machine instantiation */
>> + dc->user_creatable = false;
>> +}
>> +
>> +static const TypeInfo s390_diag318_info = {
>> + .class_init = s390_diag318_class_init,
>> + .parent = TYPE_DEVICE,
>> + .name = TYPE_S390_DIAG318,
>> + .instance_size = sizeof(DIAG318State),
>> +};
>> +
>> +static void s390_diag318_register_types(void)
>> +{
>> + type_register_static(&s390_diag318_info);
>> +}
>
> Do we really need a new device? Can't we simply glue that extended state
> to the machine state?
>
> -> target/s390x/machine.c
>
Those VM States relate to the CPU state... does it make sense to store the
diag318 info in a CPU state? (It doesn't seem necessary to store / migrate
this info for each CPU).
Should we store this in the S390CcwMachineState? Or perhaps create a generic
S390MachineState for information that needs to be stored once and migrated
once?
>> +
>> +type_init(s390_diag318_register_types)
>> diff --git a/hw/s390x/diag318.h b/hw/s390x/diag318.h
>> new file mode 100644
>> index 0000000..06d9f67
>> --- /dev/null
>> +++ b/hw/s390x/diag318.h
>> @@ -0,0 +1,40 @@
>> +/*
>> + * DIAGNOSE 0x318 functions for reset and migration
>> + *
>> + * Copyright IBM, Corp. 2019
>> + *
>> + * Authors:
>> + * Collin Walling <walling@linux.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 HW_DIAG318_H
>> +#define HW_DIAG318_H
>> +
>> +#include "qemu/osdep.h"
>> +#include "migration/vmstate.h"
>> +#include "qom/object.h"
>> +#include "hw/qdev-core.h"
>> +
>> +#define TYPE_S390_DIAG318 "diag318"
>> +#define DIAG318(obj) \
>> + OBJECT_CHECK(DIAG318State, (obj), TYPE_S390_DIAG318)
>> +
>> +typedef struct DIAG318State {
>> + /*< private >*/
>> + DeviceState parent_obj;
>> +
>> + /*< public >*/
>> + uint64_t info;
>> +} DIAG318State;
>> +
>> +typedef struct DIAG318Class {
>> + /*< private >*/
>> + DeviceClass parent_class;
>> +
>> + /*< public >*/
>> +} DIAG318Class;
>> +
>> +#endif /* HW_DIAG318_H */
>> \ No newline at end of file
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index e0e2813..d5b7a33 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -41,6 +41,7 @@
>> #include "hw/qdev-properties.h"
>> #include "hw/s390x/tod.h"
>> #include "sysemu/sysemu.h"
>> +#include "hw/s390x/diag318.h"
>>
>> S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
>> {
>> @@ -97,6 +98,7 @@ static const char *const reset_dev_types[] = {
>> "s390-sclp-event-facility",
>> "s390-flic",
>> "diag288",
>> + TYPE_S390_DIAG318,
>> };
>>
>> static void subsystem_reset(void)
>> @@ -237,6 +239,17 @@ static void s390_create_sclpconsole(const char *type, Chardev *chardev)
>> qdev_init_nofail(dev);
>> }
>>
>> +static void s390_init_diag318(void)
>> +{
>> + Object *new = object_new(TYPE_S390_DIAG318);
>> + DeviceState *dev = DEVICE(new);
>> +
>> + object_property_add_child(qdev_get_machine(), TYPE_S390_DIAG318,
>> + new, NULL);
>> + object_unref(new);
>> + qdev_init_nofail(dev);
>> +}
>> +
>> static void ccw_init(MachineState *machine)
>> {
>> int ret;
>> @@ -294,6 +307,9 @@ static void ccw_init(MachineState *machine)
>>
>> /* init the TOD clock */
>> s390_init_tod();
>> +
>> + /* init object used for migrating diag318 info */
>> + s390_init_diag318();
>> }
>>
>> static void s390_cpu_plug(HotplugHandler *hotplug_dev,
>> @@ -566,6 +582,7 @@ static void machine_set_loadparm(Object *obj, const char *val, Error **errp)
>> ms->loadparm[i] = ' '; /* pad right with spaces */
>> }
>> }
>> +
>
> unrelated change.
>
>> static inline void s390_machine_initfn(Object *obj)
>> {
>> object_property_add_bool(obj, "aes-key-wrap",
>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
>> index f57ce7b..636348c 100644
>> --- a/hw/s390x/sclp.c
>> +++ b/hw/s390x/sclp.c
>> @@ -15,6 +15,7 @@
>> #include "qemu/osdep.h"
>> #include "qemu/units.h"
>> #include "qapi/error.h"
>> +#include "qemu/error-report.h"
>> #include "cpu.h"
>> #include "sysemu/sysemu.h"
>> #include "hw/boards.h"
>> @@ -22,6 +23,7 @@
>> #include "hw/s390x/event-facility.h"
>> #include "hw/s390x/s390-pci-bus.h"
>> #include "hw/s390x/ipl.h"
>> +#include "kvm_s390x.h"
>>
>> static inline SCLPDevice *get_sclp_device(void)
>> {
>> @@ -37,10 +39,19 @@ static void prepare_cpu_entries(SCLPDevice *sclp, CPUEntry *entry, int *count)
>> {
>> MachineState *ms = MACHINE(qdev_get_machine());
>> uint8_t features[SCCB_CPU_FEATURE_LEN] = { 0 };
>> + int max_entries;
>> int i;
>>
>> + /* Calculate the max number of CPU entries that can be stored in the SCCB */
>> + max_entries = (SCCB_SIZE - offsetof(ReadInfo, entries)) / sizeof(CPUEntry);
>> +
>> s390_get_feat_block(S390_FEAT_TYPE_SCLP_CPU, features);
>> for (i = 0, *count = 0; i < ms->possible_cpus->len; i++) {
>> + if (*count == max_entries) {
>> + warn_report("Configuration only supports a max of %d CPU entries.",
>> + max_entries);
>
> I remember that "the sclp response will be limited to 247 CPUs if the
> feature is one". So we should have a double layout and make max_entries
> depending on s390_has_feat().
I'm looking back on previous discussions we had. Looks like this idea has been
mentioned before. (oops!) Perhaps I'm not understanding something.
How about we introduce a union in the ReadInfo struct. Something like:
union {
uint8_t byte_134;
struct CPUEntry entries[0];
} x;
If the diag318 facility is enabled, then we'll use that first byte
for bit indication and only allow 247 CPUs. Otherwise, we'll discard
the byte and allow the original 248 CPUs.
The offset_cpu field in the ReadInfo struct would still be used to
locate the first entry, of course.
Note: the Read SCP Info fields are on a 16-byte boundary.
>
> Regarding the message, I'd probably do a "Due to the current CPU model,
> some CPUs might be hidden from the VM (SCLP)."
>
> A VM could still manually probe for others.
>
> Thanks!
>
Right. I'll make the change to the message.
Thank you for the review!
--
Respectfully,
- Collin Walling
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 2/2] s390: diagnose 318 info reset and migration support
2020-01-27 11:36 ` Thomas Huth
@ 2020-01-27 15:58 ` Collin Walling
0 siblings, 0 replies; 23+ messages in thread
From: Collin Walling @ 2020-01-27 15:58 UTC (permalink / raw)
To: Thomas Huth, qemu-s390x, qemu-devel; +Cc: borntraeger, rth, cohuck, david
On 1/27/20 6:36 AM, Thomas Huth wrote:
> On 24/01/2020 23.14, Collin Walling wrote:
>> DIAGNOSE 0x318 (diag318) is a privileged s390x instruction that must
>> be intercepted by SIE and handled via KVM. Let's introduce some
>> functions to communicate between QEMU and KVM via ioctls. These
>> will be used to get/set the diag318 information.
>>
>> The availability of this instruction is determined by byte 134, bit 0
>> of the Read Info block. This coincidentally expands into the space used
>> for CPU entries by taking away one byte, which means VMs running with
>> the diag318 capability will not be able to retrieve information regarding
>> the 248th CPU. This will not effect performance, and VMs can still be
>
> s/effect/affect/ ?
>
>> ran with 248 CPUs.
>
> s/ran/run/ ?
>
>> In order to simplify the migration and system reset requirements of
>> the diag318 data, let's introduce it as a device class and include
>> a VMStateDescription.
>>
>> Diag318 is set to 0 during modified clear and load normal resets.
>>
>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>> ---
>> hw/s390x/Makefile.objs | 1 +
>> hw/s390x/diag318.c | 85 +++++++++++++++++++++++++++++++++++++
>> hw/s390x/diag318.h | 40 +++++++++++++++++
>> hw/s390x/s390-virtio-ccw.c | 17 ++++++++
>> hw/s390x/sclp.c | 13 ++++++
>> include/hw/s390x/sclp.h | 2 +
>> target/s390x/cpu_features.h | 1 +
>> target/s390x/cpu_features_def.inc.h | 3 ++
>> target/s390x/gen-features.c | 1 +
>> target/s390x/kvm-stub.c | 10 +++++
>> target/s390x/kvm.c | 29 +++++++++++++
>> target/s390x/kvm_s390x.h | 2 +
>> 12 files changed, 204 insertions(+)
>> create mode 100644 hw/s390x/diag318.c
>> create mode 100644 hw/s390x/diag318.h
>>
>> diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
>> index e02ed80..93621dc 100644
>> --- a/hw/s390x/Makefile.objs
>> +++ b/hw/s390x/Makefile.objs
>> @@ -34,3 +34,4 @@ obj-$(CONFIG_KVM) += s390-stattrib-kvm.o
>> obj-y += s390-ccw.o
>> obj-y += ap-device.o
>> obj-y += ap-bridge.o
>> +obj-y += diag318.o
>> diff --git a/hw/s390x/diag318.c b/hw/s390x/diag318.c
>> new file mode 100644
>> index 0000000..2d30bb2
>> --- /dev/null
>> +++ b/hw/s390x/diag318.c
>> @@ -0,0 +1,85 @@
>> +/*
>> + * DIAGNOSE 0x318 functions for reset and migration
>> + *
>> + * Copyright IBM, Corp. 2019
>
> Bump to 2020 ?
>
>> + * Authors:
>> + * Collin Walling <walling@linux.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/diag318.h"
>> +#include "qapi/error.h"
>> +#include "kvm_s390x.h"
>> +#include "sysemu/kvm.h"
>> +
>> +static int diag318_post_load(void *opaque, int version_id)
>> +{
>> + DIAG318State *d = opaque;
>> +
>> + if (kvm_enabled())
>> + kvm_s390_set_diag318_info(d->info);
>
> QEMU coding style requires curly braces also for single lines.
>
>> + return 0;
>> +}
>> +
>> +static int diag318_pre_save(void *opaque)
>> +{
>> + DIAG318State *d = opaque;
>> +
>> + if (kvm_enabled())
>> + kvm_s390_get_diag318_info(&d->info);
>
> dito
>
>> + return 0;
>> +}
>> +
>> +static bool diag318_needed(void *opaque)
>> +{
>> + return kvm_enabled() ? s390_has_feat(S390_FEAT_DIAG318) : 0;
>> +}
>> +
>> +const VMStateDescription vmstate_diag318 = {
>> + .name = "vmstate_diag318",
>> + .post_load = diag318_post_load,
>> + .pre_save = diag318_pre_save,
>> + .version_id = 1,
>> + .minimum_version_id = 1,
>> + .needed = diag318_needed,
>> + .fields = (VMStateField[]) {
>> + VMSTATE_UINT64(info, DIAG318State),
>> + VMSTATE_END_OF_LIST()
>> + }
>> +};
>> +
>> +static void s390_diag318_reset(DeviceState *dev)
>> +{
>> + if (kvm_enabled())
>> + kvm_s390_set_diag318_info(0);
>
> dito
>
>> +}
>> +
>> +static void s390_diag318_class_init(ObjectClass *klass, void *data)
>> +{
>> + DeviceClass *dc = DEVICE_CLASS(klass);
>> +
>> + dc->reset = s390_diag318_reset;
>> + dc->vmsd = &vmstate_diag318;
>> + dc->hotpluggable = false;
>> + /* Reason: Created automatically during machine instantiation */
>> + dc->user_creatable = false;
>> +}
>> +
>> +static const TypeInfo s390_diag318_info = {
>> + .class_init = s390_diag318_class_init,
>> + .parent = TYPE_DEVICE,
>> + .name = TYPE_S390_DIAG318,
>> + .instance_size = sizeof(DIAG318State),
>> +};
>> +
>> +static void s390_diag318_register_types(void)
>> +{
>> + type_register_static(&s390_diag318_info);
>> +}
>> +
>> +type_init(s390_diag318_register_types)
>> diff --git a/hw/s390x/diag318.h b/hw/s390x/diag318.h
>> new file mode 100644
>> index 0000000..06d9f67
>> --- /dev/null
>> +++ b/hw/s390x/diag318.h
>> @@ -0,0 +1,40 @@
>> +/*
>> + * DIAGNOSE 0x318 functions for reset and migration
>> + *
>> + * Copyright IBM, Corp. 2019
>
> 2020 ?
>
>> + * Authors:
>> + * Collin Walling <walling@linux.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 HW_DIAG318_H
>> +#define HW_DIAG318_H
>> +
>> +#include "qemu/osdep.h"
>> +#include "migration/vmstate.h"
>> +#include "qom/object.h"
>> +#include "hw/qdev-core.h"
>> +
>> +#define TYPE_S390_DIAG318 "diag318"
>> +#define DIAG318(obj) \
>> + OBJECT_CHECK(DIAG318State, (obj), TYPE_S390_DIAG318)
>> +
>> +typedef struct DIAG318State {
>> + /*< private >*/
>> + DeviceState parent_obj;
>> +
>> + /*< public >*/
>> + uint64_t info;
>> +} DIAG318State;
>> +
>> +typedef struct DIAG318Class {
>> + /*< private >*/
>> + DeviceClass parent_class;
>> +
>> + /*< public >*/
>> +} DIAG318Class;
>
> You don't use DIAG318Class anywhere. Please drop it.
>
>> +#endif /* HW_DIAG318_H */
>> \ No newline at end of file
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index e0e2813..d5b7a33 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -41,6 +41,7 @@
>> #include "hw/qdev-properties.h"
>> #include "hw/s390x/tod.h"
>> #include "sysemu/sysemu.h"
>> +#include "hw/s390x/diag318.h"
>>
>> S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
>> {
>> @@ -97,6 +98,7 @@ static const char *const reset_dev_types[] = {
>> "s390-sclp-event-facility",
>> "s390-flic",
>> "diag288",
>> + TYPE_S390_DIAG318,
>> };
>>
>> static void subsystem_reset(void)
>> @@ -237,6 +239,17 @@ static void s390_create_sclpconsole(const char *type, Chardev *chardev)
>> qdev_init_nofail(dev);
>> }
>>
>> +static void s390_init_diag318(void)
>> +{
>> + Object *new = object_new(TYPE_S390_DIAG318);
>
> For the very unlikely case that we ever switch QEMU to C++ ... could you
> maybe use a different variable name than "new" ? Simply "obj" maybe?
>
>> + DeviceState *dev = DEVICE(new);
>> +
>> + object_property_add_child(qdev_get_machine(), TYPE_S390_DIAG318,
>> + new, NULL);
>> + object_unref(new);
>> + qdev_init_nofail(dev);
>> +}
>> +
>
> Thomas
>
Noted your comments. Thanks for the review! I'll remember to run checkpatch
next time ;)
--
Respectfully,
- Collin Walling
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 2/2] s390: diagnose 318 info reset and migration support
2020-01-27 11:47 ` Cornelia Huck
@ 2020-01-27 16:39 ` Collin Walling
2020-01-27 17:35 ` Cornelia Huck
0 siblings, 1 reply; 23+ messages in thread
From: Collin Walling @ 2020-01-27 16:39 UTC (permalink / raw)
To: Cornelia Huck; +Cc: borntraeger, qemu-s390x, rth, qemu-devel, david
On 1/27/20 6:47 AM, Cornelia Huck wrote:
> On Fri, 24 Jan 2020 17:14:04 -0500
> Collin Walling <walling@linux.ibm.com> wrote:
>
>> DIAGNOSE 0x318 (diag318) is a privileged s390x instruction that must
>> be intercepted by SIE and handled via KVM. Let's introduce some
>> functions to communicate between QEMU and KVM via ioctls. These
>> will be used to get/set the diag318 information.
>
> Do you want to give a hint what diag 318 actually does?
>
For the sake of completeness, I'll have to get back to you on this.
>>
>> The availability of this instruction is determined by byte 134, bit 0
>> of the Read Info block. This coincidentally expands into the space used
>
> "SCLP Read Info"
>
>> for CPU entries by taking away one byte, which means VMs running with
>> the diag318 capability will not be able to retrieve information regarding
>> the 248th CPU. This will not effect performance, and VMs can still be
>> ran with 248 CPUs.
>
> Are there other ways in which that might affect guests? I assume Linux
> can deal with it? Is it ok architecture-wise?
>
> In any case, should go into the patch description :)
>
Same as above. I'll try to provide more information regarding what happens
here in my next reply.
>>
>> In order to simplify the migration and system reset requirements of
>> the diag318 data, let's introduce it as a device class and include
>> a VMStateDescription.
>>
>> Diag318 is set to 0 during modified clear and load normal resets.
>
> What exactly is set to 0? Stored values?
>
Correct. "The stored values set by DIAG318 are reset to 0 during..."
>>
>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>> ---
>> hw/s390x/Makefile.objs | 1 +
>> hw/s390x/diag318.c | 85 +++++++++++++++++++++++++++++++++++++
>> hw/s390x/diag318.h | 40 +++++++++++++++++
>> hw/s390x/s390-virtio-ccw.c | 17 ++++++++
>> hw/s390x/sclp.c | 13 ++++++
>> include/hw/s390x/sclp.h | 2 +
>> target/s390x/cpu_features.h | 1 +
>> target/s390x/cpu_features_def.inc.h | 3 ++
>> target/s390x/gen-features.c | 1 +
>> target/s390x/kvm-stub.c | 10 +++++
>> target/s390x/kvm.c | 29 +++++++++++++
>> target/s390x/kvm_s390x.h | 2 +
>> 12 files changed, 204 insertions(+)
>> create mode 100644 hw/s390x/diag318.c
>> create mode 100644 hw/s390x/diag318.h
>>
> (...)
>> diff --git a/hw/s390x/diag318.c b/hw/s390x/diag318.c
>> new file mode 100644
>> index 0000000..2d30bb2
>> --- /dev/null
>> +++ b/hw/s390x/diag318.c
>> @@ -0,0 +1,85 @@
>> +/*
>> + * DIAGNOSE 0x318 functions for reset and migration
>> + *
>> + * Copyright IBM, Corp. 2019
>
> Happy new year?
>
Woo!
>> + *
>> + * Authors:
>> + * Collin Walling <walling@linux.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/diag318.h"
>> +#include "qapi/error.h"
>> +#include "kvm_s390x.h"
>> +#include "sysemu/kvm.h"
>> +
>> +static int diag318_post_load(void *opaque, int version_id)
>> +{
>> + DIAG318State *d = opaque;
>> +
>> + if (kvm_enabled())
>
> As already noted by patchew, this needs some curly braces.
>
>> + kvm_s390_set_diag318_info(d->info);
>> +
>> + return 0;
>> +}
>> +
>> +static int diag318_pre_save(void *opaque)
>> +{
>> + DIAG318State *d = opaque;
>> +
>> + if (kvm_enabled())
>
> braces
>
>> + kvm_s390_get_diag318_info(&d->info);
>> +
>> + return 0;
>> +}
>> +
>> +static bool diag318_needed(void *opaque)
>> +{
>> + return kvm_enabled() ? s390_has_feat(S390_FEAT_DIAG318) : 0;
>
> Why do you need to guard this with kvm_enabled()? If tcg does not
> enable the feature, we should be fine; and if it emulates this in the
> future, we probably need to migrate something anyway.
>
Your explanation makes sense. My thoughts were to not even bother
registering the state description if KVM isn't enabled (but I guess
that thinking would mean that the other kvm_enabled fencing would
be redundant? Doh.)
I'll fix this.
>> +}
>> +
>> +const VMStateDescription vmstate_diag318 = {
>> + .name = "vmstate_diag318",
>> + .post_load = diag318_post_load,
>> + .pre_save = diag318_pre_save,
>> + .version_id = 1,
>> + .minimum_version_id = 1,
>> + .needed = diag318_needed,
>> + .fields = (VMStateField[]) {
>> + VMSTATE_UINT64(info, DIAG318State),
>> + VMSTATE_END_OF_LIST()
>> + }
>> +};
>> +
>> +static void s390_diag318_reset(DeviceState *dev)
>> +{
>> + if (kvm_enabled())
>
> braces
>
>> + kvm_s390_set_diag318_info(0);
>> +}
>> +
>> +static void s390_diag318_class_init(ObjectClass *klass, void *data)
>> +{
>> + DeviceClass *dc = DEVICE_CLASS(klass);
>> +
>> + dc->reset = s390_diag318_reset;
>> + dc->vmsd = &vmstate_diag318;
>> + dc->hotpluggable = false;
>> + /* Reason: Created automatically during machine instantiation */
>> + dc->user_creatable = false;
>> +}
>> +
>> +static const TypeInfo s390_diag318_info = {
>> + .class_init = s390_diag318_class_init,
>> + .parent = TYPE_DEVICE,
>> + .name = TYPE_S390_DIAG318,
>> + .instance_size = sizeof(DIAG318State),
>> +};
>> +
>> +static void s390_diag318_register_types(void)
>> +{
>> + type_register_static(&s390_diag318_info);
>> +}
>> +
>> +type_init(s390_diag318_register_types)
>> diff --git a/hw/s390x/diag318.h b/hw/s390x/diag318.h
>> new file mode 100644
>> index 0000000..06d9f67
>> --- /dev/null
>> +++ b/hw/s390x/diag318.h
>> @@ -0,0 +1,40 @@
>> +/*
>> + * DIAGNOSE 0x318 functions for reset and migration
>> + *
>> + * Copyright IBM, Corp. 2019
>
> Again, the year needs an update.
>
>> + *
>> + * Authors:
>> + * Collin Walling <walling@linux.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 HW_DIAG318_H
>> +#define HW_DIAG318_H
>> +
>> +#include "qemu/osdep.h"
>> +#include "migration/vmstate.h"
>> +#include "qom/object.h"
>> +#include "hw/qdev-core.h"
>> +
>> +#define TYPE_S390_DIAG318 "diag318"
>> +#define DIAG318(obj) \
>> + OBJECT_CHECK(DIAG318State, (obj), TYPE_S390_DIAG318)
>> +
>> +typedef struct DIAG318State {
>> + /*< private >*/
>> + DeviceState parent_obj;
>> +
>> + /*< public >*/
>> + uint64_t info;
>> +} DIAG318State;
>> +
>> +typedef struct DIAG318Class {
>> + /*< private >*/
>> + DeviceClass parent_class;
>> +
>> + /*< public >*/
>> +} DIAG318Class;
>> +
>> +#endif /* HW_DIAG318_H */
>> \ No newline at end of file
>
> And please add a newline :)
>
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index e0e2813..d5b7a33 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -41,6 +41,7 @@
>> #include "hw/qdev-properties.h"
>> #include "hw/s390x/tod.h"
>> #include "sysemu/sysemu.h"
>> +#include "hw/s390x/diag318.h"
>>
>> S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
>> {
>> @@ -97,6 +98,7 @@ static const char *const reset_dev_types[] = {
>> "s390-sclp-event-facility",
>> "s390-flic",
>> "diag288",
>> + TYPE_S390_DIAG318,
>> };
>>
>> static void subsystem_reset(void)
>> @@ -237,6 +239,17 @@ static void s390_create_sclpconsole(const char *type, Chardev *chardev)
>> qdev_init_nofail(dev);
>> }
>>
>> +static void s390_init_diag318(void)
>> +{
>> + Object *new = object_new(TYPE_S390_DIAG318);
>> + DeviceState *dev = DEVICE(new);
>> +
>> + object_property_add_child(qdev_get_machine(), TYPE_S390_DIAG318,
>> + new, NULL);
>> + object_unref(new);
>> + qdev_init_nofail(dev);
>> +}
>> +
>> static void ccw_init(MachineState *machine)
>> {
>> int ret;
>> @@ -294,6 +307,9 @@ static void ccw_init(MachineState *machine)
>>
>> /* init the TOD clock */
>> s390_init_tod();
>> +
>> + /* init object used for migrating diag318 info */
>> + s390_init_diag318();
>
> Doesn't that device do a bit more than 'migrating' info?
>
> Also, it seems a bit useless unless you're running with kvm and the
> feature bit switched on...
>
Right... I think this whole "diag318 device" thing needs some rethinking.
I made a comment on David's response regarding where to but the VMStateDescription
code for diag318. Perhaps including the related information within the S390MachineState
would be better? (I'm not sure).
>> }
>>
>> static void s390_cpu_plug(HotplugHandler *hotplug_dev,
>> @@ -566,6 +582,7 @@ static void machine_set_loadparm(Object *obj, const char *val, Error **errp)
>> ms->loadparm[i] = ' '; /* pad right with spaces */
>> }
>> }
>> +
>
> Still whitespace :)
>
>> static inline void s390_machine_initfn(Object *obj)
>> {
>> object_property_add_bool(obj, "aes-key-wrap",
>> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
>> index f57ce7b..636348c 100644
>> --- a/hw/s390x/sclp.c
>> +++ b/hw/s390x/sclp.c
>> @@ -15,6 +15,7 @@
>> #include "qemu/osdep.h"
>> #include "qemu/units.h"
>> #include "qapi/error.h"
>> +#include "qemu/error-report.h"
>> #include "cpu.h"
>> #include "sysemu/sysemu.h"
>> #include "hw/boards.h"
>> @@ -22,6 +23,7 @@
>> #include "hw/s390x/event-facility.h"
>> #include "hw/s390x/s390-pci-bus.h"
>> #include "hw/s390x/ipl.h"
>> +#include "kvm_s390x.h"
>>
>> static inline SCLPDevice *get_sclp_device(void)
>> {
>> @@ -37,10 +39,19 @@ static void prepare_cpu_entries(SCLPDevice *sclp, CPUEntry *entry, int *count)
>> {
>> MachineState *ms = MACHINE(qdev_get_machine());
>> uint8_t features[SCCB_CPU_FEATURE_LEN] = { 0 };
>> + int max_entries;
>> int i;
>>
>> + /* Calculate the max number of CPU entries that can be stored in the SCCB */
>> + max_entries = (SCCB_SIZE - offsetof(ReadInfo, entries)) / sizeof(CPUEntry);
>> +
>> s390_get_feat_block(S390_FEAT_TYPE_SCLP_CPU, features);
>> for (i = 0, *count = 0; i < ms->possible_cpus->len; i++) {
>> + if (*count == max_entries) {
>> + warn_report("Configuration only supports a max of %d CPU entries.",
>> + max_entries);
>
> IIUC, this only moans during Read Info... but you could previously add
> more cpus than what could be serviced by Read Info. So, it looks to me
> you get some messages when a guest is doing Read Info; that seems more
> confusing than helpful to me. Can't we rather warn at cpu instantiation
> time?
>
Ahh, I didn't think of that. For some reason, I was thinking that Read Info
would only be queried once.
Yes, this makes sense. I'll relocate the warning message...
>> + break;
>> + }
>> if (!ms->possible_cpus->cpus[i].cpu) {
>> continue;
>> }
>> @@ -80,6 +91,8 @@ static void read_SCP_info(SCLPDevice *sclp, SCCB *sccb)
>> s390_get_feat_block(S390_FEAT_TYPE_SCLP_CONF_CHAR_EXT,
>> read_info->conf_char_ext);
>>
>> + s390_get_feat_block(S390_FEAT_TYPE_SCLP_BYTE_134, read_info->byte_134);
>> +
>> read_info->facilities = cpu_to_be64(SCLP_HAS_CPU_INFO |
>> SCLP_HAS_IOA_RECONFIG);
>>
>
> (...)
>
>
I've noted the nits as well. Thanks for your review!
--
Respectfully,
- Collin Walling
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 2/2] s390: diagnose 318 info reset and migration support
2020-01-27 15:57 ` Collin Walling
@ 2020-01-27 17:09 ` David Hildenbrand
2020-01-27 17:29 ` Cornelia Huck
0 siblings, 1 reply; 23+ messages in thread
From: David Hildenbrand @ 2020-01-27 17:09 UTC (permalink / raw)
To: Collin Walling, qemu-s390x, qemu-devel; +Cc: borntraeger, cohuck, rth
>>> +static void s390_diag318_reset(DeviceState *dev)
>>> +{
>>> + if (kvm_enabled())
>>> + kvm_s390_set_diag318_info(0);
>>> +}
>>> +
>>> +static void s390_diag318_class_init(ObjectClass *klass, void *data)
>>> +{
>>> + DeviceClass *dc = DEVICE_CLASS(klass);
>>> +
>>> + dc->reset = s390_diag318_reset;
>>> + dc->vmsd = &vmstate_diag318;
>>> + dc->hotpluggable = false;
>>> + /* Reason: Created automatically during machine instantiation */
>>> + dc->user_creatable = false;
>>> +}
>>> +
>>> +static const TypeInfo s390_diag318_info = {
>>> + .class_init = s390_diag318_class_init,
>>> + .parent = TYPE_DEVICE,
>>> + .name = TYPE_S390_DIAG318,
>>> + .instance_size = sizeof(DIAG318State),
>>> +};
>>> +
>>> +static void s390_diag318_register_types(void)
>>> +{
>>> + type_register_static(&s390_diag318_info);
>>> +}
>>
>> Do we really need a new device? Can't we simply glue that extended state
>> to the machine state?
>>
>> -> target/s390x/machine.c
>>
>
> Those VM States relate to the CPU state... does it make sense to store the
> diag318 info in a CPU state? (It doesn't seem necessary to store / migrate
> this info for each CPU).
I'm sorry, I was looking at the wrong file ...
>
> Should we store this in the S390CcwMachineState? Or perhaps create a generic
> S390MachineState for information that needs to be stored once and migrated
> once?
... I actually thought we have something like this already. Personally,
I think that would make sense. At least spapr seems to have something
like this already (hw/ppc/spapr.c:spapr_machine_init().
@Conny?
[...]
>
> How about we introduce a union in the ReadInfo struct. Something like:
>
> union {
> uint8_t byte_134;
> struct CPUEntry entries[0];
> } x;
Or drop the "entries" pointer completely and introduce
static int cpu_entries_offset(void)
{
/*
* When we have to indicate features in byte 134, we have to move
* the start of the cpu entries.
*/
if (s390_has_feat(S390_FEAT_DIAG318)) {
return 144;
}
return 128;
}
struct CPUEntry *cpu_entries(ReadInfo *ri)
{
return (struct CPUEntry *)((void *)ri + cpu_entries_offset());
}
unsigned int cpu_entries)count(ReadInfo *ri)
{
return (SCCB_SIZE - cpu_entries_offset()) / sizeof(CPUEntry);
}
etc. (might take some tweaking to make it compile) and a comment for the
struct. Not sure what's better. Having two struct CPUEntry entries[0] is
also confusing.
Thanks!
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 2/2] s390: diagnose 318 info reset and migration support
2020-01-27 17:09 ` David Hildenbrand
@ 2020-01-27 17:29 ` Cornelia Huck
2020-01-27 17:55 ` David Hildenbrand
0 siblings, 1 reply; 23+ messages in thread
From: Cornelia Huck @ 2020-01-27 17:29 UTC (permalink / raw)
To: David Hildenbrand
Cc: borntraeger, Collin Walling, rth, qemu-s390x, qemu-devel
On Mon, 27 Jan 2020 18:09:11 +0100
David Hildenbrand <david@redhat.com> wrote:
> >>> +static void s390_diag318_reset(DeviceState *dev)
> >>> +{
> >>> + if (kvm_enabled())
> >>> + kvm_s390_set_diag318_info(0);
> >>> +}
> >>> +
> >>> +static void s390_diag318_class_init(ObjectClass *klass, void *data)
> >>> +{
> >>> + DeviceClass *dc = DEVICE_CLASS(klass);
> >>> +
> >>> + dc->reset = s390_diag318_reset;
> >>> + dc->vmsd = &vmstate_diag318;
> >>> + dc->hotpluggable = false;
> >>> + /* Reason: Created automatically during machine instantiation */
> >>> + dc->user_creatable = false;
> >>> +}
> >>> +
> >>> +static const TypeInfo s390_diag318_info = {
> >>> + .class_init = s390_diag318_class_init,
> >>> + .parent = TYPE_DEVICE,
> >>> + .name = TYPE_S390_DIAG318,
> >>> + .instance_size = sizeof(DIAG318State),
> >>> +};
> >>> +
> >>> +static void s390_diag318_register_types(void)
> >>> +{
> >>> + type_register_static(&s390_diag318_info);
> >>> +}
> >>
> >> Do we really need a new device? Can't we simply glue that extended state
> >> to the machine state?
> >>
> >> -> target/s390x/machine.c
> >>
> >
> > Those VM States relate to the CPU state... does it make sense to store the
> > diag318 info in a CPU state? (It doesn't seem necessary to store / migrate
> > this info for each CPU).
>
> I'm sorry, I was looking at the wrong file ...
>
> >
> > Should we store this in the S390CcwMachineState? Or perhaps create a generic
> > S390MachineState for information that needs to be stored once and migrated
> > once?
>
> ... I actually thought we have something like this already. Personally,
> I think that would make sense. At least spapr seems to have something
> like this already (hw/ppc/spapr.c:spapr_machine_init().
>
> @Conny?
What are you referring to? I only see the one with the FIXME in front
of it...
>
> [...]
> >
> > How about we introduce a union in the ReadInfo struct. Something like:
> >
> > union {
> > uint8_t byte_134;
> > struct CPUEntry entries[0];
> > } x;
>
> Or drop the "entries" pointer completely and introduce
>
> static int cpu_entries_offset(void)
> {
> /*
> * When we have to indicate features in byte 134, we have to move
> * the start of the cpu entries.
> */
> if (s390_has_feat(S390_FEAT_DIAG318)) {
> return 144;
> }
> return 128;
> }
>
> struct CPUEntry *cpu_entries(ReadInfo *ri)
> {
> return (struct CPUEntry *)((void *)ri + cpu_entries_offset());
> }
>
> unsigned int cpu_entries)count(ReadInfo *ri)
> {
> return (SCCB_SIZE - cpu_entries_offset()) / sizeof(CPUEntry);
> }
>
> etc. (might take some tweaking to make it compile) and a comment for the
> struct. Not sure what's better. Having two struct CPUEntry entries[0] is
> also confusing.
I think that version may end up looking better.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 2/2] s390: diagnose 318 info reset and migration support
2020-01-27 16:39 ` Collin Walling
@ 2020-01-27 17:35 ` Cornelia Huck
2020-01-27 23:05 ` Collin Walling
2020-01-28 14:37 ` Collin Walling
0 siblings, 2 replies; 23+ messages in thread
From: Cornelia Huck @ 2020-01-27 17:35 UTC (permalink / raw)
To: Collin Walling; +Cc: borntraeger, qemu-s390x, rth, qemu-devel, david
On Mon, 27 Jan 2020 11:39:02 -0500
Collin Walling <walling@linux.ibm.com> wrote:
> On 1/27/20 6:47 AM, Cornelia Huck wrote:
> > On Fri, 24 Jan 2020 17:14:04 -0500
> > Collin Walling <walling@linux.ibm.com> wrote:
> >
> >> DIAGNOSE 0x318 (diag318) is a privileged s390x instruction that must
> >> be intercepted by SIE and handled via KVM. Let's introduce some
> >> functions to communicate between QEMU and KVM via ioctls. These
> >> will be used to get/set the diag318 information.
> >
> > Do you want to give a hint what diag 318 actually does?
> >
>
> For the sake of completeness, I'll have to get back to you on this.
>
> >>
> >> The availability of this instruction is determined by byte 134, bit 0
> >> of the Read Info block. This coincidentally expands into the space used
> >
> > "SCLP Read Info"
> >
> >> for CPU entries by taking away one byte, which means VMs running with
> >> the diag318 capability will not be able to retrieve information regarding
> >> the 248th CPU. This will not effect performance, and VMs can still be
> >> ran with 248 CPUs.
> >
> > Are there other ways in which that might affect guests? I assume Linux
> > can deal with it? Is it ok architecture-wise?
> >
> > In any case, should go into the patch description :)
> >
>
> Same as above. I'll try to provide more information regarding what happens
> here in my next reply.
I think you can lift some stuff from the cover letter.
>
> >>
> >> In order to simplify the migration and system reset requirements of
> >> the diag318 data, let's introduce it as a device class and include
> >> a VMStateDescription.
> >>
> >> Diag318 is set to 0 during modified clear and load normal resets.
> >
> > What exactly is set to 0? Stored values?
> >
>
> Correct. "The stored values set by DIAG318 are reset to 0 during..."
Sounds good.
>
> >>
> >> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> >> ---
> >> hw/s390x/Makefile.objs | 1 +
> >> hw/s390x/diag318.c | 85 +++++++++++++++++++++++++++++++++++++
> >> hw/s390x/diag318.h | 40 +++++++++++++++++
> >> hw/s390x/s390-virtio-ccw.c | 17 ++++++++
> >> hw/s390x/sclp.c | 13 ++++++
> >> include/hw/s390x/sclp.h | 2 +
> >> target/s390x/cpu_features.h | 1 +
> >> target/s390x/cpu_features_def.inc.h | 3 ++
> >> target/s390x/gen-features.c | 1 +
> >> target/s390x/kvm-stub.c | 10 +++++
> >> target/s390x/kvm.c | 29 +++++++++++++
> >> target/s390x/kvm_s390x.h | 2 +
> >> 12 files changed, 204 insertions(+)
> >> create mode 100644 hw/s390x/diag318.c
> >> create mode 100644 hw/s390x/diag318.h
> >>
> > (...)
> >> +static bool diag318_needed(void *opaque)
> >> +{
> >> + return kvm_enabled() ? s390_has_feat(S390_FEAT_DIAG318) : 0;
> >
> > Why do you need to guard this with kvm_enabled()? If tcg does not
> > enable the feature, we should be fine; and if it emulates this in the
> > future, we probably need to migrate something anyway.
> >
>
> Your explanation makes sense. My thoughts were to not even bother
> registering the state description if KVM isn't enabled (but I guess
> that thinking would mean that the other kvm_enabled fencing would
> be redundant? Doh.)
My thinking was along the lines how easy it would be to do some tcg
implementation (not sure if that even makes sense.)
>
> I'll fix this.
>
> >> @@ -294,6 +307,9 @@ static void ccw_init(MachineState *machine)
> >>
> >> /* init the TOD clock */
> >> s390_init_tod();
> >> +
> >> + /* init object used for migrating diag318 info */
> >> + s390_init_diag318();
> >
> > Doesn't that device do a bit more than 'migrating' info?
> >
> > Also, it seems a bit useless unless you're running with kvm and the
> > feature bit switched on...
> >
>
> Right... I think this whole "diag318 device" thing needs some rethinking.
>
> I made a comment on David's response regarding where to but the VMStateDescription
> code for diag318. Perhaps including the related information within the S390MachineState
> would be better? (I'm not sure).
Replied to David's mail.
> >> @@ -37,10 +39,19 @@ static void prepare_cpu_entries(SCLPDevice *sclp, CPUEntry *entry, int *count)
> >> {
> >> MachineState *ms = MACHINE(qdev_get_machine());
> >> uint8_t features[SCCB_CPU_FEATURE_LEN] = { 0 };
> >> + int max_entries;
> >> int i;
> >>
> >> + /* Calculate the max number of CPU entries that can be stored in the SCCB */
> >> + max_entries = (SCCB_SIZE - offsetof(ReadInfo, entries)) / sizeof(CPUEntry);
> >> +
> >> s390_get_feat_block(S390_FEAT_TYPE_SCLP_CPU, features);
> >> for (i = 0, *count = 0; i < ms->possible_cpus->len; i++) {
> >> + if (*count == max_entries) {
> >> + warn_report("Configuration only supports a max of %d CPU entries.",
> >> + max_entries);
> >
> > IIUC, this only moans during Read Info... but you could previously add
> > more cpus than what could be serviced by Read Info. So, it looks to me
> > you get some messages when a guest is doing Read Info; that seems more
> > confusing than helpful to me. Can't we rather warn at cpu instantiation
> > time?
> >
>
> Ahh, I didn't think of that. For some reason, I was thinking that Read Info
> would only be queried once.
Linux probably only does it once, but there's nothing stopping a guest
from doing it more often :)
>
> Yes, this makes sense. I'll relocate the warning message...
>
> >> + break;
> >> + }
> >> if (!ms->possible_cpus->cpus[i].cpu) {
> >> continue;
> >> }
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 2/2] s390: diagnose 318 info reset and migration support
2020-01-27 17:29 ` Cornelia Huck
@ 2020-01-27 17:55 ` David Hildenbrand
2020-01-27 18:21 ` Collin Walling
0 siblings, 1 reply; 23+ messages in thread
From: David Hildenbrand @ 2020-01-27 17:55 UTC (permalink / raw)
To: Cornelia Huck; +Cc: borntraeger, Collin Walling, rth, qemu-s390x, qemu-devel
On 27.01.20 18:29, Cornelia Huck wrote:
> On Mon, 27 Jan 2020 18:09:11 +0100
> David Hildenbrand <david@redhat.com> wrote:
>
>>>>> +static void s390_diag318_reset(DeviceState *dev)
>>>>> +{
>>>>> + if (kvm_enabled())
>>>>> + kvm_s390_set_diag318_info(0);
>>>>> +}
>>>>> +
>>>>> +static void s390_diag318_class_init(ObjectClass *klass, void *data)
>>>>> +{
>>>>> + DeviceClass *dc = DEVICE_CLASS(klass);
>>>>> +
>>>>> + dc->reset = s390_diag318_reset;
>>>>> + dc->vmsd = &vmstate_diag318;
>>>>> + dc->hotpluggable = false;
>>>>> + /* Reason: Created automatically during machine instantiation */
>>>>> + dc->user_creatable = false;
>>>>> +}
>>>>> +
>>>>> +static const TypeInfo s390_diag318_info = {
>>>>> + .class_init = s390_diag318_class_init,
>>>>> + .parent = TYPE_DEVICE,
>>>>> + .name = TYPE_S390_DIAG318,
>>>>> + .instance_size = sizeof(DIAG318State),
>>>>> +};
>>>>> +
>>>>> +static void s390_diag318_register_types(void)
>>>>> +{
>>>>> + type_register_static(&s390_diag318_info);
>>>>> +}
>>>>
>>>> Do we really need a new device? Can't we simply glue that extended state
>>>> to the machine state?
>>>>
>>>> -> target/s390x/machine.c
>>>>
>>>
>>> Those VM States relate to the CPU state... does it make sense to store the
>>> diag318 info in a CPU state? (It doesn't seem necessary to store / migrate
>>> this info for each CPU).
>>
>> I'm sorry, I was looking at the wrong file ...
>>
>>>
>>> Should we store this in the S390CcwMachineState? Or perhaps create a generic
>>> S390MachineState for information that needs to be stored once and migrated
>>> once?
>>
>> ... I actually thought we have something like this already. Personally,
>> I think that would make sense. At least spapr seems to have something
>> like this already (hw/ppc/spapr.c:spapr_machine_init().
>>
>> @Conny?
>
> What are you referring to? I only see the one with the FIXME in front
> of it...
That's the one I mean. The fixme states something about qdev ... but
AFAIK that's only applicable if TYPE_DEVICE is involved. So maybe right
now there is no other way than registering the vmstate directly.
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 2/2] s390: diagnose 318 info reset and migration support
2020-01-27 17:55 ` David Hildenbrand
@ 2020-01-27 18:21 ` Collin Walling
2020-01-27 18:52 ` Collin Walling
0 siblings, 1 reply; 23+ messages in thread
From: Collin Walling @ 2020-01-27 18:21 UTC (permalink / raw)
To: David Hildenbrand, Cornelia Huck; +Cc: borntraeger, qemu-s390x, qemu-devel, rth
On 1/27/20 12:55 PM, David Hildenbrand wrote:
> On 27.01.20 18:29, Cornelia Huck wrote:
>> On Mon, 27 Jan 2020 18:09:11 +0100
>> David Hildenbrand <david@redhat.com> wrote:
>>
>>>>>> +static void s390_diag318_reset(DeviceState *dev)
>>>>>> +{
>>>>>> + if (kvm_enabled())
>>>>>> + kvm_s390_set_diag318_info(0);
>>>>>> +}
>>>>>> +
>>>>>> +static void s390_diag318_class_init(ObjectClass *klass, void *data)
>>>>>> +{
>>>>>> + DeviceClass *dc = DEVICE_CLASS(klass);
>>>>>> +
>>>>>> + dc->reset = s390_diag318_reset;
>>>>>> + dc->vmsd = &vmstate_diag318;
>>>>>> + dc->hotpluggable = false;
>>>>>> + /* Reason: Created automatically during machine instantiation */
>>>>>> + dc->user_creatable = false;
>>>>>> +}
>>>>>> +
>>>>>> +static const TypeInfo s390_diag318_info = {
>>>>>> + .class_init = s390_diag318_class_init,
>>>>>> + .parent = TYPE_DEVICE,
>>>>>> + .name = TYPE_S390_DIAG318,
>>>>>> + .instance_size = sizeof(DIAG318State),
>>>>>> +};
>>>>>> +
>>>>>> +static void s390_diag318_register_types(void)
>>>>>> +{
>>>>>> + type_register_static(&s390_diag318_info);
>>>>>> +}
>>>>>
>>>>> Do we really need a new device? Can't we simply glue that extended state
>>>>> to the machine state?
>>>>>
>>>>> -> target/s390x/machine.c
>>>>>
>>>>
>>>> Those VM States relate to the CPU state... does it make sense to store the
>>>> diag318 info in a CPU state? (It doesn't seem necessary to store / migrate
>>>> this info for each CPU).
>>>
>>> I'm sorry, I was looking at the wrong file ...
>>>
>>>>
>>>> Should we store this in the S390CcwMachineState? Or perhaps create a generic
>>>> S390MachineState for information that needs to be stored once and migrated
>>>> once?
>>>
>>> ... I actually thought we have something like this already. Personally,
>>> I think that would make sense. At least spapr seems to have something
>>> like this already (hw/ppc/spapr.c:spapr_machine_init().
>>>
>>> @Conny?
>>
>> What are you referring to? I only see the one with the FIXME in front
>> of it...
>
> That's the one I mean. The fixme states something about qdev ... but
> AFAIK that's only applicable if TYPE_DEVICE is involved. So maybe right
> now there is no other way than registering the vmstate directly.
>
Hmm okay. I'll take a look at how spapr does it. I think I've registered a
vmstate via register_savevm_live() in an earlier version, but had difficulties
figuring out where to store the data. I'll revisit this approach.
Thanks for the feedback!
--
Respectfully,
- Collin Walling
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 2/2] s390: diagnose 318 info reset and migration support
2020-01-27 18:21 ` Collin Walling
@ 2020-01-27 18:52 ` Collin Walling
2020-01-28 11:19 ` Cornelia Huck
0 siblings, 1 reply; 23+ messages in thread
From: Collin Walling @ 2020-01-27 18:52 UTC (permalink / raw)
To: David Hildenbrand, Cornelia Huck; +Cc: borntraeger, qemu-s390x, qemu-devel, rth
On 1/27/20 1:21 PM, Collin Walling wrote:
> On 1/27/20 12:55 PM, David Hildenbrand wrote:
>> On 27.01.20 18:29, Cornelia Huck wrote:
>>> On Mon, 27 Jan 2020 18:09:11 +0100
>>> David Hildenbrand <david@redhat.com> wrote:
>>>
>>>>>>> +static void s390_diag318_reset(DeviceState *dev)
>>>>>>> +{
>>>>>>> + if (kvm_enabled())
>>>>>>> + kvm_s390_set_diag318_info(0);
>>>>>>> +}
>>>>>>> +
>>>>>>> +static void s390_diag318_class_init(ObjectClass *klass, void *data)
>>>>>>> +{
>>>>>>> + DeviceClass *dc = DEVICE_CLASS(klass);
>>>>>>> +
>>>>>>> + dc->reset = s390_diag318_reset;
>>>>>>> + dc->vmsd = &vmstate_diag318;
>>>>>>> + dc->hotpluggable = false;
>>>>>>> + /* Reason: Created automatically during machine instantiation */
>>>>>>> + dc->user_creatable = false;
>>>>>>> +}
>>>>>>> +
>>>>>>> +static const TypeInfo s390_diag318_info = {
>>>>>>> + .class_init = s390_diag318_class_init,
>>>>>>> + .parent = TYPE_DEVICE,
>>>>>>> + .name = TYPE_S390_DIAG318,
>>>>>>> + .instance_size = sizeof(DIAG318State),
>>>>>>> +};
>>>>>>> +
>>>>>>> +static void s390_diag318_register_types(void)
>>>>>>> +{
>>>>>>> + type_register_static(&s390_diag318_info);
>>>>>>> +}
>>>>>>
>>>>>> Do we really need a new device? Can't we simply glue that extended state
>>>>>> to the machine state?
>>>>>>
>>>>>> -> target/s390x/machine.c
>>>>>>
>>>>>
>>>>> Those VM States relate to the CPU state... does it make sense to store the
>>>>> diag318 info in a CPU state? (It doesn't seem necessary to store / migrate
>>>>> this info for each CPU).
>>>>
>>>> I'm sorry, I was looking at the wrong file ...
>>>>
>>>>>
>>>>> Should we store this in the S390CcwMachineState? Or perhaps create a generic
>>>>> S390MachineState for information that needs to be stored once and migrated
>>>>> once?
>>>>
>>>> ... I actually thought we have something like this already. Personally,
>>>> I think that would make sense. At least spapr seems to have something
>>>> like this already (hw/ppc/spapr.c:spapr_machine_init().
>>>>
>>>> @Conny?
>>>
>>> What are you referring to? I only see the one with the FIXME in front
>>> of it...
>>
>> That's the one I mean. The fixme states something about qdev ... but
>> AFAIK that's only applicable if TYPE_DEVICE is involved. So maybe right
>> now there is no other way than registering the vmstate directly.
>>
>
> Hmm okay. I'll take a look at how spapr does it. I think I've registered a
> vmstate via register_savevm_live() in an earlier version, but had difficulties
> figuring out where to store the data. I'll revisit this approach.
>
> Thanks for the feedback!
>
Err perhaps not entirely in this manner...
docs/devel/migration.rst declares the register_savevm_live() function as the
"legacy way" of doing things. I'll have to see how other VMStateDescriptions
are modeled. I think vmstate_register() is what I want.
Sorry for the confusion.
--
Respectfully,
- Collin Walling
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 2/2] s390: diagnose 318 info reset and migration support
2020-01-27 17:35 ` Cornelia Huck
@ 2020-01-27 23:05 ` Collin Walling
2020-01-28 11:24 ` Cornelia Huck
2020-01-28 14:37 ` Collin Walling
1 sibling, 1 reply; 23+ messages in thread
From: Collin Walling @ 2020-01-27 23:05 UTC (permalink / raw)
To: Cornelia Huck; +Cc: borntraeger, qemu-s390x, david, qemu-devel, rth
On 1/27/20 12:35 PM, Cornelia Huck wrote:
> On Mon, 27 Jan 2020 11:39:02 -0500
> Collin Walling <walling@linux.ibm.com> wrote:
>
>> On 1/27/20 6:47 AM, Cornelia Huck wrote:
>>> On Fri, 24 Jan 2020 17:14:04 -0500
>>> Collin Walling <walling@linux.ibm.com> wrote:
>>>
[...]
>>>>
>>>> The availability of this instruction is determined by byte 134, bit 0
>>>> of the Read Info block. This coincidentally expands into the space used
>>>
>>> "SCLP Read Info"
>>>
>>>> for CPU entries by taking away one byte, which means VMs running with
>>>> the diag318 capability will not be able to retrieve information regarding
>>>> the 248th CPU. This will not effect performance, and VMs can still be
>>>> ran with 248 CPUs.
>>>
>>> Are there other ways in which that might affect guests? I assume Linux
>>> can deal with it? Is it ok architecture-wise?
>>>
>>> In any case, should go into the patch description :)
>>>
>>
>> Same as above. I'll try to provide more information regarding what happens
>> here in my next reply.
>
> I think you can lift some stuff from the cover letter.
>
Here's what I found out:
Each CPU entry holds info regarding the CPU's address / ID as well as an
indication of the availability of certain CPU features. With these patches,
we lose a CPU entry for one CPU (essentially what would be the CPU at the
tail-end of the list). This CPU exists, but is essentially in limbo... the
machine cannot access any information regarding it.
So, a VM can run with the original N max CPUs, but in reality we can only
utilize n-1.
>>
>>>>
[...]
--
Respectfully,
- Collin Walling
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 2/2] s390: diagnose 318 info reset and migration support
2020-01-27 18:52 ` Collin Walling
@ 2020-01-28 11:19 ` Cornelia Huck
0 siblings, 0 replies; 23+ messages in thread
From: Cornelia Huck @ 2020-01-28 11:19 UTC (permalink / raw)
To: Collin Walling
Cc: borntraeger, qemu-s390x, rth, qemu-devel, David Hildenbrand
On Mon, 27 Jan 2020 13:52:48 -0500
Collin Walling <walling@linux.ibm.com> wrote:
> On 1/27/20 1:21 PM, Collin Walling wrote:
> > On 1/27/20 12:55 PM, David Hildenbrand wrote:
> >> On 27.01.20 18:29, Cornelia Huck wrote:
> >>> On Mon, 27 Jan 2020 18:09:11 +0100
> >>> David Hildenbrand <david@redhat.com> wrote:
> >>>> ... I actually thought we have something like this already. Personally,
> >>>> I think that would make sense. At least spapr seems to have something
> >>>> like this already (hw/ppc/spapr.c:spapr_machine_init().
> >>>>
> >>>> @Conny?
> >>>
> >>> What are you referring to? I only see the one with the FIXME in front
> >>> of it...
> >>
> >> That's the one I mean. The fixme states something about qdev ... but
> >> AFAIK that's only applicable if TYPE_DEVICE is involved. So maybe right
> >> now there is no other way than registering the vmstate directly.
> >>
> >
> > Hmm okay. I'll take a look at how spapr does it. I think I've registered a
> > vmstate via register_savevm_live() in an earlier version, but had difficulties
> > figuring out where to store the data. I'll revisit this approach.
> >
> > Thanks for the feedback!
> >
>
> Err perhaps not entirely in this manner...
>
> docs/devel/migration.rst declares the register_savevm_live() function as the
> "legacy way" of doing things. I'll have to see how other VMStateDescriptions
> are modeled. I think vmstate_register() is what I want.
>
> Sorry for the confusion.
Ok, I've now read what the FIXME actually says :) Since the machine
does not inherit from device (but from object), vmstate_register()
looks like the right thing to do.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 2/2] s390: diagnose 318 info reset and migration support
2020-01-27 23:05 ` Collin Walling
@ 2020-01-28 11:24 ` Cornelia Huck
2020-01-28 14:38 ` Collin Walling
0 siblings, 1 reply; 23+ messages in thread
From: Cornelia Huck @ 2020-01-28 11:24 UTC (permalink / raw)
To: Collin Walling; +Cc: borntraeger, qemu-s390x, david, qemu-devel, rth
On Mon, 27 Jan 2020 18:05:36 -0500
Collin Walling <walling@linux.ibm.com> wrote:
> On 1/27/20 12:35 PM, Cornelia Huck wrote:
> > On Mon, 27 Jan 2020 11:39:02 -0500
> > Collin Walling <walling@linux.ibm.com> wrote:
> >
> >> On 1/27/20 6:47 AM, Cornelia Huck wrote:
> >>> On Fri, 24 Jan 2020 17:14:04 -0500
> >>> Collin Walling <walling@linux.ibm.com> wrote:
> >>>
>
> [...]
>
> >>>>
> >>>> The availability of this instruction is determined by byte 134, bit 0
> >>>> of the Read Info block. This coincidentally expands into the space used
> >>>
> >>> "SCLP Read Info"
> >>>
> >>>> for CPU entries by taking away one byte, which means VMs running with
> >>>> the diag318 capability will not be able to retrieve information regarding
> >>>> the 248th CPU. This will not effect performance, and VMs can still be
> >>>> ran with 248 CPUs.
> >>>
> >>> Are there other ways in which that might affect guests? I assume Linux
> >>> can deal with it? Is it ok architecture-wise?
> >>>
> >>> In any case, should go into the patch description :)
> >>>
> >>
> >> Same as above. I'll try to provide more information regarding what happens
> >> here in my next reply.
> >
> > I think you can lift some stuff from the cover letter.
> >
>
> Here's what I found out:
>
> Each CPU entry holds info regarding the CPU's address / ID as well as an
> indication of the availability of certain CPU features. With these patches,
> we lose a CPU entry for one CPU (essentially what would be the CPU at the
> tail-end of the list). This CPU exists, but is essentially in limbo... the
> machine cannot access any information regarding it.
s/machine/guest/ ?
>
> So, a VM can run with the original N max CPUs, but in reality we can only
> utilize n-1.
s/we/the guest/ ?
With those changes, it makes sense to put your explanations into the
patch description (for later reference).
>
> >>
> >>>>
>
> [...]
>
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 2/2] s390: diagnose 318 info reset and migration support
2020-01-27 17:35 ` Cornelia Huck
2020-01-27 23:05 ` Collin Walling
@ 2020-01-28 14:37 ` Collin Walling
2020-01-28 15:08 ` Cornelia Huck
1 sibling, 1 reply; 23+ messages in thread
From: Collin Walling @ 2020-01-28 14:37 UTC (permalink / raw)
To: Cornelia Huck; +Cc: borntraeger, qemu-s390x, david, qemu-devel, rth
On 1/27/20 12:35 PM, Cornelia Huck wrote:
> On Mon, 27 Jan 2020 11:39:02 -0500
> Collin Walling <walling@linux.ibm.com> wrote:
>
>> On 1/27/20 6:47 AM, Cornelia Huck wrote:
>>> On Fri, 24 Jan 2020 17:14:04 -0500
>>> Collin Walling <walling@linux.ibm.com> wrote:
>>>
>>>> DIAGNOSE 0x318 (diag318) is a privileged s390x instruction that must
>>>> be intercepted by SIE and handled via KVM. Let's introduce some
>>>> functions to communicate between QEMU and KVM via ioctls. These
>>>> will be used to get/set the diag318 information.
>>>
>>> Do you want to give a hint what diag 318 actually does?
>>>
>>
>> For the sake of completeness, I'll have to get back to you on this.
>>
The DIAGNOSE 318 instruction allows the guest to store diagnostic data
that is collected by the firmware in the case of hardware/firmware
service events. The instruction is invoked in the Linux kernel and
intercepted in KVM. QEMU needs to collect this data for migration
so that this data is consistent on the destination host.
Perhaps I should add this to the patch description as well? :)
[...]
--
Respectfully,
- Collin Walling
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 2/2] s390: diagnose 318 info reset and migration support
2020-01-28 11:24 ` Cornelia Huck
@ 2020-01-28 14:38 ` Collin Walling
0 siblings, 0 replies; 23+ messages in thread
From: Collin Walling @ 2020-01-28 14:38 UTC (permalink / raw)
To: Cornelia Huck; +Cc: borntraeger, qemu-s390x, rth, qemu-devel, david
On 1/28/20 6:24 AM, Cornelia Huck wrote:
> On Mon, 27 Jan 2020 18:05:36 -0500
> Collin Walling <walling@linux.ibm.com> wrote:
>
>> On 1/27/20 12:35 PM, Cornelia Huck wrote:
>>> On Mon, 27 Jan 2020 11:39:02 -0500
>>> Collin Walling <walling@linux.ibm.com> wrote:
>>>
>>>> On 1/27/20 6:47 AM, Cornelia Huck wrote:
>>>>> On Fri, 24 Jan 2020 17:14:04 -0500
>>>>> Collin Walling <walling@linux.ibm.com> wrote:
>>>>>
>>
>> [...]
>>
>>>>>>
>>>>>> The availability of this instruction is determined by byte 134, bit 0
>>>>>> of the Read Info block. This coincidentally expands into the space used
>>>>>
>>>>> "SCLP Read Info"
>>>>>
>>>>>> for CPU entries by taking away one byte, which means VMs running with
>>>>>> the diag318 capability will not be able to retrieve information regarding
>>>>>> the 248th CPU. This will not effect performance, and VMs can still be
>>>>>> ran with 248 CPUs.
>>>>>
>>>>> Are there other ways in which that might affect guests? I assume Linux
>>>>> can deal with it? Is it ok architecture-wise?
>>>>>
>>>>> In any case, should go into the patch description :)
>>>>>
>>>>
>>>> Same as above. I'll try to provide more information regarding what happens
>>>> here in my next reply.
>>>
>>> I think you can lift some stuff from the cover letter.
>>>
>>
>> Here's what I found out:
>>
>> Each CPU entry holds info regarding the CPU's address / ID as well as an
>> indication of the availability of certain CPU features. With these patches,
>> we lose a CPU entry for one CPU (essentially what would be the CPU at the
>> tail-end of the list). This CPU exists, but is essentially in limbo... the
>> machine cannot access any information regarding it.
>
> s/machine/guest/ ?
>
Correct.
>>
>> So, a VM can run with the original N max CPUs, but in reality we can only
>> utilize n-1.
>
> s/we/the guest/ ?
>
Correct again.
> With those changes, it makes sense to put your explanations into the
> patch description (for later reference).
>
>>
>>>>
>>>>>>
>>
>> [...]
>>
>>
>
>
--
Respectfully,
- Collin Walling
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 2/2] s390: diagnose 318 info reset and migration support
2020-01-28 14:37 ` Collin Walling
@ 2020-01-28 15:08 ` Cornelia Huck
0 siblings, 0 replies; 23+ messages in thread
From: Cornelia Huck @ 2020-01-28 15:08 UTC (permalink / raw)
To: Collin Walling; +Cc: borntraeger, qemu-s390x, david, qemu-devel, rth
On Tue, 28 Jan 2020 09:37:46 -0500
Collin Walling <walling@linux.ibm.com> wrote:
> On 1/27/20 12:35 PM, Cornelia Huck wrote:
> > On Mon, 27 Jan 2020 11:39:02 -0500
> > Collin Walling <walling@linux.ibm.com> wrote:
> >
> >> On 1/27/20 6:47 AM, Cornelia Huck wrote:
> >>> On Fri, 24 Jan 2020 17:14:04 -0500
> >>> Collin Walling <walling@linux.ibm.com> wrote:
> >>>
> >>>> DIAGNOSE 0x318 (diag318) is a privileged s390x instruction that must
> >>>> be intercepted by SIE and handled via KVM. Let's introduce some
> >>>> functions to communicate between QEMU and KVM via ioctls. These
> >>>> will be used to get/set the diag318 information.
> >>>
> >>> Do you want to give a hint what diag 318 actually does?
> >>>
> >>
> >> For the sake of completeness, I'll have to get back to you on this.
> >>
>
> The DIAGNOSE 318 instruction allows the guest to store diagnostic data
> that is collected by the firmware in the case of hardware/firmware
> service events. The instruction is invoked in the Linux kernel and
> intercepted in KVM. QEMU needs to collect this data for migration
> so that this data is consistent on the destination host.
>
> Perhaps I should add this to the patch description as well? :)
Yes, please :) It will make things easier for any unfortunate soul
wanting to find out what this is about in the future.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 0/2] Use DIAG318 to set Control Program Name & Version Codes
2020-01-24 22:14 [PATCH v6 0/2] Use DIAG318 to set Control Program Name & Version Codes Collin Walling
` (2 preceding siblings ...)
2020-01-24 22:22 ` [PATCH v6 0/2] Use DIAG318 to set Control Program Name & Version Codes no-reply
@ 2020-03-17 21:34 ` Collin Walling
3 siblings, 0 replies; 23+ messages in thread
From: Collin Walling @ 2020-03-17 21:34 UTC (permalink / raw)
To: qemu-s390x, qemu-devel; +Cc: borntraeger, rth, cohuck, frankja, david
Please note: a new patch is in the works to extend the SCCB for the SCLP
response for RSCPI. This will help alleviate the issue of losing space
for CPU
entries. The appropriate patch will be introduced at the beginning of this
series once it is ready for upstream.
Thanks for your patience and understanding,
- Collin
On 1/24/20 5:14 PM, Collin Walling wrote:
> Changes from v5 -> v6
>
> Migration and DeviceObject Code:
> - load/save/needed functions now check if kvm_enabled before calling
> kvm_get/set and has_feat (respectively)
>
> QEMU->KVM Code:
> - added kvm_s390_* stubs for get/set functions for TCG compilation
>
> VCPU Discussion:
> - calculate the maximum allowed cpu entries by taking the SCCB size,
> subtracting the offset where the CPU entries begin, then dividing
> by the size of a CPU Entry struct
> - if the number of CPU entries exceeds the maximum allowed entries,
> print a warning and break out of the loop
> - no longer imposing a reduced CPU max
>
> Last post: https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg05535.html
>
> The data associated with DIAGNOSE 0x318 helps to identify the underlying
> hypervisor level (pre-determined by an internal set of codes), as well as the
> guest environment (such as Linux, z/VM, etc). These patches, in tandem with
> KVM, allow this instruction to be enabled at the guest level, and also to
> enable migration of this data.
>
> The DIAGNOSE 0x318 instruction is a privileged instruction that is executed by
> the Linux kernel once and only once during setup (IPL). This requires
> interception by KVM to handle the instruction call safely. The instruction sets
> an 8-byte value corresponding to the environment the control program (i.e.
> guest) is running with, as well as what hypervisor it is running on.
>
> An update to the analogous KVM patches associated with this patchset are
> forthcoming and I will provide a link to the post as a reply to this chain.
>
> Guest support for the diag 318 instruction is accomplished by implementing a
> device class, a cpu model feature, and adjusting the Read Info struct. The Read
> Info struct adjustment coincidentally reduces the maximum number of VCPUs we
> can have for a single guest by one.
>
> The instruction is determined by a Read Info byte 134 bit 0. This new byte
> expands into the space of the Read Info SCCB, which also contains CPU entries
> at the tail-end of this block of data. Due to this expansion, we lose space for
> one CPU entry.
>
> A guest can still run safely with the original 248 maximum CPUs, but will lose
> access to the 248th CPU entry, meaning that the hypervisor will be unable to
> retrieve any information regarding that CPU (weather this means the guest
> will actually run with 247 CPUs when 248 are specified is uncertain to me, but
> the guest operates just fine on my end).
>
> A device class is used for this instruction in order to streamline the
> migration and reset of the DIAG 318 related data.
>
> A CPU model feature is added for this instruction, appropriately named diag318,
> and is available starting with the zEC12 full model, though as long as KVM can
> support emulation of this instruction, we can theoretically enable it for _any_
> CPU model. It is recommended to explicitly enable the feature via
> -cpu ...,diag318=on (or via libvirt feature XML).
>
> Collin L. Walling (2):
> s390/kvm: header sync for diag318
> s390: diagnose 318 info reset and migration support
>
> hw/s390x/Makefile.objs | 1 +
> hw/s390x/diag318.c | 85 +++++++++++++++++++++++++++++++++++++
> hw/s390x/diag318.h | 40 +++++++++++++++++
> hw/s390x/s390-virtio-ccw.c | 17 ++++++++
> hw/s390x/sclp.c | 13 ++++++
> include/hw/s390x/sclp.h | 2 +
> linux-headers/asm-s390/kvm.h | 4 ++
> target/s390x/cpu_features.h | 1 +
> target/s390x/cpu_features_def.inc.h | 3 ++
> target/s390x/gen-features.c | 1 +
> target/s390x/kvm-stub.c | 10 +++++
> target/s390x/kvm.c | 29 +++++++++++++
> target/s390x/kvm_s390x.h | 2 +
> 13 files changed, 208 insertions(+)
> create mode 100644 hw/s390x/diag318.c
> create mode 100644 hw/s390x/diag318.h
>
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2020-03-17 21:36 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-24 22:14 [PATCH v6 0/2] Use DIAG318 to set Control Program Name & Version Codes Collin Walling
2020-01-24 22:14 ` [PATCH v6 1/2] s390/kvm: header sync for diag318 Collin Walling
2020-01-24 22:14 ` [PATCH v6 2/2] s390: diagnose 318 info reset and migration support Collin Walling
2020-01-27 11:20 ` David Hildenbrand
2020-01-27 15:57 ` Collin Walling
2020-01-27 17:09 ` David Hildenbrand
2020-01-27 17:29 ` Cornelia Huck
2020-01-27 17:55 ` David Hildenbrand
2020-01-27 18:21 ` Collin Walling
2020-01-27 18:52 ` Collin Walling
2020-01-28 11:19 ` Cornelia Huck
2020-01-27 11:36 ` Thomas Huth
2020-01-27 15:58 ` Collin Walling
2020-01-27 11:47 ` Cornelia Huck
2020-01-27 16:39 ` Collin Walling
2020-01-27 17:35 ` Cornelia Huck
2020-01-27 23:05 ` Collin Walling
2020-01-28 11:24 ` Cornelia Huck
2020-01-28 14:38 ` Collin Walling
2020-01-28 14:37 ` Collin Walling
2020-01-28 15:08 ` Cornelia Huck
2020-01-24 22:22 ` [PATCH v6 0/2] Use DIAG318 to set Control Program Name & Version Codes no-reply
2020-03-17 21:34 ` Collin Walling
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.