Re: [PATCH] arm/vuart: move vpl011-related code to vpl011 emulator

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 11 Feb 2025, dmkhn@xxxxxxxxx wrote:
> From: Denis Mukhin <dmukhin@xxxxxxxx>
> 
> 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@xxxxxxxx>
> ---
> Link to the original patch:
>   
> https://lore.kernel.org/xen-devel/20250103-vuart-ns8250-v3-v1-2-c5d36b31d66c@xxxxxxxx/
> ---
>  xen/arch/arm/include/asm/vpl011.h |  2 +-
>  xen/arch/arm/vpl011.c             | 15 +++++++++++----
>  xen/drivers/char/console.c        | 21 +++++++--------------
>  3 files changed, 19 insertions(+), 19 deletions(-)
> 
> diff --git a/xen/arch/arm/include/asm/vpl011.h 
> b/xen/arch/arm/include/asm/vpl011.h
> index c09abcd7a9..cc83868281 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 1fc3114cce..c72f3778bf 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;

Everything else looks fine. I am a bit unsure about this -ENOSPC return
because...


>      }
>  
>      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 b4cec77247..5e6f0fb062 100644
> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -553,21 +553,14 @@ 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 )
> +        {
> +            int rc = vpl011_rx_char_xen(d, c);
> +            if ( rc )
> +                printk(KERN_WARNING "%pd: failed to process console input: 
> %d\n",
> +                       d, rc);

... it could trigger a warning here. And any prink triggerable by the
guest should be rate limited. We already have a function for that which
is guest_printk. So I think we should probably change this warning to be
guest_printk. Or change return -ENODEV into return 0.


>              rcu_unlock_domain(d);
> +        }
>  
>          break;
>      }
> -- 
> 2.34.1
> 
>