* [Qemu-devel] [PATCH v2 1/3] target-ppc: add TLB_NEED_LOCAL_FLUSH flag
@ 2016-09-09 13:14 Nikunj A Dadhania
2016-09-09 13:14 ` [Qemu-devel] [PATCH v2 2/3] target-ppc: add flag in chech_tlb_flush() Nikunj A Dadhania
2016-09-09 13:14 ` [Qemu-devel] [PATCH v2 3/3] target-ppc: tlbie should have global effect Nikunj A Dadhania
0 siblings, 2 replies; 6+ messages in thread
From: Nikunj A Dadhania @ 2016-09-09 13:14 UTC (permalink / raw)
To: qemu-ppc, david, benh; +Cc: alex.bennee, qemu-devel, rth, nikunj
Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
---
target-ppc/cpu.h | 1 +
target-ppc/helper_regs.h | 4 ++--
target-ppc/mmu-hash64.c | 4 ++--
target-ppc/mmu_helper.c | 6 +++---
4 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
index 1e808c8..71111dc 100644
--- a/target-ppc/cpu.h
+++ b/target-ppc/cpu.h
@@ -1009,6 +1009,7 @@ struct CPUPPCState {
bool tlb_dirty; /* Set to non-zero when modifying TLB */
bool kvm_sw_tlb; /* non-zero if KVM SW TLB API is active */
uint32_t tlb_need_flush; /* Delayed flush needed */
+#define TLB_NEED_LOCAL_FLUSH 0x1
#endif
/* Other registers */
diff --git a/target-ppc/helper_regs.h b/target-ppc/helper_regs.h
index 3d279f1..69204a5 100644
--- a/target-ppc/helper_regs.h
+++ b/target-ppc/helper_regs.h
@@ -157,9 +157,9 @@ static inline int hreg_store_msr(CPUPPCState *env, target_ulong value,
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;
+ if (env->tlb_need_flush & TLB_NEED_LOCAL_FLUSH) {
tlb_flush(cs, 1);
+ env->tlb_need_flush &= ~TLB_NEED_LOCAL_FLUSH;
}
}
#else
diff --git a/target-ppc/mmu-hash64.c b/target-ppc/mmu-hash64.c
index 8118143..1f52b64 100644
--- a/target-ppc/mmu-hash64.c
+++ b/target-ppc/mmu-hash64.c
@@ -110,7 +110,7 @@ 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
*/
- env->tlb_need_flush = 1;
+ env->tlb_need_flush |= TLB_NEED_LOCAL_FLUSH;
}
}
}
@@ -132,7 +132,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
*/
- env->tlb_need_flush = 1;
+ env->tlb_need_flush |= TLB_NEED_LOCAL_FLUSH;
}
}
diff --git a/target-ppc/mmu_helper.c b/target-ppc/mmu_helper.c
index 696bb03..d59d2f8 100644
--- a/target-ppc/mmu_helper.c
+++ b/target-ppc/mmu_helper.c
@@ -1965,7 +1965,7 @@ void ppc_tlb_invalidate_one(CPUPPCState *env, target_ulong addr)
* we just mark the TLB to be flushed later (context synchronizing
* event or sync instruction on 32-bit).
*/
- env->tlb_need_flush = 1;
+ env->tlb_need_flush |= TLB_NEED_LOCAL_FLUSH;
break;
#if defined(TARGET_PPC64)
case POWERPC_MMU_64B:
@@ -1979,7 +1979,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
*/
- env->tlb_need_flush = 1;
+ env->tlb_need_flush |= TLB_NEED_LOCAL_FLUSH;
break;
#endif /* defined(TARGET_PPC64) */
default:
@@ -2065,7 +2065,7 @@ void helper_store_sr(CPUPPCState *env, target_ulong srnum, target_ulong value)
}
}
#else
- env->tlb_need_flush = 1;
+ env->tlb_need_flush |= TLB_NEED_LOCAL_FLUSH;
#endif
}
}
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [Qemu-devel] [PATCH v2 2/3] target-ppc: add flag in chech_tlb_flush()
2016-09-09 13:14 [Qemu-devel] [PATCH v2 1/3] target-ppc: add TLB_NEED_LOCAL_FLUSH flag Nikunj A Dadhania
@ 2016-09-09 13:14 ` Nikunj A Dadhania
2016-09-09 13:14 ` [Qemu-devel] [PATCH v2 3/3] target-ppc: tlbie should have global effect Nikunj A Dadhania
1 sibling, 0 replies; 6+ messages in thread
From: Nikunj A Dadhania @ 2016-09-09 13:14 UTC (permalink / raw)
To: qemu-ppc, david, benh; +Cc: alex.bennee, qemu-devel, rth, nikunj
The flag will be used to indicate whether broadcast tlb flush is needed
or not.
Moreover, BookS does both ptesync and tlbsync, so make that a nop for
the server and tlbsync would generate a check flush for BookE
Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
---
hw/ppc/spapr_hcall.c | 4 ++--
target-ppc/excp_helper.c | 4 ++--
target-ppc/helper.h | 2 +-
target-ppc/helper_regs.h | 4 ++--
target-ppc/mmu_helper.c | 4 ++--
target-ppc/translate.c | 20 ++++++++++----------
6 files changed, 19 insertions(+), 19 deletions(-)
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 73af112..ef12ea0 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -201,7 +201,7 @@ static target_ulong h_remove(PowerPCCPU *cpu, sPAPRMachineState *spapr,
switch (ret) {
case REMOVE_SUCCESS:
- check_tlb_flush(env);
+ check_tlb_flush(env, 1);
return H_SUCCESS;
case REMOVE_NOT_FOUND:
@@ -282,7 +282,7 @@ static target_ulong h_bulk_remove(PowerPCCPU *cpu, sPAPRMachineState *spapr,
}
}
exit:
- check_tlb_flush(env);
+ check_tlb_flush(env, 1);
return rc;
}
diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c
index 04ed4da..3b78126 100644
--- a/target-ppc/excp_helper.c
+++ b/target-ppc/excp_helper.c
@@ -711,7 +711,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
/* Any interrupt is context synchronizing, check if TCG TLB
* needs a delayed flush on ppc64
*/
- check_tlb_flush(env);
+ check_tlb_flush(env, 0);
}
void ppc_cpu_do_interrupt(CPUState *cs)
@@ -973,7 +973,7 @@ static inline void do_rfi(CPUPPCState *env, target_ulong nip, target_ulong msr)
cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
/* Context synchronizing: check if TCG TLB needs flush */
- check_tlb_flush(env);
+ check_tlb_flush(env, 0);
}
void helper_rfi(CPUPPCState *env)
diff --git a/target-ppc/helper.h b/target-ppc/helper.h
index dcf3f95..a86e184 100644
--- a/target-ppc/helper.h
+++ b/target-ppc/helper.h
@@ -18,7 +18,7 @@ DEF_HELPER_1(rfid, void, env)
DEF_HELPER_1(hrfid, void, env)
DEF_HELPER_2(store_lpcr, void, env, tl)
#endif
-DEF_HELPER_1(check_tlb_flush, void, env)
+DEF_HELPER_2(check_tlb_flush, void, env, i32)
#endif
DEF_HELPER_3(lmw, void, env, tl, i32)
diff --git a/target-ppc/helper_regs.h b/target-ppc/helper_regs.h
index 69204a5..bcf65ce 100644
--- a/target-ppc/helper_regs.h
+++ b/target-ppc/helper_regs.h
@@ -154,7 +154,7 @@ static inline int hreg_store_msr(CPUPPCState *env, target_ulong value,
}
#if !defined(CONFIG_USER_ONLY)
-static inline void check_tlb_flush(CPUPPCState *env)
+static inline void check_tlb_flush(CPUPPCState *env, uint32_t global)
{
CPUState *cs = CPU(ppc_env_get_cpu(env));
if (env->tlb_need_flush & TLB_NEED_LOCAL_FLUSH) {
@@ -163,7 +163,7 @@ static inline void check_tlb_flush(CPUPPCState *env)
}
}
#else
-static inline void check_tlb_flush(CPUPPCState *env) { }
+static inline void check_tlb_flush(CPUPPCState *env, uint32_t global) { }
#endif
#endif /* HELPER_REGS_H */
diff --git a/target-ppc/mmu_helper.c b/target-ppc/mmu_helper.c
index d59d2f8..bf9f329 100644
--- a/target-ppc/mmu_helper.c
+++ b/target-ppc/mmu_helper.c
@@ -2867,9 +2867,9 @@ void helper_booke206_tlbflush(CPUPPCState *env, target_ulong type)
}
-void helper_check_tlb_flush(CPUPPCState *env)
+void helper_check_tlb_flush(CPUPPCState *env, unsigned int global)
{
- check_tlb_flush(env);
+ check_tlb_flush(env, global);
}
/*****************************************************************************/
diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index 618334a..77bb312 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -3064,7 +3064,7 @@ static void gen_eieio(DisasContext *ctx)
}
#if !defined(CONFIG_USER_ONLY)
-static inline void gen_check_tlb_flush(DisasContext *ctx)
+static inline void gen_check_tlb_flush(DisasContext *ctx, uint32_t global)
{
TCGv_i32 t;
TCGLabel *l;
@@ -3076,12 +3076,13 @@ static inline void gen_check_tlb_flush(DisasContext *ctx)
t = tcg_temp_new_i32();
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);
+ tcg_gen_movi_i32(t, global);
+ gen_helper_check_tlb_flush(cpu_env, t);
gen_set_label(l);
tcg_temp_free_i32(t);
}
#else
-static inline void gen_check_tlb_flush(DisasContext *ctx) { }
+static inline void gen_check_tlb_flush(DisasContext *ctx, uint32_t global) { }
#endif
/* isync */
@@ -3092,7 +3093,7 @@ static void gen_isync(DisasContext *ctx)
* kernel mode however so check MSR_PR
*/
if (!ctx->pr) {
- gen_check_tlb_flush(ctx);
+ gen_check_tlb_flush(ctx, 0);
}
gen_stop_exception(ctx);
}
@@ -3257,7 +3258,7 @@ static void gen_sync(DisasContext *ctx)
* check MSR_PR as well.
*/
if (((l == 2) || !(ctx->insns_flags & PPC_64B)) && !ctx->pr) {
- gen_check_tlb_flush(ctx);
+ gen_check_tlb_flush(ctx, 1);
}
}
@@ -4463,11 +4464,10 @@ static void gen_tlbsync(DisasContext *ctx)
#else
CHK_HV;
- /* 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_check_tlb_flush(ctx);
+ /* BookS does both ptesync and tlbsync make tlbsync a nop for server */
+ if (ctx->insns_flags & PPC_BOOKE) {
+ gen_check_tlb_flush(ctx, 1);
+ }
#endif /* defined(CONFIG_USER_ONLY) */
}
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [Qemu-devel] [PATCH v2 3/3] target-ppc: tlbie should have global effect
2016-09-09 13:14 [Qemu-devel] [PATCH v2 1/3] target-ppc: add TLB_NEED_LOCAL_FLUSH flag Nikunj A Dadhania
2016-09-09 13:14 ` [Qemu-devel] [PATCH v2 2/3] target-ppc: add flag in chech_tlb_flush() Nikunj A Dadhania
@ 2016-09-09 13:14 ` Nikunj A Dadhania
2016-09-09 23:03 ` Benjamin Herrenschmidt
1 sibling, 1 reply; 6+ messages in thread
From: Nikunj A Dadhania @ 2016-09-09 13:14 UTC (permalink / raw)
To: qemu-ppc, david, benh; +Cc: alex.bennee, qemu-devel, rth, nikunj
tlbie (H_REMOVE, H_PROTECT and H_BULK_REMOVE for pseries) should have a
global effect.
Introduces TLB_NEED_GLOBAL_FLUSH flag. During delayed flush, once taking
care of local flush, check broadcast flush(ptesync, tlbsync, etc) is
needed. Depending on the bitmask state of the tlb_need_flush, tlb is
flushed from other cpus if needed and the flags are cleared.
Suggested-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
---
hw/ppc/spapr_hcall.c | 2 ++
target-ppc/cpu.h | 1 +
target-ppc/helper_regs.h | 19 +++++++++++++++++++
target-ppc/mmu-hash64.c | 2 +-
target-ppc/mmu_helper.c | 10 +++++++---
target-ppc/translate.c | 6 ++++++
6 files changed, 36 insertions(+), 4 deletions(-)
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index ef12ea0..6144e17 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -319,6 +319,8 @@ static target_ulong h_protect(PowerPCCPU *cpu, sPAPRMachineState *spapr,
ppc_hash64_store_hpte(cpu, pte_index,
(v & ~HPTE64_V_VALID) | HPTE64_V_HPTE_DIRTY, 0);
ppc_hash64_tlb_flush_hpte(cpu, pte_index, v, r);
+ /* Flush the tlb */
+ check_tlb_flush(env, 1);
/* Don't need a memory barrier, due to qemu's global lock */
ppc_hash64_store_hpte(cpu, pte_index, v | HPTE64_V_HPTE_DIRTY, r);
return H_SUCCESS;
diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
index 71111dc..50fe0f5 100644
--- a/target-ppc/cpu.h
+++ b/target-ppc/cpu.h
@@ -1010,6 +1010,7 @@ struct CPUPPCState {
bool kvm_sw_tlb; /* non-zero if KVM SW TLB API is active */
uint32_t tlb_need_flush; /* Delayed flush needed */
#define TLB_NEED_LOCAL_FLUSH 0x1
+#define TLB_NEED_GLOBAL_FLUSH 0x2
#endif
/* Other registers */
diff --git a/target-ppc/helper_regs.h b/target-ppc/helper_regs.h
index bcf65ce..24872ca 100644
--- a/target-ppc/helper_regs.h
+++ b/target-ppc/helper_regs.h
@@ -154,6 +154,14 @@ static inline int hreg_store_msr(CPUPPCState *env, target_ulong value,
}
#if !defined(CONFIG_USER_ONLY)
+static inline void tlb_clear_flag(CPUState *cs)
+{
+ PowerPCCPU *cpu = POWERPC_CPU(cs);
+ CPUPPCState *env = &cpu->env;
+
+ env->tlb_need_flush = 0;
+}
+
static inline void check_tlb_flush(CPUPPCState *env, uint32_t global)
{
CPUState *cs = CPU(ppc_env_get_cpu(env));
@@ -161,6 +169,17 @@ static inline void check_tlb_flush(CPUPPCState *env, uint32_t global)
tlb_flush(cs, 1);
env->tlb_need_flush &= ~TLB_NEED_LOCAL_FLUSH;
}
+
+ if (global && (env->tlb_need_flush & TLB_NEED_GLOBAL_FLUSH)) {
+ CPUState *other_cs;
+ CPU_FOREACH(other_cs) {
+ if (other_cs != cs) {
+ tlb_clear_flag(other_cs);
+ tlb_flush(other_cs, 1);
+ }
+ }
+ env->tlb_need_flush &= ~TLB_NEED_GLOBAL_FLUSH;
+ }
}
#else
static inline void check_tlb_flush(CPUPPCState *env, uint32_t global) { }
diff --git a/target-ppc/mmu-hash64.c b/target-ppc/mmu-hash64.c
index 1f52b64..fdb7a78 100644
--- a/target-ppc/mmu-hash64.c
+++ b/target-ppc/mmu-hash64.c
@@ -912,7 +912,7 @@ void ppc_hash64_tlb_flush_hpte(PowerPCCPU *cpu,
* invalidate, 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);
+ cpu->env.tlb_need_flush = TLB_NEED_GLOBAL_FLUSH | TLB_NEED_LOCAL_FLUSH;
}
void ppc_hash64_update_rmls(CPUPPCState *env)
diff --git a/target-ppc/mmu_helper.c b/target-ppc/mmu_helper.c
index bf9f329..1dd057a 100644
--- a/target-ppc/mmu_helper.c
+++ b/target-ppc/mmu_helper.c
@@ -2757,7 +2757,7 @@ static inline void booke206_invalidate_ea_tlb(CPUPPCState *env, int tlbn,
void helper_booke206_tlbivax(CPUPPCState *env, target_ulong address)
{
- PowerPCCPU *cpu = ppc_env_get_cpu(env);
+ CPUState *cs;
if (address & 0x4) {
/* flush all entries */
@@ -2774,11 +2774,15 @@ void helper_booke206_tlbivax(CPUPPCState *env, target_ulong address)
if (address & 0x8) {
/* flush TLB1 entries */
booke206_invalidate_ea_tlb(env, 1, address);
- tlb_flush(CPU(cpu), 1);
+ CPU_FOREACH(cs) {
+ tlb_flush(cs, 1);
+ }
} else {
/* flush TLB0 entries */
booke206_invalidate_ea_tlb(env, 0, address);
- tlb_flush_page(CPU(cpu), address & MAS2_EPN_MASK);
+ CPU_FOREACH(cs) {
+ tlb_flush_page(cs, address & MAS2_EPN_MASK);
+ }
}
}
diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index 77bb312..2aae43b 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -4443,6 +4443,7 @@ static void gen_tlbie(DisasContext *ctx)
#if defined(CONFIG_USER_ONLY)
GEN_PRIV;
#else
+ TCGv_i32 t1;
CHK_HV;
if (NARROW_MODE(ctx)) {
@@ -4453,6 +4454,11 @@ static void gen_tlbie(DisasContext *ctx)
} else {
gen_helper_tlbie(cpu_env, cpu_gpr[rB(ctx->opcode)]);
}
+ t1 = tcg_temp_new_i32();
+ tcg_gen_ld_i32(t1, cpu_env, offsetof(CPUPPCState, tlb_need_flush));
+ tcg_gen_ori_i32(t1, t1, TLB_NEED_GLOBAL_FLUSH);
+ tcg_gen_st_i32(t1, cpu_env, offsetof(CPUPPCState, tlb_need_flush));
+ tcg_temp_free_i32(t1);
#endif /* defined(CONFIG_USER_ONLY) */
}
--
2.7.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/3] target-ppc: tlbie should have global effect
2016-09-09 13:14 ` [Qemu-devel] [PATCH v2 3/3] target-ppc: tlbie should have global effect Nikunj A Dadhania
@ 2016-09-09 23:03 ` Benjamin Herrenschmidt
2016-09-12 3:02 ` David Gibson
2016-09-12 4:43 ` Nikunj A Dadhania
0 siblings, 2 replies; 6+ messages in thread
From: Benjamin Herrenschmidt @ 2016-09-09 23:03 UTC (permalink / raw)
To: Nikunj A Dadhania, qemu-ppc, david; +Cc: alex.bennee, qemu-devel, rth
On Fri, 2016-09-09 at 18:44 +0530, Nikunj A Dadhania wrote:
> +static inline void tlb_clear_flag(CPUState *cs)
> +{
> + PowerPCCPU *cpu = POWERPC_CPU(cs);
> + CPUPPCState *env = &cpu->env;
> +
> + env->tlb_need_flush = 0;
> +}
What is the point of making this a separate function ?
Also I'm not 100% certain about the correctness of clearing
TLB_NEED_GLOBAL_FLUSH on the "other" guy.
We could have the situation where:
cpu 1: cpu 2:
sets both ..
isync (clears local flush) ..
<insert new translation>
.. set both
.. ..
.. ..
ptesync (clears global flush) .. (both gets cleared)
Now here, you can see that cpu2 never does a global flush and so the
new translation inserted by cpu 1 is not cleared while architecturally
it should be.
That being said, I doubt the above scenario can happen in practice,
but I think it's safer if you only clear the local bit on the "other"
CPUs.
> static inline void check_tlb_flush(CPUPPCState *env, uint32_t
> global)
> {
> CPUState *cs = CPU(ppc_env_get_cpu(env));
> @@ -161,6 +169,17 @@ static inline void check_tlb_flush(CPUPPCState
> *env, uint32_t global)
> tlb_flush(cs, 1);
> env->tlb_need_flush &= ~TLB_NEED_LOCAL_FLUSH;
> }
> +
> + if (global && (env->tlb_need_flush & TLB_NEED_GLOBAL_FLUSH)) {
> + CPUState *other_cs;
> + CPU_FOREACH(other_cs) {
> + if (other_cs != cs) {
> + tlb_clear_flag(other_cs);
> + tlb_flush(other_cs, 1);
> + }
> + }
> + env->tlb_need_flush &= ~TLB_NEED_GLOBAL_FLUSH;
> + }
> }
> #else
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/3] target-ppc: tlbie should have global effect
2016-09-09 23:03 ` Benjamin Herrenschmidt
@ 2016-09-12 3:02 ` David Gibson
2016-09-12 4:43 ` Nikunj A Dadhania
1 sibling, 0 replies; 6+ messages in thread
From: David Gibson @ 2016-09-12 3:02 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Nikunj A Dadhania, qemu-ppc, alex.bennee, qemu-devel, rth
[-- Attachment #1: Type: text/plain, Size: 2164 bytes --]
On Sat, Sep 10, 2016 at 09:03:56AM +1000, Benjamin Herrenschmidt wrote:
> On Fri, 2016-09-09 at 18:44 +0530, Nikunj A Dadhania wrote:
> > +static inline void tlb_clear_flag(CPUState *cs)
> > +{
> > + PowerPCCPU *cpu = POWERPC_CPU(cs);
> > + CPUPPCState *env = &cpu->env;
> > +
> > + env->tlb_need_flush = 0;
> > +}
>
> What is the point of making this a separate function ?
>
> Also I'm not 100% certain about the correctness of clearing
> TLB_NEED_GLOBAL_FLUSH on the "other" guy.
>
> We could have the situation where:
>
> cpu 1: cpu 2:
> sets both ..
> isync (clears local flush) ..
> <insert new translation>
> .. set both
> .. ..
> .. ..
> ptesync (clears global flush) .. (both gets cleared)
>
> Now here, you can see that cpu2 never does a global flush and so the
> new translation inserted by cpu 1 is not cleared while architecturally
> it should be.
>
> That being said, I doubt the above scenario can happen in practice,
> but I think it's safer if you only clear the local bit on the "other"
> CPUs.
I'll wait for a respin addressing these comments.
Please also add a cover letter on the next version.
>
> > static inline void check_tlb_flush(CPUPPCState *env, uint32_t
> > global)
> > {
> > CPUState *cs = CPU(ppc_env_get_cpu(env));
> > @@ -161,6 +169,17 @@ static inline void check_tlb_flush(CPUPPCState
> > *env, uint32_t global)
> > tlb_flush(cs, 1);
> > env->tlb_need_flush &= ~TLB_NEED_LOCAL_FLUSH;
> > }
> > +
> > + if (global && (env->tlb_need_flush & TLB_NEED_GLOBAL_FLUSH)) {
> > + CPUState *other_cs;
> > + CPU_FOREACH(other_cs) {
> > + if (other_cs != cs) {
> > + tlb_clear_flag(other_cs);
> > + tlb_flush(other_cs, 1);
> > + }
> > + }
> > + env->tlb_need_flush &= ~TLB_NEED_GLOBAL_FLUSH;
> > + }
> > }
> > #else
>
--
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: 801 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/3] target-ppc: tlbie should have global effect
2016-09-09 23:03 ` Benjamin Herrenschmidt
2016-09-12 3:02 ` David Gibson
@ 2016-09-12 4:43 ` Nikunj A Dadhania
1 sibling, 0 replies; 6+ messages in thread
From: Nikunj A Dadhania @ 2016-09-12 4:43 UTC (permalink / raw)
To: Benjamin Herrenschmidt, qemu-ppc, david; +Cc: alex.bennee, qemu-devel, rth
Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
> On Fri, 2016-09-09 at 18:44 +0530, Nikunj A Dadhania wrote:
>> +static inline void tlb_clear_flag(CPUState *cs)
>> +{
>> + PowerPCCPU *cpu = POWERPC_CPU(cs);
>> + CPUPPCState *env = &cpu->env;
>> +
>> + env->tlb_need_flush = 0;
>> +}
>
> What is the point of making this a separate function ?
When I wrote, i thought this would be used from various places. But dont
think its a requirement. Will inline it.
> Also I'm not 100% certain about the correctness of clearing
> TLB_NEED_GLOBAL_FLUSH on the "other" guy.
>
> We could have the situation where:
>
> cpu 1: cpu 2:
> sets both ..
> isync (clears local flush) ..
> <insert new translation>
> .. set both
> .. ..
> .. ..
> ptesync (clears global flush) .. (both gets cleared)
>
> Now here, you can see that cpu2 never does a global flush and so the
> new translation inserted by cpu 1 is not cleared while architecturally
> it should be.
Right, will only clear the local flag.
> That being said, I doubt the above scenario can happen in practice,
> but I think it's safer if you only clear the local bit on the "other"
> CPUs.
Regards,
Nikunj
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-09-12 4:44 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-09 13:14 [Qemu-devel] [PATCH v2 1/3] target-ppc: add TLB_NEED_LOCAL_FLUSH flag Nikunj A Dadhania
2016-09-09 13:14 ` [Qemu-devel] [PATCH v2 2/3] target-ppc: add flag in chech_tlb_flush() Nikunj A Dadhania
2016-09-09 13:14 ` [Qemu-devel] [PATCH v2 3/3] target-ppc: tlbie should have global effect Nikunj A Dadhania
2016-09-09 23:03 ` Benjamin Herrenschmidt
2016-09-12 3:02 ` David Gibson
2016-09-12 4:43 ` Nikunj A Dadhania
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.