1. Patchew
  2. Xen
  3. [PATCH v2 0/2] xen/console: updates to diagnostic messages prefixes
  4. View patch

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

dmkhn@proton.me posted 2 patches 4 months, 4 weeks ago
There is a newer version of this series
[PATCH v2 2/2] xen/console: unify printout behavior for UART emulators
Posted by dmkhn@proton.me 4 months, 4 weeks ago 
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() without rate-limiting 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@ford.com>
---
Changes since v1:
- dropped change for debug port and for HYPERVISOR_console_io hypercall
---
 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..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/drivers/char/console.c b/xen/drivers/char/console.c
index a8cb6363ea..616f4968b0 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);
         }
@@ -1029,12 +1039,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 v2 2/2] xen/console: unify printout behavior for UART emulators
Posted by Jan Beulich 4 months, 3 weeks ago 
On 05.06.2025 02:46, dmkhn@proton.me wrote:
> 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() without rate-limiting 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@ford.com>
> ---
> Changes since v1:
> - dropped change for debug port and for HYPERVISOR_console_io hypercall

Yet then what about ...

> --- 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);

... this dropping of XENLOG_G_DEBUG? In fact I'd have expected such to
be _added_ where presently missing.

Jan
Re: [PATCH v2 2/2] xen/console: unify printout behavior for UART emulators
Posted by dmkhn@proton.me 4 months, 3 weeks ago 
On Thu, Jun 05, 2025 at 08:18:34AM +0200, Jan Beulich wrote:
> On 05.06.2025 02:46, dmkhn@proton.me wrote:
> > 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() without rate-limiting 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@ford.com>
> > ---
> > Changes since v1:
> > - dropped change for debug port and for HYPERVISOR_console_io hypercall
> 
> Yet then what about ...
> 
> > --- 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);
> 
> ... this dropping of XENLOG_G_DEBUG? In fact I'd have expected such to
> be _added_ where presently missing.

vUART is a debugging facility. This flavor of UART is specifically for guest OS
early boot debugging.
I think it is not desirable to potentially lose guest messages while doing such
early guest OS boot debugging.

> 
> Jan
Re: [PATCH v2 2/2] xen/console: unify printout behavior for UART emulators
Posted by Jan Beulich 4 months, 3 weeks ago 
On 06.06.2025 09:06, dmkhn@proton.me wrote:
> On Thu, Jun 05, 2025 at 08:18:34AM +0200, Jan Beulich wrote:
>> On 05.06.2025 02:46, dmkhn@proton.me wrote:
>>> 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() without rate-limiting 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@ford.com>
>>> ---
>>> Changes since v1:
>>> - dropped change for debug port and for HYPERVISOR_console_io hypercall
>>
>> Yet then what about ...
>>
>>> --- 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);
>>
>> ... this dropping of XENLOG_G_DEBUG? In fact I'd have expected such to
>> be _added_ where presently missing.
> 
> vUART is a debugging facility. This flavor of UART is specifically for guest OS
> early boot debugging.
> I think it is not desirable to potentially lose guest messages while doing such
> early guest OS boot debugging.

That is the host admin's decision, not a policy we should enforce.

Jan
Re: [PATCH v2 2/2] xen/console: unify printout behavior for UART emulators
Posted by dmkhn@proton.me 4 months, 3 weeks ago 
On Fri, Jun 06, 2025 at 09:12:06AM +0200, Jan Beulich wrote:
> On 06.06.2025 09:06, dmkhn@proton.me wrote:
> > On Thu, Jun 05, 2025 at 08:18:34AM +0200, Jan Beulich wrote:
> >> On 05.06.2025 02:46, dmkhn@proton.me wrote:
> >>> 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() without rate-limiting 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@ford.com>
> >>> ---
> >>> Changes since v1:
> >>> - dropped change for debug port and for HYPERVISOR_console_io hypercall
> >>
> >> Yet then what about ...
> >>
> >>> --- 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);
> >>
> >> ... this dropping of XENLOG_G_DEBUG? In fact I'd have expected such to
> >> be _added_ where presently missing.
> >
> > vUART is a debugging facility. This flavor of UART is specifically for guest OS
> > early boot debugging.
> > I think it is not desirable to potentially lose guest messages while doing such
> > early guest OS boot debugging.
> 
> That is the host admin's decision, not a policy we should enforce.

re: policy: agreed, I will drop that hunk.

I think for the policy control, there can be a compile time setting (separate
patch) which enables/disables the debug output rate-limiting - and that setting
applies to:
  - vUARTs (currently vpl011 and "vuart", later ns16550 (x86) and upcoming
    emulator for RISC-V)
  - debug port on x86
  - HYPERVISOR_console_io

What do you think?

> 
> Jan
Re: [PATCH v2 2/2] xen/console: unify printout behavior for UART emulators
Posted by Jan Beulich 4 months, 3 weeks ago 
On 06.06.2025 21:47, dmkhn@proton.me wrote:
> On Fri, Jun 06, 2025 at 09:12:06AM +0200, Jan Beulich wrote:
>> On 06.06.2025 09:06, dmkhn@proton.me wrote:
>>> On Thu, Jun 05, 2025 at 08:18:34AM +0200, Jan Beulich wrote:
>>>> On 05.06.2025 02:46, dmkhn@proton.me wrote:
>>>>> 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() without rate-limiting 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@ford.com>
>>>>> ---
>>>>> Changes since v1:
>>>>> - dropped change for debug port and for HYPERVISOR_console_io hypercall
>>>>
>>>> Yet then what about ...
>>>>
>>>>> --- 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);
>>>>
>>>> ... this dropping of XENLOG_G_DEBUG? In fact I'd have expected such to
>>>> be _added_ where presently missing.
>>>
>>> vUART is a debugging facility. This flavor of UART is specifically for guest OS
>>> early boot debugging.
>>> I think it is not desirable to potentially lose guest messages while doing such
>>> early guest OS boot debugging.
>>
>> That is the host admin's decision, not a policy we should enforce.
> 
> re: policy: agreed, I will drop that hunk.
> 
> I think for the policy control, there can be a compile time setting (separate
> patch) which enables/disables the debug output rate-limiting - and that setting
> applies to:
>   - vUARTs (currently vpl011 and "vuart", later ns16550 (x86) and upcoming
>     emulator for RISC-V)
>   - debug port on x86
>   - HYPERVISOR_console_io
> 
> What do you think?

I'm not convinced, but much would depend on the justification for such a change.

Jan

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.