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 <wall...@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
