| Message ID | 1406563102-11035-2-git-send-email-elta.era@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Jul 28, 2014 at 11:58:22PM +0800, Dongxue Zhang wrote: > Use 'if' to make sure the real msb greater than the lsb. As the compiler may > not do this. What are you trying to fix exactly? These cases are defined as "unpredictable" in the MIPS ISA manual, which is what is implemented in QEMU. In addition on the MIPS64R2 implementations I tested (Cavium Octeon, Loongson 3) these cases do not trigger a reserved instruction exception. > Signed-off-by: Dongxue Zhang <elta.era@gmail.com> > --- > target-mips/translate.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/target-mips/translate.c b/target-mips/translate.c > index c381366..e2cce31 100644 > --- a/target-mips/translate.c > +++ b/target-mips/translate.c > @@ -3946,14 +3946,23 @@ static void gen_bitops (DisasContext *ctx, uint32_t opc, int rt, > break; > #if defined(TARGET_MIPS64) > case OPC_DINSM: > + if (lsb > (msb + 32)) { > + goto fail; > + } This test is always false, as lsb and msb are 5 bits values. > gen_load_gpr(t0, rt); > tcg_gen_deposit_tl(t0, t0, t1, lsb, msb + 32 - lsb + 1); > break; > case OPC_DINSU: > + if (lsb > msb) { > + goto fail; > + } > gen_load_gpr(t0, rt); > tcg_gen_deposit_tl(t0, t0, t1, lsb + 32, msb - lsb + 1); > break; > case OPC_DINS: > + if (lsb > msb) { > + goto fail; > + } > gen_load_gpr(t0, rt); > tcg_gen_deposit_tl(t0, t0, t1, lsb, msb - lsb + 1); > break; > -- > 1.8.1.2 > > >
On 28 July 2014 22:42, Aurelien Jarno <aurelien@aurel32.net> wrote: > On Mon, Jul 28, 2014 at 11:58:22PM +0800, Dongxue Zhang wrote: >> Use 'if' to make sure the real msb greater than the lsb. As the compiler may >> not do this. > > What are you trying to fix exactly? These cases are defined as > "unpredictable" in the MIPS ISA manual, which is what is implemented in > QEMU. This may be true, but the TCG README doesn't define negative lengths as being "unspecified behaviour" (ie guaranteed to at least not crash even if the result isn't specified), and in fact the implementation of tcg_gen_deposit will assert on negative lengths. We shouldn't implement guest unpredictable cases as "crash QEMU". thanks -- PMM
On Mon, Jul 28, 2014 at 11:01:02PM +0100, Peter Maydell wrote: > On 28 July 2014 22:42, Aurelien Jarno <aurelien@aurel32.net> wrote: > > On Mon, Jul 28, 2014 at 11:58:22PM +0800, Dongxue Zhang wrote: > >> Use 'if' to make sure the real msb greater than the lsb. As the compiler may > >> not do this. > > > > What are you trying to fix exactly? These cases are defined as > > "unpredictable" in the MIPS ISA manual, which is what is implemented in > > QEMU. > > This may be true, but the TCG README doesn't define negative > lengths as being "unspecified behaviour" (ie guaranteed to at > least not crash even if the result isn't specified), and in fact the > implementation of tcg_gen_deposit will assert on negative lengths. > We shouldn't implement guest unpredictable cases as "crash QEMU". Well I tried this code under QEMU, and it clearly doesn't crash. It seems the assert are not enabled with the default configuration options. That said I agree it's something to avoid, but I don't think triggering a RI exception is the thing to do (even if it is correct according the MIPS ISA manual) when real silicon output a random result instead.
On 28 July 2014 23:32, Aurelien Jarno <aurelien@aurel32.net> wrote: > On Mon, Jul 28, 2014 at 11:01:02PM +0100, Peter Maydell wrote: >> This may be true, but the TCG README doesn't define negative >> lengths as being "unspecified behaviour" (ie guaranteed to at >> least not crash even if the result isn't specified), and in fact the >> implementation of tcg_gen_deposit will assert on negative lengths. >> We shouldn't implement guest unpredictable cases as "crash QEMU". > > Well I tried this code under QEMU, and it clearly doesn't crash. It > seems the assert are not enabled with the default configuration options. Try --enable-debug... > That said I agree it's something to avoid, but I don't think triggering > a RI exception is the thing to do (even if it is correct according the > MIPS ISA manual) when real silicon output a random result instead. Yes, you could emit code to do that instead if you like. thanks -- PMM
On Mon, Jul 28, 2014 at 11:34:30PM +0100, Peter Maydell wrote: > On 28 July 2014 23:32, Aurelien Jarno <aurelien@aurel32.net> wrote: > > On Mon, Jul 28, 2014 at 11:01:02PM +0100, Peter Maydell wrote: > >> This may be true, but the TCG README doesn't define negative > >> lengths as being "unspecified behaviour" (ie guaranteed to at > >> least not crash even if the result isn't specified), and in fact the > >> implementation of tcg_gen_deposit will assert on negative lengths. > >> We shouldn't implement guest unpredictable cases as "crash QEMU". > > > > Well I tried this code under QEMU, and it clearly doesn't crash. It > > seems the assert are not enabled with the default configuration options. > > Try --enable-debug... That's my point, it's only in debug mode, not in the default configuration. > > That said I agree it's something to avoid, but I don't think triggering > > a RI exception is the thing to do (even if it is correct according the > > MIPS ISA manual) when real silicon output a random result instead. > > Yes, you could emit code to do that instead if you like. When I said random, it didn't say in the sense of random generator, but in the sense a result that might depend on the input value and the silicon implementation. It would be silly to emit code just for that, but it would be smart for example to skip the deposit op in that case instead of triggering an exception.
On 07/29/2014 06:52 AM, Aurelien Jarno wrote: > On Mon, Jul 28, 2014 at 11:34:30PM +0100, Peter Maydell wrote: >> On 28 July 2014 23:32, Aurelien Jarno <aurelien@aurel32.net> wrote: >>> On Mon, Jul 28, 2014 at 11:01:02PM +0100, Peter Maydell wrote: >>>> This may be true, but the TCG README doesn't define negative >>>> lengths as being "unspecified behaviour" (ie guaranteed to at >>>> least not crash even if the result isn't specified), and in fact the >>>> implementation of tcg_gen_deposit will assert on negative lengths. >>>> We shouldn't implement guest unpredictable cases as "crash QEMU". >>> Well I tried this code under QEMU, and it clearly doesn't crash. It >>> seems the assert are not enabled with the default configuration options. >> Try --enable-debug... > That's my point, it's only in debug mode, not in the default > configuration. Maybe remove the tcg_debug_assert in tcg_gen_deposit_i64 and tcg_gen_deposit_i64 is a better way. But it may cause other mistake in other architecture, i'm not sure. > >>> That said I agree it's something to avoid, but I don't think triggering >>> a RI exception is the thing to do (even if it is correct according the >>> MIPS ISA manual) when real silicon output a random result instead. >> Yes, you could emit code to do that instead if you like. > When I said random, it didn't say in the sense of random generator, but > in the sense a result that might depend on the input value and the > silicon implementation. It would be silly to emit code just for that, > but it would be smart for example to skip the deposit op in that case > instead of triggering an exception. > I think, debug mode shouldn't crash the qemu with an unpredictable operation, so i want to fix it. And you say there shouldn't raise RI, i agree with you. Or when lsb > msb, just leave the code and do nothing. What do you think about this way?
On 28 July 2014 23:52, Aurelien Jarno <aurelien@aurel32.net> wrote: > On Mon, Jul 28, 2014 at 11:34:30PM +0100, Peter Maydell wrote: >> On 28 July 2014 23:32, Aurelien Jarno <aurelien@aurel32.net> wrote: >> > On Mon, Jul 28, 2014 at 11:01:02PM +0100, Peter Maydell wrote: >> >> This may be true, but the TCG README doesn't define negative >> >> lengths as being "unspecified behaviour" (ie guaranteed to at >> >> least not crash even if the result isn't specified), and in fact the >> >> implementation of tcg_gen_deposit will assert on negative lengths. >> >> We shouldn't implement guest unpredictable cases as "crash QEMU". >> > >> > Well I tried this code under QEMU, and it clearly doesn't crash. It >> > seems the assert are not enabled with the default configuration options. >> >> Try --enable-debug... > > That's my point, it's only in debug mode, not in the default > configuration. Debug builds are pretty common though, it's not exactly something obscure like "only crashes on SPARC hosts". >> > That said I agree it's something to avoid, but I don't think triggering >> > a RI exception is the thing to do (even if it is correct according the >> > MIPS ISA manual) when real silicon output a random result instead. >> >> Yes, you could emit code to do that instead if you like. > > When I said random, it didn't say in the sense of random generator, but > in the sense a result that might depend on the input value and the > silicon implementation. It would be silly to emit code just for that, > but it would be smart for example to skip the deposit op in that case > instead of triggering an exception. That's what I had in mind, yes. thanks -- PMM
On Tue, Jul 29, 2014 at 08:41:08PM +0800, Elta wrote: > I think, debug mode shouldn't crash the qemu with an unpredictable > operation, > so i want to fix it. And you say there shouldn't raise RI, i agree with you. Agreed. > Or when lsb > msb, just leave the code and do nothing. What do you > think about > this way? Yes, you can use something like: if (lsb <= msb) { deposit(...) }
Ok, I got you. I will re-build a new patch for all the bitops. 2014-07-29 22:08 GMT+08:00 Aurelien Jarno <aurelien@aurel32.net>: > On Tue, Jul 29, 2014 at 08:41:08PM +0800, Elta wrote: > > I think, debug mode shouldn't crash the qemu with an unpredictable > > operation, > > so i want to fix it. And you say there shouldn't raise RI, i agree with > you. > > Agreed. > > > Or when lsb > msb, just leave the code and do nothing. What do you > > think about > > this way? > > Yes, you can use something like: > > if (lsb <= msb) { > deposit(...) > } > > -- > Aurelien Jarno GPG: 4096R/1DDD8C9B > aurelien@aurel32.net http://www.aurel32.net >
diff --git a/target-mips/translate.c b/target-mips/translate.c index c381366..e2cce31 100644 --- a/target-mips/translate.c +++ b/target-mips/translate.c @@ -3946,14 +3946,23 @@ static void gen_bitops (DisasContext *ctx, uint32_t opc, int rt, break; #if defined(TARGET_MIPS64) case OPC_DINSM: + if (lsb > (msb + 32)) { + goto fail; + } gen_load_gpr(t0, rt); tcg_gen_deposit_tl(t0, t0, t1, lsb, msb + 32 - lsb + 1); break; case OPC_DINSU: + if (lsb > msb) { + goto fail; + } gen_load_gpr(t0, rt); tcg_gen_deposit_tl(t0, t0, t1, lsb + 32, msb - lsb + 1); break; case OPC_DINS: + if (lsb > msb) { + goto fail; + } gen_load_gpr(t0, rt); tcg_gen_deposit_tl(t0, t0, t1, lsb, msb - lsb + 1); break;
Use 'if' to make sure the real msb greater than the lsb. As the compiler may not do this. Signed-off-by: Dongxue Zhang <elta.era@gmail.com> --- target-mips/translate.c | 9 +++++++++ 1 file changed, 9 insertions(+)