Use DIAG318 to set Control Program Name & Version Codes

hw/s390x/diag318.c new file +18 -0

+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 = {

(quoted diff) +2 -0

+
+type_init(s390_diag318_register_types)

hw/s390x/diag318.h new file +40 -0

new file mode 100644
index 0000000..06d9f67
@@ -0,0 +1,40 @@
+/*
+ * DIAGNOSE 0x318 functions for reset and migration
+ *
+ * Copyright IBM, Corp. 2019
+ *
+ * Authors:
+ *  Collin Walling <[email protected]>
+ *
+ * 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

hw/s390x/s390-virtio-ccw.c +16 -0

index e0e2813..d5b7a33 100644
@@ -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,

hw/s390x/s390-virtio-ccw.c +0 -0

 static inline void s390_machine_initfn(Object *obj)
 {
     object_property_add_bool(obj, "aes-key-wrap",

hw/s390x/sclp.c +3 -0

index f57ce7b..636348c 100644
@@ -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;

[...]

hw/s390x/diag318.c line 4

Quoted 5 lines

hw/s390x/diag318.c new file +4 -0

@@ -0,0 +1,85 @@
+/*
+ * DIAGNOSE 0x318 functions for reset and migration
+ *
+ * Copyright IBM, Corp. 2019

Should be 2020 now.

[...]

Quoted 18 lines

hw/s390x/diag318.c new file +18 -0

+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 = {

hw/s390x/diag318.c line 83

Quoted 10 lines

hw/s390x/diag318.c new file +10 -0

+    .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

Quoted 98 lines

(quoted diff) +2 -0

+
+type_init(s390_diag318_register_types)

hw/s390x/diag318.h new file +40 -0

new file mode 100644
index 0000000..06d9f67
@@ -0,0 +1,40 @@
+/*
+ * DIAGNOSE 0x318 functions for reset and migration
+ *
+ * Copyright IBM, Corp. 2019
+ *
+ * Authors:
+ *  Collin Walling <[email protected]>
+ *
+ * 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

hw/s390x/s390-virtio-ccw.c +16 -0

index e0e2813..d5b7a33 100644
@@ -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,

hw/s390x/s390-virtio-ccw.c static void machine_set_loadparm(Object *obj, const char *val, Error **errp) · line 585

Quoted 4 lines

hw/s390x/s390-virtio-ccw.c +1 -0

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

Quoted 30 lines

hw/s390x/s390-virtio-ccw.c +0 -0

 static inline void s390_machine_initfn(Object *obj)
 {
     object_property_add_bool(obj, "aes-key-wrap",

hw/s390x/sclp.c +3 -0

index f57ce7b..636348c 100644
@@ -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;
 

hw/s390x/sclp.c static void prepare_cpu_entries(SCLPDevice *sclp, CPUEntry *entry, int *count) · line 52

Quoted 8 lines

hw/s390x/sclp.c +6 -0

+    /* 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!

Quoted 32 lines

Quoted 18 lines

hw/s390x/diag318.c new file +18 -0

+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 = {

hw/s390x/diag318.c line 83

Quoted 10 lines

hw/s390x/diag318.c new file +10 -0

+    .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?

Quoted 2 lines

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

How about we introduce a union in the ReadInfo struct. Something like:

    union {
        uint8_t  byte_134;
        struct CPUEntry entries[0];
    } x;

Quoted 32 lines

Quoted 32 lines

Quoted 18 lines

hw/s390x/diag318.c new file +18 -0

+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 = {

hw/s390x/diag318.c line 83

Quoted 10 lines

hw/s390x/diag318.c new file +10 -0

+    .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?

Quoted 6 lines

Quoted 2 lines

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

Quoted 4 lines

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?

[...]

Quoted 7 lines

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.

Quoted 46 lines · On Mon, 27 Jan 2020 18:09:11 +0100 David Hildenbrand <[email protected]> wrote:

Quoted 32 lines

Quoted 32 lines

Quoted 18 lines

hw/s390x/diag318.c new file +18 -0

+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 = {

hw/s390x/diag318.c line 83

Quoted 10 lines

hw/s390x/diag318.c new file +10 -0

+    .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?

Quoted 6 lines

Quoted 2 lines

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

Quoted 4 lines

Should we store this in the S390CcwMachineState? Or perhaps create a generic S390MachineState for information that needs to be stored once and migrated once?

Quoted 5 lines

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

Quoted 53 lines · Cornelia Huck

Quoted 50 lines · On Mon, 27 Jan 2020 18:09:11 +0100 David Hildenbrand <[email protected]> wrote:

Quoted 32 lines

Quoted 32 lines

Quoted 18 lines

hw/s390x/diag318.c new file +18 -0

+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 = {

hw/s390x/diag318.c line 83

Quoted 10 lines

hw/s390x/diag318.c new file +10 -0

+    .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?

Quoted 6 lines

Quoted 2 lines

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

Quoted 4 lines

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().

Quoted 4 lines

Quoted 1 line

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

Quoted 54 lines · David Hildenbrand

Quoted 53 lines · Cornelia Huck

Quoted 50 lines · On Mon, 27 Jan 2020 18:09:11 +0100 David Hildenbrand <[email protected]> wrote:

Quoted 32 lines

Quoted 32 lines

Quoted 18 lines

(quoted diff) +18 -0

+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 = {

Review

Quoted 10 lines

(quoted diff) +10 -0

+    .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?

Quoted 6 lines

Quoted 2 lines

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

Quoted 4 lines

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().

Quoted 9 lines

Quoted 4 lines

Quoted 1 line

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

Quoted 19 lines · Collin Walling

Quoted 8 lines · David Hildenbrand

Quoted 7 lines · Cornelia Huck

Quoted 4 lines · On Mon, 27 Jan 2020 18:09:11 +0100 David Hildenbrand <[email protected]> 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().

Quoted 9 lines

Quoted 4 lines

Quoted 1 line

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

Quoted 6 lines

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.

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 <[email protected]>

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

hw/s390x/Makefile.objs +1 -0

index e02ed80..93621dc 100644
@@ -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

hw/s390x/diag318.c new file +0 -0

new file mode 100644
index 0000000..2d30bb2

hw/s390x/diag318.c new file +8 -0

+ * Authors:
+ *  Collin Walling <[email protected]>
+ *
+ * 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"

hw/s390x/diag318.c new file +15 -0

+    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,

hw/s390x/diag318.c new file +26 -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)

hw/s390x/diag318.h new file +0 -0

new file mode 100644
index 0000000..06d9f67

hw/s390x/diag318.h new file +23 -0

+ * Authors:
+ *  Collin Walling <[email protected]>
+ *
+ * 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;
+

hw/s390x/diag318.h new file +1 -0

+#endif /* HW_DIAG318_H */
\ No newline at end of file

hw/s390x/s390-virtio-ccw.c +2 -0

index e0e2813..d5b7a33 100644
@@ -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)

