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

[Collin Walling <walling@linux.ibm.com>]

On 6/26/19 8:33 AM, Cornelia Huck wrote:
> On Tue, 25 Jun 2019 11:17:09 -0400
> 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.
>>
>> 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. Let's reduce the maximum CPU
>> count from 248 to 247.
>>
>> 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              | 80 +++++++++++++++++++++++++++++++++++++++++
>>   hw/s390x/diag318.h              | 38 ++++++++++++++++++++
>>   hw/s390x/s390-virtio-ccw.c      | 17 +++++++++
>>   hw/s390x/sclp.c                 |  3 ++
>>   include/hw/s390x/sclp.h         |  2 ++
>>   target/s390x/cpu.h              |  8 ++++-
>>   target/s390x/cpu_features.c     |  3 ++
>>   target/s390x/cpu_features.h     |  1 +
>>   target/s390x/cpu_features_def.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 ++
>>   14 files changed, 197 insertions(+), 1 deletion(-)
>>   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..0eb80fe
>> --- /dev/null
>> +++ b/hw/s390x/diag318.c
>> @@ -0,0 +1,80 @@
>> +/*
>> + * 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;
>> +
>> +    kvm_s390_set_diag318_info(d->info);
> 
> Shouldn't we at least moan if something goes wrong here?
> 

What are some things that might go wrong at this point? We
should only call this if the diag318 feature is enabled. If
we try to toggle that feature on a machine that cannot support
it, the guest will fail to start, stating (paraphrased) "some
features are not available in the configuration: diag318"

Is there some sort of issue that could arise from QEMU that I'm not
considering?

>> +    return 0;
>> +}
>> +
>> +static int diag318_pre_save(void *opaque)
>> +{
>> +    DIAG318State *d = opaque;
>> +
>> +    kvm_s390_get_diag318_info(&d->info);
>> +    return 0;
>> +}
>> +
>> +static bool diag318_needed(void *opaque)
>> +{
>> +    return s390_has_feat(S390_FEAT_DIAG318);
> 
> What happens if we emulate diag 318 in tcg and set this feature bit? We
> probably don't want to call kvm_ functions in that case.
> 

I have not tested on tcg.

>> +}
>> +
>> +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)
>> +{
>> +    kvm_s390_set_diag318_info(0);
> 
> Same here; we probably need a s390_set_diag318_info() function that
> either calls the kvm_ function or resets some internal state.
> 

Hmm okay. I recall doing this for the TOD-clock stuff way-back. I'll
include these functions next round.

>> +}
>> +
>> +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: Set automatically during IPL */
> 
> "Created automatically during machine instantiation" ?
> 

I like it.

>> +    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/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index 87b2039..54230c7 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -38,6 +38,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)
>>   {
>> @@ -94,6 +95,7 @@ static const char *const reset_dev_types[] = {
>>       "s390-sclp-event-facility",
>>       "s390-flic",
>>       "diag288",
>> +    TYPE_S390_DIAG318,
> 
> This looks a bit odd, although it is not wrong :)
> 

It's cut-off here, but TYPE_VIRTUAL_CSS_BRIDGE is also in the list :)

>>   };
>>   
>>   static void subsystem_reset(void)
>> @@ -258,6 +260,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;
>> @@ -315,6 +328,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,
>> @@ -583,6 +599,7 @@ static void machine_set_loadparm(Object *obj, const char *val, Error **errp)
>>           ms->loadparm[i] = ' '; /* pad right with spaces */
>>       }
>>   }
>> +
> 
> unrelated whitespace change :)
> 

Whoops, thanks.

>>   static inline void s390_machine_initfn(Object *obj)
>>   {
>>       object_property_add_bool(obj, "aes-key-wrap",
> 
> (...)
> 
>> diff --git a/target/s390x/cpu_features.c b/target/s390x/cpu_features.c
>> index f64f581..77a1df5 100644
>> --- a/target/s390x/cpu_features.c
>> +++ b/target/s390x/cpu_features.c
>> @@ -127,6 +127,9 @@ static const S390FeatDef s390_features[] = {
>>       FEAT_INIT("pfmfi", S390_FEAT_TYPE_SCLP_CONF_CHAR_EXT, 9, "SIE: PFMF interpretation facility"),
>>       FEAT_INIT("ibs", S390_FEAT_TYPE_SCLP_CONF_CHAR_EXT, 10, "SIE: Interlock-and-broadcast-suppression facility"),
>>   
>> +    /* SCLP SCCB Byte 134 */
>> +    FEAT_INIT("diag318", S390_FEAT_TYPE_SCLP_BYTE_134, 0, "Control program name and version codes"),
>> +
>>       FEAT_INIT("sief2", S390_FEAT_TYPE_SCLP_CPU, 4, "SIE: interception format 2 (Virtual SIE)"),
>>       FEAT_INIT("skey", S390_FEAT_TYPE_SCLP_CPU, 5, "SIE: Storage-key facility"),
>>       FEAT_INIT("gpereh", S390_FEAT_TYPE_SCLP_CPU, 10, "SIE: Guest-PER enhancement facility"),
>> 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.h b/target/s390x/cpu_features_def.h
>> index 292b17b..4f2c23e 100644
>> --- a/target/s390x/cpu_features_def.h
>> +++ b/target/s390x/cpu_features_def.h
>> @@ -115,6 +115,9 @@ typedef enum {
>>       S390_FEAT_SIE_PFMFI,
>>       S390_FEAT_SIE_IBS,
>>   
>> +    /* Sclp Byte 134 */
>> +    S390_FEAT_DIAG318,
>> +
>>       /* Sclp Cpu */
>>       S390_FEAT_SIE_F2,
>>       S390_FEAT_SIE_SKEY,
>> diff --git a/target/s390x/gen-features.c b/target/s390x/gen-features.c
>> index dc320a0..cdd1875 100644
>> --- a/target/s390x/gen-features.c
>> +++ b/target/s390x/gen-features.c
>> @@ -521,6 +521,7 @@ static uint16_t full_GEN12_GA1[] = {
>>       S390_FEAT_AP_QUERY_CONFIG_INFO,
>>       S390_FEAT_AP_FACILITIES_TEST,
>>       S390_FEAT_AP,
>> +    S390_FEAT_DIAG318,
>>   };
>>   
>>   static uint16_t full_GEN12_GA2[] = {
> 
> The feature bit stuff probably needs some (easy) adaption on top of my
> s390-next branch.
> 

I just noticed David's changes to that code. Should make this
feature stuff a bit easier.

Thanks for the review!