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.
On Tuesday, December 10th, 2024 at 6:31 AM, Jan Beulich <jbeulich@suse.com> wrote: > > > 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. Fixed. > > > @@ -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)? Fixed. > 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). That will be a cleanup in console driver on the code path I touched wrt console focus switch. > > Jan
On Thursday, December 12th, 2024 at 1:31 AM, Roger Pau Monné <roger.pau@citrix.com> wrote: > > > 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. Thanks; fixed. > > > +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? Fixed. > > Thanks, Roger.
On 04.01.2025 04:00, Denis Mukhin wrote: > On Tuesday, December 10th, 2024 at 6:31 AM, Jan Beulich <jbeulich@suse.com> wrote: > >> >> >> 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. > > Fixed. > >> >>> @@ -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)? > > Fixed. > >> 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). > > That will be a cleanup in console driver on the code path I touched wrt console > focus switch. Yet then please don't mix entirely independent things in a single patch. Jan
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__ */ /*