* [PATCH 0/4] xen/console: Bug fixes and doc improvement @ 2019-04-02 16:42 Julien Grall 2019-04-02 16:42 ` [PATCH 1/4] xen/console: Properly buffer domU output when using CONSOLEIO_write Julien Grall ` (3 more replies) 0 siblings, 4 replies; 42+ messages in thread From: Julien Grall @ 2019-04-02 16:42 UTC (permalink / raw) To: xen-devel Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, Jan Beulich Hi all, This series contains a bunch of bug fixes for the hypercall CONSOLEIO_write and some documentation. Note the patch #2 was originally sent in standalone, I have included here with comments addressed to keep everything together. Cheers, Julien Grall (4): xen/console: Properly buffer domU output when using CONSOLEIO_write xen/console: Don't treat NUL character as the end of the buffer xen/public: Document HYPERCALL_console_io() xen/console: Simplify domU console handling in guest_console_write xen/arch/arm/early_printk.c | 5 +-- xen/common/gdbstub.c | 6 ++-- xen/drivers/char/console.c | 71 ++++++++++++++++++--------------------- xen/drivers/char/consoled.c | 7 ++-- xen/drivers/char/serial.c | 8 +++-- xen/drivers/char/xen_pv_console.c | 10 +++--- xen/drivers/video/lfb.c | 14 ++++---- xen/drivers/video/lfb.h | 4 +-- xen/drivers/video/vga.c | 14 ++++---- xen/include/public/xen.h | 21 +++++++++++- xen/include/xen/console.h | 2 +- xen/include/xen/early_printk.h | 2 +- xen/include/xen/pv_console.h | 4 +-- xen/include/xen/serial.h | 4 +-- xen/include/xen/video.h | 4 +-- 15 files changed, 97 insertions(+), 79 deletions(-) -- 2.11.0 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 1/4] xen/console: Properly buffer domU output when using CONSOLEIO_write 2019-04-02 16:42 [PATCH 0/4] xen/console: Bug fixes and doc improvement Julien Grall @ 2019-04-02 16:42 ` Julien Grall 2019-04-03 11:41 ` Wei Liu 2019-04-02 16:42 ` [PATCH 2/4] xen/console: Don't treat NUL character as the end of the buffer Julien Grall ` (2 subsequent siblings) 3 siblings, 1 reply; 42+ messages in thread From: Julien Grall @ 2019-04-02 16:42 UTC (permalink / raw) To: xen-devel Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, Jan Beulich The output will be buffered if the buffer provided by the DomU does not contain a newline. This can also happen if buffer provided by DomU is split in multiple part (Xen can only process 127 characters at the time). As Xen will remove any non-printable characters, the output buffer may be smaller than the buffer provided. However, Xen will buffer using the original length. This means that the NUL character and garbagge will be copied in the internal buffer. Once the newline is found or the internal buffer is full, only part of the internal buffer will end up to be printed. An easy way to reproduce it is: HYPERVISOR_consoleio(CONSOLEIO_write, "\33", 1); HYPERVISOR_consoleio(CONSOLEIO_write, "d", 1); HYPERVISOR_consoleio(CONSOLEIO_write, "\n", 1); In the current code, the character 'd' will not be printed. This problem can be solved by computing the size of the output buffer (i.e the buffer without the non-printable characters). Signed-off-by: Julien Grall <julien.grall@arm.com> --- I is possible to compute (kout - kbuf) only once. I didn't do it because I wasn't able to find a good name. --- xen/drivers/char/console.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c index 5f0f54201b..9bbcb0f57a 100644 --- a/xen/drivers/char/console.c +++ b/xen/drivers/char/console.c @@ -592,11 +592,11 @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, int count) guest_printk(cd, XENLOG_G_DEBUG "%s%s\n", cd->pbuf, kbuf); cd->pbuf_idx = 0; } - else if ( cd->pbuf_idx + kcount < (DOMAIN_PBUF_SIZE - 1) ) + else if ( cd->pbuf_idx + (kout - kbuf) < (DOMAIN_PBUF_SIZE - 1) ) { /* buffer the output until a newline */ - memcpy(cd->pbuf + cd->pbuf_idx, kbuf, kcount); - cd->pbuf_idx += kcount; + memcpy(cd->pbuf + cd->pbuf_idx, kbuf, kout - kbuf); + cd->pbuf_idx += (kout - kbuf); } else { -- 2.11.0 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH 1/4] xen/console: Properly buffer domU output when using CONSOLEIO_write 2019-04-02 16:42 ` [PATCH 1/4] xen/console: Properly buffer domU output when using CONSOLEIO_write Julien Grall @ 2019-04-03 11:41 ` Wei Liu 2019-04-09 10:25 ` [Xen-devel] " Julien Grall 0 siblings, 1 reply; 42+ messages in thread From: Wei Liu @ 2019-04-03 11:41 UTC (permalink / raw) To: Julien Grall Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Jan Beulich, xen-devel On Tue, Apr 02, 2019 at 05:42:35PM +0100, Julien Grall wrote: > The output will be buffered if the buffer provided by the DomU does not > contain a newline. This can also happen if buffer provided by DomU is > split in multiple part (Xen can only process 127 characters at the time). > > As Xen will remove any non-printable characters, the output buffer may > be smaller than the buffer provided. However, Xen will buffer using the > original length. This means that the NUL character and garbagge will be > copied in the internal buffer. > > Once the newline is found or the internal buffer is full, only part of > the internal buffer will end up to be printed. > > An easy way to reproduce it is: > > HYPERVISOR_consoleio(CONSOLEIO_write, "\33", 1); > HYPERVISOR_consoleio(CONSOLEIO_write, "d", 1); > HYPERVISOR_consoleio(CONSOLEIO_write, "\n", 1); > > In the current code, the character 'd' will not be printed. > > This problem can be solved by computing the size of the output buffer > (i.e the buffer without the non-printable characters). > > Signed-off-by: Julien Grall <julien.grall@arm.com> Acked-by: Wei Liu <wei.liu2@citrix.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/4] xen/console: Properly buffer domU output when using CONSOLEIO_write @ 2019-04-09 10:25 ` Julien Grall 0 siblings, 0 replies; 42+ messages in thread From: Julien Grall @ 2019-04-09 10:25 UTC (permalink / raw) To: Wei Liu Cc: Stefano Stabellini, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Jan Beulich, xen-devel Hi, On 03/04/2019 12:41, Wei Liu wrote: > On Tue, Apr 02, 2019 at 05:42:35PM +0100, Julien Grall wrote: >> The output will be buffered if the buffer provided by the DomU does not >> contain a newline. This can also happen if buffer provided by DomU is >> split in multiple part (Xen can only process 127 characters at the time). >> >> As Xen will remove any non-printable characters, the output buffer may >> be smaller than the buffer provided. However, Xen will buffer using the >> original length. This means that the NUL character and garbagge will be >> copied in the internal buffer. >> >> Once the newline is found or the internal buffer is full, only part of >> the internal buffer will end up to be printed. >> >> An easy way to reproduce it is: >> >> HYPERVISOR_consoleio(CONSOLEIO_write, "\33", 1); >> HYPERVISOR_consoleio(CONSOLEIO_write, "d", 1); >> HYPERVISOR_consoleio(CONSOLEIO_write, "\n", 1); >> >> In the current code, the character 'd' will not be printed. >> >> This problem can be solved by computing the size of the output buffer >> (i.e the buffer without the non-printable characters). >> >> Signed-off-by: Julien Grall <julien.grall@arm.com> > > Acked-by: Wei Liu <wei.liu2@citrix.com> I have committed this patch. The rest of the series need to be respined. Cheers, > -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Xen-devel] [PATCH 1/4] xen/console: Properly buffer domU output when using CONSOLEIO_write @ 2019-04-09 10:25 ` Julien Grall 0 siblings, 0 replies; 42+ messages in thread From: Julien Grall @ 2019-04-09 10:25 UTC (permalink / raw) To: Wei Liu Cc: Stefano Stabellini, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Jan Beulich, xen-devel Hi, On 03/04/2019 12:41, Wei Liu wrote: > On Tue, Apr 02, 2019 at 05:42:35PM +0100, Julien Grall wrote: >> The output will be buffered if the buffer provided by the DomU does not >> contain a newline. This can also happen if buffer provided by DomU is >> split in multiple part (Xen can only process 127 characters at the time). >> >> As Xen will remove any non-printable characters, the output buffer may >> be smaller than the buffer provided. However, Xen will buffer using the >> original length. This means that the NUL character and garbagge will be >> copied in the internal buffer. >> >> Once the newline is found or the internal buffer is full, only part of >> the internal buffer will end up to be printed. >> >> An easy way to reproduce it is: >> >> HYPERVISOR_consoleio(CONSOLEIO_write, "\33", 1); >> HYPERVISOR_consoleio(CONSOLEIO_write, "d", 1); >> HYPERVISOR_consoleio(CONSOLEIO_write, "\n", 1); >> >> In the current code, the character 'd' will not be printed. >> >> This problem can be solved by computing the size of the output buffer >> (i.e the buffer without the non-printable characters). >> >> Signed-off-by: Julien Grall <julien.grall@arm.com> > > Acked-by: Wei Liu <wei.liu2@citrix.com> I have committed this patch. The rest of the series need to be respined. Cheers, > -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 2/4] xen/console: Don't treat NUL character as the end of the buffer 2019-04-02 16:42 [PATCH 0/4] xen/console: Bug fixes and doc improvement Julien Grall 2019-04-02 16:42 ` [PATCH 1/4] xen/console: Properly buffer domU output when using CONSOLEIO_write Julien Grall @ 2019-04-02 16:42 ` Julien Grall 2019-04-02 17:49 ` Andrew Cooper ` (2 more replies) 2019-04-02 16:42 ` [PATCH 3/4] xen/public: Document HYPERCALL_console_io() Julien Grall 2019-04-02 16:42 ` [PATCH 4/4] xen/console: Simplify domU console handling in guest_console_write Julien Grall 3 siblings, 3 replies; 42+ messages in thread From: Julien Grall @ 2019-04-02 16:42 UTC (permalink / raw) To: xen-devel Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, Jan Beulich After upgrading Debian to Buster, I have began to notice console mangling when using zsh in Dom0. This is happenning because output sent by zsh to the console may contain NULs in the middle of the buffer. The actual implementation of CONSOLEIO_write considers that a buffer always terminate with a NUL and therefore will ignore anything after it. In general, NULs are perfectly legitimate in terminal streams. For instance, this could be used for padding slow terminals. See terminfo(5) section `Delays and Padding`, or search for the pcre '\bpad'. Other use cases includes using the console for dumping non-human readable information (e.g debugger, file if no network...). With the current behavior, the resulting stream will end up to be corrupted. The documentation for CONSOLEIO_write is pretty limited (to not say inexistent). From the declaration, the hypercall takes a buffer and size. So this could lead to think the NUL character is allowed in the middle of the buffer. This patch updates the console API to pass the size along the buffer down so we can remove the reliance on buffer terminating by a NUL character. Signed-off-by: Julien Grall <julien.grall@arm.com> --- This patch was originally sent standalone [1]. But the series grows to include another bug found in the console code and documentation. Change since the standalone version: - Fix early printk on Arm - Fix gdbstub - Remove unecessary leading NUL character added by Xen - Handle DomU console - Rework the commit message Below a small C program to repro the bug on Xen: int main(void) { write(1, "\r\33[0m\0\0\0\0\0\0\0\0\33[27m\33[24m\33[j\33[32mjulien\33[31m@\33[00m\33[36mjuno2-julieng:~\33[37m>", 75); write(1, "\33[K\33[32C\33[01;33m--juno2-julieng-13:44--\33[00m\33[37m\33[55D", 54); write(1, "\33[?2004h", 8); return 0; } Without this patch, the only --juno2-julieng-13:44-- will be printed in yellow. This patch was tested on Arm using serial console. I am not entirely sure whether the video and PV console is correct. I would appreciate help for testing here. [1] https://lists.xenproject.org/archives/html/xen-devel/2019-02/msg01932.html --- xen/arch/arm/early_printk.c | 5 ++-- xen/common/gdbstub.c | 6 ++-- xen/drivers/char/console.c | 58 +++++++++++++++++++-------------------- xen/drivers/char/consoled.c | 7 ++--- xen/drivers/char/serial.c | 8 ++++-- xen/drivers/char/xen_pv_console.c | 10 +++---- xen/drivers/video/lfb.c | 14 ++++++---- xen/drivers/video/lfb.h | 4 +-- xen/drivers/video/vga.c | 14 ++++++---- xen/include/xen/console.h | 2 +- xen/include/xen/early_printk.h | 2 +- xen/include/xen/pv_console.h | 4 +-- xen/include/xen/serial.h | 4 +-- xen/include/xen/video.h | 4 +-- 14 files changed, 73 insertions(+), 69 deletions(-) diff --git a/xen/arch/arm/early_printk.c b/xen/arch/arm/early_printk.c index 97466a12b1..35a47c7229 100644 --- a/xen/arch/arm/early_printk.c +++ b/xen/arch/arm/early_printk.c @@ -17,9 +17,10 @@ void early_putch(char c); void early_flush(void); -void early_puts(const char *s) +void early_puts(const char *s, unsigned int nr) { - while (*s != '\0') { + while ( nr-- > 0 ) + { if (*s == '\n') early_putch('\r'); early_putch(*s); diff --git a/xen/common/gdbstub.c b/xen/common/gdbstub.c index 07095e1ec7..08a4dda9f3 100644 --- a/xen/common/gdbstub.c +++ b/xen/common/gdbstub.c @@ -68,7 +68,7 @@ static void gdb_smp_resume(void); static char __initdata opt_gdb[30]; string_param("gdb", opt_gdb); -static void gdbstub_console_puts(const char *str); +static void gdbstub_console_puts(const char *str, unsigned int nr); /* value <-> char (de)serialzers */ static char @@ -546,14 +546,14 @@ __gdb_ctx = { static struct gdb_context *gdb_ctx = &__gdb_ctx; static void -gdbstub_console_puts(const char *str) +gdbstub_console_puts(const char *str, unsigned int nr) { const char *p; gdb_start_packet(gdb_ctx); gdb_write_to_packet_char('O', gdb_ctx); - for ( p = str; *p != '\0'; p++ ) + for ( p = str; nr > 0; p++, nr-- ) { gdb_write_to_packet_char(hex2char((*p>>4) & 0x0f), gdb_ctx ); gdb_write_to_packet_char(hex2char((*p) & 0x0f), gdb_ctx ); diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c index 9bbcb0f57a..b119bf980b 100644 --- a/xen/drivers/char/console.c +++ b/xen/drivers/char/console.c @@ -325,9 +325,9 @@ long read_console_ring(struct xen_sysctl_readconsole *op) static char serial_rx_ring[SERIAL_RX_SIZE]; static unsigned int serial_rx_cons, serial_rx_prod; -static void (*serial_steal_fn)(const char *) = early_puts; +static void (*serial_steal_fn)(const char *, unsigned int nr) = early_puts; -int console_steal(int handle, void (*fn)(const char *)) +int console_steal(int handle, void (*fn)(const char *, unsigned int nr)) { if ( (handle == -1) || (handle != sercon_handle) ) return 0; @@ -345,15 +345,15 @@ void console_giveback(int id) serial_steal_fn = NULL; } -static void sercon_puts(const char *s) +static void sercon_puts(const char *s, unsigned int nr) { if ( serial_steal_fn != NULL ) - (*serial_steal_fn)(s); + (*serial_steal_fn)(s, nr); else - serial_puts(sercon_handle, s); + serial_puts(sercon_handle, s, nr); /* Copy all serial output into PV console */ - pv_console_puts(s); + pv_console_puts(s, nr); } static void dump_console_ring_key(unsigned char key) @@ -388,8 +388,8 @@ static void dump_console_ring_key(unsigned char key) } buf[sofar] = '\0'; - sercon_puts(buf); - video_puts(buf); + sercon_puts(buf, sofar); + video_puts(buf, sofar); free_xenheap_pages(buf, order); } @@ -527,7 +527,7 @@ static inline void xen_console_write_debug_port(const char *buf, size_t len) static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, int count) { char kbuf[128]; - int kcount = 0; + unsigned int kcount = 0; struct domain *cd = current->domain; while ( count > 0 ) @@ -540,25 +540,22 @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, int count) kcount = min_t(int, count, sizeof(kbuf)-1); if ( copy_from_guest(kbuf, buffer, kcount) ) return -EFAULT; - kbuf[kcount] = '\0'; if ( is_hardware_domain(cd) ) { /* Use direct console output as it could be interactive */ spin_lock_irq(&console_lock); - sercon_puts(kbuf); - video_puts(kbuf); + sercon_puts(kbuf, kcount); + video_puts(kbuf, kcount); #ifdef CONFIG_X86 if ( opt_console_xen ) { - size_t len = strlen(kbuf); - if ( xen_guest ) - xen_hypercall_console_write(kbuf, len); + xen_hypercall_console_write(kbuf, kcount); else - xen_console_write_debug_port(kbuf, len); + xen_console_write_debug_port(kbuf, kcount); } #endif @@ -575,19 +572,20 @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, int count) char *kin = kbuf, *kout = kbuf, c; /* Strip non-printable characters */ - for ( ; ; ) + do { c = *kin++; - if ( c == '\0' || c == '\n' ) + if ( c == '\n' ) break; if ( isprint(c) || c == '\t' ) *kout++ = c; - } + } while ( --kcount > 0 ); + *kout = '\0'; spin_lock(&cd->pbuf_lock); + kcount = kin - kbuf; if ( c == '\n' ) { - kcount = kin - kbuf; cd->pbuf[cd->pbuf_idx] = '\0'; guest_printk(cd, XENLOG_G_DEBUG "%s%s\n", cd->pbuf, kbuf); cd->pbuf_idx = 0; @@ -666,16 +664,16 @@ static bool_t console_locks_busted; static void __putstr(const char *str) { + size_t len = strlen(str); + ASSERT(spin_is_locked(&console_lock)); - sercon_puts(str); - video_puts(str); + sercon_puts(str, len); + video_puts(str, len); #ifdef CONFIG_X86 if ( opt_console_xen ) { - size_t len = strlen(str); - if ( xen_guest ) xen_hypercall_console_write(str, len); else @@ -1246,6 +1244,7 @@ void debugtrace_printk(const char *fmt, ...) char cntbuf[24]; va_list args; unsigned long flags; + unsigned int nr; if ( debugtrace_bytes == 0 ) return; @@ -1257,14 +1256,15 @@ void debugtrace_printk(const char *fmt, ...) ASSERT(debugtrace_buf[debugtrace_bytes - 1] == 0); va_start(args, fmt); - vsnprintf(buf, sizeof(buf), fmt, args); + nr = vscnprintf(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); + unsigned int n = snprintf(cntbuf, sizeof(cntbuf), "%u ", ++count); + + serial_puts(sercon_handle, cntbuf, n); + serial_puts(sercon_handle, buf, nr); } else { @@ -1377,7 +1377,7 @@ void panic(const char *fmt, ...) * ************************************************************** */ -static void suspend_steal_fn(const char *str) { } +static void suspend_steal_fn(const char *str, unsigned int nr) { } static int suspend_steal_id; int console_suspend(void) diff --git a/xen/drivers/char/consoled.c b/xen/drivers/char/consoled.c index 552abf5766..222e018442 100644 --- a/xen/drivers/char/consoled.c +++ b/xen/drivers/char/consoled.c @@ -77,16 +77,13 @@ size_t consoled_guest_rx(void) if ( idx >= BUF_SZ ) { - pv_console_puts(buf); + pv_console_puts(buf, BUF_SZ); idx = 0; } } if ( idx ) - { - buf[idx] = '\0'; - pv_console_puts(buf); - } + pv_console_puts(buf, idx); /* No need for a mem barrier because every character was already consumed */ barrier(); diff --git a/xen/drivers/char/serial.c b/xen/drivers/char/serial.c index 221a14c092..7498299807 100644 --- a/xen/drivers/char/serial.c +++ b/xen/drivers/char/serial.c @@ -223,11 +223,11 @@ void serial_putc(int handle, char c) spin_unlock_irqrestore(&port->tx_lock, flags); } -void serial_puts(int handle, const char *s) +void serial_puts(int handle, const char *s, unsigned int nr) { struct serial_port *port; unsigned long flags; - char c; + unsigned int i; if ( handle == -1 ) return; @@ -238,8 +238,10 @@ void serial_puts(int handle, const char *s) spin_lock_irqsave(&port->tx_lock, flags); - while ( (c = *s++) != '\0' ) + for ( i = 0; i < nr; i++ ) { + char c = s[i]; + if ( (c == '\n') && (handle & SERHND_COOKED) ) __serial_putc(port, '\r' | ((handle & SERHND_HI) ? 0x80 : 0x00)); diff --git a/xen/drivers/char/xen_pv_console.c b/xen/drivers/char/xen_pv_console.c index cc1c1d743f..5bb303d4c8 100644 --- a/xen/drivers/char/xen_pv_console.c +++ b/xen/drivers/char/xen_pv_console.c @@ -129,13 +129,13 @@ size_t pv_console_rx(struct cpu_user_regs *regs) return recv; } -static size_t pv_ring_puts(const char *buf) +static size_t pv_ring_puts(const char *buf, unsigned int nr) { XENCONS_RING_IDX cons, prod; size_t sent = 0, avail; bool put_r = false; - while ( buf[sent] != '\0' || put_r ) + while ( sent < nr || put_r ) { cons = ACCESS_ONCE(cons_ring->out_cons); prod = cons_ring->out_prod; @@ -156,7 +156,7 @@ static size_t pv_ring_puts(const char *buf) continue; } - while ( avail && (buf[sent] != '\0' || put_r) ) + while ( avail && (sent < nr || put_r) ) { if ( put_r ) { @@ -185,7 +185,7 @@ static size_t pv_ring_puts(const char *buf) return sent; } -void pv_console_puts(const char *buf) +void pv_console_puts(const char *buf, unsigned int nr) { unsigned long flags; @@ -193,7 +193,7 @@ void pv_console_puts(const char *buf) return; spin_lock_irqsave(&tx_lock, flags); - pv_ring_puts(buf); + pv_ring_puts(buf, nr); spin_unlock_irqrestore(&tx_lock, flags); } diff --git a/xen/drivers/video/lfb.c b/xen/drivers/video/lfb.c index d0c8c492b0..93b6a33a41 100644 --- a/xen/drivers/video/lfb.c +++ b/xen/drivers/video/lfb.c @@ -59,14 +59,15 @@ static void lfb_show_line( } /* Fast mode which redraws all modified parts of a 2D text buffer. */ -void lfb_redraw_puts(const char *s) +void lfb_redraw_puts(const char *s, unsigned int nr) { unsigned int i, min_redraw_y = lfb.ypos; - char c; /* Paste characters into text buffer. */ - while ( (c = *s++) != '\0' ) + for ( i = 0; i < nr; i++ ) { + char c = s[i]; + if ( (c == '\n') || (lfb.xpos >= lfb.lfbp.text_columns) ) { if ( ++lfb.ypos >= lfb.lfbp.text_rows ) @@ -103,13 +104,14 @@ void lfb_redraw_puts(const char *s) } /* Slower line-based scroll mode which interacts better with dom0. */ -void lfb_scroll_puts(const char *s) +void lfb_scroll_puts(const char *s, unsigned int nr) { unsigned int i; - char c; - while ( (c = *s++) != '\0' ) + for ( i = 0; i < nr; i++ ) { + char c = s[i]; + if ( (c == '\n') || (lfb.xpos >= lfb.lfbp.text_columns) ) { unsigned int bytes = (lfb.lfbp.width * diff --git a/xen/drivers/video/lfb.h b/xen/drivers/video/lfb.h index ac40a66379..15599e22ef 100644 --- a/xen/drivers/video/lfb.h +++ b/xen/drivers/video/lfb.h @@ -35,8 +35,8 @@ struct lfb_prop { unsigned int text_rows; }; -void lfb_redraw_puts(const char *s); -void lfb_scroll_puts(const char *s); +void lfb_redraw_puts(const char *s, unsigned int nr); +void lfb_scroll_puts(const char *s, unsigned int nr); void lfb_carriage_return(void); void lfb_free(void); diff --git a/xen/drivers/video/vga.c b/xen/drivers/video/vga.c index 6a64fd9013..01f12aae42 100644 --- a/xen/drivers/video/vga.c +++ b/xen/drivers/video/vga.c @@ -18,9 +18,9 @@ static int vgacon_keep; static unsigned int xpos, ypos; static unsigned char *video; -static void vga_text_puts(const char *s); -static void vga_noop_puts(const char *s) {} -void (*video_puts)(const char *) = vga_noop_puts; +static void vga_text_puts(const char *s, unsigned int nr); +static void vga_noop_puts(const char *s, unsigned int nr) {} +void (*video_puts)(const char *, unsigned int nr) = vga_noop_puts; /* * 'vga=<mode-specifier>[,keep]' where <mode-specifier> is one of: @@ -177,12 +177,14 @@ void __init video_endboot(void) } } -static void vga_text_puts(const char *s) +static void vga_text_puts(const char *s, unsigned int nr) { - char c; + unsigned int i; - while ( (c = *s++) != '\0' ) + for ( i = 0; i < nr; i++ ) { + char c = s[i]; + if ( (c == '\n') || (xpos >= columns) ) { if ( ++ypos >= lines ) diff --git a/xen/include/xen/console.h b/xen/include/xen/console.h index b4f9463936..dafa53ba6b 100644 --- a/xen/include/xen/console.h +++ b/xen/include/xen/console.h @@ -38,7 +38,7 @@ struct domain *console_input_domain(void); * Steal output from the console. Returns +ve identifier, else -ve error. * Takes the handle of the serial line to steal, and steal callback function. */ -int console_steal(int handle, void (*fn)(const char *)); +int console_steal(int handle, void (*fn)(const char *, unsigned int nr)); /* Give back stolen console. Takes the identifier returned by console_steal. */ void console_giveback(int id); diff --git a/xen/include/xen/early_printk.h b/xen/include/xen/early_printk.h index 2c3e1b3519..22f8009a5f 100644 --- a/xen/include/xen/early_printk.h +++ b/xen/include/xen/early_printk.h @@ -5,7 +5,7 @@ #define __XEN_EARLY_PRINTK_H__ #ifdef CONFIG_EARLY_PRINTK -void early_puts(const char *s); +void early_puts(const char *s, unsigned int nr); #else #define early_puts NULL #endif diff --git a/xen/include/xen/pv_console.h b/xen/include/xen/pv_console.h index cb92539666..41144890e4 100644 --- a/xen/include/xen/pv_console.h +++ b/xen/include/xen/pv_console.h @@ -8,7 +8,7 @@ void pv_console_init(void); void pv_console_set_rx_handler(serial_rx_fn fn); void pv_console_init_postirq(void); -void pv_console_puts(const char *buf); +void pv_console_puts(const char *buf, unsigned int nr); size_t pv_console_rx(struct cpu_user_regs *regs); evtchn_port_t pv_console_evtchn(void); @@ -17,7 +17,7 @@ evtchn_port_t pv_console_evtchn(void); static inline void pv_console_init(void) {} static inline void pv_console_set_rx_handler(serial_rx_fn fn) { } static inline void pv_console_init_postirq(void) { } -static inline void pv_console_puts(const char *buf) { } +static inline void pv_console_puts(const char *buf, unsigned int nr) { } static inline size_t pv_console_rx(struct cpu_user_regs *regs) { return 0; } evtchn_port_t pv_console_evtchn(void) { diff --git a/xen/include/xen/serial.h b/xen/include/xen/serial.h index f2994d4093..e11d6d3ebc 100644 --- a/xen/include/xen/serial.h +++ b/xen/include/xen/serial.h @@ -114,8 +114,8 @@ int serial_parse_handle(char *conf); /* Transmit a single character via the specified COM port. */ void serial_putc(int handle, char c); -/* Transmit a NULL-terminated string via the specified COM port. */ -void serial_puts(int handle, const char *s); +/* Transmit a string via the specified COM port. */ +void serial_puts(int handle, const char *s, unsigned int nr); /* * An alternative to registering a character-receive hook. This function diff --git a/xen/include/xen/video.h b/xen/include/xen/video.h index 2e897f9df5..ddd21f374d 100644 --- a/xen/include/xen/video.h +++ b/xen/include/xen/video.h @@ -13,11 +13,11 @@ #ifdef CONFIG_VIDEO void video_init(void); -extern void (*video_puts)(const char *); +extern void (*video_puts)(const char *, unsigned int nr); void video_endboot(void); #else #define video_init() ((void)0) -#define video_puts(s) ((void)0) +#define video_puts(s, nr) ((void)0) #define video_endboot() ((void)0) #endif -- 2.11.0 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH 2/4] xen/console: Don't treat NUL character as the end of the buffer 2019-04-02 16:42 ` [PATCH 2/4] xen/console: Don't treat NUL character as the end of the buffer Julien Grall @ 2019-04-02 17:49 ` Andrew Cooper 2019-04-03 7:59 ` Jan Beulich 2019-04-05 10:00 ` [Xen-devel] " Jan Beulich 2019-04-16 20:33 ` [Xen-devel] " Stefano Stabellini 2 siblings, 1 reply; 42+ messages in thread From: Andrew Cooper @ 2019-04-02 17:49 UTC (permalink / raw) To: Julien Grall, xen-devel Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Tim Deegan, Ian Jackson, Jan Beulich On 02/04/2019 17:42, Julien Grall wrote: > diff --git a/xen/arch/arm/early_printk.c b/xen/arch/arm/early_printk.c > index 97466a12b1..35a47c7229 100644 > --- a/xen/arch/arm/early_printk.c > +++ b/xen/arch/arm/early_printk.c > @@ -17,9 +17,10 @@ > void early_putch(char c); > void early_flush(void); > > -void early_puts(const char *s) > +void early_puts(const char *s, unsigned int nr) size_t here and elsewhere please, because... > @@ -666,16 +664,16 @@ static bool_t console_locks_busted; > > static void __putstr(const char *str) > { > + size_t len = strlen(str); > + > ASSERT(spin_is_locked(&console_lock)); > > - sercon_puts(str); > - video_puts(str); > + sercon_puts(str, len); > + video_puts(str, len); ... this introduces a truncation bug for 64bit builds. I don't expect a 4G buffer to be passed, but it is not worth introducing the possibility for such a subtle bug in the first place. ~Andrew _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/4] xen/console: Don't treat NUL character as the end of the buffer 2019-04-02 17:49 ` Andrew Cooper @ 2019-04-03 7:59 ` Jan Beulich 2019-04-09 10:31 ` [Xen-devel] " Julien Grall 0 siblings, 1 reply; 42+ messages in thread From: Jan Beulich @ 2019-04-03 7:59 UTC (permalink / raw) To: Andrew Cooper Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Tim Deegan, Ian Jackson, Julien Grall, xen-devel >>> On 02.04.19 at 19:49, <andrew.cooper3@citrix.com> wrote: > On 02/04/2019 17:42, Julien Grall wrote: >> diff --git a/xen/arch/arm/early_printk.c b/xen/arch/arm/early_printk.c >> index 97466a12b1..35a47c7229 100644 >> --- a/xen/arch/arm/early_printk.c >> +++ b/xen/arch/arm/early_printk.c >> @@ -17,9 +17,10 @@ >> void early_putch(char c); >> void early_flush(void); >> >> -void early_puts(const char *s) >> +void early_puts(const char *s, unsigned int nr) > > size_t here and elsewhere please, because... > >> @@ -666,16 +664,16 @@ static bool_t console_locks_busted; >> >> static void __putstr(const char *str) >> { >> + size_t len = strlen(str); >> + >> ASSERT(spin_is_locked(&console_lock)); >> >> - sercon_puts(str); >> - video_puts(str); >> + sercon_puts(str, len); >> + video_puts(str, len); > > ... this introduces a truncation bug for 64bit builds. > > I don't expect a 4G buffer to be passed, but it is not worth introducing > the possibility for such a subtle bug in the first place. I don't entirely object to what you say, but I also don't think running into a truncation situation here would be an actual problem: Consider how long it would take to get out such a giant string, not to speak of it going to wrap our internal ring buffers many times. Looking at it from that angle, truncation may actually beneficial here; "len" may want to be unsigned int as well then. do_console_io()'s "count" is "int" as well (should really be "unsigned int" imo, just like "cmd"). I've gone through all the existing __putstr() callers: They all pass either string literals or addresses of static arrays. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/4] xen/console: Don't treat NUL character as the end of the buffer @ 2019-04-09 10:31 ` Julien Grall 0 siblings, 0 replies; 42+ messages in thread From: Julien Grall @ 2019-04-09 10:31 UTC (permalink / raw) To: Jan Beulich, Andrew Cooper Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Tim Deegan, Ian Jackson, xen-devel Hi, On 03/04/2019 08:59, Jan Beulich wrote: >>>> On 02.04.19 at 19:49, <andrew.cooper3@citrix.com> wrote: >> On 02/04/2019 17:42, Julien Grall wrote: >>> diff --git a/xen/arch/arm/early_printk.c b/xen/arch/arm/early_printk.c >>> index 97466a12b1..35a47c7229 100644 >>> --- a/xen/arch/arm/early_printk.c >>> +++ b/xen/arch/arm/early_printk.c >>> @@ -17,9 +17,10 @@ >>> void early_putch(char c); >>> void early_flush(void); >>> >>> -void early_puts(const char *s) >>> +void early_puts(const char *s, unsigned int nr) >> >> size_t here and elsewhere please, because... >> >>> @@ -666,16 +664,16 @@ static bool_t console_locks_busted; >>> >>> static void __putstr(const char *str) >>> { >>> + size_t len = strlen(str); >>> + >>> ASSERT(spin_is_locked(&console_lock)); >>> >>> - sercon_puts(str); >>> - video_puts(str); >>> + sercon_puts(str, len); >>> + video_puts(str, len); >> >> ... this introduces a truncation bug for 64bit builds. >> >> I don't expect a 4G buffer to be passed, but it is not worth introducing >> the possibility for such a subtle bug in the first place. > > I don't entirely object to what you say, but I also don't think > running into a truncation situation here would be an actual > problem: Consider how long it would take to get out such a > giant string, not to speak of it going to wrap our internal ring > buffers many times. Looking at it from that angle, truncation > may actually beneficial here; Truncation could be done explicitly in *_puts functions if necessary. So I will switch to size_t as suggested by Andrew. > "len" may want to be unsigned int > as well then. do_console_io()'s "count" is "int" as well (should > really be "unsigned int" imo, just like "cmd"). I will look at writing a patch to switch do_console_io() to unsigned int. Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Xen-devel] [PATCH 2/4] xen/console: Don't treat NUL character as the end of the buffer @ 2019-04-09 10:31 ` Julien Grall 0 siblings, 0 replies; 42+ messages in thread From: Julien Grall @ 2019-04-09 10:31 UTC (permalink / raw) To: Jan Beulich, Andrew Cooper Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Tim Deegan, Ian Jackson, xen-devel Hi, On 03/04/2019 08:59, Jan Beulich wrote: >>>> On 02.04.19 at 19:49, <andrew.cooper3@citrix.com> wrote: >> On 02/04/2019 17:42, Julien Grall wrote: >>> diff --git a/xen/arch/arm/early_printk.c b/xen/arch/arm/early_printk.c >>> index 97466a12b1..35a47c7229 100644 >>> --- a/xen/arch/arm/early_printk.c >>> +++ b/xen/arch/arm/early_printk.c >>> @@ -17,9 +17,10 @@ >>> void early_putch(char c); >>> void early_flush(void); >>> >>> -void early_puts(const char *s) >>> +void early_puts(const char *s, unsigned int nr) >> >> size_t here and elsewhere please, because... >> >>> @@ -666,16 +664,16 @@ static bool_t console_locks_busted; >>> >>> static void __putstr(const char *str) >>> { >>> + size_t len = strlen(str); >>> + >>> ASSERT(spin_is_locked(&console_lock)); >>> >>> - sercon_puts(str); >>> - video_puts(str); >>> + sercon_puts(str, len); >>> + video_puts(str, len); >> >> ... this introduces a truncation bug for 64bit builds. >> >> I don't expect a 4G buffer to be passed, but it is not worth introducing >> the possibility for such a subtle bug in the first place. > > I don't entirely object to what you say, but I also don't think > running into a truncation situation here would be an actual > problem: Consider how long it would take to get out such a > giant string, not to speak of it going to wrap our internal ring > buffers many times. Looking at it from that angle, truncation > may actually beneficial here; Truncation could be done explicitly in *_puts functions if necessary. So I will switch to size_t as suggested by Andrew. > "len" may want to be unsigned int > as well then. do_console_io()'s "count" is "int" as well (should > really be "unsigned int" imo, just like "cmd"). I will look at writing a patch to switch do_console_io() to unsigned int. Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/4] xen/console: Don't treat NUL character as the end of the buffer @ 2019-04-05 10:00 ` Jan Beulich 0 siblings, 0 replies; 42+ messages in thread From: Jan Beulich @ 2019-04-05 10:00 UTC (permalink / raw) To: Julien Grall Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, xen-devel >>> On 02.04.19 at 18:42, <julien.grall@arm.com> wrote: > @@ -345,15 +345,15 @@ void console_giveback(int id) > serial_steal_fn = NULL; > } > > -static void sercon_puts(const char *s) > +static void sercon_puts(const char *s, unsigned int nr) > { > if ( serial_steal_fn != NULL ) > - (*serial_steal_fn)(s); > + (*serial_steal_fn)(s, nr); May I ask that you take the opportunity and drop the unnecessary decoration as well? A call through a function pointer is fine to make without * and parentheses. Also a more general naming related remark: I think this and its various sibling functions are named *_puts() because of their similarity to the C library's puts(). I'm not convinced it is a good move to add a length parameter without also renaming the function to be less similar to puts(). > @@ -575,19 +572,20 @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, int count) > char *kin = kbuf, *kout = kbuf, c; > > /* Strip non-printable characters */ > - for ( ; ; ) > + do > { > c = *kin++; > - if ( c == '\0' || c == '\n' ) > + if ( c == '\n' ) > break; > if ( isprint(c) || c == '\t' ) > *kout++ = c; How does this fit with your goal of handing through nul characters? isprint('\0') pretty certainly produces false, I would think? I realize this is the DomU case, but shouldn't we try to treat this similarly? I can't really decide why the isprint() limitation here exists in the first place. In any event, if you mean to treat hwdom and DomU differently, then I think title and/or description should actually say so, and why. > - } > + } while ( --kcount > 0 ); Personally I find it odd to use "> 0" or " != 0" on variables of unsigned type. At least for the latter we indeed commonly omit it, so I think here and elsewhere the "> 0" would better be omitted in a similar fashion. > @@ -1257,14 +1256,15 @@ void debugtrace_printk(const char *fmt, ...) > ASSERT(debugtrace_buf[debugtrace_bytes - 1] == 0); > > va_start(args, fmt); > - vsnprintf(buf, sizeof(buf), fmt, args); > + nr = vscnprintf(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); > + unsigned int n = snprintf(cntbuf, sizeof(cntbuf), "%u ", ++count); To be on the safe side, perhaps better to use scnprintf() here, just like vscnprintf() gets used above? > @@ -103,13 +104,14 @@ void lfb_redraw_puts(const char *s) > } > > /* Slower line-based scroll mode which interacts better with dom0. */ > -void lfb_scroll_puts(const char *s) > +void lfb_scroll_puts(const char *s, unsigned int nr) > { > unsigned int i; > - char c; > > - while ( (c = *s++) != '\0' ) > + for ( i = 0; i < nr; i++ ) Note how i is already used in an inner loop. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Xen-devel] [PATCH 2/4] xen/console: Don't treat NUL character as the end of the buffer @ 2019-04-05 10:00 ` Jan Beulich 0 siblings, 0 replies; 42+ messages in thread From: Jan Beulich @ 2019-04-05 10:00 UTC (permalink / raw) To: Julien Grall Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, xen-devel >>> On 02.04.19 at 18:42, <julien.grall@arm.com> wrote: > @@ -345,15 +345,15 @@ void console_giveback(int id) > serial_steal_fn = NULL; > } > > -static void sercon_puts(const char *s) > +static void sercon_puts(const char *s, unsigned int nr) > { > if ( serial_steal_fn != NULL ) > - (*serial_steal_fn)(s); > + (*serial_steal_fn)(s, nr); May I ask that you take the opportunity and drop the unnecessary decoration as well? A call through a function pointer is fine to make without * and parentheses. Also a more general naming related remark: I think this and its various sibling functions are named *_puts() because of their similarity to the C library's puts(). I'm not convinced it is a good move to add a length parameter without also renaming the function to be less similar to puts(). > @@ -575,19 +572,20 @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, int count) > char *kin = kbuf, *kout = kbuf, c; > > /* Strip non-printable characters */ > - for ( ; ; ) > + do > { > c = *kin++; > - if ( c == '\0' || c == '\n' ) > + if ( c == '\n' ) > break; > if ( isprint(c) || c == '\t' ) > *kout++ = c; How does this fit with your goal of handing through nul characters? isprint('\0') pretty certainly produces false, I would think? I realize this is the DomU case, but shouldn't we try to treat this similarly? I can't really decide why the isprint() limitation here exists in the first place. In any event, if you mean to treat hwdom and DomU differently, then I think title and/or description should actually say so, and why. > - } > + } while ( --kcount > 0 ); Personally I find it odd to use "> 0" or " != 0" on variables of unsigned type. At least for the latter we indeed commonly omit it, so I think here and elsewhere the "> 0" would better be omitted in a similar fashion. > @@ -1257,14 +1256,15 @@ void debugtrace_printk(const char *fmt, ...) > ASSERT(debugtrace_buf[debugtrace_bytes - 1] == 0); > > va_start(args, fmt); > - vsnprintf(buf, sizeof(buf), fmt, args); > + nr = vscnprintf(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); > + unsigned int n = snprintf(cntbuf, sizeof(cntbuf), "%u ", ++count); To be on the safe side, perhaps better to use scnprintf() here, just like vscnprintf() gets used above? > @@ -103,13 +104,14 @@ void lfb_redraw_puts(const char *s) > } > > /* Slower line-based scroll mode which interacts better with dom0. */ > -void lfb_scroll_puts(const char *s) > +void lfb_scroll_puts(const char *s, unsigned int nr) > { > unsigned int i; > - char c; > > - while ( (c = *s++) != '\0' ) > + for ( i = 0; i < nr; i++ ) Note how i is already used in an inner loop. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/4] xen/console: Don't treat NUL character as the end of the buffer @ 2019-04-05 10:21 ` Julien Grall 0 siblings, 0 replies; 42+ messages in thread From: Julien Grall @ 2019-04-05 10:21 UTC (permalink / raw) To: Jan Beulich Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, xen-devel Hi, On 05/04/2019 11:00, Jan Beulich wrote: >>>> On 02.04.19 at 18:42, <julien.grall@arm.com> wrote: >> @@ -345,15 +345,15 @@ void console_giveback(int id) >> serial_steal_fn = NULL; >> } >> >> -static void sercon_puts(const char *s) >> +static void sercon_puts(const char *s, unsigned int nr) >> { >> if ( serial_steal_fn != NULL ) >> - (*serial_steal_fn)(s); >> + (*serial_steal_fn)(s, nr); > > May I ask that you take the opportunity and drop the unnecessary > decoration as well? A call through a function pointer is fine to make > without * and parentheses. Sure. > > Also a more general naming related remark: I think this and its > various sibling functions are named *_puts() because of their > similarity to the C library's puts(). I'm not convinced it is a good > move to add a length parameter without also renaming the function > to be less similar to puts(). It is not like our puts is similar to the C version. As suggested by the man page, we don't add a newline when printing. So I don't see any issue here to extend the number of parameters. > >> @@ -575,19 +572,20 @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, int count) >> char *kin = kbuf, *kout = kbuf, c; >> >> /* Strip non-printable characters */ >> - for ( ; ; ) >> + do >> { >> c = *kin++; >> - if ( c == '\0' || c == '\n' ) >> + if ( c == '\n' ) >> break; >> if ( isprint(c) || c == '\t' ) >> *kout++ = c; > > How does this fit with your goal of handing through nul characters? > isprint('\0') pretty certainly produces false, I would think? I realize > this is the DomU case, but shouldn't we try to treat this similarly? The point of this commit is to avoid treating NUL as end of the buffer. How NU will be treated afterwards is up to the rest of the code. In DomU, this may be added in the output buffer (see below more explanation). > I > can't really decide why the isprint() limitation here exists in the first > place. Per the discussion on the previous version, escape characters would mess up with the console if they are not properly matched. This would result to unreliable log and therefore not ideal. > > In any event, if you mean to treat hwdom and DomU differently, > then I think title and/or description should actually say so, and why. I don't see how this is treated differently. This code does exactly what commit title state. I.e the NUL does not indicate the end of the buffer anymore. So I don't see what else can be said in the commit message. Feel free to suggest it. > >> - } >> + } while ( --kcount > 0 ); > > Personally I find it odd to use "> 0" or " != 0" on variables of > unsigned type. At least for the latter we indeed commonly omit it, > so I think here and elsewhere the "> 0" would better be omitted in > a similar fashion. This makes the code just easier to read. You don't have to think whether the variable is signed or not. I am not planning to change that unless someone else think it is better to avoid "> 0". > >> @@ -1257,14 +1256,15 @@ void debugtrace_printk(const char *fmt, ...) >> ASSERT(debugtrace_buf[debugtrace_bytes - 1] == 0); >> >> va_start(args, fmt); >> - vsnprintf(buf, sizeof(buf), fmt, args); >> + nr = vscnprintf(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); >> + unsigned int n = snprintf(cntbuf, sizeof(cntbuf), "%u ", ++count); > > To be on the safe side, perhaps better to use scnprintf() here, > just like vscnprintf() gets used above? > >> @@ -103,13 +104,14 @@ void lfb_redraw_puts(const char *s) >> } >> >> /* Slower line-based scroll mode which interacts better with dom0. */ >> -void lfb_scroll_puts(const char *s) >> +void lfb_scroll_puts(const char *s, unsigned int nr) >> { >> unsigned int i; >> - char c; >> >> - while ( (c = *s++) != '\0' ) >> + for ( i = 0; i < nr; i++ ) > > Note how i is already used in an inner loop. I will have a look. > > Jan > > -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Xen-devel] [PATCH 2/4] xen/console: Don't treat NUL character as the end of the buffer @ 2019-04-05 10:21 ` Julien Grall 0 siblings, 0 replies; 42+ messages in thread From: Julien Grall @ 2019-04-05 10:21 UTC (permalink / raw) To: Jan Beulich Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, xen-devel Hi, On 05/04/2019 11:00, Jan Beulich wrote: >>>> On 02.04.19 at 18:42, <julien.grall@arm.com> wrote: >> @@ -345,15 +345,15 @@ void console_giveback(int id) >> serial_steal_fn = NULL; >> } >> >> -static void sercon_puts(const char *s) >> +static void sercon_puts(const char *s, unsigned int nr) >> { >> if ( serial_steal_fn != NULL ) >> - (*serial_steal_fn)(s); >> + (*serial_steal_fn)(s, nr); > > May I ask that you take the opportunity and drop the unnecessary > decoration as well? A call through a function pointer is fine to make > without * and parentheses. Sure. > > Also a more general naming related remark: I think this and its > various sibling functions are named *_puts() because of their > similarity to the C library's puts(). I'm not convinced it is a good > move to add a length parameter without also renaming the function > to be less similar to puts(). It is not like our puts is similar to the C version. As suggested by the man page, we don't add a newline when printing. So I don't see any issue here to extend the number of parameters. > >> @@ -575,19 +572,20 @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, int count) >> char *kin = kbuf, *kout = kbuf, c; >> >> /* Strip non-printable characters */ >> - for ( ; ; ) >> + do >> { >> c = *kin++; >> - if ( c == '\0' || c == '\n' ) >> + if ( c == '\n' ) >> break; >> if ( isprint(c) || c == '\t' ) >> *kout++ = c; > > How does this fit with your goal of handing through nul characters? > isprint('\0') pretty certainly produces false, I would think? I realize > this is the DomU case, but shouldn't we try to treat this similarly? The point of this commit is to avoid treating NUL as end of the buffer. How NU will be treated afterwards is up to the rest of the code. In DomU, this may be added in the output buffer (see below more explanation). > I > can't really decide why the isprint() limitation here exists in the first > place. Per the discussion on the previous version, escape characters would mess up with the console if they are not properly matched. This would result to unreliable log and therefore not ideal. > > In any event, if you mean to treat hwdom and DomU differently, > then I think title and/or description should actually say so, and why. I don't see how this is treated differently. This code does exactly what commit title state. I.e the NUL does not indicate the end of the buffer anymore. So I don't see what else can be said in the commit message. Feel free to suggest it. > >> - } >> + } while ( --kcount > 0 ); > > Personally I find it odd to use "> 0" or " != 0" on variables of > unsigned type. At least for the latter we indeed commonly omit it, > so I think here and elsewhere the "> 0" would better be omitted in > a similar fashion. This makes the code just easier to read. You don't have to think whether the variable is signed or not. I am not planning to change that unless someone else think it is better to avoid "> 0". > >> @@ -1257,14 +1256,15 @@ void debugtrace_printk(const char *fmt, ...) >> ASSERT(debugtrace_buf[debugtrace_bytes - 1] == 0); >> >> va_start(args, fmt); >> - vsnprintf(buf, sizeof(buf), fmt, args); >> + nr = vscnprintf(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); >> + unsigned int n = snprintf(cntbuf, sizeof(cntbuf), "%u ", ++count); > > To be on the safe side, perhaps better to use scnprintf() here, > just like vscnprintf() gets used above? > >> @@ -103,13 +104,14 @@ void lfb_redraw_puts(const char *s) >> } >> >> /* Slower line-based scroll mode which interacts better with dom0. */ >> -void lfb_scroll_puts(const char *s) >> +void lfb_scroll_puts(const char *s, unsigned int nr) >> { >> unsigned int i; >> - char c; >> >> - while ( (c = *s++) != '\0' ) >> + for ( i = 0; i < nr; i++ ) > > Note how i is already used in an inner loop. I will have a look. > > Jan > > -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/4] xen/console: Don't treat NUL character as the end of the buffer @ 2019-04-05 10:26 ` Jan Beulich 0 siblings, 0 replies; 42+ messages in thread From: Jan Beulich @ 2019-04-05 10:26 UTC (permalink / raw) To: Julien Grall Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, xen-devel >>> On 05.04.19 at 12:21, <julien.grall@arm.com> wrote: > On 05/04/2019 11:00, Jan Beulich wrote: >> In any event, if you mean to treat hwdom and DomU differently, >> then I think title and/or description should actually say so, and why. > > I don't see how this is treated differently. This code does exactly what commit > title state. I.e the NUL does not indicate the end of the buffer anymore. > > So I don't see what else can be said in the commit message. Feel free to > suggest it. Hmm, true. Please scratch that remark then. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Xen-devel] [PATCH 2/4] xen/console: Don't treat NUL character as the end of the buffer @ 2019-04-05 10:26 ` Jan Beulich 0 siblings, 0 replies; 42+ messages in thread From: Jan Beulich @ 2019-04-05 10:26 UTC (permalink / raw) To: Julien Grall Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, xen-devel >>> On 05.04.19 at 12:21, <julien.grall@arm.com> wrote: > On 05/04/2019 11:00, Jan Beulich wrote: >> In any event, if you mean to treat hwdom and DomU differently, >> then I think title and/or description should actually say so, and why. > > I don't see how this is treated differently. This code does exactly what commit > title state. I.e the NUL does not indicate the end of the buffer anymore. > > So I don't see what else can be said in the commit message. Feel free to > suggest it. Hmm, true. Please scratch that remark then. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/4] xen/console: Don't treat NUL character as the end of the buffer @ 2019-04-16 20:33 ` Stefano Stabellini 0 siblings, 0 replies; 42+ messages in thread From: Stefano Stabellini @ 2019-04-16 20:33 UTC (permalink / raw) To: Julien Grall Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Jan Beulich, xen-devel On Tue, 2 Apr 2019, Julien Grall wrote: > After upgrading Debian to Buster, I have began to notice console > mangling when using zsh in Dom0. This is happenning because output sent by > zsh to the console may contain NULs in the middle of the buffer. > > The actual implementation of CONSOLEIO_write considers that a buffer > always terminate with a NUL and therefore will ignore anything after it. > > In general, NULs are perfectly legitimate in terminal streams. For > instance, this could be used for padding slow terminals. See terminfo(5) > section `Delays and Padding`, or search for the pcre '\bpad'. > > Other use cases includes using the console for dumping non-human > readable information (e.g debugger, file if no network...). With the > current behavior, the resulting stream will end up to be corrupted. > > The documentation for CONSOLEIO_write is pretty limited (to not say > inexistent). From the declaration, the hypercall takes a buffer and size. > So this could lead to think the NUL character is allowed in the middle of > the buffer. > > This patch updates the console API to pass the size along the buffer > down so we can remove the reliance on buffer terminating by a NUL > character. > > Signed-off-by: Julien Grall <julien.grall@arm.com> > > --- > > This patch was originally sent standalone [1]. But the series grows to > include another bug found in the console code and documentation. > > Change since the standalone version: > - Fix early printk on Arm > - Fix gdbstub > - Remove unecessary leading NUL character added by Xen > - Handle DomU console > - Rework the commit message > > Below a small C program to repro the bug on Xen: > > int main(void) > { > write(1, > "\r\33[0m\0\0\0\0\0\0\0\0\33[27m\33[24m\33[j\33[32mjulien\33[31m@\33[00m\33[36mjuno2-julieng:~\33[37m>", > 75); > write(1, > "\33[K\33[32C\33[01;33m--juno2-julieng-13:44--\33[00m\33[37m\33[55D", > 54); > write(1, "\33[?2004h", 8); > > return 0; > } > > Without this patch, the only --juno2-julieng-13:44-- will be printed in > yellow. > > This patch was tested on Arm using serial console. I am not entirely > sure whether the video and PV console is correct. I would appreciate help > for testing here. > > [1] https://lists.xenproject.org/archives/html/xen-devel/2019-02/msg01932.html > --- > xen/arch/arm/early_printk.c | 5 ++-- > xen/common/gdbstub.c | 6 ++-- > xen/drivers/char/console.c | 58 +++++++++++++++++++-------------------- > xen/drivers/char/consoled.c | 7 ++--- > xen/drivers/char/serial.c | 8 ++++-- > xen/drivers/char/xen_pv_console.c | 10 +++---- > xen/drivers/video/lfb.c | 14 ++++++---- > xen/drivers/video/lfb.h | 4 +-- > xen/drivers/video/vga.c | 14 ++++++---- > xen/include/xen/console.h | 2 +- > xen/include/xen/early_printk.h | 2 +- > xen/include/xen/pv_console.h | 4 +-- > xen/include/xen/serial.h | 4 +-- > xen/include/xen/video.h | 4 +-- > 14 files changed, 73 insertions(+), 69 deletions(-) > > diff --git a/xen/arch/arm/early_printk.c b/xen/arch/arm/early_printk.c > index 97466a12b1..35a47c7229 100644 > --- a/xen/arch/arm/early_printk.c > +++ b/xen/arch/arm/early_printk.c > @@ -17,9 +17,10 @@ > void early_putch(char c); > void early_flush(void); > > -void early_puts(const char *s) > +void early_puts(const char *s, unsigned int nr) > { > - while (*s != '\0') { > + while ( nr-- > 0 ) > + { > if (*s == '\n') > early_putch('\r'); > early_putch(*s); > diff --git a/xen/common/gdbstub.c b/xen/common/gdbstub.c > index 07095e1ec7..08a4dda9f3 100644 > --- a/xen/common/gdbstub.c > +++ b/xen/common/gdbstub.c > @@ -68,7 +68,7 @@ static void gdb_smp_resume(void); > static char __initdata opt_gdb[30]; > string_param("gdb", opt_gdb); > > -static void gdbstub_console_puts(const char *str); > +static void gdbstub_console_puts(const char *str, unsigned int nr); > > /* value <-> char (de)serialzers */ > static char > @@ -546,14 +546,14 @@ __gdb_ctx = { > static struct gdb_context *gdb_ctx = &__gdb_ctx; > > static void > -gdbstub_console_puts(const char *str) > +gdbstub_console_puts(const char *str, unsigned int nr) > { > const char *p; > > gdb_start_packet(gdb_ctx); > gdb_write_to_packet_char('O', gdb_ctx); > > - for ( p = str; *p != '\0'; p++ ) > + for ( p = str; nr > 0; p++, nr-- ) > { > gdb_write_to_packet_char(hex2char((*p>>4) & 0x0f), gdb_ctx ); > gdb_write_to_packet_char(hex2char((*p) & 0x0f), gdb_ctx ); > diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c > index 9bbcb0f57a..b119bf980b 100644 > --- a/xen/drivers/char/console.c > +++ b/xen/drivers/char/console.c > @@ -325,9 +325,9 @@ long read_console_ring(struct xen_sysctl_readconsole *op) > static char serial_rx_ring[SERIAL_RX_SIZE]; > static unsigned int serial_rx_cons, serial_rx_prod; > > -static void (*serial_steal_fn)(const char *) = early_puts; > +static void (*serial_steal_fn)(const char *, unsigned int nr) = early_puts; > > -int console_steal(int handle, void (*fn)(const char *)) > +int console_steal(int handle, void (*fn)(const char *, unsigned int nr)) > { > if ( (handle == -1) || (handle != sercon_handle) ) > return 0; > @@ -345,15 +345,15 @@ void console_giveback(int id) > serial_steal_fn = NULL; > } > > -static void sercon_puts(const char *s) > +static void sercon_puts(const char *s, unsigned int nr) > { > if ( serial_steal_fn != NULL ) > - (*serial_steal_fn)(s); > + (*serial_steal_fn)(s, nr); > else > - serial_puts(sercon_handle, s); > + serial_puts(sercon_handle, s, nr); > > /* Copy all serial output into PV console */ > - pv_console_puts(s); > + pv_console_puts(s, nr); > } > > static void dump_console_ring_key(unsigned char key) > @@ -388,8 +388,8 @@ static void dump_console_ring_key(unsigned char key) > } > buf[sofar] = '\0'; > > - sercon_puts(buf); > - video_puts(buf); > + sercon_puts(buf, sofar); > + video_puts(buf, sofar); > > free_xenheap_pages(buf, order); > } > @@ -527,7 +527,7 @@ static inline void xen_console_write_debug_port(const char *buf, size_t len) > static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, int count) > { > char kbuf[128]; > - int kcount = 0; > + unsigned int kcount = 0; > struct domain *cd = current->domain; > > while ( count > 0 ) > @@ -540,25 +540,22 @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, int count) > kcount = min_t(int, count, sizeof(kbuf)-1); > if ( copy_from_guest(kbuf, buffer, kcount) ) > return -EFAULT; > - kbuf[kcount] = '\0'; > > if ( is_hardware_domain(cd) ) > { > /* Use direct console output as it could be interactive */ > spin_lock_irq(&console_lock); > > - sercon_puts(kbuf); > - video_puts(kbuf); > + sercon_puts(kbuf, kcount); > + video_puts(kbuf, kcount); > > #ifdef CONFIG_X86 > if ( opt_console_xen ) > { > - size_t len = strlen(kbuf); > - > if ( xen_guest ) > - xen_hypercall_console_write(kbuf, len); > + xen_hypercall_console_write(kbuf, kcount); > else > - xen_console_write_debug_port(kbuf, len); > + xen_console_write_debug_port(kbuf, kcount); > } > #endif > > @@ -575,19 +572,20 @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, int count) > char *kin = kbuf, *kout = kbuf, c; > > /* Strip non-printable characters */ > - for ( ; ; ) > + do > { > c = *kin++; > - if ( c == '\0' || c == '\n' ) > + if ( c == '\n' ) > break; > if ( isprint(c) || c == '\t' ) > *kout++ = c; > - } > + } while ( --kcount > 0 ); Why not while ( kcount-- > 0 ) { XXX } like you did in early_puts? Anyway this should be just style, so up to you. I read the patch and I couldn't find anything beyond what Jan already mentioned. > *kout = '\0'; > spin_lock(&cd->pbuf_lock); > + kcount = kin - kbuf; > if ( c == '\n' ) > { > - kcount = kin - kbuf; > cd->pbuf[cd->pbuf_idx] = '\0'; > guest_printk(cd, XENLOG_G_DEBUG "%s%s\n", cd->pbuf, kbuf); > cd->pbuf_idx = 0; > @@ -666,16 +664,16 @@ static bool_t console_locks_busted; > > static void __putstr(const char *str) > { > + size_t len = strlen(str); > > ASSERT(spin_is_locked(&console_lock)); > > - sercon_puts(str); > - video_puts(str); > + sercon_puts(str, len); > + video_puts(str, len); > > #ifdef CONFIG_X86 > if ( opt_console_xen ) > { > - size_t len = strlen(str); > - > if ( xen_guest ) > xen_hypercall_console_write(str, len); > else > @@ -1246,6 +1244,7 @@ void debugtrace_printk(const char *fmt, ...) > char cntbuf[24]; > va_list args; > unsigned long flags; > + unsigned int nr; > > if ( debugtrace_bytes == 0 ) > return; > @@ -1257,14 +1256,15 @@ void debugtrace_printk(const char *fmt, ...) > ASSERT(debugtrace_buf[debugtrace_bytes - 1] == 0); > > va_start(args, fmt); > - vsnprintf(buf, sizeof(buf), fmt, args); > + nr = vscnprintf(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); > + unsigned int n = snprintf(cntbuf, sizeof(cntbuf), "%u ", ++count); > + > + serial_puts(sercon_handle, cntbuf, n); > + serial_puts(sercon_handle, buf, nr); > } > else > { > @@ -1377,7 +1377,7 @@ void panic(const char *fmt, ...) > * ************************************************************** > */ > > -static void suspend_steal_fn(const char *str) { } > +static void suspend_steal_fn(const char *str, unsigned int nr) { } > static int suspend_steal_id; > > int console_suspend(void) > diff --git a/xen/drivers/char/consoled.c b/xen/drivers/char/consoled.c > index 552abf5766..222e018442 100644 > --- a/xen/drivers/char/consoled.c > +++ b/xen/drivers/char/consoled.c > @@ -77,16 +77,13 @@ size_t consoled_guest_rx(void) > > if ( idx >= BUF_SZ ) > { > - pv_console_puts(buf); > + pv_console_puts(buf, BUF_SZ); > idx = 0; > } > } > > if ( idx ) > - { > - buf[idx] = '\0'; > - pv_console_puts(buf); > - } > + pv_console_puts(buf, idx); > > /* No need for a mem barrier because every character was already consumed */ > barrier(); > diff --git a/xen/drivers/char/serial.c b/xen/drivers/char/serial.c > index 221a14c092..7498299807 100644 > --- a/xen/drivers/char/serial.c > +++ b/xen/drivers/char/serial.c > @@ -223,11 +223,11 @@ void serial_putc(int handle, char c) > spin_unlock_irqrestore(&port->tx_lock, flags); > } > > -void serial_puts(int handle, const char *s) > +void serial_puts(int handle, const char *s, unsigned int nr) > { > struct serial_port *port; > unsigned long flags; > - char c; > + unsigned int i; > > if ( handle == -1 ) > return; > @@ -238,8 +238,10 @@ void serial_puts(int handle, const char *s) > > spin_lock_irqsave(&port->tx_lock, flags); > > - while ( (c = *s++) != '\0' ) > + for ( i = 0; i < nr; i++ ) > { > + char c = s[i]; > + > if ( (c == '\n') && (handle & SERHND_COOKED) ) > __serial_putc(port, '\r' | ((handle & SERHND_HI) ? 0x80 : 0x00)); > > diff --git a/xen/drivers/char/xen_pv_console.c b/xen/drivers/char/xen_pv_console.c > index cc1c1d743f..5bb303d4c8 100644 > --- a/xen/drivers/char/xen_pv_console.c > +++ b/xen/drivers/char/xen_pv_console.c > @@ -129,13 +129,13 @@ size_t pv_console_rx(struct cpu_user_regs *regs) > return recv; > } > > -static size_t pv_ring_puts(const char *buf) > +static size_t pv_ring_puts(const char *buf, unsigned int nr) > { > XENCONS_RING_IDX cons, prod; > size_t sent = 0, avail; > bool put_r = false; > > - while ( buf[sent] != '\0' || put_r ) > + while ( sent < nr || put_r ) > { > cons = ACCESS_ONCE(cons_ring->out_cons); > prod = cons_ring->out_prod; > @@ -156,7 +156,7 @@ static size_t pv_ring_puts(const char *buf) > continue; > } > > - while ( avail && (buf[sent] != '\0' || put_r) ) > + while ( avail && (sent < nr || put_r) ) > { > if ( put_r ) > { > @@ -185,7 +185,7 @@ static size_t pv_ring_puts(const char *buf) > return sent; > } > > -void pv_console_puts(const char *buf) > +void pv_console_puts(const char *buf, unsigned int nr) > { > unsigned long flags; > > @@ -193,7 +193,7 @@ void pv_console_puts(const char *buf) > return; > > spin_lock_irqsave(&tx_lock, flags); > - pv_ring_puts(buf); > + pv_ring_puts(buf, nr); > spin_unlock_irqrestore(&tx_lock, flags); > } > > diff --git a/xen/drivers/video/lfb.c b/xen/drivers/video/lfb.c > index d0c8c492b0..93b6a33a41 100644 > --- a/xen/drivers/video/lfb.c > +++ b/xen/drivers/video/lfb.c > @@ -59,14 +59,15 @@ static void lfb_show_line( > } > > /* Fast mode which redraws all modified parts of a 2D text buffer. */ > -void lfb_redraw_puts(const char *s) > +void lfb_redraw_puts(const char *s, unsigned int nr) > { > unsigned int i, min_redraw_y = lfb.ypos; > - char c; > > /* Paste characters into text buffer. */ > - while ( (c = *s++) != '\0' ) > + for ( i = 0; i < nr; i++ ) > { > + char c = s[i]; > + > if ( (c == '\n') || (lfb.xpos >= lfb.lfbp.text_columns) ) > { > if ( ++lfb.ypos >= lfb.lfbp.text_rows ) > @@ -103,13 +104,14 @@ void lfb_redraw_puts(const char *s) > } > > /* Slower line-based scroll mode which interacts better with dom0. */ > -void lfb_scroll_puts(const char *s) > +void lfb_scroll_puts(const char *s, unsigned int nr) > { > unsigned int i; > - char c; > > - while ( (c = *s++) != '\0' ) > + for ( i = 0; i < nr; i++ ) > { > + char c = s[i]; > + > if ( (c == '\n') || (lfb.xpos >= lfb.lfbp.text_columns) ) > { > unsigned int bytes = (lfb.lfbp.width * > diff --git a/xen/drivers/video/lfb.h b/xen/drivers/video/lfb.h > index ac40a66379..15599e22ef 100644 > --- a/xen/drivers/video/lfb.h > +++ b/xen/drivers/video/lfb.h > @@ -35,8 +35,8 @@ struct lfb_prop { > unsigned int text_rows; > }; > > -void lfb_redraw_puts(const char *s); > -void lfb_scroll_puts(const char *s); > +void lfb_redraw_puts(const char *s, unsigned int nr); > +void lfb_scroll_puts(const char *s, unsigned int nr); > void lfb_carriage_return(void); > void lfb_free(void); > > diff --git a/xen/drivers/video/vga.c b/xen/drivers/video/vga.c > index 6a64fd9013..01f12aae42 100644 > --- a/xen/drivers/video/vga.c > +++ b/xen/drivers/video/vga.c > @@ -18,9 +18,9 @@ static int vgacon_keep; > static unsigned int xpos, ypos; > static unsigned char *video; > > -static void vga_text_puts(const char *s); > -static void vga_noop_puts(const char *s) {} > -void (*video_puts)(const char *) = vga_noop_puts; > +static void vga_text_puts(const char *s, unsigned int nr); > +static void vga_noop_puts(const char *s, unsigned int nr) {} > +void (*video_puts)(const char *, unsigned int nr) = vga_noop_puts; > > /* > * 'vga=<mode-specifier>[,keep]' where <mode-specifier> is one of: > @@ -177,12 +177,14 @@ void __init video_endboot(void) > } > } > > -static void vga_text_puts(const char *s) > +static void vga_text_puts(const char *s, unsigned int nr) > { > - char c; > + unsigned int i; > > - while ( (c = *s++) != '\0' ) > + for ( i = 0; i < nr; i++ ) > { > + char c = s[i]; > + > if ( (c == '\n') || (xpos >= columns) ) > { > if ( ++ypos >= lines ) > diff --git a/xen/include/xen/console.h b/xen/include/xen/console.h > index b4f9463936..dafa53ba6b 100644 > --- a/xen/include/xen/console.h > +++ b/xen/include/xen/console.h > @@ -38,7 +38,7 @@ struct domain *console_input_domain(void); > * Steal output from the console. Returns +ve identifier, else -ve error. > * Takes the handle of the serial line to steal, and steal callback function. > */ > -int console_steal(int handle, void (*fn)(const char *)); > +int console_steal(int handle, void (*fn)(const char *, unsigned int nr)); > > /* Give back stolen console. Takes the identifier returned by console_steal. */ > void console_giveback(int id); > diff --git a/xen/include/xen/early_printk.h b/xen/include/xen/early_printk.h > index 2c3e1b3519..22f8009a5f 100644 > --- a/xen/include/xen/early_printk.h > +++ b/xen/include/xen/early_printk.h > @@ -5,7 +5,7 @@ > #define __XEN_EARLY_PRINTK_H__ > > #ifdef CONFIG_EARLY_PRINTK > -void early_puts(const char *s); > +void early_puts(const char *s, unsigned int nr); > #else > #define early_puts NULL > #endif > diff --git a/xen/include/xen/pv_console.h b/xen/include/xen/pv_console.h > index cb92539666..41144890e4 100644 > --- a/xen/include/xen/pv_console.h > +++ b/xen/include/xen/pv_console.h > @@ -8,7 +8,7 @@ > void pv_console_init(void); > void pv_console_set_rx_handler(serial_rx_fn fn); > void pv_console_init_postirq(void); > -void pv_console_puts(const char *buf); > +void pv_console_puts(const char *buf, unsigned int nr); > size_t pv_console_rx(struct cpu_user_regs *regs); > evtchn_port_t pv_console_evtchn(void); > > @@ -17,7 +17,7 @@ evtchn_port_t pv_console_evtchn(void); > static inline void pv_console_init(void) {} > static inline void pv_console_set_rx_handler(serial_rx_fn fn) { } > static inline void pv_console_init_postirq(void) { } > -static inline void pv_console_puts(const char *buf) { } > +static inline void pv_console_puts(const char *buf, unsigned int nr) { } > static inline size_t pv_console_rx(struct cpu_user_regs *regs) { return 0; } > evtchn_port_t pv_console_evtchn(void) > { > diff --git a/xen/include/xen/serial.h b/xen/include/xen/serial.h > index f2994d4093..e11d6d3ebc 100644 > --- a/xen/include/xen/serial.h > +++ b/xen/include/xen/serial.h > @@ -114,8 +114,8 @@ int serial_parse_handle(char *conf); > /* Transmit a single character via the specified COM port. */ > void serial_putc(int handle, char c); > > -/* Transmit a NULL-terminated string via the specified COM port. */ > -void serial_puts(int handle, const char *s); > +/* Transmit a string via the specified COM port. */ > +void serial_puts(int handle, const char *s, unsigned int nr); > > /* > * An alternative to registering a character-receive hook. This function > diff --git a/xen/include/xen/video.h b/xen/include/xen/video.h > index 2e897f9df5..ddd21f374d 100644 > --- a/xen/include/xen/video.h > +++ b/xen/include/xen/video.h > @@ -13,11 +13,11 @@ > > #ifdef CONFIG_VIDEO > void video_init(void); > -extern void (*video_puts)(const char *); > +extern void (*video_puts)(const char *, unsigned int nr); > void video_endboot(void); > #else > #define video_init() ((void)0) > -#define video_puts(s) ((void)0) > +#define video_puts(s, nr) ((void)0) > #define video_endboot() ((void)0) > #endif > > -- > 2.11.0 > _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Xen-devel] [PATCH 2/4] xen/console: Don't treat NUL character as the end of the buffer @ 2019-04-16 20:33 ` Stefano Stabellini 0 siblings, 0 replies; 42+ messages in thread From: Stefano Stabellini @ 2019-04-16 20:33 UTC (permalink / raw) To: Julien Grall Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Jan Beulich, xen-devel On Tue, 2 Apr 2019, Julien Grall wrote: > After upgrading Debian to Buster, I have began to notice console > mangling when using zsh in Dom0. This is happenning because output sent by > zsh to the console may contain NULs in the middle of the buffer. > > The actual implementation of CONSOLEIO_write considers that a buffer > always terminate with a NUL and therefore will ignore anything after it. > > In general, NULs are perfectly legitimate in terminal streams. For > instance, this could be used for padding slow terminals. See terminfo(5) > section `Delays and Padding`, or search for the pcre '\bpad'. > > Other use cases includes using the console for dumping non-human > readable information (e.g debugger, file if no network...). With the > current behavior, the resulting stream will end up to be corrupted. > > The documentation for CONSOLEIO_write is pretty limited (to not say > inexistent). From the declaration, the hypercall takes a buffer and size. > So this could lead to think the NUL character is allowed in the middle of > the buffer. > > This patch updates the console API to pass the size along the buffer > down so we can remove the reliance on buffer terminating by a NUL > character. > > Signed-off-by: Julien Grall <julien.grall@arm.com> > > --- > > This patch was originally sent standalone [1]. But the series grows to > include another bug found in the console code and documentation. > > Change since the standalone version: > - Fix early printk on Arm > - Fix gdbstub > - Remove unecessary leading NUL character added by Xen > - Handle DomU console > - Rework the commit message > > Below a small C program to repro the bug on Xen: > > int main(void) > { > write(1, > "\r\33[0m\0\0\0\0\0\0\0\0\33[27m\33[24m\33[j\33[32mjulien\33[31m@\33[00m\33[36mjuno2-julieng:~\33[37m>", > 75); > write(1, > "\33[K\33[32C\33[01;33m--juno2-julieng-13:44--\33[00m\33[37m\33[55D", > 54); > write(1, "\33[?2004h", 8); > > return 0; > } > > Without this patch, the only --juno2-julieng-13:44-- will be printed in > yellow. > > This patch was tested on Arm using serial console. I am not entirely > sure whether the video and PV console is correct. I would appreciate help > for testing here. > > [1] https://lists.xenproject.org/archives/html/xen-devel/2019-02/msg01932.html > --- > xen/arch/arm/early_printk.c | 5 ++-- > xen/common/gdbstub.c | 6 ++-- > xen/drivers/char/console.c | 58 +++++++++++++++++++-------------------- > xen/drivers/char/consoled.c | 7 ++--- > xen/drivers/char/serial.c | 8 ++++-- > xen/drivers/char/xen_pv_console.c | 10 +++---- > xen/drivers/video/lfb.c | 14 ++++++---- > xen/drivers/video/lfb.h | 4 +-- > xen/drivers/video/vga.c | 14 ++++++---- > xen/include/xen/console.h | 2 +- > xen/include/xen/early_printk.h | 2 +- > xen/include/xen/pv_console.h | 4 +-- > xen/include/xen/serial.h | 4 +-- > xen/include/xen/video.h | 4 +-- > 14 files changed, 73 insertions(+), 69 deletions(-) > > diff --git a/xen/arch/arm/early_printk.c b/xen/arch/arm/early_printk.c > index 97466a12b1..35a47c7229 100644 > --- a/xen/arch/arm/early_printk.c > +++ b/xen/arch/arm/early_printk.c > @@ -17,9 +17,10 @@ > void early_putch(char c); > void early_flush(void); > > -void early_puts(const char *s) > +void early_puts(const char *s, unsigned int nr) > { > - while (*s != '\0') { > + while ( nr-- > 0 ) > + { > if (*s == '\n') > early_putch('\r'); > early_putch(*s); > diff --git a/xen/common/gdbstub.c b/xen/common/gdbstub.c > index 07095e1ec7..08a4dda9f3 100644 > --- a/xen/common/gdbstub.c > +++ b/xen/common/gdbstub.c > @@ -68,7 +68,7 @@ static void gdb_smp_resume(void); > static char __initdata opt_gdb[30]; > string_param("gdb", opt_gdb); > > -static void gdbstub_console_puts(const char *str); > +static void gdbstub_console_puts(const char *str, unsigned int nr); > > /* value <-> char (de)serialzers */ > static char > @@ -546,14 +546,14 @@ __gdb_ctx = { > static struct gdb_context *gdb_ctx = &__gdb_ctx; > > static void > -gdbstub_console_puts(const char *str) > +gdbstub_console_puts(const char *str, unsigned int nr) > { > const char *p; > > gdb_start_packet(gdb_ctx); > gdb_write_to_packet_char('O', gdb_ctx); > > - for ( p = str; *p != '\0'; p++ ) > + for ( p = str; nr > 0; p++, nr-- ) > { > gdb_write_to_packet_char(hex2char((*p>>4) & 0x0f), gdb_ctx ); > gdb_write_to_packet_char(hex2char((*p) & 0x0f), gdb_ctx ); > diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c > index 9bbcb0f57a..b119bf980b 100644 > --- a/xen/drivers/char/console.c > +++ b/xen/drivers/char/console.c > @@ -325,9 +325,9 @@ long read_console_ring(struct xen_sysctl_readconsole *op) > static char serial_rx_ring[SERIAL_RX_SIZE]; > static unsigned int serial_rx_cons, serial_rx_prod; > > -static void (*serial_steal_fn)(const char *) = early_puts; > +static void (*serial_steal_fn)(const char *, unsigned int nr) = early_puts; > > -int console_steal(int handle, void (*fn)(const char *)) > +int console_steal(int handle, void (*fn)(const char *, unsigned int nr)) > { > if ( (handle == -1) || (handle != sercon_handle) ) > return 0; > @@ -345,15 +345,15 @@ void console_giveback(int id) > serial_steal_fn = NULL; > } > > -static void sercon_puts(const char *s) > +static void sercon_puts(const char *s, unsigned int nr) > { > if ( serial_steal_fn != NULL ) > - (*serial_steal_fn)(s); > + (*serial_steal_fn)(s, nr); > else > - serial_puts(sercon_handle, s); > + serial_puts(sercon_handle, s, nr); > > /* Copy all serial output into PV console */ > - pv_console_puts(s); > + pv_console_puts(s, nr); > } > > static void dump_console_ring_key(unsigned char key) > @@ -388,8 +388,8 @@ static void dump_console_ring_key(unsigned char key) > } > buf[sofar] = '\0'; > > - sercon_puts(buf); > - video_puts(buf); > + sercon_puts(buf, sofar); > + video_puts(buf, sofar); > > free_xenheap_pages(buf, order); > } > @@ -527,7 +527,7 @@ static inline void xen_console_write_debug_port(const char *buf, size_t len) > static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, int count) > { > char kbuf[128]; > - int kcount = 0; > + unsigned int kcount = 0; > struct domain *cd = current->domain; > > while ( count > 0 ) > @@ -540,25 +540,22 @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, int count) > kcount = min_t(int, count, sizeof(kbuf)-1); > if ( copy_from_guest(kbuf, buffer, kcount) ) > return -EFAULT; > - kbuf[kcount] = '\0'; > > if ( is_hardware_domain(cd) ) > { > /* Use direct console output as it could be interactive */ > spin_lock_irq(&console_lock); > > - sercon_puts(kbuf); > - video_puts(kbuf); > + sercon_puts(kbuf, kcount); > + video_puts(kbuf, kcount); > > #ifdef CONFIG_X86 > if ( opt_console_xen ) > { > - size_t len = strlen(kbuf); > - > if ( xen_guest ) > - xen_hypercall_console_write(kbuf, len); > + xen_hypercall_console_write(kbuf, kcount); > else > - xen_console_write_debug_port(kbuf, len); > + xen_console_write_debug_port(kbuf, kcount); > } > #endif > > @@ -575,19 +572,20 @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, int count) > char *kin = kbuf, *kout = kbuf, c; > > /* Strip non-printable characters */ > - for ( ; ; ) > + do > { > c = *kin++; > - if ( c == '\0' || c == '\n' ) > + if ( c == '\n' ) > break; > if ( isprint(c) || c == '\t' ) > *kout++ = c; > - } > + } while ( --kcount > 0 ); Why not while ( kcount-- > 0 ) { XXX } like you did in early_puts? Anyway this should be just style, so up to you. I read the patch and I couldn't find anything beyond what Jan already mentioned. > *kout = '\0'; > spin_lock(&cd->pbuf_lock); > + kcount = kin - kbuf; > if ( c == '\n' ) > { > - kcount = kin - kbuf; > cd->pbuf[cd->pbuf_idx] = '\0'; > guest_printk(cd, XENLOG_G_DEBUG "%s%s\n", cd->pbuf, kbuf); > cd->pbuf_idx = 0; > @@ -666,16 +664,16 @@ static bool_t console_locks_busted; > > static void __putstr(const char *str) > { > + size_t len = strlen(str); > > ASSERT(spin_is_locked(&console_lock)); > > - sercon_puts(str); > - video_puts(str); > + sercon_puts(str, len); > + video_puts(str, len); > > #ifdef CONFIG_X86 > if ( opt_console_xen ) > { > - size_t len = strlen(str); > - > if ( xen_guest ) > xen_hypercall_console_write(str, len); > else > @@ -1246,6 +1244,7 @@ void debugtrace_printk(const char *fmt, ...) > char cntbuf[24]; > va_list args; > unsigned long flags; > + unsigned int nr; > > if ( debugtrace_bytes == 0 ) > return; > @@ -1257,14 +1256,15 @@ void debugtrace_printk(const char *fmt, ...) > ASSERT(debugtrace_buf[debugtrace_bytes - 1] == 0); > > va_start(args, fmt); > - vsnprintf(buf, sizeof(buf), fmt, args); > + nr = vscnprintf(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); > + unsigned int n = snprintf(cntbuf, sizeof(cntbuf), "%u ", ++count); > + > + serial_puts(sercon_handle, cntbuf, n); > + serial_puts(sercon_handle, buf, nr); > } > else > { > @@ -1377,7 +1377,7 @@ void panic(const char *fmt, ...) > * ************************************************************** > */ > > -static void suspend_steal_fn(const char *str) { } > +static void suspend_steal_fn(const char *str, unsigned int nr) { } > static int suspend_steal_id; > > int console_suspend(void) > diff --git a/xen/drivers/char/consoled.c b/xen/drivers/char/consoled.c > index 552abf5766..222e018442 100644 > --- a/xen/drivers/char/consoled.c > +++ b/xen/drivers/char/consoled.c > @@ -77,16 +77,13 @@ size_t consoled_guest_rx(void) > > if ( idx >= BUF_SZ ) > { > - pv_console_puts(buf); > + pv_console_puts(buf, BUF_SZ); > idx = 0; > } > } > > if ( idx ) > - { > - buf[idx] = '\0'; > - pv_console_puts(buf); > - } > + pv_console_puts(buf, idx); > > /* No need for a mem barrier because every character was already consumed */ > barrier(); > diff --git a/xen/drivers/char/serial.c b/xen/drivers/char/serial.c > index 221a14c092..7498299807 100644 > --- a/xen/drivers/char/serial.c > +++ b/xen/drivers/char/serial.c > @@ -223,11 +223,11 @@ void serial_putc(int handle, char c) > spin_unlock_irqrestore(&port->tx_lock, flags); > } > > -void serial_puts(int handle, const char *s) > +void serial_puts(int handle, const char *s, unsigned int nr) > { > struct serial_port *port; > unsigned long flags; > - char c; > + unsigned int i; > > if ( handle == -1 ) > return; > @@ -238,8 +238,10 @@ void serial_puts(int handle, const char *s) > > spin_lock_irqsave(&port->tx_lock, flags); > > - while ( (c = *s++) != '\0' ) > + for ( i = 0; i < nr; i++ ) > { > + char c = s[i]; > + > if ( (c == '\n') && (handle & SERHND_COOKED) ) > __serial_putc(port, '\r' | ((handle & SERHND_HI) ? 0x80 : 0x00)); > > diff --git a/xen/drivers/char/xen_pv_console.c b/xen/drivers/char/xen_pv_console.c > index cc1c1d743f..5bb303d4c8 100644 > --- a/xen/drivers/char/xen_pv_console.c > +++ b/xen/drivers/char/xen_pv_console.c > @@ -129,13 +129,13 @@ size_t pv_console_rx(struct cpu_user_regs *regs) > return recv; > } > > -static size_t pv_ring_puts(const char *buf) > +static size_t pv_ring_puts(const char *buf, unsigned int nr) > { > XENCONS_RING_IDX cons, prod; > size_t sent = 0, avail; > bool put_r = false; > > - while ( buf[sent] != '\0' || put_r ) > + while ( sent < nr || put_r ) > { > cons = ACCESS_ONCE(cons_ring->out_cons); > prod = cons_ring->out_prod; > @@ -156,7 +156,7 @@ static size_t pv_ring_puts(const char *buf) > continue; > } > > - while ( avail && (buf[sent] != '\0' || put_r) ) > + while ( avail && (sent < nr || put_r) ) > { > if ( put_r ) > { > @@ -185,7 +185,7 @@ static size_t pv_ring_puts(const char *buf) > return sent; > } > > -void pv_console_puts(const char *buf) > +void pv_console_puts(const char *buf, unsigned int nr) > { > unsigned long flags; > > @@ -193,7 +193,7 @@ void pv_console_puts(const char *buf) > return; > > spin_lock_irqsave(&tx_lock, flags); > - pv_ring_puts(buf); > + pv_ring_puts(buf, nr); > spin_unlock_irqrestore(&tx_lock, flags); > } > > diff --git a/xen/drivers/video/lfb.c b/xen/drivers/video/lfb.c > index d0c8c492b0..93b6a33a41 100644 > --- a/xen/drivers/video/lfb.c > +++ b/xen/drivers/video/lfb.c > @@ -59,14 +59,15 @@ static void lfb_show_line( > } > > /* Fast mode which redraws all modified parts of a 2D text buffer. */ > -void lfb_redraw_puts(const char *s) > +void lfb_redraw_puts(const char *s, unsigned int nr) > { > unsigned int i, min_redraw_y = lfb.ypos; > - char c; > > /* Paste characters into text buffer. */ > - while ( (c = *s++) != '\0' ) > + for ( i = 0; i < nr; i++ ) > { > + char c = s[i]; > + > if ( (c == '\n') || (lfb.xpos >= lfb.lfbp.text_columns) ) > { > if ( ++lfb.ypos >= lfb.lfbp.text_rows ) > @@ -103,13 +104,14 @@ void lfb_redraw_puts(const char *s) > } > > /* Slower line-based scroll mode which interacts better with dom0. */ > -void lfb_scroll_puts(const char *s) > +void lfb_scroll_puts(const char *s, unsigned int nr) > { > unsigned int i; > - char c; > > - while ( (c = *s++) != '\0' ) > + for ( i = 0; i < nr; i++ ) > { > + char c = s[i]; > + > if ( (c == '\n') || (lfb.xpos >= lfb.lfbp.text_columns) ) > { > unsigned int bytes = (lfb.lfbp.width * > diff --git a/xen/drivers/video/lfb.h b/xen/drivers/video/lfb.h > index ac40a66379..15599e22ef 100644 > --- a/xen/drivers/video/lfb.h > +++ b/xen/drivers/video/lfb.h > @@ -35,8 +35,8 @@ struct lfb_prop { > unsigned int text_rows; > }; > > -void lfb_redraw_puts(const char *s); > -void lfb_scroll_puts(const char *s); > +void lfb_redraw_puts(const char *s, unsigned int nr); > +void lfb_scroll_puts(const char *s, unsigned int nr); > void lfb_carriage_return(void); > void lfb_free(void); > > diff --git a/xen/drivers/video/vga.c b/xen/drivers/video/vga.c > index 6a64fd9013..01f12aae42 100644 > --- a/xen/drivers/video/vga.c > +++ b/xen/drivers/video/vga.c > @@ -18,9 +18,9 @@ static int vgacon_keep; > static unsigned int xpos, ypos; > static unsigned char *video; > > -static void vga_text_puts(const char *s); > -static void vga_noop_puts(const char *s) {} > -void (*video_puts)(const char *) = vga_noop_puts; > +static void vga_text_puts(const char *s, unsigned int nr); > +static void vga_noop_puts(const char *s, unsigned int nr) {} > +void (*video_puts)(const char *, unsigned int nr) = vga_noop_puts; > > /* > * 'vga=<mode-specifier>[,keep]' where <mode-specifier> is one of: > @@ -177,12 +177,14 @@ void __init video_endboot(void) > } > } > > -static void vga_text_puts(const char *s) > +static void vga_text_puts(const char *s, unsigned int nr) > { > - char c; > + unsigned int i; > > - while ( (c = *s++) != '\0' ) > + for ( i = 0; i < nr; i++ ) > { > + char c = s[i]; > + > if ( (c == '\n') || (xpos >= columns) ) > { > if ( ++ypos >= lines ) > diff --git a/xen/include/xen/console.h b/xen/include/xen/console.h > index b4f9463936..dafa53ba6b 100644 > --- a/xen/include/xen/console.h > +++ b/xen/include/xen/console.h > @@ -38,7 +38,7 @@ struct domain *console_input_domain(void); > * Steal output from the console. Returns +ve identifier, else -ve error. > * Takes the handle of the serial line to steal, and steal callback function. > */ > -int console_steal(int handle, void (*fn)(const char *)); > +int console_steal(int handle, void (*fn)(const char *, unsigned int nr)); > > /* Give back stolen console. Takes the identifier returned by console_steal. */ > void console_giveback(int id); > diff --git a/xen/include/xen/early_printk.h b/xen/include/xen/early_printk.h > index 2c3e1b3519..22f8009a5f 100644 > --- a/xen/include/xen/early_printk.h > +++ b/xen/include/xen/early_printk.h > @@ -5,7 +5,7 @@ > #define __XEN_EARLY_PRINTK_H__ > > #ifdef CONFIG_EARLY_PRINTK > -void early_puts(const char *s); > +void early_puts(const char *s, unsigned int nr); > #else > #define early_puts NULL > #endif > diff --git a/xen/include/xen/pv_console.h b/xen/include/xen/pv_console.h > index cb92539666..41144890e4 100644 > --- a/xen/include/xen/pv_console.h > +++ b/xen/include/xen/pv_console.h > @@ -8,7 +8,7 @@ > void pv_console_init(void); > void pv_console_set_rx_handler(serial_rx_fn fn); > void pv_console_init_postirq(void); > -void pv_console_puts(const char *buf); > +void pv_console_puts(const char *buf, unsigned int nr); > size_t pv_console_rx(struct cpu_user_regs *regs); > evtchn_port_t pv_console_evtchn(void); > > @@ -17,7 +17,7 @@ evtchn_port_t pv_console_evtchn(void); > static inline void pv_console_init(void) {} > static inline void pv_console_set_rx_handler(serial_rx_fn fn) { } > static inline void pv_console_init_postirq(void) { } > -static inline void pv_console_puts(const char *buf) { } > +static inline void pv_console_puts(const char *buf, unsigned int nr) { } > static inline size_t pv_console_rx(struct cpu_user_regs *regs) { return 0; } > evtchn_port_t pv_console_evtchn(void) > { > diff --git a/xen/include/xen/serial.h b/xen/include/xen/serial.h > index f2994d4093..e11d6d3ebc 100644 > --- a/xen/include/xen/serial.h > +++ b/xen/include/xen/serial.h > @@ -114,8 +114,8 @@ int serial_parse_handle(char *conf); > /* Transmit a single character via the specified COM port. */ > void serial_putc(int handle, char c); > > -/* Transmit a NULL-terminated string via the specified COM port. */ > -void serial_puts(int handle, const char *s); > +/* Transmit a string via the specified COM port. */ > +void serial_puts(int handle, const char *s, unsigned int nr); > > /* > * An alternative to registering a character-receive hook. This function > diff --git a/xen/include/xen/video.h b/xen/include/xen/video.h > index 2e897f9df5..ddd21f374d 100644 > --- a/xen/include/xen/video.h > +++ b/xen/include/xen/video.h > @@ -13,11 +13,11 @@ > > #ifdef CONFIG_VIDEO > void video_init(void); > -extern void (*video_puts)(const char *); > +extern void (*video_puts)(const char *, unsigned int nr); > void video_endboot(void); > #else > #define video_init() ((void)0) > -#define video_puts(s) ((void)0) > +#define video_puts(s, nr) ((void)0) > #define video_endboot() ((void)0) > #endif > > -- > 2.11.0 > _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Xen-devel] [PATCH 2/4] xen/console: Don't treat NUL character as the end of the buffer 2019-04-16 20:33 ` [Xen-devel] " Stefano Stabellini (?) @ 2019-08-05 11:40 ` Julien Grall -1 siblings, 0 replies; 42+ messages in thread From: Julien Grall @ 2019-08-05 11:40 UTC (permalink / raw) To: Stefano Stabellini Cc: Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Jan Beulich, xen-devel Hi Stefano, On 16/04/2019 21:33, Stefano Stabellini wrote: > On Tue, 2 Apr 2019, Julien Grall wrote: >> After upgrading Debian to Buster, I have began to notice console >> mangling when using zsh in Dom0. This is happenning because output sent by >> zsh to the console may contain NULs in the middle of the buffer. >> >> The actual implementation of CONSOLEIO_write considers that a buffer >> always terminate with a NUL and therefore will ignore anything after it. >> >> In general, NULs are perfectly legitimate in terminal streams. For >> instance, this could be used for padding slow terminals. See terminfo(5) >> section `Delays and Padding`, or search for the pcre '\bpad'. >> >> Other use cases includes using the console for dumping non-human >> readable information (e.g debugger, file if no network...). With the >> current behavior, the resulting stream will end up to be corrupted. >> >> The documentation for CONSOLEIO_write is pretty limited (to not say >> inexistent). From the declaration, the hypercall takes a buffer and size. >> So this could lead to think the NUL character is allowed in the middle of >> the buffer. >> >> This patch updates the console API to pass the size along the buffer >> down so we can remove the reliance on buffer terminating by a NUL >> character. >> >> Signed-off-by: Julien Grall <julien.grall@arm.com> >> >> --- >> >> This patch was originally sent standalone [1]. But the series grows to >> include another bug found in the console code and documentation. >> >> Change since the standalone version: >> - Fix early printk on Arm >> - Fix gdbstub >> - Remove unecessary leading NUL character added by Xen >> - Handle DomU console >> - Rework the commit message >> >> Below a small C program to repro the bug on Xen: >> >> int main(void) >> { >> write(1, >> "\r\33[0m\0\0\0\0\0\0\0\0\33[27m\33[24m\33[j\33[32mjulien\33[31m@\33[00m\33[36mjuno2-julieng:~\33[37m>", >> 75); >> write(1, >> "\33[K\33[32C\33[01;33m--juno2-julieng-13:44--\33[00m\33[37m\33[55D", >> 54); >> write(1, "\33[?2004h", 8); >> >> return 0; >> } >> >> Without this patch, the only --juno2-julieng-13:44-- will be printed in >> yellow. >> >> This patch was tested on Arm using serial console. I am not entirely >> sure whether the video and PV console is correct. I would appreciate help >> for testing here. >> >> [1] https://lists.xenproject.org/archives/html/xen-devel/2019-02/msg01932.html >> --- >> xen/arch/arm/early_printk.c | 5 ++-- >> xen/common/gdbstub.c | 6 ++-- >> xen/drivers/char/console.c | 58 +++++++++++++++++++-------------------- >> xen/drivers/char/consoled.c | 7 ++--- >> xen/drivers/char/serial.c | 8 ++++-- >> xen/drivers/char/xen_pv_console.c | 10 +++---- >> xen/drivers/video/lfb.c | 14 ++++++---- >> xen/drivers/video/lfb.h | 4 +-- >> xen/drivers/video/vga.c | 14 ++++++---- >> xen/include/xen/console.h | 2 +- >> xen/include/xen/early_printk.h | 2 +- >> xen/include/xen/pv_console.h | 4 +-- >> xen/include/xen/serial.h | 4 +-- >> xen/include/xen/video.h | 4 +-- >> 14 files changed, 73 insertions(+), 69 deletions(-) >> >> diff --git a/xen/arch/arm/early_printk.c b/xen/arch/arm/early_printk.c >> index 97466a12b1..35a47c7229 100644 >> --- a/xen/arch/arm/early_printk.c >> +++ b/xen/arch/arm/early_printk.c >> @@ -17,9 +17,10 @@ >> void early_putch(char c); >> void early_flush(void); >> >> -void early_puts(const char *s) >> +void early_puts(const char *s, unsigned int nr) >> { >> - while (*s != '\0') { >> + while ( nr-- > 0 ) >> + { >> if (*s == '\n') >> early_putch('\r'); >> early_putch(*s); >> diff --git a/xen/common/gdbstub.c b/xen/common/gdbstub.c >> index 07095e1ec7..08a4dda9f3 100644 >> --- a/xen/common/gdbstub.c >> +++ b/xen/common/gdbstub.c >> @@ -68,7 +68,7 @@ static void gdb_smp_resume(void); >> static char __initdata opt_gdb[30]; >> string_param("gdb", opt_gdb); >> >> -static void gdbstub_console_puts(const char *str); >> +static void gdbstub_console_puts(const char *str, unsigned int nr); >> >> /* value <-> char (de)serialzers */ >> static char >> @@ -546,14 +546,14 @@ __gdb_ctx = { >> static struct gdb_context *gdb_ctx = &__gdb_ctx; >> >> static void >> -gdbstub_console_puts(const char *str) >> +gdbstub_console_puts(const char *str, unsigned int nr) >> { >> const char *p; >> >> gdb_start_packet(gdb_ctx); >> gdb_write_to_packet_char('O', gdb_ctx); >> >> - for ( p = str; *p != '\0'; p++ ) >> + for ( p = str; nr > 0; p++, nr-- ) >> { >> gdb_write_to_packet_char(hex2char((*p>>4) & 0x0f), gdb_ctx ); >> gdb_write_to_packet_char(hex2char((*p) & 0x0f), gdb_ctx ); >> diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c >> index 9bbcb0f57a..b119bf980b 100644 >> --- a/xen/drivers/char/console.c >> +++ b/xen/drivers/char/console.c >> @@ -325,9 +325,9 @@ long read_console_ring(struct xen_sysctl_readconsole *op) >> static char serial_rx_ring[SERIAL_RX_SIZE]; >> static unsigned int serial_rx_cons, serial_rx_prod; >> >> -static void (*serial_steal_fn)(const char *) = early_puts; >> +static void (*serial_steal_fn)(const char *, unsigned int nr) = early_puts; >> >> -int console_steal(int handle, void (*fn)(const char *)) >> +int console_steal(int handle, void (*fn)(const char *, unsigned int nr)) >> { >> if ( (handle == -1) || (handle != sercon_handle) ) >> return 0; >> @@ -345,15 +345,15 @@ void console_giveback(int id) >> serial_steal_fn = NULL; >> } >> >> -static void sercon_puts(const char *s) >> +static void sercon_puts(const char *s, unsigned int nr) >> { >> if ( serial_steal_fn != NULL ) >> - (*serial_steal_fn)(s); >> + (*serial_steal_fn)(s, nr); >> else >> - serial_puts(sercon_handle, s); >> + serial_puts(sercon_handle, s, nr); >> >> /* Copy all serial output into PV console */ >> - pv_console_puts(s); >> + pv_console_puts(s, nr); >> } >> >> static void dump_console_ring_key(unsigned char key) >> @@ -388,8 +388,8 @@ static void dump_console_ring_key(unsigned char key) >> } >> buf[sofar] = '\0'; >> >> - sercon_puts(buf); >> - video_puts(buf); >> + sercon_puts(buf, sofar); >> + video_puts(buf, sofar); >> >> free_xenheap_pages(buf, order); >> } >> @@ -527,7 +527,7 @@ static inline void xen_console_write_debug_port(const char *buf, size_t len) >> static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, int count) >> { >> char kbuf[128]; >> - int kcount = 0; >> + unsigned int kcount = 0; >> struct domain *cd = current->domain; >> >> while ( count > 0 ) >> @@ -540,25 +540,22 @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, int count) >> kcount = min_t(int, count, sizeof(kbuf)-1); >> if ( copy_from_guest(kbuf, buffer, kcount) ) >> return -EFAULT; >> - kbuf[kcount] = '\0'; >> >> if ( is_hardware_domain(cd) ) >> { >> /* Use direct console output as it could be interactive */ >> spin_lock_irq(&console_lock); >> >> - sercon_puts(kbuf); >> - video_puts(kbuf); >> + sercon_puts(kbuf, kcount); >> + video_puts(kbuf, kcount); >> >> #ifdef CONFIG_X86 >> if ( opt_console_xen ) >> { >> - size_t len = strlen(kbuf); >> - >> if ( xen_guest ) >> - xen_hypercall_console_write(kbuf, len); >> + xen_hypercall_console_write(kbuf, kcount); >> else >> - xen_console_write_debug_port(kbuf, len); >> + xen_console_write_debug_port(kbuf, kcount); >> } >> #endif >> >> @@ -575,19 +572,20 @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, int count) >> char *kin = kbuf, *kout = kbuf, c; >> >> /* Strip non-printable characters */ >> - for ( ; ; ) >> + do >> { >> c = *kin++; >> - if ( c == '\0' || c == '\n' ) >> + if ( c == '\n' ) >> break; >> if ( isprint(c) || c == '\t' ) >> *kout++ = c; >> - } >> + } while ( --kcount > 0 ); > > Why not > > while ( kcount-- > 0 ) > { > XXX > } > > like you did in early_puts? Anyway this should be just style, so up to > you. This is not a style issue but a compilation problem. The variable c is used after the loop to check if it a newline (i.e \n). The variable is only set within the loop. With a while () the compiler thinks the variable may never be initialized. The do {} while fixes the problem by always executing one iteration. This is fine because count is kcount will always be > 0. Alternatively, the variable can be initialized to a constant. But I feels this is may hide other problem in the code in the future. Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 3/4] xen/public: Document HYPERCALL_console_io() 2019-04-02 16:42 [PATCH 0/4] xen/console: Bug fixes and doc improvement Julien Grall 2019-04-02 16:42 ` [PATCH 1/4] xen/console: Properly buffer domU output when using CONSOLEIO_write Julien Grall 2019-04-02 16:42 ` [PATCH 2/4] xen/console: Don't treat NUL character as the end of the buffer Julien Grall @ 2019-04-02 16:42 ` Julien Grall 2019-04-03 11:41 ` Wei Liu ` (2 more replies) 2019-04-02 16:42 ` [PATCH 4/4] xen/console: Simplify domU console handling in guest_console_write Julien Grall 3 siblings, 3 replies; 42+ messages in thread From: Julien Grall @ 2019-04-02 16:42 UTC (permalink / raw) To: xen-devel Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, Jan Beulich Currently, OS developpers will have to look at Xen code in order to know the parameters of an hypercall and how it is meant to work. This is not a trivial task as you may need to have a deep understanding of Xen internal. This patch attempts to document the behavior of HYPERCALL_console_io() to help OS developer. Signed-off-by: Julien Grall <julien.grall@arm.com> --- This is a first attempt to address the lack on documentation for hypercalls. We may want to decide a format to use in every hypercall so it can be readable for the OS developer and easily consummed by documentation tools. --- xen/include/public/xen.h | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h index ccdffc0ad1..7c119c6782 100644 --- a/xen/include/public/xen.h +++ b/xen/include/public/xen.h @@ -97,6 +97,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_ulong_t); #define __HYPERVISOR_set_timer_op 15 #define __HYPERVISOR_event_channel_op_compat 16 /* compat since 0x00030202 */ #define __HYPERVISOR_xen_version 17 +/* HYPERVISOR_console_io(int cmd, int count, XEN_GUEST_HANDLE(char) buffer); */ #define __HYPERVISOR_console_io 18 #define __HYPERVISOR_physdev_op_compat 19 /* compat since 0x00030202 */ #define __HYPERVISOR_grant_table_op 20 @@ -486,7 +487,25 @@ DEFINE_XEN_GUEST_HANDLE(mmuext_op_t); /* ` } */ /* - * Commands to HYPERVISOR_console_io(). + * Commands to HYPERVISOR_console_io() + * + * @cmd: Command (see below) + * @count: Size of the buffer to read/write + * @buffer: Pointer in the guest memory + * + * List of commands: + * + * * CONSOLEIO_write: Write the buffer on Xen console. + * For the hardware domain, all the characters in the buffer will + * be written. Characters will be printed to directly to the + * console. + * For all the other domains, only the printable characters will be + * written. Characters may be buffered until a newline (i.e '\n') is + * found. + * Return 0 on success, otherwise return an error code. + * * CONSOLE_read: Attempts to read up @count characters from Xen console. + * Return the number of character read on success, otherwise return + * an error code. */ #define CONSOLEIO_write 0 #define CONSOLEIO_read 1 -- 2.11.0 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH 3/4] xen/public: Document HYPERCALL_console_io() 2019-04-02 16:42 ` [PATCH 3/4] xen/public: Document HYPERCALL_console_io() Julien Grall @ 2019-04-03 11:41 ` Wei Liu 2019-04-03 13:04 ` Jan Beulich 2019-04-16 20:42 ` [Xen-devel] " Stefano Stabellini 2 siblings, 0 replies; 42+ messages in thread From: Wei Liu @ 2019-04-03 11:41 UTC (permalink / raw) To: Julien Grall Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Jan Beulich, xen-devel On Tue, Apr 02, 2019 at 05:42:37PM +0100, Julien Grall wrote: > Currently, OS developpers will have to look at Xen code in order to know > the parameters of an hypercall and how it is meant to work. > > This is not a trivial task as you may need to have a deep understanding > of Xen internal. > > This patch attempts to document the behavior of HYPERCALL_console_io() to > help OS developer. > > Signed-off-by: Julien Grall <julien.grall@arm.com> Acked-by: Wei Liu <wei.liu2@citrix.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 3/4] xen/public: Document HYPERCALL_console_io() 2019-04-02 16:42 ` [PATCH 3/4] xen/public: Document HYPERCALL_console_io() Julien Grall 2019-04-03 11:41 ` Wei Liu @ 2019-04-03 13:04 ` Jan Beulich 2019-04-09 11:26 ` [Xen-devel] " Julien Grall 2019-04-16 20:42 ` [Xen-devel] " Stefano Stabellini 2 siblings, 1 reply; 42+ messages in thread From: Jan Beulich @ 2019-04-03 13:04 UTC (permalink / raw) To: Julien Grall Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, xen-devel >>> On 02.04.19 at 18:42, <julien.grall@arm.com> wrote: > --- a/xen/include/public/xen.h > +++ b/xen/include/public/xen.h > @@ -97,6 +97,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_ulong_t); > #define __HYPERVISOR_set_timer_op 15 > #define __HYPERVISOR_event_channel_op_compat 16 /* compat since 0x00030202 */ > #define __HYPERVISOR_xen_version 17 > +/* HYPERVISOR_console_io(int cmd, int count, XEN_GUEST_HANDLE(char) buffer); */ > #define __HYPERVISOR_console_io 18 I pretty strongly object to this comment going where you put it. I'd be fine if you put it ... > @@ -486,7 +487,25 @@ DEFINE_XEN_GUEST_HANDLE(mmuext_op_t); > /* ` } */ > > /* > - * Commands to HYPERVISOR_console_io(). > + * Commands to HYPERVISOR_console_io() ... into here, just like done for HYPERVISOR_mmuext_op() and HYPERVISOR_update_va_mapping*() immediately above. This would then also help ... > + * @cmd: Command (see below) > + * @count: Size of the buffer to read/write > + * @buffer: Pointer in the guest memory ... associating these names. Also please note the quotation used by the mentioned existing doc comments, as well as a few other formal aspects (like them also making clear what the return type is). I think that's a model used elsewhere as well, so I think you should follow it here. The other thing is: As mentioned elsewhere, I don't think the first two parameters should be plain int. I'm not happy to see this proliferate into documentation as well, i.e. I'd prefer if this was corrected before adding documentation. Would you be willing to do this, or should I add it to my todo list? Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 3/4] xen/public: Document HYPERCALL_console_io() @ 2019-04-09 11:26 ` Julien Grall 0 siblings, 0 replies; 42+ messages in thread From: Julien Grall @ 2019-04-09 11:26 UTC (permalink / raw) To: Jan Beulich Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, xen-devel Hi Jan, On 03/04/2019 14:04, Jan Beulich wrote: >>>> On 02.04.19 at 18:42, <julien.grall@arm.com> wrote: >> --- a/xen/include/public/xen.h >> +++ b/xen/include/public/xen.h >> @@ -97,6 +97,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_ulong_t); >> #define __HYPERVISOR_set_timer_op 15 >> #define __HYPERVISOR_event_channel_op_compat 16 /* compat since 0x00030202 */ >> #define __HYPERVISOR_xen_version 17 >> +/* HYPERVISOR_console_io(int cmd, int count, XEN_GUEST_HANDLE(char) buffer); */ >> #define __HYPERVISOR_console_io 18 > > I pretty strongly object to this comment going where you put it. > I'd be fine if you put it ... > >> @@ -486,7 +487,25 @@ DEFINE_XEN_GUEST_HANDLE(mmuext_op_t); >> /* ` } */ >> >> /* >> - * Commands to HYPERVISOR_console_io(). >> + * Commands to HYPERVISOR_console_io() > > ... into here, just like done for HYPERVISOR_mmuext_op() and > HYPERVISOR_update_va_mapping*() immediately above. This > would then also help ... > >> + * @cmd: Command (see below) >> + * @count: Size of the buffer to read/write >> + * @buffer: Pointer in the guest memory > > ... associating these names. Ok. > > Also please note the quotation used by the mentioned > existing doc comments, as well as a few other formal aspects > (like them also making clear what the return type is). I think > that's a model used elsewhere as well, so I think you should > follow it here. I haven't replicated the ` because I have no idea what they are used for. I would appreciate if you provide pointer how to use them. > > The other thing is: As mentioned elsewhere, I don't think the > first two parameters should be plain int. I'm not happy to see > this proliferate into documentation as well, i.e. I'd prefer if > this was corrected before adding documentation. Would you > be willing to do this, or should I add it to my todo list? While switching from cmd from signed to unsigned should be ok. This would introduce a different behavior of for count. Are we happy with that, or shall we add a check ((int)count) > 0? In any case, I am happy to have a look at it. Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Xen-devel] [PATCH 3/4] xen/public: Document HYPERCALL_console_io() @ 2019-04-09 11:26 ` Julien Grall 0 siblings, 0 replies; 42+ messages in thread From: Julien Grall @ 2019-04-09 11:26 UTC (permalink / raw) To: Jan Beulich Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, xen-devel Hi Jan, On 03/04/2019 14:04, Jan Beulich wrote: >>>> On 02.04.19 at 18:42, <julien.grall@arm.com> wrote: >> --- a/xen/include/public/xen.h >> +++ b/xen/include/public/xen.h >> @@ -97,6 +97,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_ulong_t); >> #define __HYPERVISOR_set_timer_op 15 >> #define __HYPERVISOR_event_channel_op_compat 16 /* compat since 0x00030202 */ >> #define __HYPERVISOR_xen_version 17 >> +/* HYPERVISOR_console_io(int cmd, int count, XEN_GUEST_HANDLE(char) buffer); */ >> #define __HYPERVISOR_console_io 18 > > I pretty strongly object to this comment going where you put it. > I'd be fine if you put it ... > >> @@ -486,7 +487,25 @@ DEFINE_XEN_GUEST_HANDLE(mmuext_op_t); >> /* ` } */ >> >> /* >> - * Commands to HYPERVISOR_console_io(). >> + * Commands to HYPERVISOR_console_io() > > ... into here, just like done for HYPERVISOR_mmuext_op() and > HYPERVISOR_update_va_mapping*() immediately above. This > would then also help ... > >> + * @cmd: Command (see below) >> + * @count: Size of the buffer to read/write >> + * @buffer: Pointer in the guest memory > > ... associating these names. Ok. > > Also please note the quotation used by the mentioned > existing doc comments, as well as a few other formal aspects > (like them also making clear what the return type is). I think > that's a model used elsewhere as well, so I think you should > follow it here. I haven't replicated the ` because I have no idea what they are used for. I would appreciate if you provide pointer how to use them. > > The other thing is: As mentioned elsewhere, I don't think the > first two parameters should be plain int. I'm not happy to see > this proliferate into documentation as well, i.e. I'd prefer if > this was corrected before adding documentation. Would you > be willing to do this, or should I add it to my todo list? While switching from cmd from signed to unsigned should be ok. This would introduce a different behavior of for count. Are we happy with that, or shall we add a check ((int)count) > 0? In any case, I am happy to have a look at it. Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 3/4] xen/public: Document HYPERCALL_console_io() @ 2019-04-09 11:42 ` Jan Beulich 0 siblings, 0 replies; 42+ messages in thread From: Jan Beulich @ 2019-04-09 11:42 UTC (permalink / raw) To: Julien Grall Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, xen-devel >>> On 09.04.19 at 13:26, <julien.grall@arm.com> wrote: > On 03/04/2019 14:04, Jan Beulich wrote: >> Also please note the quotation used by the mentioned >> existing doc comments, as well as a few other formal aspects >> (like them also making clear what the return type is). I think >> that's a model used elsewhere as well, so I think you should >> follow it here. > > I haven't replicated the ` because I have no idea what they are used for. I > would appreciate if you provide pointer how to use them. Well, I can only point you at the history of things, e.g. 262e118a37 "docs/html/: Annotations for two hypercalls". >> The other thing is: As mentioned elsewhere, I don't think the >> first two parameters should be plain int. I'm not happy to see >> this proliferate into documentation as well, i.e. I'd prefer if >> this was corrected before adding documentation. Would you >> be willing to do this, or should I add it to my todo list? > > While switching from cmd from signed to unsigned should be ok. This would > introduce a different behavior of for count. Since this removes an error condition, I think this is an okay change to happen, without ... > Are we happy with that, or shall we add a check ((int)count) > 0? ... any such extra check. This also isn't going to introduce any new real risk of a long running operation or some such - if 2Gb of input data are fine, I can't see why 4Gb wouldn't be, too. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Xen-devel] [PATCH 3/4] xen/public: Document HYPERCALL_console_io() @ 2019-04-09 11:42 ` Jan Beulich 0 siblings, 0 replies; 42+ messages in thread From: Jan Beulich @ 2019-04-09 11:42 UTC (permalink / raw) To: Julien Grall Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, xen-devel >>> On 09.04.19 at 13:26, <julien.grall@arm.com> wrote: > On 03/04/2019 14:04, Jan Beulich wrote: >> Also please note the quotation used by the mentioned >> existing doc comments, as well as a few other formal aspects >> (like them also making clear what the return type is). I think >> that's a model used elsewhere as well, so I think you should >> follow it here. > > I haven't replicated the ` because I have no idea what they are used for. I > would appreciate if you provide pointer how to use them. Well, I can only point you at the history of things, e.g. 262e118a37 "docs/html/: Annotations for two hypercalls". >> The other thing is: As mentioned elsewhere, I don't think the >> first two parameters should be plain int. I'm not happy to see >> this proliferate into documentation as well, i.e. I'd prefer if >> this was corrected before adding documentation. Would you >> be willing to do this, or should I add it to my todo list? > > While switching from cmd from signed to unsigned should be ok. This would > introduce a different behavior of for count. Since this removes an error condition, I think this is an okay change to happen, without ... > Are we happy with that, or shall we add a check ((int)count) > 0? ... any such extra check. This also isn't going to introduce any new real risk of a long running operation or some such - if 2Gb of input data are fine, I can't see why 4Gb wouldn't be, too. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 3/4] xen/public: Document HYPERCALL_console_io() @ 2019-04-16 9:54 ` Julien Grall 0 siblings, 0 replies; 42+ messages in thread From: Julien Grall @ 2019-04-16 9:54 UTC (permalink / raw) To: Jan Beulich Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, xen-devel Hi Jan, On 4/9/19 12:42 PM, Jan Beulich wrote: >>>> On 09.04.19 at 13:26, <julien.grall@arm.com> wrote: >> On 03/04/2019 14:04, Jan Beulich wrote: >>> Also please note the quotation used by the mentioned >>> existing doc comments, as well as a few other formal aspects >>> (like them also making clear what the return type is). I think >>> that's a model used elsewhere as well, so I think you should >>> follow it here. >> >> I haven't replicated the ` because I have no idea what they are used for. I >> would appreciate if you provide pointer how to use them. > > Well, I can only point you at the history of things, e.g. > 262e118a37 "docs/html/: Annotations for two hypercalls". Thank you for the pointer. However, we don't seem to use this formatting everywhere. For instance, the formatting used in my patch seems to be consistent with xen/include/public/vcpu.h. Furthermore, the description of the hypercall is done on top of the definition of the hypercall number. Before I rework this patch, could we agree on what formatting we want? > >>> The other thing is: As mentioned elsewhere, I don't think the >>> first two parameters should be plain int. I'm not happy to see >>> this proliferate into documentation as well, i.e. I'd prefer if >>> this was corrected before adding documentation. Would you >>> be willing to do this, or should I add it to my todo list? >> >> While switching from cmd from signed to unsigned should be ok. This would >> introduce a different behavior of for count. > > Since this removes an error condition, I think this is an okay change > to happen, without ... > >> Are we happy with that, or shall we add a check ((int)count) > 0? > > ... any such extra check. This also isn't going to introduce any new > real risk of a long running operation or some such - if 2Gb of input > data are fine, I can't see why 4Gb wouldn't be, too. Fair point. I will update the prototype accordingly. Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Xen-devel] [PATCH 3/4] xen/public: Document HYPERCALL_console_io() @ 2019-04-16 9:54 ` Julien Grall 0 siblings, 0 replies; 42+ messages in thread From: Julien Grall @ 2019-04-16 9:54 UTC (permalink / raw) To: Jan Beulich Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, xen-devel Hi Jan, On 4/9/19 12:42 PM, Jan Beulich wrote: >>>> On 09.04.19 at 13:26, <julien.grall@arm.com> wrote: >> On 03/04/2019 14:04, Jan Beulich wrote: >>> Also please note the quotation used by the mentioned >>> existing doc comments, as well as a few other formal aspects >>> (like them also making clear what the return type is). I think >>> that's a model used elsewhere as well, so I think you should >>> follow it here. >> >> I haven't replicated the ` because I have no idea what they are used for. I >> would appreciate if you provide pointer how to use them. > > Well, I can only point you at the history of things, e.g. > 262e118a37 "docs/html/: Annotations for two hypercalls". Thank you for the pointer. However, we don't seem to use this formatting everywhere. For instance, the formatting used in my patch seems to be consistent with xen/include/public/vcpu.h. Furthermore, the description of the hypercall is done on top of the definition of the hypercall number. Before I rework this patch, could we agree on what formatting we want? > >>> The other thing is: As mentioned elsewhere, I don't think the >>> first two parameters should be plain int. I'm not happy to see >>> this proliferate into documentation as well, i.e. I'd prefer if >>> this was corrected before adding documentation. Would you >>> be willing to do this, or should I add it to my todo list? >> >> While switching from cmd from signed to unsigned should be ok. This would >> introduce a different behavior of for count. > > Since this removes an error condition, I think this is an okay change > to happen, without ... > >> Are we happy with that, or shall we add a check ((int)count) > 0? > > ... any such extra check. This also isn't going to introduce any new > real risk of a long running operation or some such - if 2Gb of input > data are fine, I can't see why 4Gb wouldn't be, too. Fair point. I will update the prototype accordingly. Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 3/4] xen/public: Document HYPERCALL_console_io() @ 2019-04-25 10:09 ` Jan Beulich 0 siblings, 0 replies; 42+ messages in thread From: Jan Beulich @ 2019-04-25 10:09 UTC (permalink / raw) To: Julien Grall Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, xen-devel >>> On 16.04.19 at 11:54, <julien.grall@arm.com> wrote: > On 4/9/19 12:42 PM, Jan Beulich wrote: >>>>> On 09.04.19 at 13:26, <julien.grall@arm.com> wrote: >>> On 03/04/2019 14:04, Jan Beulich wrote: >>>> Also please note the quotation used by the mentioned >>>> existing doc comments, as well as a few other formal aspects >>>> (like them also making clear what the return type is). I think >>>> that's a model used elsewhere as well, so I think you should >>>> follow it here. >>> >>> I haven't replicated the ` because I have no idea what they are used for. I >>> would appreciate if you provide pointer how to use them. >> >> Well, I can only point you at the history of things, e.g. >> 262e118a37 "docs/html/: Annotations for two hypercalls". > > Thank you for the pointer. However, we don't seem to use this formatting > everywhere. For instance, the formatting used in my patch seems to be > consistent with xen/include/public/vcpu.h. > > Furthermore, the description of the hypercall is done on top of the > definition of the hypercall number. Before I rework this patch, could we > agree on what formatting we want? Well, FAOD (see Ian's earlier reply) the style to be used is as suggested, and eventually vcpu.h should be switched too. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Xen-devel] [PATCH 3/4] xen/public: Document HYPERCALL_console_io() @ 2019-04-25 10:09 ` Jan Beulich 0 siblings, 0 replies; 42+ messages in thread From: Jan Beulich @ 2019-04-25 10:09 UTC (permalink / raw) To: Julien Grall Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, xen-devel >>> On 16.04.19 at 11:54, <julien.grall@arm.com> wrote: > On 4/9/19 12:42 PM, Jan Beulich wrote: >>>>> On 09.04.19 at 13:26, <julien.grall@arm.com> wrote: >>> On 03/04/2019 14:04, Jan Beulich wrote: >>>> Also please note the quotation used by the mentioned >>>> existing doc comments, as well as a few other formal aspects >>>> (like them also making clear what the return type is). I think >>>> that's a model used elsewhere as well, so I think you should >>>> follow it here. >>> >>> I haven't replicated the ` because I have no idea what they are used for. I >>> would appreciate if you provide pointer how to use them. >> >> Well, I can only point you at the history of things, e.g. >> 262e118a37 "docs/html/: Annotations for two hypercalls". > > Thank you for the pointer. However, we don't seem to use this formatting > everywhere. For instance, the formatting used in my patch seems to be > consistent with xen/include/public/vcpu.h. > > Furthermore, the description of the hypercall is done on top of the > definition of the hypercall number. Before I rework this patch, could we > agree on what formatting we want? Well, FAOD (see Ian's earlier reply) the style to be used is as suggested, and eventually vcpu.h should be switched too. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 3/4] xen/public: Document HYPERCALL_console_io() @ 2019-04-16 10:29 ` Ian Jackson 0 siblings, 0 replies; 42+ messages in thread From: Ian Jackson @ 2019-04-16 10:29 UTC (permalink / raw) To: Jan Beulich Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk, Andrew Cooper, Tim (Xen.org), George Dunlap, Julien Grall, xen-devel Jan Beulich writes ("Re: [PATCH 3/4] xen/public: Document HYPERCALL_console_io()"): > On 09.04.19 at 13:26, <julien.grall@arm.com> wrote: > > I haven't replicated the ` because I have no idea what they are used for. I > > would appreciate if you provide pointer how to use them. > > Well, I can only point you at the history of things, e.g. > 262e118a37 "docs/html/: Annotations for two hypercalls". That is a reasonable example, but: There is in fact some documentation for the ` syntax. See docs/xen-headers, which is the script which scans the .h files and makes the online hypercall documentation. The syntax doc is rather terse, I'm afraid, but it's only a 400-line script... I wrote that script because I wanted some kind of browseable documentation, and I didn't want to totally overhaul the headers. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Xen-devel] [PATCH 3/4] xen/public: Document HYPERCALL_console_io() @ 2019-04-16 10:29 ` Ian Jackson 0 siblings, 0 replies; 42+ messages in thread From: Ian Jackson @ 2019-04-16 10:29 UTC (permalink / raw) To: Jan Beulich Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk, Andrew Cooper, Tim (Xen.org), George Dunlap, Julien Grall, xen-devel Jan Beulich writes ("Re: [PATCH 3/4] xen/public: Document HYPERCALL_console_io()"): > On 09.04.19 at 13:26, <julien.grall@arm.com> wrote: > > I haven't replicated the ` because I have no idea what they are used for. I > > would appreciate if you provide pointer how to use them. > > Well, I can only point you at the history of things, e.g. > 262e118a37 "docs/html/: Annotations for two hypercalls". That is a reasonable example, but: There is in fact some documentation for the ` syntax. See docs/xen-headers, which is the script which scans the .h files and makes the online hypercall documentation. The syntax doc is rather terse, I'm afraid, but it's only a 400-line script... I wrote that script because I wanted some kind of browseable documentation, and I didn't want to totally overhaul the headers. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Xen-devel] [PATCH 3/4] xen/public: Document HYPERCALL_console_io() 2019-04-09 11:42 ` [Xen-devel] " Jan Beulich ` (2 preceding siblings ...) (?) @ 2019-08-05 9:40 ` Julien Grall 2019-08-05 10:07 ` Jan Beulich -1 siblings, 1 reply; 42+ messages in thread From: Julien Grall @ 2019-08-05 9:40 UTC (permalink / raw) To: Jan Beulich Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, xen-devel Hi, On 09/04/2019 12:42, Jan Beulich wrote: >>>> On 09.04.19 at 13:26, <julien.grall@arm.com> wrote: >> On 03/04/2019 14:04, Jan Beulich wrote: >>> Also please note the quotation used by the mentioned >>> existing doc comments, as well as a few other formal aspects >>> (like them also making clear what the return type is). I think >>> that's a model used elsewhere as well, so I think you should >>> follow it here. >> >> I haven't replicated the ` because I have no idea what they are used for. I >> would appreciate if you provide pointer how to use them. > > Well, I can only point you at the history of things, e.g. > 262e118a37 "docs/html/: Annotations for two hypercalls". > >>> The other thing is: As mentioned elsewhere, I don't think the >>> first two parameters should be plain int. I'm not happy to see >>> this proliferate into documentation as well, i.e. I'd prefer if >>> this was corrected before adding documentation. Would you >>> be willing to do this, or should I add it to my todo list? >> >> While switching from cmd from signed to unsigned should be ok. This would >> introduce a different behavior of for count. > > Since this removes an error condition, I think this is an okay change > to happen, without ... > >> Are we happy with that, or shall we add a check ((int)count) > 0? > > ... any such extra check. This also isn't going to introduce any new > real risk of a long running operation or some such - if 2Gb of input > data are fine, I can't see why 4Gb wouldn't be, too. Actually, it will not be fine at least for the read case. The return value is a 32-bit value (on AArch32 and if you want to keep compat between 64-bit and 32-bit). A negative value indicates an error. As we return the number of characters read, it means we can only handle 2GB. So I would rather limit the buffer to 2GB for everyone. Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Xen-devel] [PATCH 3/4] xen/public: Document HYPERCALL_console_io() 2019-08-05 9:40 ` Julien Grall @ 2019-08-05 10:07 ` Jan Beulich 2019-08-05 10:17 ` Julien Grall 0 siblings, 1 reply; 42+ messages in thread From: Jan Beulich @ 2019-08-05 10:07 UTC (permalink / raw) To: Julien Grall Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, xen-devel On 05.08.2019 11:40, Julien Grall wrote: > Hi, > > On 09/04/2019 12:42, Jan Beulich wrote: >>>>> On 09.04.19 at 13:26, <julien.grall@arm.com> wrote: >>> On 03/04/2019 14:04, Jan Beulich wrote: >>>> Also please note the quotation used by the mentioned >>>> existing doc comments, as well as a few other formal aspects >>>> (like them also making clear what the return type is). I think >>>> that's a model used elsewhere as well, so I think you should >>>> follow it here. >>> >>> I haven't replicated the ` because I have no idea what they are used for. I >>> would appreciate if you provide pointer how to use them. >> >> Well, I can only point you at the history of things, e.g. >> 262e118a37 "docs/html/: Annotations for two hypercalls". >> >>>> The other thing is: As mentioned elsewhere, I don't think the >>>> first two parameters should be plain int. I'm not happy to see >>>> this proliferate into documentation as well, i.e. I'd prefer if >>>> this was corrected before adding documentation. Would you >>>> be willing to do this, or should I add it to my todo list? >>> >>> While switching from cmd from signed to unsigned should be ok. This would >>> introduce a different behavior of for count. >> >> Since this removes an error condition, I think this is an okay change >> to happen, without ... >> >>> Are we happy with that, or shall we add a check ((int)count) > 0? >> >> ... any such extra check. This also isn't going to introduce any new >> real risk of a long running operation or some such - if 2Gb of input >> data are fine, I can't see why 4Gb wouldn't be, too. > > Actually, it will not be fine at least for the read case. The return value is a 32-bit value (on AArch32 and if you want to keep compat between 64-bit and 32-bit). A negative value indicates an error. As we return the number of characters read, it means we can only handle 2GB. Hmm, valid point. > So I would rather limit the buffer to 2GB for everyone. Probably fair enough then (if explicitly stated). Personally I would nevertheless allow sizes up to 4Gb-4kb, but I can see why this may not be liked by everyone. I'd like to point out though that the PTR_ERR() machinery we've inherited from Linux utilizes exactly that. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Xen-devel] [PATCH 3/4] xen/public: Document HYPERCALL_console_io() 2019-08-05 10:07 ` Jan Beulich @ 2019-08-05 10:17 ` Julien Grall 2019-08-05 10:21 ` Jan Beulich 0 siblings, 1 reply; 42+ messages in thread From: Julien Grall @ 2019-08-05 10:17 UTC (permalink / raw) To: Jan Beulich Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, xen-devel Hi Jan, On 05/08/2019 11:07, Jan Beulich wrote: > On 05.08.2019 11:40, Julien Grall wrote: >> Hi, >> >> On 09/04/2019 12:42, Jan Beulich wrote: >>>>>> On 09.04.19 at 13:26, <julien.grall@arm.com> wrote: >>>> On 03/04/2019 14:04, Jan Beulich wrote: >>>>> Also please note the quotation used by the mentioned >>>>> existing doc comments, as well as a few other formal aspects >>>>> (like them also making clear what the return type is). I think >>>>> that's a model used elsewhere as well, so I think you should >>>>> follow it here. >>>> >>>> I haven't replicated the ` because I have no idea what they are used for. I >>>> would appreciate if you provide pointer how to use them. >>> >>> Well, I can only point you at the history of things, e.g. >>> 262e118a37 "docs/html/: Annotations for two hypercalls". >>> >>>>> The other thing is: As mentioned elsewhere, I don't think the >>>>> first two parameters should be plain int. I'm not happy to see >>>>> this proliferate into documentation as well, i.e. I'd prefer if >>>>> this was corrected before adding documentation. Would you >>>>> be willing to do this, or should I add it to my todo list? >>>> >>>> While switching from cmd from signed to unsigned should be ok. This would >>>> introduce a different behavior of for count. >>> >>> Since this removes an error condition, I think this is an okay change >>> to happen, without ... >>> >>>> Are we happy with that, or shall we add a check ((int)count) > 0? >>> >>> ... any such extra check. This also isn't going to introduce any new >>> real risk of a long running operation or some such - if 2Gb of input >>> data are fine, I can't see why 4Gb wouldn't be, too. >> >> Actually, it will not be fine at least for the read case. The return value is a 32-bit value (on AArch32 and if you want to keep compat between 64-bit and 32-bit). A negative value indicates an error. As we return the number of characters read, it means we can only handle 2GB. > > Hmm, valid point. > >> So I would rather limit the buffer to 2GB for everyone. > > Probably fair enough then (if explicitly stated). Personally I would > nevertheless allow sizes up to 4Gb-4kb, but I can see why this may > not be liked by everyone. I'd like to point out though that the > PTR_ERR() machinery we've inherited from Linux utilizes exactly that. Well, that's a console buffer. The chance you have write/read more than 2GB in a row is very unlikely. So it feels to me that exposing PTR_ERR(...) like interface is not worth it. Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Xen-devel] [PATCH 3/4] xen/public: Document HYPERCALL_console_io() 2019-08-05 10:17 ` Julien Grall @ 2019-08-05 10:21 ` Jan Beulich 0 siblings, 0 replies; 42+ messages in thread From: Jan Beulich @ 2019-08-05 10:21 UTC (permalink / raw) To: Julien Grall Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, xen-devel On 05.08.2019 12:17, Julien Grall wrote: > Hi Jan, > > On 05/08/2019 11:07, Jan Beulich wrote: >> On 05.08.2019 11:40, Julien Grall wrote: >>> Hi, >>> >>> On 09/04/2019 12:42, Jan Beulich wrote: >>>>>>> On 09.04.19 at 13:26, <julien.grall@arm.com> wrote: >>>>> On 03/04/2019 14:04, Jan Beulich wrote: >>>>>> Also please note the quotation used by the mentioned >>>>>> existing doc comments, as well as a few other formal aspects >>>>>> (like them also making clear what the return type is). I think >>>>>> that's a model used elsewhere as well, so I think you should >>>>>> follow it here. >>>>> >>>>> I haven't replicated the ` because I have no idea what they are used for. I >>>>> would appreciate if you provide pointer how to use them. >>>> >>>> Well, I can only point you at the history of things, e.g. >>>> 262e118a37 "docs/html/: Annotations for two hypercalls". >>>> >>>>>> The other thing is: As mentioned elsewhere, I don't think the >>>>>> first two parameters should be plain int. I'm not happy to see >>>>>> this proliferate into documentation as well, i.e. I'd prefer if >>>>>> this was corrected before adding documentation. Would you >>>>>> be willing to do this, or should I add it to my todo list? >>>>> >>>>> While switching from cmd from signed to unsigned should be ok. This would >>>>> introduce a different behavior of for count. >>>> >>>> Since this removes an error condition, I think this is an okay change >>>> to happen, without ... >>>> >>>>> Are we happy with that, or shall we add a check ((int)count) > 0? >>>> >>>> ... any such extra check. This also isn't going to introduce any new >>>> real risk of a long running operation or some such - if 2Gb of input >>>> data are fine, I can't see why 4Gb wouldn't be, too. >>> >>> Actually, it will not be fine at least for the read case. The return value is a 32-bit value (on AArch32 and if you want to keep compat between 64-bit and 32-bit). A negative value indicates an error. As we return the number of characters read, it means we can only handle 2GB. >> >> Hmm, valid point. >> >>> So I would rather limit the buffer to 2GB for everyone. >> >> Probably fair enough then (if explicitly stated). Personally I would >> nevertheless allow sizes up to 4Gb-4kb, but I can see why this may >> not be liked by everyone. I'd like to point out though that the >> PTR_ERR() machinery we've inherited from Linux utilizes exactly that. > > Well, that's a console buffer. The chance you have write/read more than 2GB in a row is very unlikely. > > So it feels to me that exposing PTR_ERR(...) like interface is not worth it. Sure, hence me having said "personally I would ..." - I don't expect you to go that route. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 3/4] xen/public: Document HYPERCALL_console_io() @ 2019-04-16 20:42 ` Stefano Stabellini 0 siblings, 0 replies; 42+ messages in thread From: Stefano Stabellini @ 2019-04-16 20:42 UTC (permalink / raw) To: Julien Grall Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Jan Beulich, xen-devel On Tue, 2 Apr 2019, Julien Grall wrote: > Currently, OS developpers will have to look at Xen code in order to know > the parameters of an hypercall and how it is meant to work. > > This is not a trivial task as you may need to have a deep understanding > of Xen internal. > > This patch attempts to document the behavior of HYPERCALL_console_io() to > help OS developer. > > Signed-off-by: Julien Grall <julien.grall@arm.com> I love this, thank you! > --- > > This is a first attempt to address the lack on documentation for > hypercalls. We may want to decide a format to use in every hypercall so > it can be readable for the OS developer and easily consummed by > documentation tools. > --- > xen/include/public/xen.h | 21 ++++++++++++++++++++- > 1 file changed, 20 insertions(+), 1 deletion(-) > > diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h > index ccdffc0ad1..7c119c6782 100644 > --- a/xen/include/public/xen.h > +++ b/xen/include/public/xen.h > @@ -97,6 +97,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_ulong_t); > #define __HYPERVISOR_set_timer_op 15 > #define __HYPERVISOR_event_channel_op_compat 16 /* compat since 0x00030202 */ > #define __HYPERVISOR_xen_version 17 > +/* HYPERVISOR_console_io(int cmd, int count, XEN_GUEST_HANDLE(char) buffer); */ I agree with Jan: I wouldn't put this comment here. Maybe down below with the other comment. The reason is that I think it would be best not to break the simple list of hypercall numbers. > #define __HYPERVISOR_console_io 18 > #define __HYPERVISOR_physdev_op_compat 19 /* compat since 0x00030202 */ > #define __HYPERVISOR_grant_table_op 20 > @@ -486,7 +487,25 @@ DEFINE_XEN_GUEST_HANDLE(mmuext_op_t); > /* ` } */ > > /* > - * Commands to HYPERVISOR_console_io(). > + * Commands to HYPERVISOR_console_io() > + * > + * @cmd: Command (see below) > + * @count: Size of the buffer to read/write > + * @buffer: Pointer in the guest memory > + * > + * List of commands: > + * > + * * CONSOLEIO_write: Write the buffer on Xen console. > + * For the hardware domain, all the characters in the buffer will > + * be written. Characters will be printed to directly to the > + * console. > + * For all the other domains, only the printable characters will be > + * written. Characters may be buffered until a newline (i.e '\n') is > + * found. > + * Return 0 on success, otherwise return an error code. > + * * CONSOLE_read: Attempts to read up @count characters from Xen console. > + * Return the number of character read on success, otherwise return > + * an error code. This is great! My only suggestion for improvement would be to use a special annotation for the return value. I think it would make it easier to spot. Maybe something like @return: * CONSOLEIO_write: Write the buffer on Xen console. For the hardware domain, all the characters in the buffer will be written. Characters will be printed to directly to the console. For all the other domains, only the printable characters will be written. Characters may be buffered until a newline (i.e '\n') is found. @return: 0 on success, otherwise return an error code. * CONSOLE_read: Attempts to read up @count characters from Xen console. @return: the number of character read on success, otherwise return an error code. > #define CONSOLEIO_write 0 > #define CONSOLEIO_read 1 > -- > 2.11.0 > _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Xen-devel] [PATCH 3/4] xen/public: Document HYPERCALL_console_io() @ 2019-04-16 20:42 ` Stefano Stabellini 0 siblings, 0 replies; 42+ messages in thread From: Stefano Stabellini @ 2019-04-16 20:42 UTC (permalink / raw) To: Julien Grall Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Jan Beulich, xen-devel On Tue, 2 Apr 2019, Julien Grall wrote: > Currently, OS developpers will have to look at Xen code in order to know > the parameters of an hypercall and how it is meant to work. > > This is not a trivial task as you may need to have a deep understanding > of Xen internal. > > This patch attempts to document the behavior of HYPERCALL_console_io() to > help OS developer. > > Signed-off-by: Julien Grall <julien.grall@arm.com> I love this, thank you! > --- > > This is a first attempt to address the lack on documentation for > hypercalls. We may want to decide a format to use in every hypercall so > it can be readable for the OS developer and easily consummed by > documentation tools. > --- > xen/include/public/xen.h | 21 ++++++++++++++++++++- > 1 file changed, 20 insertions(+), 1 deletion(-) > > diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h > index ccdffc0ad1..7c119c6782 100644 > --- a/xen/include/public/xen.h > +++ b/xen/include/public/xen.h > @@ -97,6 +97,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_ulong_t); > #define __HYPERVISOR_set_timer_op 15 > #define __HYPERVISOR_event_channel_op_compat 16 /* compat since 0x00030202 */ > #define __HYPERVISOR_xen_version 17 > +/* HYPERVISOR_console_io(int cmd, int count, XEN_GUEST_HANDLE(char) buffer); */ I agree with Jan: I wouldn't put this comment here. Maybe down below with the other comment. The reason is that I think it would be best not to break the simple list of hypercall numbers. > #define __HYPERVISOR_console_io 18 > #define __HYPERVISOR_physdev_op_compat 19 /* compat since 0x00030202 */ > #define __HYPERVISOR_grant_table_op 20 > @@ -486,7 +487,25 @@ DEFINE_XEN_GUEST_HANDLE(mmuext_op_t); > /* ` } */ > > /* > - * Commands to HYPERVISOR_console_io(). > + * Commands to HYPERVISOR_console_io() > + * > + * @cmd: Command (see below) > + * @count: Size of the buffer to read/write > + * @buffer: Pointer in the guest memory > + * > + * List of commands: > + * > + * * CONSOLEIO_write: Write the buffer on Xen console. > + * For the hardware domain, all the characters in the buffer will > + * be written. Characters will be printed to directly to the > + * console. > + * For all the other domains, only the printable characters will be > + * written. Characters may be buffered until a newline (i.e '\n') is > + * found. > + * Return 0 on success, otherwise return an error code. > + * * CONSOLE_read: Attempts to read up @count characters from Xen console. > + * Return the number of character read on success, otherwise return > + * an error code. This is great! My only suggestion for improvement would be to use a special annotation for the return value. I think it would make it easier to spot. Maybe something like @return: * CONSOLEIO_write: Write the buffer on Xen console. For the hardware domain, all the characters in the buffer will be written. Characters will be printed to directly to the console. For all the other domains, only the printable characters will be written. Characters may be buffered until a newline (i.e '\n') is found. @return: 0 on success, otherwise return an error code. * CONSOLE_read: Attempts to read up @count characters from Xen console. @return: the number of character read on success, otherwise return an error code. > #define CONSOLEIO_write 0 > #define CONSOLEIO_read 1 > -- > 2.11.0 > _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 4/4] xen/console: Simplify domU console handling in guest_console_write 2019-04-02 16:42 [PATCH 0/4] xen/console: Bug fixes and doc improvement Julien Grall ` (2 preceding siblings ...) 2019-04-02 16:42 ` [PATCH 3/4] xen/public: Document HYPERCALL_console_io() Julien Grall @ 2019-04-02 16:42 ` Julien Grall 2019-04-03 11:42 ` Wei Liu 2019-04-16 20:48 ` [Xen-devel] " Stefano Stabellini 3 siblings, 2 replies; 42+ messages in thread From: Julien Grall @ 2019-04-02 16:42 UTC (permalink / raw) To: xen-devel Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, Jan Beulich 2 paths in the domU console handling are now the same. So they can be merged to make the code simpler. Signed-off-by: Julien Grall <julien.grall@arm.com> --- xen/drivers/char/console.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c index b119bf980b..5483d66512 100644 --- a/xen/drivers/char/console.c +++ b/xen/drivers/char/console.c @@ -584,13 +584,8 @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, int count) *kout = '\0'; spin_lock(&cd->pbuf_lock); kcount = kin - kbuf; - if ( c == '\n' ) - { - cd->pbuf[cd->pbuf_idx] = '\0'; - guest_printk(cd, XENLOG_G_DEBUG "%s%s\n", cd->pbuf, kbuf); - cd->pbuf_idx = 0; - } - else if ( cd->pbuf_idx + (kout - kbuf) < (DOMAIN_PBUF_SIZE - 1) ) + if ( c != '\n' && + (cd->pbuf_idx + (kout - kbuf) < (DOMAIN_PBUF_SIZE - 1)) ) { /* buffer the output until a newline */ memcpy(cd->pbuf + cd->pbuf_idx, kbuf, kout - kbuf); -- 2.11.0 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH 4/4] xen/console: Simplify domU console handling in guest_console_write 2019-04-02 16:42 ` [PATCH 4/4] xen/console: Simplify domU console handling in guest_console_write Julien Grall @ 2019-04-03 11:42 ` Wei Liu 2019-04-16 20:48 ` [Xen-devel] " Stefano Stabellini 1 sibling, 0 replies; 42+ messages in thread From: Wei Liu @ 2019-04-03 11:42 UTC (permalink / raw) To: Julien Grall Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Jan Beulich, xen-devel On Tue, Apr 02, 2019 at 05:42:38PM +0100, Julien Grall wrote: > 2 paths in the domU console handling are now the same. So they can be > merged to make the code simpler. > > Signed-off-by: Julien Grall <julien.grall@arm.com> Acked-by: Wei Liu <wei.liu2@citrix.com> (This obviously is dependent on the acceptance of patch 2) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 4/4] xen/console: Simplify domU console handling in guest_console_write @ 2019-04-16 20:48 ` Stefano Stabellini 0 siblings, 0 replies; 42+ messages in thread From: Stefano Stabellini @ 2019-04-16 20:48 UTC (permalink / raw) To: Julien Grall Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Jan Beulich, xen-devel On Tue, 2 Apr 2019, Julien Grall wrote: > 2 paths in the domU console handling are now the same. So they can be > merged to make the code simpler. > > Signed-off-by: Julien Grall <julien.grall@arm.com> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > --- > xen/drivers/char/console.c | 9 ++------- > 1 file changed, 2 insertions(+), 7 deletions(-) > > diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c > index b119bf980b..5483d66512 100644 > --- a/xen/drivers/char/console.c > +++ b/xen/drivers/char/console.c > @@ -584,13 +584,8 @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, int count) > *kout = '\0'; > spin_lock(&cd->pbuf_lock); > kcount = kin - kbuf; > - if ( c == '\n' ) > - { > - cd->pbuf[cd->pbuf_idx] = '\0'; > - guest_printk(cd, XENLOG_G_DEBUG "%s%s\n", cd->pbuf, kbuf); > - cd->pbuf_idx = 0; > - } > - else if ( cd->pbuf_idx + (kout - kbuf) < (DOMAIN_PBUF_SIZE - 1) ) > + if ( c != '\n' && > + (cd->pbuf_idx + (kout - kbuf) < (DOMAIN_PBUF_SIZE - 1)) ) > { > /* buffer the output until a newline */ > memcpy(cd->pbuf + cd->pbuf_idx, kbuf, kout - kbuf); > -- > 2.11.0 > _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [Xen-devel] [PATCH 4/4] xen/console: Simplify domU console handling in guest_console_write @ 2019-04-16 20:48 ` Stefano Stabellini 0 siblings, 0 replies; 42+ messages in thread From: Stefano Stabellini @ 2019-04-16 20:48 UTC (permalink / raw) To: Julien Grall Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan, Jan Beulich, xen-devel On Tue, 2 Apr 2019, Julien Grall wrote: > 2 paths in the domU console handling are now the same. So they can be > merged to make the code simpler. > > Signed-off-by: Julien Grall <julien.grall@arm.com> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > --- > xen/drivers/char/console.c | 9 ++------- > 1 file changed, 2 insertions(+), 7 deletions(-) > > diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c > index b119bf980b..5483d66512 100644 > --- a/xen/drivers/char/console.c > +++ b/xen/drivers/char/console.c > @@ -584,13 +584,8 @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, int count) > *kout = '\0'; > spin_lock(&cd->pbuf_lock); > kcount = kin - kbuf; > - if ( c == '\n' ) > - { > - cd->pbuf[cd->pbuf_idx] = '\0'; > - guest_printk(cd, XENLOG_G_DEBUG "%s%s\n", cd->pbuf, kbuf); > - cd->pbuf_idx = 0; > - } > - else if ( cd->pbuf_idx + (kout - kbuf) < (DOMAIN_PBUF_SIZE - 1) ) > + if ( c != '\n' && > + (cd->pbuf_idx + (kout - kbuf) < (DOMAIN_PBUF_SIZE - 1)) ) > { > /* buffer the output until a newline */ > memcpy(cd->pbuf + cd->pbuf_idx, kbuf, kout - kbuf); > -- > 2.11.0 > _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 42+ messages in thread
end of thread, other threads:[~2019-08-05 11:40 UTC | newest] Thread overview: 42+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-04-02 16:42 [PATCH 0/4] xen/console: Bug fixes and doc improvement Julien Grall 2019-04-02 16:42 ` [PATCH 1/4] xen/console: Properly buffer domU output when using CONSOLEIO_write Julien Grall 2019-04-03 11:41 ` Wei Liu 2019-04-09 10:25 ` Julien Grall 2019-04-09 10:25 ` [Xen-devel] " Julien Grall 2019-04-02 16:42 ` [PATCH 2/4] xen/console: Don't treat NUL character as the end of the buffer Julien Grall 2019-04-02 17:49 ` Andrew Cooper 2019-04-03 7:59 ` Jan Beulich 2019-04-09 10:31 ` Julien Grall 2019-04-09 10:31 ` [Xen-devel] " Julien Grall 2019-04-05 10:00 ` Jan Beulich 2019-04-05 10:00 ` [Xen-devel] " Jan Beulich 2019-04-05 10:21 ` Julien Grall 2019-04-05 10:21 ` [Xen-devel] " Julien Grall 2019-04-05 10:26 ` Jan Beulich 2019-04-05 10:26 ` [Xen-devel] " Jan Beulich 2019-04-16 20:33 ` Stefano Stabellini 2019-04-16 20:33 ` [Xen-devel] " Stefano Stabellini 2019-08-05 11:40 ` Julien Grall 2019-04-02 16:42 ` [PATCH 3/4] xen/public: Document HYPERCALL_console_io() Julien Grall 2019-04-03 11:41 ` Wei Liu 2019-04-03 13:04 ` Jan Beulich 2019-04-09 11:26 ` Julien Grall 2019-04-09 11:26 ` [Xen-devel] " Julien Grall 2019-04-09 11:42 ` Jan Beulich 2019-04-09 11:42 ` [Xen-devel] " Jan Beulich 2019-04-16 9:54 ` Julien Grall 2019-04-16 9:54 ` [Xen-devel] " Julien Grall 2019-04-25 10:09 ` Jan Beulich 2019-04-25 10:09 ` [Xen-devel] " Jan Beulich 2019-04-16 10:29 ` Ian Jackson 2019-04-16 10:29 ` [Xen-devel] " Ian Jackson 2019-08-05 9:40 ` Julien Grall 2019-08-05 10:07 ` Jan Beulich 2019-08-05 10:17 ` Julien Grall 2019-08-05 10:21 ` Jan Beulich 2019-04-16 20:42 ` Stefano Stabellini 2019-04-16 20:42 ` [Xen-devel] " Stefano Stabellini 2019-04-02 16:42 ` [PATCH 4/4] xen/console: Simplify domU console handling in guest_console_write Julien Grall 2019-04-03 11:42 ` Wei Liu 2019-04-16 20:48 ` Stefano Stabellini 2019-04-16 20:48 ` [Xen-devel] " Stefano Stabellini
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.