[v2,17/35] xen/console: introduce consoled_is_enabled()

Commit Message

Denis Mukhin via B4 Relay Dec. 6, 2024, 4:41 a.m. UTC

From: Denis Mukhin <[e-mail]>

There are few places which check pv_shim console under CONFIG_PV_SHIM in xen
console driver. Instead of #ifdef-ing, use new consoled_is_enabled() to
customize the logic.

Header file now can be included w/o CONFIG_X86.

Signature of consoled_guest_{rx,tx} has changed to account for follow-on
console switch logic cleanup.

Signed-off-by: Denis Mukhin <[e-mail]>
---
 xen/drivers/char/console.c  | 10 +++-------
 xen/drivers/char/consoled.c | 18 ++++++++++++++----
 xen/include/xen/consoled.h  | 35 +++++++++++++++++++++++++++++++++--
 3 files changed, 50 insertions(+), 13 deletions(-)

Comments

Jan Beulich Dec. 10, 2024, 2:31 p.m. UTC | #1

On 06.12.2024 05:41, Denis Mukhin via B4 Relay wrote:
> --- a/xen/drivers/char/consoled.c
> +++ b/xen/drivers/char/consoled.c
> @@ -43,13 +43,13 @@ struct xencons_interface *consoled_get_ring_addr(void)
>  static char buf[BUF_SZ + 1];
>  
>  /* Receives characters from a domain's PV console */
> -void consoled_guest_rx(void)
> +int consoled_guest_rx(void)
>  {
>      size_t idx = 0;
>      XENCONS_RING_IDX cons, prod;
>  
>      if ( !cons_ring )
> -        return;
> +        return 0;
>  
>      spin_lock(&rx_lock);
>  
> @@ -91,15 +91,17 @@ void consoled_guest_rx(void)
>  
>   out:
>      spin_unlock(&rx_lock);
> +
> +    return 0;
>  }
>  
>  /* Sends a character into a domain's PV console */
> -void consoled_guest_tx(char c)
> +int consoled_guest_tx(char c)
>  {
>      XENCONS_RING_IDX cons, prod;
>  
>      if ( !cons_ring )
> -        return;
> +        return 0;
>  
>      cons = ACCESS_ONCE(cons_ring->in_cons);
>      prod = cons_ring->in_prod;
> @@ -118,6 +120,7 @@ void consoled_guest_tx(char c)
>  
>      cons_ring->in[MASK_XENCONS_IDX(prod++, cons_ring->in)] = c;
>  
> +
>      /* Write to the ring before updating the pointer */

No excess blank lines please.

> @@ -125,6 +128,13 @@ void consoled_guest_tx(char c)
>   notify:
>      /* Always notify the guest: prevents receive path from getting stuck. */
>      pv_shim_inject_evtchn(pv_console_evtchn());
> +
> +    return 0;
> +}

For both of the functions - what use is it to make the functions return
a value, when all they'd ever return is zero (and callers don't care)?
I'm also having a hard time seeing how this adjustment is related to ...

> +bool consoled_is_enabled(void)
> +{
> +    return pv_shim && pv_console;
>  }

... the introduction of this function (which by itself is probably fine).

Jan

Patch

diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index f034ce5aab3f3bf59b0df9fa583ee9ce32dbf665..60c055396b697869b04b9132b0dcfa832fabe932 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -33,9 +33,9 @@ 
 #include <xen/pv_console.h>
 #include <asm/setup.h>
 #include <xen/sections.h>
+#include <xen/consoled.h>
 
 #ifdef CONFIG_X86
-#include <xen/consoled.h>
 #include <asm/guest.h>
 #endif
 #ifdef CONFIG_SBSA_VUART_CONSOLE
@@ -505,11 +505,9 @@  static void console_find_owner(void)
             break;
         }
 
-#ifdef CONFIG_PV_SHIM
-        if ( next_rx == 1 )
+        if ( consoled_is_enabled() && next_rx == 1 )
             domid = get_initial_domain_id();
         else
