| Message ID | 20190128094625.4428-8-clg@kaod.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ppc: support for the baremetal XIVE interrupt controller (POWER9) | expand |
On Mon, Jan 28, 2019 at 10:46:13AM +0100, Cédric Le Goater wrote: > From: Benjamin Herrenschmidt <benh@kernel.crashing.org> > > There's no point in going out of translation on an SMT OR with > mttcg since the backend won't do anything useful such as pausing, > it's only useful on traditional TCG to give time to other > processors. Is it actively harmful in the MTTCG case, or just pointless? > > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> > Signed-off-by: Cédric Le Goater <clg@kaod.org> > --- > 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 e169c43643a1..7d40a1fbe6bd 100644 > --- a/target/ppc/translate.c > +++ b/target/ppc/translate.c > @@ -1580,7 +1580,7 @@ static void gen_pause(DisasContext *ctx) > tcg_temp_free_i32(t0); > > /* Stop translation, this gives other CPUs a chance to run */ > - gen_exception_nip(ctx, EXCP_HLT, ctx->base.pc_next); > + gen_exception_nip(ctx, EXCP_INTERRUPT, ctx->base.pc_next); I don't see how this change relates to the rest. > } > #endif /* defined(TARGET_PPC64) */ > > @@ -1662,7 +1662,9 @@ static void gen_or(DisasContext *ctx) > * than no-op, e.g., miso(rs=26), yield(27), mdoio(29), mdoom(30), > * and all currently undefined. > */ > - gen_pause(ctx); > + if (!mttcg_enabled) { > + gen_pause(ctx); > + } > #endif > #endif > }
On Tue, 2019-02-12 at 16:59 +1100, David Gibson wrote: > On Mon, Jan 28, 2019 at 10:46:13AM +0100, Cédric Le Goater wrote: > > From: Benjamin Herrenschmidt <benh@kernel.crashing.org> > > > > There's no point in going out of translation on an SMT OR with > > mttcg since the backend won't do anything useful such as pausing, > > it's only useful on traditional TCG to give time to other > > processors. > > Is it actively harmful in the MTTCG case, or just pointless? I think it can hurt performance, I don't remember for sure :) > > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> > > Signed-off-by: Cédric Le Goater <clg@kaod.org> > > --- > > 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 e169c43643a1..7d40a1fbe6bd 100644 > > --- a/target/ppc/translate.c > > +++ b/target/ppc/translate.c > > @@ -1580,7 +1580,7 @@ static void gen_pause(DisasContext *ctx) > > tcg_temp_free_i32(t0); > > > > /* Stop translation, this gives other CPUs a chance to run */ > > - gen_exception_nip(ctx, EXCP_HLT, ctx->base.pc_next); > > + gen_exception_nip(ctx, EXCP_INTERRUPT, ctx->base.pc_next); > > I don't see how this change relates to the rest. Yeah not sure anymore :-) > > } > > #endif /* defined(TARGET_PPC64) */ > > > > @@ -1662,7 +1662,9 @@ static void gen_or(DisasContext *ctx) > > * than no-op, e.g., miso(rs=26), yield(27), mdoio(29), mdoom(30), > > * and all currently undefined. > > */ > > - gen_pause(ctx); > > + if (!mttcg_enabled) { > > + gen_pause(ctx); > > + } > > #endif > > #endif > > }
On Wed, Feb 13, 2019 at 11:03:12AM +1100, Benjamin Herrenschmidt wrote: > On Tue, 2019-02-12 at 16:59 +1100, David Gibson wrote: > > On Mon, Jan 28, 2019 at 10:46:13AM +0100, Cédric Le Goater wrote: > > > From: Benjamin Herrenschmidt <benh@kernel.crashing.org> > > > > > > There's no point in going out of translation on an SMT OR with > > > mttcg since the backend won't do anything useful such as pausing, > > > it's only useful on traditional TCG to give time to other > > > processors. > > > > Is it actively harmful in the MTTCG case, or just pointless? > > I think it can hurt performance, I don't remember for sure :) > > > > Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> > > > Signed-off-by: Cédric Le Goater <clg@kaod.org> > > > --- > > > 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 e169c43643a1..7d40a1fbe6bd 100644 > > > --- a/target/ppc/translate.c > > > +++ b/target/ppc/translate.c > > > @@ -1580,7 +1580,7 @@ static void gen_pause(DisasContext *ctx) > > > tcg_temp_free_i32(t0); > > > > > > /* Stop translation, this gives other CPUs a chance to run */ > > > - gen_exception_nip(ctx, EXCP_HLT, ctx->base.pc_next); > > > + gen_exception_nip(ctx, EXCP_INTERRUPT, ctx->base.pc_next); > > > > I don't see how this change relates to the rest. > > Yeah not sure anymore :-) Oh. That certainly doesn't make this easier to review. So, all these target/ppc patches are only indirectly related to XIVE pnv support. Cédric, can you split them out into their own series on the next spin. > > > > } > > > #endif /* defined(TARGET_PPC64) */ > > > > > > @@ -1662,7 +1662,9 @@ static void gen_or(DisasContext *ctx) > > > * than no-op, e.g., miso(rs=26), yield(27), mdoio(29), mdoom(30), > > > * and all currently undefined. > > > */ > > > - gen_pause(ctx); > > > + if (!mttcg_enabled) { > > > + gen_pause(ctx); > > > + } > > > #endif > > > #endif > > > } >
On 2/13/19 5:54 AM, David Gibson wrote: > On Wed, Feb 13, 2019 at 11:03:12AM +1100, Benjamin Herrenschmidt wrote: >> On Tue, 2019-02-12 at 16:59 +1100, David Gibson wrote: >>> On Mon, Jan 28, 2019 at 10:46:13AM +0100, Cédric Le Goater wrote: >>>> From: Benjamin Herrenschmidt <benh@kernel.crashing.org> >>>> >>>> There's no point in going out of translation on an SMT OR with >>>> mttcg since the backend won't do anything useful such as pausing, >>>> it's only useful on traditional TCG to give time to other >>>> processors. >>> >>> Is it actively harmful in the MTTCG case, or just pointless? >> >> I think it can hurt performance, I don't remember for sure :) >> >>>> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> >>>> Signed-off-by: Cédric Le Goater <clg@kaod.org> >>>> --- >>>> 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 e169c43643a1..7d40a1fbe6bd 100644 >>>> --- a/target/ppc/translate.c >>>> +++ b/target/ppc/translate.c >>>> @@ -1580,7 +1580,7 @@ static void gen_pause(DisasContext *ctx) >>>> tcg_temp_free_i32(t0); >>>> >>>> /* Stop translation, this gives other CPUs a chance to run */ >>>> - gen_exception_nip(ctx, EXCP_HLT, ctx->base.pc_next); >>>> + gen_exception_nip(ctx, EXCP_INTERRUPT, ctx->base.pc_next); >>> >>> I don't see how this change relates to the rest. >> >> Yeah not sure anymore :-) > > Oh. That certainly doesn't make this easier to review. > > So, all these target/ppc patches are only indirectly related to XIVE > pnv support. Cédric, can you split them out into their own series on > the next spin. Sure. I will address your comments and resend them first. Thanks, C. > >> >>>> } >>>> #endif /* defined(TARGET_PPC64) */ >>>> >>>> @@ -1662,7 +1662,9 @@ static void gen_or(DisasContext *ctx) >>>> * than no-op, e.g., miso(rs=26), yield(27), mdoio(29), mdoom(30), >>>> * and all currently undefined. >>>> */ >>>> - gen_pause(ctx); >>>> + if (!mttcg_enabled) { >>>> + gen_pause(ctx); >>>> + } >>>> #endif >>>> #endif >>>> } >> >
diff --git a/target/ppc/translate.c b/target/ppc/translate.c index e169c43643a1..7d40a1fbe6bd 100644 --- a/target/ppc/translate.c +++ b/target/ppc/translate.c @@ -1580,7 +1580,7 @@ static void gen_pause(DisasContext *ctx) tcg_temp_free_i32(t0); /* Stop translation, this gives other CPUs a chance to run */ - gen_exception_nip(ctx, EXCP_HLT, ctx->base.pc_next); + gen_exception_nip(ctx, EXCP_INTERRUPT, ctx->base.pc_next); } #endif /* defined(TARGET_PPC64) */ @@ -1662,7 +1662,9 @@ static void gen_or(DisasContext *ctx) * than no-op, e.g., miso(rs=26), yield(27), mdoio(29), mdoom(30), * and all currently undefined. */ - gen_pause(ctx); + if (!mttcg_enabled) { + gen_pause(ctx); + } #endif #endif }