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

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

On 11/8/21 12:40, Christian Borntraeger wrote:
> 
> 
> Am 08.11.21 um 18:02 schrieb Janosch Frank:
>> On 11/5/21 23:46, Collin Walling wrote:
>>> 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 by the kernel.
>>>
>>> Additionally, the diag 318 data reset is handled via the CPU reset
>>> code. The set_diag318 code can be merged into the handler function
>>> and the helper functions can consequently be removed.
>>>
>>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>>
>> Fixes tag?
>>
>>> ---
>>>   hw/s390x/s390-virtio-ccw.c   |  3 ---
>>>   target/s390x/cpu-sysemu.c    |  7 -------
>>>   target/s390x/cpu.h           |  5 ++---
>>>   target/s390x/kvm/kvm.c       | 19 +++++--------------
>>>   target/s390x/kvm/kvm_s390x.h |  1 -
>>>   5 files changed, 7 insertions(+), 28 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));
>>> -    }
>>>       s390_ipl_clear_reset_request();
>>>   }
>>> 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;
>>
>> Before we brought this upstream I had a conversation with the
>> architect because I was confused about this myself. He said: SIGP does
>> not affect 318 data but all 308 subcode resets do (0,1,3,4).
>>
>> Hence I'd much rather move this out of the automatic reset areas and
>> clear it by hand for diag308 resets. And then add a big comment with a
>> warning to never move this into the automatic clearing areas.
> 
> the automatic cleaning areas are also used for the big hammer
> system_reset in QEMU which acts like a power cycle. And those fields are
> initialized always on such events.
>  So you cannot avoid clearing this for the big hammer things anyway.

The data needs to be reset during a clear reset as well. Correct me if
I'm wrong here: the 308 resets will invoke the qemu reset, which will
eventually invoke the machine reset. The s390_machine_reset code ends
with a clear reset request. I believe having the 318 field in the struct
accomplishes what we need: it is reset on clear and 308 subcodes and
avoids being tampered by SIGP.

>>
>>> +
>>>       /* 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..ed9c477b6f 100644
>>> --- a/target/s390x/kvm/kvm.c
>>> +++ b/target/s390x/kvm/kvm.c
>>> @@ -1576,18 +1576,6 @@ static int handle_sw_breakpoint(S390CPU *cpu,
>>> struct kvm_run *run)
>>>       return -ENOENT;
>>>   }
>>> -void kvm_s390_set_diag318(CPUState *cs, uint64_t diag318_info)
>>> -{
>>> -    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;
>>> -    }
>>> -}
>>> -
>>>   static void handle_diag_318(S390CPU *cpu, struct kvm_run *run)
>>>   {
>>>       uint64_t reg = (run->s390_sieic.ipa & 0x00f0) >> 4;
>>> @@ -1604,8 +1592,11 @@ 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));
>>> +        CPUS390XState *env = &S390_CPU(t)->env;
>>> +
>>> +        env->diag318_info = diag318_info;
>>> +        t->kvm_run->s.regs.diag318 = diag318_info;
>>> +        t->kvm_run->kvm_dirty_regs |= KVM_SYNC_DIAG318;
>>>       }
>>>   }
>>> 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 */
>>>
>>
> 


-- 
Regards,
Collin

Stay safe and stay healthy