Quoted 10 lines · Collin Walling

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/ ?

Quoted 1 line

ran with 248 CPUs.

s/ran/run/ ?

Quoted 39 lines

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 <[email protected]>

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

hw/s390x/Makefile.objs +1 -0

index e02ed80..93621dc 100644
@@ -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

hw/s390x/diag318.c new file +0 -0

new file mode 100644
index 0000000..2d30bb2

hw/s390x/diag318.c line 4

Quoted 4 lines

hw/s390x/diag318.c new file +4 -0

@@ -0,0 +1,85 @@
+/*
+ * DIAGNOSE 0x318 functions for reset and migration
+ *
+ * Copyright IBM, Corp. 2019

Bump to 2020 ?

Quoted 8 lines

hw/s390x/diag318.c new file +8 -0

+ * Authors:
+ *  Collin Walling <[email protected]>
+ *
+ * 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"

hw/s390x/diag318.c line 23

Quoted 10 lines

hw/s390x/diag318.c new file +10 -0

+#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.

hw/s390x/diag318.c line 33

Quoted 9 lines

hw/s390x/diag318.c new file +9 -0

+    return 0;
+}
+
+static int diag318_pre_save(void *opaque)
+{
+    DIAG318State *d = opaque;
+
+    if (kvm_enabled())
+        kvm_s390_get_diag318_info(&d->info);

dito

Quoted 15 lines

hw/s390x/diag318.c new file +15 -0

+    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,

hw/s390x/diag318.c line 59

Quoted 10 lines

hw/s390x/diag318.c new file +10 -0

+    .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

Quoted 32 lines

hw/s390x/diag318.c new file +26 -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)

hw/s390x/diag318.h new file +0 -0

new file mode 100644
index 0000000..06d9f67

hw/s390x/diag318.h line 4

Quoted 4 lines

hw/s390x/diag318.h new file +4 -0

@@ -0,0 +1,40 @@
+/*
+ * DIAGNOSE 0x318 functions for reset and migration
+ *
+ * Copyright IBM, Corp. 2019

2020 ?

Quoted 23 lines

hw/s390x/diag318.h new file +23 -0

+ * Authors:
+ *  Collin Walling <[email protected]>
+ *
+ * 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;
+

hw/s390x/diag318.h line 38

Quoted 10 lines

hw/s390x/diag318.h new file +10 -0

+    /*< public >*/
+    uint64_t info;
+} DIAG318State;
+
+typedef struct DIAG318Class {
+    /*< private >*/
+    DeviceClass parent_class;
+
+    /*< public >*/
+} DIAG318Class;

