Re: [Qemu-ppc] [PATCH v2] spapr: compute interrupt vector address from LPCR

[Cédric Le Goater]

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.

> 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