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

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

[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 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>
---
 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;
+
     /* 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 */
-- 
2.31.1

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

[Collin Walling]

On 11/5/21 18: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>

I neglected to mention that this addresses a bug that was discovered
internally, observed by a recent patch I sent upstream:

[PATCH] KVM: s390x: add debug statement for diag 318 CPNC data

This patch removes most of the code from one of my past patches. Does it
make more sense to revert the old patch and then introduce a follow-up
that includes the additions introduced by this new one?

commit e2c6cd567422bfa563be026b9741a1854aecdc06
Author: Collin L. Walling <walling@linux.ibm.com>
Date:   Fri Nov 13 17:10:22 2020 -0500

    s390/kvm: fix diag318 propagation and reset functionality

There is also a bug where hotplugged CPUs are not acquiring the CPNC as
well. I will address a fix to this in a follow-up patch in the future.

-- 
Regards,
Collin

Stay safe and stay healthy

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

[Christian Borntraeger]

Am 05.11.21 um 23:46 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 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>
[..]
> 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;
> -

The move makes perfect sense.


>   #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;

I am not sure if this part works fine. What happens if
another CPU is currently in SIE (not stopped).
Then this change will be not visible in that CPU and in
fact this change will be overwritten when the CPU exits to QEMU.

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

[Collin Walling]

On 11/8/21 03:07, Christian Borntraeger wrote:
> 
> 
> Am 05.11.21 um 23:46 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 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>

[...]

>> 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;
> 
> I am not sure if this part works fine. What happens if
> another CPU is currently in SIE (not stopped).
> Then this change will be not visible in that CPU and in
> fact this change will be overwritten when the CPU exits to QEMU.
> 

Ah, I should've paid more attention to what run_on_cpu does. I now see
that it makes CPU changes atomic. I'll reintroduce the helper as a
static function and use the run_on_cpu again.


-- 
Regards,
Collin

Stay safe and stay healthy

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

[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.

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

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

[Collin Walling]

On 11/8/21 12:02, Janosch Frank wrote:
> 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?
> 

Will include this on v2 post. Thank you.

[...]

-- 
Regards,
Collin

Stay safe and stay healthy

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

[Christian Borntraeger]

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

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

[Collin Walling]

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