From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55241) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1alp2r-0003Ke-0k for qemu-devel@nongnu.org; Thu, 31 Mar 2016 22:42:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1alp2p-0005Ej-LB for qemu-devel@nongnu.org; Thu, 31 Mar 2016 22:42:44 -0400 Date: Fri, 1 Apr 2016 13:43:38 +1100 From: David Gibson Message-ID: <20160401024338.GL416@voom.redhat.com> References: <1459352314-12552-1-git-send-email-clg@fr.ibm.com> <20160331045542.GB416@voom.redhat.com> <56FCCE05.5040303@ilande.co.uk> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="3xQkynibq3FKlJyM" Content-Disposition: inline In-Reply-To: <56FCCE05.5040303@ilande.co.uk> Subject: Re: [Qemu-devel] [Qemu-ppc] [PATCH v2] spapr: compute interrupt vector address from LPCR List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Mark Cave-Ayland Cc: Thomas Huth , =?iso-8859-1?Q?C=E9dric?= Le Goater , qemu-ppc@nongnu.org, qemu-devel@nongnu.org, Greg Kurz --3xQkynibq3FKlJyM Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Mar 31, 2016 at 08:13:09AM +0100, Mark Cave-Ayland wrote: > On 31/03/16 05:55, David Gibson wrote: >=20 > > On Wed, Mar 30, 2016 at 05:38:34PM +0200, C=E9dric Le Goater wrote: > >> This address is changed by the linux kernel using the H_SET_MODE hcall > >> and needs to be restored when migrating a spapr VM running in > >> TCG. This can be done using the AIL bits from the LPCR register. > >> > >> The patch introduces a helper routine cpu_ppc_get_excp_prefix() which > >> returns the effective address offset of the interrupt handler > >> depending on the LPCR_AIL bits. The same helper can be used in the > >> H_SET_MODE hcall, which lets us remove the H_SET_MODE_ADDR_TRANS_* > >> defines. > >> > >> Signed-off-by: C=E9dric Le Goater > >=20 > > I've applied this (with Greg's minor amendments) to ppc-for-2.6), > > since it certainly improves behaviour, although I have a couple of > > queries: > >=20 > >> --- > >> > >> Changes since v1: > >> > >> - moved helper routine under target-ppc/ > >> - moved the restore of excp_prefix under cpu_post_load() > >> > >> hw/ppc/spapr_hcall.c | 13 ++----------- > >> include/hw/ppc/spapr.h | 5 ----- > >> target-ppc/cpu.h | 9 +++++++++ > >> target-ppc/machine.c | 20 +++++++++++++++++++- > >> target-ppc/translate_init.c | 14 ++++++++++++++ > >> 5 files changed, 44 insertions(+), 17 deletions(-) > >> > >> Index: qemu-dgibson-for-2.6.git/hw/ppc/spapr_hcall.c > >> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > >> --- qemu-dgibson-for-2.6.git.orig/hw/ppc/spapr_hcall.c > >> +++ qemu-dgibson-for-2.6.git/hw/ppc/spapr_hcall.c > >> @@ -835,17 +835,8 @@ static target_ulong h_set_mode_resource_ > >> return H_P4; > >> } > >> =20 > >> - switch (mflags) { > >> - case H_SET_MODE_ADDR_TRANS_NONE: > >> - prefix =3D 0; > >> - break; > >> - case H_SET_MODE_ADDR_TRANS_0001_8000: > >> - prefix =3D 0x18000; > >> - break; > >> - case H_SET_MODE_ADDR_TRANS_C000_0000_0000_4000: > >> - prefix =3D 0xC000000000004000ULL; > >> - break; > >> - default: > >> + prefix =3D cpu_ppc_get_excp_prefix(mflags); > >> + if (prefix =3D=3D (target_ulong) -1ULL) { > >> return H_UNSUPPORTED_FLAG; > >> } > >> =20 > >> Index: qemu-dgibson-for-2.6.git/target-ppc/machine.c > >> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > >> --- qemu-dgibson-for-2.6.git.orig/target-ppc/machine.c > >> +++ qemu-dgibson-for-2.6.git/target-ppc/machine.c > >> @@ -156,12 +156,26 @@ static void cpu_pre_save(void *opaque) > >> } > >> } > >> =20 > >> + > >> +static int cpu_post_load_excp_prefix(CPUPPCState *env) > >> +{ > >> + int ail =3D (env->spr[SPR_LPCR] & LPCR_AIL) >> LPCR_AIL_SHIFT; > >> + target_ulong prefix =3D cpu_ppc_get_excp_prefix(ail); > >> + > >> + if (prefix =3D=3D (target_ulong) -1ULL) { > >> + return -EINVAL; > >> + } > >> + env->excp_prefix =3D prefix; > >> + return 0; > >> +} > >> + > >> static int cpu_post_load(void *opaque, int version_id) > >> { > >> PowerPCCPU *cpu =3D opaque; > >> CPUPPCState *env =3D &cpu->env; > >> int i; > >> target_ulong msr; > >> + int ret =3D 0; > >> =20 > >> /* > >> * We always ignore the source PVR. The user or management > >> @@ -201,7 +215,11 @@ static int cpu_post_load(void *opaque, i > >> =20 > >> hreg_compute_mem_idx(env); > >> =20 > >> - return 0; > >> + if (env->spr[SPR_LPCR] & LPCR_AIL) { > >> + ret =3D cpu_post_load_excp_prefix(env); > >> + } > >=20 > > Why not call this unconditionally? If AIL =3D=3D 0 it will still do the > > right thing. > >=20 > > Aren't there also circumstances where the exception prefix can depend > > on the MSR? Do those need to be handled somewhere? >=20 > Yes indeed - this was part of my patchset last year to fix up various > migration issues for the Mac PPC machines (see commit > 2360b6e84f78d41fa0f76555a947148b73645259). >=20 > I agree that having the env->excp_prefix logic split like this isn't a > particularly great idea. Let's just have a single helper function as in > the patch above and use that in both places (and in fact with recent > changes I have a feeling env->excp_prefix is one of the few remaining > reasons that the above change is still required, but please check this). Right. So, what I'd really prefer to see here is a single update_excp_prefix() helper which will set env->excp_prefix based on whatever registers are relevant (LPCR, MSR and probably the cpu model). That would be called on incoming migration, and after updating any of the relevant registers (so from the mtmsr and mtlpcr emulations). H_SET_MODE should be handled by first updating the LPCR value, then calling the update helper. C=E9dric, I'm ok if you provide a version of that which only handles POWER7 and POWER8 (i.e. spapr compatible models for now). Mark - if you can supply corrections to that outline for Macintosh era models, that would be great. I do want to get the basic migration problem fixed before 2.6 is release. --=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 --3xQkynibq3FKlJyM Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJW/eBaAAoJEGw4ysog2bOSmX4P/RXROfdgZC/k0XrWyiaFI/Xl p9iQbZU+7vG7MzrUV8yBdPxm3h6DA6IX7VhG/hJZuOzEv1ylwJcVWlODXWseXNdW mExhgaXUmQGsLiSP0omOfVypstYPc7/bicLbbuDSvdfkE7lsBayOkxJ36yhlUP6W UJl3T9fM8N2hg+9PiMdI61JdDC+clDQR8yd+OpGWenYnZHL52+NT0sOX07s13cQA u7IGOQEhCsE+UnkYbb2s4CTQvvZcaULX/qAq3vwfLZMuwqDF+9swDlL85LJM2bdG 8lsr3V3v4kV2GPlHwZqUANB791tI9OT62PIx1BgeT2dUPCpfOM8tGS4a8cIzsqjp gw+/2jsND+vPm1cQ0NBe0SCdBA3ZWeQaQ6Iy6NevpUcfA3/GMDo22JaD/J91Yo7O bzIQn33U25iIUaLWyG8WWgTBFUGnIB5MMadMEcggVumNpH8MxT5g87/ke9qbyAXy H1ywjM/Tf5Ma5/qTiWfjextt7NK/ua4Zn2D0YekwYUpS1o/pzrv78HZyf4ASDNJL SEsEVxSwcgXlXfvbSW6D5v8jbdnzKtwErM0UoEMcI1WXdn4Dq4Ilhn+7J6YNBfC2 RTR2AX7TIEiwdMqFepY7WXIcg9CIcYKpGAyVjRrWY4crkusVAd5iLJWavnfnSKmC EkNskaYtkMhRIQko/gCV =mnUb -----END PGP SIGNATURE----- --3xQkynibq3FKlJyM--