| Message ID | 20190828080028.18205-2-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen: debugtrace cleanup and per-cpu buffer support | expand |
On 28.08.2019 10:00, Juergen Gross wrote: > Today dumping the debugtrace buffers is done via sercon_puts(), while > direct printing of trace entries (after toggling output to the console) > is using serial_puts(). > > Use sercon_puts() in both cases, as the difference between both is not > really making sense. No matter that I like this change, I'm not convinced it's correct: There are two differences between the functions, neither of which I could qualify as "not really making sense": Why is it obvious that it makes no sense for the debugtrace output to bypass one or both of serial_steal_fn() and pv_console_puts()? If you're convinced of this, please provide the "why"-s behind the sentence above. Jan
On 03.09.19 12:00, Jan Beulich wrote: > On 28.08.2019 10:00, Juergen Gross wrote: >> Today dumping the debugtrace buffers is done via sercon_puts(), while >> direct printing of trace entries (after toggling output to the console) >> is using serial_puts(). >> >> Use sercon_puts() in both cases, as the difference between both is not >> really making sense. > > No matter that I like this change, I'm not convinced it's correct: > There are two differences between the functions, neither of which > I could qualify as "not really making sense": Why is it obvious > that it makes no sense for the debugtrace output to bypass one or > both of serial_steal_fn() and pv_console_puts()? If you're > convinced of this, please provide the "why"-s behind the sentence > above. Okay, I'll use: Use sercon_puts() in both cases, to be consistent between dumping the buffer when switching debugtrace to console output and when printing a debugtrace entry directly to console. Juergen
On 03.09.2019 12:22, Juergen Gross wrote: > On 03.09.19 12:00, Jan Beulich wrote: >> On 28.08.2019 10:00, Juergen Gross wrote: >>> Today dumping the debugtrace buffers is done via sercon_puts(), while >>> direct printing of trace entries (after toggling output to the console) >>> is using serial_puts(). >>> >>> Use sercon_puts() in both cases, as the difference between both is not >>> really making sense. >> >> No matter that I like this change, I'm not convinced it's correct: >> There are two differences between the functions, neither of which >> I could qualify as "not really making sense": Why is it obvious >> that it makes no sense for the debugtrace output to bypass one or >> both of serial_steal_fn() and pv_console_puts()? If you're >> convinced of this, please provide the "why"-s behind the sentence >> above. > > Okay, I'll use: > > Use sercon_puts() in both cases, to be consistent between dumping the > buffer when switching debugtrace to console output and when printing > a debugtrace entry directly to console. Well, this is better as an explanation indeed, but it still doesn't make clear whether it wasn't in fact wanted for there to be a difference in where output gets sent. I may believe that bypassing pv_console_puts() wasn't intended, but the steal function bypass had been there in 3.2 already, so may well have been on purpose. Jan
On 03.09.19 13:47, Jan Beulich wrote: > On 03.09.2019 12:22, Juergen Gross wrote: >> On 03.09.19 12:00, Jan Beulich wrote: >>> On 28.08.2019 10:00, Juergen Gross wrote: >>>> Today dumping the debugtrace buffers is done via sercon_puts(), while >>>> direct printing of trace entries (after toggling output to the console) >>>> is using serial_puts(). >>>> >>>> Use sercon_puts() in both cases, as the difference between both is not >>>> really making sense. >>> >>> No matter that I like this change, I'm not convinced it's correct: >>> There are two differences between the functions, neither of which >>> I could qualify as "not really making sense": Why is it obvious >>> that it makes no sense for the debugtrace output to bypass one or >>> both of serial_steal_fn() and pv_console_puts()? If you're >>> convinced of this, please provide the "why"-s behind the sentence >>> above. >> >> Okay, I'll use: >> >> Use sercon_puts() in both cases, to be consistent between dumping the >> buffer when switching debugtrace to console output and when printing >> a debugtrace entry directly to console. > > Well, this is better as an explanation indeed, but it still doesn't > make clear whether it wasn't in fact wanted for there to be a > difference in where output gets sent. I may believe that bypassing > pv_console_puts() wasn't intended, but the steal function bypass > had been there in 3.2 already, so may well have been on purpose. There are two users of console_steal(): suspend handling - I believe it is a good idea to not use the serial interface while it is potentially uninitialized. gdb support - Why should that be special? Not treating the output data appropriate to the attached debugger will be worse than encapsulating it in a way the debugger can handle. Juergen
On 03.09.2019 13:58, Juergen Gross wrote: > On 03.09.19 13:47, Jan Beulich wrote: >> On 03.09.2019 12:22, Juergen Gross wrote: >>> On 03.09.19 12:00, Jan Beulich wrote: >>>> On 28.08.2019 10:00, Juergen Gross wrote: >>>>> Today dumping the debugtrace buffers is done via sercon_puts(), while >>>>> direct printing of trace entries (after toggling output to the console) >>>>> is using serial_puts(). >>>>> >>>>> Use sercon_puts() in both cases, as the difference between both is not >>>>> really making sense. >>>> >>>> No matter that I like this change, I'm not convinced it's correct: >>>> There are two differences between the functions, neither of which >>>> I could qualify as "not really making sense": Why is it obvious >>>> that it makes no sense for the debugtrace output to bypass one or >>>> both of serial_steal_fn() and pv_console_puts()? If you're >>>> convinced of this, please provide the "why"-s behind the sentence >>>> above. >>> >>> Okay, I'll use: >>> >>> Use sercon_puts() in both cases, to be consistent between dumping the >>> buffer when switching debugtrace to console output and when printing >>> a debugtrace entry directly to console. >> >> Well, this is better as an explanation indeed, but it still doesn't >> make clear whether it wasn't in fact wanted for there to be a >> difference in where output gets sent. I may believe that bypassing >> pv_console_puts() wasn't intended, but the steal function bypass >> had been there in 3.2 already, so may well have been on purpose. > > There are two users of console_steal(): > > suspend handling - I believe it is a good idea to not use the serial > interface while it is potentially uninitialized. I agree here. > gdb support - Why should that be special? Not treating the output data > appropriate to the attached debugger will be worse than encapsulating > it in a way the debugger can handle. I'm not sure here - it may well have been intentional to avoid cluttering the debugger console. Jan
On 03.09.19 14:09, Jan Beulich wrote: > On 03.09.2019 13:58, Juergen Gross wrote: >> On 03.09.19 13:47, Jan Beulich wrote: >>> On 03.09.2019 12:22, Juergen Gross wrote: >>>> On 03.09.19 12:00, Jan Beulich wrote: >>>>> On 28.08.2019 10:00, Juergen Gross wrote: >>>>>> Today dumping the debugtrace buffers is done via sercon_puts(), while >>>>>> direct printing of trace entries (after toggling output to the console) >>>>>> is using serial_puts(). >>>>>> >>>>>> Use sercon_puts() in both cases, as the difference between both is not >>>>>> really making sense. >>>>> >>>>> No matter that I like this change, I'm not convinced it's correct: >>>>> There are two differences between the functions, neither of which >>>>> I could qualify as "not really making sense": Why is it obvious >>>>> that it makes no sense for the debugtrace output to bypass one or >>>>> both of serial_steal_fn() and pv_console_puts()? If you're >>>>> convinced of this, please provide the "why"-s behind the sentence >>>>> above. >>>> >>>> Okay, I'll use: >>>> >>>> Use sercon_puts() in both cases, to be consistent between dumping the >>>> buffer when switching debugtrace to console output and when printing >>>> a debugtrace entry directly to console. >>> >>> Well, this is better as an explanation indeed, but it still doesn't >>> make clear whether it wasn't in fact wanted for there to be a >>> difference in where output gets sent. I may believe that bypassing >>> pv_console_puts() wasn't intended, but the steal function bypass >>> had been there in 3.2 already, so may well have been on purpose. >> >> There are two users of console_steal(): >> >> suspend handling - I believe it is a good idea to not use the serial >> interface while it is potentially uninitialized. > > I agree here. > >> gdb support - Why should that be special? Not treating the output data >> appropriate to the attached debugger will be worse than encapsulating >> it in a way the debugger can handle. > > I'm not sure here - it may well have been intentional to avoid > cluttering the debugger console. But won't using serial_puts() clutter the debugger console? "normal" printk() output seem to be handled in a special way with gdb active, while just using serial_puts() with attached gdb is looking at least suspicious to me. There seems to be a special format, e.g. text output of the hypervisor is prepended with 'O' and the data is sent in hex representation. I can't imagine just sending some ASCII text is the desired approach for debugtrace output. Juergen
On 03.09.2019 14:22, Juergen Gross wrote: > On 03.09.19 14:09, Jan Beulich wrote: >> On 03.09.2019 13:58, Juergen Gross wrote: >>> On 03.09.19 13:47, Jan Beulich wrote: >>>> On 03.09.2019 12:22, Juergen Gross wrote: >>>>> On 03.09.19 12:00, Jan Beulich wrote: >>>>>> On 28.08.2019 10:00, Juergen Gross wrote: >>>>>>> Today dumping the debugtrace buffers is done via sercon_puts(), while >>>>>>> direct printing of trace entries (after toggling output to the console) >>>>>>> is using serial_puts(). >>>>>>> >>>>>>> Use sercon_puts() in both cases, as the difference between both is not >>>>>>> really making sense. >>>>>> >>>>>> No matter that I like this change, I'm not convinced it's correct: >>>>>> There are two differences between the functions, neither of which >>>>>> I could qualify as "not really making sense": Why is it obvious >>>>>> that it makes no sense for the debugtrace output to bypass one or >>>>>> both of serial_steal_fn() and pv_console_puts()? If you're >>>>>> convinced of this, please provide the "why"-s behind the sentence >>>>>> above. >>>>> >>>>> Okay, I'll use: >>>>> >>>>> Use sercon_puts() in both cases, to be consistent between dumping the >>>>> buffer when switching debugtrace to console output and when printing >>>>> a debugtrace entry directly to console. >>>> >>>> Well, this is better as an explanation indeed, but it still doesn't >>>> make clear whether it wasn't in fact wanted for there to be a >>>> difference in where output gets sent. I may believe that bypassing >>>> pv_console_puts() wasn't intended, but the steal function bypass >>>> had been there in 3.2 already, so may well have been on purpose. >>> >>> There are two users of console_steal(): >>> >>> suspend handling - I believe it is a good idea to not use the serial >>> interface while it is potentially uninitialized. >> >> I agree here. >> >>> gdb support - Why should that be special? Not treating the output data >>> appropriate to the attached debugger will be worse than encapsulating >>> it in a way the debugger can handle. >> >> I'm not sure here - it may well have been intentional to avoid >> cluttering the debugger console. > > But won't using serial_puts() clutter the debugger console? "normal" > printk() output seem to be handled in a special way with gdb active, > while just using serial_puts() with attached gdb is looking at least > suspicious to me. There seems to be a special format, e.g. text output > of the hypervisor is prepended with 'O' and the data is sent in hex > representation. I can't imagine just sending some ASCII text is the > desired approach for debugtrace output. Oh, did I get it backwards, I'm sorry. Yes, I agree. With the slightly adjusted description Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan
diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c index d67e1993f2..f49c6f29a8 100644 --- a/xen/drivers/char/console.c +++ b/xen/drivers/char/console.c @@ -346,7 +346,7 @@ void console_giveback(int id) serial_steal_fn = NULL; } -static void sercon_puts(const char *s, size_t nr) +void console_serial_puts(const char *s, size_t nr) { if ( serial_steal_fn != NULL ) serial_steal_fn(s, nr); @@ -388,7 +388,7 @@ static void dump_console_ring_key(unsigned char key) c += len; } - sercon_puts(buf, sofar); + console_serial_puts(buf, sofar); video_puts(buf, sofar); free_xenheap_pages(buf, order); @@ -547,7 +547,7 @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, /* Use direct console output as it could be interactive */ spin_lock_irq(&console_lock); - sercon_puts(kbuf, kcount); + console_serial_puts(kbuf, kcount); video_puts(kbuf, kcount); #ifdef CONFIG_X86 @@ -674,7 +674,7 @@ static void __putstr(const char *str) ASSERT(spin_is_locked(&console_lock)); - sercon_puts(str, len); + console_serial_puts(str, len); video_puts(str, len); #ifdef CONFIG_X86 @@ -1186,12 +1186,12 @@ static void debugtrace_dump_worker(void) /* Print oldest portion of the ring. */ ASSERT(debugtrace_buf[debugtrace_bytes - 1] == 0); if ( debugtrace_buf[debugtrace_prd] != '\0' ) - sercon_puts(&debugtrace_buf[debugtrace_prd], - debugtrace_bytes - debugtrace_prd - 1); + console_serial_puts(&debugtrace_buf[debugtrace_prd], + debugtrace_bytes - debugtrace_prd - 1); /* Print youngest portion of the ring. */ debugtrace_buf[debugtrace_prd] = '\0'; - sercon_puts(&debugtrace_buf[0], debugtrace_prd); + console_serial_puts(&debugtrace_buf[0], debugtrace_prd); memset(debugtrace_buf, '\0', debugtrace_bytes); @@ -1274,8 +1274,8 @@ void debugtrace_printk(const char *fmt, ...) { unsigned int n = scnprintf(cntbuf, sizeof(cntbuf), "%u ", ++count); - serial_puts(sercon_handle, cntbuf, n); - serial_puts(sercon_handle, buf, nr); + console_serial_puts(cntbuf, n); + console_serial_puts(buf, nr); } else { diff --git a/xen/include/xen/console.h b/xen/include/xen/console.h index ba914f9e5b..53c56191ba 100644 --- a/xen/include/xen/console.h +++ b/xen/include/xen/console.h @@ -46,6 +46,9 @@ void console_giveback(int id); int console_suspend(void); int console_resume(void); +/* Emit a string via the serial console. */ +void console_serial_puts(const char *s, size_t nr); + extern int8_t opt_console_xen; #endif /* __CONSOLE_H__ */
Today dumping the debugtrace buffers is done via sercon_puts(), while direct printing of trace entries (after toggling output to the console) is using serial_puts(). Use sercon_puts() in both cases, as the difference between both is not really making sense. In order to prepare moving debugtrace functionality to an own source file rename sercon_puts() to console_serial_puts() and make it globally visible. Signed-off-by: Juergen Gross <jgross@suse.com> --- xen/drivers/char/console.c | 18 +++++++++--------- xen/include/xen/console.h | 3 +++ 2 files changed, 12 insertions(+), 9 deletions(-)