[PATCH v1 0/2] xen/console: updates to diagnostic messages prefixes

[PATCH v1 0/2] xen/console: updates to diagnostic messages prefixes

[dmkhn]

Patch 1 is purely cosmetic change, adds a symbol for hypervisor's messages.

Patch 2 updates the logic how the domain prefix is formed.

[1] Link to CI: https://gitlab.com/xen-project/people/dmukhin/xen/-/pipelines/1845446760

Denis Mukhin (2):
  xen/console: introduce CONSOLE_PREFIX
  xen/console: unify printout behavior for UART emulators

 xen/arch/arm/vpl011.c      |  6 +++---
 xen/arch/arm/vuart.c       |  2 +-
 xen/arch/x86/hvm/hvm.c     |  2 +-
 xen/drivers/char/console.c | 32 +++++++++++++++++++++++++++-----
 4 files changed, 32 insertions(+), 10 deletions(-)

-- 
2.34.1

[PATCH v1 1/2] xen/console: introduce CONSOLE_PREFIX

[dmkhn]

From: Denis Mukhin <dmukhin@ford.com>

Add CONSOLE_PREFIX symbol to keep the prefix of the hypervisor's diagnostic
messages.

No functional change.

Signed-off-by: Denis Mukhin <dmukhin@ford.com>
---
 xen/drivers/char/console.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index c15987f5bb..27e3f8d8c6 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -61,6 +61,9 @@ enum {
     CONSOLE_ALL             = CONSOLE_DEFAULT | CONSOLE_RING,
 };
 
+/* Prefix for hypervisor's diagnostic console messages. */
+#define CONSOLE_PREFIX      "(XEN) "
+
 static void console_send(const char *str, size_t len, unsigned int flags);
 
 /* console: comma-separated list of console outputs. */
@@ -998,7 +1001,7 @@ static void vprintk_common(const char *fmt, va_list args, const char *prefix)
 
 void vprintk(const char *fmt, va_list args)
 {
-    vprintk_common(fmt, args, "(XEN) ");
+    vprintk_common(fmt, args, CONSOLE_PREFIX);
 }
 
 void printk(const char *fmt, ...)
@@ -1269,7 +1272,7 @@ int __printk_ratelimit(int ratelimit_ms, int ratelimit_burst)
             snprintf(lost_str, sizeof(lost_str), "%d", lost);
             /* console_lock may already be acquired by printk(). */
             rspin_lock(&console_lock);
-            printk_start_of_line("(XEN) ");
+            printk_start_of_line(CONSOLE_PREFIX);
             __putstr("printk: ");
             __putstr(lost_str);
             __putstr(" messages suppressed.\n");
-- 
2.34.1

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

[dmkhn]

From: Denis Mukhin <dmukhin@ford.com>

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
behavior with debug I/O port 0xe9 handler in x86 port 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@ford.com>
---
 xen/arch/arm/vpl011.c      |  6 +++---
 xen/arch/arm/vuart.c       |  2 +-
 xen/arch/x86/hvm/hvm.c     |  2 +-
 xen/drivers/char/console.c | 25 ++++++++++++++++++++++---
 4 files changed, 27 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
index 66047bf33c..6298e377b3 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..8c9f9e2182 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, "%s", uart->buf);
         uart->idx = 0;
     }
     spin_unlock(&uart->lock);
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 4cb2e13046..8cc94bee81 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -577,7 +577,7 @@ static int cf_check hvm_print_line(
     if ( (cd->pbuf_idx == (DOMAIN_PBUF_SIZE - 1)) || (c == '\n') )
     {
         cd->pbuf[cd->pbuf_idx] = '\0';
-        guest_printk(cd, XENLOG_G_DEBUG "%s\n", cd->pbuf);
+        guest_printk(cd, "%s\n", cd->pbuf);
         cd->pbuf_idx = 0;
     }
     spin_unlock(&cd->pbuf_lock);
diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index 27e3f8d8c6..16d6003a0f 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -724,7 +724,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);
         }
@@ -755,7 +765,7 @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer,
             else
             {
                 cd->pbuf[cd->pbuf_idx] = '\0';
-                guest_printk(cd, XENLOG_G_DEBUG "%s%s\n", cd->pbuf, kbuf);
+                guest_printk(cd, "%s%s\n", cd->pbuf, kbuf);
                 cd->pbuf_idx = 0;
             }
             spin_unlock(&cd->pbuf_lock);
@@ -1013,12 +1023,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);
 
     va_start(args, fmt);
     vprintk_common(fmt, args, prefix);
-- 
2.34.1

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

[Jan Beulich]

On 31.05.2025 02:04, dmkhn@proton.me wrote:
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -577,7 +577,7 @@ static int cf_check hvm_print_line(
>      if ( (cd->pbuf_idx == (DOMAIN_PBUF_SIZE - 1)) || (c == '\n') )
>      {
>          cd->pbuf[cd->pbuf_idx] = '\0';
> -        guest_printk(cd, XENLOG_G_DEBUG "%s\n", cd->pbuf);
> +        guest_printk(cd, "%s\n", cd->pbuf);
>          cd->pbuf_idx = 0;
>      }

Why this and ...

> @@ -755,7 +765,7 @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer,
>              else
>              {
>                  cd->pbuf[cd->pbuf_idx] = '\0';
> -                guest_printk(cd, XENLOG_G_DEBUG "%s%s\n", cd->pbuf, kbuf);
> +                guest_printk(cd, "%s%s\n", cd->pbuf, kbuf);
>                  cd->pbuf_idx = 0;
>              }

... this change? There's no compensation for it ...

> @@ -1013,12 +1023,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);
>  
>      va_start(args, fmt);
>      vprintk_common(fmt, args, prefix);

... here afaics, so it looks like you're undermining rate-limiting of
those messages.

Jan

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

[dmkhn]

On Wed, Jun 04, 2025 at 12:48:05PM +0200, Jan Beulich wrote:
> On 31.05.2025 02:04, dmkhn@proton.me wrote:
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -577,7 +577,7 @@ static int cf_check hvm_print_line(
> >      if ( (cd->pbuf_idx == (DOMAIN_PBUF_SIZE - 1)) || (c == '\n') )
> >      {
> >          cd->pbuf[cd->pbuf_idx] = '\0';
> > -        guest_printk(cd, XENLOG_G_DEBUG "%s\n", cd->pbuf);
> > +        guest_printk(cd, "%s\n", cd->pbuf);
> >          cd->pbuf_idx = 0;
> >      }
> 
> Why this and ...
> 
> > @@ -755,7 +765,7 @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer,
> >              else
> >              {
> >                  cd->pbuf[cd->pbuf_idx] = '\0';
> > -                guest_printk(cd, XENLOG_G_DEBUG "%s%s\n", cd->pbuf, kbuf);
> > +                guest_printk(cd, "%s%s\n", cd->pbuf, kbuf);
> >                  cd->pbuf_idx = 0;
> >              }
> 
> ... this change? There's no compensation for it ...
> 
> > @@ -1013,12 +1023,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);
> >
> >      va_start(args, fmt);
> >      vprintk_common(fmt, args, prefix);
> 
> ... here afaics, so it looks like you're undermining rate-limiting of
> those messages.

I droppped behavior change for I/O debug port on x86 and HYPERVISOR_console_io
hypercall.

But my understanding is that all guest debugging facilities, if enabled, should
not be rate-limited.

> 
> Jan

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

[Jan Beulich]

