* [Qemu-devel] [PATCH v2] spapr: compute interrupt vector address from LPCR
@ 2016-03-30 15:38 Cédric Le Goater
2016-03-30 17:01 ` Greg Kurz
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Cédric Le Goater @ 2016-03-30 15:38 UTC (permalink / raw)
To: David Gibson
Cc: Thomas Huth, Cédric Le Goater, qemu-ppc, qemu-devel, Greg Kurz
This address is changed by the linux kernel using the H_SET_MODE hcall
and needs to be restored when migrating a spapr VM running in
TCG. This can be done using the AIL bits from the LPCR register.
The patch introduces a helper routine cpu_ppc_get_excp_prefix() which
returns the effective address offset of the interrupt handler
depending on the LPCR_AIL bits. The same helper can be used in the
H_SET_MODE hcall, which lets us remove the H_SET_MODE_ADDR_TRANS_*
defines.
Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
---
Changes since v1:
- moved helper routine under target-ppc/
- moved the restore of excp_prefix under cpu_post_load()
hw/ppc/spapr_hcall.c | 13 ++-----------
include/hw/ppc/spapr.h | 5 -----
target-ppc/cpu.h | 9 +++++++++
target-ppc/machine.c | 20 +++++++++++++++++++-
target-ppc/translate_init.c | 14 ++++++++++++++
5 files changed, 44 insertions(+), 17 deletions(-)
Index: qemu-dgibson-for-2.6.git/hw/ppc/spapr_hcall.c
===================================================================
--- qemu-dgibson-for-2.6.git.orig/hw/ppc/spapr_hcall.c
+++ qemu-dgibson-for-2.6.git/hw/ppc/spapr_hcall.c
@@ -835,17 +835,8 @@ static target_ulong h_set_mode_resource_
return H_P4;
}
- switch (mflags) {
- case H_SET_MODE_ADDR_TRANS_NONE:
- prefix = 0;
- break;
- case H_SET_MODE_ADDR_TRANS_0001_8000:
- prefix = 0x18000;
- break;
- case H_SET_MODE_ADDR_TRANS_C000_0000_0000_4000:
- prefix = 0xC000000000004000ULL;
- break;
- default:
+ prefix = cpu_ppc_get_excp_prefix(mflags);
+ if (prefix == (target_ulong) -1ULL) {
return H_UNSUPPORTED_FLAG;
}
Index: qemu-dgibson-for-2.6.git/target-ppc/machine.c
===================================================================
--- qemu-dgibson-for-2.6.git.orig/target-ppc/machine.c
+++ qemu-dgibson-for-2.6.git/target-ppc/machine.c
@@ -156,12 +156,26 @@ static void cpu_pre_save(void *opaque)
}
}
+
+static int cpu_post_load_excp_prefix(CPUPPCState *env)
+{
+ int ail = (env->spr[SPR_LPCR] & LPCR_AIL) >> LPCR_AIL_SHIFT;
+ target_ulong prefix = cpu_ppc_get_excp_prefix(ail);
+
+ if (prefix == (target_ulong) -1ULL) {
+ return -EINVAL;
+ }
+ env->excp_prefix = prefix;
+ return 0;
+}
+
static int cpu_post_load(void *opaque, int version_id)
{
PowerPCCPU *cpu = opaque;
CPUPPCState *env = &cpu->env;
int i;
target_ulong msr;
+ int ret = 0;
/*
* We always ignore the source PVR. The user or management
@@ -201,7 +215,11 @@ static int cpu_post_load(void *opaque, i
hreg_compute_mem_idx(env);
- return 0;
+ if (env->spr[SPR_LPCR] & LPCR_AIL) {
+ ret = cpu_post_load_excp_prefix(env);
+ }
+
+ return ret;
}
static bool fpu_needed(void *opaque)
Index: qemu-dgibson-for-2.6.git/target-ppc/translate_init.c
===================================================================
--- qemu-dgibson-for-2.6.git.orig/target-ppc/translate_init.c
+++ qemu-dgibson-for-2.6.git/target-ppc/translate_init.c
@@ -8522,6 +8522,20 @@ void cpu_ppc_set_papr(PowerPCCPU *cpu)
}
}
+target_ulong cpu_ppc_get_excp_prefix(target_ulong ail)
+{
+ switch (ail) {
+ case AIL_NONE:
+ return 0;
+ case AIL_0001_8000:
+ return 0x18000;
+ case AIL_C000_0000_0000_4000:
+ return 0xC000000000004000ULL;
+ default:
+ return (target_ulong) -1ULL;
+ }
+}
+
#endif /* !defined(CONFIG_USER_ONLY) */
#endif /* defined (TARGET_PPC64) */
Index: qemu-dgibson-for-2.6.git/target-ppc/cpu.h
===================================================================
--- qemu-dgibson-for-2.6.git.orig/target-ppc/cpu.h
+++ qemu-dgibson-for-2.6.git/target-ppc/cpu.h
@@ -1269,6 +1269,7 @@ void store_booke_tsr (CPUPPCState *env,
void ppc_tlb_invalidate_all (CPUPPCState *env);
void ppc_tlb_invalidate_one (CPUPPCState *env, target_ulong addr);
void cpu_ppc_set_papr(PowerPCCPU *cpu);
+target_ulong cpu_ppc_get_excp_prefix(target_ulong ail);
#endif
#endif
@@ -2277,6 +2278,14 @@ enum {
HMER_XSCOM_STATUS_LSH = (63 - 23),
};
+/* Alternate Interrupt Location (AIL) */
+enum {
+ AIL_NONE = 0,
+ AIL_RESERVED = 1,
+ AIL_0001_8000 = 2,
+ AIL_C000_0000_0000_4000 = 3,
+};
+
/*****************************************************************************/
static inline target_ulong cpu_read_xer(CPUPPCState *env)
Index: qemu-dgibson-for-2.6.git/include/hw/ppc/spapr.h
===================================================================
--- qemu-dgibson-for-2.6.git.orig/include/hw/ppc/spapr.h
+++ qemu-dgibson-for-2.6.git/include/hw/ppc/spapr.h
@@ -204,11 +204,6 @@ struct sPAPRMachineState {
#define H_SET_MODE_ENDIAN_BIG 0
#define H_SET_MODE_ENDIAN_LITTLE 1
-/* Flags for H_SET_MODE_RESOURCE_ADDR_TRANS_MODE */
-#define H_SET_MODE_ADDR_TRANS_NONE 0
-#define H_SET_MODE_ADDR_TRANS_0001_8000 2
-#define H_SET_MODE_ADDR_TRANS_C000_0000_0000_4000 3
-
/* VASI States */
#define H_VASI_INVALID 0
#define H_VASI_ENABLED 1
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v2] spapr: compute interrupt vector address from LPCR
2016-03-30 15:38 [Qemu-devel] [PATCH v2] spapr: compute interrupt vector address from LPCR Cédric Le Goater
@ 2016-03-30 17:01 ` Greg Kurz
2016-03-30 17:15 ` [Qemu-devel] [Qemu-ppc] " Greg Kurz
2016-03-30 17:12 ` [Qemu-devel] " Greg Kurz
2016-03-31 4:55 ` David Gibson
2 siblings, 1 reply; 15+ messages in thread
From: Greg Kurz @ 2016-03-30 17:01 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: Thomas Huth, qemu-ppc, qemu-devel, David Gibson
On Wed, 30 Mar 2016 17:38:34 +0200
Cédric Le Goater <clg@fr.ibm.com> wrote:
> This address is changed by the linux kernel using the H_SET_MODE hcall
> and needs to be restored when migrating a spapr VM running in
> TCG. This can be done using the AIL bits from the LPCR register.
>
> The patch introduces a helper routine cpu_ppc_get_excp_prefix() which
> returns the effective address offset of the interrupt handler
> depending on the LPCR_AIL bits. The same helper can be used in the
> H_SET_MODE hcall, which lets us remove the H_SET_MODE_ADDR_TRANS_*
> defines.
>
<clarification>
... because these H_SET_MODE_ADDR_TRANS_* defines, which refer to the values
0, 1, 2 and 3 for the mflag argument to the H_SET_MODE hypercall as described
in section 14.5.4.3.8 of PAPR, are actually values for bits 39:40 (AIL) of the
LPCR register, as described in section 2.2 of PowerISA.
</clarification>
> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
> ---
>
Reviewed-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
> Changes since v1:
>
> - moved helper routine under target-ppc/
> - moved the restore of excp_prefix under cpu_post_load()
>
> hw/ppc/spapr_hcall.c | 13 ++-----------
> include/hw/ppc/spapr.h | 5 -----
> target-ppc/cpu.h | 9 +++++++++
> target-ppc/machine.c | 20 +++++++++++++++++++-
> target-ppc/translate_init.c | 14 ++++++++++++++
> 5 files changed, 44 insertions(+), 17 deletions(-)
>
> Index: qemu-dgibson-for-2.6.git/hw/ppc/spapr_hcall.c
> ===================================================================
> --- qemu-dgibson-for-2.6.git.orig/hw/ppc/spapr_hcall.c
> +++ qemu-dgibson-for-2.6.git/hw/ppc/spapr_hcall.c
> @@ -835,17 +835,8 @@ static target_ulong h_set_mode_resource_
> return H_P4;
> }
>
> - switch (mflags) {
> - case H_SET_MODE_ADDR_TRANS_NONE:
> - prefix = 0;
> - break;
> - case H_SET_MODE_ADDR_TRANS_0001_8000:
> - prefix = 0x18000;
> - break;
> - case H_SET_MODE_ADDR_TRANS_C000_0000_0000_4000:
> - prefix = 0xC000000000004000ULL;
> - break;
> - default:
> + prefix = cpu_ppc_get_excp_prefix(mflags);
> + if (prefix == (target_ulong) -1ULL) {
> return H_UNSUPPORTED_FLAG;
> }
>
> Index: qemu-dgibson-for-2.6.git/target-ppc/machine.c
> ===================================================================
> --- qemu-dgibson-for-2.6.git.orig/target-ppc/machine.c
> +++ qemu-dgibson-for-2.6.git/target-ppc/machine.c
> @@ -156,12 +156,26 @@ static void cpu_pre_save(void *opaque)
> }
> }
>
> +
> +static int cpu_post_load_excp_prefix(CPUPPCState *env)
> +{
> + int ail = (env->spr[SPR_LPCR] & LPCR_AIL) >> LPCR_AIL_SHIFT;
> + target_ulong prefix = cpu_ppc_get_excp_prefix(ail);
> +
> + if (prefix == (target_ulong) -1ULL) {
> + return -EINVAL;
> + }
> + env->excp_prefix = prefix;
> + return 0;
> +}
> +
> static int cpu_post_load(void *opaque, int version_id)
> {
> PowerPCCPU *cpu = opaque;
> CPUPPCState *env = &cpu->env;
> int i;
> target_ulong msr;
> + int ret = 0;
>
> /*
> * We always ignore the source PVR. The user or management
> @@ -201,7 +215,11 @@ static int cpu_post_load(void *opaque, i
>
> hreg_compute_mem_idx(env);
>
> - return 0;
> + if (env->spr[SPR_LPCR] & LPCR_AIL) {
> + ret = cpu_post_load_excp_prefix(env);
> + }
> +
> + return ret;
> }
>
> static bool fpu_needed(void *opaque)
> Index: qemu-dgibson-for-2.6.git/target-ppc/translate_init.c
> ===================================================================
> --- qemu-dgibson-for-2.6.git.orig/target-ppc/translate_init.c
> +++ qemu-dgibson-for-2.6.git/target-ppc/translate_init.c
> @@ -8522,6 +8522,20 @@ void cpu_ppc_set_papr(PowerPCCPU *cpu)
> }
> }
>
> +target_ulong cpu_ppc_get_excp_prefix(target_ulong ail)
> +{
> + switch (ail) {
> + case AIL_NONE:
> + return 0;
> + case AIL_0001_8000:
> + return 0x18000;
> + case AIL_C000_0000_0000_4000:
> + return 0xC000000000004000ULL;
> + default:
> + return (target_ulong) -1ULL;
> + }
> +}
> +
> #endif /* !defined(CONFIG_USER_ONLY) */
>
> #endif /* defined (TARGET_PPC64) */
> Index: qemu-dgibson-for-2.6.git/target-ppc/cpu.h
> ===================================================================
> --- qemu-dgibson-for-2.6.git.orig/target-ppc/cpu.h
> +++ qemu-dgibson-for-2.6.git/target-ppc/cpu.h
> @@ -1269,6 +1269,7 @@ void store_booke_tsr (CPUPPCState *env,
> void ppc_tlb_invalidate_all (CPUPPCState *env);
> void ppc_tlb_invalidate_one (CPUPPCState *env, target_ulong addr);
> void cpu_ppc_set_papr(PowerPCCPU *cpu);
> +target_ulong cpu_ppc_get_excp_prefix(target_ulong ail);
> #endif
> #endif
>
> @@ -2277,6 +2278,14 @@ enum {
> HMER_XSCOM_STATUS_LSH = (63 - 23),
> };
>
> +/* Alternate Interrupt Location (AIL) */
> +enum {
> + AIL_NONE = 0,
> + AIL_RESERVED = 1,
> + AIL_0001_8000 = 2,
> + AIL_C000_0000_0000_4000 = 3,
> +};
> +
> /*****************************************************************************/
>
> static inline target_ulong cpu_read_xer(CPUPPCState *env)
> Index: qemu-dgibson-for-2.6.git/include/hw/ppc/spapr.h
> ===================================================================
> --- qemu-dgibson-for-2.6.git.orig/include/hw/ppc/spapr.h
> +++ qemu-dgibson-for-2.6.git/include/hw/ppc/spapr.h
> @@ -204,11 +204,6 @@ struct sPAPRMachineState {
> #define H_SET_MODE_ENDIAN_BIG 0
> #define H_SET_MODE_ENDIAN_LITTLE 1
>
> -/* Flags for H_SET_MODE_RESOURCE_ADDR_TRANS_MODE */
> -#define H_SET_MODE_ADDR_TRANS_NONE 0
> -#define H_SET_MODE_ADDR_TRANS_0001_8000 2
> -#define H_SET_MODE_ADDR_TRANS_C000_0000_0000_4000 3
> -
> /* VASI States */
> #define H_VASI_INVALID 0
> #define H_VASI_ENABLED 1
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v2] spapr: compute interrupt vector address from LPCR
2016-03-30 15:38 [Qemu-devel] [PATCH v2] spapr: compute interrupt vector address from LPCR Cédric Le Goater
2016-03-30 17:01 ` Greg Kurz
@ 2016-03-30 17:12 ` Greg Kurz
2016-03-31 8:20 ` Cédric Le Goater
2016-03-31 4:55 ` David Gibson
2 siblings, 1 reply; 15+ messages in thread
From: Greg Kurz @ 2016-03-30 17:12 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: Thomas Huth, qemu-ppc, qemu-devel, David Gibson
On Wed, 30 Mar 2016 17:38:34 +0200
Cédric Le Goater <clg@fr.ibm.com> wrote:
> This address is changed by the linux kernel using the H_SET_MODE hcall
> and needs to be restored when migrating a spapr VM running in
> TCG. This can be done using the AIL bits from the LPCR register.
>
> The patch introduces a helper routine cpu_ppc_get_excp_prefix() which
> returns the effective address offset of the interrupt handler
> depending on the LPCR_AIL bits. The same helper can be used in the
> H_SET_MODE hcall, which lets us remove the H_SET_MODE_ADDR_TRANS_*
> defines.
>
> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
> ---
>
Sorry I hit the send button too quickly... Just a nit but my Reviewed-by stands.
> Changes since v1:
>
> - moved helper routine under target-ppc/
> - moved the restore of excp_prefix under cpu_post_load()
>
> hw/ppc/spapr_hcall.c | 13 ++-----------
> include/hw/ppc/spapr.h | 5 -----
> target-ppc/cpu.h | 9 +++++++++
> target-ppc/machine.c | 20 +++++++++++++++++++-
> target-ppc/translate_init.c | 14 ++++++++++++++
> 5 files changed, 44 insertions(+), 17 deletions(-)
>
> Index: qemu-dgibson-for-2.6.git/hw/ppc/spapr_hcall.c
> ===================================================================
> --- qemu-dgibson-for-2.6.git.orig/hw/ppc/spapr_hcall.c
> +++ qemu-dgibson-for-2.6.git/hw/ppc/spapr_hcall.c
> @@ -835,17 +835,8 @@ static target_ulong h_set_mode_resource_
> return H_P4;
> }
>
> - switch (mflags) {
> - case H_SET_MODE_ADDR_TRANS_NONE:
> - prefix = 0;
> - break;
> - case H_SET_MODE_ADDR_TRANS_0001_8000:
> - prefix = 0x18000;
> - break;
> - case H_SET_MODE_ADDR_TRANS_C000_0000_0000_4000:
> - prefix = 0xC000000000004000ULL;
> - break;
> - default:
> + prefix = cpu_ppc_get_excp_prefix(mflags);
> + if (prefix == (target_ulong) -1ULL) {
+ if (prefix == (target_ulong) (-1ULL)) {
to make ./scripts/checkpatch.pl happy :)
> return H_UNSUPPORTED_FLAG;
> }
>
> Index: qemu-dgibson-for-2.6.git/target-ppc/machine.c
> ===================================================================
> --- qemu-dgibson-for-2.6.git.orig/target-ppc/machine.c
> +++ qemu-dgibson-for-2.6.git/target-ppc/machine.c
> @@ -156,12 +156,26 @@ static void cpu_pre_save(void *opaque)
> }
> }
>
> +
> +static int cpu_post_load_excp_prefix(CPUPPCState *env)
> +{
> + int ail = (env->spr[SPR_LPCR] & LPCR_AIL) >> LPCR_AIL_SHIFT;
> + target_ulong prefix = cpu_ppc_get_excp_prefix(ail);
> +
> + if (prefix == (target_ulong) -1ULL) {
> + return -EINVAL;
> + }
> + env->excp_prefix = prefix;
> + return 0;
> +}
> +
> static int cpu_post_load(void *opaque, int version_id)
> {
> PowerPCCPU *cpu = opaque;
> CPUPPCState *env = &cpu->env;
> int i;
> target_ulong msr;
> + int ret = 0;
>
> /*
> * We always ignore the source PVR. The user or management
> @@ -201,7 +215,11 @@ static int cpu_post_load(void *opaque, i
>
> hreg_compute_mem_idx(env);
>
> - return 0;
> + if (env->spr[SPR_LPCR] & LPCR_AIL) {
> + ret = cpu_post_load_excp_prefix(env);
> + }
> +
> + return ret;
> }
>
> static bool fpu_needed(void *opaque)
> Index: qemu-dgibson-for-2.6.git/target-ppc/translate_init.c
> ===================================================================
> --- qemu-dgibson-for-2.6.git.orig/target-ppc/translate_init.c
> +++ qemu-dgibson-for-2.6.git/target-ppc/translate_init.c
> @@ -8522,6 +8522,20 @@ void cpu_ppc_set_papr(PowerPCCPU *cpu)
> }
> }
>
> +target_ulong cpu_ppc_get_excp_prefix(target_ulong ail)
> +{
> + switch (ail) {
> + case AIL_NONE:
> + return 0;
> + case AIL_0001_8000:
> + return 0x18000;
> + case AIL_C000_0000_0000_4000:
> + return 0xC000000000004000ULL;
> + default:
> + return (target_ulong) -1ULL;
> + }
> +}
> +
> #endif /* !defined(CONFIG_USER_ONLY) */
>
> #endif /* defined (TARGET_PPC64) */
> Index: qemu-dgibson-for-2.6.git/target-ppc/cpu.h
> ===================================================================
> --- qemu-dgibson-for-2.6.git.orig/target-ppc/cpu.h
> +++ qemu-dgibson-for-2.6.git/target-ppc/cpu.h
> @@ -1269,6 +1269,7 @@ void store_booke_tsr (CPUPPCState *env,
> void ppc_tlb_invalidate_all (CPUPPCState *env);
> void ppc_tlb_invalidate_one (CPUPPCState *env, target_ulong addr);
> void cpu_ppc_set_papr(PowerPCCPU *cpu);
> +target_ulong cpu_ppc_get_excp_prefix(target_ulong ail);
> #endif
> #endif
>
> @@ -2277,6 +2278,14 @@ enum {
> HMER_XSCOM_STATUS_LSH = (63 - 23),
> };
>
> +/* Alternate Interrupt Location (AIL) */
> +enum {
> + AIL_NONE = 0,
> + AIL_RESERVED = 1,
> + AIL_0001_8000 = 2,
> + AIL_C000_0000_0000_4000 = 3,
> +};
> +
> /*****************************************************************************/
>
> static inline target_ulong cpu_read_xer(CPUPPCState *env)
> Index: qemu-dgibson-for-2.6.git/include/hw/ppc/spapr.h
> ===================================================================
> --- qemu-dgibson-for-2.6.git.orig/include/hw/ppc/spapr.h
> +++ qemu-dgibson-for-2.6.git/include/hw/ppc/spapr.h
> @@ -204,11 +204,6 @@ struct sPAPRMachineState {
> #define H_SET_MODE_ENDIAN_BIG 0
> #define H_SET_MODE_ENDIAN_LITTLE 1
>
> -/* Flags for H_SET_MODE_RESOURCE_ADDR_TRANS_MODE */
> -#define H_SET_MODE_ADDR_TRANS_NONE 0
> -#define H_SET_MODE_ADDR_TRANS_0001_8000 2
> -#define H_SET_MODE_ADDR_TRANS_C000_0000_0000_4000 3
> -
> /* VASI States */
> #define H_VASI_INVALID 0
> #define H_VASI_ENABLED 1
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [PATCH v2] spapr: compute interrupt vector address from LPCR
2016-03-30 17:01 ` Greg Kurz
@ 2016-03-30 17:15 ` Greg Kurz
0 siblings, 0 replies; 15+ messages in thread
From: Greg Kurz @ 2016-03-30 17:15 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: Thomas Huth, qemu-ppc, qemu-devel, David Gibson
On Wed, 30 Mar 2016 19:01:31 +0200
Greg Kurz <gkurz@linux.vnet.ibm.com> wrote:
> On Wed, 30 Mar 2016 17:38:34 +0200
> Cédric Le Goater <clg@fr.ibm.com> wrote:
>
> > This address is changed by the linux kernel using the H_SET_MODE hcall
> > and needs to be restored when migrating a spapr VM running in
> > TCG. This can be done using the AIL bits from the LPCR register.
> >
> > The patch introduces a helper routine cpu_ppc_get_excp_prefix() which
> > returns the effective address offset of the interrupt handler
> > depending on the LPCR_AIL bits. The same helper can be used in the
> > H_SET_MODE hcall, which lets us remove the H_SET_MODE_ADDR_TRANS_*
> > defines.
> >
>
> <clarification>
> ... because these H_SET_MODE_ADDR_TRANS_* defines, which refer to the values
> 0, 1, 2 and 3 for the mflag argument to the H_SET_MODE hypercall as described
> in section 14.5.4.3.8 of PAPR, are actually values for bits 39:40 (AIL) of the
> LPCR register, as described in section 2.2 of PowerISA.
read "section section 2.2 of chapter 2 of book-III S in PowerISA" :)
> </clarification>
>
> > Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
> > ---
> >
>
> Reviewed-by: Greg Kurz <gkurz@linux.vnet.ibm.com>
>
> > Changes since v1:
> >
> > - moved helper routine under target-ppc/
> > - moved the restore of excp_prefix under cpu_post_load()
> >
> > hw/ppc/spapr_hcall.c | 13 ++-----------
> > include/hw/ppc/spapr.h | 5 -----
> > target-ppc/cpu.h | 9 +++++++++
> > target-ppc/machine.c | 20 +++++++++++++++++++-
> > target-ppc/translate_init.c | 14 ++++++++++++++
> > 5 files changed, 44 insertions(+), 17 deletions(-)
> >
> > Index: qemu-dgibson-for-2.6.git/hw/ppc/spapr_hcall.c
> > ===================================================================
> > --- qemu-dgibson-for-2.6.git.orig/hw/ppc/spapr_hcall.c
> > +++ qemu-dgibson-for-2.6.git/hw/ppc/spapr_hcall.c
> > @@ -835,17 +835,8 @@ static target_ulong h_set_mode_resource_
> > return H_P4;
> > }
> >
> > - switch (mflags) {
> > - case H_SET_MODE_ADDR_TRANS_NONE:
> > - prefix = 0;
> > - break;
> > - case H_SET_MODE_ADDR_TRANS_0001_8000:
> > - prefix = 0x18000;
> > - break;
> > - case H_SET_MODE_ADDR_TRANS_C000_0000_0000_4000:
> > - prefix = 0xC000000000004000ULL;
> > - break;
> > - default:
> > + prefix = cpu_ppc_get_excp_prefix(mflags);
> > + if (prefix == (target_ulong) -1ULL) {
> > return H_UNSUPPORTED_FLAG;
> > }
> >
> > Index: qemu-dgibson-for-2.6.git/target-ppc/machine.c
> > ===================================================================
> > --- qemu-dgibson-for-2.6.git.orig/target-ppc/machine.c
> > +++ qemu-dgibson-for-2.6.git/target-ppc/machine.c
> > @@ -156,12 +156,26 @@ static void cpu_pre_save(void *opaque)
> > }
> > }
> >
> > +
> > +static int cpu_post_load_excp_prefix(CPUPPCState *env)
> > +{
> > + int ail = (env->spr[SPR_LPCR] & LPCR_AIL) >> LPCR_AIL_SHIFT;
> > + target_ulong prefix = cpu_ppc_get_excp_prefix(ail);
> > +
> > + if (prefix == (target_ulong) -1ULL) {
> > + return -EINVAL;
> > + }
> > + env->excp_prefix = prefix;
> > + return 0;
> > +}
> > +
> > static int cpu_post_load(void *opaque, int version_id)
> > {
> > PowerPCCPU *cpu = opaque;
> > CPUPPCState *env = &cpu->env;
> > int i;
> > target_ulong msr;
> > + int ret = 0;
> >
> > /*
> > * We always ignore the source PVR. The user or management
> > @@ -201,7 +215,11 @@ static int cpu_post_load(void *opaque, i
> >
> > hreg_compute_mem_idx(env);
> >
> > - return 0;
> > + if (env->spr[SPR_LPCR] & LPCR_AIL) {
> > + ret = cpu_post_load_excp_prefix(env);
> > + }
> > +
> > + return ret;
> > }
> >
> > static bool fpu_needed(void *opaque)
> > Index: qemu-dgibson-for-2.6.git/target-ppc/translate_init.c
> > ===================================================================
> > --- qemu-dgibson-for-2.6.git.orig/target-ppc/translate_init.c
> > +++ qemu-dgibson-for-2.6.git/target-ppc/translate_init.c
> > @@ -8522,6 +8522,20 @@ void cpu_ppc_set_papr(PowerPCCPU *cpu)
> > }
> > }
> >
> > +target_ulong cpu_ppc_get_excp_prefix(target_ulong ail)
> > +{
> > + switch (ail) {
> > + case AIL_NONE:
> > + return 0;
> > + case AIL_0001_8000:
> > + return 0x18000;
> > + case AIL_C000_0000_0000_4000:
> > + return 0xC000000000004000ULL;
> > + default:
> > + return (target_ulong) -1ULL;
> > + }
> > +}
> > +
> > #endif /* !defined(CONFIG_USER_ONLY) */
> >
> > #endif /* defined (TARGET_PPC64) */
> > Index: qemu-dgibson-for-2.6.git/target-ppc/cpu.h
> > ===================================================================
> > --- qemu-dgibson-for-2.6.git.orig/target-ppc/cpu.h
> > +++ qemu-dgibson-for-2.6.git/target-ppc/cpu.h
> > @@ -1269,6 +1269,7 @@ void store_booke_tsr (CPUPPCState *env,
> > void ppc_tlb_invalidate_all (CPUPPCState *env);
> > void ppc_tlb_invalidate_one (CPUPPCState *env, target_ulong addr);
> > void cpu_ppc_set_papr(PowerPCCPU *cpu);
> > +target_ulong cpu_ppc_get_excp_prefix(target_ulong ail);
> > #endif
> > #endif
> >
> > @@ -2277,6 +2278,14 @@ enum {
> > HMER_XSCOM_STATUS_LSH = (63 - 23),
> > };
> >
> > +/* Alternate Interrupt Location (AIL) */
> > +enum {
> > + AIL_NONE = 0,
> > + AIL_RESERVED = 1,
> > + AIL_0001_8000 = 2,
> > + AIL_C000_0000_0000_4000 = 3,
> > +};
> > +
> > /*****************************************************************************/
> >
> > static inline target_ulong cpu_read_xer(CPUPPCState *env)
> > Index: qemu-dgibson-for-2.6.git/include/hw/ppc/spapr.h
> > ===================================================================
> > --- qemu-dgibson-for-2.6.git.orig/include/hw/ppc/spapr.h
> > +++ qemu-dgibson-for-2.6.git/include/hw/ppc/spapr.h
> > @@ -204,11 +204,6 @@ struct sPAPRMachineState {
> > #define H_SET_MODE_ENDIAN_BIG 0
> > #define H_SET_MODE_ENDIAN_LITTLE 1
> >
> > -/* Flags for H_SET_MODE_RESOURCE_ADDR_TRANS_MODE */
> > -#define H_SET_MODE_ADDR_TRANS_NONE 0
> > -#define H_SET_MODE_ADDR_TRANS_0001_8000 2
> > -#define H_SET_MODE_ADDR_TRANS_C000_0000_0000_4000 3
> > -
> > /* VASI States */
> > #define H_VASI_INVALID 0
> > #define H_VASI_ENABLED 1
> >
> >
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v2] spapr: compute interrupt vector address from LPCR
2016-03-30 15:38 [Qemu-devel] [PATCH v2] spapr: compute interrupt vector address from LPCR Cédric Le Goater
2016-03-30 17:01 ` Greg Kurz
2016-03-30 17:12 ` [Qemu-devel] " Greg Kurz
@ 2016-03-31 4:55 ` David Gibson
2016-03-31 7:13 ` [Qemu-devel] [Qemu-ppc] " Mark Cave-Ayland
` (2 more replies)
2 siblings, 3 replies; 15+ messages in thread
From: David Gibson @ 2016-03-31 4:55 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: Thomas Huth, qemu-ppc, qemu-devel, Greg Kurz
[-- Attachment #1: Type: text/plain, Size: 6242 bytes --]
On Wed, Mar 30, 2016 at 05:38:34PM +0200, Cédric Le Goater wrote:
> This address is changed by the linux kernel using the H_SET_MODE hcall
> and needs to be restored when migrating a spapr VM running in
> TCG. This can be done using the AIL bits from the LPCR register.
>
> The patch introduces a helper routine cpu_ppc_get_excp_prefix() which
> returns the effective address offset of the interrupt handler
> depending on the LPCR_AIL bits. The same helper can be used in the
> H_SET_MODE hcall, which lets us remove the H_SET_MODE_ADDR_TRANS_*
> defines.
>
> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
I've applied this (with Greg's minor amendments) to ppc-for-2.6),
since it certainly improves behaviour, although I have a couple of
queries:
> ---
>
> Changes since v1:
>
> - moved helper routine under target-ppc/
> - moved the restore of excp_prefix under cpu_post_load()
>
> hw/ppc/spapr_hcall.c | 13 ++-----------
> include/hw/ppc/spapr.h | 5 -----
> target-ppc/cpu.h | 9 +++++++++
> target-ppc/machine.c | 20 +++++++++++++++++++-
> target-ppc/translate_init.c | 14 ++++++++++++++
> 5 files changed, 44 insertions(+), 17 deletions(-)
>
> Index: qemu-dgibson-for-2.6.git/hw/ppc/spapr_hcall.c
> ===================================================================
> --- qemu-dgibson-for-2.6.git.orig/hw/ppc/spapr_hcall.c
> +++ qemu-dgibson-for-2.6.git/hw/ppc/spapr_hcall.c
> @@ -835,17 +835,8 @@ static target_ulong h_set_mode_resource_
> return H_P4;
> }
>
> - switch (mflags) {
> - case H_SET_MODE_ADDR_TRANS_NONE:
> - prefix = 0;
> - break;
> - case H_SET_MODE_ADDR_TRANS_0001_8000:
> - prefix = 0x18000;
> - break;
> - case H_SET_MODE_ADDR_TRANS_C000_0000_0000_4000:
> - prefix = 0xC000000000004000ULL;
> - break;
> - default:
> + prefix = cpu_ppc_get_excp_prefix(mflags);
> + if (prefix == (target_ulong) -1ULL) {
> return H_UNSUPPORTED_FLAG;
> }
>
> Index: qemu-dgibson-for-2.6.git/target-ppc/machine.c
> ===================================================================
> --- qemu-dgibson-for-2.6.git.orig/target-ppc/machine.c
> +++ qemu-dgibson-for-2.6.git/target-ppc/machine.c
> @@ -156,12 +156,26 @@ static void cpu_pre_save(void *opaque)
> }
> }
>
> +
> +static int cpu_post_load_excp_prefix(CPUPPCState *env)
> +{
> + int ail = (env->spr[SPR_LPCR] & LPCR_AIL) >> LPCR_AIL_SHIFT;
> + target_ulong prefix = cpu_ppc_get_excp_prefix(ail);
> +
> + if (prefix == (target_ulong) -1ULL) {
> + return -EINVAL;
> + }
> + env->excp_prefix = prefix;
> + return 0;
> +}
> +
> static int cpu_post_load(void *opaque, int version_id)
> {
> PowerPCCPU *cpu = opaque;
> CPUPPCState *env = &cpu->env;
> int i;
> target_ulong msr;
> + int ret = 0;
>
> /*
> * We always ignore the source PVR. The user or management
> @@ -201,7 +215,11 @@ static int cpu_post_load(void *opaque, i
>
> hreg_compute_mem_idx(env);
>
> - return 0;
> + if (env->spr[SPR_LPCR] & LPCR_AIL) {
> + ret = cpu_post_load_excp_prefix(env);
> + }
Why not call this unconditionally? If AIL == 0 it will still do the
right thing.
Aren't there also circumstances where the exception prefix can depend
on the MSR? Do those need to be handled somewhere?
> +
> + return ret;
> }
>
> static bool fpu_needed(void *opaque)
> Index: qemu-dgibson-for-2.6.git/target-ppc/translate_init.c
> ===================================================================
> --- qemu-dgibson-for-2.6.git.orig/target-ppc/translate_init.c
> +++ qemu-dgibson-for-2.6.git/target-ppc/translate_init.c
> @@ -8522,6 +8522,20 @@ void cpu_ppc_set_papr(PowerPCCPU *cpu)
> }
> }
>
> +target_ulong cpu_ppc_get_excp_prefix(target_ulong ail)
> +{
> + switch (ail) {
> + case AIL_NONE:
> + return 0;
> + case AIL_0001_8000:
> + return 0x18000;
> + case AIL_C000_0000_0000_4000:
> + return 0xC000000000004000ULL;
> + default:
> + return (target_ulong) -1ULL;
> + }
> +}
> +
> #endif /* !defined(CONFIG_USER_ONLY) */
>
> #endif /* defined (TARGET_PPC64) */
> Index: qemu-dgibson-for-2.6.git/target-ppc/cpu.h
> ===================================================================
> --- qemu-dgibson-for-2.6.git.orig/target-ppc/cpu.h
> +++ qemu-dgibson-for-2.6.git/target-ppc/cpu.h
> @@ -1269,6 +1269,7 @@ void store_booke_tsr (CPUPPCState *env,
> void ppc_tlb_invalidate_all (CPUPPCState *env);
> void ppc_tlb_invalidate_one (CPUPPCState *env, target_ulong addr);
> void cpu_ppc_set_papr(PowerPCCPU *cpu);
> +target_ulong cpu_ppc_get_excp_prefix(target_ulong ail);
> #endif
> #endif
>
> @@ -2277,6 +2278,14 @@ enum {
> HMER_XSCOM_STATUS_LSH = (63 - 23),
> };
>
> +/* Alternate Interrupt Location (AIL) */
> +enum {
> + AIL_NONE = 0,
> + AIL_RESERVED = 1,
> + AIL_0001_8000 = 2,
> + AIL_C000_0000_0000_4000 = 3,
> +};
> +
> /*****************************************************************************/
>
> static inline target_ulong cpu_read_xer(CPUPPCState *env)
> Index: qemu-dgibson-for-2.6.git/include/hw/ppc/spapr.h
> ===================================================================
> --- qemu-dgibson-for-2.6.git.orig/include/hw/ppc/spapr.h
> +++ qemu-dgibson-for-2.6.git/include/hw/ppc/spapr.h
> @@ -204,11 +204,6 @@ struct sPAPRMachineState {
> #define H_SET_MODE_ENDIAN_BIG 0
> #define H_SET_MODE_ENDIAN_LITTLE 1
>
> -/* Flags for H_SET_MODE_RESOURCE_ADDR_TRANS_MODE */
> -#define H_SET_MODE_ADDR_TRANS_NONE 0
> -#define H_SET_MODE_ADDR_TRANS_0001_8000 2
> -#define H_SET_MODE_ADDR_TRANS_C000_0000_0000_4000 3
> -
> /* VASI States */
> #define H_VASI_INVALID 0
> #define H_VASI_ENABLED 1
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [PATCH v2] spapr: compute interrupt vector address from LPCR
2016-03-31 4:55 ` David Gibson
@ 2016-03-31 7:13 ` Mark Cave-Ayland
2016-04-01 2:43 ` David Gibson
2016-03-31 8:16 ` [Qemu-devel] " Cédric Le Goater
2016-04-01 3:34 ` [Qemu-devel] [Qemu-ppc] " David Gibson
2 siblings, 1 reply; 15+ messages in thread
From: Mark Cave-Ayland @ 2016-03-31 7:13 UTC (permalink / raw)
To: David Gibson, Cédric Le Goater
Cc: Thomas Huth, qemu-ppc, qemu-devel, Greg Kurz
On 31/03/16 05:55, David Gibson wrote:
> On Wed, Mar 30, 2016 at 05:38:34PM +0200, Cédric Le Goater wrote:
>> This address is changed by the linux kernel using the H_SET_MODE hcall
>> and needs to be restored when migrating a spapr VM running in
>> TCG. This can be done using the AIL bits from the LPCR register.
>>
>> The patch introduces a helper routine cpu_ppc_get_excp_prefix() which
>> returns the effective address offset of the interrupt handler
>> depending on the LPCR_AIL bits. The same helper can be used in the
>> H_SET_MODE hcall, which lets us remove the H_SET_MODE_ADDR_TRANS_*
>> defines.
>>
>> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
>
> I've applied this (with Greg's minor amendments) to ppc-for-2.6),
> since it certainly improves behaviour, although I have a couple of
> queries:
>
>> ---
>>
>> Changes since v1:
>>
>> - moved helper routine under target-ppc/
>> - moved the restore of excp_prefix under cpu_post_load()
>>
>> hw/ppc/spapr_hcall.c | 13 ++-----------
>> include/hw/ppc/spapr.h | 5 -----
>> target-ppc/cpu.h | 9 +++++++++
>> target-ppc/machine.c | 20 +++++++++++++++++++-
>> target-ppc/translate_init.c | 14 ++++++++++++++
>> 5 files changed, 44 insertions(+), 17 deletions(-)
>>
>> Index: qemu-dgibson-for-2.6.git/hw/ppc/spapr_hcall.c
>> ===================================================================
>> --- qemu-dgibson-for-2.6.git.orig/hw/ppc/spapr_hcall.c
>> +++ qemu-dgibson-for-2.6.git/hw/ppc/spapr_hcall.c
>> @@ -835,17 +835,8 @@ static target_ulong h_set_mode_resource_
>> return H_P4;
>> }
>>
>> - switch (mflags) {
>> - case H_SET_MODE_ADDR_TRANS_NONE:
>> - prefix = 0;
>> - break;
>> - case H_SET_MODE_ADDR_TRANS_0001_8000:
>> - prefix = 0x18000;
>> - break;
>> - case H_SET_MODE_ADDR_TRANS_C000_0000_0000_4000:
>> - prefix = 0xC000000000004000ULL;
>> - break;
>> - default:
>> + prefix = cpu_ppc_get_excp_prefix(mflags);
>> + if (prefix == (target_ulong) -1ULL) {
>> return H_UNSUPPORTED_FLAG;
>> }
>>
>> Index: qemu-dgibson-for-2.6.git/target-ppc/machine.c
>> ===================================================================
>> --- qemu-dgibson-for-2.6.git.orig/target-ppc/machine.c
>> +++ qemu-dgibson-for-2.6.git/target-ppc/machine.c
>> @@ -156,12 +156,26 @@ static void cpu_pre_save(void *opaque)
>> }
>> }
>>
>> +
>> +static int cpu_post_load_excp_prefix(CPUPPCState *env)
>> +{
>> + int ail = (env->spr[SPR_LPCR] & LPCR_AIL) >> LPCR_AIL_SHIFT;
>> + target_ulong prefix = cpu_ppc_get_excp_prefix(ail);
>> +
>> + if (prefix == (target_ulong) -1ULL) {
>> + return -EINVAL;
>> + }
>> + env->excp_prefix = prefix;
>> + return 0;
>> +}
>> +
>> static int cpu_post_load(void *opaque, int version_id)
>> {
>> PowerPCCPU *cpu = opaque;
>> CPUPPCState *env = &cpu->env;
>> int i;
>> target_ulong msr;
>> + int ret = 0;
>>
>> /*
>> * We always ignore the source PVR. The user or management
>> @@ -201,7 +215,11 @@ static int cpu_post_load(void *opaque, i
>>
>> hreg_compute_mem_idx(env);
>>
>> - return 0;
>> + if (env->spr[SPR_LPCR] & LPCR_AIL) {
>> + ret = cpu_post_load_excp_prefix(env);
>> + }
>
> Why not call this unconditionally? If AIL == 0 it will still do the
> right thing.
>
> Aren't there also circumstances where the exception prefix can depend
> on the MSR? Do those need to be handled somewhere?
Yes indeed - this was part of my patchset last year to fix up various
migration issues for the Mac PPC machines (see commit
2360b6e84f78d41fa0f76555a947148b73645259).
I agree that having the env->excp_prefix logic split like this isn't a
particularly great idea. Let's just have a single helper function as in
the patch above and use that in both places (and in fact with recent
changes I have a feeling env->excp_prefix is one of the few remaining
reasons that the above change is still required, but please check this).
ATB,
Mark.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v2] spapr: compute interrupt vector address from LPCR
2016-03-31 4:55 ` David Gibson
2016-03-31 7:13 ` [Qemu-devel] [Qemu-ppc] " Mark Cave-Ayland
@ 2016-03-31 8:16 ` Cédric Le Goater
2016-04-01 3:34 ` [Qemu-devel] [Qemu-ppc] " David Gibson
2 siblings, 0 replies; 15+ messages in thread
From: Cédric Le Goater @ 2016-03-31 8:16 UTC (permalink / raw)
To: David Gibson; +Cc: Thomas Huth, qemu-ppc, qemu-devel, Greg Kurz
On 03/31/2016 06:55 AM, David Gibson wrote:
> On Wed, Mar 30, 2016 at 05:38:34PM +0200, Cédric Le Goater wrote:
>> This address is changed by the linux kernel using the H_SET_MODE hcall
>> and needs to be restored when migrating a spapr VM running in
>> TCG. This can be done using the AIL bits from the LPCR register.
>>
>> The patch introduces a helper routine cpu_ppc_get_excp_prefix() which
>> returns the effective address offset of the interrupt handler
>> depending on the LPCR_AIL bits. The same helper can be used in the
>> H_SET_MODE hcall, which lets us remove the H_SET_MODE_ADDR_TRANS_*
>> defines.
>>
>> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
>
> I've applied this (with Greg's minor amendments) to ppc-for-2.6),
> since it certainly improves behaviour, although I have a couple of
> queries:
>
>> ---
>>
>> Changes since v1:
>>
>> - moved helper routine under target-ppc/
>> - moved the restore of excp_prefix under cpu_post_load()
>>
>> hw/ppc/spapr_hcall.c | 13 ++-----------
>> include/hw/ppc/spapr.h | 5 -----
>> target-ppc/cpu.h | 9 +++++++++
>> target-ppc/machine.c | 20 +++++++++++++++++++-
>> target-ppc/translate_init.c | 14 ++++++++++++++
>> 5 files changed, 44 insertions(+), 17 deletions(-)
>>
>> Index: qemu-dgibson-for-2.6.git/hw/ppc/spapr_hcall.c
>> ===================================================================
>> --- qemu-dgibson-for-2.6.git.orig/hw/ppc/spapr_hcall.c
>> +++ qemu-dgibson-for-2.6.git/hw/ppc/spapr_hcall.c
>> @@ -835,17 +835,8 @@ static target_ulong h_set_mode_resource_
>> return H_P4;
>> }
>>
>> - switch (mflags) {
>> - case H_SET_MODE_ADDR_TRANS_NONE:
>> - prefix = 0;
>> - break;
>> - case H_SET_MODE_ADDR_TRANS_0001_8000:
>> - prefix = 0x18000;
>> - break;
>> - case H_SET_MODE_ADDR_TRANS_C000_0000_0000_4000:
>> - prefix = 0xC000000000004000ULL;
>> - break;
>> - default:
>> + prefix = cpu_ppc_get_excp_prefix(mflags);
>> + if (prefix == (target_ulong) -1ULL) {
>> return H_UNSUPPORTED_FLAG;
>> }
>>
>> Index: qemu-dgibson-for-2.6.git/target-ppc/machine.c
>> ===================================================================
>> --- qemu-dgibson-for-2.6.git.orig/target-ppc/machine.c
>> +++ qemu-dgibson-for-2.6.git/target-ppc/machine.c
>> @@ -156,12 +156,26 @@ static void cpu_pre_save(void *opaque)
>> }
>> }
>>
>> +
>> +static int cpu_post_load_excp_prefix(CPUPPCState *env)
>> +{
>> + int ail = (env->spr[SPR_LPCR] & LPCR_AIL) >> LPCR_AIL_SHIFT;
>> + target_ulong prefix = cpu_ppc_get_excp_prefix(ail);
>> +
>> + if (prefix == (target_ulong) -1ULL) {
>> + return -EINVAL;
>> + }
>> + env->excp_prefix = prefix;
>> + return 0;
>> +}
>> +
>> static int cpu_post_load(void *opaque, int version_id)
>> {
>> PowerPCCPU *cpu = opaque;
>> CPUPPCState *env = &cpu->env;
>> int i;
>> target_ulong msr;
>> + int ret = 0;
>>
>> /*
>> * We always ignore the source PVR. The user or management
>> @@ -201,7 +215,11 @@ static int cpu_post_load(void *opaque, i
>>
>> hreg_compute_mem_idx(env);
>>
>> - return 0;
>> + if (env->spr[SPR_LPCR] & LPCR_AIL) {
>> + ret = cpu_post_load_excp_prefix(env);
>> + }
>
> Why not call this unconditionally? If AIL == 0 it will still do the
> right thing.
yes, indeed. the test is not required.
> Aren't there also circumstances where the exception prefix can depend
> on the MSR? Do those need to be handled somewhere?
Well, if translation is not enabled, LPCR_AIL should be 0, and so should
be excp_prefix. Is that something we should check/enforce at restart ?
Looking closer, we could probably get rid of excp_prefix (for spapr) and
compute the value directly from LPCR in powerpc_excp() where we really need
it.
C.
>> +
>> + return ret;
>> }
>>
>> static bool fpu_needed(void *opaque)
>> Index: qemu-dgibson-for-2.6.git/target-ppc/translate_init.c
>> ===================================================================
>> --- qemu-dgibson-for-2.6.git.orig/target-ppc/translate_init.c
>> +++ qemu-dgibson-for-2.6.git/target-ppc/translate_init.c
>> @@ -8522,6 +8522,20 @@ void cpu_ppc_set_papr(PowerPCCPU *cpu)
>> }
>> }
>>
>> +target_ulong cpu_ppc_get_excp_prefix(target_ulong ail)
>> +{
>> + switch (ail) {
>> + case AIL_NONE:
>> + return 0;
>> + case AIL_0001_8000:
>> + return 0x18000;
>> + case AIL_C000_0000_0000_4000:
>> + return 0xC000000000004000ULL;
>> + default:
>> + return (target_ulong) -1ULL;
>> + }
>> +}
>> +
>> #endif /* !defined(CONFIG_USER_ONLY) */
>>
>> #endif /* defined (TARGET_PPC64) */
>> Index: qemu-dgibson-for-2.6.git/target-ppc/cpu.h
>> ===================================================================
>> --- qemu-dgibson-for-2.6.git.orig/target-ppc/cpu.h
>> +++ qemu-dgibson-for-2.6.git/target-ppc/cpu.h
>> @@ -1269,6 +1269,7 @@ void store_booke_tsr (CPUPPCState *env,
>> void ppc_tlb_invalidate_all (CPUPPCState *env);
>> void ppc_tlb_invalidate_one (CPUPPCState *env, target_ulong addr);
>> void cpu_ppc_set_papr(PowerPCCPU *cpu);
>> +target_ulong cpu_ppc_get_excp_prefix(target_ulong ail);
>> #endif
>> #endif
>>
>> @@ -2277,6 +2278,14 @@ enum {
>> HMER_XSCOM_STATUS_LSH = (63 - 23),
>> };
>>
>> +/* Alternate Interrupt Location (AIL) */
>> +enum {
>> + AIL_NONE = 0,
>> + AIL_RESERVED = 1,
>> + AIL_0001_8000 = 2,
>> + AIL_C000_0000_0000_4000 = 3,
>> +};
>> +
>> /*****************************************************************************/
>>
>> static inline target_ulong cpu_read_xer(CPUPPCState *env)
>> Index: qemu-dgibson-for-2.6.git/include/hw/ppc/spapr.h
>> ===================================================================
>> --- qemu-dgibson-for-2.6.git.orig/include/hw/ppc/spapr.h
>> +++ qemu-dgibson-for-2.6.git/include/hw/ppc/spapr.h
>> @@ -204,11 +204,6 @@ struct sPAPRMachineState {
>> #define H_SET_MODE_ENDIAN_BIG 0
>> #define H_SET_MODE_ENDIAN_LITTLE 1
>>
>> -/* Flags for H_SET_MODE_RESOURCE_ADDR_TRANS_MODE */
>> -#define H_SET_MODE_ADDR_TRANS_NONE 0
>> -#define H_SET_MODE_ADDR_TRANS_0001_8000 2
>> -#define H_SET_MODE_ADDR_TRANS_C000_0000_0000_4000 3
>> -
>> /* VASI States */
>> #define H_VASI_INVALID 0
>> #define H_VASI_ENABLED 1
>>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v2] spapr: compute interrupt vector address from LPCR
2016-03-30 17:12 ` [Qemu-devel] " Greg Kurz
@ 2016-03-31 8:20 ` Cédric Le Goater
2016-03-31 9:33 ` Cédric Le Goater
0 siblings, 1 reply; 15+ messages in thread
From: Cédric Le Goater @ 2016-03-31 8:20 UTC (permalink / raw)
To: Greg Kurz; +Cc: Thomas Huth, qemu-ppc, qemu-devel, David Gibson
On 03/30/2016 07:12 PM, Greg Kurz wrote:
> On Wed, 30 Mar 2016 17:38:34 +0200
> Cédric Le Goater <clg@fr.ibm.com> wrote:
>
>> This address is changed by the linux kernel using the H_SET_MODE hcall
>> and needs to be restored when migrating a spapr VM running in
>> TCG. This can be done using the AIL bits from the LPCR register.
>>
>> The patch introduces a helper routine cpu_ppc_get_excp_prefix() which
>> returns the effective address offset of the interrupt handler
>> depending on the LPCR_AIL bits. The same helper can be used in the
>> H_SET_MODE hcall, which lets us remove the H_SET_MODE_ADDR_TRANS_*
>> defines.
>>
>> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
>> ---
>>
>
> Sorry I hit the send button too quickly... Just a nit but my Reviewed-by stands.
>
>> Changes since v1:
>>
>> - moved helper routine under target-ppc/
>> - moved the restore of excp_prefix under cpu_post_load()
>>
>> hw/ppc/spapr_hcall.c | 13 ++-----------
>> include/hw/ppc/spapr.h | 5 -----
>> target-ppc/cpu.h | 9 +++++++++
>> target-ppc/machine.c | 20 +++++++++++++++++++-
>> target-ppc/translate_init.c | 14 ++++++++++++++
>> 5 files changed, 44 insertions(+), 17 deletions(-)
>>
>> Index: qemu-dgibson-for-2.6.git/hw/ppc/spapr_hcall.c
>> ===================================================================
>> --- qemu-dgibson-for-2.6.git.orig/hw/ppc/spapr_hcall.c
>> +++ qemu-dgibson-for-2.6.git/hw/ppc/spapr_hcall.c
>> @@ -835,17 +835,8 @@ static target_ulong h_set_mode_resource_
>> return H_P4;
>> }
>>
>> - switch (mflags) {
>> - case H_SET_MODE_ADDR_TRANS_NONE:
>> - prefix = 0;
>> - break;
>> - case H_SET_MODE_ADDR_TRANS_0001_8000:
>> - prefix = 0x18000;
>> - break;
>> - case H_SET_MODE_ADDR_TRANS_C000_0000_0000_4000:
>> - prefix = 0xC000000000004000ULL;
>> - break;
>> - default:
>> + prefix = cpu_ppc_get_excp_prefix(mflags);
>> + if (prefix == (target_ulong) -1ULL) {
>
> + if (prefix == (target_ulong) (-1ULL)) {
>
> to make ./scripts/checkpatch.pl happy :)
yes.
The funny thing is that checkpatch.pl does not complain for the exact
same line below. Looks like a checkpatch.pl bug to me.
C.
>
>> return H_UNSUPPORTED_FLAG;
>> }
>>
>> Index: qemu-dgibson-for-2.6.git/target-ppc/machine.c
>> ===================================================================
>> --- qemu-dgibson-for-2.6.git.orig/target-ppc/machine.c
>> +++ qemu-dgibson-for-2.6.git/target-ppc/machine.c
>> @@ -156,12 +156,26 @@ static void cpu_pre_save(void *opaque)
>> }
>> }
>>
>> +
>> +static int cpu_post_load_excp_prefix(CPUPPCState *env)
>> +{
>> + int ail = (env->spr[SPR_LPCR] & LPCR_AIL) >> LPCR_AIL_SHIFT;
>> + target_ulong prefix = cpu_ppc_get_excp_prefix(ail);
>> +
>> + if (prefix == (target_ulong) -1ULL) {
>> + return -EINVAL;
>> + }
>> + env->excp_prefix = prefix;
>> + return 0;
>> +}
>> +
>> static int cpu_post_load(void *opaque, int version_id)
>> {
>> PowerPCCPU *cpu = opaque;
>> CPUPPCState *env = &cpu->env;
>> int i;
>> target_ulong msr;
>> + int ret = 0;
>>
>> /*
>> * We always ignore the source PVR. The user or management
>> @@ -201,7 +215,11 @@ static int cpu_post_load(void *opaque, i
>>
>> hreg_compute_mem_idx(env);
>>
>> - return 0;
>> + if (env->spr[SPR_LPCR] & LPCR_AIL) {
>> + ret = cpu_post_load_excp_prefix(env);
>> + }
>> +
>> + return ret;
>> }
>>
>> static bool fpu_needed(void *opaque)
>> Index: qemu-dgibson-for-2.6.git/target-ppc/translate_init.c
>> ===================================================================
>> --- qemu-dgibson-for-2.6.git.orig/target-ppc/translate_init.c
>> +++ qemu-dgibson-for-2.6.git/target-ppc/translate_init.c
>> @@ -8522,6 +8522,20 @@ void cpu_ppc_set_papr(PowerPCCPU *cpu)
>> }
>> }
>>
>> +target_ulong cpu_ppc_get_excp_prefix(target_ulong ail)
>> +{
>> + switch (ail) {
>> + case AIL_NONE:
>> + return 0;
>> + case AIL_0001_8000:
>> + return 0x18000;
>> + case AIL_C000_0000_0000_4000:
>> + return 0xC000000000004000ULL;
>> + default:
>> + return (target_ulong) -1ULL;
>> + }
>> +}
>> +
>> #endif /* !defined(CONFIG_USER_ONLY) */
>>
>> #endif /* defined (TARGET_PPC64) */
>> Index: qemu-dgibson-for-2.6.git/target-ppc/cpu.h
>> ===================================================================
>> --- qemu-dgibson-for-2.6.git.orig/target-ppc/cpu.h
>> +++ qemu-dgibson-for-2.6.git/target-ppc/cpu.h
>> @@ -1269,6 +1269,7 @@ void store_booke_tsr (CPUPPCState *env,
>> void ppc_tlb_invalidate_all (CPUPPCState *env);
>> void ppc_tlb_invalidate_one (CPUPPCState *env, target_ulong addr);
>> void cpu_ppc_set_papr(PowerPCCPU *cpu);
>> +target_ulong cpu_ppc_get_excp_prefix(target_ulong ail);
>> #endif
>> #endif
>>
>> @@ -2277,6 +2278,14 @@ enum {
>> HMER_XSCOM_STATUS_LSH = (63 - 23),
>> };
>>
>> +/* Alternate Interrupt Location (AIL) */
>> +enum {
>> + AIL_NONE = 0,
>> + AIL_RESERVED = 1,
>> + AIL_0001_8000 = 2,
>> + AIL_C000_0000_0000_4000 = 3,
>> +};
>> +
>> /*****************************************************************************/
>>
>> static inline target_ulong cpu_read_xer(CPUPPCState *env)
>> Index: qemu-dgibson-for-2.6.git/include/hw/ppc/spapr.h
>> ===================================================================
>> --- qemu-dgibson-for-2.6.git.orig/include/hw/ppc/spapr.h
>> +++ qemu-dgibson-for-2.6.git/include/hw/ppc/spapr.h
>> @@ -204,11 +204,6 @@ struct sPAPRMachineState {
>> #define H_SET_MODE_ENDIAN_BIG 0
>> #define H_SET_MODE_ENDIAN_LITTLE 1
>>
>> -/* Flags for H_SET_MODE_RESOURCE_ADDR_TRANS_MODE */
>> -#define H_SET_MODE_ADDR_TRANS_NONE 0
>> -#define H_SET_MODE_ADDR_TRANS_0001_8000 2
>> -#define H_SET_MODE_ADDR_TRANS_C000_0000_0000_4000 3
>> -
>> /* VASI States */
>> #define H_VASI_INVALID 0
>> #define H_VASI_ENABLED 1
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [PATCH v2] spapr: compute interrupt vector address from LPCR
2016-03-31 8:20 ` Cédric Le Goater
@ 2016-03-31 9:33 ` Cédric Le Goater
0 siblings, 0 replies; 15+ messages in thread
From: Cédric Le Goater @ 2016-03-31 9:33 UTC (permalink / raw)
To: Greg Kurz; +Cc: Thomas Huth, qemu-ppc, qemu-devel, David Gibson
>>> Index: qemu-dgibson-for-2.6.git/hw/ppc/spapr_hcall.c
>>> ===================================================================
>>> --- qemu-dgibson-for-2.6.git.orig/hw/ppc/spapr_hcall.c
>>> +++ qemu-dgibson-for-2.6.git/hw/ppc/spapr_hcall.c
>>> @@ -835,17 +835,8 @@ static target_ulong h_set_mode_resource_
>>> return H_P4;
>>> }
>>>
>>> - switch (mflags) {
>>> - case H_SET_MODE_ADDR_TRANS_NONE:
>>> - prefix = 0;
>>> - break;
>>> - case H_SET_MODE_ADDR_TRANS_0001_8000:
>>> - prefix = 0x18000;
>>> - break;
>>> - case H_SET_MODE_ADDR_TRANS_C000_0000_0000_4000:
>>> - prefix = 0xC000000000004000ULL;
>>> - break;
>>> - default:
>>> + prefix = cpu_ppc_get_excp_prefix(mflags);
>>> + if (prefix == (target_ulong) -1ULL) {
>>
>> + if (prefix == (target_ulong) (-1ULL)) {
>>
>> to make ./scripts/checkpatch.pl happy :)
>
> yes.
>
> The funny thing is that checkpatch.pl does not complain for the exact
> same line below. Looks like a checkpatch.pl bug to me.
Well, the reason is that a new possible type 'target_ulong' is caught a
little late in the checkpatch process and this is why the script complains
at the first occurrence.
Pointer casts are caught early but not casts, probably because this is too
complex to catch with a regex.
So the easy fix would be to add 'target_ulong' in the @typeList array.
This seems reasonable enough as 'target_ulong' is a common qemu type.
C.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [PATCH v2] spapr: compute interrupt vector address from LPCR
2016-03-31 7:13 ` [Qemu-devel] [Qemu-ppc] " Mark Cave-Ayland
@ 2016-04-01 2:43 ` David Gibson
2016-04-03 17:57 ` Cédric Le Goater
0 siblings, 1 reply; 15+ messages in thread
From: David Gibson @ 2016-04-01 2:43 UTC (permalink / raw)
To: Mark Cave-Ayland
Cc: Thomas Huth, Cédric Le Goater, qemu-ppc, qemu-devel, Greg Kurz
[-- Attachment #1: Type: text/plain, Size: 5399 bytes --]
On Thu, Mar 31, 2016 at 08:13:09AM +0100, Mark Cave-Ayland wrote:
> On 31/03/16 05:55, David Gibson wrote:
>
> > On Wed, Mar 30, 2016 at 05:38:34PM +0200, Cédric Le Goater wrote:
> >> This address is changed by the linux kernel using the H_SET_MODE hcall
> >> and needs to be restored when migrating a spapr VM running in
> >> TCG. This can be done using the AIL bits from the LPCR register.
> >>
> >> The patch introduces a helper routine cpu_ppc_get_excp_prefix() which
> >> returns the effective address offset of the interrupt handler
> >> depending on the LPCR_AIL bits. The same helper can be used in the
> >> H_SET_MODE hcall, which lets us remove the H_SET_MODE_ADDR_TRANS_*
> >> defines.
> >>
> >> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
> >
> > I've applied this (with Greg's minor amendments) to ppc-for-2.6),
> > since it certainly improves behaviour, although I have a couple of
> > queries:
> >
> >> ---
> >>
> >> Changes since v1:
> >>
> >> - moved helper routine under target-ppc/
> >> - moved the restore of excp_prefix under cpu_post_load()
> >>
> >> hw/ppc/spapr_hcall.c | 13 ++-----------
> >> include/hw/ppc/spapr.h | 5 -----
> >> target-ppc/cpu.h | 9 +++++++++
> >> target-ppc/machine.c | 20 +++++++++++++++++++-
> >> target-ppc/translate_init.c | 14 ++++++++++++++
> >> 5 files changed, 44 insertions(+), 17 deletions(-)
> >>
> >> Index: qemu-dgibson-for-2.6.git/hw/ppc/spapr_hcall.c
> >> ===================================================================
> >> --- qemu-dgibson-for-2.6.git.orig/hw/ppc/spapr_hcall.c
> >> +++ qemu-dgibson-for-2.6.git/hw/ppc/spapr_hcall.c
> >> @@ -835,17 +835,8 @@ static target_ulong h_set_mode_resource_
> >> return H_P4;
> >> }
> >>
> >> - switch (mflags) {
> >> - case H_SET_MODE_ADDR_TRANS_NONE:
> >> - prefix = 0;
> >> - break;
> >> - case H_SET_MODE_ADDR_TRANS_0001_8000:
> >> - prefix = 0x18000;
> >> - break;
> >> - case H_SET_MODE_ADDR_TRANS_C000_0000_0000_4000:
> >> - prefix = 0xC000000000004000ULL;
> >> - break;
> >> - default:
> >> + prefix = cpu_ppc_get_excp_prefix(mflags);
> >> + if (prefix == (target_ulong) -1ULL) {
> >> return H_UNSUPPORTED_FLAG;
> >> }
> >>
> >> Index: qemu-dgibson-for-2.6.git/target-ppc/machine.c
> >> ===================================================================
> >> --- qemu-dgibson-for-2.6.git.orig/target-ppc/machine.c
> >> +++ qemu-dgibson-for-2.6.git/target-ppc/machine.c
> >> @@ -156,12 +156,26 @@ static void cpu_pre_save(void *opaque)
> >> }
> >> }
> >>
> >> +
> >> +static int cpu_post_load_excp_prefix(CPUPPCState *env)
> >> +{
> >> + int ail = (env->spr[SPR_LPCR] & LPCR_AIL) >> LPCR_AIL_SHIFT;
> >> + target_ulong prefix = cpu_ppc_get_excp_prefix(ail);
> >> +
> >> + if (prefix == (target_ulong) -1ULL) {
> >> + return -EINVAL;
> >> + }
> >> + env->excp_prefix = prefix;
> >> + return 0;
> >> +}
> >> +
> >> static int cpu_post_load(void *opaque, int version_id)
> >> {
> >> PowerPCCPU *cpu = opaque;
> >> CPUPPCState *env = &cpu->env;
> >> int i;
> >> target_ulong msr;
> >> + int ret = 0;
> >>
> >> /*
> >> * We always ignore the source PVR. The user or management
> >> @@ -201,7 +215,11 @@ static int cpu_post_load(void *opaque, i
> >>
> >> hreg_compute_mem_idx(env);
> >>
> >> - return 0;
> >> + if (env->spr[SPR_LPCR] & LPCR_AIL) {
> >> + ret = cpu_post_load_excp_prefix(env);
> >> + }
> >
> > Why not call this unconditionally? If AIL == 0 it will still do the
> > right thing.
> >
> > Aren't there also circumstances where the exception prefix can depend
> > on the MSR? Do those need to be handled somewhere?
>
> Yes indeed - this was part of my patchset last year to fix up various
> migration issues for the Mac PPC machines (see commit
> 2360b6e84f78d41fa0f76555a947148b73645259).
>
> I agree that having the env->excp_prefix logic split like this isn't a
> particularly great idea. Let's just have a single helper function as in
> the patch above and use that in both places (and in fact with recent
> changes I have a feeling env->excp_prefix is one of the few remaining
> reasons that the above change is still required, but please check this).
Right.
So, what I'd really prefer to see here is a single
update_excp_prefix() helper which will set env->excp_prefix based on
whatever registers are relevant (LPCR, MSR and probably the cpu
model). That would be called on incoming migration, and after
updating any of the relevant registers (so from the mtmsr and mtlpcr
emulations). H_SET_MODE should be handled by first updating the LPCR
value, then calling the update helper.
Cédric, I'm ok if you provide a version of that which only handles
POWER7 and POWER8 (i.e. spapr compatible models for now). Mark - if
you can supply corrections to that outline for Macintosh era models,
that would be great.
I do want to get the basic migration problem fixed before 2.6 is
release.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [PATCH v2] spapr: compute interrupt vector address from LPCR
2016-03-31 4:55 ` David Gibson
2016-03-31 7:13 ` [Qemu-devel] [Qemu-ppc] " Mark Cave-Ayland
2016-03-31 8:16 ` [Qemu-devel] " Cédric Le Goater
@ 2016-04-01 3:34 ` David Gibson
2 siblings, 0 replies; 15+ messages in thread
From: David Gibson @ 2016-04-01 3:34 UTC (permalink / raw)
To: Cédric Le Goater; +Cc: Thomas Huth, qemu-ppc, qemu-devel, Greg Kurz
[-- Attachment #1: Type: text/plain, Size: 1215 bytes --]
On Thu, Mar 31, 2016 at 03:55:42PM +1100, David Gibson wrote:
> On Wed, Mar 30, 2016 at 05:38:34PM +0200, Cédric Le Goater wrote:
> > This address is changed by the linux kernel using the H_SET_MODE hcall
> > and needs to be restored when migrating a spapr VM running in
> > TCG. This can be done using the AIL bits from the LPCR register.
> >
> > The patch introduces a helper routine cpu_ppc_get_excp_prefix() which
> > returns the effective address offset of the interrupt handler
> > depending on the LPCR_AIL bits. The same helper can be used in the
> > H_SET_MODE hcall, which lets us remove the H_SET_MODE_ADDR_TRANS_*
> > defines.
> >
> > Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
>
> I've applied this (with Greg's minor amendments) to ppc-for-2.6),
> since it certainly improves behaviour, although I have a couple of
> queries:
And.. I've take it out again now. In addition to the fact that I'd
like some rework suggested elsewhere, it breaks compile for 32-bit ppc
targets.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [PATCH v2] spapr: compute interrupt vector address from LPCR
2016-04-01 2:43 ` David Gibson
@ 2016-04-03 17:57 ` Cédric Le Goater
2016-04-04 4:16 ` David Gibson
0 siblings, 1 reply; 15+ messages in thread
From: Cédric Le Goater @ 2016-04-03 17:57 UTC (permalink / raw)
To: David Gibson, Mark Cave-Ayland
Cc: Thomas Huth, qemu-ppc, qemu-devel, Greg Kurz
On 04/01/2016 04:43 AM, David Gibson wrote:
> On Thu, Mar 31, 2016 at 08:13:09AM +0100, Mark Cave-Ayland wrote:
>> On 31/03/16 05:55, David Gibson wrote:
>>
>>> On Wed, Mar 30, 2016 at 05:38:34PM +0200, Cédric Le Goater wrote:
>>>> This address is changed by the linux kernel using the H_SET_MODE hcall
>>>> and needs to be restored when migrating a spapr VM running in
>>>> TCG. This can be done using the AIL bits from the LPCR register.
>>>>
>>>> The patch introduces a helper routine cpu_ppc_get_excp_prefix() which
>>>> returns the effective address offset of the interrupt handler
>>>> depending on the LPCR_AIL bits. The same helper can be used in the
>>>> H_SET_MODE hcall, which lets us remove the H_SET_MODE_ADDR_TRANS_*
>>>> defines.
>>>>
>>>> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
>>>
>>> I've applied this (with Greg's minor amendments) to ppc-for-2.6),
>>> since it certainly improves behaviour, although I have a couple of
>>> queries:
>>>
>>>> ---
>>>>
>>>> Changes since v1:
>>>>
>>>> - moved helper routine under target-ppc/
>>>> - moved the restore of excp_prefix under cpu_post_load()
>>>>
>>>> hw/ppc/spapr_hcall.c | 13 ++-----------
>>>> include/hw/ppc/spapr.h | 5 -----
>>>> target-ppc/cpu.h | 9 +++++++++
>>>> target-ppc/machine.c | 20 +++++++++++++++++++-
>>>> target-ppc/translate_init.c | 14 ++++++++++++++
>>>> 5 files changed, 44 insertions(+), 17 deletions(-)
>>>>
>>>> Index: qemu-dgibson-for-2.6.git/hw/ppc/spapr_hcall.c
>>>> ===================================================================
>>>> --- qemu-dgibson-for-2.6.git.orig/hw/ppc/spapr_hcall.c
>>>> +++ qemu-dgibson-for-2.6.git/hw/ppc/spapr_hcall.c
>>>> @@ -835,17 +835,8 @@ static target_ulong h_set_mode_resource_
>>>> return H_P4;
>>>> }
>>>>
>>>> - switch (mflags) {
>>>> - case H_SET_MODE_ADDR_TRANS_NONE:
>>>> - prefix = 0;
>>>> - break;
>>>> - case H_SET_MODE_ADDR_TRANS_0001_8000:
>>>> - prefix = 0x18000;
>>>> - break;
>>>> - case H_SET_MODE_ADDR_TRANS_C000_0000_0000_4000:
>>>> - prefix = 0xC000000000004000ULL;
>>>> - break;
>>>> - default:
>>>> + prefix = cpu_ppc_get_excp_prefix(mflags);
>>>> + if (prefix == (target_ulong) -1ULL) {
>>>> return H_UNSUPPORTED_FLAG;
>>>> }
>>>>
>>>> Index: qemu-dgibson-for-2.6.git/target-ppc/machine.c
>>>> ===================================================================
>>>> --- qemu-dgibson-for-2.6.git.orig/target-ppc/machine.c
>>>> +++ qemu-dgibson-for-2.6.git/target-ppc/machine.c
>>>> @@ -156,12 +156,26 @@ static void cpu_pre_save(void *opaque)
>>>> }
>>>> }
>>>>
>>>> +
>>>> +static int cpu_post_load_excp_prefix(CPUPPCState *env)
>>>> +{
>>>> + int ail = (env->spr[SPR_LPCR] & LPCR_AIL) >> LPCR_AIL_SHIFT;
>>>> + target_ulong prefix = cpu_ppc_get_excp_prefix(ail);
>>>> +
>>>> + if (prefix == (target_ulong) -1ULL) {
>>>> + return -EINVAL;
>>>> + }
>>>> + env->excp_prefix = prefix;
>>>> + return 0;
>>>> +}
>>>> +
>>>> static int cpu_post_load(void *opaque, int version_id)
>>>> {
>>>> PowerPCCPU *cpu = opaque;
>>>> CPUPPCState *env = &cpu->env;
>>>> int i;
>>>> target_ulong msr;
>>>> + int ret = 0;
>>>>
>>>> /*
>>>> * We always ignore the source PVR. The user or management
>>>> @@ -201,7 +215,11 @@ static int cpu_post_load(void *opaque, i
>>>>
>>>> hreg_compute_mem_idx(env);
>>>>
>>>> - return 0;
>>>> + if (env->spr[SPR_LPCR] & LPCR_AIL) {
>>>> + ret = cpu_post_load_excp_prefix(env);
>>>> + }
>>>
>>> Why not call this unconditionally? If AIL == 0 it will still do the
>>> right thing.
>>>
>>> Aren't there also circumstances where the exception prefix can depend
>>> on the MSR? Do those need to be handled somewhere?
>>
>> Yes indeed - this was part of my patchset last year to fix up various
>> migration issues for the Mac PPC machines (see commit
>> 2360b6e84f78d41fa0f76555a947148b73645259).
>>
>> I agree that having the env->excp_prefix logic split like this isn't a
>> particularly great idea. Let's just have a single helper function as in
>> the patch above and use that in both places (and in fact with recent
>> changes I have a feeling env->excp_prefix is one of the few remaining
>> reasons that the above change is still required, but please check this).
>
> Right.
>
> So, what I'd really prefer to see here is a single
> update_excp_prefix() helper which will set env->excp_prefix based on
> whatever registers are relevant (LPCR, MSR and probably the cpu
> model). That would be called on incoming migration, and after
> updating any of the relevant registers (so from the mtmsr and mtlpcr
> emulations). H_SET_MODE should be handled by first updating the LPCR
> value, then calling the update helper.
>
> Cédric, I'm ok if you provide a version of that which only handles
> POWER7 and POWER8 (i.e. spapr compatible models for now).
Sure.
But first, could you please take a look at a reworked patch of Ben who
had forseen the issue ? I kept the AIL fix, added some extra for ILE
and cleanup H_SET_MODE. Tests under KVM,TCG on LE,BE guests went fine.
If you think this is too risky for 2.6, I will work on what you asked for.
> Mark - if
> you can supply corrections to that outline for Macintosh era models,
> that would be great.
>
> I do want to get the basic migration problem fixed before 2.6 is
> release.
Thanks,
C.
>From bb9d1e06a0ea2132902db724f61c8dc655aa1a96 Mon Sep 17 00:00:00 2001
From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Date: Thu, 4 Jun 2015 03:39:19 +1000
Subject: [PATCH 18/77] ppc: Rework POWER7 & POWER8 exception model
This patch fixes the current AIL implementation for POWER8. The
interrupt vector address can be calculated directly from LPCR when the
exception is handled. The excp_prefix update becomes useless and we
can cleanup the H_SET_MODE hcall.
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
[clg: Removed LPES0/1 handling for HV vs. !HV
Fixed LPCR_ILE case for POWERPC_EXCP_POWER8 ]
Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
---
hw/ppc/spapr_hcall.c | 16 --------------
include/hw/ppc/spapr.h | 5 ----
target-ppc/cpu.h | 10 ++++++++
target-ppc/excp_helper.c | 49 ++++++++++++++++++++++++++++++++++++++++++--
target-ppc/translate_init.c | 2 -
5 files changed, 59 insertions(+), 23 deletions(-)
Index: qemu-dgibson-for-2.6.git/target-ppc/cpu.h
===================================================================
--- qemu-dgibson-for-2.6.git.orig/target-ppc/cpu.h
+++ qemu-dgibson-for-2.6.git/target-ppc/cpu.h
@@ -167,6 +167,8 @@ enum powerpc_excp_t {
POWERPC_EXCP_970,
/* POWER7 exception model */
POWERPC_EXCP_POWER7,
+ /* POWER8 exception model */
+ POWERPC_EXCP_POWER8,
#endif /* defined(TARGET_PPC64) */
};
@@ -2277,6 +2279,14 @@ enum {
HMER_XSCOM_STATUS_LSH = (63 - 23),
};
+/* Alternate Interrupt Location (AIL) */
+enum {
+ AIL_NONE = 0,
+ AIL_RESERVED = 1,
+ AIL_0001_8000 = 2,
+ AIL_C000_0000_0000_4000 = 3,
+};
+
/*****************************************************************************/
static inline target_ulong cpu_read_xer(CPUPPCState *env)
Index: qemu-dgibson-for-2.6.git/target-ppc/excp_helper.c
===================================================================
--- qemu-dgibson-for-2.6.git.orig/target-ppc/excp_helper.c
+++ qemu-dgibson-for-2.6.git/target-ppc/excp_helper.c
@@ -77,7 +77,7 @@ static inline void powerpc_excp(PowerPCC
CPUPPCState *env = &cpu->env;
target_ulong msr, new_msr, vector;
int srr0, srr1, asrr0, asrr1;
- int lpes0, lpes1, lev;
+ int lpes0, lpes1, lev, ail;
if (0) {
/* XXX: find a suitable condition to enable the hypervisor mode */
@@ -108,6 +108,25 @@ static inline void powerpc_excp(PowerPCC
asrr0 = -1;
asrr1 = -1;
+ /* Exception targetting modifiers
+ *
+ * AIL is initialized here but can be cleared by
+ * selected exceptions
+ */
+#if defined(TARGET_PPC64)
+ if (excp_model == POWERPC_EXCP_POWER7 ||
+ excp_model == POWERPC_EXCP_POWER8) {
+ if (excp_model == POWERPC_EXCP_POWER8) {
+ ail = (env->spr[SPR_LPCR] & LPCR_AIL) >> LPCR_AIL_SHIFT;
+ } else {
+ ail = 0;
+ }
+ } else
+#endif /* defined(TARGET_PPC64) */
+ {
+ ail = 0;
+ }
+
switch (excp) {
case POWERPC_EXCP_NONE:
/* Should never happen */
@@ -146,6 +165,7 @@ static inline void powerpc_excp(PowerPCC
/* XXX: find a suitable condition to enable the hypervisor mode */
new_msr |= (target_ulong)MSR_HVB;
}
+ ail = 0;
/* machine check exceptions don't have ME set */
new_msr &= ~((target_ulong)1 << MSR_ME);
@@ -344,6 +364,7 @@ static inline void powerpc_excp(PowerPCC
/* XXX: find a suitable condition to enable the hypervisor mode */
new_msr |= (target_ulong)MSR_HVB;
}
+ ail = 0;
goto store_next;
case POWERPC_EXCP_DSEG: /* Data segment exception */
if (lpes1 == 0) {
@@ -630,7 +651,8 @@ static inline void powerpc_excp(PowerPCC
}
#ifdef TARGET_PPC64
- if (excp_model == POWERPC_EXCP_POWER7) {
+ if (excp_model == POWERPC_EXCP_POWER7 ||
+ excp_model == POWERPC_EXCP_POWER8) {
if (env->spr[SPR_LPCR] & LPCR_ILE) {
new_msr |= (target_ulong)1 << MSR_LE;
}
@@ -650,6 +672,29 @@ static inline void powerpc_excp(PowerPCC
excp);
}
vector |= env->excp_prefix;
+
+ /* AIL only works if there is no HV transition and we are running with
+ * translations enabled
+ */
+ if (!((msr >> MSR_IR) & 1) || !((msr >> MSR_DR) & 1)) {
+ ail = 0;
+ }
+ /* Handle AIL */
+ if (ail) {
+ new_msr |= (1 << MSR_IR) | (1 << MSR_DR);
+ switch(ail) {
+ case AIL_0001_8000:
+ vector |= 0x18000;
+ break;
+ case AIL_C000_0000_0000_4000:
+ vector |= 0xc000000000004000ull;
+ break;
+ default:
+ cpu_abort(cs, "Invalid AIL combination %d\n", ail);
+ break;
+ }
+ }
+
#if defined(TARGET_PPC64)
if (excp_model == POWERPC_EXCP_BOOKE) {
if (env->spr[SPR_BOOKE_EPCR] & EPCR_ICM) {
Index: qemu-dgibson-for-2.6.git/target-ppc/translate_init.c
===================================================================
--- qemu-dgibson-for-2.6.git.orig/target-ppc/translate_init.c
+++ qemu-dgibson-for-2.6.git/target-ppc/translate_init.c
@@ -8487,7 +8487,7 @@ POWERPC_FAMILY(POWER8)(ObjectClass *oc,
pcc->handle_mmu_fault = ppc_hash64_handle_mmu_fault;
pcc->sps = &POWER7_POWER8_sps;
#endif
- pcc->excp_model = POWERPC_EXCP_POWER7;
+ pcc->excp_model = POWERPC_EXCP_POWER8;
pcc->bus_model = PPC_FLAGS_INPUT_POWER7;
pcc->bfd_mach = bfd_mach_ppc64;
pcc->flags = POWERPC_FLAG_VRE | POWERPC_FLAG_SE |
Index: qemu-dgibson-for-2.6.git/hw/ppc/spapr_hcall.c
===================================================================
--- qemu-dgibson-for-2.6.git.orig/hw/ppc/spapr_hcall.c
+++ qemu-dgibson-for-2.6.git/hw/ppc/spapr_hcall.c
@@ -823,7 +823,6 @@ static target_ulong h_set_mode_resource_
{
CPUState *cs;
PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
- target_ulong prefix;
if (!(pcc->insns_flags2 & PPC2_ISA207S)) {
return H_P2;
@@ -835,25 +834,12 @@ static target_ulong h_set_mode_resource_
return H_P4;
}
- switch (mflags) {
- case H_SET_MODE_ADDR_TRANS_NONE:
- prefix = 0;
- break;
- case H_SET_MODE_ADDR_TRANS_0001_8000:
- prefix = 0x18000;
- break;
- case H_SET_MODE_ADDR_TRANS_C000_0000_0000_4000:
- prefix = 0xC000000000004000ULL;
- break;
- default:
+ if (mflags == AIL_RESERVED) {
return H_UNSUPPORTED_FLAG;
}
CPU_FOREACH(cs) {
- CPUPPCState *env = &POWERPC_CPU(cpu)->env;
-
set_spr(cs, SPR_LPCR, mflags << LPCR_AIL_SHIFT, LPCR_AIL);
- env->excp_prefix = prefix;
}
return H_SUCCESS;
Index: qemu-dgibson-for-2.6.git/include/hw/ppc/spapr.h
===================================================================
--- qemu-dgibson-for-2.6.git.orig/include/hw/ppc/spapr.h
+++ qemu-dgibson-for-2.6.git/include/hw/ppc/spapr.h
@@ -204,11 +204,6 @@ struct sPAPRMachineState {
#define H_SET_MODE_ENDIAN_BIG 0
#define H_SET_MODE_ENDIAN_LITTLE 1
-/* Flags for H_SET_MODE_RESOURCE_ADDR_TRANS_MODE */
-#define H_SET_MODE_ADDR_TRANS_NONE 0
-#define H_SET_MODE_ADDR_TRANS_0001_8000 2
-#define H_SET_MODE_ADDR_TRANS_C000_0000_0000_4000 3
-
/* VASI States */
#define H_VASI_INVALID 0
#define H_VASI_ENABLED 1
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [PATCH v2] spapr: compute interrupt vector address from LPCR
2016-04-03 17:57 ` Cédric Le Goater
@ 2016-04-04 4:16 ` David Gibson
2016-04-04 14:47 ` Cédric Le Goater
0 siblings, 1 reply; 15+ messages in thread
From: David Gibson @ 2016-04-04 4:16 UTC (permalink / raw)
To: Cédric Le Goater
Cc: qemu-ppc, Thomas Huth, Mark Cave-Ayland, qemu-devel, Greg Kurz
[-- Attachment #1: Type: text/plain, Size: 14617 bytes --]
On Sun, Apr 03, 2016 at 07:57:50PM +0200, Cédric Le Goater wrote:
> On 04/01/2016 04:43 AM, David Gibson wrote:
> > On Thu, Mar 31, 2016 at 08:13:09AM +0100, Mark Cave-Ayland wrote:
> >> On 31/03/16 05:55, David Gibson wrote:
> >>
> >>> On Wed, Mar 30, 2016 at 05:38:34PM +0200, Cédric Le Goater wrote:
> >>>> This address is changed by the linux kernel using the H_SET_MODE hcall
> >>>> and needs to be restored when migrating a spapr VM running in
> >>>> TCG. This can be done using the AIL bits from the LPCR register.
> >>>>
> >>>> The patch introduces a helper routine cpu_ppc_get_excp_prefix() which
> >>>> returns the effective address offset of the interrupt handler
> >>>> depending on the LPCR_AIL bits. The same helper can be used in the
> >>>> H_SET_MODE hcall, which lets us remove the H_SET_MODE_ADDR_TRANS_*
> >>>> defines.
> >>>>
> >>>> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
> >>>
> >>> I've applied this (with Greg's minor amendments) to ppc-for-2.6),
> >>> since it certainly improves behaviour, although I have a couple of
> >>> queries:
> >>>
> >>>> ---
> >>>>
> >>>> Changes since v1:
> >>>>
> >>>> - moved helper routine under target-ppc/
> >>>> - moved the restore of excp_prefix under cpu_post_load()
> >>>>
> >>>> hw/ppc/spapr_hcall.c | 13 ++-----------
> >>>> include/hw/ppc/spapr.h | 5 -----
> >>>> target-ppc/cpu.h | 9 +++++++++
> >>>> target-ppc/machine.c | 20 +++++++++++++++++++-
> >>>> target-ppc/translate_init.c | 14 ++++++++++++++
> >>>> 5 files changed, 44 insertions(+), 17 deletions(-)
> >>>>
> >>>> Index: qemu-dgibson-for-2.6.git/hw/ppc/spapr_hcall.c
> >>>> ===================================================================
> >>>> --- qemu-dgibson-for-2.6.git.orig/hw/ppc/spapr_hcall.c
> >>>> +++ qemu-dgibson-for-2.6.git/hw/ppc/spapr_hcall.c
> >>>> @@ -835,17 +835,8 @@ static target_ulong h_set_mode_resource_
> >>>> return H_P4;
> >>>> }
> >>>>
> >>>> - switch (mflags) {
> >>>> - case H_SET_MODE_ADDR_TRANS_NONE:
> >>>> - prefix = 0;
> >>>> - break;
> >>>> - case H_SET_MODE_ADDR_TRANS_0001_8000:
> >>>> - prefix = 0x18000;
> >>>> - break;
> >>>> - case H_SET_MODE_ADDR_TRANS_C000_0000_0000_4000:
> >>>> - prefix = 0xC000000000004000ULL;
> >>>> - break;
> >>>> - default:
> >>>> + prefix = cpu_ppc_get_excp_prefix(mflags);
> >>>> + if (prefix == (target_ulong) -1ULL) {
> >>>> return H_UNSUPPORTED_FLAG;
> >>>> }
> >>>>
> >>>> Index: qemu-dgibson-for-2.6.git/target-ppc/machine.c
> >>>> ===================================================================
> >>>> --- qemu-dgibson-for-2.6.git.orig/target-ppc/machine.c
> >>>> +++ qemu-dgibson-for-2.6.git/target-ppc/machine.c
> >>>> @@ -156,12 +156,26 @@ static void cpu_pre_save(void *opaque)
> >>>> }
> >>>> }
> >>>>
> >>>> +
> >>>> +static int cpu_post_load_excp_prefix(CPUPPCState *env)
> >>>> +{
> >>>> + int ail = (env->spr[SPR_LPCR] & LPCR_AIL) >> LPCR_AIL_SHIFT;
> >>>> + target_ulong prefix = cpu_ppc_get_excp_prefix(ail);
> >>>> +
> >>>> + if (prefix == (target_ulong) -1ULL) {
> >>>> + return -EINVAL;
> >>>> + }
> >>>> + env->excp_prefix = prefix;
> >>>> + return 0;
> >>>> +}
> >>>> +
> >>>> static int cpu_post_load(void *opaque, int version_id)
> >>>> {
> >>>> PowerPCCPU *cpu = opaque;
> >>>> CPUPPCState *env = &cpu->env;
> >>>> int i;
> >>>> target_ulong msr;
> >>>> + int ret = 0;
> >>>>
> >>>> /*
> >>>> * We always ignore the source PVR. The user or management
> >>>> @@ -201,7 +215,11 @@ static int cpu_post_load(void *opaque, i
> >>>>
> >>>> hreg_compute_mem_idx(env);
> >>>>
> >>>> - return 0;
> >>>> + if (env->spr[SPR_LPCR] & LPCR_AIL) {
> >>>> + ret = cpu_post_load_excp_prefix(env);
> >>>> + }
> >>>
> >>> Why not call this unconditionally? If AIL == 0 it will still do the
> >>> right thing.
> >>>
> >>> Aren't there also circumstances where the exception prefix can depend
> >>> on the MSR? Do those need to be handled somewhere?
> >>
> >> Yes indeed - this was part of my patchset last year to fix up various
> >> migration issues for the Mac PPC machines (see commit
> >> 2360b6e84f78d41fa0f76555a947148b73645259).
> >>
> >> I agree that having the env->excp_prefix logic split like this isn't a
> >> particularly great idea. Let's just have a single helper function as in
> >> the patch above and use that in both places (and in fact with recent
> >> changes I have a feeling env->excp_prefix is one of the few remaining
> >> reasons that the above change is still required, but please check this).
> >
> > Right.
> >
> > So, what I'd really prefer to see here is a single
> > update_excp_prefix() helper which will set env->excp_prefix based on
> > whatever registers are relevant (LPCR, MSR and probably the cpu
> > model). That would be called on incoming migration, and after
> > updating any of the relevant registers (so from the mtmsr and mtlpcr
> > emulations). H_SET_MODE should be handled by first updating the LPCR
> > value, then calling the update helper.
> >
> > Cédric, I'm ok if you provide a version of that which only handles
> > POWER7 and POWER8 (i.e. spapr compatible models for now).
>
> Sure.
>
> But first, could you please take a look at a reworked patch of Ben who
> had forseen the issue ? I kept the AIL fix, added some extra for ILE
> and cleanup H_SET_MODE. Tests under KVM,TCG on LE,BE guests went fine.
>
> If you think this is too risky for 2.6, I will work on what you asked for.
Ah, yes this approach does seem more correct. It looks like it leaves
room for further cleanups to excp_prefix later (such as completely
removing it in favour of calculating it from reg values at exception
time). But for now it looks like it will address the migration issue
at hand.
I've applied it to ppc-for-2.6.
>
> > Mark - if
> > you can supply corrections to that outline for Macintosh era models,
> > that would be great.
> >
> > I do want to get the basic migration problem fixed before 2.6 is
> > release.
>
>
> Thanks,
>
> C.
>
> >From bb9d1e06a0ea2132902db724f61c8dc655aa1a96 Mon Sep 17 00:00:00 2001
> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Date: Thu, 4 Jun 2015 03:39:19 +1000
> Subject: [PATCH 18/77] ppc: Rework POWER7 & POWER8 exception model
>
> This patch fixes the current AIL implementation for POWER8. The
> interrupt vector address can be calculated directly from LPCR when the
> exception is handled. The excp_prefix update becomes useless and we
> can cleanup the H_SET_MODE hcall.
>
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> [clg: Removed LPES0/1 handling for HV vs. !HV
> Fixed LPCR_ILE case for POWERPC_EXCP_POWER8 ]
> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
> ---
> hw/ppc/spapr_hcall.c | 16 --------------
> include/hw/ppc/spapr.h | 5 ----
> target-ppc/cpu.h | 10 ++++++++
> target-ppc/excp_helper.c | 49 ++++++++++++++++++++++++++++++++++++++++++--
> target-ppc/translate_init.c | 2 -
> 5 files changed, 59 insertions(+), 23 deletions(-)
>
> Index: qemu-dgibson-for-2.6.git/target-ppc/cpu.h
> ===================================================================
> --- qemu-dgibson-for-2.6.git.orig/target-ppc/cpu.h
> +++ qemu-dgibson-for-2.6.git/target-ppc/cpu.h
> @@ -167,6 +167,8 @@ enum powerpc_excp_t {
> POWERPC_EXCP_970,
> /* POWER7 exception model */
> POWERPC_EXCP_POWER7,
> + /* POWER8 exception model */
> + POWERPC_EXCP_POWER8,
> #endif /* defined(TARGET_PPC64) */
> };
>
> @@ -2277,6 +2279,14 @@ enum {
> HMER_XSCOM_STATUS_LSH = (63 - 23),
> };
>
> +/* Alternate Interrupt Location (AIL) */
> +enum {
> + AIL_NONE = 0,
> + AIL_RESERVED = 1,
> + AIL_0001_8000 = 2,
> + AIL_C000_0000_0000_4000 = 3,
> +};
> +
> /*****************************************************************************/
>
> static inline target_ulong cpu_read_xer(CPUPPCState *env)
> Index: qemu-dgibson-for-2.6.git/target-ppc/excp_helper.c
> ===================================================================
> --- qemu-dgibson-for-2.6.git.orig/target-ppc/excp_helper.c
> +++ qemu-dgibson-for-2.6.git/target-ppc/excp_helper.c
> @@ -77,7 +77,7 @@ static inline void powerpc_excp(PowerPCC
> CPUPPCState *env = &cpu->env;
> target_ulong msr, new_msr, vector;
> int srr0, srr1, asrr0, asrr1;
> - int lpes0, lpes1, lev;
> + int lpes0, lpes1, lev, ail;
>
> if (0) {
> /* XXX: find a suitable condition to enable the hypervisor mode */
> @@ -108,6 +108,25 @@ static inline void powerpc_excp(PowerPCC
> asrr0 = -1;
> asrr1 = -1;
>
> + /* Exception targetting modifiers
> + *
> + * AIL is initialized here but can be cleared by
> + * selected exceptions
> + */
> +#if defined(TARGET_PPC64)
> + if (excp_model == POWERPC_EXCP_POWER7 ||
> + excp_model == POWERPC_EXCP_POWER8) {
> + if (excp_model == POWERPC_EXCP_POWER8) {
> + ail = (env->spr[SPR_LPCR] & LPCR_AIL) >> LPCR_AIL_SHIFT;
> + } else {
> + ail = 0;
> + }
> + } else
> +#endif /* defined(TARGET_PPC64) */
> + {
> + ail = 0;
> + }
> +
> switch (excp) {
> case POWERPC_EXCP_NONE:
> /* Should never happen */
> @@ -146,6 +165,7 @@ static inline void powerpc_excp(PowerPCC
> /* XXX: find a suitable condition to enable the hypervisor mode */
> new_msr |= (target_ulong)MSR_HVB;
> }
> + ail = 0;
>
> /* machine check exceptions don't have ME set */
> new_msr &= ~((target_ulong)1 << MSR_ME);
> @@ -344,6 +364,7 @@ static inline void powerpc_excp(PowerPCC
> /* XXX: find a suitable condition to enable the hypervisor mode */
> new_msr |= (target_ulong)MSR_HVB;
> }
> + ail = 0;
> goto store_next;
> case POWERPC_EXCP_DSEG: /* Data segment exception */
> if (lpes1 == 0) {
> @@ -630,7 +651,8 @@ static inline void powerpc_excp(PowerPCC
> }
>
> #ifdef TARGET_PPC64
> - if (excp_model == POWERPC_EXCP_POWER7) {
> + if (excp_model == POWERPC_EXCP_POWER7 ||
> + excp_model == POWERPC_EXCP_POWER8) {
> if (env->spr[SPR_LPCR] & LPCR_ILE) {
> new_msr |= (target_ulong)1 << MSR_LE;
> }
> @@ -650,6 +672,29 @@ static inline void powerpc_excp(PowerPCC
> excp);
> }
> vector |= env->excp_prefix;
> +
> + /* AIL only works if there is no HV transition and we are running with
> + * translations enabled
> + */
> + if (!((msr >> MSR_IR) & 1) || !((msr >> MSR_DR) & 1)) {
> + ail = 0;
> + }
> + /* Handle AIL */
> + if (ail) {
> + new_msr |= (1 << MSR_IR) | (1 << MSR_DR);
> + switch(ail) {
> + case AIL_0001_8000:
> + vector |= 0x18000;
> + break;
> + case AIL_C000_0000_0000_4000:
> + vector |= 0xc000000000004000ull;
> + break;
> + default:
> + cpu_abort(cs, "Invalid AIL combination %d\n", ail);
> + break;
> + }
> + }
> +
> #if defined(TARGET_PPC64)
> if (excp_model == POWERPC_EXCP_BOOKE) {
> if (env->spr[SPR_BOOKE_EPCR] & EPCR_ICM) {
> Index: qemu-dgibson-for-2.6.git/target-ppc/translate_init.c
> ===================================================================
> --- qemu-dgibson-for-2.6.git.orig/target-ppc/translate_init.c
> +++ qemu-dgibson-for-2.6.git/target-ppc/translate_init.c
> @@ -8487,7 +8487,7 @@ POWERPC_FAMILY(POWER8)(ObjectClass *oc,
> pcc->handle_mmu_fault = ppc_hash64_handle_mmu_fault;
> pcc->sps = &POWER7_POWER8_sps;
> #endif
> - pcc->excp_model = POWERPC_EXCP_POWER7;
> + pcc->excp_model = POWERPC_EXCP_POWER8;
> pcc->bus_model = PPC_FLAGS_INPUT_POWER7;
> pcc->bfd_mach = bfd_mach_ppc64;
> pcc->flags = POWERPC_FLAG_VRE | POWERPC_FLAG_SE |
> Index: qemu-dgibson-for-2.6.git/hw/ppc/spapr_hcall.c
> ===================================================================
> --- qemu-dgibson-for-2.6.git.orig/hw/ppc/spapr_hcall.c
> +++ qemu-dgibson-for-2.6.git/hw/ppc/spapr_hcall.c
> @@ -823,7 +823,6 @@ static target_ulong h_set_mode_resource_
> {
> CPUState *cs;
> PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> - target_ulong prefix;
>
> if (!(pcc->insns_flags2 & PPC2_ISA207S)) {
> return H_P2;
> @@ -835,25 +834,12 @@ static target_ulong h_set_mode_resource_
> return H_P4;
> }
>
> - switch (mflags) {
> - case H_SET_MODE_ADDR_TRANS_NONE:
> - prefix = 0;
> - break;
> - case H_SET_MODE_ADDR_TRANS_0001_8000:
> - prefix = 0x18000;
> - break;
> - case H_SET_MODE_ADDR_TRANS_C000_0000_0000_4000:
> - prefix = 0xC000000000004000ULL;
> - break;
> - default:
> + if (mflags == AIL_RESERVED) {
> return H_UNSUPPORTED_FLAG;
> }
>
> CPU_FOREACH(cs) {
> - CPUPPCState *env = &POWERPC_CPU(cpu)->env;
> -
> set_spr(cs, SPR_LPCR, mflags << LPCR_AIL_SHIFT, LPCR_AIL);
> - env->excp_prefix = prefix;
> }
>
> return H_SUCCESS;
> Index: qemu-dgibson-for-2.6.git/include/hw/ppc/spapr.h
> ===================================================================
> --- qemu-dgibson-for-2.6.git.orig/include/hw/ppc/spapr.h
> +++ qemu-dgibson-for-2.6.git/include/hw/ppc/spapr.h
> @@ -204,11 +204,6 @@ struct sPAPRMachineState {
> #define H_SET_MODE_ENDIAN_BIG 0
> #define H_SET_MODE_ENDIAN_LITTLE 1
>
> -/* Flags for H_SET_MODE_RESOURCE_ADDR_TRANS_MODE */
> -#define H_SET_MODE_ADDR_TRANS_NONE 0
> -#define H_SET_MODE_ADDR_TRANS_0001_8000 2
> -#define H_SET_MODE_ADDR_TRANS_C000_0000_0000_4000 3
> -
> /* VASI States */
> #define H_VASI_INVALID 0
> #define H_VASI_ENABLED 1
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [PATCH v2] spapr: compute interrupt vector address from LPCR
2016-04-04 4:16 ` David Gibson
@ 2016-04-04 14:47 ` Cédric Le Goater
2016-04-05 0:54 ` David Gibson
0 siblings, 1 reply; 15+ messages in thread
From: Cédric Le Goater @ 2016-04-04 14:47 UTC (permalink / raw)
To: David Gibson
Cc: qemu-ppc, Thomas Huth, Mark Cave-Ayland, qemu-devel, Greg Kurz
On 04/04/2016 06:16 AM, David Gibson wrote:
> On Sun, Apr 03, 2016 at 07:57:50PM +0200, Cédric Le Goater wrote:
>> On 04/01/2016 04:43 AM, David Gibson wrote:
>>> On Thu, Mar 31, 2016 at 08:13:09AM +0100, Mark Cave-Ayland wrote:
>>>> On 31/03/16 05:55, David Gibson wrote:
>>>>
>>>>> On Wed, Mar 30, 2016 at 05:38:34PM +0200, Cédric Le Goater wrote:
>>>>>> This address is changed by the linux kernel using the H_SET_MODE hcall
>>>>>> and needs to be restored when migrating a spapr VM running in
>>>>>> TCG. This can be done using the AIL bits from the LPCR register.
>>>>>>
>>>>>> The patch introduces a helper routine cpu_ppc_get_excp_prefix() which
>>>>>> returns the effective address offset of the interrupt handler
>>>>>> depending on the LPCR_AIL bits. The same helper can be used in the
>>>>>> H_SET_MODE hcall, which lets us remove the H_SET_MODE_ADDR_TRANS_*
>>>>>> defines.
>>>>>>
>>>>>> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
>>>>>
>>>>> I've applied this (with Greg's minor amendments) to ppc-for-2.6),
>>>>> since it certainly improves behaviour, although I have a couple of
>>>>> queries:
>>>>>
>>>>>> ---
>>>>>>
>>>>>> Changes since v1:
>>>>>>
>>>>>> - moved helper routine under target-ppc/
>>>>>> - moved the restore of excp_prefix under cpu_post_load()
>>>>>>
>>>>>> hw/ppc/spapr_hcall.c | 13 ++-----------
>>>>>> include/hw/ppc/spapr.h | 5 -----
>>>>>> target-ppc/cpu.h | 9 +++++++++
>>>>>> target-ppc/machine.c | 20 +++++++++++++++++++-
>>>>>> target-ppc/translate_init.c | 14 ++++++++++++++
>>>>>> 5 files changed, 44 insertions(+), 17 deletions(-)
>>>>>>
>>>>>> Index: qemu-dgibson-for-2.6.git/hw/ppc/spapr_hcall.c
>>>>>> ===================================================================
>>>>>> --- qemu-dgibson-for-2.6.git.orig/hw/ppc/spapr_hcall.c
>>>>>> +++ qemu-dgibson-for-2.6.git/hw/ppc/spapr_hcall.c
>>>>>> @@ -835,17 +835,8 @@ static target_ulong h_set_mode_resource_
>>>>>> return H_P4;
>>>>>> }
>>>>>>
>>>>>> - switch (mflags) {
>>>>>> - case H_SET_MODE_ADDR_TRANS_NONE:
>>>>>> - prefix = 0;
>>>>>> - break;
>>>>>> - case H_SET_MODE_ADDR_TRANS_0001_8000:
>>>>>> - prefix = 0x18000;
>>>>>> - break;
>>>>>> - case H_SET_MODE_ADDR_TRANS_C000_0000_0000_4000:
>>>>>> - prefix = 0xC000000000004000ULL;
>>>>>> - break;
>>>>>> - default:
>>>>>> + prefix = cpu_ppc_get_excp_prefix(mflags);
>>>>>> + if (prefix == (target_ulong) -1ULL) {
>>>>>> return H_UNSUPPORTED_FLAG;
>>>>>> }
>>>>>>
>>>>>> Index: qemu-dgibson-for-2.6.git/target-ppc/machine.c
>>>>>> ===================================================================
>>>>>> --- qemu-dgibson-for-2.6.git.orig/target-ppc/machine.c
>>>>>> +++ qemu-dgibson-for-2.6.git/target-ppc/machine.c
>>>>>> @@ -156,12 +156,26 @@ static void cpu_pre_save(void *opaque)
>>>>>> }
>>>>>> }
>>>>>>
>>>>>> +
>>>>>> +static int cpu_post_load_excp_prefix(CPUPPCState *env)
>>>>>> +{
>>>>>> + int ail = (env->spr[SPR_LPCR] & LPCR_AIL) >> LPCR_AIL_SHIFT;
>>>>>> + target_ulong prefix = cpu_ppc_get_excp_prefix(ail);
>>>>>> +
>>>>>> + if (prefix == (target_ulong) -1ULL) {
>>>>>> + return -EINVAL;
>>>>>> + }
>>>>>> + env->excp_prefix = prefix;
>>>>>> + return 0;
>>>>>> +}
>>>>>> +
>>>>>> static int cpu_post_load(void *opaque, int version_id)
>>>>>> {
>>>>>> PowerPCCPU *cpu = opaque;
>>>>>> CPUPPCState *env = &cpu->env;
>>>>>> int i;
>>>>>> target_ulong msr;
>>>>>> + int ret = 0;
>>>>>>
>>>>>> /*
>>>>>> * We always ignore the source PVR. The user or management
>>>>>> @@ -201,7 +215,11 @@ static int cpu_post_load(void *opaque, i
>>>>>>
>>>>>> hreg_compute_mem_idx(env);
>>>>>>
>>>>>> - return 0;
>>>>>> + if (env->spr[SPR_LPCR] & LPCR_AIL) {
>>>>>> + ret = cpu_post_load_excp_prefix(env);
>>>>>> + }
>>>>>
>>>>> Why not call this unconditionally? If AIL == 0 it will still do the
>>>>> right thing.
>>>>>
>>>>> Aren't there also circumstances where the exception prefix can depend
>>>>> on the MSR? Do those need to be handled somewhere?
>>>>
>>>> Yes indeed - this was part of my patchset last year to fix up various
>>>> migration issues for the Mac PPC machines (see commit
>>>> 2360b6e84f78d41fa0f76555a947148b73645259).
>>>>
>>>> I agree that having the env->excp_prefix logic split like this isn't a
>>>> particularly great idea. Let's just have a single helper function as in
>>>> the patch above and use that in both places (and in fact with recent
>>>> changes I have a feeling env->excp_prefix is one of the few remaining
>>>> reasons that the above change is still required, but please check this).
>>>
>>> Right.
>>>
>>> So, what I'd really prefer to see here is a single
>>> update_excp_prefix() helper which will set env->excp_prefix based on
>>> whatever registers are relevant (LPCR, MSR and probably the cpu
>>> model). That would be called on incoming migration, and after
>>> updating any of the relevant registers (so from the mtmsr and mtlpcr
>>> emulations). H_SET_MODE should be handled by first updating the LPCR
>>> value, then calling the update helper.
>>>
>>> Cédric, I'm ok if you provide a version of that which only handles
>>> POWER7 and POWER8 (i.e. spapr compatible models for now).
>>
>> Sure.
>>
>> But first, could you please take a look at a reworked patch of Ben who
>> had forseen the issue ? I kept the AIL fix, added some extra for ILE
>> and cleanup H_SET_MODE. Tests under KVM,TCG on LE,BE guests went fine.
>>
>> If you think this is too risky for 2.6, I will work on what you asked for.
>
> Ah, yes this approach does seem more correct. It looks like it leaves
> room for further cleanups to excp_prefix later (such as completely
> removing it in favour of calculating it from reg values at exception
> time).
Yes. There is still a :
vector |= env->excp_prefix;
which is a bit confusing for P7/P8 guests as 'excp_prefix' is not used.
The code section calculating 'vector' in powerpc_excp() could be isolated
in its own routine I guess. We could clarify things there.
> But for now it looks like it will address the migration issue at hand.
>
> I've applied it to ppc-for-2.6.
Tell me if you want a proper resend by mail.
Thanks,
C.
>>
>>> Mark - if
>>> you can supply corrections to that outline for Macintosh era models,
>>> that would be great.
>>>
>>> I do want to get the basic migration problem fixed before 2.6 is
>>> release.
>>
>>
>> Thanks,
>>
>> C.
>>
>> >From bb9d1e06a0ea2132902db724f61c8dc655aa1a96 Mon Sep 17 00:00:00 2001
>> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> Date: Thu, 4 Jun 2015 03:39:19 +1000
>> Subject: [PATCH 18/77] ppc: Rework POWER7 & POWER8 exception model
>>
>> This patch fixes the current AIL implementation for POWER8. The
>> interrupt vector address can be calculated directly from LPCR when the
>> exception is handled. The excp_prefix update becomes useless and we
>> can cleanup the H_SET_MODE hcall.
>>
>> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> [clg: Removed LPES0/1 handling for HV vs. !HV
>> Fixed LPCR_ILE case for POWERPC_EXCP_POWER8 ]
>> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
>> ---
>> hw/ppc/spapr_hcall.c | 16 --------------
>> include/hw/ppc/spapr.h | 5 ----
>> target-ppc/cpu.h | 10 ++++++++
>> target-ppc/excp_helper.c | 49 ++++++++++++++++++++++++++++++++++++++++++--
>> target-ppc/translate_init.c | 2 -
>> 5 files changed, 59 insertions(+), 23 deletions(-)
>>
>> Index: qemu-dgibson-for-2.6.git/target-ppc/cpu.h
>> ===================================================================
>> --- qemu-dgibson-for-2.6.git.orig/target-ppc/cpu.h
>> +++ qemu-dgibson-for-2.6.git/target-ppc/cpu.h
>> @@ -167,6 +167,8 @@ enum powerpc_excp_t {
>> POWERPC_EXCP_970,
>> /* POWER7 exception model */
>> POWERPC_EXCP_POWER7,
>> + /* POWER8 exception model */
>> + POWERPC_EXCP_POWER8,
>> #endif /* defined(TARGET_PPC64) */
>> };
>>
>> @@ -2277,6 +2279,14 @@ enum {
>> HMER_XSCOM_STATUS_LSH = (63 - 23),
>> };
>>
>> +/* Alternate Interrupt Location (AIL) */
>> +enum {
>> + AIL_NONE = 0,
>> + AIL_RESERVED = 1,
>> + AIL_0001_8000 = 2,
>> + AIL_C000_0000_0000_4000 = 3,
>> +};
>> +
>> /*****************************************************************************/
>>
>> static inline target_ulong cpu_read_xer(CPUPPCState *env)
>> Index: qemu-dgibson-for-2.6.git/target-ppc/excp_helper.c
>> ===================================================================
>> --- qemu-dgibson-for-2.6.git.orig/target-ppc/excp_helper.c
>> +++ qemu-dgibson-for-2.6.git/target-ppc/excp_helper.c
>> @@ -77,7 +77,7 @@ static inline void powerpc_excp(PowerPCC
>> CPUPPCState *env = &cpu->env;
>> target_ulong msr, new_msr, vector;
>> int srr0, srr1, asrr0, asrr1;
>> - int lpes0, lpes1, lev;
>> + int lpes0, lpes1, lev, ail;
>>
>> if (0) {
>> /* XXX: find a suitable condition to enable the hypervisor mode */
>> @@ -108,6 +108,25 @@ static inline void powerpc_excp(PowerPCC
>> asrr0 = -1;
>> asrr1 = -1;
>>
>> + /* Exception targetting modifiers
>> + *
>> + * AIL is initialized here but can be cleared by
>> + * selected exceptions
>> + */
>> +#if defined(TARGET_PPC64)
>> + if (excp_model == POWERPC_EXCP_POWER7 ||
>> + excp_model == POWERPC_EXCP_POWER8) {
>> + if (excp_model == POWERPC_EXCP_POWER8) {
>> + ail = (env->spr[SPR_LPCR] & LPCR_AIL) >> LPCR_AIL_SHIFT;
>> + } else {
>> + ail = 0;
>> + }
>> + } else
>> +#endif /* defined(TARGET_PPC64) */
>> + {
>> + ail = 0;
>> + }
>> +
>> switch (excp) {
>> case POWERPC_EXCP_NONE:
>> /* Should never happen */
>> @@ -146,6 +165,7 @@ static inline void powerpc_excp(PowerPCC
>> /* XXX: find a suitable condition to enable the hypervisor mode */
>> new_msr |= (target_ulong)MSR_HVB;
>> }
>> + ail = 0;
>>
>> /* machine check exceptions don't have ME set */
>> new_msr &= ~((target_ulong)1 << MSR_ME);
>> @@ -344,6 +364,7 @@ static inline void powerpc_excp(PowerPCC
>> /* XXX: find a suitable condition to enable the hypervisor mode */
>> new_msr |= (target_ulong)MSR_HVB;
>> }
>> + ail = 0;
>> goto store_next;
>> case POWERPC_EXCP_DSEG: /* Data segment exception */
>> if (lpes1 == 0) {
>> @@ -630,7 +651,8 @@ static inline void powerpc_excp(PowerPCC
>> }
>>
>> #ifdef TARGET_PPC64
>> - if (excp_model == POWERPC_EXCP_POWER7) {
>> + if (excp_model == POWERPC_EXCP_POWER7 ||
>> + excp_model == POWERPC_EXCP_POWER8) {
>> if (env->spr[SPR_LPCR] & LPCR_ILE) {
>> new_msr |= (target_ulong)1 << MSR_LE;
>> }
>> @@ -650,6 +672,29 @@ static inline void powerpc_excp(PowerPCC
>> excp);
>> }
>> vector |= env->excp_prefix;
>> +
>> + /* AIL only works if there is no HV transition and we are running with
>> + * translations enabled
>> + */
>> + if (!((msr >> MSR_IR) & 1) || !((msr >> MSR_DR) & 1)) {
>> + ail = 0;
>> + }
>> + /* Handle AIL */
>> + if (ail) {
>> + new_msr |= (1 << MSR_IR) | (1 << MSR_DR);
>> + switch(ail) {
>> + case AIL_0001_8000:
>> + vector |= 0x18000;
>> + break;
>> + case AIL_C000_0000_0000_4000:
>> + vector |= 0xc000000000004000ull;
>> + break;
>> + default:
>> + cpu_abort(cs, "Invalid AIL combination %d\n", ail);
>> + break;
>> + }
>> + }
>> +
>> #if defined(TARGET_PPC64)
>> if (excp_model == POWERPC_EXCP_BOOKE) {
>> if (env->spr[SPR_BOOKE_EPCR] & EPCR_ICM) {
>> Index: qemu-dgibson-for-2.6.git/target-ppc/translate_init.c
>> ===================================================================
>> --- qemu-dgibson-for-2.6.git.orig/target-ppc/translate_init.c
>> +++ qemu-dgibson-for-2.6.git/target-ppc/translate_init.c
>> @@ -8487,7 +8487,7 @@ POWERPC_FAMILY(POWER8)(ObjectClass *oc,
>> pcc->handle_mmu_fault = ppc_hash64_handle_mmu_fault;
>> pcc->sps = &POWER7_POWER8_sps;
>> #endif
>> - pcc->excp_model = POWERPC_EXCP_POWER7;
>> + pcc->excp_model = POWERPC_EXCP_POWER8;
>> pcc->bus_model = PPC_FLAGS_INPUT_POWER7;
>> pcc->bfd_mach = bfd_mach_ppc64;
>> pcc->flags = POWERPC_FLAG_VRE | POWERPC_FLAG_SE |
>> Index: qemu-dgibson-for-2.6.git/hw/ppc/spapr_hcall.c
>> ===================================================================
>> --- qemu-dgibson-for-2.6.git.orig/hw/ppc/spapr_hcall.c
>> +++ qemu-dgibson-for-2.6.git/hw/ppc/spapr_hcall.c
>> @@ -823,7 +823,6 @@ static target_ulong h_set_mode_resource_
>> {
>> CPUState *cs;
>> PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
>> - target_ulong prefix;
>>
>> if (!(pcc->insns_flags2 & PPC2_ISA207S)) {
>> return H_P2;
>> @@ -835,25 +834,12 @@ static target_ulong h_set_mode_resource_
>> return H_P4;
>> }
>>
>> - switch (mflags) {
>> - case H_SET_MODE_ADDR_TRANS_NONE:
>> - prefix = 0;
>> - break;
>> - case H_SET_MODE_ADDR_TRANS_0001_8000:
>> - prefix = 0x18000;
>> - break;
>> - case H_SET_MODE_ADDR_TRANS_C000_0000_0000_4000:
>> - prefix = 0xC000000000004000ULL;
>> - break;
>> - default:
>> + if (mflags == AIL_RESERVED) {
>> return H_UNSUPPORTED_FLAG;
>> }
>>
>> CPU_FOREACH(cs) {
>> - CPUPPCState *env = &POWERPC_CPU(cpu)->env;
>> -
>> set_spr(cs, SPR_LPCR, mflags << LPCR_AIL_SHIFT, LPCR_AIL);
>> - env->excp_prefix = prefix;
>> }
>>
>> return H_SUCCESS;
>> Index: qemu-dgibson-for-2.6.git/include/hw/ppc/spapr.h
>> ===================================================================
>> --- qemu-dgibson-for-2.6.git.orig/include/hw/ppc/spapr.h
>> +++ qemu-dgibson-for-2.6.git/include/hw/ppc/spapr.h
>> @@ -204,11 +204,6 @@ struct sPAPRMachineState {
>> #define H_SET_MODE_ENDIAN_BIG 0
>> #define H_SET_MODE_ENDIAN_LITTLE 1
>>
>> -/* Flags for H_SET_MODE_RESOURCE_ADDR_TRANS_MODE */
>> -#define H_SET_MODE_ADDR_TRANS_NONE 0
>> -#define H_SET_MODE_ADDR_TRANS_0001_8000 2
>> -#define H_SET_MODE_ADDR_TRANS_C000_0000_0000_4000 3
>> -
>> /* VASI States */
>> #define H_VASI_INVALID 0
>> #define H_VASI_ENABLED 1
>>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [PATCH v2] spapr: compute interrupt vector address from LPCR
2016-04-04 14:47 ` Cédric Le Goater
@ 2016-04-05 0:54 ` David Gibson
0 siblings, 0 replies; 15+ messages in thread
From: David Gibson @ 2016-04-05 0:54 UTC (permalink / raw)
To: Cédric Le Goater
Cc: qemu-ppc, Thomas Huth, Mark Cave-Ayland, qemu-devel, Greg Kurz
[-- Attachment #1: Type: text/plain, Size: 7098 bytes --]
On Mon, Apr 04, 2016 at 04:47:53PM +0200, Cédric Le Goater wrote:
> On 04/04/2016 06:16 AM, David Gibson wrote:
> > On Sun, Apr 03, 2016 at 07:57:50PM +0200, Cédric Le Goater wrote:
> >> On 04/01/2016 04:43 AM, David Gibson wrote:
> >>> On Thu, Mar 31, 2016 at 08:13:09AM +0100, Mark Cave-Ayland wrote:
> >>>> On 31/03/16 05:55, David Gibson wrote:
> >>>>
> >>>>> On Wed, Mar 30, 2016 at 05:38:34PM +0200, Cédric Le Goater wrote:
> >>>>>> This address is changed by the linux kernel using the H_SET_MODE hcall
> >>>>>> and needs to be restored when migrating a spapr VM running in
> >>>>>> TCG. This can be done using the AIL bits from the LPCR register.
> >>>>>>
> >>>>>> The patch introduces a helper routine cpu_ppc_get_excp_prefix() which
> >>>>>> returns the effective address offset of the interrupt handler
> >>>>>> depending on the LPCR_AIL bits. The same helper can be used in the
> >>>>>> H_SET_MODE hcall, which lets us remove the H_SET_MODE_ADDR_TRANS_*
> >>>>>> defines.
> >>>>>>
> >>>>>> Signed-off-by: Cédric Le Goater <clg@fr.ibm.com>
> >>>>>
> >>>>> I've applied this (with Greg's minor amendments) to ppc-for-2.6),
> >>>>> since it certainly improves behaviour, although I have a couple of
> >>>>> queries:
> >>>>>
> >>>>>> ---
> >>>>>>
> >>>>>> Changes since v1:
> >>>>>>
> >>>>>> - moved helper routine under target-ppc/
> >>>>>> - moved the restore of excp_prefix under cpu_post_load()
> >>>>>>
> >>>>>> hw/ppc/spapr_hcall.c | 13 ++-----------
> >>>>>> include/hw/ppc/spapr.h | 5 -----
> >>>>>> target-ppc/cpu.h | 9 +++++++++
> >>>>>> target-ppc/machine.c | 20 +++++++++++++++++++-
> >>>>>> target-ppc/translate_init.c | 14 ++++++++++++++
> >>>>>> 5 files changed, 44 insertions(+), 17 deletions(-)
> >>>>>>
> >>>>>> Index: qemu-dgibson-for-2.6.git/hw/ppc/spapr_hcall.c
> >>>>>> ===================================================================
> >>>>>> --- qemu-dgibson-for-2.6.git.orig/hw/ppc/spapr_hcall.c
> >>>>>> +++ qemu-dgibson-for-2.6.git/hw/ppc/spapr_hcall.c
> >>>>>> @@ -835,17 +835,8 @@ static target_ulong h_set_mode_resource_
> >>>>>> return H_P4;
> >>>>>> }
> >>>>>>
> >>>>>> - switch (mflags) {
> >>>>>> - case H_SET_MODE_ADDR_TRANS_NONE:
> >>>>>> - prefix = 0;
> >>>>>> - break;
> >>>>>> - case H_SET_MODE_ADDR_TRANS_0001_8000:
> >>>>>> - prefix = 0x18000;
> >>>>>> - break;
> >>>>>> - case H_SET_MODE_ADDR_TRANS_C000_0000_0000_4000:
> >>>>>> - prefix = 0xC000000000004000ULL;
> >>>>>> - break;
> >>>>>> - default:
> >>>>>> + prefix = cpu_ppc_get_excp_prefix(mflags);
> >>>>>> + if (prefix == (target_ulong) -1ULL) {
> >>>>>> return H_UNSUPPORTED_FLAG;
> >>>>>> }
> >>>>>>
> >>>>>> Index: qemu-dgibson-for-2.6.git/target-ppc/machine.c
> >>>>>> ===================================================================
> >>>>>> --- qemu-dgibson-for-2.6.git.orig/target-ppc/machine.c
> >>>>>> +++ qemu-dgibson-for-2.6.git/target-ppc/machine.c
> >>>>>> @@ -156,12 +156,26 @@ static void cpu_pre_save(void *opaque)
> >>>>>> }
> >>>>>> }
> >>>>>>
> >>>>>> +
> >>>>>> +static int cpu_post_load_excp_prefix(CPUPPCState *env)
> >>>>>> +{
> >>>>>> + int ail = (env->spr[SPR_LPCR] & LPCR_AIL) >> LPCR_AIL_SHIFT;
> >>>>>> + target_ulong prefix = cpu_ppc_get_excp_prefix(ail);
> >>>>>> +
> >>>>>> + if (prefix == (target_ulong) -1ULL) {
> >>>>>> + return -EINVAL;
> >>>>>> + }
> >>>>>> + env->excp_prefix = prefix;
> >>>>>> + return 0;
> >>>>>> +}
> >>>>>> +
> >>>>>> static int cpu_post_load(void *opaque, int version_id)
> >>>>>> {
> >>>>>> PowerPCCPU *cpu = opaque;
> >>>>>> CPUPPCState *env = &cpu->env;
> >>>>>> int i;
> >>>>>> target_ulong msr;
> >>>>>> + int ret = 0;
> >>>>>>
> >>>>>> /*
> >>>>>> * We always ignore the source PVR. The user or management
> >>>>>> @@ -201,7 +215,11 @@ static int cpu_post_load(void *opaque, i
> >>>>>>
> >>>>>> hreg_compute_mem_idx(env);
> >>>>>>
> >>>>>> - return 0;
> >>>>>> + if (env->spr[SPR_LPCR] & LPCR_AIL) {
> >>>>>> + ret = cpu_post_load_excp_prefix(env);
> >>>>>> + }
> >>>>>
> >>>>> Why not call this unconditionally? If AIL == 0 it will still do the
> >>>>> right thing.
> >>>>>
> >>>>> Aren't there also circumstances where the exception prefix can depend
> >>>>> on the MSR? Do those need to be handled somewhere?
> >>>>
> >>>> Yes indeed - this was part of my patchset last year to fix up various
> >>>> migration issues for the Mac PPC machines (see commit
> >>>> 2360b6e84f78d41fa0f76555a947148b73645259).
> >>>>
> >>>> I agree that having the env->excp_prefix logic split like this isn't a
> >>>> particularly great idea. Let's just have a single helper function as in
> >>>> the patch above and use that in both places (and in fact with recent
> >>>> changes I have a feeling env->excp_prefix is one of the few remaining
> >>>> reasons that the above change is still required, but please check this).
> >>>
> >>> Right.
> >>>
> >>> So, what I'd really prefer to see here is a single
> >>> update_excp_prefix() helper which will set env->excp_prefix based on
> >>> whatever registers are relevant (LPCR, MSR and probably the cpu
> >>> model). That would be called on incoming migration, and after
> >>> updating any of the relevant registers (so from the mtmsr and mtlpcr
> >>> emulations). H_SET_MODE should be handled by first updating the LPCR
> >>> value, then calling the update helper.
> >>>
> >>> Cédric, I'm ok if you provide a version of that which only handles
> >>> POWER7 and POWER8 (i.e. spapr compatible models for now).
> >>
> >> Sure.
> >>
> >> But first, could you please take a look at a reworked patch of Ben who
> >> had forseen the issue ? I kept the AIL fix, added some extra for ILE
> >> and cleanup H_SET_MODE. Tests under KVM,TCG on LE,BE guests went fine.
> >>
> >> If you think this is too risky for 2.6, I will work on what you asked for.
> >
> > Ah, yes this approach does seem more correct. It looks like it leaves
> > room for further cleanups to excp_prefix later (such as completely
> > removing it in favour of calculating it from reg values at exception
> > time).
>
> Yes. There is still a :
>
> vector |= env->excp_prefix;
>
> which is a bit confusing for P7/P8 guests as 'excp_prefix' is not used.
> The code section calculating 'vector' in powerpc_excp() could be isolated
> in its own routine I guess. We could clarify things there.
Right.
> > But for now it looks like it will address the migration issue at hand.
> >
> > I've applied it to ppc-for-2.6.
>
> Tell me if you want a proper resend by mail.
No, I think I have it adequately cleaned up myself.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2016-04-05 1:03 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-30 15:38 [Qemu-devel] [PATCH v2] spapr: compute interrupt vector address from LPCR Cédric Le Goater
2016-03-30 17:01 ` Greg Kurz
2016-03-30 17:15 ` [Qemu-devel] [Qemu-ppc] " Greg Kurz
2016-03-30 17:12 ` [Qemu-devel] " Greg Kurz
2016-03-31 8:20 ` Cédric Le Goater
2016-03-31 9:33 ` Cédric Le Goater
2016-03-31 4:55 ` David Gibson
2016-03-31 7:13 ` [Qemu-devel] [Qemu-ppc] " Mark Cave-Ayland
2016-04-01 2:43 ` David Gibson
2016-04-03 17:57 ` Cédric Le Goater
2016-04-04 4:16 ` David Gibson
2016-04-04 14:47 ` Cédric Le Goater
2016-04-05 0:54 ` David Gibson
2016-03-31 8:16 ` [Qemu-devel] " Cédric Le Goater
2016-04-01 3:34 ` [Qemu-devel] [Qemu-ppc] " David Gibson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.