qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v1 0/3] POWER9/POWER10 exception model fixes
@ 2021-04-13 12:54 Nicholas Piggin
  2021-04-13 12:54 ` [PATCH v1 1/3] target/ppc: Fix POWER9 radix guest HV interrupt AIL behaviour Nicholas Piggin
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Nicholas Piggin @ 2021-04-13 12:54 UTC (permalink / raw)
  To: qemu-ppc; +Cc: Cédric Le Goater, qemu-devel, Nicholas Piggin, David Gibson

This fixes a powernv9 bug, and several POWER10 bits (which affect
powernv10 and pseries power10).

I think I got the logic right but I didn't individually test and
verify every case, so any glances over it would be appreciated.

Thanks,
Nick

Nicholas Piggin (3):
  target/ppc: Fix POWER9 radix guest HV interrupt AIL behaviour
  target/ppc: POWER10 supports scv
  target/ppc: Add POWER10 exception model

 hw/ppc/spapr_hcall.c            |   5 ++
 target/ppc/cpu-qom.h            |   2 +
 target/ppc/cpu.h                |   5 +-
 target/ppc/excp_helper.c        | 111 ++++++++++++++++++++++++++++----
 target/ppc/translate.c          |   3 +-
 target/ppc/translate_init.c.inc |   4 +-
 6 files changed, 111 insertions(+), 19 deletions(-)

-- 
2.23.0



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

* [PATCH v1 1/3] target/ppc: Fix POWER9 radix guest HV interrupt AIL behaviour
  2021-04-13 12:54 [PATCH v1 0/3] POWER9/POWER10 exception model fixes Nicholas Piggin
@ 2021-04-13 12:54 ` Nicholas Piggin
  2021-04-13 13:48   ` Fabiano Rosas
  2021-04-13 14:31   ` [EXTERNAL] " Cédric Le Goater
  2021-04-13 12:54 ` [PATCH v1 2/3] target/ppc: POWER10 supports scv Nicholas Piggin
  2021-04-13 12:54 ` [PATCH v1 3/3] target/ppc: Add POWER10 exception model Nicholas Piggin
  2 siblings, 2 replies; 13+ messages in thread
From: Nicholas Piggin @ 2021-04-13 12:54 UTC (permalink / raw)
  To: qemu-ppc; +Cc: Cédric Le Goater, qemu-devel, Nicholas Piggin, David Gibson

ISA v3.0 radix guest execution has a quirk in AIL behaviour such that
the LPCR[AIL] value can apply to hypervisor interrupts.

This affects machines that emulate HV=1 mode (i.e., powernv9).

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 target/ppc/excp_helper.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 85de7e6c90..b8881c0f85 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -791,14 +791,23 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
 #endif
 
     /*
-     * AIL only works if there is no HV transition and we are running
-     * with translations enabled
+     * AIL only works if MSR[IR] and MSR[DR] are both enabled.
      */
