Message-ID: <CAAHg+Hj4HB20FDYLZ+NdjF3+2xp7E0-PNm41d74K4WB8OfB4fg@mail.gmail.com>
Date:	Fri, 19 Apr 2013 00:36:46 +0530
From:	Pranavkumar Sawargaonkar <pranavkumar@...aro.org>
To:	Marc Zyngier <marc.zyngier@....com>
Cc:	"kvmarm@...ts.cs.columbia.edu" <kvmarm@...ts.cs.columbia.edu>,
	"linaro-kernel@...ts.linaro.org" <linaro-kernel@...ts.linaro.org>,
	Anup Patel <anup.patel@...aro.org>,
	Patch Tracking <patches@...aro.org>,
	Rusty Russell <rusty@...tcorp.com.au>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [RFC] arm64: Early printk support for virtio-mmio console devices.

On 18 April 2013 13:06, Marc Zyngier <marc.zyngier@....com> wrote:
> 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?

I have posted kvmtool side changes on kvmarm list.
[RFC] KVMTOOL: Early printk support for virtio-mmio console.
Patch has an implementation of early_rw config register to print early writes.
I have successfully booted a guest on armv8 foundation model throwing
early printks before actual virtio driver takes control.

Thanks,
Pranav

>
>>>
>>> > 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