Re: [Qemu-ppc] [QEMU-PPC] [PATCH 1/3] target/ppc: Factor out the parsing in kvmppc_get_cpu_characteristics()

[Greg Kurz]

Hi Suraj,

It seems that you forgot to Cc: address@hidden which is mandatory.

On Fri, 11 May 2018 16:25:07 +1000
Suraj Jitindar Singh <address@hidden> wrote:

> Factor out the parsing of struct kvm_ppc_cpu_char in
> kvmppc_get_cpu_characteristics() into a separate function for each cap
> for simplicity.
> 

I'd rather say clarity here.

> Signed-off-by: Suraj Jitindar Singh <address@hidden>
> ---
>  target/ppc/kvm.c | 59 
> +++++++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 39 insertions(+), 20 deletions(-)
> 
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index cbe13b18d1..2c0c34e125 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -2412,6 +2412,41 @@ bool kvmppc_has_cap_mmu_hash_v3(void)
>      return cap_mmu_hash_v3;
>  }
>  
> +static int parse_cap_ppc_safe_cache(struct kvm_ppc_cpu_char c)
> +{
> +    if (~c.behaviour & c.behaviour_mask & H_CPU_BEHAV_L1D_FLUSH_PR) {
> +        return 2;
> +    } else if ((c.character & c.character_mask & H_CPU_CHAR_L1D_THREAD_PRIV) 
> &&
> +               (c.character & c.character_mask
> +                & (H_CPU_CHAR_L1D_FLUSH_ORI30 | 
> H_CPU_CHAR_L1D_FLUSH_TRIG2))) {
> +        return 1;
> +    }
> +
> +    return 0;
> +}
> +
> +static int parse_cap_ppc_safe_bounds_check(struct kvm_ppc_cpu_char c)
> +{
> +    if (~c.behaviour & c.behaviour_mask & H_CPU_BEHAV_BNDS_CHK_SPEC_BAR) {
> +        return 2;
> +    } else if (c.character & c.character_mask & H_CPU_CHAR_SPEC_BAR_ORI31) {
> +        return 1;
> +    }
> +
> +    return 0;
> +}
> +
> +static int parse_cap_ppc_safe_indirect_branch(struct kvm_ppc_cpu_char c)
> +{
> +    if (c.character & c.character_mask & H_CPU_CHAR_CACHE_COUNT_DIS) {
> +        return  SPAPR_CAP_FIXED_CCD;
> +    } else if (c.character & c.character_mask & 
> H_CPU_CHAR_BCCTRL_SERIALISED) {
> +        return SPAPR_CAP_FIXED_IBS;
> +    }
> +
> +    return 0;
> +}
> +
>  static void kvmppc_get_cpu_characteristics(KVMState *s)
>  {
>      struct kvm_ppc_cpu_char c;
> @@ -2430,26 +2465,10 @@ static void kvmppc_get_cpu_characteristics(KVMState 
> *s)
>      if (ret < 0) {
>          return;
>      }
> -    /* Parse and set cap_ppc_safe_cache */
> -    if (~c.behaviour & c.behaviour_mask & H_CPU_BEHAV_L1D_FLUSH_PR) {
> -        cap_ppc_safe_cache = 2;
> -    } else if ((c.character & c.character_mask & H_CPU_CHAR_L1D_THREAD_PRIV) 
> &&
> -               (c.character & c.character_mask
> -                & (H_CPU_CHAR_L1D_FLUSH_ORI30 | 
> H_CPU_CHAR_L1D_FLUSH_TRIG2))) {
> -        cap_ppc_safe_cache = 1;
> -    }
> -    /* Parse and set cap_ppc_safe_bounds_check */
> -    if (~c.behaviour & c.behaviour_mask & H_CPU_BEHAV_BNDS_CHK_SPEC_BAR) {
> -        cap_ppc_safe_bounds_check = 2;
> -    } else if (c.character & c.character_mask & H_CPU_CHAR_SPEC_BAR_ORI31) {
> -        cap_ppc_safe_bounds_check = 1;
> -    }
> -    /* Parse and set cap_ppc_safe_indirect_branch */
> -    if (c.character & c.character_mask & H_CPU_CHAR_CACHE_COUNT_DIS) {
> -        cap_ppc_safe_indirect_branch = SPAPR_CAP_FIXED_CCD;
> -    } else if (c.character & c.character_mask & 
> H_CPU_CHAR_BCCTRL_SERIALISED) {
> -        cap_ppc_safe_indirect_branch = SPAPR_CAP_FIXED_IBS;
> -    }
> +
> +    cap_ppc_safe_cache = parse_cap_ppc_safe_cache(c);
> +    cap_ppc_safe_bounds_check = parse_cap_ppc_safe_bounds_check(c);
> +    cap_ppc_safe_indirect_branch = parse_cap_ppc_safe_indirect_branch(c);

Since the code now always sets the caps, maybe you can drop these
lines upper in kvmppc_get_cpu_characteristics():

    /* Assume broken */
    cap_ppc_safe_cache = 0;
    cap_ppc_safe_bounds_check = 0;
    cap_ppc_safe_indirect_branch = 0;

Anyway,

Reviewed-by: Greg Kurz <address@hidden>

>  }
>  
>  int kvmppc_get_cap_safe_cache(void)