Re: [PATCH for-8.2] target/s390x/kvm: Simplify the GPRs, ACRs, CRs and prefix synchronization code

["Cédric Le Goater" <clegoate@redhat.com>]

Hello Thomas,

On 8/28/23 20:20, Thomas Huth wrote:
> On 14/08/2023 21.06, Thomas Huth wrote:
>> KVM_SYNC_GPRS, KVM_SYNC_ACRS, KVM_SYNC_CRS and KVM_SYNC_PREFIX are
>> available since kernel 3.10. Since we already require at least kernel
>> 3.15 in the s390x KVM code, we can assume that the KVM_CAP_SYNC_REGS
>> sync code is always possible for these registers, and remove the
>> related checks and fallbacks via KVM_SET_REGS and KVM_GET_REGS.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   target/s390x/kvm/kvm.c | 119 ++++++++++++-----------------------------
>>   1 file changed, 34 insertions(+), 85 deletions(-)
> 
> Ping! Anybody any comments on this one?


Only minor comments, see below.

> 
>   Thomas
> 
>> diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
>> index a9e5880349..ff415f7b30 100644
>> --- a/target/s390x/kvm/kvm.c
>> +++ b/target/s390x/kvm/kvm.c
>> @@ -148,7 +148,6 @@ const KVMCapabilityInfo kvm_arch_required_capabilities[] = {
>>       KVM_CAP_LAST_INFO
>>   };
>> -static int cap_sync_regs;
>>   static int cap_async_pf;
>>   static int cap_mem_op;
>>   static int cap_mem_op_extension;
>> @@ -342,21 +341,28 @@ static void ccw_machine_class_foreach(ObjectClass *oc, void *opaque)
>>   int kvm_arch_init(MachineState *ms, KVMState *s)
>>   {
>> +    int required_caps[] = {
>> +        KVM_CAP_DEVICE_CTRL,
>> +        KVM_CAP_SYNC_REGS,
>> +    };
>> +
>> +    for (int i = 0; i < ARRAY_SIZE(required_caps); i++) {
>> +        if (!kvm_check_extension(s, required_caps[i])) {
>> +            error_report("KVM is missing capability #%d - "
>> +                         "please use kernel 3.15 or newer", required_caps[i]);
>> +            return -1;
>> +        }
>> +    }
>> +
>>       object_class_foreach(ccw_machine_class_foreach, TYPE_S390_CCW_MACHINE,
>>                            false, NULL);
>> -    if (!kvm_check_extension(kvm_state, KVM_CAP_DEVICE_CTRL)) {
>> -        error_report("KVM is missing capability KVM_CAP_DEVICE_CTRL - "
>> -                     "please use kernel 3.15 or newer");
>> -        return -1;
>> -    }
>>       if (!kvm_check_extension(s, KVM_CAP_S390_COW)) {
>>           error_report("KVM is missing capability KVM_CAP_S390_COW - "
>>                        "unsupported environment");
>>           return -1;
>>       }
>> -    cap_sync_regs = kvm_check_extension(s, KVM_CAP_SYNC_REGS);
>>       cap_async_pf = kvm_check_extension(s, KVM_CAP_ASYNC_PF);
>>       cap_mem_op = kvm_check_extension(s, KVM_CAP_S390_MEM_OP);
>>       cap_mem_op_extension = kvm_check_extension(s, KVM_CAP_S390_MEM_OP_EXTENSION);

I would introduce a first patch for the changes above, checking the
KVM capabilities, and a second for get/put register changes.


>> @@ -463,15 +469,15 @@ void kvm_s390_reset_vcpu_normal(S390CPU *cpu)
>>   static int can_sync_regs(CPUState *cs, int regs)
>>   {
>> -    return cap_sync_regs && (cs->kvm_run->kvm_valid_regs & regs) == regs;
>> +    return (cs->kvm_run->kvm_valid_regs & regs) == regs;
>>   }
>>   int kvm_arch_put_registers(CPUState *cs, int level)
>>   {
>> +    const int required_syncs = KVM_SYNC_GPRS | KVM_SYNC_ACRS |
>> +                               KVM_SYNC_CRS | KVM_SYNC_PREFIX;

May be introduce a KVM_SYNC_s390_REQUIRED_FIELDS define for these fields,
as we already have KVM_SYNC_S390_VALID_FIELDS.

>>       S390CPU *cpu = S390_CPU(cs);
>>       CPUS390XState *env = &cpu->env;
>> -    struct kvm_sregs sregs;
>> -    struct kvm_regs regs;
>>       struct kvm_fpu fpu = {};
>>       int r;
>>       int i;
>> @@ -480,21 +486,16 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>>       cs->kvm_run->psw_addr = env->psw.addr;
>>       cs->kvm_run->psw_mask = env->psw.mask;
>> -    if (can_sync_regs(cs, KVM_SYNC_GPRS)) {
>> -        for (i = 0; i < 16; i++) {
>> -            cs->kvm_run->s.regs.gprs[i] = env->regs[i];
>> -            cs->kvm_run->kvm_dirty_regs |= KVM_SYNC_GPRS;
>> -        }
>> -    } else {
>> -        for (i = 0; i < 16; i++) {
>> -            regs.gprs[i] = env->regs[i];
>> -        }
>> -        r = kvm_vcpu_ioctl(cs, KVM_SET_REGS, &regs);
>> -        if (r < 0) {
>> -            return r;
>> -        }
>> +    g_assert((cs->kvm_run->kvm_valid_regs & required_syncs) == required_syncs);

this also is :

           g_assert(can_sync_regs(cs, KVM_SYNC_s390_REQUIRED_FIELDS));

the rest looks good.

Thanks,

C.

>> +    cs->kvm_run->kvm_dirty_regs |= required_syncs;
>> +    for (i = 0; i < 16; i++) {
>> +        cs->kvm_run->s.regs.gprs[i] = env->regs[i];
>> +        cs->kvm_run->s.regs.acrs[i] = env->aregs[i];
>> +        cs->kvm_run->s.regs.crs[i] = env->cregs[i];
>>       }
>> +    cs->kvm_run->s.regs.prefix = env->psa;
>> +
>>       if (can_sync_regs(cs, KVM_SYNC_VRS)) {
>>           for (i = 0; i < 32; i++) {
>>               cs->kvm_run->s.regs.vrs[i][0] = env->vregs[i][0];
>> @@ -572,25 +573,6 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>>           }
>>       }
>> -    /* access registers and control registers*/
>> -    if (can_sync_regs(cs, KVM_SYNC_ACRS | KVM_SYNC_CRS)) {
>> -        for (i = 0; i < 16; i++) {
>> -            cs->kvm_run->s.regs.acrs[i] = env->aregs[i];
>> -            cs->kvm_run->s.regs.crs[i] = env->cregs[i];
>> -        }
>> -        cs->kvm_run->kvm_dirty_regs |= KVM_SYNC_ACRS;
>> -        cs->kvm_run->kvm_dirty_regs |= KVM_SYNC_CRS;
>> -    } else {
>> -        for (i = 0; i < 16; i++) {
>> -            sregs.acrs[i] = env->aregs[i];
>> -            sregs.crs[i] = env->cregs[i];
>> -        }
>> -        r = kvm_vcpu_ioctl(cs, KVM_SET_SREGS, &sregs);
>> -        if (r < 0) {
>> -            return r;
>> -        }
>> -    }
>> -
>>       if (can_sync_regs(cs, KVM_SYNC_GSCB)) {
>>           memcpy(cs->kvm_run->s.regs.gscb, env->gscb, 32);
>>           cs->kvm_run->kvm_dirty_regs |= KVM_SYNC_GSCB;
>> @@ -612,22 +594,15 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>>           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;
>> -        cs->kvm_run->kvm_dirty_regs |= KVM_SYNC_PREFIX;
>> -    } else {
>> -        /* prefix is only supported via sync regs */
>> -    }
>>       return 0;
>>   }
>>   int kvm_arch_get_registers(CPUState *cs)
>>   {
>> +    const int required_syncs = KVM_SYNC_GPRS | KVM_SYNC_ACRS |
>> +                               KVM_SYNC_CRS | KVM_SYNC_PREFIX;
>>       S390CPU *cpu = S390_CPU(cs);
>>       CPUS390XState *env = &cpu->env;
>> -    struct kvm_sregs sregs;
>> -    struct kvm_regs regs;
>>       struct kvm_fpu fpu;
>>       int i, r;
>> @@ -635,37 +610,16 @@ int kvm_arch_get_registers(CPUState *cs)
>>       env->psw.addr = cs->kvm_run->psw_addr;
>>       env->psw.mask = cs->kvm_run->psw_mask;
>> -    /* the GPRS */
>> -    if (can_sync_regs(cs, KVM_SYNC_GPRS)) {
>> -        for (i = 0; i < 16; i++) {
>> -            env->regs[i] = cs->kvm_run->s.regs.gprs[i];
>> -        }
>> -    } else {
>> -        r = kvm_vcpu_ioctl(cs, KVM_GET_REGS, &regs);
>> -        if (r < 0) {
>> -            return r;
>> -        }
>> -         for (i = 0; i < 16; i++) {
>> -            env->regs[i] = regs.gprs[i];
>> -        }
>> +    /* the GPRS, ACRS and CRS */
>> +    g_assert((cs->kvm_run->kvm_valid_regs & required_syncs) == required_syncs);
>> +    for (i = 0; i < 16; i++) {
>> +        env->regs[i] = cs->kvm_run->s.regs.gprs[i];
>> +        env->aregs[i] = cs->kvm_run->s.regs.acrs[i];
>> +        env->cregs[i] = cs->kvm_run->s.regs.crs[i];
>>       }
>> -    /* The ACRS and CRS */
>> -    if (can_sync_regs(cs, KVM_SYNC_ACRS | KVM_SYNC_CRS)) {
>> -        for (i = 0; i < 16; i++) {
>> -            env->aregs[i] = cs->kvm_run->s.regs.acrs[i];
>> -            env->cregs[i] = cs->kvm_run->s.regs.crs[i];
>> -        }
>> -    } else {
>> -        r = kvm_vcpu_ioctl(cs, KVM_GET_SREGS, &sregs);
>> -        if (r < 0) {
>> -            return r;
>> -        }
>> -         for (i = 0; i < 16; i++) {
>> -            env->aregs[i] = sregs.acrs[i];
>> -            env->cregs[i] = sregs.crs[i];
>> -        }
>> -    }
>> +    /* The prefix */
>> +    env->psa = cs->kvm_run->s.regs.prefix;
>>       /* Floating point and vector registers */
>>       if (can_sync_regs(cs, KVM_SYNC_VRS)) {
>> @@ -690,11 +644,6 @@ int kvm_arch_get_registers(CPUState *cs)
>>           env->fpc = fpu.fpc;
>>       }
>> -    /* The prefix */
>> -    if (can_sync_regs(cs, KVM_SYNC_PREFIX)) {
>> -        env->psa = cs->kvm_run->s.regs.prefix;
>> -    }
>> -
>>       if (can_sync_regs(cs, KVM_SYNC_ARCH0)) {
>>           env->cputm = cs->kvm_run->s.regs.cputm;
>>           env->ckc = cs->kvm_run->s.regs.ckc;
>