[Qemu-devel] PATCH: allow Sparc hosts to run arm/mips/sparc-softmmu

[Qemu-devel] PATCH: allow Sparc hosts to run arm/mips/sparc-softmmu

[Ben Taylor]

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


This patch fixes crashes when testing with arm-test-0.2.tar.gz and mips-test-0.2.tar.gz.
Without the patch, both arm-test and mips-test segfault when trying to boot.

The original patch was authored by Martin Bochnig.


[-- Attachment #2: qemu-sparc-cpuexec-c.diff --]
[-- Type: text/x-patch, Size: 570 bytes --]

--- qemu/cpu-exec.c.ORIG	2007-03-13 09:46:51.940624000 -0400
+++ qemu/cpu-exec.c	2007-03-13 09:33:34.130534000 -0400
@@ -738,7 +744,10 @@
 #else
                 gen_func();
 #endif
+/* sparc hosts don't seem to like this method very much */
+#if !(defined(__sparc__) && !defined(TARGET_I386) && !defined(TARGET_X86_64) && !defined(TARGET_PPC))
                 env->current_tb = NULL;
+#endif
                 /* reset soft MMU for next block (it can currently
                    only be set by a memory fault) */
 #if defined(TARGET_I386) && !defined(CONFIG_SOFTMMU)

Re: [Qemu-devel] PATCH: allow Sparc hosts to run arm/mips/sparc-softmmu

[Paul Brook]

On Tuesday 13 March 2007 14:01, Ben Taylor wrote:
> This patch fixes crashes when testing with arm-test-0.2.tar.gz and
> mips-test-0.2.tar.gz. Without the patch, both arm-test and mips-test
> segfault when trying to boot.

I don't believe this is correct. You're going to have to come up with a fairly 
convincing argument to get a patch like this accepted.
"don't seem to like this very much" is not a useful comment.

Paul

Re: [Qemu-devel] PATCH: allow Sparc hosts to run arm/mips/sparc-softmmu

[Ben Taylor]

---- Paul Brook <paul@codesourcery.com> wrote: 
> On Tuesday 13 March 2007 14:01, Ben Taylor wrote:
> > This patch fixes crashes when testing with arm-test-0.2.tar.gz and
> > mips-test-0.2.tar.gz. Without the patch, both arm-test and mips-test
> > segfault when trying to boot.
> 
> I don't believe this is correct. You're going to have to come up with a fairly 
> convincing argument to get a patch like this accepted.
> "don't seem to like this very much" is not a useful comment.

Yeah, so the comment is not very useful.  I copied it from the author.
I'll see if he can actually explain why this patch is important.

However, it's very wax-on, wax-off kind of thing.  Without the patch, arm-test
and mips-test crash.  With the patch, I can run both tests.

Ben

Re: [Qemu-devel] PATCH: allow Sparc hosts to run arm/mips/sparc-softmmu

[Paul Brook]

> However, it's very wax-on, wax-off kind of thing.  Without the patch,
> arm-test and mips-test crash.  With the patch, I can run both tests.

As I've said before it's not sufficient to say that a patch fixes a bug, you 
have to explain *what* bug you are fixing, *how* it fixes the bug, and *why* 
it's the correct way to fix it. In order to review the patch I need to be 
able to follow your logic for creating the patch. If you don't understand the 
patch you should not be submitting it.

In this specific case:

* What: "qemu crashes" is not a particularly useful description of the failure 
mode. I want to know how (eg. segfault, abort, infinite loop, does wrong 
thing) it crashes, which bit of code it's executing when it crashes, and how 
it got to that point.

* How: I'm also not convinced your #ifdef does what you think it does, though 
It's somewhat unclear what you're trying to achieve. I'm guessing you 
intended to disable the code on sparc hosted arm+mips targets 

* Why: "I randomly changed things until it started working" is not a valid 
justification for a change. Why doesn't this failure occur on other hosts? 
Why only arm and mips targets?

Paul

Re: [Qemu-devel] PATCH: allow Sparc hosts to run arm/mips/sparc-softmmu

[Rob Landley]

On Tuesday 13 March 2007 10:25 am, Ben Taylor wrote:
> However, it's very wax-on, wax-off kind of thing.  Without the patch,
> arm-test and mips-test crash.  With the patch, I can run both tests.

Could we get a reproduction sequence for the crashes please?

Rob
-- 
Vista: Windows Millenium Second Edition

Re: [Qemu-devel] PATCH: allow Sparc hosts to run arm/mips/sparc-softmmu

[Ben Taylor]

---- Rob Landley <rob@landley.net> wrote: 
> On Tuesday 13 March 2007 10:25 am, Ben Taylor wrote:
> > However, it's very wax-on, wax-off kind of thing.  Without the patch,
> > arm-test and mips-test crash.  With the patch, I can run both tests.
> 
> Could we get a reproduction sequence for the crashes please?

Running 0.9.0-CVS plus a few patches, on a Solaris 10/FCS 
heavily patched host, compiled with blastwave gcc-3.4.3.

When I run the arm-test as suggested by the readme, qemu
core dumps with a SIGSEGV at the line that was patched.  I agree
with Paul that is probalby not the right fix.  It does just happen to
fix it on both mips-test and arm-test.

Uncommmenting the define for DEBUG_EXEC in cpu-exec.c 
and recompiling, and starting the arm-test with "-vnc :0 -S", 
connecting with VNC, going to the qemu monitor and enabling 
all logging and hitting continue, the qemu again dumps core, 
having processed exactly one translation block.   With the patch,
and started exactly the same way, the first TB log looks exactly
the same, but continues without "crashing".

If I put a fprintf(logfile...) above the line, it works, no crash.

I am not very good at debugging as it is, and I'm pretty sure that there's
some sort of left over from the TB that is causing the problem.  I'm
suspecting maybe a page boundry, just from having stepi'd all the
instructions in the TB before it SEGV's on that line.  The last address
looked suspciously like a page boundry.  However, that's just an
uneducated SWAG at what the problem is.

Does that help?

Ben