* [Qemu-devel] [PULL 01/12] ppc: Remove MMU_MODEn_SUFFIX definitions
2016-05-31 0:41 [Qemu-devel] [PULL 00/12] ppc-for-2.7 queue 20160531 David Gibson
@ 2016-05-31 0:41 ` David Gibson
2016-05-31 0:41 ` [Qemu-devel] [PULL 02/12] ppc: Use split I/D mmu modes to avoid flushes on interrupts David Gibson
` (11 subsequent siblings)
12 siblings, 0 replies; 31+ messages in thread
From: David Gibson @ 2016-05-31 0:41 UTC (permalink / raw)
To: peter.maydell
Cc: agraf, pbonzini, bharata.rao, benh, qemu-ppc, qemu-devel,
David Gibson
From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
We don't use the resulting accessors and this gets in the way of
the split I/D TLB work.
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
target-ppc/cpu.h | 3 ---
1 file changed, 3 deletions(-)
diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
index cd33539..02e71ea 100644
--- a/target-ppc/cpu.h
+++ b/target-ppc/cpu.h
@@ -1242,9 +1242,6 @@ int ppc_dcr_write (ppc_dcr_t *dcr_env, int dcrn, uint32_t val);
#define cpu_list ppc_cpu_list
/* MMU modes definitions */
-#define MMU_MODE0_SUFFIX _user
-#define MMU_MODE1_SUFFIX _kernel
-#define MMU_MODE2_SUFFIX _hypv
#define MMU_USER_IDX 0
static inline int cpu_mmu_index (CPUPPCState *env, bool ifetch)
{
--
2.5.5
^ permalink raw reply related [flat|nested] 31+ messages in thread* [Qemu-devel] [PULL 02/12] ppc: Use split I/D mmu modes to avoid flushes on interrupts
2016-05-31 0:41 [Qemu-devel] [PULL 00/12] ppc-for-2.7 queue 20160531 David Gibson
2016-05-31 0:41 ` [Qemu-devel] [PULL 01/12] ppc: Remove MMU_MODEn_SUFFIX definitions David Gibson
@ 2016-05-31 0:41 ` David Gibson
2016-06-01 19:33 ` [Qemu-devel] [Qemu-ppc] " Mark Cave-Ayland
2016-05-31 0:41 ` [Qemu-devel] [PULL 03/12] ppc: Do some batching of TCG tlb flushes David Gibson
` (10 subsequent siblings)
12 siblings, 1 reply; 31+ messages in thread
From: David Gibson @ 2016-05-31 0:41 UTC (permalink / raw)
To: peter.maydell
Cc: agraf, pbonzini, bharata.rao, benh, qemu-ppc, qemu-devel,
David Gibson
From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
We rework the way the MMU indices are calculated, providing separate
indices for I and D side based on MSR:IR and MSR:DR respectively,
and thus no longer need to flush the TLB on context changes. This also
adds correct support for HV as a separate address space.
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
target-ppc/cpu.h | 11 +++++++---
target-ppc/excp_helper.c | 11 ----------
target-ppc/helper_regs.h | 54 +++++++++++++++++++++++++++++++++++++++++-------
target-ppc/machine.c | 5 ++++-
target-ppc/translate.c | 7 ++++---
5 files changed, 63 insertions(+), 25 deletions(-)
diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
index 02e71ea..2c8c8c0 100644
--- a/target-ppc/cpu.h
+++ b/target-ppc/cpu.h
@@ -359,6 +359,8 @@ struct ppc_slb_t {
#define MSR_EP 6 /* Exception prefix on 601 */
#define MSR_IR 5 /* Instruction relocate */
#define MSR_DR 4 /* Data relocate */
+#define MSR_IS 5 /* Instruction address space (BookE) */
+#define MSR_DS 4 /* Data address space (BookE) */
#define MSR_PE 3 /* Protection enable on 403 */
#define MSR_PX 2 /* Protection exclusive on 403 x */
#define MSR_PMM 2 /* Performance monitor mark on POWER x */
@@ -410,6 +412,8 @@ struct ppc_slb_t {
#define msr_ep ((env->msr >> MSR_EP) & 1)
#define msr_ir ((env->msr >> MSR_IR) & 1)
#define msr_dr ((env->msr >> MSR_DR) & 1)
+#define msr_is ((env->msr >> MSR_IS) & 1)
+#define msr_ds ((env->msr >> MSR_DS) & 1)
#define msr_pe ((env->msr >> MSR_PE) & 1)
#define msr_px ((env->msr >> MSR_PX) & 1)
#define msr_pmm ((env->msr >> MSR_PMM) & 1)
@@ -889,7 +893,7 @@ struct ppc_segment_page_sizes {
/*****************************************************************************/
/* The whole PowerPC CPU context */
-#define NB_MMU_MODES 3
+#define NB_MMU_MODES 8
#define PPC_CPU_OPCODES_LEN 0x40
#define PPC_CPU_INDIRECT_OPCODES_LEN 0x20
@@ -1053,7 +1057,8 @@ struct CPUPPCState {
/* Those resources are used only in QEMU core */
target_ulong hflags; /* hflags is a MSR & HFLAGS_MASK */
target_ulong hflags_nmsr; /* specific hflags, not coming from MSR */
- int mmu_idx; /* precomputed MMU index to speed up mem accesses */
+ int immu_idx; /* precomputed MMU index to speed up insn access */
+ int dmmu_idx; /* precomputed MMU index to speed up data accesses */
/* Power management */
int (*check_pow)(CPUPPCState *env);
@@ -1245,7 +1250,7 @@ int ppc_dcr_write (ppc_dcr_t *dcr_env, int dcrn, uint32_t val);
#define MMU_USER_IDX 0
static inline int cpu_mmu_index (CPUPPCState *env, bool ifetch)
{
- return env->mmu_idx;
+ return ifetch ? env->immu_idx : env->dmmu_idx;
}
#include "exec/cpu-all.h"
diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c
index 288903e..ba3caec 100644
--- a/target-ppc/excp_helper.c
+++ b/target-ppc/excp_helper.c
@@ -646,9 +646,6 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
if (env->spr[SPR_LPCR] & LPCR_AIL) {
new_msr |= (1 << MSR_IR) | (1 << MSR_DR);
- } else if (msr & ((1 << MSR_IR) | (1 << MSR_DR))) {
- /* If we disactivated any translation, flush TLBs */
- tlb_flush(cs, 1);
}
#ifdef TARGET_PPC64
@@ -721,14 +718,6 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
/* Reset exception state */
cs->exception_index = POWERPC_EXCP_NONE;
env->error_code = 0;
-
- if ((env->mmu_model == POWERPC_MMU_BOOKE) ||
- (env->mmu_model == POWERPC_MMU_BOOKE206)) {
- /* XXX: The BookE changes address space when switching modes,
- we should probably implement that as different MMU indexes,
- but for the moment we do it the slow way and flush all. */
- tlb_flush(cs, 1);
- }
}
void ppc_cpu_do_interrupt(CPUState *cs)
diff --git a/target-ppc/helper_regs.h b/target-ppc/helper_regs.h
index 271fddf..f7edd5b 100644
--- a/target-ppc/helper_regs.h
+++ b/target-ppc/helper_regs.h
@@ -41,11 +41,50 @@ static inline void hreg_swap_gpr_tgpr(CPUPPCState *env)
static inline void hreg_compute_mem_idx(CPUPPCState *env)
{
- /* Precompute MMU index */
- if (msr_pr == 0 && msr_hv != 0) {
- env->mmu_idx = 2;
+ /* This is our encoding for server processors
+ *
+ * 0 = Guest User space virtual mode
+ * 1 = Guest Kernel space virtual mode
+ * 2 = Guest Kernel space real mode
+ * 3 = HV User space virtual mode
+ * 4 = HV Kernel space virtual mode
+ * 5 = HV Kernel space real mode
+ *
+ * The combination PR=1 IR&DR=0 is invalid, we will treat
+ * it as IR=DR=1
+ *
+ * For BookE, we need 8 MMU modes as follow:
+ *
+ * 0 = AS 0 HV User space
+ * 1 = AS 0 HV Kernel space
+ * 2 = AS 1 HV User space
+ * 3 = AS 1 HV Kernel space
+ * 4 = AS 0 Guest User space
+ * 5 = AS 0 Guest Kernel space
+ * 6 = AS 1 Guest User space
+ * 7 = AS 1 Guest Kernel space
+ */
+ if (env->mmu_model & POWERPC_MMU_BOOKE) {
+ env->immu_idx = env->dmmu_idx = msr_pr ? 0 : 1;
+ env->immu_idx += msr_is ? 2 : 0;
+ env->dmmu_idx += msr_ds ? 2 : 0;
+ env->immu_idx += msr_gs ? 4 : 0;
+ env->dmmu_idx += msr_gs ? 4 : 0;
} else {
- env->mmu_idx = 1 - msr_pr;
+ /* First calucalte a base value independent of HV */
+ if (msr_pr != 0) {
+ /* User space, ignore IR and DR */
+ env->immu_idx = env->dmmu_idx = 0;
+ } else {
+ /* Kernel, setup a base I/D value */
+ env->immu_idx = msr_ir ? 1 : 2;
+ env->dmmu_idx = msr_dr ? 1 : 2;
+ }
+ /* Then offset it for HV */
+ if (msr_hv) {
+ env->immu_idx += 3;
+ env->dmmu_idx += 3;
+ }
}
}
@@ -82,9 +121,10 @@ static inline int hreg_store_msr(CPUPPCState *env, target_ulong value,
}
if (((value >> MSR_IR) & 1) != msr_ir ||
((value >> MSR_DR) & 1) != msr_dr) {
- /* Flush all tlb when changing translation mode */
- tlb_flush(cs, 1);
- excp = POWERPC_EXCP_NONE;
+ cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
+ }
+ if ((env->mmu_model & POWERPC_MMU_BOOKE) &&
+ ((value >> MSR_GS) & 1) != msr_gs) {
cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
}
if (unlikely((env->flags & POWERPC_FLAG_TGPR) &&
diff --git a/target-ppc/machine.c b/target-ppc/machine.c
index f6c7256..4820f22 100644
--- a/target-ppc/machine.c
+++ b/target-ppc/machine.c
@@ -97,9 +97,12 @@ static int cpu_load_old(QEMUFile *f, void *opaque, int version_id)
qemu_get_betls(f, &env->nip);
qemu_get_betls(f, &env->hflags);
qemu_get_betls(f, &env->hflags_nmsr);
- qemu_get_sbe32s(f, &env->mmu_idx);
+ qemu_get_sbe32(f); /* Discard unused mmu_idx */
qemu_get_sbe32(f); /* Discard unused power_mode */
+ /* Recompute mmu indices */
+ hreg_compute_mem_idx(env);
+
return 0;
}
diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index f5ceae5..b757634 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -11220,8 +11220,9 @@ void ppc_cpu_dump_state(CPUState *cs, FILE *f, fprintf_function cpu_fprintf,
env->nip, env->lr, env->ctr, cpu_read_xer(env),
cs->cpu_index);
cpu_fprintf(f, "MSR " TARGET_FMT_lx " HID0 " TARGET_FMT_lx " HF "
- TARGET_FMT_lx " idx %d\n", env->msr, env->spr[SPR_HID0],
- env->hflags, env->mmu_idx);
+ TARGET_FMT_lx " iidx %d didx %d\n",
+ env->msr, env->spr[SPR_HID0],
+ env->hflags, env->immu_idx, env->dmmu_idx);
#if !defined(NO_TIMER_DUMP)
cpu_fprintf(f, "TB %08" PRIu32 " %08" PRIu64
#if !defined(CONFIG_USER_ONLY)
@@ -11428,7 +11429,7 @@ void gen_intermediate_code(CPUPPCState *env, struct TranslationBlock *tb)
ctx.spr_cb = env->spr_cb;
ctx.pr = msr_pr;
ctx.hv = !msr_pr && msr_hv;
- ctx.mem_idx = env->mmu_idx;
+ ctx.mem_idx = env->dmmu_idx;
ctx.insns_flags = env->insns_flags;
ctx.insns_flags2 = env->insns_flags2;
ctx.access_type = -1;
--
2.5.5
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [Qemu-ppc] [PULL 02/12] ppc: Use split I/D mmu modes to avoid flushes on interrupts
2016-05-31 0:41 ` [Qemu-devel] [PULL 02/12] ppc: Use split I/D mmu modes to avoid flushes on interrupts David Gibson
@ 2016-06-01 19:33 ` Mark Cave-Ayland
2016-06-02 3:15 ` David Gibson
0 siblings, 1 reply; 31+ messages in thread
From: Mark Cave-Ayland @ 2016-06-01 19:33 UTC (permalink / raw)
To: David Gibson, peter.maydell; +Cc: qemu-devel, qemu-ppc, bharata.rao, pbonzini
On 31/05/16 01:41, David Gibson wrote:
> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>
> We rework the way the MMU indices are calculated, providing separate
> indices for I and D side based on MSR:IR and MSR:DR respectively,
> and thus no longer need to flush the TLB on context changes. This also
> adds correct support for HV as a separate address space.
>
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> target-ppc/cpu.h | 11 +++++++---
> target-ppc/excp_helper.c | 11 ----------
> target-ppc/helper_regs.h | 54 +++++++++++++++++++++++++++++++++++++++++-------
> target-ppc/machine.c | 5 ++++-
> target-ppc/translate.c | 7 ++++---
> 5 files changed, 63 insertions(+), 25 deletions(-)
>
> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> index 02e71ea..2c8c8c0 100644
> --- a/target-ppc/cpu.h
> +++ b/target-ppc/cpu.h
> @@ -359,6 +359,8 @@ struct ppc_slb_t {
> #define MSR_EP 6 /* Exception prefix on 601 */
> #define MSR_IR 5 /* Instruction relocate */
> #define MSR_DR 4 /* Data relocate */
> +#define MSR_IS 5 /* Instruction address space (BookE) */
> +#define MSR_DS 4 /* Data address space (BookE) */
> #define MSR_PE 3 /* Protection enable on 403 */
> #define MSR_PX 2 /* Protection exclusive on 403 x */
> #define MSR_PMM 2 /* Performance monitor mark on POWER x */
> @@ -410,6 +412,8 @@ struct ppc_slb_t {
> #define msr_ep ((env->msr >> MSR_EP) & 1)
> #define msr_ir ((env->msr >> MSR_IR) & 1)
> #define msr_dr ((env->msr >> MSR_DR) & 1)
> +#define msr_is ((env->msr >> MSR_IS) & 1)
> +#define msr_ds ((env->msr >> MSR_DS) & 1)
> #define msr_pe ((env->msr >> MSR_PE) & 1)
> #define msr_px ((env->msr >> MSR_PX) & 1)
> #define msr_pmm ((env->msr >> MSR_PMM) & 1)
> @@ -889,7 +893,7 @@ struct ppc_segment_page_sizes {
>
> /*****************************************************************************/
> /* The whole PowerPC CPU context */
> -#define NB_MMU_MODES 3
> +#define NB_MMU_MODES 8
>
> #define PPC_CPU_OPCODES_LEN 0x40
> #define PPC_CPU_INDIRECT_OPCODES_LEN 0x20
> @@ -1053,7 +1057,8 @@ struct CPUPPCState {
> /* Those resources are used only in QEMU core */
> target_ulong hflags; /* hflags is a MSR & HFLAGS_MASK */
> target_ulong hflags_nmsr; /* specific hflags, not coming from MSR */
> - int mmu_idx; /* precomputed MMU index to speed up mem accesses */
> + int immu_idx; /* precomputed MMU index to speed up insn access */
> + int dmmu_idx; /* precomputed MMU index to speed up data accesses */
>
> /* Power management */
> int (*check_pow)(CPUPPCState *env);
> @@ -1245,7 +1250,7 @@ int ppc_dcr_write (ppc_dcr_t *dcr_env, int dcrn, uint32_t val);
> #define MMU_USER_IDX 0
> static inline int cpu_mmu_index (CPUPPCState *env, bool ifetch)
> {
> - return env->mmu_idx;
> + return ifetch ? env->immu_idx : env->dmmu_idx;
> }
>
> #include "exec/cpu-all.h"
> diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c
> index 288903e..ba3caec 100644
> --- a/target-ppc/excp_helper.c
> +++ b/target-ppc/excp_helper.c
> @@ -646,9 +646,6 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>
> if (env->spr[SPR_LPCR] & LPCR_AIL) {
> new_msr |= (1 << MSR_IR) | (1 << MSR_DR);
> - } else if (msr & ((1 << MSR_IR) | (1 << MSR_DR))) {
> - /* If we disactivated any translation, flush TLBs */
> - tlb_flush(cs, 1);
> }
>
> #ifdef TARGET_PPC64
> @@ -721,14 +718,6 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
> /* Reset exception state */
> cs->exception_index = POWERPC_EXCP_NONE;
> env->error_code = 0;
> -
> - if ((env->mmu_model == POWERPC_MMU_BOOKE) ||
> - (env->mmu_model == POWERPC_MMU_BOOKE206)) {
> - /* XXX: The BookE changes address space when switching modes,
> - we should probably implement that as different MMU indexes,
> - but for the moment we do it the slow way and flush all. */
> - tlb_flush(cs, 1);
> - }
> }
>
> void ppc_cpu_do_interrupt(CPUState *cs)
> diff --git a/target-ppc/helper_regs.h b/target-ppc/helper_regs.h
> index 271fddf..f7edd5b 100644
> --- a/target-ppc/helper_regs.h
> +++ b/target-ppc/helper_regs.h
> @@ -41,11 +41,50 @@ static inline void hreg_swap_gpr_tgpr(CPUPPCState *env)
>
> static inline void hreg_compute_mem_idx(CPUPPCState *env)
> {
> - /* Precompute MMU index */
> - if (msr_pr == 0 && msr_hv != 0) {
> - env->mmu_idx = 2;
> + /* This is our encoding for server processors
> + *
> + * 0 = Guest User space virtual mode
> + * 1 = Guest Kernel space virtual mode
> + * 2 = Guest Kernel space real mode
> + * 3 = HV User space virtual mode
> + * 4 = HV Kernel space virtual mode
> + * 5 = HV Kernel space real mode
> + *
> + * The combination PR=1 IR&DR=0 is invalid, we will treat
> + * it as IR=DR=1
> + *
> + * For BookE, we need 8 MMU modes as follow:
> + *
> + * 0 = AS 0 HV User space
> + * 1 = AS 0 HV Kernel space
> + * 2 = AS 1 HV User space
> + * 3 = AS 1 HV Kernel space
> + * 4 = AS 0 Guest User space
> + * 5 = AS 0 Guest Kernel space
> + * 6 = AS 1 Guest User space
> + * 7 = AS 1 Guest Kernel space
> + */
> + if (env->mmu_model & POWERPC_MMU_BOOKE) {
> + env->immu_idx = env->dmmu_idx = msr_pr ? 0 : 1;
> + env->immu_idx += msr_is ? 2 : 0;
> + env->dmmu_idx += msr_ds ? 2 : 0;
> + env->immu_idx += msr_gs ? 4 : 0;
> + env->dmmu_idx += msr_gs ? 4 : 0;
> } else {
> - env->mmu_idx = 1 - msr_pr;
> + /* First calucalte a base value independent of HV */
> + if (msr_pr != 0) {
> + /* User space, ignore IR and DR */
> + env->immu_idx = env->dmmu_idx = 0;
> + } else {
> + /* Kernel, setup a base I/D value */
> + env->immu_idx = msr_ir ? 1 : 2;
> + env->dmmu_idx = msr_dr ? 1 : 2;
> + }
> + /* Then offset it for HV */
> + if (msr_hv) {
> + env->immu_idx += 3;
> + env->dmmu_idx += 3;
> + }
> }
> }
>
> @@ -82,9 +121,10 @@ static inline int hreg_store_msr(CPUPPCState *env, target_ulong value,
> }
> if (((value >> MSR_IR) & 1) != msr_ir ||
> ((value >> MSR_DR) & 1) != msr_dr) {
> - /* Flush all tlb when changing translation mode */
> - tlb_flush(cs, 1);
> - excp = POWERPC_EXCP_NONE;
> + cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
> + }
> + if ((env->mmu_model & POWERPC_MMU_BOOKE) &&
> + ((value >> MSR_GS) & 1) != msr_gs) {
> cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
> }
> if (unlikely((env->flags & POWERPC_FLAG_TGPR) &&
> diff --git a/target-ppc/machine.c b/target-ppc/machine.c
> index f6c7256..4820f22 100644
> --- a/target-ppc/machine.c
> +++ b/target-ppc/machine.c
> @@ -97,9 +97,12 @@ static int cpu_load_old(QEMUFile *f, void *opaque, int version_id)
> qemu_get_betls(f, &env->nip);
> qemu_get_betls(f, &env->hflags);
> qemu_get_betls(f, &env->hflags_nmsr);
> - qemu_get_sbe32s(f, &env->mmu_idx);
> + qemu_get_sbe32(f); /* Discard unused mmu_idx */
> qemu_get_sbe32(f); /* Discard unused power_mode */
>
> + /* Recompute mmu indices */
> + hreg_compute_mem_idx(env);
> +
> return 0;
> }
>
> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
> index f5ceae5..b757634 100644
> --- a/target-ppc/translate.c
> +++ b/target-ppc/translate.c
> @@ -11220,8 +11220,9 @@ void ppc_cpu_dump_state(CPUState *cs, FILE *f, fprintf_function cpu_fprintf,
> env->nip, env->lr, env->ctr, cpu_read_xer(env),
> cs->cpu_index);
> cpu_fprintf(f, "MSR " TARGET_FMT_lx " HID0 " TARGET_FMT_lx " HF "
> - TARGET_FMT_lx " idx %d\n", env->msr, env->spr[SPR_HID0],
> - env->hflags, env->mmu_idx);
> + TARGET_FMT_lx " iidx %d didx %d\n",
> + env->msr, env->spr[SPR_HID0],
> + env->hflags, env->immu_idx, env->dmmu_idx);
> #if !defined(NO_TIMER_DUMP)
> cpu_fprintf(f, "TB %08" PRIu32 " %08" PRIu64
> #if !defined(CONFIG_USER_ONLY)
> @@ -11428,7 +11429,7 @@ void gen_intermediate_code(CPUPPCState *env, struct TranslationBlock *tb)
> ctx.spr_cb = env->spr_cb;
> ctx.pr = msr_pr;
> ctx.hv = !msr_pr && msr_hv;
> - ctx.mem_idx = env->mmu_idx;
> + ctx.mem_idx = env->dmmu_idx;
> ctx.insns_flags = env->insns_flags;
> ctx.insns_flags2 = env->insns_flags2;
> ctx.access_type = -1;
Apologies to have to report another regression, however this patch
causes a kernel panic in Darwin PPC under TCG:
./qemu-system-ppc -cdrom darwinppc-602.iso -boot d
The panic backtrace on-screen points towards one of the SCSI modules, so
perhaps this patch doesn't handle DMA correctly when trying to access
the CDROM?
ATB,
Mark.
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [Qemu-ppc] [PULL 02/12] ppc: Use split I/D mmu modes to avoid flushes on interrupts
2016-06-01 19:33 ` [Qemu-devel] [Qemu-ppc] " Mark Cave-Ayland
@ 2016-06-02 3:15 ` David Gibson
2016-06-02 5:32 ` Mark Cave-Ayland
0 siblings, 1 reply; 31+ messages in thread
From: David Gibson @ 2016-06-02 3:15 UTC (permalink / raw)
To: Mark Cave-Ayland
Cc: peter.maydell, qemu-devel, qemu-ppc, bharata.rao, pbonzini
[-- Attachment #1: Type: text/plain, Size: 10463 bytes --]
On Wed, Jun 01, 2016 at 08:33:30PM +0100, Mark Cave-Ayland wrote:
> On 31/05/16 01:41, David Gibson wrote:
>
> > From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> >
> > We rework the way the MMU indices are calculated, providing separate
> > indices for I and D side based on MSR:IR and MSR:DR respectively,
> > and thus no longer need to flush the TLB on context changes. This also
> > adds correct support for HV as a separate address space.
> >
> > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> > target-ppc/cpu.h | 11 +++++++---
> > target-ppc/excp_helper.c | 11 ----------
> > target-ppc/helper_regs.h | 54 +++++++++++++++++++++++++++++++++++++++++-------
> > target-ppc/machine.c | 5 ++++-
> > target-ppc/translate.c | 7 ++++---
> > 5 files changed, 63 insertions(+), 25 deletions(-)
> >
> > diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> > index 02e71ea..2c8c8c0 100644
> > --- a/target-ppc/cpu.h
> > +++ b/target-ppc/cpu.h
> > @@ -359,6 +359,8 @@ struct ppc_slb_t {
> > #define MSR_EP 6 /* Exception prefix on 601 */
> > #define MSR_IR 5 /* Instruction relocate */
> > #define MSR_DR 4 /* Data relocate */
> > +#define MSR_IS 5 /* Instruction address space (BookE) */
> > +#define MSR_DS 4 /* Data address space (BookE) */
> > #define MSR_PE 3 /* Protection enable on 403 */
> > #define MSR_PX 2 /* Protection exclusive on 403 x */
> > #define MSR_PMM 2 /* Performance monitor mark on POWER x */
> > @@ -410,6 +412,8 @@ struct ppc_slb_t {
> > #define msr_ep ((env->msr >> MSR_EP) & 1)
> > #define msr_ir ((env->msr >> MSR_IR) & 1)
> > #define msr_dr ((env->msr >> MSR_DR) & 1)
> > +#define msr_is ((env->msr >> MSR_IS) & 1)
> > +#define msr_ds ((env->msr >> MSR_DS) & 1)
> > #define msr_pe ((env->msr >> MSR_PE) & 1)
> > #define msr_px ((env->msr >> MSR_PX) & 1)
> > #define msr_pmm ((env->msr >> MSR_PMM) & 1)
> > @@ -889,7 +893,7 @@ struct ppc_segment_page_sizes {
> >
> > /*****************************************************************************/
> > /* The whole PowerPC CPU context */
> > -#define NB_MMU_MODES 3
> > +#define NB_MMU_MODES 8
> >
> > #define PPC_CPU_OPCODES_LEN 0x40
> > #define PPC_CPU_INDIRECT_OPCODES_LEN 0x20
> > @@ -1053,7 +1057,8 @@ struct CPUPPCState {
> > /* Those resources are used only in QEMU core */
> > target_ulong hflags; /* hflags is a MSR & HFLAGS_MASK */
> > target_ulong hflags_nmsr; /* specific hflags, not coming from MSR */
> > - int mmu_idx; /* precomputed MMU index to speed up mem accesses */
> > + int immu_idx; /* precomputed MMU index to speed up insn access */
> > + int dmmu_idx; /* precomputed MMU index to speed up data accesses */
> >
> > /* Power management */
> > int (*check_pow)(CPUPPCState *env);
> > @@ -1245,7 +1250,7 @@ int ppc_dcr_write (ppc_dcr_t *dcr_env, int dcrn, uint32_t val);
> > #define MMU_USER_IDX 0
> > static inline int cpu_mmu_index (CPUPPCState *env, bool ifetch)
> > {
> > - return env->mmu_idx;
> > + return ifetch ? env->immu_idx : env->dmmu_idx;
> > }
> >
> > #include "exec/cpu-all.h"
> > diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c
> > index 288903e..ba3caec 100644
> > --- a/target-ppc/excp_helper.c
> > +++ b/target-ppc/excp_helper.c
> > @@ -646,9 +646,6 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
> >
> > if (env->spr[SPR_LPCR] & LPCR_AIL) {
> > new_msr |= (1 << MSR_IR) | (1 << MSR_DR);
> > - } else if (msr & ((1 << MSR_IR) | (1 << MSR_DR))) {
> > - /* If we disactivated any translation, flush TLBs */
> > - tlb_flush(cs, 1);
> > }
> >
> > #ifdef TARGET_PPC64
> > @@ -721,14 +718,6 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
> > /* Reset exception state */
> > cs->exception_index = POWERPC_EXCP_NONE;
> > env->error_code = 0;
> > -
> > - if ((env->mmu_model == POWERPC_MMU_BOOKE) ||
> > - (env->mmu_model == POWERPC_MMU_BOOKE206)) {
> > - /* XXX: The BookE changes address space when switching modes,
> > - we should probably implement that as different MMU indexes,
> > - but for the moment we do it the slow way and flush all. */
> > - tlb_flush(cs, 1);
> > - }
> > }
> >
> > void ppc_cpu_do_interrupt(CPUState *cs)
> > diff --git a/target-ppc/helper_regs.h b/target-ppc/helper_regs.h
> > index 271fddf..f7edd5b 100644
> > --- a/target-ppc/helper_regs.h
> > +++ b/target-ppc/helper_regs.h
> > @@ -41,11 +41,50 @@ static inline void hreg_swap_gpr_tgpr(CPUPPCState *env)
> >
> > static inline void hreg_compute_mem_idx(CPUPPCState *env)
> > {
> > - /* Precompute MMU index */
> > - if (msr_pr == 0 && msr_hv != 0) {
> > - env->mmu_idx = 2;
> > + /* This is our encoding for server processors
> > + *
> > + * 0 = Guest User space virtual mode
> > + * 1 = Guest Kernel space virtual mode
> > + * 2 = Guest Kernel space real mode
> > + * 3 = HV User space virtual mode
> > + * 4 = HV Kernel space virtual mode
> > + * 5 = HV Kernel space real mode
> > + *
> > + * The combination PR=1 IR&DR=0 is invalid, we will treat
> > + * it as IR=DR=1
> > + *
> > + * For BookE, we need 8 MMU modes as follow:
> > + *
> > + * 0 = AS 0 HV User space
> > + * 1 = AS 0 HV Kernel space
> > + * 2 = AS 1 HV User space
> > + * 3 = AS 1 HV Kernel space
> > + * 4 = AS 0 Guest User space
> > + * 5 = AS 0 Guest Kernel space
> > + * 6 = AS 1 Guest User space
> > + * 7 = AS 1 Guest Kernel space
> > + */
> > + if (env->mmu_model & POWERPC_MMU_BOOKE) {
> > + env->immu_idx = env->dmmu_idx = msr_pr ? 0 : 1;
> > + env->immu_idx += msr_is ? 2 : 0;
> > + env->dmmu_idx += msr_ds ? 2 : 0;
> > + env->immu_idx += msr_gs ? 4 : 0;
> > + env->dmmu_idx += msr_gs ? 4 : 0;
> > } else {
> > - env->mmu_idx = 1 - msr_pr;
> > + /* First calucalte a base value independent of HV */
> > + if (msr_pr != 0) {
> > + /* User space, ignore IR and DR */
> > + env->immu_idx = env->dmmu_idx = 0;
> > + } else {
> > + /* Kernel, setup a base I/D value */
> > + env->immu_idx = msr_ir ? 1 : 2;
> > + env->dmmu_idx = msr_dr ? 1 : 2;
> > + }
> > + /* Then offset it for HV */
> > + if (msr_hv) {
> > + env->immu_idx += 3;
> > + env->dmmu_idx += 3;
> > + }
> > }
> > }
> >
> > @@ -82,9 +121,10 @@ static inline int hreg_store_msr(CPUPPCState *env, target_ulong value,
> > }
> > if (((value >> MSR_IR) & 1) != msr_ir ||
> > ((value >> MSR_DR) & 1) != msr_dr) {
> > - /* Flush all tlb when changing translation mode */
> > - tlb_flush(cs, 1);
> > - excp = POWERPC_EXCP_NONE;
> > + cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
> > + }
> > + if ((env->mmu_model & POWERPC_MMU_BOOKE) &&
> > + ((value >> MSR_GS) & 1) != msr_gs) {
> > cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
> > }
> > if (unlikely((env->flags & POWERPC_FLAG_TGPR) &&
> > diff --git a/target-ppc/machine.c b/target-ppc/machine.c
> > index f6c7256..4820f22 100644
> > --- a/target-ppc/machine.c
> > +++ b/target-ppc/machine.c
> > @@ -97,9 +97,12 @@ static int cpu_load_old(QEMUFile *f, void *opaque, int version_id)
> > qemu_get_betls(f, &env->nip);
> > qemu_get_betls(f, &env->hflags);
> > qemu_get_betls(f, &env->hflags_nmsr);
> > - qemu_get_sbe32s(f, &env->mmu_idx);
> > + qemu_get_sbe32(f); /* Discard unused mmu_idx */
> > qemu_get_sbe32(f); /* Discard unused power_mode */
> >
> > + /* Recompute mmu indices */
> > + hreg_compute_mem_idx(env);
> > +
> > return 0;
> > }
> >
> > diff --git a/target-ppc/translate.c b/target-ppc/translate.c
> > index f5ceae5..b757634 100644
> > --- a/target-ppc/translate.c
> > +++ b/target-ppc/translate.c
> > @@ -11220,8 +11220,9 @@ void ppc_cpu_dump_state(CPUState *cs, FILE *f, fprintf_function cpu_fprintf,
> > env->nip, env->lr, env->ctr, cpu_read_xer(env),
> > cs->cpu_index);
> > cpu_fprintf(f, "MSR " TARGET_FMT_lx " HID0 " TARGET_FMT_lx " HF "
> > - TARGET_FMT_lx " idx %d\n", env->msr, env->spr[SPR_HID0],
> > - env->hflags, env->mmu_idx);
> > + TARGET_FMT_lx " iidx %d didx %d\n",
> > + env->msr, env->spr[SPR_HID0],
> > + env->hflags, env->immu_idx, env->dmmu_idx);
> > #if !defined(NO_TIMER_DUMP)
> > cpu_fprintf(f, "TB %08" PRIu32 " %08" PRIu64
> > #if !defined(CONFIG_USER_ONLY)
> > @@ -11428,7 +11429,7 @@ void gen_intermediate_code(CPUPPCState *env, struct TranslationBlock *tb)
> > ctx.spr_cb = env->spr_cb;
> > ctx.pr = msr_pr;
> > ctx.hv = !msr_pr && msr_hv;
> > - ctx.mem_idx = env->mmu_idx;
> > + ctx.mem_idx = env->dmmu_idx;
> > ctx.insns_flags = env->insns_flags;
> > ctx.insns_flags2 = env->insns_flags2;
> > ctx.access_type = -1;
>
> Apologies to have to report another regression, however this patch
> causes a kernel panic in Darwin PPC under TCG:
>
> ./qemu-system-ppc -cdrom darwinppc-602.iso -boot d
>
> The panic backtrace on-screen points towards one of the SCSI modules, so
> perhaps this patch doesn't handle DMA correctly when trying to access
> the CDROM?
Bother. It's a lot less obvious to me how this one would cause
trouble. Possibly a bug in the CD-ROM device which was masked by the
pervious mem access stuff.
--
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] 31+ messages in thread* Re: [Qemu-devel] [Qemu-ppc] [PULL 02/12] ppc: Use split I/D mmu modes to avoid flushes on interrupts
2016-06-02 3:15 ` David Gibson
@ 2016-06-02 5:32 ` Mark Cave-Ayland
0 siblings, 0 replies; 31+ messages in thread
From: Mark Cave-Ayland @ 2016-06-02 5:32 UTC (permalink / raw)
To: David Gibson; +Cc: peter.maydell, pbonzini, qemu-ppc, qemu-devel, bharata.rao
On 02/06/16 04:15, David Gibson wrote:
> On Wed, Jun 01, 2016 at 08:33:30PM +0100, Mark Cave-Ayland wrote:
>> On 31/05/16 01:41, David Gibson wrote:
>>
>>> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>>>
>>> We rework the way the MMU indices are calculated, providing separate
>>> indices for I and D side based on MSR:IR and MSR:DR respectively,
>>> and thus no longer need to flush the TLB on context changes. This also
>>> adds correct support for HV as a separate address space.
>>>
>>> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>> ---
>>> target-ppc/cpu.h | 11 +++++++---
>>> target-ppc/excp_helper.c | 11 ----------
>>> target-ppc/helper_regs.h | 54 +++++++++++++++++++++++++++++++++++++++++-------
>>> target-ppc/machine.c | 5 ++++-
>>> target-ppc/translate.c | 7 ++++---
>>> 5 files changed, 63 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
>>> index 02e71ea..2c8c8c0 100644
>>> --- a/target-ppc/cpu.h
>>> +++ b/target-ppc/cpu.h
>>> @@ -359,6 +359,8 @@ struct ppc_slb_t {
>>> #define MSR_EP 6 /* Exception prefix on 601 */
>>> #define MSR_IR 5 /* Instruction relocate */
>>> #define MSR_DR 4 /* Data relocate */
>>> +#define MSR_IS 5 /* Instruction address space (BookE) */
>>> +#define MSR_DS 4 /* Data address space (BookE) */
>>> #define MSR_PE 3 /* Protection enable on 403 */
>>> #define MSR_PX 2 /* Protection exclusive on 403 x */
>>> #define MSR_PMM 2 /* Performance monitor mark on POWER x */
>>> @@ -410,6 +412,8 @@ struct ppc_slb_t {
>>> #define msr_ep ((env->msr >> MSR_EP) & 1)
>>> #define msr_ir ((env->msr >> MSR_IR) & 1)
>>> #define msr_dr ((env->msr >> MSR_DR) & 1)
>>> +#define msr_is ((env->msr >> MSR_IS) & 1)
>>> +#define msr_ds ((env->msr >> MSR_DS) & 1)
>>> #define msr_pe ((env->msr >> MSR_PE) & 1)
>>> #define msr_px ((env->msr >> MSR_PX) & 1)
>>> #define msr_pmm ((env->msr >> MSR_PMM) & 1)
>>> @@ -889,7 +893,7 @@ struct ppc_segment_page_sizes {
>>>
>>> /*****************************************************************************/
>>> /* The whole PowerPC CPU context */
>>> -#define NB_MMU_MODES 3
>>> +#define NB_MMU_MODES 8
>>>
>>> #define PPC_CPU_OPCODES_LEN 0x40
>>> #define PPC_CPU_INDIRECT_OPCODES_LEN 0x20
>>> @@ -1053,7 +1057,8 @@ struct CPUPPCState {
>>> /* Those resources are used only in QEMU core */
>>> target_ulong hflags; /* hflags is a MSR & HFLAGS_MASK */
>>> target_ulong hflags_nmsr; /* specific hflags, not coming from MSR */
>>> - int mmu_idx; /* precomputed MMU index to speed up mem accesses */
>>> + int immu_idx; /* precomputed MMU index to speed up insn access */
>>> + int dmmu_idx; /* precomputed MMU index to speed up data accesses */
>>>
>>> /* Power management */
>>> int (*check_pow)(CPUPPCState *env);
>>> @@ -1245,7 +1250,7 @@ int ppc_dcr_write (ppc_dcr_t *dcr_env, int dcrn, uint32_t val);
>>> #define MMU_USER_IDX 0
>>> static inline int cpu_mmu_index (CPUPPCState *env, bool ifetch)
>>> {
>>> - return env->mmu_idx;
>>> + return ifetch ? env->immu_idx : env->dmmu_idx;
>>> }
>>>
>>> #include "exec/cpu-all.h"
>>> diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c
>>> index 288903e..ba3caec 100644
>>> --- a/target-ppc/excp_helper.c
>>> +++ b/target-ppc/excp_helper.c
>>> @@ -646,9 +646,6 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>>>
>>> if (env->spr[SPR_LPCR] & LPCR_AIL) {
>>> new_msr |= (1 << MSR_IR) | (1 << MSR_DR);
>>> - } else if (msr & ((1 << MSR_IR) | (1 << MSR_DR))) {
>>> - /* If we disactivated any translation, flush TLBs */
>>> - tlb_flush(cs, 1);
>>> }
>>>
>>> #ifdef TARGET_PPC64
>>> @@ -721,14 +718,6 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
>>> /* Reset exception state */
>>> cs->exception_index = POWERPC_EXCP_NONE;
>>> env->error_code = 0;
>>> -
>>> - if ((env->mmu_model == POWERPC_MMU_BOOKE) ||
>>> - (env->mmu_model == POWERPC_MMU_BOOKE206)) {
>>> - /* XXX: The BookE changes address space when switching modes,
>>> - we should probably implement that as different MMU indexes,
>>> - but for the moment we do it the slow way and flush all. */
>>> - tlb_flush(cs, 1);
>>> - }
>>> }
>>>
>>> void ppc_cpu_do_interrupt(CPUState *cs)
>>> diff --git a/target-ppc/helper_regs.h b/target-ppc/helper_regs.h
>>> index 271fddf..f7edd5b 100644
>>> --- a/target-ppc/helper_regs.h
>>> +++ b/target-ppc/helper_regs.h
>>> @@ -41,11 +41,50 @@ static inline void hreg_swap_gpr_tgpr(CPUPPCState *env)
>>>
>>> static inline void hreg_compute_mem_idx(CPUPPCState *env)
>>> {
>>> - /* Precompute MMU index */
>>> - if (msr_pr == 0 && msr_hv != 0) {
>>> - env->mmu_idx = 2;
>>> + /* This is our encoding for server processors
>>> + *
>>> + * 0 = Guest User space virtual mode
>>> + * 1 = Guest Kernel space virtual mode
>>> + * 2 = Guest Kernel space real mode
>>> + * 3 = HV User space virtual mode
>>> + * 4 = HV Kernel space virtual mode
>>> + * 5 = HV Kernel space real mode
>>> + *
>>> + * The combination PR=1 IR&DR=0 is invalid, we will treat
>>> + * it as IR=DR=1
>>> + *
>>> + * For BookE, we need 8 MMU modes as follow:
>>> + *
>>> + * 0 = AS 0 HV User space
>>> + * 1 = AS 0 HV Kernel space
>>> + * 2 = AS 1 HV User space
>>> + * 3 = AS 1 HV Kernel space
>>> + * 4 = AS 0 Guest User space
>>> + * 5 = AS 0 Guest Kernel space
>>> + * 6 = AS 1 Guest User space
>>> + * 7 = AS 1 Guest Kernel space
>>> + */
>>> + if (env->mmu_model & POWERPC_MMU_BOOKE) {
>>> + env->immu_idx = env->dmmu_idx = msr_pr ? 0 : 1;
>>> + env->immu_idx += msr_is ? 2 : 0;
>>> + env->dmmu_idx += msr_ds ? 2 : 0;
>>> + env->immu_idx += msr_gs ? 4 : 0;
>>> + env->dmmu_idx += msr_gs ? 4 : 0;
>>> } else {
>>> - env->mmu_idx = 1 - msr_pr;
>>> + /* First calucalte a base value independent of HV */
>>> + if (msr_pr != 0) {
>>> + /* User space, ignore IR and DR */
>>> + env->immu_idx = env->dmmu_idx = 0;
>>> + } else {
>>> + /* Kernel, setup a base I/D value */
>>> + env->immu_idx = msr_ir ? 1 : 2;
>>> + env->dmmu_idx = msr_dr ? 1 : 2;
>>> + }
>>> + /* Then offset it for HV */
>>> + if (msr_hv) {
>>> + env->immu_idx += 3;
>>> + env->dmmu_idx += 3;
>>> + }
>>> }
>>> }
>>>
>>> @@ -82,9 +121,10 @@ static inline int hreg_store_msr(CPUPPCState *env, target_ulong value,
>>> }
>>> if (((value >> MSR_IR) & 1) != msr_ir ||
>>> ((value >> MSR_DR) & 1) != msr_dr) {
>>> - /* Flush all tlb when changing translation mode */
>>> - tlb_flush(cs, 1);
>>> - excp = POWERPC_EXCP_NONE;
>>> + cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
>>> + }
>>> + if ((env->mmu_model & POWERPC_MMU_BOOKE) &&
>>> + ((value >> MSR_GS) & 1) != msr_gs) {
>>> cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
>>> }
>>> if (unlikely((env->flags & POWERPC_FLAG_TGPR) &&
>>> diff --git a/target-ppc/machine.c b/target-ppc/machine.c
>>> index f6c7256..4820f22 100644
>>> --- a/target-ppc/machine.c
>>> +++ b/target-ppc/machine.c
>>> @@ -97,9 +97,12 @@ static int cpu_load_old(QEMUFile *f, void *opaque, int version_id)
>>> qemu_get_betls(f, &env->nip);
>>> qemu_get_betls(f, &env->hflags);
>>> qemu_get_betls(f, &env->hflags_nmsr);
>>> - qemu_get_sbe32s(f, &env->mmu_idx);
>>> + qemu_get_sbe32(f); /* Discard unused mmu_idx */
>>> qemu_get_sbe32(f); /* Discard unused power_mode */
>>>
>>> + /* Recompute mmu indices */
>>> + hreg_compute_mem_idx(env);
>>> +
>>> return 0;
>>> }
>>>
>>> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
>>> index f5ceae5..b757634 100644
>>> --- a/target-ppc/translate.c
>>> +++ b/target-ppc/translate.c
>>> @@ -11220,8 +11220,9 @@ void ppc_cpu_dump_state(CPUState *cs, FILE *f, fprintf_function cpu_fprintf,
>>> env->nip, env->lr, env->ctr, cpu_read_xer(env),
>>> cs->cpu_index);
>>> cpu_fprintf(f, "MSR " TARGET_FMT_lx " HID0 " TARGET_FMT_lx " HF "
>>> - TARGET_FMT_lx " idx %d\n", env->msr, env->spr[SPR_HID0],
>>> - env->hflags, env->mmu_idx);
>>> + TARGET_FMT_lx " iidx %d didx %d\n",
>>> + env->msr, env->spr[SPR_HID0],
>>> + env->hflags, env->immu_idx, env->dmmu_idx);
>>> #if !defined(NO_TIMER_DUMP)
>>> cpu_fprintf(f, "TB %08" PRIu32 " %08" PRIu64
>>> #if !defined(CONFIG_USER_ONLY)
>>> @@ -11428,7 +11429,7 @@ void gen_intermediate_code(CPUPPCState *env, struct TranslationBlock *tb)
>>> ctx.spr_cb = env->spr_cb;
>>> ctx.pr = msr_pr;
>>> ctx.hv = !msr_pr && msr_hv;
>>> - ctx.mem_idx = env->mmu_idx;
>>> + ctx.mem_idx = env->dmmu_idx;
>>> ctx.insns_flags = env->insns_flags;
>>> ctx.insns_flags2 = env->insns_flags2;
>>> ctx.access_type = -1;
>>
>> Apologies to have to report another regression, however this patch
>> causes a kernel panic in Darwin PPC under TCG:
>>
>> ./qemu-system-ppc -cdrom darwinppc-602.iso -boot d
>>
>> The panic backtrace on-screen points towards one of the SCSI modules, so
>> perhaps this patch doesn't handle DMA correctly when trying to access
>> the CDROM?
>
> Bother. It's a lot less obvious to me how this one would cause
> trouble. Possibly a bug in the CD-ROM device which was masked by the
> pervious mem access stuff.
Some more testing shows that HelenOS also fails with this patch applied.
I'm not sure exactly how helpful this is, however the minimal diff below
in conjunction with a revert of tlbia patch is enough to allow both
Darwin and HelenOS to boot once again (note that I've tried each flush
individually, and both are required):
diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c
index a37009e..6b81a09 100644
--- a/target-ppc/excp_helper.c
+++ b/target-ppc/excp_helper.c
@@ -646,6 +646,9 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int
excp_model, int excp)
if (env->spr[SPR_LPCR] & LPCR_AIL) {
new_msr |= (1 << MSR_IR) | (1 << MSR_DR);
+ } else if (msr & ((1 << MSR_IR) | (1 << MSR_DR))) {
+ /* If we disactivated any translation, flush TLBs */
+ tlb_flush(cs, 1);
}
#ifdef TARGET_PPC64
diff --git a/target-ppc/helper_regs.h b/target-ppc/helper_regs.h
index 57da931..8437211 100644
--- a/target-ppc/helper_regs.h
+++ b/target-ppc/helper_regs.h
@@ -121,6 +121,9 @@ static inline int hreg_store_msr(CPUPPCState *env,
target_ulong value,
}
if (((value >> MSR_IR) & 1) != msr_ir ||
((value >> MSR_DR) & 1) != msr_dr) {
+ /* Flush all tlb when changing translation mode */
+ tlb_flush(cs, 1);
+ excp = POWERPC_EXCP_NONE;
cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
}
if ((env->mmu_model & POWERPC_MMU_BOOKE) &&
ATB,
Mark.
^ permalink raw reply related [flat|nested] 31+ messages in thread
* [Qemu-devel] [PULL 03/12] ppc: Do some batching of TCG tlb flushes
2016-05-31 0:41 [Qemu-devel] [PULL 00/12] ppc-for-2.7 queue 20160531 David Gibson
2016-05-31 0:41 ` [Qemu-devel] [PULL 01/12] ppc: Remove MMU_MODEn_SUFFIX definitions David Gibson
2016-05-31 0:41 ` [Qemu-devel] [PULL 02/12] ppc: Use split I/D mmu modes to avoid flushes on interrupts David Gibson
@ 2016-05-31 0:41 ` David Gibson
2016-05-31 0:41 ` [Qemu-devel] [PULL 04/12] ppc: tlbie, tlbia and tlbisync are HV only David Gibson
` (9 subsequent siblings)
12 siblings, 0 replies; 31+ messages in thread
From: David Gibson @ 2016-05-31 0:41 UTC (permalink / raw)
To: peter.maydell
Cc: agraf, pbonzini, bharata.rao, benh, qemu-ppc, qemu-devel,
Cédric Le Goater, David Gibson
From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
On ppc64 especially, we flush the tlb on any slbie or tlbie instruction.
However, those instructions often come in bursts of 3 or more (context
switch will favor a series of slbie's for example to an slbia if the
SLB has less than a certain number of entries in it, and tlbie's can
happen in a series, with PAPR, H_BULK_REMOVE can remove up to 4 entries
at a time.
Doing a tlb_flush() each time is a waste of time. We end up doing a memset
of the whole TLB, reloading it for the next instruction, memset'ing again,
etc...
Those instructions don't have to take effect immediately. For slbie, they
can wait for the next context synchronizing event. For tlbie, the next
tlbsync.
This implements batching by keeping a flag that indicates that we have a
TLB in need of flushing. We check it on interrupts, rfi's, isync's and
tlbsync and flush the TLB if needed.
This reduces the number of tlb_flush() on a boot to a ubuntu installer
first dialog screen from roughly 360K down to 36K.
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
[clg: added a 'CPUPPCState *' variable in h_remove() and
h_bulk_remove() ]
Signed-off-by: Cédric Le Goater <clg@kaod.org>
[dwg: removed spurious whitespace change, use 0/1 not true/false
consistently, since tlb_need_flush has int type]
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
hw/ppc/spapr_hcall.c | 14 +++++++++++---
target-ppc/cpu.h | 2 ++
target-ppc/excp_helper.c | 8 ++++++++
target-ppc/helper.h | 1 +
target-ppc/helper_regs.h | 13 +++++++++++++
target-ppc/mmu-hash64.c | 11 +++--------
target-ppc/mmu_helper.c | 9 ++++++++-
target-ppc/translate.c | 39 ++++++++++++++++++++++++++++++++++++---
8 files changed, 82 insertions(+), 15 deletions(-)
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index feb3629..9a3f4ec 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -186,6 +186,7 @@ static RemoveResult remove_hpte(PowerPCCPU *cpu, target_ulong ptex,
static target_ulong h_remove(PowerPCCPU *cpu, sPAPRMachineState *spapr,
target_ulong opcode, target_ulong *args)
{
+ CPUPPCState *env = &cpu->env;
target_ulong flags = args[0];
target_ulong pte_index = args[1];
target_ulong avpn = args[2];
@@ -196,6 +197,7 @@ static target_ulong h_remove(PowerPCCPU *cpu, sPAPRMachineState *spapr,
switch (ret) {
case REMOVE_SUCCESS:
+ check_tlb_flush(env);
return H_SUCCESS;
case REMOVE_NOT_FOUND:
@@ -232,7 +234,9 @@ static target_ulong h_remove(PowerPCCPU *cpu, sPAPRMachineState *spapr,
static target_ulong h_bulk_remove(PowerPCCPU *cpu, sPAPRMachineState *spapr,
target_ulong opcode, target_ulong *args)
{
+ CPUPPCState *env = &cpu->env;
int i;
+ target_ulong rc = H_SUCCESS;
for (i = 0; i < H_BULK_REMOVE_MAX_BATCH; i++) {
target_ulong *tsh = &args[i*2];
@@ -265,14 +269,18 @@ static target_ulong h_bulk_remove(PowerPCCPU *cpu, sPAPRMachineState *spapr,
break;
case REMOVE_PARM:
- return H_PARAMETER;
+ rc = H_PARAMETER;
+ goto exit;
case REMOVE_HW:
- return H_HARDWARE;
+ rc = H_HARDWARE;
+ goto exit;
}
}
+ exit:
+ check_tlb_flush(env);
- return H_SUCCESS;
+ return rc;
}
static target_ulong h_protect(PowerPCCPU *cpu, sPAPRMachineState *spapr,
diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
index 2c8c8c0..98a24a5 100644
--- a/target-ppc/cpu.h
+++ b/target-ppc/cpu.h
@@ -958,6 +958,8 @@ struct CPUPPCState {
/* PowerPC 64 SLB area */
ppc_slb_t slb[MAX_SLB_ENTRIES];
int32_t slb_nr;
+ /* tcg TLB needs flush (deferred slb inval instruction typically) */
+ uint32_t tlb_need_flush;
#endif
/* segment registers */
hwaddr htab_base;
diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c
index ba3caec..a37009e 100644
--- a/target-ppc/excp_helper.c
+++ b/target-ppc/excp_helper.c
@@ -718,6 +718,11 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
/* Reset exception state */
cs->exception_index = POWERPC_EXCP_NONE;
env->error_code = 0;
+
+ /* Any interrupt is context synchronizing, check if TCG TLB
+ * needs a delayed flush on ppc64
+ */
+ check_tlb_flush(env);
}
void ppc_cpu_do_interrupt(CPUState *cs)
@@ -943,6 +948,9 @@ static inline void do_rfi(CPUPPCState *env, target_ulong nip, target_ulong msr,
* as rfi is always the last insn of a TB
*/
cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
+
+ /* Context synchronizing: check if TCG TLB needs flush */
+ check_tlb_flush(env);
}
void helper_rfi(CPUPPCState *env)
diff --git a/target-ppc/helper.h b/target-ppc/helper.h
index e5a8f7b..0526322 100644
--- a/target-ppc/helper.h
+++ b/target-ppc/helper.h
@@ -16,6 +16,7 @@ DEF_HELPER_1(rfmci, void, env)
DEF_HELPER_1(rfid, void, env)
DEF_HELPER_1(hrfid, void, env)
#endif
+DEF_HELPER_1(check_tlb_flush, void, env)
#endif
DEF_HELPER_3(lmw, void, env, tl, i32)
diff --git a/target-ppc/helper_regs.h b/target-ppc/helper_regs.h
index f7edd5b..57da931 100644
--- a/target-ppc/helper_regs.h
+++ b/target-ppc/helper_regs.h
@@ -151,4 +151,17 @@ static inline int hreg_store_msr(CPUPPCState *env, target_ulong value,
return excp;
}
+#if !defined(CONFIG_USER_ONLY) && defined(TARGET_PPC64)
+static inline void check_tlb_flush(CPUPPCState *env)
+{
+ CPUState *cs = CPU(ppc_env_get_cpu(env));
+ if (env->tlb_need_flush) {
+ env->tlb_need_flush = 0;
+ tlb_flush(cs, 1);
+ }
+}
+#else
+static inline void check_tlb_flush(CPUPPCState *env) { }
+#endif
+
#endif /* !defined(__HELPER_REGS_H__) */
diff --git a/target-ppc/mmu-hash64.c b/target-ppc/mmu-hash64.c
index 17e2480..ea6e99a 100644
--- a/target-ppc/mmu-hash64.c
+++ b/target-ppc/mmu-hash64.c
@@ -99,10 +99,8 @@ void dump_slb(FILE *f, fprintf_function cpu_fprintf, PowerPCCPU *cpu)
void helper_slbia(CPUPPCState *env)
{
- PowerPCCPU *cpu = ppc_env_get_cpu(env);
- int n, do_invalidate;
+ int n;
- do_invalidate = 0;
/* XXX: Warning: slbia never invalidates the first segment */
for (n = 1; n < env->slb_nr; n++) {
ppc_slb_t *slb = &env->slb[n];
@@ -113,12 +111,9 @@ void helper_slbia(CPUPPCState *env)
* and we still don't have a tlb_flush_mask(env, n, mask)
* in QEMU, we just invalidate all TLBs
*/
- do_invalidate = 1;
+ env->tlb_need_flush = 1;
}
}
- if (do_invalidate) {
- tlb_flush(CPU(cpu), 1);
- }
}
void helper_slbie(CPUPPCState *env, target_ulong addr)
@@ -138,7 +133,7 @@ void helper_slbie(CPUPPCState *env, target_ulong addr)
* and we still don't have a tlb_flush_mask(env, n, mask)
* in QEMU, we just invalidate all TLBs
*/
- tlb_flush(CPU(cpu), 1);
+ env->tlb_need_flush = 1;
}
}
diff --git a/target-ppc/mmu_helper.c b/target-ppc/mmu_helper.c
index 2e0e3ca..1499af72 100644
--- a/target-ppc/mmu_helper.c
+++ b/target-ppc/mmu_helper.c
@@ -27,6 +27,7 @@
#include "exec/exec-all.h"
#include "exec/cpu_ldst.h"
#include "exec/log.h"
+#include "helper_regs.h"
//#define DEBUG_MMU
//#define DEBUG_BATS
@@ -1924,6 +1925,7 @@ void ppc_tlb_invalidate_all(CPUPPCState *env)
case POWERPC_MMU_2_06a:
case POWERPC_MMU_2_07:
case POWERPC_MMU_2_07a:
+ env->tlb_need_flush = 0;
#endif /* defined(TARGET_PPC64) */
tlb_flush(CPU(cpu), 1);
break;
@@ -1986,7 +1988,7 @@ void ppc_tlb_invalidate_one(CPUPPCState *env, target_ulong addr)
* and we still don't have a tlb_flush_mask(env, n, mask) in QEMU,
* we just invalidate all TLBs
*/
- tlb_flush(CPU(cpu), 1);
+ env->tlb_need_flush = 1;
break;
#endif /* defined(TARGET_PPC64) */
default:
@@ -2875,6 +2877,11 @@ void helper_booke206_tlbflush(CPUPPCState *env, target_ulong type)
}
+void helper_check_tlb_flush(CPUPPCState *env)
+{
+ check_tlb_flush(env);
+}
+
/*****************************************************************************/
/* try to fill the TLB and return an exception if error. If retaddr is
diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index b757634..dfd3010 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -3275,9 +3275,32 @@ static void gen_eieio(DisasContext *ctx)
{
}
+#if !defined(CONFIG_USER_ONLY) && defined(TARGET_PPC64)
+static inline void gen_check_tlb_flush(DisasContext *ctx)
+{
+ TCGv_i32 t = tcg_temp_new_i32();
+ TCGLabel *l = gen_new_label();
+
+ tcg_gen_ld_i32(t, cpu_env, offsetof(CPUPPCState, tlb_need_flush));
+ tcg_gen_brcondi_i32(TCG_COND_EQ, t, 0, l);
+ gen_helper_check_tlb_flush(cpu_env);
+ gen_set_label(l);
+ tcg_temp_free_i32(t);
+}
+#else
+static inline void gen_check_tlb_flush(DisasContext *ctx) { }
+#endif
+
/* isync */
static void gen_isync(DisasContext *ctx)
{
+ /*
+ * We need to check for a pending TLB flush. This can only happen in
+ * kernel mode however so check MSR_PR
+ */
+ if (!ctx->pr) {
+ gen_check_tlb_flush(ctx);
+ }
gen_stop_exception(ctx);
}
@@ -3434,6 +3457,15 @@ STCX(stqcx_, 16);
/* sync */
static void gen_sync(DisasContext *ctx)
{
+ uint32_t l = (ctx->opcode >> 21) & 3;
+
+ /*
+ * For l == 2, it's a ptesync, We need to check for a pending TLB flush.
+ * This can only happen in kernel mode however so check MSR_PR as well.
+ */
+ if (l == 2 && !ctx->pr) {
+ gen_check_tlb_flush(ctx);
+ }
}
/* wait */
@@ -4851,10 +4883,11 @@ static void gen_tlbsync(DisasContext *ctx)
gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
return;
}
- /* This has no effect: it should ensure that all previous
- * tlbie have completed
+ /* tlbsync is a nop for server, ptesync handles delayed tlb flush,
+ * embedded however needs to deal with tlbsync. We don't try to be
+ * fancy and swallow the overhead of checking for both.
*/
- gen_stop_exception(ctx);
+ gen_check_tlb_flush(ctx);
#endif
}
--
2.5.5
^ permalink raw reply related [flat|nested] 31+ messages in thread* [Qemu-devel] [PULL 04/12] ppc: tlbie, tlbia and tlbisync are HV only
2016-05-31 0:41 [Qemu-devel] [PULL 00/12] ppc-for-2.7 queue 20160531 David Gibson
` (2 preceding siblings ...)
2016-05-31 0:41 ` [Qemu-devel] [PULL 03/12] ppc: Do some batching of TCG tlb flushes David Gibson
@ 2016-05-31 0:41 ` David Gibson
2016-05-31 22:28 ` [Qemu-devel] [Qemu-ppc] " Mark Cave-Ayland
2016-05-31 0:41 ` [Qemu-devel] [PULL 05/12] ppc: Change 'invalid' bit mask of tlbiel and tlbie David Gibson
` (8 subsequent siblings)
12 siblings, 1 reply; 31+ messages in thread
From: David Gibson @ 2016-05-31 0:41 UTC (permalink / raw)
To: peter.maydell
Cc: agraf, pbonzini, bharata.rao, benh, qemu-ppc, qemu-devel,
David Gibson
From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Not that anything remotely recent supports tlbia but ...
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
target-ppc/translate.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index dfd3010..690ffd2 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -4858,7 +4858,7 @@ static void gen_tlbie(DisasContext *ctx)
#if defined(CONFIG_USER_ONLY)
gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
#else
- if (unlikely(ctx->pr)) {
+ if (unlikely(ctx->pr || !ctx->hv)) {
gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
return;
}
@@ -4879,7 +4879,7 @@ static void gen_tlbsync(DisasContext *ctx)
#if defined(CONFIG_USER_ONLY)
gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
#else
- if (unlikely(ctx->pr)) {
+ if (unlikely(ctx->pr || !ctx->hv)) {
gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
return;
}
@@ -4898,7 +4898,7 @@ static void gen_slbia(DisasContext *ctx)
#if defined(CONFIG_USER_ONLY)
gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
#else
- if (unlikely(ctx->pr)) {
+ if (unlikely(ctx->pr || !ctx->hv)) {
gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
return;
}
--
2.5.5
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [Qemu-ppc] [PULL 04/12] ppc: tlbie, tlbia and tlbisync are HV only
2016-05-31 0:41 ` [Qemu-devel] [PULL 04/12] ppc: tlbie, tlbia and tlbisync are HV only David Gibson
@ 2016-05-31 22:28 ` Mark Cave-Ayland
2016-06-01 2:15 ` David Gibson
0 siblings, 1 reply; 31+ messages in thread
From: Mark Cave-Ayland @ 2016-05-31 22:28 UTC (permalink / raw)
To: David Gibson, peter.maydell; +Cc: qemu-devel, qemu-ppc, bharata.rao, pbonzini
On 31/05/16 01:41, David Gibson wrote:
> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>
> Not that anything remotely recent supports tlbia but ...
>
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> target-ppc/translate.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
> index dfd3010..690ffd2 100644
> --- a/target-ppc/translate.c
> +++ b/target-ppc/translate.c
> @@ -4858,7 +4858,7 @@ static void gen_tlbie(DisasContext *ctx)
> #if defined(CONFIG_USER_ONLY)
> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
> #else
> - if (unlikely(ctx->pr)) {
> + if (unlikely(ctx->pr || !ctx->hv)) {
> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
> return;
> }
> @@ -4879,7 +4879,7 @@ static void gen_tlbsync(DisasContext *ctx)
> #if defined(CONFIG_USER_ONLY)
> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
> #else
> - if (unlikely(ctx->pr)) {
> + if (unlikely(ctx->pr || !ctx->hv)) {
> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
> return;
> }
> @@ -4898,7 +4898,7 @@ static void gen_slbia(DisasContext *ctx)
> #if defined(CONFIG_USER_ONLY)
> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
> #else
> - if (unlikely(ctx->pr)) {
> + if (unlikely(ctx->pr || !ctx->hv)) {
> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
> return;
> }
Unfortunately this patch breaks qemu-system-ppc for both g3beige and
mac99 under TCG causing a freeze in OpenBIOS when starting
qemu-system-ppc with no parameters.
Note that there is also another regression that has recently landed in
git master so you'll also need to revert
e7c9136977cb99c6eb52c9139f7b8d8b5fa87db9 in order to get back to a
functioning OpenBIOS.
ATB,
Mark.
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [Qemu-ppc] [PULL 04/12] ppc: tlbie, tlbia and tlbisync are HV only
2016-05-31 22:28 ` [Qemu-devel] [Qemu-ppc] " Mark Cave-Ayland
@ 2016-06-01 2:15 ` David Gibson
2016-06-01 7:03 ` Mark Cave-Ayland
0 siblings, 1 reply; 31+ messages in thread
From: David Gibson @ 2016-06-01 2:15 UTC (permalink / raw)
To: Mark Cave-Ayland
Cc: peter.maydell, qemu-devel, qemu-ppc, bharata.rao, pbonzini
[-- Attachment #1: Type: text/plain, Size: 2544 bytes --]
On Tue, May 31, 2016 at 11:28:49PM +0100, Mark Cave-Ayland wrote:
> On 31/05/16 01:41, David Gibson wrote:
>
> > From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> >
> > Not that anything remotely recent supports tlbia but ...
> >
> > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> > target-ppc/translate.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/target-ppc/translate.c b/target-ppc/translate.c
> > index dfd3010..690ffd2 100644
> > --- a/target-ppc/translate.c
> > +++ b/target-ppc/translate.c
> > @@ -4858,7 +4858,7 @@ static void gen_tlbie(DisasContext *ctx)
> > #if defined(CONFIG_USER_ONLY)
> > gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
> > #else
> > - if (unlikely(ctx->pr)) {
> > + if (unlikely(ctx->pr || !ctx->hv)) {
> > gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
> > return;
> > }
> > @@ -4879,7 +4879,7 @@ static void gen_tlbsync(DisasContext *ctx)
> > #if defined(CONFIG_USER_ONLY)
> > gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
> > #else
> > - if (unlikely(ctx->pr)) {
> > + if (unlikely(ctx->pr || !ctx->hv)) {
> > gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
> > return;
> > }
> > @@ -4898,7 +4898,7 @@ static void gen_slbia(DisasContext *ctx)
> > #if defined(CONFIG_USER_ONLY)
> > gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
> > #else
> > - if (unlikely(ctx->pr)) {
> > + if (unlikely(ctx->pr || !ctx->hv)) {
> > gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
> > return;
> > }
>
> Unfortunately this patch breaks qemu-system-ppc for both g3beige and
> mac99 under TCG causing a freeze in OpenBIOS when starting
> qemu-system-ppc with no parameters.
Bother, sorry.
I think this is because I applied this without the patch that treats
machines with no hypervisor mode (e.g. Apples) as always being in
hypervisor mode.
> Note that there is also another regression that has recently landed in
> git master so you'll also need to revert
> e7c9136977cb99c6eb52c9139f7b8d8b5fa87db9 in order to get back to a
> functioning OpenBIOS.
I'd preter to see it fixed rather than just reverted..
--
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] 31+ messages in thread* Re: [Qemu-devel] [Qemu-ppc] [PULL 04/12] ppc: tlbie, tlbia and tlbisync are HV only
2016-06-01 2:15 ` David Gibson
@ 2016-06-01 7:03 ` Mark Cave-Ayland
2016-06-02 3:17 ` David Gibson
0 siblings, 1 reply; 31+ messages in thread
From: Mark Cave-Ayland @ 2016-06-01 7:03 UTC (permalink / raw)
To: David Gibson; +Cc: peter.maydell, pbonzini, qemu-ppc, qemu-devel, bharata.rao
On 01/06/16 03:15, David Gibson wrote:
> On Tue, May 31, 2016 at 11:28:49PM +0100, Mark Cave-Ayland wrote:
>> On 31/05/16 01:41, David Gibson wrote:
>>
>>> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>>>
>>> Not that anything remotely recent supports tlbia but ...
>>>
>>> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>> ---
>>> target-ppc/translate.c | 6 +++---
>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
>>> index dfd3010..690ffd2 100644
>>> --- a/target-ppc/translate.c
>>> +++ b/target-ppc/translate.c
>>> @@ -4858,7 +4858,7 @@ static void gen_tlbie(DisasContext *ctx)
>>> #if defined(CONFIG_USER_ONLY)
>>> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
>>> #else
>>> - if (unlikely(ctx->pr)) {
>>> + if (unlikely(ctx->pr || !ctx->hv)) {
>>> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
>>> return;
>>> }
>>> @@ -4879,7 +4879,7 @@ static void gen_tlbsync(DisasContext *ctx)
>>> #if defined(CONFIG_USER_ONLY)
>>> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
>>> #else
>>> - if (unlikely(ctx->pr)) {
>>> + if (unlikely(ctx->pr || !ctx->hv)) {
>>> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
>>> return;
>>> }
>>> @@ -4898,7 +4898,7 @@ static void gen_slbia(DisasContext *ctx)
>>> #if defined(CONFIG_USER_ONLY)
>>> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
>>> #else
>>> - if (unlikely(ctx->pr)) {
>>> + if (unlikely(ctx->pr || !ctx->hv)) {
>>> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
>>> return;
>>> }
>>
>> Unfortunately this patch breaks qemu-system-ppc for both g3beige and
>> mac99 under TCG causing a freeze in OpenBIOS when starting
>> qemu-system-ppc with no parameters.
>
> Bother, sorry.
>
> I think this is because I applied this without the patch that treats
> machines with no hypervisor mode (e.g. Apples) as always being in
> hypervisor mode.
No problem, I can cope for a couple of days or so.
>> Note that there is also another regression that has recently landed in
>> git master so you'll also need to revert
>> e7c9136977cb99c6eb52c9139f7b8d8b5fa87db9 in order to get back to a
>> functioning OpenBIOS.
>
> I'd preter to see it fixed rather than just reverted..
Looks like the original author has found the bug, so there should be a
fix coming up for this soon (I only included it here in case you needed
an explicit test case).
ATB,
Mark.
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [Qemu-ppc] [PULL 04/12] ppc: tlbie, tlbia and tlbisync are HV only
2016-06-01 7:03 ` Mark Cave-Ayland
@ 2016-06-02 3:17 ` David Gibson
2016-06-02 7:37 ` Cédric Le Goater
2016-06-14 7:37 ` Thomas Huth
0 siblings, 2 replies; 31+ messages in thread
From: David Gibson @ 2016-06-02 3:17 UTC (permalink / raw)
To: Mark Cave-Ayland
Cc: peter.maydell, pbonzini, qemu-ppc, qemu-devel, bharata.rao, clg
[-- Attachment #1: Type: text/plain, Size: 3647 bytes --]
On Wed, Jun 01, 2016 at 08:03:08AM +0100, Mark Cave-Ayland wrote:
> On 01/06/16 03:15, David Gibson wrote:
>
> > On Tue, May 31, 2016 at 11:28:49PM +0100, Mark Cave-Ayland wrote:
> >> On 31/05/16 01:41, David Gibson wrote:
> >>
> >>> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> >>>
> >>> Not that anything remotely recent supports tlbia but ...
> >>>
> >>> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> >>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> >>> ---
> >>> target-ppc/translate.c | 6 +++---
> >>> 1 file changed, 3 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
> >>> index dfd3010..690ffd2 100644
> >>> --- a/target-ppc/translate.c
> >>> +++ b/target-ppc/translate.c
> >>> @@ -4858,7 +4858,7 @@ static void gen_tlbie(DisasContext *ctx)
> >>> #if defined(CONFIG_USER_ONLY)
> >>> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
> >>> #else
> >>> - if (unlikely(ctx->pr)) {
> >>> + if (unlikely(ctx->pr || !ctx->hv)) {
> >>> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
> >>> return;
> >>> }
> >>> @@ -4879,7 +4879,7 @@ static void gen_tlbsync(DisasContext *ctx)
> >>> #if defined(CONFIG_USER_ONLY)
> >>> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
> >>> #else
> >>> - if (unlikely(ctx->pr)) {
> >>> + if (unlikely(ctx->pr || !ctx->hv)) {
> >>> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
> >>> return;
> >>> }
> >>> @@ -4898,7 +4898,7 @@ static void gen_slbia(DisasContext *ctx)
> >>> #if defined(CONFIG_USER_ONLY)
> >>> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
> >>> #else
> >>> - if (unlikely(ctx->pr)) {
> >>> + if (unlikely(ctx->pr || !ctx->hv)) {
> >>> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
> >>> return;
> >>> }
> >>
> >> Unfortunately this patch breaks qemu-system-ppc for both g3beige and
> >> mac99 under TCG causing a freeze in OpenBIOS when starting
> >> qemu-system-ppc with no parameters.
> >
> > Bother, sorry.
> >
> > I think this is because I applied this without the patch that treats
> > machines with no hypervisor mode (e.g. Apples) as always being in
> > hypervisor mode.
>
> No problem, I can cope for a couple of days or so.
Cédric,
Not sure if you've seen this thread, but one of the HV-mode patches
caused a regression on Mac. I think it's because I didn't include the
other patch which treats Apple-mode PPCs as always having HV=1.
Can you make sending your updated version of that patch a priority,
even if the rest of the batch of HV patches isn't ready yet.
> >> Note that there is also another regression that has recently landed in
> >> git master so you'll also need to revert
> >> e7c9136977cb99c6eb52c9139f7b8d8b5fa87db9 in order to get back to a
> >> functioning OpenBIOS.
> >
> > I'd preter to see it fixed rather than just reverted..
>
> Looks like the original author has found the bug, so there should be a
> fix coming up for this soon (I only included it here in case you needed
> an explicit test case).
Ok.
So, yeah, I'm not really set up to test Mac machines which means I
don't easily catch regressions like this.
Mark,
Could you look into adding a testcase to "make check" that will at
least catch these unsubtle breaks boot type regressions?
--
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] 31+ messages in thread* Re: [Qemu-devel] [Qemu-ppc] [PULL 04/12] ppc: tlbie, tlbia and tlbisync are HV only
2016-06-02 3:17 ` David Gibson
@ 2016-06-02 7:37 ` Cédric Le Goater
2016-06-02 7:45 ` Mark Cave-Ayland
2016-06-14 7:37 ` Thomas Huth
1 sibling, 1 reply; 31+ messages in thread
From: Cédric Le Goater @ 2016-06-02 7:37 UTC (permalink / raw)
To: David Gibson, Mark Cave-Ayland
Cc: peter.maydell, pbonzini, qemu-ppc, qemu-devel, bharata.rao
On 06/02/2016 05:17 AM, David Gibson wrote:
> On Wed, Jun 01, 2016 at 08:03:08AM +0100, Mark Cave-Ayland wrote:
>> On 01/06/16 03:15, David Gibson wrote:
>>
>>> On Tue, May 31, 2016 at 11:28:49PM +0100, Mark Cave-Ayland wrote:
>>>> On 31/05/16 01:41, David Gibson wrote:
>>>>
>>>>> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>>>>>
>>>>> Not that anything remotely recent supports tlbia but ...
>>>>>
>>>>> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>>>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>>>> ---
>>>>> target-ppc/translate.c | 6 +++---
>>>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
>>>>> index dfd3010..690ffd2 100644
>>>>> --- a/target-ppc/translate.c
>>>>> +++ b/target-ppc/translate.c
>>>>> @@ -4858,7 +4858,7 @@ static void gen_tlbie(DisasContext *ctx)
>>>>> #if defined(CONFIG_USER_ONLY)
>>>>> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
>>>>> #else
>>>>> - if (unlikely(ctx->pr)) {
>>>>> + if (unlikely(ctx->pr || !ctx->hv)) {
>>>>> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
>>>>> return;
>>>>> }
>>>>> @@ -4879,7 +4879,7 @@ static void gen_tlbsync(DisasContext *ctx)
>>>>> #if defined(CONFIG_USER_ONLY)
>>>>> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
>>>>> #else
>>>>> - if (unlikely(ctx->pr)) {
>>>>> + if (unlikely(ctx->pr || !ctx->hv)) {
>>>>> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
>>>>> return;
>>>>> }
>>>>> @@ -4898,7 +4898,7 @@ static void gen_slbia(DisasContext *ctx)
>>>>> #if defined(CONFIG_USER_ONLY)
>>>>> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
>>>>> #else
>>>>> - if (unlikely(ctx->pr)) {
>>>>> + if (unlikely(ctx->pr || !ctx->hv)) {
>>>>> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
>>>>> return;
>>>>> }
>>>>
>>>> Unfortunately this patch breaks qemu-system-ppc for both g3beige and
>>>> mac99 under TCG causing a freeze in OpenBIOS when starting
>>>> qemu-system-ppc with no parameters.
>>>
>>> Bother, sorry.
>>>
>>> I think this is because I applied this without the patch that treats
>>> machines with no hypervisor mode (e.g. Apples) as always being in
>>> hypervisor mode.
>>
>> No problem, I can cope for a couple of days or so.
>
> Cédric,
>
> Not sure if you've seen this thread, but one of the HV-mode patches
> caused a regression on Mac. I think it's because I didn't include the
> other patch which treats Apple-mode PPCs as always having HV=1.
I missed that as I didn't put myself in Cc :/
> Can you make sending your updated version of that patch a priority,
> even if the rest of the batch of HV patches isn't ready yet.
sure. I will/should today or tomorrow. I suppose we want these patches :
[05/12] ppc: Fix hreg_store_msr() so that non-HV mode cannot alter MSR:HV
http://patchwork.ozlabs.org/patch/618083/
[07/12] ppc: Better figure out if processor has HV mode
http://patchwork.ozlabs.org/patch/618089/
Mark,
I tried to boot a darwinppc-602.iso with :
qemu-system-ppc -M g3beige -cdrom darwinx86-602.iso -boot d
but I get a :
"No valid state has been set by load or ..."
or we don't need to go further ? may be I need a newer FW.
Could you try the two patches above please ? They apply on top of Dave's
ppc-for-2.7-20160531 and seem to have a good behavior with the small test
I could do.
Thanks,
C.
>>>> Note that there is also another regression that has recently landed in
>>>> git master so you'll also need to revert
>>>> e7c9136977cb99c6eb52c9139f7b8d8b5fa87db9 in order to get back to a
>>>> functioning OpenBIOS.
>>>
>>> I'd preter to see it fixed rather than just reverted..
>>
>> Looks like the original author has found the bug, so there should be a
>> fix coming up for this soon (I only included it here in case you needed
>> an explicit test case).
>
> Ok.
>
> So, yeah, I'm not really set up to test Mac machines which means I
> don't easily catch regressions like this.
>
> Mark,
>
> Could you look into adding a testcase to "make check" that will at
> least catch these unsubtle breaks boot type regressions?
>
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [Qemu-ppc] [PULL 04/12] ppc: tlbie, tlbia and tlbisync are HV only
2016-06-02 7:37 ` Cédric Le Goater
@ 2016-06-02 7:45 ` Mark Cave-Ayland
2016-06-02 8:23 ` Cédric Le Goater
0 siblings, 1 reply; 31+ messages in thread
From: Mark Cave-Ayland @ 2016-06-02 7:45 UTC (permalink / raw)
To: Cédric Le Goater, David Gibson
Cc: peter.maydell, pbonzini, qemu-ppc, qemu-devel, bharata.rao
On 02/06/16 08:37, Cédric Le Goater wrote:
> On 06/02/2016 05:17 AM, David Gibson wrote:
>> On Wed, Jun 01, 2016 at 08:03:08AM +0100, Mark Cave-Ayland wrote:
>>> On 01/06/16 03:15, David Gibson wrote:
>>>
>>>> On Tue, May 31, 2016 at 11:28:49PM +0100, Mark Cave-Ayland wrote:
>>>>> On 31/05/16 01:41, David Gibson wrote:
>>>>>
>>>>>> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>>>>>>
>>>>>> Not that anything remotely recent supports tlbia but ...
>>>>>>
>>>>>> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>>>>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>>>>> ---
>>>>>> target-ppc/translate.c | 6 +++---
>>>>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
>>>>>> index dfd3010..690ffd2 100644
>>>>>> --- a/target-ppc/translate.c
>>>>>> +++ b/target-ppc/translate.c
>>>>>> @@ -4858,7 +4858,7 @@ static void gen_tlbie(DisasContext *ctx)
>>>>>> #if defined(CONFIG_USER_ONLY)
>>>>>> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
>>>>>> #else
>>>>>> - if (unlikely(ctx->pr)) {
>>>>>> + if (unlikely(ctx->pr || !ctx->hv)) {
>>>>>> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
>>>>>> return;
>>>>>> }
>>>>>> @@ -4879,7 +4879,7 @@ static void gen_tlbsync(DisasContext *ctx)
>>>>>> #if defined(CONFIG_USER_ONLY)
>>>>>> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
>>>>>> #else
>>>>>> - if (unlikely(ctx->pr)) {
>>>>>> + if (unlikely(ctx->pr || !ctx->hv)) {
>>>>>> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
>>>>>> return;
>>>>>> }
>>>>>> @@ -4898,7 +4898,7 @@ static void gen_slbia(DisasContext *ctx)
>>>>>> #if defined(CONFIG_USER_ONLY)
>>>>>> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
>>>>>> #else
>>>>>> - if (unlikely(ctx->pr)) {
>>>>>> + if (unlikely(ctx->pr || !ctx->hv)) {
>>>>>> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
>>>>>> return;
>>>>>> }
>>>>>
>>>>> Unfortunately this patch breaks qemu-system-ppc for both g3beige and
>>>>> mac99 under TCG causing a freeze in OpenBIOS when starting
>>>>> qemu-system-ppc with no parameters.
>>>>
>>>> Bother, sorry.
>>>>
>>>> I think this is because I applied this without the patch that treats
>>>> machines with no hypervisor mode (e.g. Apples) as always being in
>>>> hypervisor mode.
>>>
>>> No problem, I can cope for a couple of days or so.
>>
>> Cédric,
>>
>> Not sure if you've seen this thread, but one of the HV-mode patches
>> caused a regression on Mac. I think it's because I didn't include the
>> other patch which treats Apple-mode PPCs as always having HV=1.
>
> I missed that as I didn't put myself in Cc :/
>
>> Can you make sending your updated version of that patch a priority,
>> even if the rest of the batch of HV patches isn't ready yet.
>
> sure. I will/should today or tomorrow. I suppose we want these patches :
>
> [05/12] ppc: Fix hreg_store_msr() so that non-HV mode cannot alter MSR:HV
> http://patchwork.ozlabs.org/patch/618083/
>
> [07/12] ppc: Better figure out if processor has HV mode
> http://patchwork.ozlabs.org/patch/618089/
>
>
> Mark,
>
> I tried to boot a darwinppc-602.iso with :
>
> qemu-system-ppc -M g3beige -cdrom darwinx86-602.iso -boot d
>
> but I get a :
>
> "No valid state has been set by load or ..."
>
> or we don't need to go further ? may be I need a newer FW.
Hmmm that looks like you've got an x86 ISO there which is why
OpenBIOS/PPC fails to execute the bootloader. The image I use for
testing can be found here:
https://opensource.apple.com/static/iso/darwinppc-602.cdr.gz (simply
gunzip and then rename to .iso).
> Could you try the two patches above please ? They apply on top of Dave's
> ppc-for-2.7-20160531 and seem to have a good behavior with the small test
> I could do.
I'll try and take a look tomorrow, however in the meantime see if the
above image enables you to replicate the issue locally.
ATB,
Mark.
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [Qemu-ppc] [PULL 04/12] ppc: tlbie, tlbia and tlbisync are HV only
2016-06-02 7:45 ` Mark Cave-Ayland
@ 2016-06-02 8:23 ` Cédric Le Goater
2016-06-02 8:47 ` Mark Cave-Ayland
0 siblings, 1 reply; 31+ messages in thread
From: Cédric Le Goater @ 2016-06-02 8:23 UTC (permalink / raw)
To: Mark Cave-Ayland, David Gibson
Cc: peter.maydell, pbonzini, qemu-ppc, qemu-devel, bharata.rao
On 06/02/2016 09:45 AM, Mark Cave-Ayland wrote:
> On 02/06/16 08:37, Cédric Le Goater wrote:
>> On 06/02/2016 05:17 AM, David Gibson wrote:
>>> On Wed, Jun 01, 2016 at 08:03:08AM +0100, Mark Cave-Ayland wrote:
>>>> On 01/06/16 03:15, David Gibson wrote:
>>>>
>>>>> On Tue, May 31, 2016 at 11:28:49PM +0100, Mark Cave-Ayland wrote:
>>>>>> On 31/05/16 01:41, David Gibson wrote:
>>>>>>
>>>>>>> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>>>>>>>
>>>>>>> Not that anything remotely recent supports tlbia but ...
>>>>>>>
>>>>>>> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>>>>>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>>>>>> ---
>>>>>>> target-ppc/translate.c | 6 +++---
>>>>>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>>>>>
>>>>>>> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
>>>>>>> index dfd3010..690ffd2 100644
>>>>>>> --- a/target-ppc/translate.c
>>>>>>> +++ b/target-ppc/translate.c
>>>>>>> @@ -4858,7 +4858,7 @@ static void gen_tlbie(DisasContext *ctx)
>>>>>>> #if defined(CONFIG_USER_ONLY)
>>>>>>> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
>>>>>>> #else
>>>>>>> - if (unlikely(ctx->pr)) {
>>>>>>> + if (unlikely(ctx->pr || !ctx->hv)) {
>>>>>>> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
>>>>>>> return;
>>>>>>> }
>>>>>>> @@ -4879,7 +4879,7 @@ static void gen_tlbsync(DisasContext *ctx)
>>>>>>> #if defined(CONFIG_USER_ONLY)
>>>>>>> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
>>>>>>> #else
>>>>>>> - if (unlikely(ctx->pr)) {
>>>>>>> + if (unlikely(ctx->pr || !ctx->hv)) {
>>>>>>> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
>>>>>>> return;
>>>>>>> }
>>>>>>> @@ -4898,7 +4898,7 @@ static void gen_slbia(DisasContext *ctx)
>>>>>>> #if defined(CONFIG_USER_ONLY)
>>>>>>> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
>>>>>>> #else
>>>>>>> - if (unlikely(ctx->pr)) {
>>>>>>> + if (unlikely(ctx->pr || !ctx->hv)) {
>>>>>>> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
>>>>>>> return;
>>>>>>> }
>>>>>>
>>>>>> Unfortunately this patch breaks qemu-system-ppc for both g3beige and
>>>>>> mac99 under TCG causing a freeze in OpenBIOS when starting
>>>>>> qemu-system-ppc with no parameters.
>>>>>
>>>>> Bother, sorry.
>>>>>
>>>>> I think this is because I applied this without the patch that treats
>>>>> machines with no hypervisor mode (e.g. Apples) as always being in
>>>>> hypervisor mode.
>>>>
>>>> No problem, I can cope for a couple of days or so.
>>>
>>> Cédric,
>>>
>>> Not sure if you've seen this thread, but one of the HV-mode patches
>>> caused a regression on Mac. I think it's because I didn't include the
>>> other patch which treats Apple-mode PPCs as always having HV=1.
>>
>> I missed that as I didn't put myself in Cc :/
>>
>>> Can you make sending your updated version of that patch a priority,
>>> even if the rest of the batch of HV patches isn't ready yet.
>>
>> sure. I will/should today or tomorrow. I suppose we want these patches :
>>
>> [05/12] ppc: Fix hreg_store_msr() so that non-HV mode cannot alter MSR:HV
>> http://patchwork.ozlabs.org/patch/618083/
>>
>> [07/12] ppc: Better figure out if processor has HV mode
>> http://patchwork.ozlabs.org/patch/618089/
>>
>>
>> Mark,
>>
>> I tried to boot a darwinppc-602.iso with :
>>
>> qemu-system-ppc -M g3beige -cdrom darwinx86-602.iso -boot d
>>
>> but I get a :
>>
>> "No valid state has been set by load or ..."
>>
>> or we don't need to go further ? may be I need a newer FW.
>
> Hmmm that looks like you've got an x86 ISO there which is why
> OpenBIOS/PPC fails to execute the bootloader. The image I use for
> testing can be found here:
> https://opensource.apple.com/static/iso/darwinppc-602.cdr.gz (simply
> gunzip and then rename to .iso).
Got it. much better with ppc :) ppc is not that omnipotent.
>> Could you try the two patches above please ? They apply on top of Dave's
>> ppc-for-2.7-20160531 and seem to have a good behavior with the small test
>> I could do.
>
> I'll try and take a look tomorrow, however in the meantime see if the
> above image enables you to replicate the issue locally.
so, on top of ppc-for-2.7-20160531, with your fix for :
ppc: Use split I/D mmu modes to avoid flushes on interrupts
and these two patches :
[05/12] ppc: Fix hreg_store_msr() so that non-HV mode cannot alter MSR:HV
http://patchwork.ozlabs.org/patch/618083/
[07/12] ppc: Better figure out if processor has HV mode
http://patchwork.ozlabs.org/patch/618089/
The darwin cd boots correctly up to :
...
The following devices are available for installation :
and then loops on something. But I don't get a kernel panic anymore.
Dave,
I still need to dig the !msr_pr removal.
Thanks,
C.
> ATB,
>
> Mark.
>
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [Qemu-ppc] [PULL 04/12] ppc: tlbie, tlbia and tlbisync are HV only
2016-06-02 8:23 ` Cédric Le Goater
@ 2016-06-02 8:47 ` Mark Cave-Ayland
2016-06-02 18:09 ` Mark Cave-Ayland
2016-06-03 7:12 ` David Gibson
0 siblings, 2 replies; 31+ messages in thread
From: Mark Cave-Ayland @ 2016-06-02 8:47 UTC (permalink / raw)
To: Cédric Le Goater, David Gibson
Cc: peter.maydell, pbonzini, qemu-ppc, qemu-devel, bharata.rao
On 02/06/16 09:23, Cédric Le Goater wrote:
> On 06/02/2016 09:45 AM, Mark Cave-Ayland wrote:
>> On 02/06/16 08:37, Cédric Le Goater wrote:
>>> On 06/02/2016 05:17 AM, David Gibson wrote:
>>>> On Wed, Jun 01, 2016 at 08:03:08AM +0100, Mark Cave-Ayland wrote:
>>>>> On 01/06/16 03:15, David Gibson wrote:
>>>>>
>>>>>> On Tue, May 31, 2016 at 11:28:49PM +0100, Mark Cave-Ayland wrote:
>>>>>>> On 31/05/16 01:41, David Gibson wrote:
>>>>>>>
>>>>>>>> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>>>>>>>>
>>>>>>>> Not that anything remotely recent supports tlbia but ...
>>>>>>>>
>>>>>>>> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>>>>>>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>>>>>>> ---
>>>>>>>> target-ppc/translate.c | 6 +++---
>>>>>>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
>>>>>>>> index dfd3010..690ffd2 100644
>>>>>>>> --- a/target-ppc/translate.c
>>>>>>>> +++ b/target-ppc/translate.c
>>>>>>>> @@ -4858,7 +4858,7 @@ static void gen_tlbie(DisasContext *ctx)
>>>>>>>> #if defined(CONFIG_USER_ONLY)
>>>>>>>> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
>>>>>>>> #else
>>>>>>>> - if (unlikely(ctx->pr)) {
>>>>>>>> + if (unlikely(ctx->pr || !ctx->hv)) {
>>>>>>>> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
>>>>>>>> return;
>>>>>>>> }
>>>>>>>> @@ -4879,7 +4879,7 @@ static void gen_tlbsync(DisasContext *ctx)
>>>>>>>> #if defined(CONFIG_USER_ONLY)
>>>>>>>> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
>>>>>>>> #else
>>>>>>>> - if (unlikely(ctx->pr)) {
>>>>>>>> + if (unlikely(ctx->pr || !ctx->hv)) {
>>>>>>>> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
>>>>>>>> return;
>>>>>>>> }
>>>>>>>> @@ -4898,7 +4898,7 @@ static void gen_slbia(DisasContext *ctx)
>>>>>>>> #if defined(CONFIG_USER_ONLY)
>>>>>>>> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
>>>>>>>> #else
>>>>>>>> - if (unlikely(ctx->pr)) {
>>>>>>>> + if (unlikely(ctx->pr || !ctx->hv)) {
>>>>>>>> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
>>>>>>>> return;
>>>>>>>> }
>>>>>>>
>>>>>>> Unfortunately this patch breaks qemu-system-ppc for both g3beige and
>>>>>>> mac99 under TCG causing a freeze in OpenBIOS when starting
>>>>>>> qemu-system-ppc with no parameters.
>>>>>>
>>>>>> Bother, sorry.
>>>>>>
>>>>>> I think this is because I applied this without the patch that treats
>>>>>> machines with no hypervisor mode (e.g. Apples) as always being in
>>>>>> hypervisor mode.
>>>>>
>>>>> No problem, I can cope for a couple of days or so.
>>>>
>>>> Cédric,
>>>>
>>>> Not sure if you've seen this thread, but one of the HV-mode patches
>>>> caused a regression on Mac. I think it's because I didn't include the
>>>> other patch which treats Apple-mode PPCs as always having HV=1.
>>>
>>> I missed that as I didn't put myself in Cc :/
>>>
>>>> Can you make sending your updated version of that patch a priority,
>>>> even if the rest of the batch of HV patches isn't ready yet.
>>>
>>> sure. I will/should today or tomorrow. I suppose we want these patches :
>>>
>>> [05/12] ppc: Fix hreg_store_msr() so that non-HV mode cannot alter MSR:HV
>>> http://patchwork.ozlabs.org/patch/618083/
>>>
>>> [07/12] ppc: Better figure out if processor has HV mode
>>> http://patchwork.ozlabs.org/patch/618089/
>>>
>>>
>>> Mark,
>>>
>>> I tried to boot a darwinppc-602.iso with :
>>>
>>> qemu-system-ppc -M g3beige -cdrom darwinx86-602.iso -boot d
>>>
>>> but I get a :
>>>
>>> "No valid state has been set by load or ..."
>>>
>>> or we don't need to go further ? may be I need a newer FW.
>>
>> Hmmm that looks like you've got an x86 ISO there which is why
>> OpenBIOS/PPC fails to execute the bootloader. The image I use for
>> testing can be found here:
>> https://opensource.apple.com/static/iso/darwinppc-602.cdr.gz (simply
>> gunzip and then rename to .iso).
>
> Got it. much better with ppc :) ppc is not that omnipotent.
:)
>>> Could you try the two patches above please ? They apply on top of Dave's
>>> ppc-for-2.7-20160531 and seem to have a good behavior with the small test
>>> I could do.
>>
>> I'll try and take a look tomorrow, however in the meantime see if the
>> above image enables you to replicate the issue locally.
>
>
> so, on top of ppc-for-2.7-20160531, with your fix for :
>
> ppc: Use split I/D mmu modes to avoid flushes on interrupts
Unfortunately this isn't really a fix: the whole point of splitting the
MMU modes is to be able to avoid these expensive cache flushes in the
first place. Then again it could be that this is exposing an existing
bug elsewhere...
> and these two patches :
>
> [05/12] ppc: Fix hreg_store_msr() so that non-HV mode cannot alter MSR:HV
> http://patchwork.ozlabs.org/patch/618083/
>
> [07/12] ppc: Better figure out if processor has HV mode
> http://patchwork.ozlabs.org/patch/618089/
>
> The darwin cd boots correctly up to :
>
> ...
> The following devices are available for installation :
>
> and then loops on something. But I don't get a kernel panic anymore.
Yes, that effectively matches what I see here - glad that you are now
able to reproduce this.
ATB,
Mark.
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [Qemu-ppc] [PULL 04/12] ppc: tlbie, tlbia and tlbisync are HV only
2016-06-02 8:47 ` Mark Cave-Ayland
@ 2016-06-02 18:09 ` Mark Cave-Ayland
2016-06-02 18:19 ` Cédric Le Goater
2016-06-03 7:12 ` David Gibson
1 sibling, 1 reply; 31+ messages in thread
From: Mark Cave-Ayland @ 2016-06-02 18:09 UTC (permalink / raw)
To: Cédric Le Goater, David Gibson
Cc: peter.maydell, bharata.rao, qemu-ppc, qemu-devel, pbonzini
On 02/06/16 09:47, Mark Cave-Ayland wrote:
> On 02/06/16 09:23, Cédric Le Goater wrote:
>
>> On 06/02/2016 09:45 AM, Mark Cave-Ayland wrote:
>>> On 02/06/16 08:37, Cédric Le Goater wrote:
>>>> On 06/02/2016 05:17 AM, David Gibson wrote:
>>>>> On Wed, Jun 01, 2016 at 08:03:08AM +0100, Mark Cave-Ayland wrote:
>>>>>> On 01/06/16 03:15, David Gibson wrote:
>>>>>>
>>>>>>> On Tue, May 31, 2016 at 11:28:49PM +0100, Mark Cave-Ayland wrote:
>>>>>>>> On 31/05/16 01:41, David Gibson wrote:
>>>>>>>>
>>>>>>>>> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>>>>>>>>>
>>>>>>>>> Not that anything remotely recent supports tlbia but ...
>>>>>>>>>
>>>>>>>>> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>>>>>>>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>>>>>>>> ---
>>>>>>>>> target-ppc/translate.c | 6 +++---
>>>>>>>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
>>>>>>>>> index dfd3010..690ffd2 100644
>>>>>>>>> --- a/target-ppc/translate.c
>>>>>>>>> +++ b/target-ppc/translate.c
>>>>>>>>> @@ -4858,7 +4858,7 @@ static void gen_tlbie(DisasContext *ctx)
>>>>>>>>> #if defined(CONFIG_USER_ONLY)
>>>>>>>>> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
>>>>>>>>> #else
>>>>>>>>> - if (unlikely(ctx->pr)) {
>>>>>>>>> + if (unlikely(ctx->pr || !ctx->hv)) {
>>>>>>>>> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
>>>>>>>>> return;
>>>>>>>>> }
>>>>>>>>> @@ -4879,7 +4879,7 @@ static void gen_tlbsync(DisasContext *ctx)
>>>>>>>>> #if defined(CONFIG_USER_ONLY)
>>>>>>>>> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
>>>>>>>>> #else
>>>>>>>>> - if (unlikely(ctx->pr)) {
>>>>>>>>> + if (unlikely(ctx->pr || !ctx->hv)) {
>>>>>>>>> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
>>>>>>>>> return;
>>>>>>>>> }
>>>>>>>>> @@ -4898,7 +4898,7 @@ static void gen_slbia(DisasContext *ctx)
>>>>>>>>> #if defined(CONFIG_USER_ONLY)
>>>>>>>>> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
>>>>>>>>> #else
>>>>>>>>> - if (unlikely(ctx->pr)) {
>>>>>>>>> + if (unlikely(ctx->pr || !ctx->hv)) {
>>>>>>>>> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
>>>>>>>>> return;
>>>>>>>>> }
>>>>>>>>
>>>>>>>> Unfortunately this patch breaks qemu-system-ppc for both g3beige and
>>>>>>>> mac99 under TCG causing a freeze in OpenBIOS when starting
>>>>>>>> qemu-system-ppc with no parameters.
>>>>>>>
>>>>>>> Bother, sorry.
>>>>>>>
>>>>>>> I think this is because I applied this without the patch that treats
>>>>>>> machines with no hypervisor mode (e.g. Apples) as always being in
>>>>>>> hypervisor mode.
>>>>>>
>>>>>> No problem, I can cope for a couple of days or so.
>>>>>
>>>>> Cédric,
>>>>>
>>>>> Not sure if you've seen this thread, but one of the HV-mode patches
>>>>> caused a regression on Mac. I think it's because I didn't include the
>>>>> other patch which treats Apple-mode PPCs as always having HV=1.
>>>>
>>>> I missed that as I didn't put myself in Cc :/
>>>>
>>>>> Can you make sending your updated version of that patch a priority,
>>>>> even if the rest of the batch of HV patches isn't ready yet.
>>>>
>>>> sure. I will/should today or tomorrow. I suppose we want these patches :
>>>>
>>>> [05/12] ppc: Fix hreg_store_msr() so that non-HV mode cannot alter MSR:HV
>>>> http://patchwork.ozlabs.org/patch/618083/
>>>>
>>>> [07/12] ppc: Better figure out if processor has HV mode
>>>> http://patchwork.ozlabs.org/patch/618089/
>>>>
>>>>
>>>> Mark,
>>>>
>>>> I tried to boot a darwinppc-602.iso with :
>>>>
>>>> qemu-system-ppc -M g3beige -cdrom darwinx86-602.iso -boot d
>>>>
>>>> but I get a :
>>>>
>>>> "No valid state has been set by load or ..."
>>>>
>>>> or we don't need to go further ? may be I need a newer FW.
>>>
>>> Hmmm that looks like you've got an x86 ISO there which is why
>>> OpenBIOS/PPC fails to execute the bootloader. The image I use for
>>> testing can be found here:
>>> https://opensource.apple.com/static/iso/darwinppc-602.cdr.gz (simply
>>> gunzip and then rename to .iso).
>>
>> Got it. much better with ppc :) ppc is not that omnipotent.
>
> :)
>
>>>> Could you try the two patches above please ? They apply on top of Dave's
>>>> ppc-for-2.7-20160531 and seem to have a good behavior with the small test
>>>> I could do.
>>>
>>> I'll try and take a look tomorrow, however in the meantime see if the
>>> above image enables you to replicate the issue locally.
>>
>>
>> so, on top of ppc-for-2.7-20160531, with your fix for :
>>
>> ppc: Use split I/D mmu modes to avoid flushes on interrupts
>
> Unfortunately this isn't really a fix: the whole point of splitting the
> MMU modes is to be able to avoid these expensive cache flushes in the
> first place. Then again it could be that this is exposing an existing
> bug elsewhere...
>
>> and these two patches :
>>
>> [05/12] ppc: Fix hreg_store_msr() so that non-HV mode cannot alter MSR:HV
>> http://patchwork.ozlabs.org/patch/618083/
>>
>> [07/12] ppc: Better figure out if processor has HV mode
>> http://patchwork.ozlabs.org/patch/618089/
>>
>> The darwin cd boots correctly up to :
>>
>> ...
>> The following devices are available for installation :
>>
>> and then loops on something. But I don't get a kernel panic anymore.
>
> Yes, that effectively matches what I see here - glad that you are now
> able to reproduce this.
Just to add in case it wasn't clear from my previous reply - this is
actually the correct and expected behaviour. If you add a hard disk
device with -hda then it will appear on-screen below the prompt at which
point you can proceed with the installation as normal.
ATB,
Mark.
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [Qemu-ppc] [PULL 04/12] ppc: tlbie, tlbia and tlbisync are HV only
2016-06-02 18:09 ` Mark Cave-Ayland
@ 2016-06-02 18:19 ` Cédric Le Goater
0 siblings, 0 replies; 31+ messages in thread
From: Cédric Le Goater @ 2016-06-02 18:19 UTC (permalink / raw)
To: Mark Cave-Ayland, David Gibson
Cc: peter.maydell, bharata.rao, qemu-ppc, qemu-devel, pbonzini
On 06/02/2016 08:09 PM, Mark Cave-Ayland wrote:
> On 02/06/16 09:47, Mark Cave-Ayland wrote:
>
>> On 02/06/16 09:23, Cédric Le Goater wrote:
>>
>>> On 06/02/2016 09:45 AM, Mark Cave-Ayland wrote:
>>>> On 02/06/16 08:37, Cédric Le Goater wrote:
>>>>> On 06/02/2016 05:17 AM, David Gibson wrote:
>>>>>> On Wed, Jun 01, 2016 at 08:03:08AM +0100, Mark Cave-Ayland wrote:
>>>>>>> On 01/06/16 03:15, David Gibson wrote:
>>>>>>>
>>>>>>>> On Tue, May 31, 2016 at 11:28:49PM +0100, Mark Cave-Ayland wrote:
>>>>>>>>> On 31/05/16 01:41, David Gibson wrote:
>>>>>>>>>
>>>>>>>>>> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>>>>>>>>>>
>>>>>>>>>> Not that anything remotely recent supports tlbia but ...
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>>>>>>>>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>>>>>>>>> ---
>>>>>>>>>> target-ppc/translate.c | 6 +++---
>>>>>>>>>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
>>>>>>>>>> index dfd3010..690ffd2 100644
>>>>>>>>>> --- a/target-ppc/translate.c
>>>>>>>>>> +++ b/target-ppc/translate.c
>>>>>>>>>> @@ -4858,7 +4858,7 @@ static void gen_tlbie(DisasContext *ctx)
>>>>>>>>>> #if defined(CONFIG_USER_ONLY)
>>>>>>>>>> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
>>>>>>>>>> #else
>>>>>>>>>> - if (unlikely(ctx->pr)) {
>>>>>>>>>> + if (unlikely(ctx->pr || !ctx->hv)) {
>>>>>>>>>> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
>>>>>>>>>> return;
>>>>>>>>>> }
>>>>>>>>>> @@ -4879,7 +4879,7 @@ static void gen_tlbsync(DisasContext *ctx)
>>>>>>>>>> #if defined(CONFIG_USER_ONLY)
>>>>>>>>>> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
>>>>>>>>>> #else
>>>>>>>>>> - if (unlikely(ctx->pr)) {
>>>>>>>>>> + if (unlikely(ctx->pr || !ctx->hv)) {
>>>>>>>>>> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
>>>>>>>>>> return;
>>>>>>>>>> }
>>>>>>>>>> @@ -4898,7 +4898,7 @@ static void gen_slbia(DisasContext *ctx)
>>>>>>>>>> #if defined(CONFIG_USER_ONLY)
>>>>>>>>>> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
>>>>>>>>>> #else
>>>>>>>>>> - if (unlikely(ctx->pr)) {
>>>>>>>>>> + if (unlikely(ctx->pr || !ctx->hv)) {
>>>>>>>>>> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
>>>>>>>>>> return;
>>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> Unfortunately this patch breaks qemu-system-ppc for both g3beige and
>>>>>>>>> mac99 under TCG causing a freeze in OpenBIOS when starting
>>>>>>>>> qemu-system-ppc with no parameters.
>>>>>>>>
>>>>>>>> Bother, sorry.
>>>>>>>>
>>>>>>>> I think this is because I applied this without the patch that treats
>>>>>>>> machines with no hypervisor mode (e.g. Apples) as always being in
>>>>>>>> hypervisor mode.
>>>>>>>
>>>>>>> No problem, I can cope for a couple of days or so.
>>>>>>
>>>>>> Cédric,
>>>>>>
>>>>>> Not sure if you've seen this thread, but one of the HV-mode patches
>>>>>> caused a regression on Mac. I think it's because I didn't include the
>>>>>> other patch which treats Apple-mode PPCs as always having HV=1.
>>>>>
>>>>> I missed that as I didn't put myself in Cc :/
>>>>>
>>>>>> Can you make sending your updated version of that patch a priority,
>>>>>> even if the rest of the batch of HV patches isn't ready yet.
>>>>>
>>>>> sure. I will/should today or tomorrow. I suppose we want these patches :
>>>>>
>>>>> [05/12] ppc: Fix hreg_store_msr() so that non-HV mode cannot alter MSR:HV
>>>>> http://patchwork.ozlabs.org/patch/618083/
>>>>>
>>>>> [07/12] ppc: Better figure out if processor has HV mode
>>>>> http://patchwork.ozlabs.org/patch/618089/
>>>>>
>>>>>
>>>>> Mark,
>>>>>
>>>>> I tried to boot a darwinppc-602.iso with :
>>>>>
>>>>> qemu-system-ppc -M g3beige -cdrom darwinx86-602.iso -boot d
>>>>>
>>>>> but I get a :
>>>>>
>>>>> "No valid state has been set by load or ..."
>>>>>
>>>>> or we don't need to go further ? may be I need a newer FW.
>>>>
>>>> Hmmm that looks like you've got an x86 ISO there which is why
>>>> OpenBIOS/PPC fails to execute the bootloader. The image I use for
>>>> testing can be found here:
>>>> https://opensource.apple.com/static/iso/darwinppc-602.cdr.gz (simply
>>>> gunzip and then rename to .iso).
>>>
>>> Got it. much better with ppc :) ppc is not that omnipotent.
>>
>> :)
>>
>>>>> Could you try the two patches above please ? They apply on top of Dave's
>>>>> ppc-for-2.7-20160531 and seem to have a good behavior with the small test
>>>>> I could do.
>>>>
>>>> I'll try and take a look tomorrow, however in the meantime see if the
>>>> above image enables you to replicate the issue locally.
>>>
>>>
>>> so, on top of ppc-for-2.7-20160531, with your fix for :
>>>
>>> ppc: Use split I/D mmu modes to avoid flushes on interrupts
>>
>> Unfortunately this isn't really a fix: the whole point of splitting the
>> MMU modes is to be able to avoid these expensive cache flushes in the
>> first place. Then again it could be that this is exposing an existing
>> bug elsewhere...
>>
>>> and these two patches :
>>>
>>> [05/12] ppc: Fix hreg_store_msr() so that non-HV mode cannot alter MSR:HV
>>> http://patchwork.ozlabs.org/patch/618083/
>>>
>>> [07/12] ppc: Better figure out if processor has HV mode
>>> http://patchwork.ozlabs.org/patch/618089/
>>>
>>> The darwin cd boots correctly up to :
>>>
>>> ...
>>> The following devices are available for installation :
>>>
>>> and then loops on something. But I don't get a kernel panic anymore.
>>
>> Yes, that effectively matches what I see here - glad that you are now
>> able to reproduce this.
>
> Just to add in case it wasn't clear from my previous reply - this is
> actually the correct and expected behaviour. If you add a hard disk
> device with -hda then it will appear on-screen below the prompt at which
> point you can proceed with the installation as normal.
yes. I tried that and it did.
C.
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [PULL 04/12] ppc: tlbie, tlbia and tlbisync are HV only
2016-06-02 8:47 ` Mark Cave-Ayland
2016-06-02 18:09 ` Mark Cave-Ayland
@ 2016-06-03 7:12 ` David Gibson
1 sibling, 0 replies; 31+ messages in thread
From: David Gibson @ 2016-06-03 7:12 UTC (permalink / raw)
To: Mark Cave-Ayland
Cc: Cédric Le Goater, peter.maydell, pbonzini, qemu-ppc,
qemu-devel, bharata.rao
[-- Attachment #1: Type: text/plain, Size: 6144 bytes --]
On Thu, Jun 02, 2016 at 09:47:01AM +0100, Mark Cave-Ayland wrote:
> On 02/06/16 09:23, Cédric Le Goater wrote:
>
> > On 06/02/2016 09:45 AM, Mark Cave-Ayland wrote:
> >> On 02/06/16 08:37, Cédric Le Goater wrote:
> >>> On 06/02/2016 05:17 AM, David Gibson wrote:
> >>>> On Wed, Jun 01, 2016 at 08:03:08AM +0100, Mark Cave-Ayland wrote:
> >>>>> On 01/06/16 03:15, David Gibson wrote:
> >>>>>
> >>>>>> On Tue, May 31, 2016 at 11:28:49PM +0100, Mark Cave-Ayland wrote:
> >>>>>>> On 31/05/16 01:41, David Gibson wrote:
> >>>>>>>
> >>>>>>>> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> >>>>>>>>
> >>>>>>>> Not that anything remotely recent supports tlbia but ...
> >>>>>>>>
> >>>>>>>> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> >>>>>>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> >>>>>>>> ---
> >>>>>>>> target-ppc/translate.c | 6 +++---
> >>>>>>>> 1 file changed, 3 insertions(+), 3 deletions(-)
> >>>>>>>>
> >>>>>>>> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
> >>>>>>>> index dfd3010..690ffd2 100644
> >>>>>>>> --- a/target-ppc/translate.c
> >>>>>>>> +++ b/target-ppc/translate.c
> >>>>>>>> @@ -4858,7 +4858,7 @@ static void gen_tlbie(DisasContext *ctx)
> >>>>>>>> #if defined(CONFIG_USER_ONLY)
> >>>>>>>> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
> >>>>>>>> #else
> >>>>>>>> - if (unlikely(ctx->pr)) {
> >>>>>>>> + if (unlikely(ctx->pr || !ctx->hv)) {
> >>>>>>>> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
> >>>>>>>> return;
> >>>>>>>> }
> >>>>>>>> @@ -4879,7 +4879,7 @@ static void gen_tlbsync(DisasContext *ctx)
> >>>>>>>> #if defined(CONFIG_USER_ONLY)
> >>>>>>>> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
> >>>>>>>> #else
> >>>>>>>> - if (unlikely(ctx->pr)) {
> >>>>>>>> + if (unlikely(ctx->pr || !ctx->hv)) {
> >>>>>>>> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
> >>>>>>>> return;
> >>>>>>>> }
> >>>>>>>> @@ -4898,7 +4898,7 @@ static void gen_slbia(DisasContext *ctx)
> >>>>>>>> #if defined(CONFIG_USER_ONLY)
> >>>>>>>> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
> >>>>>>>> #else
> >>>>>>>> - if (unlikely(ctx->pr)) {
> >>>>>>>> + if (unlikely(ctx->pr || !ctx->hv)) {
> >>>>>>>> gen_inval_exception(ctx, POWERPC_EXCP_PRIV_OPC);
> >>>>>>>> return;
> >>>>>>>> }
> >>>>>>>
> >>>>>>> Unfortunately this patch breaks qemu-system-ppc for both g3beige and
> >>>>>>> mac99 under TCG causing a freeze in OpenBIOS when starting
> >>>>>>> qemu-system-ppc with no parameters.
> >>>>>>
> >>>>>> Bother, sorry.
> >>>>>>
> >>>>>> I think this is because I applied this without the patch that treats
> >>>>>> machines with no hypervisor mode (e.g. Apples) as always being in
> >>>>>> hypervisor mode.
> >>>>>
> >>>>> No problem, I can cope for a couple of days or so.
> >>>>
> >>>> Cédric,
> >>>>
> >>>> Not sure if you've seen this thread, but one of the HV-mode patches
> >>>> caused a regression on Mac. I think it's because I didn't include the
> >>>> other patch which treats Apple-mode PPCs as always having HV=1.
> >>>
> >>> I missed that as I didn't put myself in Cc :/
> >>>
> >>>> Can you make sending your updated version of that patch a priority,
> >>>> even if the rest of the batch of HV patches isn't ready yet.
> >>>
> >>> sure. I will/should today or tomorrow. I suppose we want these patches :
> >>>
> >>> [05/12] ppc: Fix hreg_store_msr() so that non-HV mode cannot alter MSR:HV
> >>> http://patchwork.ozlabs.org/patch/618083/
> >>>
> >>> [07/12] ppc: Better figure out if processor has HV mode
> >>> http://patchwork.ozlabs.org/patch/618089/
> >>>
> >>>
> >>> Mark,
> >>>
> >>> I tried to boot a darwinppc-602.iso with :
> >>>
> >>> qemu-system-ppc -M g3beige -cdrom darwinx86-602.iso -boot d
> >>>
> >>> but I get a :
> >>>
> >>> "No valid state has been set by load or ..."
> >>>
> >>> or we don't need to go further ? may be I need a newer FW.
> >>
> >> Hmmm that looks like you've got an x86 ISO there which is why
> >> OpenBIOS/PPC fails to execute the bootloader. The image I use for
> >> testing can be found here:
> >> https://opensource.apple.com/static/iso/darwinppc-602.cdr.gz (simply
> >> gunzip and then rename to .iso).
> >
> > Got it. much better with ppc :) ppc is not that omnipotent.
>
> :)
>
> >>> Could you try the two patches above please ? They apply on top of Dave's
> >>> ppc-for-2.7-20160531 and seem to have a good behavior with the small test
> >>> I could do.
> >>
> >> I'll try and take a look tomorrow, however in the meantime see if the
> >> above image enables you to replicate the issue locally.
> >
> >
> > so, on top of ppc-for-2.7-20160531, with your fix for :
> >
> > ppc: Use split I/D mmu modes to avoid flushes on interrupts
>
> Unfortunately this isn't really a fix: the whole point of splitting the
> MMU modes is to be able to avoid these expensive cache flushes in the
> first place.
Yeah, the "fix" makes the I/D split patch basically worthless.
> Then again it could be that this is exposing an existing
> bug elsewhere...
I strongly suspect that's the case, we just need to work out what.
>
> > and these two patches :
> >
> > [05/12] ppc: Fix hreg_store_msr() so that non-HV mode cannot alter MSR:HV
> > http://patchwork.ozlabs.org/patch/618083/
> >
> > [07/12] ppc: Better figure out if processor has HV mode
> > http://patchwork.ozlabs.org/patch/618089/
> >
> > The darwin cd boots correctly up to :
> >
> > ...
> > The following devices are available for installation :
> >
> > and then loops on something. But I don't get a kernel panic anymore.
>
> Yes, that effectively matches what I see here - glad that you are now
> able to reproduce this.
>
>
> ATB,
>
> Mark.
>
--
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] 31+ messages in thread
* Re: [Qemu-devel] [Qemu-ppc] [PULL 04/12] ppc: tlbie, tlbia and tlbisync are HV only
2016-06-02 3:17 ` David Gibson
2016-06-02 7:37 ` Cédric Le Goater
@ 2016-06-14 7:37 ` Thomas Huth
1 sibling, 0 replies; 31+ messages in thread
From: Thomas Huth @ 2016-06-14 7:37 UTC (permalink / raw)
To: David Gibson, Mark Cave-Ayland; +Cc: qemu-devel, qemu-ppc
[-- Attachment #1: Type: text/plain, Size: 1420 bytes --]
On 02.06.2016 05:17, David Gibson wrote:
> On Wed, Jun 01, 2016 at 08:03:08AM +0100, Mark Cave-Ayland wrote:
>> On 01/06/16 03:15, David Gibson wrote:
>>
>>> On Tue, May 31, 2016 at 11:28:49PM +0100, Mark Cave-Ayland wrote:
[...]
>>>> Note that there is also another regression that has recently landed in
>>>> git master so you'll also need to revert
>>>> e7c9136977cb99c6eb52c9139f7b8d8b5fa87db9 in order to get back to a
>>>> functioning OpenBIOS.
>>>
>>> I'd preter to see it fixed rather than just reverted..
>>
>> Looks like the original author has found the bug, so there should be a
>> fix coming up for this soon (I only included it here in case you needed
>> an explicit test case).
>
> Ok.
>
> So, yeah, I'm not really set up to test Mac machines which means I
> don't easily catch regressions like this.
>
> Mark,
>
> Could you look into adding a testcase to "make check" that will at
> least catch these unsubtle breaks boot type regressions?
I think I've just found a nice way to check whether the OpenBIOS
machines can at least successfully run through the OpenBIOS boot
sequence: You can use the "-prom-env" parameter of QEMU to execute some
Forth code there, so this can be used to signal a successful test to the
qtest environment.
Unless you've got a better test in the works already, I polish up my
patch and submit it later today or tomorrow...
Thomas
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 31+ messages in thread
* [Qemu-devel] [PULL 05/12] ppc: Change 'invalid' bit mask of tlbiel and tlbie
2016-05-31 0:41 [Qemu-devel] [PULL 00/12] ppc-for-2.7 queue 20160531 David Gibson
` (3 preceding siblings ...)
2016-05-31 0:41 ` [Qemu-devel] [PULL 04/12] ppc: tlbie, tlbia and tlbisync are HV only David Gibson
@ 2016-05-31 0:41 ` David Gibson
2016-05-31 0:41 ` [Qemu-devel] [PULL 06/12] ppc: Fix sign extension issue in mtmsr(d) emulation David Gibson
` (7 subsequent siblings)
12 siblings, 0 replies; 31+ messages in thread
From: David Gibson @ 2016-05-31 0:41 UTC (permalink / raw)
To: peter.maydell
Cc: agraf, pbonzini, bharata.rao, benh, qemu-ppc, qemu-devel,
David Gibson
From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Otherwise it will trip on the forms used in recent architecture.
Ideally, we should have different handlers for different architecture
levels but our current implementation of TLB flushing is dumb enough
that this will do for now.
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
target-ppc/translate.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index 690ffd2..868ef31 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -9946,8 +9946,10 @@ GEN_HANDLER2(slbmfee, "slbmfee", 0x1F, 0x13, 0x1C, 0x001F0001, PPC_SEGMENT_64B),
GEN_HANDLER2(slbmfev, "slbmfev", 0x1F, 0x13, 0x1A, 0x001F0001, PPC_SEGMENT_64B),
#endif
GEN_HANDLER(tlbia, 0x1F, 0x12, 0x0B, 0x03FFFC01, PPC_MEM_TLBIA),
-GEN_HANDLER(tlbiel, 0x1F, 0x12, 0x08, 0x03FF0001, PPC_MEM_TLBIE),
-GEN_HANDLER(tlbie, 0x1F, 0x12, 0x09, 0x03FF0001, PPC_MEM_TLBIE),
+/* XXX Those instructions will need to be handled differently for
+ * different ISA versions */
+GEN_HANDLER(tlbiel, 0x1F, 0x12, 0x08, 0x001F0001, PPC_MEM_TLBIE),
+GEN_HANDLER(tlbie, 0x1F, 0x12, 0x09, 0x001F0001, PPC_MEM_TLBIE),
GEN_HANDLER(tlbsync, 0x1F, 0x16, 0x11, 0x03FFF801, PPC_MEM_TLBSYNC),
#if defined(TARGET_PPC64)
GEN_HANDLER(slbia, 0x1F, 0x12, 0x0F, 0x03FFFC01, PPC_SLBI),
--
2.5.5
^ permalink raw reply related [flat|nested] 31+ messages in thread* [Qemu-devel] [PULL 06/12] ppc: Fix sign extension issue in mtmsr(d) emulation
2016-05-31 0:41 [Qemu-devel] [PULL 00/12] ppc-for-2.7 queue 20160531 David Gibson
` (4 preceding siblings ...)
2016-05-31 0:41 ` [Qemu-devel] [PULL 05/12] ppc: Change 'invalid' bit mask of tlbiel and tlbie David Gibson
@ 2016-05-31 0:41 ` David Gibson
2016-05-31 0:41 ` [Qemu-devel] [PULL 07/12] ppc: Get out of emulation on SMT "OR" ops David Gibson
` (6 subsequent siblings)
12 siblings, 0 replies; 31+ messages in thread
From: David Gibson @ 2016-05-31 0:41 UTC (permalink / raw)
To: peter.maydell
Cc: agraf, pbonzini, bharata.rao, benh, qemu-ppc, qemu-devel,
Michael Neuling, David Gibson
From: Michael Neuling <mikey@neuling.org>
Signed-off-by: Michael Neuling <mikey@neuling.org>
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
target-ppc/translate.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index 868ef31..51f6eb1 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -4381,7 +4381,7 @@ static void gen_mtmsrd(DisasContext *ctx)
/* Special form that does not need any synchronisation */
TCGv t0 = tcg_temp_new();
tcg_gen_andi_tl(t0, cpu_gpr[rS(ctx->opcode)], (1 << MSR_RI) | (1 << MSR_EE));
- tcg_gen_andi_tl(cpu_msr, cpu_msr, ~((1 << MSR_RI) | (1 << MSR_EE)));
+ tcg_gen_andi_tl(cpu_msr, cpu_msr, ~(target_ulong)((1 << MSR_RI) | (1 << MSR_EE)));
tcg_gen_or_tl(cpu_msr, cpu_msr, t0);
tcg_temp_free(t0);
} else {
@@ -4412,7 +4412,7 @@ static void gen_mtmsr(DisasContext *ctx)
/* Special form that does not need any synchronisation */
TCGv t0 = tcg_temp_new();
tcg_gen_andi_tl(t0, cpu_gpr[rS(ctx->opcode)], (1 << MSR_RI) | (1 << MSR_EE));
- tcg_gen_andi_tl(cpu_msr, cpu_msr, ~((1 << MSR_RI) | (1 << MSR_EE)));
+ tcg_gen_andi_tl(cpu_msr, cpu_msr, ~(target_ulong)((1 << MSR_RI) | (1 << MSR_EE)));
tcg_gen_or_tl(cpu_msr, cpu_msr, t0);
tcg_temp_free(t0);
} else {
--
2.5.5
^ permalink raw reply related [flat|nested] 31+ messages in thread* [Qemu-devel] [PULL 07/12] ppc: Get out of emulation on SMT "OR" ops
2016-05-31 0:41 [Qemu-devel] [PULL 00/12] ppc-for-2.7 queue 20160531 David Gibson
` (5 preceding siblings ...)
2016-05-31 0:41 ` [Qemu-devel] [PULL 06/12] ppc: Fix sign extension issue in mtmsr(d) emulation David Gibson
@ 2016-05-31 0:41 ` David Gibson
2025-08-27 14:07 ` Philippe Mathieu-Daudé
2016-05-31 0:41 ` [Qemu-devel] [PULL 08/12] ppc: Add PPC_64H instruction flag to POWER7 and POWER8 David Gibson
` (5 subsequent siblings)
12 siblings, 1 reply; 31+ messages in thread
From: David Gibson @ 2016-05-31 0:41 UTC (permalink / raw)
To: peter.maydell
Cc: agraf, pbonzini, bharata.rao, benh, qemu-ppc, qemu-devel,
David Gibson
From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Otherwise tight loops at smt_low for example, which OPAL does,
eat so much CPU that we can't boot a kernel anymore. With that,
I can boot 8 CPUs just fine with powernv.
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
target-ppc/translate.c | 21 ++++++++++++++++++---
1 file changed, 18 insertions(+), 3 deletions(-)
diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index 51f6eb1..fe10bf8 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -1392,6 +1392,19 @@ GEN_LOGICAL2(nand, tcg_gen_nand_tl, 0x0E, PPC_INTEGER);
/* nor & nor. */
GEN_LOGICAL2(nor, tcg_gen_nor_tl, 0x03, PPC_INTEGER);
+#if defined(TARGET_PPC64)
+static void gen_pause(DisasContext *ctx)
+{
+ TCGv_i32 t0 = tcg_const_i32(0);
+ tcg_gen_st_i32(t0, cpu_env,
+ -offsetof(PowerPCCPU, env) + offsetof(CPUState, halted));
+ tcg_temp_free_i32(t0);
+
+ /* Stop translation, this gives other CPUs a chance to run */
+ gen_exception_err(ctx, EXCP_HLT, 1);
+}
+#endif /* defined(TARGET_PPC64) */
+
/* or & or. */
static void gen_or(DisasContext *ctx)
{
@@ -1447,7 +1460,7 @@ static void gen_or(DisasContext *ctx)
}
break;
case 7:
- if (ctx->hv) {
+ if (ctx->hv && !ctx->pr) {
/* Set process priority to very high */
prio = 7;
}
@@ -1464,6 +1477,10 @@ static void gen_or(DisasContext *ctx)
tcg_gen_ori_tl(t0, t0, ((uint64_t)prio) << 50);
gen_store_spr(SPR_PPR, t0);
tcg_temp_free(t0);
+ /* Pause us out of TCG otherwise spin loops with smt_low
+ * eat too much CPU and the kernel hangs
+ */
+ gen_pause(ctx);
}
#endif
}
@@ -1489,8 +1506,6 @@ static void gen_ori(DisasContext *ctx)
target_ulong uimm = UIMM(ctx->opcode);
if (rS(ctx->opcode) == rA(ctx->opcode) && uimm == 0) {
- /* NOP */
- /* XXX: should handle special NOPs for POWER series */
return;
}
tcg_gen_ori_tl(cpu_gpr[rA(ctx->opcode)], cpu_gpr[rS(ctx->opcode)], uimm);
--
2.5.5
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PULL 07/12] ppc: Get out of emulation on SMT "OR" ops
2016-05-31 0:41 ` [Qemu-devel] [PULL 07/12] ppc: Get out of emulation on SMT "OR" ops David Gibson
@ 2025-08-27 14:07 ` Philippe Mathieu-Daudé
2025-09-24 17:19 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 31+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-08-27 14:07 UTC (permalink / raw)
To: benh, qemu-ppc
Cc: agraf, David Gibson, pbonzini, peter.maydell, bharata.rao,
qemu-devel, Nicholas Piggin
Hi,
(old patch merged as commit b68e60e6f0d2865e961a800fb8db96a7fc6494c4)
On 31/5/16 02:41, David Gibson wrote:
> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>
> Otherwise tight loops at smt_low for example, which OPAL does,
> eat so much CPU that we can't boot a kernel anymore. With that,
> I can boot 8 CPUs just fine with powernv.
>
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> target-ppc/translate.c | 21 ++++++++++++++++++---
> 1 file changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
> index 51f6eb1..fe10bf8 100644
> --- a/target-ppc/translate.c
> +++ b/target-ppc/translate.c
> @@ -1392,6 +1392,19 @@ GEN_LOGICAL2(nand, tcg_gen_nand_tl, 0x0E, PPC_INTEGER);
> /* nor & nor. */
> GEN_LOGICAL2(nor, tcg_gen_nor_tl, 0x03, PPC_INTEGER);
>
> +#if defined(TARGET_PPC64)
> +static void gen_pause(DisasContext *ctx)
> +{
> + TCGv_i32 t0 = tcg_const_i32(0);
> + tcg_gen_st_i32(t0, cpu_env,
> + -offsetof(PowerPCCPU, env) + offsetof(CPUState, halted));
This sets CPUState::halted = 0, as unhalted state (a.k.a. "running"),
is that expected? I'd expect "Pause" to halt the vCPU to avoid spinning
(as mentioned below).
> + tcg_temp_free_i32(t0);
> +
> + /* Stop translation, this gives other CPUs a chance to run */
> + gen_exception_err(ctx, EXCP_HLT, 1);
> +}
> +#endif /* defined(TARGET_PPC64) */
> +
> /* or & or. */
> static void gen_or(DisasContext *ctx)
> {
> @@ -1447,7 +1460,7 @@ static void gen_or(DisasContext *ctx)
> }
> break;
> case 7:
> - if (ctx->hv) {
> + if (ctx->hv && !ctx->pr) {
> /* Set process priority to very high */
> prio = 7;
> }
> @@ -1464,6 +1477,10 @@ static void gen_or(DisasContext *ctx)
> tcg_gen_ori_tl(t0, t0, ((uint64_t)prio) << 50);
> gen_store_spr(SPR_PPR, t0);
> tcg_temp_free(t0);
> + /* Pause us out of TCG otherwise spin loops with smt_low
> + * eat too much CPU and the kernel hangs
> + */
> + gen_pause(ctx);
> }
> #endif
> }
> @@ -1489,8 +1506,6 @@ static void gen_ori(DisasContext *ctx)
> target_ulong uimm = UIMM(ctx->opcode);
>
> if (rS(ctx->opcode) == rA(ctx->opcode) && uimm == 0) {
> - /* NOP */
> - /* XXX: should handle special NOPs for POWER series */
> return;
> }
> tcg_gen_ori_tl(cpu_gpr[rA(ctx->opcode)], cpu_gpr[rS(ctx->opcode)], uimm);
Note there is a similar code in gen_wait() doing the same (but
correctlyIMO):
3366 /*
3367 * wait without WC field or with WC=0 waits for an exception /
interrupt
3368 * to occur.
3369 */
3370 if (wc == 0) {
3371 TCGv_i32 t0 = tcg_constant_i32(1);
3372 tcg_gen_st_i32(t0, tcg_env,
3373 -offsetof(PowerPCCPU, env) +
offsetof(CPUState, halted));
3374 /* Stop translation, as the CPU is supposed to sleep from now */
3375 gen_exception_nip(ctx, EXCP_HLT, ctx->base.pc_next);
3376 }
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PULL 07/12] ppc: Get out of emulation on SMT "OR" ops
2025-08-27 14:07 ` Philippe Mathieu-Daudé
@ 2025-09-24 17:19 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 31+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-09-24 17:19 UTC (permalink / raw)
To: benh, qemu-ppc
Cc: agraf, David Gibson, pbonzini, peter.maydell, bharata.rao,
qemu-devel, Nicholas Piggin
On 27/8/25 16:07, Philippe Mathieu-Daudé wrote:
> Hi,
>
> (old patch merged as commit b68e60e6f0d2865e961a800fb8db96a7fc6494c4)
>
> On 31/5/16 02:41, David Gibson wrote:
>> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>>
>> Otherwise tight loops at smt_low for example, which OPAL does,
>> eat so much CPU that we can't boot a kernel anymore. With that,
>> I can boot 8 CPUs just fine with powernv.
>>
>> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>> ---
>> target-ppc/translate.c | 21 ++++++++++++++++++---
>> 1 file changed, 18 insertions(+), 3 deletions(-)
>>
>> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
>> index 51f6eb1..fe10bf8 100644
>> --- a/target-ppc/translate.c
>> +++ b/target-ppc/translate.c
>> @@ -1392,6 +1392,19 @@ GEN_LOGICAL2(nand, tcg_gen_nand_tl, 0x0E,
>> PPC_INTEGER);
>> /* nor & nor. */
>> GEN_LOGICAL2(nor, tcg_gen_nor_tl, 0x03, PPC_INTEGER);
>> +#if defined(TARGET_PPC64)
>> +static void gen_pause(DisasContext *ctx)
>> +{
>> + TCGv_i32 t0 = tcg_const_i32(0);
>> + tcg_gen_st_i32(t0, cpu_env,
>> + -offsetof(PowerPCCPU, env) + offsetof(CPUState,
>> halted));
>
> This sets CPUState::halted = 0, as unhalted state (a.k.a. "running"),
> is that expected? I'd expect "Pause" to halt the vCPU to avoid spinning
> (as mentioned below).
>
>> + tcg_temp_free_i32(t0);
>> +
>> + /* Stop translation, this gives other CPUs a chance to run */
>> + gen_exception_err(ctx, EXCP_HLT, 1);
>> +}
>> +#endif /* defined(TARGET_PPC64) */
>> +
>> /* or & or. */
>> static void gen_or(DisasContext *ctx)
>> {
>> @@ -1447,7 +1460,7 @@ static void gen_or(DisasContext *ctx)
>> }
>> break;
>> case 7:
>> - if (ctx->hv) {
>> + if (ctx->hv && !ctx->pr) {
>> /* Set process priority to very high */
>> prio = 7;
>> }
>> @@ -1464,6 +1477,10 @@ static void gen_or(DisasContext *ctx)
>> tcg_gen_ori_tl(t0, t0, ((uint64_t)prio) << 50);
>> gen_store_spr(SPR_PPR, t0);
>> tcg_temp_free(t0);
>> + /* Pause us out of TCG otherwise spin loops with smt_low
>> + * eat too much CPU and the kernel hangs
>> + */
>> + gen_pause(ctx);
>> }
>> #endif
>> }
>> @@ -1489,8 +1506,6 @@ static void gen_ori(DisasContext *ctx)
>> target_ulong uimm = UIMM(ctx->opcode);
>> if (rS(ctx->opcode) == rA(ctx->opcode) && uimm == 0) {
>> - /* NOP */
>> - /* XXX: should handle special NOPs for POWER series */
>> return;
>> }
>> tcg_gen_ori_tl(cpu_gpr[rA(ctx->opcode)], cpu_gpr[rS(ctx-
>> >opcode)], uimm);
> Note there is a similar code in gen_wait() doing the same (but
> correctlyIMO):
>
> 3366 /*
> 3367 * wait without WC field or with WC=0 waits for an exception /
> interrupt
> 3368 * to occur.
> 3369 */
> 3370 if (wc == 0) {
> 3371 TCGv_i32 t0 = tcg_constant_i32(1);
> 3372 tcg_gen_st_i32(t0, tcg_env,
> 3373 -offsetof(PowerPCCPU, env) +
> offsetof(CPUState, halted));
> 3374 /* Stop translation, as the CPU is supposed to sleep from
> now */
> 3375 gen_exception_nip(ctx, EXCP_HLT, ctx->base.pc_next);
> 3376 }
>
I'll post a patch as RFC to see...
^ permalink raw reply [flat|nested] 31+ messages in thread
* [Qemu-devel] [PULL 08/12] ppc: Add PPC_64H instruction flag to POWER7 and POWER8
2016-05-31 0:41 [Qemu-devel] [PULL 00/12] ppc-for-2.7 queue 20160531 David Gibson
` (6 preceding siblings ...)
2016-05-31 0:41 ` [Qemu-devel] [PULL 07/12] ppc: Get out of emulation on SMT "OR" ops David Gibson
@ 2016-05-31 0:41 ` David Gibson
2016-05-31 0:41 ` [Qemu-devel] [PULL 09/12] exec: Remove cpu from cpus list during cpu_exec_exit() David Gibson
` (4 subsequent siblings)
12 siblings, 0 replies; 31+ messages in thread
From: David Gibson @ 2016-05-31 0:41 UTC (permalink / raw)
To: peter.maydell
Cc: agraf, pbonzini, bharata.rao, benh, qemu-ppc, qemu-devel,
David Gibson
From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
This will enable decoding of hrfid
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
target-ppc/translate_init.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index a003c10..8301076 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -8359,7 +8359,7 @@ POWERPC_FAMILY(POWER7)(ObjectClass *oc, void *data)
PPC_CACHE | PPC_CACHE_ICBI | PPC_CACHE_DCBZ |
PPC_MEM_SYNC | PPC_MEM_EIEIO |
PPC_MEM_TLBIE | PPC_MEM_TLBSYNC |
- PPC_64B | PPC_ALTIVEC |
+ PPC_64B | PPC_64H | PPC_ALTIVEC |
PPC_SEGMENT_64B | PPC_SLBI |
PPC_POPCNTB | PPC_POPCNTWD;
pcc->insns_flags2 = PPC2_VSX | PPC2_DFP | PPC2_DBRX | PPC2_ISA205 |
@@ -8439,7 +8439,7 @@ POWERPC_FAMILY(POWER8)(ObjectClass *oc, void *data)
PPC_CACHE | PPC_CACHE_ICBI | PPC_CACHE_DCBZ |
PPC_MEM_SYNC | PPC_MEM_EIEIO |
PPC_MEM_TLBIE | PPC_MEM_TLBSYNC |
- PPC_64B | PPC_64BX | PPC_ALTIVEC |
+ PPC_64B | PPC_64H | PPC_64BX | PPC_ALTIVEC |
PPC_SEGMENT_64B | PPC_SLBI |
PPC_POPCNTB | PPC_POPCNTWD;
pcc->insns_flags2 = PPC2_VSX | PPC2_VSX207 | PPC2_DFP | PPC2_DBRX |
--
2.5.5
^ permalink raw reply related [flat|nested] 31+ messages in thread* [Qemu-devel] [PULL 09/12] exec: Remove cpu from cpus list during cpu_exec_exit()
2016-05-31 0:41 [Qemu-devel] [PULL 00/12] ppc-for-2.7 queue 20160531 David Gibson
` (7 preceding siblings ...)
2016-05-31 0:41 ` [Qemu-devel] [PULL 08/12] ppc: Add PPC_64H instruction flag to POWER7 and POWER8 David Gibson
@ 2016-05-31 0:41 ` David Gibson
2016-05-31 0:41 ` [Qemu-devel] [PULL 10/12] exec: Do vmstate unregistration from cpu_exec_exit() David Gibson
` (3 subsequent siblings)
12 siblings, 0 replies; 31+ messages in thread
From: David Gibson @ 2016-05-31 0:41 UTC (permalink / raw)
To: peter.maydell
Cc: agraf, pbonzini, bharata.rao, benh, qemu-ppc, qemu-devel,
Bharata B Rao, David Gibson
From: Bharata B Rao <bharata@linux.vnet.ibm.com>
CPUState *cpu gets added to the cpus list during cpu_exec_init(). It
should be removed from cpu_exec_exit().
cpu_exec_exit() is called from generic CPU::instance_finalize and some
archs like PowerPC call it from CPU unrealizefn. So ensure that we
dequeue the cpu only once.
Now -1 value for cpu->cpu_index indicates that we have already dequeued
the cpu for CONFIG_USER_ONLY case also.
Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
exec.c | 32 ++++++++++++++++++++++++--------
1 file changed, 24 insertions(+), 8 deletions(-)
diff --git a/exec.c b/exec.c
index a3a93ae..a1dfc01 100644
--- a/exec.c
+++ b/exec.c
@@ -612,15 +612,9 @@ static int cpu_get_free_index(Error **errp)
return cpu;
}
-void cpu_exec_exit(CPUState *cpu)
+static void cpu_release_index(CPUState *cpu)
{
- if (cpu->cpu_index == -1) {
- /* cpu_index was never allocated by this @cpu or was already freed. */
- return;
- }
-
bitmap_clear(cpu_index_map, cpu->cpu_index, 1);
- cpu->cpu_index = -1;
}
#else
@@ -635,11 +629,33 @@ static int cpu_get_free_index(Error **errp)
return cpu_index;
}
-void cpu_exec_exit(CPUState *cpu)
+static void cpu_release_index(CPUState *cpu)
{
+ return;
}
#endif
+void cpu_exec_exit(CPUState *cpu)
+{
+#if defined(CONFIG_USER_ONLY)
+ cpu_list_lock();
+#endif
+ if (cpu->cpu_index == -1) {
+ /* cpu_index was never allocated by this @cpu or was already freed. */
+#if defined(CONFIG_USER_ONLY)
+ cpu_list_unlock();
+#endif
+ return;
+ }
+
+ QTAILQ_REMOVE(&cpus, cpu, node);
+ cpu_release_index(cpu);
+ cpu->cpu_index = -1;
+#if defined(CONFIG_USER_ONLY)
+ cpu_list_unlock();
+#endif
+}
+
void cpu_exec_init(CPUState *cpu, Error **errp)
{
CPUClass *cc = CPU_GET_CLASS(cpu);
--
2.5.5
^ permalink raw reply related [flat|nested] 31+ messages in thread* [Qemu-devel] [PULL 10/12] exec: Do vmstate unregistration from cpu_exec_exit()
2016-05-31 0:41 [Qemu-devel] [PULL 00/12] ppc-for-2.7 queue 20160531 David Gibson
` (8 preceding siblings ...)
2016-05-31 0:41 ` [Qemu-devel] [PULL 09/12] exec: Remove cpu from cpus list during cpu_exec_exit() David Gibson
@ 2016-05-31 0:41 ` David Gibson
2016-05-31 0:41 ` [Qemu-devel] [PULL 11/12] cpu: Reclaim vCPU objects David Gibson
` (2 subsequent siblings)
12 siblings, 0 replies; 31+ messages in thread
From: David Gibson @ 2016-05-31 0:41 UTC (permalink / raw)
To: peter.maydell
Cc: agraf, pbonzini, bharata.rao, benh, qemu-ppc, qemu-devel,
Bharata B Rao, David Gibson
From: Bharata B Rao <bharata@linux.vnet.ibm.com>
cpu_exec_init() does vmstate_register for the CPU device. This needs to be
undone from cpu_exec_exit(). This change is needed to support CPU hot
removal.
Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
[dwg: added missing include to fix compile on some archs]
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
exec.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/exec.c b/exec.c
index a1dfc01..7261172 100644
--- a/exec.c
+++ b/exec.c
@@ -57,6 +57,8 @@
#include "exec/ram_addr.h"
#include "exec/log.h"
+#include "migration/vmstate.h"
+
#include "qemu/range.h"
#ifndef _WIN32
#include "qemu/mmap-alloc.h"
@@ -637,6 +639,8 @@ static void cpu_release_index(CPUState *cpu)
void cpu_exec_exit(CPUState *cpu)
{
+ CPUClass *cc = CPU_GET_CLASS(cpu);
+
#if defined(CONFIG_USER_ONLY)
cpu_list_lock();
#endif
@@ -654,6 +658,13 @@ void cpu_exec_exit(CPUState *cpu)
#if defined(CONFIG_USER_ONLY)
cpu_list_unlock();
#endif
+
+ if (cc->vmsd != NULL) {
+ vmstate_unregister(NULL, cc->vmsd, cpu);
+ }
+ if (qdev_get_vmsd(DEVICE(cpu)) == NULL) {
+ vmstate_unregister(NULL, &vmstate_cpu_common, cpu);
+ }
}
void cpu_exec_init(CPUState *cpu, Error **errp)
--
2.5.5
^ permalink raw reply related [flat|nested] 31+ messages in thread* [Qemu-devel] [PULL 11/12] cpu: Reclaim vCPU objects
2016-05-31 0:41 [Qemu-devel] [PULL 00/12] ppc-for-2.7 queue 20160531 David Gibson
` (9 preceding siblings ...)
2016-05-31 0:41 ` [Qemu-devel] [PULL 10/12] exec: Do vmstate unregistration from cpu_exec_exit() David Gibson
@ 2016-05-31 0:41 ` David Gibson
2016-05-31 0:41 ` [Qemu-devel] [PULL 12/12] cpu: Add a sync version of cpu_remove() David Gibson
2016-06-02 12:42 ` [Qemu-devel] [PULL 00/12] ppc-for-2.7 queue 20160531 Peter Maydell
12 siblings, 0 replies; 31+ messages in thread
From: David Gibson @ 2016-05-31 0:41 UTC (permalink / raw)
To: peter.maydell
Cc: agraf, pbonzini, bharata.rao, benh, qemu-ppc, qemu-devel,
Gu Zheng, Chen Fan, Zhu Guihua, Bharata B Rao, David Gibson
From: Gu Zheng <guz.fnst@cn.fujitsu.com>
In order to deal well with the kvm vcpus (which can not be removed without any
protection), we do not close KVM vcpu fd, just record and mark it as stopped
into a list, so that we can reuse it for the appending cpu hot-add request if
possible. It is also the approach that kvm guys suggested:
https://www.mail-archive.com/kvm@vger.kernel.org/msg102839.html
Signed-off-by: Chen Fan <chen.fan.fnst@cn.fujitsu.com>
Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
Signed-off-by: Zhu Guihua <zhugh.fnst@cn.fujitsu.com>
Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
[- Explicit CPU_REMOVE() from qemu_kvm/tcg_destroy_vcpu()
isn't needed as it is done from cpu_exec_exit()
- Use iothread mutex instead of global mutex during
destroy
- Don't cleanup vCPU object from vCPU thread context
but leave it to the callers (device_add/device_del)]
Reviewed-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
cpus.c | 39 +++++++++++++++++++++++++++++++++--
include/qom/cpu.h | 10 +++++++++
include/sysemu/kvm.h | 1 +
kvm-all.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++-
kvm-stub.c | 5 +++++
5 files changed, 109 insertions(+), 3 deletions(-)
diff --git a/cpus.c b/cpus.c
index e75895a..3e3ef95 100644
--- a/cpus.c
+++ b/cpus.c
@@ -972,6 +972,18 @@ void async_run_on_cpu(CPUState *cpu, void (*func)(void *data), void *data)
qemu_cpu_kick(cpu);
}
+static void qemu_kvm_destroy_vcpu(CPUState *cpu)
+{
+ if (kvm_destroy_vcpu(cpu) < 0) {
+ error_report("kvm_destroy_vcpu failed");
+ exit(EXIT_FAILURE);
+ }
+}
+
+static void qemu_tcg_destroy_vcpu(CPUState *cpu)
+{
+}
+
static void flush_queued_work(CPUState *cpu)
{
struct qemu_work_item *wi;
@@ -1061,7 +1073,7 @@ static void *qemu_kvm_cpu_thread_fn(void *arg)
cpu->created = true;
qemu_cond_signal(&qemu_cpu_cond);
- while (1) {
+ do {
if (cpu_can_run(cpu)) {
r = kvm_cpu_exec(cpu);
if (r == EXCP_DEBUG) {
@@ -1069,8 +1081,10 @@ static void *qemu_kvm_cpu_thread_fn(void *arg)
}
}
qemu_kvm_wait_io_event(cpu);
- }
+ } while (!cpu->unplug || cpu_can_run(cpu));
+ qemu_kvm_destroy_vcpu(cpu);
+ qemu_mutex_unlock_iothread();
return NULL;
}
@@ -1124,6 +1138,7 @@ static void tcg_exec_all(void);
static void *qemu_tcg_cpu_thread_fn(void *arg)
{
CPUState *cpu = arg;
+ CPUState *remove_cpu = NULL;
rcu_register_thread();
@@ -1161,6 +1176,16 @@ static void *qemu_tcg_cpu_thread_fn(void *arg)
}
}
qemu_tcg_wait_io_event(QTAILQ_FIRST(&cpus));
+ CPU_FOREACH(cpu) {
+ if (cpu->unplug && !cpu_can_run(cpu)) {
+ remove_cpu = cpu;
+ break;
+ }
+ }
+ if (remove_cpu) {
+ qemu_tcg_destroy_vcpu(remove_cpu);
+ remove_cpu = NULL;
+ }
}
return NULL;
@@ -1317,6 +1342,13 @@ void resume_all_vcpus(void)
}
}
+void cpu_remove(CPUState *cpu)
+{
+ cpu->stop = true;
+ cpu->unplug = true;
+ qemu_cpu_kick(cpu);
+}
+
/* For temporary buffers for forming a name */
#define VCPU_THREAD_NAME_SIZE 16
@@ -1533,6 +1565,9 @@ static void tcg_exec_all(void)
break;
}
} else if (cpu->stop || cpu->stopped) {
+ if (cpu->unplug) {
+ next_cpu = CPU_NEXT(cpu);
+ }
break;
}
}
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index c9ba16c..3b57757 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -244,6 +244,7 @@ struct qemu_work_item {
* @halted: Nonzero if the CPU is in suspended state.
* @stop: Indicates a pending stop request.
* @stopped: Indicates the CPU has been artificially stopped.
+ * @unplug: Indicates a pending CPU unplug request.
* @crash_occurred: Indicates the OS reported a crash (panic) for this CPU
* @tcg_exit_req: Set to force TCG to stop executing linked TBs for this
* CPU and return to its top level loop.
@@ -296,6 +297,7 @@ struct CPUState {
bool created;
bool stop;
bool stopped;
+ bool unplug;
bool crash_occurred;
bool exit_request;
bool tb_flushed;
@@ -763,6 +765,14 @@ void cpu_exit(CPUState *cpu);
void cpu_resume(CPUState *cpu);
/**
+ * cpu_remove:
+ * @cpu: The CPU to remove.
+ *
+ * Requests the CPU to be removed.
+ */
+void cpu_remove(CPUState *cpu);
+
+/**
* qemu_init_vcpu:
* @cpu: The vCPU to initialize.
*
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index f357ccd..65569ed 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -216,6 +216,7 @@ int kvm_has_intx_set_mask(void);
int kvm_init_vcpu(CPUState *cpu);
int kvm_cpu_exec(CPUState *cpu);
+int kvm_destroy_vcpu(CPUState *cpu);
#ifdef NEED_CPU_H
#include "cpu.h"
diff --git a/kvm-all.c b/kvm-all.c
index e56f385..d317dcb 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -61,6 +61,12 @@
#define KVM_MSI_HASHTAB_SIZE 256
+struct KVMParkedVcpu {
+ unsigned long vcpu_id;
+ int kvm_fd;
+ QLIST_ENTRY(KVMParkedVcpu) node;
+};
+
struct KVMState
{
AccelState parent_obj;
@@ -94,6 +100,7 @@ struct KVMState
QTAILQ_HEAD(msi_hashtab, KVMMSIRoute) msi_hashtab[KVM_MSI_HASHTAB_SIZE];
#endif
KVMMemoryListener memory_listener;
+ QLIST_HEAD(, KVMParkedVcpu) kvm_parked_vcpus;
};
KVMState *kvm_state;
@@ -237,6 +244,53 @@ static int kvm_set_user_memory_region(KVMMemoryListener *kml, KVMSlot *slot)
return kvm_vm_ioctl(s, KVM_SET_USER_MEMORY_REGION, &mem);
}
+int kvm_destroy_vcpu(CPUState *cpu)
+{
+ KVMState *s = kvm_state;
+ long mmap_size;
+ struct KVMParkedVcpu *vcpu = NULL;
+ int ret = 0;
+
+ DPRINTF("kvm_destroy_vcpu\n");
+
+ mmap_size = kvm_ioctl(s, KVM_GET_VCPU_MMAP_SIZE, 0);
+ if (mmap_size < 0) {
+ ret = mmap_size;
+ DPRINTF("KVM_GET_VCPU_MMAP_SIZE failed\n");
+ goto err;
+ }
+
+ ret = munmap(cpu->kvm_run, mmap_size);
+ if (ret < 0) {
+ goto err;
+ }
+
+ vcpu = g_malloc0(sizeof(*vcpu));
+ vcpu->vcpu_id = kvm_arch_vcpu_id(cpu);
+ vcpu->kvm_fd = cpu->kvm_fd;
+ QLIST_INSERT_HEAD(&kvm_state->kvm_parked_vcpus, vcpu, node);
+err:
+ return ret;
+}
+
+static int kvm_get_vcpu(KVMState *s, unsigned long vcpu_id)
+{
+ struct KVMParkedVcpu *cpu;
+
+ QLIST_FOREACH(cpu, &s->kvm_parked_vcpus, node) {
+ if (cpu->vcpu_id == vcpu_id) {
+ int kvm_fd;
+
+ QLIST_REMOVE(cpu, node);
+ kvm_fd = cpu->kvm_fd;
+ g_free(cpu);
+ return kvm_fd;
+ }
+ }
+
+ return kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)vcpu_id);
+}
+
int kvm_init_vcpu(CPUState *cpu)
{
KVMState *s = kvm_state;
@@ -245,7 +299,7 @@ int kvm_init_vcpu(CPUState *cpu)
DPRINTF("kvm_init_vcpu\n");
- ret = kvm_vm_ioctl(s, KVM_CREATE_VCPU, (void *)kvm_arch_vcpu_id(cpu));
+ ret = kvm_get_vcpu(s, kvm_arch_vcpu_id(cpu));
if (ret < 0) {
DPRINTF("kvm_create_vcpu failed\n");
goto err;
@@ -1501,6 +1555,7 @@ static int kvm_init(MachineState *ms)
#ifdef KVM_CAP_SET_GUEST_DEBUG
QTAILQ_INIT(&s->kvm_sw_breakpoints);
#endif
+ QLIST_INIT(&s->kvm_parked_vcpus);
s->vmfd = -1;
s->fd = qemu_open("/dev/kvm", O_RDWR);
if (s->fd == -1) {
diff --git a/kvm-stub.c b/kvm-stub.c
index 63735a8..07c09d1 100644
--- a/kvm-stub.c
+++ b/kvm-stub.c
@@ -32,6 +32,11 @@ bool kvm_allowed;
bool kvm_readonly_mem_allowed;
bool kvm_ioeventfd_any_length_allowed;
+int kvm_destroy_vcpu(CPUState *cpu)
+{
+ return -ENOSYS;
+}
+
int kvm_init_vcpu(CPUState *cpu)
{
return -ENOSYS;
--
2.5.5
^ permalink raw reply related [flat|nested] 31+ messages in thread* [Qemu-devel] [PULL 12/12] cpu: Add a sync version of cpu_remove()
2016-05-31 0:41 [Qemu-devel] [PULL 00/12] ppc-for-2.7 queue 20160531 David Gibson
` (10 preceding siblings ...)
2016-05-31 0:41 ` [Qemu-devel] [PULL 11/12] cpu: Reclaim vCPU objects David Gibson
@ 2016-05-31 0:41 ` David Gibson
2016-06-02 12:42 ` [Qemu-devel] [PULL 00/12] ppc-for-2.7 queue 20160531 Peter Maydell
12 siblings, 0 replies; 31+ messages in thread
From: David Gibson @ 2016-05-31 0:41 UTC (permalink / raw)
To: peter.maydell
Cc: agraf, pbonzini, bharata.rao, benh, qemu-ppc, qemu-devel,
Bharata B Rao, David Gibson
From: Bharata B Rao <bharata@linux.vnet.ibm.com>
This sync API will be used by the CPU hotplug code to wait for the CPU to
completely get removed before flagging the failure to the device_add
command.
Sync version of this call is needed to correctly recover from CPU
realization failures when ->plug() handler fails.
Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
cpus.c | 12 ++++++++++++
include/qom/cpu.h | 8 ++++++++
2 files changed, 20 insertions(+)
diff --git a/cpus.c b/cpus.c
index 3e3ef95..326742f 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1084,6 +1084,8 @@ static void *qemu_kvm_cpu_thread_fn(void *arg)
} while (!cpu->unplug || cpu_can_run(cpu));
qemu_kvm_destroy_vcpu(cpu);
+ cpu->created = false;
+ qemu_cond_signal(&qemu_cpu_cond);
qemu_mutex_unlock_iothread();
return NULL;
}
@@ -1184,6 +1186,8 @@ static void *qemu_tcg_cpu_thread_fn(void *arg)
}
if (remove_cpu) {
qemu_tcg_destroy_vcpu(remove_cpu);
+ cpu->created = false;
+ qemu_cond_signal(&qemu_cpu_cond);
remove_cpu = NULL;
}
}
@@ -1349,6 +1353,14 @@ void cpu_remove(CPUState *cpu)
qemu_cpu_kick(cpu);
}
+void cpu_remove_sync(CPUState *cpu)
+{
+ cpu_remove(cpu);
+ while (cpu->created) {
+ qemu_cond_wait(&qemu_cpu_cond, &qemu_global_mutex);
+ }
+}
+
/* For temporary buffers for forming a name */
#define VCPU_THREAD_NAME_SIZE 16
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 3b57757..32f3af3 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -772,6 +772,14 @@ void cpu_resume(CPUState *cpu);
*/
void cpu_remove(CPUState *cpu);
+ /**
+ * cpu_remove_sync:
+ * @cpu: The CPU to remove.
+ *
+ * Requests the CPU to be removed and waits till it is removed.
+ */
+void cpu_remove_sync(CPUState *cpu);
+
/**
* qemu_init_vcpu:
* @cpu: The vCPU to initialize.
--
2.5.5
^ permalink raw reply related [flat|nested] 31+ messages in thread* Re: [Qemu-devel] [PULL 00/12] ppc-for-2.7 queue 20160531
2016-05-31 0:41 [Qemu-devel] [PULL 00/12] ppc-for-2.7 queue 20160531 David Gibson
` (11 preceding siblings ...)
2016-05-31 0:41 ` [Qemu-devel] [PULL 12/12] cpu: Add a sync version of cpu_remove() David Gibson
@ 2016-06-02 12:42 ` Peter Maydell
12 siblings, 0 replies; 31+ messages in thread
From: Peter Maydell @ 2016-06-02 12:42 UTC (permalink / raw)
To: David Gibson
Cc: Alexander Graf, Paolo Bonzini, Bharata B Rao,
Benjamin Herrenschmidt, qemu-ppc@nongnu.org, QEMU Developers
On 31 May 2016 at 01:41, David Gibson <david@gibson.dropbear.id.au> wrote:
> The following changes since commit d6550e9ed2e1a60d889dfb721de00d9a4e3bafbe:
>
> Merge remote-tracking branch 'remotes/riku/tags/pull-linux-user-20160527' into staging (2016-05-27 14:05:48 +0100)
>
> are available in the git repository at:
>
> git://github.com/dgibson/qemu.git tags/ppc-for-2.7-20160531
>
> for you to fetch changes up to 2c579042e3be50bb40a233a6986348b4f40ed026:
>
> cpu: Add a sync version of cpu_remove() (2016-05-30 14:17:05 +1000)
>
> ----------------------------------------------------------------
> ppc patch queue for 2016-05-31
>
> Here's another ppc patch queue. This batch is all preliminaries
> towards two significant features:
>
> 1) Full hypervisor-mode support for POWER8
> Patches 1-8 start fixing various bugs with TCG's handling of
> hypervisor mode
>
> 2) CPU hotplug support
> Patches 9-12 make some preliminary fixes towards implementing CPU
> hotplug on ppc64 (and other non-x86 platforms). These patches are
> actually to generic code, not ppc, but are included here with
> Paolo's ACK.
>
Applied, thanks.
-- PMM
^ permalink raw reply [flat|nested] 31+ messages in thread