[v3] s390: kvm: adjust diag318 resets to retain data

[PATCH v3] s390: kvm: adjust diag318 resets to retain data

Posted by Collin Walling 2 years, 6 months ago

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.

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

Changelog:

    v2
    - handler uses run_on_cpu again
    - reworded commit message slightly
    - added fixes and reported-by tags 

    v3
    - nixed code reduction changes
    - added a comment to diag318 handler to briefly describe
        when relevent data is zeroed

---
 target/s390x/cpu.h     | 4 ++--
 target/s390x/kvm/kvm.c | 4 ++++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 3153d053e9..88aace36ff 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 */
diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
index 5b1fdb55c4..6acf14d5ec 100644
--- a/target/s390x/kvm/kvm.c
+++ b/target/s390x/kvm/kvm.c
@@ -1585,6 +1585,10 @@ void kvm_s390_set_diag318(CPUState *cs, uint64_t diag318_info)
         env->diag318_info = diag318_info;
         cs->kvm_run->s.regs.diag318 = diag318_info;
         cs->kvm_run->kvm_dirty_regs |= KVM_SYNC_DIAG318;
+        /*
+         * diag 318 info is zeroed during a clear reset and
+         * diag 308 IPL subcodes.
+         */
     }
 }
 
-- 
2.31.1

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

Posted by Janosch Frank 2 years, 6 months ago

On 11/9/21 21:56, 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.

I'd add something like:
The s390 machine reset code takes care of zeroing the diag318 data on VM 
resets which also cover resets caused by diag308.

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

Reviewed-by: Janosch Frank <frankja@linux.ibm.com>

> 
> Changelog:
> 
>      v2
>      - handler uses run_on_cpu again
>      - reworded commit message slightly
>      - added fixes and reported-by tags
> 
>      v3
>      - nixed code reduction changes
>      - added a comment to diag318 handler to briefly describe
>          when relevent data is zeroed
> 
> ---
>   target/s390x/cpu.h     | 4 ++--
>   target/s390x/kvm/kvm.c | 4 ++++
>   2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index 3153d053e9..88aace36ff 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 */
> diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
> index 5b1fdb55c4..6acf14d5ec 100644
> --- a/target/s390x/kvm/kvm.c
> +++ b/target/s390x/kvm/kvm.c
> @@ -1585,6 +1585,10 @@ void kvm_s390_set_diag318(CPUState *cs, uint64_t diag318_info)
>           env->diag318_info = diag318_info;
>           cs->kvm_run->s.regs.diag318 = diag318_info;
>           cs->kvm_run->kvm_dirty_regs |= KVM_SYNC_DIAG318;
> +        /*
> +         * diag 318 info is zeroed during a clear reset and
> +         * diag 308 IPL subcodes.
> +         */
>       }
>   }
>   
>

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

Posted by Collin Walling 2 years, 6 months ago

On 11/10/21 07:42, Janosch Frank wrote:
> On 11/9/21 21:56, 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.
> 
> I'd add something like:
> The s390 machine reset code takes care of zeroing the diag318 data on VM
> resets which also cover resets caused by diag308.
> 
>>
>> 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>
>> ---
> 
> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>

The CPNC portion of the diag318 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 diag318 instruction
has been invoked.

The s390_machine_reset code already takes care of zeroing the diag318
data on VM resets, which also cover resets caused by diag308.

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>
Reviewed-by: Janosch Frank <frankja@linux.ibm.com>

> 
>>
>> Changelog:
>>
>>      v2
>>      - handler uses run_on_cpu again
>>      - reworded commit message slightly
>>      - added fixes and reported-by tags
>>
>>      v3
>>      - nixed code reduction changes
>>      - added a comment to diag318 handler to briefly describe
>>          when relevent data is zeroed
>>
>> ---
>>   target/s390x/cpu.h     | 4 ++--
>>   target/s390x/kvm/kvm.c | 4 ++++
>>   2 files changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
>> index 3153d053e9..88aace36ff 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 */
>> diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
>> index 5b1fdb55c4..6acf14d5ec 100644
>> --- a/target/s390x/kvm/kvm.c
>> +++ b/target/s390x/kvm/kvm.c
>> @@ -1585,6 +1585,10 @@ void kvm_s390_set_diag318(CPUState *cs,
>> uint64_t diag318_info)
>>           env->diag318_info = diag318_info;
>>           cs->kvm_run->s.regs.diag318 = diag318_info;
>>           cs->kvm_run->kvm_dirty_regs |= KVM_SYNC_DIAG318;
>> +        /*
>> +         * diag 318 info is zeroed during a clear reset and
>> +         * diag 308 IPL subcodes.
>> +         */
>>       }
>>   }
>>  
> 
> 


-- 
Regards,
Collin

Stay safe and stay healthy

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

Posted by Christian Borntraeger 2 years, 5 months ago

Am 09.11.21 um 21:56 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.
> 
> 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>

Reviewed-by: Christian Borntraeger <borntraeger@linux.ibm.com>

maybe add cc stable just in case there will be one.
Can you resend with the final patch description and add Thomas as TO (not cc)
as this should probably go via Thomas tree.
> ---
> 
> Changelog:
> 
>      v2
>      - handler uses run_on_cpu again
>      - reworded commit message slightly
>      - added fixes and reported-by tags
> 
>      v3
>      - nixed code reduction changes
>      - added a comment to diag318 handler to briefly describe
>          when relevent data is zeroed
> 
> ---
>   target/s390x/cpu.h     | 4 ++--
>   target/s390x/kvm/kvm.c | 4 ++++
>   2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index 3153d053e9..88aace36ff 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 */
> diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
> index 5b1fdb55c4..6acf14d5ec 100644
> --- a/target/s390x/kvm/kvm.c
> +++ b/target/s390x/kvm/kvm.c
> @@ -1585,6 +1585,10 @@ void kvm_s390_set_diag318(CPUState *cs, uint64_t diag318_info)
>           env->diag318_info = diag318_info;
>           cs->kvm_run->s.regs.diag318 = diag318_info;
>           cs->kvm_run->kvm_dirty_regs |= KVM_SYNC_DIAG318;
> +        /*
> +         * diag 318 info is zeroed during a clear reset and
> +         * diag 308 IPL subcodes.
> +         */
>       }
>   }
>   
>

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

Posted by Collin Walling 2 years, 5 months ago

On 11/17/21 02:43, Christian Borntraeger wrote:
> Am 09.11.21 um 21:56 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.
>>
>> 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>
> 
> Reviewed-by: Christian Borntraeger <borntraeger@linux.ibm.com>
> 
> maybe add cc stable just in case there will be one.
> Can you resend with the final patch description and add Thomas as TO
> (not cc)
> as this should probably go via Thomas tree.

Done. Thank you.

>> ---
>>
>> Changelog:
>>
>>      v2
>>      - handler uses run_on_cpu again
>>      - reworded commit message slightly
>>      - added fixes and reported-by tags
>>
>>      v3
>>      - nixed code reduction changes
>>      - added a comment to diag318 handler to briefly describe
>>          when relevent data is zeroed
>>
>> ---
>>   target/s390x/cpu.h     | 4 ++--
>>   target/s390x/kvm/kvm.c | 4 ++++
>>   2 files changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
>> index 3153d053e9..88aace36ff 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 */
>> diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
>> index 5b1fdb55c4..6acf14d5ec 100644
>> --- a/target/s390x/kvm/kvm.c
>> +++ b/target/s390x/kvm/kvm.c
>> @@ -1585,6 +1585,10 @@ void kvm_s390_set_diag318(CPUState *cs,
>> uint64_t diag318_info)
>>           env->diag318_info = diag318_info;
>>           cs->kvm_run->s.regs.diag318 = diag318_info;
>>           cs->kvm_run->kvm_dirty_regs |= KVM_SYNC_DIAG318;
>> +        /*
>> +         * diag 318 info is zeroed during a clear reset and
>> +         * diag 308 IPL subcodes.
>> +         */
>>       }
>>   }
>>  
> 


-- 
Regards,
Collin

Stay safe and stay healthy