* Re: [PATCH] target/ppc: Ease L=0 requirement on cmp/cmpi/cmpl/cmpli for ppc32
2021-07-15 12:29 [PATCH] target/ppc: Ease L=0 requirement on cmp/cmpi/cmpl/cmpli for ppc32 matheus.ferst
@ 2021-07-15 13:12 ` BALATON Zoltan
2021-07-15 13:14 ` BALATON Zoltan
2021-07-19 2:42 ` David Gibson
2 siblings, 0 replies; 8+ messages in thread
From: BALATON Zoltan @ 2021-07-15 13:12 UTC (permalink / raw)
To: Matheus Ferst; +Cc: richard.henderson, groug, qemu-ppc, qemu-devel, david
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);
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] target/ppc: Ease L=0 requirement on cmp/cmpi/cmpl/cmpli for ppc32
2021-07-15 12:29 [PATCH] target/ppc: Ease L=0 requirement on cmp/cmpi/cmpl/cmpli for ppc32 matheus.ferst
2021-07-15 13:12 ` BALATON Zoltan
@ 2021-07-15 13:14 ` BALATON Zoltan
2021-07-15 14:29 ` Matheus K. Ferst
2021-07-19 2:42 ` David Gibson
2 siblings, 1 reply; 8+ messages in thread
From: BALATON Zoltan @ 2021-07-15 13:14 UTC (permalink / raw)
To: Matheus Ferst; +Cc: richard.henderson, groug, qemu-ppc, qemu-devel, david
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);
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] target/ppc: Ease L=0 requirement on cmp/cmpi/cmpl/cmpli for ppc32
2021-07-15 13:14 ` BALATON Zoltan
@ 2021-07-15 14:29 ` Matheus K. Ferst
0 siblings, 0 replies; 8+ messages in thread
From: Matheus K. Ferst @ 2021-07-15 14:29 UTC (permalink / raw)
To: BALATON Zoltan; +Cc: richard.henderson, groug, qemu-ppc, qemu-devel, david
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>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] target/ppc: Ease L=0 requirement on cmp/cmpi/cmpl/cmpli for ppc32
2021-07-15 12:29 [PATCH] target/ppc: Ease L=0 requirement on cmp/cmpi/cmpl/cmpli for ppc32 matheus.ferst
2021-07-15 13:12 ` BALATON Zoltan
2021-07-15 13:14 ` BALATON Zoltan
@ 2021-07-19 2:42 ` David Gibson
2021-07-19 10:21 ` BALATON Zoltan
2021-07-19 20:07 ` Richard Henderson
2 siblings, 2 replies; 8+ messages in thread
From: David Gibson @ 2021-07-19 2:42 UTC (permalink / raw)
To: matheus.ferst; +Cc: richard.henderson, qemu-ppc, qemu-devel, groug
[-- 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 --]
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] target/ppc: Ease L=0 requirement on cmp/cmpi/cmpl/cmpli for ppc32
2021-07-19 2:42 ` David Gibson
@ 2021-07-19 10:21 ` BALATON Zoltan
2021-07-19 12:09 ` David Gibson
2021-07-19 20:07 ` Richard Henderson
1 sibling, 1 reply; 8+ messages in thread
From: BALATON Zoltan @ 2021-07-19 10:21 UTC (permalink / raw)
To: David Gibson
Cc: qemu-ppc, richard.henderson, matheus.ferst, qemu-devel, groug
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);
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] target/ppc: Ease L=0 requirement on cmp/cmpi/cmpl/cmpli for ppc32
2021-07-19 10:21 ` BALATON Zoltan
@ 2021-07-19 12:09 ` David Gibson
0 siblings, 0 replies; 8+ messages in thread
From: David Gibson @ 2021-07-19 12:09 UTC (permalink / raw)
To: BALATON Zoltan
Cc: qemu-ppc, richard.henderson, matheus.ferst, qemu-devel, groug
[-- 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 --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] target/ppc: Ease L=0 requirement on cmp/cmpi/cmpl/cmpli for ppc32
2021-07-19 2:42 ` David Gibson
2021-07-19 10:21 ` BALATON Zoltan
@ 2021-07-19 20:07 ` Richard Henderson
1 sibling, 0 replies; 8+ messages in thread
From: Richard Henderson @ 2021-07-19 20:07 UTC (permalink / raw)
To: David Gibson, matheus.ferst; +Cc: qemu-ppc, qemu-devel, groug
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~
^ permalink raw reply [flat|nested] 8+ messages in thread