[v2,16/35] xen/console: introduce printk_common()

Commit Message

Denis Mukhin via B4 Relay Dec. 6, 2024, 4:41 a.m. UTC

From: Denis Mukhin <[e-mail]>

Introduce new printk() variant for convenient printouts which skip '(XEN)'
prefix on xen console. This is needed for the case when physical console is
owned by a domain w/ in-hypervisor UART emulation enabled.

Signed-off-by: Denis Mukhin <[e-mail]>
---
 automation/eclair_analysis/ECLAIR/deviations.ecl | 2 +-
 xen/drivers/char/console.c                       | 8 ++++++++
 xen/include/xen/lib.h                            | 3 +++
 3 files changed, 12 insertions(+), 1 deletion(-)

Comments

Jan Beulich Dec. 10, 2024, 2:27 p.m. UTC | #1

On 06.12.2024 05:41, Denis Mukhin via B4 Relay wrote:
> Introduce new printk() variant for convenient printouts which skip '(XEN)'
> prefix on xen console. This is needed for the case when physical console is
> owned by a domain w/ in-hypervisor UART emulation enabled.

Imo it should still be guest_printk() which is then used from there.

> --- a/xen/include/xen/lib.h
> +++ b/xen/include/xen/lib.h
> @@ -61,6 +61,9 @@ debugtrace_printk(const char *fmt, ...) {}
>  extern void printk(const char *fmt, ...)
>      __attribute__ ((format (printf, 1, 2), cold));
>  
> +extern void printk_common(const char *fmt, ...)
> +    __attribute__ ((format (printf, 1, 2)));

No "cold" attribute, compared to printk()?

Jan

Roger Pau Monné Dec. 12, 2024, 9:14 a.m. UTC | #2

On Thu, Dec 05, 2024 at 08:41:46PM -0800, Denis Mukhin via B4 Relay wrote:
> From: Denis Mukhin <[e-mail]>
> 
> Introduce new printk() variant for convenient printouts which skip '(XEN)'
> prefix on xen console. This is needed for the case when physical console is
> owned by a domain w/ in-hypervisor UART emulation enabled.

IIRC the ns8250 can only send or receive one byte (character) at a
time, so you should likely put that on the console as soon as it's
received?

For the hardware domain we explicitly don't buffer writes to the
console (see guest_console_write() hardware domain special handling).

I wonder however how you deal with domains that don't have the console
focus (ie: != console_rx), as for those I think you still want to use
the (d<domid>) prefix?

Thanks, Roger.

Jan Beulich Dec. 12, 2024, 11:57 a.m. UTC | #3

On 12.12.2024 10:14, Roger Pau Monné wrote:
> On Thu, Dec 05, 2024 at 08:41:46PM -0800, Denis Mukhin via B4 Relay wrote:
>> From: Denis Mukhin <[e-mail]>
>>
>> Introduce new printk() variant for convenient printouts which skip '(XEN)'
>> prefix on xen console. This is needed for the case when physical console is
>> owned by a domain w/ in-hypervisor UART emulation enabled.
> 
> IIRC the ns8250 can only send or receive one byte (character) at a
> time, so you should likely put that on the console as soon as it's
> received?
> 
> For the hardware domain we explicitly don't buffer writes to the
> console (see guest_console_write() hardware domain special handling).
> 
> I wonder however how you deal with domains that don't have the console
> focus (ie: != console_rx), as for those I think you still want to use
> the (d<domid>) prefix?

Imo no matter what domain has the focus, the (d<domid>) prefix should
always be logged. Just to avoid possible confusion.

Jan

Roger Pau Monné Dec. 12, 2024, 12:15 p.m. UTC | #4

On Thu, Dec 12, 2024 at 12:57:25PM +0100, Jan Beulich wrote:
> On 12.12.2024 10:14, Roger Pau Monné wrote:
> > On Thu, Dec 05, 2024 at 08:41:46PM -0800, Denis Mukhin via B4 Relay wrote:
> >> From: Denis Mukhin <[e-mail]>
> >>
> >> Introduce new printk() variant for convenient printouts which skip '(XEN)'
> >> prefix on xen console. This is needed for the case when physical console is
> >> owned by a domain w/ in-hypervisor UART emulation enabled.
> > 
> > IIRC the ns8250 can only send or receive one byte (character) at a
> > time, so you should likely put that on the console as soon as it's
> > received?
> > 
> > For the hardware domain we explicitly don't buffer writes to the
> > console (see guest_console_write() hardware domain special handling).
> > 
> > I wonder however how you deal with domains that don't have the console
> > focus (ie: != console_rx), as for those I think you still want to use
> > the (d<domid>) prefix?
> 
> Imo no matter what domain has the focus, the (d<domid>) prefix should
> always be logged. Just to avoid possible confusion.

