Re: [Qemu-devel] [PATCH 3/7 v2] KVM regsync: S390: Implement kvm_arch_[get/put]_registers regmap paramter

[Alexander Graf]

On 10.01.2013, at 16:28, Jason J. Herne wrote:

> From: "Jason J. Herne" <address@hidden>
> 
> S390 re-implementation of kvm_arch_get_registers and kvm_arch_put_registers
> functions to take advantage of the register map parameter.
> 
> Signed-off-by: Jason J. Herne <address@hidden>
> Reviewed-by: Christian Borntraeger <address@hidden>
> ---
> target-s390x/kvm.c |  165 ++++++++++++++++++++++++++--------------------------
> 1 file changed, 84 insertions(+), 81 deletions(-)
> 
> diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c
> index 4b87f1c..ab6b27b 100644
> --- a/target-s390x/kvm.c
> +++ b/target-s390x/kvm.c
> @@ -88,7 +88,7 @@ void kvm_arch_reset_vcpu(CPUState *cpu)
>     /* FIXME: add code to reset vcpu. */
> }
> 
> -int kvm_arch_put_registers(CPUState *cs, int level)
> +int kvm_arch_put_registers(CPUState *cs, int regmap)
> {
>     S390CPU *cpu = S390_CPU(cs);
>     CPUS390XState *env = &cpu->env;
> @@ -97,57 +97,56 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>     int ret;
>     int i;
> 
> -    /* always save the PSW  and the GPRS*/
> -    cs->kvm_run->psw_addr = env->psw.addr;
> -    cs->kvm_run->psw_mask = env->psw.mask;
> +    if (regmap & KVM_REGSYNC_S390_RUNTIME_REGS) {
> +        cs->kvm_run->psw_addr = env->psw.addr;
> +        cs->kvm_run->psw_mask = env->psw.mask;
> 
> -    if (cap_sync_regs && cs->kvm_run->kvm_valid_regs & 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];
> -        }
> -        ret = kvm_vcpu_ioctl(cs, KVM_SET_REGS, &regs);
> -        if (ret < 0) {
> -            return ret;
> +        if (cap_sync_regs && cs->kvm_run->kvm_valid_regs & 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];
> +            }
> +            ret = kvm_vcpu_ioctl(cs, KVM_SET_REGS, &regs);
> +            if (ret < 0) {
> +                return ret;
> +            }
>         }
>     }
> 
> -    /* Do we need to save more than that? */
> -    if (level == KVM_REGSYNC_RUNTIME_STATE) {
> -        return 0;
> -    }
> -
> -    if (cap_sync_regs &&
> -        cs->kvm_run->kvm_valid_regs & KVM_SYNC_ACRS &&
> -        cs->kvm_run->kvm_valid_regs & 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];
> +    if (regmap & KVM_REGSYNC_S390_RESET_REGS) {
> +        if (cap_sync_regs &&
> +            cs->kvm_run->kvm_valid_regs & KVM_SYNC_ACRS &&
> +            cs->kvm_run->kvm_valid_regs & 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];
> +            }
> +            ret = kvm_vcpu_ioctl(cs, KVM_SET_SREGS, &sregs);
> +            if (ret < 0) {
> +                return ret;
> +            }
>         }
> -        ret = kvm_vcpu_ioctl(cs, KVM_SET_SREGS, &sregs);
> -        if (ret < 0) {
> -            return ret;
> +
> +        /* Finally the prefix */
> +        if (cap_sync_regs && cs->kvm_run->kvm_valid_regs & 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 */
>         }
>     }
> 
> -    /* Finally the prefix */
> -    if (cap_sync_regs && cs->kvm_run->kvm_valid_regs & 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;
> }
> 
> @@ -160,49 +159,53 @@ int kvm_arch_get_registers(CPUState *cs, int regmap)
>     int ret;
>     int i;
> 
> -    /* get the PSW */
> -    env->psw.addr = cs->kvm_run->psw_addr;
> -    env->psw.mask = cs->kvm_run->psw_mask;
> +    if (regmap & KVM_REGSYNC_S390_RUNTIME_REGS) {
> +        /* get the PSW */
> +        env->psw.addr = cs->kvm_run->psw_addr;
> +        env->psw.mask = cs->kvm_run->psw_mask;
> 
> -    /* the GPRS */
> -    if (cap_sync_regs && cs->kvm_run->kvm_valid_regs & KVM_SYNC_GPRS) {
> -        for (i = 0; i < 16; i++) {
> -            env->regs[i] = cs->kvm_run->s.regs.gprs[i];
> -        }
> -    } else {
> -        ret = kvm_vcpu_ioctl(cs, KVM_GET_REGS, &regs);
> -        if (ret < 0) {
> -            return ret;
> -        }
> -         for (i = 0; i < 16; i++) {
> -            env->regs[i] = regs.gprs[i];
> +        /* the GPRS */
> +        if (cap_sync_regs && cs->kvm_run->kvm_valid_regs & KVM_SYNC_GPRS) {
> +            for (i = 0; i < 16; i++) {
> +                env->regs[i] = cs->kvm_run->s.regs.gprs[i];
> +            }
> +        } else {
> +            ret = kvm_vcpu_ioctl(cs, KVM_GET_REGS, &regs);
> +            if (ret < 0) {
> +                return ret;
> +            }
> +             for (i = 0; i < 16; i++) {
> +                env->regs[i] = regs.gprs[i];
> +            }
>         }
>     }
> 
> -    /* The ACRS and CRS */
> -    if (cap_sync_regs &&
> -        cs->kvm_run->kvm_valid_regs & KVM_SYNC_ACRS &&
> -        cs->kvm_run->kvm_valid_regs & 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 {
> -        ret = kvm_vcpu_ioctl(cs, KVM_GET_SREGS, &sregs);
> -        if (ret < 0) {
> -            return ret;
> -        }
> -         for (i = 0; i < 16; i++) {
> -            env->aregs[i] = sregs.acrs[i];
> -            env->cregs[i] = sregs.crs[i];
> +    if (regmap & KVM_REGSYNC_S390_RESET_REGS) {

Here you're changing the way arch_get_registers() behaves in a patch that 
should be neutral for external callers. Please extract the bits where you 
individually get registers into a separate patch with separate explanation why 
you're restricting it.

The basic idea behind RESET_REGS is that you don't have to sync them. They are 
usually write-only. So get(RESET) should really never happen.


Alex

> +        /* The ACRS and CRS */
> +        if (cap_sync_regs &&
> +            cs->kvm_run->kvm_valid_regs & KVM_SYNC_ACRS &&
> +            cs->kvm_run->kvm_valid_regs & 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 {
> +            ret = kvm_vcpu_ioctl(cs, KVM_GET_SREGS, &sregs);
> +            if (ret < 0) {
> +                return ret;
> +            }
> +             for (i = 0; i < 16; i++) {
> +                env->aregs[i] = sregs.acrs[i];
> +                env->cregs[i] = sregs.crs[i];
> +            }
>         }
> -    }
> 
> -    /* Finally the prefix */
> -    if (cap_sync_regs && cs->kvm_run->kvm_valid_regs & KVM_SYNC_PREFIX) {
> -        env->psa = cs->kvm_run->s.regs.prefix;
> -    } else {
> -        /* no prefix without sync regs */
> +        /* Finally the prefix */
> +        if (cap_sync_regs && cs->kvm_run->kvm_valid_regs & KVM_SYNC_PREFIX) {
> +            env->psa = cs->kvm_run->s.regs.prefix;
> +        } else {
> +            /* no prefix without sync regs */
> +        }
>     }
> 
>     return 0;
> -- 
> 1.7.9.5
>