Re: [RFC PATCH] s390: kvm: reduce frequency of CPU syncs of diag318 info

[Collin Walling]

On 11/23/21 01:14, Christian Borntraeger wrote:
> 
> Am 22.11.21 um 23:33 schrieb Collin Walling:
>> DIAGNOSE 0318 is invoked only once during IPL. As such, the
>> diag318 info will only change once initially and during resets.
>> Let's only sync the register to convey the info to KVM if and
>> only if the diag318 info has changed. Only set the dirty bit
>> flag for diag318 whenever it must be updated.
> 
> Is this really necessary? In my logs the sync only happens for larger
> changes (like reset) operations and then yes we log again.
> But I think it is ok to see such a log entry in these rare events.

Albeit a micro-optimization, I don't see why the diag318 dirtied bit
must to be set during /every/ sync. It makes more sense to set the flag
after the register was actually modified.

>>
>> The migration handler will invoke the set_diag318 helper on
>> post_load to ensure the info is set on the destination machine.
>>
>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>> ---
>>   target/s390x/kvm/kvm.c |  5 -----
>>   target/s390x/machine.c | 14 ++++++++++++++
>>   2 files changed, 14 insertions(+), 5 deletions(-)
>>
>> diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
>> index 6acf14d5ec..6a141399f7 100644
>> --- a/target/s390x/kvm/kvm.c
>> +++ b/target/s390x/kvm/kvm.c
>> @@ -599,11 +599,6 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>>           cs->kvm_run->kvm_dirty_regs |= KVM_SYNC_ETOKEN;
>>       }
>>   -    if (can_sync_regs(cs, KVM_SYNC_DIAG318)) {
>> -        cs->kvm_run->s.regs.diag318 = env->diag318_info;
>> -        cs->kvm_run->kvm_dirty_regs |= KVM_SYNC_DIAG318;
>> -    }
>> -
>>       /* Finally the prefix */
>>       if (can_sync_regs(cs, KVM_SYNC_PREFIX)) {
>>           cs->kvm_run->s.regs.prefix = env->psa;
>> diff --git a/target/s390x/machine.c b/target/s390x/machine.c
>> index 37a076858c..a5d113ce3a 100644
>> --- a/target/s390x/machine.c
>> +++ b/target/s390x/machine.c
>> @@ -234,6 +234,19 @@ const VMStateDescription vmstate_etoken = {
>>       }
>>   };
>>   +static int diag318_post_load(void *opaque, int version_id)
>> +{
>> +    S390CPU *cpu = opaque;
>> +    CPUState *cs = CPU(cpu);
>> +    CPUS390XState *env = &S390_CPU(cs)->env;
>> +
>> +    if (kvm_enabled()) {
>> +        kvm_s390_set_diag318(cs, env->diag318_info);
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>   static bool diag318_needed(void *opaque)
>>   {
>>       return s390_has_feat(S390_FEAT_DIAG_318);
>> @@ -243,6 +256,7 @@ const VMStateDescription vmstate_diag318 = {
>>       .name = "cpu/diag318",
>>       .version_id = 1,
>>       .minimum_version_id = 1,
>> +    .post_load = diag318_post_load,
>>       .needed = diag318_needed,
>>       .fields = (VMStateField[]) {
>>           VMSTATE_UINT64(env.diag318_info, S390CPU),
>>
> 


-- 
Regards,
Collin

Stay safe and stay healthy