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

[Collin Walling]

On 5/9/19 5:58 AM, Christian Borntraeger 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;
>
> But we also do not need to zero it out? Can't you just drop these 2 lines?

Absolutely. I'll drop them.

> > +
> > +    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;
> > +}
>
> I would like to have a cpumodel entry that allows to disable that feature. And 
> we should
> then check for this.

Noted.

> > +
> > +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;
> > +    }
>
> same here -> cpumodel
> Then we can get rid of the enabled bool.

Noted.

[...]
> >   
> >   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;
>
> Maybe this should depend on the presence of this feature in the cpumodel?

That's a good idea. This would allow the user to fine tune their VM to
either allow the original max 248 _or_ enable the diag318 instruction.

Do you think the reduction to 240 max CPUs still makes sense? I do not
know what the future of the RSCPI looks like, but if we end up with more
bytes indicating facility bits, we could simply reduce the the max CPUs
dynamically depending on which features the user / CPU model enables.

In this case, if diag318 is enabled then reduce the max CPUs to 247.
CPU models can protect us WRT migration -- we wouldn't have to concern
ourselves if the destination machine can support the same number of
CPUs.

In the future, if feature X is enabled, then reduce the max CPUs to as
many as we can fit in the remainder of the SCCB.

> >   }
> >   DEFINE_CCW_MACHINE(4_0, "4.0", true);
> >   
> > diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> > index 4510a80..9cfa188 100644
> > --- a/hw/s390x/sclp.c
> > +++ b/hw/s390x/sclp.c
> > @@ -22,6 +22,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)
> >   {
> > @@ -74,6 +75,10 @@ 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);
> >   
> > +    /* Enable diag318 for guest if KVM supports emulation */
> > +    if (kvm_s390_has_diag318())
> > +        read_info->fac134 = 0x80;
>
> I think we should rather make this part of the cpumodel

As per an older discussion, does it still make sense to restrict this
feature starting with a particular CPU model (we decided to allow this
feature starting with the zEC12 model). We have the opportunity to
simply enable this feature for *all* CPU models since this only requires
KVM to support emulation.

Pseudo:

if diag318_cpu_feat_is_enabled
        if kvm_has_diag318
                set facility bit for diag318
        else
                error "KVM does not support emulation of diag318"
                <terminate>

The next question would be this: do we want this feature to be enabled
by default, or only if it is specified by the user?

Default enabled: VMs will have the diag318 capability, but max CPUs will
also be reduced by default. Use will have to explicitly disable diag318
to allow the max 248 CPUs.

User enabled: VMs will have to be configured to enable this capability
(users might not realize / care to add this feature themselves). This is
as simple as providing the CPU model in a guest XML or command line and
providing the diag318feature. The max CPUs will not be altered by
default.

> > +
> >       read_info->facilities = cpu_to_be64(SCLP_HAS_CPU_INFO |
> >                                           SCLP_HAS_IOA_RECONFIG);
> >   
> > diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
> > index f9db243..d47e10a 100644
> > --- a/include/hw/s390x/sclp.h
> > +++ b/include/hw/s390x/sclp.h
> > @@ -133,6 +133,8 @@ typedef struct ReadInfo {
> >       uint16_t highest_cpu;
> >       uint8_t  _reserved5[124 - 122];     /* 122-123 */
> >       uint32_t hmfai;
> > +    uint8_t  _reserved7[134 - 128];     /* 128-133 */
> > +    uint8_t  fac134;
> >       struct CPUEntry entries[0];
> >   } QEMU_PACKED ReadInfo;
> >   
> > diff --git a/linux-headers/asm-s390/kvm.h b/linux-headers/asm-s390/kvm.h
> > index 0265482..735f5a4 100644
> > --- a/linux-headers/asm-s390/kvm.h
> > +++ b/linux-headers/asm-s390/kvm.h
> > @@ -74,6 +74,7 @@ struct kvm_s390_io_adapter_req {
> >   #define KVM_S390_VM_CRYPTO            2
> >   #define KVM_S390_VM_CPU_MODEL         3
> >   #define KVM_S390_VM_MIGRATION         4
> > +#define KVM_S390_VM_MISC               5
>
> Can you split that out into a separate patch (header sync). (We also need the 
> relevant kernel
> patch first, I could not find it right now).

Gah, I keep forgetting about splitting this. Thanks for the reminder :)

[...]