* [Qemu-devel] [PATCH] Revert "target-sparc: Make cpu_dst local to OP=2 insns"
@ 2012-10-20 14:48 Aurelien Jarno
2012-10-20 22:48 ` Richard Henderson
0 siblings, 1 reply; 6+ messages in thread
From: Aurelien Jarno @ 2012-10-20 14:48 UTC (permalink / raw)
To: qemu-devel; +Cc: Blue Swirl, Aurelien Jarno, Richard Henderson
Commit 5793f2a47e201d251856c7956d6f7907ec0d9f1f causes data corruption
in system mode:
| [....] Synthesizing the initial hotplug events...udevd[291]: timeout: killing 'net.agent' [302]
| udevd[291]: 'net.agent' [302] terminated by signal 9 (Killed)
| udevd[299]: timeout: killing '/sbin/modprobe -b of:NpackagesT<NULL>' [313]
| udevd[301]: timeout: killing '/sbin/modprobe -b of:Nvirtual-memoryT<NULL>' [314]
| udevd[300]: timeout: killing '/sbin/modprobe -b of:NmemoryTmemory' [315]
| udevd[299]: '/sbin/modprobe -b of:NpackagesT<NULL>' [313] terminated by signal 9 (Killed)
| udevd[301]: '/sbin/modprobe -b of:Nvirtual-memoryT<NULL>' [314] terminated by signal 9 (Killed)
| udevd[300]: '/sbin/modprobe -b of:NmemoryTmemory' [315] terminated by signal 9 (Killed)
| udevd[290]: timeout '/sbin/blkid -o udev -p /dev/vda1'
| done.
| [ ok ] Waiting for /dev to be fully populated...done.
| [....] Activating swap...[ 44.814485] Adding 1048568k swap on /dev/vda2. Priority:-1 extents:1 across:1048568k
| done.
| [ 46.619096] EXT4-fs (vda4): re-mounted. Opts: (null)
| [....] Checking root file system...fsck from util-linux 2.20.1
| e2fsck 1.42.5 (29-Jul-2012)
| ext2fs_open2: The ext2 superblock is corrupt
| fsck.ext4: Superblock invalid, trying backup blocks...
| fsck.ext4: The ext2 superblock is corrupt while trying to open /dev/vda4
|
| The superblock could not be read or does not describe a correct ext2
| filesystem. If the device is valid and it really contains an ext2
| filesystem (and not swap or ufs or something else), then the superblock
| is corrupt, and you might try running e2fsck with an alternate superblock:
| e2fsck -b 8193 <device>
|
| fsck died with exit status 8
| udevd[332]: timeout '/sbin/blkid -o udev -p /dev/vda4'
I am not sure it is the real problem, but at least the optimization of
using the destination register as a temporary is wrong when the
instruction might trigger an exception. In that case the result is
written to the destination register while it should have not.
This reverts commit 5793f2a47e201d251856c7956d6f7907ec0d9f1f.
Cc: Blue Swirl <blauwirbel@gmail.com>
Cc: Richard Henderson <rth@twiddle.net>
Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
target-sparc/translate.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/target-sparc/translate.c b/target-sparc/translate.c
index 4321393..04f3ac4 100644
--- a/target-sparc/translate.c
+++ b/target-sparc/translate.c
@@ -48,7 +48,7 @@ static TCGv cpu_y;
#ifndef CONFIG_USER_ONLY
static TCGv cpu_tbr;
#endif
-static TCGv cpu_cond;
+static TCGv cpu_cond, cpu_dst;
#ifdef TARGET_SPARC64
static TCGv_i32 cpu_xcc, cpu_asi, cpu_fprs;
static TCGv cpu_gsr;
@@ -2525,6 +2525,7 @@ static void disas_sparc_insn(DisasContext * dc, unsigned int insn)
}
opc = GET_FIELD(insn, 0, 1);
+
rd = GET_FIELD(insn, 2, 6);
switch (opc) {
@@ -2633,7 +2634,6 @@ static void disas_sparc_insn(DisasContext * dc, unsigned int insn)
case 2: /* FPU & Logical Operations */
{
unsigned int xop = GET_FIELD(insn, 7, 12);
- TCGv cpu_dst = gen_dest_gpr(dc, rd);
TCGv cpu_tmp0;
if (xop == 0x3a) { /* generate trap */
@@ -5295,9 +5295,13 @@ static inline void gen_intermediate_code_internal(TranslationBlock * tb,
last_pc = dc->pc;
insn = cpu_ldl_code(env, dc->pc);
+ cpu_dst = tcg_temp_new();
+
disas_sparc_insn(dc, insn);
num_insns++;
+ tcg_temp_free(cpu_dst);
+
if (dc->is_br)
break;
/* if the next PC is different, we abort now */
--
1.7.10.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH] Revert "target-sparc: Make cpu_dst local to OP=2 insns"
2012-10-20 14:48 [Qemu-devel] [PATCH] Revert "target-sparc: Make cpu_dst local to OP=2 insns" Aurelien Jarno
@ 2012-10-20 22:48 ` Richard Henderson
2012-10-20 23:17 ` Aurelien Jarno
0 siblings, 1 reply; 6+ messages in thread
From: Richard Henderson @ 2012-10-20 22:48 UTC (permalink / raw)
To: Aurelien Jarno; +Cc: Blue Swirl, qemu-devel
On 2012-10-21 00:48, Aurelien Jarno wrote:
> I am not sure it is the real problem, but at least the optimization of
> using the destination register as a temporary is wrong when the
> instruction might trigger an exception. In that case the result is
> written to the destination register while it should have not.
>
> This reverts commit 5793f2a47e201d251856c7956d6f7907ec0d9f1f.
Which insn might trigger an exception? Most OP=2 insns don't. There's
divide, but that's done out-of-line, so the assignment to dst does not
happen before the exception...
Is this sparc64? I assume so, since I did test sparc32...
r~
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] Revert "target-sparc: Make cpu_dst local to OP=2 insns"
2012-10-20 22:48 ` Richard Henderson
@ 2012-10-20 23:17 ` Aurelien Jarno
2012-10-21 21:29 ` Richard Henderson
0 siblings, 1 reply; 6+ messages in thread
From: Aurelien Jarno @ 2012-10-20 23:17 UTC (permalink / raw)
To: Richard Henderson; +Cc: Blue Swirl, qemu-devel
On Sun, Oct 21, 2012 at 08:48:52AM +1000, Richard Henderson wrote:
> On 2012-10-21 00:48, Aurelien Jarno wrote:
> > I am not sure it is the real problem, but at least the optimization of
> > using the destination register as a temporary is wrong when the
> > instruction might trigger an exception. In that case the result is
> > written to the destination register while it should have not.
> >
> > This reverts commit 5793f2a47e201d251856c7956d6f7907ec0d9f1f.
>
> Which insn might trigger an exception? Most OP=2 insns don't. There's
> divide, but that's done out-of-line, so the assignment to dst does not
> happen before the exception...
Indeed there a are a few one triggering exception, but I looked too
quickly and indeed they do the assignment before. There should be
another problem elsewhere as reverting this patch fixes the issue.
> Is this sparc64? I assume so, since I did test sparc32...
>
Yes it's with a sparc64 kernel. I can reproduce the problem with both a
32 and 64-bit userland, though it happens earlier with a 32-bit
userland.
--
Aurelien Jarno GPG: 1024D/F1BCDB73
aurelien@aurel32.net http://www.aurel32.net
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] Revert "target-sparc: Make cpu_dst local to OP=2 insns"
2012-10-20 23:17 ` Aurelien Jarno
@ 2012-10-21 21:29 ` Richard Henderson
2012-10-21 22:25 ` Aurelien Jarno
0 siblings, 1 reply; 6+ messages in thread
From: Richard Henderson @ 2012-10-21 21:29 UTC (permalink / raw)
To: Aurelien Jarno; +Cc: Blue Swirl, qemu-devel
On 2012-10-21 09:17, Aurelien Jarno wrote:
> On Sun, Oct 21, 2012 at 08:48:52AM +1000, Richard Henderson wrote:
>> On 2012-10-21 00:48, Aurelien Jarno wrote:
>>> I am not sure it is the real problem, but at least the optimization of
>>> using the destination register as a temporary is wrong when the
>>> instruction might trigger an exception. In that case the result is
>>> written to the destination register while it should have not.
>>>
>>> This reverts commit 5793f2a47e201d251856c7956d6f7907ec0d9f1f.
>>
>> Which insn might trigger an exception? Most OP=2 insns don't. There's
>> divide, but that's done out-of-line, so the assignment to dst does not
>> happen before the exception...
>
> Indeed there a are a few one triggering exception, but I looked too
> quickly and indeed they do the assignment before. There should be
> another problem elsewhere as reverting this patch fixes the issue.
>
>> Is this sparc64? I assume so, since I did test sparc32...
>>
>
> Yes it's with a sparc64 kernel. I can reproduce the problem with both a
> 32 and 64-bit userland, though it happens earlier with a 32-bit
> userland.
>
Sadly, I don't seem to be able to boot a "real" sparc64 system at all,
even with a qemu version before the patch set in question went in.
(1) The recent "vga cleanup" has totally broken video for all sparc.
This is bisectable to f2898771435701df33145cacabeb42c6aa3a9a16,
your patch that adjusts sun4u. One can see this problem just by
booting OpenBIOS; no kernel needed.
(2) Switching to serial console, the kernel gets stuck talking to the
disks. It gets so far, and then reports lost interrupts forever:
$ ../bld/sparc64-softmmu/qemu-system-sparc64 -kernel sparc64 -initrd initrd.gz -append "root=/dev/ram console=ttyS0" -hda ./hda.img -cdrom ./debian-6.0.6-sparc-netinst.iso -nographic
...
[ 14.488838] ata1.00: ATA-7: QEMU HARDDISK, 1.2.50, max UDMA/100
[ 14.489893] ata1.00: 25165824 sectors, multi 16: LBA48
[ 14.504620] ata1.00: configured for UDMA/33
[ 14.549962] scsi 0:0:0:0: Direct-Access ATA QEMU HARDDISK 1.2. PQ: 0 ANSI: 5
[ 14.732381] ata2.00: ATAPI: QEMU DVD-ROM, 1.2.50, max UDMA/100
[ 14.734341] ata2.00: configured for UDMA/33
[ 35.596592] ata2: lost interrupt (Status 0x50)
[ 56.594942] ata2: lost interrupt (Status 0x50)
I re-examined the opcodes in OP=2. The only one left that throws an inline exception
is Tcc, which of course doesn't write to a destination register at all. If, somehow,
there is some exception (or other write-before-use) that I've missed, then the following
patch should fix it, without moving the cpu_dst variable back to global scope. If this
patch does not fix the problem, then there's some erroneous use of cpu_dst that I've
missed, and the original patch is question is broken for some other reason.
Given my experience above, I wonder what you're testing differently? I'm trying the
stock standard debian 6.0.6 (current?) distribution media images. The kernel/initrd
images above are pulled out of that iso, as I couldn't make OpenBIOS boot the cdrom
image directly.
r~
diff --git a/target-sparc/translate.c b/target-sparc/translate.c
index 4321393..9e46f14 100644
--- a/target-sparc/translate.c
+++ b/target-sparc/translate.c
@@ -2633,7 +2633,7 @@ static void disas_sparc_insn(DisasContext * dc, unsigned int insn)
case 2: /* FPU & Logical Operations */
{
unsigned int xop = GET_FIELD(insn, 7, 12);
- TCGv cpu_dst = gen_dest_gpr(dc, rd);
+ TCGv cpu_dst = get_temp_tl(dc);
TCGv cpu_tmp0;
if (xop == 0x3a) { /* generate trap */
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH] Revert "target-sparc: Make cpu_dst local to OP=2 insns"
2012-10-21 21:29 ` Richard Henderson
@ 2012-10-21 22:25 ` Aurelien Jarno
2012-10-22 22:15 ` Aurelien Jarno
0 siblings, 1 reply; 6+ messages in thread
From: Aurelien Jarno @ 2012-10-21 22:25 UTC (permalink / raw)
To: Richard Henderson; +Cc: Blue Swirl, qemu-devel
On Mon, Oct 22, 2012 at 07:29:19AM +1000, Richard Henderson wrote:
> On 2012-10-21 09:17, Aurelien Jarno wrote:
> > On Sun, Oct 21, 2012 at 08:48:52AM +1000, Richard Henderson wrote:
> >> On 2012-10-21 00:48, Aurelien Jarno wrote:
> >>> I am not sure it is the real problem, but at least the optimization of
> >>> using the destination register as a temporary is wrong when the
> >>> instruction might trigger an exception. In that case the result is
> >>> written to the destination register while it should have not.
> >>>
> >>> This reverts commit 5793f2a47e201d251856c7956d6f7907ec0d9f1f.
> >>
> >> Which insn might trigger an exception? Most OP=2 insns don't. There's
> >> divide, but that's done out-of-line, so the assignment to dst does not
> >> happen before the exception...
> >
> > Indeed there a are a few one triggering exception, but I looked too
> > quickly and indeed they do the assignment before. There should be
> > another problem elsewhere as reverting this patch fixes the issue.
> >
> >> Is this sparc64? I assume so, since I did test sparc32...
> >>
> >
> > Yes it's with a sparc64 kernel. I can reproduce the problem with both a
> > 32 and 64-bit userland, though it happens earlier with a 32-bit
> > userland.
> >
>
> Sadly, I don't seem to be able to boot a "real" sparc64 system at all,
> even with a qemu version before the patch set in question went in.
>
> (1) The recent "vga cleanup" has totally broken video for all sparc.
> This is bisectable to f2898771435701df33145cacabeb42c6aa3a9a16,
> your patch that adjusts sun4u. One can see this problem just by
> booting OpenBIOS; no kernel needed.
Oops, it's something I haven't realized, because my scripts are either
passing -vga none or -vga std. I'll try to come with a patch, but in the
meantime the problem can be workarounded by passing -vga std.
> (2) Switching to serial console, the kernel gets stuck talking to the
> disks. It gets so far, and then reports lost interrupts forever:
>
> $ ../bld/sparc64-softmmu/qemu-system-sparc64 -kernel sparc64 -initrd initrd.gz -append "root=/dev/ram console=ttyS0" -hda ./hda.img -cdrom ./debian-6.0.6-sparc-netinst.iso -nographic
> ...
> [ 14.488838] ata1.00: ATA-7: QEMU HARDDISK, 1.2.50, max UDMA/100
> [ 14.489893] ata1.00: 25165824 sectors, multi 16: LBA48
> [ 14.504620] ata1.00: configured for UDMA/33
> [ 14.549962] scsi 0:0:0:0: Direct-Access ATA QEMU HARDDISK 1.2. PQ: 0 ANSI: 5
> [ 14.732381] ata2.00: ATAPI: QEMU DVD-ROM, 1.2.50, max UDMA/100
> [ 14.734341] ata2.00: configured for UDMA/33
> [ 35.596592] ata2: lost interrupt (Status 0x50)
> [ 56.594942] ata2: lost interrupt (Status 0x50)
Yes, the emulated IDE controller is currently buggy and causes the
kernel to go in a dead loop. Instead of using Debian Squeeze, I am using
Debian Wheezy which provides virtio support. Then the system can boot
with virtio-disk and virtio-net.
> I re-examined the opcodes in OP=2. The only one left that throws an inline exception
> is Tcc, which of course doesn't write to a destination register at all. If, somehow,
> there is some exception (or other write-before-use) that I've missed, then the following
> patch should fix it, without moving the cpu_dst variable back to global scope. If this
> patch does not fix the problem, then there's some erroneous use of cpu_dst that I've
> missed, and the original patch is question is broken for some other reason.
The attached patch fixes the problem.
> Given my experience above, I wonder what you're testing differently? I'm trying the
> stock standard debian 6.0.6 (current?) distribution media images. The kernel/initrd
> images above are pulled out of that iso, as I couldn't make OpenBIOS boot the cdrom
> image directly.
I'll try to make an image so that people can test sparc64 more easily.
--
Aurelien Jarno GPG: 1024D/F1BCDB73
aurelien@aurel32.net http://www.aurel32.net
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] Revert "target-sparc: Make cpu_dst local to OP=2 insns"
2012-10-21 22:25 ` Aurelien Jarno
@ 2012-10-22 22:15 ` Aurelien Jarno
0 siblings, 0 replies; 6+ messages in thread
From: Aurelien Jarno @ 2012-10-22 22:15 UTC (permalink / raw)
To: Richard Henderson; +Cc: Blue Swirl, qemu-devel
On Mon, Oct 22, 2012 at 12:25:23AM +0200, Aurelien Jarno wrote:
> On Mon, Oct 22, 2012 at 07:29:19AM +1000, Richard Henderson wrote:
> > Given my experience above, I wonder what you're testing differently? I'm trying the
> > stock standard debian 6.0.6 (current?) distribution media images. The kernel/initrd
> > images above are pulled out of that iso, as I couldn't make OpenBIOS boot the cdrom
> > image directly.
>
>
> I'll try to make an image so that people can test sparc64 more easily.
>
You can find a Debian Wheezy image and the script to run it on:
http://temp.aurel32.net/qemu-sparc/
--
Aurelien Jarno GPG: 1024D/F1BCDB73
aurelien@aurel32.net http://www.aurel32.net
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-10-22 22:15 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-10-20 14:48 [Qemu-devel] [PATCH] Revert "target-sparc: Make cpu_dst local to OP=2 insns" Aurelien Jarno
2012-10-20 22:48 ` Richard Henderson
2012-10-20 23:17 ` Aurelien Jarno
2012-10-21 21:29 ` Richard Henderson
2012-10-21 22:25 ` Aurelien Jarno
2012-10-22 22:15 ` Aurelien Jarno
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).