Instead of living in drivers/char/console.c move the debugtrace
related coding to a new file common/debugtrace.c

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/common/Makefile        |   1 +
 xen/common/debugtrace.c    | 177 +++++++++++++++++++++++++++++++++++++++++++++
 xen/drivers/char/console.c | 176 +-------------------------------------------
 xen/include/xen/console.h  |   4 +
 4 files changed, 184 insertions(+), 174 deletions(-)
 create mode 100644 xen/common/debugtrace.c

diff --git a/xen/common/Makefile b/xen/common/Makefile
index eddda5daa6..62b34e69e9 100644
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -4,6 +4,7 @@ obj-y += bsearch.o
 obj-$(CONFIG_CORE_PARKING) += core_parking.o
 obj-y += cpu.o
 obj-y += cpupool.o
+obj-$(CONFIG_DEBUG_TRACE) += debugtrace.o
 obj-$(CONFIG_HAS_DEVICE_TREE) += device_tree.o
 obj-y += domctl.o
 obj-y += domain.o
diff --git a/xen/common/debugtrace.c b/xen/common/debugtrace.c
new file mode 100644
index 0000000000..ad10154495
--- /dev/null
+++ b/xen/common/debugtrace.c
@@ -0,0 +1,177 @@
+/******************************************************************************
+ * debugtrace.c
+ *
+ * Debugtrace for Xen
+ */
+
+
+#include <xen/console.h>
+#include <xen/init.h>
+#include <xen/keyhandler.h>
+#include <xen/lib.h>
+#include <xen/mm.h>
+#include <xen/serial.h>
+#include <xen/spinlock.h>
+#include <xen/watchdog.h>
+
+/* Send output direct to console, or buffer it? */
+static volatile int debugtrace_send_to_console;
+
+static char        *debugtrace_buf; /* Debug-trace buffer */
+static unsigned int debugtrace_prd; /* Producer index     */
+static unsigned int debugtrace_kilobytes = 128, debugtrace_bytes;
+static unsigned int debugtrace_used;
+static DEFINE_SPINLOCK(debugtrace_lock);
+integer_param("debugtrace", debugtrace_kilobytes);
+
+static void debugtrace_dump_worker(void)
+{
+    if ( (debugtrace_bytes == 0) || !debugtrace_used )
+        return;
+
+    printk("debugtrace_dump() starting\n");
+
+    /* Print oldest portion of the ring. */
+    ASSERT(debugtrace_buf[debugtrace_bytes - 1] == 0);
+    sercon_puts(&debugtrace_buf[debugtrace_prd]);
+
+    /* Print youngest portion of the ring. */
+    debugtrace_buf[debugtrace_prd] = '\0';
+    sercon_puts(&debugtrace_buf[0]);
+
+    memset(debugtrace_buf, '\0', debugtrace_bytes);
+
+    printk("debugtrace_dump() finished\n");
+}
+
+static void debugtrace_toggle(void)
+{
+    unsigned long flags;
+
+    watchdog_disable();
+    spin_lock_irqsave(&debugtrace_lock, flags);
+
+    /*
+     * Dump the buffer *before* toggling, in case the act of dumping the
+     * buffer itself causes more printk() invocations.
+     */
+    printk("debugtrace_printk now writing to %s.\n",
+           !debugtrace_send_to_console ? "console": "buffer");
+    if ( !debugtrace_send_to_console )
+        debugtrace_dump_worker();
+
+    debugtrace_send_to_console = !debugtrace_send_to_console;
+
+    spin_unlock_irqrestore(&debugtrace_lock, flags);
+    watchdog_enable();
+
+}
+
+void debugtrace_dump(void)
+{
+    unsigned long flags;
+
+    watchdog_disable();
+    spin_lock_irqsave(&debugtrace_lock, flags);
+
+    debugtrace_dump_worker();
+
+    spin_unlock_irqrestore(&debugtrace_lock, flags);
+    watchdog_enable();
+}
+
+static void debugtrace_add_to_buf(char *buf)
+{
+    char *p;
+
+    for ( p = buf; *p != '\0'; p++ )
+    {
+        debugtrace_buf[debugtrace_prd++] = *p;
+        /* Always leave a nul byte at the end of the buffer. */
+        if ( debugtrace_prd == (debugtrace_bytes - 1) )
+            debugtrace_prd = 0;
+    }
+}
+
+void debugtrace_printk(const char *fmt, ...)
+{
+    static char buf[1024], last_buf[1024];
+    static unsigned int count, last_count, last_prd;
+
+    char          cntbuf[24];
+    va_list       args;
+    unsigned long flags;
+
+    if ( debugtrace_bytes == 0 )
+        return;
+
+    debugtrace_used = 1;
+
+    spin_lock_irqsave(&debugtrace_lock, flags);
+
+    ASSERT(debugtrace_buf[debugtrace_bytes - 1] == 0);
+
+    va_start(args, fmt);
+    vsnprintf(buf, sizeof(buf), fmt, args);
+    va_end(args);
+
+    if ( debugtrace_send_to_console )
+    {
+        snprintf(cntbuf, sizeof(cntbuf), "%u ", ++count);
+        serial_puts(sercon_handle, cntbuf);
+        serial_puts(sercon_handle, buf);
+    }
+    else
+    {
+        if ( strcmp(buf, last_buf) )
+        {
+            last_prd = debugtrace_prd;
+            last_count = ++count;
+            safe_strcpy(last_buf, buf);
+            snprintf(cntbuf, sizeof(cntbuf), "%u ", count);
+        }
+        else
+        {
+            debugtrace_prd = last_prd;
+            snprintf(cntbuf, sizeof(cntbuf), "%u-%u ", last_count, ++count);
+        }
+        debugtrace_add_to_buf(cntbuf);
+        debugtrace_add_to_buf(buf);
+    }
+
+    spin_unlock_irqrestore(&debugtrace_lock, flags);
+}
+
+static void debugtrace_key(unsigned char key)
+{
+    debugtrace_toggle();
+}
+
+static int __init debugtrace_init(void)
+{
+    int order;
+    unsigned int kbytes, bytes;
+
+    /* Round size down to next power of two. */
+    while ( (kbytes = (debugtrace_kilobytes & (debugtrace_kilobytes-1))) != 0 )
+        debugtrace_kilobytes = kbytes;
+
+    bytes = debugtrace_kilobytes << 10;
+    if ( bytes == 0 )
+        return 0;
+
+    order = get_order_from_bytes(bytes);
+    debugtrace_buf = alloc_xenheap_pages(order, 0);
+    ASSERT(debugtrace_buf != NULL);
+
+    memset(debugtrace_buf, '\0', bytes);
+
+    debugtrace_bytes = bytes;
+
+    register_keyhandler('T', debugtrace_key,
+                        "toggle debugtrace to console/buffer", 0);
+
+    return 0;
+}
+__initcall(debugtrace_init);
+
diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index d728e737d1..9f87dc74ac 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -92,7 +92,7 @@ static char *__read_mostly conring = _conring;
 static uint32_t __read_mostly conring_size = _CONRING_SIZE;
 static uint32_t conringc, conringp;
 
-static int __read_mostly sercon_handle = -1;
+int __read_mostly sercon_handle = -1;
 
 #ifdef CONFIG_X86
 /* Tristate: 0 disabled, 1 user enabled, -1 default enabled */
@@ -346,7 +346,7 @@ void console_giveback(int id)
         serial_steal_fn = NULL;
 }
 
-static void sercon_puts(const char *s)
+void sercon_puts(const char *s)
 {
     if ( serial_steal_fn != NULL )
         (*serial_steal_fn)(s);
@@ -1155,178 +1155,6 @@ int printk_ratelimit(void)
     return __printk_ratelimit(printk_ratelimit_ms, printk_ratelimit_burst);
 }
 
-/*
- * **************************************************************
- * *************** Serial console ring buffer *******************
- * **************************************************************
- */
-
-#ifdef CONFIG_DEBUG_TRACE
-
-/* Send output direct to console, or buffer it? */
-static volatile int debugtrace_send_to_console;
-
-static char        *debugtrace_buf; /* Debug-trace buffer */
-static unsigned int debugtrace_prd; /* Producer index     */
-static unsigned int debugtrace_kilobytes = 128, debugtrace_bytes;
-static unsigned int debugtrace_used;
-static DEFINE_SPINLOCK(debugtrace_lock);
-integer_param("debugtrace", debugtrace_kilobytes);
-
-static void debugtrace_dump_worker(void)
-{
-    if ( (debugtrace_bytes == 0) || !debugtrace_used )
-        return;
-
-    printk("debugtrace_dump() starting\n");
-
-    /* Print oldest portion of the ring. */
-    ASSERT(debugtrace_buf[debugtrace_bytes - 1] == 0);
-    sercon_puts(&debugtrace_buf[debugtrace_prd]);
-
-    /* Print youngest portion of the ring. */
-    debugtrace_buf[debugtrace_prd] = '\0';
-    sercon_puts(&debugtrace_buf[0]);
-
-    memset(debugtrace_buf, '\0', debugtrace_bytes);
-
-    printk("debugtrace_dump() finished\n");
-}
-
-static void debugtrace_toggle(void)
-{
-    unsigned long flags;
-
-    watchdog_disable();
-    spin_lock_irqsave(&debugtrace_lock, flags);
-
-    /*
-     * Dump the buffer *before* toggling, in case the act of dumping the
-     * buffer itself causes more printk() invocations.
-     */
-    printk("debugtrace_printk now writing to %s.\n",
-           !debugtrace_send_to_console ? "console": "buffer");
-    if ( !debugtrace_send_to_console )
-        debugtrace_dump_worker();
-
-    debugtrace_send_to_console = !debugtrace_send_to_console;
-
-    spin_unlock_irqrestore(&debugtrace_lock, flags);
-    watchdog_enable();
-
-}
-
-void debugtrace_dump(void)
-{
-    unsigned long flags;
-
-    watchdog_disable();
-    spin_lock_irqsave(&debugtrace_lock, flags);
-
-    debugtrace_dump_worker();
-
-    spin_unlock_irqrestore(&debugtrace_lock, flags);
-    watchdog_enable();
-}
-
-static void debugtrace_add_to_buf(char *buf)
-{
-    char *p;
-
-    for ( p = buf; *p != '\0'; p++ )
-    {
-        debugtrace_buf[debugtrace_prd++] = *p;
-        /* Always leave a nul byte at the end of the buffer. */
-        if ( debugtrace_prd == (debugtrace_bytes - 1) )
-            debugtrace_prd = 0;
-    }
-}
-
-void debugtrace_printk(const char *fmt, ...)
-{
-    static char buf[1024], last_buf[1024];
-    static unsigned int count, last_count, last_prd;
-
-    char          cntbuf[24];
-    va_list       args;
-    unsigned long flags;
-
-    if ( debugtrace_bytes == 0 )
-        return;
-
-    debugtrace_used = 1;
-
-    spin_lock_irqsave(&debugtrace_lock, flags);
-
-    ASSERT(debugtrace_buf[debugtrace_bytes - 1] == 0);
-
-    va_start(args, fmt);
-    vsnprintf(buf, sizeof(buf), fmt, args);
-    va_end(args);
-
-    if ( debugtrace_send_to_console )
-    {
-        snprintf(cntbuf, sizeof(cntbuf), "%u ", ++count);
-        serial_puts(sercon_handle, cntbuf);
-        serial_puts(sercon_handle, buf);
-    }
-    else
-    {
-        if ( strcmp(buf, last_buf) )
-        {
-            last_prd = debugtrace_prd;
-            last_count = ++count;
-            safe_strcpy(last_buf, buf);
-            snprintf(cntbuf, sizeof(cntbuf), "%u ", count);
-        }
-        else
-        {
-            debugtrace_prd = last_prd;
-            snprintf(cntbuf, sizeof(cntbuf), "%u-%u ", last_count, ++count);
-        }
-        debugtrace_add_to_buf(cntbuf);
-        debugtrace_add_to_buf(buf);
-    }
-
-    spin_unlock_irqrestore(&debugtrace_lock, flags);
-}
-
-static void debugtrace_key(unsigned char key)
-{
-    debugtrace_toggle();
-}
-
-static int __init debugtrace_init(void)
-{
-    int order;
-    unsigned int kbytes, bytes;
-
-    /* Round size down to next power of two. */
-    while ( (kbytes = (debugtrace_kilobytes & (debugtrace_kilobytes-1))) != 0 )
-        debugtrace_kilobytes = kbytes;
-
-    bytes = debugtrace_kilobytes << 10;
-    if ( bytes == 0 )
-        return 0;
-
-    order = get_order_from_bytes(bytes);
-    debugtrace_buf = alloc_xenheap_pages(order, 0);
-    ASSERT(debugtrace_buf != NULL);
-
-    memset(debugtrace_buf, '\0', bytes);
-
-    debugtrace_bytes = bytes;
-
-    register_keyhandler('T', debugtrace_key,
-                        "toggle debugtrace to console/buffer", 0);
-
-    return 0;
-}
-__initcall(debugtrace_init);
-
-#endif /* !CONFIG_DEBUG_TRACE */
-
-
 /*
  * **************************************************************
  * *************** Debugging/tracing/error-report ***************
diff --git a/xen/include/xen/console.h b/xen/include/xen/console.h
index b4f9463936..26aede22e4 100644
--- a/xen/include/xen/console.h
+++ b/xen/include/xen/console.h
@@ -48,4 +48,8 @@ int console_resume(void);
 
 extern int8_t opt_console_xen;
 
+/* Issue string via serial line. */
+extern int sercon_handle;
+void sercon_puts(const char *s);
+
 #endif /* __CONSOLE_H__ */
