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

[Collin Walling <walling@linux.ibm.com>]

On 4/3/19 8:30 AM, Cornelia Huck wrote:
> On Wed, 3 Apr 2019 13:46:07 +0200
> David Hildenbrand <david@redhat.com> 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 <walling@linux.ibm.com>
>>> ---
>>>
>>> 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?
>>
>> 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.

>>>   
>>>   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 */
>