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

[Suraj Jitindar Singh]

On Fri, 2018-05-11 at 19:28 +0200, Greg Kurz wrote:
> Hi Suraj,
> 
> It seems that you forgot to Cc: address@hidden which is
> mandatory.

Ah yes...

> 
> 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);

We return before here if the host doesn't have the capability or fails.
I guess since these are global they default to zero... I would still
rather set them for clarity.

> 
> 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,

Thanks :)

> 
> Reviewed-by: Greg Kurz <address@hidden>
> 
> >  }
> >  
> >  int kvmppc_get_cap_safe_cache(void)
> 
>