Message-ID: <bb1193983de3ea9f4df1a570441df34c@localhost>
Date:	Thu, 18 Apr 2013 09:36:12 +0200
From:	Marc Zyngier <marc.zyngier@....com>
To:	Pranavkumar Sawargaonkar <pranavkumar@...aro.org>
Cc:	<kvmarm@...ts.cs.columbia.edu>, <linaro-kernel@...ts.linaro.org>,
	Anup Patel <anup.patel@...aro.org>, <patches@...aro.org>,
	<rusty@...tcorp.com.au>, <linux-kernel@...r.kernel.org>,
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [RFC] arm64: Early printk support for virtio-mmio console devices.

On Thu, 18 Apr 2013 12:47:18 +0530, Pranavkumar Sawargaonkar
<pranavkumar@...aro.org> wrote:
> Hi Marc,
> 
> On 18 April 2013 12:19, Marc Zyngier <maz@...terjones.org> wrote:
> 
>> Hi Pranavkumar,
>>
>> On Thu, 18 Apr 2013 11:22:24 +0530, PranavkumarSawargaonkar
>> <pranavkumar@...aro.org> wrote:
>> > From: Pranavkumar Sawargaonkar <pranavkumar@...aro.org>
>> >
>> > This patch implements early printk support for virtio-mmio console
>> devices
>> > without using any hypercalls.
>> >
>> > The current virtio early printk code in kernel expects that
hypervisor
>> > will provide some mechanism generally a hypercall to support early
>> printk.
>> > This patch does not break existing hypercall based early print
support.
>> >
>> > This implementation adds:
>> > 1. Early read-write register named early_rw in virtio console's
config
>> > space.
>> > 2. Two host feature flags namely VIRTIO_CONSOLE_F_EARLY_READ and
>> > VIRTIO_CONSOLE_F_EARLY_WRITE for telling guest about early-read and
>> > early-write capability in console device.
>> >
>> > Early write mechanism:
>> > 1. When a guest wants to out some character, it has to simply write
the
>> > character to early_rw register in config space of virtio console
>> > device.
>> >
>> > Early read mechanism:
>> > 1. When a guest wants to in some character, it has to simply read the
>> > early_rw register in config space of virtio console device. Lets say
we
>> get
>> > 32-bit value X.
>> > 2. If most significant bit of X is set (i.e. X & 0x80000000 ==
>> 0x80000000)
>> > then least significant 8 bits of X represents input charaacter else
>> guest
>> > need to try again reading early_rw register.
>> >
>> > Note: This patch only includes kernel side changes for early printk,
>> > the
>> > host/hypervisor side emulation of early_rw register is out of scope
>> here.
>>
>> Well, that's unfortunate, as it makes it quite difficult to understand
>> the
>> impact of this patch.
>> Has the virtio side been posted somewhere? I expect you've implemented
>> something in kvmtool...
>>
> 
> Yes i have implemented kvmtool side also and code change is really small
> (not really a clean code currently)
> I can post it also but since it is specific to kvmtool i have not posted
it
> with rfc.

Doesn't really if the code needs some rework at this point (I expect the
patch to be fairly small indeed). Any chance you could post it to the KVM
list?

