Message-Id: <96D4393D-E2FF-4967-A608-FCEBD169D77D@suse.de>
Date:	Thu, 18 Apr 2013 10:53:58 +0200
From:	Alexander Graf <agraf@...e.de>
To:	Pranavkumar Sawargaonkar <pranavkumar@...aro.org>
Cc:	Marc Zyngier <marc.zyngier@....com>,
	"linaro-kernel@...ts.linaro.org" <linaro-kernel@...ts.linaro.org>,
	Patch Tracking <patches@...aro.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"kvmarm@...ts.cs.columbia.edu" <kvmarm@...ts.cs.columbia.edu>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [RFC] arm64: Early printk support for virtio-mmio console devices.



Am 18.04.2013 um 10:48 schrieb Pranavkumar Sawargaonkar <pranavkumar@...aro.org>:

> Hi Marc,
> 
> 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?
> Yeah patch is very small, i will post it on kvm list. I have tested
> patch on foundation model.
>> 
>>>> 
>>>>> 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...
> 
> Early printk code initializes earlier (from  parse_early_param in
> arch/arm64/setup.c) than fdt un-flattened call (unflatten_device_tree)
> . Hence using dts to pass this is not possible for passing the
> address.

Then don't print anything until the fdt is unflattened?

Alex

> 
>> 
>>>> 
>>>>> +}
>>>>> +
>>>>> +/*
>>>>>  * 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,
> Definitely not.
>> 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.
> 
> Actually i thought adding a config register will be easier to add a
> code than writing entire emulation as 8250 emulation will require to
> deal with dealing with more registers and more code.
> 
> Thanks,
> Pranav
> 
>> 
>> Thanks,
>> 
>>        M.
>> --
>> Fast, cheap, reliable. Pick two.
> _______________________________________________
> kvmarm mailing list
> kvmarm@...ts.cs.columbia.edu
> https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
--
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