Re: [Qemu-ppc] [RFC 11/17] ppc: Add ppc_set_compat_all()

[Alexey Kardashevskiy]

On 30/10/16 22:12, David Gibson wrote:
> Once a compatiblity mode is negotiated with the guest,
> h_client_architecture_support() uses run_on_cpu() to update each CPU to
> the new mode.  We're going to want this logic somewhere else shortly,
> so make a helper function to do this global update.
> 
> We put it in target-ppc/compat.c - it makes as much sense at the CPU level
> as it does at the machine level.  We also move the cpu_synchronize_state()
> into ppc_set_compat(), since it doesn't really make any sense to call that
> without synchronizing state.
> 
> Signed-off-by: David Gibson <address@hidden>
> ---
>  hw/ppc/spapr_hcall.c | 31 +++++--------------------------
>  target-ppc/compat.c  | 36 ++++++++++++++++++++++++++++++++++++
>  target-ppc/cpu.h     |  3 +++
>  3 files changed, 44 insertions(+), 26 deletions(-)
> 
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 3bd6d06..4eaf9a6 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -881,20 +881,6 @@ static target_ulong h_set_mode(PowerPCCPU *cpu, 
> sPAPRMachineState *spapr,
>      return ret;
>  }
>  
> -typedef struct {
> -    uint32_t compat_pvr;
> -    Error *err;
> -} SetCompatState;
> -
> -static void do_set_compat(CPUState *cs, void *arg)
> -{
> -    PowerPCCPU *cpu = POWERPC_CPU(cs);
> -    SetCompatState *s = arg;
> -
> -    cpu_synchronize_state(cs);
> -    ppc_set_compat(cpu, s->compat_pvr, &s->err);
> -}
> -
>  static target_ulong h_client_architecture_support(PowerPCCPU *cpu,
>                                                    sPAPRMachineState *spapr,
>                                                    target_ulong opcode,
> @@ -902,7 +888,6 @@ static target_ulong 
> h_client_architecture_support(PowerPCCPU *cpu,
>  {
>      target_ulong list = ppc64_phys_to_real(args[0]);
>      target_ulong ov_table;
> -    CPUState *cs;
>      bool explicit_match = false; /* Matched the CPU's real PVR */
>      uint32_t max_compat = cpu->max_compat;
>      uint32_t best_compat = 0;
> @@ -949,18 +934,12 @@ static target_ulong 
> h_client_architecture_support(PowerPCCPU *cpu,
>  
>      /* Update CPUs */
>      if (cpu->compat_pvr != best_compat) {
> -        CPU_FOREACH(cs) {
> -            SetCompatState s = {
> -                .compat_pvr = best_compat,
> -                .err = NULL,
> -            };
> +        Error *local_err = NULL;
>  
> -            run_on_cpu(cs, do_set_compat, &s);
> -
> -            if (s.err) {
> -                error_report_err(s.err);
> -                return H_HARDWARE;
> -            }
> +        ppc_set_compat_all(best_compat, &local_err);
> +        if (local_err) {
> +            error_report_err(local_err);
> +            return H_HARDWARE;
>          }
>      }
>  
> diff --git a/target-ppc/compat.c b/target-ppc/compat.c
> index 1059555..0b12b58 100644
> --- a/target-ppc/compat.c
> +++ b/target-ppc/compat.c
> @@ -124,6 +124,8 @@ void ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, 
> Error **errp)
>          pcr = compat->pcr;
>      }
>  
> +    cpu_synchronize_state(CPU(cpu));
> +
>      cpu->compat_pvr = compat_pvr;
>      env->spr[SPR_PCR] = pcr & pcc->pcr_mask;
>  
> @@ -136,6 +138,40 @@ void ppc_set_compat(PowerPCCPU *cpu, uint32_t 
> compat_pvr, Error **errp)
>      }
>  }
>  
> +#if !defined(CONFIG_USER_ONLY)
> +typedef struct {
> +    uint32_t compat_pvr;
> +    Error *err;
> +} SetCompatState;
> +
> +static void do_set_compat(CPUState *cs, void *arg)
> +{
> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> +    SetCompatState *s = arg;
> +
> +    ppc_set_compat(cpu, s->compat_pvr, &s->err);
> +}
> +
> +void ppc_set_compat_all(uint32_t compat_pvr, Error **errp)
> +{
> +    CPUState *cs;
> +
> +    CPU_FOREACH(cs) {
> +        SetCompatState s = {
> +            .compat_pvr = compat_pvr,
> +            .err = NULL,
> +        };
> +
> +        run_on_cpu(cs, do_set_compat, &s);
> +
> +        if (s.err) {
> +            error_propagate(errp, s.err);
> +            return;
> +        }
> +    }
> +}
> +#endif
> +
>  int ppc_compat_max_threads(PowerPCCPU *cpu)
>  {
>      const CompatInfo *compat = compat_by_pvr(cpu->compat_pvr);
> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> index 91e8be8..201a655 100644
> --- a/target-ppc/cpu.h
> +++ b/target-ppc/cpu.h
> @@ -1317,6 +1317,9 @@ static inline int cpu_mmu_index (CPUPPCState *env, bool 
> ifetch)
>  bool ppc_check_compat(PowerPCCPU *cpu, uint32_t compat_pvr,
>                        uint32_t min_compat_pvr, uint32_t max_compat_pvr);
>  void ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, Error **errp);
> +#if !defined(CONFIG_USER_ONLY)
> +void ppc_set_compat_all(uint32_t compat_pvr, Error **errp);
> +#endif

I would put all ppc*compat*() under #if defined(CONFIG_USER_ONLY) &&
defined(TARGET_PPC64) (or even moved this to target-ppc/Makefile.objs).
Otherwise, functions like ppc_check_compat() have #if
!defined(CONFIG_USER_ONLY) which suggests that the rest of
ppc_check_compat() can actually be executed in ppc64-linux-user (while it
cannot, can it?).




>  int ppc_compat_max_threads(PowerPCCPU *cpu);
>  #endif /* defined(TARGET_PPC64) */
>  
> 


-- 
Alexey