-#endif
             domid = next_rx - 1;
         d = rcu_lock_domain_by_id(domid);
         if ( d )
@@ -573,10 +571,8 @@  static void __serial_rx(char c)
 #endif
     }
 
-#ifdef CONFIG_X86
-    if ( pv_shim && pv_console )
+    if ( consoled_is_enabled() )
         consoled_guest_tx(c);
-#endif
 }
 
 static void cf_check serial_rx(char c)
diff --git a/xen/drivers/char/consoled.c b/xen/drivers/char/consoled.c
index b415b632cecc0a80e161b701d7b70ba4f3cc5fb8..d6624e7697f56e1a1959b0efa5dca104f34af002 100644
--- a/xen/drivers/char/consoled.c
+++ b/xen/drivers/char/consoled.c
@@ -43,13 +43,13 @@  struct xencons_interface *consoled_get_ring_addr(void)
 static char buf[BUF_SZ + 1];
 
 /* Receives characters from a domain's PV console */
-void consoled_guest_rx(void)
+int consoled_guest_rx(void)
 {
     size_t idx = 0;
     XENCONS_RING_IDX cons, prod;
 
     if ( !cons_ring )
-        return;
+        return 0;
 
     spin_lock(&rx_lock);
 
@@ -91,15 +91,17 @@  void consoled_guest_rx(void)
 
  out:
     spin_unlock(&rx_lock);
+
+    return 0;
 }
 
 /* Sends a character into a domain's PV console */
-void consoled_guest_tx(char c)
+int consoled_guest_tx(char c)
 {
     XENCONS_RING_IDX cons, prod;
 
     if ( !cons_ring )
-        return;
+        return 0;
 
     cons = ACCESS_ONCE(cons_ring->in_cons);
     prod = cons_ring->in_prod;
@@ -118,6 +120,7 @@  void consoled_guest_tx(char c)
 
     cons_ring->in[MASK_XENCONS_IDX(prod++, cons_ring->in)] = c;
 
+
     /* Write to the ring before updating the pointer */
     smp_wmb();
     ACCESS_ONCE(cons_ring->in_prod) = prod;
@@ -125,6 +128,13 @@  void consoled_guest_tx(char c)
  notify:
     /* Always notify the guest: prevents receive path from getting stuck. */
     pv_shim_inject_evtchn(pv_console_evtchn());
+
+    return 0;
+}
+
+bool consoled_is_enabled(void)
+{
+    return pv_shim && pv_console;
 }
 
 /*
diff --git a/xen/include/xen/consoled.h b/xen/include/xen/consoled.h
index bd7ab6329ee8a7c466484021247241ded8ed03c7..696677fa5a3be458a0ec93360e08376c3471f95b 100644
--- a/xen/include/xen/consoled.h
+++ b/xen/include/xen/consoled.h
@@ -3,10 +3,41 @@ 
 
 #include <public/io/console.h>
 
+#if defined(CONFIG_PV_SHIM)
+
 void consoled_set_ring_addr(struct xencons_interface *ring);
 struct xencons_interface *consoled_get_ring_addr(void);
-void consoled_guest_rx(void);
-void consoled_guest_tx(char c);
+int consoled_guest_rx(void);
+int consoled_guest_tx(char c);
+bool consoled_is_enabled(void);
+
+#else
+
+static inline void consoled_set_ring_addr(struct xencons_interface *ring)
+{
+}
+
+static inline struct xencons_interface *consoled_get_ring_addr(void)
+{
+    return NULL;
+}
+
+static inline int consoled_guest_rx(void)
+{
+    return 0;
+}
+
+static inline int consoled_guest_tx(char c)
+{
+    return 0;
+}
+
+static inline bool consoled_is_enabled(void)
+{
+    return false;
+}
+
+#endif /* #if defined(CONFIG_PV_SHIM) */
 
 #endif /* __XEN_CONSOLED_H__ */
 /*