-    if (!((msr >> MSR_IR) & 1) || !((msr >> MSR_DR) & 1) ||
-        ((new_msr & MSR_HVB) && !(msr & MSR_HVB))) {
+    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",
-- 
2.23.0



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

* [PATCH v1 2/3] target/ppc: POWER10 supports scv
  2021-04-13 12:54 [PATCH v1 0/3] POWER9/POWER10 exception model fixes Nicholas Piggin
  2021-04-13 12:54 ` [PATCH v1 1/3] target/ppc: Fix POWER9 radix guest HV interrupt AIL behaviour Nicholas Piggin
@ 2021-04-13 12:54 ` Nicholas Piggin
  2021-04-13 14:18   ` [EXTERNAL] " Cédric Le Goater
  2021-04-13 15:54   ` Fabiano Rosas
  2021-04-13 12:54 ` [PATCH v1 3/3] target/ppc: Add POWER10 exception model Nicholas Piggin
  2 siblings, 2 replies; 13+ messages in thread
From: Nicholas Piggin @ 2021-04-13 12:54 UTC (permalink / raw)
  To: qemu-ppc; +Cc: Cédric Le Goater, qemu-devel, Nicholas Piggin, David Gibson

This must have slipped through the cracks between adding POWER10 support
and scv support.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 target/ppc/translate_init.c.inc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/ppc/translate_init.c.inc b/target/ppc/translate_init.c.inc
index c03a7c4f52..70f9b9b150 100644
--- a/target/ppc/translate_init.c.inc
+++ b/target/ppc/translate_init.c.inc
@@ -9323,7 +9323,7 @@ POWERPC_FAMILY(POWER10)(ObjectClass *oc, void *data)
     pcc->flags = POWERPC_FLAG_VRE | POWERPC_FLAG_SE |
                  POWERPC_FLAG_BE | POWERPC_FLAG_PMM |
                  POWERPC_FLAG_BUS_CLK | POWERPC_FLAG_CFAR |
-                 POWERPC_FLAG_VSX | POWERPC_FLAG_TM;
+                 POWERPC_FLAG_VSX | POWERPC_FLAG_TM | POWERPC_FLAG_SCV;
     pcc->l1_dcache_size = 0x8000;
     pcc->l1_icache_size = 0x8000;
     pcc->interrupts_big_endian = ppc_cpu_interrupts_big_endian_lpcr;
-- 
2.23.0



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

* [PATCH v1 3/3] target/ppc: Add POWER10 exception model
  2021-04-13 12:54 [PATCH v1 0/3] POWER9/POWER10 exception model fixes Nicholas Piggin
  2021-04-13 12:54 ` [PATCH v1 1/3] target/ppc: Fix POWER9 radix guest HV interrupt AIL behaviour Nicholas Piggin
  2021-04-13 12:54 ` [PATCH v1 2/3] target/ppc: POWER10 supports scv Nicholas Piggin
@ 2021-04-13 12:54 ` Nicholas Piggin
  2021-04-13 15:53   ` Fabiano Rosas
  2 siblings, 1 reply; 13+ messages in thread
From: Nicholas Piggin @ 2021-04-13 12:54 UTC (permalink / raw)
  To: qemu-ppc; +Cc: 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            |   5 ++
 target/ppc/cpu-qom.h            |   2 +
 target/ppc/cpu.h                |   5 +-
 target/ppc/excp_helper.c        | 114 ++++++++++++++++++++++++++------
 target/ppc/translate.c          |   3 +-
 target/ppc/translate_init.c.inc |   2 +-
 6 files changed, 107 insertions(+), 24 deletions(-)

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 7b5cd3553c..2f65f20f72 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1399,6 +1399,11 @@ static target_ulong h_set_mode_resource_addr_trans_mode(PowerPCCPU *cpu,
         return H_UNSUPPORTED_FLAG;
     }
 
+    if (mflags == AIL_0001_8000 && (pcc->insns_flags2 & PPC2_ISA310)) {
+        /* AIL 2 is also reserved in ISA v3.1 */
+        return H_UNSUPPORTED_FLAG;
+    }
+
     spapr_set_all_lpcrs(mflags << LPCR_AIL_SHIFT, LPCR_AIL);
 
     return H_SUCCESS;
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 e73416da68..24e53e0ac3 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 b8881c0f85..e8bf0598b4 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -197,7 +197,8 @@ 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;
+    int ail, hail, using_ail;
     bool lpes0;
 
     qemu_log_mask(CPU_LOG_INT, "Raise exception at " TARGET_FMT_lx
@@ -239,24 +240,39 @@ 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
+     * ail/hail are initialized here but can be cleared by
+     * selected exceptions, and other conditions afterwards.
      */
 #if defined(TARGET_PPC64)
     if (excp_model == POWERPC_EXCP_POWER7 ||
         excp_model == POWERPC_EXCP_POWER8 ||
-        excp_model == POWERPC_EXCP_POWER9) {
+        excp_model == POWERPC_EXCP_POWER9 ||
+        excp_model == POWERPC_EXCP_POWER10) {
         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;
         }
+        if (excp_model == POWERPC_EXCP_POWER10) {
+            if (AIL_0001_8000) {
+                ail = 0; /* AIL=2 is reserved in ISA v3.1 */
+            }
+
+            if (env->spr[SPR_LPCR] & LPCR_HAIL) {
+                hail = AIL_C000_0000_0000_4000;
+            } else {
+                hail = 0;
+            }
+        } else {
+            hail = ail;
+        }
     } else
 #endif /* defined(TARGET_PPC64) */
     {
         lpes0 = true;
         ail = 0;
+        hail = 0;
     }
 
     /*
@@ -316,6 +332,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
             new_msr |= (target_ulong)MSR_HVB;
         }
         ail = 0;
+        hail = 0;
 
         /* machine check exceptions don't have ME set */
         new_msr &= ~((target_ulong)1 << MSR_ME);
@@ -520,6 +537,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
             }
         }
         ail = 0;
+        hail = 0;
         break;
     case POWERPC_EXCP_DSEG:      /* Data segment exception                   */
     case POWERPC_EXCP_ISEG:      /* Instruction segment exception            */
@@ -773,7 +791,8 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
         } else if (env->spr[SPR_LPCR] & LPCR_ILE) {
             new_msr |= (target_ulong)1 << MSR_LE;
         }
-    } else if (excp_model == POWERPC_EXCP_POWER9) {
+    } else if (excp_model == POWERPC_EXCP_POWER9 ||
+               excp_model == POWERPC_EXCP_POWER10) {
         if (new_msr & MSR_HVB) {
             if (env->spr[SPR_HID0] & HID0_POWER9_HILE) {
                 new_msr |= (target_ulong)1 << MSR_LE;
@@ -791,22 +810,77 @@ 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.
+     * The ail variable is for AIL behaviour for interrupts that begin
+     * execution with MSR[HV]=0, and the hail variable is for AIL behaviour for
+     * interrupts that begin execution with MSR[HV]=1.
+     *
+     * There is a large matrix of behaviours depending on the processor and
+     * the current operating modes when the interrupt is taken, and the
+     * interrupt type. The below tables lists behaviour for interrupts except
+     * for SRESET, machine check, and HMI (which are all taken in real mode
+     * with AIL 0).
+     *
+     * 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).
+     * This is good for performance but allows the guest to influence the
+     * AIL of the hypervisor interrupt 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).
+     *
+     * 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   |
+     * +--------------------------------------------------------------------+
      */
-    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)) {
+    if (excp_model == POWERPC_EXCP_POWER8 ||
+        excp_model == POWERPC_EXCP_POWER9) {
+        if (!((msr >> MSR_IR) & 1) || !((msr >> MSR_DR) & 1)) {
             ail = 0;
+            hail = 0;
+        } else if ((new_msr & MSR_HVB) && !(msr & MSR_HVB)) {
+            if (!(env->spr[SPR_LPCR] & LPCR_HR)) {
+                hail = 0;
+            }
+        }
+    } else if (excp_model == POWERPC_EXCP_POWER10) {
+        if ((new_msr & MSR_HVB) == (msr & MSR_HVB)) {
+            if (!((msr >> MSR_IR) & 1) || !((msr >> MSR_DR) & 1)) {
+                ail = 0;
+                hail = 0;
+            }
         }
     }
+    if (new_msr & MSR_HVB) {
+        using_ail = hail;
+    } else {
+        using_ail = ail;
+    }
 
     vector = env->excp_vectors[excp];
     if (vector == (target_ulong)-1ULL) {
@@ -849,18 +923,18 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
         env->spr[srr1] = msr;
 
         /* Handle AIL */
-        if (ail) {
+        if (using_ail) {
             new_msr |= (1 << MSR_IR) | (1 << MSR_DR);
-            vector |= ppc_excp_vector_offset(cs, ail);
+            vector |= ppc_excp_vector_offset(cs, using_ail);
         }
 
 #if defined(TARGET_PPC64)
     } else {
         /* scv AIL is a little different */
-        if (ail) {
+        if (using_ail) {
             new_msr |= (1 << MSR_IR) | (1 << MSR_DR);
         }
-        if (ail == AIL_C000_0000_0000_4000) {
+        if (using_ail == AIL_C000_0000_0000_4000) {
             vector |= 0xc000000000003000ull;
         } else {
             vector |= 0x0000000000017000ull;
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 70f9b9b150..5d427e9d38 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	[flat|nested] 13+ messages in thread

* Re: [PATCH v1 1/3] target/ppc: Fix POWER9 radix guest HV interrupt AIL behaviour
  2021-04-13 12:54 ` [PATCH v1 1/3] target/ppc: Fix POWER9 radix guest HV interrupt AIL behaviour Nicholas Piggin
@ 2021-04-13 13:48   ` Fabiano Rosas
  2021-04-14  3:26     ` Nicholas Piggin
  2021-04-13 14:31   ` [EXTERNAL] " Cédric Le Goater
  1 sibling, 1 reply; 13+ messages in thread
From: Fabiano Rosas @ 2021-04-13 13:48 UTC (permalink / raw)
  To: Nicholas Piggin, qemu-ppc
  Cc: Cédric Le Goater, qemu-devel, Nicholas Piggin, David Gibson

Nicholas Piggin <npiggin@gmail.com> writes:

> ISA v3.0 radix guest execution has a quirk in AIL behaviour such that
> the LPCR[AIL] value can apply to hypervisor interrupts.
>
> This affects machines that emulate HV=1 mode (i.e., powernv9).
>

Ah ok, so we actually want to replicate the quirk in the pnv
machine. Took me a while.

Reviewed-by: Fabiano Rosas <farosas@linux.ibm.com>

> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  target/ppc/excp_helper.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 85de7e6c90..b8881c0f85 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -791,14 +791,23 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>  #endif
>  
>      /*
> -     * AIL only works if there is no HV transition and we are running
> -     * with translations enabled
> +     * AIL only works if MSR[IR] and MSR[DR] are both enabled.
>       */
> -    if (!((msr >> MSR_IR) & 1) || !((msr >> MSR_DR) & 1) ||
> -        ((new_msr & MSR_HVB) && !(msr & MSR_HVB))) {
> +    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",


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

* Re: [EXTERNAL] [PATCH v1 2/3] target/ppc: POWER10 supports scv
  2021-04-13 12:54 ` [PATCH v1 2/3] target/ppc: POWER10 supports scv Nicholas Piggin
@ 2021-04-13 14:18   ` Cédric Le Goater
  2021-04-13 15:54   ` Fabiano Rosas
  1 sibling, 0 replies; 13+ messages in thread
From: Cédric Le Goater @ 2021-04-13 14:18 UTC (permalink / raw)
  To: Nicholas Piggin, qemu-ppc; +Cc: Cédric Le Goater, qemu-devel, David Gibson

Nick, 

I really prefer clg@kaod.org :)

On 4/13/21 2:54 PM, Nicholas Piggin wrote:
> This must have slipped through the cracks between adding POWER10 support
> and scv support.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

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

and 

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

on a recently enabled scv enabled distro.

Thanks,

C.  


> ---
>  target/ppc/translate_init.c.inc | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/ppc/translate_init.c.inc b/target/ppc/translate_init.c.inc
> index c03a7c4f52..70f9b9b150 100644
> --- a/target/ppc/translate_init.c.inc
> +++ b/target/ppc/translate_init.c.inc
> @@ -9323,7 +9323,7 @@ POWERPC_FAMILY(POWER10)(ObjectClass *oc, void *data)
>      pcc->flags = POWERPC_FLAG_VRE | POWERPC_FLAG_SE |
>                   POWERPC_FLAG_BE | POWERPC_FLAG_PMM |
>                   POWERPC_FLAG_BUS_CLK | POWERPC_FLAG_CFAR |
> -                 POWERPC_FLAG_VSX | POWERPC_FLAG_TM;
> +                 POWERPC_FLAG_VSX | POWERPC_FLAG_TM | POWERPC_FLAG_SCV;
>      pcc->l1_dcache_size = 0x8000;
>      pcc->l1_icache_size = 0x8000;
>      pcc->interrupts_big_endian = ppc_cpu_interrupts_big_endian_lpcr;
> 



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

* Re: [EXTERNAL] [PATCH v1 1/3] target/ppc: Fix POWER9 radix guest HV interrupt AIL behaviour
  2021-04-13 12:54 ` [PATCH v1 1/3] target/ppc: Fix POWER9 radix guest HV interrupt AIL behaviour Nicholas Piggin
  2021-04-13 13:48   ` Fabiano Rosas
@ 2021-04-13 14:31   ` Cédric Le Goater
  1 sibling, 0 replies; 13+ messages in thread
From: Cédric Le Goater @ 2021-04-13 14:31 UTC (permalink / raw)
  To: Nicholas Piggin, qemu-ppc; +Cc: Cédric Le Goater, qemu-devel, David Gibson

On 4/13/21 2:54 PM, Nicholas Piggin wrote:
> ISA v3.0 radix guest execution has a quirk in AIL behaviour such that
> the LPCR[AIL] value can apply to hypervisor interrupts.

Shouldn't we test for P9 ? But I think you are using a new exception 
model for P10 in the next patch. 

I guess it's ok for now.

> 
> This affects machines that emulate HV=1 mode (i.e., powernv9).
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  target/ppc/excp_helper.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 85de7e6c90..b8881c0f85 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -791,14 +791,23 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>  #endif
> 
>      /*
> -     * AIL only works if there is no HV transition and we are running
> -     * with translations enabled
> +     * AIL only works if MSR[IR] and MSR[DR] are both enabled.
>       */
> -    if (!((msr >> MSR_IR) & 1) || !((msr >> MSR_DR) & 1) ||
> -        ((new_msr & MSR_HVB) && !(msr & MSR_HVB))) {
> +    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)) {

We have a ppc64_v3_radix() helper but this is minor.

C.

> +            ail = 0;
> +        }
> +    }
> +
>      vector = env->excp_vectors[excp];
>      if (vector == (target_ulong)-1ULL) {
>          cpu_abort(cs, "Raised an exception without defined vector %d\n",
> 



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

* Re: [PATCH v1 3/3] target/ppc: Add POWER10 exception model
  2021-04-13 12:54 ` [PATCH v1 3/3] target/ppc: Add POWER10 exception model Nicholas Piggin