-- 
2.16.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

On 28.07.2019 10:40, Juergen Gross wrote:
> @@ -1155,178 +1155,6 @@ int printk_ratelimit(void)
>       return __printk_ratelimit(printk_ratelimit_ms, printk_ratelimit_burst);
>   }
>   
> -/*
> - * **************************************************************
> - * *************** Serial console ring buffer *******************
> - * **************************************************************
> - */

I acknowledge that this comment has at best been displaced from what
it would properly belong to, so is indeed perhaps best dropped. But ...

> -#ifdef CONFIG_DEBUG_TRACE
> -
> -/* Send output direct to console, or buffer it? */
> -static volatile int debugtrace_send_to_console;
> -
> -static char        *debugtrace_buf; /* Debug-trace buffer */
> -static unsigned int debugtrace_prd; /* Producer index     */
> -static unsigned int debugtrace_kilobytes = 128, debugtrace_bytes;
> -static unsigned int debugtrace_used;
> -static DEFINE_SPINLOCK(debugtrace_lock);
> -integer_param("debugtrace", debugtrace_kilobytes);
> -
> -static void debugtrace_dump_worker(void)
> -{
> -    if ( (debugtrace_bytes == 0) || !debugtrace_used )
> -        return;
> -
> -    printk("debugtrace_dump() starting\n");
> -
> -    /* Print oldest portion of the ring. */
> -    ASSERT(debugtrace_buf[debugtrace_bytes - 1] == 0);
> -    sercon_puts(&debugtrace_buf[debugtrace_prd]);
> -
> -    /* Print youngest portion of the ring. */
> -    debugtrace_buf[debugtrace_prd] = '\0';
> -    sercon_puts(&debugtrace_buf[0]);
> -
> -    memset(debugtrace_buf, '\0', debugtrace_bytes);
> -
> -    printk("debugtrace_dump() finished\n");
> -}
> -
> -static void debugtrace_toggle(void)
> -{
> -    unsigned long flags;
> -
> -    watchdog_disable();
> -    spin_lock_irqsave(&debugtrace_lock, flags);
> -
> -    /*
> -     * Dump the buffer *before* toggling, in case the act of dumping the
> -     * buffer itself causes more printk() invocations.
> -     */
> -    printk("debugtrace_printk now writing to %s.\n",
> -           !debugtrace_send_to_console ? "console": "buffer");
> -    if ( !debugtrace_send_to_console )
> -        debugtrace_dump_worker();
> -
> -    debugtrace_send_to_console = !debugtrace_send_to_console;
> -
> -    spin_unlock_irqrestore(&debugtrace_lock, flags);
> -    watchdog_enable();
> -
> -}
> -
> -void debugtrace_dump(void)
> -{
> -    unsigned long flags;
> -
> -    watchdog_disable();
> -    spin_lock_irqsave(&debugtrace_lock, flags);
> -
> -    debugtrace_dump_worker();
> -
> -    spin_unlock_irqrestore(&debugtrace_lock, flags);
> -    watchdog_enable();
> -}
> -
> -static void debugtrace_add_to_buf(char *buf)
> -{
> -    char *p;
> -
> -    for ( p = buf; *p != '\0'; p++ )
> -    {
> -        debugtrace_buf[debugtrace_prd++] = *p;
> -        /* Always leave a nul byte at the end of the buffer. */
> -        if ( debugtrace_prd == (debugtrace_bytes - 1) )
> -            debugtrace_prd = 0;
> -    }
> -}
> -
> -void debugtrace_printk(const char *fmt, ...)
> -{
> -    static char buf[1024], last_buf[1024];
> -    static unsigned int count, last_count, last_prd;
> -
> -    char          cntbuf[24];
> -    va_list       args;
> -    unsigned long flags;
> -
> -    if ( debugtrace_bytes == 0 )
> -        return;
> -
> -    debugtrace_used = 1;
> -
> -    spin_lock_irqsave(&debugtrace_lock, flags);
> -
> -    ASSERT(debugtrace_buf[debugtrace_bytes - 1] == 0);
> -
> -    va_start(args, fmt);
> -    vsnprintf(buf, sizeof(buf), fmt, args);
> -    va_end(args);
> -
> -    if ( debugtrace_send_to_console )
> -    {
> -        snprintf(cntbuf, sizeof(cntbuf), "%u ", ++count);
> -        serial_puts(sercon_handle, cntbuf);
> -        serial_puts(sercon_handle, buf);
> -    }
> -    else
> -    {
> -        if ( strcmp(buf, last_buf) )
> -        {
> -            last_prd = debugtrace_prd;
> -            last_count = ++count;
> -            safe_strcpy(last_buf, buf);
> -            snprintf(cntbuf, sizeof(cntbuf), "%u ", count);
> -        }
> -        else
> -        {
> -            debugtrace_prd = last_prd;
> -            snprintf(cntbuf, sizeof(cntbuf), "%u-%u ", last_count, ++count);
> -        }
> -        debugtrace_add_to_buf(cntbuf);
> -        debugtrace_add_to_buf(buf);
> -    }
> -
> -    spin_unlock_irqrestore(&debugtrace_lock, flags);
> -}
> -
> -static void debugtrace_key(unsigned char key)
> -{
> -    debugtrace_toggle();
> -}
> -
> -static int __init debugtrace_init(void)
> -{
> -    int order;
> -    unsigned int kbytes, bytes;
> -
> -    /* Round size down to next power of two. */
> -    while ( (kbytes = (debugtrace_kilobytes & (debugtrace_kilobytes-1))) != 0 )
> -        debugtrace_kilobytes = kbytes;
> -
> -    bytes = debugtrace_kilobytes << 10;
> -    if ( bytes == 0 )
> -        return 0;
> -
> -    order = get_order_from_bytes(bytes);
> -    debugtrace_buf = alloc_xenheap_pages(order, 0);
> -    ASSERT(debugtrace_buf != NULL);
> -
> -    memset(debugtrace_buf, '\0', bytes);
> -
> -    debugtrace_bytes = bytes;
> -
> -    register_keyhandler('T', debugtrace_key,
> -                        "toggle debugtrace to console/buffer", 0);
> -
> -    return 0;
> -}
> -__initcall(debugtrace_init);
> -
> -#endif /* !CONFIG_DEBUG_TRACE */
> -
> -
>   /*
>    * **************************************************************
>    * *************** Debugging/tracing/error-report ***************

... what about this one? There's only panic() between it and the next
such comment, and I don't think the "Debugging/tracing" part of it
are applicable (anymore).

> --- a/xen/include/xen/console.h
> +++ b/xen/include/xen/console.h
> @@ -48,4 +48,8 @@ int console_resume(void);
>   
>   extern int8_t opt_console_xen;
>   
> +/* Issue string via serial line. */
> +extern int sercon_handle;
> +void sercon_puts(const char *s);

