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