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-ia64-devel

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

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