You don't use DIAG318Class anywhere. Please drop it.

Quoted 23 lines

hw/s390x/diag318.h new file +1 -0

+#endif /* HW_DIAG318_H */
\ No newline at end of file

hw/s390x/s390-virtio-ccw.c +2 -0

index e0e2813..d5b7a33 100644
@@ -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)

hw/s390x/s390-virtio-ccw.c static void s390_create_sclpconsole(const char *type, Chardev *chardev) · line 244

Quoted 6 lines

hw/s390x/s390-virtio-ccw.c +3 -0

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

hw/s390x/s390-virtio-ccw.c static void s390_create_sclpconsole(const char *type, Chardev *chardev) · line 252

Quoted 8 lines

hw/s390x/s390-virtio-ccw.c +8 -0

+    DeviceState *dev = DEVICE(new);
+
+    object_property_add_child(qdev_get_machine(), TYPE_S390_DIAG318,
+                              new, NULL);
+    object_unref(new);
+    qdev_init_nofail(dev);
+}
+

Thomas

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 <[email protected]>

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

hw/s390x/diag318.c new file +8 -0

+ *
+ * Authors:
+ *  Collin Walling <[email protected]>
+ *
+ * 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.
+ */
+

hw/s390x/diag318.c new file +8 -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,

hw/s390x/diag318.c new file +27 -0

+        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)

hw/s390x/diag318.h new file +0 -0

new file mode 100644
index 0000000..06d9f67

hw/s390x/diag318.h new file +27 -0

+ *
+ * Authors:
+ *  Collin Walling <[email protected]>
+ *
+ * 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;

hw/s390x/s390-virtio-ccw.c +13 -0

index e0e2813..d5b7a33 100644
@@ -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;

