qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH 0/2] ppc: rework AIL logic, add POWER10 exception model
@ 2021-04-14  3:23 Nicholas Piggin
  2021-04-14  3:23 ` [RFC PATCH 1/2] target/ppc: rework AIL logic in interrupt delivery Nicholas Piggin
  2021-04-14  3:23 ` [RFC PATCH 2/2] target/ppc: Add POWER10 exception model Nicholas Piggin
  0 siblings, 2 replies; 8+ messages in thread
From: Nicholas Piggin @ 2021-04-14  3:23 UTC (permalink / raw)
  To: qemu-ppc
  Cc: Fabiano Rosas, Cédric Le Goater, qemu-devel,
	Nicholas Piggin, David Gibson

This applies on top of patches 1,2 from the previous series (i.e., these
two patches replace patch 3).

Function should be the same, but this way seems much cleaner. It does
include a "cleanup" patch before the POWER10 fix, but arguably this is
a better way to go even as a bug fix (backport, etc).

Comments, opinions?

Thanks,
Nick

Nicholas Piggin (2):
  target/ppc: rework AIL logic in interrupt delivery
  target/ppc: Add POWER10 exception model

 hw/ppc/spapr_hcall.c            |   8 +-
 target/ppc/cpu-qom.h            |   2 +
 target/ppc/cpu.h                |  13 +-
 target/ppc/excp_helper.c        | 204 ++++++++++++++++++++++----------
 target/ppc/translate.c          |   3 +-
 target/ppc/translate_init.c.inc |   4 +-
 6 files changed, 160 insertions(+), 74 deletions(-)

-- 
2.23.0



^ permalink raw reply	[flat|nested] 8+ messages in thread

* [RFC PATCH 1/2] target/ppc: rework AIL logic in interrupt delivery
  2021-04-14  3:23 [RFC PATCH 0/2] ppc: rework AIL logic, add POWER10 exception model Nicholas Piggin