I guess avoiding their exposure was one of the reasons the debug trace
code lived in the place you move it from. I'm unconvinced non-console
code is actually supposed to make use of either, but I'm not opposed
enough to nak the change. I don't think though the comment fits well
with the variable declaration.

Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

On 29.07.19 14:45, Jan Beulich wrote:
> On 28.07.2019 10:40, Juergen Gross wrote:
>> -#endif /* !CONFIG_DEBUG_TRACE */
>> -
>> -
>>    /*
>>     * **************************************************************
>>     * *************** Debugging/tracing/error-report ***************
> 
> ... what about this one? There's only panic() between it and the next
> such comment, and I don't think the "Debugging/tracing" part of it
> are applicable (anymore).

True. I'll remove the "Debugging/tracing" part.

> 
>> --- a/xen/include/xen/console.h
>> +++ b/xen/include/xen/console.h
>> @@ -48,4 +48,8 @@ int console_resume(void);
>>    
>>    extern int8_t opt_console_xen;
>>    
>> +/* Issue string via serial line. */
>> +extern int sercon_handle;
>> +void sercon_puts(const char *s);
> 
> I guess avoiding their exposure was one of the reasons the debug trace
> code lived in the place you move it from. I'm unconvinced non-console
> code is actually supposed to make use of either, but I'm not opposed
> enough to nak the change. I don't think though the comment fits well
> with the variable declaration.

