[PATCH] target/ppc: Ease L=0 requirement on cmp/cmpi/cmpl/cmpli for ppc32

[PATCH] target/ppc: Ease L=0 requirement on cmp/cmpi/cmpl/cmpli for ppc32

[matheus.ferst]

From: Matheus Ferst <matheus.ferst@eldorado.org.br>

In commit 8f0a4b6a9, we started to require L=0 for ppc32 to match what
The Programming Environments Manual say:

"For 32-bit implementations, the L field must be cleared, otherwise
the instruction form is invalid."

Further digging, however, shown that older CPUs have different behavior
concerning invalid forms. E.g.: 440 and 405 manuals say that:

"Unless otherwise noted, the PPC440 will execute all invalid instruction
forms without causing an Illegal Instruction exception".

While the PowerISA has an arguably more restrictive:

"In general, any attempt to execute an invalid form of an instruction
will either cause the system illegal instruction error handler to be
invoked or yield boundedly undefined results."

Finally, BALATON Zoltan (CC'ed) reported that the stricter behavior
broke AROS boot on sam460ex. This patch address this regression by only
logging a guest error, except for CPUs known to raise an exception for
this case (e500 and e500mc).

Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
---
 target/ppc/translate/fixedpoint-impl.c.inc | 58 +++++++++++++++++++++-
 1 file changed, 56 insertions(+), 2 deletions(-)

diff --git a/target/ppc/translate/fixedpoint-impl.c.inc b/target/ppc/translate/fixedpoint-impl.c.inc
index f4fcfadbfc..1c35b60eb4 100644
--- a/target/ppc/translate/fixedpoint-impl.c.inc
+++ b/target/ppc/translate/fixedpoint-impl.c.inc
@@ -145,8 +145,35 @@ TRANS64(PSTD, do_ldst_PLS_D, false, true, MO_Q)
 
 static bool do_cmp_X(DisasContext *ctx, arg_X_bfl *a, bool s)
 {
+    if ((ctx->insns_flags & PPC_64B) == 0) {
+        /*
+         * For 32-bit implementations, The Programming Environments Manual says
+         * that "the L field must be cleared, otherwise the instruction form is
+         * invalid." It seems, however, that most 32-bit CPUs ignore invalid
+         * forms (e.g., section "Instruction Formats" of the 405 and 440
+         * manuals, "Integer Compare Instructions" of the 601 manual), with the
+         * notable exception of the e500 and e500mc, where L=1 was reported to
+         * cause an exception.
+         */
+        if (a->l) {
+            if ((ctx->insns_flags2 & PPC2_BOOKE206)) {
+                /*
+                 * For 32-bit Book E v2.06 implementations (i.e. e500/e500mc),
+                 * generate an illegal instruction exception.
+                 */
+                return false;
+            } else {
+                qemu_log_mask(LOG_GUEST_ERROR,
+                        "Invalid form of CMP%s at 0x" TARGET_FMT_lx ", L = 1\n",
+                        s ? "" : "L", ctx->cia);
+            }
+        }
+        gen_op_cmp32(cpu_gpr[a->ra], cpu_gpr[a->rb], s, a->bf);
+        return true;
+    }
+
+    /* For 64-bit implementations, deal with bit L accordingly. */
     if (a->l) {
-        REQUIRE_64BIT(ctx);
         gen_op_cmp(cpu_gpr[a->ra], cpu_gpr[a->rb], s, a->bf);
     } else {
         gen_op_cmp32(cpu_gpr[a->ra], cpu_gpr[a->rb], s, a->bf);
@@ -156,8 +183,35 @@ static bool do_cmp_X(DisasContext *ctx, arg_X_bfl *a, bool s)
 
 static bool do_cmp_D(DisasContext *ctx, arg_D_bf *a, bool s)
 {
+    if ((ctx->insns_flags & PPC_64B) == 0) {
+        /*
+         * For 32-bit implementations, The Programming Environments Manual says
+         * that "the L field must be cleared, otherwise the instruction form is
+         * invalid." It seems, however, that most 32-bit CPUs ignore invalid
+         * forms (e.g., section "Instruction Formats" of the 405 and 440
+         * manuals, "Integer Compare Instructions" of the 601 manual), with the
+         * notable exception of the e500 and e500mc, where L=1 was reported to
+         * cause an exception.
+         */
+        if (a->l) {
+            if ((ctx->insns_flags2 & PPC2_BOOKE206)) {
+                /*
+                 * For 32-bit Book E v2.06 implementations (i.e. e500/e500mc),
+                 * generate an illegal instruction exception.
+                 */
+                return false;
+            } else {
+                qemu_log_mask(LOG_GUEST_ERROR,
+                        "Invalid form of CMP%s at 0x" TARGET_FMT_lx ", L = 1\n",
+                        s ? "I" : "LI", ctx->cia);
+            }
+        }
+        gen_op_cmp32(cpu_gpr[a->ra], tcg_constant_tl(a->imm), s, a->bf);
+        return true;
+    }
+
+    /* For 64-bit implementations, deal with bit L accordingly. */
     if (a->l) {
-        REQUIRE_64BIT(ctx);
         gen_op_cmp(cpu_gpr[a->ra], tcg_constant_tl(a->imm), s, a->bf);
     } else {
         gen_op_cmp32(cpu_gpr[a->ra], tcg_constant_tl(a->imm), s, a->bf);
-- 
2.25.1

Re: [PATCH] target/ppc: Ease L=0 requirement on cmp/cmpi/cmpl/cmpli for ppc32

[BALATON Zoltan]

On Thu, 15 Jul 2021, matheus.ferst@eldorado.org.br wrote:
> From: Matheus Ferst <matheus.ferst@eldorado.org.br>
>
> In commit 8f0a4b6a9, we started to require L=0 for ppc32 to match what
> The Programming Environments Manual say:
>
> "For 32-bit implementations, the L field must be cleared, otherwise
> the instruction form is invalid."
>
> Further digging, however, shown that older CPUs have different behavior
> concerning invalid forms. E.g.: 440 and 405 manuals say that:
>
> "Unless otherwise noted, the PPC440 will execute all invalid instruction
> forms without causing an Illegal Instruction exception".
>
> While the PowerISA has an arguably more restrictive:
>
> "In general, any attempt to execute an invalid form of an instruction
> will either cause the system illegal instruction error handler to be
> invoked or yield boundedly undefined results."
>
> Finally, BALATON Zoltan (CC'ed) reported that the stricter behavior
> broke AROS boot on sam460ex. This patch address this regression by only
> logging a guest error, except for CPUs known to raise an exception for
> this case (e500 and e500mc).
>
> Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>

Tested-by: BALATON Zoltan <balaton@eik.bme.hu>

This fixed AROS on sam460ex and also found a similar problem with pegasos2 
firmware version 1.2 (using G4 CPU) that this patch fixes as well 
(assuming that firmware runs on real hardware the G4 also seems to behave 
like this, ignoring invalid bits). Thank you.

Regards,
BALATON Zoltan

> ---
> target/ppc/translate/fixedpoint-impl.c.inc | 58 +++++++++++++++++++++-
> 1 file changed, 56 insertions(+), 2 deletions(-)
>
> diff --git a/target/ppc/translate/fixedpoint-impl.c.inc b/target/ppc/translate/fixedpoint-impl.c.inc
> index f4fcfadbfc..1c35b60eb4 100644
> --- a/target/ppc/translate/fixedpoint-impl.c.inc
> +++ b/target/ppc/translate/fixedpoint-impl.c.inc
> @@ -145,8 +145,35 @@ TRANS64(PSTD, do_ldst_PLS_D, false, true, MO_Q)
>
> static bool do_cmp_X(DisasContext *ctx, arg_X_bfl *a, bool s)
> {
> +    if ((ctx->insns_flags & PPC_64B) == 0) {
> +        /*
> +         * For 32-bit implementations, The Programming Environments Manual says
> +         * that "the L field must be cleared, otherwise the instruction form is
> +         * invalid." It seems, however, that most 32-bit CPUs ignore invalid
> +         * forms (e.g., section "Instruction Formats" of the 405 and 440
> +         * manuals, "Integer Compare Instructions" of the 601 manual), with the
> +         * notable exception of the e500 and e500mc, where L=1 was reported to
> +         * cause an exception.
> +         */
> +        if (a->l) {
> +            if ((ctx->insns_flags2 & PPC2_BOOKE206)) {
> +                /*
> +                 * For 32-bit Book E v2.06 implementations (i.e. e500/e500mc),
> +                 * generate an illegal instruction exception.
> +                 */
> +                return false;
> +            } else {
> +                qemu_log_mask(LOG_GUEST_ERROR,
> +                        "Invalid form of CMP%s at 0x" TARGET_FMT_lx ", L = 1\n",
> +                        s ? "" : "L", ctx->cia);
> +            }
> +        }
> +        gen_op_cmp32(cpu_gpr[a->ra], cpu_gpr[a->rb], s, a->bf);
> +        return true;
> +    }
> +
> +    /* For 64-bit implementations, deal with bit L accordingly. */
>     if (a->l) {
> -        REQUIRE_64BIT(ctx);
>         gen_op_cmp(cpu_gpr[a->ra], cpu_gpr[a->rb], s, a->bf);
>     } else {
>         gen_op_cmp32(cpu_gpr[a->ra], cpu_gpr[a->rb], s, a->bf);
> @@ -156,8 +183,35 @@ static bool do_cmp_X(DisasContext *ctx, arg_X_bfl *a, bool s)
>
> static bool do_cmp_D(DisasContext *ctx, arg_D_bf *a, bool s)
> {
> +    if ((ctx->insns_flags & PPC_64B) == 0) {
> +        /*
> +         * For 32-bit implementations, The Programming Environments Manual says
> +         * that "the L field must be cleared, otherwise the instruction form is
> +         * invalid." It seems, however, that most 32-bit CPUs ignore invalid
> +         * forms (e.g., section "Instruction Formats" of the 405 and 440
> +         * manuals, "Integer Compare Instructions" of the 601 manual), with the
> +         * notable exception of the e500 and e500mc, where L=1 was reported to
> +         * cause an exception.
> +         */
> +        if (a->l) {
> +            if ((ctx->insns_flags2 & PPC2_BOOKE206)) {
> +                /*
> +                 * For 32-bit Book E v2.06 implementations (i.e. e500/e500mc),
> +                 * generate an illegal instruction exception.
> +                 */
> +                return false;
> +            } else {
> +                qemu_log_mask(LOG_GUEST_ERROR,
> +                        "Invalid form of CMP%s at 0x" TARGET_FMT_lx ", L = 1\n",
> +                        s ? "I" : "LI", ctx->cia);
> +            }
> +        }
> +        gen_op_cmp32(cpu_gpr[a->ra], tcg_constant_tl(a->imm), s, a->bf);
> +        return true;
> +    }
> +
> +    /* For 64-bit implementations, deal with bit L accordingly. */
>     if (a->l) {
> -        REQUIRE_64BIT(ctx);
>         gen_op_cmp(cpu_gpr[a->ra], tcg_constant_tl(a->imm), s, a->bf);
>     } else {
>         gen_op_cmp32(cpu_gpr[a->ra], tcg_constant_tl(a->imm), s, a->bf);
>

Re: [PATCH] target/ppc: Ease L=0 requirement on cmp/cmpi/cmpl/cmpli for ppc32

[BALATON Zoltan]

On Thu, 15 Jul 2021, matheus.ferst@eldorado.org.br wrote:
> From: Matheus Ferst <matheus.ferst@eldorado.org.br>
>
> In commit 8f0a4b6a9, we started to require L=0 for ppc32 to match what
> The Programming Environments Manual say:
>
> "For 32-bit implementations, the L field must be cleared, otherwise
> the instruction form is invalid."
>
> Further digging, however, shown that older CPUs have different behavior
> concerning invalid forms. E.g.: 440 and 405 manuals say that:
>
> "Unless otherwise noted, the PPC440 will execute all invalid instruction
> forms without causing an Illegal Instruction exception".
>
> While the PowerISA has an arguably more restrictive:
>
> "In general, any attempt to execute an invalid form of an instruction
> will either cause the system illegal instruction error handler to be
> invoked or yield boundedly undefined results."
>
> Finally, BALATON Zoltan (CC'ed) reported that the stricter behavior

By the way, instead of putting this in the commit message usually a 
Reported-by tag is used instead to note who reported the problem but I 
don't mind either way, just seems unusual to have it in commit message.

Regards,
BALATON Zoltan

> broke AROS boot on sam460ex. This patch address this regression by only
> logging a guest error, except for CPUs known to raise an exception for
> this case (e500 and e500mc).
>
> Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
> ---
> target/ppc/translate/fixedpoint-impl.c.inc | 58 +++++++++++++++++++++-
> 1 file changed, 56 insertions(+), 2 deletions(-)
>
> diff --git a/target/ppc/translate/fixedpoint-impl.c.inc b/target/ppc/translate/fixedpoint-impl.c.inc
> index f4fcfadbfc..1c35b60eb4 100644
> --- a/target/ppc/translate/fixedpoint-impl.c.inc
> +++ b/target/ppc/translate/fixedpoint-impl.c.inc
> @@ -145,8 +145,35 @@ TRANS64(PSTD, do_ldst_PLS_D, false, true, MO_Q)
>
> static bool do_cmp_X(DisasContext *ctx, arg_X_bfl *a, bool s)
> {
> +    if ((ctx->insns_flags & PPC_64B) == 0) {
> +        /*
> +         * For 32-bit implementations, The Programming Environments Manual says
> +         * that "the L field must be cleared, otherwise the instruction form is
> +         * invalid." It seems, however, that most 32-bit CPUs ignore invalid
> +         * forms (e.g., section "Instruction Formats" of the 405 and 440
> +         * manuals, "Integer Compare Instructions" of the 601 manual), with the
> +         * notable exception of the e500 and e500mc, where L=1 was reported to
> +         * cause an exception.
> +         */
> +        if (a->l) {
> +            if ((ctx->insns_flags2 & PPC2_BOOKE206)) {
> +                /*
> +                 * For 32-bit Book E v2.06 implementations (i.e. e500/e500mc),
> +                 * generate an illegal instruction exception.
> +                 */
> +                return false;
> +            } else {
> +                qemu_log_mask(LOG_GUEST_ERROR,
> +                        "Invalid form of CMP%s at 0x" TARGET_FMT_lx ", L = 1\n",
> +                        s ? "" : "L", ctx->cia);
> +            }
> +        }
> +        gen_op_cmp32(cpu_gpr[a->ra], cpu_gpr[a->rb], s, a->bf);
> +        return true;
> +    }
> +
> +    /* For 64-bit implementations, deal with bit L accordingly. */
>     if (a->l) {
> -        REQUIRE_64BIT(ctx);
>         gen_op_cmp(cpu_gpr[a->ra], cpu_gpr[a->rb], s, a->bf);
>     } else {
>         gen_op_cmp32(cpu_gpr[a->ra], cpu_gpr[a->rb], s, a->bf);
> @@ -156,8 +183,35 @@ static bool do_cmp_X(DisasContext *ctx, arg_X_bfl *a, bool s)
>
> static bool do_cmp_D(DisasContext *ctx, arg_D_bf *a, bool s)
> {
> +    if ((ctx->insns_flags & PPC_64B) == 0) {
> +        /*
> +         * For 32-bit implementations, The Programming Environments Manual says
> +         * that "the L field must be cleared, otherwise the instruction form is
> +         * invalid." It seems, however, that most 32-bit CPUs ignore invalid
> +         * forms (e.g., section "Instruction Formats" of the 405 and 440
> +         * manuals, "Integer Compare Instructions" of the 601 manual), with the
> +         * notable exception of the e500 and e500mc, where L=1 was reported to
> +         * cause an exception.
> +         */
> +        if (a->l) {
> +            if ((ctx->insns_flags2 & PPC2_BOOKE206)) {
> +                /*
> +                 * For 32-bit Book E v2.06 implementations (i.e. e500/e500mc),
> +                 * generate an illegal instruction exception.
> +                 */
> +                return false;
> +            } else {
> +                qemu_log_mask(LOG_GUEST_ERROR,
> +                        "Invalid form of CMP%s at 0x" TARGET_FMT_lx ", L = 1\n",
> +                        s ? "I" : "LI", ctx->cia);
> +            }
> +        }
> +        gen_op_cmp32(cpu_gpr[a->ra], tcg_constant_tl(a->imm), s, a->bf);
> +        return true;
> +    }
> +
> +    /* For 64-bit implementations, deal with bit L accordingly. */
>     if (a->l) {
> -        REQUIRE_64BIT(ctx);
>         gen_op_cmp(cpu_gpr[a->ra], tcg_constant_tl(a->imm), s, a->bf);
>     } else {
>         gen_op_cmp32(cpu_gpr[a->ra], tcg_constant_tl(a->imm), s, a->bf);
>

Re: [PATCH] target/ppc: Ease L=0 requirement on cmp/cmpi/cmpl/cmpli for ppc32

[Matheus K. Ferst]

On 15/07/2021 10:14, BALATON Zoltan wrote:
> [E-MAIL EXTERNO] Não clique em links ou abra anexos, a menos que você 
> possa confirmar o remetente e saber que o conteúdo é seguro. Em caso de 
> e-mail suspeito entre imediatamente em contato com o DTI.
> 
> On Thu, 15 Jul 2021, matheus.ferst@eldorado.org.br wrote:
>> From: Matheus Ferst <matheus.ferst@eldorado.org.br>
>>
>> In commit 8f0a4b6a9, we started to require L=0 for ppc32 to match what
>> The Programming Environments Manual say:
>>
>> "For 32-bit implementations, the L field must be cleared, otherwise
>> the instruction form is invalid."
>>
>> Further digging, however, shown that older CPUs have different behavior
>> concerning invalid forms. E.g.: 440 and 405 manuals say that:
>>
>> "Unless otherwise noted, the PPC440 will execute all invalid instruction
>> forms without causing an Illegal Instruction exception".
>>
>> While the PowerISA has an arguably more restrictive:
>>
>> "In general, any attempt to execute an invalid form of an instruction
>> will either cause the system illegal instruction error handler to be
>> invoked or yield boundedly undefined results."
>>
>> Finally, BALATON Zoltan (CC'ed) reported that the stricter behavior
> 
> By the way, instead of putting this in the commit message usually a
> Reported-by tag is used instead to note who reported the problem but I
> don't mind either way, just seems unusual to have it in commit message.
> 

Ah, I forgot the tag... again. I swear I'll get used to email workflow 
someday. I can send it again if someone thinks it's better.

-- 
Matheus K. Ferst
Instituto de Pesquisas ELDORADO <http://www.eldorado.org.br/>
Analista de Software Júnior
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>

Re: [PATCH] target/ppc: Ease L=0 requirement on cmp/cmpi/cmpl/cmpli for ppc32

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 6021 bytes --]

On Thu, Jul 15, 2021 at 09:29:50AM -0300, matheus.ferst@eldorado.org.br wrote:
> From: Matheus Ferst <matheus.ferst@eldorado.org.br>
> 
> In commit 8f0a4b6a9, we started to require L=0 for ppc32 to match what
> The Programming Environments Manual say:
> 
> "For 32-bit implementations, the L field must be cleared, otherwise
> the instruction form is invalid."
> 
> Further digging, however, shown that older CPUs have different behavior
> concerning invalid forms. E.g.: 440 and 405 manuals say that:
> 
> "Unless otherwise noted, the PPC440 will execute all invalid instruction
> forms without causing an Illegal Instruction exception".
> 
> While the PowerISA has an arguably more restrictive:
> 
> "In general, any attempt to execute an invalid form of an instruction
> will either cause the system illegal instruction error handler to be
> invoked or yield boundedly undefined results."

That's actually less restrictive.  "boundedly undefined" lets the
implementation do nearly anything that won't mess up a hypervisor.
Both ignoring the illegal bits and issuing an invalid instruction
exception are definitely permissible within the meaning of "boundedly
undefined".

> Finally, BALATON Zoltan (CC'ed) reported that the stricter behavior
> broke AROS boot on sam460ex. This patch address this regression by only
> logging a guest error, except for CPUs known to raise an exception for
> this case (e500 and e500mc).

So.. as a rule of thumb, I'd prefer to have qemu give explicit
failures (e.g. program check traps) where there's implementation
specific or architecture undefined behaviour.  On the other hand,
having a real guest that relies on the specific behaviour of real
implementations is a compelling reason to break that rule of thumb.


Given it's a behavioural change, I'm disinclined to squeeze this in
for qemu-6.1, but I'll consider it for 6.2.  Richard, any thoughts?

> 
> Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
> ---
>  target/ppc/translate/fixedpoint-impl.c.inc | 58 +++++++++++++++++++++-
>  1 file changed, 56 insertions(+), 2 deletions(-)
> 
> diff --git a/target/ppc/translate/fixedpoint-impl.c.inc b/target/ppc/translate/fixedpoint-impl.c.inc
> index f4fcfadbfc..1c35b60eb4 100644
> --- a/target/ppc/translate/fixedpoint-impl.c.inc
> +++ b/target/ppc/translate/fixedpoint-impl.c.inc
> @@ -145,8 +145,35 @@ TRANS64(PSTD, do_ldst_PLS_D, false, true, MO_Q)
>  
>  static bool do_cmp_X(DisasContext *ctx, arg_X_bfl *a, bool s)
>  {
> +    if ((ctx->insns_flags & PPC_64B) == 0) {
> +        /*
> +         * For 32-bit implementations, The Programming Environments Manual says
> +         * that "the L field must be cleared, otherwise the instruction form is
> +         * invalid." It seems, however, that most 32-bit CPUs ignore invalid
> +         * forms (e.g., section "Instruction Formats" of the 405 and 440
> +         * manuals, "Integer Compare Instructions" of the 601 manual), with the
> +         * notable exception of the e500 and e500mc, where L=1 was reported to
> +         * cause an exception.
> +         */
> +        if (a->l) {
> +            if ((ctx->insns_flags2 & PPC2_BOOKE206)) {
> +                /*
> +                 * For 32-bit Book E v2.06 implementations (i.e. e500/e500mc),
> +                 * generate an illegal instruction exception.
> +                 */
> +                return false;
> +            } else {
> +                qemu_log_mask(LOG_GUEST_ERROR,
> +                        "Invalid form of CMP%s at 0x" TARGET_FMT_lx ", L = 1\n",
> +                        s ? "" : "L", ctx->cia);
> +            }
> +        }
> +        gen_op_cmp32(cpu_gpr[a->ra], cpu_gpr[a->rb], s, a->bf);
> +        return true;
> +    }
> +
> +    /* For 64-bit implementations, deal with bit L accordingly. */
>      if (a->l) {
> -        REQUIRE_64BIT(ctx);
>          gen_op_cmp(cpu_gpr[a->ra], cpu_gpr[a->rb], s, a->bf);
>      } else {
>          gen_op_cmp32(cpu_gpr[a->ra], cpu_gpr[a->rb], s, a->bf);
> @@ -156,8 +183,35 @@ static bool do_cmp_X(DisasContext *ctx, arg_X_bfl *a, bool s)
>  
>  static bool do_cmp_D(DisasContext *ctx, arg_D_bf *a, bool s)
>  {
> +    if ((ctx->insns_flags & PPC_64B) == 0) {
> +        /*
> +         * For 32-bit implementations, The Programming Environments Manual says
> +         * that "the L field must be cleared, otherwise the instruction form is
> +         * invalid." It seems, however, that most 32-bit CPUs ignore invalid
> +         * forms (e.g., section "Instruction Formats" of the 405 and 440
> +         * manuals, "Integer Compare Instructions" of the 601 manual), with the
> +         * notable exception of the e500 and e500mc, where L=1 was reported to
> +         * cause an exception.
> +         */
> +        if (a->l) {
> +            if ((ctx->insns_flags2 & PPC2_BOOKE206)) {
> +                /*
> +                 * For 32-bit Book E v2.06 implementations (i.e. e500/e500mc),
> +                 * generate an illegal instruction exception.
> +                 */
> +                return false;
> +            } else {
> +                qemu_log_mask(LOG_GUEST_ERROR,
> +                        "Invalid form of CMP%s at 0x" TARGET_FMT_lx ", L = 1\n",
> +                        s ? "I" : "LI", ctx->cia);
> +            }
> +        }
> +        gen_op_cmp32(cpu_gpr[a->ra], tcg_constant_tl(a->imm), s, a->bf);
> +        return true;
> +    }
> +
> +    /* For 64-bit implementations, deal with bit L accordingly. */
>      if (a->l) {
> -        REQUIRE_64BIT(ctx);
>          gen_op_cmp(cpu_gpr[a->ra], tcg_constant_tl(a->imm), s, a->bf);
>      } else {
>          gen_op_cmp32(cpu_gpr[a->ra], tcg_constant_tl(a->imm), s, a->bf);

-- 
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: 833 bytes --]

Re: [PATCH] target/ppc: Ease L=0 requirement on cmp/cmpi/cmpl/cmpli for ppc32

[BALATON Zoltan]

On Mon, 19 Jul 2021, David Gibson wrote:
> On Thu, Jul 15, 2021 at 09:29:50AM -0300, matheus.ferst@eldorado.org.br wrote:
>> From: Matheus Ferst <matheus.ferst@eldorado.org.br>
>>
>> In commit 8f0a4b6a9, we started to require L=0 for ppc32 to match what
>> The Programming Environments Manual say:
>>
>> "For 32-bit implementations, the L field must be cleared, otherwise
>> the instruction form is invalid."
>>
>> Further digging, however, shown that older CPUs have different behavior
>> concerning invalid forms. E.g.: 440 and 405 manuals say that:
>>
>> "Unless otherwise noted, the PPC440 will execute all invalid instruction
>> forms without causing an Illegal Instruction exception".
>>
>> While the PowerISA has an arguably more restrictive:
>>
>> "In general, any attempt to execute an invalid form of an instruction
>> will either cause the system illegal instruction error handler to be
>> invoked or yield boundedly undefined results."
>
> That's actually less restrictive.  "boundedly undefined" lets the
> implementation do nearly anything that won't mess up a hypervisor.
> Both ignoring the illegal bits and issuing an invalid instruction
> exception are definitely permissible within the meaning of "boundedly
> undefined".
>
>> Finally, BALATON Zoltan (CC'ed) reported that the stricter behavior
>> broke AROS boot on sam460ex. This patch address this regression by only
>> logging a guest error, except for CPUs known to raise an exception for
>> this case (e500 and e500mc).
>
> So.. as a rule of thumb, I'd prefer to have qemu give explicit
> failures (e.g. program check traps) where there's implementation
> specific or architecture undefined behaviour.  On the other hand,
> having a real guest that relies on the specific behaviour of real
> implementations is a compelling reason to break that rule of thumb.

One still should get log messages about it with -d guest_errors so that 
can be used for identifying problems with guest code that otherwise runs 
fine on real CPU.

> Given it's a behavioural change, I'm disinclined to squeeze this in
> for qemu-6.1, but I'll consider it for 6.2.  Richard, any thoughts?

Well, it's a regression from 6.0 and delaying it to 6.2 means we would 
have a release with a known issue that prevents a guest from running which 
could be fixed by this patch so I argue this is a bug fix that should be 
in 6.1. The behaviour change was the patch this one fixes (8f0a4b6a9, 
mentioned in commit message but could also be a Fixes: tag).

Regards,
BALATON Zoltan

>> Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
>> ---
>>  target/ppc/translate/fixedpoint-impl.c.inc | 58 +++++++++++++++++++++-
>>  1 file changed, 56 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/ppc/translate/fixedpoint-impl.c.inc b/target/ppc/translate/fixedpoint-impl.c.inc
>> index f4fcfadbfc..1c35b60eb4 100644
>> --- a/target/ppc/translate/fixedpoint-impl.c.inc
>> +++ b/target/ppc/translate/fixedpoint-impl.c.inc
>> @@ -145,8 +145,35 @@ TRANS64(PSTD, do_ldst_PLS_D, false, true, MO_Q)
>>
>>  static bool do_cmp_X(DisasContext *ctx, arg_X_bfl *a, bool s)
>>  {
>> +    if ((ctx->insns_flags & PPC_64B) == 0) {
>> +        /*
>> +         * For 32-bit implementations, The Programming Environments Manual says
>> +         * that "the L field must be cleared, otherwise the instruction form is
>> +         * invalid." It seems, however, that most 32-bit CPUs ignore invalid
>> +         * forms (e.g., section "Instruction Formats" of the 405 and 440
>> +         * manuals, "Integer Compare Instructions" of the 601 manual), with the
>> +         * notable exception of the e500 and e500mc, where L=1 was reported to
>> +         * cause an exception.
>> +         */
>> +        if (a->l) {
>> +            if ((ctx->insns_flags2 & PPC2_BOOKE206)) {
>> +                /*
>> +                 * For 32-bit Book E v2.06 implementations (i.e. e500/e500mc),
>> +                 * generate an illegal instruction exception.
>> +                 */
>> +                return false;
>> +            } else {
>> +                qemu_log_mask(LOG_GUEST_ERROR,
>> +                        "Invalid form of CMP%s at 0x" TARGET_FMT_lx ", L = 1\n",
>> +                        s ? "" : "L", ctx->cia);
>> +            }
>> +        }
>> +        gen_op_cmp32(cpu_gpr[a->ra], cpu_gpr[a->rb], s, a->bf);
>> +        return true;
>> +    }
>> +
>> +    /* For 64-bit implementations, deal with bit L accordingly. */
>>      if (a->l) {
>> -        REQUIRE_64BIT(ctx);
>>          gen_op_cmp(cpu_gpr[a->ra], cpu_gpr[a->rb], s, a->bf);
>>      } else {
>>          gen_op_cmp32(cpu_gpr[a->ra], cpu_gpr[a->rb], s, a->bf);
>> @@ -156,8 +183,35 @@ static bool do_cmp_X(DisasContext *ctx, arg_X_bfl *a, bool s)
>>
>>  static bool do_cmp_D(DisasContext *ctx, arg_D_bf *a, bool s)
>>  {
>> +    if ((ctx->insns_flags & PPC_64B) == 0) {
>> +        /*
>> +         * For 32-bit implementations, The Programming Environments Manual says
>> +         * that "the L field must be cleared, otherwise the instruction form is
>> +         * invalid." It seems, however, that most 32-bit CPUs ignore invalid
>> +         * forms (e.g., section "Instruction Formats" of the 405 and 440
>> +         * manuals, "Integer Compare Instructions" of the 601 manual), with the
>> +         * notable exception of the e500 and e500mc, where L=1 was reported to
>> +         * cause an exception.
>> +         */
>> +        if (a->l) {
>> +            if ((ctx->insns_flags2 & PPC2_BOOKE206)) {
>> +                /*
>> +                 * For 32-bit Book E v2.06 implementations (i.e. e500/e500mc),
>> +                 * generate an illegal instruction exception.
>> +                 */
>> +                return false;
>> +            } else {
>> +                qemu_log_mask(LOG_GUEST_ERROR,
>> +                        "Invalid form of CMP%s at 0x" TARGET_FMT_lx ", L = 1\n",
>> +                        s ? "I" : "LI", ctx->cia);
>> +            }
>> +        }
>> +        gen_op_cmp32(cpu_gpr[a->ra], tcg_constant_tl(a->imm), s, a->bf);
>> +        return true;
>> +    }
>> +
>> +    /* For 64-bit implementations, deal with bit L accordingly. */
>>      if (a->l) {
>> -        REQUIRE_64BIT(ctx);
>>          gen_op_cmp(cpu_gpr[a->ra], tcg_constant_tl(a->imm), s, a->bf);
>>      } else {
>>          gen_op_cmp32(cpu_gpr[a->ra], tcg_constant_tl(a->imm), s, a->bf);
>
>

Re: [PATCH] target/ppc: Ease L=0 requirement on cmp/cmpi/cmpl/cmpli for ppc32

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 7319 bytes --]

On Mon, Jul 19, 2021 at 12:21:11PM +0200, BALATON Zoltan wrote:
> On Mon, 19 Jul 2021, David Gibson wrote:
> > On Thu, Jul 15, 2021 at 09:29:50AM -0300, matheus.ferst@eldorado.org.br wrote:
> > > From: Matheus Ferst <matheus.ferst@eldorado.org.br>
> > > 
> > > In commit 8f0a4b6a9, we started to require L=0 for ppc32 to match what
> > > The Programming Environments Manual say:
> > > 
> > > "For 32-bit implementations, the L field must be cleared, otherwise
> > > the instruction form is invalid."
> > > 
> > > Further digging, however, shown that older CPUs have different behavior
> > > concerning invalid forms. E.g.: 440 and 405 manuals say that:
> > > 
> > > "Unless otherwise noted, the PPC440 will execute all invalid instruction
> > > forms without causing an Illegal Instruction exception".
> > > 
> > > While the PowerISA has an arguably more restrictive:
> > > 
> > > "In general, any attempt to execute an invalid form of an instruction
> > > will either cause the system illegal instruction error handler to be
> > > invoked or yield boundedly undefined results."
> > 
> > That's actually less restrictive.  "boundedly undefined" lets the
> > implementation do nearly anything that won't mess up a hypervisor.
> > Both ignoring the illegal bits and issuing an invalid instruction
> > exception are definitely permissible within the meaning of "boundedly
> > undefined".
> > 
> > > Finally, BALATON Zoltan (CC'ed) reported that the stricter behavior
> > > broke AROS boot on sam460ex. This patch address this regression by only
> > > logging a guest error, except for CPUs known to raise an exception for
> > > this case (e500 and e500mc).
> > 
> > So.. as a rule of thumb, I'd prefer to have qemu give explicit
> > failures (e.g. program check traps) where there's implementation
> > specific or architecture undefined behaviour.  On the other hand,
> > having a real guest that relies on the specific behaviour of real
> > implementations is a compelling reason to break that rule of thumb.
> 
> One still should get log messages about it with -d guest_errors so that can
> be used for identifying problems with guest code that otherwise runs fine on
> real CPU.
> 
> > Given it's a behavioural change, I'm disinclined to squeeze this in
> > for qemu-6.1, but I'll consider it for 6.2.  Richard, any thoughts?
> 
> Well, it's a regression from 6.0 and delaying it to 6.2 means we would have

Ah, well that changes everything.  The commit message should
definitely mention that it's a regression.

> a release with a known issue that prevents a guest from running which could
> be fixed by this patch so I argue this is a bug fix that should be in 6.1.
> The behaviour change was the patch this one fixes (8f0a4b6a9, mentioned in
> commit message but could also be a Fixes: tag).
> 
> Regards,
> BALATON Zoltan
> 
> > > Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
> > > ---
> > >  target/ppc/translate/fixedpoint-impl.c.inc | 58 +++++++++++++++++++++-
> > >  1 file changed, 56 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/target/ppc/translate/fixedpoint-impl.c.inc b/target/ppc/translate/fixedpoint-impl.c.inc
> > > index f4fcfadbfc..1c35b60eb4 100644
> > > --- a/target/ppc/translate/fixedpoint-impl.c.inc
> > > +++ b/target/ppc/translate/fixedpoint-impl.c.inc
> > > @@ -145,8 +145,35 @@ TRANS64(PSTD, do_ldst_PLS_D, false, true, MO_Q)
> > > 
> > >  static bool do_cmp_X(DisasContext *ctx, arg_X_bfl *a, bool s)
> > >  {
> > > +    if ((ctx->insns_flags & PPC_64B) == 0) {
> > > +        /*
> > > +         * For 32-bit implementations, The Programming Environments Manual says
> > > +         * that "the L field must be cleared, otherwise the instruction form is
> > > +         * invalid." It seems, however, that most 32-bit CPUs ignore invalid
> > > +         * forms (e.g., section "Instruction Formats" of the 405 and 440
> > > +         * manuals, "Integer Compare Instructions" of the 601 manual), with the
> > > +         * notable exception of the e500 and e500mc, where L=1 was reported to
> > > +         * cause an exception.
> > > +         */
> > > +        if (a->l) {
> > > +            if ((ctx->insns_flags2 & PPC2_BOOKE206)) {
> > > +                /*
> > > +                 * For 32-bit Book E v2.06 implementations (i.e. e500/e500mc),
> > > +                 * generate an illegal instruction exception.
> > > +                 */
> > > +                return false;
> > > +            } else {
> > > +                qemu_log_mask(LOG_GUEST_ERROR,
> > > +                        "Invalid form of CMP%s at 0x" TARGET_FMT_lx ", L = 1\n",
> > > +                        s ? "" : "L", ctx->cia);
> > > +            }
> > > +        }
> > > +        gen_op_cmp32(cpu_gpr[a->ra], cpu_gpr[a->rb], s, a->bf);
> > > +        return true;
> > > +    }
> > > +
> > > +    /* For 64-bit implementations, deal with bit L accordingly. */
> > >      if (a->l) {
> > > -        REQUIRE_64BIT(ctx);
> > >          gen_op_cmp(cpu_gpr[a->ra], cpu_gpr[a->rb], s, a->bf);
> > >      } else {
> > >          gen_op_cmp32(cpu_gpr[a->ra], cpu_gpr[a->rb], s, a->bf);
> > > @@ -156,8 +183,35 @@ static bool do_cmp_X(DisasContext *ctx, arg_X_bfl *a, bool s)
> > > 
> > >  static bool do_cmp_D(DisasContext *ctx, arg_D_bf *a, bool s)
> > >  {
> > > +    if ((ctx->insns_flags & PPC_64B) == 0) {
> > > +        /*
> > > +         * For 32-bit implementations, The Programming Environments Manual says
> > > +         * that "the L field must be cleared, otherwise the instruction form is
> > > +         * invalid." It seems, however, that most 32-bit CPUs ignore invalid
> > > +         * forms (e.g., section "Instruction Formats" of the 405 and 440
> > > +         * manuals, "Integer Compare Instructions" of the 601 manual), with the
> > > +         * notable exception of the e500 and e500mc, where L=1 was reported to
> > > +         * cause an exception.
> > > +         */
> > > +        if (a->l) {
> > > +            if ((ctx->insns_flags2 & PPC2_BOOKE206)) {
> > > +                /*
> > > +                 * For 32-bit Book E v2.06 implementations (i.e. e500/e500mc),
> > > +                 * generate an illegal instruction exception.
> > > +                 */
> > > +                return false;
> > > +            } else {
> > > +                qemu_log_mask(LOG_GUEST_ERROR,
> > > +                        "Invalid form of CMP%s at 0x" TARGET_FMT_lx ", L = 1\n",
> > > +                        s ? "I" : "LI", ctx->cia);
> > > +            }
> > > +        }
> > > +        gen_op_cmp32(cpu_gpr[a->ra], tcg_constant_tl(a->imm), s, a->bf);
> > > +        return true;
> > > +    }
> > > +
> > > +    /* For 64-bit implementations, deal with bit L accordingly. */
> > >      if (a->l) {
> > > -        REQUIRE_64BIT(ctx);
> > >          gen_op_cmp(cpu_gpr[a->ra], tcg_constant_tl(a->imm), s, a->bf);
> > >      } else {
> > >          gen_op_cmp32(cpu_gpr[a->ra], tcg_constant_tl(a->imm), s, a->bf);
> > 
> > 
> 

-- 
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: 833 bytes --]

Re: [PATCH] target/ppc: Ease L=0 requirement on cmp/cmpi/cmpl/cmpli for ppc32

[Richard Henderson]

On 7/18/21 4:42 PM, David Gibson wrote:
> Given it's a behavioural change, I'm disinclined to squeeze this in
> for qemu-6.1, but I'll consider it for 6.2.  Richard, any thoughts?

The behavioral change happened in 6.1, and this is (partially) reverting that change. 
With this patch, the new behaviour reverts to (1) raising an exception for e500* and (2) 
otherwise, logging a guest error.

It should go in, IMO.


r~