Re: [PATCH v2 23/35] xen/console: introduce console_write()

[Denis Mukhin <dmkhn@xxxxxxxxx>]

On Thursday, December 12th, 2024 at 4:04 AM, Roger Pau Monné 
<roger.pau@xxxxxxxxxx> wrote:

>
>
> On Thu, Dec 05, 2024 at 08:41:53PM -0800, Denis Mukhin via B4 Relay wrote:
>
> > From: Denis Mukhin dmukhin@xxxxxxxx
> >
> > PV Linux kernel uses HYPERVISOR_console_io hypercall for early console which
> > ends up being handled by Xen's console driver's guest_console_write().
> >
> > guest_console_write() duplicates the code from __putstr(), elimitate code
> > duplication.
>
>
> It might be better to split the code that unifies
> guest_console_write() and __putstr() as a non-functional change.
> While the introduction of use_conring is likely a functional change.

Actually `use_conring` is not needed.
guest_console_write() depends on `opt_console_to_ring` to call
conring_puts() while __putstr() has unconditional call to conring_puts().
I solved this by adding another parameter to console_write().

>
> > Signed-off-by: Denis Mukhin dmukhin@xxxxxxxx
> > ---
> > xen/drivers/char/console.c | 97 
> > +++++++++++++++++++++++++---------------------
> > 1 file changed, 53 insertions(+), 44 deletions(-)
> >
> > diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> > index 
> > ce3639a4cdcda00ea63e3bf119bc3b242cbfdf6a..115967d179998cba4a81578caba09db4e4aca7f7
> >  100644
> > --- a/xen/drivers/char/console.c
> > +++ b/xen/drivers/char/console.c
> > @@ -63,6 +63,8 @@ static const char __initconst warning_sync_console[] =
> > "However it can introduce SIGNIFICANT latencies and affect\n"
> > "timekeeping. It is NOT recommended for production use!\n";
> >
> > +/* Flag: use conring for early console; switches to opt_console_to_ring */
> > +static bool __read_mostly use_conring = true;
>
>
> __ro_after_init instead of __read_mostly.

I dropped use_conring.

>
> > /* console_to_ring: send guest (incl. dom 0) console data to console ring. 
> > */
> > static bool __read_mostly opt_console_to_ring;
> > boolean_param("console_to_ring", opt_console_to_ring);
> > @@ -661,6 +663,16 @@ static void cf_check notify_dom0_con_ring(void *unused)
> > static DECLARE_SOFTIRQ_TASKLET(notify_dom0_con_ring_tasklet,
> > notify_dom0_con_ring, NULL);
> >
> > +static bool console_locks_busted;
> > +
> > +static void conring_write(const char *str, size_t len)
> > +{
> > + conring_puts(str, len);
> > +
> > + if ( !console_locks_busted )
> > + tasklet_schedule(&notify_dom0_con_ring_tasklet);
> > +}
> > +
> > #ifdef CONFIG_X86
> > static inline void xen_console_write_debug_port(const char *buf, size_t len)
> > {
> > @@ -669,8 +681,44 @@ static inline void xen_console_write_debug_port(const 
> > char *buf, size_t len)
> > : "=&S" (tmp), "=&c" (tmp)
> > : "0" (buf), "1" (len), "d" (XEN_HVM_DEBUGCONS_IOPORT) );
> > }
> > +
> > +static void xen_console_write(const char *str, size_t len)
> > +{
> > + if ( xen_guest )
> > + xen_hypercall_console_write(str, len);
> > + else
> > + xen_console_write_debug_port(str, len);
> > +}
> > +#else
> > +static inline void xen_console_write(const char *str, size_t len)
> > +{
>
>
> opt_console_xen would only be set on x86 with the current command line
> parsing done in console_init_preirq(), so you could add an
> ASSERT_UNREACHABLE() here.

I re-arranged the code, `static inline` variant is not needed.

>
> > +}
> > #endif
> >
> > +/*
> > + * Write characters to console.
> > + *
> > + * That will handle all possible scenarios working w/ console
> > + * - serial console;
> > + * - video output;
> > + * - __HYPERVISOR_console_io hypercall (x86 only);
> > + * - debug I/O port (x86 only);
> > + * - forward to Xen event channel.
>
>
> "Xen event channel" is not the correct term. I would use "PV
> console". The event channel is just used to send the notification.

Thank you; fixed.

>
> > + */
> > +static void console_write(const char *str, size_t len)
> > +{
> > + ASSERT(rspin_is_locked(&console_lock));
> > +
> > + console_serial_puts(str, len);
> > + video_puts(str, len);
> > +
> > + if ( opt_console_xen )
> > + xen_console_write(str, len);
>
>
> Are you sure this builds? opt_console_xen is only defined on x86, and
> AFAICT console_write() is generic. AFAICT you need to keep the X86
> preprocessor guards, or alternatively do something like:

This is strange; CI did pass on that:
   https://gitlab.com/xen-project/people/dmukhin/xen/-/pipelines/1576164352
Fixed.

>
> #define opt_console_xen false
>
> For non-x86 arches in xen/console.h
>
> > +
> > + if ( use_conring )
> > + conring_write(str, len);
> > +}
> > +
> > static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer,
> > unsigned int count)
> > {
> > @@ -691,28 +739,8 @@ 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 */
> > nrspin_lock_irq(&console_lock);
> > -
> > - console_serial_puts(kbuf, kcount);
> > - video_puts(kbuf, kcount);
> > -
> > -#ifdef CONFIG_X86
> > - if ( opt_console_xen )
> > - {
> > - if ( xen_guest )
> > - xen_hypercall_console_write(kbuf, kcount);
> > - else
> > - xen_console_write_debug_port(kbuf, kcount);
> > - }
> > -#endif
> > -
> > - if ( opt_console_to_ring )
> > - {
> > - conring_puts(kbuf, kcount);
> > - tasklet_schedule(&notify_dom0_con_ring_tasklet);
> > - }
> > -
> > + console_write(kbuf, kcount);
> > nrspin_unlock_irq(&console_lock);
> > }
> > else
> > @@ -813,31 +841,9 @@ long do_console_io(
> > * *****************************************************
> > */
> >
> > -static bool console_locks_busted;
> > -
> > static void __putstr(const char *str)
> > {
> > - size_t len = strlen(str);
> > -
> > - ASSERT(rspin_is_locked(&console_lock));
> > -
> > - console_serial_puts(str, len);
> > - video_puts(str, len);
> > -
> > -#ifdef CONFIG_X86
> > - if ( opt_console_xen )
> > - {
> > - if ( xen_guest )
> > - xen_hypercall_console_write(str, len);
> > - else
> > - xen_console_write_debug_port(str, len);
> > - }
> > -#endif
> > -
> > - conring_puts(str, len);
> > -
> > - if ( !console_locks_busted )
> > - tasklet_schedule(&notify_dom0_con_ring_tasklet);
> > + console_write(str, strlen(str));
> > }
> >
> > static int printk_prefix_check(char *p, char **pp)
> > @@ -1171,6 +1177,9 @@ void __init console_endboot(void)
> >
> > video_endboot();
> >
> > + use_conring = opt_console_to_ring;
> > + smp_wmb();
>
>
> Do you really need the barrier? If so it would need a comment
> describing exactly why it's needed. I don't think it's possible for
> the write to be reordered past the return of the function, which would
> be enough to ensure correctness?

I see where I got confused: the right place to add this initialization was
console_init_postirq(). I dropped the barrier and of `use_conring` altogether.

>
> Thanks, Roger.