xen/console: introduce console_{get,put}_domain()

Commit Message

Denis Mukhin Feb. 18, 2025, 8:31 a.m. UTC

From: Denis Mukhin <[e-mail]>

console_input_domain() takes an RCU lock to protect domain structure.
That implies call to rcu_unlock_domain() after use.

Introduce a pair of console_get_domain() / console_put_domain() to highlight
the correct use of the call within the code interacting with Xen console
driver.

The new calls used in __serial_rx(), which also fixed console forwarding to
late hardware domains which run with domain IDs different from 0.

Signed-off-by: Denis Mukhin <[e-mail]>
---
Link to the original patch:
  https://lore.kernel.org/xen-devel/[e-mail]/
---
 xen/arch/arm/vpl011.c      |  6 ++---
 xen/drivers/char/console.c | 53 +++++++++++++++++++-------------------
 xen/include/xen/console.h  |  3 ++-
 3 files changed, 32 insertions(+), 30 deletions(-)

Comments

Jan Beulich Feb. 19, 2025, 1:52 p.m. UTC | #1

On 18.02.2025 09:31, [e-mail] 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 <[e-mail]>
Given they're reasonably simple to make, I could once again offer to
adjust while committing (provided an Arm ack also arrives).

Jan

Patch

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.