@ 2021-04-13 15:53   ` Fabiano Rosas
  2021-04-13 17:09     ` Cédric Le Goater
  2021-04-14  3:28     ` Nicholas Piggin
  0 siblings, 2 replies; 13+ messages in thread
From: Fabiano Rosas @ 2021-04-13 15:53 UTC (permalink / raw)
  To: Nicholas Piggin, qemu-ppc
  Cc: Cédric Le Goater, qemu-devel, Nicholas Piggin, David Gibson

Nicholas Piggin <npiggin@gmail.com> writes:

> 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            |   5 ++
>  target/ppc/cpu-qom.h            |   2 +
>  target/ppc/cpu.h                |   5 +-
>  target/ppc/excp_helper.c        | 114 ++++++++++++++++++++++++++------
>  target/ppc/translate.c          |   3 +-
>  target/ppc/translate_init.c.inc |   2 +-
>  6 files changed, 107 insertions(+), 24 deletions(-)
>
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 7b5cd3553c..2f65f20f72 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1399,6 +1399,11 @@ static target_ulong h_set_mode_resource_addr_trans_mode(PowerPCCPU *cpu,
>          return H_UNSUPPORTED_FLAG;
>      }
>  
> +    if (mflags == AIL_0001_8000 && (pcc->insns_flags2 & PPC2_ISA310)) {
> +        /* AIL 2 is also reserved in ISA v3.1 */
> +        return H_UNSUPPORTED_FLAG;
> +    }
> +
>      spapr_set_all_lpcrs(mflags << LPCR_AIL_SHIFT, LPCR_AIL);
>  
>      return H_SUCCESS;
> 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 e73416da68..24e53e0ac3 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 b8881c0f85..e8bf0598b4 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -197,7 +197,8 @@ 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;
> +    int ail, hail, using_ail;
>      bool lpes0;
>  
>      qemu_log_mask(CPU_LOG_INT, "Raise exception at " TARGET_FMT_lx
> @@ -239,24 +240,39 @@ 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
> +     * ail/hail are initialized here but can be cleared by
> +     * selected exceptions, and other conditions afterwards.
>       */
>  #if defined(TARGET_PPC64)
>      if (excp_model == POWERPC_EXCP_POWER7 ||
>          excp_model == POWERPC_EXCP_POWER8 ||
> -        excp_model == POWERPC_EXCP_POWER9) {
> +        excp_model == POWERPC_EXCP_POWER9 ||
> +        excp_model == POWERPC_EXCP_POWER10) {
>          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;
>          }
> +        if (excp_model == POWERPC_EXCP_POWER10) {
> +            if (AIL_0001_8000) {

if (2)

> +                ail = 0; /* AIL=2 is reserved in ISA v3.1 */
> +            }
> +
> +            if (env->spr[SPR_LPCR] & LPCR_HAIL) {
> +                hail = AIL_C000_0000_0000_4000;
> +            } else {
> +                hail = 0;
> +            }
> +        } else {
> +            hail = ail;

I think it's better if we set hail = 0 here. Read on...

> +        }
>      } else
>  #endif /* defined(TARGET_PPC64) */
>      {
>          lpes0 = true;
>          ail = 0;
> +        hail = 0;
>      }
>  
>      /*
> @@ -316,6 +332,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>              new_msr |= (target_ulong)MSR_HVB;
>          }
>          ail = 0;
> +        hail = 0;
>  
>          /* machine check exceptions don't have ME set */
>          new_msr &= ~((target_ulong)1 << MSR_ME);
> @@ -520,6 +537,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>              }
>          }
>          ail = 0;
> +        hail = 0;
>          break;
>      case POWERPC_EXCP_DSEG:      /* Data segment exception                   */
>      case POWERPC_EXCP_ISEG:      /* Instruction segment exception            */
> @@ -773,7 +791,8 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>          } else if (env->spr[SPR_LPCR] & LPCR_ILE) {
>              new_msr |= (target_ulong)1 << MSR_LE;
>          }
> -    } else if (excp_model == POWERPC_EXCP_POWER9) {
> +    } else if (excp_model == POWERPC_EXCP_POWER9 ||
> +               excp_model == POWERPC_EXCP_POWER10) {
>          if (new_msr & MSR_HVB) {
>              if (env->spr[SPR_HID0] & HID0_POWER9_HILE) {
>                  new_msr |= (target_ulong)1 << MSR_LE;
> @@ -791,22 +810,77 @@ 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.
> +     * The ail variable is for AIL behaviour for interrupts that begin
> +     * execution with MSR[HV]=0, and the hail variable is for AIL behaviour for
> +     * interrupts that begin execution with MSR[HV]=1.
> +     *
> +     * There is a large matrix of behaviours depending on the processor and
> +     * the current operating modes when the interrupt is taken, and the
> +     * interrupt type. The below tables lists behaviour for interrupts except
> +     * for SRESET, machine check, and HMI (which are all taken in real mode
> +     * with AIL 0).
> +     *
> +     * 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).
> +     * This is good for performance but allows the guest to influence the
> +     * AIL of the hypervisor interrupt 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).

I'd put take these two paragraphs in the commit message as well. Just
alter the beginning:
s/The difference with POWER9 being that/In POWER9,/

> +     *
> +     * 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   |
> +     * +--------------------------------------------------------------------+
>       */
> -    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)) {
> +    if (excp_model == POWERPC_EXCP_POWER8 ||
> +        excp_model == POWERPC_EXCP_POWER9) {
> +        if (!((msr >> MSR_IR) & 1) || !((msr >> MSR_DR) & 1)) {
>              ail = 0;
> +            hail = 0;
> +        } else if ((new_msr & MSR_HVB) && !(msr & MSR_HVB)) {
> +            if (!(env->spr[SPR_LPCR] & LPCR_HR)) {
> +                hail = 0;
> +            }
> +        }
> +    } else if (excp_model == POWERPC_EXCP_POWER10) {
> +        if ((new_msr & MSR_HVB) == (msr & MSR_HVB)) {
> +            if (!((msr >> MSR_IR) & 1) || !((msr >> MSR_DR) & 1)) {
> +                ail = 0;
> +                hail = 0;
> +            }
>          }
>      }
> +    if (new_msr & MSR_HVB) {
> +        using_ail = hail;
> +    } else {
> +        using_ail = ail;
> +    }

So if at the start of the function we set:

<= P7:
ail = 0;
hail = 0;

P8,P9:
ail = LPCR_AIL;
hail = 0;

P10:
ail = LPCR_AIL, except AIL2;
hail = AIL3;

Then we could do this, which I think is more readable (comments are just
for the email):

// If there's an HV transition to 1, then HPT zeroes AIL and radix
// doesn't. For P10 we'll use HAIL anyway, so no need to check.

if ((new_msr & MSR_HVB) && !(msr & MSR_HVB)) &&
    !(env->spr[SPR_LPCR] & LPCR_HR)) {
          ail = 0;
}

// If P10 and HV=1, use HAIL instead of AIL

if (excp_model == POWERPC_EXCP_POWER10 && (new_msr & MSR_HVB)) {
   ail = hail;
}

// The IR|DR check always takes precedence and zeroes AIL/HAIL, no
matter the processor version.

if (!((msr >> MSR_IR) & 1) || !((msr >> MSR_DR) & 1)) {
    ail = 0;
}

(...)

vector |= ppc_excp_vector_offset(cs, ail);

I think that should work...*squints* 
What do you think?

>  
>      vector = env->excp_vectors[excp];
>      if (vector == (target_ulong)-1ULL) {
> @@ -849,18 +923,18 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>          env->spr[srr1] = msr;
>  
>          /* Handle AIL */
> -        if (ail) {
> +        if (using_ail) {
>              new_msr |= (1 << MSR_IR) | (1 << MSR_DR);
> -            vector |= ppc_excp_vector_offset(cs, ail);
> +            vector |= ppc_excp_vector_offset(cs, using_ail);
>          }
>  
>  #if defined(TARGET_PPC64)
>      } else {
>          /* scv AIL is a little different */
> -        if (ail) {
> +        if (using_ail) {
>              new_msr |= (1 << MSR_IR) | (1 << MSR_DR);
>          }
> -        if (ail == AIL_C000_0000_0000_4000) {
> +        if (using_ail == AIL_C000_0000_0000_4000) {
>              vector |= 0xc000000000003000ull;
>          } else {
>              vector |= 0x0000000000017000ull;
> 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 70f9b9b150..5d427e9d38 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] 13+ messages in thread

* Re: [PATCH v1 2/3] target/ppc: POWER10 supports scv
  2021-04-13 12:54 ` [PATCH v1 2/3] target/ppc: POWER10 supports scv Nicholas Piggin
  2021-04-13 14:18   ` [EXTERNAL] " Cédric Le Goater
@ 2021-04-13 15:54   ` Fabiano Rosas
  1 sibling, 0 replies; 13+ messages in thread