@ 2021-04-14  3:23 ` Nicholas Piggin
  2021-04-14 15:24   ` [EXTERNAL] " Cédric Le Goater
  2021-04-14  3:23 ` [RFC PATCH 2/2] target/ppc: Add POWER10 exception model Nicholas Piggin
  1 sibling, 1 reply; 8+ messages in thread
From: Nicholas Piggin @ 2021-04-14  3:23 UTC (permalink / raw)
  To: qemu-ppc
  Cc: Fabiano Rosas, Cédric Le Goater, qemu-devel,
	Nicholas Piggin, David Gibson

The AIL logic is becoming unmanageable spread all over powerpc_excp(),
and it is slated to get even worse with POWER10 support.

Move it all to a new helper function.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 hw/ppc/spapr_hcall.c            |   3 +-
 target/ppc/cpu.h                |   8 --
 target/ppc/excp_helper.c        | 161 ++++++++++++++++++++------------
 target/ppc/translate_init.c.inc |   2 +-
 4 files changed, 104 insertions(+), 70 deletions(-)

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 7b5cd3553c..2fbe04a689 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1395,7 +1395,8 @@ static target_ulong h_set_mode_resource_addr_trans_mode(PowerPCCPU *cpu,
         return H_P4;
     }
 
-    if (mflags == AIL_RESERVED) {
+    if (mflags == 1) {
+        /* AIL=1 is reserved */
         return H_UNSUPPORTED_FLAG;
     }
 
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index e73416da68..5200a16d23 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -2375,14 +2375,6 @@ enum {
     HMER_XSCOM_STATUS_MASK      = PPC_BITMASK(21, 23),
 };
 
-/* Alternate Interrupt Location (AIL) */
-enum {
-    AIL_NONE                = 0,
-    AIL_RESERVED            = 1,
-    AIL_0001_8000           = 2,
-    AIL_C000_0000_0000_4000 = 3,
-};
-
 /*****************************************************************************/
 
 #define is_isa300(ctx) (!!(ctx->insns_flags2 & PPC2_ISA300))
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index b8881c0f85..9ff316767c 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -136,25 +136,107 @@ static int powerpc_reset_wakeup(CPUState *cs, CPUPPCState *env, int excp,
     return POWERPC_EXCP_RESET;
 }
 
-static uint64_t ppc_excp_vector_offset(CPUState *cs, int ail)
+/*
+ * AIL - Alternate Interrupt Location, a mode that allows interrupts to be
+ * taken with the MMU on, and which uses an alternate location (e.g., so the
+ * kernel/hv can map the vectors there with an effective address).
+ *
+ * An interrupt is considered to be taken "with AIL" or "AIL applies" if they
+ * are delivered in this way. AIL requires the LPCR to be set to enable this
+ * mode, and a number of conditions have to be true for AIL to apply.
+ *
+ * First of all, SRESET, MCE, and HMI are always delivered without AIL,
+ * because they are specifically want to be in real mode (e.g., MCE might
+ * be signaling a SLB multi-hit which requires SLB flush before the MMU can
+ * be enabled).
+ *
+ * After that, behaviour depends on the current MSR[IR], MSR[DR], MSR[HV], and
+ * whether or not the interrupt changes MSR[HV] from 0 to 1, and the current
+ * radix mode (LPCR[HR]).
+ *
+ * POWER8, POWER9 with LPCR[HR]=0
+ * | LPCR[AIL] | MSR[IR||DR] | MSR[HV] | new MSR[HV] | AIL |
+ * +-----------+-------------+---------+-------------+-----+
+ * | a         | 00/01/10    | x       | x           | 0   |
+ * | a         | 11          | 0       | 1           | 0   |
+ * | a         | 11          | 1       | 1           | a   |
+ * | a         | 11          | 0       | 0           | a   |
+ * +-------------------------------------------------------+
+ *
+ * POWER9 with LPCR[HR]=1
+ * | LPCR[AIL] | MSR[IR||DR] | MSR[HV] | new MSR[HV] | AIL |
+ * +-----------+-------------+---------+-------------+-----+
+ * | a         | 00/01/10    | x       | x           | 0   |
+ * | a         | 11          | x       | x           | a   |
+ * +-------------------------------------------------------+
+ *
+ * The difference with POWER9 being that MSR[HV] 0->1 interrupts can be
+ * sent to the hypervisor in AIL mode if the guest is radix (LPCR[HR]=1).
+ */
+static inline void ppc_excp_apply_ail(PowerPCCPU *cpu, int excp_model, int excp,
+                                      target_ulong msr,
+                                      target_ulong *new_msr,
+                                      target_ulong *vector)
 {
-    uint64_t offset = 0;
+#if defined(TARGET_PPC64)
+    CPUPPCState *env = &cpu->env;
+    bool mmu_all_on = ((msr >> MSR_IR) & 1) && ((msr >> MSR_DR) & 1);
+    bool hv_escalation = !(msr & MSR_HVB) && (*new_msr & MSR_HVB);
+    int ail = 0;
+
+    if (excp == POWERPC_EXCP_MCHECK ||
+        excp == POWERPC_EXCP_RESET ||
+        excp == POWERPC_EXCP_HV_MAINT) {
+        /* SRESET, MCE, HMI never apply AIL */
+        return;
+    }
 
-    switch (ail) {
-    case AIL_NONE:
-        break;
-    case AIL_0001_8000:
-        offset = 0x18000;
-        break;
-    case AIL_C000_0000_0000_4000:
-        offset = 0xc000000000004000ull;
-        break;
-    default:
-        cpu_abort(cs, "Invalid AIL combination %d\n", ail);
-        break;
+    if (excp_model == POWERPC_EXCP_POWER8 ||
+        excp_model == POWERPC_EXCP_POWER9) {
+        if (!mmu_all_on) {
+            /* AIL only works if MSR[IR] and MSR[DR] are both enabled. */
+            return;
+        }
+        if (hv_escalation && !(env->spr[SPR_LPCR] & LPCR_HR)) {
+            /*
+             * AIL does not work if there is a MSR[HV] 0->1 transition and the
+             * partition is in HPT mode. For radix guests, such interrupts are
+             * allowed to be delivered to the hypervisor in ail mode.
+             */
+            return;
+        }
+
+        ail = (env->spr[SPR_LPCR] & LPCR_AIL) >> LPCR_AIL_SHIFT;
+        if (ail != 2 && ail != 3) {
+            /* AIL=1 is reserved */
+            return;
+        }
+    } else {
+        /* Other processors do not support AIL */
+        return;
     }
 
-    return offset;
+    /*
+     * AIL applies, so the new MSR gets IR and DR set, and an offset applied
+     * to the new IP.
+     */
+    *new_msr |= (1 << MSR_IR) | (1 << MSR_DR);
+
+    if (excp != POWERPC_EXCP_SYSCALL_VECTORED) {
+        if (ail == 2) {
+            *vector |= 0x0000000000018000ull;
+        } else if (ail == 3) {
+            *vector |= 0xc000000000004000ull;
+        }
+    } else {
+        /* scv AIL is a little different */
+        if (ail == 3) {
+            /* Un-apply the base offset */
+            *vector &= ~0x0000000000017000ull;
+            *vector |= 0xc000000000003000ull;
+        }
+    }
+#endif
 }
 
 static inline void powerpc_set_excp_state(PowerPCCPU *cpu,
@@ -197,7 +279,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
     CPUState *cs = CPU(cpu);
     CPUPPCState *env = &cpu->env;
     target_ulong msr, new_msr, vector;
-    int srr0, srr1, asrr0, asrr1, lev = -1, ail;
+    int srr0, srr1, asrr0, asrr1, lev = -1;
     bool lpes0;
 
     qemu_log_mask(CPU_LOG_INT, "Raise exception at " TARGET_FMT_lx
@@ -238,25 +320,16 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
      *
      * On anything else, we behave as if LPES0 is 1
      * (externals don't alter MSR:HV)
-     *
-     * 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 ||
         excp_model == POWERPC_EXCP_POWER9) {
         lpes0 = !!(env->spr[SPR_LPCR] & LPCR_LPES0);
-        if (excp_model != POWERPC_EXCP_POWER7) {
-            ail = (env->spr[SPR_LPCR] & LPCR_AIL) >> LPCR_AIL_SHIFT;
-        } else {
-            ail = 0;
-        }
     } else
 #endif /* defined(TARGET_PPC64) */
     {
         lpes0 = true;
-        ail = 0;
     }
 
     /*
@@ -315,7 +388,6 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
              */
             new_msr |= (target_ulong)MSR_HVB;
         }
-        ail = 0;
 
         /* machine check exceptions don't have ME set */
         new_msr &= ~((target_ulong)1 << MSR_ME);
@@ -519,7 +591,6 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
                           "exception %d with no HV support\n", excp);
             }
         }
-        ail = 0;
         break;
     case POWERPC_EXCP_DSEG:      /* Data segment exception                   */
     case POWERPC_EXCP_ISEG:      /* Instruction segment exception            */
@@ -790,24 +861,6 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
     }
 #endif
 
-    /*
-     * AIL only works if MSR[IR] and MSR[DR] are both enabled.
-     */
-    if (!((msr >> MSR_IR) & 1) || !((msr >> MSR_DR) & 1)) {
-        ail = 0;
-    }
-
-    /*
-     * AIL does not work if there is a MSR[HV] 0->1 transition and the
-     * partition is in HPT mode. For radix guests, such interrupts are
-     * allowed to be delivered to the hypervisor in ail mode.
-     */
-    if ((new_msr & MSR_HVB) && !(msr & MSR_HVB)) {
-        if (!(env->spr[SPR_LPCR] & LPCR_HR)) {
-            ail = 0;
-        }
-    }
-
     vector = env->excp_vectors[excp];
     if (vector == (target_ulong)-1ULL) {
         cpu_abort(cs, "Raised an exception without defined vector %d\n",
@@ -848,23 +901,8 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
         /* Save MSR */
         env->spr[srr1] = msr;
 
-        /* Handle AIL */
-        if (ail) {
-            new_msr |= (1 << MSR_IR) | (1 << MSR_DR);
-            vector |= ppc_excp_vector_offset(cs, ail);
-        }
-
 #if defined(TARGET_PPC64)
     } else {
-        /* scv AIL is a little different */
-        if (ail) {
-            new_msr |= (1 << MSR_IR) | (1 << MSR_DR);
-        }
-        if (ail == AIL_C000_0000_0000_4000) {
-            vector |= 0xc000000000003000ull;
-        } else {
-            vector |= 0x0000000000017000ull;
-        }
         vector += lev * 0x20;
 
         env->lr = env->nip;
@@ -872,6 +910,9 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
 #endif
     }
 
+    /* This can update new_msr and vector if AIL applies */
+    ppc_excp_apply_ail(cpu, excp_model, excp, msr, &new_msr, &vector);
+
     powerpc_set_excp_state(cpu, vector, new_msr);
 }
 
diff --git a/target/ppc/translate_init.c.inc b/target/ppc/translate_init.c.inc
index 70f9b9b150..a82d9ed647 100644
--- a/target/ppc/translate_init.c.inc
+++ b/target/ppc/translate_init.c.inc
@@ -3457,7 +3457,7 @@ static void init_excp_POWER9(CPUPPCState *env)
 
 #if !defined(CONFIG_USER_ONLY)
     env->excp_vectors[POWERPC_EXCP_HVIRT]    = 0x00000EA0;
-    env->excp_vectors[POWERPC_EXCP_SYSCALL_VECTORED] = 0x00000000;
+    env->excp_vectors[POWERPC_EXCP_SYSCALL_VECTORED] = 0x00017000;
 #endif
 }
 
-- 
2.23.0



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [RFC PATCH 2/2] target/ppc: Add POWER10 exception model
  2021-04-14  3:23 [RFC PATCH 0/2] ppc: rework AIL logic, add POWER10 exception model Nicholas Piggin
  2021-04-14  3:23 ` [RFC PATCH 1/2] target/ppc: rework AIL logic in interrupt delivery Nicholas Piggin
