Message ID | 20210715122950.2366428-1-matheus.ferst@eldorado.org.br |
---|---|
State | New |
Headers | show |
Series | target/ppc: Ease L=0 requirement on cmp/cmpi/cmpl/cmpli for ppc32 | expand |
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); >
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); >
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.
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);
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); > >
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); > > > > >
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~
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);