| Message ID | 5701599E.7050503@fr.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Apr 03, 2016 at 07:57:50PM +0200, Cédric 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é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 <clg@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). > > > > 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édric, I'm ok if you provide a version of that which only handles > > POWER7 and POWER8 (i.e. spapr compatible models for now). > > Sure. > > But first, could you please take a look at a reworked patch of Ben who > 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. 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). But for now it looks like it will address the migration issue at hand. I've applied it to ppc-for-2.6. > > > 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. > > > Thanks, > > C. > > >From bb9d1e06a0ea2132902db724f61c8dc655aa1a96 Mon Sep 17 00:00:00 2001 > From: Benjamin Herrenschmidt <benh@kernel.crashing.org> > Date: Thu, 4 Jun 2015 03:39:19 +1000 > Subject: [PATCH 18/77] ppc: Rework POWER7 & POWER8 exception model > > This patch fixes the current AIL implementation for POWER8. The > interrupt vector address can be calculated directly from LPCR when the > exception is handled. The excp_prefix update becomes useless and we > can cleanup the H_SET_MODE hcall. > > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> > [clg: Removed LPES0/1 handling for HV vs. !HV > Fixed LPCR_ILE case for POWERPC_EXCP_POWER8 ] > Signed-off-by: Cédric Le Goater <clg@fr.ibm.com> > --- > hw/ppc/spapr_hcall.c | 16 -------------- > include/hw/ppc/spapr.h | 5 ---- > target-ppc/cpu.h | 10 ++++++++ > target-ppc/excp_helper.c | 49 ++++++++++++++++++++++++++++++++++++++++++-- > target-ppc/translate_init.c | 2 - > 5 files changed, 59 insertions(+), 23 deletions(-) > > Index: qemu-dgibson-for-2.6.git/target-ppc/cpu.h > =================================================================== > --- qemu-dgibson-for-2.6.git.orig/target-ppc/cpu.h > +++ qemu-dgibson-for-2.6.git/target-ppc/cpu.h > @@ -167,6 +167,8 @@ enum powerpc_excp_t { > POWERPC_EXCP_970, > /* POWER7 exception model */ > POWERPC_EXCP_POWER7, > + /* POWER8 exception model */ > + POWERPC_EXCP_POWER8, > #endif /* defined(TARGET_PPC64) */ > }; > > @@ -2277,6 +2279,14 @@ enum { > HMER_XSCOM_STATUS_LSH = (63 - 23), > }; > > +/* Alternate Interrupt Location (AIL) */ > +enum { > + AIL_NONE = 0, > + AIL_RESERVED = 1, > + AIL_0001_8000 = 2, > + AIL_C000_0000_0000_4000 = 3, > +}; > + > /*****************************************************************************/ > > static inline target_ulong cpu_read_xer(CPUPPCState *env) > Index: qemu-dgibson-for-2.6.git/target-ppc/excp_helper.c > =================================================================== > --- qemu-dgibson-for-2.6.git.orig/target-ppc/excp_helper.c > +++ qemu-dgibson-for-2.6.git/target-ppc/excp_helper.c > @@ -77,7 +77,7 @@ static inline void powerpc_excp(PowerPCC > CPUPPCState *env = &cpu->env; > target_ulong msr, new_msr, vector; > int srr0, srr1, asrr0, asrr1; > - int lpes0, lpes1, lev; > + int lpes0, lpes1, lev, ail; > > if (0) { > /* XXX: find a suitable condition to enable the hypervisor mode */ > @@ -108,6 +108,25 @@ static inline void powerpc_excp(PowerPCC > asrr0 = -1; > asrr1 = -1; > > + /* Exception targetting modifiers > + * > + * AIL is initialized here but can be cleared by > + * selected exceptions > + */ > +#if defined(TARGET_PPC64) > + if (excp_model == POWERPC_EXCP_POWER7 || > + excp_model == POWERPC_EXCP_POWER8) { > + if (excp_model == POWERPC_EXCP_POWER8) { > + ail = (env->spr[SPR_LPCR] & LPCR_AIL) >> LPCR_AIL_SHIFT; > + } else { > + ail = 0; > + } > + } else > +#endif /* defined(TARGET_PPC64) */ > + { > + ail = 0; > + } > + > switch (excp) { > case POWERPC_EXCP_NONE: > /* Should never happen */ > @@ -146,6 +165,7 @@ static inline void powerpc_excp(PowerPCC > /* XXX: find a suitable condition to enable the hypervisor mode */ > new_msr |= (target_ulong)MSR_HVB; > } > + ail = 0; > > /* machine check exceptions don't have ME set */ > new_msr &= ~((target_ulong)1 << MSR_ME); > @@ -344,6 +364,7 @@ static inline void powerpc_excp(PowerPCC > /* XXX: find a suitable condition to enable the hypervisor mode */ > new_msr |= (target_ulong)MSR_HVB; > } > + ail = 0; > goto store_next; > case POWERPC_EXCP_DSEG: /* Data segment exception */ > if (lpes1 == 0) { > @@ -630,7 +651,8 @@ static inline void powerpc_excp(PowerPCC > } > > #ifdef TARGET_PPC64 > - if (excp_model == POWERPC_EXCP_POWER7) { > + if (excp_model == POWERPC_EXCP_POWER7 || > + excp_model == POWERPC_EXCP_POWER8) { > if (env->spr[SPR_LPCR] & LPCR_ILE) { > new_msr |= (target_ulong)1 << MSR_LE; > } > @@ -650,6 +672,29 @@ static inline void powerpc_excp(PowerPCC > excp); > } > vector |= env->excp_prefix; > + > + /* AIL only works if there is no HV transition and we are running with > + * translations enabled > + */ > + if (!((msr >> MSR_IR) & 1) || !((msr >> MSR_DR) & 1)) { > + ail = 0; > + } > + /* Handle AIL */ > + if (ail) { > + new_msr |= (1 << MSR_IR) | (1 << MSR_DR); > + switch(ail) { > + case AIL_0001_8000: > + vector |= 0x18000; > + break; > + case AIL_C000_0000_0000_4000: > + vector |= 0xc000000000004000ull; > + break; > + default: > + cpu_abort(cs, "Invalid AIL combination %d\n", ail); > + break; > + } > + } > + > #if defined(TARGET_PPC64) > if (excp_model == POWERPC_EXCP_BOOKE) { > if (env->spr[SPR_BOOKE_EPCR] & EPCR_ICM) { > Index: qemu-dgibson-for-2.6.git/target-ppc/translate_init.c > =================================================================== > --- qemu-dgibson-for-2.6.git.orig/target-ppc/translate_init.c > +++ qemu-dgibson-for-2.6.git/target-ppc/translate_init.c > @@ -8487,7 +8487,7 @@ POWERPC_FAMILY(POWER8)(ObjectClass *oc, > pcc->handle_mmu_fault = ppc_hash64_handle_mmu_fault; > pcc->sps = &POWER7_POWER8_sps; > #endif > - pcc->excp_model = POWERPC_EXCP_POWER7; > + pcc->excp_model = POWERPC_EXCP_POWER8; > pcc->bus_model = PPC_FLAGS_INPUT_POWER7; > pcc->bfd_mach = bfd_mach_ppc64; > pcc->flags = POWERPC_FLAG_VRE | POWERPC_FLAG_SE | > 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 > @@ -823,7 +823,6 @@ static target_ulong h_set_mode_resource_ > { > CPUState *cs; > PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu); > - target_ulong prefix; > > if (!(pcc->insns_flags2 & PPC2_ISA207S)) { > return H_P2; > @@ -835,25 +834,12 @@ 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: > + if (mflags == AIL_RESERVED) { > return H_UNSUPPORTED_FLAG; > } > > CPU_FOREACH(cs) { > - CPUPPCState *env = &POWERPC_CPU(cpu)->env; > - > set_spr(cs, SPR_LPCR, mflags << LPCR_AIL_SHIFT, LPCR_AIL); > - env->excp_prefix = prefix; > } > > return H_SUCCESS; > Index: qemu-dgibson-for-2.6.git/include/hw/ppc/spapr.h > =================================================================== > --- qemu-dgibson-for-2.6.git.orig/include/hw/ppc/spapr.h > +++ qemu-dgibson-for-2.6.git/include/hw/ppc/spapr.h > @@ -204,11 +204,6 @@ struct sPAPRMachineState { > #define H_SET_MODE_ENDIAN_BIG 0 > #define H_SET_MODE_ENDIAN_LITTLE 1 > > -/* Flags for H_SET_MODE_RESOURCE_ADDR_TRANS_MODE */ > -#define H_SET_MODE_ADDR_TRANS_NONE 0 > -#define H_SET_MODE_ADDR_TRANS_0001_8000 2 > -#define H_SET_MODE_ADDR_TRANS_C000_0000_0000_4000 3 > - > /* VASI States */ > #define H_VASI_INVALID 0 > #define H_VASI_ENABLED 1 >
On 04/04/2016 06:16 AM, David Gibson wrote: > On Sun, Apr 03, 2016 at 07:57:50PM +0200, Cédric 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é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 <clg@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). >>> >>> 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édric, I'm ok if you provide a version of that which only handles >>> POWER7 and POWER8 (i.e. spapr compatible models for now). >> >> Sure. >> >> But first, could you please take a look at a reworked patch of Ben who >> 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. > > 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). Yes. There is still a : vector |= env->excp_prefix; which is a bit confusing for P7/P8 guests as 'excp_prefix' is not used. The code section calculating 'vector' in powerpc_excp() could be isolated in its own routine I guess. We could clarify things there. > But for now it looks like it will address the migration issue at hand. > > I've applied it to ppc-for-2.6. Tell me if you want a proper resend by mail. Thanks, C. >> >>> 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. >> >> >> Thanks, >> >> C. >> >> >From bb9d1e06a0ea2132902db724f61c8dc655aa1a96 Mon Sep 17 00:00:00 2001 >> From: Benjamin Herrenschmidt <benh@kernel.crashing.org> >> Date: Thu, 4 Jun 2015 03:39:19 +1000 >> Subject: [PATCH 18/77] ppc: Rework POWER7 & POWER8 exception model >> >> This patch fixes the current AIL implementation for POWER8. The >> interrupt vector address can be calculated directly from LPCR when the >> exception is handled. The excp_prefix update becomes useless and we >> can cleanup the H_SET_MODE hcall. >> >> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> >> [clg: Removed LPES0/1 handling for HV vs. !HV >> Fixed LPCR_ILE case for POWERPC_EXCP_POWER8 ] >> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com> >> --- >> hw/ppc/spapr_hcall.c | 16 -------------- >> include/hw/ppc/spapr.h | 5 ---- >> target-ppc/cpu.h | 10 ++++++++ >> target-ppc/excp_helper.c | 49 ++++++++++++++++++++++++++++++++++++++++++-- >> target-ppc/translate_init.c | 2 - >> 5 files changed, 59 insertions(+), 23 deletions(-) >> >> Index: qemu-dgibson-for-2.6.git/target-ppc/cpu.h >> =================================================================== >> --- qemu-dgibson-for-2.6.git.orig/target-ppc/cpu.h >> +++ qemu-dgibson-for-2.6.git/target-ppc/cpu.h >> @@ -167,6 +167,8 @@ enum powerpc_excp_t { >> POWERPC_EXCP_970, >> /* POWER7 exception model */ >> POWERPC_EXCP_POWER7, >> + /* POWER8 exception model */ >> + POWERPC_EXCP_POWER8, >> #endif /* defined(TARGET_PPC64) */ >> }; >> >> @@ -2277,6 +2279,14 @@ enum { >> HMER_XSCOM_STATUS_LSH = (63 - 23), >> }; >> >> +/* Alternate Interrupt Location (AIL) */ >> +enum { >> + AIL_NONE = 0, >> + AIL_RESERVED = 1, >> + AIL_0001_8000 = 2, >> + AIL_C000_0000_0000_4000 = 3, >> +}; >> + >> /*****************************************************************************/ >> >> static inline target_ulong cpu_read_xer(CPUPPCState *env) >> Index: qemu-dgibson-for-2.6.git/target-ppc/excp_helper.c >> =================================================================== >> --- qemu-dgibson-for-2.6.git.orig/target-ppc/excp_helper.c >> +++ qemu-dgibson-for-2.6.git/target-ppc/excp_helper.c >> @@ -77,7 +77,7 @@ static inline void powerpc_excp(PowerPCC >> CPUPPCState *env = &cpu->env; >> target_ulong msr, new_msr, vector; >> int srr0, srr1, asrr0, asrr1; >> - int lpes0, lpes1, lev; >> + int lpes0, lpes1, lev, ail; >> >> if (0) { >> /* XXX: find a suitable condition to enable the hypervisor mode */ >> @@ -108,6 +108,25 @@ static inline void powerpc_excp(PowerPCC >> asrr0 = -1; >> asrr1 = -1; >> >> + /* Exception targetting modifiers >> + * >> + * AIL is initialized here but can be cleared by >> + * selected exceptions >> + */ >> +#if defined(TARGET_PPC64) >> + if (excp_model == POWERPC_EXCP_POWER7 || >> + excp_model == POWERPC_EXCP_POWER8) { >> + if (excp_model == POWERPC_EXCP_POWER8) { >> + ail = (env->spr[SPR_LPCR] & LPCR_AIL) >> LPCR_AIL_SHIFT; >> + } else { >> + ail = 0; >> + } >> + } else >> +#endif /* defined(TARGET_PPC64) */ >> + { >> + ail = 0; >> + } >> + >> switch (excp) { >> case POWERPC_EXCP_NONE: >> /* Should never happen */ >> @@ -146,6 +165,7 @@ static inline void powerpc_excp(PowerPCC >> /* XXX: find a suitable condition to enable the hypervisor mode */ >> new_msr |= (target_ulong)MSR_HVB; >> } >> + ail = 0; >> >> /* machine check exceptions don't have ME set */ >> new_msr &= ~((target_ulong)1 << MSR_ME); >> @@ -344,6 +364,7 @@ static inline void powerpc_excp(PowerPCC >> /* XXX: find a suitable condition to enable the hypervisor mode */ >> new_msr |= (target_ulong)MSR_HVB; >> } >> + ail = 0; >> goto store_next; >> case POWERPC_EXCP_DSEG: /* Data segment exception */ >> if (lpes1 == 0) { >> @@ -630,7 +651,8 @@ static inline void powerpc_excp(PowerPCC >> } >> >> #ifdef TARGET_PPC64 >> - if (excp_model == POWERPC_EXCP_POWER7) { >> + if (excp_model == POWERPC_EXCP_POWER7 || >> + excp_model == POWERPC_EXCP_POWER8) { >> if (env->spr[SPR_LPCR] & LPCR_ILE) { >> new_msr |= (target_ulong)1 << MSR_LE; >> } >> @@ -650,6 +672,29 @@ static inline void powerpc_excp(PowerPCC >> excp); >> } >> vector |= env->excp_prefix; >> + >> + /* AIL only works if there is no HV transition and we are running with >> + * translations enabled >> + */ >> + if (!((msr >> MSR_IR) & 1) || !((msr >> MSR_DR) & 1)) { >> + ail = 0; >> + } >> + /* Handle AIL */ >> + if (ail) { >> + new_msr |= (1 << MSR_IR) | (1 << MSR_DR); >> + switch(ail) { >> + case AIL_0001_8000: >> + vector |= 0x18000; >> + break; >> + case AIL_C000_0000_0000_4000: >> + vector |= 0xc000000000004000ull; >> + break; >> + default: >> + cpu_abort(cs, "Invalid AIL combination %d\n", ail); >> + break; >> + } >> + } >> + >> #if defined(TARGET_PPC64) >> if (excp_model == POWERPC_EXCP_BOOKE) { >> if (env->spr[SPR_BOOKE_EPCR] & EPCR_ICM) { >> Index: qemu-dgibson-for-2.6.git/target-ppc/translate_init.c >> =================================================================== >> --- qemu-dgibson-for-2.6.git.orig/target-ppc/translate_init.c >> +++ qemu-dgibson-for-2.6.git/target-ppc/translate_init.c >> @@ -8487,7 +8487,7 @@ POWERPC_FAMILY(POWER8)(ObjectClass *oc, >> pcc->handle_mmu_fault = ppc_hash64_handle_mmu_fault; >> pcc->sps = &POWER7_POWER8_sps; >> #endif >> - pcc->excp_model = POWERPC_EXCP_POWER7; >> + pcc->excp_model = POWERPC_EXCP_POWER8; >> pcc->bus_model = PPC_FLAGS_INPUT_POWER7; >> pcc->bfd_mach = bfd_mach_ppc64; >> pcc->flags = POWERPC_FLAG_VRE | POWERPC_FLAG_SE | >> 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 >> @@ -823,7 +823,6 @@ static target_ulong h_set_mode_resource_ >> { >> CPUState *cs; >> PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu); >> - target_ulong prefix; >> >> if (!(pcc->insns_flags2 & PPC2_ISA207S)) { >> return H_P2; >> @@ -835,25 +834,12 @@ 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: >> + if (mflags == AIL_RESERVED) { >> return H_UNSUPPORTED_FLAG; >> } >> >> CPU_FOREACH(cs) { >> - CPUPPCState *env = &POWERPC_CPU(cpu)->env; >> - >> set_spr(cs, SPR_LPCR, mflags << LPCR_AIL_SHIFT, LPCR_AIL); >> - env->excp_prefix = prefix; >> } >> >> return H_SUCCESS; >> Index: qemu-dgibson-for-2.6.git/include/hw/ppc/spapr.h >> =================================================================== >> --- qemu-dgibson-for-2.6.git.orig/include/hw/ppc/spapr.h >> +++ qemu-dgibson-for-2.6.git/include/hw/ppc/spapr.h >> @@ -204,11 +204,6 @@ struct sPAPRMachineState { >> #define H_SET_MODE_ENDIAN_BIG 0 >> #define H_SET_MODE_ENDIAN_LITTLE 1 >> >> -/* Flags for H_SET_MODE_RESOURCE_ADDR_TRANS_MODE */ >> -#define H_SET_MODE_ADDR_TRANS_NONE 0 >> -#define H_SET_MODE_ADDR_TRANS_0001_8000 2 >> -#define H_SET_MODE_ADDR_TRANS_C000_0000_0000_4000 3 >> - >> /* VASI States */ >> #define H_VASI_INVALID 0 >> #define H_VASI_ENABLED 1 >> >
On Mon, Apr 04, 2016 at 04:47:53PM +0200, Cédric Le Goater wrote: > On 04/04/2016 06:16 AM, David Gibson wrote: > > On Sun, Apr 03, 2016 at 07:57:50PM +0200, Cédric 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é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 <clg@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). > >>> > >>> 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édric, I'm ok if you provide a version of that which only handles > >>> POWER7 and POWER8 (i.e. spapr compatible models for now). > >> > >> Sure. > >> > >> But first, could you please take a look at a reworked patch of Ben who > >> 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. > > > > 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). > > Yes. There is still a : > > vector |= env->excp_prefix; > > which is a bit confusing for P7/P8 guests as 'excp_prefix' is not used. > The code section calculating 'vector' in powerpc_excp() could be isolated > 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. > > > > I've applied it to ppc-for-2.6. > > Tell me if you want a proper resend by mail. No, I think I have it adequately cleaned up myself.
Index: qemu-dgibson-for-2.6.git/target-ppc/cpu.h =================================================================== --- qemu-dgibson-for-2.6.git.orig/target-ppc/cpu.h +++ qemu-dgibson-for-2.6.git/target-ppc/cpu.h @@ -167,6 +167,8 @@ enum powerpc_excp_t { POWERPC_EXCP_970, /* POWER7 exception model */ POWERPC_EXCP_POWER7, + /* POWER8 exception model */ + POWERPC_EXCP_POWER8, #endif /* defined(TARGET_PPC64) */ }; @@ -2277,6 +2279,14 @@ enum { HMER_XSCOM_STATUS_LSH = (63 - 23), }; +/* Alternate Interrupt Location (AIL) */ +enum { + AIL_NONE = 0, + AIL_RESERVED = 1, + AIL_0001_8000 = 2, + AIL_C000_0000_0000_4000 = 3, +}; + /*****************************************************************************/ static inline target_ulong cpu_read_xer(CPUPPCState *env) Index: qemu-dgibson-for-2.6.git/target-ppc/excp_helper.c =================================================================== --- qemu-dgibson-for-2.6.git.orig/target-ppc/excp_helper.c +++ qemu-dgibson-for-2.6.git/target-ppc/excp_helper.c @@ -77,7 +77,7 @@ static inline void powerpc_excp(PowerPCC CPUPPCState *env = &cpu->env; target_ulong msr, new_msr, vector; int srr0, srr1, asrr0, asrr1; - int lpes0, lpes1, lev; + int lpes0, lpes1, lev, ail; if (0) { /* XXX: find a suitable condition to enable the hypervisor mode */ @@ -108,6 +108,25 @@ static inline void powerpc_excp(PowerPCC asrr0 = -1; asrr1 = -1; + /* Exception targetting modifiers + * + * AIL is initialized here but can be cleared by + * selected exceptions + */ +#if defined(TARGET_PPC64) + if (excp_model == POWERPC_EXCP_POWER7 || + excp_model == POWERPC_EXCP_POWER8) { + if (excp_model == POWERPC_EXCP_POWER8) { + ail = (env->spr[SPR_LPCR] & LPCR_AIL) >> LPCR_AIL_SHIFT; + } else { + ail = 0; + } + } else +#endif /* defined(TARGET_PPC64) */ + { + ail = 0; + } + switch (excp) { case POWERPC_EXCP_NONE: /* Should never happen */ @@ -146,6 +165,7 @@ static inline void powerpc_excp(PowerPCC /* XXX: find a suitable condition to enable the hypervisor mode */ new_msr |= (target_ulong)MSR_HVB; } + ail = 0; /* machine check exceptions don't have ME set */ new_msr &= ~((target_ulong)1 << MSR_ME); @@ -344,6 +364,7 @@ static inline void powerpc_excp(PowerPCC /* XXX: find a suitable condition to enable the hypervisor mode */ new_msr |= (target_ulong)MSR_HVB; } + ail = 0; goto store_next; case POWERPC_EXCP_DSEG: /* Data segment exception */ if (lpes1 == 0) { @@ -630,7 +651,8 @@ static inline void powerpc_excp(PowerPCC } #ifdef TARGET_PPC64 - if (excp_model == POWERPC_EXCP_POWER7) { + if (excp_model == POWERPC_EXCP_POWER7 || + excp_model == POWERPC_EXCP_POWER8) { if (env->spr[SPR_LPCR] & LPCR_ILE) { new_msr |= (target_ulong)1 << MSR_LE; } @@ -650,6 +672,29 @@ static inline void powerpc_excp(PowerPCC excp); } vector |= env->excp_prefix; + + /* AIL only works if there is no HV transition and we are running with + * translations enabled + */ + if (!((msr >> MSR_IR) & 1) || !((msr >> MSR_DR) & 1)) { + ail = 0; + } + /* Handle AIL */ + if (ail) { + new_msr |= (1 << MSR_IR) | (1 << MSR_DR); + switch(ail) { + case AIL_0001_8000: + vector |= 0x18000; + break; + case AIL_C000_0000_0000_4000: + vector |= 0xc000000000004000ull; + break; + default: + cpu_abort(cs, "Invalid AIL combination %d\n", ail); + break; + } + } + #if defined(TARGET_PPC64) if (excp_model == POWERPC_EXCP_BOOKE) { if (env->spr[SPR_BOOKE_EPCR] & EPCR_ICM) { Index: qemu-dgibson-for-2.6.git/target-ppc/translate_init.c =================================================================== --- qemu-dgibson-for-2.6.git.orig/target-ppc/translate_init.c +++ qemu-dgibson-for-2.6.git/target-ppc/translate_init.c @@ -8487,7 +8487,7 @@ POWERPC_FAMILY(POWER8)(ObjectClass *oc, pcc->handle_mmu_fault = ppc_hash64_handle_mmu_fault; pcc->sps = &POWER7_POWER8_sps; #endif - pcc->excp_model = POWERPC_EXCP_POWER7; + pcc->excp_model = POWERPC_EXCP_POWER8; pcc->bus_model = PPC_FLAGS_INPUT_POWER7; pcc->bfd_mach = bfd_mach_ppc64; pcc->flags = POWERPC_FLAG_VRE | POWERPC_FLAG_SE | 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 @@ -823,7 +823,6 @@ static target_ulong h_set_mode_resource_ { CPUState *cs; PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu); - target_ulong prefix; if (!(pcc->insns_flags2 & PPC2_ISA207S)) { return H_P2; @@ -835,25 +834,12 @@ 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: + if (mflags == AIL_RESERVED) { return H_UNSUPPORTED_FLAG; } CPU_FOREACH(cs) { - CPUPPCState *env = &POWERPC_CPU(cpu)->env; - set_spr(cs, SPR_LPCR, mflags << LPCR_AIL_SHIFT, LPCR_AIL); - env->excp_prefix = prefix; } return H_SUCCESS; Index: qemu-dgibson-for-2.6.git/include/hw/ppc/spapr.h =================================================================== --- qemu-dgibson-for-2.6.git.orig/include/hw/ppc/spapr.h +++ qemu-dgibson-for-2.6.git/include/hw/ppc/spapr.h @@ -204,11 +204,6 @@ struct sPAPRMachineState { #define H_SET_MODE_ENDIAN_BIG 0 #define H_SET_MODE_ENDIAN_LITTLE 1 -/* Flags for H_SET_MODE_RESOURCE_ADDR_TRANS_MODE */ -#define H_SET_MODE_ADDR_TRANS_NONE 0 -#define H_SET_MODE_ADDR_TRANS_0001_8000 2 -#define H_SET_MODE_ADDR_TRANS_C000_0000_0000_4000 3 - /* VASI States */ #define H_VASI_INVALID 0 #define H_VASI_ENABLED 1