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 <da...@gibson.dropbear.id.au>
> >>>>> ---
> >>>>> 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
