Re: [Qemu-devel] [PATCH v4] s390: diagnose 318 info reset and migration support

[David Hildenbrand]

On 13.05.19 09:46, David Hildenbrand wrote:
> On 02.05.19 00:31, 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 related information (also known
>> as the "Control Program Code" or "CPC"), as well as check the system
>> if KVM supports handling this instruction.
>>
>> 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, which means VMs running with the diag318 capability
>> will have a reduced maximum CPU count. To alleviate this, let's calculate
>> the actual max CPU entry space by subtracting the size of Read Info from
>> the SCCB size then dividing that number by the size of a CPU entry. If
>> this value is less than the value denoted by S390_MAX_CPUS, then let's
>> reduce the max cpus for s390 from 248 to 240 in an effort to anticipate
>> this potentially happening again in the future.
>>
>> 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 reset on during modified clear and load normal.
>>
>> Signed-off-by: Collin Walling <address@hidden>
>> ---
>>  hw/s390x/Makefile.objs       |   1 +
>>  hw/s390x/diag318.c           | 100 
>> +++++++++++++++++++++++++++++++++++++++++++
>>  hw/s390x/diag318.h           |  39 +++++++++++++++++
>>  hw/s390x/s390-virtio-ccw.c   |  23 ++++++++++
>>  hw/s390x/sclp.c              |   5 +++
>>  include/hw/s390x/sclp.h      |   2 +
>>  linux-headers/asm-s390/kvm.h |   4 ++
>>  target/s390x/kvm-stub.c      |  15 +++++++
>>  target/s390x/kvm.c           |  32 ++++++++++++++
>>  target/s390x/kvm_s390x.h     |   3 ++
>>  10 files changed, 224 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..94b44da
>> --- /dev/null
>> +++ b/hw/s390x/diag318.c
>> @@ -0,0 +1,100 @@
>> +/*
>> + * DIAGNOSE 0x318 functions for reset and migration
>> + *
>> + * Copyright IBM, Corp. 2019
>> + *
>> + * Authors:
>> + *  Collin Walling <address@hidden>
>> + *
>> + * 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;
>> +
>> +    kvm_s390_set_cpc(d->cpc);
>> +
>> +    /* It is not necessary to retain a copy of the cpc after migration. */
>> +    d->cpc = 0;
>> +
>> +    return 0;
>> +}
>> +
>> +static int diag318_pre_save(void *opaque)
>> +{
>> +    DIAG318State *d = opaque;
>> +
>> +    kvm_s390_get_cpc(&d->cpc);
>> +    return 0;
>> +}
>> +
>> +static bool diag318_needed(void *opaque)
>> +{
>> +    DIAG318State *d = opaque;
>> +
>> +    return d->enabled;
>> +}
>> +
>> +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(cpc, DIAG318State),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>> +
>> +static void s390_diag318_realize(DeviceState *dev, Error **errp)
>> +{
>> +    DIAG318State *d = DIAG318(dev);
>> +
>> +    if (kvm_s390_has_diag318()) {
>> +        d->enabled = true;
>> +    }
>> +}
>> +
>> +static void s390_diag318_reset(DeviceState *dev)
>> +{
>> +    DIAG318State *d = DIAG318(dev);
>> +
>> +    if (d->enabled) {
>> +        kvm_s390_set_cpc(0);
>> +    }
>> +}
>> +
>> +static void s390_diag318_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +
>> +    dc->realize = s390_diag318_realize;
>> +    dc->reset = s390_diag318_reset;
>> +    dc->vmsd = &vmstate_diag318;
>> +    dc->hotpluggable = false;
>> +    /* Reason: Set automatically during IPL */
>> +    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..c154b0a
>> --- /dev/null
>> +++ b/hw/s390x/diag318.h
>> @@ -0,0 +1,39 @@
>> +/*
>> + * DIAGNOSE 0x318 functions for reset and migration
>> + *
>> + * Copyright IBM, Corp. 2019
>> + *
>> + * Authors:
>> + *  Collin Walling <address@hidden>
>> + *
>> + * 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 DIAG318_H
>> + #define DIAG318_H
>> +
>> +#include "qemu/osdep.h"
>> +#include "hw/qdev.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 cpc;
>> +    bool enabled;
>> +} DIAG318State;
>> +
>> +typedef struct DIAG318Class {
>> +    /*< private >*/
>> +    DeviceClass parent_class;
>> +
>> +    /*< public >*/
>> +} DIAG318Class;
>> +
>> +#endif /* DIAG318_H */
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index d11069b..44a424b 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -36,6 +36,7 @@
>>  #include "cpu_models.h"
>>  #include "hw/nmi.h"
>>  #include "hw/s390x/tod.h"
>> +#include "hw/s390x/diag318.h"
>>  
>>  S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
>>  {
>> @@ -92,6 +93,7 @@ static const char *const reset_dev_types[] = {
>>      "s390-sclp-event-facility",
>>      "s390-flic",
>>      "diag288",
>> +    TYPE_S390_DIAG318,
>>  };
>>  
>>  static void subsystem_reset(void)
>> @@ -246,6 +248,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;
>> @@ -302,6 +315,8 @@ static void ccw_init(MachineState *machine)
>>  
>>      /* init the TOD clock */
>>      s390_init_tod();
>> +
>> +    s390_init_diag318();
>>  }
>>  
>>  static void s390_cpu_plug(HotplugHandler *hotplug_dev,
>> @@ -570,6 +585,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",
>> @@ -652,6 +668,13 @@ static void 
>> ccw_machine_4_0_instance_options(MachineState *machine)
>>  
>>  static void ccw_machine_4_0_class_options(MachineClass *mc)
>>  {
>> +    /*
>> +     * Read Info might reveal more bytes used to detect facilities, thus
>> +     * reducing the number of CPU entries. Let's reduce the max CPUs by
>> +     * an arbitrary number in effort to anticipate future facility bytes.
>> +     */
>> +    if ((SCCB_SIZE - sizeof(ReadInfo)) / sizeof(CPUEntry) < S390_MAX_CPUS)
>> +        mc->max_cpus = S390_MAX_CPUS - 8;
> 
> This is too complicated, just set it always to 240.
> 
> However, I am still not sure how to best handle this scenario. One
> solution is
> 
> 1. Set it statically to 240 for machine > 4.1
> 2. Keep the old machines unmodifed
> 3. Don't indicate the CPU feature for machines <= 4.0
> 
> #3 is the problematic part, as it mixes host CPU features and machines.
> Bad. The host CPU model should always look the same on all machines. I
> don't like this.
> 

FWIW, #3 is only an issue when modeling it via the CPU model, like
Christian suggested.

I suggest the following

1. Set the max #cpus for 4.1 to 240 (already done)
2. Keep it for the other machines unmodified (as suggested by Thomas)
3. Create the layout of the SCCB depending on the machine type (to be done)

If we want to model diag318 via a CPU feature (which makes sense for
migration):

4. Disable diag318 with a warning if used with a machine < 4.1

-- 

Thanks,

David / dhildenb