[prev in list] [next in list] [prev in thread] [next in thread]
List: qemu-s390x
Subject: Re: [qemu-s390x] [Qemu-devel] [PATCH v5 2/2] s390: diagnose 318 info reset and migration support
From: Collin Walling <walling () linux ! ibm ! com>
Date: 2019-06-26 14:07:39
Message-ID: 7a47fc60-e490-3c95-66f5-3e16a2ad9da7 () linux ! ibm ! com
[Download RAW message or body]
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!
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic