[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH] cdb: split cdb into arch independet/dependet part

On Jan 13, 2006, at 5:28 AM, Isaku Yamahata wrote:

attached the updated one. I tested only very roughly.
Hollis, is this patch ok for ppc version?

Thanks Isaku! This whole patch has whitespace issues though; please replace your tabs with 4 spaces.

+       . = ALIGN(32);
+       __gdbstub_text_start = . ;
+       *(.gdbstub.text)
+       __gdbstub_text_end = .  ;
+       . = ALIGN(32)           ;
+/* Append this to the declaration of any function needed to be used by
+ * the gdb stub.  Any functions marked with this will not be allowed
+ * to have a breakpoint set in them. */
+#define __gdbstub __attribute__((section(".gdbstub.text")))

These make sense if we want to avoid setting a breakpoint in GDB stub code or code called by the GDB stub, such as serial output. However, these symbols aren't currently used. (You probably inherited this from the PPC stub.) At least it's worth a TODO comment, possibly in gdb_cmd_write_mem(): we shouldn't be writing to memory between those two symbols.

+/* XXX move to some shared location... */
+hex2char(unsigned long x)

With hex2char, char2hex, str2hex, str2ulong, I expect that they will be needed in the arch-specific gdb backends. Actually I guess they just need to be added to gdbstub.h; I'm not sure why I added that comment.

+ for (ctx->in_bytes = 0; ctx->in_bytes < sizeof(ctx->in_buf); ctx->in_bytes++) {

Exceeds 80 chars.

+       return 0;
+       if (received_csum == csum) {
+               return 0;
+       } else {
+               return -1;
+       }

What is this about? As far as I know, the GDB protocol unconditionally requires the checksum.

+       //XXX optimization to use best native load size
+       //XXX optimization to use best native store size

I think it's reasonable to leave it up to the arch code to decide what size access to use, so these comments could go.

+       .signum                         = 1,
+       .in_bytes                       = 0,
+       .out_offset                     = 0,

Blank lines.

+       /* Try to make things a little more stable by disabling
+          interrupts while we're here. */
+       local_irq_save(flags);
+       local_irq_restore(flags);

Shouldn't we enter __trap_to_gdb with interrupts disabled already? That's how it's done on PPC, I believe x86 can choose in the IDT, and I'm sure IA64 can too?

Hmm, actually, how does this interact with interrupt-driven ns16550?

Hollis Blanchard
IBM Linux Technology Center

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.