ppc: Stop dumping state on all exceptions in linux-user

PATCH Superseded Benjamin Herrenschmidt 3 participants 8 messages Aug 3, 2016 – Aug 8, 2016

Share & tools

Benjamin Herrenschmidt

author

2016-08-03 08:02 UTC #0 lore raw

PATCH 1/1 ppc: Stop dumping state on all exceptions in linux-user

Other archs don't do it, some programs catch signals just fine and those dumps just clutter the output. Keep the dumps for cases that aren't supposed to happen such as unknown codes.

Signed-off-by Benjamin Herrenschmidt <[email protected]>

---
 linux-user/main.c | 14 --------------
 1 file changed, 14 deletions(-)

linux-user/main.c +0 ~~-14~~

index eb9975c..8fbc5a6 100644
@@ -1651,9 +1651,6 @@ void cpu_loop(CPUPPCState *env)
                       "Aborting\n");
             break;
         case POWERPC_EXCP_DSI:      /* Data storage exception                */
-            EXCP_DUMP(env, "Invalid data memory access: 0x" TARGET_FMT_lx "\n",
-                      env->spr[SPR_DAR]);
-            /* XXX: check this. Seems bugged */
             switch (env->error_code & 0xFF000000) {
             case 0x40000000:
             case 0x42000000:

@@ -1684,9 +1681,6 @@ void cpu_loop(CPUPPCState *env)
             queue_signal(env, info.si_signo, &info);
             break;
         case POWERPC_EXCP_ISI:      /* Instruction storage exception         */
-            EXCP_DUMP(env, "Invalid instruction fetch: 0x\n" TARGET_FMT_lx
-                      "\n", env->spr[SPR_SRR0]);
-            /* XXX: check this */
             switch (env->error_code & 0xFF000000) {
             case 0x40000000:
                 info.si_signo = TARGET_SIGSEGV;

@@ -1716,7 +1710,6 @@ void cpu_loop(CPUPPCState *env)
                       "Aborting\n");
             break;
         case POWERPC_EXCP_ALIGN:    /* Alignment exception                   */
-            EXCP_DUMP(env, "Unaligned memory access\n");
             /* XXX: check this */
             info.si_signo = TARGET_SIGBUS;
             info.si_errno = 0;

@@ -1729,7 +1722,6 @@ void cpu_loop(CPUPPCState *env)
             /* XXX: check this */
             switch (env->error_code & ~0xF) {
             case POWERPC_EXCP_FP:
-                EXCP_DUMP(env, "Floating point program exception\n");
                 info.si_signo = TARGET_SIGFPE;
                 info.si_errno = 0;
                 switch (env->error_code & 0xF) {

@@ -1765,7 +1757,6 @@ void cpu_loop(CPUPPCState *env)
                 }
                 break;
             case POWERPC_EXCP_INVAL:
-                EXCP_DUMP(env, "Invalid instruction\n");
                 info.si_signo = TARGET_SIGILL;
                 info.si_errno = 0;
                 switch (env->error_code & 0xF) {

@@ -1789,7 +1780,6 @@ void cpu_loop(CPUPPCState *env)
                 }
                 break;
             case POWERPC_EXCP_PRIV:
-                EXCP_DUMP(env, "Privilege violation\n");
                 info.si_signo = TARGET_SIGILL;
                 info.si_errno = 0;
                 switch (env->error_code & 0xF) {

@@ -1819,7 +1809,6 @@ void cpu_loop(CPUPPCState *env)
             queue_signal(env, info.si_signo, &info);
             break;
         case POWERPC_EXCP_FPU:      /* Floating-point unavailable exception  */
-            EXCP_DUMP(env, "No floating point allowed\n");
             info.si_signo = TARGET_SIGILL;
             info.si_errno = 0;
             info.si_code = TARGET_ILL_COPROC;

@@ -1831,7 +1820,6 @@ void cpu_loop(CPUPPCState *env)
                       "Aborting\n");
             break;
         case POWERPC_EXCP_APU:      /* Auxiliary processor unavailable       */
-            EXCP_DUMP(env, "No APU instruction allowed\n");
             info.si_signo = TARGET_SIGILL;
             info.si_errno = 0;
             info.si_code = TARGET_ILL_COPROC;

@@ -1859,7 +1847,6 @@ void cpu_loop(CPUPPCState *env)
                       "Aborting\n");
             break;
         case POWERPC_EXCP_SPEU:     /* SPE/embedded floating-point unavail.  */
-            EXCP_DUMP(env, "No SPE/floating-point instruction allowed\n");
             info.si_signo = TARGET_SIGILL;
             info.si_errno = 0;
             info.si_code = TARGET_ILL_COPROC;

@@ -1923,7 +1910,6 @@ void cpu_loop(CPUPPCState *env)
                       "while in user mode. Aborting\n");
             break;
         case POWERPC_EXCP_VPU:      /* Vector unavailable exception          */