@ 2021-04-14  3:23 ` Nicholas Piggin
  2021-04-14 15:54   ` [EXTERNAL] " Cédric Le Goater
  1 sibling, 1 reply; 8+ messages in thread
From: Nicholas Piggin @ 2021-04-14  3:23 UTC (permalink / raw)
  To: qemu-ppc
  Cc: Fabiano Rosas, Cédric Le Goater, qemu-devel,
	Nicholas Piggin, David Gibson

POWER10 adds a new bit that modifies interrupt behaviour, LPCR[HAIL],
and it removes support for the LPCR[AIL]=0b10 mode.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 hw/ppc/spapr_hcall.c            |  7 +++++-
 target/ppc/cpu-qom.h            |  2 ++
 target/ppc/cpu.h                |  5 ++--
 target/ppc/excp_helper.c        | 43 +++++++++++++++++++++++++++++++++
 target/ppc/translate.c          |  3 ++-
 target/ppc/translate_init.c.inc |  2 +-
 6 files changed, 57 insertions(+), 5 deletions(-)

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 2fbe04a689..6802cd4dc8 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1396,7 +1396,12 @@ static target_ulong h_set_mode_resource_addr_trans_mode(PowerPCCPU *cpu,
     }
 
     if (mflags == 1) {
-        /* AIL=1 is reserved */
+        /* AIL=1 is reserved in POWER8/POWER9 */
+        return H_UNSUPPORTED_FLAG;
+    }
+
+    if (mflags == 2 && (pcc->insns_flags2 & PPC2_ISA310)) {
+        /* AIL=2 is also reserved in POWER10 (ISA v3.1) */
         return H_UNSUPPORTED_FLAG;
     }
 