sercon_handle is used for calling serial_puts(), so maybe instead of
directly using serial_puts() with sercon_handle I should add a wrapper
to console.c (e.g. console_serial_puts())? It should be noted that
serial_puts() is called only in case of debugtrace output toggled to go
to the console. I guess using serial_puts() in that case is meant to
avoid too many software layers when doing the output. It would be
possible to use sercon_puts() for that case, too, resulting in the
inability to use debugtrace_printk() in the then additionally needed
paths (or better: to use it with output redirected to console).

sercon_puts() could use another wrapper, e.g. console_debug_puts().

Would you like that better?


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

On 29.07.2019 15:30, Juergen Gross wrote:
> On 29.07.19 14:45, Jan Beulich wrote:
>> On 28.07.2019 10:40, Juergen Gross wrote:
>>> -#endif /* !CONFIG_DEBUG_TRACE */
>>> -
>>> -
>>>    /*
>>>     * **************************************************************
>>>     * *************** Debugging/tracing/error-report ***************
>>
>> ... what about this one? There's only panic() between it and the next
>> such comment, and I don't think the "Debugging/tracing" part of it
>> are applicable (anymore).
> 
> True. I'll remove the "Debugging/tracing" part.
> 
>>
>>> --- a/xen/include/xen/console.h
>>> +++ b/xen/include/xen/console.h
>>> @@ -48,4 +48,8 @@ int console_resume(void);
>>>    extern int8_t opt_console_xen;
>>> +/* Issue string via serial line. */
>>> +extern int sercon_handle;
>>> +void sercon_puts(const char *s);
>>
>> I guess avoiding their exposure was one of the reasons the debug trace
>> code lived in the place you move it from. I'm unconvinced non-console
>> code is actually supposed to make use of either, but I'm not opposed
>> enough to nak the change. I don't think though the comment fits well
>> with the variable declaration.
> 
> sercon_handle is used for calling serial_puts(), so maybe instead of
> directly using serial_puts() with sercon_handle I should add a wrapper
> to console.c (e.g. console_serial_puts())? It should be noted that
> serial_puts() is called only in case of debugtrace output toggled to go
> to the console. I guess using serial_puts() in that case is meant to
> avoid too many software layers when doing the output.