>>
>> > Signed-off-by: Anup Patel <anup.patel@...aro.org>
>> > ---
>> >  arch/arm64/kernel/early_printk.c    |   24 ++++++++++++++++++++++++
>> >  include/uapi/linux/virtio_console.h |    4 ++++
>> >  2 files changed, 28 insertions(+)
>> >
>> > diff --git a/arch/arm64/kernel/early_printk.c
>> > b/arch/arm64/kernel/early_printk.c
>> > index ac974f4..a82b5aa 100644
>> > --- a/arch/arm64/kernel/early_printk.c
>> > +++ b/arch/arm64/kernel/early_printk.c
>> > @@ -25,6 +25,9 @@
>> >
>> >  #include <linux/amba/serial.h>
>> >  #include <linux/serial_reg.h>
>> > +#include <linux/virtio_ids.h>
>> > +#include <linux/virtio_mmio.h>
>> > +#include <linux/virtio_console.h>
>> >
>> >  static void __iomem *early_base;
>> >  static void (*printch)(char ch);
>> > @@ -53,6 +56,26 @@ static void smh_printch(char ch)
>> >  }
>> >
>> >  /*
>> > + * VIRTIO MMIO based debug console.
>> > + */
>> > +static void virtio_console_early_printch(char ch)
>> > +{
>> > +     u32 tmp;
>> > +     struct virtio_console_config *p = early_base +
>> > VIRTIO_MMIO_CONFIG;
>> > +
>> > +     tmp = readl_relaxed(early_base + VIRTIO_MMIO_DEVICE_ID);
>> > +     if (tmp != VIRTIO_ID_CONSOLE) {
>> > +             return;
>> > +     }
>> > +
>> > +     tmp = readl_relaxed(early_base + VIRTIO_MMIO_HOST_FEATURES);
>> > +     if (!(tmp & (1 << VIRTIO_CONSOLE_F_EARLY_WRITE))) {
>> > +             return;
>> > +     }
>> > +     writeb_relaxed(ch, &p->early_rw);
>>
>> So here, you end up trapping 3 times per character being output on the
>> console. Surely there's a better way. How about remembering the result
of
>> these tests in a static variable?
>>
> Yeah surely it is a better idea to remember using static variable, so
that
> after initialize once, it will trap only one time.

Also, would it be possible to directly get the base address from DT? It
would save having to pass the address (which is not known before runtime in
the case of kvmtool). Not sure if it is available that early though...

>>
>> > +}
>> > +
>> > +/*
>> >   * 8250/16550 (8-bit aligned registers) single character TX.
>> >   */
>> >  static void uart8250_8bit_printch(char ch)
>> > @@ -82,6 +105,7 @@ static const struct earlycon_match
earlycon_match[]
>> > __initconst = {
>> >       { .name = "smh", .printch = smh_printch, },
>> >       { .name = "uart8250-8bit", .printch = uart8250_8bit_printch, },
>> >       { .name = "uart8250-32bit", .printch = uart8250_32bit_printch,
},
>> > +     { .name = "virtio-console", .printch =
>> virtio_console_early_printch,
>> },
>> >       {}
>> >  };
>> >
>> > diff --git a/include/uapi/linux/virtio_console.h
>> > b/include/uapi/linux/virtio_console.h
>> > index ee13ab6..1171cb4 100644
>> > --- a/include/uapi/linux/virtio_console.h
>> > +++ b/include/uapi/linux/virtio_console.h
>> > @@ -38,6 +38,8 @@
>> >  /* Feature bits */
>> >  #define VIRTIO_CONSOLE_F_SIZE        0       /* Does host provide
>> console size? */
>> >  #define VIRTIO_CONSOLE_F_MULTIPORT 1 /* Does host provide multiple
>> ports?
>> >  */
>> > +#define VIRTIO_CONSOLE_F_EARLY_READ 2        /* Does host support
>> > early
>> read?
>> */
>> > +#define VIRTIO_CONSOLE_F_EARLY_WRITE 3       /* Does host support
>> > early
>> write?
>> > */
>> >
>> >  #define VIRTIO_CONSOLE_BAD_ID                (~(u32)0)
>> >
>> > @@ -48,6 +50,8 @@ struct virtio_console_config {
>> >       __u16 rows;
>> >       /* max. number of ports this device can hold */
>> >       __u32 max_nr_ports;
>> > +     /* early read/write register */
>> > +     __u32 early_rw;
>> >  } __attribute__((packed));
>> >
>> >  /*
>>
>> So that bit is clearly a spec change. How does it work with PCI, or any
>> other virtio transport?
>>
> I am not sure about PCI hence just posted for MMIO.
> 
>>
>> Overall, I'm a bit concerned with adding features that don't really
match
>> the way virtio is supposed to work. The whole goal of virtio is to
>> minimize
>> the amount of trapping, and here you end up trapping on each and every
>> access.
>>
>> If you need an early console, why not simply wire the 8250 emulation in
>> kvmtool to be useable from the MMIO bus? I reckon this would solve your
>> problem in a more elegant way...
>>
> Emulation will solve the issue but having a virtio early read or write
has
> one more advantage i.e.
> In case of mach-virt we might need early read-write support for virtio
> console to see if kernel is not panic before actual virtio drivers takes
> control.
> Also if someone wants to have UEFI or uboot running on mach-virt then we
> also need early input facility in virtio console.

That's exactly why I was suggesting using the 8250 emulation. It is
supported by everything under the sun. I do not immediately see what the
gain is with this virtio approach, as compared to 8250 emulation.

Don't misunderstand me, I like the idea of having a virtio-only system,
specially if we can make it work with other transports. I just want to
outline that there may be a simpler way for your particular use case.

Thanks,

        M.
-- 
Fast, cheap, reliable. Pick two.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists