From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58597) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1anFOd-0002gO-Qf for qemu-devel@nongnu.org; Mon, 04 Apr 2016 21:03:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1anFOZ-0003aG-8s for qemu-devel@nongnu.org; Mon, 04 Apr 2016 21:03:07 -0400 Date: Tue, 5 Apr 2016 10:54:06 +1000 From: David Gibson Message-ID: <20160405005406.GP16485@voom.fritz.box> References: <1459352314-12552-1-git-send-email-clg@fr.ibm.com> <20160331045542.GB416@voom.redhat.com> <56FCCE05.5040303@ilande.co.uk> <20160401024338.GL416@voom.redhat.com> <5701599E.7050503@fr.ibm.com> <20160404041632.GI16485@voom.fritz.box> <57027E99.8090704@fr.ibm.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="LpcCHpaCAbC4X43d" Content-Disposition: inline In-Reply-To: <57027E99.8090704@fr.ibm.com> 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: =?iso-8859-1?Q?C=E9dric?= Le Goater Cc: qemu-ppc@nongnu.org, Thomas Huth , Mark Cave-Ayland , qemu-devel@nongnu.org, Greg Kurz --LpcCHpaCAbC4X43d Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Apr 04, 2016 at 04:47:53PM +0200, C=E9dric Le Goater wrote: > On 04/04/2016 06:16 AM, David Gibson wrote: > > On Sun, Apr 03, 2016 at 07:57:50PM +0200, C=E9dric Le Goater wrote: > >> On 04/01/2016 04:43 AM, David Gibson wrote: > >>> On Thu, Mar 31, 2016 at 08:13:09AM +0100, Mark Cave-Ayland wrote: > >>>> On 31/03/16 05:55, David Gibson wrote: > >>>> > >>>>> 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 h= call > >>>>>> 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() wh= ich > >>>>>> 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 > >>>>> > >>>>> 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: > >>>>> > >>>>>> --- > >>>>>> > >>>>>> 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); > >>>>>> + } > >>>>> > >>>>> Why not call this unconditionally? If AIL =3D=3D 0 it will still d= o the > >>>>> right thing. > >>>>> > >>>>> Aren't there also circumstances where the exception prefix can depe= nd > >>>>> on the MSR? Do those need to be handled somewhere? > >>>> > >>>> Yes indeed - this was part of my patchset last year to fix up various > >>>> migration issues for the Mac PPC machines (see commit > >>>> 2360b6e84f78d41fa0f76555a947148b73645259). > >>>> > >>>> 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 th= is). > >>> > >>> 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). =20 > >> > >> Sure.=20 > >> > >> But first, could you please take a look at a reworked patch of Ben who= =20 > >> had forseen the issue ? I kept the AIL fix, added some extra for ILE > >> and cleanup H_SET_MODE. Tests under KVM,TCG on LE,BE guests went fine. > >> > >> If you think this is too risky for 2.6, I will work on what you asked = for. > >=20 > > Ah, yes this approach does seem more correct. It looks like it leaves > > room for further cleanups to excp_prefix later (such as completely > > removing it in favour of calculating it from reg values at exception > > time). =20 >=20 > Yes. There is still a : >=20 > vector |=3D env->excp_prefix; >=20 > which is a bit confusing for P7/P8 guests as 'excp_prefix' is not used.= =20 > The code section calculating 'vector' in powerpc_excp() could be isolated= =20 > in its own routine I guess. We could clarify things there. Right. > > But for now it looks like it will address the migration issue at hand. > >=20 > > I've applied it to ppc-for-2.6. >=20 > Tell me if you want a proper resend by mail. No, I think I have it adequately cleaned up myself. --=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 --LpcCHpaCAbC4X43d Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJXAwyuAAoJEGw4ysog2bOSF0AP/jGOMuRMCFE/OBNrWJwBJmBr JyY84ctaC71bVqT6sIf9UmV7PudtDH7p72ZlPNjXYLP7cLSK7dlNstG3ZE+k+/U2 0NQgs06tqJCDCX6ZDg/5iSW8lCi4lrauVUmv4pOkHWVPUrzVl8Oai+Wpj3z2D0Ht E4lQccn6Ink/MuE2Hfq1QiEdWZHywaoQKl7KvIM3k6PjWENMuEkFp52qRPnqNWyS Yyr+ebGy762LYp1jyYHIXXdCIclpr9sLO8RDtbgeur6QEOMUemh//8jKREyqRXS5 cFZFdrbPZCObYmHRhbqEJXSTM4e351nIzYOplBX10ZZAQCFeLSndVV/BxC65v6ZW +NvsZCrRjpPu3pdP2kfGNVEa7cwmCJaQpG8jBdzklDJXordfxOs8016y0d82ktp8 Hld+1CczaYc0jfM/A72jY4MEHEVopvpLFMHkAT6YjKyqpJKQYE+ULwlMLQtaDVbP cojFvE3GpfprbeBab08KCR+eZW666kaak3la5RwbJCNgeHlr54jhyNi/1ZGxcaH2 4AwU9Yzb2nVYrOEajlVYKgbtqHOXtizch6jWVYukaoA0Dy6sg8LXPtKduilyoz4u KRY6PPfO7KHFnqPL1ZBgDIQ2v+Z8ieFlEhd8F5U0jTby5cEvkXMT4debQYSj+BBN lZ1qwLqwxSzy3DrS5hfk =BqYg -----END PGP SIGNATURE----- --LpcCHpaCAbC4X43d--