-            EXCP_DUMP(env, "No Altivec instructions allowed\n");
             info.si_signo = TARGET_SIGILL;
             info.si_errno = 0;
             info.si_code = TARGET_ILL_COPROC;

Benjamin Herrenschmidt

author

2016-08-03 11:29 UTC #2 lore raw

Quoted 12 lines · Peter Maydell

On 3 August 2016 at 09:02, Benjamin Herrenschmidt
Quoted 11 lines
<[email protected]> wrote:
Other archs don't do it, some programs catch signals just fine and those dumps just clutter the output. Keep the dumps for cases that aren't supposed to happen such as unknown codes.
Quoted 1 line
Quoted 1 line
Signed-off-by Benjamin Herrenschmidt <[email protected]>
---
linux-user/main.c | 14 --------------
 1 file changed, 14 deletions(-)

Quoted 15 lines

Quoted 11 lines
linux-user/main.c +0 -0
index eb9975c..8fbc5a6 100644
@@ -1651,9 +1651,6 @@ void cpu_loop(CPUPPCState *env)
                        "Aborting\n");
              break;
case POWERPC_EXCP_DSI: /* Data storage exception */ - EXCP_DUMP(env, "Invalid data memory access: 0x" TARGET_FMT_lx "\n", - env->spr[SPR_DAR]);
- /* XXX: check this. Seems bugged */
Are you removing these XXX comments because you've checked the error code cases below? If so it would be useful to say so in the commit message...

Well I fix some of it in another patch...

To the best of my understanding we only ever generate 0x40000000 and 0x42000000 via ppc_cpu_handle_mmu_fault() in user_only_helper.c unless there's another path to DSI in user mode that I missed.

That being said, the comment isn't useful anyway. Nobody looks at it :-)

I can respin with the comment back in or do put some blurb in the cset comment, let me know.

BTW. I noticed, the generic path from the original signal coming from the host kernel through to here loses the information that would allow us to differenciate between a map error (no mapping) or a protection fault.

It's a bit unfortunate, not sure if we can do much about it though without stopping to hijack the existing MMU translate hook in the CPU class and using a user mode dediated one that takes that sort of info as an argument.

Cheers,
Ben.

Peter Maydell

2016-08-03 11:32 UTC #3 lore raw

Quoted 7 lines · Benjamin Herrenschmidt

To the best of my understanding we only ever generate 0x40000000 and 0x42000000 via ppc_cpu_handle_mmu_fault() in user_only_helper.c unless there's another path to DSI in user mode that I missed.
That being said, the comment isn't useful anyway. Nobody looks at it :-)

Well, it means when somebody does come along to clean up the code or just to read it they have some idea of whether the code is believed-to-be-good or not.

Quoted 2 lines

I can respin with the comment back in or do put some blurb in the cset comment, let me know.

I think if you're going to remove the comment you should specifically say you're doing it because you believe the code is actually correct.

Quoted 9 lines

BTW. I noticed, the generic path from the original signal coming from the host kernel through to here loses the information that would allow us to differenciate between a map error (no mapping) or a protection fault.
It's a bit unfortunate, not sure if we can do much about it though without stopping to hijack the existing MMU translate hook in the CPU class and using a user mode dediated one that takes that sort of info as an argument.

Yeah, there's an LTP test that fails because of this (we send a SIGSEGV when we should be sending a SIGBUS). It's a bit painful to fix though, since as you say we've effectively thrown away some information. I'm inclined to put this in the big pile of "bugs we could fix if we ever implemented linux-user-with-softmmu" and otherwise ignore it, unless you have a real-world program that needs this and makes some kind of bodge fix worthwhile.

thanks
-- PMM

Benjamin Herrenschmidt

author

2016-08-03 11:40 UTC #4 lore raw

Quoted 8 lines · Peter Maydell

Yeah, there's an LTP test that fails because of this (we send a SIGSEGV when we should be sending a SIGBUS). It's a bit painful to fix though, since as you say we've effectively thrown away some information. I'm inclined to put this in the big pile of "bugs we could fix if we ever implemented linux-user-with-softmmu" and otherwise ignore it, unless you have a real-world program that needs this and makes some kind of bodge fix worthwhile.

Nope, not really...

If we care, a simpler fix would be to add a "translate_user_fault" hook to the CPU class that takes more info about the original signal than handle_mmu_fault does, and call it when available (with a fallback) from user-exec.c

