Aurelien Jarno wrote:
> On Thu, May 08, 2008 at 11:33:42PM -0500, Jason Wessel wrote:
>> This patch also fixes the defect where branches were incorrectly
>> calculated for the update to the env->nip when setting MSR_SE on
>> a branch instruction.
>
> Please find my comments inside.
>
>> ---
>>  target-ppc/translate.c |   41 ++++++++++++++++++++++++++++++-----------
>>  1 file changed, 30 insertions(+), 11 deletions(-)
>>
[clip]
>> @@ -2803,8 +2806,17 @@ static always_inline void gen_goto_tb (D
>>          else
>>  #endif
>>              gen_op_b_T1();
>> -        if (ctx->singlestep_enabled)
>> -            gen_op_debug();
>> +       if (unlikely(ctx->singlestep_enabled)) {
>> +           if (ctx->singlestep_enabled & GDBSTUB_SINGLE_STEP) {
>> +               gen_update_nip(ctx, dest);
>> +               gen_op_debug();
>> +           } else {
>> +               target_ulong tmp = ctx->nip;
>> +               ctx->nip = dest;
>> +               GEN_EXCP(ctx, POWERPC_EXCP_TRACE, 0);
>> +               ctx->nip = tmp;
>
> What's the point of generating POWERPC_EXCP_TRACE in gen_goto_tb()?
> ctx->singlestep_enabled == CPU_SINGLE_STEP corresponds to MSR_SE, which
> does not generate trace exception for branch instructions.
>
> MSR_BE does generate trace exception for branch instructions, but that's
> already handled in gen_intermediate_code_internal() by testing
> branch_step.

But in this case that is not the way the hardware works.  The linux
kernel is only setting up the MSR_SE bit to step the instructions in
user space for the PPC 6xx architecture.

The 604 processor manual indicates (as well as when I tested it on
real hardware) that the MSR_SE should generate a trace exception upon
the completion of any instruction except for the "special cases"
(see below).

You have certainly pointed out a different problem however.  I tested
the MSR_BE functionality and it does not work at all for the same
reason that the MSR_SE did not work with branches.  The exception is
generated in the wrong place, and the result is that the simulation
stops at the wrong instruction when MSR_BE is set.  To address this,
I created a new patch which is attached.  The new patch also fixes the
case for using the qemu backend debugger and the MSR_SE or MSR_BE on
the simulated cpu at the same time.

The reference here for MSR_SE the single stepping is from:

http://www-01.ibm.com/chips/techlib/techlib.nsf/techdocs/852569B20050FF7785256996006E34F3/$file/604eUM_book.pdf

--- from manual page 4-8 ---

The processor generates a single-step trace exception upon the
successful execution of the next instruction (unless that instruction
is an rfi instruction). Successful execution means that the
instruction caused no other exception.

-------

And then there is the general 32bit ppc arch manual.

http://www.freescale.com/files/product/doc/MPCFPE32B.pdf

In there it states that the MSR_SE is an optional feature  and that
you should see the specific ppc part documentation, but it
does further describe:

--- from manual page 6-29 ---

If this facility is implemented, a trace interrupt occurs when no
higher priority interrupt exists and either of the conditions
described above exist. The following are not traced:

     * rfi instruction
     * sc and trap instructions that trap
     * Other instructions that cause interrupts (other than trace
interrupts)
     * The first instruction of any interrupt handler
     * Instructions that are emulated by software

MSR[SE,BE] are both cleared when the trace interrupt is taken. In the
normal use of this function,

MSR[SE,BE] are restored when the interrupt handler returns to the
interrupted program using an rfi instruction.

------

At least with these changes you can now more accurately debug a user
space application with gdb running inside linux on the qemu-system-ppc
as well as freely use the qemu backend debugger with single stepping.

Jason.

ps

The "return;" just after the "out:"  is there to eliminate some compiler
warnings.  This could be fixed by further changing all the "goto out;"
statements with return;  I had to move the ctx->exception type update
prior to any calls to gen_goto_tb(), so the single step logic would
apply correctly only in the branch case and not for any other kind of
exception.  I figured I'd limit the change scope to make the patch as
readable as possible.