Re: [PATCH v3 2/2] target/s390x/kvm: Simplify the GPRs, ACRs, CRs and prefix synchronization code

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

On 10/11/23 10:05, 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 also 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>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   target/s390x/kvm/kvm.c | 106 ++++++++++-------------------------------
>   1 file changed, 24 insertions(+), 82 deletions(-)
> 
> diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
> index b3e2eaa2eb..dc76f63d94 100644
> --- a/target/s390x/kvm/kvm.c
> +++ b/target/s390x/kvm/kvm.c
> @@ -138,7 +138,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;
> @@ -359,7 +358,6 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>           return -1;
>       }
>   
> -    cap_sync_regs = true;
>       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);
> @@ -466,37 +464,28 @@ 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;
>   }
>   
> +#define KVM_SYNC_REQUIRED_REGS (KVM_SYNC_GPRS | KVM_SYNC_ACRS | \
> +                                KVM_SYNC_CRS | KVM_SYNC_PREFIX)
> +
>   int kvm_arch_put_registers(CPUState *cs, int level)
>   {
>       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;
>   
> +    g_assert(can_sync_regs(cs, KVM_SYNC_REQUIRED_REGS));
> +
>       /* always save the PSW  and the GPRS*/
>       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;
> -        }
> -    }
> +    memcpy(cs->kvm_run->s.regs.gprs, env->regs, sizeof(cs->kvm_run->s.regs.gprs));
> +    cs->kvm_run->kvm_dirty_regs |= KVM_SYNC_GPRS;
>   
>       if (can_sync_regs(cs, KVM_SYNC_VRS)) {
>           for (i = 0; i < 32; i++) {
> @@ -529,6 +518,15 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>           return 0;
>       }
>   
> +    /*
> +     * Access registers, control registers and the prefix - these are
> +     * always available via kvm_sync_regs in the kernels that we support
> +     */
> +    memcpy(cs->kvm_run->s.regs.acrs, env->aregs, sizeof(cs->kvm_run->s.regs.acrs));
> +    memcpy(cs->kvm_run->s.regs.crs, env->cregs, sizeof(cs->kvm_run->s.regs.crs));
> +    cs->kvm_run->s.regs.prefix = env->psa;
> +    cs->kvm_run->kvm_dirty_regs |= KVM_SYNC_ACRS | KVM_SYNC_CRS | KVM_SYNC_PREFIX;
> +
>       if (can_sync_regs(cs, KVM_SYNC_ARCH0)) {
>           cs->kvm_run->s.regs.cputm = env->cputm;
>           cs->kvm_run->s.regs.ckc = env->ckc;
> @@ -575,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;
> @@ -615,13 +594,6 @@ 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;
>   }
>   
> @@ -629,8 +601,6 @@ int kvm_arch_get_registers(CPUState *cs)
>   {
>       S390CPU *cpu = S390_CPU(cs);
>       CPUS390XState *env = &cpu->env;
> -    struct kvm_sregs sregs;
> -    struct kvm_regs regs;
>       struct kvm_fpu fpu;
>       int i, r;
>   
> @@ -638,37 +608,14 @@ 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(can_sync_regs(cs, KVM_SYNC_REQUIRED_REGS));
> +    memcpy(env->regs, cs->kvm_run->s.regs.gprs, sizeof(env->regs));
> +    memcpy(env->aregs, cs->kvm_run->s.regs.acrs, sizeof(env->aregs));
> +    memcpy(env->cregs, cs->kvm_run->s.regs.crs, sizeof(env->cregs));
>   
> -    /* 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)) {
> @@ -693,11 +640,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;