On 31/03/16 05:55, David Gibson wrote: > On Wed, Mar 30, 2016 at 05:38:34PM +0200, Cédric 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édric Le Goater <c...@fr.ibm.com> > > 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 >> =================================================================== >> --- 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; >> } >> >> - switch (mflags) { >> - case H_SET_MODE_ADDR_TRANS_NONE: >> - prefix = 0; >> - break; >> - case H_SET_MODE_ADDR_TRANS_0001_8000: >> - prefix = 0x18000; >> - break; >> - case H_SET_MODE_ADDR_TRANS_C000_0000_0000_4000: >> - prefix = 0xC000000000004000ULL; >> - break; >> - default: >> + prefix = cpu_ppc_get_excp_prefix(mflags); >> + if (prefix == (target_ulong) -1ULL) { >> return H_UNSUPPORTED_FLAG; >> } >> >> Index: qemu-dgibson-for-2.6.git/target-ppc/machine.c >> =================================================================== >> --- 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) >> } >> } >> >> + >> +static int cpu_post_load_excp_prefix(CPUPPCState *env) >> +{ >> + int ail = (env->spr[SPR_LPCR] & LPCR_AIL) >> LPCR_AIL_SHIFT; >> + target_ulong prefix = cpu_ppc_get_excp_prefix(ail); >> + >> + if (prefix == (target_ulong) -1ULL) { >> + return -EINVAL; >> + } >> + env->excp_prefix = prefix; >> + return 0; >> +} >> + >> static int cpu_post_load(void *opaque, int version_id) >> { >> PowerPCCPU *cpu = opaque; >> CPUPPCState *env = &cpu->env; >> int i; >> target_ulong msr; >> + int ret = 0; >> >> /* >> * We always ignore the source PVR. The user or management >> @@ -201,7 +215,11 @@ static int cpu_post_load(void *opaque, i >> >> hreg_compute_mem_idx(env); >> >> - return 0; >> + if (env->spr[SPR_LPCR] & LPCR_AIL) { >> + ret = cpu_post_load_excp_prefix(env); >> + } > > Why not call this unconditionally? If AIL == 0 it will still do the > right thing. > > Aren't there also circumstances where the exception prefix can depend > 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 this). ATB, Mark.