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

[Collin Walling]

On 6/26/19 8:33 AM, Cornelia Huck wrote:
> On Tue, 25 Jun 2019 11:17:09 -0400
> Collin Walling <address@hidden> 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 <address@hidden>
> > ---
> >   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 <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_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!