From: Fabiano Rosas @ 2021-04-13 15:54 UTC (permalink / raw)
  To: Nicholas Piggin, qemu-ppc
  Cc: Cédric Le Goater, qemu-devel, Nicholas Piggin, David Gibson

Nicholas Piggin <npiggin@gmail.com> writes:

> This must have slipped through the cracks between adding POWER10 support
> and scv support.
>

Reviewed-by: Fabiano Rosas <farosas@linux.ibm.com>

> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  target/ppc/translate_init.c.inc | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/ppc/translate_init.c.inc b/target/ppc/translate_init.c.inc
> index c03a7c4f52..70f9b9b150 100644
> --- a/target/ppc/translate_init.c.inc
> +++ b/target/ppc/translate_init.c.inc
> @@ -9323,7 +9323,7 @@ POWERPC_FAMILY(POWER10)(ObjectClass *oc, void *data)
>      pcc->flags = POWERPC_FLAG_VRE | POWERPC_FLAG_SE |
>                   POWERPC_FLAG_BE | POWERPC_FLAG_PMM |
>                   POWERPC_FLAG_BUS_CLK | POWERPC_FLAG_CFAR |
> -                 POWERPC_FLAG_VSX | POWERPC_FLAG_TM;
> +                 POWERPC_FLAG_VSX | POWERPC_FLAG_TM | POWERPC_FLAG_SCV;
>      pcc->l1_dcache_size = 0x8000;
>      pcc->l1_icache_size = 0x8000;
>      pcc->interrupts_big_endian = ppc_cpu_interrupts_big_endian_lpcr;


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

* Re: [PATCH v1 3/3] target/ppc: Add POWER10 exception model
  2021-04-13 15:53   ` Fabiano Rosas
@ 2021-04-13 17:09     ` Cédric Le Goater
  2021-04-14  1:56       ` Nicholas Piggin
  2021-04-14  3:28     ` Nicholas Piggin
  1 sibling, 1 reply; 13+ messages in thread
From: Cédric Le Goater @ 2021-04-13 17:09 UTC (permalink / raw)
  To: Fabiano Rosas, Nicholas Piggin, qemu-ppc
  Cc: Cédric Le Goater, qemu-devel, David Gibson

