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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Dec 05, 2024 at 08:41:47PM -0800, Denis Mukhin via B4 Relay wrote:
> From: Denis Mukhin <dmukhin@xxxxxxxx>
> 
> 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 <dmukhin@xxxxxxxx>
> ---
>  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(-)
> 
> 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;
> +}

You don't need to provide dummy implementations of
consoled_{set,get}_ring_addr() and consoled_guest_rx(), they are only
called from code that's build when CONFIG_PV_SHIM is selected.

> +static inline int consoled_guest_tx(char c)
> +{
> +    return 0;

For consoled_guest_tx() you want to add an ASSERT_UNREACHABLE(), as
it should never be called if !CONFIG_PV_SHIM?

Thanks, Roger.