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

[Christian Borntraeger <borntraeger@de.ibm.com>]

Am 17.11.21 um 16:23 schrieb Collin Walling:
> 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>
> Reviewed-by: Christian Borntraeger <borntraeger@linux.ibm.com>

Thomas, I think this is good to go. Will you take it via your tree?

> ---
> 
> Changelog:
> 
>      v4
>      - fixed up commit message and added r-b's
> 
>      v3
>      - reverted changes from previous versions
>      - simply relocate diag318_info in CPU State struct
>      - add comment in set_diag318 to explain resets
> 
>      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.
> +         */
>       }
>   }
>   
>