On 4/13/21 5:53 PM, Fabiano Rosas wrote:
> Nicholas Piggin <npiggin@gmail.com> writes:
> 
>> 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            |   5 ++
>>  target/ppc/cpu-qom.h            |   2 +
>>  target/ppc/cpu.h                |   5 +-
>>  target/ppc/excp_helper.c        | 114 ++++++++++++++++++++++++++------
>>  target/ppc/translate.c          |   3 +-
>>  target/ppc/translate_init.c.inc |   2 +-
>>  6 files changed, 107 insertions(+), 24 deletions(-)
>>
>> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
>> index 7b5cd3553c..2f65f20f72 100644
>> --- a/hw/ppc/spapr_hcall.c
>> +++ b/hw/ppc/spapr_hcall.c
>> @@ -1399,6 +1399,11 @@ static target_ulong h_set_mode_resource_addr_trans_mode(PowerPCCPU *cpu,
>>          return H_UNSUPPORTED_FLAG;
>>      }
>>  
>> +    if (mflags == AIL_0001_8000 && (pcc->insns_flags2 & PPC2_ISA310)) {
>> +        /* AIL 2 is also reserved in ISA v3.1 */
>> +        return H_UNSUPPORTED_FLAG;
>> +    }
>> +
>>      spapr_set_all_lpcrs(mflags << LPCR_AIL_SHIFT, LPCR_AIL);
>>  
>>      return H_SUCCESS;
>> 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 e73416da68..24e53e0ac3 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 b8881c0f85..e8bf0598b4 100644
>> --- a/target/ppc/excp_helper.c
>> +++ b/target/ppc/excp_helper.c
>> @@ -197,7 +197,8 @@ 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;
>> +    int ail, hail, using_ail;
>>      bool lpes0;
>>  
>>      qemu_log_mask(CPU_LOG_INT, "Raise exception at " TARGET_FMT_lx
>> @@ -239,24 +240,39 @@ 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
>> +     * ail/hail are initialized here but can be cleared by
>> +     * selected exceptions, and other conditions afterwards.
>>       */
>>  #if defined(TARGET_PPC64)
>>      if (excp_model == POWERPC_EXCP_POWER7 ||
>>          excp_model == POWERPC_EXCP_POWER8 ||
>> -        excp_model == POWERPC_EXCP_POWER9) {
>> +        excp_model == POWERPC_EXCP_POWER9 ||
>> +        excp_model == POWERPC_EXCP_POWER10) {
>>          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;
>>          }
>> +        if (excp_model == POWERPC_EXCP_POWER10) {
>> +            if (AIL_0001_8000) {
> 
> if (2)
> 
>> +                ail = 0; /* AIL=2 is reserved in ISA v3.1 */
>> +            }
>> +
>> +            if (env->spr[SPR_LPCR] & LPCR_HAIL) {
>> +                hail = AIL_C000_0000_0000_4000;
>> +            } else {
>> +                hail = 0;
>> +            }
>> +        } else {
>> +            hail = ail;
> 
> I think it's better if we set hail = 0 here. Read on...
> 
>> +        }
>>      } else
>>  #endif /* defined(TARGET_PPC64) */
>>      {
>>          lpes0 = true;
>>          ail = 0;
>> +        hail = 0;
>>      }
>>  
>>      /*
>> @@ -316,6 +332,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>>              new_msr |= (target_ulong)MSR_HVB;
>>          }
>>          ail = 0;
>> +        hail = 0;
>>  
>>          /* machine check exceptions don't have ME set */
>>          new_msr &= ~((target_ulong)1 << MSR_ME);
>> @@ -520,6 +537,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>>              }
>>          }
>>          ail = 0;
>> +        hail = 0;
>>          break;
>>      case POWERPC_EXCP_DSEG:      /* Data segment exception                   */
>>      case POWERPC_EXCP_ISEG:      /* Instruction segment exception            */
>> @@ -773,7 +791,8 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>>          } else if (env->spr[SPR_LPCR] & LPCR_ILE) {
>>              new_msr |= (target_ulong)1 << MSR_LE;
>>          }
>> -    } else if (excp_model == POWERPC_EXCP_POWER9) {
>> +    } else if (excp_model == POWERPC_EXCP_POWER9 ||
>> +               excp_model == POWERPC_EXCP_POWER10) {
>>          if (new_msr & MSR_HVB) {
>>              if (env->spr[SPR_HID0] & HID0_POWER9_HILE) {
>>                  new_msr |= (target_ulong)1 << MSR_LE;
>> @@ -791,22 +810,77 @@ 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.
>> +     * The ail variable is for AIL behaviour for interrupts that begin
>> +     * execution with MSR[HV]=0, and the hail variable is for AIL behaviour for
>> +     * interrupts that begin execution with MSR[HV]=1.
>> +     *
>> +     * There is a large matrix of behaviours depending on the processor and
>> +     * the current operating modes when the interrupt is taken, and the
>> +     * interrupt type. The below tables lists behaviour for interrupts except
>> +     * for SRESET, machine check, and HMI (which are all taken in real mode
>> +     * with AIL 0).
>> +     *
>> +     * 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).
>> +     * This is good for performance but allows the guest to influence the
>> +     * AIL of the hypervisor interrupt 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).
> 
> I'd put take these two paragraphs in the commit message as well. Just
> alter the beginning:
> s/The difference with POWER9 being that/In POWER9,/
> 
>> +     *
>> +     * 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   |
>> +     * +--------------------------------------------------------------------+
>>       */
>> -    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)) {
>> +    if (excp_model == POWERPC_EXCP_POWER8 ||
>> +        excp_model == POWERPC_EXCP_POWER9) {
>> +        if (!((msr >> MSR_IR) & 1) || !((msr >> MSR_DR) & 1)) {
>>              ail = 0;
>> +            hail = 0;
>> +        } else if ((new_msr & MSR_HVB) && !(msr & MSR_HVB)) {
>> +            if (!(env->spr[SPR_LPCR] & LPCR_HR)) {
>> +                hail = 0;
>> +            }
>> +        }
>> +    } else if (excp_model == POWERPC_EXCP_POWER10) {
>> +        if ((new_msr & MSR_HVB) == (msr & MSR_HVB)) {
>> +            if (!((msr >> MSR_IR) & 1) || !((msr >> MSR_DR) & 1)) {
>> +                ail = 0;
>> +                hail = 0;
>> +            }
>>          }
>>      }
>> +    if (new_msr & MSR_HVB) {
>> +        using_ail = hail;
>> +    } else {
>> +        using_ail = ail;
>> +    }
> 
> So if at the start of the function we set:
> 
> <= P7:
> ail = 0;
> hail = 0;
> 
> P8,P9:
> ail = LPCR_AIL;
> hail = 0;
> 
> P10:
> ail = LPCR_AIL, except AIL2;
> hail = AIL3;
> 
> Then we could do this, which I think is more readable (comments are just
> for the email):
> 
> // If there's an HV transition to 1, then HPT zeroes AIL and radix
> // doesn't. For P10 we'll use HAIL anyway, so no need to check.
> 
> if ((new_msr & MSR_HVB) && !(msr & MSR_HVB)) &&
>     !(env->spr[SPR_LPCR] & LPCR_HR)) {
>           ail = 0;
> }
> 
> // If P10 and HV=1, use HAIL instead of AIL
> 
> if (excp_model == POWERPC_EXCP_POWER10 && (new_msr & MSR_HVB)) {
>    ail = hail;
> }
> 
> // The IR|DR check always takes precedence and zeroes AIL/HAIL, no
> matter the processor version.
> 
> if (!((msr >> MSR_IR) & 1) || !((msr >> MSR_DR) & 1)) {
>     ail = 0;
> }
> 
> (...)
> 
> vector |= ppc_excp_vector_offset(cs, ail);
> 
> I think that should work...*squints* 
> What do you think?

The powerpc_excp() routine is insane.

It feels that we should be duplicating the code in handlers for each 
of the POWER[7-10] CPUs and keep a default powerpc_excp() for the
others.

C.

