On Fri, Mar 22, 2019 at 07:03:46PM +0100, Greg Kurz wrote: > Even if all ISAs up to v3 indeed mention: > > If the "decrement and test CTR" option is specified (BO2=0), the > instruction form is invalid. > > The UMs of all existing 64-bit server class processors say:
I've applied this series because it fixes an immediate problem, but I
have some significant reservations about it, read on..
> If BO[2] = 0, the contents of CTR (before any update) are used as the
> target address and for the test of the contents of CTR to resolve the
> branch. The contents of the CTR are then decremented and written back
> to the CTR.
So, if that's what the hardware does, I guess that's what we need to
do. That behaviour seems totally bizarre though - how can it make
sense for the same register value to act as both the branch target and
a flag/counter? Or am I misreading something?
> The linux kernel has spectre v2 mitigation code that relies on a
> BO[2] = 0 variant of bcctr, which is now activated by default on
> spapr, even with TCG. This causes linux guests to panic with
> the default machine type under TCG.
>
> Since any CPU model can provide its own behaviour for invalid forms,
> we could possibly introduce a new instruction flag to handle this.
> In practice, since the behaviour is shared by all 64-bit server
> processors starting with 970 up to POWER9, let's reuse the
> PPC_SEGMENT_64B flag. Caveat: this may have to be fixed later if
> POWER10 introduces a different behaviour.
Yeah.. this makes me nervous. It's going to be very non-obvious that
a flag about MMU behaviour is linked to an obscure conditional branch
behaviour, so I suspect the chances of forgetting to fix that later if
necessary are close to 100%.
> The existing behaviour of throwing a program interrupt is kept for
> all other CPU models.
>
> Signed-off-by: Greg Kurz <gr...@kaod.org>
> ---
> target/ppc/translate.c | 52
> ++++++++++++++++++++++++++++++++++--------------
> 1 file changed, 37 insertions(+), 15 deletions(-)
>
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index aaafa3a715d8..d3aaa6482c6a 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -3747,22 +3747,44 @@ static void gen_bcond(DisasContext *ctx, int type)
> if ((bo & 0x4) == 0) {
> /* Decrement and test CTR */
> TCGv temp = tcg_temp_new();
> - if (unlikely(type == BCOND_CTR)) {
> - gen_inval_exception(ctx, POWERPC_EXCP_INVAL_INVAL);
> - tcg_temp_free(temp);
> - tcg_temp_free(target);
> - return;
> - }
> - tcg_gen_subi_tl(cpu_ctr, cpu_ctr, 1);
> - if (NARROW_MODE(ctx)) {
> - tcg_gen_ext32u_tl(temp, cpu_ctr);
> - } else {
> - tcg_gen_mov_tl(temp, cpu_ctr);
> - }
> - if (bo & 0x2) {
> - tcg_gen_brcondi_tl(TCG_COND_NE, temp, 0, l1);
> +
> + if (type == BCOND_CTR) {
> + /*
> + * All ISAs up to v3 describe this form of bcctr as invalid but
> + * some processors, ie. 64-bit server processors compliant with
> + * arch 2.x, do implement a "test and decrement" logic instead,
> + * as described in their respective UMs.
> + */
> + if (unlikely(!(ctx->insns_flags & PPC_SEGMENT_64B))) {
> + gen_inval_exception(ctx, POWERPC_EXCP_INVAL_INVAL);
> + tcg_temp_free(temp);
> + tcg_temp_free(target);
> + return;
> + }
> +
> + if (NARROW_MODE(ctx)) {
> + tcg_gen_ext32u_tl(temp, cpu_ctr);
> + } else {
> + tcg_gen_mov_tl(temp, cpu_ctr);
> + }
> + if (bo & 0x2) {
> + tcg_gen_brcondi_tl(TCG_COND_NE, temp, 0, l1);
> + } else {
> + tcg_gen_brcondi_tl(TCG_COND_EQ, temp, 0, l1);
> + }
> + tcg_gen_subi_tl(cpu_ctr, cpu_ctr, 1);
> } else {
> - tcg_gen_brcondi_tl(TCG_COND_EQ, temp, 0, l1);
> + tcg_gen_subi_tl(cpu_ctr, cpu_ctr, 1);
> + if (NARROW_MODE(ctx)) {
> + tcg_gen_ext32u_tl(temp, cpu_ctr);
> + } else {
> + tcg_gen_mov_tl(temp, cpu_ctr);
> + }
> + if (bo & 0x2) {
> + tcg_gen_brcondi_tl(TCG_COND_NE, temp, 0, l1);
> + } else {
> + tcg_gen_brcondi_tl(TCG_COND_EQ, temp, 0, l1);
> + }
> }
> tcg_temp_free(temp);
> }
>
--
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
signature.asc
Description: PGP signature