Hmm, I'd rather expect this to be used to avoid doing anything else
sercon_puts() does besides calling serial_puts(). These other steps
are also why I think this is to remain a console internal interface.

> It would be
> possible to use sercon_puts() for that case, too, resulting in the
> inability to use debugtrace_printk() in the then additionally needed
> paths (or better: to use it with output redirected to console).
> 
> sercon_puts() could use another wrapper, e.g. console_debug_puts().
> 
> Would you like that better?

Probably not. I wonder whether splitting out this code is really a
good step.

Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

On 29.07.19 16:00, Jan Beulich wrote:
> On 29.07.2019 15:30, Juergen Gross wrote:
>> On 29.07.19 14:45, Jan Beulich wrote:
>>> On 28.07.2019 10:40, Juergen Gross wrote:
>>>> -#endif /* !CONFIG_DEBUG_TRACE */
>>>> -
>>>> -
>>>>     /*
>>>>      * **************************************************************
>>>>      * *************** Debugging/tracing/error-report ***************
>>>
>>> ... what about this one? There's only panic() between it and the next
>>> such comment, and I don't think the "Debugging/tracing" part of it
>>> are applicable (anymore).
>>
>> True. I'll remove the "Debugging/tracing" part.
>>
>>>
>>>> --- a/xen/include/xen/console.h
>>>> +++ b/xen/include/xen/console.h
>>>> @@ -48,4 +48,8 @@ int console_resume(void);
>>>>     extern int8_t opt_console_xen;
>>>> +/* Issue string via serial line. */
>>>> +extern int sercon_handle;
>>>> +void sercon_puts(const char *s);
>>>
>>> I guess avoiding their exposure was one of the reasons the debug trace
>>> code lived in the place you move it from. I'm unconvinced non-console
>>> code is actually supposed to make use of either, but I'm not opposed
>>> enough to nak the change. I don't think though the comment fits well
>>> with the variable declaration.
>>
>> sercon_handle is used for calling serial_puts(), so maybe instead of
>> directly using serial_puts() with sercon_handle I should add a wrapper
>> to console.c (e.g. console_serial_puts())? It should be noted that
>> serial_puts() is called only in case of debugtrace output toggled to go
>> to the console. I guess using serial_puts() in that case is meant to
>> avoid too many software layers when doing the output.
> 
> Hmm, I'd rather expect this to be used to avoid doing anything else
> sercon_puts() does besides calling serial_puts(). These other steps
> are also why I think this is to remain a console internal interface.

