Re: [PATCH v2] s390x: kvm: adjust diag318 resets to retain data

[Janosch Frank <frankja@linux.ibm.com>]

On 11/9/21 16:58, Collin Walling wrote:
> On 11/9/21 05:48, Janosch Frank wrote:
>> On 11/9/21 08:32, Christian Borntraeger wrote:
>>>
>>>
>>> Am 08.11.21 um 22:13 schrieb Collin Walling:
>>>> The CPNC portion of the diag 318 data is erroneously reset during an
>>>> initial CPU reset caused by SIGP. Let's go ahead and relocate the
>>>> diag318_info field within the CPUS390XState struct such that it is
>>>> only zeroed during a clear reset. This way, the CPNC will be retained
>>>> for each VCPU in the configuration after the diag 318 instruction
>>>> has been invoked.
>>>>
>>>> Additionally, the diag 318 data reset is handled via the CPU reset
>>>> code during a clear reset. This means some of the diag 318-specific
>>>> reset code can now be removed.
>>>>
>>>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>>>> Fixes: fabdada9357b ("s390: guest support for diagnose 0x318")
>>>> Reported-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>>
>>> It would be good to add at least a comment in the diag308 handlers
>>> where the value of cpnc is resetted during the resets that Janosch
>>> mentioned.
>>>>
>>>> ---
>>>>
>>>> Changelog:
>>>>
>>>>      v2
>>>>      - handler uses run_on_cpu again
>>>>      - reworded commit message slightly
>>>>      - added fixes and reported-by tags
>>>>
>>>> ---
>>>>     hw/s390x/s390-virtio-ccw.c   |  3 ---
>>>>     target/s390x/cpu-sysemu.c    |  7 -------
>>>>     target/s390x/cpu.h           |  5 ++---
>>>>     target/s390x/kvm/kvm.c       | 14 +++++---------
>>>>     target/s390x/kvm/kvm_s390x.h |  1 -
>>>>     5 files changed, 7 insertions(+), 23 deletions(-)
>>>>
>>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>>>> index 653587ea62..51dcb83b0c 100644
>>>> --- a/hw/s390x/s390-virtio-ccw.c
>>>> +++ b/hw/s390x/s390-virtio-ccw.c
>>>> @@ -489,9 +489,6 @@ static void s390_machine_reset(MachineState
>>>> *machine)
>>>>             g_assert_not_reached();
>>>>         }
>>>>     -    CPU_FOREACH(t) {
>>>> -        run_on_cpu(t, s390_do_cpu_set_diag318,
>>>> RUN_ON_CPU_HOST_ULONG(0));
>>>> -    }
>>
>> This needs to stay for the move to the clear reset area to work on other
>> diag308 subcodes. The reset will then be done twice for the clear reset
>> but that's fine.
>>
>> Moving the diag318 data into the clear area means we only reset it on a
>> full cpu reset which is only done for diagnose 308 subcode 0 and maybe a
>> QEMU reset, I didn't fully follow the code there.
>>
>> The other subcodes do an initial reset on the calling cpu and normal
>> resets on the others or just normal resets which don't touch the 318
>> data if we move it into the clear reset area.
>>> Or did I miss something fundamental here?
>>
>>
> While the diag318 data will not be cleared during an initial and load
> normal resets _directly_, the s390_machine_reset function ends with the
> call below:
> 
>>>>         s390_ipl_clear_reset_request();
>>>>     }
> 
> The clear reset request is invoked after each of the reset types at the
> tail end of the function. Because of this, the diag318 data will be
> reset during the 308 subcodes by way of the clear reset at the end.

This changes a few values in ipl.c and is not in fact a cpu clear reset.

QEMU knows a lot of resets. The VM resets here mainly depict diagnose 
308 subcodes which set the cpus and the channel subsystem into a 
specific state for the purpose of IPL (subcode 3 and 4 are how we IPL, 
subcode 10 is PV IPL).

The CPU resets can be triggered by SIGP (normal and initial, NOT clear 
reset) or are triggered as part of a VM reset mentioned above.

> 
>>>>     diff --git a/target/s390x/cpu-sysemu.c b/target/s390x/cpu-sysemu.c
>>>> index 5471e01ee8..6d9f6d4402 100644
>>>> --- a/target/s390x/cpu-sysemu.c
>>>> +++ b/target/s390x/cpu-sysemu.c
>>>> @@ -299,10 +299,3 @@ void s390_enable_css_support(S390CPU *cpu)
>>>>             kvm_s390_enable_css_support(cpu);
>>>>         }
>>>>     }
>>>> -
>>>> -void s390_do_cpu_set_diag318(CPUState *cs, run_on_cpu_data arg)
>>>> -{
>>>> -    if (kvm_enabled()) {
>>>> -        kvm_s390_set_diag318(cs, arg.host_ulong);
>>>> -    }
>>>> -}
>>>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
>>>> index 3153d053e9..1b94b91d87 100644
>>>> --- a/target/s390x/cpu.h
>>>> +++ b/target/s390x/cpu.h
>>>> @@ -63,6 +63,8 @@ struct CPUS390XState {
>>>>         uint64_t etoken;       /* etoken */
>>>>         uint64_t etoken_extension; /* etoken extension */
>>>>     +    uint64_t diag318_info;
>>>> +
>>>>         /* Fields up to this point are not cleared by initial CPU
>>>> reset */
>>>>         struct {} start_initial_reset_fields;
>>>>     @@ -118,8 +120,6 @@ struct CPUS390XState {
>>>>         uint16_t external_call_addr;
>>>>         DECLARE_BITMAP(emergency_signals, S390_MAX_CPUS);
>>>>     -    uint64_t diag318_info;
>>>> -
>>>>     #if !defined(CONFIG_USER_ONLY)
>>>>         uint64_t tlb_fill_tec;   /* translation exception code during
>>>> tlb_fill */
>>>>         int tlb_fill_exc;        /* exception number seen during
>>>> tlb_fill */
>>>> @@ -780,7 +780,6 @@ int s390_set_memory_limit(uint64_t new_limit,
>>>> uint64_t *hw_limit);
>>>>     void s390_set_max_pagesize(uint64_t pagesize, Error **errp);
>>>>     void s390_cmma_reset(void);
>>>>     void s390_enable_css_support(S390CPU *cpu);
>>>> -void s390_do_cpu_set_diag318(CPUState *cs, run_on_cpu_data arg);
>>>>     int s390_assign_subch_ioeventfd(EventNotifier *notifier, uint32_t
>>>> sch_id,
>>>>                                     int vq, bool assign);
>>>>     #ifndef CONFIG_USER_ONLY
>>>> diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
>>>> index 5b1fdb55c4..8970d9ca55 100644
>>>> --- a/target/s390x/kvm/kvm.c
>>>> +++ b/target/s390x/kvm/kvm.c
>>>> @@ -1576,16 +1576,13 @@ static int handle_sw_breakpoint(S390CPU *cpu,
>>>> struct kvm_run *run)
>>>>         return -ENOENT;
>>>>     }
>>>>     -void kvm_s390_set_diag318(CPUState *cs, uint64_t diag318_info)
>>>> +static void set_diag_318(CPUState *cs, run_on_cpu_data arg)
>>>>     {
>>>>         CPUS390XState *env = &S390_CPU(cs)->env;
>>>>     -    /* Feat bit is set only if KVM supports sync for diag318 */
>>>> -    if (s390_has_feat(S390_FEAT_DIAG_318)) {
>>>> -        env->diag318_info = diag318_info;
>>>> -        cs->kvm_run->s.regs.diag318 = diag318_info;
>>>> -        cs->kvm_run->kvm_dirty_regs |= KVM_SYNC_DIAG318;
>>>> -    }
>>>> +    env->diag318_info = arg.host_ulong;
>>>> +    cs->kvm_run->s.regs.diag318 = arg.host_ulong;
>>>> +    cs->kvm_run->kvm_dirty_regs |= KVM_SYNC_DIAG318;
>>>>     }
>>>>        static void handle_diag_318(S390CPU *cpu, struct kvm_run *run)
>>>> @@ -1604,8 +1601,7 @@ static void handle_diag_318(S390CPU *cpu,
>>>> struct kvm_run *run)
>>>>         }
>>>>            CPU_FOREACH(t) {
>>>> -        run_on_cpu(t, s390_do_cpu_set_diag318,
>>>> -                   RUN_ON_CPU_HOST_ULONG(diag318_info));
>>>> +        run_on_cpu(t, set_diag_318,
>>>> RUN_ON_CPU_HOST_ULONG(diag318_info));
>>>>         }
>>>>     }
>>>>     diff --git a/target/s390x/kvm/kvm_s390x.h
>>>> b/target/s390x/kvm/kvm_s390x.h
>>>> index 05a5e1e6f4..8c244ee84d 100644
>>>> --- a/target/s390x/kvm/kvm_s390x.h
>>>> +++ b/target/s390x/kvm/kvm_s390x.h
>>>> @@ -44,6 +44,5 @@ void kvm_s390_set_max_pagesize(uint64_t pagesize,
>>>> Error **errp);
>>>>     void kvm_s390_crypto_reset(void);
>>>>     void kvm_s390_restart_interrupt(S390CPU *cpu);
>>>>     void kvm_s390_stop_interrupt(S390CPU *cpu);
>>>> -void kvm_s390_set_diag318(CPUState *cs, uint64_t diag318_info);
>>>>        #endif /* KVM_S390X_H */
>>>>
>>
>>
> 
>