| Message ID | 20250218083048.596012-1-dmkhn@proton.me (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | xen/console: introduce console_{get,put}_domain() | expand |
On 18.02.2025 09:31, dmkhn@proton.me wrote: > @@ -546,31 +555,23 @@ static void __serial_rx(char c) > * getting stuck. > */ > send_global_virq(VIRQ_CONSOLE); > - break; > - > + } > #ifdef CONFIG_SBSA_VUART_CONSOLE > - default: > - { > - struct domain *d = rcu_lock_domain_by_id(console_rx - 1); > - > - if ( d ) > - { > - int rc = vpl011_rx_char_xen(d, c); > - if ( rc ) > - guest_printk(d, XENLOG_G_WARNING > - "failed to process console input: %d\n", rc); > - rcu_unlock_domain(d); > - } > - > - break; > - } > + else > + /* Deliver input to the emulated UART. */ > + rc = vpl011_rx_char_xen(d, c); > #endif > - } > > #ifdef CONFIG_X86 > if ( pv_shim && pv_console ) > consoled_guest_tx(c); > #endif > + > + if ( rc ) > + guest_printk(d, XENLOG_G_WARNING > + "failed to process console input: %d\n", rc); Wouldn't this better live ahead of the four shim related lines? Also please correct the log level specifier here. I realize you only move what was there before, but I consider i bad practice to move buggy code. gprintk() already prepends XENLOG_GUEST, so instead of XENLOG_G_WARNING is should just be XENLOG_WARNING. (Line wrapping is also odd here, at least according to my taste. But since that's not written down anywhere, I wouldn't insist on adjusting that as well.) With both adjustments (provided you agree, of course) Reviewed-by: Jan Beulich <jbeulich@suse.com> Given they're reasonably simple to make, I could once again offer to adjust while committing (provided an Arm ack also arrives). Jan
On Wed, 19 Feb 2025, Jan Beulich wrote: > On 18.02.2025 09:31, dmkhn@proton.me wrote: > > @@ -546,31 +555,23 @@ static void __serial_rx(char c) > > * getting stuck. > > */ > > send_global_virq(VIRQ_CONSOLE); > > - break; > > - > > + } > > #ifdef CONFIG_SBSA_VUART_CONSOLE > > - default: > > - { > > - struct domain *d = rcu_lock_domain_by_id(console_rx - 1); > > - > > - if ( d ) > > - { > > - int rc = vpl011_rx_char_xen(d, c); > > - if ( rc ) > > - guest_printk(d, XENLOG_G_WARNING > > - "failed to process console input: %d\n", rc); > > - rcu_unlock_domain(d); > > - } > > - > > - break; > > - } > > + else > > + /* Deliver input to the emulated UART. */ > > + rc = vpl011_rx_char_xen(d, c); > > #endif > > - } > > > > #ifdef CONFIG_X86 > > if ( pv_shim && pv_console ) > > consoled_guest_tx(c); > > #endif > > + > > + if ( rc ) > > + guest_printk(d, XENLOG_G_WARNING > > + "failed to process console input: %d\n", rc); > > Wouldn't this better live ahead of the four shim related lines? > > Also please correct the log level specifier here. I realize you only move > what was there before, but I consider i bad practice to move buggy code. > gprintk() already prepends XENLOG_GUEST, so instead of XENLOG_G_WARNING > is should just be XENLOG_WARNING. (Line wrapping is also odd here, at > least according to my taste. But since that's not written down anywhere, > I wouldn't insist on adjusting that as well.) > > With both adjustments (provided you agree, of course) > Reviewed-by: Jan Beulich <jbeulich@suse.com> > Given they're reasonably simple to make, I could once again offer to > adjust while committing (provided an Arm ack also arrives). Acked-by: Stefano Stabellini <sstabellini@kernel.org>
On Wednesday, February 19th, 2025 at 5:52 AM, Jan Beulich <jbeulich@suse.com> wrote: > > > On 18.02.2025 09:31, dmkhn@proton.me wrote: > > > @@ -546,31 +555,23 @@ static void __serial_rx(char c) > > * getting stuck. > > */ > > send_global_virq(VIRQ_CONSOLE); > > - break; > > - > > + } > > #ifdef CONFIG_SBSA_VUART_CONSOLE > > - default: > > - { > > - struct domain d = rcu_lock_domain_by_id(console_rx - 1); > > - > > - if ( d ) > > - { > > - int rc = vpl011_rx_char_xen(d, c); > > - if ( rc ) > > - guest_printk(d, XENLOG_G_WARNING > > - "failed to process console input: %d\n", rc); > > - rcu_unlock_domain(d); > > - } > > - > > - break; > > - } > > + else > > + / Deliver input to the emulated UART. */ > > + rc = vpl011_rx_char_xen(d, c); > > #endif > > - } > > > > #ifdef CONFIG_X86 > > if ( pv_shim && pv_console ) > > consoled_guest_tx(c); > > #endif > > + > > + if ( rc ) > > + guest_printk(d, XENLOG_G_WARNING > > + "failed to process console input: %d\n", rc); > > > Wouldn't this better live ahead of the four shim related lines? > > Also please correct the log level specifier here. I realize you only move > what was there before, but I consider i bad practice to move buggy code. > gprintk() already prepends XENLOG_GUEST, so instead of XENLOG_G_WARNING > is should just be XENLOG_WARNING. (Line wrapping is also odd here, at > least according to my taste. But since that's not written down anywhere, > I wouldn't insist on adjusting that as well.) > > With both adjustments (provided you agree, of course) Thanks a lot for help and review! > Reviewed-by: Jan Beulich jbeulich@suse.com > > Given they're reasonably simple to make, I could once again offer to > adjust while committing (provided an Arm ack also arrives). > > Jan
On Friday, February 21st, 2025 at 4:04 PM, Stefano Stabellini <sstabellini@kernel.org> wrote: > > > On Wed, 19 Feb 2025, Jan Beulich wrote: > > > On 18.02.2025 09:31, dmkhn@proton.me wrote: > > > > > @@ -546,31 +555,23 @@ static void __serial_rx(char c) > > > * getting stuck. > > > */ > > > send_global_virq(VIRQ_CONSOLE); > > > - break; > > > - > > > + } > > > #ifdef CONFIG_SBSA_VUART_CONSOLE > > > - default: > > > - { > > > - struct domain d = rcu_lock_domain_by_id(console_rx - 1); > > > - > > > - if ( d ) > > > - { > > > - int rc = vpl011_rx_char_xen(d, c); > > > - if ( rc ) > > > - guest_printk(d, XENLOG_G_WARNING > > > - "failed to process console input: %d\n", rc); > > > - rcu_unlock_domain(d); > > > - } > > > - > > > - break; > > > - } > > > + else > > > + / Deliver input to the emulated UART. */ > > > + rc = vpl011_rx_char_xen(d, c); > > > #endif > > > - } > > > > > > #ifdef CONFIG_X86 > > > if ( pv_shim && pv_console ) > > > consoled_guest_tx(c); > > > #endif > > > + > > > + if ( rc ) > > > + guest_printk(d, XENLOG_G_WARNING > > > + "failed to process console input: %d\n", rc); > > > > Wouldn't this better live ahead of the four shim related lines? > > > > Also please correct the log level specifier here. I realize you only move > > what was there before, but I consider i bad practice to move buggy code. > > gprintk() already prepends XENLOG_GUEST, so instead of XENLOG_G_WARNING > > is should just be XENLOG_WARNING. (Line wrapping is also odd here, at > > least according to my taste. But since that's not written down anywhere, > > I wouldn't insist on adjusting that as well.) > > > > With both adjustments (provided you agree, of course) > > Reviewed-by: Jan Beulich jbeulich@suse.com > > Given they're reasonably simple to make, I could once again offer to > > adjust while committing (provided an Arm ack also arrives). > > > Acked-by: Stefano Stabellini sstabellini@kernel.org Thank you!
diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c index c72f3778bf..66047bf33c 100644 --- a/xen/arch/arm/vpl011.c +++ b/xen/arch/arm/vpl011.c @@ -78,7 +78,7 @@ static void vpl011_write_data_xen(struct domain *d, uint8_t data) unsigned long flags; struct vpl011 *vpl011 = &d->arch.vpl011; struct vpl011_xen_backend *intf = vpl011->backend.xen; - struct domain *input = console_input_domain(); + struct domain *input = console_get_domain(); VPL011_LOCK(d, flags); @@ -123,8 +123,8 @@ static void vpl011_write_data_xen(struct domain *d, uint8_t data) vpl011_update_interrupt_status(d); VPL011_UNLOCK(d, flags); - if ( input != NULL ) - rcu_unlock_domain(input); + + console_put_domain(input); } /* diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c index 2e23910dfa..992b37962e 100644 --- a/xen/drivers/char/console.c +++ b/xen/drivers/char/console.c @@ -474,15 +474,18 @@ static unsigned int __read_mostly console_rx = 0; #define max_console_rx (max_init_domid + 1) -#ifdef CONFIG_SBSA_VUART_CONSOLE -/* Make sure to rcu_unlock_domain after use */ -struct domain *console_input_domain(void) +struct domain *console_get_domain(void) { if ( console_rx == 0 ) return NULL; return rcu_lock_domain_by_id(console_rx - 1); } -#endif + +void console_put_domain(struct domain *d) +{ + if ( d ) + rcu_unlock_domain(d); +} static void switch_serial_input(void) { @@ -528,12 +531,18 @@ static void switch_serial_input(void) static void __serial_rx(char c) { - switch ( console_rx ) - { - case 0: + struct domain *d; + int rc = 0; + + if ( console_rx == 0 ) return handle_keypress(c, false); - case 1: + d = console_get_domain(); + if ( !d ) + return; + + if ( is_hardware_domain(d) ) + { /* * Deliver input to the hardware domain buffer, unless it is * already full. @@ -546,31 +555,23 @@ static void __serial_rx(char c) * getting stuck. */ send_global_virq(VIRQ_CONSOLE); - break; - + } #ifdef CONFIG_SBSA_VUART_CONSOLE - default: - { - struct domain *d = rcu_lock_domain_by_id(console_rx - 1); - - if ( d ) - { - int rc = vpl011_rx_char_xen(d, c); - if ( rc ) - guest_printk(d, XENLOG_G_WARNING - "failed to process console input: %d\n", rc); - rcu_unlock_domain(d); - } - - break; - } + else + /* Deliver input to the emulated UART. */ + rc = vpl011_rx_char_xen(d, c); #endif - } #ifdef CONFIG_X86 if ( pv_shim && pv_console ) consoled_guest_tx(c); #endif + + if ( rc ) + guest_printk(d, XENLOG_G_WARNING + "failed to process console input: %d\n", rc); + + console_put_domain(d); } static void cf_check serial_rx(char c) diff --git a/xen/include/xen/console.h b/xen/include/xen/console.h index c4650231be..83cbc9fbda 100644 --- a/xen/include/xen/console.h +++ b/xen/include/xen/console.h @@ -32,7 +32,8 @@ void console_end_sync(void); void console_start_log_everything(void); void console_end_log_everything(void); -struct domain *console_input_domain(void); +struct domain *console_get_domain(void); +void console_put_domain(struct domain *d); /* * Steal output from the console. Returns +ve identifier, else -ve error.