hw/s390x/s390-virtio-ccw.c +0 -0

 static inline void s390_machine_initfn(Object *obj)
 {
     object_property_add_bool(obj, "aes-key-wrap",

hw/s390x/sclp.c +3 -0

index f57ce7b..636348c 100644
@@ -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;

hw/s390x/sclp.c +4 -0

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

Quoted 4 lines · On Fri, 24 Jan 2020 17:14:04 -0500 Collin Walling <[email protected]> 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?

Quoted 3 lines

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"

Quoted 4 lines

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 :)

Quoted 6 lines

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?

Quoted 19 lines

Signed-off-by Collin Walling <[email protected]>

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

(...)

hw/s390x/diag318.c line 4

Quoted 10 lines

hw/s390x/diag318.c new file +4 -0

new file mode 100644
index 0000000..2d30bb2
@@ -0,0 +1,85 @@
+/*
+ * DIAGNOSE 0x318 functions for reset and migration
+ *
+ * Copyright IBM, Corp. 2019

Happy new year?

Quoted 8 lines

hw/s390x/diag318.c new file +8 -0

+ *
+ * Authors:
+ *  Collin Walling <[email protected]>
+ *
+ * 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.
+ */
+

hw/s390x/diag318.c line 22

Quoted 10 lines

hw/s390x/diag318.c new file +10 -0

+#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.

hw/s390x/diag318.c line 32

Quoted 10 lines

hw/s390x/diag318.c new file +10 -0

+        kvm_s390_set_diag318_info(d->info);
+
+    return 0;
+}
+
+static int diag318_pre_save(void *opaque)
+{
+    DIAG318State *d = opaque;
+
+    if (kvm_enabled())

braces

hw/s390x/diag318.c line 40

Quoted 8 lines

hw/s390x/diag318.c new file +8 -0

+        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.

Quoted 8 lines

hw/s390x/diag318.c new file +8 -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,

hw/s390x/diag318.c line 58

Quoted 10 lines

hw/s390x/diag318.c new file +10 -0

+    .needed = diag318_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT64(info, DIAG318State),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static void s390_diag318_reset(DeviceState *dev)
+{
+    if (kvm_enabled())

braces

Quoted 33 lines

hw/s390x/diag318.c new file +27 -0

+        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)

hw/s390x/diag318.h new file +0 -0

new file mode 100644
index 0000000..06d9f67

hw/s390x/diag318.h line 4

Quoted 4 lines

hw/s390x/diag318.h new file +4 -0

@@ -0,0 +1,40 @@
+/*
+ * DIAGNOSE 0x318 functions for reset and migration
+ *
+ * Copyright IBM, Corp. 2019

Again, the year needs an update.

Quoted 27 lines

hw/s390x/diag318.h new file +27 -0

+ *
+ * Authors:
+ *  Collin Walling <[email protected]>
+ *
+ * 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;

hw/s390x/diag318.h line 40

Quoted 10 lines

hw/s390x/diag318.h new file +9 -0

+
+typedef struct DIAG318Class {
+    /*< private >*/
+    DeviceClass parent_class;
+
+    /*< public >*/
+} DIAG318Class;
+
+#endif /* HW_DIAG318_H */
\ No newline at end of file

And please add a newline :)

Quoted 39 lines

hw/s390x/s390-virtio-ccw.c +13 -0

index e0e2813..d5b7a33 100644
@@ -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;

hw/s390x/s390-virtio-ccw.c static void ccw_init(MachineState *machine) · line 312

Quoted 6 lines

hw/s390x/s390-virtio-ccw.c +3 -0

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

hw/s390x/s390-virtio-ccw.c static void machine_set_loadparm(Object *obj, const char *val, Error **errp) · line 585

Quoted 8 lines

hw/s390x/s390-virtio-ccw.c +1 -0

 }
 
 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 :)

Quoted 30 lines