WE don't do that currently for the hardware domain, because we avoid
doing any kind of line processing, as characters from the hardware
domain are send straight to the console without waiting for the
newline terminator (like we do for other domains).

Are you suggesting that in case of the console input being shared
between multiple domains they should all be treated as plain domUs and
thus lines should be buffered?

Thanks, Roger.

Jan Beulich Dec. 12, 2024, 12:52 p.m. UTC | #5

On 12.12.2024 13:15, Roger Pau Monné wrote:
> On Thu, Dec 12, 2024 at 12:57:25PM +0100, Jan Beulich wrote:
>> On 12.12.2024 10:14, Roger Pau Monné wrote:
>>> On Thu, Dec 05, 2024 at 08:41:46PM -0800, Denis Mukhin via B4 Relay wrote:
>>>> From: Denis Mukhin <[e-mail]>
>>>>
>>>> Introduce new printk() variant for convenient printouts which skip '(XEN)'
>>>> prefix on xen console. This is needed for the case when physical console is
>>>> owned by a domain w/ in-hypervisor UART emulation enabled.
>>>
>>> IIRC the ns8250 can only send or receive one byte (character) at a
>>> time, so you should likely put that on the console as soon as it's
>>> received?
>>>
>>> For the hardware domain we explicitly don't buffer writes to the
>>> console (see guest_console_write() hardware domain special handling).
>>>
>>> I wonder however how you deal with domains that don't have the console
>>> focus (ie: != console_rx), as for those I think you still want to use
>>> the (d<domid>) prefix?
>>
>> Imo no matter what domain has the focus, the (d<domid>) prefix should
>> always be logged. Just to avoid possible confusion.
> 
> WE don't do that currently for the hardware domain, because we avoid
> doing any kind of line processing, as characters from the hardware
> domain are send straight to the console without waiting for the
> newline terminator (like we do for other domains).

Right, and that's kind of special, and aiui intentionally so. These are
the only un-prefixed lines logged.

> Are you suggesting that in case of the console input being shared
> between multiple domains they should all be treated as plain domUs and
> thus lines should be buffered?

No, I'm actually not suggesting anything here beyond perhaps reducing
the scope of this series to just what the equivalent of vpl011 would be
for the 8250 / 16550 case.

Jan

Roger Pau Monné Dec. 12, 2024, 3:25 p.m. UTC | #6

On Thu, Dec 12, 2024 at 01:52:49PM +0100, Jan Beulich wrote:
> On 12.12.2024 13:15, Roger Pau Monné wrote:
> > On Thu, Dec 12, 2024 at 12:57:25PM +0100, Jan Beulich wrote:
> >> On 12.12.2024 10:14, Roger Pau Monné wrote:
> >>> On Thu, Dec 05, 2024 at 08:41:46PM -0800, Denis Mukhin via B4 Relay wrote:
> >>>> From: Denis Mukhin <[e-mail]>
> >>>>
> >>>> Introduce new printk() variant for convenient printouts which skip '(XEN)'
> >>>> prefix on xen console. This is needed for the case when physical console is
> >>>> owned by a domain w/ in-hypervisor UART emulation enabled.
> >>>
> >>> IIRC the ns8250 can only send or receive one byte (character) at a
> >>> time, so you should likely put that on the console as soon as it's
> >>> received?
> >>>
> >>> For the hardware domain we explicitly don't buffer writes to the
> >>> console (see guest_console_write() hardware domain special handling).
> >>>
> >>> I wonder however how you deal with domains that don't have the console
> >>> focus (ie: != console_rx), as for those I think you still want to use
> >>> the (d<domid>) prefix?
> >>
> >> Imo no matter what domain has the focus, the (d<domid>) prefix should
> >> always be logged. Just to avoid possible confusion.
> > 
> > WE don't do that currently for the hardware domain, because we avoid
> > doing any kind of line processing, as characters from the hardware
> > domain are send straight to the console without waiting for the
> > newline terminator (like we do for other domains).
> 
> Right, and that's kind of special, and aiui intentionally so. These are
> the only un-prefixed lines logged.
> 
> > Are you suggesting that in case of the console input being shared
> > between multiple domains they should all be treated as plain domUs and
> > thus lines should be buffered?
> 
> No, I'm actually not suggesting anything here beyond perhaps reducing
> the scope of this series to just what the equivalent of vpl011 would be
> for the 8250 / 16550 case.

Indeed, reducing the scope would make it easier to get the actual
feature reviewed.  There's a huge amount of pre-patching that will
possibly take some time to get agreement on (if suitable).

Thanks, Roger.

Stefano Stabellini Dec. 13, 2024, 1:03 a.m. UTC | #7

