WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

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

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... */
+char
+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.

+#ifdef CONFIG_GDB_NO_CSUM
+       return 0;
+#else
+       if (received_csum == csum) {
+               return 0;
+       } else {
+               return -1;
+       }
+#endif

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
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel