[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [Qemu-ppc] [PATCH v2] spapr: compute interrupt vector a
|
From: |
Cédric Le Goater |
|
Subject: |
Re: [Qemu-devel] [Qemu-ppc] [PATCH v2] spapr: compute interrupt vector address from LPCR |
|
Date: |
Mon, 4 Apr 2016 16:47:53 +0200 |
|
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Icedove/38.7.0 |
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 <address@hidden>
>>>>>
>>>>> 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 <address@hidden>
>> 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 <address@hidden>
>> [clg: Removed LPES0/1 handling for HV vs. !HV
>> Fixed LPCR_ILE case for POWERPC_EXCP_POWER8 ]
>> Signed-off-by: Cédric Le Goater <address@hidden>
>> ---
>> 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
>>
>