> 
>>  
>>      vector = env->excp_vectors[excp];
>>      if (vector == (target_ulong)-1ULL) {
>> @@ -849,18 +923,18 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>>          env->spr[srr1] = msr;
>>  
>>          /* Handle AIL */
>> -        if (ail) {
>> +        if (using_ail) {
>>              new_msr |= (1 << MSR_IR) | (1 << MSR_DR);
>> -            vector |= ppc_excp_vector_offset(cs, ail);
>> +            vector |= ppc_excp_vector_offset(cs, using_ail);
>>          }
>>  
>>  #if defined(TARGET_PPC64)
>>      } else {
>>          /* scv AIL is a little different */
>> -        if (ail) {
>> +        if (using_ail) {
>>              new_msr |= (1 << MSR_IR) | (1 << MSR_DR);
>>          }
>> -        if (ail == AIL_C000_0000_0000_4000) {
>> +        if (using_ail == AIL_C000_0000_0000_4000) {
>>              vector |= 0xc000000000003000ull;
>>          } else {
>>              vector |= 0x0000000000017000ull;
>> 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 70f9b9b150..5d427e9d38 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] 13+ messages in thread

* Re: [PATCH v1 3/3] target/ppc: Add POWER10 exception model
  2021-04-13 17:09     ` Cédric Le Goater
@ 2021-04-14  1:56       ` Nicholas Piggin
  0 siblings, 0 replies; 13+ messages in thread
From: Nicholas Piggin @ 2021-04-14  1:56 UTC (permalink / raw)
  To: Cédric Le Goater, Fabiano Rosas, qemu-ppc
  Cc: Cédric Le Goater, qemu-devel, David Gibson

Excerpts from Cédric Le Goater's message of April 14, 2021 3:09 am:
> On 4/13/21 5:53 PM, Fabiano Rosas wrote:
>> Nicholas Piggin <npiggin@gmail.com> writes:
>> 
>>> 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            |   5 ++
>>>  target/ppc/cpu-qom.h            |   2 +
>>>  target/ppc/cpu.h                |   5 +-
>>>  target/ppc/excp_helper.c        | 114 ++++++++++++++++++++++++++------
>>>  target/ppc/translate.c          |   3 +-
>>>  target/ppc/translate_init.c.inc |   2 +-
>>>  6 files changed, 107 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
>>> index 7b5cd3553c..2f65f20f72 100644
>>> --- a/hw/ppc/spapr_hcall.c
>>> +++ b/hw/ppc/spapr_hcall.c
>>> @@ -1399,6 +1399,11 @@ static target_ulong h_set_mode_resource_addr_trans_mode(PowerPCCPU *cpu,
>>>          return H_UNSUPPORTED_FLAG;
>>>      }
>>>  
>>> +    if (mflags == AIL_0001_8000 && (pcc->insns_flags2 & PPC2_ISA310)) {
>>> +        /* AIL 2 is also reserved in ISA v3.1 */
>>> +        return H_UNSUPPORTED_FLAG;
>>> +    }
>>> +
>>>      spapr_set_all_lpcrs(mflags << LPCR_AIL_SHIFT, LPCR_AIL);
>>>  
>>>      return H_SUCCESS;
>>> 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 e73416da68..24e53e0ac3 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 b8881c0f85..e8bf0598b4 100644
>>> --- a/target/ppc/excp_helper.c
>>> +++ b/target/ppc/excp_helper.c
>>> @@ -197,7 +197,8 @@ 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;
>>> +    int ail, hail, using_ail;
>>>      bool lpes0;
>>>  
>>>      qemu_log_mask(CPU_LOG_INT, "Raise exception at " TARGET_FMT_lx
>>> @@ -239,24 +240,39 @@ 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
>>> +     * ail/hail are initialized here but can be cleared by
>>> +     * selected exceptions, and other conditions afterwards.
>>>       */
>>>  #if defined(TARGET_PPC64)
>>>      if (excp_model == POWERPC_EXCP_POWER7 ||
>>>          excp_model == POWERPC_EXCP_POWER8 ||
>>> -        excp_model == POWERPC_EXCP_POWER9) {
>>> +        excp_model == POWERPC_EXCP_POWER9 ||
>>> +        excp_model == POWERPC_EXCP_POWER10) {
>>>          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;
>>>          }
>>> +        if (excp_model == POWERPC_EXCP_POWER10) {
>>> +            if (AIL_0001_8000) {
>> 
>> if (2)
>> 
>>> +                ail = 0; /* AIL=2 is reserved in ISA v3.1 */
>>> +            }
>>> +
>>> +            if (env->spr[SPR_LPCR] & LPCR_HAIL) {
>>> +                hail = AIL_C000_0000_0000_4000;
>>> +            } else {
>>> +                hail = 0;
>>> +            }
>>> +        } else {
>>> +            hail = ail;
>> 
>> I think it's better if we set hail = 0 here. Read on...
>> 
>>> +        }
>>>      } else
>>>  #endif /* defined(TARGET_PPC64) */
>>>      {
>>>          lpes0 = true;
>>>          ail = 0;
>>> +        hail = 0;
>>>      }
>>>  
>>>      /*
>>> @@ -316,6 +332,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>>>              new_msr |= (target_ulong)MSR_HVB;
>>>          }
>>>          ail = 0;
>>> +        hail = 0;
>>>  
>>>          /* machine check exceptions don't have ME set */
>>>          new_msr &= ~((target_ulong)1 << MSR_ME);
>>> @@ -520,6 +537,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>>>              }
>>>          }
>>>          ail = 0;
>>> +        hail = 0;
>>>          break;
>>>      case POWERPC_EXCP_DSEG:      /* Data segment exception                   */
>>>      case POWERPC_EXCP_ISEG:      /* Instruction segment exception            */
>>> @@ -773,7 +791,8 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>>>          } else if (env->spr[SPR_LPCR] & LPCR_ILE) {
>>>              new_msr |= (target_ulong)1 << MSR_LE;
>>>          }
>>> -    } else if (excp_model == POWERPC_EXCP_POWER9) {
>>> +    } else if (excp_model == POWERPC_EXCP_POWER9 ||
>>> +               excp_model == POWERPC_EXCP_POWER10) {
>>>          if (new_msr & MSR_HVB) {
>>>              if (env->spr[SPR_HID0] & HID0_POWER9_HILE) {
>>>                  new_msr |= (target_ulong)1 << MSR_LE;
>>> @@ -791,22 +810,77 @@ 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.
>>> +     * The ail variable is for AIL behaviour for interrupts that begin
>>> +     * execution with MSR[HV]=0, and the hail variable is for AIL behaviour for
>>> +     * interrupts that begin execution with MSR[HV]=1.
>>> +     *
>>> +     * There is a large matrix of behaviours depending on the processor and
>>> +     * the current operating modes when the interrupt is taken, and the
>>> +     * interrupt type. The below tables lists behaviour for interrupts except
>>> +     * for SRESET, machine check, and HMI (which are all taken in real mode
>>> +     * with AIL 0).
>>> +     *
>>> +     * 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).
>>> +     * This is good for performance but allows the guest to influence the
>>> +     * AIL of the hypervisor interrupt 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).
>> 
>> I'd put take these two paragraphs in the commit message as well. Just
>> alter the beginning:
>> s/The difference with POWER9 being that/In POWER9,/
>> 
>>> +     *
>>> +     * 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   |
>>> +     * +--------------------------------------------------------------------+
>>>       */
>>> -    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)) {
>>> +    if (excp_model == POWERPC_EXCP_POWER8 ||
>>> +        excp_model == POWERPC_EXCP_POWER9) {
>>> +        if (!((msr >> MSR_IR) & 1) || !((msr >> MSR_DR) & 1)) {
>>>              ail = 0;
>>> +            hail = 0;
>>> +        } else if ((new_msr & MSR_HVB) && !(msr & MSR_HVB)) {
>>> +            if (!(env->spr[SPR_LPCR] & LPCR_HR)) {
>>> +                hail = 0;
>>> +            }
>>> +        }
>>> +    } else if (excp_model == POWERPC_EXCP_POWER10) {
>>> +        if ((new_msr & MSR_HVB) == (msr & MSR_HVB)) {
>>> +            if (!((msr >> MSR_IR) & 1) || !((msr >> MSR_DR) & 1)) {
>>> +                ail = 0;
>>> +                hail = 0;
>>> +            }
>>>          }
>>>      }
>>> +    if (new_msr & MSR_HVB) {
>>> +        using_ail = hail;
>>> +    } else {
>>> +        using_ail = ail;
>>> +    }
>> 
>> So if at the start of the function we set:
>> 
>> <= P7:
>> ail = 0;
>> hail = 0;
>> 
>> P8,P9:
>> ail = LPCR_AIL;
>> hail = 0;
>> 
>> P10:
>> ail = LPCR_AIL, except AIL2;
>> hail = AIL3;
>> 
>> Then we could do this, which I think is more readable (comments are just
>> for the email):
>> 
>> // If there's an HV transition to 1, then HPT zeroes AIL and radix
>> // doesn't. For P10 we'll use HAIL anyway, so no need to check.
>> 
>> if ((new_msr & MSR_HVB) && !(msr & MSR_HVB)) &&
>>     !(env->spr[SPR_LPCR] & LPCR_HR)) {
>>           ail = 0;
>> }
>> 
>> // If P10 and HV=1, use HAIL instead of AIL
>> 
>> if (excp_model == POWERPC_EXCP_POWER10 && (new_msr & MSR_HVB)) {
>>    ail = hail;
>> }
>> 
>> // The IR|DR check always takes precedence and zeroes AIL/HAIL, no
>> matter the processor version.
>> 
>> if (!((msr >> MSR_IR) & 1) || !((msr >> MSR_DR) & 1)) {
>>     ail = 0;
>> }
>> 
>> (...)
>> 
>> vector |= ppc_excp_vector_offset(cs, ail);
>> 
>> I think that should work...*squints* 
>> What do you think?
> 
> The powerpc_excp() routine is insane.
> 
> It feels that we should be duplicating the code in handlers for each 
> of the POWER[7-10] CPUs and keep a default powerpc_excp() for the
> others.

