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

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

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 <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?
>>>

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