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

[Isaku Yamahata <yamahata@xxxxxxxxxxxxx>]

Hi.

Thanks for your review.
I updated the patch following your comments except
the interrupt-related related comment.
I included your signed-off-by to the patch, is it ok?


On Fri, Jan 13, 2006 at 09:46:04AM -0600, Hollis Blanchard wrote:

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

untabified.


> >+    . = 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.

added __gdbstub_text_start, __gdbstub_text_end declarations and
a comment above gdb_cmd_write_mem()


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

added protptypes to gdb.h and removed two XXX comments.


> >+    for (ctx->in_bytes = 0; ctx->in_bytes < sizeof(ctx->in_buf); 
> >ctx->in_bytes++) {
> Exceeds 80 chars.

fixed.


> >+#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.

removed.


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

removed these comments.


> >+    .signum                         = 1,
> >+
> >+    .in_bytes                       = 0,
> >+
> >+    .out_offset                     = 0,
> 
> Blank lines.

removed 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?

Left as it was. 
As Keir explained, it is a good safe guard.

-- 
yamahata

Attachment: 8605:3ca0840ea69e.patch
Description: Text document

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel