Re: [PATCH v2 20/35] xen/console: introduce console_owner_domid()

[Denis Mukhin <dmkhn@xxxxxxxxx>]

On Thursday, January 9th, 2025 at 3:46 PM, Stefano Stabellini 
<sstabellini@xxxxxxxxxx> wrote:

> 
> 
> On Thu, 9 Jan 2025, Roger Pau Monné wrote:
> 
> > On Wed, Jan 08, 2025 at 04:29:24PM -0800, Stefano Stabellini wrote:
> > 
> > > On Wed, 8 Jan 2025, Denis Mukhin wrote:
> > > 
> > > > On Wednesday, January 8th, 2025 at 12:35 AM, Roger Pau Monné 
> > > > roger.pau@xxxxxxxxxx wrote:
> > > > 
> > > > > On Wed, Jan 08, 2025 at 09:13:02AM +0100, Jan Beulich wrote:
> > > > > 
> > > > > > On 08.01.2025 09:04, Roger Pau Monné wrote:
> > > > > > 
> > > > > > > On Wed, Jan 08, 2025 at 08:28:32AM +0100, Jan Beulich wrote:
> > > > > > > 
> > > > > > > > On 08.01.2025 00:40, Stefano Stabellini wrote:
> > > > > > > > 
> > > > > > > > > On Tue, 7 Jan 2025, Jan Beulich wrote:
> > > > > > > > > 
> > > > > > > > > > On 06.01.2025 19:48, Stefano Stabellini wrote:
> > > > > > > > > > 
> > > > > > > > > > > On Mon, 6 Jan 2025, Jan Beulich wrote:
> > > > > > > > > > > 
> > > > > > > > > > > > On 04.01.2025 05:15, Denis Mukhin wrote:
> > > > > > > > > > > > 
> > > > > > > > > > > > > On Tuesday, December 10th, 2024 at 11:28 PM, Jan 
> > > > > > > > > > > > > Beulich jbeulich@xxxxxxxx wrote:
> > > > > > > > > > > > > 
> > > > > > > > > > > > > > On 06.12.2024 05:41, Denis Mukhin via B4 Relay 
> > > > > > > > > > > > > > wrote:
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > From: Denis Mukhin dmukhin@xxxxxxxx
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > console_owner_domid() is introduced to obtain the 
> > > > > > > > > > > > > > > "console owner" domain ID.
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > The call is used in NS8250 emulator to identify 
> > > > > > > > > > > > > > > the case when physical xen
> > > > > > > > > > > > > > > console focus is owned by the domain w/ NS8250 
> > > > > > > > > > > > > > > emulator, in which case,
> > > > > > > > > > > > > > > messages from guest OS are formatted w/o '(XEN)' 
> > > > > > > > > > > > > > > prefix.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > Such messages ought to be processed through 
> > > > > > > > > > > > > > guest_printk(), which wants a
> > > > > > > > > > > > > > domain pointer, not a domid_t anyway. Plus isn't 
> > > > > > > > > > > > > > that going to be
> > > > > > > > > > > > > > current->domain anyway at the callsite, eliminating 
> > > > > > > > > > > > > > the need for such a
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > helper altogether?
> > > > > > > > > > > > > 
> > > > > > > > > > > > > If the current domain is owning the physical console 
> > > > > > > > > > > > > and printing, say, Linux
> > > > > > > > > > > > > login prompt, there's no need to add "(XEN)" for 
> > > > > > > > > > > > > every printout; adding timestamps
> > > > > > > > > > > > > can be disabled from Xen command line.
> > > > > > > > > > > > 
> > > > > > > > > > > > Surely there shouldn't be (XEN), but without (d<N>) 
> > > > > > > > > > > > it'll be ambiguous in a log
> > > > > > > > > > > > which domain a message came from. As long as only Dom0 
> > > > > > > > > > > > messages are left un-
> > > > > > > > > > > > prefixed, that's likely fine. Yet as soon as multiple 
> > > > > > > > > > > > domains can issue such
> > > > > > > > > > > > messages (and have console "focus") I think the prefix 
> > > > > > > > > > > > needs to be there.
> > > > > > > > > > > 
> > > > > > > > > > > It looks like we are aligned on the desired behavior,
> > > > > > > > > > 
> > > > > > > > > > Hmm, no, I don't think we are. I don't ...
> > > > > > > > > > 
> > > > > > > > > > > but for clarity,
> > > > > > > > > > > see https://marc.info/?l=xen-devel&m=173405161613716, 
> > > > > > > > > > > also copy/pasted
> > > > > > > > > > > here:
> > > > > > > > > > > 
> > > > > > > > > > > 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
> > > > > > > > > > 
> > > > > > > > > > ... view this model as a desirable one. It leaves room for 
> > > > > > > > > > ambiguity.
> > > > > > > > > 
> > > > > > > > > Adding a few more people in CC for feedback.
> > > > > > > > > 
> > > > > > > > > My priority is to keep the architectures aligned. It might be 
> > > > > > > > > OK to
> > > > > > > > > change output format, but then let's do it uniformly on ARM 
> > > > > > > > > as well.
> > > > > > > > > 
> > > > > > > > > Jan, please clarify what you think would be better than the 
> > > > > > > > > above. Is it
> > > > > > > > > the following? I don't think I understood your preference.
> > > > > > > > > 
> > > > > > > > > - DOM$NUM for Dom0 and DomUs when not in focus, otherwise no 
> > > > > > > > > prefix
> > > > > > > > 
> > > > > > > > No, I mean like we have it with guest_printk() today. (XEN) for 
> > > > > > > > Xen's
> > > > > > > > own messages, (d<N>) for ordinary domains' ones, and no prefix
> > > > > > > > exclusively for the hardware/control domain. What is best to do 
> > > > > > > > when
> > > > > > > > hardware and control domains are distinct I'm uncertain - I'd be
> > > > > > > > inclined to suggest that the hardware domain then stay the one 
> > > > > > > > without
> > > > > > > > any prefix.
> > > > > > > 
> > > > > > > One concern I have with this approach is whether the addition of 
> > > > > > > the
> > > > > > > (d<N>) prefixes will skew output of interactive applications. So 
> > > > > > > far
> > > > > > > the prefix is added to output from all domains different than dom0
> > > > > > > because the console is not interactive for them, and hence no 
> > > > > > > input
> > > > > > > can be consumed.
> > > > > > 
> > > > > > Hmm, that's an aspect I have to admit I didn't think of.
> > > > > > 
> > > > > > > If that changes however, and domains different than dom0 can get 
> > > > > > > input
> > > > > > > from the Xen console then I wonder how much the added prefix will 
> > > > > > > skew
> > > > > > > output. Another possible option would be to not print the prefix 
> > > > > > > for
> > > > > > > the domain that has the console input assigned (current target), 
> > > > > > > and
> > > > > > > print it for all other domains (even for dom0 when not in focus).
> > > > > > 
> > > > > > That's largely what aiui was proposed. My extra requirement there 
> > > > > > would
> > > > > > then be that we make sure a log message is always emitted when 
> > > > > > console
> > > > > > focus shifts, so it's possible to identify the owner for any part of
> > > > > > the log.
> > > > > 
> > > > > Indeed, printing console input shifting should be a requirement
> > > > > regardless of how we decide to print the prefix.
> > > > 
> > > > Console input focus switch is indicated after pressing Crtl+aaa, e.g.:
> > > > [[
> > > > ...
> > > > (XEN) [15359.353038] *** Serial input to Xen (type 'CTRL-aaa' to switch 
> > > > input)
> > > > (XEN) [15361.176754] *** Serial input to DOM0 (type 'CTRL-aaa' to 
> > > > switch input)
> > > > (XEN) [15711.297202] *** Serial input to DOM1 (type 'CTRL-aaa' to 
> > > > switch input)
> > > > ...
> > > > ]]
> > > 
> > > Roger, Jan, you should use Xen Dom0less more :-) This is the way it
> > > already works on ARM. Let me expand on my earlier message that was too
> > > terse.
> > 
> > Hehe, I should use ARM more in general I think :).
> > 
> > > At boot time, Xen prints messages with the (XEN) prefix as usual:
> > > 
> > > (XEN) Brought up 4 CPUs
> > > 
> > > When Dom0 starts, it has not prefix (and has input by default):
> > > 
> > > [ 0.000000] Booting Linux on physical CPU 0x0000000000 [0x410fd034]
> > > 
> > > When a DomU starts, it has the following prefix (and doesn't have
> > > input):
> > > 
> > > (XEN) DOM1: [ 0.000000] Booting Linux on physical CPU 0x0000000000 
> > > [0x410fd034]
> > > 
> > > Eventually, both Linuxes finish booting, you can login into Dom0 as
> > > usual. Messages from Dom1, if any, are still printed with (XEN) DOM1 as
> > > prefix.
> > > 
> > > You can switch input to Dom1 with Ctrx-aaa, the same way that you do
> > > today to switch between Xen and Dom0. Xen prints a message:
> > > 
> > > (XEN) *** Serial input to DOM1 (type 'CTRL-a' three times to switch input)
> > > 
> > > Now, as you type, you send characters to Dom1. And Dom1 doesn't have the
> > > DOM1 prefix any longer, while it is still has (XEN) because the message
> > > is printed by Xen on behalf of the domain:
> > > 
> > > (XEN) / # echo hello world
> > > (XEN) hello world
> > > 
> > > If Dom0 prints anything in the backgrounds, it shows without prefixes:
> > > 
> > > hello world from dom0
> > > 
> > > If another domain, dom2, prints anything in the background, it shows
> > > with (XEN) DOM2 prefix:
> > > 
> > > (XEN) DOM2: hello from dom2
> > > 
> > > I think it makes sense to be consistent across architectures and we
> > > should default to the same behavior on x86 too. If we want to make
> > > improvements, the one thing I could potentially see changing is adding a
> > > DOM0 prefix to Dom0 messages when Dom0 does not have input. If we do
> > > that, let's do it on both ARM and x86 architectures.
> > 
> > The usual prefix is (d<domid>) IIRC, that's what guest_printk() uses,
> > is there any reason dom0less uses "(XEN) DOM<domid>:" instead of the
> > guest_printk() prefix?
> > 
> > My preference would be use to (d<domid>) prefix for any guest output
> > that doesn't belong to the domain that's the recipient of the input
> > (iow: not in console input focus). And drop the (XEN) prefix from
> > guest output.
> > 
> > The rest looks all sensible to me. I think we should avoid adding any
> > prefixes to guest output when it has the console focus, as otherwise
> > interactive applications might not work correctly.
> 
> 
> I am OK with what you describe, I would kindly ask Denis to also modify
> ARM vpl011 to match. Looking at the code, I don't know where the (XEN)
> prefix is coming from, but for the DOM<domid> to d<domid> change, it