diff --git a/target/ppc/cpu-qom.h b/target/ppc/cpu-qom.h
index 118baf8d41..06b6571bc9 100644
--- a/target/ppc/cpu-qom.h
+++ b/target/ppc/cpu-qom.h
@@ -116,6 +116,8 @@ enum powerpc_excp_t {
     POWERPC_EXCP_POWER8,
     /* POWER9 exception model           */
     POWERPC_EXCP_POWER9,
+    /* POWER10 exception model           */
+    POWERPC_EXCP_POWER10,
 };
 
 /*****************************************************************************/
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 5200a16d23..9d35cdfa92 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -354,10 +354,11 @@ typedef struct ppc_v3_pate_t {
 #define LPCR_PECE_U_SHIFT (63 - 19)
 #define LPCR_PECE_U_MASK  (0x7ull << LPCR_PECE_U_SHIFT)
 #define LPCR_HVEE         PPC_BIT(17) /* Hypervisor Virt Exit Enable */
-#define LPCR_RMLS_SHIFT   (63 - 37)
+#define LPCR_RMLS_SHIFT   (63 - 37)   /* RMLS (removed in ISA v3.0) */
 #define LPCR_RMLS         (0xfull << LPCR_RMLS_SHIFT)
+#define LPCR_HAIL         PPC_BIT(37) /* ISA v3.1 HV AIL=3 equivalent */
 #define LPCR_ILE          PPC_BIT(38)
-#define LPCR_AIL_SHIFT    (63 - 40)      /* Alternate interrupt location */
+#define LPCR_AIL_SHIFT    (63 - 40)   /* Alternate interrupt location */
 #define LPCR_AIL          (3ull << LPCR_AIL_SHIFT)
 #define LPCR_UPRT         PPC_BIT(41) /* Use Process Table */
 #define LPCR_EVIRT        PPC_BIT(42) /* Enhanced Virtualisation */
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 9ff316767c..19931361a0 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -172,6 +172,26 @@ static int powerpc_reset_wakeup(CPUState *cs, CPUPPCState *env, int excp,
  *
  * The difference with POWER9 being that MSR[HV] 0->1 interrupts can be
  * sent to the hypervisor in AIL mode if the guest is radix (LPCR[HR]=1).
+ * This is good for performance but allows the guest to influence the
+ * AIL of hypervisor interrupts using its MSR, and also the hypervisor
+ * must disallow guest interrupts (MSR[HV] 0->0) from using AIL if the
+ * hypervisor does not want to use AIL for its MSR[HV] 0->1 interrupts.
+ *
+ * POWER10 addresses those issues with a new LPCR[HAIL] bit that is
+ * applied to interrupt that begin execution with MSR[HV]=1 (so both
+ * MSR[HV] 0->1 and 1->1).
+ *
+ * HAIL=1 is equivalent to AIL=3, for interrupts delivered with MSR[HV]=1.
+ *
+ * POWER10 behaviour is
+ * | LPCR[AIL] | LPCR[HAIL] | MSR[IR||DR] | MSR[HV] | new MSR[HV] | AIL |
+ * +-----------+------------+-------------+---------+-------------+-----+
+ * | a         | h          | 00/01/10    | 0       | 0           | 0   |
+ * | a         | h          | 11          | 0       | 0           | a   |
+ * | a         | h          | x           | 0       | 1           | h   |
+ * | a         | h          | 00/01/10    | 1       | 1           | 0   |
+ * | a         | h          | 11          | 1       | 1           | h   |
+ * +--------------------------------------------------------------------+
  */
 static inline void ppc_excp_apply_ail(PowerPCCPU *cpu, int excp_model, int excp,
                                       target_ulong msr,
@@ -211,6 +231,29 @@ static inline void ppc_excp_apply_ail(PowerPCCPU *cpu, int excp_model, int excp,
             /* AIL=1 is reserved */
             return;
         }
+
+    } else if (excp_model == POWERPC_EXCP_POWER10) {
+        if (!mmu_all_on && !hv_escalation) {
+            /*
+             * AIL works for HV interrupts even with guest MSR[IR/DR] disabled.
+             * Guest->guest and HV->HV interrupts do require MMU on.
+             */
+            return;
+        }
+
+        if (*new_msr & MSR_HVB) {
+            if (!(env->spr[SPR_LPCR] & LPCR_HAIL)) {
+                /* HV interrupts depend on LPCR[HAIL] */
+                return;
+            }
+            ail = 3; /* HAIL=1 gives AIL=3 behaviour for HV interrupts */
+        } else {
+            ail = (env->spr[SPR_LPCR] & LPCR_AIL) >> LPCR_AIL_SHIFT;
+        }
+        if (ail != 3) {
+            /* AIL=1 and AIL=2 are reserved */
+            return;
+        }
     } else {
         /* Other processors do not support AIL */
         return;
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 0984ce637b..e9ed001229 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -7731,7 +7731,8 @@ void ppc_cpu_dump_state(CPUState *cs, FILE *f, int flags)
 #if defined(TARGET_PPC64)
     if (env->excp_model == POWERPC_EXCP_POWER7 ||
         env->excp_model == POWERPC_EXCP_POWER8 ||
-        env->excp_model == POWERPC_EXCP_POWER9)  {
+        env->excp_model == POWERPC_EXCP_POWER9 ||
+        env->excp_model == POWERPC_EXCP_POWER10)  {
         qemu_fprintf(f, "HSRR0 " TARGET_FMT_lx " HSRR1 " TARGET_FMT_lx "\n",
                      env->spr[SPR_HSRR0], env->spr[SPR_HSRR1]);
     }
diff --git a/target/ppc/translate_init.c.inc b/target/ppc/translate_init.c.inc
index a82d9ed647..76d82cc2f6 100644
--- a/target/ppc/translate_init.c.inc
+++ b/target/ppc/translate_init.c.inc
@@ -9317,7 +9317,7 @@ POWERPC_FAMILY(POWER10)(ObjectClass *oc, void *data)
     pcc->radix_page_info = &POWER10_radix_page_info;
     pcc->lrg_decr_bits = 56;
 #endif
-    pcc->excp_model = POWERPC_EXCP_POWER9;
+    pcc->excp_model = POWERPC_EXCP_POWER10;
     pcc->bus_model = PPC_FLAGS_INPUT_POWER9;
     pcc->bfd_mach = bfd_mach_ppc64;
     pcc->flags = POWERPC_FLAG_VRE | POWERPC_FLAG_SE |
-- 
2.23.0



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [EXTERNAL] [RFC PATCH 1/2] target/ppc: rework AIL logic in interrupt delivery
  2021-04-14  3:23 ` [RFC PATCH 1/2] target/ppc: rework AIL logic in interrupt delivery Nicholas Piggin
@ 2021-04-14 15:24   ` Cédric Le Goater
  2021-04-15  5:25     ` Nicholas Piggin
  0 siblings, 1 reply; 8+ messages in thread
From: Cédric Le Goater @ 2021-04-14 15:24 UTC (permalink / raw)
  To: Nicholas Piggin, qemu-ppc
  Cc: Fabiano Rosas, Cédric Le Goater, qemu-devel, David Gibson

On 4/14/21 5:23 AM, Nicholas Piggin wrote:
> The AIL logic is becoming unmanageable spread all over powerpc_excp(),
> and it is slated to get even worse with POWER10 support.
> 
> Move it all to a new helper function.

Reviewed-by: Cédric Le Goater <clg@kaod.org>
Tested-by: Cédric Le Goater <clg@kaod.org>

Thanks for the effort and the documentation. One minor comment below,

C.

> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  hw/ppc/spapr_hcall.c            |   3 +-
>  target/ppc/cpu.h                |   8 --
>  target/ppc/excp_helper.c        | 161 ++++++++++++++++++++------------
>  target/ppc/translate_init.c.inc |   2 +-
>  4 files changed, 104 insertions(+), 70 deletions(-)
> 
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 7b5cd3553c..2fbe04a689 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1395,7 +1395,8 @@ static target_ulong h_set_mode_resource_addr_trans_mode(PowerPCCPU *cpu,
>          return H_P4;
>      }
> 
> -    if (mflags == AIL_RESERVED) {
> +    if (mflags == 1) {
> +        /* AIL=1 is reserved */
>          return H_UNSUPPORTED_FLAG;
>      }
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index e73416da68..5200a16d23 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -2375,14 +2375,6 @@ enum {
>      HMER_XSCOM_STATUS_MASK      = PPC_BITMASK(21, 23),
>  };
> 
> -/* Alternate Interrupt Location (AIL) */
> -enum {
> -    AIL_NONE                = 0,
> -    AIL_RESERVED            = 1,
> -    AIL_0001_8000           = 2,
> -    AIL_C000_0000_0000_4000 = 3,
> -};

I kind of like these. No big deal.

> -
>  /*****************************************************************************/
> 
>  #define is_isa300(ctx) (!!(ctx->insns_flags2 & PPC2_ISA300))
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index b8881c0f85..9ff316767c 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -136,25 +136,107 @@ static int powerpc_reset_wakeup(CPUState *cs, CPUPPCState *env, int excp,
>      return POWERPC_EXCP_RESET;
>  }
> 
> -static uint64_t ppc_excp_vector_offset(CPUState *cs, int ail)
> +/*
> + * AIL - Alternate Interrupt Location, a mode that allows interrupts to be
> + * taken with the MMU on, and which uses an alternate location (e.g., so the
> + * kernel/hv can map the vectors there with an effective address).
> + *
> + * An interrupt is considered to be taken "with AIL" or "AIL applies" if they
> + * are delivered in this way. AIL requires the LPCR to be set to enable this
> + * mode, and a number of conditions have to be true for AIL to apply.
> + *
> + * First of all, SRESET, MCE, and HMI are always delivered without AIL,
> + * because they are specifically want to be in real mode (e.g., MCE might
> + * be signaling a SLB multi-hit which requires SLB flush before the MMU can
> + * be enabled).
> + *
> + * After that, behaviour depends on the current MSR[IR], MSR[DR], MSR[HV], and
> + * whether or not the interrupt changes MSR[HV] from 0 to 1, and the current
> + * radix mode (LPCR[HR]).
> + *
> + * POWER8, POWER9 with LPCR[HR]=0
> + * | LPCR[AIL] | MSR[IR||DR] | MSR[HV] | new MSR[HV] | AIL |
> + * +-----------+-------------+---------+-------------+-----+
> + * | a         | 00/01/10    | x       | x           | 0   |
> + * | a         | 11          | 0       | 1           | 0   |
> + * | a         | 11          | 1       | 1           | a   |
> + * | a         | 11          | 0       | 0           | a   |
> + * +-------------------------------------------------------+
> + *
> + * POWER9 with LPCR[HR]=1
> + * | LPCR[AIL] | MSR[IR||DR] | MSR[HV] | new MSR[HV] | AIL |
> + * +-----------+-------------+---------+-------------+-----+
> + * | a         | 00/01/10    | x       | x           | 0   |
> + * | a         | 11          | x       | x           | a   |
> + * +-------------------------------------------------------+
> + *
> + * The difference with POWER9 being that MSR[HV] 0->1 interrupts can be
> + * sent to the hypervisor in AIL mode if the guest is radix (LPCR[HR]=1).
> + */
> +static inline void ppc_excp_apply_ail(PowerPCCPU *cpu, int excp_model, int excp,
> +                                      target_ulong msr,
> +                                      target_ulong *new_msr,
> +                                      target_ulong *vector)
>  {
> -    uint64_t offset = 0;
> +#if defined(TARGET_PPC64)
> +    CPUPPCState *env = &cpu->env;
> +    bool mmu_all_on = ((msr >> MSR_IR) & 1) && ((msr >> MSR_DR) & 1);
> +    bool hv_escalation = !(msr & MSR_HVB) && (*new_msr & MSR_HVB);
> +    int ail = 0;
> +
> +    if (excp == POWERPC_EXCP_MCHECK ||
> +        excp == POWERPC_EXCP_RESET ||
> +        excp == POWERPC_EXCP_HV_MAINT) {
> +        /* SRESET, MCE, HMI never apply AIL */
> +        return;
> +    }
> 
> -    switch (ail) {
> -    case AIL_NONE:
> -        break;
> -    case AIL_0001_8000:
> -        offset = 0x18000;
> -        break;
> -    case AIL_C000_0000_0000_4000:
> -        offset = 0xc000000000004000ull;
> -        break;
> -    default:
> -        cpu_abort(cs, "Invalid AIL combination %d\n", ail);

Could we keep this abort ? 


> -        break;
> +    if (excp_model == POWERPC_EXCP_POWER8 ||
> +        excp_model == POWERPC_EXCP_POWER9) {
> +        if (!mmu_all_on) {
> +            /* AIL only works if MSR[IR] and MSR[DR] are both enabled. */
> +            return;
> +        }
> +        if (hv_escalation && !(env->spr[SPR_LPCR] & LPCR_HR)) {
> +            /*
> +             * AIL does not work if there is a MSR[HV] 0->1 transition and the
> +             * partition is in HPT mode. For radix guests, such interrupts are
> +             * allowed to be delivered to the hypervisor in ail mode.
> +             */
> +            return;
> +        }
> +
> +        ail = (env->spr[SPR_LPCR] & LPCR_AIL) >> LPCR_AIL_SHIFT;
> +        if (ail != 2 && ail != 3) {
> +            /* AIL=1 is reserved */
> +            return;
> +        }
> +    } else {
> +        /* Other processors do not support AIL */
> +        return;
>      }
> 
> -    return offset;
> +    /*
> +     * AIL applies, so the new MSR gets IR and DR set, and an offset applied
> +     * to the new IP.
> +     */
> +    *new_msr |= (1 << MSR_IR) | (1 << MSR_DR);
> +
> +    if (excp != POWERPC_EXCP_SYSCALL_VECTORED) {
> +        if (ail == 2) {
> +            *vector |= 0x0000000000018000ull;
> +        } else if (ail == 3) {
> +            *vector |= 0xc000000000004000ull;
> +        }
> +    } else {
> +        /* scv AIL is a little different */
> +        if (ail == 3) {
> +            /* Un-apply the base offset */
> +            *vector &= ~0x0000000000017000ull;
> +            *vector |= 0xc000000000003000ull;
> +        }
> +    }
> +#endif
>  }
> 
>  static inline void powerpc_set_excp_state(PowerPCCPU *cpu,
> @@ -197,7 +279,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>      CPUState *cs = CPU(cpu);
>      CPUPPCState *env = &cpu->env;
>      target_ulong msr, new_msr, vector;
> -    int srr0, srr1, asrr0, asrr1, lev = -1, ail;
> +    int srr0, srr1, asrr0, asrr1, lev = -1;
>      bool lpes0;
> 
>      qemu_log_mask(CPU_LOG_INT, "Raise exception at " TARGET_FMT_lx
> @@ -238,25 +320,16 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>       *
>       * On anything else, we behave as if LPES0 is 1
>       * (externals don't alter MSR:HV)
> -     *
> -     * 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 ||
>          excp_model == POWERPC_EXCP_POWER9) {
>          lpes0 = !!(env->spr[SPR_LPCR] & LPCR_LPES0);
> -        if (excp_model != POWERPC_EXCP_POWER7) {
> -            ail = (env->spr[SPR_LPCR] & LPCR_AIL) >> LPCR_AIL_SHIFT;
> -        } else {
> -            ail = 0;
> -        }
>      } else
>  #endif /* defined(TARGET_PPC64) */
>      {
>          lpes0 = true;
> -        ail = 0;
>      }
> 
>      /*
> @@ -315,7 +388,6 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>               */
>              new_msr |= (target_ulong)MSR_HVB;
>          }
> -        ail = 0;
> 
>          /* machine check exceptions don't have ME set */
>          new_msr &= ~((target_ulong)1 << MSR_ME);
> @@ -519,7 +591,6 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>                            "exception %d with no HV support\n", excp);
>              }
>          }
> -        ail = 0;
>          break;
>      case POWERPC_EXCP_DSEG:      /* Data segment exception                   */
>      case POWERPC_EXCP_ISEG:      /* Instruction segment exception            */
> @@ -790,24 +861,6 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>      }
>  #endif
> 
> -    /*
> -     * AIL only works if MSR[IR] and MSR[DR] are both enabled.
> -     */
> -    if (!((msr >> MSR_IR) & 1) || !((msr >> MSR_DR) & 1)) {
> -        ail = 0;
> -    }
> -
> -    /*
> -     * AIL does not work if there is a MSR[HV] 0->1 transition and the
> -     * partition is in HPT mode. For radix guests, such interrupts are
> -     * allowed to be delivered to the hypervisor in ail mode.
> -     */
> -    if ((new_msr & MSR_HVB) && !(msr & MSR_HVB)) {
> -        if (!(env->spr[SPR_LPCR] & LPCR_HR)) {
> -            ail = 0;
> -        }
> -    }
> -
>      vector = env->excp_vectors[excp];
>      if (vector == (target_ulong)-1ULL) {
>          cpu_abort(cs, "Raised an exception without defined vector %d\n",
> @@ -848,23 +901,8 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>          /* Save MSR */
>          env->spr[srr1] = msr;
> 
> -        /* Handle AIL */
> -        if (ail) {
> -            new_msr |= (1 << MSR_IR) | (1 << MSR_DR);
> -            vector |= ppc_excp_vector_offset(cs, ail);
> -        }
> -
>  #if defined(TARGET_PPC64)
>      } else {
> -        /* scv AIL is a little different */
> -        if (ail) {
> -            new_msr |= (1 << MSR_IR) | (1 << MSR_DR);
> -        }
> -        if (ail == AIL_C000_0000_0000_4000) {
> -            vector |= 0xc000000000003000ull;
> -        } else {
> -            vector |= 0x0000000000017000ull;
> -        }
>          vector += lev * 0x20;
> 
>          env->lr = env->nip;
> @@ -872,6 +910,9 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>  #endif
>      }
> 
> +    /* This can update new_msr and vector if AIL applies */
> +    ppc_excp_apply_ail(cpu, excp_model, excp, msr, &new_msr, &vector);
> +
>      powerpc_set_excp_state(cpu, vector, new_msr);
>  }
> 
> diff --git a/target/ppc/translate_init.c.inc b/target/ppc/translate_init.c.inc
> index 70f9b9b150..a82d9ed647 100644
> --- a/target/ppc/translate_init.c.inc
> +++ b/target/ppc/translate_init.c.inc
> @@ -3457,7 +3457,7 @@ static void init_excp_POWER9(CPUPPCState *env)
> 
>  #if !defined(CONFIG_USER_ONLY)
>      env->excp_vectors[POWERPC_EXCP_HVIRT]    = 0x00000EA0;
> -    env->excp_vectors[POWERPC_EXCP_SYSCALL_VECTORED] = 0x00000000;
> +    env->excp_vectors[POWERPC_EXCP_SYSCALL_VECTORED] = 0x00017000;
>  #endif
>  }
> 



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [EXTERNAL] [RFC PATCH 2/2] target/ppc: Add POWER10 exception model
  2021-04-14  3:23 ` [RFC PATCH 2/2] target/ppc: Add POWER10 exception model Nicholas Piggin
@ 2021-04-14 15:54   ` Cédric Le Goater
  2021-04-15  5:28     ` Nicholas Piggin
  0 siblings, 1 reply; 8+ messages in thread
From: Cédric Le Goater @ 2021-04-14 15:54 UTC (permalink / raw)
  To: Nicholas Piggin, qemu-ppc
  Cc: Fabiano Rosas, Cédric Le Goater, qemu-devel, David Gibson

On 4/14/21 5:23 AM, Nicholas Piggin wrote:
> POWER10 adds a new bit that modifies interrupt behaviour, LPCR[HAIL],
> and it removes support for the LPCR[AIL]=0b10 mode.

This looks good but it's missing the MSR_LE setting. A part from that : 

Reviewed-by: Cédric Le Goater <clg@kaod.org>

and 

Tested-by: Cédric Le Goater <clg@kaod.org>

distros using scv on P10 now need your patch to boot :

"powerpc/powernv: Enable HAIL (HV AIL) for ISA v3.1 processors"

I guess it will get merged in time. 

Thanks,

C.


> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  hw/ppc/spapr_hcall.c            |  7 +++++-
>  target/ppc/cpu-qom.h            |  2 ++
>  target/ppc/cpu.h                |  5 ++--
>  target/ppc/excp_helper.c        | 43 +++++++++++++++++++++++++++++++++
>  target/ppc/translate.c          |  3 ++-
>  target/ppc/translate_init.c.inc |  2 +-
>  6 files changed, 57 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 2fbe04a689..6802cd4dc8 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1396,7 +1396,12 @@ static target_ulong h_set_mode_resource_addr_trans_mode(PowerPCCPU *cpu,
>      }
> 
>      if (mflags == 1) {
> -        /* AIL=1 is reserved */
> +        /* AIL=1 is reserved in POWER8/POWER9 */
> +        return H_UNSUPPORTED_FLAG;
> +    }
> +
> +    if (mflags == 2 && (pcc->insns_flags2 & PPC2_ISA310)) {
> +        /* AIL=2 is also reserved in POWER10 (ISA v3.1) */
>          return H_UNSUPPORTED_FLAG;
>      }
> 
> diff --git a/target/ppc/cpu-qom.h b/target/ppc/cpu-qom.h
> index 118baf8d41..06b6571bc9 100644
> --- a/target/ppc/cpu-qom.h
> +++ b/target/ppc/cpu-qom.h
> @@ -116,6 +116,8 @@ enum powerpc_excp_t {
>      POWERPC_EXCP_POWER8,
>      /* POWER9 exception model           */
>      POWERPC_EXCP_POWER9,
> +    /* POWER10 exception model           */
> +    POWERPC_EXCP_POWER10,
>  };
> 
>  /*****************************************************************************/
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 5200a16d23..9d35cdfa92 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -354,10 +354,11 @@ typedef struct ppc_v3_pate_t {
>  #define LPCR_PECE_U_SHIFT (63 - 19)
>  #define LPCR_PECE_U_MASK  (0x7ull << LPCR_PECE_U_SHIFT)
>  #define LPCR_HVEE         PPC_BIT(17) /* Hypervisor Virt Exit Enable */
> -#define LPCR_RMLS_SHIFT   (63 - 37)
> +#define LPCR_RMLS_SHIFT   (63 - 37)   /* RMLS (removed in ISA v3.0) */
>  #define LPCR_RMLS         (0xfull << LPCR_RMLS_SHIFT)
> +#define LPCR_HAIL         PPC_BIT(37) /* ISA v3.1 HV AIL=3 equivalent */
>  #define LPCR_ILE          PPC_BIT(38)
> -#define LPCR_AIL_SHIFT    (63 - 40)      /* Alternate interrupt location */
> +#define LPCR_AIL_SHIFT    (63 - 40)   /* Alternate interrupt location */
>  #define LPCR_AIL          (3ull << LPCR_AIL_SHIFT)
>  #define LPCR_UPRT         PPC_BIT(41) /* Use Process Table */
>  #define LPCR_EVIRT        PPC_BIT(42) /* Enhanced Virtualisation */
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 9ff316767c..19931361a0 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -172,6 +172,26 @@ static int powerpc_reset_wakeup(CPUState *cs, CPUPPCState *env, int excp,
>   *
>   * The difference with POWER9 being that MSR[HV] 0->1 interrupts can be
>   * sent to the hypervisor in AIL mode if the guest is radix (LPCR[HR]=1).
> + * This is good for performance but allows the guest to influence the
> + * AIL of hypervisor interrupts using its MSR, and also the hypervisor
> + * must disallow guest interrupts (MSR[HV] 0->0) from using AIL if the
> + * hypervisor does not want to use AIL for its MSR[HV] 0->1 interrupts.
> + *
> + * POWER10 addresses those issues with a new LPCR[HAIL] bit that is
> + * applied to interrupt that begin execution with MSR[HV]=1 (so both
> + * MSR[HV] 0->1 and 1->1).
> + *
> + * HAIL=1 is equivalent to AIL=3, for interrupts delivered with MSR[HV]=1.
> + *
> + * POWER10 behaviour is
> + * | LPCR[AIL] | LPCR[HAIL] | MSR[IR||DR] | MSR[HV] | new MSR[HV] | AIL |
> + * +-----------+------------+-------------+---------+-------------+-----+
> + * | a         | h          | 00/01/10    | 0       | 0           | 0   |
> + * | a         | h          | 11          | 0       | 0           | a   |
> + * | a         | h          | x           | 0       | 1           | h   |
> + * | a         | h          | 00/01/10    | 1       | 1           | 0   |
> + * | a         | h          | 11          | 1       | 1           | h   |
> + * +--------------------------------------------------------------------+
>   */
>  static inline void ppc_excp_apply_ail(PowerPCCPU *cpu, int excp_model, int excp,
>                                        target_ulong msr,
> @@ -211,6 +231,29 @@ static inline void ppc_excp_apply_ail(PowerPCCPU *cpu, int excp_model, int excp,
>              /* AIL=1 is reserved */
>              return;
>          }
> +
> +    } else if (excp_model == POWERPC_EXCP_POWER10) {
> +        if (!mmu_all_on && !hv_escalation) {
> +            /*
> +             * AIL works for HV interrupts even with guest MSR[IR/DR] disabled.
> +             * Guest->guest and HV->HV interrupts do require MMU on.
> +             */
> +            return;
> +        }
> +
> +        if (*new_msr & MSR_HVB) {
> +            if (!(env->spr[SPR_LPCR] & LPCR_HAIL)) {
> +                /* HV interrupts depend on LPCR[HAIL] */
> +                return;
> +            }
> +            ail = 3; /* HAIL=1 gives AIL=3 behaviour for HV interrupts */
> +        } else {
> +            ail = (env->spr[SPR_LPCR] & LPCR_AIL) >> LPCR_AIL_SHIFT;
> +        }
> +        if (ail != 3) {
> +            /* AIL=1 and AIL=2 are reserved */
> +            return;
> +        }
>      } else {
>          /* Other processors do not support AIL */
>          return;
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 0984ce637b..e9ed001229 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -7731,7 +7731,8 @@ void ppc_cpu_dump_state(CPUState *cs, FILE *f, int flags)
>  #if defined(TARGET_PPC64)
>      if (env->excp_model == POWERPC_EXCP_POWER7 ||
>          env->excp_model == POWERPC_EXCP_POWER8 ||
> -        env->excp_model == POWERPC_EXCP_POWER9)  {
> +        env->excp_model == POWERPC_EXCP_POWER9 ||
> +        env->excp_model == POWERPC_EXCP_POWER10)  {
>          qemu_fprintf(f, "HSRR0 " TARGET_FMT_lx " HSRR1 " TARGET_FMT_lx "\n",
>                       env->spr[SPR_HSRR0], env->spr[SPR_HSRR1]);
>      }
> diff --git a/target/ppc/translate_init.c.inc b/target/ppc/translate_init.c.inc
> index a82d9ed647..76d82cc2f6 100644
> --- a/target/ppc/translate_init.c.inc
> +++ b/target/ppc/translate_init.c.inc
> @@ -9317,7 +9317,7 @@ POWERPC_FAMILY(POWER10)(ObjectClass *oc, void *data)
>      pcc->radix_page_info = &POWER10_radix_page_info;
>      pcc->lrg_decr_bits = 56;
>  #endif
> -    pcc->excp_model = POWERPC_EXCP_POWER9;
> +    pcc->excp_model = POWERPC_EXCP_POWER10;
>      pcc->bus_model = PPC_FLAGS_INPUT_POWER9;
>      pcc->bfd_mach = bfd_mach_ppc64;
>      pcc->flags = POWERPC_FLAG_VRE | POWERPC_FLAG_SE |
> 



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [EXTERNAL] [RFC PATCH 1/2] target/ppc: rework AIL logic in interrupt delivery
  2021-04-14 15:24   ` [EXTERNAL] " Cédric Le Goater
@ 2021-04-15  5:25     ` Nicholas Piggin
  0 siblings, 0 replies; 8+ messages in thread
From: Nicholas Piggin @ 2021-04-15  5:25 UTC (permalink / raw)
  To: Cédric Le Goater, qemu-ppc
  Cc: Fabiano Rosas, Cédric Le Goater, qemu-devel, David Gibson

Excerpts from Cédric Le Goater's message of April 15, 2021 1:24 am:
> On 4/14/21 5:23 AM, Nicholas Piggin wrote:
>> The AIL logic is becoming unmanageable spread all over powerpc_excp(),
>> and it is slated to get even worse with POWER10 support.
>> 
>> Move it all to a new helper function.
> 
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> Tested-by: Cédric Le Goater <clg@kaod.org>
> 
> Thanks for the effort and the documentation. One minor comment below,
> 
> C.
> 
>> 
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>>  hw/ppc/spapr_hcall.c            |   3 +-
>>  target/ppc/cpu.h                |   8 --
>>  target/ppc/excp_helper.c        | 161 ++++++++++++++++++++------------
>>  target/ppc/translate_init.c.inc |   2 +-
>>  4 files changed, 104 insertions(+), 70 deletions(-)
>> 
>> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
>> index 7b5cd3553c..2fbe04a689 100644
>> --- a/hw/ppc/spapr_hcall.c
>> +++ b/hw/ppc/spapr_hcall.c
>> @@ -1395,7 +1395,8 @@ static target_ulong h_set_mode_resource_addr_trans_mode(PowerPCCPU *cpu,
>>          return H_P4;
>>      }
>> 
>> -    if (mflags == AIL_RESERVED) {
>> +    if (mflags == 1) {
>> +        /* AIL=1 is reserved */
>>          return H_UNSUPPORTED_FLAG;
>>      }
>> 
>> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
>> index e73416da68..5200a16d23 100644
>> --- a/target/ppc/cpu.h
>> +++ b/target/ppc/cpu.h
>> @@ -2375,14 +2375,6 @@ enum {
>>      HMER_XSCOM_STATUS_MASK      = PPC_BITMASK(21, 23),
>>  };
>> 
>> -/* Alternate Interrupt Location (AIL) */
>> -enum {
>> -    AIL_NONE                = 0,
>> -    AIL_RESERVED            = 1,
>> -    AIL_0001_8000           = 2,
>> -    AIL_C000_0000_0000_4000 = 3,
>> -};
> 
> I kind of like these. No big deal.

My thinking was they actually are just a POWER8 model of the AIL bits 
(e.g., they don't represent scv properly or AIL=2 reserved in P10), and 
they spread the meaning over multiple files. After this patch it's all
just in that single function.

>> 
>> -    switch (ail) {
>> -    case AIL_NONE:
>> -        break;
>> -    case AIL_0001_8000:
>> -        offset = 0x18000;
>> -        break;
>> -    case AIL_C000_0000_0000_4000:
>> -        offset = 0xc000000000004000ull;
>> -        break;
>> -    default:
>> -        cpu_abort(cs, "Invalid AIL combination %d\n", ail);
> 
> Could we keep this abort ? 

Well the abort is no longer there because we explicitly handle all 
cases, the reserved ones by just ignoring them. I don't know what
the hardware actually does if you tried to set it (it should ignore)
but I think this is nicer to not abort.

Thanks,
Nick


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [EXTERNAL] [RFC PATCH 2/2] target/ppc: Add POWER10 exception model
  2021-04-14 15:54   ` [EXTERNAL] " Cédric Le Goater
@ 2021-04-15  5:28     ` Nicholas Piggin
  2021-04-15  6:50       ` Cédric Le Goater
  0 siblings, 1 reply; 8+ messages in thread
From: Nicholas Piggin @ 2021-04-15  5:28 UTC (permalink / raw)
  To: Cédric Le Goater, qemu-ppc
  Cc: Fabiano Rosas, Cédric Le Goater, qemu-devel, David Gibson

Excerpts from Cédric Le Goater's message of April 15, 2021 1:54 am:
> On 4/14/21 5:23 AM, Nicholas Piggin wrote:
>> POWER10 adds a new bit that modifies interrupt behaviour, LPCR[HAIL],
>> and it removes support for the LPCR[AIL]=0b10 mode.
> 
> This looks good but it's missing the MSR_LE setting. A part from that : 

Oh, and lpes as well. Looks like a mis-merged from my original patch.
Thanks for catching it, great.

> 
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> 
> and 
> 
> Tested-by: Cédric Le Goater <clg@kaod.org>

Thanks, this was tested after you added the MSR_LE bit?
> 
> distros using scv on P10 now need your patch to boot :
> 
> "powerpc/powernv: Enable HAIL (HV AIL) for ISA v3.1 processors"
> 
> I guess it will get merged in time. 

Yes, unfortunately. Real hardware crashes the same way though, so
nothing to be done about it.

Thanks,
Nick


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [EXTERNAL] [RFC PATCH 2/2] target/ppc: Add POWER10 exception model
  2021-04-15  5:28     ` Nicholas Piggin
@ 2021-04-15  6:50       ` Cédric Le Goater
  0 siblings, 0 replies; 8+ messages in thread
From: Cédric Le Goater @ 2021-04-15  6:50 UTC (permalink / raw)
  To: Nicholas Piggin, qemu-ppc
  Cc: Fabiano Rosas, Cédric Le Goater, qemu-devel, David Gibson

On 4/15/21 7:28 AM, Nicholas Piggin wrote:
> Excerpts from Cédric Le Goater's message of April 15, 2021 1:54 am:
>> On 4/14/21 5:23 AM, Nicholas Piggin wrote:
>>> POWER10 adds a new bit that modifies interrupt behaviour, LPCR[HAIL],
>>> and it removes support for the LPCR[AIL]=0b10 mode.
>>
>> This looks good but it's missing the MSR_LE setting. A part from that : 
> 
> Oh, and lpes as well. Looks like a mis-merged from my original patch.
> Thanks for catching it, great.
> 
>>
>> Reviewed-by: Cédric Le Goater <clg@kaod.org>
>>
>> and 
>>
>> Tested-by: Cédric Le Goater <clg@kaod.org>
> 
> Thanks, this was tested after you added the MSR_LE bit?

yes.

Thanks,

C.

>> distros using scv on P10 now need your patch to boot :
>>
>> "powerpc/powernv: Enable HAIL (HV AIL) for ISA v3.1 processors"
>>
>> I guess it will get merged in time. 
> 
> Yes, unfortunately. Real hardware crashes the same way though, so
> nothing to be done about it.




^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-04-15  6:52 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-14  3:23 [RFC PATCH 0/2] ppc: rework AIL logic, add POWER10 exception model Nicholas Piggin
2021-04-14  3:23 ` [RFC PATCH 1/2] target/ppc: rework AIL logic in interrupt delivery Nicholas Piggin
2021-04-14 15:24   ` [EXTERNAL] " Cédric Le Goater
2021-04-15  5:25     ` Nicholas Piggin
2021-04-14  3:23 ` [RFC PATCH 2/2] target/ppc: Add POWER10 exception model Nicholas Piggin
2021-04-14 15:54   ` [EXTERNAL] " Cédric Le Goater
2021-04-15  5:28     ` Nicholas Piggin
2021-04-15  6:50       ` Cédric Le Goater

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).