| Message ID | 20250103-vuart-ns8250-v3-v1-16-c5d36b31d66c@ford.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86: introduce NS16550-compatible UART emulator | expand |
On 04.01.2025 02:58, Denis Mukhin via B4 Relay wrote: > --- a/xen/drivers/char/console.c > +++ b/xen/drivers/char/console.c > @@ -40,6 +40,11 @@ > #include <asm/guest.h> > #endif > > +/* Console flags. */ > +enum { > + CONSOLE_RING = BIT(0, U), > +}; These aren't console flags, but flags passed to the new console_write(). The comment wants adjusting accordingly, and I guess the enum would also best move immediately ahead of the function. > @@ -636,6 +641,16 @@ static void cf_check notify_dom0_con_ring(void *unused) > static DECLARE_SOFTIRQ_TASKLET(notify_dom0_con_ring_tasklet, > notify_dom0_con_ring, NULL); > > +static bool console_locks_busted; > + > +static void conring_write(const char *str, size_t len) > +{ > + conring_puts(str, len); > + > + if ( !console_locks_busted ) > + tasklet_schedule(¬ify_dom0_con_ring_tasklet); > +} This doesn't really need to be a separate function, does it? > @@ -644,8 +659,47 @@ static inline void xen_console_write_debug_port(const char *buf, size_t len) > : "=&S" (tmp), "=&c" (tmp) > : "0" (buf), "1" (len), "d" (XEN_HVM_DEBUGCONS_IOPORT) ); > } > + > +static void xen_console_write(const char *str, size_t len) > +{ > + if ( xen_guest ) > + xen_hypercall_console_write(str, len); > + else > + xen_console_write_debug_port(str, len); > +} Nor does this, I suppose. The more that the sole call to here ... > #endif > > +/* > + * Write characters to console. > + * > + * That will handle all possible scenarios working w/ console > + * - serial console; > + * - VGA console (x86 only); > + * - __HYPERVISOR_console_io hypercall (x86 only); > + * - debug I/O port (x86 only); > + * - PV console. > + */ > +static void console_write(const char *str, size_t len, unsigned int flags) > +{ > + ASSERT(rspin_is_locked(&console_lock)); > + > + console_serial_puts(str, len); > + video_puts(str, len); > + > +#ifdef CONFIG_X86 > + if ( opt_console_xen ) > + xen_console_write(str, len); > +#endif ... continues to sit in an #ifdef. > @@ -666,28 +720,8 @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, > > if ( is_hardware_domain(cd) ) > { > - /* Use direct console output as it could be interactive */ This comment looks like being removed with no replacement? Why? > nrspin_lock_irq(&console_lock); > - > - console_serial_puts(kbuf, kcount); > - video_puts(kbuf, kcount); > - > -#ifdef CONFIG_X86 > - if ( opt_console_xen ) > - { > - if ( xen_guest ) > - xen_hypercall_console_write(kbuf, kcount); > - else > - xen_console_write_debug_port(kbuf, kcount); > - } > -#endif > - > - if ( opt_console_to_ring ) > - { > - conring_puts(kbuf, kcount); > - tasklet_schedule(¬ify_dom0_con_ring_tasklet); > - } > - > + console_write(kbuf, kcount, !!opt_console_to_ring); Here you assume CONSOLE_RING is bit 0. Please don't create such (then also un-annotated) implicit dependencies. > @@ -788,33 +822,6 @@ long do_console_io( > * ***************************************************** > */ > > -static bool console_locks_busted; > - > -static void __putstr(const char *str) > -{ Please note the comment up from here, the tail of which is visible in context. I wonder whether the new function wouldn't better live here, then also reducing the diff quite a bit. Requiring a forward decl for this to work isn't very nice, but a reasonable price to pay, I think. Jan
diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c index a33132b8fad95a1ad7e0aab4aef3fa3e46a5c03a..2f776b2f07cb15fae8060f3574db73234a38727a 100644 --- a/xen/drivers/char/console.c +++ b/xen/drivers/char/console.c @@ -40,6 +40,11 @@ #include <asm/guest.h> #endif +/* Console flags. */ +enum { + CONSOLE_RING = BIT(0, U), +}; + /* console: comma-separated list of console outputs. */ static char __initdata opt_console[30] = OPT_CONSOLE_STR; string_param("console", opt_console); @@ -636,6 +641,16 @@ static void cf_check notify_dom0_con_ring(void *unused) static DECLARE_SOFTIRQ_TASKLET(notify_dom0_con_ring_tasklet, notify_dom0_con_ring, NULL); +static bool console_locks_busted; + +static void conring_write(const char *str, size_t len) +{ + conring_puts(str, len); + + if ( !console_locks_busted ) + tasklet_schedule(¬ify_dom0_con_ring_tasklet); +} + #ifdef CONFIG_X86 static inline void xen_console_write_debug_port(const char *buf, size_t len) { @@ -644,8 +659,47 @@ static inline void xen_console_write_debug_port(const char *buf, size_t len) : "=&S" (tmp), "=&c" (tmp) : "0" (buf), "1" (len), "d" (XEN_HVM_DEBUGCONS_IOPORT) ); } + +static void xen_console_write(const char *str, size_t len) +{ + if ( xen_guest ) + xen_hypercall_console_write(str, len); + else + xen_console_write_debug_port(str, len); +} #endif +/* + * Write characters to console. + * + * That will handle all possible scenarios working w/ console + * - serial console; + * - VGA console (x86 only); + * - __HYPERVISOR_console_io hypercall (x86 only); + * - debug I/O port (x86 only); + * - PV console. + */ +static void console_write(const char *str, size_t len, unsigned int flags) +{ + ASSERT(rspin_is_locked(&console_lock)); + + console_serial_puts(str, len); + video_puts(str, len); + +#ifdef CONFIG_X86 + if ( opt_console_xen ) + xen_console_write(str, len); +#endif + + if ( flags & CONSOLE_RING ) + conring_write(str, len); +} + +static inline void __putstr(const char *str) +{ + console_write(str, strlen(str), CONSOLE_RING); +} + static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, unsigned int count) { @@ -666,28 +720,8 @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer, if ( is_hardware_domain(cd) ) { - /* Use direct console output as it could be interactive */ nrspin_lock_irq(&console_lock); - - console_serial_puts(kbuf, kcount); - video_puts(kbuf, kcount); - -#ifdef CONFIG_X86 - if ( opt_console_xen ) - { - if ( xen_guest ) - xen_hypercall_console_write(kbuf, kcount); - else - xen_console_write_debug_port(kbuf, kcount); - } -#endif - - if ( opt_console_to_ring ) - { - conring_puts(kbuf, kcount); - tasklet_schedule(¬ify_dom0_con_ring_tasklet); - } - + console_write(kbuf, kcount, !!opt_console_to_ring); nrspin_unlock_irq(&console_lock); } else @@ -788,33 +822,6 @@ long do_console_io( * ***************************************************** */ -static bool console_locks_busted; - -static void __putstr(const char *str) -{ - size_t len = strlen(str); - - ASSERT(rspin_is_locked(&console_lock)); - - console_serial_puts(str, len); - video_puts(str, len); - -#ifdef CONFIG_X86 - if ( opt_console_xen ) - { - if ( xen_guest ) - xen_hypercall_console_write(str, len); - else - xen_console_write_debug_port(str, len); - } -#endif - - conring_puts(str, len); - - if ( !console_locks_busted ) - tasklet_schedule(¬ify_dom0_con_ring_tasklet); -} - static int printk_prefix_check(char *p, char **pp) { int loglvl = -1;