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

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

Posted by Collin Walling 2 years, 5 months ago

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

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

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

Posted by Christian Borntraeger 2 years, 5 months ago

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

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

Posted by Thomas Huth 2 years, 5 months ago

On 23/11/2021 10.28, Christian Borntraeger wrote:
> 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?

Yes, queued to s390x-next now:

https://gitlab.com/thuth/qemu/-/commits/s390x-next/

  Thomas

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

Posted by Collin Walling 2 years, 5 months ago

Polite ping. I may have missed if this patch was picked already. Thanks!


-- 
Regards,
Collin

Stay safe and stay healthy

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

Posted by Thomas Huth 2 years, 5 months ago

On 01/12/2021 19.45, Collin Walling wrote:
> Polite ping. I may have missed if this patch was picked already. Thanks!

I've already queued it to my s390x-next branch:

  https://gitlab.com/thuth/qemu/-/commits/s390x-next/

It just came in very late for 6.2, and it didn't seem too critical to me, so 
I didn't sent a separate pull request for this one. Thus it will get merged 
once the hard freeze period of QEMU is over.

  Thomas

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

Posted by Collin Walling 2 years, 5 months ago

On 12/2/21 04:23, Thomas Huth wrote:
> On 01/12/2021 19.45, Collin Walling wrote:
>> Polite ping. I may have missed if this patch was picked already. Thanks!
> 
> I've already queued it to my s390x-next branch:
> 
>  https://gitlab.com/thuth/qemu/-/commits/s390x-next/
> 
> It just came in very late for 6.2, and it didn't seem too critical to
> me, so I didn't sent a separate pull request for this one. Thus it will
> get merged once the hard freeze period of QEMU is over.
> 
>  Thomas
> 

Apologies, I missed this. My mail filters are sometimes messed up and I
didn't see this thread. All is good :)

-- 
Regards,
Collin

Stay safe and stay healthy