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

[Collin Walling]

On 4/3/19 10:16 AM, Collin Walling wrote:
> On 4/3/19 8:30 AM, Cornelia Huck wrote:
> > On Wed, 3 Apr 2019 13:46:07 +0200
> > David Hildenbrand <address@hidden> wrote:
> >
> > > On 01.04.19 23:48, 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.
> > > >
> > > > Diag318 must also be reset on a load normal and modified clear, so
> > > > we use the set function (wrapped in a reset function) to explicitly
> > > > set the diag318 info to 0 for these cases.
> > > >
> > > > Lastly, we want to ensure the diag318 info is migrated. The diag318
> > > > info migration is handled via a VMStateDescription. This feature is
> > > > only supported on QEMU machines 4.0 and later.
> > >
> > > This has to become 4.1
> > >
> > > > Signed-off-by: Collin Walling <address@hidden>
> > > > ---
> > > >
> > > > This version is posted in tandem with a new kernel patch that changes
> > > > how the execution of the diag 0x318 instruction is handled. A link to
> > > > this series will be attached as a reply to this series for convenience.
> > > >
> > > > Changelog:
> > > >
> > > >      v3
> > > >          - removed CPU model code
> > > >          - removed RSCPI and SCLP code
> > > >          - reverted max cpus back to 248 (previous patches limited this
> > > >              to 247)
> > > >          - introduced VMStateDescription handlers for migration
> > > >          - disabled migration of diag318 info for machines 3.1 and 
> > > > older
> > > >              - a warning is printed if migration is disabled and KVM
> > > >                supports handling this instruction
> > > >
> > > > ---
> > > >   hw/s390x/s390-virtio-ccw.c   |  6 ++++
> > > >   linux-headers/asm-s390/kvm.h |  4 +++
> > > >   target/s390x/diag.c          | 63 
> > > > ++++++++++++++++++++++++++++++++++++
> > > >   target/s390x/internal.h      |  5 ++-
> > > >   target/s390x/kvm-stub.c      | 15 +++++++++
> > > >   target/s390x/kvm.c           | 32 ++++++++++++++++++
> > > >   target/s390x/kvm_s390x.h     |  3 ++
> > > >   7 files changed, 127 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> > > > index d11069b860..2a50868496 100644
> > > > --- a/hw/s390x/s390-virtio-ccw.c
> > > > +++ b/hw/s390x/s390-virtio-ccw.c
> > > > @@ -36,6 +36,7 @@
> > > >   #include "cpu_models.h"
> > > >   #include "hw/nmi.h"
> > > >   #include "hw/s390x/tod.h"
> > > > +#include "internal.h"
> > > >   S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
> > > >   {
> > > > @@ -302,6 +303,8 @@ static void ccw_init(MachineState *machine)
> > > >       /* init the TOD clock */
> > > >       s390_init_tod();
> > > > +
> > > > +    diag318_register_migration();
> > > >   }
> > > >   static void s390_cpu_plug(HotplugHandler *hotplug_dev,
> > > > @@ -352,6 +355,7 @@ static void s390_machine_reset(void)
> > > >           }
> > > >           subsystem_reset();
> > > >           s390_crypto_reset();
> > > > +        diag318_reset();
> > >
> > > Shouldn't this go into subsystem_reset?
> > >
> > > Aren't you missing resets during external/reipl resets?

Certainly makes sense to do this during reipl as well. The diag318 info 
is to be reset during a clear, power-on, and load normal. I'll look into
it as I investigate this "fake device" route.

> > > Also, I was wondering if this would be worth creating a fake device like
> > > diag288. The resets can be handled similar to diag288. Resets during
> > > external/reipl reset would come natural.
>
> I'll look into it.
>
> > I like the idea of adding a new device. Would also make the migration
> > code nicer (as you suggested below).
>
> Right. I always forget this step.

Whoops -- Meant that response to be under the "linux header" comment.

> > > >   static void ccw_machine_3_1_class_options(MachineClass *mc)
> > > > diff --git a/linux-headers/asm-s390/kvm.h 
> > > > b/linux-headers/asm-s390/kvm.h
> > > > index 0265482f8f..735f5a46e8 100644
> > > > --- a/linux-headers/asm-s390/kvm.h
> > > > +++ b/linux-headers/asm-s390/kvm.h
> >
> > Updates of linux headers should always go into a separate patch that
> > can be replaced by a proper headers update when applying.
> >
> > > > @@ -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
> > > >   /* kvm attributes for mem_ctrl */
> > > >   #define KVM_S390_VM_MEM_ENABLE_CMMA    0
> > > > @@ -168,6 +169,9 @@ struct kvm_s390_vm_cpu_subfunc {
> > > >   #define KVM_S390_VM_MIGRATION_START    1
> > > >   #define KVM_S390_VM_MIGRATION_STATUS    2
> > > > +/* kvm attributes for KVM_S390_VM_MISC */
> > > > +#define KVM_S390_VM_MISC_CPC        0
> > > > +
> > > >   /* for KVM_GET_REGS and KVM_SET_REGS */
> > > >   struct kvm_regs {
> > > >       /* general purpose regs for s390 */