It's getting pretty wild :(

I was trying to come up with something in the way of a "fix" but I agree 
it needs a bigger reorganisation.

What would help a lot actually is moving all the AIL logic together. The 
only reason it's split completely in two like this is so a few 
interrupts can clear it. I think what would work better is just do all
that in one place, in the AIL logic. Let me see what that looks like. It
might actually be a saner fix in the end.

Thanks,
Nick


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

* Re: [PATCH v1 1/3] target/ppc: Fix POWER9 radix guest HV interrupt AIL behaviour
  2021-04-13 13:48   ` Fabiano Rosas
@ 2021-04-14  3:26     ` Nicholas Piggin
  0 siblings, 0 replies; 13+ messages in thread
From: Nicholas Piggin @ 2021-04-14  3:26 UTC (permalink / raw)
  To: Fabiano Rosas, qemu-ppc; +Cc: Cédric Le Goater, qemu-devel, David Gibson

Excerpts from Fabiano Rosas's message of April 13, 2021 11:48 pm:
> Nicholas Piggin <npiggin@gmail.com> writes:
> 
>> ISA v3.0 radix guest execution has a quirk in AIL behaviour such that
>> the LPCR[AIL] value can apply to hypervisor interrupts.
>>
>> This affects machines that emulate HV=1 mode (i.e., powernv9).
>>
> 
> Ah ok, so we actually want to replicate the quirk in the pnv
> machine. Took me a while.

Yes. Quirk is probably the wrong word for me to use. It's architected 
behaviour so it must be implemented, it's just slightly surprising /
easy to miss.

> 
> Reviewed-by: Fabiano Rosas <farosas@linux.ibm.com>

Thanks,
Nick

> 
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>>  target/ppc/excp_helper.c | 17 +++++++++++++----
>>  1 file changed, 13 insertions(+), 4 deletions(-)
>>
>> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
>> index 85de7e6c90..b8881c0f85 100644
>> --- a/target/ppc/excp_helper.c
>> +++ b/target/ppc/excp_helper.c
>> @@ -791,14 +791,23 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>>  #endif
>>  
>>      /*
>> -     * AIL only works if there is no HV transition and we are running
>> -     * with translations enabled
>> +     * AIL only works if MSR[IR] and MSR[DR] are both enabled.
>>       */
>> -    if (!((msr >> MSR_IR) & 1) || !((msr >> MSR_DR) & 1) ||
>> -        ((new_msr & MSR_HVB) && !(msr & MSR_HVB))) {
>> +    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",
> 


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

* Re: [PATCH v1 3/3] target/ppc: Add POWER10 exception model
  2021-04-13 15:53   ` Fabiano Rosas
  2021-04-13 17:09     ` Cédric Le Goater
@ 2021-04-14  3:28     ` Nicholas Piggin
  1 sibling, 0 replies; 13+ messages in thread
From: Nicholas Piggin @ 2021-04-14  3:28 UTC (permalink / raw)
  To: Fabiano Rosas, qemu-ppc; +Cc: Cédric Le Goater, qemu-devel, David Gibson

Excerpts from Fabiano Rosas's message of April 14, 2021 1:53 am:
> Nicholas Piggin <npiggin@gmail.com> writes:
> 
>> 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>
>> ---

[snip]

Thanks for the suggestions, I will consider them if people prefer to go 
this way rather than the "cleanup first" approach in the RFC I just 
posted.

Thanks,
Nick


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

end of thread, other threads:[~2021-04-14  3:29 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-13 12:54 [PATCH v1 0/3] POWER9/POWER10 exception model fixes Nicholas Piggin
2021-04-13 12:54 ` [PATCH v1 1/3] target/ppc: Fix POWER9 radix guest HV interrupt AIL behaviour Nicholas Piggin
2021-04-13 13:48   ` Fabiano Rosas
2021-04-14  3:26     ` Nicholas Piggin
2021-04-13 14:31   ` [EXTERNAL] " Cédric Le Goater
2021-04-13 12:54 ` [PATCH v1 2/3] target/ppc: POWER10 supports scv Nicholas Piggin
2021-04-13 14:18   ` [EXTERNAL] " Cédric Le Goater
2021-04-13 15:54   ` Fabiano Rosas
2021-04-13 12:54 ` [PATCH v1 3/3] target/ppc: Add POWER10 exception model Nicholas Piggin
2021-04-13 15:53   ` Fabiano Rosas
2021-04-13 17:09     ` Cédric Le Goater
2021-04-14  1:56       ` Nicholas Piggin
2021-04-14  3:28     ` Nicholas Piggin

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).