| Message ID | 20250103-vuart-ns8250-v3-v1-2-c5d36b31d66c@ford.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86: introduce NS16550-compatible UART emulator | expand |
On 2025-01-03 20:58, Denis Mukhin via B4 Relay wrote: > From: Denis Mukhin <dmukhin@ford.com> > > Xen console driver has vpl011-related logic which shall belong vpl011 emulator > code (Arm port). Move vpl011-related code from arch-independent console driver > to Arm's vpl011.c. > > Signed-off-by: Denis Mukhin <dmukhin@ford.com> > diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c > index 4cb397116b44935214801c496b30e44c9399c59a..1411c991977b5fb26ee5709e523b7bc32b612808 100644 > --- a/xen/drivers/char/console.c > +++ b/xen/drivers/char/console.c > @@ -579,6 +571,9 @@ static void __serial_rx(char c) > if ( pv_shim && pv_console ) > consoled_guest_tx(c); > #endif > + > + if ( rc ) > + printk(KERN_ERR "d%pd: failed to process console input: %d\n", d, rc); > } > > static void cf_check serial_rx(char c) > This will print the ENOSPC that was formerly silent. Since this is input from the console, that seems more informative to the user and okay to me. Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
On 21.01.2025 23:56, Jason Andryuk wrote: > On 2025-01-03 20:58, Denis Mukhin via B4 Relay wrote: >> --- a/xen/drivers/char/console.c >> +++ b/xen/drivers/char/console.c > >> @@ -579,6 +571,9 @@ static void __serial_rx(char c) >> if ( pv_shim && pv_console ) >> consoled_guest_tx(c); >> #endif >> + >> + if ( rc ) >> + printk(KERN_ERR "d%pd: failed to process console input: %d\n", d, rc); >> } >> >> static void cf_check serial_rx(char c) >> > > This will print the ENOSPC that was formerly silent. Since this is > input from the console, that seems more informative to the user and okay > to me. I don't view this as okay. For one it needs to be a guest log level, such that rate limiting can suitably suppress too many of these messages in a short time (which in particular might happen if the ENOSPC reason isn't dealt with by the receiving domain). And then I wonder whether this wouldn't better be even more strongly limited, perhaps to just once per domain. I'm also unconvinced of KERN_ERR - from Xen's perspective nothing error- like has happened, once again most notably for the ENOSPC case. I'd view this as a warning at best. Finally: Why d%pd? It ought to be just %pd. Jan
On 2025-01-22 02:26, Jan Beulich wrote: > On 21.01.2025 23:56, Jason Andryuk wrote: >> On 2025-01-03 20:58, Denis Mukhin via B4 Relay wrote: >>> --- a/xen/drivers/char/console.c >>> +++ b/xen/drivers/char/console.c >> >>> @@ -579,6 +571,9 @@ static void __serial_rx(char c) >>> if ( pv_shim && pv_console ) >>> consoled_guest_tx(c); >>> #endif >>> + >>> + if ( rc ) >>> + printk(KERN_ERR "d%pd: failed to process console input: %d\n", d, rc); >>> } >>> >>> static void cf_check serial_rx(char c) >>> >> >> This will print the ENOSPC that was formerly silent. Since this is >> input from the console, that seems more informative to the user and okay >> to me. > > I don't view this as okay. For one it needs to be a guest log level, such > that rate limiting can suitably suppress too many of these messages in a > short time (which in particular might happen if the ENOSPC reason isn't > dealt with by the receiving domain). And then I wonder whether this > wouldn't better be even more strongly limited, perhaps to just once per > domain. I was thinking of a user typing into the console. Silently dropping characters is frustrating when you don't know it is happening. On the other hand, if the domU is echoing characters, then the user receives feedback on their typing. So maybe silently ignoring ENOSPC is okay? ENODEV could be okay just once. It could also be helpful to get the message if you come back a week later and try to type into the same console. But these the user should rate limit themselves when they just keep getting errors :) > I'm also unconvinced of KERN_ERR - from Xen's perspective nothing error- > like has happened, once again most notably for the ENOSPC case. I'd view > this as a warning at best. > > Finally: Why d%pd? It ought to be just %pd. Yes, thanks. Regards, Jason
diff --git a/xen/arch/arm/include/asm/vpl011.h b/xen/arch/arm/include/asm/vpl011.h index c09abcd7a9b3356d0809743517934adae00087f5..cc838682815c0d049ba33d3bf9966a64b2e527dd 100644 --- a/xen/arch/arm/include/asm/vpl011.h +++ b/xen/arch/arm/include/asm/vpl011.h @@ -69,7 +69,7 @@ struct vpl011_init_info { int domain_vpl011_init(struct domain *d, struct vpl011_init_info *info); void domain_vpl011_deinit(struct domain *d); -void vpl011_rx_char_xen(struct domain *d, char c); +int vpl011_rx_char_xen(struct domain *d, char c); #else static inline int domain_vpl011_init(struct domain *d, struct vpl011_init_info *info) diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c index 1fc3114cce9ddb48cf199834c8e9abe8cfba92b5..c72f3778bfedf9434f9d1a0cd7fa33852563112d 100644 --- a/xen/arch/arm/vpl011.c +++ b/xen/arch/arm/vpl011.c @@ -567,16 +567,21 @@ static void vpl011_data_avail(struct domain *d, /* * vpl011_rx_char_xen adds a char to a domain's vpl011 receive buffer. - * It is only used when the vpl011 backend is in Xen. */ -void vpl011_rx_char_xen(struct domain *d, char c) +int vpl011_rx_char_xen(struct domain *d, char c) { unsigned long flags; struct vpl011 *vpl011 = &d->arch.vpl011; struct vpl011_xen_backend *intf = vpl011->backend.xen; XENCONS_RING_IDX in_cons, in_prod, in_fifo_level; - ASSERT(!vpl011->backend_in_domain); + /* Forward input iff the vpl011 backend is in Xen. */ + if ( vpl011->backend_in_domain ) + return -ENODEV; + + if ( intf == NULL ) + return -ENODEV; + VPL011_LOCK(d, flags); in_cons = intf->in_cons; @@ -584,7 +589,7 @@ void vpl011_rx_char_xen(struct domain *d, char c) if ( xencons_queued(in_prod, in_cons, sizeof(intf->in)) == sizeof(intf->in) ) { VPL011_UNLOCK(d, flags); - return; + return -ENOSPC; } intf->in[xencons_mask(in_prod, sizeof(intf->in))] = c; @@ -596,6 +601,8 @@ void vpl011_rx_char_xen(struct domain *d, char c) vpl011_data_avail(d, in_fifo_level, sizeof(intf->in), 0, SBSA_UART_FIFO_SIZE); VPL011_UNLOCK(d, flags); + + return 0; } static void vpl011_notification(struct vcpu *v, unsigned int port) diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c index 4cb397116b44935214801c496b30e44c9399c59a..1411c991977b5fb26ee5709e523b7bc32b612808 100644 --- a/xen/drivers/char/console.c +++ b/xen/drivers/char/console.c @@ -529,6 +529,8 @@ static void switch_serial_input(void) static void __serial_rx(char c) { + int rc = 0; + switch ( console_rx ) { case 0: @@ -554,21 +556,11 @@ static void __serial_rx(char c) { struct domain *d = rcu_lock_domain_by_id(console_rx - 1); - /* - * If we have a properly initialized vpl011 console for the - * domain, without a full PV ring to Dom0 (in that case input - * comes from the PV ring), then send the character to it. - */ - if ( d != NULL && - !d->arch.vpl011.backend_in_domain && - d->arch.vpl011.backend.xen != NULL ) - vpl011_rx_char_xen(d, c); - else - printk("Cannot send chars to Dom%d: no UART available\n", - console_rx - 1); - - if ( d != NULL ) + if ( d ) + { + rc = vpl011_rx_char_xen(d, c); rcu_unlock_domain(d); + } break; } @@ -579,6 +571,9 @@ static void __serial_rx(char c) if ( pv_shim && pv_console ) consoled_guest_tx(c); #endif + + if ( rc ) + printk(KERN_ERR "d%pd: failed to process console input: %d\n", d, rc); } static void cf_check serial_rx(char c)