To me it seems a little bit strange to have the buffer dumping using
sercon_puts() while issuing the actual trace entries to console isn't
using it.

> 
>> It would be
>> possible to use sercon_puts() for that case, too, resulting in the
>> inability to use debugtrace_printk() in the then additionally needed
>> paths (or better: to use it with output redirected to console).
>>
>> sercon_puts() could use another wrapper, e.g. console_debug_puts().
>>
>> Would you like that better?
> 
> Probably not. I wonder whether splitting out this code is really a
> good step.

I'm not fighting for it, I just thought it would better be put into a
file of its own.

In case you disagree and others are not pushing for separation I'm fine
to drop this patch.


Juergen


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

On 29.07.2019 16:28, Juergen Gross wrote:
> On 29.07.19 16:00, Jan Beulich wrote:
>> On 29.07.2019 15:30, Juergen Gross wrote:
>>> On 29.07.19 14:45, Jan Beulich wrote:
>>>> On 28.07.2019 10:40, Juergen Gross wrote:
>>>>> -#endif /* !CONFIG_DEBUG_TRACE */
>>>>> -
>>>>> -
>>>>>     /*
>>>>>      * **************************************************************
>>>>>      * *************** Debugging/tracing/error-report ***************
>>>>
>>>> ... what about this one? There's only panic() between it and the next
>>>> such comment, and I don't think the "Debugging/tracing" part of it
>>>> are applicable (anymore).
>>>
>>> True. I'll remove the "Debugging/tracing" part.
>>>
>>>>
>>>>> --- a/xen/include/xen/console.h
>>>>> +++ b/xen/include/xen/console.h
>>>>> @@ -48,4 +48,8 @@ int console_resume(void);
>>>>>     extern int8_t opt_console_xen;
>>>>> +/* Issue string via serial line. */
>>>>> +extern int sercon_handle;
>>>>> +void sercon_puts(const char *s);
>>>>
>>>> I guess avoiding their exposure was one of the reasons the debug trace
>>>> code lived in the place you move it from. I'm unconvinced non-console
>>>> code is actually supposed to make use of either, but I'm not opposed
>>>> enough to nak the change. I don't think though the comment fits well
>>>> with the variable declaration.
>>>
>>> sercon_handle is used for calling serial_puts(), so maybe instead of
>>> directly using serial_puts() with sercon_handle I should add a wrapper
>>> to console.c (e.g. console_serial_puts())? It should be noted that
>>> serial_puts() is called only in case of debugtrace output toggled to go
>>> to the console. I guess using serial_puts() in that case is meant to
>>> avoid too many software layers when doing the output.
>>
>> Hmm, I'd rather expect this to be used to avoid doing anything else
>> sercon_puts() does besides calling serial_puts(). These other steps
>> are also why I think this is to remain a console internal interface.
> 
> To me it seems a little bit strange to have the buffer dumping using
> sercon_puts() while issuing the actual trace entries to console isn't
> using it.

I guess I agree.

>>> It would be
>>> possible to use sercon_puts() for that case, too, resulting in the
>>> inability to use debugtrace_printk() in the then additionally needed
>>> paths (or better: to use it with output redirected to console).
>>>
>>> sercon_puts() could use another wrapper, e.g. console_debug_puts().
>>>
>>> Would you like that better?
>>
>> Probably not. I wonder whether splitting out this code is really a
>> good step.
> 
> I'm not fighting for it, I just thought it would better be put into a
> file of its own.
> 
> In case you disagree and others are not pushing for separation I'm fine
> to drop this patch.

Well, I don't mind the separation as long as it's indeed properly
separated in the end.

Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel