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

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

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