From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48049) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c4gGN-00085k-8X for qemu-devel@nongnu.org; Wed, 09 Nov 2016 22:42:56 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c4gGL-0000Uh-Q3 for qemu-devel@nongnu.org; Wed, 09 Nov 2016 22:42:55 -0500 Date: Thu, 10 Nov 2016 14:13:27 +1100 From: David Gibson Message-ID: <20161110031327.GG18060@umbus.fritz.box> References: <1477825928-10803-1-git-send-email-david@gibson.dropbear.id.au> <1477825928-10803-12-git-send-email-david@gibson.dropbear.id.au> <20161108051847.GS28688@umbus.fritz.box> <40dfd693-9f80-4492-4c96-8e608d3527d7@ozlabs.ru> <20161109035229.GD427@umbus.fritz.box> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="xs+9IvWevLaxKUtW" Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [RFC 11/17] ppc: Add ppc_set_compat_all() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexey Kardashevskiy Cc: nikunj@linux.vnet.ibm.com, mdroth@linux.vnet.ibm.com, thuth@redhat.com, lvivier@redhat.com, qemu-ppc@nongnu.org, qemu-devel@nongnu.org --xs+9IvWevLaxKUtW Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 CP= U to > >>>>> the new mode. We're going to want this logic somewhere else shortl= y, > >>>>> 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 CP= U 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 ca= ll that > >>>>> without synchronizing state. > >>>>> > >>>>> Signed-off-by: David Gibson > >>>>> --- > >>>>> 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; > >>>>> } > >>>>> =20 > >>>>> -typedef struct { > >>>>> - uint32_t compat_pvr; > >>>>> - Error *err; > >>>>> -} SetCompatState; > >>>>> - > >>>>> -static void do_set_compat(CPUState *cs, void *arg) > >>>>> -{ > >>>>> - PowerPCCPU *cpu =3D POWERPC_CPU(cs); > >>>>> - SetCompatState *s =3D arg; > >>>>> - > >>>>> - cpu_synchronize_state(cs); > >>>>> - ppc_set_compat(cpu, s->compat_pvr, &s->err); > >>>>> -} > >>>>> - > >>>>> static target_ulong h_client_architecture_support(PowerPCCPU *cpu, > >>>>> sPAPRMachineStat= e *spapr, > >>>>> target_ulong opc= ode, > >>>>> @@ -902,7 +888,6 @@ static target_ulong h_client_architecture_suppo= rt(PowerPCCPU *cpu, > >>>>> { > >>>>> target_ulong list =3D ppc64_phys_to_real(args[0]); > >>>>> target_ulong ov_table; > >>>>> - CPUState *cs; > >>>>> bool explicit_match =3D false; /* Matched the CPU's real PVR */ > >>>>> uint32_t max_compat =3D cpu->max_compat; > >>>>> uint32_t best_compat =3D 0; > >>>>> @@ -949,18 +934,12 @@ static target_ulong h_client_architecture_sup= port(PowerPCCPU *cpu, > >>>>> =20 > >>>>> /* Update CPUs */ > >>>>> if (cpu->compat_pvr !=3D best_compat) { > >>>>> - CPU_FOREACH(cs) { > >>>>> - SetCompatState s =3D { > >>>>> - .compat_pvr =3D best_compat, > >>>>> - .err =3D NULL, > >>>>> - }; > >>>>> + Error *local_err =3D NULL; > >>>>> =20 > >>>>> - 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; > >>>>> } > >>>>> } > >>>>> =20 > >>>>> 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 c= ompat_pvr, Error **errp) > >>>>> pcr =3D compat->pcr; > >>>>> } > >>>>> =20 > >>>>> + cpu_synchronize_state(CPU(cpu)); > >>>>> + > >>>>> cpu->compat_pvr =3D compat_pvr; > >>>>> env->spr[SPR_PCR] =3D pcr & pcc->pcr_mask; > >>>>> =20 > >>>>> @@ -136,6 +138,40 @@ void ppc_set_compat(PowerPCCPU *cpu, uint32_t = compat_pvr, Error **errp) > >>>>> } > >>>>> } > >>>>> =20 > >>>>> +#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 =3D POWERPC_CPU(cs); > >>>>> + SetCompatState *s =3D 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 =3D { > >>>>> + .compat_pvr =3D compat_pvr, > >>>>> + .err =3D 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 =3D 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.obj= s). > >>> > >>> 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 (whi= le 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 ke= ep > >> working old userspace software which relies on some opcodes to be inva= lid. > >> > >> 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? > >=20 > > True, I can't really see any reason to do that. > >=20 > > 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. >=20 > What would make a compelling reason? :) >=20 > 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. --=20 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 --xs+9IvWevLaxKUtW Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJYI+XTAAoJEGw4ysog2bOScLIQANF2T11cu8rKOCjioKSk/LUu ybL5v+ZnqKMRnRU2mvj5ACLq6CuzRmJt8Ciz4JA4Lr5oFCqusm/xYB/7/bICyoRu bQOmmDkWnCRIQS0YW2VVrqTY0y6IzLzXJwkThUzS/Pox4fYUNHKhnCopxG33ojNe DWaaZ1B6z8nOWnMZfC3fAI4fQC/Z+E2YfpUZc/ketFW55G+X7S5s+lHBLylXTz3S pht5gjQ+ftiahDIXnBDhOu2EyEl5Mk7nBxeMQYXaP10/MPuPAuigTc+JMIr0OTy5 Yw/CHUiWr5o2OIUzYReUTYOid6qB9KaKVwVZxlxBO/S8U502SNIbcwoFiENkP7Lz Xwbttc6aZva/c9R3VOOKAl9q0J/hDL/hhOH2GrVvE2cZWRvwjmEGFbPfCV3MXXFX V213OwouY4yEVw8ZvFW41ryOjST16wUXRaNGidU3hT3RM37othOwTcM8KZVecnqX Kh7UpyohmqFUN1G7DnFBOGMG9tanjHg3RkS6vFbDztC8BErSqSnquimXjx23YqQE XMW1Ms9mLy/kwuHcSf65ig7dCl+f2RR9WUzz+Zq1dYbHxZdp+89P8zyfKLb7q/CP jt1orPDEOhXxIZBsak89UcFNF15hp9n69eI6z7Ul/hB7N2ZRyu2zyd2TRnF9xoEw SkdwBPY/xQVUEHME1Q/q =7keh -----END PGP SIGNATURE----- --xs+9IvWevLaxKUtW--