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

[David Gibson]

On Wed, Nov 09, 2016 at 04:18:20PM +1100, Alexey Kardashevskiy wrote:
> On 09/11/16 14:52, David Gibson wrote:
> > On Wed, Nov 09, 2016 at 12:27:47PM +1100, Alexey Kardashevskiy wrote:
> >> On 08/11/16 16:18, David Gibson wrote:
> >>> On Fri, Nov 04, 2016 at 03:01:40PM +1100, Alexey Kardashevskiy wrote:
> >>>> 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).
> >>>
> >>> I was originally going to do that, but decided against it.
> >>>
> >>>> 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?).
> >>>
> >>> It won't be, but there's no theoretical reason they couldn't be.  User
> >>> mode, like spapr, doesn't execute hypervisor privilege code, and so
> >>> the PCR isn't owned by the "guest" (if you can call the user mode
> >>> executable that).  Which means it could make sense to set it from the
> >>> outside, although that's not something we currently do.
> >>
> >> Compatibility modes are designed to disable sets of instructions to keep
> >> working old userspace software which relies on some opcodes to be invalid.
> >>
> >> linux-user is TCG, right? The user can pick any CPU he likes if there is
> >> need to run such an old software, why on earth would anyone bother with
> >> this compat mode in linux-user?
> > 
> > True, I can't really see any reason to do that.
> > 
> > On the other hand, compat mode does at least make theoretical sense,
> > whereas, for example, compat mode on powernv is fundamentally
> > nonsense.  At this point I'm not terribly include to take away the
> > (token) user-only support unless there's a compelling reason *not* to
> > include it.
> 
> What would make a compelling reason? :)
> 
> This will make makefile simpler and will reduce number of #ifdef, and in
> fact it is not supported now anyway, it has not even been tried.

Hm, ok, you convinced me.  I'll make the compat stuff only compile on
softmmu.

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature