Re: [Qemu-devel] RFC: Why does target/m68k RTE insn. use gen_exception
>On Monday, July 1, 2019, 06:10:55 PM GMT+9, Peter Maydell wrote: > > On Sat, 29 Jun 2019 at 17:37, Lucien Murray-Pitts> > wrote: > > However for the m68k the do_transaction_failed function pointer field > > has not been implemented.>Er, I implemented that in commit > > e1aaf3a88e95ab007. Are >you working with an out-of-date version of QEMU ? Sorry not pulled in a long time, you are right that is there now - I dont generally check the development list outside of replies, and was focused on the stepping issue - I will be more careful of that in future. Thanks for the heads up. Further to my initial problem I noticed that TRAP #0 also jumps to the handlers +1 instruction. Same behavior can also be seen with ARM "SWI #0". (PC shows 0x0C vs the expected 0x08) Putting a "BRA $" / "B $" so that it loops on the first address of the handler results in the step stopping, of course, at the expected "first instruction" of the vector handler. So it would seem this maybe a wider problem than just the m68K.Since I dont really understand the TCG complete execution method, and how it fits in with the GNU RSP "s" step command I am going to take some time to work this out. I appreciate any hints people can provide, but I dont mind plugging away - I am learning and surprising myself how much there is to this. Cheers,Luc
Re: [Qemu-devel] RFC: Why does target/m68k RTE insn. use gen_exception
Hi Laurent / Richard, (resent email ) Does anyone have any knowledge why gen_exception(s, s->base.pc_next, EXCP_RTE); is generated for "RTE" instruction, where as the "RTS" goes a gen_jmp?( note see target/m68k/translate.c in functions DISAS_INSN(rte) and DISAS_INSN(rts) Cheers,Luc
[Qemu-devel] RFC: Why does target/m68k RTE insn. use gen_exception
Hi folks, Does anyone have any knowledge why gen_exception(s, s->base.pc_next, EXCP_RTE);
Re: [Qemu-devel] [PATCH] Regression for m68k causing Single-Step via GDB/RSP to not single step
Hi Richard/Laurent,
Great catch, I also just stumbled on this problem as well which I didnt see
with my other application code.
But I have another problem after applying the changes from your email, "RTE"
and breakpoints around a MOV/BusException/RTE behave oddly.
I would like to test with the same software you are using, could you tell me
what M68K machine setup, and images you use as well as your debugger please?
Cheers,Luc
On Wednesday, June 19, 2019, 04:59:12 AM GMT+9, Laurent Vivier
wrote:
Le 18/06/2019 à 21:39, Richard Henderson a écrit :
> On 6/18/19 11:44 AM, Laurent Vivier wrote:
>> Le 26/05/2019 à 09:50, Lucien Murray-Pitts a écrit :
>>> A regression that was introduced, with the refactor to TranslatorOps,
>>> drops two lines that update the PC when single-stepping is being performed.
>>> ( short commit 11ab74b )
>>>
>>> This patch resolves that issue.
>>
>> Fixes: 11ab74b01e0a ("target/m68k: Convert to TranslatorOps")
>>
>>> Signed-off-by: Lucien Murray-Pitts
>>> ---
>>> target/m68k/translate.c | 2 ++
>>> 1 file changed, 2 insertions(+)
>>>
>>> diff --git a/target/m68k/translate.c b/target/m68k/translate.c
>>> index f0534a4ba0..2922ea79c3 100644
>>> --- a/target/m68k/translate.c
>>> +++ b/target/m68k/translate.c
>>> @@ -6130,6 +6130,8 @@ static void m68k_tr_tb_stop(DisasContextBase *dcbase,
>>> CPUState *cpu)
>>> return;
>>> }
>>> if (dc->base.singlestep_enabled) {
>>> + update_cc_op(dc);
>>> + tcg_gen_movi_i32(QREG_PC, dc->pc);
>>> gen_helper_raise_exception(cpu_env, tcg_const_i32(EXCP_DEBUG));
>>> return;
>>> }
>>>
>>
>> I've tested this fix single-stepping on a kernel, these two lines are
>> not enough to fix the problem. In fact four lines have been dropped and
>> we must re-add them all:
>>
>> iff --git a/target/m68k/translate.c b/target/m68k/translate.c
>> index d0f6d1f5cc..6c78001501 100644
>> --- a/target/m68k/translate.c
>> +++ b/target/m68k/translate.c
>> @@ -6200,6 +6200,10 @@ static void m68k_tr_tb_stop(DisasContextBase *dcbase,
>> CPUState *cpu)
>> return;
>> }
>> if (dc->base.singlestep_enabled) {
>> + if (dc->base.is_jmp != DISAS_JUMP) {
>> + update_cc_op(dc);
>> + tcg_gen_movi_i32(QREG_PC, dc->pc);
>> + }
>> gen_helper_raise_exception(cpu_env, tcg_const_i32(EXCP_DEBUG));
>> return;
>> }
>
> Even this isn't quite right, according to the comments in the switch that
> follows. I think it'd be best written like so.
>
>
> r~
>
>
> diff --git a/target/m68k/translate.c b/target/m68k/translate.c
> index 2ae537461f..b61c7ea0f1 100644
> --- a/target/m68k/translate.c
> +++ b/target/m68k/translate.c
> @@ -6124,27 +6124,34 @@ static void m68k_tr_tb_stop(DisasContextBase *dcbase,
> CPUState *cpu)
> {
> DisasContext *dc = container_of(dcbase, DisasContext, base);
>
> - if (dc->base.is_jmp == DISAS_NORETURN) {
> - return;
> - }
> - if (dc->base.singlestep_enabled) {
> - gen_helper_raise_exception(cpu_env, tcg_const_i32(EXCP_DEBUG));
> - return;
> - }
> -
> switch (dc->base.is_jmp) {
> + case DISAS_NORETURN:
> + break;
> case DISAS_TOO_MANY:
> update_cc_op(dc);
> - gen_jmp_tb(dc, 0, dc->pc);
> + if (dc->base.singlestep_enabled) {
> + tcg_gen_movi_i32(QREG_PC, dc->pc);
> + gen_helper_raise_exception(cpu_env, tcg_const_i32(EXCP_DEBUG));
> + } else {
> + gen_jmp_tb(dc, 0, dc->pc);
> + }
> break;
> case DISAS_JUMP:
> /* We updated CC_OP and PC in gen_jmp/gen_jmp_im. */
> - tcg_gen_lookup_and_goto_ptr();
> + if (dc->base.singlestep_enabled) {
> + gen_helper_raise_exception(cpu_env, tcg_const_i32(EXCP_DEBUG));
> + } else {
> + tcg_gen_lookup_and_goto_ptr();
> + }
> break;
> case DISAS_EXIT:
> /* We updated CC_OP and PC in gen_exit_tb, but also modified
> other state that may require returning to the main loop. */
> - tcg_gen_exit_tb(NULL, 0);
> + if (dc->base.singlestep_enabled) {
> + gen_helper_raise_exception(cpu_env, tcg_const_i32(EXCP_DEBUG));
> + } else {
> + tcg_gen_exit_tb(NULL, 0);
> + }
> break;
> default:
> g_assert_not_reached();
>
Yes, it works too.
Could you formally send a patch?
Thanks,
Laurent
Re: [Qemu-devel] [PATCH] Incorrect Stack Pointer shadow register support on some m68k CPUs
> On Sunday, May 26, 2019, 10:10:39 PM GMT+9, Laurent Vivier wrote: > On 26/05/2019 09:28, Lucien Murray-Pitts wrote: >> On CPU32 and the early 68000 and 68010 the ISP doesnt exist. >> These CPUs only have SSP/USP. >> [SNIP] >> The movec instruction when accessing these shadow registers >> in some configurations should issue a TRAP. This patch does not >> add this funcitonality to the helpers. >> >I think it's better to also update movec in the same patch.[LMP] Movec should >be undefined (coldfire manual) for registers it doesnt know. The MC680X0 >manual is less clear.Technically this could be just leaving the operation of >the instruction alone and allowing it to pass back MSP/ISP/USP as it currently >does. My thinking is this is less likely to break anything So I will only add a comment, to state that on 68000/10/CF/CPU32 it should be "undefined" >Could you also update the comment about sp in CPUM68KState structure definition? [LMP] Done >And, if possible, could you fix style problem reported by patchew. [LMP] Done
[Qemu-devel] Failure to submit patches, two questions - what should I do?
> On Sunday, May 26, 2019, 4:45:26 PM GMT+9, wrote: > Subject; [Qemu-devel] [PATCH] Incorrect Stack Pointer shadow register support on some m68k CPUs > .> snip> .> === OUTPUT BEGIN === > ERROR: Author email address is mangled by the mailing list > #2: > Author: Lucien Murray-Pitts via Qemu-devel > > WARNING: Block comments use a leading /* on a separate line > #46: FILE: target/m68k/cpu.h:465: > +/* The ColdFire core ISA is a RISC-style reduction of the 68000 series > > WARNING: Block comments use * on subsequent lines > #47: FILE: target/m68k/cpu.h:466:> > +/* The ColdFire core ISA is a RISC-style reduction of the 68000 series > + Whilst the 68000 flourished by adding extended stack/instructions > in>.> snip Q1: Name mangling seems to be a bug, whats going on - how should I be submiting now? ( perl script didnt catch it AND there seems to already be a patch from half year or more ago .. https://patchwork.kernel.org/patch/10662525/ ) whats the correct action here? Q2: I am getting a WARNING but I believe it is an exception in this case. yes I know it breaks the coding style BUT this coding style was already there for these comments. Should I submit this patch with a move to the RIGHT coding style? or will this patch be accepted as the code is older style?
Re: [Qemu-devel] [PATCH v8 05/16] gdbstub: add multiprocess support to vCont packets
> On Friday, March 22, 2019, 11:01:34 PM GMT+9, Luc Michel wrote: > Lucien: do you plan to send a re-roll? Otherwise I'll do it on next > Monday (25/03) because I would like this bug to be fixed before it hits 4.0. I am on assignment in China which is why I havent gotten to it, I will attempt to get to it this Sunday. Cheers,Luc
Re: [Qemu-devel] m68k gdb has stopped single stepping correctly
Hi Thomas, > did you ever sent the patch? I can't find it on the mailing list, and I think >this bug is still pending?Yes, I have patches but was waiting for my first one >to be successful in order to make sure I got the process down. I will finish >that shortly. CheersLuc On 24/01/2019 14.38, Alex Bennée wrote: > > Lucien Anti-Spam via Qemu-devel writes: > >> > On Thursday, January 24, 2019, 3:08:07 AM GMT+9, Emilio G. Cota >> wrote: > > On Wed, Jan 23, 2019 at 15:58:27 +, Lucien >>Anti-Spam via Qemu-devel wrote:> > Hi folks, >>>> I noticed that with 3.x release that the GDB options (-S -s) for certain >>>> CPU results in very weird stepping.Usually stops afer a few steps, whilst >>>> the stub continues responding the PC doesnt update, however, I have only >>>> deeply looked at the m68k. >>>> In the case of the m68K the SR gets the trace bit set (T=10b), and the PC >>>> doesnt update.The m68k gdbstub, and main gdbstub seem mostly unchanged.But >>>> it seems the INSN handling has changed greatly for the m68k. >>>> Does anyone have any ideas what happened?>> Can you please bisect to find >>>> at which point things start misbehaving? >>> >>> Thanks, >>> Emilio >> Understood, I was hoping my original post might jog someone's memory about >> the issue. >> Apparently not, so after some digging I found that it was introduced with >> the refactor to TranslatorOps, specifically two lines got dropped that >> update the PC if single-stepping is being performed ( commit >> 11ab74b01e0a8ea4973eed89c6b90fa6e4fb9fb6 ) >> Since its not valid to revert, shall I go ahead and submit a patch for >> these two lines? > > Yes please! Hi Lucien, did you ever sent the patch? I can't find it on the mailing list, and I think this bug is still pending? Thomas
Re: [Qemu-devel] [PATCH] Fix for RSP vCont packet
Hi Luc, Great feedback from both you and Eric - im blown away, I dont thinktheres a single 1byte ascii character left unturned there :)My coding style snuck in erywhere, as did my Scottish English! On Friday, February 1, 2019, 10:33:18 PM GMT+9, Luc Michel wrote: > > + GDBThreadIdKind vcontThreadType ; > The coding style ... CODING_STYLE ... Ah I didnt pick up on this whilst reading the Wiki for coding style, thank you! > > + vcontThreadType = GDB_ALL_THREAD ;> + pid = 1 ; > The spec is not clear but I would opt for GDB_ALL_PROCESSES instead of > GDB_ALL_THREAD here. pid = 1; is clearly wrong since you don't know if > this PID exists or is currently attached. I was in a quandary here, and I did see your previous email too.However, I looked at how the call to read_thread_id() behaved and copied over the same behavior as if the command was vCont;c:-1which I understand to mean continue all threads? So maybe we should look at altering read_thread_id() which sets pid=1 when thereis no "ppid." part to the thread-id description, and if you set "tid=-1" then it performs a GDB_ALL_THREAD >From the vCont spec I also got;"p-1 ... means all processes""An action with no >thread-id matches all threads." "tid=-1 ... matches all threads" But it is not very clear what happens if you omit ppid and supply tid as -1. What do you think? Cheers, Luc
Re: [Qemu-devel] [PATCH] Fix for RSP vCont packet
On Thursday, January 31, 2019, 12:47:23 PM GMT+9, Eric Blake wrote: On 1/30/19 6:41 PM, Lucien Anti-Spam via Qemu-devel wrote: > > This fixes a regression in rsp packet vCont due to recently added > > multiprocess support. (Short commit hash: e40e520). > > > Your mailer completely botched the message (wrong line endings?) Can ... Sorry about that - none of my linux machines have smtp access and my workaround mangled it.I am trying to work out another way around that now.
[Qemu-devel] [PATCH] Fix for RSP vCont packet
This fixes a regression in rsp packet vCont due to recently added multiprocess
support. (Short commit hash: e40e520).
The result is that vCont now does not recognise the case where no
process/thread is provided after the action.
This may not show up with GDB, but using Lauterbach Trace32, and Hexrays IDA
Pro this issue is immediately seen.The response is a "$#00" empty packet,
showing it is unsupported packet.
This is defined in the RSP document as "An action with no thread-id matches all
threads."(https://sourceware.org/gdb/current/onlinedocs/gdb/Packets.html#vCont-packet
)
Thus the valid vCont packets now are as below, however parsing is still not
very strict. vCont;c/s - Step/Continue all threads
vCont;c/s:[pX.]Y - Step/Continue optional process X, thread Y
vCont;C##/S##:[pX.]Y - Step/Continue with signal ## on optional process X,
thread Y * If X or Y are -1 then it applies the action to all
processes/threads.
Signed-off-by: Lucien Murray-Pitts --- gdbstub.c |
16 ++-- 1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/gdbstub.c b/gdbstub.cindex bfc7afb509..ce0dde2e24 100644---
a/gdbstub.c+++ b/gdbstub.c@@ -1169,6 +1169,7 @@ static int
is_query_packet(const char *p, const char *query, char separator) */ static
int gdb_handle_vcont(GDBState *s, const char *p) {+ GDBThreadIdKind
vcontThreadType ; int res, signal = 0; char cur_action; char
*newstates;@@ -1218,12 +1219,23 @@ static int gdb_handle_vcont(GDBState *s,
const char *p) goto out; } - if (*p++ != ':') {+
/*+ * In the case we have vCont;c or vCont;s - action is on all
threads+ * Alternatively vCont;c;s:p1.1 is a possible, but meaningless
format,+ * And in the else the "vCont;c:p1.1;... format is supported.+
*/+ if (*p == '\0' || *p == ';') {+ vcontThreadType =
GDB_ALL_THREADS ;+ pid = 1 ;+ tid = 1 ;+ } else if
(*p++ == ':') {+ vcontThreadType = read_thread_id(p, , , )
;+ } else { res = -ENOTSUP; goto out; }
- switch (read_thread_id(p, , , )) {+ switch
(vcontThreadType) { case GDB_READ_THREAD_ERR: res =
-EINVAL; goto out;-- 2.17.2
[Qemu-devel] gdb run / pause broken for some debuggers
Hi gdbstub maintainers (CC: Luc Michel as he wrote the patch that probably
caused this issue)
I have found an issue with IDA Pro when moving to QEMU3.x where RUN/PAUSE has
stopped working.
I am pretty sure its to do with this debugger sending no thread-id with the
vCont packet action
That is that IDA Pro sends "vCont;c" (action=c, thread-id=none).
This is defined in the RSP document as "An action with no thread-id matches all
threads."
(https://sourceware.org/gdb/current/onlinedocs/gdb/Packets.html#vCont-packet )
In commit e40e5204af8388e605df2325d9b562c05919350e, which added multiprocess
support for vCont packets, the new code errors right away if it finds no ":"
action\thread-id separator as shown below:
if (*p++ != ':') { res = -ENOTSUP;...
The upper function calling gdb_handle_vcont when -EINVAL or -ERANGE will spit
out an "E22" error message, but for -ENOTSUP we do goto-unknown_command label
results in IDA Pro thinking the gdbstub is dead.
res = gdb_handle_vcont(s, p); if (res) {
if ((res == -EINVAL) || (res == -ERANGE)) { put_packet(s,
"E22"); ... else goto unknown_command:
Maybe we should extend this to add a "-ENOTSUP" clause so the "E22" is also
returned?
This "unknown_command:" branch should probably send a "E nn" packet rather than
just a blank "+" ack although here I do admit the spec is very unclear but in
most cases they do spec out "E nn" when some part of the command turns to
garbage.
... unknown_command: /* put empty packet */ buf[0]
= '\0'; put_packet(s, buf);
If people more experienced in this agree I will go ahead and try at a patch for
both of these issues.
Cheers,Luc
Re: [Qemu-devel] m68k gdb has stopped single stepping correctly
> On Thursday, January 24, 2019, 3:08:07 AM GMT+9, Emilio G. Cota wrote: > > On Wed, Jan 23, 2019 at 15:58:27 +0000, Lucien Anti-Spam via Qemu-devel wrote:> > Hi folks, > > I noticed that with 3.x release that the GDB options (-S -s) for certain > > CPU results in very weird stepping.Usually stops afer a few steps, whilst > > the stub continues responding the PC doesnt update, however, I have only > > deeply looked at the m68k. > > In the case of the m68K the SR gets the trace bit set (T=10b), and the PC > > doesnt update.The m68k gdbstub, and main gdbstub seem mostly unchanged.But > > it seems the INSN handling has changed greatly for the m68k. > > Does anyone have any ideas what happened?>> Can you please bisect to find > > at which point things start misbehaving? > > Thanks, > Emilio Understood, I was hoping my original post might jog someone's memory about the issue. Apparently not, so after some digging I found that it was introduced with the refactor to TranslatorOps, specifically two lines got dropped that update the PC if single-stepping is being performed ( commit 11ab74b01e0a8ea4973eed89c6b90fa6e4fb9fb6 ) Since its not valid to revert, shall I go ahead and submit a patch for these two lines? Also I noticed a lack of gdb-stub tests, but there are cpu tests (eg. check-qtest-m68k). I was considering it might be interesting to write some basic network code to send / receive gdb packets to re-use these cpu tests for step, break-point, register update, and so on. I saw a gdb-python test but I felt this would need specific kernel \ gdb for each cpu with is likely to end in a lot of problems getting right gdbs - simple packet send/receive/check would be better? What do people think, what would be the right approach to this? Cheers,Luc
[Qemu-devel] m68k gdb has stopped single stepping correctly
Hi folks, I noticed that with 3.x release that the GDB options (-S -s) for certain CPU results in very weird stepping.Usually stops afer a few steps, whilst the stub continues responding the PC doesnt update, however, I have only deeply looked at the m68k. In the case of the m68K the SR gets the trace bit set (T=10b), and the PC doesnt update.The m68k gdbstub, and main gdbstub seem mostly unchanged.But it seems the INSN handling has changed greatly for the m68k. Does anyone have any ideas what happened? Cheers,Luc