hw/s390x/s390-virtio-ccw.c +0 -0

 static inline void s390_machine_initfn(Object *obj)
 {
     object_property_add_bool(obj, "aes-key-wrap",

hw/s390x/sclp.c +3 -0

index f57ce7b..636348c 100644
@@ -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;
 

hw/s390x/sclp.c static void prepare_cpu_entries(SCLPDevice *sclp, CPUEntry *entry, int *count) · line 52

Quoted 8 lines

hw/s390x/sclp.c +6 -0

+    /* 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?

Quoted 9 lines

hw/s390x/sclp.c +2 -0

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

Quoted 5 lines

hw/s390x/sclp.c +2 -0

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

(...)

Quoted 10 lines · Cornelia Huck

Quoted 4 lines · On Fri, 24 Jan 2020 17:14:04 -0500 Collin Walling <[email protected]> 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.

Quoted 11 lines

Quoted 3 lines

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"

Quoted 4 lines

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.

Quoted 5 lines

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.

Quoted 6 lines

Quoted 6 lines

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.

Quoted 4 lines

Quoted 1 line

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..."

Quoted 22 lines

Quoted 20 lines

Signed-off-by Collin Walling <[email protected]>

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

(...)

Quoted 8 lines

hw/s390x/diag318.c line 40

Quoted 3 lines

hw/s390x/diag318.c new file +3 -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.

Quoted 10 lines

hw/s390x/s390-virtio-ccw.c static void ccw_init(MachineState *machine) · line 312

Quoted 7 lines

hw/s390x/s390-virtio-ccw.c +3 -0

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

Quoted 3 lines

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).

Quoted 7 lines

Quoted 7 lines

hw/s390x/sclp.c +1 -0

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

Quoted 15 lines

hw/s390x/sclp.c static void prepare_cpu_entries(SCLPDevice *sclp, CPUEntry *entry, int *count) · line 52

Quoted 8 lines

hw/s390x/sclp.c +6 -0

+    /* 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...

Quoted 5 lines

Quoted 5 lines

hw/s390x/sclp.c +2 -0

+            break;
+        }
         if (!ms->possible_cpus->cpus[i].cpu) {
             continue;
         }

Quoted 4 lines · On Mon, 27 Jan 2020 11:39:02 -0500 Collin Walling <[email protected]> wrote:

Quoted 3 lines · Cornelia Huck

On Fri, 24 Jan 2020 17:14:04 -0500
Collin Walling <[email protected]> wrote:

Quoted 14 lines

Quoted 14 lines

Quoted 3 lines

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"

Quoted 4 lines

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?

Quoted 5 lines

Quoted 2 lines

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.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 7 lines · Cornelia Huck

Quoted 4 lines · On Mon, 27 Jan 2020 11:39:02 -0500 Collin Walling <[email protected]> wrote:

Quoted 3 lines · Cornelia Huck

On Fri, 24 Jan 2020 17:14:04 -0500
Collin Walling <[email protected]> wrote:

[...]

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

Quoted 3 lines

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"

Quoted 4 lines

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?

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

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.

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

[...]

Quoted 36 lines · On Mon, 27 Jan 2020 18:05:36 -0500 Collin Walling <[email protected]> wrote:

Quoted 7 lines · Cornelia Huck

Quoted 4 lines · On Mon, 27 Jan 2020 11:39:02 -0500 Collin Walling <[email protected]> wrote:

Quoted 3 lines · Cornelia Huck

On Fri, 24 Jan 2020 17:14:04 -0500
Collin Walling <[email protected]> wrote:

[...]

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

Quoted 3 lines

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"

Quoted 4 lines

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?

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

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:

Quoted 5 lines

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/ ?

Quoted 3 lines

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).

Quoted 7 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

[...]

Quoted 9 lines · On Mon, 27 Jan 2020 11:39:02 -0500 Collin Walling <[email protected]> wrote:

Quoted 8 lines · Cornelia Huck

Quoted 4 lines · On Fri, 24 Jan 2020 17:14:04 -0500 Collin Walling <[email protected]> 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.

Quoted 5 lines

Quoted 2 lines

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.

Quoted 12 lines · Cornelia Huck

Quoted 9 lines · On Mon, 27 Jan 2020 11:39:02 -0500 Collin Walling <[email protected]> wrote:

Quoted 8 lines · Cornelia Huck

Quoted 4 lines · On Fri, 24 Jan 2020 17:14:04 -0500 Collin Walling <[email protected]> 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.

Quoted 5 lines

Quoted 5 lines

Quoted 2 lines

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? :)

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