That does mean going through all the cpu_signal_handler() variants in there though to make them extract more useful info.

Not sure it's worthwhile...

As far user-with-softmmu, I'm not too sure... softmmu significantly increases the overhead of load and stores. Maybe after we add 128-bit integers to TGC to alleviate that a bit ? :-)

Cheers,
Ben
.

Richard Henderson

2016-08-06 09:53 UTC #5 lore raw

Quoted 3 lines · Benjamin Herrenschmidt

As far user-with-softmmu, I'm not too sure... softmmu significantly increases the overhead of load and stores. Maybe after we add 128-bit integers to TGC to alleviate that a bit ? :-)

It wouldn't be mandatory, but there are certain bugs we can't fix without it. The big issues to be fixed with softmmu are

(1) Host page size > guest page size.

E.g. there are many programs (i386, sparc, etc, all with 4k pages) that you can't even load, much less run, on a ppc64 host using a 64k page size.

(2) Host virtual address space bits != guest virtual address space bits

My alpha emulation has run into this. A real Alpha guest has a 44-bit address space, but an x86_64 host has a 48-bit address space. The x86_64 kernel cannot be persuaded to reliably map memory below (1ul << 44), so I have to pretend than Alpha has a 48-bit address space. (Indeed, I set this to 63 bits, so that it works for even wider va, like on ppc64 and sparc64.)

More theoretically, if the guest uses high bits for some purpose (e.g. ia64 segmentation in the top 3 bits), and the host doesn't have a full 64-bit virtual address space, then we cannot even map the program, since we cannot set bits 61-63 to non-zero values.

Benjamin Herrenschmidt

author

2016-08-07 00:50 UTC #6 lore raw

Quoted 6 lines · Richard Henderson

Quoted 4 lines · Benjamin Herrenschmidt
As far user-with-softmmu, I'm not too sure... softmmu significantly increases the overhead of load and stores. Maybe after we add 128-bit integers to TGC to alleviate that a bit ? :-)

Quoted 7 lines

It wouldn't be mandatory, but there are certain bugs we can't fix without it. The big issues to be fixed with softmmu are
(1) Host page size > guest page size.
E.g. there are many programs (i386, sparc, etc, all with 4k pages) that you can't even load, much less run, on a ppc64 host using a 64k page size.

Can't we advertise the host page size to the guest ? Or there are too many compiled-in assumptions ?

Quoted 7 lines

Quoted 1 line
(2) Host virtual address space bits != guest virtual address space bits
My alpha emulation has run into this. A real Alpha guest has a 44-bit address space, but an x86_64 host has a 48-bit address space. The x86_64 kernel cannot be persuaded to reliably map memory below (1ul << 44), so I have to pretend than Alpha has a 48-bit address space. (Indeed, I set this to 63 bits, so that it works for even wider va, like on ppc64 and sparc64.)

You can't just set a no-access VMA covering the top of the address space ? Are alpha programs relying on the fact that they won't get addresses above 44 ?

Quoted 4 lines

Quoted 1 line
More theoretically, if the guest uses high bits for some purpose (e.g. ia64
segmentation in the top 3 bits), and the host doesn't have a full 64-bit virtual address space, then we cannot even map the program, since we cannot set bits 61-63 to non-zero values.

I see. We could definitely have the option then.

Cheers,
Ben.

Richard Henderson

2016-08-08 06:59 UTC #7 lore raw

Quoted 10 lines · Benjamin Herrenschmidt

Quoted 9 lines · Richard Henderson
Quoted 4 lines · Benjamin Herrenschmidt
As far user-with-softmmu, I'm not too sure... softmmu significantly increases the overhead of load and stores. Maybe after we add 128-bit integers to TGC to alleviate that a bit ? :-)
It wouldn't be mandatory, but there are certain bugs we can't fix without it. The big issues to be fixed with softmmu are

Quoted 7 lines

Quoted 4 lines
(1) Host page size > guest page size.
E.g. there are many programs (i386, sparc, etc, all with 4k pages) that you can't even load, much less run, on a ppc64 host using a 64k page size.
Can't we advertise the host page size to the guest ? Or there are too many compiled-in assumptions ?

Sometimes. However:

Each guest has a set of page sizes that are legal. The largest of these is baked into the static linker via MAXPAGESIZE. If the host page size is larger than this guest maximum, the layout of the guest binary may be impossible to satisfy on the host. There are examples of this even within linux-user-test-0.3.

For those guests whose set of page sizes contains only one size, the page size is often baked into executables via the constant PAGE_SIZE, rather than performing a runtime query on getpagesize(2).