| Message ID | 20241205-vuart-ns8250-v1-17-e9aa923127eb@ford.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce NS8250 UART emulator | expand |
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
On Thu, Dec 05, 2024 at 08:41:47PM -0800, Denis Mukhin via B4 Relay wrote: > From: Denis Mukhin <dmukhin@ford.com> > > 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@ford.com> > --- > 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.
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__ */ /*