On 05.06.2025 02:57, dmkhn@proton.me wrote:
> On Wed, Jun 04, 2025 at 12:48:05PM +0200, Jan Beulich wrote:
>> On 31.05.2025 02:04, dmkhn@proton.me wrote:
>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -577,7 +577,7 @@ static int cf_check hvm_print_line(
>>>      if ( (cd->pbuf_idx == (DOMAIN_PBUF_SIZE - 1)) || (c == '\n') )
>>>      {
>>>          cd->pbuf[cd->pbuf_idx] = '\0';
>>> -        guest_printk(cd, XENLOG_G_DEBUG "%s\n", cd->pbuf);
>>> +        guest_printk(cd, "%s\n", cd->pbuf);
>>>          cd->pbuf_idx = 0;
>>>      }
>>
>> Why this and ...
>>
>>> @@ -755,7 +765,7 @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer,
>>>              else
>>>              {
>>>                  cd->pbuf[cd->pbuf_idx] = '\0';
>>> -                guest_printk(cd, XENLOG_G_DEBUG "%s%s\n", cd->pbuf, kbuf);
>>> +                guest_printk(cd, "%s%s\n", cd->pbuf, kbuf);
>>>                  cd->pbuf_idx = 0;
>>>              }
>>
>> ... this change? There's no compensation for it ...
>>
>>> @@ -1013,12 +1023,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);
>>>
>>>      va_start(args, fmt);
>>>      vprintk_common(fmt, args, prefix);
>>
>> ... here afaics, so it looks like you're undermining rate-limiting of
>> those messages.
> 
> I droppped behavior change for I/O debug port on x86 and HYPERVISOR_console_io
> hypercall.
> 
> But my understanding is that all guest debugging facilities, if enabled, should
> not be rate-limited.

I certainly disagree there. How much rate limiting to apply to guest output is a
matter of the guest_loglvl= command line option. Its default settings are the way
they are for a reason.

Jan

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

[dmkhn]

On Thu, Jun 05, 2025 at 08:15:15AM +0200, Jan Beulich wrote:
> On 05.06.2025 02:57, dmkhn@proton.me wrote:
> > On Wed, Jun 04, 2025 at 12:48:05PM +0200, Jan Beulich wrote:
> >> On 31.05.2025 02:04, dmkhn@proton.me wrote:
> >>> --- a/xen/arch/x86/hvm/hvm.c
> >>> +++ b/xen/arch/x86/hvm/hvm.c
> >>> @@ -577,7 +577,7 @@ static int cf_check hvm_print_line(
> >>>      if ( (cd->pbuf_idx == (DOMAIN_PBUF_SIZE - 1)) || (c == '\n') )
> >>>      {
> >>>          cd->pbuf[cd->pbuf_idx] = '\0';
> >>> -        guest_printk(cd, XENLOG_G_DEBUG "%s\n", cd->pbuf);
> >>> +        guest_printk(cd, "%s\n", cd->pbuf);
> >>>          cd->pbuf_idx = 0;
> >>>      }
> >>
> >> Why this and ...
> >>
> >>> @@ -755,7 +765,7 @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer,
> >>>              else
> >>>              {
> >>>                  cd->pbuf[cd->pbuf_idx] = '\0';
> >>> -                guest_printk(cd, XENLOG_G_DEBUG "%s%s\n", cd->pbuf, kbuf);
> >>> +                guest_printk(cd, "%s%s\n", cd->pbuf, kbuf);
> >>>                  cd->pbuf_idx = 0;
> >>>              }
> >>
> >> ... this change? There's no compensation for it ...
> >>
> >>> @@ -1013,12 +1023,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);
> >>>
> >>>      va_start(args, fmt);
> >>>      vprintk_common(fmt, args, prefix);
> >>
> >> ... here afaics, so it looks like you're undermining rate-limiting of
> >> those messages.
> >
> > I droppped behavior change for I/O debug port on x86 and HYPERVISOR_console_io
> > hypercall.
> >
> > But my understanding is that all guest debugging facilities, if enabled, should
> > not be rate-limited.
> 
> I certainly disagree there. How much rate limiting to apply to guest output is a
> matter of the guest_loglvl= command line option. Its default settings are the way
> they are for a reason.

Oh, I see!
Thanks for clarification!

> 
> Jan
>