Yep, I implemented such logic in v3 iteration of the series:
  
https://lore.kernel.org/xen-devel/20250103-vuart-ns8250-v3-v1-24-c5d36b31d66c@xxxxxxxx/

printk() adds "(XEN)" prefix (drivers/char/console.c)

> 
> would be probably something like this:
> 
> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
> index 1fc3114cce..959d172e96 100644
> --- a/xen/arch/arm/vpl011.c
> +++ b/xen/arch/arm/vpl011.c
> @@ -107,7 +107,10 @@ 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);
> 
> + if ( in_focus(d) )
> + printk("%s", intf->out);
> 
> + else
> + guest_printk("%s", d, intf->out);
> 
> intf->out_prod = 0;
> 
> }
> }
> 
> There is also one additional change needed to add the (d<domid>) prefix
> 
> for dom0 when not in focus. The Dom0 print typically comes from

This change I missed. Thanks!

> do_console_io, so in pseudocode we would need something like:
> 
> diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> index 7da8c5296f..5d250b642a 100644
> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -643,6 +643,9 @@ static long 
> guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer,
> /* Use direct console output as it could be interactive */
> nrspin_lock_irq(&console_lock);
> 
> + if ( !in_focus(cd) )
> + add_prefix(kbuf, "(d0) ");
> +
> console_serial_puts(kbuf, kcount);
> video_puts(kbuf, kcount);