On Thu, 12 Dec 2024, Jan Beulich wrote:
> On 12.12.2024 13:15, Roger Pau Monné wrote:
> > On Thu, Dec 12, 2024 at 12:57:25PM +0100, Jan Beulich wrote:
> >> On 12.12.2024 10:14, Roger Pau Monné wrote:
> >>> On Thu, Dec 05, 2024 at 08:41:46PM -0800, Denis Mukhin via B4 Relay wrote:
> >>>> From: Denis Mukhin <[e-mail]>
> >>>>
> >>>> Introduce new printk() variant for convenient printouts which skip '(XEN)'
> >>>> prefix on xen console. This is needed for the case when physical console is
> >>>> owned by a domain w/ in-hypervisor UART emulation enabled.
> >>>
> >>> IIRC the ns8250 can only send or receive one byte (character) at a
> >>> time, so you should likely put that on the console as soon as it's
> >>> received?
> >>>
> >>> For the hardware domain we explicitly don't buffer writes to the
> >>> console (see guest_console_write() hardware domain special handling).
> >>>
> >>> I wonder however how you deal with domains that don't have the console
> >>> focus (ie: != console_rx), as for those I think you still want to use
> >>> the (d<domid>) prefix?
> >>
> >> Imo no matter what domain has the focus, the (d<domid>) prefix should
> >> always be logged. Just to avoid possible confusion.
> > 
> > WE don't do that currently for the hardware domain, because we avoid
> > doing any kind of line processing, as characters from the hardware
> > domain are send straight to the console without waiting for the
> > newline terminator (like we do for other domains).
> 
> Right, and that's kind of special, and aiui intentionally so. These are
> the only un-prefixed lines logged.

I think we should provide a consistent behavior across architectures.
The current behavior with vpl011 and dom0less on ARM is the following:

- no prefix for Dom0 output
- DOM$NUM for DomUs when not in focus, otherwise no prefix

It is OK to change this behavior, but in that case I would ask that we
change it consistently also for ARM.


> > Are you suggesting that in case of the console input being shared
> > between multiple domains they should all be treated as plain domUs and
> > thus lines should be buffered?
> 
> No, I'm actually not suggesting anything here beyond perhaps reducing
> the scope of this series to just what the equivalent of vpl011 would be
> for the 8250 / 16550 case.

I agree with this

Patch

diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl b/automation/eclair_analysis/ECLAIR/deviations.ecl
index 2f58f292036e3561118ce9664a92756b1b938739..c59d075262e9e6618ea2a2d27611a537ded3a776 100644
--- a/automation/eclair_analysis/ECLAIR/deviations.ecl
+++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
@@ -522,7 +522,7 @@  safe."
 -config=MC3R1.R17.1,reports+={deliberate,"any_area(^.*va_list.*$&&context(ancestor_or_self(^.*printf\\(.*\\)$)))"}
 -config=MC3R1.R17.1,reports+={deliberate,"any_area(^.*va_list.*$&&context(ancestor_or_self(name(panic)&&kind(function))))"}
 -config=MC3R1.R17.1,reports+={deliberate,"any_area(^.*va_list.*$&&context(ancestor_or_self(name(elf_call_log_callback)&&kind(function))))"}
--config=MC3R1.R17.1,reports+={deliberate,"any_area(^.*va_list.*$&&context(ancestor_or_self(name(vprintk_common)&&kind(function))))"}
+-config=MC3R1.R17.1,reports+={deliberate,"any_area(^.*va_list.*$&&context(ancestor_or_self(name(^v?printk_common)&&kind(function))))"}
 -config=MC3R1.R17.1,macros+={hide , "^va_(arg|start|copy|end)$"}
 -doc_end
 
diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index 86bf899ada8f8221ffc77bcffb1f58777a22198e..f034ce5aab3f3bf59b0df9fa583ee9ce32dbf665 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -968,6 +968,14 @@  void printk(const char *fmt, ...)
     va_end(args);
 }
 
+void printk_common(const char *fmt, ...)
+{
+    va_list args;
+    va_start(args, fmt);
+    vprintk_common("", fmt, args);
+    va_end(args);
+}
+
 void guest_printk(const struct domain *d, const char *fmt, ...)
 {
     va_list args;
diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
index 81b722ea3e801e9089aaf8758249feb3a758c4f7..8a7ff2e8af9089796ff28ef8d01c00e9845782ca 100644
--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -61,6 +61,9 @@  debugtrace_printk(const char *fmt, ...) {}
 extern void printk(const char *fmt, ...)
     __attribute__ ((format (printf, 1, 2), cold));
 
+extern void printk_common(const char *fmt, ...)
+    __attribute__ ((format (printf, 1, 2)));
+
 #define printk_once(fmt, args...)               \
 ({                                              \
     static bool __read_mostly once_;            \