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(-)
From: Denis Mukhin <dmukhin@ford.com>
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 <dmukhin@ford.com>
---
Link to the original patch:
https://lore.kernel.org/xen-devel/20250103-vuart-ns8250-v3-v1-4-c5d36b31d66c@ford.com/
---
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(-)
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.
--
2.34.1On 18.02.2025 09:31, dmkhn@proton.me wrote: > From: Denis Mukhin <dmukhin@ford.com> > > 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 <dmukhin@ford.com> > --- > Link to the original patch: > https://lore.kernel.org/xen-devel/20250103-vuart-ns8250-v3-v1-4-c5d36b31d66c@ford.com/ > --- > 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(-) > This patch doesn't apply to staging. Looks like it depends on "arm/vuart: move vpl011-related code to vpl011 emulator" without this being said anywhere. Jan
On Wednesday, February 26th, 2025 at 3:30 AM, Jan Beulich <jbeulich@suse.com> wrote: > > > On 18.02.2025 09:31, dmkhn@proton.me wrote: > > > From: Denis Mukhin dmukhin@ford.com > > > > 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 dmukhin@ford.com > > --- > > Link to the original patch: > > https://lore.kernel.org/xen-devel/20250103-vuart-ns8250-v3-v1-4-c5d36b31d66c@ford.com/ > > --- > > 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(-) > > > This patch doesn't apply to staging. Looks like it depends on "arm/vuart: > move vpl011-related code to vpl011 emulator" without this being said anywhere. Correct, this patch depends on https://lore.kernel.org/xen-devel/20250212211802.1669675-1-dmukhin@ford.com/ and I have R-b: https://lore.kernel.org/xen-devel/alpine.DEB.2.22.394.2502121412500.619090@ubuntu-linux-20-04-desktop/ > > Jan
On 26.02.2025 19:39, Denis Mukhin wrote: > On Wednesday, February 26th, 2025 at 3:30 AM, Jan Beulich <jbeulich@suse.com> wrote: > >> >> >> On 18.02.2025 09:31, dmkhn@proton.me wrote: >> >>> From: Denis Mukhin dmukhin@ford.com >>> >>> 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 dmukhin@ford.com >>> --- >>> Link to the original patch: >>> https://lore.kernel.org/xen-devel/20250103-vuart-ns8250-v3-v1-4-c5d36b31d66c@ford.com/ >>> --- >>> 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(-) >> >> >> This patch doesn't apply to staging. Looks like it depends on "arm/vuart: >> move vpl011-related code to vpl011 emulator" without this being said anywhere. > > Correct, this patch depends on > https://lore.kernel.org/xen-devel/20250212211802.1669675-1-dmukhin@ford.com/ > and I have R-b: > https://lore.kernel.org/xen-devel/alpine.DEB.2.22.394.2502121412500.619090@ubuntu-linux-20-04-desktop/ I'm aware of that. Yet that other patch is too intrusive for my taste to put in an only half-open tree. Plus you should never make assumptions about the order in which patches may make it in; if there are dependencies, they need to be stated (prominently). Jan
On 18.02.2025 09:31, dmkhn@proton.me 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 <jbeulich@suse.com>
Given they're reasonably simple to make, I could once again offer to
adjust while committing (provided an Arm ack also arrives).
Jan
On Wednesday, February 19th, 2025 at 5:52 AM, Jan Beulich <jbeulich@suse.com> wrote:
>
>
> On 18.02.2025 09:31, dmkhn@proton.me 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)
Thanks a lot for help and review!
> Reviewed-by: Jan Beulich jbeulich@suse.com
>
> Given they're reasonably simple to make, I could once again offer to
> adjust while committing (provided an Arm ack also arrives).
>
> Jan
On Wed, 19 Feb 2025, Jan Beulich wrote:
> On 18.02.2025 09:31, dmkhn@proton.me 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 <jbeulich@suse.com>
> Given they're reasonably simple to make, I could once again offer to
> adjust while committing (provided an Arm ack also arrives).
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
On Friday, February 21st, 2025 at 4:04 PM, Stefano Stabellini <sstabellini@kernel.org> wrote:
>
>
> On Wed, 19 Feb 2025, Jan Beulich wrote:
>
> > On 18.02.2025 09:31, dmkhn@proton.me 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 jbeulich@suse.com
> > Given they're reasonably simple to make, I could once again offer to
> > adjust while committing (provided an Arm ack also arrives).
>
>
> Acked-by: Stefano Stabellini sstabellini@kernel.org
Thank you!
© 2016 - 2025 Red Hat, Inc.