Re: [PATCH 1/2] hvc_xen: add earlycon support

[Stefano Stabellini]

On Wed, 24 Feb 2016, Boris Ostrovsky wrote:
> On 02/24/2016 07:23 AM, Stefano Stabellini wrote:
> > Introduce EARLYCON support in hvc_xen, useful for early debugging on arm
> > and arm64, where xen early_printk is not available. Differently from
> > xenboot_write_console on x86, we won't just return if !xen_pv_domain(),
> > because arm and arm64 guests are actually xen_hvm_domain() from linux
> > pov, and also because we want to capture all possible writes, including
> > the ones earlier than xen_early_init().
> > 
> > Signed-off-by: Stefano Stabellini<stefano.stabellini@xxxxxxxxxxxxx>
> > ---
> >   drivers/tty/hvc/hvc_xen.c |   48
> > ++++++++++++++++++++++++++++++++++-----------
> >   1 file changed, 37 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c
> > index fa816b7..34e8e9f 100644
> > --- a/drivers/tty/hvc/hvc_xen.c
> > +++ b/drivers/tty/hvc/hvc_xen.c
> > @@ -25,6 +25,7 @@
> >   #include <linux/init.h>
> >   #include <linux/types.h>
> >   #include <linux/list.h>
> > +#include <linux/serial_core.h>
> >     #include <asm/io.h>
> >   #include <asm/xen/hypervisor.h>
> > @@ -597,21 +598,11 @@ static int xen_cons_init(void)
> >   }
> >   console_initcall(xen_cons_init);
> >   -#ifdef CONFIG_EARLY_PRINTK
> > -static void xenboot_write_console(struct console *console, const char
> > *string,
> > -				  unsigned len)
> > +static void domU_write_early_console(const char *string, unsigned len)
> >   {
> >   	unsigned int linelen, off = 0;
> >   	const char *pos;
> >   -	if (!xen_pv_domain())
> > -		return;
> > -
> > -	dom0_write_console(0, string, len);
> > -
> > -	if (xen_initial_domain())
> > -		return;
> > -
> >   	domU_write_console(0, "(early) ", 8);
> >   	while (off < len && NULL != (pos = strchr(string+off, '\n'))) {
> >   		linelen = pos-string+off;
> > @@ -625,6 +616,21 @@ static void xenboot_write_console(struct console
> > *console, const char *string,
> >   		domU_write_console(0, string+off, len-off);
> >   }
> >   +#ifdef CONFIG_EARLY_PRINTK
> > +static void xenboot_write_console(struct console *console, const char
> > *string,
> > +				  unsigned len)
> > +{
> > +	if (!xen_pv_domain())
> > +		return;
> 
> This, BTW, will break PVHv2 code (which is not there yet). Your patch is not
> making the code any different from what it is now but I wonder whether
> xenboot_write_console() can be made to work for HVM guests at this time since
> you making changes there anyway.
> 
> Or we can delay it until HVMlite/PVHv2 patches make it in.

I don't think it can be made to work with HVM guests unfortunately,
because in the HVM case Xen support is going to be discovered only at
some later point. We have the same problem on ARM, where all guests are
xen_hvm_domain(). In that case I solved the problem by calling
dom0_write_console unconditionally from xenboot_earlycon_write, see
below.

I could do the same here by dropping the if (!xen_pv_domain()) check
above, but then if somebody specifies earlyprintk=xenboot on a non-Xen
environment, I expect Linux would crash.


> > +
> > +	dom0_write_console(0, string, len);
> > +
> > +	if (xen_initial_domain())
> > +		return;
> > +
> > +	domU_write_early_console(string, len);
> > +}
> > +
> >   struct console xenboot_console = {
> >   	.name		= "xenboot",
> >   	.write		= xenboot_write_console,
> > @@ -664,3 +670,23 @@ void xen_raw_printk(const char *fmt, ...)
> >     	xen_raw_console_write(buf);
> >   }
> > +
> > +static void xenboot_earlycon_write(struct console *console,
> > +				  const char *string,
> > +				  unsigned len)
> > +{
> > +	dom0_write_console(0, string, len);
> > +
> > +	if (xen_initial_domain())
> > +		return;
> > +
> > +	domU_write_early_console(string, len);
> > +}

My explanation above made me realize that actually
domU_write_early_console can never be called here, so I'll drop it in
the next version.


> xenboot_earlycon_write() and xenboot_write_console() share most of the code,
> can it be factored out?

No, they are actually different. I don't think it would be better to
introduce #ifdef CONFIG_X86 in this file.


> > +
> > +static int __init xenboot_earlycon_setup(struct earlycon_device *device,
> > +					    const char *opt)
> > +{
> > +	device->con->write = xenboot_earlycon_write;
> > +	return 0;
> > +}
> > +EARLYCON_DECLARE(xenboot, xenboot_earlycon_setup);
>