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

[Thomas Huth]

On 09/05/2019 22.50, Collin Walling wrote:
> 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.

Please also not that ccw_machine_4_0_class_options() is called from
ccw_machine_3_1_class_options() and thus all older machine class option
functions - you have to make sure to restore the 248 for them.

As a test, please check that you can migrate a s390-ccw-virtio-3.1
machine with 248 CPUs from a qemu without your patch to a QEMU with your
patch and back.

 Thomas