Re: [PATCH v3 2/2] xen/console: unify printout behavior for UART emulators

[dmkhn@xxxxxxxxx]

On Wed, Jun 25, 2025 at 12:50:57PM +0200, Roger Pau Monné wrote:
> On Fri, Jun 06, 2025 at 08:11:26PM +0000, dmkhn@xxxxxxxxx wrote:
> > From: Denis Mukhin <dmukhin@xxxxxxxx>
> >
> > If virtual UART from domain X prints on the physical console, the behavior 
> > is
> > updated to (see [1]):
> > - console focus in domain X: do not prefix messages;
> > - no console focus in domain X: prefix all messages with "(dX)".
> >
> > Use guest_printk() in all current in-hypervisor UART emulators. That aligns 
> > the
> > behavior with debug I/O port 0xe9 handler on x86 and slightly improves the
> > logging since guest_printk() already prints the domain ID. guest_printk() 
> > was
> > modified to account for console focus ownership.
> >
> > Modify guest_console_write() for hardware domain case by adding domain ID to
> > the message when hwdom does not have console focus.
> >
> > [1] 
> > https://lore.kernel.org/xen-devel/alpine.DEB.2.22.394.2412121655360.463523@ubuntu-linux-20-04-desktop/
> > Signed-off-by: Denis Mukhin <dmukhin@xxxxxxxx>
> > ---
> > Changes since v2:
> > - dropped rate-limiting change for vuart
> > ---
> >  xen/arch/arm/vpl011.c      |  6 +++---
> >  xen/arch/arm/vuart.c       |  2 +-
> >  xen/drivers/char/console.c | 23 +++++++++++++++++++++--
> >  3 files changed, 25 insertions(+), 6 deletions(-)
> >
> > diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
> > index 480fc664fc..2b6f2a09bc 100644
> > --- a/xen/arch/arm/vpl011.c
> > +++ b/xen/arch/arm/vpl011.c
> > @@ -87,7 +87,7 @@ static void vpl011_write_data_xen(struct domain *d, 
> > uint8_t data)
> >      {
> >          if ( intf->out_prod == 1 )
> >          {
> > -            printk("%c", data);
> > +            guest_printk(d, "%c", data);
> >              intf->out_prod = 0;
> >          }
> >          else
> > @@ -95,7 +95,7 @@ static void vpl011_write_data_xen(struct domain *d, 
> > uint8_t data)
> >              if ( data != '\n' )
> >                  intf->out[intf->out_prod++] = '\n';
> >              intf->out[intf->out_prod++] = '\0';
> > -            printk("%s", intf->out);
> > +            guest_printk(d, "%s", intf->out);
> >              intf->out_prod = 0;
> >          }
> >      }
> > @@ -107,7 +107,7 @@ static void vpl011_write_data_xen(struct domain *d, 
> > uint8_t data)
> >              if ( data != '\n' )
> >                  intf->out[intf->out_prod++] = '\n';
> >              intf->out[intf->out_prod++] = '\0';
> > -            printk("DOM%u: %s", d->domain_id, intf->out);
> > +            guest_printk(d, "%s", intf->out);
> >              intf->out_prod = 0;
> >          }
> >      }
> > diff --git a/xen/arch/arm/vuart.c b/xen/arch/arm/vuart.c
> > index bd2f425214..6641f9d775 100644
> > --- a/xen/arch/arm/vuart.c
> > +++ b/xen/arch/arm/vuart.c
> > @@ -89,7 +89,7 @@ static void vuart_print_char(struct vcpu *v, char c)
> >          if ( c != '\n' )
> >              uart->buf[uart->idx++] = '\n';
> >          uart->buf[uart->idx] = '\0';
> > -        printk(XENLOG_G_DEBUG "DOM%u: %s", d->domain_id, uart->buf);
> > +        guest_printk(d, XENLOG_G_DEBUG "%s", uart->buf);
> >          uart->idx = 0;
> >      }
> >      spin_unlock(&uart->lock);
> > diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> > index 6e77b4af82..3855962af7 100644
> > --- a/xen/drivers/char/console.c
> > +++ b/xen/drivers/char/console.c
> > @@ -740,7 +740,17 @@ static long 
> > guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer,
> >          if ( is_hardware_domain(cd) )
> >          {
> >              /* Use direct console output as it could be interactive */
> > +            char prefix[16] = "";
> > +            struct domain *consd;
> > +
> > +            consd = console_get_domain();
> > +            if ( consd != cd )
> > +                snprintf(prefix, sizeof(prefix), "(d%d) ", cd->domain_id);
> > +            console_put_domain(consd);
> > +
> >              nrspin_lock_irq(&console_lock);
> > +            if ( prefix[0] != '\0' )
> > +                console_send(prefix, strlen(prefix), flags);
> >              console_send(kbuf, kcount, flags);
> >              nrspin_unlock_irq(&console_lock);
> >          }
> > @@ -1032,12 +1042,21 @@ void printk(const char *fmt, ...)
> >      va_end(args);
> >  }
> >
> > +/*
> > + * Print message from the guest on the diagnostic console.
> > + * Prefixes all messages w/ "(dX)" if domain X does not own physical 
> > console
> > + * focus.
> > + */
> >  void guest_printk(const struct domain *d, const char *fmt, ...)
> >  {
> >      va_list args;
> > -    char prefix[16];
> > +    char prefix[16] = "";
> > +    struct domain *consd;
> >
> > -    snprintf(prefix, sizeof(prefix), "(d%d) ", d->domain_id);
> > +    consd = console_get_domain();
> > +    if ( consd != d )
> > +        snprintf(prefix, sizeof(prefix), "(d%d) ", d->domain_id);
> > +    console_put_domain(consd);
> 
> It might be helpful to abstract this into a separate helper, as it's
> used by both functions:
> 
> static void fill_console_prefix(char *prefix, unsigned int len,
>                                 const struct domain *d)
> {
>     struct domain *consd = console_get_domain();
> 
>     if ( consd ? consd != d : !is_hardware_domain(d)) )
>        snprintf(prefix, len, "(d%d) ", d->domain_id);
>     console_put_domain(consd);
> }
> 
> Note the above code should also handle the current discussion of not
> printing the (d0) prefix for the hardware domain when the console
> target is Xen.  I think this keeps the previous behavior when console
> input is switched to Xen, while still providing unified (dX) prefixes
> otherwise.

Thanks, will do.

> 
> Thanks, Roger.
>