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 <wall...@linux.ibm.com>
>>> ---
>>> 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 <wall...@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_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
